From: Amanda Shafack <firstname.lastname@example.org> To: Eric Sunshine <email@example.com> Cc: Amanda Shafack via GitGitGadget <firstname.lastname@example.org>, Git List <email@example.com>, Emily Shaffer <firstname.lastname@example.org>, Jonathan Nieder <email@example.com> Subject: Re: [PATCH v2] t9832,t2200: avoid using pipes in git commands Date: Sun, 18 Oct 2020 21:40:54 +0100 [thread overview] Message-ID: <CAGxm6oVtOuQjOEbPVtn_KOjE9nBmW=49jkdE-KRdAg-i0Quxsg@mail.gmail.com> (raw) In-Reply-To: <CAPig+cR7Hm9m1EiWkr5tKYS3r_zJf98XT7OQ+-Jt4EWkaFQTDw@mail.gmail.com> On Sun, Oct 18, 2020 at 8:25 PM Eric Sunshine <firstname.lastname@example.org> wrote: > > On Sun, Oct 18, 2020 at 10:42 AM Amanda Shafack via GitGitGadget > <email@example.com> wrote: > > t9832,t2200: avoid using pipes in git commands > > The subject is a bit confusing since pipes aren't used in Git > commands; instead, Git commands may be components of pipes. However, > even that is too imprecise. The issue this patch is addressing is that > we want to avoid Git commands _upstream_ in a pipe. It's perfectly > acceptable for the Git command to be the final element of a pipe since > the pipe returns the exit code of the final command. So, to be more > precise, the subject could say: > > t2200,t9832: avoid using `git` upstream in a pipe > > Nit: It's subjective, but it feels a bit more natural to list the test > numbers in ascending order rather than descending order, which is why > I swapped them around in the example above. > I agree it looks more appropriate. > > When a git command is upstream in a pipe, an unexpected failure of > > the git command will go unnoticed. > > > > Write out the output of the git command to a file, so as to actively > > catch a failure of the git command. > > It's easy to see from the patch itself that the output of the Git > command is now written to a file, so it's not necessary to say so in > the commit message. Therefore, the entire body of the commit message > could be written more succinctly, perhaps like this: > > Avoid placing `git` upstream in a pipe since doing so throws away > its exit code, thus an unexpected failure may go unnoticed. > > The actual patch itself looks fine, and these comments about the > commit message are quite minor, thus there probably is no need to > re-roll (though feel free to do so if you think the bit of extra > polishing of the commit message is worthwhile). I believe it's best practice to optimize one's work as much as possible, so I have included these changes. Thanks for the detailed explanation. > Thanks. -- Cheers! Amanda Shafack
next prev parent reply other threads:[~2020-10-18 20:41 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 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 [this message] 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='CAGxm6oVtOuQjOEbPVtn_KOjE9nBmW=49jkdE-KRdAg-i0Quxsg@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 v2] 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).