Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

From: Andrei Rybak <rybak.a.v@gmail.com>
Date: Aug 19, 2018, 17:50 UTC
Message-ID: <73346b91-6d19-651a-c361-1666a39681f0@gmail.com>
In-Reply-To: <xmqq4lfsbvru.fsf@gitster-ct.c.googlers.com>

On 17/08/18 22:09, Junio C Hamano wrote:

Show 27 quoted lines

> Andrei Rybak <rybak.a.v@gmail.com> writes:
>>
>> I'll try something like the following on the weekend:
>>
>> 	test_cmp () {
>> 		if test "$1" != - && ! test -s "$1"
>> 		then
>> 			echo >&4 "error: trying to compare empty file '$1'"
>> 			return 1
>> 		fi
>> 		if test "$2" != - && ! test -s "$2"
>> 		then
>> 			echo >&4 "error: trying to compare empty file '$2'"
>> 			return 1
>> 		fi
>> 		test_cmp_allow_empty "$@"
>> 	}
> 
> I actually think the above gives way too confusing output, when the
> actual output is empty and we are expecting some output.
> 
> The tester wants to hear from test_cmp "your 'git cmd' produced some
> output when we are expecting none" as the primary message.  We are
> trying to find bugs in "git" under development, and diagnosing iffy
> tests is secondary.  But with your change, the first thing that is
> checked is if 'expect' is an empty file and that is what we get
> complaints about, without even looking at what is in 'actual'.

I came up with two solutions for this issue:

  1. Check both files at the same time (combination with Gábor's
     function):

	test_cmp () {
		if test "$1" != - &&
		   test "$2" != - &&
		   ! test -s "$1" && 
		   ! test -s "$2"
		then
			error "bug in test script: using test_cmp to check empty file; use test_must_be_empty instead"
		fi
		test_cmp_allow_empty "$@"
	}

     This will still be reporting to the developer clearly, but
     will only catch cases exactly like the bogus test in t5310.

  2. Enable this check via variable, smth like EMPTY_CMP_LINT=1