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

[Junio C Hamano <gitster@pobox.com>]

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

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

>  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 &&

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