Re: [PATCH 1/2] t6300: unify %(trailers) and %(contents:trailers) tests

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 19, 2020, 17:31 UTC
Message-ID: <xmqq1rk2v8y5.fsf@gitster.c.googlers.com>
In-Reply-To: <bd0bb8d0ef0936866c2a957e5391424a7481a33c.1597841551.git.gitgitgadget@gmail.com>

"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 18 quoted lines

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index a83579fbdf..495848c881 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -776,60 +776,39 @@ test_expect_success 'set up trailers for next test' '
>  '
>  
>  test_expect_success '%(trailers:unfold) unfolds trailers' '
> -	git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
>  	{
>  		unfold <trailers
>  		echo
>  	} >expect &&
> +	git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
> +	test_cmp expect actual &&
> +	git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
>  	test_cmp expect actual
>  '

Hmph, what is this one doing? Ah, OK, trailers:unfold is tested as before (just the steps to prepare 'expect' and 'actual' got swapped), and because the same expectation holds for contents:trailers:unfold, we can test it at the same. Makes sense.

Show 12 quoted lines

>  test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
> -	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
> -	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
> -	test_cmp actual reverse &&
>  	{
>  		grep -v patch.description <trailers | unfold &&
>  		echo
>  	} >expect &&
> +	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
> +	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
> +	test_cmp actual reverse &&
> +	test_cmp expect actual &&

This uses different pattern. It may be cleaner to test one side at a time, as we have prepared the 'expect' that should be the same for both, and compare with the expected pattern one at a time; that would eliminate the need for 'reverse', too. I.e.

	{
		grep -v patch.description trailers | unfold && echo
	} >expect &&
	git for-each-ref ... only,unfold ... >actual &&
	test_cmp expect actual &&
	git for-each-ref ... unfold,only ... >actual &&
	test_cmp expect actual &&

Show 16 quoted lines

> @@ -839,14 +818,7 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
>  	fatal: unknown %(trailers) argument: unsupported
>  	EOF
>  	test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
> -	test_i18ncmp expect actual
> -'
> -
> -test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
> -	# error message cannot be checked under i18n
> -	cat >expect <<-EOF &&
> -	fatal: unknown %(trailers) argument: unsupported
> -	EOF
> +	test_i18ncmp expect actual &&
>  	test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
>  	test_i18ncmp expect actual
>  '

Doesn't this highlight a small bug, where an end-user request for an unknown %(contents:trailers:unsupported) is flagged as an error about %(trailers)? Is it OK because we expect that users who use the longer %(contents:trailers) to know that it is a synonym for %(trailers) and the latter is the official way to write it?

Thanks.