{"thread":{"id":"60301","subject":"[PATCH 0/1] *** Avoid using Pipes ***","startedAt":"2023-10-03T17:49:15Z","lastAt":"2024-01-20T02:16:20Z","messageCount":10,"participants":["ach.lumap@gmail.com","Eric Sunshine","Junio C Hamano","Achu Luma","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"482577","messageId":"20231003174853.1732-1-ach.lumap@gmail.com","threadId":"60301","inReplyTo":null,"subject":"[PATCH 0/1] *** Avoid using Pipes ***","fromName":"","fromEmail":"ach.lumap@gmail.com","sentAt":"2023-10-03T17:48:52Z","receivedAt":"2023-10-03T17:49:15Z","isPatch":true,"sender":{"key":"ach.lumap@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142904668?v=4"},"body":"From: achluma <achluma@gmail.com>\n\n*** BLURB HERE ***\n\nachluma (1):\n  t2400: avoid using pipes\n\n t/t2400-worktree-add.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\n\nbase-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415\n-- \n2.41.0.windows.1\n\n"},{"id":"482578","messageId":"20231003174853.1732-2-ach.lumap@gmail.com","threadId":"60301","inReplyTo":"20231003174853.1732-1-ach.lumap@gmail.com","subject":"[PATCH 1/1] t2400: avoid using pipes","fromName":"","fromEmail":"ach.lumap@gmail.com","sentAt":"2023-10-03T17:48:53Z","receivedAt":"2023-10-03T17:49:45Z","isPatch":true,"sender":{"key":"ach.lumap@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142904668?v=4"},"body":"From: achluma <ach.lumap@gmail.com>\n\nThe exit code of the preceding command in a pipe is disregarded,\nso it's advisable to refrain from relying on it. Instead, by\nsaving the output of a Git command to a file, we gain the\nability to examine the exit codes of both commands separately.\n\nSigned-off-by: achluma <ach.lumap@gmail.com>\n---\n t/t2400-worktree-add.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex df4aff7825..7ead05bb98 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -468,7 +468,8 @@ test_expect_success 'put a worktree under rebase' '\n \t\tcd under-rebase &&\n \t\tset_fake_editor &&\n \t\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n-\t\tgit worktree list | grep \"under-rebase.*detached HEAD\"\n+\t\tgit worktree list >actual && \n+\t\tgrep \"under-rebase.*detached HEAD\" actual\n \t)\n '\n \n@@ -509,7 +510,8 @@ test_expect_success 'checkout a branch under bisect' '\n \t\tgit bisect start &&\n \t\tgit bisect bad &&\n \t\tgit bisect good HEAD~2 &&\n-\t\tgit worktree list | grep \"under-bisect.*detached HEAD\" &&\n+\t\tgit worktree list >actual && \n+\t\tgrep \"under-bisect.*detached HEAD\" actual &&\n \t\ttest_must_fail git worktree add new-bisect under-bisect &&\n \t\t! test -d new-bisect\n \t)\n-- \n2.41.0.windows.1\n\n"},{"id":"482582","messageId":"CAPig+cSkZ_brRh_ijFRgz3sP9ou5se9-xeRg=C+cV3c3-v3Wtg@mail.gmail.com","threadId":"60301","inReplyTo":"20231003174853.1732-2-ach.lumap@gmail.com","subject":"Re: [PATCH 1/1] t2400: avoid using pipes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-10-03T18:01:13Z","receivedAt":"2023-10-03T18:01:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Oct 3, 2023 at 1:49 PM <ach.lumap@gmail.com> wrote:\n> t2400: avoid using pipes\n\nPipes themselves are not necessarily problematic, and there are many\nplaces in the test suite where they are legitimately used. Rather...\n\n> The exit code of the preceding command in a pipe is disregarded,\n> so it's advisable to refrain from relying on it. Instead, by\n> saving the output of a Git command to a file, we gain the\n> ability to examine the exit codes of both commands separately.\n\n... as you correctly explain here, we don't want to lose the exit code\nfrom the Git command. Thus, if you want to convey more information to\nreaders of `git log --oneline` (or other such commands), a better\nsubject for the patch might be:\n\n    t2400: avoid losing Git exit code\n\nThat minor comment aside (which is probably not worth a reroll), the\ncommit message properly explains why this change is desirable and the\npatch itself looks good.\n\n> Signed-off-by: achluma <ach.lumap@gmail.com>\n> ---\n> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n> @@ -468,7 +468,8 @@ test_expect_success 'put a worktree under rebase' '\n>                 cd under-rebase &&\n>                 set_fake_editor &&\n>                 FAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n> -               git worktree list | grep \"under-rebase.*detached HEAD\"\n> +               git worktree list >actual &&\n\nThanks for following the style guideline and omitting whitespace\nbetween the redirection operator and the destination file.\n\n> +               grep \"under-rebase.*detached HEAD\" actual\n>         )\n>  '\n>\n> @@ -509,7 +510,8 @@ test_expect_success 'checkout a branch under bisect' '\n>                 git bisect start &&\n>                 git bisect bad &&\n>                 git bisect good HEAD~2 &&\n> -               git worktree list | grep \"under-bisect.*detached HEAD\" &&\n> +               git worktree list >actual &&\n> +               grep \"under-bisect.*detached HEAD\" actual &&\n>                 test_must_fail git worktree add new-bisect under-bisect &&\n>                 ! test -d new-bisect\n>         )\n"},{"id":"482586","messageId":"xmqqr0mbzgx5.fsf@gitster.g","threadId":"60301","inReplyTo":"CAPig+cSkZ_brRh_ijFRgz3sP9ou5se9-xeRg=C+cV3c3-v3Wtg@mail.gmail.com","subject":"Re: [PATCH 1/1] t2400: avoid using pipes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-03T18:42:30Z","receivedAt":"2023-10-03T18:42:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Tue, Oct 3, 2023 at 1:49 PM <ach.lumap@gmail.com> wrote:\n>> t2400: avoid using pipes\n>\n> Pipes themselves are not necessarily problematic, and there are many\n> places in the test suite where they are legitimately used. Rather...\n> ...\n> readers of `git log --oneline` (or other such commands), a better\n> subject for the patch might be:\n>\n>     t2400: avoid losing Git exit code\n>\n> That minor comment aside (which is probably not worth a reroll), the\n> commit message properly explains why this change is desirable and the\n> patch itself looks good.\n\nThanks for writing and reviewing.  Will queue.\n"},{"id":"485272","messageId":"20231130165429.2595-1-ach.lumap@gmail.com","threadId":"60301","inReplyTo":"20231003174853.1732-1-ach.lumap@gmail.com","subject":"[PATCH v2 0/1] *** Avoid using Pipes ***","fromName":"Achu Luma","fromEmail":"ach.lumap@gmail.com","sentAt":"2023-11-30T16:54:28Z","receivedAt":"2023-11-30T16:57:09Z","isPatch":true,"sender":{"key":"ach.lumap@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142904668?v=4"},"body":"*** BLURB HERE ***\n\nAchu Luma (1):\n  t2400: avoid using pipes\n\n t/t2400-worktree-add.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\n\nbase-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415\n-- \n2.41.0.windows.1\n\n"},{"id":"485275","messageId":"20231130165429.2595-2-ach.lumap@gmail.com","threadId":"60301","inReplyTo":"20231130165429.2595-1-ach.lumap@gmail.com","subject":"[PATCH v2 1/1] t2400: avoid using pipes","fromName":"Achu Luma","fromEmail":"ach.lumap@gmail.com","sentAt":"2023-11-30T16:54:29Z","receivedAt":"2023-11-30T17:37:11Z","isPatch":true,"sender":{"key":"ach.lumap@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142904668?v=4"},"body":"The exit code of the preceding command in a pipe is disregarded,\nso it's advisable to refrain from relying on it. Instead, by\nsaving the output of a Git command to a file, we gain the\nability to examine the exit codes of both commands separately.\n\nSigned-off-by: achluma <ach.lumap@gmail.com>\n---\n t/t2400-worktree-add.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex df4aff7825..7ead05bb98 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -468,7 +468,8 @@ test_expect_success 'put a worktree under rebase' '\n \t\tcd under-rebase &&\n \t\tset_fake_editor &&\n \t\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n-\t\tgit worktree list | grep \"under-rebase.*detached HEAD\"\n+\t\tgit worktree list >actual && \n+\t\tgrep \"under-rebase.*detached HEAD\" actual\n \t)\n '\n \n@@ -509,7 +510,8 @@ test_expect_success 'checkout a branch under bisect' '\n \t\tgit bisect start &&\n \t\tgit bisect bad &&\n \t\tgit bisect good HEAD~2 &&\n-\t\tgit worktree list | grep \"under-bisect.*detached HEAD\" &&\n+\t\tgit worktree list >actual && \n+\t\tgrep \"under-bisect.*detached HEAD\" actual &&\n \t\ttest_must_fail git worktree add new-bisect under-bisect &&\n \t\t! test -d new-bisect\n \t)\n-- \n2.41.0.windows.1\n\n"},{"id":"485277","messageId":"CAP8UFD0KDdwoJw6AzLUpqos=bLumcmDax59_MfQ9TUFqmmpcoA@mail.gmail.com","threadId":"60301","inReplyTo":"20231130165429.2595-2-ach.lumap@gmail.com","subject":"Re: [PATCH v2 1/1] t2400: avoid using pipes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-11-30T18:16:09Z","receivedAt":"2023-11-30T18:16:24Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Luma,\n\nOn Thu, Nov 30, 2023 at 6:37 PM Achu Luma <ach.lumap@gmail.com> wrote:\n>\n> The exit code of the preceding command in a pipe is disregarded,\n> so it's advisable to refrain from relying on it. Instead, by\n> saving the output of a Git command to a file, we gain the\n> ability to examine the exit codes of both commands separately.\n>\n> Signed-off-by: achluma <ach.lumap@gmail.com>\n\nI think the issue with merging your patch (in\nhttps://lore.kernel.org/git/xmqqedibzgi1.fsf@gitster.g/) was that this\n\"Signed-off-by: ...\" line didn't show your full real name and didn't\nmatch your name in your email address.\n\nAssuming that \"Achu Luma\" is your full real name, you should replace\n\"achluma\" with \"Achu Luma\" in the \"Signed-off-by: ...\" line.\n\nAlso it's better not to send a cover letter patch like\nhttps://lore.kernel.org/git/20231130165429.2595-1-ach.lumap@gmail.com/\nwith no content for small patches like this.\n\nWhen you resend, please also make sure to use [Outreachy] in the patch\nsubject and to increment the version number of the patch, using for\nexample \"[PATCH v3]\".\n\nIt would be nice too if after the line starting with --- below, you\ncould describe in a few lines the changes in the new version of the\npatch compared to the previous version.\n\n> ---\n\nHere (after the line starting with --- above) is the place where you\ncan tell what changed in the patch compared to the previous version.\n\nNote that when there is a cover letter patch, it's better to talk\nabout changes in the new version in the cover letter, but I dont think\nit's worth sending a cover letter patch.\n\nThanks,\nChristian.\n\n>  t/t2400-worktree-add.sh | 6 ++++--\n>  1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n> index df4aff7825..7ead05bb98 100755\n> --- a/t/t2400-worktree-add.sh\n> +++ b/t/t2400-worktree-add.sh\n> @@ -468,7 +468,8 @@ test_expect_success 'put a worktree under rebase' '\n>                 cd under-rebase &&\n>                 set_fake_editor &&\n>                 FAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n> -               git worktree list | grep \"under-rebase.*detached HEAD\"\n> +               git worktree list >actual &&\n> +               grep \"under-rebase.*detached HEAD\" actual\n>         )\n>  '\n>\n> @@ -509,7 +510,8 @@ test_expect_success 'checkout a branch under bisect' '\n>                 git bisect start &&\n>                 git bisect bad &&\n>                 git bisect good HEAD~2 &&\n> -               git worktree list | grep \"under-bisect.*detached HEAD\" &&\n> +               git worktree list >actual &&\n> +               grep \"under-bisect.*detached HEAD\" actual &&\n>                 test_must_fail git worktree add new-bisect under-bisect &&\n>                 ! test -d new-bisect\n>         )\n> --\n> 2.41.0.windows.1\n>\n>\n"},{"id":"485352","messageId":"20231204153740.2992-1-ach.lumap@gmail.com","threadId":"60301","inReplyTo":"CAP8UFD0KDdwoJw6AzLUpqos=bLumcmDax59_MfQ9TUFqmmpcoA@mail.gmail.com","subject":"[Outreachy][PATCH v3] t2400: avoid using pipes","fromName":"Achu Luma","fromEmail":"ach.lumap@gmail.com","sentAt":"2023-12-04T15:37:40Z","receivedAt":"2023-12-04T15:38:00Z","isPatch":true,"sender":{"key":"ach.lumap@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142904668?v=4"},"body":"The exit code of the preceding command in a pipe is disregarded,\nso it's advisable to refrain from relying on it. Instead, by\nsaving the output of a Git command to a file, we gain the\nability to examine the exit codes of both commands separately.\n\nSigned-off-by: Achu Luma <ach.lumap@gmail.com>\n---\n Since v2 I don't send a cover  letter anymore, and I changed \n my \"Signed-of-by: ...\" line so that it\n contains my full real name and I added \"Outreachy\" to the subject.\n\n t/t2400-worktree-add.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex df4aff7825..7ead05bb98 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -468,7 +468,8 @@ test_expect_success 'put a worktree under rebase' '\n \t\tcd under-rebase &&\n \t\tset_fake_editor &&\n \t\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n-\t\tgit worktree list | grep \"under-rebase.*detached HEAD\"\n+\t\tgit worktree list >actual && \n+\t\tgrep \"under-rebase.*detached HEAD\" actual\n \t)\n '\n \n@@ -509,7 +510,8 @@ test_expect_success 'checkout a branch under bisect' '\n \t\tgit bisect start &&\n \t\tgit bisect bad &&\n \t\tgit bisect good HEAD~2 &&\n-\t\tgit worktree list | grep \"under-bisect.*detached HEAD\" &&\n+\t\tgit worktree list >actual && \n+\t\tgrep \"under-bisect.*detached HEAD\" actual &&\n \t\ttest_must_fail git worktree add new-bisect under-bisect &&\n \t\t! test -d new-bisect\n \t)\n-- \n2.41.0.windows.1\n\n"},{"id":"485479","messageId":"xmqqr0jw1kbq.fsf@gitster.g","threadId":"60301","inReplyTo":"20231204153740.2992-1-ach.lumap@gmail.com","subject":"Re: [Outreachy][PATCH v3] t2400: avoid using pipes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-08T21:00:09Z","receivedAt":"2023-12-08T21:00:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Achu Luma <ach.lumap@gmail.com> writes:\n\n> Subject: Re: [Outreachy][PATCH v3] t2400: avoid using pipes\n\n\"avoid using pipes\" is a means to an end.  And it is more important\nto tell readers what that \"end\" is.  With this patch, what are we\ntrying to achieve?  Cater to platforms that lack pipes?  Help\nplatforms that cannot run two processes at the same time, so let one\nrun and store the result in a file, and then let the other one run,\nto reduce the CPU load?\n\nIf we run a \"git\" command, especially a command we are testing, on\nthe upstream side of a pipe, we lose information.  We cannot tell\nwhat exit status the command exited with.  That is what we care\nabout.\n\nSo, it is better to say that in the title, e.g.,\n\n    Subject: [PATCH] t2400: avoid losing exit status to pipes\n\n> The exit code of the preceding command in a pipe is disregarded,\n> so it's advisable to refrain from relying on it.\n\nIt is unclear what \"it\" refers to here.  We cannot rely on the exit\ncode of the command on the upstream side of a pipe, obviously.\n\n> Instead, by\n> saving the output of a Git command to a file, we gain the\n> ability to examine the exit codes of both commands separately.\n\nSurely.  I personally think that the title that says what the\npurpose of the patch is clearly should be sufficient without any\nfurther description in the body, though.\n>\n> Signed-off-by: Achu Luma <ach.lumap@gmail.com>\n> ---\n>  Since v2 I don't send a cover  letter anymore, and I changed \n>  my \"Signed-of-by: ...\" line so that it\n>  contains my full real name and I added \"Outreachy\" to the subject.\n\nNicely done.\n\n>\n>  t/t2400-worktree-add.sh | 6 ++++--\n>  1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n> index df4aff7825..7ead05bb98 100755\n> --- a/t/t2400-worktree-add.sh\n> +++ b/t/t2400-worktree-add.sh\n> @@ -468,7 +468,8 @@ test_expect_success 'put a worktree under rebase' '\n>  \t\tcd under-rebase &&\n>  \t\tset_fake_editor &&\n>  \t\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n> -\t\tgit worktree list | grep \"under-rebase.*detached HEAD\"\n> +\t\tgit worktree list >actual && \n> +\t\tgrep \"under-rebase.*detached HEAD\" actual\n>  \t)\n>  '\n>  \n> @@ -509,7 +510,8 @@ test_expect_success 'checkout a branch under bisect' '\n>  \t\tgit bisect start &&\n>  \t\tgit bisect bad &&\n>  \t\tgit bisect good HEAD~2 &&\n> -\t\tgit worktree list | grep \"under-bisect.*detached HEAD\" &&\n> +\t\tgit worktree list >actual && \n> +\t\tgrep \"under-bisect.*detached HEAD\" actual &&\n>  \t\ttest_must_fail git worktree add new-bisect under-bisect &&\n>  \t\t! test -d new-bisect\n>  \t)\n"},{"id":"487129","messageId":"20240120021547.199-1-ach.lumap@gmail.com","threadId":"60301","inReplyTo":"20231204153740.2992-1-ach.lumap@gmail.com","subject":"[Outreachy][PATCH v4] t2400: avoid losing exit status to pipes","fromName":"Achu Luma","fromEmail":"ach.lumap@gmail.com","sentAt":"2024-01-20T02:15:47Z","receivedAt":"2024-01-20T02:16:20Z","isPatch":true,"sender":{"key":"ach.lumap@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142904668?v=4"},"body":"The exit code of the preceding command in a pipe is disregarded. So\nif that preceding command is a Git command that fails, the test would\nnot fail. Instead, by saving the output of that Git command to a file,\nand removing the pipe, we make sure the test will fail if that Git\ncommand fails.\n\nSigned-off-by: Achu Luma <ach.lumap@gmail.com>\n---\n The difference between v3 and v4 is:\n - Changed subject to better reflect what the patch is doing.\n - Updated the commit message.\n\n t/t2400-worktree-add.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex 3742971105..b597004adb 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -490,7 +490,8 @@ test_expect_success 'put a worktree under rebase' '\n \t\tcd under-rebase &&\n \t\tset_fake_editor &&\n \t\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n-\t\tgit worktree list | grep \"under-rebase.*detached HEAD\"\n+\t\tgit worktree list >actual &&\n+\t\tgrep \"under-rebase.*detached HEAD\" actual\n \t)\n '\n\n@@ -531,7 +532,8 @@ test_expect_success 'checkout a branch under bisect' '\n \t\tgit bisect start &&\n \t\tgit bisect bad &&\n \t\tgit bisect good HEAD~2 &&\n-\t\tgit worktree list | grep \"under-bisect.*detached HEAD\" &&\n+\t\tgit worktree list >actual &&\n+\t\tgrep \"under-bisect.*detached HEAD\" actual &&\n \t\ttest_must_fail git worktree add new-bisect under-bisect &&\n \t\t! test -d new-bisect\n \t)\n--\n2.43.0.windows.1\n\n"}]}