<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Sep 28, 2015 at 5:29 PM, Kevin Benton <span dir="ltr"><<a href="mailto:blak111@gmail.com" target="_blank">blak111@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.</div></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>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.</div></div><div class="gmail_extra"><div><div class="h5"><br><div class="gmail_quote">On Mon, Sep 28, 2015 at 11:00 PM, Assaf Muller <span dir="ltr"><<a href="mailto:amuller@redhat.com" target="_blank">amuller@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Mon, Sep 28, 2015 at 12:40 PM, Zane Bitter <span dir="ltr"><<a href="mailto:zbitter@redhat.com" target="_blank">zbitter@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On 28/09/15 05:47, Gorka Eguileor wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 26/09, Morgan Fainberg wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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).<br>
<br>
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.<br>
</blockquote></blockquote>
<br></span>
+1<span><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
</blockquote></blockquote>
<br></span>
+1<span><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Sent via mobile<br>
<br>
</blockquote>
<br>
I have to disagree, as reviewers we have to make sure that guidelines<br>
are followed, if we have an explicit guideline that states that<br>
the limit length is 72 chars, I will -1 any patch that doesn't follow<br>
the guideline, just as I would do with i18n guideline violations.<br>
</blockquote>
<br></span>
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.<br>
<br>
Is there anything quite so frightening as a large group of people blindly enforcing rules with total indifference to any sense of overarching purpose?<br>
<br>
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.<br>
<br>
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.</blockquote><div><br></div></span><div>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</div><div>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,</div><div>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</div><div>issue at this point.</div><div><br></div><div>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</div><div>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,</div><div>not by your want or need to improve your stats on stackalytics. The latter is an enormous waste of your time.</div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.)<span><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Typos are a completely different matter and they should not be grouped<br>
together with guideline infringements.<br>
</blockquote>
<br></span>
"Violations"? "Infringements"? It's line wrapping, not a felony case.<span><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I agree that it is a waste of time and resources when you have to -1 a<br>
patch for this, but there multiple solutions, you can make sure your<br>
editor does auto wrapping at the right length (I have mine configured<br>
this way), or create a git-enforce policy with a client-side hook, or do<br>
like Ihar is trying to do and push for a guideline change.<br>
<br>
I don't mind changing the guideline to any other length, but as long as<br>
it is 72 chars I will keep enforcing it, as it is not the place of<br>
reviewers to decide which guidelines are worthy of being enforced and<br>
which ones are not.<br>
</blockquote>
<br></span>
Of course it is.<br>
<br>
If we're not here to use our brains, why are we here? Serious question. Feel free to use any definition of 'here'.<span><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Cheers,<br>
Gorka.<br>
<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Sep 26, 2015, at 21:19, Ian Wells <<a href="mailto:ijw.ubuntu@cack.org.uk" target="_blank">ijw.ubuntu@cack.org.uk</a>> wrote:<br>
<br>
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.<br>
</blockquote></blockquote></blockquote>
<br></span>
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.<span><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
</blockquote></blockquote></blockquote>
<br></span>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
cheers,<br>
Zane.<div><div><br>
<br>
__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</div></div></blockquote></div></div></div><br></div></div>
<br>__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
<br></blockquote></div><br><br clear="all"><div><br></div></div></div><span class="HOEnZb"><font color="#888888">-- <br><div><div>Kevin Benton</div></div>
</font></span></div>
<br>__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
<br></blockquote></div><br></div></div>