Re: [PATCH v2] test_cmp: diagnose incorrect arguments

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Oct 15, 2020 at 8:17 PM Jeff King <peff@peff.net> wrote:
> 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'.

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