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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 21, 2020, 07:07 UTC
Message-ID: <CAPig+cSaq4vTK7CtvxB2bd0=WTW+d=s0H2RMquyCEf+q0YVn2w@mail.gmail.com>
In-Reply-To: <733c674bd1901c931a8917045eb72f661872f462.1608516320.git.gitgitgadget@gmail.com>

On Sun, Dec 20, 2020 at 9:05 PM Nipunn Koorapati via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 14 quoted lines

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

Show 9 quoted lines

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