On 28/09, Clint Byrum wrote: > 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.
Since we are not all native speakers expecting everyone to realize that difference - which is completely right - may be a little optimistic, moreover considering that parts of those guidelines may even be written by non natives. Let's say I interpret all "should" instances in that guideline as rules that don't need to be strictly enforced, I see that the Change-Id "should not be changed when rebasing" - this one would certainly be fun to watch if we didn't follow it - the blueprint "should give the name of a Launchpad blueprint" - I don't know any core that would not -1 a patch if he notices the BP reference missing - and machine targeted metadata "should all be grouped together at the end of the commit message" - this one everyone follows instinctively, so no problem. And if we look at the i18n guidelines, almost everything is using should, but on reviews these are treated as strict *must* because of the implications. Anyway, it's a matter of opinion and afaik in Cinder we don't even have a real problem with downvoting for the commit message length, I don't see more than 1 every couple of months or so. Cheers, Gorka. > > > 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 __________________________________________________________________________ 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
