Excerpts from Kevin Benton's message of 2015-09-28 14:29:14 -0700: > I think a blanket statement about what people's motivations are is not > fair. We've seen in this thread that some people want to enforce the limit > of 72 chars and it's not about padding their stats. > > The issue here is that we have a guideline with a very specific number. If > we don't care to enforce it, why do we even bother? "Please do this, unless > you don't feel like it", is going to be hard for many people to review in a > way that pleases everyone. >
Please do read said guidelines. "Must" would be used if it were to be "enforced". It "should" be formatted that way. > On Mon, Sep 28, 2015 at 11:00 PM, Assaf Muller <amul...@redhat.com> wrote: > > > > > > > On Mon, Sep 28, 2015 at 12:40 PM, Zane Bitter <zbit...@redhat.com> wrote: > > > >> On 28/09/15 05:47, Gorka Eguileor wrote: > >> > >>> On 26/09, Morgan Fainberg wrote: > >>> > >>>> As a core (and former PTL) I just ignored commit message -1s unless > >>>> there is something majorly wrong (no bug id where one is needed, etc). > >>>> > >>>> I appreciate well formatted commits, but can we let this one go? This > >>>> discussion is so far into the meta-bike-shedding (bike shedding about > >>>> bike > >>>> shedding commit messages) ... If a commit message is *that* bad a -1 (or > >>>> just fixing it?) Might be worth it. However, if a commit isn't missing > >>>> key > >>>> info (bug id? Bp? Etc) and isn't one long incredibly unbroken sentence > >>>> moving from topic to topic, there isn't a good reason to block the > >>>> review. > >>>> > >>> > >> +1 > >> > >> It is not worth having a bot -1 bad commits or even having gerrit muck > >>>> with them. Let's do the job of the reviewer and actually review code > >>>> instead of going crazy with commit messages. > >>>> > >>> > >> +1 > >> > >> Sent via mobile > >>>> > >>>> > >>> I have to disagree, as reviewers we have to make sure that guidelines > >>> are followed, if we have an explicit guideline that states that > >>> the limit length is 72 chars, I will -1 any patch that doesn't follow > >>> the guideline, just as I would do with i18n guideline violations. > >>> > >> > >> Apparently you're unaware of the definition of the word 'guideline'. It's > >> a guide. If it were a hard-and-fast rule then we would have a bot enforcing > >> it already. > >> > >> Is there anything quite so frightening as a large group of people blindly > >> enforcing rules with total indifference to any sense of overarching > >> purpose? > >> > >> A reminder that the reason for this guideline is to ensure that none of > >> the broad variety of tools that are available in the Git ecosystem > >> effectively become unusable with the OpenStack repos due to wildly > >> inconsistent formatting. And of course, even that goal has to be balanced > >> against our other goals, such as building a healthy community and > >> occasionally shipping some software. > >> > >> There are plenty of ways to achieve that goal other than blanket drive-by > >> -1's for trivial inconsistencies in the formatting of individual commit > >> messages. > > > > > > The actual issue is that we as a community (Speaking of the Neutron > > community at least) are stat-crazed. We have a fair number of contributors > > that -1 for trivial issues to retain their precious stats with alarming > > zeal. That is the real issue. All of these commit message issues, > > translation mishaps, > > comment typos etc are excuses for people to boost their stats without > > contributing their time or energy in to the project. I am beyond bitter > > about this > > issue at this point. > > > > I'll say what I've always said about this issue: The review process is > > about collaboration. I imagine that the author is sitting next to me, and > > we're going > > through the patch together for the purpose of improving it. Review > > comments should be motivated by a thirst to improve the proposed code in a > > real way, > > not by your want or need to improve your stats on stackalytics. The latter > > is an enormous waste of your time. > > > > > >> A polite comment and a link to the guidelines is a great way to educate > >> new contributors. For core reviewers especially, a comment like that and a > >> +1 review will *almost always* get you the change you want in double-quick > >> time. (Any contributor who knows they are 30s work away from a +2 is going > >> to be highly motivated.) > >> > >> Typos are a completely different matter and they should not be grouped > >>> together with guideline infringements. > >>> > >> > >> "Violations"? "Infringements"? It's line wrapping, not a felony case. > >> > >> I agree that it is a waste of time and resources when you have to -1 a > >>> patch for this, but there multiple solutions, you can make sure your > >>> editor does auto wrapping at the right length (I have mine configured > >>> this way), or create a git-enforce policy with a client-side hook, or do > >>> like Ihar is trying to do and push for a guideline change. > >>> > >>> I don't mind changing the guideline to any other length, but as long as > >>> it is 72 chars I will keep enforcing it, as it is not the place of > >>> reviewers to decide which guidelines are worthy of being enforced and > >>> which ones are not. > >>> > >> > >> Of course it is. > >> > >> If we're not here to use our brains, why are we here? Serious question. > >> Feel free to use any definition of 'here'. > >> > >> Cheers, > >>> Gorka. > >>> > >>> > >>> > >>> On Sep 26, 2015, at 21:19, Ian Wells <ijw.ubu...@cack.org.uk> wrote: > >>>>> > >>>>> Can I ask a different question - could we reject a few simple-to-check > >>>>> things on the push, like bad commit messages? For things that take 2 > >>>>> seconds to fix and do make people's lives better, it's not that they're > >>>>> rejected, it's that the whole rejection cycle via gerrit review > >>>>> (push/wait > >>>>> for tests to run/check website/swear/find change/fix/push again) is out > >>>>> of > >>>>> proportion to the effort taken to fix it. > >>>>> > >>>> > >> I would welcome a confirmation step - but *not* an outright rejection - > >> that runs *locally* in git-review before the change is pushed. Right now, > >> gerrit gives you a warning after the review is pushed, at which point it is > >> too late. > >> > >> It seems here that there's benefit to 72 line messages - not that > >>>>> everyone sees that benefit, but it is present - but it doesn't outweigh > >>>>> the > >>>>> current cost. > >>>>> > >>>> > >> Yes, 72 columns is the correct guideline IMHO. It's used virtually > >> throughout the Git ecosystem now. Back in the early days of Git it wasn't > >> at all clear - should you have no line breaks at all and let each tool do > >> its own soft line wrapping? If not, where should you wrap? Now there's a > >> clear consensus that you hard wrap at 72. Vi wraps git commit messages at > >> 72 by default. > >> > >> The output of "git log" indents commit messages by four spaces, so > >> anything longer than 76 gets ugly, hard-to-read line-wrapping. I've also > >> noticed that Launchpad (or at least the bot that posts commit messages to > >> Launchpad when patches merge) does a hard wrap at 72 characters. > >> > >> A much better idea than modifying the guideline would be to put > >> documentation on the wiki about how to set up your editor so that this is > >> never an issue. You shouldn't even have to even think about the line length > >> for at least 99% of commits. > >> > >> cheers, > >> Zane. > >> > >> > >> __________________________________________________________________________ > >> OpenStack Development Mailing List (not for usage questions) > >> Unsubscribe: > >> openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >> > > > > > > __________________________________________________________________________ > > OpenStack Development Mailing List (not for usage questions) > > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > > > > > __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
