On Mon, Sep 28, 2015 at 5:29 PM, Kevin Benton <blak...@gmail.com> wrote:
> 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. > I took this golden opportunity to kidnap the thread and invoke a general rant, it's not specific to the 72 characters git commit title discussion. > > 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. > > 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 >> >> > > > -- > Kevin Benton > > __________________________________________________________________________ > 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