From: Eric Sunshine <firstname.lastname@example.org> To: Amanda Shafack via GitGitGadget <email@example.com> Cc: Git List <firstname.lastname@example.org>, Emily Shaffer <email@example.com>, Jonathan Nieder <firstname.lastname@example.org>, Amanda Shafack <email@example.com> Subject: Re: [PATCH 1/2] t9832,t2200: avoid using pipes in git commands Date: Sun, 18 Oct 2020 02:11:47 -0400 [thread overview] Message-ID: <CAPig+cQnfcc9W3qE5XkyLiwNyK8Wm2SsAyjzatDWEVUi3s8APw@mail.gmail.com> (raw) In-Reply-To: <firstname.lastname@example.org> On Sat, Oct 17, 2020 at 12:02 PM Amanda Shafack via GitGitGadget <email@example.com> wrote: > When the upstream of a pipe throws an error, the downstream still > executes normally. This happens because the exit code of the upstream > in a pipe is ignored. This behavior can make debugging very hard > incase a test fails. To be more precise, we're not worried about difficulty of debugging when a Git command is upstream in a pipe. What we are more concerned with is that an unexpected failure of the Git command will go unnoticed (unless it also happens to produce wrong output which is later caught somewhere downstream). By avoiding placing Git upstream in a pipe, we can actively catch a failure of the Git command. The commit message should focus on that reason instead. > Also, pipes are prone to deadlocks. If the > upstream gets full, the commands downstream will never start. I don't think this is ever an issue in Git tests, so talking about it here probably only muddies the waters, thus makes the commit message less precise and more hand-wavy. Dropping this sentence is recommended. > Commit c6f44e1da5 (t9813: avoid using pipes, 2017-01-04) noticed that > the exit code of upstream in the pipe is ignored, thus using pipes > should be avoided. There are a lot of commits in the project history which perform this sort of transformation, so it's not clear to the reader why this particular commit is called out as being important. If it's not, then I'd recommend dropping this sentence, as well. > diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh > test_expect_success '"add -u non-existent" should fail' ' > test_must_fail git add -u non-existent && > - ! (git ls-files | grep "non-existent") > + ! ( > + git ls-files >actual && > + grep "non-existent" actual > + ) > ' This transformation appears to be incorrect. See my review of patch [2/2] for details. > diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh > @@ -68,7 +68,8 @@ EOF > - git show refs/remotes/p4-unshelved/$change | grep -q "Further description" && > + git show refs/remotes/p4-unshelved/$change >actual && > + grep -q "Further description" actual && This transformation looks fine.
next prev parent reply other threads:[~2020-10-18 6:12 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-17 16:02 [PATCH 0/2] [Outreachy][Patch v1] t9832,t2200: avoid using pipes in git related commands Amanda Shafack via GitGitGadget 2020-10-17 16:02 ` [PATCH 1/2] t9832,t2200: avoid using pipes in git commands Amanda Shafack via GitGitGadget 2020-10-18 6:11 ` Eric Sunshine [this message] 2020-10-18 13:57 ` Amanda Shafack 2020-10-17 16:02 ` [PATCH 2/2] t2200: modify code syntax Amanda Shafack via GitGitGadget 2020-10-18 5:55 ` Eric Sunshine 2020-10-18 13:59 ` Amanda Shafack 2020-10-18 14:42 ` [PATCH v2] t9832,t2200: avoid using pipes in git commands Amanda Shafack via GitGitGadget 2020-10-18 19:25 ` Eric Sunshine 2020-10-18 20:04 ` Junio C Hamano 2020-10-18 22:04 ` Amanda Shafack 2020-10-18 20:40 ` Amanda Shafack 2020-10-18 20:48 ` [PATCH v3] t2200,t9832: avoid using 'git' upstream in a pipe Amanda Shafack via GitGitGadget 2020-10-18 21:01 ` Junio C Hamano
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAPig+cQnfcc9W3qE5XkyLiwNyK8Wm2SsAyjzatDWEVUi3s8APw@mail.gmail.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH 1/2] t9832,t2200: avoid using pipes in git commands' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).