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

From: Patrick Steinhardt <ps@pks.im>
Date: May 6, 2024, 07:58 UTC
Message-ID: <ZjiNr-PUue2_mJGr@tanuki>
In-Reply-To: <CAOLa=ZSNbZPByO9QyeAGaR1pWXMB7ge_GF7M5fydxP-cse-X3g@mail.gmail.com>

On Fri, May 03, 2024 at 06:08:57AM -0700, Karthik Nayak wrote:

Show 13 quoted lines

> 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]

Show 18 quoted lines

> > @@ -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