Re: [PATCH v2 06/13] builtin/config: introduce "list" subcommand

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]

On Tue, Mar 12, 2024 at 10:45:56PM -0400, Eric Sunshine wrote:
> On Mon, Mar 11, 2024 at 7:20 PM Patrick Steinhardt <ps@pks.im> wrote:
> > [...]
> > Introduce the first such new subcommand, which is "git config list". To
> > retain backwards compatibility we only conditionally use subcommands and
> > will fall back to the old syntax in case no subcommand was detected.
> > This should help to transition to the new-style syntax until we
> > eventually deprecate and remove the old-style syntax.
> > [...]
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/builtin/config.c b/builtin/config.c
> > @@ -761,6 +811,22 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> > +       /*
> > +        * This is somewhat hacky: we first parse the command line while
> > +        * keeping all args intact in order to determine whether a subcommand
> > +        * has been specified. If so, we re-parse it a second time, but this
> > +        * time we drop KEEP_ARGV0. This is so that we don't munge the command
> > +        * line in case no subcommand was given, which would otherwise confuse
> > +        * us when parsing the implicit modes.
> > +        */
> > +       argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage,
> 
> Upon reading this, I wasn't quite sure what "when parsing the implicit
> modes" meant. I suppose it is referring to the legacy style command
> invocation?

Yeah, indeed. Let me rephrase it as "when parsing the legacy-style
command modes", which is hopefully a bit clearer.

> > diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> > @@ -11,6 +11,21 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> > +for mode in legacy subcommands
> > +do
> > +
> > +case "$mode" in
> > +legacy)
> > +       mode_prefix="--"
> > +       ;;
> > +subcommands)
> > +       mode_prefix=""
> > +       ;;
> > +*)
> > +       echo "unknown mode $mode" >&2
> > +       exit 1;;
> > +esac
> 
> t/test-lib.sh defines a BUG() function for signaling the sort of
> programmer error handled by the "*" arm of this `case`.

Ah, right, I'll use that.

> An alternative simpler implementation, which wouldn't require any sort
> of programmer-error fallback, would be:
> 
>     for mode_prefix in -- "" # legacy & subcommand modes
>     do
>         test_expect_success '...' '
>             ...
>         '
>         ...
>     done

It works now, but will stop working in subsequent patches. Not all modes
can be translated this easily. E.g. "git config config.key value" needs
to be translated to "git config set config.key value".

> > @@ -527,6 +542,7 @@ test_expect_success 'editing stdin is an error' '
> >  test_expect_success 'refer config from subdirectory' '
> > +       test_when_finished "rm -r x" &&
> >         mkdir x &&
> >         test_cmp_config -C x strasse --file=../other-config --get ein.bahn
> >  '
> 
> Is this an unrelated cleanup?
> 
> > @@ -1072,6 +1088,7 @@ test_expect_success 'inner whitespace kept verbatim' '
> >  test_expect_success SYMLINKS 'symlinked configuration' '
> > +       test_when_finished "rm myconfig" &&
> >         ln -s notyet myconfig &&
> >         git config --file=myconfig test.frotz nitfol &&
> >         test -h myconfig &&
> 
> Ditto.
> 
> Same question regarding similar changes to several later tests.

These are required because we now re-run tests twice. Consequently,
state from the first run may still be around in the second run. The
tests here broke because of that, which I've fixed by cleaning up the
state. I'll clarify this in the commit messages.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]