Re: [PATCH v6 03/22] cat-file tests: test for missing object with -t and -s

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 16, 2021, 22:52 UTC
Message-ID: <87fsu4f729.fsf@evledraar.gmail.com>
In-Reply-To: <YUOhqnj7vwr00Qap@nand.local>

On Thu, Sep 16 2021, Taylor Blau wrote:

Show 15 quoted lines

> On Tue, Sep 07, 2021 at 12:57:58PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> Test for what happens when the -t and -s flags are asked to operate on
>> a missing object, this extends tests added in 3e370f9faf0 (t1006: add
>> tests for git cat-file --allow-unknown-type, 2015-05-03). The -t and
>> -s flags are the only ones that can be combined with
>> --allow-unknown-type, so let's test with and without that flag.
>
> I'm a little skeptical to have tests for all four pairs of `-t` or `-s`
> and "with `--allow-unknown-type` and without `--allow-unknown-type`".
>
> Testing both the presence and absence of `--allow-unknown-type` seems
> useful to me, but I'm not sure what testing `-t` and `-s` separately
> buys us.
>
> (If you really feel the need test both, I'd encourage looping like:

Thanks, I'll try to simplify it.

Show 13 quoted lines

>     for arg in -t -s
>     do
>       test_must_fail git cat-file $arg $missing_oid >out 2>err &&
>       test_must_be_empty out &&
>       test_cmp expect.err err &&
>
>       test_must_fail git cat-file $arg --allow-unknown-type $missing_oid >out 2>err &&
>       test_must_be_empty out &&
>       test_cmp expect.err err
>     done &&
>
> but I would be equally or perhaps even happier to just have one of the
> two tests).

A loop like that can be further simplified as just (just inlining arg=-s):

	test_must_fail git cat-file -s $missing_oid >out 2>err &&
	test_must_be_empty out &&
	test_cmp expect.err err &&

	test_must_fail git cat-file -s --allow-unknown-type $missing_oid >out 2>err &&
	test_must_be_empty out &&
	test_cmp expect.err err

:)

I.e. unless you end &&-chains in loops in the test framework with an || return 1 you're only testing your last iteration. Aside from whatever I'm doing here I generally prefer to either just spell it out twice (if small enough), or:

    for arg in -t -s
    do
        test_expect_success '...' "[... use $arg ...]"
    done

Which both nicely get around the issue of that easy-to-make mistake.

We've got some in-tree tests that are broken this way, well, at least 4cf67869b2a (list-objects.c: don't segfault for missing cmdline objects, 2018-12-05). But I think I'll leave that for a #leftoverbits submission given my outstanding patch queue..., oh there's another one in t1010-mktree.sh ... :)