Sorry for the late reply.
On Wed, Sep 20, 2023 at 03:56:28PM -0700, Junio C Hamano wrote:
> 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 "()".
>
> [...]
>
> > + 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
