On Mon, Apr 17, 2017 at 8:16 AM, Boris Zbarsky <[email protected]> wrote:
> A quick reminder to patch authors and reviewers. > > Changesets should have commit messages. The commit message should > describe not just the "what" of the change but also the "why". This is > especially true in cases when the "what" is obvious from the diff anyway; > for larger changes it makes sense to have a summary of the "what" in the > commit message. > > As a specific example, if your diff is a one-line change that changes a > method call argument from "true" to "false", having a commit message that > says "change argument to mymethod from true to false" is not very helpful > at all. A good commit message in this situation will at least mention the > meaning for the argument. If that does not make it clear why the change is > being made, the commit message should explain the "why". > > Thank you, > Boris > > P.S. Yes, this was prompted by a specific changeset I saw. This > changeset had been marked r+, which means neither the patch author not the > reviewer really thought about this problem. > A litmus test I apply when writing commit messages is "can the reviewer [or any subsequent reader of the changeset] discern enough context from the commit message alone to conduct review or understand the reason for the change?" If yes, it is a good commit message. If no, it may* not be a good commit message. I think it was Richard Newman who first told me this analogy: commit messages are like fragments of a larger picture - if you combine all the commit messages together, you construct a story for the history of the code as if you were reading a good book. A bunch of fragments like "Bug 123456 - Fix foo" don't construct a meaningful story and are therefore not very useful. An explicit goal of commit messages I write is that the reviewer/reader doesn't need to read anything other than the changeset (like Bugzilla comments) to conduct review or understand the changeset's context. That's not to say they won't read those things and/or verify that content of the commit message is true. But in cases where the reviewer trusts the author, the words they've written, and agrees with the premise of the commit message, it can save a ton of the reviewer's time. Also, when I as a reviewer see a detailed commit message, I can assess the comprehension the author has on the subject and this helps me vary scrutiny to different aspects of the change, as appropriate. I can also interpret the commit message as a thesis statement or hypothesis and then gauge whether the code changes actually do that. If the commit message and code aren't in agreement, it's a good sign extra scrutiny is needed. From my experience, the extra context in commit messages has prevented tons of bugs from making it past review. * Sometimes there's just too much context and you need to reference things external to the changeset. _______________________________________________ dev-platform mailing list [email protected] https://lists.mozilla.org/listinfo/dev-platform
