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

From: Patrick Steinhardt <[email-masked]>
Date: Mon, 6 May 2024 09:58:39 +0200

On Fri, May 03, 2024 at 06:08:57AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <[email-masked]> 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