Re: [PATCH 01/10] cat-file tests: test bad usage

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 7, 2021, 01:07 UTC
Message-ID: <CAPig+cSuSaZsTh8_FBHNpT1LKGWWFPd-bkjvfF=0fZDQaDiqvQ@mail.gmail.com>
In-Reply-To: <patch-01.10-c8040da8e55-20211106T214259Z-avarab@gmail.com>

On Sat, Nov 6, 2021 at 5:47 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 15 quoted lines

> Stress test the usage emitted when options are combined in ways that
> isn't supported. Let's test various option combinations, some of these
> we buggily allow right now.
>
> E.g. this reveals a bug in 321459439e1 (cat-file: support
> --textconv/--filters in batch mode, 2016-09-09) that we'll fix in a
> subsequent commit. We're supposed to be emitting a relevant message
> when --batch-all-objects is combined with --textconv or --filters, but
> we don't.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> @@ -4,6 +4,96 @@ test_description='git cat-file'
> +test_cmdmode_usage() {

Style nit: add space before ()

Show 11 quoted lines

> +       test_expect_code 129 "$@" 2>err &&
> +       grep "^error:.*is incompatible with" err
> +}
> +
> +test_expect_success 'usage: cmdmode' '
> +       test_cmdmode_usage git cat-file -e -p &&
> +       test_cmdmode_usage git cat-file -p -t &&
> +       test_cmdmode_usage git cat-file -t -s &&
> +       test_cmdmode_usage git cat-file -s --textconv &&
> +       test_cmdmode_usage git cat-file --textconv --filters
> +'

A minor observation: I usually avoid combining tests into a conglomerate since it makes it harder to discover at a glance the problematic test if one does start failing. I'd probably have used a separate test_expect_success() invocation for each allowed switch combination (in other words, five distinct tests instead of all five cases stuffed into a single test). Not a big deal.

> +test_incompatible_usage() {

Style nit: add space before ()

> +       test_expect_code 129 "$@" 2>err &&
> +       grep -E "^error:.*$switch.*needs" err
> +}

What is `$switch`? There doesn't seem to be any such variable defined which means the regex is really:

    ^error:.*.*needs

thus matches "by accident".

Show 20 quoted lines

> +for opt in $short_modes
> +do
> +       test_expect_success "usage: $opt requires another option" '
> +               test_expect_code 129 git cat-file $opt
> +       '
> +
> +       for opt2 in --batch \
> +               --batch-check \
> +               --follow-symlinks
> +       do
> +               test_expect_failure "usage: incompatible options: $opt and $opt2" '
> +                       test_incompatible_usage git cat-file $opt $opt2
> +               '
> +       done
> +
> +       opt2="--path=foo HEAD:some-path.txt"
> +       test_expect_success "usage: incompatible options: $opt and $opt2" '
> +               test_incompatible_usage git cat-file $opt $opt2
> +       '
> +done

So, the only reason the final `opt2` is not part of the for-loop:

    for opt2 in --batch \
        --batch-check \
        --follow-symlinks \
        "--path=foo HEAD:some-path.txt"

is that it succeeds but the others fail?

Show 14 quoted lines

> +for opt in --buffer \
> +       --follow-symlinks \
> +       --batch-all-objects
> +do
> +       status=success
> +       if test $opt = "--buffer"
> +       then
> +               status=failure
> +       fi
> +       test_expect_$status "usage: bad option combination: $opt without batch mode" '
> +               test_expect_code 129 git cat-file $opt &&
> +               test_expect_code 129 git cat-file $opt commit HEAD
> +       '
> +done

In this case, `status` differentiates between success and failure...