[GSoC] [PATCH 1/1] t9811: avoid using pipes

[GSoC] [PATCH 1/1] t9811: avoid using pipes

[Anthony Wang]

The exit code of the upstream in a pipe is suppressed thus we lose any
exit codes of git commands that are piped. In order to ensure we
pick up the exit code, we can write the output of the git command to
a file, testing the exit codes of both the commands.

Signed-off-by: Anthony Wang <anthonywang513@gmail.com>
---
 t/t9811-git-p4-label-import.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 5ac5383fb7..5abac938d0 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '
 		cd "$git" &&
 		git p4 sync --import-labels &&
 
-		git tag | grep TAG_F1 &&
-		git tag | grep -q TAG_F1_1 &&
-		git tag | grep -q TAG_F1_2 &&
+		git tag >output &&
+		grep TAG_F1 output &&
+		grep -q TAG_F1_1 output &&
+		grep -q TAG_F1_2 output &&
 
 		cd main &&
 
@@ -208,7 +209,8 @@ test_expect_success 'use git config to enable import/export of tags' '
 		git p4 rebase --verbose &&
 		git p4 submit --verbose &&
 		git tag &&
-		git tag | grep TAG_F1_1
+		git tag >output &&
+		grep TAG_F1_1 output
 	) &&
 	(
 		cd "$cli" &&
-- 
2.39.5 (Apple Git-154)

Re: [GSoC] [PATCH 1/1] t9811: avoid using pipes

[Patrick Steinhardt]

On Sat, Apr 05, 2025 at 12:37:18PM +0200, Anthony Wang wrote:
> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
> index 5ac5383fb7..5abac938d0 100755
> --- a/t/t9811-git-p4-label-import.sh
> +++ b/t/t9811-git-p4-label-import.sh
> @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '
>  		cd "$git" &&
>  		git p4 sync --import-labels &&
>  
> -		git tag | grep TAG_F1 &&
> -		git tag | grep -q TAG_F1_1 &&
> -		git tag | grep -q TAG_F1_2 &&
> +		git tag >output &&
> +		grep TAG_F1 output &&
> +		grep -q TAG_F1_1 output &&
> +		grep -q TAG_F1_2 output &&
>  
>  		cd main &&
>  

I was a bit puzzled why we use `grep` for the first invocation, but
`grep -q` for the other ones. It made me double check that there is no
surrounding shell that expects to read the output generated by that
line, but as far as I can see that is not the case. We might want to
drop them to avoid such confusion (and explain in the commit message why
we want to drop it).

While at it, you can also adapt all adapted calls of `grep` to instead
use `test_grep`. The benefit of that function is that it will provide
helpful debug output in case the test ever was to fail by printing
contents of the non-matching file.

Other than that the patch looks good to me, thanks!

Patrick

[GSoC] [PATCH v2 0/3] t9811: Improve test coverage and clarity

[Anthony Wang]

In order to expose more testing outputs, we remove the piping of `git tag` 
outputs in order to expose the exit codes. In addition, we remove `-q` 
tags on instances of `grep` to ensure clarity. We also replace `grep` 
with `test_grep` to provide helpful debug output in case of test failure.

changes in v2:
--------------

- patch #2 and #3 have been added to reduce confusion caused by the 
  implications of `grep` followed by `grep -q`, and increase debug 
  output.
  

 t/t9811-git-p4-label-import.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
2.39.5 (Apple Git-154)

[GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes

[Anthony Wang]

The exit code of the upstream in a pipe is suppressed
thus we lose any exit codes of git commands that are piped. In order to
ensure we pick up the exit code, we can write the output of the git command
to a file, testing the exit codes of both the commands.

---
 t/t9811-git-p4-label-import.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 5ac5383fb7..5abac938d0 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '
 		cd "$git" &&
 		git p4 sync --import-labels &&
 
-		git tag | grep TAG_F1 &&
-		git tag | grep -q TAG_F1_1 &&
-		git tag | grep -q TAG_F1_2 &&
+		git tag >output &&
+		grep TAG_F1 output &&
+		grep -q TAG_F1_1 output &&
+		grep -q TAG_F1_2 output &&
 
 		cd main &&
 
@@ -208,7 +209,8 @@ test_expect_success 'use git config to enable import/export of tags' '
 		git p4 rebase --verbose &&
 		git p4 submit --verbose &&
 		git tag &&
-		git tag | grep TAG_F1_1
+		git tag >output &&
+		grep TAG_F1_1 output
 	) &&
 	(
 		cd "$cli" &&
-- 
2.39.5 (Apple Git-154)

[GSoC] [PATCH v2 2/3] t9811: Remove the -q quiet mode from some instances of grep

[Anthony Wang]

Remove the `-q` quiet mode from some instances of `grep`,
as the lack of `-q` on the `grep` on line 99 implies that its output is
required, when that is not the case. This change ensures consistency and
avoids confusion about whether the output of `grep` is used.

---
 t/t9811-git-p4-label-import.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 5abac938d0..e69dae55dc 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -97,8 +97,8 @@ test_expect_success 'two labels on the same changelist' '
 
 		git tag >output &&
 		grep TAG_F1 output &&
-		grep -q TAG_F1_1 output &&
-		grep -q TAG_F1_2 output &&
+		grep TAG_F1_1 output &&
+		grep TAG_F1_2 output &&
 
 		cd main &&
 
-- 
2.39.5 (Apple Git-154)

[GSoC] [PATCH v2 3/3] t9811: Change `grep` to `test_grep` for debug output

[Anthony Wang]

Change `grep` to `test_grep` to provide helpful debug
output in case of test failure by printing contents of the non-matching file.

Signed-off-by: Anthony Wang <anthonywang513@gmail.com>
---
 t/t9811-git-p4-label-import.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index e69dae55dc..e9c2aad2aa 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -96,9 +96,9 @@ test_expect_success 'two labels on the same changelist' '
 		git p4 sync --import-labels &&
 
 		git tag >output &&
-		grep TAG_F1 output &&
-		grep TAG_F1_1 output &&
-		grep TAG_F1_2 output &&
+		test_grep TAG_F1 output &&
+		test_grep TAG_F1_1 output &&
+		test_grep TAG_F1_2 output &&
 
 		cd main &&
 
-- 
2.39.5 (Apple Git-154)

Re: [GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes

[Eric Sunshine]

On Mon, Apr 7, 2025 at 7:18 AM Anthony Wang <anthonywang513@gmail.com> wrote:
> The exit code of the upstream in a pipe is suppressed
> thus we lose any exit codes of git commands that are piped. In order to
> ensure we pick up the exit code, we can write the output of the git command
> to a file, testing the exit codes of both the commands.
> ---

Missing sign-off.

> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
> @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '
> -               git tag | grep TAG_F1 &&
> -               git tag | grep -q TAG_F1_1 &&
> -               git tag | grep -q TAG_F1_2 &&
> +               git tag >output &&
> +               grep TAG_F1 output &&
> +               grep -q TAG_F1_1 output &&
> +               grep -q TAG_F1_2 output &&

Since process creation is so expensive on Microsoft Windows, folks on
that platform should also appreciate that this eliminates two git-tag
invocations. Nice.

Re: [GSoC] [PATCH v2 2/3] t9811: Remove the -q quiet mode from some instances of grep

[Eric Sunshine]

On Mon, Apr 7, 2025 at 7:18 AM Anthony Wang <anthonywang513@gmail.com> wrote:
> Remove the `-q` quiet mode from some instances of `grep`,
> as the lack of `-q` on the `grep` on line 99 implies that its output is
> required, when that is not the case. This change ensures consistency and
> avoids confusion about whether the output of `grep` is used.
> ---

Missing sign-off.

Rather than mentioning line 99 explicitly, it probably would be more
helpful to readers to say instead:

    ... the lack of `-q` on the "TAG_F1" `grep` implies...

> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
> @@ -97,8 +97,8 @@ test_expect_success 'two labels on the same changelist' '
>                 git tag >output &&
>                 grep TAG_F1 output &&
> -               grep -q TAG_F1_1 output &&
> -               grep -q TAG_F1_2 output &&
> +               grep TAG_F1_1 output &&
> +               grep TAG_F1_2 output &&

For such a simple change, it probably would be more common on this
project to combine patches [2/3] (which drops `-q`) and [3/3] (which
replaces `grep` with `test_grep`), and to simply explain as a
side-note in the commit message of the combined patch why `-q` is
being dropped. However, it's also fairly subjective, and others may
feel differently, so keeping the changes as separate patches is also
valid.

Re: [GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes

[Junio C Hamano]

Anthony Wang <anthonywang513@gmail.com> writes:

> The exit code of the upstream in a pipe is suppressed
> thus we lose any exit codes of git commands that are piped. In order to
> ensure we pick up the exit code, we can write the output of the git command
> to a file, testing the exit codes of both the commands.

Sort of correct, but ...

> ---
>  t/t9811-git-p4-label-import.sh | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Missing sign-off.

> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
> index 5ac5383fb7..5abac938d0 100755
> --- a/t/t9811-git-p4-label-import.sh
> +++ b/t/t9811-git-p4-label-import.sh
> @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '
>  		cd "$git" &&
>  		git p4 sync --import-labels &&
>  
> -		git tag | grep TAG_F1 &&
> -		git tag | grep -q TAG_F1_1 &&
> -		git tag | grep -q TAG_F1_2 &&
> +		git tag >output &&
> +		grep TAG_F1 output &&
> +		grep -q TAG_F1_1 output &&
> +		grep -q TAG_F1_2 output &&

Think what these tests are trying to do.  After a "git p4 sync"
operation, they want to ensure that tags TAG_F1_1 and TAG_F1_2
exist?  Does the test want to see a tag "TAG_F1", or is it only that
the test is written in a so sloppy way that grepping for TAG_F1 will
be happy when any one of TAG_F1_1, TAG_F1_2 and TAG_F1_ONLY exists,
making its purpose of verifying that the tags are in the expected
state pretty much useless, and that is the reason why it needs to be
followed up with the two extra tests?

What is the desired state you want to ensure after "git p4 sync"
operation above?  I do not do "git p4", but you may know better than
I do, as you are the one who is patching this test file ;-)  I am
guessing that you want TAG_F1_1 and TAG_F1_2 to exist and you do not
want TAG_F1_ONLY to exist?

If so, instead of grepping around, we should be testing that in a
more direct way, perhaps with something like

	git show-ref --verify refs/tags/TAG_F1_1 &&
	git show-ref --verify refs/tags/TAG_F1_2 &&
	test_must_fail 	git show-ref --verify refs/tags/TAG_F1_ONLY &&

no?

>  		cd main &&
>  
> @@ -208,7 +209,8 @@ test_expect_success 'use git config to enable import/export of tags' '
>  		git p4 rebase --verbose &&
>  		git p4 submit --verbose &&
>  		git tag &&
> -		git tag | grep TAG_F1_1
> +		git tag >output &&
> +		grep TAG_F1_1 output
>  	) &&
>  	(
>  		cd "$cli" &&

[GSoC] [PATCH v3 0/3] t9811: Improve test coverage and clarity

[Anthony Wang]

In order to expose more testing outputs, we remove the piping of `git tag` 
outputs in order to expose the exit codes. In addition, we remove `-q` 
tags on instances of `grep` to ensure clarity. We also replace `grep` 
with `test_grep` to provide helpful debug output in case of test failure.

--------------

changes in v3:
- patch #1 and #2 were missing my sign-off, which has now been added.
- patch #2 referenced a line number, which was not informative. A new 
  discription has been added referencing the context of the code.

changes in v2:
- patch #2 and #3 have been added to reduce confusion caused by the 
  implications of `grep` followed by `grep -q`, and increase debug 
  output.


 t/t9811-git-p4-label-import.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
2.39.5 (Apple Git-154)

[GSoC] [PATCH v3 1/3] t9811: avoid using pipes to expose exit codes

[Anthony Wang]

The exit code of the upstream in a pipe is suppressed
thus we lose any exit codes of git commands that are piped. In order to
ensure we pick up the exit code, we can write the output of the git command
to a file, testing the exit codes of both the commands.

Signed-off-by: Anthony Wang <anthonywang513@gmail.com>
---
 t/t9811-git-p4-label-import.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 5ac5383fb7..5abac938d0 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '
 		cd "$git" &&
 		git p4 sync --import-labels &&
 
-		git tag | grep TAG_F1 &&
-		git tag | grep -q TAG_F1_1 &&
-		git tag | grep -q TAG_F1_2 &&
+		git tag >output &&
+		grep TAG_F1 output &&
+		grep -q TAG_F1_1 output &&
+		grep -q TAG_F1_2 output &&
 
 		cd main &&
 
@@ -208,7 +209,8 @@ test_expect_success 'use git config to enable import/export of tags' '
 		git p4 rebase --verbose &&
 		git p4 submit --verbose &&
 		git tag &&
-		git tag | grep TAG_F1_1
+		git tag >output &&
+		grep TAG_F1_1 output
 	) &&
 	(
 		cd "$cli" &&
-- 
2.39.5 (Apple Git-154)

[GSoC] [PATCH v3 2/3] t9811: Remove the -q quiet mode from some instances of grep

[Anthony Wang]

Remove the `-q` quiet mode from some instances of `grep`,
as the lack of `-q` on the "TAG_F1" `grep` implies that its output is
required, when that is not the case. This change ensures consistency and
avoids confusion about whether the output of `grep` is used.

Signed-off-by: Anthony Wang <anthonywang513@gmail.com>
---
 t/t9811-git-p4-label-import.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 5abac938d0..e69dae55dc 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -97,8 +97,8 @@ test_expect_success 'two labels on the same changelist' '
 
 		git tag >output &&
 		grep TAG_F1 output &&
-		grep -q TAG_F1_1 output &&
-		grep -q TAG_F1_2 output &&
+		grep TAG_F1_1 output &&
+		grep TAG_F1_2 output &&
 
 		cd main &&
 
-- 
2.39.5 (Apple Git-154)

[GSoC] [PATCH v3 3/3] t9811: Change `grep` to `test_grep` for debug output

[Anthony Wang]

Change `grep` to `test_grep` to provide helpful debug
output in case of test failure by printing contents of the non-matching file.

Signed-off-by: Anthony Wang <anthonywang513@gmail.com>
---
 t/t9811-git-p4-label-import.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index e69dae55dc..e9c2aad2aa 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -96,9 +96,9 @@ test_expect_success 'two labels on the same changelist' '
 		git p4 sync --import-labels &&
 
 		git tag >output &&
-		grep TAG_F1 output &&
-		grep TAG_F1_1 output &&
-		grep TAG_F1_2 output &&
+		test_grep TAG_F1 output &&
+		test_grep TAG_F1_1 output &&
+		test_grep TAG_F1_2 output &&
 
 		cd main &&
 
-- 
2.39.5 (Apple Git-154)

Re: [GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes

[Anthony Wang]

From: Anthony Wang <anthonywang513@gmail.com>


On Mon, Apr 7, 2025 at 7:20 PM Junio C Hamano <gitster@pobox.com> wrote:
> Anthony Wang <anthonywang513@gmail.com> writes:
> 
> > The exit code of the upstream in a pipe is suppressed
> > thus we lose any exit codes of git commands that are piped. In order to
> > ensure we pick up the exit code, we can write the output of the git command
> > to a file, testing the exit codes of both the commands.
> 
> Sort of correct, but ...

Would it be more correct to say that the shell only returns the exit code of the 
last command in the pipeline? Then, by writing out the output of the git command 
to a file, we can test the exit codes of both the commands? If not, I am not 
quite sure what the additional nuance is in this case.

> 
> > ---
> >  t/t9811-git-p4-label-import.sh | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> Missing sign-off.
> 
> > diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
> > index 5ac5383fb7..5abac938d0 100755
> > --- a/t/t9811-git-p4-label-import.sh
> > +++ b/t/t9811-git-p4-label-import.sh
> > @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '
> >               cd "$git" &&
> >               git p4 sync --import-labels &&
> > 
> > -             git tag | grep TAG_F1 &&
> > -             git tag | grep -q TAG_F1_1 &&
> > -             git tag | grep -q TAG_F1_2 &&
> > +             git tag >output &&
> > +             grep TAG_F1 output &&
> > +             grep -q TAG_F1_1 output &&
> > +             grep -q TAG_F1_2 output &&
> 
> Think what these tests are trying to do.  After a "git p4 sync"
> operation, they want to ensure that tags TAG_F1_1 and TAG_F1_2
> exist?  Does the test want to see a tag "TAG_F1", or is it only that
> the test is written in a so sloppy way that grepping for TAG_F1 will
> be happy when any one of TAG_F1_1, TAG_F1_2 and TAG_F1_ONLY exists,
> making its purpose of verifying that the tags are in the expected
> state pretty much useless, and that is the reason why it needs to be
> followed up with the two extra tests?

I see, it seems I didn't fully consider the goals of the test. It does
indeed seem like the test has some redundancies. If I am understanding
correctly, the test grepping for TAG_F1 is not even necessary, and it
seems like the tests for TAG_F1_1, TAG_F1_2 were added afterwards to
ensure that grep TAG_F1 was not passing when TAG_F1_ONLY existed. If
this is the case, would it be better to just remove the grep TAG_F1 test
and just keep the two tests for TAG_F1_1 and TAG_F1_2? This would
simplify the test and make it clearer that we are only interested in
the existence of those two tags.

> 
> What is the desired state you want to ensure after "git p4 sync"
> operation above?  I do not do "git p4", but you may know better than
> I do, as you are the one who is patching this test file ;-)  I am
> guessing that you want TAG_F1_1 and TAG_F1_2 to exist and you do not
> want TAG_F1_ONLY to exist?
> 
> If so, instead of grepping around, we should be testing that in a
> more direct way, perhaps with something like
> 
>         git show-ref --verify refs/tags/TAG_F1_1 &&
>         git show-ref --verify refs/tags/TAG_F1_2 &&
>         test_must_fail  git show-ref --verify refs/tags/TAG_F1_ONLY &&
> 
> no?
> 

Possibly, but I believe adding the test_must_fail check would be modifying
the original intent of the test, as it would pass even with the existence of
TAG_F1_ONLY. However, if we are only performing actions to cause TAG_F1_1 
and TAG_F1_2 to exist, then it would be an issue if TAG_F1_ONLY existed.

My original plan was not to change any test coverage, and to only make small
changes following the goals of the microproject, but if you think it would
be beneficial to change the test to check for the absence of TAG_F1_ONLY,
then I will proceed.

> >               cd main &&
> > 
> > @@ -208,7 +209,8 @@ test_expect_success 'use git config to enable import/export of tags' '
> >               git p4 rebase --verbose &&
> >               git p4 submit --verbose &&
> >               git tag &&
> > -             git tag | grep TAG_F1_1
> > +             git tag >output &&
> > +             grep TAG_F1_1 output
> >       ) &&
> >       (
> >               cd "$cli" &&

Re: [GSoC] [PATCH v2 2/3] t9811: Remove the -q quiet mode from some instances of grep

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> For such a simple change, it probably would be more common on this
> project to combine patches [2/3] (which drops `-q`) and [3/3] (which
> replaces `grep` with `test_grep`), and to simply explain as a
> side-note in the commit message of the combined patch why `-q` is
> being dropped.

True, and in this case, I'd say this is better done as a single
patch to go directly to "show-ref --verify" that loses an extra
external command per tag to kill three birds with a single patch.
(1) lose the problem of pipe hiding the status, (2) clearly say
what is expected to exist and not to exist, and (3) futureproof
the test so that new tags with similar names (e.g. earlier steps
may be updated to start creating tag T_TAG_F1_1, and grep would
happily report a partial match with TAG_F1_1) will not make the
verification invalid.

Thanks.

Re: [GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes

[Junio C Hamano]

Anthony Wang <anthonywang513@gmail.com> writes:

>> If so, instead of grepping around, we should be testing that in a
>> more direct way, perhaps with something like
>> 
>>         git show-ref --verify refs/tags/TAG_F1_1 &&
>>         git show-ref --verify refs/tags/TAG_F1_2 &&
>>         test_must_fail  git show-ref --verify refs/tags/TAG_F1_ONLY &&
>> 
>> no?
>> 
>
> Possibly, but I believe adding the test_must_fail check would be modifying
> the original intent of the test, as it would pass even with the existence of
> TAG_F1_ONLY. However, if we are only performing actions to cause TAG_F1_1 
> and TAG_F1_2 to exist, then it would be an issue if TAG_F1_ONLY existed.

I view it a bit differently.

Use of "grep" over the output of "git tag" is simply a sloppy
programming.  If the test wanted to verify "TAG_F1_1 exists", it
shouldn't have grepped for TAG_F1_1, because another tag T_TAG_F1_1
would produce a false positive hit if the earlier test gets updated.

Similarly, not verifying what should not exist is being sloppy.
People who come up with a new feature (in this case, "git p4 sync"
involving tags) tend to test positive effects to show how their
shiny new toy does things, and forgets to test lack of effects to
ensure that their shiny new toy does *not* do what they should not
do.

If the original test were written solidly and use of pipe hiding
exit code were the only problem it had, I would agree that making
minimum change should be preferrable, but the original test seems to
be so sloppy in this case.

Thanks.

Re: [GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes

[Anthony Wang]

On Tue, Apr 8, 2025 at 2:17 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Anthony Wang <anthonywang513@gmail.com> writes:
>
> >> If so, instead of grepping around, we should be testing that in a
> >> more direct way, perhaps with something like
> >>
> >>         git show-ref --verify refs/tags/TAG_F1_1 &&
> >>         git show-ref --verify refs/tags/TAG_F1_2 &&
> >>         test_must_fail  git show-ref --verify refs/tags/TAG_F1_ONLY &&
> >>
> >> no?
> >>
> >
> > Possibly, but I believe adding the test_must_fail check would be modifying
> > the original intent of the test, as it would pass even with the existence of
> > TAG_F1_ONLY. However, if we are only performing actions to cause TAG_F1_1
> > and TAG_F1_2 to exist, then it would be an issue if TAG_F1_ONLY existed.
>
> I view it a bit differently.
>
> Use of "grep" over the output of "git tag" is simply a sloppy
> programming.  If the test wanted to verify "TAG_F1_1 exists", it
> shouldn't have grepped for TAG_F1_1, because another tag T_TAG_F1_1
> would produce a false positive hit if the earlier test gets updated.
>
> Similarly, not verifying what should not exist is being sloppy.
> People who come up with a new feature (in this case, "git p4 sync"
> involving tags) tend to test positive effects to show how their
> shiny new toy does things, and forgets to test lack of effects to
> ensure that their shiny new toy does *not* do what they should not
> do.
>

I see, I agree that the test is written just to check that the feature
does the intended thing, and not properly written as a tests. I will
make the changes and submit a new version.

> If the original test were written solidly and use of pipe hiding
> exit code were the only problem it had, I would agree that making
> minimum change should be preferrable, but the original test seems to
> be so sloppy in this case.
>

To this point, I have a question about when to modify code when making
patches. My understanding is that we should try to only modify the code
neccesary to fix the bug, and not modify other parts of the code.
However, because in this case the test itself does not correctly test
for the intended behavior, we should modify because we are already
touching this piece of code. Is this correct? Would it then be desired
to check the rest of the tests in this file for further oversights and
correct them as well, or would that be overstepping boundaries? Sorry
for the questions, I just want to understand the best practices as well
as I can.

Thanks,
Anthony

[GSoC] [PATCH v4 0/1] t9811: Improve test coverage and clarity

[Anthony Wang]

In order to expose more testing outputs, we remove the piping of `git tag` 
outputs in order to expose the exit codes. In addition, we change the 
usage of `grep` to `git show-ref --verify` to check for the existence of
expected tags in a cleaner manner, preventing false positives. We also
check to make sure TAG_F1_ONLY does not exist, as it is not expected to be 
created in this test.

--------------
changes in v4:
- changed `grep` to `git show-ref --verify` to check for the existence of 
  tags, and added a test_must_fail check for the unexpected tag.
- consolidated the three commits into one, as the changes were not 
  significant enough to warrant three separate commits.
- added a new commit message to clarify the changes made.

changes in v3:
- patch #1 and #2 were missing my sign-off, which has now been added.
- patch #2 referenced a line number, which was not informative. A new 
  discription has been added referencing the context of the code.

changes in v2:
- patch #2 and #3 have been added to reduce confusion caused by the 
  implications of `grep` followed by `grep -q`, and increase debug 
  output.


 t/t9811-git-p4-label-import.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
2.39.5 (Apple Git-154)

[GSoC] [PATCH v4 1/1] Remove the pipe following the `git tag`, ensuring the exit code is not hidden. Add explicit verification to check for expected and unexpected tags, increasing specificity and future-proofing a portion of the test.

[Anthony Wang]

Signed-off-by: Anthony Wang <anthonywang513@gmail.com>
---
 t/t9811-git-p4-label-import.sh | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 5ac5383fb7..cd06f39519 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '
 		cd "$git" &&
 		git p4 sync --import-labels &&
 
-		git tag | grep TAG_F1 &&
-		git tag | grep -q TAG_F1_1 &&
-		git tag | grep -q TAG_F1_2 &&
+		git tag &&
+		git show-ref --verify refs/tags/TAG_F1_1 &&
+        git show-ref --verify refs/tags/TAG_F1_2 &&
+        test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
 
 		cd main &&
 
@@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '
 		git p4 rebase --verbose &&
 		git p4 submit --verbose &&
 		git tag &&
-		git tag | grep TAG_F1_1
+		git show-ref --verify refs/tags/TAG_F1_1 &&
 	) &&
 	(
 		cd "$cli" &&
-- 
2.39.5 (Apple Git-154)

Re: [GSoC] [PATCH v4 1/1] Remove the pipe following the `git tag`, ensuring the exit code is not hidden. Add explicit verification to check for expected and unexpected tags, increasing specificity and future-proofing a portion of the test.

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2054 bytes --]

Anthony Wang <anthonywang513@gmail.com> writes:

Seems like the subject and message is combined together?

> Signed-off-by: Anthony Wang <anthonywang513@gmail.com>
> ---
>  t/t9811-git-p4-label-import.sh | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>

A quick check shows that there is a whitespace issue in this patch when
applied on top of master 9d22ac5122 (The third batch, 2025-04-07):

9d22ac51228304102deb62f30c3ecba6377e1237

--- 92f98eb326 Remove the pipe following the `git tag`, ensuring the
exit code is not hidden. Add explicit verification to check for
expected and unexpected tags, increasing specificity and
future-proofing a portion of the test.
t/t9811-git-p4-label-import.sh:100: indent with spaces.
+ git show-ref --verify refs/tags/TAG_F1_2 &&
t/t9811-git-p4-label-import.sh:101: indent with spaces.
+ test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
A whitespace issue was found in one or more of the commits.
Run the following command to resolve whitespace issues:
git rebase --whitespace=fix @~1

> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
> index 5ac5383fb7..cd06f39519 100755
> --- a/t/t9811-git-p4-label-import.sh
> +++ b/t/t9811-git-p4-label-import.sh
> @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '
>  		cd "$git" &&
>  		git p4 sync --import-labels &&
>
> -		git tag | grep TAG_F1 &&
> -		git tag | grep -q TAG_F1_1 &&
> -		git tag | grep -q TAG_F1_2 &&
> +		git tag &&
> +		git show-ref --verify refs/tags/TAG_F1_1 &&
> +        git show-ref --verify refs/tags/TAG_F1_2 &&
> +        test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
>

Looks like this is the whitespace issue.

>  		cd main &&
>
> @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '
>  		git p4 rebase --verbose &&
>  		git p4 submit --verbose &&
>  		git tag &&
> -		git tag | grep TAG_F1_1
> +		git show-ref --verify refs/tags/TAG_F1_1 &&
>  	) &&
>  	(
>  		cd "$cli" &&
> --
> 2.39.5 (Apple Git-154)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[GSoC] [PATCH v5 0/1] t9811: Improve test coverage and clarity

[Anthony Wang]

In order to expose more testing outputs, we remove the piping of `git tag` 
outputs in order to expose the exit codes. In addition, we change the 
usage of `grep` to `git show-ref --verify` to check for the existence of
expected tags in a cleaner manner, preventing false positives. We also
check to make sure TAG_F1_ONLY does not exist, as it is not expected to be 
created in this test.

--------------
changes in v5:
- moved commit message to message body, and fixed subject line
- fixed whitespace issues in patch #1

changes in v4:
- changed `grep` to `git show-ref --verify` to check for the existence of 
  tags, and added a test_must_fail check for the unexpected tag.
- consolidated the three commits into one, as the changes were not 
  significant enough to warrant three separate commits.
- added a new commit message to clarify the changes made.

changes in v3:
- patch #1 and #2 were missing my sign-off, which has now been added.
- patch #2 referenced a line number, which was not informative. A new 
  discription has been added referencing the context of the code.

changes in v2:
- patch #2 and #3 have been added to reduce confusion caused by the 
  implications of `grep` followed by `grep -q`, and increase debug 
  output.

 t/t9811-git-p4-label-import.sh | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.39.5 (Apple Git-154)

[GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity

[Anthony Wang]

Remove the pipe following the `git tag`, ensuring the exit code is not 
hidden. Add explicit verification to check for expected and unexpected 
tags, increasing specificity and future-proofing a portion of the test.

---
 t/t9811-git-p4-label-import.sh | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 5ac5383fb7..593de09eb4 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '
 		cd "$git" &&
 		git p4 sync --import-labels &&
 
-		git tag | grep TAG_F1 &&
-		git tag | grep -q TAG_F1_1 &&
-		git tag | grep -q TAG_F1_2 &&
+		git tag &&
+		git show-ref --verify refs/tags/TAG_F1_1 &&
+		git show-ref --verify refs/tags/TAG_F1_2 &&
+		test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
 
 		cd main &&
 
@@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '
 		git p4 rebase --verbose &&
 		git p4 submit --verbose &&
 		git tag &&
-		git tag | grep TAG_F1_1
+		git show-ref --verify refs/tags/TAG_F1_1 &&
 	) &&
 	(
 		cd "$cli" &&
-- 
2.39.5 (Apple Git-154)

Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity

[Junio C Hamano]

Anthony Wang <anthonywang513@gmail.com> writes:

> Subject: Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity

"Improve" -> "improve".

I am not sure about "clarity", but I agree that the main thrust of
this change is no longer "avoid hiding exit status of git behind
pipe", but more about "use 'show-ref --verify' to validate the right
thing (meaning: grepping just for a string that could be a substring
is an unreliable test)".  Perhaps that is giving more clarity?
coverage to test negative outcome is "while at it", just like we
(incidentally) lost pipes that used to hide exit status, so I am not
sure if it deserves mention on the commit title.

    t9811: be more precise to check tag creation

    The tests grep tagnames they expect to exist from "git tag"
    output, which can be fooled by false positive if an unexpected
    tag whose name has the expected tagname as its substring.  Fix
    them by using "git show-ref --verify" instead.

    While we are at it, add a negative test to verify that a tag
    that is involved in earlier tests that is not supposed to appear
    in the result does indeed not appear in the resulting
    repository.

    Incidentally, this would also correct the problem the original
    had, which lost the exit status of "git tag" that was placed
    upstream of a pipe.

or something, perhaps?

Also you'd need to sign-off your patch.

Thanks.

> Remove the pipe following the `git tag`, ensuring the exit code is not 
> hidden. Add explicit verification to check for expected and unexpected 
> tags, increasing specificity and future-proofing a portion of the test.
>
> ---
>  t/t9811-git-p4-label-import.sh | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
> index 5ac5383fb7..593de09eb4 100755
> --- a/t/t9811-git-p4-label-import.sh
> +++ b/t/t9811-git-p4-label-import.sh
> @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '
>  		cd "$git" &&
>  		git p4 sync --import-labels &&
>  
> -		git tag | grep TAG_F1 &&
> -		git tag | grep -q TAG_F1_1 &&
> -		git tag | grep -q TAG_F1_2 &&
> +		git tag &&
> +		git show-ref --verify refs/tags/TAG_F1_1 &&
> +		git show-ref --verify refs/tags/TAG_F1_2 &&
> +		test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
>  
>  		cd main &&
>  
> @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '
>  		git p4 rebase --verbose &&
>  		git p4 submit --verbose &&
>  		git tag &&
> -		git tag | grep TAG_F1_1
> +		git show-ref --verify refs/tags/TAG_F1_1 &&
>  	) &&
>  	(
>  		cd "$cli" &&

Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity

[Eric Sunshine]

On Tue, Apr 8, 2025 at 5:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>     t9811: be more precise to check tag creation
>
>     The tests grep tagnames they expect to exist from "git tag"

s/tagnames/tag names/ perhaps?

>     output, which can be fooled by false positive if an unexpected
>     tag whose name has the expected tagname as its substring.  Fix
>     them by using "git show-ref --verify" instead.
>
>     While we are at it, add a negative test to verify that a tag
>     that is involved in earlier tests that is not supposed to appear
>     in the result does indeed not appear in the resulting
>     repository.
>
>     Incidentally, this would also correct the problem the original
>     had, which lost the exit status of "git tag" that was placed
>     upstream of a pipe.
>
> or something, perhaps?

Yes, better and much more illuminating.

> > -             git tag | grep TAG_F1 &&
> > -             git tag | grep -q TAG_F1_1 &&
> > -             git tag | grep -q TAG_F1_2 &&
> > +             git tag &&
> > +             git show-ref --verify refs/tags/TAG_F1_1 &&
> > +             git show-ref --verify refs/tags/TAG_F1_2 &&
> > +             test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&

Do we still need the standalone `git tag` invocation above?

> > @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '
> >               git p4 submit --verbose &&
> >               git tag &&
> > -             git tag | grep TAG_F1_1
> > +             git show-ref --verify refs/tags/TAG_F1_1 &&

Similarly, it's not clear why there is a standalone `git tag`
invocation here. Does it buy us anything or am I missing something
obvious? The originating commit[*] doesn't explain its purpose.

[*] e71f6a53e2 (git p4: add test for tag import/export enabled via
config, 2012-05-11)

Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity

[Anthony Wang]

On Tue, Apr 8, 2025 at 11:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Tue, Apr 8, 2025 at 5:21 PM Junio C Hamano <gitster@pobox.com> wrote:
> >     t9811: be more precise to check tag creation
> >
> >     The tests grep tagnames they expect to exist from "git tag"
>
> s/tagnames/tag names/ perhaps?

How does "t9811: be more precise to check importing of tags" sound?
The tags are created in the p4 repository, and what we care about is if
`git p4 sync --import-labels` correctly imports the labels from the p4
repository. Specifying tags instead of tag indicates that there are
multiple tags, which is the whole purpose of the test.

>
> >     output, which can be fooled by false positive if an unexpected
> >     tag whose name has the expected tagname as its substring.  Fix
> >     them by using "git show-ref --verify" instead.
> >
> >     While we are at it, add a negative test to verify that a tag
> >     that is involved in earlier tests that is not supposed to appear
> >     in the result does indeed not appear in the resulting
> >     repository.
> >
> >     Incidentally, this would also correct the problem the original
> >     had, which lost the exit status of "git tag" that was placed
> >     upstream of a pipe.
> >
> > or something, perhaps?
>
> Yes, better and much more illuminating.

I see, that message does communicate the intentions of the changes
much better than I did. I will expand the message and write something
along the lines of what you have recommended.

>
> > > -             git tag | grep TAG_F1 &&
> > > -             git tag | grep -q TAG_F1_1 &&
> > > -             git tag | grep -q TAG_F1_2 &&
> > > +             git tag &&
> > > +             git show-ref --verify refs/tags/TAG_F1_1 &&
> > > +             git show-ref --verify refs/tags/TAG_F1_2 &&
> > > +             test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
>
> Do we still need the standalone `git tag` invocation above?

The original intent of the patch was to expose the exit code of
`git tag`. By keeping the standalone `git tag` we are able to pick up
the exit code if there are any issues, such as if `git p4 sync
--import-labels` somehow breaks `git tag`. I believe this is the intent
of the standalone `git tag` in the section below, and as such, I added
it to the top section in order to test the `git tag` functionality, as we
removed the other `git tag` instances. However, I can understand
if it is unnecessary, and I will remove it.

>
> > > @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '
> > >               git p4 submit --verbose &&
> > >               git tag &&
> > > -             git tag | grep TAG_F1_1
> > > +             git show-ref --verify refs/tags/TAG_F1_1 &&
>
> Similarly, it's not clear why there is a standalone `git tag`
> invocation here. Does it buy us anything or am I missing something
> obvious? The originating commit[*] doesn't explain its purpose.
>

Again, as we have removed the other `git tag` instances, it might
still have value to test the result of running the `git tag` command.

> [*] e71f6a53e2 (git p4: add test for tag import/export enabled via
> config, 2012-05-11)

Thank you very much for the valuable feedback, it has been very
eye opening to see how you two approach patching code like this.
I wanted to bump one of my previous questions, as I am very
curious about what the best practice is here:

To this point, I have a question about when to modify code when making
patches. My understanding is that we should try to only modify the code
necessary to fix the bug, and not modify other parts of the code.
However, because in this case the test itself does not correctly test
for the intended behavior, we should modify because we are already
touching this piece of code. Is this correct? Would it then be desired
to check the rest of the tests in this file for further oversights and
correct them as well, or would that be overstepping boundaries?

Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity

[Junio C Hamano]

Anthony Wang <anthonywang513@gmail.com> writes:

>> >     The tests grep tagnames they expect to exist from "git tag"
>>
>> s/tagnames/tag names/ perhaps?
>
> How does "t9811: be more precise to check importing of tags" sound?

Excellent.

>> > > +             git tag &&
>> > > +             git show-ref --verify refs/tags/TAG_F1_1 &&
>> > > +             git show-ref --verify refs/tags/TAG_F1_2 &&
>> > > +             test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
>>
>> Do we still need the standalone `git tag` invocation above?
>
> The original intent of the patch was to expose the exit code of
> `git tag`.

Is it?  I somehow thought that "git tag" is not what is being tested
by this script.  Rather, it assumed that "git tag" works perfectly
well, and validated what "git p4" left in the resulting repository
based on that assumption, i.e. "git tag" works perfectly well to
tell us what tags are in the repository.

It is true that "git tag" to list all available tags may fail, but
then catching that is outside the scope of this script no?  It is
even more so, since now we do not even depend on the correct
operation of "git tag" anymore to validate what "git p4" did---we
now use "show-ref --verify" for that, so we do not even care if "git
tag" segfaults in this part of the test, no?

> However, because in this case the test itself does not correctly test
> for the intended behavior, we should modify because we are already
> touching this piece of code. Is this correct? Would it then be desired
> to check the rest of the tests in this file for further oversights and
> correct them as well, or would that be overstepping boundaries?

Just like any real world problems, there unfortunately is no bright
red line between "yeah these are related enough and in the same spot
and it is better to clean up while we are at it" and "that is way
too much for this single topic" that can be described in a textbook.
A rule of thumb I personally use is to put me in the shoes of an
imaginary typical Git developer with moderate competence, who hasn't
seen or worked on a particular part of the system being updated.  If
I can easily imagine that the developer can clearly see a need for
clean up (in this case, "the part of the code only tests positive
results and forgets about negative check") while fixing something
else (in this case, "use of 'git tag' piped to 'grep' has at least
two problems, loss of exit code and false match") and the additional
effort would be smaller than 10-20 minutes, I'd say it would be
worth doing and anything larger would be better to leave to another
day, but a lot of ingredients in that statement are very much
subjective (starting from "what's the average competence level we
expect from our people?").

[GSoC] [PATCH v6 0/1] t9811: be more precise to check importing of tags

[Anthony Wang]

In order to expose more testing outputs, we remove the piping of `git tag` 
outputs in order to expose the exit codes. In addition, we change the 
usage of `grep` to `git show-ref --verify` to check for the existence of
expected tags in a cleaner manner, preventing false positives. We also
check to make sure TAG_F1_ONLY does not exist, as it is not expected to be 
created in this test.

--------------
changes in v6:
- removed unneccesary calls to `git tag`
- changed commit message to better describe rationale for changes
- added missing sign-off to patch #1

changes in v5:
- moved commit message to message body, and fixed subject line
- fixed whitespace issues in patch #1

changes in v4:
- changed `grep` to `git show-ref --verify` to check for the existence of 
  tags, and added a test_must_fail check for the unexpected tag.
- consolidated the three commits into one, as the changes were not 
  significant enough to warrant three separate commits.
- added a new commit message to clarify the changes made.

changes in v3:
- patch #1 and #2 were missing my sign-off, which has now been added.
- patch #2 referenced a line number, which was not informative. A new 
  discription has been added referencing the context of the code.

changes in v2:
- patch #2 and #3 have been added to reduce confusion caused by the 
  implications of `grep` followed by `grep -q`, and increase debug 
  output.

 t/t9811-git-p4-label-import.sh | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

-- 
2.39.5 (Apple Git-154)

[GSoC] [PATCH v6 1/1] t9811: be more precise to check importing of tags

[Anthony Wang]

The tests use grep to search the output of `git tag` for tagnames they 
expect to exist, which can incorrectly pass if an unxpected tag
has the expected tag as its substring. We fix this by using `git 
show-ref --verify` instead.

Additionally, we add a negative test to verify that a possible
uninteded tag does not show up in the imported repository.

This change also fixes the original problem, where piping the
output of `git tag` caused the exit codes to be lost.

Signed-off-by: Anthony Wang <anthonywang513@gmail.com>
---
 t/t9811-git-p4-label-import.sh | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 5ac5383fb7..39856629c0 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -95,9 +95,9 @@ test_expect_success 'two labels on the same changelist' '
 		cd "$git" &&
 		git p4 sync --import-labels &&
 
-		git tag | grep TAG_F1 &&
-		git tag | grep -q TAG_F1_1 &&
-		git tag | grep -q TAG_F1_2 &&
+		git show-ref --verify refs/tags/TAG_F1_1 &&
+		git show-ref --verify refs/tags/TAG_F1_2 &&
+		test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
 
 		cd main &&
 
@@ -207,8 +207,7 @@ test_expect_success 'use git config to enable import/export of tags' '
 		git tag CFG_A_GIT_TAG &&
 		git p4 rebase --verbose &&
 		git p4 submit --verbose &&
-		git tag &&
-		git tag | grep TAG_F1_1
+		git show-ref --verify refs/tags/TAG_F1_1 &&
 	) &&
 	(
 		cd "$cli" &&
-- 
2.39.5 (Apple Git-154)

Re: [GSoC] [PATCH v6 1/1] t9811: be more precise to check importing of tags

[Junio C Hamano]

Anthony Wang <anthonywang513@gmail.com> writes:

> The tests use grep to search the output of `git tag` for tagnames they 
> expect to exist, which can incorrectly pass if an unxpected tag
> has the expected tag as its substring. We fix this by using `git 
> show-ref --verify` instead.
>
> Additionally, we add a negative test to verify that a possible
> uninteded tag does not show up in the imported repository.
>
> This change also fixes the original problem, where piping the
> output of `git tag` caused the exit codes to be lost.

The word "original" is misleading; perhaps phase it as "additional"
instead?

That is because not allowing to notice potential breakage by hiding
the exit status behind pipes is just as bad as falsely taking a
partial tagname match as success, and there is no reason to call one
"original" problem, implying the other problem(s) are different.

Other than that, looks excellent.

Thanks.


> Signed-off-by: Anthony Wang <anthonywang513@gmail.com>
> ---
>  t/t9811-git-p4-label-import.sh | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
> index 5ac5383fb7..39856629c0 100755
> --- a/t/t9811-git-p4-label-import.sh
> +++ b/t/t9811-git-p4-label-import.sh
> @@ -95,9 +95,9 @@ test_expect_success 'two labels on the same changelist' '
>  		cd "$git" &&
>  		git p4 sync --import-labels &&
>  
> -		git tag | grep TAG_F1 &&
> -		git tag | grep -q TAG_F1_1 &&
> -		git tag | grep -q TAG_F1_2 &&
> +		git show-ref --verify refs/tags/TAG_F1_1 &&
> +		git show-ref --verify refs/tags/TAG_F1_2 &&
> +		test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
>  
>  		cd main &&
>  
> @@ -207,8 +207,7 @@ test_expect_success 'use git config to enable import/export of tags' '
>  		git tag CFG_A_GIT_TAG &&
>  		git p4 rebase --verbose &&
>  		git p4 submit --verbose &&
> -		git tag &&
> -		git tag | grep TAG_F1_1
> +		git show-ref --verify refs/tags/TAG_F1_1 &&
>  	) &&
>  	(
>  		cd "$cli" &&

[GSoC] [PATCH v7 0/1] t9811: be more precise to check importing of tags

[Anthony Wang]

In order to expose more testing outputs, we remove the piping of `git tag` 
outputs in order to expose the exit codes. In addition, we change the 
usage of `grep` to `git show-ref --verify` to check for the existence of
expected tags in a cleaner manner, preventing false positives. We also
check to make sure TAG_F1_ONLY does not exist, as it is not expected to be 
created in this test.

--------------
changes in v7:
- changed wording from "the original problem" to "an additional problem"
  as both problems are important, and there is not an "original" problem
  
changes in v6:
- removed unneccesary calls to `git tag`
- changed commit message to better describe rationale for changes
- added missing sign-off to patch #1

changes in v5:
- moved commit message to message body, and fixed subject line
- fixed whitespace issues in patch #1

changes in v4:
- changed `grep` to `git show-ref --verify` to check for the existence of 
  tags, and added a test_must_fail check for the unexpected tag.
- consolidated the three commits into one, as the changes were not 
  significant enough to warrant three separate commits.
- added a new commit message to clarify the changes made.

changes in v3:
- patch #1 and #2 were missing my sign-off, which has now been added.
- patch #2 referenced a line number, which was not informative. A new 
  discription has been added referencing the context of the code.

changes in v2:
- patch #2 and #3 have been added to reduce confusion caused by the 
  implications of `grep` followed by `grep -q`, and increase debug 
  output.

 t/t9811-git-p4-label-import.sh | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

--
2.39.5 (Apple Git-154)

[GSoC] [PATCH v7 1/1] t9811: be more precise to check importing of tags

[Anthony Wang]

The tests use grep to search the output of `git tag` for tagnames they 
expect to exist, which can incorrectly pass if an unxpected tag
has the expected tag as its substring. We fix this by using `git 
show-ref --verify` instead.

Additionally, we add a negative test to verify that a possible
uninteded tag does not show up in the imported repository.

This change also fixes an additional problem, where piping the
output of `git tag` caused the exit codes to be lost.

Signed-off-by: Anthony Wang <anthonywang513@gmail.com>
---
 t/t9811-git-p4-label-import.sh | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 5ac5383fb7..39856629c0 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -95,9 +95,9 @@ test_expect_success 'two labels on the same changelist' '
 		cd "$git" &&
 		git p4 sync --import-labels &&
 
-		git tag | grep TAG_F1 &&
-		git tag | grep -q TAG_F1_1 &&
-		git tag | grep -q TAG_F1_2 &&
+		git show-ref --verify refs/tags/TAG_F1_1 &&
+		git show-ref --verify refs/tags/TAG_F1_2 &&
+		test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
 
 		cd main &&
 
@@ -207,8 +207,7 @@ test_expect_success 'use git config to enable import/export of tags' '
 		git tag CFG_A_GIT_TAG &&
 		git p4 rebase --verbose &&
 		git p4 submit --verbose &&
-		git tag &&
-		git tag | grep TAG_F1_1
+		git show-ref --verify refs/tags/TAG_F1_1 &&
 	) &&
 	(
 		cd "$cli" &&
-- 
2.39.5 (Apple Git-154)

Re: [GSoC] [PATCH v6 1/1] t9811: be more precise to check importing of tags

[Anthony Wang]

On Tue, Apr 15, 2025 at 4:55 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Anthony Wang <anthonywang513@gmail.com> writes:
>
> > The tests use grep to search the output of `git tag` for tagnames they
> > expect to exist, which can incorrectly pass if an unxpected tag
> > has the expected tag as its substring. We fix this by using `git
> > show-ref --verify` instead.
> >
> > Additionally, we add a negative test to verify that a possible
> > uninteded tag does not show up in the imported repository.
> >
> > This change also fixes the original problem, where piping the
> > output of `git tag` caused the exit codes to be lost.
>
> The word "original" is misleading; perhaps phase it as "additional"
> instead?
>
> That is because not allowing to notice potential breakage by hiding
> the exit status behind pipes is just as bad as falsely taking a
> partial tagname match as success, and there is no reason to call one
> "original" problem, implying the other problem(s) are different.
>
> Other than that, looks excellent.
>
> Thanks.
>

Changed and resubmitted the patch. Thank you for the guidance on this
microproject, I appreciate the help as I am still new to open-source, and
I now understand the workflow and style of Git much better.

>
> > Signed-off-by: Anthony Wang <anthonywang513@gmail.com>
> > ---
> >  t/t9811-git-p4-label-import.sh | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
> > index 5ac5383fb7..39856629c0 100755
> > --- a/t/t9811-git-p4-label-import.sh
> > +++ b/t/t9811-git-p4-label-import.sh
> > @@ -95,9 +95,9 @@ test_expect_success 'two labels on the same changelist' '
> >               cd "$git" &&
> >               git p4 sync --import-labels &&
> >
> > -             git tag | grep TAG_F1 &&
> > -             git tag | grep -q TAG_F1_1 &&
> > -             git tag | grep -q TAG_F1_2 &&
> > +             git show-ref --verify refs/tags/TAG_F1_1 &&
> > +             git show-ref --verify refs/tags/TAG_F1_2 &&
> > +             test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
> >
> >               cd main &&
> >
> > @@ -207,8 +207,7 @@ test_expect_success 'use git config to enable import/export of tags' '
> >               git tag CFG_A_GIT_TAG &&
> >               git p4 rebase --verbose &&
> >               git p4 submit --verbose &&
> > -             git tag &&
> > -             git tag | grep TAG_F1_1
> > +             git show-ref --verify refs/tags/TAG_F1_1 &&
> >       ) &&
> >       (
> >               cd "$cli" &&

Re: [GSoC] [PATCH v7 1/1] t9811: be more precise to check importing of tags

[Junio C Hamano]

Anthony Wang <anthonywang513@gmail.com> writes:

> Additionally, we add a negative test to verify that a possible
> uninteded tag does not show up in the imported repository.

With this we tightened the tests to insist that TAG_F1_ONLY does not
exist, but it seems that our CI tests at least on macos seems to
think that the tag should exist.

https://github.com/git/git/actions/runs/14526556144/job/40759116464#step:4:1944

> +		git show-ref --verify refs/tags/TAG_F1_1 &&
> +		git show-ref --verify refs/tags/TAG_F1_2 &&
> +		test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&

And because of that, this third line which does not correspond to
any tests in the original makes the thing fail.

I think negative test was what I suggested, but I didn't know if
that particular tag used for the negative test should or should not
exist in the test at that point (I do not do Perforce, so I still do
not know the answer to that question; in any case, due to lack of p4
in my environment, my local testing did not catch this breakage).

Sorry about the confusion.

Let's add this on top.

-- >8 ---- >8 ---- >8 --
Subject: [PATCH] t9811: fix misconversion of test

The previous commit started to insist TAG_F1_ONLY to be missing,
which was not in the original.  Let's not to be overly eager in the
conversion.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t9811-git-p4-label-import.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 39856629c0..9637a46d6f 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -97,7 +97,6 @@ test_expect_success 'two labels on the same changelist' '
 
 		git show-ref --verify refs/tags/TAG_F1_1 &&
 		git show-ref --verify refs/tags/TAG_F1_2 &&
-		test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
 
 		cd main &&
 
-- 
2.49.0-524-g64a58d64d1

Re: [GSoC] [PATCH v7 1/1] t9811: be more precise to check importing of tags

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Let's add this on top.

Well, it turns out that it wasn't enough.

--- >8 ------ >8 ------ >8 ---
Subject: [PATCH] t9811: fix misconversion of test

The previous commit started to insist TAG_F1_ONLY to be missing,
which was not in the original.  Let's not to be overly eager in the
conversion.

Aso, the other hunk in the commit introduced shell syntax errors,
breaking the test to fail.  Fix it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t9811-git-p4-label-import.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 39856629c0..7614dfbd95 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -97,7 +97,6 @@ test_expect_success 'two labels on the same changelist' '
 
 		git show-ref --verify refs/tags/TAG_F1_1 &&
 		git show-ref --verify refs/tags/TAG_F1_2 &&
-		test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
 
 		cd main &&
 
@@ -207,7 +206,7 @@ test_expect_success 'use git config to enable import/export of tags' '
 		git tag CFG_A_GIT_TAG &&
 		git p4 rebase --verbose &&
 		git p4 submit --verbose &&
-		git show-ref --verify refs/tags/TAG_F1_1 &&
+		git show-ref --verify refs/tags/TAG_F1_1
 	) &&
 	(
 		cd "$cli" &&
-- 
2.49.0-524-g64a58d64d1

Re: [GSoC] [PATCH v7 1/1] t9811: be more precise to check importing of tags

[Eric Sunshine]

On Fri, Apr 18, 2025 at 5:03 PM Junio C Hamano <gitster@pobox.com> wrote:
> Subject: [PATCH] t9811: fix misconversion of test
>
> The previous commit started to insist TAG_F1_ONLY to be missing,
> which was not in the original.  Let's not to be overly eager in the
> conversion.

s/to be/be/

> Aso, the other hunk in the commit introduced shell syntax errors,
> breaking the test to fail.  Fix it.

s/Aso/Also/
s/breaking/causing/

> Signed-off-by: Junio C Hamano <gitster@pobox.com>