We've started to get a bit curmudgeonly when reviewing patches.
We're devoting a lot of space to style issues, without making it clear
that we're willing to make minor cleanups when committing a patch.
Sometimes it seems like we're using style reviews as a way to "push"
the patch back at the original submitter for a time -- as though we're
trying to get it off our hands while the submitter cleans it up, so
that when it comes back next time our job is that much easier.
Also, Mark Phippard and Ben Collins-Sussman have pointed out that we
often behave as though we assume everyone is trying to become a
committer. But many people don't care to make a long term investment
in learning our coding conventions. They just want to toss us a quick
bugfix or three, and get on with their lives. SteveKing of TSVN is a
good example of this, and there's nothing wrong with it.
This phenomenon was recently discussed in the "[PATCH] Issue 1628"
thread. These messages give a good overview:
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=10317
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=103182
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=103205
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=103233
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=103244
Now, I am not proposing that we ignore style issues, nor stop having
code conventions, or anything like that. The code we check in does
need to meet our guidelines. But... well, let me just quote from
Mark's mail (103244) above:
> Personally, I like all of the style comments and the fact that
> the code is reviewed so thoroughly. It is one of the things that
> makes the code maintainable. Who would want to read a book where
> each paragraph was written by a different author with no common
> editor? My point was that there are many different kinds of
> posts that come across this list, and they tend to all be treated
> the same.
>
> I think there is a problem that no one has pointed out yet here,
> and that is that once you start offering style review, or even a
> review that gets into some of the substance there is almost an
> implicit acceptance that the patch itself is worthy of inclusion.
> If it later turns out that there are some legitimate exceptions,
> then it leads to more hard feelings than were necessary. "Why
> did you make me cleanup and resubmit a patch 3 times only to then
> tell me there is no way you would commit it?"
>
> [...] I think if you are inclined to review a patch the first
> thing you should ask yourself is whether you have all of the
> "meta-information" you need to do so. Does the log message or
> the email include enough of this information so that you are
> clear about and agree with the objectives? If not, then the
> first review comments should probably just be to collect that
> information and engage that discussion. Everything else should
> be able to flow from there.
He's onto something there.
I'd like to ask that we all keep the following principles in mind when
reviewing patches:
* If you have a lot of style nits, try to mention them once at the
top, and point the submitter to the relevant parts of
hacking.html. Don't devote the majority of space to style
issues, at least not for fly-by contributors. We want their
bugfixes; we don't need their souls and firstborns as well.
* When you do point out style issues, make it clear that you're
just mentioning them for next time -- i.e., that you can fix them
up yourself when you commit this patch. Of course, if you're
asking the author to resubmit anyway for substantive reasons,
then you might as well ask them if they'd mind cleaning up the
stylistic stuff while they're at it. But even then, don't make
it a hard requirement.
* If you're doing a style review but don't have time to do a deep
review, state so up front, so the person knows that merely
cleaning up the style isn't necessarily a guarantee that the
patch will be committed. (But frankly, it might be better just
not to do style-only reviews anymore.)
I used to think that maximum parallelization was the goal, that it
made sense to iterate a patch many times, because it's better to have
the submitter making the incremental improvements than an SVN
developer doing it. Also I thought it made sense to try and educate
every contributor about our coding practices. In retrospect, this was
naïve and a bit myopic: many people feel they've made enough effort
just by writing the initial patch, and don't want to be told to go
back and do more.
We need to be willing to do more patch massagement ourselves. We
should point out the style guidelines unobstrusively, and those who
make an effort to follow them are obviously interested in contributing
for the long-term. But that doesn't mean other kinds of people should
be left out in the cold.
This project has always had a reputation for being extremely
newcomer-friendly. We are the place you can come and not get your
head bitten off, even if you are a newbie whose patch needs to be
entirely rewritten. I think that reputation is valuable, both in
practical terms and otherwise, and although we have not lost it yet,
we could slowly squander it by turning into High Priests guarding the
Temple of Subversion from the masses :-). Let's make sure that
doesn't happen.
-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 00:08:16 2005