{"thread":{"id":"54449","subject":"[PATCH 0/2] [Outreachy][Patch v1] t9832,t2200: avoid using pipes in git related commands","startedAt":"2020-10-17T16:02:37Z","lastAt":"2020-10-18T22:05:18Z","messageCount":14,"participants":["Amanda Shafack via GitGitGadget","Eric Sunshine","Amanda Shafack","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"407801","messageId":"pull.885.git.git.1602950552.gitgitgadget@gmail.com","threadId":"54449","inReplyTo":null,"subject":"[PATCH 0/2] [Outreachy][Patch v1] t9832,t2200: avoid using pipes in git related commands","fromName":"Amanda Shafack via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-17T16:02:30Z","receivedAt":"2020-10-17T16:02:37Z","isPatch":true,"sender":{"key":"shafack.likhene@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17765231?v=4"},"body":"When the upstream of a pipe throws an error, the downstream still executes\nnormally. This happens because the exit code of the upstream in a pipe is\nignored. This behavior can make debugging very hard in case a test fails.\nAlso, pipes are prone to deadlocks. If the upstream gets full, the commands\ndownstream will never start.\n\nWrite out the output of the git command to a file, so as to test the exit\ncodes of both commands.\n\nCommit c6f44e1da5 (t9813: avoid using pipes, 2017-01-04) noticed that the\nexit code of upstream in the pipe is ignored, thus using pipes should be\navoided.\n\nSigned-off-by: Amanda Shafack shafack.likhene@gmail.com\n[shafack.likhene@gmail.com]\n\nAmanda  Shafack (1):\n  t2200: modify code syntax\n\nAmanda Shafack (1):\n  t9832,t2200: avoid using pipes in git commands\n\n t/t2200-add-update.sh | 3 ++-\n t/t9832-unshelve.sh   | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\n\nbase-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-885%2Flkmandy%2Favoid-pipes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-885/lkmandy/avoid-pipes-v1\nPull-Request: https://github.com/git/git/pull/885\n-- \ngitgitgadget\n"},{"id":"407802","messageId":"0a1550cb225591a53118bd8f7782d95de5a4df34.1602950552.git.gitgitgadget@gmail.com","threadId":"54449","inReplyTo":"pull.885.git.git.1602950552.gitgitgadget@gmail.com","subject":"[PATCH 2/2] t2200: modify code syntax","fromName":"Amanda Shafack via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-17T16:02:32Z","receivedAt":"2020-10-17T16:02:39Z","isPatch":true,"sender":{"key":"shafack.likhene@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17765231?v=4"},"body":"From: Amanda  Shafack <shafack.likhene@gmail.com>\n\nUsing the logical not operator on both the git and grep command is\nredundant, since the main check is done at the level of the grep\ncommand.\n\nApply the logical not operator only to the grep command.\n\nCo-authored-by: Johannes Schindelin <dscho@github.com>\nSigned-off-by: Amanda Shafack <shafack.likhene@gmail.com>\n---\n t/t2200-add-update.sh | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\nindex 2d850bb372..7cb7a70382 100755\n--- a/t/t2200-add-update.sh\n+++ b/t/t2200-add-update.sh\n@@ -179,10 +179,8 @@ test_expect_success 'add -u resolves unmerged paths' '\n \n test_expect_success '\"add -u non-existent\" should fail' '\n \ttest_must_fail git add -u non-existent &&\n-\t! (\n-\t\tgit ls-files >actual &&\n-\t\tgrep \"non-existent\" actual\n-\t)\n+\tgit ls-files >actual &&\n+\t! grep \"non-existent\" actual\n '\n \n test_done\n-- \ngitgitgadget\n"},{"id":"407803","messageId":"fc2da014a62c387009e2175435286afef64d6598.1602950552.git.gitgitgadget@gmail.com","threadId":"54449","inReplyTo":"pull.885.git.git.1602950552.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t9832,t2200: avoid using pipes in git commands","fromName":"Amanda Shafack via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-17T16:02:31Z","receivedAt":"2020-10-17T16:02:39Z","isPatch":true,"sender":{"key":"shafack.likhene@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17765231?v=4"},"body":"From: Amanda Shafack <shafack.likhene@gmail.com>\n\nWhen the upstream of a pipe throws an error, the downstream still\nexecutes normally. This happens because the exit code of the upstream\nin a pipe is ignored. This behavior can make debugging very hard\nincase a test fails. Also, pipes are prone to deadlocks. If the\nupstream gets full, the commands downstream will never start.\n\nWrite out the output of the git command to a file, so as to test the\nexit codes of both commands.\n\nCommit c6f44e1da5 (t9813: avoid using pipes, 2017-01-04) noticed that\nthe exit code of upstream in the pipe is ignored, thus using pipes\nshould be avoided.\n\nSigned-off-by: Amanda Shafack <shafack.likhene@gmail.com>\n---\n t/t2200-add-update.sh | 5 ++++-\n t/t9832-unshelve.sh   | 3 ++-\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\nindex f764b7e3f5..2d850bb372 100755\n--- a/t/t2200-add-update.sh\n+++ b/t/t2200-add-update.sh\n@@ -179,7 +179,10 @@ test_expect_success 'add -u resolves unmerged paths' '\n \n test_expect_success '\"add -u non-existent\" should fail' '\n \ttest_must_fail git add -u non-existent &&\n-\t! (git ls-files | grep \"non-existent\")\n+\t! (\n+\t\tgit ls-files >actual &&\n+\t\tgrep \"non-existent\" actual\n+\t)\n '\n \n test_done\ndiff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh\nindex 7194fb2855..6b3cb0414a 100755\n--- a/t/t9832-unshelve.sh\n+++ b/t/t9832-unshelve.sh\n@@ -68,7 +68,8 @@ EOF\n \t\tcd \"$git\" &&\n \t\tchange=$(last_shelved_change) &&\n \t\tgit p4 unshelve $change &&\n-\t\tgit show refs/remotes/p4-unshelved/$change | grep -q \"Further description\" &&\n+\t\tgit show refs/remotes/p4-unshelved/$change >actual &&\n+\t\tgrep -q \"Further description\" actual &&\n \t\tgit cherry-pick refs/remotes/p4-unshelved/$change &&\n \t\ttest_path_is_file file2 &&\n \t\ttest_cmp file1 \"$cli\"/file1 &&\n-- \ngitgitgadget\n\n"},{"id":"407837","messageId":"CAPig+cRuutHJ64aXQy5X9h-Vg4=_NKKyL57KdwbvXj4cGRS1PQ@mail.gmail.com","threadId":"54449","inReplyTo":"0a1550cb225591a53118bd8f7782d95de5a4df34.1602950552.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] t2200: modify code syntax","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-18T05:55:55Z","receivedAt":"2020-10-18T05:56:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 17, 2020 at 12:02 PM Amanda Shafack via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> Using the logical not operator on both the git and grep command is\n> redundant, since the main check is done at the level of the grep\n> command.\n>\n> Apply the logical not operator only to the grep command.\n> ---\n> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\n> @@ -179,10 +179,8 @@ test_expect_success 'add -u resolves unmerged paths' '\n> -       ! (\n> -               git ls-files >actual &&\n> -               grep \"non-existent\" actual\n> -       )\n> +       git ls-files >actual &&\n> +       ! grep \"non-existent\" actual\n\nThe commit message talks only about redundancy of applying the logical\nNOT operator to the combined subshell content, but what it misses is\nthat transformation of:\n\n    ! (git ls-files | grep \"non-existent\")\n\nin patch [1/2] into:\n\n    ! (\n        git ls-files >actual &&\n        grep \"non-existent\" actual\n    )\n\nis actively wrong. In particular, if `git ls-files` itself fails, then\nthe `grep` will never get run, and the subshell will exit with a\nfailure, however, the NOT then turns that failure into a success,\nwhich is quite undesirable. The test should fail if either `git\nls-files` fails or if the `grep` expectation is not met; it should not\nsucceed when `git ls-files` fails.\n\nSo, the correct thing to do is to merge [1/2] and [2/2] into a single\npatch which directly transforms:\n\n    ! (git ls-files | grep \"non-existent\")\n\ninto:\n\n    git ls-files >actual &&\n    ! grep \"non-existent\" actual\n"},{"id":"407839","messageId":"CAPig+cQnfcc9W3qE5XkyLiwNyK8Wm2SsAyjzatDWEVUi3s8APw@mail.gmail.com","threadId":"54449","inReplyTo":"fc2da014a62c387009e2175435286afef64d6598.1602950552.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t9832,t2200: avoid using pipes in git commands","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-18T06:11:47Z","receivedAt":"2020-10-18T06:12:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 17, 2020 at 12:02 PM Amanda Shafack via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> When the upstream of a pipe throws an error, the downstream still\n> executes normally. This happens because the exit code of the upstream\n> in a pipe is ignored. This behavior can make debugging very hard\n> incase a test fails.\n\nTo be more precise, we're not worried about difficulty of debugging\nwhen a Git command is upstream in a pipe. What we are more concerned\nwith is that an unexpected failure of the Git command will go\nunnoticed (unless it also happens to produce wrong output which is\nlater caught somewhere downstream). By avoiding placing Git upstream\nin a pipe, we can actively catch a failure of the Git command. The\ncommit message should focus on that reason instead.\n\n> Also, pipes are prone to deadlocks. If the\n> upstream gets full, the commands downstream will never start.\n\nI don't think this is ever an issue in Git tests, so talking about it\nhere probably only muddies the waters, thus makes the commit message\nless precise and more hand-wavy. Dropping this sentence is\nrecommended.\n\n> Commit c6f44e1da5 (t9813: avoid using pipes, 2017-01-04) noticed that\n> the exit code of upstream in the pipe is ignored, thus using pipes\n> should be avoided.\n\nThere are a lot of commits in the project history which perform this\nsort of transformation, so it's not clear to the reader why this\nparticular commit is called out as being important. If it's not, then\nI'd recommend dropping this sentence, as well.\n\n> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\n>  test_expect_success '\"add -u non-existent\" should fail' '\n>         test_must_fail git add -u non-existent &&\n> -       ! (git ls-files | grep \"non-existent\")\n> +       ! (\n> +               git ls-files >actual &&\n> +               grep \"non-existent\" actual\n> +       )\n>  '\n\nThis transformation appears to be incorrect. See my review of patch\n[2/2] for details.\n\n> diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh\n> @@ -68,7 +68,8 @@ EOF\n> -               git show refs/remotes/p4-unshelved/$change | grep -q \"Further description\" &&\n> +               git show refs/remotes/p4-unshelved/$change >actual &&\n> +               grep -q \"Further description\" actual &&\n\nThis transformation looks fine.\n"},{"id":"407845","messageId":"CAGxm6oWT=LG7KczvKaa5ta=LBAkFsHHYsCXLB7tBfn==Z0g9DQ@mail.gmail.com","threadId":"54449","inReplyTo":"CAPig+cQnfcc9W3qE5XkyLiwNyK8Wm2SsAyjzatDWEVUi3s8APw@mail.gmail.com","subject":"Re: [PATCH 1/2] t9832,t2200: avoid using pipes in git commands","fromName":"Amanda Shafack","fromEmail":"shafack.likhene@gmail.com","sentAt":"2020-10-18T13:57:51Z","receivedAt":"2020-10-18T13:58:59Z","isPatch":true,"sender":{"key":"shafack.likhene@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17765231?v=4"},"body":"Thanks for your feedback, I now have a better understanding. I have\nmade the changes you requested.\n\n\nOn Sun, Oct 18, 2020 at 7:11 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Sat, Oct 17, 2020 at 12:02 PM Amanda Shafack via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> > When the upstream of a pipe throws an error, the downstream still\n> > executes normally. This happens because the exit code of the upstream\n> > in a pipe is ignored. This behavior can make debugging very hard\n> > incase a test fails.\n>\n> To be more precise, we're not worried about difficulty of debugging\n> when a Git command is upstream in a pipe. What we are more concerned\n> with is that an unexpected failure of the Git command will go\n> unnoticed (unless it also happens to produce wrong output which is\n> later caught somewhere downstream). By avoiding placing Git upstream\n> in a pipe, we can actively catch a failure of the Git command. The\n> commit message should focus on that reason instead.\n>\n> > Also, pipes are prone to deadlocks. If the\n> > upstream gets full, the commands downstream will never start.\n>\n> I don't think this is ever an issue in Git tests, so talking about it\n> here probably only muddies the waters, thus makes the commit message\n> less precise and more hand-wavy. Dropping this sentence is\n> recommended.\n>\n> > Commit c6f44e1da5 (t9813: avoid using pipes, 2017-01-04) noticed that\n> > the exit code of upstream in the pipe is ignored, thus using pipes\n> > should be avoided.\n>\n> There are a lot of commits in the project history which perform this\n> sort of transformation, so it's not clear to the reader why this\n> particular commit is called out as being important. If it's not, then\n> I'd recommend dropping this sentence, as well.\n>\n> > diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\n> >  test_expect_success '\"add -u non-existent\" should fail' '\n> >         test_must_fail git add -u non-existent &&\n> > -       ! (git ls-files | grep \"non-existent\")\n> > +       ! (\n> > +               git ls-files >actual &&\n> > +               grep \"non-existent\" actual\n> > +       )\n> >  '\n>\n> This transformation appears to be incorrect. See my review of patch\n> [2/2] for details.\n>\n> > diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh\n> > @@ -68,7 +68,8 @@ EOF\n> > -               git show refs/remotes/p4-unshelved/$change | grep -q \"Further description\" &&\n> > +               git show refs/remotes/p4-unshelved/$change >actual &&\n> > +               grep -q \"Further description\" actual &&\n>\n> This transformation looks fine.\n\n\n\n-- \n\nCheers!\n\nAmanda  Shafack\n"},{"id":"407846","messageId":"CAGxm6oXhW8x2jkN-P4emhp6BvAwk6urzFTauQBDhJdMV_0d1fg@mail.gmail.com","threadId":"54449","inReplyTo":"CAPig+cRuutHJ64aXQy5X9h-Vg4=_NKKyL57KdwbvXj4cGRS1PQ@mail.gmail.com","subject":"Re: [PATCH 2/2] t2200: modify code syntax","fromName":"Amanda Shafack","fromEmail":"shafack.likhene@gmail.com","sentAt":"2020-10-18T13:59:12Z","receivedAt":"2020-10-18T13:59:51Z","isPatch":true,"sender":{"key":"shafack.likhene@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17765231?v=4"},"body":"I agree with you. I have made these changes as well.\nThanks\n\n\nOn Sun, Oct 18, 2020 at 6:56 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Sat, Oct 17, 2020 at 12:02 PM Amanda Shafack via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> > Using the logical not operator on both the git and grep command is\n> > redundant, since the main check is done at the level of the grep\n> > command.\n> >\n> > Apply the logical not operator only to the grep command.\n> > ---\n> > diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\n> > @@ -179,10 +179,8 @@ test_expect_success 'add -u resolves unmerged paths' '\n> > -       ! (\n> > -               git ls-files >actual &&\n> > -               grep \"non-existent\" actual\n> > -       )\n> > +       git ls-files >actual &&\n> > +       ! grep \"non-existent\" actual\n>\n> The commit message talks only about redundancy of applying the logical\n> NOT operator to the combined subshell content, but what it misses is\n> that transformation of:\n>\n>     ! (git ls-files | grep \"non-existent\")\n>\n> in patch [1/2] into:\n>\n>     ! (\n>         git ls-files >actual &&\n>         grep \"non-existent\" actual\n>     )\n>\n> is actively wrong. In particular, if `git ls-files` itself fails, then\n> the `grep` will never get run, and the subshell will exit with a\n> failure, however, the NOT then turns that failure into a success,\n> which is quite undesirable. The test should fail if either `git\n> ls-files` fails or if the `grep` expectation is not met; it should not\n> succeed when `git ls-files` fails.\n>\n> So, the correct thing to do is to merge [1/2] and [2/2] into a single\n> patch which directly transforms:\n>\n>     ! (git ls-files | grep \"non-existent\")\n>\n> into:\n>\n>     git ls-files >actual &&\n>     ! grep \"non-existent\" actual\n\n\n\n-- \n\nCheers!\n\nAmanda  Shafack\n"},{"id":"407847","messageId":"pull.885.v2.git.git.1603032125151.gitgitgadget@gmail.com","threadId":"54449","inReplyTo":"pull.885.git.git.1602950552.gitgitgadget@gmail.com","subject":"[PATCH v2] t9832,t2200: avoid using pipes in git commands","fromName":"Amanda Shafack via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-18T14:42:04Z","receivedAt":"2020-10-18T14:43:47Z","isPatch":true,"sender":{"key":"shafack.likhene@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17765231?v=4"},"body":"From: Amanda Shafack <shafack.likhene@gmail.com>\n\nWhen a git command is upstream in a pipe, an unexpected failure of\nthe git command will go unnoticed.\n\nWrite out the output of the git command to a file, so as to actively\ncatch a failure of the git command.\n\nSigned-off-by: Amanda Shafack <shafack.likhene@gmail.com>\n---\n    [Outreachy][Patch v2] t9832,t2200: avoid using pipes in git related\n    commands\n    \n    Changes since v1:\n    \n     * Merged patch [1/2] and [2/2] into a single patch\n     * Revised commit message\n    \n    Signed-off-by: Amanda Shafack shafack.likhene@gmail.com\n    [shafack.likhene@gmail.com]\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-885%2Flkmandy%2Favoid-pipes-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-885/lkmandy/avoid-pipes-v2\nPull-Request: https://github.com/git/git/pull/885\n\nRange-diff vs v1:\n\n 1:  fc2da014a6 ! 1:  bca393e69a t9832,t2200: avoid using pipes in git commands\n     @@ Metadata\n       ## Commit message ##\n          t9832,t2200: avoid using pipes in git commands\n      \n     -    When the upstream of a pipe throws an error, the downstream still\n     -    executes normally. This happens because the exit code of the upstream\n     -    in a pipe is ignored. This behavior can make debugging very hard\n     -    incase a test fails. Also, pipes are prone to deadlocks. If the\n     -    upstream gets full, the commands downstream will never start.\n     +    When a git command is upstream in a pipe, an unexpected failure of\n     +    the git command will go unnoticed.\n      \n     -    Write out the output of the git command to a file, so as to test the\n     -    exit codes of both commands.\n     -\n     -    Commit c6f44e1da5 (t9813: avoid using pipes, 2017-01-04) noticed that\n     -    the exit code of upstream in the pipe is ignored, thus using pipes\n     -    should be avoided.\n     +    Write out the output of the git command to a file, so as to actively\n     +    catch a failure of the git command.\n      \n          Signed-off-by: Amanda Shafack <shafack.likhene@gmail.com>\n      \n     @@ t/t2200-add-update.sh: test_expect_success 'add -u resolves unmerged paths' '\n       test_expect_success '\"add -u non-existent\" should fail' '\n       \ttest_must_fail git add -u non-existent &&\n      -\t! (git ls-files | grep \"non-existent\")\n     -+\t! (\n     -+\t\tgit ls-files >actual &&\n     -+\t\tgrep \"non-existent\" actual\n     -+\t)\n     ++\tgit ls-files >actual &&\n     ++\t! grep \"non-existent\" actual\n       '\n       \n       test_done\n 2:  0a1550cb22 < -:  ---------- t2200: modify code syntax\n\n\n t/t2200-add-update.sh | 3 ++-\n t/t9832-unshelve.sh   | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\nindex f764b7e3f5..7cb7a70382 100755\n--- a/t/t2200-add-update.sh\n+++ b/t/t2200-add-update.sh\n@@ -179,7 +179,8 @@ test_expect_success 'add -u resolves unmerged paths' '\n \n test_expect_success '\"add -u non-existent\" should fail' '\n \ttest_must_fail git add -u non-existent &&\n-\t! (git ls-files | grep \"non-existent\")\n+\tgit ls-files >actual &&\n+\t! grep \"non-existent\" actual\n '\n \n test_done\ndiff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh\nindex 7194fb2855..6b3cb0414a 100755\n--- a/t/t9832-unshelve.sh\n+++ b/t/t9832-unshelve.sh\n@@ -68,7 +68,8 @@ EOF\n \t\tcd \"$git\" &&\n \t\tchange=$(last_shelved_change) &&\n \t\tgit p4 unshelve $change &&\n-\t\tgit show refs/remotes/p4-unshelved/$change | grep -q \"Further description\" &&\n+\t\tgit show refs/remotes/p4-unshelved/$change >actual &&\n+\t\tgrep -q \"Further description\" actual &&\n \t\tgit cherry-pick refs/remotes/p4-unshelved/$change &&\n \t\ttest_path_is_file file2 &&\n \t\ttest_cmp file1 \"$cli\"/file1 &&\n\nbase-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a\n-- \ngitgitgadget\n"},{"id":"407854","messageId":"CAPig+cR7Hm9m1EiWkr5tKYS3r_zJf98XT7OQ+-Jt4EWkaFQTDw@mail.gmail.com","threadId":"54449","inReplyTo":"pull.885.v2.git.git.1603032125151.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] t9832,t2200: avoid using pipes in git commands","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-18T19:25:40Z","receivedAt":"2020-10-18T19:35:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 18, 2020 at 10:42 AM Amanda Shafack via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> t9832,t2200: avoid using pipes in git commands\n\nThe subject is a bit confusing since pipes aren't used in Git\ncommands; instead, Git commands may be components of pipes. However,\neven that is too imprecise. The issue this patch is addressing is that\nwe want to avoid Git commands _upstream_ in a pipe. It's perfectly\nacceptable for the Git command to be the final element of a pipe since\nthe pipe returns the exit code of the final command. So, to be more\nprecise, the subject could say:\n\n    t2200,t9832: avoid using `git` upstream in a pipe\n\nNit: It's subjective, but it feels a bit more natural to list the test\nnumbers in ascending order rather than descending order, which is why\nI swapped them around in the example above.\n\n> When a git command is upstream in a pipe, an unexpected failure of\n> the git command will go unnoticed.\n>\n> Write out the output of the git command to a file, so as to actively\n> catch a failure of the git command.\n\nIt's easy to see from the patch itself that the output of the Git\ncommand is now written to a file, so it's not necessary to say so in\nthe commit message. Therefore, the entire body of the commit message\ncould be written more succinctly, perhaps like this:\n\n    Avoid placing `git` upstream in a pipe since doing so throws away\n    its exit code, thus an unexpected failure may go unnoticed.\n\nThe actual patch itself looks fine, and these comments about the\ncommit message are quite minor, thus there probably is no need to\nre-roll (though feel free to do so if you think the bit of extra\npolishing of the commit message is worthwhile).\n\nThanks.\n"},{"id":"407859","messageId":"xmqqsgab7128.fsf@gitster.c.googlers.com","threadId":"54449","inReplyTo":"CAPig+cR7Hm9m1EiWkr5tKYS3r_zJf98XT7OQ+-Jt4EWkaFQTDw@mail.gmail.com","subject":"Re: [PATCH v2] t9832,t2200: avoid using pipes in git commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-18T20:04:47Z","receivedAt":"2020-10-18T20:04:52Z","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> precise, the subject could say:\n>\n>     t2200,t9832: avoid using `git` upstream in a pipe\n>\n> Nit: It's subjective, but it feels a bit more natural to list the test\n> numbers in ascending order rather than descending order, which is why\n> I swapped them around in the example above.\n\n;-)\n\n>> When a git command is upstream in a pipe, an unexpected failure of\n>> the git command will go unnoticed.\n>>\n>> Write out the output of the git command to a file, so as to actively\n>> catch a failure of the git command.\n>\n> It's easy to see from the patch itself that the output of the Git\n> command is now written to a file, so it's not necessary to say so in\n> the commit message. Therefore, the entire body of the commit message\n> could be written more succinctly, perhaps like this:\n>\n>     Avoid placing `git` upstream in a pipe since doing so throws away\n>     its exit code, thus an unexpected failure may go unnoticed.\n\nYup.\n\n> The actual patch itself looks fine, and these comments about the\n> commit message are quite minor, thus there probably is no need to\n> re-roll (though feel free to do so if you think the bit of extra\n> polishing of the commit message is worthwhile).\n\nIIUC, the microproject experience aims new contributors to get used\nto the style of communication that happens during review cycles of a\ntypical topic, using a trivial dip-the-toes-in-the-water problem as\nan example.  I'd rather not to see contributors get into the habit\nof leaving loose ends and have somebody else clean after them.\n\nThanks.\n\n"},{"id":"407863","messageId":"CAGxm6oVtOuQjOEbPVtn_KOjE9nBmW=49jkdE-KRdAg-i0Quxsg@mail.gmail.com","threadId":"54449","inReplyTo":"CAPig+cR7Hm9m1EiWkr5tKYS3r_zJf98XT7OQ+-Jt4EWkaFQTDw@mail.gmail.com","subject":"Re: [PATCH v2] t9832,t2200: avoid using pipes in git commands","fromName":"Amanda Shafack","fromEmail":"shafack.likhene@gmail.com","sentAt":"2020-10-18T20:40:54Z","receivedAt":"2020-10-18T20:41:34Z","isPatch":true,"sender":{"key":"shafack.likhene@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17765231?v=4"},"body":"On Sun, Oct 18, 2020 at 8:25 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Sun, Oct 18, 2020 at 10:42 AM Amanda Shafack via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> > t9832,t2200: avoid using pipes in git commands\n>\n> The subject is a bit confusing since pipes aren't used in Git\n> commands; instead, Git commands may be components of pipes. However,\n> even that is too imprecise. The issue this patch is addressing is that\n> we want to avoid Git commands _upstream_ in a pipe. It's perfectly\n> acceptable for the Git command to be the final element of a pipe since\n> the pipe returns the exit code of the final command. So, to be more\n> precise, the subject could say:\n>\n>     t2200,t9832: avoid using `git` upstream in a pipe\n>\n> Nit: It's subjective, but it feels a bit more natural to list the test\n> numbers in ascending order rather than descending order, which is why\n> I swapped them around in the example above.\n>\n\nI agree it looks more appropriate.\n\n> > When a git command is upstream in a pipe, an unexpected failure of\n> > the git command will go unnoticed.\n> >\n> > Write out the output of the git command to a file, so as to actively\n> > catch a failure of the git command.\n>\n> It's easy to see from the patch itself that the output of the Git\n> command is now written to a file, so it's not necessary to say so in\n> the commit message. Therefore, the entire body of the commit message\n> could be written more succinctly, perhaps like this:\n>\n>     Avoid placing `git` upstream in a pipe since doing so throws away\n>     its exit code, thus an unexpected failure may go unnoticed.\n>\n> The actual patch itself looks fine, and these comments about the\n> commit message are quite minor, thus there probably is no need to\n> re-roll (though feel free to do so if you think the bit of extra\n> polishing of the commit message is worthwhile).\n\nI believe it's best practice to optimize one's work as much as\npossible, so I have included these changes.\nThanks for the detailed explanation.\n\n> Thanks.\n\n\n\n-- \n\nCheers!\n\nAmanda  Shafack\n"},{"id":"407864","messageId":"pull.885.v3.git.git.1603054088108.gitgitgadget@gmail.com","threadId":"54449","inReplyTo":"pull.885.v2.git.git.1603032125151.gitgitgadget@gmail.com","subject":"[PATCH v3] t2200,t9832: avoid using 'git' upstream in a pipe","fromName":"Amanda Shafack via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-18T20:48:07Z","receivedAt":"2020-10-18T20:48:12Z","isPatch":true,"sender":{"key":"shafack.likhene@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17765231?v=4"},"body":"From: Amanda Shafack <shafack.likhene@gmail.com>\n\nAvoid placing `git` upstream in a pipe since doing so throws away\nits exit code, thus an unexpected failure may go unnoticed.\n\nSigned-off-by: Amanda Shafack <shafack.likhene@gmail.com>\n---\n    [Outreachy][Patch v3] t2200,t9832: avoid using 'git' upstream in a pipe\n    \n    Changes since v2:\n    \n     * Optimized commit message\n    \n    Signed-off-by: Amanda Shafack shafack.likhene@gmail.com\n    [shafack.likhene@gmail.com]\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-885%2Flkmandy%2Favoid-pipes-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-885/lkmandy/avoid-pipes-v3\nPull-Request: https://github.com/git/git/pull/885\n\nRange-diff vs v2:\n\n 1:  bca393e69a ! 1:  d5f508b055 t9832,t2200: avoid using pipes in git commands\n     @@ Metadata\n      Author: Amanda Shafack <shafack.likhene@gmail.com>\n      \n       ## Commit message ##\n     -    t9832,t2200: avoid using pipes in git commands\n     +    t2200,t9832: avoid using 'git' upstream in a pipe\n      \n     -    When a git command is upstream in a pipe, an unexpected failure of\n     -    the git command will go unnoticed.\n     -\n     -    Write out the output of the git command to a file, so as to actively\n     -    catch a failure of the git command.\n     +    Avoid placing `git` upstream in a pipe since doing so throws away\n     +    its exit code, thus an unexpected failure may go unnoticed.\n      \n          Signed-off-by: Amanda Shafack <shafack.likhene@gmail.com>\n      \n\n\n t/t2200-add-update.sh | 3 ++-\n t/t9832-unshelve.sh   | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\nindex f764b7e3f5..7cb7a70382 100755\n--- a/t/t2200-add-update.sh\n+++ b/t/t2200-add-update.sh\n@@ -179,7 +179,8 @@ test_expect_success 'add -u resolves unmerged paths' '\n \n test_expect_success '\"add -u non-existent\" should fail' '\n \ttest_must_fail git add -u non-existent &&\n-\t! (git ls-files | grep \"non-existent\")\n+\tgit ls-files >actual &&\n+\t! grep \"non-existent\" actual\n '\n \n test_done\ndiff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh\nindex 7194fb2855..6b3cb0414a 100755\n--- a/t/t9832-unshelve.sh\n+++ b/t/t9832-unshelve.sh\n@@ -68,7 +68,8 @@ EOF\n \t\tcd \"$git\" &&\n \t\tchange=$(last_shelved_change) &&\n \t\tgit p4 unshelve $change &&\n-\t\tgit show refs/remotes/p4-unshelved/$change | grep -q \"Further description\" &&\n+\t\tgit show refs/remotes/p4-unshelved/$change >actual &&\n+\t\tgrep -q \"Further description\" actual &&\n \t\tgit cherry-pick refs/remotes/p4-unshelved/$change &&\n \t\ttest_path_is_file file2 &&\n \t\ttest_cmp file1 \"$cli\"/file1 &&\n\nbase-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a\n-- \ngitgitgadget\n"},{"id":"407865","messageId":"xmqqv9f7uu2l.fsf@gitster.c.googlers.com","threadId":"54449","inReplyTo":"pull.885.v3.git.git.1603054088108.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] t2200,t9832: avoid using 'git' upstream in a pipe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-18T21:01:54Z","receivedAt":"2020-10-18T21:02:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Amanda  Shafack  via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Amanda Shafack <shafack.likhene@gmail.com>\n>\n> Avoid placing `git` upstream in a pipe since doing so throws away\n> its exit code, thus an unexpected failure may go unnoticed.\n\nWell explained.\n\n> Signed-off-by: Amanda Shafack <shafack.likhene@gmail.com>\n> ---\n\n\n\n> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\n> index f764b7e3f5..7cb7a70382 100755\n> --- a/t/t2200-add-update.sh\n> +++ b/t/t2200-add-update.sh\n> @@ -179,7 +179,8 @@ test_expect_success 'add -u resolves unmerged paths' '\n>  \n>  test_expect_success '\"add -u non-existent\" should fail' '\n>  \ttest_must_fail git add -u non-existent &&\n> -\t! (git ls-files | grep \"non-existent\")\n> +\tgit ls-files >actual &&\n> +\t! grep \"non-existent\" actual\n>  '\n\nIn the scope of this patch, the above change is a good rewrite.\nLet's stop the iteration here---you've demonstrated through the\nmicroproject that you can now comfortably be involved in the review\ndiscussion.\n\nIn a larger scope of \"can we write this particular line better?\",\nhowever, the above may not be the _best_ answer.  For example,\n\n\ttest_must_fail git ls-files --error-unmatch non-existent\n\nwould be another and a more direct way to ensure that the named path\ndoes not appear in the index.\n\nThanks.\n"},{"id":"407866","messageId":"CAGxm6oWAmL8db0ycsQ8QqUGugrh7MU8eOS8O1rt-ASLuwBayug@mail.gmail.com","threadId":"54449","inReplyTo":"xmqqsgab7128.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] t9832,t2200: avoid using pipes in git commands","fromName":"Amanda Shafack","fromEmail":"shafack.likhene@gmail.com","sentAt":"2020-10-18T22:04:18Z","receivedAt":"2020-10-18T22:05:18Z","isPatch":true,"sender":{"key":"shafack.likhene@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17765231?v=4"},"body":"On Sun, Oct 18, 2020 at 9:04 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>\n> > precise, the subject could say:\n> >\n> >     t2200,t9832: avoid using `git` upstream in a pipe\n> >\n> > Nit: It's subjective, but it feels a bit more natural to list the test\n> > numbers in ascending order rather than descending order, which is why\n> > I swapped them around in the example above.\n>\n> ;-)\n>\n> >> When a git command is upstream in a pipe, an unexpected failure of\n> >> the git command will go unnoticed.\n> >>\n> >> Write out the output of the git command to a file, so as to actively\n> >> catch a failure of the git command.\n> >\n> > It's easy to see from the patch itself that the output of the Git\n> > command is now written to a file, so it's not necessary to say so in\n> > the commit message. Therefore, the entire body of the commit message\n> > could be written more succinctly, perhaps like this:\n> >\n> >     Avoid placing `git` upstream in a pipe since doing so throws away\n> >     its exit code, thus an unexpected failure may go unnoticed.\n>\n> Yup.\n>\n> > The actual patch itself looks fine, and these comments about the\n> > commit message are quite minor, thus there probably is no need to\n> > re-roll (though feel free to do so if you think the bit of extra\n> > polishing of the commit message is worthwhile).\n>\n> IIUC, the microproject experience aims new contributors to get used\n> to the style of communication that happens during review cycles of a\n> typical topic, using a trivial dip-the-toes-in-the-water problem as\n> an example.  I'd rather not to see contributors get into the habit\n> of leaving loose ends and have somebody else clean after them.\n\nTaken very seriously\n\n> Thanks.\n\n\n-- \n\nCheers!\n\nAmanda  Shafack\n"}]}