On Tue, Jun 11, 2013 at 10:00:56AM -0700, Junio C Hamano wrote:
> Michael Haggerty <mhagger@xxxxxxxxxxxx> 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" ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html