Re: [PATCH v2] t9832,t2200: avoid using pipes in git commands

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 18, 2020, 19:25 UTC
Message-ID: <CAPig+cR7Hm9m1EiWkr5tKYS3r_zJf98XT7OQ+-Jt4EWkaFQTDw@mail.gmail.com>
In-Reply-To: <pull.885.v2.git.git.1603032125151.gitgitgadget@gmail.com>

On Sun, Oct 18, 2020 at 10:42 AM Amanda Shafack via GitGitGadget <gitgitgadget@gmail.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.

Show 5 quoted lines

> 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.