Re: [PATCH v4 07/14] builtin/config: introduce "list" subcommand

> On Fri, May 03, 2024 at 06:08:57AM -0700, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> > While git-config(1) has several modes, those modes are not exposed with
>> > subcommands but instead by specifying e.g. `--unset` or `--list`. This
>>
>> s/specifying/specifying flags/ perhaps?
>>
>> > user interface is not really in line with how our more modern commands
>> > work, where it is a lot more customary to say e.g. `git remote list`.
>>
>> Tangent: I totally agree with the patch, but it would be nice to have a
>> 'DesigningCommands' document which would highlight UX do's and don'ts.
>> It would be nice to add that as reference in discussions.
>
> I agree that we should have that discussion, but feel like it should be
> part of a separate patch series. If I were to adopt that into this
> series I very much think that the resulting discussions would take quite
> a while to settle.
>
> I'll start the discussion soonish in a separate thread.
>

> [snip]
>> > @@ -2590,7 +2608,7 @@ test_expect_success '--fixed-value uses exact string matching' '
>> >  	test_cmp expect actual &&
>> >
>> >  	git config --file=config --fixed-value --replace-all fixed.test bogus "$META" &&
>> > -	git config --file=config --list >actual &&
>> > +	git config ${mode_prefix}list --file=config >actual &&
>> >  	cat >expect <<-EOF &&
>> >  	fixed.test=bogus
>> >  	fixed.test=bogus
>> > @@ -2751,4 +2769,6 @@ test_expect_success 'specifying multiple modes causes failure' '
>> >  	test_cmp expect err
>> >  '
>> >
>> > +done
>> > +
>> >
>> Nit: Wouldn't it be better if the tests are indented here? That way you
>> know it's part of a loop.
>
> We would basically have to reindent 2500 lines of code. I don't think
> that'd be a helpful to reviewers :)
>
> Patrick