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 |
| *dest = xstrdup(value); | ||
| return 0; | ||
| } | ||
|
|
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. |
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>
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> |
| @@ -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 Fri, Feb 13, 2026 at 11:55:08PM +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. 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.
It might make sense to document the rationale behind this decision in
the commit message.
> 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);
We could probably use puts(3p) instead, but as we know the length of the
data ahead of time it might be more efficient to use fwrite(3p) indeed.
Ultimately I guess it doesn't matter much.
PatrickThere was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
Patrick Steinhardt <ps@pks.im> writes:
>> - 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);
>
> We could probably use puts(3p) instead, but as we know the length of the
> data ahead of time it might be more efficient to use fwrite(3p) indeed.
> Ultimately I guess it doesn't matter much.
>
> Patrick
If we are not always doing LF-delimited output, puts(3) would not
help us very much, I suspect.There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Tue, Feb 17, 2026 at 08:11:40AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> - 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);
> >
> > We could probably use puts(3p) instead, but as we know the length of the
> > data ahead of time it might be more efficient to use fwrite(3p) indeed.
> > Ultimately I guess it doesn't matter much.
> >
> > Patrick
>
> If we are not always doing LF-delimited output, puts(3) would not
> help us very much, I suspect.
D'oh, right.
Patrick
| @@ -266,10 +242,14 @@ struct strbuf_list { | |||
| * append it into strbuf `buf`. Returns a negative value on failure, | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Fri, Feb 13, 2026 at 11:55:07PM +0000, Derrick Stolee via GitGitGadget wrote:
> 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)
I'd propose to either make this a bool, or turn it into an enum flag so
that it becomes easier to see at the callsite what the magic "true" or
"1" means:
enum format_config_flags {
/*
* Do not die in case the value cannot be parsed properly, but
* return an error instead.
*/
FORMAT_CONFIG_GENTLY = (1 << 0),
};
format_config(opts, buf, key, value, kv, FORMAT_CONFIG_GENTLY);
I personally prefer this option over using a bool, even though it's a
bit more verbose.
Patrick
| struct strbuf_list { | ||
| struct strbuf *items; | ||
| int nr; | ||
| int alloc; |
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Fri, Feb 13, 2026 at 11:55:09PM +0000, Derrick Stolee via GitGitGadget wrote:
> 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_,
Why do we have the trailing underscores here?
> @@ -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;
> }
Okay. We bubble up the return value now, but we know that the return
value will only be different in case `gently != 0`. Otherwise, any error
would cause us to die.
PatrickThere was a problem hiding this comment.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
On 2/17/26 4:05 AM, Patrick Steinhardt wrote:
> On Fri, Feb 13, 2026 at 11:55:09PM +0000, Derrick Stolee via GitGitGadget wrote:
>> 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_,
> > Why do we have the trailing underscores here?
This is all to match the existing names from format_config(). This may help to
recognize moved lines by keeping the variable names the same. Definitely not
my preference to use this name format, but I thought it fitting to avoid a
rename of all variables.
Thanks,
-Stolee
| printf("%s%c", key_, opts->term); | ||
|
|
||
| strbuf_addstr(buf, v ? "true" : "false"); | ||
| return 0; |
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Fri, Feb 13, 2026 at 11:55:11PM +0000, Derrick Stolee via GitGitGadget wrote:
> 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_);
This function also returns `1` in case `!value`. Is this intended? I
guess so due to our implicit bool thingy, and `git_config_bool_or_int()`
seems to behave the same.
PatrickThere was a problem hiding this comment.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
On 2/17/26 4:05 AM, Patrick Steinhardt wrote:
> On Fri, Feb 13, 2026 at 11:55:11PM +0000, Derrick Stolee via GitGitGadget wrote:
>> 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_);
> > This function also returns `1` in case `!value`. Is this intended? I
> guess so due to our implicit bool thingy, and `git_config_bool_or_int()`
> seems to behave the same.
Do you mean in the case of a NULL value?
Based on the rules for iterating through config values, deep down in
get_value() the value parameter sent to the function is never NULL.
It may be an empty string, but never NULL.
Thanks,
-Stolee
| strbuf_addstr(buf, value_); | ||
| else | ||
| strbuf_addstr(buf, v ? "true" : "false"); | ||
| return 0; |
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Fri, Feb 13, 2026 at 11:55:14PM +0000, Derrick Stolee via GitGitGadget wrote:
> 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 */
Okay, this is the first callsite where we return a vaule `> 0`, if I see
correctly. But in `show_all_config()` we check for `res >= 0`, and if so
we would print the configuration regardless.
But `buf` will now be an empty string. So wouldn't this cause us to
print such an empty string, too? I'm not quite sure whether this
behaviour is intentional or not, or whether I'm missing something here.
In any case, I think this should be documented in the commit message.
Patrick
| @@ -223,11 +223,6 @@ static int parse_attr(const char *name, size_t len) | |||
| return -1; | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Fri, Feb 13, 2026 at 11:55:16PM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/color.c b/color.c
> index 07ac8c9d40..ec8872d2dd 100644
> --- a/color.c
> +++ b/color.c
> @@ -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
> }
As far as I can see this isn't really about whether or not the function
should be gentle. It's rather whether or not the function should print
an error message when it sees an error.
So should we rename the parameter to `quiet`?
>
> +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);
> +}
And if so, this should probably be called `color_parse_quiet()`.
PatrickThere was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Feb 13, 2026 at 11:55:16PM +0000, Derrick Stolee via GitGitGadget wrote:
>> diff --git a/color.c b/color.c
>> index 07ac8c9d40..ec8872d2dd 100644
>> --- a/color.c
>> +++ b/color.c
>> @@ -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
>> }
>
> As far as I can see this isn't really about whether or not the function
> should be gentle. It's rather whether or not the function should print
> an error message when it sees an error.
Do you mean that this error() call is not die(), the flag does not
fit the usual "gently" criteria? In other words, should we make
this call die() if we call it "gently"?
>
> So should we rename the parameter to `quiet`?
>
>>
>> +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);
>> +}
>
> And if so, this should probably be called `color_parse_quiet()`.
>
> PatrickThere was a problem hiding this comment.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
On 2/17/26 11:20 AM, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> >> On Fri, Feb 13, 2026 at 11:55:16PM +0000, Derrick Stolee via GitGitGadget wrote:
>>> diff --git a/color.c b/color.c
>>> index 07ac8c9d40..ec8872d2dd 100644
>>> --- a/color.c
>>> +++ b/color.c
>>> @@ -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
>>> }
>>
>> As far as I can see this isn't really about whether or not the function
>> should be gentle. It's rather whether or not the function should print
>> an error message when it sees an error.
> > Do you mean that this error() call is not die(), the flag does not
> fit the usual "gently" criteria? In other words, should we make
> this call die() if we call it "gently"?
This is an interesting case where the existing color parsing logic is
not following the typical pattern that uses die() on a failed parse.
If we want to change the behavior to die() later, then that could be
considered, though I don't want to consider the ramifications right now.
I think the easiest "local" fix is to use the 'quiet' way, though it adds
some asymmetry in the config code in how it uses the 'gently' parameter.
Let me give this a try in the next version so we can see how it feels.
Thanks,
-StoleeThere was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
Derrick Stolee <stolee@gmail.com> writes:
>> Do you mean that this error() call is not die(), the flag does not
>> fit the usual "gently" criteria? In other words, should we make
>> this call die() if we call it "gently"?
>
> This is an interesting case where the existing color parsing logic is
> not following the typical pattern that uses die() on a failed parse.
I see. I personally would view that an existing bug worth fixing,
but I ...
> If we want to change the behavior to die() later, then that could be
> considered, though I don't want to consider the ramifications right now.
... agree with you that it should be fixed outside the scope of this
topic.
> I think the easiest "local" fix is to use the 'quiet' way, though it adds
> some asymmetry in the config code in how it uses the 'gently' parameter.
Or, just add comments to the function that takes gently but does not
die() to warn those who would add new callers. They can pass
gently=1 if they want to handle the errors themselves and keep it
that way. If they want the function to die, well they have to wait
until the function is fixed to behave like everybody else.
| if (opts->show_scope) | ||
| show_config_scope(opts, kvi, buf); | ||
| if (opts->show_origin) | ||
| show_config_origin(opts, kvi, buf); |
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Fri, Feb 13, 2026 at 11:55:18PM +0000, Derrick Stolee via GitGitGadget wrote:
> 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) {
I very much prefer this layout. Switches are more verbose, but if you
ask me they are easier to parse.
> + case TYPE_INT:
> + res = format_config_int64(buf, key_, value_, kvi, gently);
> + break;
> +
That being said, I'm not a huge fan of the empty newlines here. But feel
free to ignore.
> + 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:
Should we maybe handle all valid types explicitly and have the `default`
case `BUG()` instead?
Patrick
|
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. Expecting a reroll? cf. <aZQvSvBEebHFf9Bb@pks.im> source: <pull.2044.v2.git.1771026918.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@4161162. |
|
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. Expecting a reroll? cf. <aZQvSvBEebHFf9Bb@pks.im> source: <pull.2044.v2.git.1771026918.gitgitgadget@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. Our intention is that if there is an error in parsing, then the row is not output. This is necessary to avoid the caller needing to build their own validator to understand the difference between valid, canonicalized types and other raw string values. The raw values will always be available to the user if they do not specify the --type=<X> option. The current behavior is more complicated, including error messages on bad parsing or potentially complete failure of the command. We add tests at this point that demonstrate the current behavior so we can witness the fix in future changes that parse these values quietly and gently. 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>
Move the logic for formatting path config values into a helper method and use gentle parsing when needed. We need to be careful about how to handle the ':(optional)' macro, which as tested in t1311-config-optional.sh must allow for ignoring a missing path when other multiple values exist, but cause 'git config get' to fail if it is the only possible value and thus no result is output. In the case of our list, we need to omit those values silently. This necessitates the use of the 'gently' parameter here. Signed-off-by: Derrick Stolee <stolee@gmail.com>
Move the logic for formatting expiry date config values into a helper method and use quiet parsing when needed. Note that git_config_expiry_date() will show an error on a bad parse and not die() like most other git_config...() parsers. Thus, we use 'quietly' here instead of 'gently'. 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_quietly(). This is in contrast to an ..._gently() version because the original does not die(), so both options are technically 'gentle'. To accomplish this, convert the implementation of color_parse_mem() into a static color_parse_mem_1() helper that adds a 'quiet' parameter. The color_parse_quietly() 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 quiet 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>
The --type=<X> option for 'git config' has previously been defined using macros, but using a typed enum is better for tracking the possible values. Move the definition up to make sure it is defined before a macro uses some of its terms. Update the initializer for config_display_options to explicitly set 'type' to TYPE_NONE even though this is implied by a zero value. This assists in knowing that the switch statement added in the previous change has a complete set of cases for a properly-valued enum. Signed-off-by: Derrick Stolee <stolee@gmail.com>
derrickstolee
force-pushed
the
config-list-type
branch
from
48fc882 to
3c32c03
Compare
Author
|
/submit |
|
Submitted as pull.2044.v3.git.1771849615.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
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. Expecting a reroll? cf. <aZQvSvBEebHFf9Bb@pks.im> source: <pull.2044.v2.git.1771026918.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
Updates in V3
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