On Tue, Apr 18, 2017 at 03:42:40AM +0300, smaug wrote: > On 04/18/2017 03:12 AM, 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". > > I've yet to see that to happen. What is crucial is fast way to browse > through the blame in time. So commit messages should be short and > descriptive. Telling what and why. (I very often need to go back to CVS era > code). I won't spend time reading several paragraphs of commit messages. > Those are just too long. > Basically first line should tell the essentials 'what' and maybe the most > obvious 'why' and the next couple of lines explaining 'why' more precisely. > Don't write stories which will make blame-browsing slower.
What sort of blame-browsing makes more than the first line of the commit message a burden? I'm curious, because that doesn't match my use of blame. And I've done my share of archeology and for old enough stuff you often end up with one of: - a completely useless commit message *and* useless bug (if there's even a bug number) - a mostly useless commit message and some information in the bug. You're lucky if it's all contained in a few sentences. - a mostly useless commit message and tons of information in the bug, that you have to spend quite some time parsing through. In *all* cases, you have to go switch between your blame and bugzilla. It's a PITA. Now, when you have everything in VCS, you just work with your blame. Obviously, CVS-era commits are not going to change, but do you really want to keep things in the same state for commits done now, when you (or someone else) goes through blame in a few years? Mike _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
