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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 17, 2018, 20:09 UTC
Message-ID: <xmqq4lfsbvru.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <eeb04c94-50c1-13ee-880b-ea380031a685@gmail.com>

Andrei Rybak <rybak.a.v@gmail.com> writes:

Show 8 quoted lines

> I think it would be a good trade-off to allow these helper functions to skip
> checking emptiness of arguments for test_cmp. Such patch will require only
> s/test_cmp/&_allow_empty/ for these helper functions and it will help catch
> cases as bogus test in t5310.
>
> I'll try something like the following on the weekend:
>
> 	test_cmp() {

Style: SP before and after ().

Show 12 quoted lines

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

	: >expect &&
	git cmd >actual &&
	test_cmp expect actual

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

It's the same story, and it is even worse than the above, when we are expecting some output but the command produced no output, i.e.

	echo Everything up-to-date. >expect &&
	git cmd >actual &&
	test_cmp expect actual

We should get a complaint from test_cmp that 'actual' does not match the string we were expecting (and even better, we show how they are different by running them thru "diff -u"), not that 'actual' is an empty file.

Show 6 quoted lines

> 	test_cmp_allow_empty() {
> 		$GIT_TEST_CMP "$@"
> 	}
>
> (I'm not sure about redirections in test lib functions. The two if's would
> probably be in a separate function to be re-used by test_i18ncmp.)