Re: Please write good commit messages before asking for code review

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

Re: Please write good commit messages before asking for code review

On 03/10/2017 12:43 AM, Ben Kelly wrote: On Thu, Mar 9, 2017 at 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

Re: Please write good commit messages before asking for code review

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

Re: Please write good commit messages before asking for code review

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.

Re: Please write good commit messages before asking for code review

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

Re: Please write good commit messages before asking for code review

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

Re: Please write good commit messages before asking for code review

On Thu, Mar 9, 2017 at 2:53 PM, Ben Kelly wrote: > > > On Thu, Mar 9, 2017 at 5:48 PM, Eric Rescorla wrote: > >> >> >> On Thu, Mar 9, 2017 at 2:43 PM, Ben Kelly wrote: >> >>> (Just continuing the thread here.) >>> >>> Personally I prefer

Re: Please write good commit messages before asking for code review

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

Re: Please write good commit messages before asking for code review

On 2017-03-09 5:48 PM, Eric Rescorla wrote: > > > On Thu, Mar 9, 2017 at 2:43 PM, Ben Kelly > wrote: > > On Thu, Mar 9, 2017 at 5:35 PM, Mike Hommey > wrote: > > > On Thu, Mar 09, 2017

Re: Please write good commit messages before asking for code review

On Thu, Mar 9, 2017 at 5:48 PM, Eric Rescorla wrote: > > > On Thu, Mar 9, 2017 at 2:43 PM, Ben Kelly wrote: > >> (Just continuing the thread here.) >> >> Personally I prefer looking at the bug for the full context and single >> point of truth. Also, security

Re: Please write good commit messages before asking for code review

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

Re: Please write good commit messages before asking for code review

On 2017-03-09 5:07 PM, Andrew McCreight wrote: > On Thu, Mar 9, 2017 at 1:55 PM, Nicholas Alexander > wrote: > >> On Thu, Mar 9, 2017 at 1:48 PM, Boris Zbarsky wrote: >> >>> On 3/9/17 4:35 PM, Eric Rescorla wrote: >>> I'm in favor of good commit

Re: Please write good commit messages before asking for code review

On Thu, Mar 9, 2017 at 2:43 PM, Ben Kelly wrote: > On Thu, Mar 9, 2017 at 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

Re: Please write good commit messages before asking for code review

On Thu, Mar 9, 2017 at 5:43 PM, Ben Kelly 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 >

Re: Please write good commit messages before asking for code review

On Thu, Mar 09, 2017 at 02:07:51PM -0800, Andrew McCreight wrote: > On Thu, Mar 9, 2017 at 1:55 PM, Nicholas Alexander > wrote: > > > On Thu, Mar 9, 2017 at 1:48 PM, Boris Zbarsky wrote: > > > > > On 3/9/17 4:35 PM, Eric Rescorla wrote: > > > > > >> I'm

Re: Please write good commit messages before asking for code review

On Thu, Mar 9, 2017 at 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

Re: Please write good commit messages before asking for code review

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 >

Re: Please write good commit messages before asking for code review

On Thu, Mar 9, 2017 at 2:07 PM, Andrew McCreight wrote: > On Thu, Mar 9, 2017 at 1:55 PM, Nicholas Alexander > > wrote: > > > On Thu, Mar 9, 2017 at 1:48 PM, Boris Zbarsky wrote: > > > > > On 3/9/17 4:35 PM, Eric Rescorla

Re: Please write good commit messages before asking for code review

On Thu, Mar 9, 2017 at 1:55 PM, Nicholas Alexander wrote: > On Thu, Mar 9, 2017 at 1:48 PM, Boris Zbarsky wrote: > > > 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 > >>

Re: Please write good commit messages before asking for code review

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 On 2017-03-09 3:29 PM, Kyle Machulis wrote: This has actually been confusing me in reviews, since the

Re: Please write good commit messages before asking for code review

On 2017-03-09 4:48 PM, Boris Zbarsky wrote: 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

Re: Please write good commit messages before asking for code review

On Thu, Mar 9, 2017 at 1:48 PM, Boris Zbarsky wrote: > 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

Re: Please write good commit messages before asking for code review

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

Re: Please write good commit messages before asking for code review

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

Re: Please write good commit messages before asking for code review

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 On Thu, Mar 9, 2017 at 12:10 PM, Boris Zbarsky

Re: Please write good commit messages before asking for code review

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,

Re: Please write good commit messages before asking for code review

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

Re: Please write good commit messages before asking for code review

Incidentally, MozReview now allows you to provide feedback on the commit message in the diffviewer. On Mar 9, 2017 2:47 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