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

[Kousik Sanagavarapu <five231003@gmail.com>]

Sorry for the late reply.

On Wed, Sep 20, 2023 at 03:56:28PM -0700, Junio C Hamano wrote:
> 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 "()".
> 
> [...]
>
> > +	printf '%s\n' "$3">expect
> 
> Style: need SP before (but not after) '>'.

I'll make these style changes, they slipped by.

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

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

Yeah, I had copied it from test_atom. Although I didn't realize that it
was bad to implement test_bad_atom the way I did. Thanks for such a nice
explanation. So I guess we can include the test_atom cleanup in this
series?

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

So I read the the other replies and it seems that it indeed hides a
breakage and yeah I hadn't tested PATCH 1/2 independently. I'll change
this too.

Thanks