Re: [PATCH v2] test_cmp: diagnose incorrect arguments

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 16, 2020, 18:36 UTC
Message-ID: <xmqqzh4maugq.fsf@gitster.c.googlers.com>
In-Reply-To: <CAPig+cSU=1GcQuqZab+0Vff_A-JmD59wEc3RMr3wDojpgRYUuw@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 23 quoted lines

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

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.

Show 7 quoted lines

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