Patch Pilot report

[Stephen J. Turnbull]

Andrew Cowie writes:

 > My experience, however has been contrary to [the hope that "doing
 > the work for them" will have a demonstration effect and they'll
 > DTRT next time].
 > 
 > Again and again whenever I have just done it for them and pushed a
 > patch over the edge and even tutored a contributor on what "would
 > be nice", I consistently get exactly the same garbage when they
 > submit their next contribution.

I'm going to take your "consistent" literally, please correct me if
I'm wrong.

Yes, "just fixing" the patch has had no demonstration effect.
Occasional contributors as a group sometimes actually see it and say
"what you did is *a lot* nicer than what I submitted!", but it doesn't
affect their later submissions.

No, "tutoring on 'nice patching'" (ie, pointing them to XEmacs patch
submission guidelines and explaining verbally how that applies) has
mixed success: a few get the whole thing the first time, most get it
partially the second time (ie, they need to be told twice to apply a
particular practice), some never get it, and some ...

 > Indeed, I have had comments like "huh? It's your job [maintainer] to
 > clean up (radically fix, realign with project architecture, correct
 > style, write documentation, write patches) patches".

... yeah, what he said.

But I've found it to be quite effective to fix the patch and send it
back to the contributor, with additional commentary where a program
comment is inappropriate (ie, because any experienced XEmacs
contributor knows that's how it done, even thought this contributor
got it wrong), and explaining minimum effort fixes for niggles like
"in XEmacs function definitions are like

int
funcall_internal (int count, Lisp_Object fun_args)
{
    /* code goes here */
}

not

int funcall_internal (int count, Lisp_Object fun_args) {
    /* code goes here */
}".

(Eg, how to set a CC mode hook to automatically set up GNU conventions
for editing C files, but *only* in directories where some parent
contains the string "emacs".)

 > [I believe this to be a legacy of the Apache worldview

I think it's just human nature.  People like to think they've done the
lion's share of the work by localizing the defect and removing the
symptom.  After that, there's "a little" cleanup that they expect the
maintainer will be happy to do, because it makes his product "much
better" (after all, "I was so annoyed that I actually went and fixed
it, didn't I!?") at very low cost.  The contributor often has never
maintained a high quality open source project himself, and although he
knows documentation and testing are high-cost because he has to do it
at work, obviously your standards are way lower because he thinks
yours suck. :-(  They also are generally unaware of stuff like the
estimate that "working code" is only 1/3 of the cost of a product, and
only 1/9 of a systems product (which the core programs of XEmacs and
bzr are to some extent, since they have to provide open-ended support
for plugins).  [Estimates per Fred Brooks, The Mythical Man-Month.]

 > Despite my best efforts, it's only been rare that I've managed to get a
 > contributor to raise their game, and it's never happened when I just
 > fixed things for someone.

Maybe it's just that XEmacs doesn't attract people who are
fundamentally GUI; even our Windows and Mac users who contribute are
Unix fugitives.  But our contributors generally are sympathetic,
except for a few people with severe autistic/megalomaniac tendencies
who think their way of doing it is actually better.  Sure, many will
say, "sorry, it's good enough for me, and I can leave it in init.el
indefinitely," but the "sorry" is genuine.

However, as I wrote above, "just fixing" does not lead to people
"raising their game."

 > Which is frustrating, because if I play the ogre and keep the bar high
 > and push back, most people just go away.

Pushing back is going to have that effect.  On the other hand, there a
a couple of people where I've pushed back *hard*, with success.  I
stripped one guy of his commit privilege because he trespassed on a
plugin maintainer's turf.  Most of the review board thought I was
being an asshole, I think, but eventually he conceded the point and I
restored it.  (Note that any of the reviewers could have done so, just
as unilaterally as I removed it, but they didn't.)  He's still around
and he's been much better all around.  Not only does he ping plugin
maintainers before changing our distribution, but he's dramatically
increased the amount of test cases he submits (often without running
them himself, tho ;-).  I still dislike his work on philosophical/
design grounds, but for general code hygiene, he improved dramatically
after that.  We can work together now, although it's not always
comfortable.

That's the extreme case by far, but "push back" is important, I
think.  OTOH, sometimes you want to apply judo, where you deliberately
roll -- and take him with you.  How?  Well ...

 > Perhaps if your code review system was to respond with: "your patch was
 > merged, but this&this&this (ie, diff) were changed from your submission
 > to acceptance..." it might help with the learning factor. Hard to say.

This is a good idea, and if done automatically by diff would be very
cheap.  But I think on top of the diff some specific and encouraging
reviewer comments would be very effective.