On Mon, Apr 17, 2017 at 5:12 PM, <gsquel...@mozilla.com> wrote: > On Tuesday, April 18, 2017 at 11:58:11 AM UTC+12, smaug wrote: > > On 04/18/2017 02:36 AM, Gregory Szorc wrote: > > > On Mon, Apr 17, 2017 at 4:10 PM, smaug <sm...@welho.com> wrote: > > > > > >> On 04/17/2017 06:16 PM, Boris Zbarsky 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. > > >>> > > >> > > >> > > >> And reminder, commit messages should *not* be stories about how you > ended > > >> up with this particular change. They should just tell "what" and > "why". > > >> It seems like using mozreview leads occasionally writing stories > (which is > > >> totally fine as a bugzilla comment). > > >> > > > > > > I disagree somewhat. As a reviewer, I often appreciate the extra > context if > > > it helps me - the reviewer - or a future me - an archeologist or patch > > > author - understand the situation better. > > > > That is why we have links to the bug. Bug should always be the unite of > truth telling > > why some change was done. Bugs tend to have so much more context about > the change than any individual commit message can or should have. > > But then some bugs may accumulate hundreds of comments and it becomes > unreasonable to expect future maintainers to read through them all, when > the commit descriptions could just present a selectively-useful history of > the "how we got to this solution". >
Exactly. As a reviewer, when I see an empty commit message and load the bug to get more context and see a wall of text, my reaction is to utter an expletive. But if I see a descriptive commit message and I trust what's written there, I can avoid loading the bug and just get on with reviewing. In other words, that commit message just saved me a bunch of time and got you feedback sooner all without upsetting me. And generally speaking, we want to optimize for reducing time spent on review because reviewers are a more limited (and therefore more valuable) resource than patch authors. Of course, you can't prevent all bug scouring. But I argue it shouldn't be necessary in the common case. Furthermore, I would expect a good commit message to direct the reviewer to the bug if it contains useful context. Along that vein, perhaps we need a way to "flag" a bug/comment so the reviewing interface can tell any future reviewer "see bug / comment for important context." That way we don't have to put so much trust in commit messages (which can have omissions, often unintentionally, possibly maliciously). > > > > > > If that context prevents the > > > reviewer or a future patch author from asking "why didn't we do X [and > > > spending a few hours waiting for a reply or trying to find an answer]" > then > > > that context was justified. I do agree there are frivolous stories > that do > > > little more than entertain (e.g. the first paragraphs of > > > https://hg.mozilla.org/mozilla-central/rev/6b064f9f6e10) and we > should not > > > be encouraging that style. > > > > > > > > >> Overlong unrelated commit messages just make it harder to read blame. > > >> > > > > > > This is the tail wagging the dog. Please file a bug against the tool > for > > > letting best practices interfere with an important workflow. > > > > > _______________________________________________ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
