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 v2] t9832,t2200: avoid using pipes in git commands Date: Sun, 18 Oct 2020 15:25:40 -0400 [thread overview] Message-ID: <CAPig+cR7Hm9m1EiWkr5tKYS3r_zJf98XT7OQ+-Jt4EWkaFQTDw@mail.gmail.com> (raw) In-Reply-To: <firstname.lastname@example.org> 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. > 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). Thanks.
next prev parent reply other threads:[~2020-10-18 19:35 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 [this message] 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+cR7Hm9m1EiWkr5tKYS3r_zJf98XT7OQ+-Jt4EWkaFQTDw@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).