| Message ID | 20230920191654.6133-2-five231003@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add mailmap support to ref-filter | expand |
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 &&
Junio C Hamano <gitster@pobox.com> writes: >> + 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. ... It does not help that both 'head' and > 'sym' refer to a local branch ... Ah, this "sym" thing is a (rather unnatural) symbolic ref inside refs/heads/ hierarchy, so the naming makes halfway sense. As it is also used in test_atom, I no longer find it all that much disturbing. Everything else I said in my review still stands, I would think. Thanks.
Junio C Hamano <gitster@pobox.com> writes: >> +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? I tried. The fatal message does say ...argument: localpart trim" as I suspected, when you ask for 'taggeremail:localpart trim'. I think I know what is going on. With the [PATCH 1/2] as-is, this piece does not pass. But because the error message from parsing gets broken by [PATCH 2/2], after applying [PATCH 2/2], the error message will become what the above test expects, hiding the new breakage in the code. And it probably was not noticed before you sent the patches, because you did not test [PATCH 1/2] alone.
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
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() { + case "$1" in + head) ref=refs/heads/main ;; + tag) ref=refs/tags/testtag ;; + sym) ref=refs/heads/sym ;; + *) ref=$1 ;; + esac + printf '%s\n' "$3">expect + 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 + " +} + +test_bad_atom head 'authoremail:foo' \ + 'fatal: unrecognized %(authoremail) argument: foo' + +test_bad_atom tag 'taggeremail:localpart trim' \ + 'fatal: unrecognized %(taggeremail) argument: trim' + test_date () { f=$1 && committer_date=$2 &&
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(+)