Re: [PATCH v2] test_cmp: diagnose incorrect arguments

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 16, 2020, 02:18 UTC
Message-ID: <CAPig+cSU=1GcQuqZab+0Vff_A-JmD59wEc3RMr3wDojpgRYUuw@mail.gmail.com>
In-Reply-To: <20201016001704.GA2937048@coredump.intra.peff.net>

On Thu, Oct 15, 2020 at 8:17 PM Jeff King <peff@peff.net> wrote:

Show 12 quoted lines

> On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:
> > [...] Make it easier for test authors to discover such problems early
> > by sanity-checking the arguments to test_cmp(). [...]
>
> This patch caused some interesting confusion for me today.
>   error: bug in the test script: test_cmp 'r2/.git/HEAD' missing
> which was somewhat unhelpful (or at least less helpful than a regular
> test failure). The test in question does this:
>         test_cmp r0/.git/HEAD r2/.git/HEAD &&
> and expects to fail if an earlier step didn't correctly create r2. Is it
> a bug or misuse of test_cmp for it to do so? I could see an argument
> that it is, but I'm also not sure if there's a convenient alternative.

I can see the argument going both ways as to whether it's a misuse of 'test_cmp'.

Show 6 quoted lines

> The best I could come up with is:
>
>   test_path_is_file r2/.git/HEAD &&
>   test_cmp r0/.git/HEAD r2/.git/HEAD
>
> which isn't that great.

Ævar ran into the same issue recently[1] and came up with the same workaround. Despite its good intention (trying to catch bugs in 'test_expect_failure' tests), this change[2] doesn't seem to have caught any genuine bugs (it wouldn't even have caught the bug which served as its inspiration[3]), but has nevertheless caused a couple hiccups already. As such, I would not be opposed to seeing the change reverted.

[1]: https://lore.kernel.org/git/20200921104000.2304-15-avarab@gmail.com/ [2]: https://lore.kernel.org/git/20200809060810.31370-1-sunshine@sunshineco.com/ [3]: https://lore.kernel.org/git/7f408b7d4069403b969d334f4940ebf87f1dc797.1596906081.git.gitgitgadget@gmail.com/