> On Sep 28, 2015, at 3:00 PM, Assaf Muller <amul...@redhat.com> wrote: > > > > On Mon, Sep 28, 2015 at 12:40 PM, Zane Bitter <zbit...@redhat.com > <mailto: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.
This is kind of a thread-jack, but to respond to your concern, I think the infra team has a nice writeup on how to review that addresses your concern: http://docs.openstack.org/infra/system-config/project.html#review-criteria <http://docs.openstack.org/infra/system-config/project.html#review-criteria> I’ve certainly seen plenty of neutron reviewers that I’d prefer to see following the above link (myself included, on occasion.) Thanks, doug > > 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 > <mailto: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://openstack-dev-requ...@lists.openstack.org/?subject:unsubscribe> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > <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
