Re: [PATCH v2] test_cmp: diagnose incorrect arguments

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 16, 2020, 23:14 UTC
Message-ID: <xmqqy2k5931k.fsf@gitster.c.googlers.com>
In-Reply-To: <xmqqv9f9ao0v.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 25 quoted lines

>  test_cmp() {
> -	test $# -eq 2 || BUG "test_cmp requires two arguments"
> -	if ! eval "$GIT_TEST_CMP" '"$@"'
> -	then
> -		test "x$1" = x- || test -e "$1" || BUG "test_cmp '$1' missing"
> -		test "x$2" = x- || test -e "$2" || BUG "test_cmp '$2' missing"
> -		return 1
> -	fi
> +	eval "$GIT_TEST_CMP" '"$@"'
>  }
>  
>  # Check that the given config key has the expected value.
> @@ -940,13 +934,7 @@ test_cmp_config() {
>  # test_cmp_bin - helper to compare binary files
>  
>  test_cmp_bin() {
> -	test $# -eq 2 || BUG "test_cmp_bin requires two arguments"
> -	if ! cmp "$@"
> -	then
> -		test "x$1" = x- || test -e "$1" || BUG "test_cmp_bin '$1' missing"
> -		test "x$2" = x- || test -e "$2" || BUG "test_cmp_bin '$2' missing"
> -		return 1
> -	fi
> +	cmp "$@"
>  }

Looking at this again, I think we could keep the "we should have two arguments, no more than, no less than, but exactly two". But I think those who write new tests are working to eventually make them pass, so hopefully they'll notice and investigate test_cmp that yields false anyway, I guess.