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

[Andrei Rybak <rybak.a.v@gmail.com>]

On 19/08/18 22:32, Jeff King wrote:
> On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:
> 
>>   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.
> 
> Doesn't that have the opposite issue? If we expect non-empty output but
> the command produces empty output, we'd say "bug in the test script".
> But that is not true at all; it's a failed test.

No. Only when both "$1" and "$2" are empty files will the function above
report "bug in test script". From patch's commit message:

  ... both invocations produce empty 'pack{a,b}.objects' files, and the
  subsequent 'test_cmp' happily finds those two empty files identical.

That's what I meant by "will only catch cases exactly like the bogus
test in t5310".

However ...

> If we assume that "expect" is first (which is our convention but not
> necessarily guaranteed), then I think the best logic is something like:
> 
>   if $1 is empty; then
>     bug in the test script
>   elif test_cmp_allow_empty "$@"
>     test failure
> 
> We do not need to check $2 at all. An empty one is either irrelevant (if
> the expectation is empty), or a test failure (because it would not match
> the non-empty $1).

... this is indeed a better solution. I written out the cases for
updated test_cmp to straighten out my thinking:

 * both $1 and $2 are empty:
     bogus test:
       needs either fixing generation of both expect and actual
       or switching to test_must_be_empty
   OR
     bogus helper function, as Gábor described above:
       needs to switch to test_cmp_allow_empty
 
 * $1 is non-empty && $2 is empty
   proceeding with test
   test failure from GIT_TEST_CMP
 
 * $1 is empty && $2 is non-empty
   bogus test - needs either switching to test_must_be_empty
   (and after that test_must_be_empty will report failure)
   or fixing generation of expect (and after that test result
   depends on contents).
 
 * both $1 and $2 are non-empty
   proceeding with test
   result depends on contents