Kousik Sanagavarapu <five231003@xxxxxxxxx> 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@xxxxxxxxx>
> Mentored-by: Hariom Verma <hariom18599@xxxxxxxxx>
> Signed-off-by: Kousik Sanagavarapu <five231003@xxxxxxxxx>
> ---
> 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 &&
