On Wed, Jan 1, 2020 at 3:48 AM Denton Liu [off-list ref] wrote:

On Fri, Dec 27, 2019 at 04:42:53PM -0500, Eric Sunshine wrote:

quoted

On Fri, Dec 27, 2019 at 8:48 AM Denton Liu [off-list ref] wrote:

quoted

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.

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.