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. 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.ubu...@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-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
