John Peacock <jpeacock@rowman.com> writes:
> However, as I believe others have mentioned before, I can personally
> attest to the unfortunate reality that sometimes the stylistic review
> takes on a life of its own. It is sometimes true that the reviewer
> doesn't have time to do a deep analysis, but can at least comment on
> the commit message or formatting issues. This does no one any good,
> because complaints about the commit message get fixed and then the
> patch gets ripped up because the code was no good to begin with.
>
> The Keywords-as-hash patch that I adopted as my own, updated and
> changed repeatedly through many iterations is a prime example. It
> took me more than a year before someone went through the code and
> said, basically, this is a poor design. Max rewrote it almost
> completely (and it's still not committed, is it?), which I could have
> done had someone told me that it needed more than tweaking. But I'm
> not bitter ;-), since I've basically moved on. Please note that I
> first updated Plasma's patch in March of 2004 and the changes are
> still not applied.
>
> Karl, I know you and the other core committers mean well as the
> gatekeepers of the project. But I can provide links to all of the
> reviews of my various patchs (as well as the apology from Julian for
> the length of time it took to get the point of serious hacking, which
> was 11 months). This isn't just a perception, it is a reality.
Hey, I didn't say it never happens (and I'm sorry it happened to you).
I just said it's not what happens in the majority of cases.
> The project needs to professionalize how [unsolicited] patch
> submissions are handled, not in the sense of getting paid for review,
> but in making the review consistent. How you review a core programmer
> must be very different from how you review an outsider, else you are
> going to drive away the fresh blood. I would go so far as to suggest
> that a couple of simple rules should be followed:
>
> 1) Don't perform a commit message review the first time a patch has
> been submitted from a new[ish] submitter unless the rest of the patch
> is all but ready to commit. There is nothing more frustrating
> (personal experience speaking) at submitting a patch and being told
> that it cannot be reviewed without a correctly formatted commit
> message. Just bite your tongue (or change your sig file to include a
> link to HACKING). :-)
I agree. We should review the log message for *content*, of course,
but not be picky about formatting.
> 2) All core committers should try to take turns explicitely reviewing
> patches. I know this is a mostly volunteer effort (Collabnet people
> aside) but assuming someone else is going to review often means that
> unless someone find a patch "interesting" then _no_one_ is going to
> review (more personal experience here). You can trade if a patch you
> have a personal stake in comes in on your day off, but making an
> effort to make sure that *all* patches get at least some due
> consideration will improve the project in the long run. It's bad
> enough that the source code can be very sophisticated and the learning
> curve steep just to formulate patch that tests clean, but not being
> able to get someone to even look at the patch in detail is very
> offputting.
Those who have time are already doing it...
> 3) Once a patch has passed the sniff test (no glaring design
> problems), which may be a couple of rounds of patch submission and
> possibly a lot of list discussion, then start moving into the
> stylistic changes, which are now the only thing keeping the patch from
> being committed. Sometimes, this is just best done by the committer,
> rather than kicking the patch back to the orginal submitter (or his
> followup). I know this does happen now, but at least from my point of
> view, there is too much effort spent explaining why a particular patch
> need reformatting before being committed. Making every single patch
> review (or so it seems) into a lesson in the project style gets old
> real fast. ;-)
>
> Please take this as constructive criticism (the spirit it is given).
I completely agree with these suggestions (a thought: would you like
to write this up for a new section in hacking.html?).
But, I also think Valik has been making things out to be quite a bit
worse than they really are. Perception may be reality, but that
doesn't mean we have to accept Valik's perceptions in particular as
gospel :-).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 26 22:18:13 2005