Make 'git config list --type=<X>' parse and filter types#2044
Open
derrickstolee wants to merge 13 commits intogitgitgadget:masterfrom
Open
Make 'git config list --type=<X>' parse and filter types#2044derrickstolee wants to merge 13 commits intogitgitgadget:masterfrom
derrickstolee wants to merge 13 commits intogitgitgadget:masterfrom
Conversation
In anticipation of using format_config() in this method, move show_all_config() lower in the file without changes. Signed-off-by: Derrick Stolee <stolee@gmail.com>
derrickstolee
force-pushed
the
config-list-type
branch
2 times, most recently
from
1dc14e8 to
e27d52c
Compare
Author
|
/submit |
|
Submitted as pull.2044.git.1770698579.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Problem: 'git config list' doesn't respect --type=<X>!
;-).
As there is no "inherent" type associated with each configuration
variable (in other words, type of a particular configuration
variable is something determined by the caller that wants the value
of that variable), "git config list/get --type=auto" would not work,
but it would not be too bad to allow "git config list --type=path"
to treat everything as if it is a path and having to filter nonsense
out of the result (like "core.bare = true/false" or even "core.bare"
without value that means true, which may make the "*force*
interpreting it as path" approach to barf), which is an inevitable
consequence.
> This boils down to the fact that the iterator function show_all_config()
> doesn't call format_config(), which includes the type-parsing code.
>
> This wasn't super trivial to update:
>
> 1. format_config() uses git_config_parse_*() methods, which die() on a bad
> parse.
> 2. The path parsing code didn't have a gentle version.
> 3. The two paths ('git config list' and 'git config --list') needed to
> standardize their display options to work with format_config().
Thanks for dealing with them. These are what I would have expected
as part of the "inevitable consequence".
> 4. Finally, we need to filter out key-value pairs that don't match the
> given type.
This one, however, I need to see the actual code before commenting,
as I do not think key-value pairs have inherent types. The _only_
special case where you can tell what type the thing is is the
valueless true, which we can safely say is inherently boolean.
Everything else is text string, sometimes interpreted as boolean,
sometimes number, sometimes human-scaled number, sometimes path
(with possible tilde expansion), etc.
> This is marked as an RFC because I need to add some more tests and because
> this is a behavior change! If there are any tools currently passing the
> --type=<X> argument to git config list then they will have a change of
> behavior with this series. It's an easy workaround: drop the --type argument
> or add --no-type to go back to the previous behavior.
>
> Thanks for any and all feedback, -Stolee
>
> Derrick Stolee (5):
> config: move show_all_config()
> parse: add git_parse_maybe_pathname()
> config: allow format_config() to filter
> config: create special init for list mode
> config: make 'git config list --type=<X>' work
>
> Documentation/git-config.adoc | 3 +
> builtin/config.c | 130 ++++++++++++++++++++++++----------
> config.c | 14 +---
> parse.c | 24 +++++++
> parse.h | 2 +
> t/t1300-config.sh | 26 ++++++-
> 6 files changed, 147 insertions(+), 52 deletions(-)
>
>
> base-commit: 67ad42147a7acc2af6074753ebd03d904476118f
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2044%2Fderrickstolee%2Fconfig-list-type-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2044/derrickstolee/config-list-type-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2044 |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -3,6 +3,7 @@ | |||
| #include "abspath.h" | |||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Derrick Stolee <stolee@gmail.com>
>
> The format_config() method in builtin/config.c currently only uses
> git_config_*() methods for parsing. This allows parsing errors to result
> in die() messages appropriate with keys in the error message.
>
> In a future change we will want to use format_config() within 'git
> config list' to help format the output, including when --type=<X>
> arguments are provided. When the parsing fails in that case, that
> key-value pair should be omitted instead of causing a failure across the
> entire command.
>
> This change is formatted in such a way that the if/else-if structure
> allows the default die_on_error version to appear first and then be
> followed by the gentle parsing mode immediately afterwards.
>
> The only callers right now have die_on_parse set to 1.
Certainly you meant die-on-parse-errors, not unconditionally die
when asked to parse ;-).
I wonder if a "bool gently" like everybody else takes would be
easier to understand by more developers and readers, though.
> + if (opts->type == TYPE_INT && die_on_parse) {
> strbuf_addf(buf, "%"PRId64,
> git_config_int64(key_, value_ ? value_ : "", kvi));
> + } else if (opts->type == TYPE_INT) {
> + int64_t v;
> + int ret = git_parse_int64(value_, &v);
> +
> + if (ret)
> + return -1;
> +
> + strbuf_addf(buf, "%"PRId64, v);
> + }
So, this follows the typical layout that was described in the
proposed log message. I wonder if it is too much to break the set
of helper functions further down so that this part of the caller can
say something like:
switch (opts->type) {
case TYPE_INT:
format_config_int(buf, key_, value_, kvi, gently);
break;
and similar case arms for other types?
There was a problem hiding this comment.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
On 2/10/2026 12:04 AM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> The format_config() method in builtin/config.c currently only uses
>> git_config_*() methods for parsing. This allows parsing errors to result
>> in die() messages appropriate with keys in the error message.
>>
>> In a future change we will want to use format_config() within 'git
>> config list' to help format the output, including when --type=<X>
>> arguments are provided. When the parsing fails in that case, that
>> key-value pair should be omitted instead of causing a failure across the
>> entire command.
>>
>> This change is formatted in such a way that the if/else-if structure
>> allows the default die_on_error version to appear first and then be
>> followed by the gentle parsing mode immediately afterwards.
>>
>> The only callers right now have die_on_parse set to 1.
>
> Certainly you meant die-on-parse-errors, not unconditionally die
> when asked to parse ;-).
>
> I wonder if a "bool gently" like everybody else takes would be
> easier to understand by more developers and readers, though.
'gently' makes a lot more sense.
>> + if (opts->type == TYPE_INT && die_on_parse) {
>> strbuf_addf(buf, "%"PRId64,
>> git_config_int64(key_, value_ ? value_ : "", kvi));
>> + } else if (opts->type == TYPE_INT) {
>> + int64_t v;
>> + int ret = git_parse_int64(value_, &v);
>> +
>> + if (ret)
>> + return -1;
>> +
>> + strbuf_addf(buf, "%"PRId64, v);
>> + }
>
> So, this follows the typical layout that was described in the
> proposed log message. I wonder if it is too much to break the set
> of helper functions further down so that this part of the caller can
> say something like:
>
> switch (opts->type) {
> case TYPE_INT:
> format_config_int(buf, key_, value_, kvi, gently);
> break;
>
> and similar case arms for other types?
I had a similar feeling that such a refactor would be necessary.
I didn't want to go through that careful work if it wasn't
justified by positive reactions to the RFC. Thanks for calling it
out, and I'll definitely put in that effort if we find this worth
a v2.
Thanks,
-Stolee
|
Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 2/9/2026 11:59 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:...
>> This boils down to the fact that the iterator function show_all_config()
>> doesn't call format_config(), which includes the type-parsing code.
>>
>> This wasn't super trivial to update:
>>
>> 1. format_config() uses git_config_parse_*() methods, which die() on a bad
>> parse.
>> 2. The path parsing code didn't have a gentle version.
>> 3. The two paths ('git config list' and 'git config --list') needed to
>> standardize their display options to work with format_config().
>
> Thanks for dealing with them. These are what I would have expected
> as part of the "inevitable consequence".
>
>> 4. Finally, we need to filter out key-value pairs that don't match the
>> given type.
>
> This one, however, I need to see the actual code before commenting,
> as I do not think key-value pairs have inherent types. The _only_
> special case where you can tell what type the thing is is the
> valueless true, which we can safely say is inherently boolean.
> Everything else is text string, sometimes interpreted as boolean,
> sometimes number, sometimes human-scaled number, sometimes path
> (with possible tilde expansion), etc.
This is the crux of this series. If the caller asks for a type, then I
see a couple different ways to react:
1. If the value fails to parse in that type, then don't list that
result, allowing the caller to have confidence that every result
is of the correct format.
2. If the value fails to parse in that type, then list it in its
base string. The caller would need to do extra parsing to check
that the results match the correct format.
I chose option 1. It avoids showing results that would result in
'git config get --type=<X> <key>' to die().
I'd be interested to hear if there are reasons to go with option 2,
or if there exists an alternative option that I don't see.
Reordering your message somewhat:
> As there is no "inherent" type associated with each configuration
> variable (in other words, type of a particular configuration
> variable is something determined by the caller that wants the value
> of that variable), "git config list/get --type=auto" would not work,
> but it would not be too bad to allow "git config list --type=path"
> to treat everything as if it is a path and having to filter nonsense
> out of the result (like "core.bare = true/false" or even "core.bare"
> without value that means true, which may make the "*force*
> interpreting it as path" approach to barf), which is an inevitable
> consequence.
I agree that there is not inherent type, so the user can only
specify the type that they are expecting. To me, this is a request to
filter the results.
I don't think we'll have much success if we try to guess the type,
such as trying to parse an int, then a bool, then a path.
Thanks,
-Stolee
|
|
This patch series was integrated into seen via git@c6d7b35. |
|
This patch series was integrated into seen via git@923ffa2. |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Feb 10, 2026 at 04:42:54AM +0000, Derrick Stolee via GitGitGadget wrote:
> This is marked as an RFC because I need to add some more tests and because
> this is a behavior change! If there are any tools currently passing the
> --type=<X> argument to git config list then they will have a change of
> behavior with this series. It's an easy workaround: drop the --type argument
> or add --no-type to go back to the previous behavior.
I think this is not a huge problem. It simply reads like a bug to me
that the command accepts the option, but doesn't honor it. Sure, it can
lead to different behaviour, but I think that's acceptable.
Patrick |
|
User |
| @@ -1278,24 +1278,12 @@ int git_config_string(char **dest, const char *var, const char *value) | |||
|
|
|||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Tue, Feb 10, 2026 at 04:42:56AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> This extraction of logic from config.c's git_config_pathname() allows
> for parsing a fully-qualified path from a relative path along with
> validation of the existence of the path without failing with a die().
That sentence is quite something. I had to read it thrice to understand
what it wants to say :)
> diff --git a/parse.c b/parse.c
> index 48313571aa..3f37f0b93a 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -209,3 +210,26 @@ unsigned long git_env_ulong(const char *k, unsigned long val)
> die(_("failed to parse %s"), k);
> return val;
> }
> +
> +int git_parse_maybe_pathname(const char *value, char **dest)
> +{
> + bool is_optional;
> + char *path;
> +
> + if (!value)
> + return -1;
> +
> + is_optional = skip_prefix(value, ":(optional)", &value);
> + path = interpolate_path(value, 0);
> + if (!path)
> + return -1;
> +
> + if (is_optional && is_missing_file(path)) {
> + free(path);
> + *dest = NULL;
> + return 0;
> + }
> +
> + *dest = path;
> + return 0;
> +}
Okay. So the difference is that this function here doesn't cause us to
die in case the path is not marked as optional and missing. Makes sense.
> diff --git a/parse.h b/parse.h
> index ea32de9a91..4f97c3727a 100644
> --- a/parse.h
> +++ b/parse.h
> @@ -19,4 +19,6 @@ int git_parse_maybe_bool_text(const char *value);
> int git_env_bool(const char *, int);
> unsigned long git_env_ulong(const char *, unsigned long);
>
> +int git_parse_maybe_pathname(const char *value, char **dest);
I think this function could use some explanation what it actually does,
as the behaviour is non-trivial:
- I think the ":(optional)" part needs to be documented properly to
say that we return successfully with a NULL string in case the
target path doesn't exist.
- We should document that it expands "~" and "%(prefix)" (even though
the latter feels somewhat coincidental to me).
- The path is not resolved to an absolute path.
Thanks!
Patrick
| @@ -240,6 +240,9 @@ Valid `<type>`'s include: | |||
| that the given value is canonicalize-able as an ANSI color, but it is written | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Tue, Feb 10, 2026 at 04:42:59AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> Previously, the --type=<X> argument to 'git config list' was ignored and
> did nothing. Now, we add the use of format_config() to the
> show_all_config() function so each key-value pair is attempted to be
> parsed.
>
> If there is an error in parsing, then the row is not output.
I was a bit surprised at first, but now that I think about it a bit more
I think this is sensible behaviour. If I ask for `git config list
--type=int`, then I don't want to see any non-int configuration. I
wouldn't even expect a warning, as the option essentially works like a
filter.
> This is a change in behavior! We are starting to respect an option that
> was previously ignored, leading to potential user confusion. This is
> probably still a good option, since the --type argument did not change
> behavior at all previously, so users can get the behavior they expect by
> removing the --type argument or adding the --no-type argument.
Yeah, I fully agree that this is a sensible change in behaviour. It is
obviously broken right now, so I would claim that this is simply a bug
fix.
> diff --git a/Documentation/git-config.adoc b/Documentation/git-config.adoc
> index ac3b536a15..5300dd4c51 100644
> --- a/Documentation/git-config.adoc
> +++ b/Documentation/git-config.adoc
The synopsis of `git config list` should also be amended.
> diff --git a/builtin/config.c b/builtin/config.c
> index e69b26af6a..c83514b4ff 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -363,21 +363,12 @@ static int show_all_config(const char *key_, const char *value_,
> {
> const struct config_display_options *opts = cb;
> const struct key_value_info *kvi = ctx->kvi;
> + struct strbuf formatted = STRBUF_INIT;
>
> - if (opts->show_origin || opts->show_scope) {
> - struct strbuf buf = STRBUF_INIT;
> - if (opts->show_scope)
> - show_config_scope(opts, kvi, &buf);
> - if (opts->show_origin)
> - show_config_origin(opts, kvi, &buf);
> - /* Use fwrite as "buf" can contain \0's if "end_null" is set. */
> - fwrite(buf.buf, 1, buf.len, stdout);
> - strbuf_release(&buf);
> - }
> - if (!opts->omit_values && value_)
> - printf("%s%c%s%c", key_, opts->delim, value_, opts->term);
> - else
> - printf("%s%c", key_, opts->term);
> + if (format_config(opts, &formatted, key_, value_, kvi, 0) >= 0)
> + fwrite(formatted.buf, 1, formatted.len, stdout);
> +
> + strbuf_release(&formatted);
> return 0;
> }
>
I wonder whether there is a good argument to be made here that we should
keep the old logic in case no "--type=" parameter was given. In that
case, for example the following output would remain the same:
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 9850fcd5b5..b5ce900126 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -2459,9 +2459,10 @@ done
>
> cat >.git/config <<-\EOF &&
> [section]
> -foo = true
> +foo = True
> number = 10
> big = 1M
> +path = ~/dir
> EOF
>
> test_expect_success 'identical modern --type specifiers are allowed' '
I'm not really sure whether we want that though. I actually like that
this also leads to some code duplication, so maybe this is fine?
PatrickThere was a problem hiding this comment.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
On 2/11/2026 7:13 AM, Patrick Steinhardt wrote:
> On Tue, Feb 10, 2026 at 04:42:59AM +0000, Derrick Stolee via GitGitGadget wrote:
>> diff --git a/Documentation/git-config.adoc b/Documentation/git-config.adoc
>> index ac3b536a15..5300dd4c51 100644
>> --- a/Documentation/git-config.adoc
>> +++ b/Documentation/git-config.adoc
>
> The synopsis of `git config list` should also be amended.
Good point. Will fix.
>> diff --git a/builtin/config.c b/builtin/config.c
>> index e69b26af6a..c83514b4ff 100644
>> --- a/builtin/config.c
>> +++ b/builtin/config.c
>> @@ -363,21 +363,12 @@ static int show_all_config(const char *key_, const char *value_,
>> {
>> const struct config_display_options *opts = cb;
>> const struct key_value_info *kvi = ctx->kvi;
>> + struct strbuf formatted = STRBUF_INIT;
>>
>> - if (opts->show_origin || opts->show_scope) {
>> - struct strbuf buf = STRBUF_INIT;
>> - if (opts->show_scope)
>> - show_config_scope(opts, kvi, &buf);
>> - if (opts->show_origin)
>> - show_config_origin(opts, kvi, &buf);
>> - /* Use fwrite as "buf" can contain \0's if "end_null" is set. */
>> - fwrite(buf.buf, 1, buf.len, stdout);
>> - strbuf_release(&buf);
>> - }
>> - if (!opts->omit_values && value_)
>> - printf("%s%c%s%c", key_, opts->delim, value_, opts->term);
>> - else
>> - printf("%s%c", key_, opts->term);
>> + if (format_config(opts, &formatted, key_, value_, kvi, 0) >= 0)
>> + fwrite(formatted.buf, 1, formatted.len, stdout);
>> +
>> + strbuf_release(&formatted);
>> return 0;
>> }
>>
>
> I wonder whether there is a good argument to be made here that we should
> keep the old logic in case no "--type=" parameter was given. In that
> case, for example the following output would remain the same:
If no `--type=` parameter is given, then this new implementation does
the exact same thing as the display_options use a string format (which
does not mutate the config values).
>> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
>> index 9850fcd5b5..b5ce900126 100755
>> --- a/t/t1300-config.sh
>> +++ b/t/t1300-config.sh
>> @@ -2459,9 +2459,10 @@ done
>>
>> cat >.git/config <<-\EOF &&
>> [section]
>> -foo = true
>> +foo = True
>> number = 10
>> big = 1M
>> +path = ~/dir
>> EOF
>>
>> test_expect_success 'identical modern --type specifiers are allowed' '
>
> I'm not really sure whether we want that though. I actually like that
> this also leads to some code duplication, so maybe this is fine?
The change you highlight here is a difference in the config file _contents_
and not the expected output. These changes are to help demonstrate that the
bool and path types make meaningful conversions when listing these values.
The previous tests for getting bool values did not demonstrate the way it
modifies case, for example.
Thanks,
-Stolee
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Wed, Feb 11, 2026 at 12:49:19PM -0500, Derrick Stolee wrote:
> On 2/11/2026 7:13 AM, Patrick Steinhardt wrote:
> > On Tue, Feb 10, 2026 at 04:42:59AM +0000, Derrick Stolee via GitGitGadget wrote:
> >> diff --git a/builtin/config.c b/builtin/config.c
> >> index e69b26af6a..c83514b4ff 100644
> >> --- a/builtin/config.c
> >> +++ b/builtin/config.c
> >> @@ -363,21 +363,12 @@ static int show_all_config(const char *key_, const char *value_,
> >> {
> >> const struct config_display_options *opts = cb;
> >> const struct key_value_info *kvi = ctx->kvi;
> >> + struct strbuf formatted = STRBUF_INIT;
> >>
> >> - if (opts->show_origin || opts->show_scope) {
> >> - struct strbuf buf = STRBUF_INIT;
> >> - if (opts->show_scope)
> >> - show_config_scope(opts, kvi, &buf);
> >> - if (opts->show_origin)
> >> - show_config_origin(opts, kvi, &buf);
> >> - /* Use fwrite as "buf" can contain \0's if "end_null" is set. */
> >> - fwrite(buf.buf, 1, buf.len, stdout);
> >> - strbuf_release(&buf);
> >> - }
> >> - if (!opts->omit_values && value_)
> >> - printf("%s%c%s%c", key_, opts->delim, value_, opts->term);
> >> - else
> >> - printf("%s%c", key_, opts->term);
> >> + if (format_config(opts, &formatted, key_, value_, kvi, 0) >= 0)
> >> + fwrite(formatted.buf, 1, formatted.len, stdout);
> >> +
> >> + strbuf_release(&formatted);
> >> return 0;
> >> }
> >>
> >
> > I wonder whether there is a good argument to be made here that we should
> > keep the old logic in case no "--type=" parameter was given. In that
> > case, for example the following output would remain the same:
>
> If no `--type=` parameter is given, then this new implementation does
> the exact same thing as the display_options use a string format (which
> does not mutate the config values).
>
> >> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> >> index 9850fcd5b5..b5ce900126 100755
> >> --- a/t/t1300-config.sh
> >> +++ b/t/t1300-config.sh
> >> @@ -2459,9 +2459,10 @@ done
> >>
> >> cat >.git/config <<-\EOF &&
> >> [section]
> >> -foo = true
> >> +foo = True
> >> number = 10
> >> big = 1M
> >> +path = ~/dir
> >> EOF
> >>
> >> test_expect_success 'identical modern --type specifiers are allowed' '
> >
> > I'm not really sure whether we want that though. I actually like that
> > this also leads to some code duplication, so maybe this is fine?
>
> The change you highlight here is a difference in the config file _contents_
> and not the expected output. These changes are to help demonstrate that the
> bool and path types make meaningful conversions when listing these values.
Ooh, right. Completely missed that, thanks for the clarification.
Patrick
|
This branch is now known as |
|
This patch series was integrated into seen via git@922f600. |
derrickstolee
force-pushed
the
config-list-type
branch
from
e27d52c to
2fbbd6b
Compare
derrickstolee
changed the title
This parameter is set to 0 for all current callers and is UNUSED. However, we will start using this option in future changes and in a critical change that requires gentle parsing (not using die()) to try parsing all values in a list. Signed-off-by: Derrick Stolee <stolee@gmail.com>
derrickstolee
force-pushed
the
config-list-type
branch
2 times, most recently
from
9304a46 to
185f737
Compare
|
This patch series was integrated into seen via git@505d550. |
Previously, the --type=<X> argument to 'git config list' was ignored and did nothing. Now, we add the use of format_config() to the show_all_config() function so each key-value pair is attempted to be parsed. This is our first use of the 'gently' parameter with a nonzero value. When listing multiple values, our initial settings for the output format is different. Add a new init helper to specify the fact that keys should be shown and also add the default delimiters as they were unset in some cases. If there is an error in parsing, then the row is not output. This is a change in behavior! We are starting to respect an option that was previously ignored, leading to potential user confusion. This is probably still a good option, since the --type argument did not change behavior at all previously, so users can get the behavior they expect by removing the --type argument or adding the --no-type argument. t1300-config.sh is updated with the current behavior of this formatting logic to justify the upcoming refactoring of format_config() that will incrementally fix some of these cases to be more user-friendly. Signed-off-by: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting int64 config values into a helper method and use gentle parsing when needed. Signed-off-by: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting bool config values into a helper method and use gentle parsing when needed. This makes 'git config list --type=bool' not fail when coming across a non-boolean value. Such unparseable values are filtered out quietly. Signed-off-by: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting bool-or-int config values into a helper method and use gentle parsing when needed. Signed-off-by: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting bool-or-string config values into a helper. This parsing has always been gentle, so this is not unlocking new behavior. This extraction is only to match the formatting of the other cases that do need a behavior change. Signed-off-by: Derrick Stolee <stolee@gmail.com>
The git_config_pathname() method parses a config value as a path, but always die()s on an error. Move this logic into a gentler parsing algorithm that will return an error value instead of ending the process. Signed-off-by: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting path config values into a helper method and use gentle parsing when needed. Signed-off-by: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting expiry date config values into a helper method and use gentle parsing when needed. There is an unfortunate asymmetry in these two parsing methods, but we need to treat a positive response from parse_expiry_date() as an error or we will get incorrect values. This updates the behavior of 'git config list --type=expiry-date' to be quiet when attempting parsing on non-date values. Signed-off-by: Derrick Stolee <stolee@gmail.com>
When parsing colors, a failed parse leads to an error message due to the result returning error(). To allow for quiet parsing, create color_parse_gently(). To accomplish this, convert the implementation of color_parse_mem() into a static color_parse_mem_1() helper that adds a 'gently' parameter. The color_parse_gently() method can then use this. Since it is a near equivalent to color_parse(), move that method down in the file so they can be nearby while also appearing after color_parse_mem_1(). Signed-off-by: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting color config value into a helper method and use gentle parsing when needed. This removes error messages when parsing a list of config values that do not match color formats. Signed-off-by: Derrick Stolee <stolee@gmail.com>
The recent changes have replaced the bodies of most if/else-if cases with simple helper method calls. This makes it easy to adapt the structure into a clearer switch statement, leaving a simple if/else in the default case. Make things a little simpler to read by reducing the nesting depth via a new goto statement when we want to skip values. Signed-off-by: Derrick Stolee <stolee@gmail.com>
derrickstolee
force-pushed
the
config-list-type
branch
from
185f737 to
48fc882
Compare
|
This patch series was integrated into seen via git@3cb6a3a. |
Author
|
/submit |
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): I started down this road based on feedback on my 'git config-batch' RFC [1].
[1]
https://lore.kernel.org/git/pull.2033.git.1770214803.gitgitgadget@gmail.com/
I had described my intention to use 'git config-batch' as a single process
to load multiple config values one-by-one. Brian mentioned that 'git config
list -z' would probably suffice, so I started experimenting in that
direction [2].
[2]
https://github.com/git-ecosystem/git-credential-manager/compare/main...derrickstolee:config-list
However, I ran into a problem: the most critical performance bottleneck is
related to path-formatted config values that are queried with 'git config
get --type=path -z'. It wasn't hard to update things to lazily load the full
list of config values by type [3], but I then noticed a big problem!
[3]
https://github.com/git-ecosystem/git-credential-manager/commit/d403c8e24ce6f37da920cce23842dd5a6cf6481d
Problem: 'git config list' doesn't respect --type=<X>!
This boils down to the fact that the iterator function show_all_config()
doesn't call format_config(), which includes the type-parsing code.
This wasn't super trivial to update:
1. format_config() uses git_config_parse_*() methods, which die() on a bad
parse.
2. The path parsing code didn't have a gentle version.
3. The two paths ('git config list' and 'git config --list') needed to
standardize their display options to work with format_config().
4. Finally, we need to filter out key-value pairs that don't match the
given type.
Updates in v2
=============
Based on the positive feedback in round one, this is no longer an RFC.
* format_config() now uses a 'gently' parameter instead of 'die_on_parse'
(flipped).
* format_config() is more carefully updated with helper methods and a
global refactor.
* New gentle parsing code is introduced right before the format_config()
helper is created to use it.
* I squashed the change that updates the display_opts initial state into
the patch that uses format_config() for the 'list' command. The initial
state change on its own leads to test failures, so I am making a slightly
bigger patch to keep things passing tests at every change.
* More tests for 'git config list --type=<X>' are added.
* I rearranged things so the 'git config list --type' integration follows
the format_config() update immediately. The tests at that time show what
such a trivial implementation would do, including failing on bool parsing
and having several error messages for color and expiry-date parsing. The
tests modify as these issues are fixed with gentle parsers.
* I have a prototype implementation of GCM using this option in [4] and it
gets the performance improvements I was hoping for. It requires polish
and a compatibility check that uses the Git version to guarantee that
this --type behavior change is recognized.
[4] https://github.com/git-ecosystem/git-credential-manager/pull/2268
Thanks for any and all feedback, -Stolee
Derrick Stolee (13):
config: move show_all_config()
config: add 'gently' parameter to format_config()
config: make 'git config list --type=<X>' work
config: format int64s gently
config: format bools gently
config: format bools or ints gently
config: format bools or strings in helper
parse: add git_parse_maybe_pathname()
config: format paths gently
config: format expiry dates gently
color: add color_parse_gently()
config: format colors gently
config: restructure format_config()
Documentation/git-config.adoc | 3 +
builtin/config.c | 283 +++++++++++++++++++++++++---------
color.c | 25 ++-
color.h | 1 +
config.c | 14 +-
parse.c | 24 +++
parse.h | 2 +
t/t1300-config.sh | 58 ++++++-
8 files changed, 318 insertions(+), 92 deletions(-)
base-commit: 67ad42147a7acc2af6074753ebd03d904476118f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2044%2Fderrickstolee%2Fconfig-list-type-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2044/derrickstolee/config-list-type-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2044
Range-diff vs v1:
1: bca83d8ca8 = 1: bca83d8ca8 config: move show_all_config()
3: d9e0424010 ! 2: 93c94a1b25 config: allow format_config() to filter
@@ Metadata
Author: Derrick Stolee <stolee@gmail.com>
## Commit message ##
- config: allow format_config() to filter
+ config: add 'gently' parameter to format_config()
- The format_config() method in builtin/config.c currently only uses
- git_config_*() methods for parsing. This allows parsing errors to result
- in die() messages appropriate with keys in the error message.
-
- In a future change we will want to use format_config() within 'git
- config list' to help format the output, including when --type=<X>
- arguments are provided. When the parsing fails in that case, that
- key-value pair should be omitted instead of causing a failure across the
- entire command.
-
- This change is formatted in such a way that the if/else-if structure
- allows the default die_on_error version to appear first and then be
- followed by the gentle parsing mode immediately afterwards.
-
- The only callers right now have die_on_parse set to 1.
+ This parameter is set to 0 for all current callers and is UNUSED.
+ However, we will start using this option in future changes and in a
+ critical change that requires gentle parsing (not using die()) to try
+ parsing all values in a list.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
## builtin/config.c ##
-@@
- #include "abspath.h"
- #include "config.h"
- #include "color.h"
-+#include "date.h"
- #include "editor.h"
- #include "environment.h"
- #include "gettext.h"
@@ builtin/config.c: struct strbuf_list {
+ * append it into strbuf `buf`. Returns a negative value on failure,
+ * 0 on success, 1 on a missing optional value (i.e., telling the
+ * caller to pretend that <key_,value_> did not exist).
++ *
++ * Note: 'gently' is currently ignored, but will be implemented in
++ * a future change.
*/
static int format_config(const struct config_display_options *opts,
struct strbuf *buf, const char *key_,
- const char *value_, const struct key_value_info *kvi)
+ const char *value_, const struct key_value_info *kvi,
-+ int die_on_parse)
++ int gently UNUSED)
{
if (opts->show_scope)
show_config_scope(opts, kvi, buf);
-@@ builtin/config.c: static int format_config(const struct config_display_options *opts,
- if (opts->show_keys)
- strbuf_addch(buf, opts->key_delim);
-
-- if (opts->type == TYPE_INT)
-+ if (opts->type == TYPE_INT && die_on_parse) {
- strbuf_addf(buf, "%"PRId64,
- git_config_int64(key_, value_ ? value_ : "", kvi));
-- else if (opts->type == TYPE_BOOL)
-+ } else if (opts->type == TYPE_INT) {
-+ int64_t v;
-+ int ret = git_parse_int64(value_, &v);
-+
-+ if (ret)
-+ return -1;
-+
-+ strbuf_addf(buf, "%"PRId64, v);
-+ }
-+ else if (opts->type == TYPE_BOOL && die_on_parse) {
- strbuf_addstr(buf, git_config_bool(key_, value_) ?
- "true" : "false");
-- else if (opts->type == TYPE_BOOL_OR_INT) {
-- int is_bool, v;
-- v = git_config_bool_or_int(key_, value_, kvi,
-- &is_bool);
-+ } else if (opts->type == TYPE_BOOL) {
-+ int value = git_parse_maybe_bool(value_);
-+
-+ if (value < 0)
-+ return -1;
-+
-+ strbuf_addstr(buf, value ? "true" : "false");
-+ } else if (opts->type == TYPE_BOOL_OR_INT && die_on_parse) {
-+ int is_bool = 0;
-+ int v = git_config_bool_or_int(key_, value_, kvi,
-+ &is_bool);
-+ if (is_bool)
-+ strbuf_addstr(buf, v ? "true" : "false");
-+ else
-+ strbuf_addf(buf, "%d", v);
-+ } else if (opts->type == TYPE_BOOL_OR_INT) {
-+ int is_bool = 0;
-+ int v = git_parse_maybe_bool_text(value_);
-+
-+ if (v < 0)
-+ return -1;
-+
- if (is_bool)
- strbuf_addstr(buf, v ? "true" : "false");
- else
- strbuf_addf(buf, "%d", v);
- } else if (opts->type == TYPE_BOOL_OR_STR) {
-+ /* Note: this can't fail to parse! */
- int v = git_parse_maybe_bool(value_);
- if (v < 0)
- strbuf_addstr(buf, value_);
- else
- strbuf_addstr(buf, v ? "true" : "false");
-- } else if (opts->type == TYPE_PATH) {
-+ } else if (opts->type == TYPE_PATH && die_on_parse) {
- char *v;
- if (git_config_pathname(&v, key_, value_) < 0)
- return -1;
-@@ builtin/config.c: static int format_config(const struct config_display_options *opts,
- else
- return 1; /* :(optional)no-such-file */
- free((char *)v);
-- } else if (opts->type == TYPE_EXPIRY_DATE) {
-+ } else if (opts->type == TYPE_PATH) {
-+ char *v;
-+ if (git_parse_maybe_pathname(value_, &v) < 0)
-+ return -1;
-+ if (v)
-+ strbuf_addstr(buf, v);
-+ else
-+ return 1; /* :(optional)no-such-file */
-+ free((char *)v);
-+ } else if (opts->type == TYPE_EXPIRY_DATE && die_on_parse) {
- timestamp_t t;
- if (git_config_expiry_date(&t, key_, value_) < 0)
- return -1;
- strbuf_addf(buf, "%"PRItime, t);
-- } else if (opts->type == TYPE_COLOR) {
-+ } else if (opts->type == TYPE_EXPIRY_DATE) {
-+ timestamp_t t;
-+ if (parse_expiry_date(value_, &t) < 0)
-+ return -1;
-+ strbuf_addf(buf, "%"PRItime, t);
-+ } else if (opts->type == TYPE_COLOR && die_on_parse) {
- char v[COLOR_MAXLEN];
- if (git_config_color(v, key_, value_) < 0)
- return -1;
- strbuf_addstr(buf, v);
-+ } else if (opts->type == TYPE_COLOR) {
-+ char v[COLOR_MAXLEN];
-+ if (color_parse(value_, v) < 0)
-+ return -1;
-+ strbuf_addstr(buf, v);
- } else if (value_) {
- strbuf_addstr(buf, value_);
- } else {
@@ builtin/config.c: static int collect_config(const char *key_, const char *value_,
strbuf_init(&values->items[values->nr], 0);
status = format_config(data->display_opts, &values->items[values->nr++],
- key_, value_, kvi);
-+ key_, value_, kvi, 1);
++ key_, value_, kvi, 0);
if (status < 0)
return status;
if (status) {
@@ builtin/config.c: static int get_value(const struct config_location_options *opt
status = format_config(display_opts, item, key_,
- display_opts->default_value, &kvi);
-+ display_opts->default_value, &kvi, 1);
++ display_opts->default_value, &kvi, 0);
if (status < 0)
die(_("failed to format default config value: %s"),
display_opts->default_value);
@@ builtin/config.c: static int get_urlmatch(const struct config_location_options *
status = format_config(&display_opts, &buf, item->string,
matched->value_is_null ? NULL : matched->value.buf,
- &matched->kvi);
-+ &matched->kvi, 1);
++ &matched->kvi, 0);
if (!status)
fwrite(buf.buf, 1, buf.len, stdout);
strbuf_release(&buf);
5: e27d52c4a5 ! 3: 6d2a48a3b7 config: make 'git config list --type=<X>' work
@@ Commit message
Previously, the --type=<X> argument to 'git config list' was ignored and
did nothing. Now, we add the use of format_config() to the
show_all_config() function so each key-value pair is attempted to be
- parsed.
+ parsed. This is our first use of the 'gently' parameter with a nonzero
+ value.
+
+ When listing multiple values, our initial settings for the output format
+ is different. Add a new init helper to specify the fact that keys should
+ be shown and also add the default delimiters as they were unset in some
+ cases.
If there is an error in parsing, then the row is not output.
@@ Commit message
behavior at all previously, so users can get the behavior they expect by
removing the --type argument or adding the --no-type argument.
+ t1300-config.sh is updated with the current behavior of this formatting
+ logic to justify the upcoming refactoring of format_config() that will
+ incrementally fix some of these cases to be more user-friendly.
+
Signed-off-by: Derrick Stolee <stolee@gmail.com>
## Documentation/git-config.adoc ##
@@ builtin/config.c: static int show_all_config(const char *key_, const char *value
- printf("%s%c%s%c", key_, opts->delim, value_, opts->term);
- else
- printf("%s%c", key_, opts->term);
-+ if (format_config(opts, &formatted, key_, value_, kvi, 0) >= 0)
++ if (format_config(opts, &formatted, key_, value_, kvi, 1) >= 0)
+ fwrite(formatted.buf, 1, formatted.len, stdout);
+
+ strbuf_release(&formatted);
return 0;
}
+@@ builtin/config.c: static void display_options_init(struct config_display_options *opts)
+ }
+ }
+
++static void display_options_init_list(struct config_display_options *opts)
++{
++ opts->show_keys = 1;
++
++ if (opts->end_nul) {
++ display_options_init(opts);
++ } else {
++ opts->term = '\n';
++ opts->delim = ' ';
++ opts->key_delim = '=';
++ }
++}
++
+ static int cmd_config_list(int argc, const char **argv, const char *prefix,
+ struct repository *repo UNUSED)
+ {
+@@ builtin/config.c: static int cmd_config_list(int argc, const char **argv, const char *prefix,
+ check_argc(argc, 0, 0);
+
+ location_options_init(&location_opts, prefix);
+- display_options_init(&display_opts);
++ display_options_init_list(&display_opts);
+
+ setup_auto_pager("config", 1);
+
+@@ builtin/config.c: static int cmd_config_actions(int argc, const char **argv, const char *prefix)
+
+ if (actions == ACTION_LIST) {
+ check_argc(argc, 0, 0);
++ display_options_init_list(&display_opts);
+ if (config_with_options(show_all_config, &display_opts,
+ &location_opts.source, the_repository,
+ &location_opts.options) < 0) {
## t/t1300-config.sh ##
@@ t/t1300-config.sh: done
@@ t/t1300-config.sh: done
number = 10
big = 1M
+path = ~/dir
++red = red
++blue = Blue
++date = Fri Jun 4 15:46:55 2010
EOF
test_expect_success 'identical modern --type specifiers are allowed' '
@@ t/t1300-config.sh: test_expect_success 'unset type specifiers may be reset to co
+ section.big=true
+ EOF
+
-+ git config ${mode_prefix}list --type=bool >actual &&
-+ test_cmp expect actual
++ test_must_fail git config ${mode_prefix}list --type=bool
+'
+
+test_expect_success 'list --type=path shows only canonicalizable path values' '
@@ t/t1300-config.sh: test_expect_success 'unset type specifiers may be reset to co
+ section.number=10
+ section.big=1M
+ section.path=$HOME/dir
++ section.red=red
++ section.blue=Blue
++ section.date=Fri Jun 4 15:46:55 2010
++ EOF
++
++ git config ${mode_prefix}list --type=path >actual 2>err &&
++ test_cmp expect actual &&
++ test_must_be_empty err
++'
++
++test_expect_success 'list --type=expiry-date shows only canonicalizable dates' '
++ cat >expecterr <<-EOF &&
++ error: '\''True'\'' for '\''section.foo'\'' is not a valid timestamp
++ error: '\''~/dir'\'' for '\''section.path'\'' is not a valid timestamp
++ error: '\''red'\'' for '\''section.red'\'' is not a valid timestamp
++ error: '\''Blue'\'' for '\''section.blue'\'' is not a valid timestamp
++ EOF
++
++ git config ${mode_prefix}list --type=expiry-date >actual 2>err &&
++
++ # section.number and section.big parse as relative dates that could
++ # have clock skew in their results.
++ test_grep section.big actual &&
++ test_grep section.number actual &&
++ test_grep "section.date=$(git config --type=expiry-date section.$key)" actual &&
++ test_cmp expecterr err
++'
++
++test_expect_success 'list --type=color shows only canonicalizable color values' '
++ cat >expect <<-EOF &&
++ section.number=<>
++ section.red=<RED>
++ section.blue=<BLUE>
++ EOF
++
++ cat >expecterr <<-EOF &&
++ error: invalid color value: True
++ error: invalid color value: 1M
++ error: invalid color value: ~/dir
++ error: invalid color value: Fri Jun 4 15:46:55 2010
+ EOF
+
-+ git config ${mode_prefix}list --type=path >actual &&
-+ test_cmp expect actual
++ git config ${mode_prefix}list --type=color >actual.raw 2>err &&
++ test_decode_color <actual.raw >actual &&
++ test_cmp expect actual &&
++ test_cmp expecterr err
+'
+
test_expect_success '--type rejects unknown specifiers' '
4: 5601a5a84f ! 4: 2bca4d2316 config: create special init for list mode
@@ Metadata
Author: Derrick Stolee <stolee@gmail.com>
## Commit message ##
- config: create special init for list mode
+ config: format int64s gently
- When listing multiple values, our initial settings for the output format
- is different. Add a new init helper to specify the fact that keys should
- be shown and also add the default delimiters as they were unset in some
- cases.
-
- There are two places, differing by the 'git config list' and 'git config
- --list' modes.
+ Move the logic for formatting int64 config values into a helper method
+ and use gentle parsing when needed.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
## builtin/config.c ##
-@@ builtin/config.c: static void display_options_init(struct config_display_options *opts)
- }
- }
+@@ builtin/config.c: struct strbuf_list {
+ int alloc;
+ };
-+static void display_options_init_list(struct config_display_options *opts)
++static int format_config_int64(struct strbuf *buf,
++ const char *key_,
++ const char *value_,
++ const struct key_value_info *kvi,
++ int gently)
+{
-+ opts->show_keys = 1;
-+
-+ if (opts->end_nul) {
-+ display_options_init(opts);
++ int64_t v = 0;
++ if (gently) {
++ if (git_parse_int64(value_, &v))
++ return -1;
+ } else {
-+ opts->term = '\n';
-+ opts->delim = ' ';
-+ opts->key_delim = '=';
++ /* may die() */
++ v = git_config_int64(key_, value_ ? value_ : "", kvi);
+ }
++
++ strbuf_addf(buf, "%"PRId64, v);
++ return 0;
+}
+
- static int cmd_config_list(int argc, const char **argv, const char *prefix,
- struct repository *repo UNUSED)
+ /*
+ * Format the configuration key-value pair (`key_`, `value_`) and
+ * append it into strbuf `buf`. Returns a negative value on failure,
+@@ builtin/config.c: struct strbuf_list {
+ static int format_config(const struct config_display_options *opts,
+ struct strbuf *buf, const char *key_,
+ const char *value_, const struct key_value_info *kvi,
+- int gently UNUSED)
++ int gently)
{
-@@ builtin/config.c: static int cmd_config_list(int argc, const char **argv, const char *prefix,
- check_argc(argc, 0, 0);
++ int res = 0;
+ if (opts->show_scope)
+ show_config_scope(opts, kvi, buf);
+ if (opts->show_origin)
+@@ builtin/config.c: static int format_config(const struct config_display_options *opts,
+ strbuf_addch(buf, opts->key_delim);
- location_options_init(&location_opts, prefix);
-- display_options_init(&display_opts);
-+ display_options_init_list(&display_opts);
-
- setup_auto_pager("config", 1);
-
-@@ builtin/config.c: static int cmd_config_actions(int argc, const char **argv, const char *prefix)
+ if (opts->type == TYPE_INT)
+- strbuf_addf(buf, "%"PRId64,
+- git_config_int64(key_, value_ ? value_ : "", kvi));
++ res = format_config_int64(buf, key_, value_, kvi, gently);
+ else if (opts->type == TYPE_BOOL)
+ strbuf_addstr(buf, git_config_bool(key_, value_) ?
+ "true" : "false");
+@@ builtin/config.c: static int format_config(const struct config_display_options *opts,
+ }
+ }
+ strbuf_addch(buf, opts->term);
+- return 0;
++ return res;
+ }
- if (actions == ACTION_LIST) {
- check_argc(argc, 0, 0);
-+ display_options_init_list(&display_opts);
- if (config_with_options(show_all_config, &display_opts,
- &location_opts.source, the_repository,
- &location_opts.options) < 0) {
+ static int show_all_config(const char *key_, const char *value_,
-: ---------- > 5: f8e0b8304f config: format bools gently
-: ---------- > 6: 0a428d2ffe config: format bools or ints gently
-: ---------- > 7: 3fec3abbd6 config: format bools or strings in helper
2: 8d3a6a8265 ! 8: fafafc5465 parse: add git_parse_maybe_pathname()
@@ Metadata
## Commit message ##
parse: add git_parse_maybe_pathname()
- This extraction of logic from config.c's git_config_pathname() allows
- for parsing a fully-qualified path from a relative path along with
- validation of the existence of the path without failing with a die().
+ The git_config_pathname() method parses a config value as a path, but
+ always die()s on an error. Move this logic into a gentler parsing
+ algorithm that will return an error value instead of ending the process.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
-: ---------- > 9: d1cfa0c5e1 config: format paths gently
-: ---------- > 10: 9221ca2352 config: format expiry dates gently
-: ---------- > 11: ddf6131ac9 color: add color_parse_gently()
-: ---------- > 12: d14937e6d1 config: format colors gently
-: ---------- > 13: 48fc882785 config: restructure format_config()
--
gitgitgadget |
|
User |
|
Submitted as pull.2044.v2.git.1771026918.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
In anticipation of using format_config() in this method, move
show_all_config() lower in the file without changes.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
builtin/config.c | 48 ++++++++++++++++++++++++------------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 288ebdfdaa..237f7a934d 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -231,30 +231,6 @@ static void show_config_scope(const struct config_display_options *opts,
strbuf_addch(buf, term);
}
-static int show_all_config(const char *key_, const char *value_,
- const struct config_context *ctx,
- void *cb)
-{
- const struct config_display_options *opts = cb;
- const struct key_value_info *kvi = ctx->kvi;
-
- if (opts->show_origin || opts->show_scope) {
- struct strbuf buf = STRBUF_INIT;
- if (opts->show_scope)
- show_config_scope(opts, kvi, &buf);
- if (opts->show_origin)
- show_config_origin(opts, kvi, &buf);
- /* Use fwrite as "buf" can contain \0's if "end_null" is set. */
- fwrite(buf.buf, 1, buf.len, stdout);
- strbuf_release(&buf);
- }
- if (!opts->omit_values && value_)
- printf("%s%c%s%c", key_, opts->delim, value_, opts->term);
- else
- printf("%s%c", key_, opts->term);
- return 0;
-}
-
struct strbuf_list {
struct strbuf *items;
int nr;
@@ -332,6 +308,30 @@ static int format_config(const struct config_display_options *opts,
return 0;
}
+static int show_all_config(const char *key_, const char *value_,
+ const struct config_context *ctx,
+ void *cb)
+{
+ const struct config_display_options *opts = cb;
+ const struct key_value_info *kvi = ctx->kvi;
+
+ if (opts->show_origin || opts->show_scope) {
+ struct strbuf buf = STRBUF_INIT;
+ if (opts->show_scope)
+ show_config_scope(opts, kvi, &buf);
+ if (opts->show_origin)
+ show_config_origin(opts, kvi, &buf);
+ /* Use fwrite as "buf" can contain \0's if "end_null" is set. */
+ fwrite(buf.buf, 1, buf.len, stdout);
+ strbuf_release(&buf);
+ }
+ if (!opts->omit_values && value_)
+ printf("%s%c%s%c", key_, opts->delim, value_, opts->term);
+ else
+ printf("%s%c", key_, opts->term);
+ return 0;
+}
+
#define GET_VALUE_ALL (1 << 0)
#define GET_VALUE_KEY_REGEXP (1 << 1)
--
gitgitgadget
|
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
This parameter is set to 0 for all current callers and is UNUSED.
However, we will start using this option in future changes and in a
critical change that requires gentle parsing (not using die()) to try
parsing all values in a list.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
builtin/config.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 237f7a934d..b4c4228311 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -242,10 +242,14 @@ struct strbuf_list {
* append it into strbuf `buf`. Returns a negative value on failure,
* 0 on success, 1 on a missing optional value (i.e., telling the
* caller to pretend that <key_,value_> did not exist).
+ *
+ * Note: 'gently' is currently ignored, but will be implemented in
+ * a future change.
*/
static int format_config(const struct config_display_options *opts,
struct strbuf *buf, const char *key_,
- const char *value_, const struct key_value_info *kvi)
+ const char *value_, const struct key_value_info *kvi,
+ int gently UNUSED)
{
if (opts->show_scope)
show_config_scope(opts, kvi, buf);
@@ -372,7 +376,7 @@ static int collect_config(const char *key_, const char *value_,
strbuf_init(&values->items[values->nr], 0);
status = format_config(data->display_opts, &values->items[values->nr++],
- key_, value_, kvi);
+ key_, value_, kvi, 0);
if (status < 0)
return status;
if (status) {
@@ -463,7 +467,7 @@ static int get_value(const struct config_location_options *opts,
strbuf_init(item, 0);
status = format_config(display_opts, item, key_,
- display_opts->default_value, &kvi);
+ display_opts->default_value, &kvi, 0);
if (status < 0)
die(_("failed to format default config value: %s"),
display_opts->default_value);
@@ -743,7 +747,7 @@ static int get_urlmatch(const struct config_location_options *opts,
status = format_config(&display_opts, &buf, item->string,
matched->value_is_null ? NULL : matched->value.buf,
- &matched->kvi);
+ &matched->kvi, 0);
if (!status)
fwrite(buf.buf, 1, buf.len, stdout);
strbuf_release(&buf);
--
gitgitgadget
|
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
Previously, the --type=<X> argument to 'git config list' was ignored and
did nothing. Now, we add the use of format_config() to the
show_all_config() function so each key-value pair is attempted to be
parsed. This is our first use of the 'gently' parameter with a nonzero
value.
When listing multiple values, our initial settings for the output format
is different. Add a new init helper to specify the fact that keys should
be shown and also add the default delimiters as they were unset in some
cases.
If there is an error in parsing, then the row is not output.
This is a change in behavior! We are starting to respect an option that
was previously ignored, leading to potential user confusion. This is
probably still a good option, since the --type argument did not change
behavior at all previously, so users can get the behavior they expect by
removing the --type argument or adding the --no-type argument.
t1300-config.sh is updated with the current behavior of this formatting
logic to justify the upcoming refactoring of format_config() that will
incrementally fix some of these cases to be more user-friendly.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
Documentation/git-config.adoc | 3 ++
builtin/config.c | 35 ++++++++++--------
t/t1300-config.sh | 70 ++++++++++++++++++++++++++++++++++-
3 files changed, 92 insertions(+), 16 deletions(-)
diff --git a/Documentation/git-config.adoc b/Documentation/git-config.adoc
index ac3b536a15..5300dd4c51 100644
--- a/Documentation/git-config.adoc
+++ b/Documentation/git-config.adoc
@@ -240,6 +240,9 @@ Valid `<type>`'s include:
that the given value is canonicalize-able as an ANSI color, but it is written
as-is.
+
+If the command is in `list` mode, then the `--type <type>` argument will apply
+to each listed config value. If the value does not successfully parse in that
+format, then it will be omitted from the list.
--bool::
--int::
diff --git a/builtin/config.c b/builtin/config.c
index b4c4228311..4c4c791883 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -318,21 +318,12 @@ static int show_all_config(const char *key_, const char *value_,
{
const struct config_display_options *opts = cb;
const struct key_value_info *kvi = ctx->kvi;
+ struct strbuf formatted = STRBUF_INIT;
- if (opts->show_origin || opts->show_scope) {
- struct strbuf buf = STRBUF_INIT;
- if (opts->show_scope)
- show_config_scope(opts, kvi, &buf);
- if (opts->show_origin)
- show_config_origin(opts, kvi, &buf);
- /* Use fwrite as "buf" can contain \0's if "end_null" is set. */
- fwrite(buf.buf, 1, buf.len, stdout);
- strbuf_release(&buf);
- }
- if (!opts->omit_values && value_)
- printf("%s%c%s%c", key_, opts->delim, value_, opts->term);
- else
- printf("%s%c", key_, opts->term);
+ if (format_config(opts, &formatted, key_, value_, kvi, 1) >= 0)
+ fwrite(formatted.buf, 1, formatted.len, stdout);
+
+ strbuf_release(&formatted);
return 0;
}
@@ -872,6 +863,19 @@ static void display_options_init(struct config_display_options *opts)
}
}
+static void display_options_init_list(struct config_display_options *opts)
+{
+ opts->show_keys = 1;
+
+ if (opts->end_nul) {
+ display_options_init(opts);
+ } else {
+ opts->term = '\n';
+ opts->delim = ' ';
+ opts->key_delim = '=';
+ }
+}
+
static int cmd_config_list(int argc, const char **argv, const char *prefix,
struct repository *repo UNUSED)
{
@@ -890,7 +894,7 @@ static int cmd_config_list(int argc, const char **argv, const char *prefix,
check_argc(argc, 0, 0);
location_options_init(&location_opts, prefix);
- display_options_init(&display_opts);
+ display_options_init_list(&display_opts);
setup_auto_pager("config", 1);
@@ -1321,6 +1325,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix)
if (actions == ACTION_LIST) {
check_argc(argc, 0, 0);
+ display_options_init_list(&display_opts);
if (config_with_options(show_all_config, &display_opts,
&location_opts.source, the_repository,
&location_opts.options) < 0) {
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 9850fcd5b5..362e580604 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2459,9 +2459,13 @@ done
cat >.git/config <<-\EOF &&
[section]
-foo = true
+foo = True
number = 10
big = 1M
+path = ~/dir
+red = red
+blue = Blue
+date = Fri Jun 4 15:46:55 2010
EOF
test_expect_success 'identical modern --type specifiers are allowed' '
@@ -2503,6 +2507,70 @@ test_expect_success 'unset type specifiers may be reset to conflicting ones' '
test_cmp_config 1048576 --type=bool --no-type --type=int section.big
'
+test_expect_success 'list --type=bool shows only canonicalizable bool values' '
+ cat >expect <<-EOF &&
+ section.foo=true
+ section.number=true
+ section.big=true
+ EOF
+
+ test_must_fail git config ${mode_prefix}list --type=bool
+'
+
+test_expect_success 'list --type=path shows only canonicalizable path values' '
+ cat >expect <<-EOF &&
+ section.foo=True
+ section.number=10
+ section.big=1M
+ section.path=$HOME/dir
+ section.red=red
+ section.blue=Blue
+ section.date=Fri Jun 4 15:46:55 2010
+ EOF
+
+ git config ${mode_prefix}list --type=path >actual 2>err &&
+ test_cmp expect actual &&
+ test_must_be_empty err
+'
+
+test_expect_success 'list --type=expiry-date shows only canonicalizable dates' '
+ cat >expecterr <<-EOF &&
+ error: '\''True'\'' for '\''section.foo'\'' is not a valid timestamp
+ error: '\''~/dir'\'' for '\''section.path'\'' is not a valid timestamp
+ error: '\''red'\'' for '\''section.red'\'' is not a valid timestamp
+ error: '\''Blue'\'' for '\''section.blue'\'' is not a valid timestamp
+ EOF
+
+ git config ${mode_prefix}list --type=expiry-date >actual 2>err &&
+
+ # section.number and section.big parse as relative dates that could
+ # have clock skew in their results.
+ test_grep section.big actual &&
+ test_grep section.number actual &&
+ test_grep "section.date=$(git config --type=expiry-date section.$key)" actual &&
+ test_cmp expecterr err
+'
+
+test_expect_success 'list --type=color shows only canonicalizable color values' '
+ cat >expect <<-EOF &&
+ section.number=<>
+ section.red=<RED>
+ section.blue=<BLUE>
+ EOF
+
+ cat >expecterr <<-EOF &&
+ error: invalid color value: True
+ error: invalid color value: 1M
+ error: invalid color value: ~/dir
+ error: invalid color value: Fri Jun 4 15:46:55 2010
+ EOF
+
+ git config ${mode_prefix}list --type=color >actual.raw 2>err &&
+ test_decode_color <actual.raw >actual &&
+ test_cmp expect actual &&
+ test_cmp expecterr err
+'
+
test_expect_success '--type rejects unknown specifiers' '
test_must_fail git config --type=nonsense section.foo 2>error &&
test_grep "unrecognized --type argument" error
--
gitgitgadget
|
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting int64 config values into a helper method
and use gentle parsing when needed.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
builtin/config.c | 27 +++++++++++++++++++++++----
1 file changed, 23 insertions(+), 4 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 4c4c791883..d259a91d53 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -237,6 +237,25 @@ struct strbuf_list {
int alloc;
};
+static int format_config_int64(struct strbuf *buf,
+ const char *key_,
+ const char *value_,
+ const struct key_value_info *kvi,
+ int gently)
+{
+ int64_t v = 0;
+ if (gently) {
+ if (git_parse_int64(value_, &v))
+ return -1;
+ } else {
+ /* may die() */
+ v = git_config_int64(key_, value_ ? value_ : "", kvi);
+ }
+
+ strbuf_addf(buf, "%"PRId64, v);
+ return 0;
+}
+
/*
* Format the configuration key-value pair (`key_`, `value_`) and
* append it into strbuf `buf`. Returns a negative value on failure,
@@ -249,8 +268,9 @@ struct strbuf_list {
static int format_config(const struct config_display_options *opts,
struct strbuf *buf, const char *key_,
const char *value_, const struct key_value_info *kvi,
- int gently UNUSED)
+ int gently)
{
+ int res = 0;
if (opts->show_scope)
show_config_scope(opts, kvi, buf);
if (opts->show_origin)
@@ -262,8 +282,7 @@ static int format_config(const struct config_display_options *opts,
strbuf_addch(buf, opts->key_delim);
if (opts->type == TYPE_INT)
- strbuf_addf(buf, "%"PRId64,
- git_config_int64(key_, value_ ? value_ : "", kvi));
+ res = format_config_int64(buf, key_, value_, kvi, gently);
else if (opts->type == TYPE_BOOL)
strbuf_addstr(buf, git_config_bool(key_, value_) ?
"true" : "false");
@@ -309,7 +328,7 @@ static int format_config(const struct config_display_options *opts,
}
}
strbuf_addch(buf, opts->term);
- return 0;
+ return res;
}
static int show_all_config(const char *key_, const char *value_,
--
gitgitgadget
|
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting bool config values into a helper method
and use gentle parsing when needed.
This makes 'git config list --type=bool' not fail when coming across a
non-boolean value. Such unparseable values are filtered out quietly.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
builtin/config.c | 21 +++++++++++++++++++--
t/t1300-config.sh | 4 +++-
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index d259a91d53..2c169fc126 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -256,6 +256,24 @@ static int format_config_int64(struct strbuf *buf,
return 0;
}
+static int format_config_bool(struct strbuf *buf,
+ const char *key_,
+ const char *value_,
+ int gently)
+{
+ int v = 0;
+ if (gently) {
+ if ((v = git_parse_maybe_bool(value_)) < 0)
+ return -1;
+ } else {
+ /* may die() */
+ v = git_config_bool(key_, value_);
+ }
+
+ strbuf_addstr(buf, v ? "true" : "false");
+ return 0;
+}
+
/*
* Format the configuration key-value pair (`key_`, `value_`) and
* append it into strbuf `buf`. Returns a negative value on failure,
@@ -284,8 +302,7 @@ static int format_config(const struct config_display_options *opts,
if (opts->type == TYPE_INT)
res = format_config_int64(buf, key_, value_, kvi, gently);
else if (opts->type == TYPE_BOOL)
- strbuf_addstr(buf, git_config_bool(key_, value_) ?
- "true" : "false");
+ res = format_config_bool(buf, key_, value_, gently);
else if (opts->type == TYPE_BOOL_OR_INT) {
int is_bool, v;
v = git_config_bool_or_int(key_, value_, kvi,
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 362e580604..59a82b9aef 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2514,7 +2514,9 @@ test_expect_success 'list --type=bool shows only canonicalizable bool values' '
section.big=true
EOF
- test_must_fail git config ${mode_prefix}list --type=bool
+ git config ${mode_prefix}list --type=bool >actual 2>err &&
+ test_cmp expect actual &&
+ test_must_be_empty err
'
test_expect_success 'list --type=path shows only canonicalizable path values' '
--
gitgitgadget
|
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting bool-or-int config values into a helper
method and use gentle parsing when needed.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
builtin/config.c | 40 +++++++++++++++++++++++++++++++---------
1 file changed, 31 insertions(+), 9 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 2c169fc126..2c93e1725b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -274,6 +274,34 @@ static int format_config_bool(struct strbuf *buf,
return 0;
}
+static int format_config_bool_or_int(struct strbuf *buf,
+ const char *key_,
+ const char *value_,
+ const struct key_value_info *kvi,
+ int gently)
+{
+ int v, is_bool = 0;
+
+ if (gently) {
+ v = git_parse_maybe_bool_text(value_);
+
+ if (v >= 0)
+ is_bool = 1;
+ else if (git_parse_int(value_, &v))
+ return -1;
+ } else {
+ v = git_config_bool_or_int(key_, value_, kvi,
+ &is_bool);
+ }
+
+ if (is_bool)
+ strbuf_addstr(buf, v ? "true" : "false");
+ else
+ strbuf_addf(buf, "%d", v);
+
+ return 0;
+}
+
/*
* Format the configuration key-value pair (`key_`, `value_`) and
* append it into strbuf `buf`. Returns a negative value on failure,
@@ -303,15 +331,9 @@ static int format_config(const struct config_display_options *opts,
res = format_config_int64(buf, key_, value_, kvi, gently);
else if (opts->type == TYPE_BOOL)
res = format_config_bool(buf, key_, value_, gently);
- else if (opts->type == TYPE_BOOL_OR_INT) {
- int is_bool, v;
- v = git_config_bool_or_int(key_, value_, kvi,
- &is_bool);
- if (is_bool)
- strbuf_addstr(buf, v ? "true" : "false");
- else
- strbuf_addf(buf, "%d", v);
- } else if (opts->type == TYPE_BOOL_OR_STR) {
+ else if (opts->type == TYPE_BOOL_OR_INT)
+ res = format_config_bool_or_int(buf, key_, value_, kvi, gently);
+ else if (opts->type == TYPE_BOOL_OR_STR) {
int v = git_parse_maybe_bool(value_);
if (v < 0)
strbuf_addstr(buf, value_);
--
gitgitgadget
|
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting bool-or-string config values into a
helper. This parsing has always been gentle, so this is not unlocking
new behavior. This extraction is only to match the formatting of the
other cases that do need a behavior change.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
builtin/config.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 2c93e1725b..0c539ff98e 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -302,6 +302,18 @@ static int format_config_bool_or_int(struct strbuf *buf,
return 0;
}
+/* This mode is always gentle. */
+static int format_config_bool_or_str(struct strbuf *buf,
+ const char *value_)
+{
+ int v = git_parse_maybe_bool(value_);
+ if (v < 0)
+ strbuf_addstr(buf, value_);
+ else
+ strbuf_addstr(buf, v ? "true" : "false");
+ return 0;
+}
+
/*
* Format the configuration key-value pair (`key_`, `value_`) and
* append it into strbuf `buf`. Returns a negative value on failure,
@@ -333,13 +345,9 @@ static int format_config(const struct config_display_options *opts,
res = format_config_bool(buf, key_, value_, gently);
else if (opts->type == TYPE_BOOL_OR_INT)
res = format_config_bool_or_int(buf, key_, value_, kvi, gently);
- else if (opts->type == TYPE_BOOL_OR_STR) {
- int v = git_parse_maybe_bool(value_);
- if (v < 0)
- strbuf_addstr(buf, value_);
- else
- strbuf_addstr(buf, v ? "true" : "false");
- } else if (opts->type == TYPE_PATH) {
+ else if (opts->type == TYPE_BOOL_OR_STR)
+ res = format_config_bool_or_str(buf, value_);
+ else if (opts->type == TYPE_PATH) {
char *v;
if (git_config_pathname(&v, key_, value_) < 0)
return -1;
--
gitgitgadget
|
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
The git_config_pathname() method parses a config value as a path, but
always die()s on an error. Move this logic into a gentler parsing
algorithm that will return an error value instead of ending the process.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
config.c | 14 +-------------
parse.c | 24 ++++++++++++++++++++++++
parse.h | 2 ++
3 files changed, 27 insertions(+), 13 deletions(-)
diff --git a/config.c b/config.c
index 7f6d53b473..83257b7a97 100644
--- a/config.c
+++ b/config.c
@@ -1278,24 +1278,12 @@ int git_config_string(char **dest, const char *var, const char *value)
int git_config_pathname(char **dest, const char *var, const char *value)
{
- bool is_optional;
- char *path;
-
if (!value)
return config_error_nonbool(var);
- is_optional = skip_prefix(value, ":(optional)", &value);
- path = interpolate_path(value, 0);
- if (!path)
+ if (git_parse_maybe_pathname(value, dest) < 0)
die(_("failed to expand user dir in: '%s'"), value);
- if (is_optional && is_missing_file(path)) {
- free(path);
- *dest = NULL;
- return 0;
- }
-
- *dest = path;
return 0;
}
diff --git a/parse.c b/parse.c
index 48313571aa..3f37f0b93a 100644
--- a/parse.c
+++ b/parse.c
@@ -1,6 +1,7 @@
#include "git-compat-util.h"
#include "gettext.h"
#include "parse.h"
+#include "path.h"
static uintmax_t get_unit_factor(const char *end)
{
@@ -209,3 +210,26 @@ unsigned long git_env_ulong(const char *k, unsigned long val)
die(_("failed to parse %s"), k);
return val;
}
+
+int git_parse_maybe_pathname(const char *value, char **dest)
+{
+ bool is_optional;
+ char *path;
+
+ if (!value)
+ return -1;
+
+ is_optional = skip_prefix(value, ":(optional)", &value);
+ path = interpolate_path(value, 0);
+ if (!path)
+ return -1;
+
+ if (is_optional && is_missing_file(path)) {
+ free(path);
+ *dest = NULL;
+ return 0;
+ }
+
+ *dest = path;
+ return 0;
+}
diff --git a/parse.h b/parse.h
index ea32de9a91..4f97c3727a 100644
--- a/parse.h
+++ b/parse.h
@@ -19,4 +19,6 @@ int git_parse_maybe_bool_text(const char *value);
int git_env_bool(const char *, int);
unsigned long git_env_ulong(const char *, unsigned long);
+int git_parse_maybe_pathname(const char *value, char **dest);
+
#endif /* PARSE_H */
--
gitgitgadget
|
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting path config values into a helper method
and use gentle parsing when needed.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
builtin/config.c | 35 +++++++++++++++++++++++++----------
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 0c539ff98e..4664651dd2 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -314,6 +314,28 @@ static int format_config_bool_or_str(struct strbuf *buf,
return 0;
}
+static int format_config_path(struct strbuf *buf,
+ const char *key_,
+ const char *value_,
+ int gently)
+{
+ char *v;
+ if (gently) {
+ if (git_parse_maybe_pathname(value_, &v) < 0)
+ return -1;
+ } else if (git_config_pathname(&v, key_, value_) < 0) {
+ return -1;
+ }
+
+ if (v)
+ strbuf_addstr(buf, v);
+ else
+ return 1; /* :(optional)no-such-file */
+
+ free(v);
+ return 0;
+}
+
/*
* Format the configuration key-value pair (`key_`, `value_`) and
* append it into strbuf `buf`. Returns a negative value on failure,
@@ -347,16 +369,9 @@ static int format_config(const struct config_display_options *opts,
res = format_config_bool_or_int(buf, key_, value_, kvi, gently);
else if (opts->type == TYPE_BOOL_OR_STR)
res = format_config_bool_or_str(buf, value_);
- else if (opts->type == TYPE_PATH) {
- char *v;
- if (git_config_pathname(&v, key_, value_) < 0)
- return -1;
- if (v)
- strbuf_addstr(buf, v);
- else
- return 1; /* :(optional)no-such-file */
- free((char *)v);
- } else if (opts->type == TYPE_EXPIRY_DATE) {
+ else if (opts->type == TYPE_PATH)
+ res = format_config_path(buf, key_, value_, gently);
+ else if (opts->type == TYPE_EXPIRY_DATE) {
timestamp_t t;
if (git_config_expiry_date(&t, key_, value_) < 0)
return -1;
--
gitgitgadget
|
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting expiry date config values into a helper
method and use gentle parsing when needed.
There is an unfortunate asymmetry in these two parsing methods, but we
need to treat a positive response from parse_expiry_date() as an error
or we will get incorrect values.
This updates the behavior of 'git config list --type=expiry-date' to be
quiet when attempting parsing on non-date values.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
builtin/config.c | 27 +++++++++++++++++++++------
t/t1300-config.sh | 9 +--------
2 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 4664651dd2..71b685d943 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -3,6 +3,7 @@
#include "abspath.h"
#include "config.h"
#include "color.h"
+#include "date.h"
#include "editor.h"
#include "environment.h"
#include "gettext.h"
@@ -336,6 +337,23 @@ static int format_config_path(struct strbuf *buf,
return 0;
}
+static int format_config_expiry_date(struct strbuf *buf,
+ const char *key_,
+ const char *value_,
+ int gently)
+{
+ timestamp_t t;
+ if (gently) {
+ if (parse_expiry_date(value_, &t))
+ return -1;
+ } else if (git_config_expiry_date(&t, key_, value_) < 0) {
+ return -1;
+ }
+
+ strbuf_addf(buf, "%"PRItime, t);
+ return 0;
+}
+
/*
* Format the configuration key-value pair (`key_`, `value_`) and
* append it into strbuf `buf`. Returns a negative value on failure,
@@ -371,12 +389,9 @@ static int format_config(const struct config_display_options *opts,
res = format_config_bool_or_str(buf, value_);
else if (opts->type == TYPE_PATH)
res = format_config_path(buf, key_, value_, gently);
- else if (opts->type == TYPE_EXPIRY_DATE) {
- timestamp_t t;
- if (git_config_expiry_date(&t, key_, value_) < 0)
- return -1;
- strbuf_addf(buf, "%"PRItime, t);
- } else if (opts->type == TYPE_COLOR) {
+ else if (opts->type == TYPE_EXPIRY_DATE)
+ res = format_config_expiry_date(buf, key_, value_, gently);
+ else if (opts->type == TYPE_COLOR) {
char v[COLOR_MAXLEN];
if (git_config_color(v, key_, value_) < 0)
return -1;
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 59a82b9aef..c134d85d8a 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2536,13 +2536,6 @@ test_expect_success 'list --type=path shows only canonicalizable path values' '
'
test_expect_success 'list --type=expiry-date shows only canonicalizable dates' '
- cat >expecterr <<-EOF &&
- error: '\''True'\'' for '\''section.foo'\'' is not a valid timestamp
- error: '\''~/dir'\'' for '\''section.path'\'' is not a valid timestamp
- error: '\''red'\'' for '\''section.red'\'' is not a valid timestamp
- error: '\''Blue'\'' for '\''section.blue'\'' is not a valid timestamp
- EOF
-
git config ${mode_prefix}list --type=expiry-date >actual 2>err &&
# section.number and section.big parse as relative dates that could
@@ -2550,7 +2543,7 @@ test_expect_success 'list --type=expiry-date shows only canonicalizable dates' '
test_grep section.big actual &&
test_grep section.number actual &&
test_grep "section.date=$(git config --type=expiry-date section.$key)" actual &&
- test_cmp expecterr err
+ test_must_be_empty err
'
test_expect_success 'list --type=color shows only canonicalizable color values' '
--
gitgitgadget
|
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
When parsing colors, a failed parse leads to an error message due to the
result returning error(). To allow for quiet parsing, create
color_parse_gently().
To accomplish this, convert the implementation of color_parse_mem() into
a static color_parse_mem_1() helper that adds a 'gently' parameter. The
color_parse_gently() method can then use this. Since it is a near
equivalent to color_parse(), move that method down in the file so they
can be nearby while also appearing after color_parse_mem_1().
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
color.c | 25 ++++++++++++++++++-------
color.h | 1 +
2 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/color.c b/color.c
index 07ac8c9d40..ec8872d2dd 100644
--- a/color.c
+++ b/color.c
@@ -223,11 +223,6 @@ static int parse_attr(const char *name, size_t len)
return -1;
}
-int color_parse(const char *value, char *dst)
-{
- return color_parse_mem(value, strlen(value), dst);
-}
-
/*
* Write the ANSI color codes for "c" to "out"; the string should
* already have the ANSI escape code in it. "out" should have enough
@@ -264,7 +259,8 @@ static int color_empty(const struct color *c)
return c->type <= COLOR_NORMAL;
}
-int color_parse_mem(const char *value, int value_len, char *dst)
+static int color_parse_mem_1(const char *value, int value_len,
+ char *dst, int gently)
{
const char *ptr = value;
int len = value_len;
@@ -365,10 +361,25 @@ int color_parse_mem(const char *value, int value_len, char *dst)
OUT(0);
return 0;
bad:
- return error(_("invalid color value: %.*s"), value_len, value);
+ return gently ? -1 : error(_("invalid color value: %.*s"), value_len, value);
#undef OUT
}
+int color_parse_mem(const char *value, int value_len, char *dst)
+{
+ return color_parse_mem_1(value, value_len, dst, 0);
+}
+
+int color_parse(const char *value, char *dst)
+{
+ return color_parse_mem(value, strlen(value), dst);
+}
+
+int color_parse_gently(const char *value, char *dst)
+{
+ return color_parse_mem_1(value, strlen(value), dst, 1);
+}
+
enum git_colorbool git_config_colorbool(const char *var, const char *value)
{
if (value) {
diff --git a/color.h b/color.h
index 43e6c9ad09..30c783405d 100644
--- a/color.h
+++ b/color.h
@@ -118,6 +118,7 @@ bool want_color_fd(int fd, enum git_colorbool var);
* terminal.
*/
int color_parse(const char *value, char *dst);
+int color_parse_gently(const char *value, char *dst);
int color_parse_mem(const char *value, int len, char *dst);
/*
--
gitgitgadget
|
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting color config value into a helper method
and use gentle parsing when needed.
This removes error messages when parsing a list of config values that do
not match color formats.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
builtin/config.c | 27 +++++++++++++++++++++------
t/t1300-config.sh | 9 +--------
2 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 71b685d943..e8c02e5f21 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -354,6 +354,24 @@ static int format_config_expiry_date(struct strbuf *buf,
return 0;
}
+static int format_config_color(struct strbuf *buf,
+ const char *key_,
+ const char *value_,
+ int gently)
+{
+ char v[COLOR_MAXLEN];
+
+ if (gently) {
+ if (color_parse_gently(value_, v) < 0)
+ return -1;
+ } else if (git_config_color(v, key_, value_) < 0) {
+ return -1;
+ }
+
+ strbuf_addstr(buf, v);
+ return 0;
+}
+
/*
* Format the configuration key-value pair (`key_`, `value_`) and
* append it into strbuf `buf`. Returns a negative value on failure,
@@ -391,12 +409,9 @@ static int format_config(const struct config_display_options *opts,
res = format_config_path(buf, key_, value_, gently);
else if (opts->type == TYPE_EXPIRY_DATE)
res = format_config_expiry_date(buf, key_, value_, gently);
- else if (opts->type == TYPE_COLOR) {
- char v[COLOR_MAXLEN];
- if (git_config_color(v, key_, value_) < 0)
- return -1;
- strbuf_addstr(buf, v);
- } else if (value_) {
+ else if (opts->type == TYPE_COLOR)
+ res = format_config_color(buf, key_, value_, gently);
+ else if (value_) {
strbuf_addstr(buf, value_);
} else {
/* Just show the key name; back out delimiter */
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index c134d85d8a..79b2ee203c 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2553,17 +2553,10 @@ test_expect_success 'list --type=color shows only canonicalizable color values'
section.blue=<BLUE>
EOF
- cat >expecterr <<-EOF &&
- error: invalid color value: True
- error: invalid color value: 1M
- error: invalid color value: ~/dir
- error: invalid color value: Fri Jun 4 15:46:55 2010
- EOF
-
git config ${mode_prefix}list --type=color >actual.raw 2>err &&
test_decode_color <actual.raw >actual &&
test_cmp expect actual &&
- test_cmp expecterr err
+ test_must_be_empty err
'
test_expect_success '--type rejects unknown specifiers' '
--
gitgitgadget
|
|
"Derrick Stolee via GitGitGadget" wrote on the Git mailing list (how to reply to this email): From: Derrick Stolee <stolee@gmail.com>
The recent changes have replaced the bodies of most if/else-if cases
with simple helper method calls. This makes it easy to adapt the
structure into a clearer switch statement, leaving a simple if/else in
the default case.
Make things a little simpler to read by reducing the nesting depth via a
new goto statement when we want to skip values.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
builtin/config.c | 59 ++++++++++++++++++++++++++++++++----------------
1 file changed, 40 insertions(+), 19 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index e8c02e5f21..1de3ce0eaa 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -393,25 +393,44 @@ static int format_config(const struct config_display_options *opts,
show_config_origin(opts, kvi, buf);
if (opts->show_keys)
strbuf_addstr(buf, key_);
- if (!opts->omit_values) {
- if (opts->show_keys)
- strbuf_addch(buf, opts->key_delim);
-
- if (opts->type == TYPE_INT)
- res = format_config_int64(buf, key_, value_, kvi, gently);
- else if (opts->type == TYPE_BOOL)
- res = format_config_bool(buf, key_, value_, gently);
- else if (opts->type == TYPE_BOOL_OR_INT)
- res = format_config_bool_or_int(buf, key_, value_, kvi, gently);
- else if (opts->type == TYPE_BOOL_OR_STR)
- res = format_config_bool_or_str(buf, value_);
- else if (opts->type == TYPE_PATH)
- res = format_config_path(buf, key_, value_, gently);
- else if (opts->type == TYPE_EXPIRY_DATE)
- res = format_config_expiry_date(buf, key_, value_, gently);
- else if (opts->type == TYPE_COLOR)
- res = format_config_color(buf, key_, value_, gently);
- else if (value_) {
+
+ if (opts->omit_values)
+ goto terminator;
+
+ if (opts->show_keys)
+ strbuf_addch(buf, opts->key_delim);
+
+ switch (opts->type) {
+ case TYPE_INT:
+ res = format_config_int64(buf, key_, value_, kvi, gently);
+ break;
+
+ case TYPE_BOOL:
+ res = format_config_bool(buf, key_, value_, gently);
+ break;
+
+ case TYPE_BOOL_OR_INT:
+ res = format_config_bool_or_int(buf, key_, value_, kvi, gently);
+ break;
+
+ case TYPE_BOOL_OR_STR:
+ res = format_config_bool_or_str(buf, value_);
+ break;
+
+ case TYPE_PATH:
+ res = format_config_path(buf, key_, value_, gently);
+ break;
+
+ case TYPE_EXPIRY_DATE:
+ res = format_config_expiry_date(buf, key_, value_, gently);
+ break;
+
+ case TYPE_COLOR:
+ res = format_config_color(buf, key_, value_, gently);
+ break;
+
+ default:
+ if (value_) {
strbuf_addstr(buf, value_);
} else {
/* Just show the key name; back out delimiter */
@@ -419,6 +438,8 @@ static int format_config(const struct config_display_options *opts,
strbuf_setlen(buf, buf->len - 1);
}
}
+
+terminator:
strbuf_addch(buf, opts->term);
return res;
}
--
gitgitgadget |
| struct strbuf_list { | ||
| struct strbuf *items; | ||
| int nr; | ||
| int alloc; |
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +static int format_config_int64(struct strbuf *buf,
> + const char *key_,
> + const char *value_,
> + const struct key_value_info *kvi,
> + int gently)
> +{
> + int64_t v = 0;
> + if (gently) {
> + if (git_parse_int64(value_, &v))
> + return -1;
> + } else {
> + /* may die() */
> + v = git_config_int64(key_, value_ ? value_ : "", kvi);
> + }
> +
> + strbuf_addf(buf, "%"PRId64, v);
> + return 0;
> +}
This establishes the pattern the next handful of patches follow. We
already have in parse.c helpers that we can use for the gentler
parsing, and otherwise we'd use git_config_*() that the caller of
these new helpers were using originally.
I'd have preferred to have the blank line moved to the gap between
the decl and the first statement, i.e.,
> +{
> + int64_t v = 0;
> +
> + if (gently) {
> + if (git_parse_int64(value_, &v))
> + return -1;
> + } else {
> + /* may die() */
> + v = git_config_int64(key_, value_ ? value_ : "", kvi);
> + }
> + strbuf_addf(buf, "%"PRId64, v);
> + return 0;
> +}
These "format X gently" steps look very good.
|
There was a status update in the "Cooking" section about the branch "git config list" is taught to show the values interpreted for specific type with "--type=<X>" option. Comments? source: <pull.2044.git.1770698579.gitgitgadget@gmail.com> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I started down this road based on feedback on my 'git config-batch' RFC [1].
[1] https://lore.kernel.org/git/pull.2033.git.1770214803.gitgitgadget@gmail.com/
I had described my intention to use 'git config-batch' as a single process to load multiple config values one-by-one. Brian mentioned that 'git config list -z' would probably suffice, so I started experimenting in that direction [2].
[2] git-ecosystem/git-credential-manager@main...derrickstolee:config-list
However, I ran into a problem: the most critical performance bottleneck is related to path-formatted config values that are queried with 'git config get --type=path -z'. It wasn't hard to update things to lazily load the full list of config values by type [3], but I then noticed a big problem!
[3] git-ecosystem/git-credential-manager@d403c8e
Problem: 'git config list' doesn't respect
--type=<X>!This boils down to the fact that the iterator function
show_all_config()doesn't callformat_config(), which includes the type-parsing code.This wasn't super trivial to update:
format_config()usesgit_config_parse_*()methods, whichdie()on a bad parse.format_config().Updates in v2
Based on the positive feedback in round one, this is no longer an RFC.
format_config()now uses a 'gently' parameter instead of 'die_on_parse' (flipped).format_config()is more carefully updated with helper methods and a global refactor.format_config()helper is created to use it.git config list --type=<X>' are added.--typebehavior change is recognized.[4] git-ecosystem/git-credential-manager#2268
Thanks for any and all feedback,
-Stolee
cc: gitster@pobox.com
cc: "brian m. carlson" sandals@crustytoothpaste.net
cc: Phillip Wood phillip.wood123@gmail.com
cc: "Kristoffer Haugsbakk" kristofferhaugsbakk@fastmail.com
cc: Jean-Noël Avila jn.avila@free.fr
cc: Patrick Steinhardt ps@pks.im
cc: "Derrick Stolee via GitGitGadget" gitgitgadget@gmail.com