On Wed, Jan 1, 2020 at 3:48 AM Denton Liu <liu.denton@xxxxxxxxx> wrote:
> 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@xxxxxxxxx> 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.
> 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.
