kfogel@collab.net wrote:
> A lot of claims are flying around lately about how picky we are about
> formatting. I suggest that before giving the claims too much
> credence, people should look over some of our patch- and
> commit-reviews themselves and see how much is really about
> automatically fixable formatting issues. It's not all that much.
If running `indent -gnu` on the source files would get us eveb 90% of
the way to the existing carbon-mediated style, then that certainly has
my vote!
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.
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). :-)
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.
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).
John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 26 21:59:38 2005