Please write good commit messages before asking for code review

[Ehsan Akhgari]

I review a large number of patches on a typical day, and usually I have to
spend a fair amount of time to just understand what the patch is doing. As
the patch author, you can do a lot to help make this easier by *writing
better commit messages*. Starting now, I'm going to try out a new practice
for a while: I'm going to first review the commit message of all patches,
and if I can't understand what the patch does by reading the commit message
before reading any of the code, I'll r- and ask for another version of the
patch.

I thank you in advance for your cooperation by writing great commit
messages. Happy hacking!

Cheers,
--
Ehsan

[Mike Conley]

Incidentally, MozReview now allows you to provide feedback on the commit
message in the diffviewer.

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Boris Zbarsky]

On 3/9/17 2:46 PM, Ehsan Akhgari wrote:
> Starting now, I'm going to try out a new practice
> for a while: I'm going to first review the commit message of all patches,
> and if I can't understand what the patch does by reading the commit message
> before reading any of the code, I'll r- and ask for another version of the
> patch.

I will be doing likewise.

I believe dbaron does this already.

I encourage others to do this too.

-Boris

[Kyle Machulis]

This has actually been confusing me in reviews, since the commit-message
looks like another file in the diff. Not sure of a better way to show this
isn't going to be a file when checked in, but with the way it's shown as
part of the change set in the patch right now, it's hard to discern.

On Thu, Mar 9, 2017 at 11:53 AM, Mike Conley <mco...@mozilla.com> wrote:

> Incidentally, MozReview now allows you to provide feedback on the commit
> message in the diffviewer.
>
> On Mar 9, 2017 2:47 PM, "Ehsan Akhgari" <ehsan....@gmail.com> wrote:
>
> > I review a large number of patches on a typical day, and usually I have
> to
> > spend a fair amount of time to just understand what the patch is doing.
> As
> > the patch author, you can do a lot to help make this easier by *writing

> > better commit messages*. Starting now, I'm going to try out a new

> practice
> > for a while: I'm going to first review the commit message of all patches,
> > and if I can't understand what the patch does by reading the commit
> message
> > before reading any of the code, I'll r- and ask for another version of
> the
> > patch.
> >

[Eric Rescorla]

I'm in favor of good commit messages, but I would note that current m-c
convention really pushes against this, because people seem to feel that
commit messages should be one line. Not sure what to do about that,
but thought I would mention it.

-Ekr

[Boris Zbarsky]

On 3/9/17 4:35 PM, Eric Rescorla wrote:
> I'm in favor of good commit messages, but I would note that current m-c
> convention really pushes against this, because people seem to feel that
> commit messages should be one line.

They feel wrong, and we should tell them so. ;) The first line should
include a brief summary of the change. The rest of the commit message
should explain details as needed.

-Boris

[Mark Côté]

Note that commit messages now show up as "virtual" files in MozReview
and can be commented on and issues raised against.

Mark

[L. David Baron]

On Thursday 2017-03-09 13:35 -0800, Eric Rescorla wrote:
> I'm in favor of good commit messages, but I would note that current m-c
> convention really pushes against this, because people seem to feel that

> commit messages should be one line. Not sure what to do about that,
> but thought I would mention it.

In Mercurial, the first line of the commit message has different
semantics from the rest of the lines, in that some tools display
only the first line of the commit message and not the whole message.
This is true of "hg log" (without -v), and also of a bunch of the
tools we have that look at Mercurial commits (e.g., treeherder).
(I'm not sure if this is true for any git tools, though.)

So the style I want people to use is making the first line be a
summary description of what's being changed (probably 60-120
characters, without wrapping), and then (when needed, which is a
significant portion of the time) explaining in more detail in later
wrapped lines.

I think there are a decent number of commits in m-c that do use
multi-line commit messages (although not as many as I like), but
many of the tools that show lists of commits hide all but the first
line (as I think they should, for summary views).

An old example of this that I like is this commit:
https://hg.mozilla.org/mozilla-central/rev/830111e10951

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla https://www.mozilla.org/ 𝄂
Before I built a wall I'd ask to know
What I was walling in or walling out,
And to whom I was like to give offense.
- Robert Frost, Mending Wall (1914)

[Nicholas Alexander]

Greg Szorc has been a vocal proponent of descriptive commit messages -- I
consider https://bugzilla.mozilla.org/show_bug.cgi?id=1271035 a work of art
-- and has converted many, including myself, to the cause. I'm thrilled to
see this practice get traction!

Nick

[Mark Côté]

Oops just saw this after I posted separately about this feature.

Yeah, I agree it's a bit confusing. We have a few ideas for making this
better differentiated; will open a bug.

Mark

[Andrew McCreight]

On Thu, Mar 9, 2017 at 1:55 PM, Nicholas Alexander <nalex...@mozilla.com>
wrote:

> Greg Szorc has been a vocal proponent of descriptive commit messages -- I
> consider https://bugzilla.mozilla.org/show_bug.cgi?id=1271035 a work of
> art
> -- and has converted many, including myself, to the cause. I'm thrilled to
> see this practice get traction!
>

While that is certainly entertaining to read, personally I don't think it
is a great commit message. Anybody who wants to figure out what the patch
is actually doing and why it is doing it has to read through paragraphs
about barleywine to find that out.

On the subject of long commit messages, here's a commit message I wrote
that had 3 paragraphs to explain a patch that just changed a 0 to a 1:
https://hg.mozilla.org/integration/autoland/rev/bf059ec2bdc9

[Nicholas Alexander]

On Thu, Mar 9, 2017 at 2:07 PM, Andrew McCreight <amccr...@mozilla.com>
wrote:

> On Thu, Mar 9, 2017 at 1:55 PM, Nicholas Alexander <nalex...@mozilla.com
> >
> wrote:
>

> > Greg Szorc has been a vocal proponent of descriptive commit messages -- I
> > consider https://bugzilla.mozilla.org/show_bug.cgi?id=1271035 a work of
> > art
> > -- and has converted many, including myself, to the cause. I'm thrilled
> to
> > see this practice get traction!
> >
>
> While that is certainly entertaining to read, personally I don't think it
> is a great commit message. Anybody who wants to figure out what the patch
> is actually doing and why it is doing it has to read through paragraphs
> about barleywine to find that out.
>

I suppose art is in the eye of the beholder.

>
> On the subject of long commit messages, here's a commit message I wrote
> that had 3 paragraphs to explain a patch that just changed a 0 to a 1:
> https://hg.mozilla.org/integration/autoland/rev/bf059ec2bdc9

This must have come up in a different discussion, 'cuz I've read (and
admired!) this particular commit message before.

Nick

[Mike Hommey]

On Thu, Mar 09, 2017 at 02:46:53PM -0500, Ehsan Akhgari wrote:
> I review a large number of patches on a typical day, and usually I have to
> spend a fair amount of time to just understand what the patch is doing. As
> the patch author, you can do a lot to help make this easier by *writing
> better commit messages*. Starting now, I'm going to try out a new practice
> for a while: I'm going to first review the commit message of all patches,
> and if I can't understand what the patch does by reading the commit message
> before reading any of the code, I'll r- and ask for another version of the
> patch.

Sometimes, the commit message does explain what it does in a sufficient
manner, but finding out why requires reading the bug, assuming it's
written there. I think this information should also be in the commit
message.

Mike

[Ben Kelly]

(Just continuing the thread here.)

Personally I prefer looking at the bug for the full context and single
point of truth. Also, security bugs typically can't have extensive commit
messages and moving a lot of context to commit messages might paint a
target on security patches.

[Mike Hommey]

On Thu, Mar 09, 2017 at 02:07:51PM -0800, Andrew McCreight wrote:
> On Thu, Mar 9, 2017 at 1:55 PM, Nicholas Alexander <nalex...@mozilla.com>
> wrote:
>

> > Greg Szorc has been a vocal proponent of descriptive commit messages -- I
> > consider https://bugzilla.mozilla.org/show_bug.cgi?id=1271035 a work of
> > art
> > -- and has converted many, including myself, to the cause. I'm thrilled to
> > see this practice get traction!
> >
>
> While that is certainly entertaining to read, personally I don't think it
> is a great commit message. Anybody who wants to figure out what the patch
> is actually doing and why it is doing it has to read through paragraphs
> about barleywine to find that out.
>

> On the subject of long commit messages, here's a commit message I wrote
> that had 3 paragraphs to explain a patch that just changed a 0 to a 1:
> https://hg.mozilla.org/integration/autoland/rev/bf059ec2bdc9

I'd argue some of this commit message should actually be in the
code comment.

Mike

[Jeff Muizelaar]

On Thu, Mar 9, 2017 at 5:43 PM, Ben Kelly <bke...@mozilla.com> wrote:
> Personally I prefer looking at the bug for the full context and single
> point of truth. Also, security bugs typically can't have extensive commit
> messages and moving a lot of context to commit messages might paint a
> target on security patches.

The bug being inaccessible already makes collecting a list of security
patches trivial.

-Jeff

[Eric Rescorla]

On Thu, Mar 9, 2017 at 2:43 PM, Ben Kelly <bke...@mozilla.com> wrote:

> On Thu, Mar 9, 2017 at 5:35 PM, Mike Hommey <m...@glandium.org> wrote:
>
> > On Thu, Mar 09, 2017 at 02:46:53PM -0500, Ehsan Akhgari wrote:
> > > I review a large number of patches on a typical day, and usually I have
> > to
> > > spend a fair amount of time to just understand what the patch is doing.
> > As
> > > the patch author, you can do a lot to help make this easier by *writing
> > > better commit messages*. Starting now, I'm going to try out a new
> > practice
> > > for a while: I'm going to first review the commit message of all
> patches,
> > > and if I can't understand what the patch does by reading the commit
> > message
> > > before reading any of the code, I'll r- and ask for another version of
> > the
> > > patch.
> >
> > Sometimes, the commit message does explain what it does in a sufficient
> > manner, but finding out why requires reading the bug, assuming it's
> > written there. I think this information should also be in the commit
> > message.
>
>
> (Just continuing the thread here.)
>

> Personally I prefer looking at the bug for the full context and single
> point of truth. Also, security bugs typically can't have extensive commit
> messages and moving a lot of context to commit messages might paint a
> target on security patches.
>

Can't you determine that by just looking to see if the bug is visible?

-Ekr

[Ehsan Akhgari]

On 2017-03-09 5:07 PM, Andrew McCreight wrote:
> On Thu, Mar 9, 2017 at 1:55 PM, Nicholas Alexander <nalex...@mozilla.com>
> wrote:
>

Yes, what Boris, David and others said. I think the above is a great
and succinct summary of what a good commit message should be like.

>>
>> Greg Szorc has been a vocal proponent of descriptive commit messages -- I
>> consider https://bugzilla.mozilla.org/show_bug.cgi?id=1271035 a work of
>> art
>> -- and has converted many, including myself, to the cause. I'm thrilled to
>> see this practice get traction!
>>
>
> While that is certainly entertaining to read, personally I don't think it
> is a great commit message. Anybody who wants to figure out what the patch
> is actually doing and why it is doing it has to read through paragraphs
> about barleywine to find that out.

Ugh.. Yeah this is a pretty bad commit message, definitely a good
example of what I'd r-. While I appreciate humor in its right place, I
don't think that is the purpose of a commit message. Neither is telling
a story. I really hope this is obvious...

> On the subject of long commit messages, here's a commit message I wrote
> that had 3 paragraphs to explain a patch that just changed a 0 to a 1:
> https://hg.mozilla.org/integration/autoland/rev/bf059ec2bdc9

I think the length of the commit message is entirely missing the point.
"Typo fix" is a perfectly fine commit message depending on the commit.
The point is helping the reviewer and future readers of the commit to
understand what it is that it's doing...

[Ehsan Akhgari]

On 2017-03-09 5:35 PM, Mike Hommey wrote:
> On Thu, Mar 09, 2017 at 02:46:53PM -0500, Ehsan Akhgari wrote:
>> I review a large number of patches on a typical day, and usually I have to
>> spend a fair amount of time to just understand what the patch is doing. As
>> the patch author, you can do a lot to help make this easier by *writing
>> better commit messages*. Starting now, I'm going to try out a new practice
>> for a while: I'm going to first review the commit message of all patches,
>> and if I can't understand what the patch does by reading the commit message
>> before reading any of the code, I'll r- and ask for another version of the
>> patch.
>
> Sometimes, the commit message does explain what it does in a sufficient
> manner, but finding out why requires reading the bug, assuming it's
> written there. I think this information should also be in the commit
> message.

Yes, and in case the commit message is missing this information, that is
also grounds for r-. :-)

[Ben Kelly]

On Thu, Mar 9, 2017 at 5:48 PM, Eric Rescorla <e...@rtfm.com> wrote:

>
>
> On Thu, Mar 9, 2017 at 2:43 PM, Ben Kelly <bke...@mozilla.com> wrote:
>
>> (Just continuing the thread here.)
>>
>> Personally I prefer looking at the bug for the full context and single
>> point of truth. Also, security bugs typically can't have extensive commit
>> messages and moving a lot of context to commit messages might paint a
>> target on security patches.
>>
>
> Can't you determine that by just looking to see if the bug is visible?
>

So you are saying we should just write SECURE BUG REDACTED in these commit
messages now? Or do we have to fabricate a paragraph to match other
commits now?

Right now our security bug process asks about the commit message and if it
"paints a target" on the patch. If you want to change our commit message
policy, please adjust that or take it into account.

And I also agree with the other commenters here that complexity should be
described in code comments.

Ultimately as long as the code is explained via comments, the bug is
up-to-date, and our secure bug process isn't broken I don't have a strong
opinion here.

[Ehsan Akhgari]

On 2017-03-09 5:48 PM, Eric Rescorla wrote:
>
>
> On Thu, Mar 9, 2017 at 2:43 PM, Ben Kelly <bke...@mozilla.com

> <mailto:bke...@mozilla.com>> wrote:

>
> On Thu, Mar 9, 2017 at 5:35 PM, Mike Hommey <m...@glandium.org
> <mailto:m...@glandium.org>> wrote:
>
> > On Thu, Mar 09, 2017 at 02:46:53PM -0500, Ehsan Akhgari wrote:
> > > I review a large number of patches on a typical day, and usually I have
> > to
> > > spend a fair amount of time to just understand what the patch is doing.
> > As
> > > the patch author, you can do a lot to help make this easier by *writing
> > > better commit messages*. Starting now, I'm going to try out a new
> > practice
> > > for a while: I'm going to first review the commit message of all patches,
> > > and if I can't understand what the patch does by reading the commit
> > message
> > > before reading any of the code, I'll r- and ask for another version of
> > the
> > > patch.
> >
> > Sometimes, the commit message does explain what it does in a sufficient
> > manner, but finding out why requires reading the bug, assuming it's
> > written there. I think this information should also be in the commit
> > message.
>
>

> (Just continuing the thread here.)
>
> Personally I prefer looking at the bug for the full context and single
> point of truth. Also, security bugs typically can't have extensive
> commit
> messages and moving a lot of context to commit messages might paint a
> target on security patches.
>
>
> Can't you determine that by just looking to see if the bug is visible?

Yeah, but I think Ben's main point is that for *security fixes* you
aren't supposed to disclose any security sensitive information in the
commit message that isn't obvious from the code change. I usually try
to basically describe the code change in English, therefore not really
reveal any information other than what is available in the diff itself
in the commit message.

At any rate, security fixes are an exception to the general rule.

[Boris Zbarsky]

On 3/9/17 5:53 PM, Ben Kelly wrote:
> Right now our security bug process asks about the commit message and if it
> "paints a target" on the patch.

It asks this:

Do comments in the patch, the check-in comment, or tests included
in the patch paint a bulls-eye on the security problem?

I always interpreted that to mean "does the commit message give clues
about how to exploit the pre-patch state?". So a commit message like
"Make sure that SVG elements with a 'fill' attribute that are adopted
into another document don't leave dangling pointers behind" would
probably be a bad idea for a security bug....

I don't think there's anything you can do in the commit message to not
make it scream out "security bug" to anyone who's actually trying, if
you include the bug# in there....

-Boris

[Eric Rescorla]

On Thu, Mar 9, 2017 at 2:53 PM, Ben Kelly <bke...@mozilla.com> wrote:

>
>
> On Thu, Mar 9, 2017 at 5:48 PM, Eric Rescorla <e...@rtfm.com> wrote:

>
>>
>>
>> On Thu, Mar 9, 2017 at 2:43 PM, Ben Kelly <bke...@mozilla.com> wrote:
>>
>>> (Just continuing the thread here.)
>>>
>>> Personally I prefer looking at the bug for the full context and single
>>> point of truth. Also, security bugs typically can't have extensive
>>> commit
>>> messages and moving a lot of context to commit messages might paint a
>>> target on security patches.
>>>
>>
>> Can't you determine that by just looking to see if the bug is visible?
>>
>

> So you are saying we should just write SECURE BUG REDACTED in these commit
> messages now? Or do we have to fabricate a paragraph to match other
> commits now?
>

I'm not saying either of these things. What I am saying is that it's
trivial to determine
security bugs by checking to see if you can see them in Bugzilla. Do you
disagree
with this?

Right now our security bug process asks about the commit message and if it

> "paints a target" on the patch. If you want to change our commit message
> policy, please adjust that or take it into account.
>
> And I also agree with the other commenters here that complexity should be
> described in code comments.
>

You are arguing with other people here, not me.

-Ekr

[Mike Hommey]

On Thu, Mar 09, 2017 at 05:50:36PM -0500, Ehsan Akhgari wrote:
> > On the subject of long commit messages, here's a commit message I wrote
> > that had 3 paragraphs to explain a patch that just changed a 0 to a 1:
> > https://hg.mozilla.org/integration/autoland/rev/bf059ec2bdc9
>
> I think the length of the commit message is entirely missing the point.
> "Typo fix" is a perfectly fine commit message depending on the commit.
> The point is helping the reviewer and future readers of the commit to
> understand what it is that it's doing...

Come to think of it, I think there are two orthogonal goals, and that
only one of them should be addressed in commit messages.

- why the code has been changed.
- what the code is doing.

The former belongs in the commit message, but the latter, most of the
time, belongs in code comments. It's painful to figure out why some
tricky code works the way it works by looking at its history (which
usually requires jumping through several commits before finding the one
that actually introduced the code), when it could have been spelled out
in a comment.

Mike

[L. David Baron]

On Friday 2017-03-10 07:46 +0900, Mike Hommey wrote:
> I'd argue some of this commit message should actually be in the
> code comment.

Yes. The commit message should be largely about *change* (how this
revision of code is different from earlier ones), that is, what is
changing, why it's changing, what justifies that the change is safe,
how other code needs to adapt to the change. The comments in the
code should be about the static state of the code (that is, state as
of the new revision, rather than across revisions), including what
justifies that the code is correct, safe, etc.

Information about why the static state of the code is correct is
better placed in comments than the commit message, although if the
patch is large it may need to summarize, or point to particular
comments (e.g., "see the comment in Foo.h for a full description of
these new methods").

[Ehsan Akhgari]

On 2017-03-09 6:03 PM, L. David Baron wrote:
> On Friday 2017-03-10 07:46 +0900, Mike Hommey wrote:
>> I'd argue some of this commit message should actually be in the
>> code comment.
>
> Yes. The commit message should be largely about *change* (how this
> revision of code is different from earlier ones), that is, what is
> changing, why it's changing, what justifies that the change is safe,
> how other code needs to adapt to the change. The comments in the
> code should be about the static state of the code (that is, state as
> of the new revision, rather than across revisions), including what
> justifies that the code is correct, safe, etc.
>
> Information about why the static state of the code is correct is
> better placed in comments than the commit message, although if the
> patch is large it may need to summarize, or point to particular
> comments (e.g., "see the comment in Foo.h for a full description of
> these new methods").

I could not have put this any better.

[Gabriele Svelto]

On 09/03/2017 22:35, Eric Rescorla wrote:
> I'm in favor of good commit messages, but I would note that current m-c
> convention really pushes against this, because people seem to feel that

> commit messages should be one line. Not sure what to do about that,
> but thought I would mention it.

Yeah, I always thought we had a policy of one-line commit messages but
I think that originated back in the day when one would attach a patch on
Bugzilla and added a comment there describing it in detail. With
mozreview it seems more sensible to put that detailed description in the
commit message.

Gabriele

[Honza Bambas]

Thanks for bringing this up here, Ehsan.

I have to concur. For more then just trivial patches I usually ask for
a description of the patch in bugzilla - the commit message is usually
not enough. Anything that can help the review by understanding the
intention and structure of the patch helps a lot and often is even
necessary. The more the better. From time to time I r- a patch and ask
for this info if I find it too complicated to review without it.

Thank you as well!

-hb-

On 3/9/17 8:46 PM, Ehsan Akhgari wrote:
> I review a large number of patches on a typical day, and usually I have to
> spend a fair amount of time to just understand what the patch is doing. As
> the patch author, you can do a lot to help make this easier by *writing
> better commit messages*. Starting now, I'm going to try out a new practice
> for a while: I'm going to first review the commit message of all patches,
> and if I can't understand what the patch does by reading the commit message
> before reading any of the code, I'll r- and ask for another version of the
> patch.
>

[smaug]

Exactly. In practice I tend to just read the first line of a commit message and the most important part there is the bug number.
I've never been a fan of overlong commit messages.
MozReview makes this a bit different since it decouples bugs and changes, so there seeing more explanation in the commit message is more useful.

[smaug]

On 03/10/2017 12:43 AM, Ben Kelly wrote:

[tusha...@gmail.com]

On Thursday, March 9, 2017 at 11:47:44 AM UTC-8, Ehsan Akhgari wrote:
> I review a large number of patches on a typical day, and usually I have to
> spend a fair amount of time to just understand what the patch is doing. As
> the patch author, you can do a lot to help make this easier by *writing
> better commit messages*. Starting now, I'm going to try out a new practice
> for a while: I'm going to first review the commit message of all patches,
> and if I can't understand what the patch does by reading the commit message
> before reading any of the code, I'll r- and ask for another version of the
> patch.
>

> I thank you in advance for your cooperation by writing great commit
> messages. Happy hacking!
>
> Cheers,

> --
> Ehsan