On Mon, Sep 28, 2015 at 4: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 should note that as the previous PTL, for the most part I viewed stats as garbage. Keep in mind I nominated two new core reviewers whose stats were lowe but who are incredibly important members of our community [1]. I did this because they are the type of people to be core reviewers, and we had a long conversation on this. So, I agree with you, this stats thing is awful. And Stackalytics hasn't helped it, but made it much worse. [1] http://lists.openstack.org/pipermail/openstack-dev/2015-August/071869.html > 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
