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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 13, 2024, 02:45 UTC
Message-ID: <CAPig+cTybDAqZeGXPi+hi5zpPD=7nGpknzM6MForaaff4H4htA@mail.gmail.com>
In-Reply-To: <53401299fa1f51954834e2507a2282cf60b02f20.1710198711.git.ps@pks.im>

On Mon, Mar 11, 2024 at 7:20 PM Patrick Steinhardt <ps@pks.im> wrote:

Show 20 quoted lines

> [...]
> 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?

Show 16 quoted lines

> 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`.

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

Show 6 quoted lines

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

Show 6 quoted lines

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