Open Stack

On Mon, Sep 28, 2015 at 5:29 PM, Kevin Benton <blak111 at 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 <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'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
>>
>>
>
>
> --
> Kevin Benton
>
> __________________________________________________________________________
> 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/e2776b27/attachment.html>