Re: [PATCH v3 1/3] test-lib-functions: handle --add in test_config

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Dec 20, 2020 at 9:05 PM Nipunn Koorapati via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> test_config fails to unset the configuration variable when
> using --add, as it tries to run git config --unset-all --add
>
> Tell test_config to invoke test_unconfig with the arg $2 when
> the arg $1 is --add
>
> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -381,6 +381,7 @@ test_unconfig () {
>                 config_dir=$1
>                 shift
>         fi
> +       echo git ${config_dir:+-C "$config_dir"} config --unset-all "$@"

Stray debugging gunk?

> @@ -400,7 +401,13 @@ test_config () {
> -       test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$1'" &&
> +
> +       first_arg=$1
> +       if test "$1" = --add; then
> +               first_arg=$2
> +       fi
> +
> +       test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$first_arg'" &&

Several comments...

Style on this project is to place `then` on its own line (as seen a
few lines above this change):

    if test "$1" = --add
    then
        ...

This logic would be easier to understand if the variable was named
`varname` or `cfgvar` (or something), which better conveys intention,
rather than `first_arg`.

It feels odd to single out `--add` when there are other similar
options, such as `--replace-all`, `--fixed-value`, or even `--type`
which people might try using in the future.

This new option parsing is somewhat brittle. If a caller uses
`test_config --add -C <dir> ...`, it won't work as expected. Perhaps
that's not likely to happen, but it would be easy enough to fix by
unifying and generalizing option parsing a bit. Doing so would also
make it easy for the other options mentioned above to be added later
if ever needed. For instance:

    options=
    while test $# != 0
    do
        case "$1" in
        -C)
            config_dir=$2
            shift
            ;;
        --add)
            options="$options $1"
            ;;
        *)
            break
            ;;
        esac
        shift
    done

Finally, as this is a one-off case, it might be simpler just to drop
this patch altogether and open-code the cleanup in the test itself in
patch [2/3] rather than bothering with test_config() in that
particular case. For example:

    test_when_finished "test_unconfig -C two remote.one.push" &&
    git config -C two --add remote.one.push : &&
    test_must_fail git -C two push one &&
    git config -C two --add remote.one.push ^refs/heads/master &&
    git -C two push one