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 17, 2018, 19:27 UTC
Message-ID: <eeb04c94-50c1-13ee-880b-ea380031a685@gmail.com>
In-Reply-To: <CAM0VKjkT7fBJRie_3f4B13BHT9hp9MxRhuX5r1sogh2x7KQzbg@mail.gmail.com>

On 17/08/18 19:39, SZEDER Gábor wrote:

Show 14 quoted lines

> 
> See, we have quite a few tests that extract repetitive common tasks
> into helper functions, which sometimes includes preparing the expected
> results and running 'test_cmp', e.g. something like this
> (oversimplified) example:
> 
>   check_cmd () {
>         git cmd $1 >actual &&
>         echo "$2" >expect &&
>         test_cmp expect actual
>   }
> 
>   check_cmd --foo    FOO
>   check_cmd --no-foo ""

I've only had time to look into this from t0001 up to t0008-ignores.sh, where test_check_ignore does this. If these helper functions need to allow comparing empty files -- how about adding special variation of cmp functions for cases like this: test_cmp_allow_empty and test_i18ncmp_allow_empty?

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() {
		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 "$@"
	}
	
	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.)