Re: [PATCH] Documentation/CommunityGuidelines

From: John Keeping <john@keeping.me.uk>
Date: Jun 11, 2013, 18:29 UTC
Message-ID: <20130611182936.GM22905@serenity.lan>
In-Reply-To: <7v38sod1kn.fsf@alter.siamese.dyndns.org>

On Tue, Jun 11, 2013 at 10:00:56AM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> > * When reviewing other peoples' code, be tactful and constructive.  Set
> > high expectations, but do what you can to help the submitter achieve
> > them.  Don't demand changes based only on your personal preferences.
> > Don't let the perfect be the enemy of the good.
> 
> I think this is 30% aimed at me (as I think I do about that much of
> the reviews around here).  I fully agree with most of them, but the
> last sentence is a bit too fuzzy to be a practically useful
> guideline.  Somebody's bare minimum is somebody else's perfection.
> An unqualified "perfect is the enemy of good" is often incorrectly
> used to justify "It works for me." and "There already are other
> codepaths that do it in the same wrong way.", both of which make
> things _worse_ for the long term project health.

One thing that I think is missing from these proposals so far is some clear indication that a review should not be confrontational. Consider the following two review comments (taken from a recent example that happened to stick in my mind, but I don't want to single out any one individual here):

    Ugh, why this roundabout-passive-past tone?  Use imperative tone
    like this:

...

vs.

    We normally use the imperative in commit messages, perhaps like
    this?

...

Both say the same thing but the first immediately puts the submitter on the defensive. If I see something like that on one of my patches I have to consciously resist the urge to reply immediately and instead review what I'm about to send once I've calmed down.

I realise that we shouldn't take offence to review comments, but we are all human and it is sometimes hard not to take things personally.

In the examples above, the first makes it feel like the submitter is fighting to get a patch included, but the second feels like we're collaborating to get to the best result for the project.

As my mother would say, "politeness costs nothing" ;-)