Open Stack

On Mon, Sep 28, 2015 at 4:00 PM, Assaf Muller <amuller at redhat.com> wrote:

>
>
> On Mon, Sep 28, 2015 at 12:40 PM, Zane Bitter <zbitter at 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.ubuntu at 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-request at 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-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150928/6a080083/attachment.html>