From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 12 Feb 2026 06:39:06 GMT
Subject: Re: [PATCH 5/5] config: make 'git config list --type=<X>' work
Message-ID: <aY11ikm_sgDlneV7@pks.im>
In-Reply-To: <1fb94c08-c36a-445b-b613-dda33c238d6e@gmail.com>

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