Open Stack

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.

> 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
> >
> >
>