Open Stack

On 28/09, Clint Byrum wrote:
> 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.

Since we are not all native speakers expecting everyone to realize that
difference - which is completely right - may be a little optimistic,
moreover considering that parts of those guidelines may even be written
by non natives.

Let's say I interpret all "should" instances in that guideline as rules
that don't need to be strictly enforced, I see that the Change-Id
"should not be changed when rebasing" - this one would certainly be fun
to watch if we didn't follow it - the blueprint "should give the name of
a Launchpad blueprint" - I don't know any core that would not -1 a patch
if he notices the BP reference missing - and machine targeted metadata
"should all be grouped together at the end of the commit message" - this
one everyone follows instinctively, so no problem.

And if we look at the i18n guidelines, almost everything is using
should, but on reviews these are treated as strict *must* because of the
implications.

Anyway, it's a matter of opinion and afaik in Cinder we don't even have
a real problem with downvoting for the commit message length, I don't
see more than 1 every couple of months or so.

Cheers,
Gorka.

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