Re: [PATCH v2] test_cmp: diagnose incorrect arguments

Eric Sunshine [off-list ref] writes:

On Thu, Oct 15, 2020 at 8:17 PM Jeff King [off-list ref] wrote:

quoted

On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:

quoted

[...] 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'.

quoted

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.

Hmph, I agree that the "both must be file" is a bit too eager and
ignores that "they must match, but the possible reasons they may not
include one of them may be missing" use case.

Æ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.

Sounds good.  Anybody wants to do the honors?