Re: [PATCH 1/2] t/t6300: introduce test_bad_atom()

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

Kousik Sanagavarapu <five231003@gmail.com> writes:

> Introduce a new function "test_bad_atom()", which is similar to
> "test_atom()" but should be used to check whether the correct error
> message is shown on stderr.
>
> Like "test_atom()", the new function takes three arguments. The three
> arguments specify the ref, the format and the expected error message
> respectively, with an optional fourth argument for tweaking
> "test_expect_*" (which is by default "success").
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Hariom Verma <hariom18599@gmail.com>
> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> ---
>  t/t6300-for-each-ref.sh | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 7b943fd34c..15b4622f57 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -267,6 +267,26 @@ test_expect_success 'arguments to %(objectname:short=) must be positive integers
>  	test_must_fail git for-each-ref --format="%(objectname:short=foo)"
>  '
>  
> +test_bad_atom() {

Style: have SP on both sides of "()".

> +	case "$1" in
> +	head) ref=refs/heads/main ;;
> +	 tag) ref=refs/tags/testtag ;;
> +	 sym) ref=refs/heads/sym ;;
> +	   *) ref=$1 ;;
> +	esac

Somehow this indirection makes the two examples we see below harder
to understand.  Why shouldn't we just write the full refname on th
command line of test_bad_atom?  That would make it crystal clear
which ref each test works on.  It does not help that both 'head' and
'sym' refer to a local branch (if the former referred to .git/HEAD
or .git/remotes/origin/HEAD it may have been an excellent choice of
the name, but that is not what is going on).

> +	printf '%s\n' "$3">expect

Style: need SP before (but not after) '>'.

> +	test_expect_${4:-success} $PREREQ "err basic atom: $1 $2" "
> +		test_must_fail git for-each-ref --format='%($2)' $ref 2>actual &&
> +		test_cmp expect actual
> +	"

It is error prone to have the executable part of test_expect_{success,failure}
inside a pair of double quotes and have $variable interpolated
_before_ even the arguments to test_expect_{success,failure} are
formed.  It is much more preferrable to write

	test_bad_atom () {
		ref=$1 format=$2
		printf '%s\n' "$3" >expect
		$test_do=test_expect_${4:-success}

		$test_do $PREREQ "err basic atom: $ref $format" '
			test_must_fail git for-each-ref \
				--format="%($format)" "$ref" 2>error &&
			test_cmp expect error
		'
	}

This is primarily because you cannot control what is in "$2" to
ensure the correctness of the test we see above locally (i.e. if
your caller has a single-quote in "$2", the shell script you create
for running test_expect_{success,failure} would be syntactically
incorrect).  By enclosing the executable part inside a pair of
single quotes, and having the $variables interpolated when that
executable part is `eval`ed when test_expect_{success,failure} runs,
you will avoid such problems, and those reading the above can locally
know that you are aware of and correctly avoiding such problems.

I guess three among four problems I just pointed out you blindly
copied from test_atom.  But let's not spread badness (preliminary
clean-up to existing badness would be welcome instead).

> +}
> +
> +test_bad_atom head 'authoremail:foo' \
> +	'fatal: unrecognized %(authoremail) argument: foo'
> +
> +test_bad_atom tag 'taggeremail:localpart trim' \
> +	'fatal: unrecognized %(taggeremail) argument:  trim'

It is strange to see double SP before 'trim' in this error message.
Are we etching a code mistake in stone here?  Wouldn't the error
message say "...argument: localpart trim" instead, perhaps?

>  test_date () {
>  	f=$1 &&
>  	committer_date=$2 &&