Re: [PATCH 05/16] t2018: don't lose return code of git commands

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 1, 2020, 09:21 UTC
Message-ID: <CAPig+cSeuZRySuaOAvNTiy1eyH0AQ4is-g8LLA7-wC+CztuH0A@mail.gmail.com>
In-Reply-To: <20200101084840.GC5152@generichostname>

On Wed, Jan 1, 2020 at 3:48 AM Denton Liu <liu.denton@gmail.com> wrote:

Show 15 quoted lines

> On Fri, Dec 27, 2019 at 04:42:53PM -0500, Eric Sunshine wrote:
> > On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@gmail.com> wrote:
> > > We had some git commands wrapped in non-assignment command substitutions
> > > which would result in their return codes to be lost. Rewrite these
> > > instances so that their return codes are now checked.
> >
> > Try writing your commit messages in imperative mood:
> >
> >     Fix invocations of Git commands so their exit codes are not lost
> >     within command substitutions...
>
> I thought that the preferred form of commit messages is to introduce the
> problem that I'm trying to solve first ("We had some git commands losing
> return codes") then, after that, describe the changes I made in
> imperative mood ("Rewrite these instances...").

My suggested rewrite includes both the problem ("exit codes ... lost within command substitutions") and the change in one sentence. The shorter you can make the commit message without omitting important information and without being outright cryptic, the better. Let the patch itself do the talking as much as possible.

Here's an example, from another patch in this series, which provides too much information:

    While we're at it, replace the test_might_fail(), which should
    only be used on git commands, with a compound command that always
    returns 0, even if the underlying ls fails.

The bit about "...with a compound command..." is unnecessary and wastes reviewer time and is far more difficult to understand than simply looking at the code in the patch itself. This would have been sufficient:

    While here, stop using test_might_fail() with 'ls' since it should
    only be used with git commands.

Show 6 quoted lines

> From what I can tell,
> all of my commit messages conform to this template.
> I'd prefer to keep the "problem statement" introduction in my commit
> messages as it primes readers so they know "why" before "what" but I
> can't think of any way to phrase the "problem statement" part in a way
> that sounds good without resorting to past tense. Any suggestions?

Using past tense makes it ambiguous to reviewers whether you're talking about the state of the code which existed some time back or if you're talking about the state of the code immediately before this patch is applied. Try rewriting your commit messages without words like "previously", "before", "had", "now", etc. For instance, the commit message for this patch could be rewritten (keeping your wording as much as possible) like this:

    t2018: don't lose return code of git commands

    The exit code of git commands wrapped in non-assignment command
    substitutions is lost. Rewrite these invocations so the exit codes
    are checked.