patcht9811: avoid using pipes

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

On Sat, Apr 05, 2025 at 12:37:18PM +0200, Anthony Wang wrote:

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

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

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

On Mon, Apr 7, 2025 at 7:18 AM Anthony Wang <anthonywang513@gmail.com> wrote:

Missing sign-off.

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

Anthony Wang <anthonywang513@gmail.com> writes:

Sort of correct, but ...

Missing sign-off.

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?

From: Anthony Wang <anthonywang513@gmail.com>

On Mon, Apr 7, 2025 at 7:20 PM Junio C Hamano <gitster@pobox.com> wrote:

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.

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.

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.

Anthony Wang <anthonywang513@gmail.com> writes:

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.

On Tue, Apr 8, 2025 at 2:17 AM Junio C Hamano <gitster@pobox.com> wrote:

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.

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

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

On Mon, Apr 7, 2025 at 7:18 AM Anthony Wang <anthonywang513@gmail.com> wrote:

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

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.

Eric Sunshine <sunshine@sunshineco.com> writes:

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.

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

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

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

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

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

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

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

Anthony Wang <anthonywang513@gmail.com> writes:

Seems like the subject and message is combined together?

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

Looks like this is the whitespace issue.

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

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

Anthony Wang <anthonywang513@gmail.com> writes:

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

On Tue, Apr 8, 2025 at 5:21 PM Junio C Hamano <gitster@pobox.com> wrote:

s/tagnames/tag names/ perhaps?

Yes, better and much more illuminating.

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

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)

On Tue, Apr 8, 2025 at 11:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

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.

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.

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.

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.

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?

Anthony Wang <anthonywang513@gmail.com> writes:

Excellent.

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?

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

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

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

Anthony Wang <anthonywang513@gmail.com> writes:

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.

On Tue, Apr 15, 2025 at 4:55 PM Junio C Hamano <gitster@pobox.com> wrote:

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.

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)

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

Anthony Wang <anthonywang513@gmail.com> writes:

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

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

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

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

On Fri, Apr 18, 2025 at 5:03 PM Junio C Hamano <gitster@pobox.com> wrote:

s/to be/be/

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