{"thread":{"id":"63258","subject":"[GSoC] [PATCH 1/1] t9811: avoid using pipes","startedAt":"2025-04-05T10:37:24Z","lastAt":"2025-04-18T21:41:15Z","messageCount":35,"participants":["Anthony Wang","Patrick Steinhardt","Eric Sunshine","Junio C Hamano","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"515693","messageId":"20250405103718.25160-1-anthonywang03@icloud.com","threadId":"63258","inReplyTo":null,"subject":"[GSoC] [PATCH 1/1] t9811: avoid using pipes","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-05T10:37:18Z","receivedAt":"2025-04-05T10:37:24Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"The exit code of the upstream in a pipe is suppressed thus we lose any\nexit codes of git commands that are piped. In order to ensure we\npick up the exit code, we can write the output of the git command to\na file, testing the exit codes of both the commands.\n\nSigned-off-by: Anthony Wang \n---\n t/t9811-git-p4-label-import.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 5ac5383fb7..5abac938d0 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '\n \t\tcd \"$git\" &&\n \t\tgit p4 sync --import-labels &&\n \n-\t\tgit tag | grep TAG_F1 &&\n-\t\tgit tag | grep -q TAG_F1_1 &&\n-\t\tgit tag | grep -q TAG_F1_2 &&\n+\t\tgit tag >output &&\n+\t\tgrep TAG_F1 output &&\n+\t\tgrep -q TAG_F1_1 output &&\n+\t\tgrep -q TAG_F1_2 output &&\n \n \t\tcd main &&\n \n@@ -208,7 +209,8 @@ test_expect_success 'use git config to enable import/export of tags' '\n \t\tgit p4 rebase --verbose &&\n \t\tgit p4 submit --verbose &&\n \t\tgit tag &&\n-\t\tgit tag | grep TAG_F1_1\n+\t\tgit tag >output &&\n+\t\tgrep TAG_F1_1 output\n \t) &&\n \t(\n \t\tcd \"$cli\" &&\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515743","messageId":"Z_OEAxJENE_mGkVa@pks.im","threadId":"63258","inReplyTo":"20250405103718.25160-1-anthonywang03@icloud.com","subject":"Re: [GSoC] [PATCH 1/1] t9811: avoid using pipes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-07T07:51:31Z","receivedAt":"2025-04-07T07:51:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Apr 05, 2025 at 12:37:18PM +0200, Anthony Wang wrote:\n> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n> index 5ac5383fb7..5abac938d0 100755\n> --- a/t/t9811-git-p4-label-import.sh\n> +++ b/t/t9811-git-p4-label-import.sh\n> @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '\n> \t\tcd \"$git\" &&\n> \t\tgit p4 sync --import-labels &&\n> \n> -\t\tgit tag | grep TAG_F1 &&\n> -\t\tgit tag | grep -q TAG_F1_1 &&\n> -\t\tgit tag | grep -q TAG_F1_2 &&\n> +\t\tgit tag >output &&\n> +\t\tgrep TAG_F1 output &&\n> +\t\tgrep -q TAG_F1_1 output &&\n> +\t\tgrep -q TAG_F1_2 output &&\n> \n> \t\tcd main &&\n> \n\nI was a bit puzzled why we use `grep` for the first invocation, but\n`grep -q` for the other ones. It made me double check that there is no\nsurrounding shell that expects to read the output generated by that\nline, but as far as I can see that is not the case. We might want to\ndrop them to avoid such confusion (and explain in the commit message why\nwe want to drop it).\n\nWhile at it, you can also adapt all adapted calls of `grep` to instead\nuse `test_grep`. The benefit of that function is that it will provide\nhelpful debug output in case the test ever was to fail by printing\ncontents of the non-matching file.\n\nOther than that the patch looks good to me, thanks!\n\nPatrick\n"},{"id":"515751","messageId":"20250407111824.46518-1-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"Z_OEAxJENE_mGkVa@pks.im","subject":"[GSoC] [PATCH v2 0/3] t9811: Improve test coverage and clarity","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-07T11:18:21Z","receivedAt":"2025-04-07T11:18:29Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"In order to expose more testing outputs, we remove the piping of `git tag` \noutputs in order to expose the exit codes. In addition, we remove `-q` \ntags on instances of `grep` to ensure clarity. We also replace `grep` \nwith `test_grep` to provide helpful debug output in case of test failure.\n\nchanges in v2:\n--------------\n\n- patch #2 and #3 have been added to reduce confusion caused by the \n implications of `grep` followed by `grep -q`, and increase debug \n output.\n \n\n t/t9811-git-p4-label-import.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515752","messageId":"20250407111824.46518-2-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250407111824.46518-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-07T11:18:22Z","receivedAt":"2025-04-07T11:18:33Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"The exit code of the upstream in a pipe is suppressed\nthus we lose any exit codes of git commands that are piped. In order to\nensure we pick up the exit code, we can write the output of the git command\nto a file, testing the exit codes of both the commands.\n\n---\n t/t9811-git-p4-label-import.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 5ac5383fb7..5abac938d0 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '\n \t\tcd \"$git\" &&\n \t\tgit p4 sync --import-labels &&\n \n-\t\tgit tag | grep TAG_F1 &&\n-\t\tgit tag | grep -q TAG_F1_1 &&\n-\t\tgit tag | grep -q TAG_F1_2 &&\n+\t\tgit tag >output &&\n+\t\tgrep TAG_F1 output &&\n+\t\tgrep -q TAG_F1_1 output &&\n+\t\tgrep -q TAG_F1_2 output &&\n \n \t\tcd main &&\n \n@@ -208,7 +209,8 @@ test_expect_success 'use git config to enable import/export of tags' '\n \t\tgit p4 rebase --verbose &&\n \t\tgit p4 submit --verbose &&\n \t\tgit tag &&\n-\t\tgit tag | grep TAG_F1_1\n+\t\tgit tag >output &&\n+\t\tgrep TAG_F1_1 output\n \t) &&\n \t(\n \t\tcd \"$cli\" &&\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515753","messageId":"20250407111824.46518-3-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250407111824.46518-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v2 2/3] t9811: Remove the -q quiet mode from some instances of grep","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-07T11:18:23Z","receivedAt":"2025-04-07T11:18:34Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"Remove the `-q` quiet mode from some instances of `grep`,\nas the lack of `-q` on the `grep` on line 99 implies that its output is\nrequired, when that is not the case. This change ensures consistency and\navoids confusion about whether the output of `grep` is used.\n\n---\n t/t9811-git-p4-label-import.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 5abac938d0..e69dae55dc 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -97,8 +97,8 @@ test_expect_success 'two labels on the same changelist' '\n \n \t\tgit tag >output &&\n \t\tgrep TAG_F1 output &&\n-\t\tgrep -q TAG_F1_1 output &&\n-\t\tgrep -q TAG_F1_2 output &&\n+\t\tgrep TAG_F1_1 output &&\n+\t\tgrep TAG_F1_2 output &&\n \n \t\tcd main &&\n \n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515754","messageId":"20250407111824.46518-4-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250407111824.46518-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v2 3/3] t9811: Change `grep` to `test_grep` for debug output","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-07T11:18:24Z","receivedAt":"2025-04-07T11:18:36Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"Change `grep` to `test_grep` to provide helpful debug\noutput in case of test failure by printing contents of the non-matching file.\n\nSigned-off-by: Anthony Wang \n---\n t/t9811-git-p4-label-import.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex e69dae55dc..e9c2aad2aa 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -96,9 +96,9 @@ test_expect_success 'two labels on the same changelist' '\n \t\tgit p4 sync --import-labels &&\n \n \t\tgit tag >output &&\n-\t\tgrep TAG_F1 output &&\n-\t\tgrep TAG_F1_1 output &&\n-\t\tgrep TAG_F1_2 output &&\n+\t\ttest_grep TAG_F1 output &&\n+\t\ttest_grep TAG_F1_1 output &&\n+\t\ttest_grep TAG_F1_2 output &&\n \n \t\tcd main &&\n \n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515783","messageId":"CAPig+cQ78ubvrEDtW0NKPSxtyv8-4GO8+Mxy0cjJKn2Cvwnxjw@mail.gmail.com","threadId":"63258","inReplyTo":"20250407111824.46518-2-anthonywang03@icloud.com","subject":"Re: [GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-04-07T16:17:35Z","receivedAt":"2025-04-07T16:17:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 7, 2025 at 7:18 AM Anthony Wang wrote:\n> The exit code of the upstream in a pipe is suppressed\n> thus we lose any exit codes of git commands that are piped. In order to\n> ensure we pick up the exit code, we can write the output of the git command\n> to a file, testing the exit codes of both the commands.\n> ---\n\nMissing sign-off.\n\n> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n> @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '\n> - git tag | grep TAG_F1 &&\n> - git tag | grep -q TAG_F1_1 &&\n> - git tag | grep -q TAG_F1_2 &&\n> + git tag >output &&\n> + grep TAG_F1 output &&\n> + grep -q TAG_F1_1 output &&\n> + grep -q TAG_F1_2 output &&\n\nSince process creation is so expensive on Microsoft Windows, folks on\nthat platform should also appreciate that this eliminates two git-tag\ninvocations. Nice.\n"},{"id":"515784","messageId":"CAPig+cQbMUQAHsx92QNLAvfkUKRFoVoWznhPQok0PN6HW3wxzQ@mail.gmail.com","threadId":"63258","inReplyTo":"20250407111824.46518-3-anthonywang03@icloud.com","subject":"Re: [GSoC] [PATCH v2 2/3] t9811: Remove the -q quiet mode from some instances of grep","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-04-07T16:26:23Z","receivedAt":"2025-04-07T16:26:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 7, 2025 at 7:18 AM Anthony Wang wrote:\n> Remove the `-q` quiet mode from some instances of `grep`,\n> as the lack of `-q` on the `grep` on line 99 implies that its output is\n> required, when that is not the case. This change ensures consistency and\n> avoids confusion about whether the output of `grep` is used.\n> ---\n\nMissing sign-off.\n\nRather than mentioning line 99 explicitly, it probably would be more\nhelpful to readers to say instead:\n\n ... the lack of `-q` on the \"TAG_F1\" `grep` implies...\n\n> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n> @@ -97,8 +97,8 @@ test_expect_success 'two labels on the same changelist' '\n> git tag >output &&\n> grep TAG_F1 output &&\n> - grep -q TAG_F1_1 output &&\n> - grep -q TAG_F1_2 output &&\n> + grep TAG_F1_1 output &&\n> + grep TAG_F1_2 output &&\n\nFor such a simple change, it probably would be more common on this\nproject to combine patches [2/3] (which drops `-q`) and [3/3] (which\nreplaces `grep` with `test_grep`), and to simply explain as a\nside-note in the commit message of the combined patch why `-q` is\nbeing dropped. However, it's also fairly subjective, and others may\nfeel differently, so keeping the changes as separate patches is also\nvalid.\n"},{"id":"515787","messageId":"xmqqo6x7nb2a.fsf@gitster.g","threadId":"63258","inReplyTo":"20250407111824.46518-2-anthonywang03@icloud.com","subject":"Re: [GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-07T17:19:57Z","receivedAt":"2025-04-07T17:20:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anthony Wang writes:\n\n> The exit code of the upstream in a pipe is suppressed\n> thus we lose any exit codes of git commands that are piped. In order to\n> ensure we pick up the exit code, we can write the output of the git command\n> to a file, testing the exit codes of both the commands.\n\nSort of correct, but ...\n\n> ---\n> t/t9811-git-p4-label-import.sh | 10 ++++++----\n> 1 file changed, 6 insertions(+), 4 deletions(-)\n\nMissing sign-off.\n\n> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n> index 5ac5383fb7..5abac938d0 100755\n> --- a/t/t9811-git-p4-label-import.sh\n> +++ b/t/t9811-git-p4-label-import.sh\n> @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '\n> \t\tcd \"$git\" &&\n> \t\tgit p4 sync --import-labels &&\n> \n> -\t\tgit tag | grep TAG_F1 &&\n> -\t\tgit tag | grep -q TAG_F1_1 &&\n> -\t\tgit tag | grep -q TAG_F1_2 &&\n> +\t\tgit tag >output &&\n> +\t\tgrep TAG_F1 output &&\n> +\t\tgrep -q TAG_F1_1 output &&\n> +\t\tgrep -q TAG_F1_2 output &&\n\nThink what these tests are trying to do. After a \"git p4 sync\"\noperation, they want to ensure that tags TAG_F1_1 and TAG_F1_2\nexist? Does the test want to see a tag \"TAG_F1\", or is it only that\nthe test is written in a so sloppy way that grepping for TAG_F1 will\nbe happy when any one of TAG_F1_1, TAG_F1_2 and TAG_F1_ONLY exists,\nmaking its purpose of verifying that the tags are in the expected\nstate pretty much useless, and that is the reason why it needs to be\nfollowed up with the two extra tests?\n\nWhat is the desired state you want to ensure after \"git p4 sync\"\noperation above? I do not do \"git p4\", but you may know better than\nI do, as you are the one who is patching this test file ;-) I am\nguessing that you want TAG_F1_1 and TAG_F1_2 to exist and you do not\nwant TAG_F1_ONLY to exist?\n\nIf so, instead of grepping around, we should be testing that in a\nmore direct way, perhaps with something like\n\n\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n\tgit show-ref --verify refs/tags/TAG_F1_2 &&\n\ttest_must_fail \tgit show-ref --verify refs/tags/TAG_F1_ONLY &&\n\nno?\n\n> \t\tcd main &&\n> \n> @@ -208,7 +209,8 @@ test_expect_success 'use git config to enable import/export of tags' '\n> \t\tgit p4 rebase --verbose &&\n> \t\tgit p4 submit --verbose &&\n> \t\tgit tag &&\n> -\t\tgit tag | grep TAG_F1_1\n> +\t\tgit tag >output &&\n> +\t\tgrep TAG_F1_1 output\n> \t) &&\n> \t(\n> \t\tcd \"$cli\" &&\n"},{"id":"515788","messageId":"20250407172504.50849-1-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250405103718.25160-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v3 0/3] t9811: Improve test coverage and clarity","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-07T17:25:01Z","receivedAt":"2025-04-07T17:25:10Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"In order to expose more testing outputs, we remove the piping of `git tag` \noutputs in order to expose the exit codes. In addition, we remove `-q` \ntags on instances of `grep` to ensure clarity. We also replace `grep` \nwith `test_grep` to provide helpful debug output in case of test failure.\n\n--------------\n\nchanges in v3:\n- patch #1 and #2 were missing my sign-off, which has now been added.\n- patch #2 referenced a line number, which was not informative. A new \n discription has been added referencing the context of the code.\n\nchanges in v2:\n- patch #2 and #3 have been added to reduce confusion caused by the \n implications of `grep` followed by `grep -q`, and increase debug \n output.\n\n\n t/t9811-git-p4-label-import.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515789","messageId":"20250407172504.50849-2-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250407172504.50849-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v3 1/3] t9811: avoid using pipes to expose exit codes","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-07T17:25:02Z","receivedAt":"2025-04-07T17:25:12Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"The exit code of the upstream in a pipe is suppressed\nthus we lose any exit codes of git commands that are piped. In order to\nensure we pick up the exit code, we can write the output of the git command\nto a file, testing the exit codes of both the commands.\n\nSigned-off-by: Anthony Wang \n---\n t/t9811-git-p4-label-import.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 5ac5383fb7..5abac938d0 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '\n \t\tcd \"$git\" &&\n \t\tgit p4 sync --import-labels &&\n \n-\t\tgit tag | grep TAG_F1 &&\n-\t\tgit tag | grep -q TAG_F1_1 &&\n-\t\tgit tag | grep -q TAG_F1_2 &&\n+\t\tgit tag >output &&\n+\t\tgrep TAG_F1 output &&\n+\t\tgrep -q TAG_F1_1 output &&\n+\t\tgrep -q TAG_F1_2 output &&\n \n \t\tcd main &&\n \n@@ -208,7 +209,8 @@ test_expect_success 'use git config to enable import/export of tags' '\n \t\tgit p4 rebase --verbose &&\n \t\tgit p4 submit --verbose &&\n \t\tgit tag &&\n-\t\tgit tag | grep TAG_F1_1\n+\t\tgit tag >output &&\n+\t\tgrep TAG_F1_1 output\n \t) &&\n \t(\n \t\tcd \"$cli\" &&\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515790","messageId":"20250407172504.50849-3-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250407172504.50849-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v3 2/3] t9811: Remove the -q quiet mode from some instances of grep","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-07T17:25:03Z","receivedAt":"2025-04-07T17:25:15Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"Remove the `-q` quiet mode from some instances of `grep`,\nas the lack of `-q` on the \"TAG_F1\" `grep` implies that its output is\nrequired, when that is not the case. This change ensures consistency and\navoids confusion about whether the output of `grep` is used.\n\nSigned-off-by: Anthony Wang \n---\n t/t9811-git-p4-label-import.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 5abac938d0..e69dae55dc 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -97,8 +97,8 @@ test_expect_success 'two labels on the same changelist' '\n \n \t\tgit tag >output &&\n \t\tgrep TAG_F1 output &&\n-\t\tgrep -q TAG_F1_1 output &&\n-\t\tgrep -q TAG_F1_2 output &&\n+\t\tgrep TAG_F1_1 output &&\n+\t\tgrep TAG_F1_2 output &&\n \n \t\tcd main &&\n \n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515791","messageId":"20250407172504.50849-4-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250407172504.50849-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v3 3/3] t9811: Change `grep` to `test_grep` for debug output","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-07T17:25:04Z","receivedAt":"2025-04-07T17:25:17Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"Change `grep` to `test_grep` to provide helpful debug\noutput in case of test failure by printing contents of the non-matching file.\n\nSigned-off-by: Anthony Wang \n---\n t/t9811-git-p4-label-import.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex e69dae55dc..e9c2aad2aa 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -96,9 +96,9 @@ test_expect_success 'two labels on the same changelist' '\n \t\tgit p4 sync --import-labels &&\n \n \t\tgit tag >output &&\n-\t\tgrep TAG_F1 output &&\n-\t\tgrep TAG_F1_1 output &&\n-\t\tgrep TAG_F1_2 output &&\n+\t\ttest_grep TAG_F1 output &&\n+\t\ttest_grep TAG_F1_1 output &&\n+\t\ttest_grep TAG_F1_2 output &&\n \n \t\tcd main &&\n \n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515803","messageId":"20250407212834.53183-1-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"xmqqo6x7nb2a.fsf@gitster.g","subject":"Re: [GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-07T21:28:34Z","receivedAt":"2025-04-07T21:28:39Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"From: Anthony Wang \n\n\nOn Mon, Apr 7, 2025 at 7:20 PM Junio C Hamano wrote:\n> Anthony Wang writes:\n> \n> > The exit code of the upstream in a pipe is suppressed\n> > thus we lose any exit codes of git commands that are piped. In order to\n> > ensure we pick up the exit code, we can write the output of the git command\n> > to a file, testing the exit codes of both the commands.\n> \n> Sort of correct, but ...\n\nWould it be more correct to say that the shell only returns the exit code of the \nlast command in the pipeline? Then, by writing out the output of the git command \nto a file, we can test the exit codes of both the commands? If not, I am not \nquite sure what the additional nuance is in this case.\n\n> \n> > ---\n> > t/t9811-git-p4-label-import.sh | 10 ++++++----\n> > 1 file changed, 6 insertions(+), 4 deletions(-)\n> \n> Missing sign-off.\n> \n> > diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n> > index 5ac5383fb7..5abac938d0 100755\n> > --- a/t/t9811-git-p4-label-import.sh\n> > +++ b/t/t9811-git-p4-label-import.sh\n> > @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '\n> > cd \"$git\" &&\n> > git p4 sync --import-labels &&\n> > \n> > - git tag | grep TAG_F1 &&\n> > - git tag | grep -q TAG_F1_1 &&\n> > - git tag | grep -q TAG_F1_2 &&\n> > + git tag >output &&\n> > + grep TAG_F1 output &&\n> > + grep -q TAG_F1_1 output &&\n> > + grep -q TAG_F1_2 output &&\n> \n> Think what these tests are trying to do. After a \"git p4 sync\"\n> operation, they want to ensure that tags TAG_F1_1 and TAG_F1_2\n> exist? Does the test want to see a tag \"TAG_F1\", or is it only that\n> the test is written in a so sloppy way that grepping for TAG_F1 will\n> be happy when any one of TAG_F1_1, TAG_F1_2 and TAG_F1_ONLY exists,\n> making its purpose of verifying that the tags are in the expected\n> state pretty much useless, and that is the reason why it needs to be\n> followed up with the two extra tests?\n\nI see, it seems I didn't fully consider the goals of the test. It does\nindeed seem like the test has some redundancies. If I am understanding\ncorrectly, the test grepping for TAG_F1 is not even necessary, and it\nseems like the tests for TAG_F1_1, TAG_F1_2 were added afterwards to\nensure that grep TAG_F1 was not passing when TAG_F1_ONLY existed. If\nthis is the case, would it be better to just remove the grep TAG_F1 test\nand just keep the two tests for TAG_F1_1 and TAG_F1_2? This would\nsimplify the test and make it clearer that we are only interested in\nthe existence of those two tags.\n\n> \n> What is the desired state you want to ensure after \"git p4 sync\"\n> operation above? I do not do \"git p4\", but you may know better than\n> I do, as you are the one who is patching this test file ;-) I am\n> guessing that you want TAG_F1_1 and TAG_F1_2 to exist and you do not\n> want TAG_F1_ONLY to exist?\n> \n> If so, instead of grepping around, we should be testing that in a\n> more direct way, perhaps with something like\n> \n> git show-ref --verify refs/tags/TAG_F1_1 &&\n> git show-ref --verify refs/tags/TAG_F1_2 &&\n> test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n> \n> no?\n> \n\nPossibly, but I believe adding the test_must_fail check would be modifying\nthe original intent of the test, as it would pass even with the existence of\nTAG_F1_ONLY. However, if we are only performing actions to cause TAG_F1_1 \nand TAG_F1_2 to exist, then it would be an issue if TAG_F1_ONLY existed.\n\nMy original plan was not to change any test coverage, and to only make small\nchanges following the goals of the microproject, but if you think it would\nbe beneficial to change the test to check for the absence of TAG_F1_ONLY,\nthen I will proceed.\n\n> > cd main &&\n> > \n> > @@ -208,7 +209,8 @@ test_expect_success 'use git config to enable import/export of tags' '\n> > git p4 rebase --verbose &&\n> > git p4 submit --verbose &&\n> > git tag &&\n> > - git tag | grep TAG_F1_1\n> > + git tag >output &&\n> > + grep TAG_F1_1 output\n> > ) &&\n> > (\n> > cd \"$cli\" &&\n"},{"id":"515806","messageId":"xmqqecy3lj0f.fsf@gitster.g","threadId":"63258","inReplyTo":"CAPig+cQbMUQAHsx92QNLAvfkUKRFoVoWznhPQok0PN6HW3wxzQ@mail.gmail.com","subject":"Re: [GSoC] [PATCH v2 2/3] t9811: Remove the -q quiet mode from some instances of grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-07T22:11:12Z","receivedAt":"2025-04-07T22:11:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> For such a simple change, it probably would be more common on this\n> project to combine patches [2/3] (which drops `-q`) and [3/3] (which\n> replaces `grep` with `test_grep`), and to simply explain as a\n> side-note in the commit message of the combined patch why `-q` is\n> being dropped.\n\nTrue, and in this case, I'd say this is better done as a single\npatch to go directly to \"show-ref --verify\" that loses an extra\nexternal command per tag to kill three birds with a single patch.\n(1) lose the problem of pipe hiding the status, (2) clearly say\nwhat is expected to exist and not to exist, and (3) futureproof\nthe test so that new tags with similar names (e.g. earlier steps\nmay be updated to start creating tag T_TAG_F1_1, and grep would\nhappily report a partial match with TAG_F1_1) will not make the\nverification invalid.\n\nThanks.\n\n\n\n"},{"id":"515811","messageId":"xmqqtt6zjyma.fsf@gitster.g","threadId":"63258","inReplyTo":"20250407212834.53183-1-anthonywang03@icloud.com","subject":"Re: [GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-08T00:17:01Z","receivedAt":"2025-04-08T00:17:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anthony Wang writes:\n\n>> If so, instead of grepping around, we should be testing that in a\n>> more direct way, perhaps with something like\n>> \n>> git show-ref --verify refs/tags/TAG_F1_1 &&\n>> git show-ref --verify refs/tags/TAG_F1_2 &&\n>> test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n>> \n>> no?\n>> \n>\n> Possibly, but I believe adding the test_must_fail check would be modifying\n> the original intent of the test, as it would pass even with the existence of\n> TAG_F1_ONLY. However, if we are only performing actions to cause TAG_F1_1 \n> and TAG_F1_2 to exist, then it would be an issue if TAG_F1_ONLY existed.\n\nI view it a bit differently.\n\nUse of \"grep\" over the output of \"git tag\" is simply a sloppy\nprogramming. If the test wanted to verify \"TAG_F1_1 exists\", it\nshouldn't have grepped for TAG_F1_1, because another tag T_TAG_F1_1\nwould produce a false positive hit if the earlier test gets updated.\n\nSimilarly, not verifying what should not exist is being sloppy.\nPeople who come up with a new feature (in this case, \"git p4 sync\"\ninvolving tags) tend to test positive effects to show how their\nshiny new toy does things, and forgets to test lack of effects to\nensure that their shiny new toy does *not* do what they should not\ndo.\n\nIf the original test were written solidly and use of pipe hiding\nexit code were the only problem it had, I would agree that making\nminimum change should be preferrable, but the original test seems to\nbe so sloppy in this case.\n\nThanks.\n"},{"id":"515831","messageId":"CAOSofofS311fMqnDpyFpUVC8vYcTcNy0EwKfvxfzEXWBGWvRkQ@mail.gmail.com","threadId":"63258","inReplyTo":"xmqqtt6zjyma.fsf@gitster.g","subject":"Re: [GSoC] [PATCH v2 1/3] t9811: avoid using pipes to expose exit codes","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-08T07:35:48Z","receivedAt":"2025-04-08T07:36:00Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"On Tue, Apr 8, 2025 at 2:17 AM Junio C Hamano wrote:\n>\n> Anthony Wang writes:\n>\n> >> If so, instead of grepping around, we should be testing that in a\n> >> more direct way, perhaps with something like\n> >>\n> >> git show-ref --verify refs/tags/TAG_F1_1 &&\n> >> git show-ref --verify refs/tags/TAG_F1_2 &&\n> >> test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n> >>\n> >> no?\n> >>\n> >\n> > Possibly, but I believe adding the test_must_fail check would be modifying\n> > the original intent of the test, as it would pass even with the existence of\n> > TAG_F1_ONLY. However, if we are only performing actions to cause TAG_F1_1\n> > and TAG_F1_2 to exist, then it would be an issue if TAG_F1_ONLY existed.\n>\n> I view it a bit differently.\n>\n> Use of \"grep\" over the output of \"git tag\" is simply a sloppy\n> programming. If the test wanted to verify \"TAG_F1_1 exists\", it\n> shouldn't have grepped for TAG_F1_1, because another tag T_TAG_F1_1\n> would produce a false positive hit if the earlier test gets updated.\n>\n> Similarly, not verifying what should not exist is being sloppy.\n> People who come up with a new feature (in this case, \"git p4 sync\"\n> involving tags) tend to test positive effects to show how their\n> shiny new toy does things, and forgets to test lack of effects to\n> ensure that their shiny new toy does *not* do what they should not\n> do.\n>\n\nI see, I agree that the test is written just to check that the feature\ndoes the intended thing, and not properly written as a tests. I will\nmake the changes and submit a new version.\n\n> If the original test were written solidly and use of pipe hiding\n> exit code were the only problem it had, I would agree that making\n> minimum change should be preferrable, but the original test seems to\n> be so sloppy in this case.\n>\n\nTo this point, I have a question about when to modify code when making\npatches. My understanding is that we should try to only modify the code\nneccesary to fix the bug, and not modify other parts of the code.\nHowever, because in this case the test itself does not correctly test\nfor the intended behavior, we should modify because we are already\ntouching this piece of code. Is this correct? Would it then be desired\nto check the rest of the tests in this file for further oversights and\ncorrect them as well, or would that be overstepping boundaries? Sorry\nfor the questions, I just want to understand the best practices as well\nas I can.\n\nThanks,\nAnthony\n"},{"id":"515832","messageId":"20250408080802.56341-1-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250405103718.25160-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v4 0/1] t9811: Improve test coverage and clarity","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-08T08:08:01Z","receivedAt":"2025-04-08T08:08:22Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"In order to expose more testing outputs, we remove the piping of `git tag` \noutputs in order to expose the exit codes. In addition, we change the \nusage of `grep` to `git show-ref --verify` to check for the existence of\nexpected tags in a cleaner manner, preventing false positives. We also\ncheck to make sure TAG_F1_ONLY does not exist, as it is not expected to be \ncreated in this test.\n\n--------------\nchanges in v4:\n- changed `grep` to `git show-ref --verify` to check for the existence of \n tags, and added a test_must_fail check for the unexpected tag.\n- consolidated the three commits into one, as the changes were not \n significant enough to warrant three separate commits.\n- added a new commit message to clarify the changes made.\n\nchanges in v3:\n- patch #1 and #2 were missing my sign-off, which has now been added.\n- patch #2 referenced a line number, which was not informative. A new \n discription has been added referencing the context of the code.\n\nchanges in v2:\n- patch #2 and #3 have been added to reduce confusion caused by the \n implications of `grep` followed by `grep -q`, and increase debug \n output.\n\n\n t/t9811-git-p4-label-import.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515833","messageId":"20250408080802.56341-2-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250408080802.56341-1-anthonywang03@icloud.com","subject":"[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.","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-08T08:08:02Z","receivedAt":"2025-04-08T08:08:24Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"Signed-off-by: Anthony Wang \n---\n t/t9811-git-p4-label-import.sh | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 5ac5383fb7..cd06f39519 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '\n \t\tcd \"$git\" &&\n \t\tgit p4 sync --import-labels &&\n \n-\t\tgit tag | grep TAG_F1 &&\n-\t\tgit tag | grep -q TAG_F1_1 &&\n-\t\tgit tag | grep -q TAG_F1_2 &&\n+\t\tgit tag &&\n+\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n+ git show-ref --verify refs/tags/TAG_F1_2 &&\n+ test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n \n \t\tcd main &&\n \n@@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n \t\tgit p4 rebase --verbose &&\n \t\tgit p4 submit --verbose &&\n \t\tgit tag &&\n-\t\tgit tag | grep TAG_F1_1\n+\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n \t) &&\n \t(\n \t\tcd \"$cli\" &&\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515847","messageId":"CAOLa=ZQPgSNOAnhc8AHwzkgphz3RCF1YkPdC9LKPi8Hnhz4ijg@mail.gmail.com","threadId":"63258","inReplyTo":"20250408080802.56341-2-anthonywang03@icloud.com","subject":"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.","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-04-08T09:55:57Z","receivedAt":"2025-04-08T09:55:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Anthony Wang writes:\n\nSeems like the subject and message is combined together?\n\n> Signed-off-by: Anthony Wang \n> ---\n> t/t9811-git-p4-label-import.sh | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n>\n\nA quick check shows that there is a whitespace issue in this patch when\napplied on top of master 9d22ac5122 (The third batch, 2025-04-07):\n\n9d22ac51228304102deb62f30c3ecba6377e1237\n\n--- 92f98eb326 Remove the pipe following the `git tag`, ensuring the\nexit code is not hidden. Add explicit verification to check for\nexpected and unexpected tags, increasing specificity and\nfuture-proofing a portion of the test.\nt/t9811-git-p4-label-import.sh:100: indent with spaces.\n+ git show-ref --verify refs/tags/TAG_F1_2 &&\nt/t9811-git-p4-label-import.sh:101: indent with spaces.\n+ test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\nA whitespace issue was found in one or more of the commits.\nRun the following command to resolve whitespace issues:\ngit rebase --whitespace=fix @~1\n\n> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n> index 5ac5383fb7..cd06f39519 100755\n> --- a/t/t9811-git-p4-label-import.sh\n> +++ b/t/t9811-git-p4-label-import.sh\n> @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '\n> \t\tcd \"$git\" &&\n> \t\tgit p4 sync --import-labels &&\n>\n> -\t\tgit tag | grep TAG_F1 &&\n> -\t\tgit tag | grep -q TAG_F1_1 &&\n> -\t\tgit tag | grep -q TAG_F1_2 &&\n> +\t\tgit tag &&\n> +\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n> + git show-ref --verify refs/tags/TAG_F1_2 &&\n> + test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n>\n\nLooks like this is the whitespace issue.\n\n> \t\tcd main &&\n>\n> @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n> \t\tgit p4 rebase --verbose &&\n> \t\tgit p4 submit --verbose &&\n> \t\tgit tag &&\n> -\t\tgit tag | grep TAG_F1_1\n> +\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n> \t) &&\n> \t(\n> \t\tcd \"$cli\" &&\n> --\n> 2.39.5 (Apple Git-154)\n"},{"id":"515860","messageId":"20250408114841.58592-1-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250405103718.25160-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v5 0/1] t9811: Improve test coverage and clarity","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-08T11:48:40Z","receivedAt":"2025-04-08T11:48:47Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"In order to expose more testing outputs, we remove the piping of `git tag` \noutputs in order to expose the exit codes. In addition, we change the \nusage of `grep` to `git show-ref --verify` to check for the existence of\nexpected tags in a cleaner manner, preventing false positives. We also\ncheck to make sure TAG_F1_ONLY does not exist, as it is not expected to be \ncreated in this test.\n\n--------------\nchanges in v5:\n- moved commit message to message body, and fixed subject line\n- fixed whitespace issues in patch #1\n\nchanges in v4:\n- changed `grep` to `git show-ref --verify` to check for the existence of \n tags, and added a test_must_fail check for the unexpected tag.\n- consolidated the three commits into one, as the changes were not \n significant enough to warrant three separate commits.\n- added a new commit message to clarify the changes made.\n\nchanges in v3:\n- patch #1 and #2 were missing my sign-off, which has now been added.\n- patch #2 referenced a line number, which was not informative. A new \n discription has been added referencing the context of the code.\n\nchanges in v2:\n- patch #2 and #3 have been added to reduce confusion caused by the \n implications of `grep` followed by `grep -q`, and increase debug \n output.\n\n t/t9811-git-p4-label-import.sh | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515861","messageId":"20250408114841.58592-2-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250408114841.58592-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-08T11:48:41Z","receivedAt":"2025-04-08T11:48:49Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"Remove the pipe following the `git tag`, ensuring the exit code is not \nhidden. Add explicit verification to check for expected and unexpected \ntags, increasing specificity and future-proofing a portion of the test.\n\n---\n t/t9811-git-p4-label-import.sh | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 5ac5383fb7..593de09eb4 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '\n \t\tcd \"$git\" &&\n \t\tgit p4 sync --import-labels &&\n \n-\t\tgit tag | grep TAG_F1 &&\n-\t\tgit tag | grep -q TAG_F1_1 &&\n-\t\tgit tag | grep -q TAG_F1_2 &&\n+\t\tgit tag &&\n+\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n+\t\tgit show-ref --verify refs/tags/TAG_F1_2 &&\n+\t\ttest_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n \n \t\tcd main &&\n \n@@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n \t\tgit p4 rebase --verbose &&\n \t\tgit p4 submit --verbose &&\n \t\tgit tag &&\n-\t\tgit tag | grep TAG_F1_1\n+\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n \t) &&\n \t(\n \t\tcd \"$cli\" &&\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"515897","messageId":"xmqqbjt6wdrk.fsf@gitster.g","threadId":"63258","inReplyTo":"20250408114841.58592-2-anthonywang03@icloud.com","subject":"Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-08T21:21:19Z","receivedAt":"2025-04-08T21:21:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anthony Wang writes:\n\n> Subject: Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity\n\n\"Improve\" -> \"improve\".\n\nI am not sure about \"clarity\", but I agree that the main thrust of\nthis change is no longer \"avoid hiding exit status of git behind\npipe\", but more about \"use 'show-ref --verify' to validate the right\nthing (meaning: grepping just for a string that could be a substring\nis an unreliable test)\". Perhaps that is giving more clarity?\ncoverage to test negative outcome is \"while at it\", just like we\n(incidentally) lost pipes that used to hide exit status, so I am not\nsure if it deserves mention on the commit title.\n\n t9811: be more precise to check tag creation\n\n The tests grep tagnames they expect to exist from \"git tag\"\n output, which can be fooled by false positive if an unexpected\n tag whose name has the expected tagname as its substring. Fix\n them by using \"git show-ref --verify\" instead.\n\n While we are at it, add a negative test to verify that a tag\n that is involved in earlier tests that is not supposed to appear\n in the result does indeed not appear in the resulting\n repository.\n\n Incidentally, this would also correct the problem the original\n had, which lost the exit status of \"git tag\" that was placed\n upstream of a pipe.\n\nor something, perhaps?\n\nAlso you'd need to sign-off your patch.\n\nThanks.\n\n> Remove the pipe following the `git tag`, ensuring the exit code is not \n> hidden. Add explicit verification to check for expected and unexpected \n> tags, increasing specificity and future-proofing a portion of the test.\n>\n> ---\n> t/t9811-git-p4-label-import.sh | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n> index 5ac5383fb7..593de09eb4 100755\n> --- a/t/t9811-git-p4-label-import.sh\n> +++ b/t/t9811-git-p4-label-import.sh\n> @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' '\n> \t\tcd \"$git\" &&\n> \t\tgit p4 sync --import-labels &&\n> \n> -\t\tgit tag | grep TAG_F1 &&\n> -\t\tgit tag | grep -q TAG_F1_1 &&\n> -\t\tgit tag | grep -q TAG_F1_2 &&\n> +\t\tgit tag &&\n> +\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n> +\t\tgit show-ref --verify refs/tags/TAG_F1_2 &&\n> +\t\ttest_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n> \n> \t\tcd main &&\n> \n> @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n> \t\tgit p4 rebase --verbose &&\n> \t\tgit p4 submit --verbose &&\n> \t\tgit tag &&\n> -\t\tgit tag | grep TAG_F1_1\n> +\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n> \t) &&\n> \t(\n> \t\tcd \"$cli\" &&\n"},{"id":"515898","messageId":"CAPig+cSo2hPpA0DOn7Tr3pcYfN6jxyyo+n0Y0QetvKvNVoLqbA@mail.gmail.com","threadId":"63258","inReplyTo":"xmqqbjt6wdrk.fsf@gitster.g","subject":"Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-04-08T21:28:38Z","receivedAt":"2025-04-08T21:28:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 8, 2025 at 5:21 PM Junio C Hamano wrote:\n> t9811: be more precise to check tag creation\n>\n> The tests grep tagnames they expect to exist from \"git tag\"\n\ns/tagnames/tag names/ perhaps?\n\n> output, which can be fooled by false positive if an unexpected\n> tag whose name has the expected tagname as its substring. Fix\n> them by using \"git show-ref --verify\" instead.\n>\n> While we are at it, add a negative test to verify that a tag\n> that is involved in earlier tests that is not supposed to appear\n> in the result does indeed not appear in the resulting\n> repository.\n>\n> Incidentally, this would also correct the problem the original\n> had, which lost the exit status of \"git tag\" that was placed\n> upstream of a pipe.\n>\n> or something, perhaps?\n\nYes, better and much more illuminating.\n\n> > - git tag | grep TAG_F1 &&\n> > - git tag | grep -q TAG_F1_1 &&\n> > - git tag | grep -q TAG_F1_2 &&\n> > + git tag &&\n> > + git show-ref --verify refs/tags/TAG_F1_1 &&\n> > + git show-ref --verify refs/tags/TAG_F1_2 &&\n> > + test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n\nDo we still need the standalone `git tag` invocation above?\n\n> > @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n> > git p4 submit --verbose &&\n> > git tag &&\n> > - git tag | grep TAG_F1_1\n> > + git show-ref --verify refs/tags/TAG_F1_1 &&\n\nSimilarly, it's not clear why there is a standalone `git tag`\ninvocation here. Does it buy us anything or am I missing something\nobvious? The originating commit[*] doesn't explain its purpose.\n\n[*] e71f6a53e2 (git p4: add test for tag import/export enabled via\nconfig, 2012-05-11)\n"},{"id":"515906","messageId":"CAOSofofyfDpFqFzApwMnmYdFb4nk8DS7WwWmYKLPbe+okOE=Tw@mail.gmail.com","threadId":"63258","inReplyTo":"CAPig+cSo2hPpA0DOn7Tr3pcYfN6jxyyo+n0Y0QetvKvNVoLqbA@mail.gmail.com","subject":"Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-09T08:15:53Z","receivedAt":"2025-04-09T08:16:06Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"On Tue, Apr 8, 2025 at 11:28 PM Eric Sunshine wrote:\n>\n> On Tue, Apr 8, 2025 at 5:21 PM Junio C Hamano wrote:\n> > t9811: be more precise to check tag creation\n> >\n> > The tests grep tagnames they expect to exist from \"git tag\"\n>\n> s/tagnames/tag names/ perhaps?\n\nHow does \"t9811: be more precise to check importing of tags\" sound?\nThe tags are created in the p4 repository, and what we care about is if\n`git p4 sync --import-labels` correctly imports the labels from the p4\nrepository. Specifying tags instead of tag indicates that there are\nmultiple tags, which is the whole purpose of the test.\n\n>\n> > output, which can be fooled by false positive if an unexpected\n> > tag whose name has the expected tagname as its substring. Fix\n> > them by using \"git show-ref --verify\" instead.\n> >\n> > While we are at it, add a negative test to verify that a tag\n> > that is involved in earlier tests that is not supposed to appear\n> > in the result does indeed not appear in the resulting\n> > repository.\n> >\n> > Incidentally, this would also correct the problem the original\n> > had, which lost the exit status of \"git tag\" that was placed\n> > upstream of a pipe.\n> >\n> > or something, perhaps?\n>\n> Yes, better and much more illuminating.\n\nI see, that message does communicate the intentions of the changes\nmuch better than I did. I will expand the message and write something\nalong the lines of what you have recommended.\n\n>\n> > > - git tag | grep TAG_F1 &&\n> > > - git tag | grep -q TAG_F1_1 &&\n> > > - git tag | grep -q TAG_F1_2 &&\n> > > + git tag &&\n> > > + git show-ref --verify refs/tags/TAG_F1_1 &&\n> > > + git show-ref --verify refs/tags/TAG_F1_2 &&\n> > > + test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n>\n> Do we still need the standalone `git tag` invocation above?\n\nThe original intent of the patch was to expose the exit code of\n`git tag`. By keeping the standalone `git tag` we are able to pick up\nthe exit code if there are any issues, such as if `git p4 sync\n--import-labels` somehow breaks `git tag`. I believe this is the intent\nof the standalone `git tag` in the section below, and as such, I added\nit to the top section in order to test the `git tag` functionality, as we\nremoved the other `git tag` instances. However, I can understand\nif it is unnecessary, and I will remove it.\n\n>\n> > > @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n> > > git p4 submit --verbose &&\n> > > git tag &&\n> > > - git tag | grep TAG_F1_1\n> > > + git show-ref --verify refs/tags/TAG_F1_1 &&\n>\n> Similarly, it's not clear why there is a standalone `git tag`\n> invocation here. Does it buy us anything or am I missing something\n> obvious? The originating commit[*] doesn't explain its purpose.\n>\n\nAgain, as we have removed the other `git tag` instances, it might\nstill have value to test the result of running the `git tag` command.\n\n> [*] e71f6a53e2 (git p4: add test for tag import/export enabled via\n> config, 2012-05-11)\n\nThank you very much for the valuable feedback, it has been very\neye opening to see how you two approach patching code like this.\nI wanted to bump one of my previous questions, as I am very\ncurious about what the best practice is here:\n\nTo this point, I have a question about when to modify code when making\npatches. My understanding is that we should try to only modify the code\nnecessary to fix the bug, and not modify other parts of the code.\nHowever, because in this case the test itself does not correctly test\nfor the intended behavior, we should modify because we are already\ntouching this piece of code. Is this correct? Would it then be desired\nto check the rest of the tests in this file for further oversights and\ncorrect them as well, or would that be overstepping boundaries?\n"},{"id":"515913","messageId":"xmqqa58ptqd9.fsf@gitster.g","threadId":"63258","inReplyTo":"CAOSofofyfDpFqFzApwMnmYdFb4nk8DS7WwWmYKLPbe+okOE=Tw@mail.gmail.com","subject":"Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-09T13:29:38Z","receivedAt":"2025-04-09T13:29:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anthony Wang writes:\n\n>> > The tests grep tagnames they expect to exist from \"git tag\"\n>>\n>> s/tagnames/tag names/ perhaps?\n>\n> How does \"t9811: be more precise to check importing of tags\" sound?\n\nExcellent.\n\n>> > > + git tag &&\n>> > > + git show-ref --verify refs/tags/TAG_F1_1 &&\n>> > > + git show-ref --verify refs/tags/TAG_F1_2 &&\n>> > > + test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n>>\n>> Do we still need the standalone `git tag` invocation above?\n>\n> The original intent of the patch was to expose the exit code of\n> `git tag`.\n\nIs it? I somehow thought that \"git tag\" is not what is being tested\nby this script. Rather, it assumed that \"git tag\" works perfectly\nwell, and validated what \"git p4\" left in the resulting repository\nbased on that assumption, i.e. \"git tag\" works perfectly well to\ntell us what tags are in the repository.\n\nIt is true that \"git tag\" to list all available tags may fail, but\nthen catching that is outside the scope of this script no? It is\neven more so, since now we do not even depend on the correct\noperation of \"git tag\" anymore to validate what \"git p4\" did---we\nnow use \"show-ref --verify\" for that, so we do not even care if \"git\ntag\" segfaults in this part of the test, no?\n\n> However, because in this case the test itself does not correctly test\n> for the intended behavior, we should modify because we are already\n> touching this piece of code. Is this correct? Would it then be desired\n> to check the rest of the tests in this file for further oversights and\n> correct them as well, or would that be overstepping boundaries?\n\nJust like any real world problems, there unfortunately is no bright\nred line between \"yeah these are related enough and in the same spot\nand it is better to clean up while we are at it\" and \"that is way\ntoo much for this single topic\" that can be described in a textbook.\nA rule of thumb I personally use is to put me in the shoes of an\nimaginary typical Git developer with moderate competence, who hasn't\nseen or worked on a particular part of the system being updated. If\nI can easily imagine that the developer can clearly see a need for\nclean up (in this case, \"the part of the code only tests positive\nresults and forgets about negative check\") while fixing something\nelse (in this case, \"use of 'git tag' piped to 'grep' has at least\ntwo problems, loss of exit code and false match\") and the additional\neffort would be smaller than 10-20 minutes, I'd say it would be\nworth doing and anything larger would be better to leave to another\nday, but a lot of ingredients in that statement are very much\nsubjective (starting from \"what's the average competence level we\nexpect from our people?\").\n\n"},{"id":"516027","messageId":"20250412061957.81747-1-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250405103718.25160-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v6 0/1] t9811: be more precise to check importing of tags","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-12T06:19:56Z","receivedAt":"2025-04-12T06:20:04Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"In order to expose more testing outputs, we remove the piping of `git tag` \noutputs in order to expose the exit codes. In addition, we change the \nusage of `grep` to `git show-ref --verify` to check for the existence of\nexpected tags in a cleaner manner, preventing false positives. We also\ncheck to make sure TAG_F1_ONLY does not exist, as it is not expected to be \ncreated in this test.\n\n--------------\nchanges in v6:\n- removed unneccesary calls to `git tag`\n- changed commit message to better describe rationale for changes\n- added missing sign-off to patch #1\n\nchanges in v5:\n- moved commit message to message body, and fixed subject line\n- fixed whitespace issues in patch #1\n\nchanges in v4:\n- changed `grep` to `git show-ref --verify` to check for the existence of \n tags, and added a test_must_fail check for the unexpected tag.\n- consolidated the three commits into one, as the changes were not \n significant enough to warrant three separate commits.\n- added a new commit message to clarify the changes made.\n\nchanges in v3:\n- patch #1 and #2 were missing my sign-off, which has now been added.\n- patch #2 referenced a line number, which was not informative. A new \n discription has been added referencing the context of the code.\n\nchanges in v2:\n- patch #2 and #3 have been added to reduce confusion caused by the \n implications of `grep` followed by `grep -q`, and increase debug \n output.\n\n t/t9811-git-p4-label-import.sh | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"516028","messageId":"20250412061957.81747-2-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250412061957.81747-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v6 1/1] t9811: be more precise to check importing of tags","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-12T06:19:57Z","receivedAt":"2025-04-12T06:20:09Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"The tests use grep to search the output of `git tag` for tagnames they \nexpect to exist, which can incorrectly pass if an unxpected tag\nhas the expected tag as its substring. We fix this by using `git \nshow-ref --verify` instead.\n\nAdditionally, we add a negative test to verify that a possible\nuninteded tag does not show up in the imported repository.\n\nThis change also fixes the original problem, where piping the\noutput of `git tag` caused the exit codes to be lost.\n\nSigned-off-by: Anthony Wang \n---\n t/t9811-git-p4-label-import.sh | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 5ac5383fb7..39856629c0 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -95,9 +95,9 @@ test_expect_success 'two labels on the same changelist' '\n \t\tcd \"$git\" &&\n \t\tgit p4 sync --import-labels &&\n \n-\t\tgit tag | grep TAG_F1 &&\n-\t\tgit tag | grep -q TAG_F1_1 &&\n-\t\tgit tag | grep -q TAG_F1_2 &&\n+\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n+\t\tgit show-ref --verify refs/tags/TAG_F1_2 &&\n+\t\ttest_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n \n \t\tcd main &&\n \n@@ -207,8 +207,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n \t\tgit tag CFG_A_GIT_TAG &&\n \t\tgit p4 rebase --verbose &&\n \t\tgit p4 submit --verbose &&\n-\t\tgit tag &&\n-\t\tgit tag | grep TAG_F1_1\n+\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n \t) &&\n \t(\n \t\tcd \"$cli\" &&\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"516200","messageId":"xmqqjz7lo4o4.fsf@gitster.g","threadId":"63258","inReplyTo":"20250412061957.81747-2-anthonywang03@icloud.com","subject":"Re: [GSoC] [PATCH v6 1/1] t9811: be more precise to check importing of tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-15T14:55:23Z","receivedAt":"2025-04-15T14:55:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anthony Wang writes:\n\n> The tests use grep to search the output of `git tag` for tagnames they \n> expect to exist, which can incorrectly pass if an unxpected tag\n> has the expected tag as its substring. We fix this by using `git \n> show-ref --verify` instead.\n>\n> Additionally, we add a negative test to verify that a possible\n> uninteded tag does not show up in the imported repository.\n>\n> This change also fixes the original problem, where piping the\n> output of `git tag` caused the exit codes to be lost.\n\nThe word \"original\" is misleading; perhaps phase it as \"additional\"\ninstead?\n\nThat is because not allowing to notice potential breakage by hiding\nthe exit status behind pipes is just as bad as falsely taking a\npartial tagname match as success, and there is no reason to call one\n\"original\" problem, implying the other problem(s) are different.\n\nOther than that, looks excellent.\n\nThanks.\n\n\n> Signed-off-by: Anthony Wang \n> ---\n> t/t9811-git-p4-label-import.sh | 9 ++++-----\n> 1 file changed, 4 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n> index 5ac5383fb7..39856629c0 100755\n> --- a/t/t9811-git-p4-label-import.sh\n> +++ b/t/t9811-git-p4-label-import.sh\n> @@ -95,9 +95,9 @@ test_expect_success 'two labels on the same changelist' '\n> \t\tcd \"$git\" &&\n> \t\tgit p4 sync --import-labels &&\n> \n> -\t\tgit tag | grep TAG_F1 &&\n> -\t\tgit tag | grep -q TAG_F1_1 &&\n> -\t\tgit tag | grep -q TAG_F1_2 &&\n> +\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n> +\t\tgit show-ref --verify refs/tags/TAG_F1_2 &&\n> +\t\ttest_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n> \n> \t\tcd main &&\n> \n> @@ -207,8 +207,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n> \t\tgit tag CFG_A_GIT_TAG &&\n> \t\tgit p4 rebase --verbose &&\n> \t\tgit p4 submit --verbose &&\n> -\t\tgit tag &&\n> -\t\tgit tag | grep TAG_F1_1\n> +\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n> \t) &&\n> \t(\n> \t\tcd \"$cli\" &&\n"},{"id":"516292","messageId":"20250416145939.24207-1-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250405103718.25160-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v7 0/1] t9811: be more precise to check importing of tags","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-16T14:59:38Z","receivedAt":"2025-04-16T14:59:50Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"In order to expose more testing outputs, we remove the piping of `git tag` \noutputs in order to expose the exit codes. In addition, we change the \nusage of `grep` to `git show-ref --verify` to check for the existence of\nexpected tags in a cleaner manner, preventing false positives. We also\ncheck to make sure TAG_F1_ONLY does not exist, as it is not expected to be \ncreated in this test.\n\n--------------\nchanges in v7:\n- changed wording from \"the original problem\" to \"an additional problem\"\n as both problems are important, and there is not an \"original\" problem\n \nchanges in v6:\n- removed unneccesary calls to `git tag`\n- changed commit message to better describe rationale for changes\n- added missing sign-off to patch #1\n\nchanges in v5:\n- moved commit message to message body, and fixed subject line\n- fixed whitespace issues in patch #1\n\nchanges in v4:\n- changed `grep` to `git show-ref --verify` to check for the existence of \n tags, and added a test_must_fail check for the unexpected tag.\n- consolidated the three commits into one, as the changes were not \n significant enough to warrant three separate commits.\n- added a new commit message to clarify the changes made.\n\nchanges in v3:\n- patch #1 and #2 were missing my sign-off, which has now been added.\n- patch #2 referenced a line number, which was not informative. A new \n discription has been added referencing the context of the code.\n\nchanges in v2:\n- patch #2 and #3 have been added to reduce confusion caused by the \n implications of `grep` followed by `grep -q`, and increase debug \n output.\n\n t/t9811-git-p4-label-import.sh | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\n--\n2.39.5 (Apple Git-154)\n\n"},{"id":"516293","messageId":"20250416145939.24207-2-anthonywang03@icloud.com","threadId":"63258","inReplyTo":"20250416145939.24207-1-anthonywang03@icloud.com","subject":"[GSoC] [PATCH v7 1/1] t9811: be more precise to check importing of tags","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-16T14:59:39Z","receivedAt":"2025-04-16T14:59:52Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"The tests use grep to search the output of `git tag` for tagnames they \nexpect to exist, which can incorrectly pass if an unxpected tag\nhas the expected tag as its substring. We fix this by using `git \nshow-ref --verify` instead.\n\nAdditionally, we add a negative test to verify that a possible\nuninteded tag does not show up in the imported repository.\n\nThis change also fixes an additional problem, where piping the\noutput of `git tag` caused the exit codes to be lost.\n\nSigned-off-by: Anthony Wang \n---\n t/t9811-git-p4-label-import.sh | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 5ac5383fb7..39856629c0 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -95,9 +95,9 @@ test_expect_success 'two labels on the same changelist' '\n \t\tcd \"$git\" &&\n \t\tgit p4 sync --import-labels &&\n \n-\t\tgit tag | grep TAG_F1 &&\n-\t\tgit tag | grep -q TAG_F1_1 &&\n-\t\tgit tag | grep -q TAG_F1_2 &&\n+\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n+\t\tgit show-ref --verify refs/tags/TAG_F1_2 &&\n+\t\ttest_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n \n \t\tcd main &&\n \n@@ -207,8 +207,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n \t\tgit tag CFG_A_GIT_TAG &&\n \t\tgit p4 rebase --verbose &&\n \t\tgit p4 submit --verbose &&\n-\t\tgit tag &&\n-\t\tgit tag | grep TAG_F1_1\n+\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n \t) &&\n \t(\n \t\tcd \"$cli\" &&\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"516294","messageId":"CAOSofod9m7wHafP5=_tcMYW_oN9_icSmVX7rQioUkc_RC6pVYw@mail.gmail.com","threadId":"63258","inReplyTo":"xmqqjz7lo4o4.fsf@gitster.g","subject":"Re: [GSoC] [PATCH v6 1/1] t9811: be more precise to check importing of tags","fromName":"Anthony Wang","fromEmail":"anthonywang513@gmail.com","sentAt":"2025-04-16T15:03:55Z","receivedAt":"2025-04-16T15:04:08Z","isPatch":true,"sender":{"key":"anthonywang513@gmail.com","avatar":"https://avatars.githubusercontent.com/u/71832477?v=4"},"body":"On Tue, Apr 15, 2025 at 4:55 PM Junio C Hamano wrote:\n>\n> Anthony Wang writes:\n>\n> > The tests use grep to search the output of `git tag` for tagnames they\n> > expect to exist, which can incorrectly pass if an unxpected tag\n> > has the expected tag as its substring. We fix this by using `git\n> > show-ref --verify` instead.\n> >\n> > Additionally, we add a negative test to verify that a possible\n> > uninteded tag does not show up in the imported repository.\n> >\n> > This change also fixes the original problem, where piping the\n> > output of `git tag` caused the exit codes to be lost.\n>\n> The word \"original\" is misleading; perhaps phase it as \"additional\"\n> instead?\n>\n> That is because not allowing to notice potential breakage by hiding\n> the exit status behind pipes is just as bad as falsely taking a\n> partial tagname match as success, and there is no reason to call one\n> \"original\" problem, implying the other problem(s) are different.\n>\n> Other than that, looks excellent.\n>\n> Thanks.\n>\n\nChanged and resubmitted the patch. Thank you for the guidance on this\nmicroproject, I appreciate the help as I am still new to open-source, and\nI now understand the workflow and style of Git much better.\n\n>\n> > Signed-off-by: Anthony Wang \n> > ---\n> > t/t9811-git-p4-label-import.sh | 9 ++++-----\n> > 1 file changed, 4 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n> > index 5ac5383fb7..39856629c0 100755\n> > --- a/t/t9811-git-p4-label-import.sh\n> > +++ b/t/t9811-git-p4-label-import.sh\n> > @@ -95,9 +95,9 @@ test_expect_success 'two labels on the same changelist' '\n> > cd \"$git\" &&\n> > git p4 sync --import-labels &&\n> >\n> > - git tag | grep TAG_F1 &&\n> > - git tag | grep -q TAG_F1_1 &&\n> > - git tag | grep -q TAG_F1_2 &&\n> > + git show-ref --verify refs/tags/TAG_F1_1 &&\n> > + git show-ref --verify refs/tags/TAG_F1_2 &&\n> > + test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n> >\n> > cd main &&\n> >\n> > @@ -207,8 +207,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n> > git tag CFG_A_GIT_TAG &&\n> > git p4 rebase --verbose &&\n> > git p4 submit --verbose &&\n> > - git tag &&\n> > - git tag | grep TAG_F1_1\n> > + git show-ref --verify refs/tags/TAG_F1_1 &&\n> > ) &&\n> > (\n> > cd \"$cli\" &&\n"},{"id":"516373","messageId":"xmqqikn1bapz.fsf@gitster.g","threadId":"63258","inReplyTo":"20250416145939.24207-2-anthonywang03@icloud.com","subject":"Re: [GSoC] [PATCH v7 1/1] t9811: be more precise to check importing of tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-18T18:12:08Z","receivedAt":"2025-04-18T18:12:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anthony Wang writes:\n\n> Additionally, we add a negative test to verify that a possible\n> uninteded tag does not show up in the imported repository.\n\nWith this we tightened the tests to insist that TAG_F1_ONLY does not\nexist, but it seems that our CI tests at least on macos seems to\nthink that the tag should exist.\n\nhttps://github.com/git/git/actions/runs/14526556144/job/40759116464#step:4:1944\n\n> +\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n> +\t\tgit show-ref --verify refs/tags/TAG_F1_2 &&\n> +\t\ttest_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n\nAnd because of that, this third line which does not correspond to\nany tests in the original makes the thing fail.\n\nI think negative test was what I suggested, but I didn't know if\nthat particular tag used for the negative test should or should not\nexist in the test at that point (I do not do Perforce, so I still do\nnot know the answer to that question; in any case, due to lack of p4\nin my environment, my local testing did not catch this breakage).\n\nSorry about the confusion.\n\nLet's add this on top.\n\n-- >8 ---- >8 ---- >8 --\nSubject: [PATCH] t9811: fix misconversion of test\n\nThe previous commit started to insist TAG_F1_ONLY to be missing,\nwhich was not in the original. Let's not to be overly eager in the\nconversion.\n\nSigned-off-by: Junio C Hamano \n---\n t/t9811-git-p4-label-import.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 39856629c0..9637a46d6f 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -97,7 +97,6 @@ test_expect_success 'two labels on the same changelist' '\n \n \t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n \t\tgit show-ref --verify refs/tags/TAG_F1_2 &&\n-\t\ttest_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n \n \t\tcd main &&\n \n-- \n2.49.0-524-g64a58d64d1\n\n"},{"id":"516378","messageId":"xmqq34e5b2s2.fsf@gitster.g","threadId":"63258","inReplyTo":"xmqqikn1bapz.fsf@gitster.g","subject":"Re: [GSoC] [PATCH v7 1/1] t9811: be more precise to check importing of tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-18T21:03:41Z","receivedAt":"2025-04-18T21:03:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Let's add this on top.\n\nWell, it turns out that it wasn't enough.\n\n--- >8 ------ >8 ------ >8 ---\nSubject: [PATCH] t9811: fix misconversion of test\n\nThe previous commit started to insist TAG_F1_ONLY to be missing,\nwhich was not in the original. Let's not to be overly eager in the\nconversion.\n\nAso, the other hunk in the commit introduced shell syntax errors,\nbreaking the test to fail. Fix it.\n\nSigned-off-by: Junio C Hamano \n---\n t/t9811-git-p4-label-import.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 39856629c0..7614dfbd95 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -97,7 +97,6 @@ test_expect_success 'two labels on the same changelist' '\n \n \t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n \t\tgit show-ref --verify refs/tags/TAG_F1_2 &&\n-\t\ttest_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&\n \n \t\tcd main &&\n \n@@ -207,7 +206,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n \t\tgit tag CFG_A_GIT_TAG &&\n \t\tgit p4 rebase --verbose &&\n \t\tgit p4 submit --verbose &&\n-\t\tgit show-ref --verify refs/tags/TAG_F1_1 &&\n+\t\tgit show-ref --verify refs/tags/TAG_F1_1\n \t) &&\n \t(\n \t\tcd \"$cli\" &&\n-- \n2.49.0-524-g64a58d64d1\n\n"},{"id":"516382","messageId":"CAPig+cS79mpfStmBBN2a-PeQAwrgtbj6HJmmuv6C1QEDFcOUgw@mail.gmail.com","threadId":"63258","inReplyTo":"xmqq34e5b2s2.fsf@gitster.g","subject":"Re: [GSoC] [PATCH v7 1/1] t9811: be more precise to check importing of tags","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-04-18T21:41:04Z","receivedAt":"2025-04-18T21:41:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 18, 2025 at 5:03 PM Junio C Hamano wrote:\n> Subject: [PATCH] t9811: fix misconversion of test\n>\n> The previous commit started to insist TAG_F1_ONLY to be missing,\n> which was not in the original. Let's not to be overly eager in the\n> conversion.\n\ns/to be/be/\n\n> Aso, the other hunk in the commit introduced shell syntax errors,\n> breaking the test to fail. Fix it.\n\ns/Aso/Also/\ns/breaking/causing/\n\n> Signed-off-by: Junio C Hamano \n"}]}