"Valik" <vampirevalik@hotmail.com> writes:
> I want to add in my observations to this as well. I've been reading the
> list for a few days now and I'm just astounded at the process it takes
> to get a patch committed. As an outsider looking in, your methods are
> very draconian. I can't recall the number of patch threads I've read in
> the last couple days but I walk away from every one of them with the
> same thought in mind: These guys critique more on "ugly" code than they
> do on poorly written code.
>
> The overwhelming majority of the reviewer critiques center around code
> formatting. There is a very minor amount of actual critiques over code
> problems, just formatting. IMO, if it's so important that your code be
> formatted so perfectly, use a tidy program. I find it ridiculous that
> you _force_ people to use your coding conventions to submit a patch.
> These people do all the work of writing and testing the code and you
> throw it right back in their face with style issues.
A mail like this is hard to respond to, because it raises some very
good points, while at the same time making a few unjust accusations.
I'll try to respond constructively to the criticisms, but also point
out where I think your tirade went a bit overboard :-).
First, yes, you have a good point: we *are* a bit too harsh on new
contributors about coding style. It's fine for us to point out
stylistic issues, but we should also make it clear to people that our
comments are mainly for the sake of future patches, and that we're
happy to tweak those things ourselves at commit time. Right now,
we're making it sound like a 100% clean patch is absolutely required
before we'll consider committing it, and that's just not true. (Look
over the output of the 'svn log' output and see all the commit-time
tweaks committers have made to contributed patches, for example.)
One way to improve this situation would be to ask committers to make
it clear that stylistic fixups are often optional. We could try to
end most patch reviews with something like "Anyway, if you don't have
time to fix these things up, it's no problem, I can take care of it,
just letting you know for next time." Sure, we prefer to distribute
that work back to the original submitter if possible (parallelism
makes the world go round), but we don't want people to feel punished
for submitting patches.
So, as to the main point, I think you're onto something. Thank you
for taking the time to write about it.
However, two things:
First, certain style conventions make patches much more readable (for
example, avoiding the use of tabs, so that the diff doesn't have
indentation problems). Since a patch is written by 1 person, and
reviewed by N people, it's reasonable to ask an author to make some
effort -- no, not a week of hard labor, just some effort -- to make
the patch conveniently comprehensible to its intended audience. It's
O(1) vs O(N). If the patch is hard to read, the extra effort to read
it is O(N) on the number of reviewers. But if the original author
takes the time to format it in the expected way, that effort is O(1),
since only the submitter does it.
Second, you exaggerate the situation quite a bit, see below.
> Again, I want to emphasize this. I've consumed a large amount of this
> list in a few days so I've seen a lot of threads all at once. In
> virtually every patch thread, the reviewers have found more formatting
> errors than anything. And there were more than a few comments from the
> committers who committed a patch begrudgingly because there were still a
> few minor formatting errors. I consider committing a patch but still
> making a side comment about "there were style errors but I fixed them"
> to be begrudging.
If a committer commits a patch, but makes modifications to it, of
course he should say he tweaked it. That's not begrudging, it's just
honest (unless the committer actually says something pejorative -- but
we almost never do. Did you have specific examples?).
To fail to point out that the patch was tweaked would be to
misrepresent the commit. Indeed it would make the original
contributor liable for changes introduced by the committer! We've got
to be accurate with people's reputations, that's just good manners.
So I don't know why you find this "begrudging", but judging from
others' reactions over the years, I'm pretty sure most people take it
in the spirit in which it's intended.
> Speaking specifically about Stefan, he made it quite clear he was giving
> you a proof of concept patch very early on. I'm surprised he's put as
> much effort into this as he has given his personality which I've seen in
> action on the TortoiseSVN list. He reminds me a bit of me, which is to
> say, often times not very patient with people. Your first priority with
> the proof of concept code was to get it into your preferred style. Once
> that is done, and only then, did you actually comment on the merits of
> the code itself.
>
> Did I wonder into the twilight zone? In my realm, the merits of the
> code take precedence over... well, anything, really. I can write you
> some beautiful code... that doesn't work. Would that be more likely to
> speed up the patch committing process? At the moment, I'm only half
> joking when I say that I think I could just about get away with writing
> perfectly formatted code (Per the subversion manifesto) that has
> absolutely nothing to do with the log message describing the
> feature/problem.
See, this is what I mean by exaggerated.
I double-dare you :-). I will pay you $100 (US) if you succeed in
getting a patch into Subversion by writing a perfectly-formatted patch
and submitting it with an unrelated log message. In fact, I will pay
you $100 if the patch is merely reviewed and no committer notices the
discrepancy, regardless of whether the patch is ever applied. (Brian
Fitzpatrick, sitting next to me, says he'll put in another $50, so now
your prize is $150. Good luck.)
Seriously: don't say you're only "half joking" unless you really mean
that. We catch things other than style issues all the time; surely
you've noticed that.
You weakened your otherwise valid point by making factually untrue
accusations as well. You said "Your first priority with the proof of
concept code was to get it into your preferred style. Once that is
done, and only then, did you actually comment on the merits of the
code itself." And yet Brane's *very first* response to Steve's post
included a major substantive comment. See it here:
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=102897
This just highlights a general problem with your post: you greatly
overestimate the ratio of style comments to substance comments. I've
been reading patch reviews on this list for five years (and done
plenty of reviews myself), and while there has lately been a bit too
much emphasis on style issues, patches that have substantive problems
received detailed comment on those problems. Have you really read so
many patch threads that you can make such a sweeping generalization?
Perhaps you're getting fooled by the old "Amount Of Space" problem:
the amount of space taken up on the screen by style comments can be
equal to or greater than the amount of space taken up by substantive
comments, even though the substantive comments are obviously more
important. This is because a style issues tend to be pervasive,
whereas each substantive issue will generally only get commented on
once.
Does this mean the reviewer thinks the style issues are of greater
importance than the substantive problems? No, not at all. The
reviewer is counting on the recipient to understand the relative
priorities of the comments. If there were a way we could render all
style comments in a small, unobtrusive font, while putting substantive
comments in a large, attention-getting font, we would do that. But
it's a lot easier to write plaintext mails and assume people will use
context and general programming knowledge to figure out what's most
important.
> I'm not directing my comments at anybody in particular (Well not
> exactly, my comments are directed at "cat COMMITTERS", I suppose). The
> style guidelines stated in the HACKING file should be just that,
> guidelines; recommendations. They should not be mandatory to the point
> of having patches rejected over minutia. I wonder how many potential
> developers have been turned away for the same reasons I am or have
> cracked the HACKING file and seen that it's enormous. I'm sorry, but
> I'm not reading an 1800 line guide just to write code in your stead.
> I've a great many better things to do than read 1800+ lines to even
> stand a chance of getting a patch accepted within the first 4 versions
> due to style issues. Those better things including watching paint dry,
> counting the ticks of a clock, counting the individual pixels on my
> monitor and stapling my crotch.
Okay, I admit it, that was damn funny :-).
> Some of my comments are said somewhat tongue-in-cheek. But before
> anybody goes and gets their panties in a bunch, stop and think for a
> minute and review some of the recent patch threads over the last 3 or 4
> weeks. Stand outside the Subversion world for a minute and think, what
> if you wanted to submit to a project and they wanted their code to use
> Hungarian Notation (Which I gather is apparently the devil's mark in
> source code around here). Take yourself out of this little bubble and
> look in from the outside. I think you guys are smart enough to see that
> you've gotten your priorities more than a little misplaced with regards
> to reviewing and accepting patches. I just hope that somewhere along
> the line you stop and take a long hard look at what these people are
> doing; which is to say, things that you are not doing so you should be
> just a bit more grateful for less stylish code and more willing to fix
> up styling problems since that's a trivial exercise compared to the task
> of writing and testing code.
We are generally willing to fix up stylistic problems. What we need
to do is make it *clear* that we are willing to do so, and that our
comments are usually meant to convey a sentiment like: "If you're
resubmitting this (for substantive reasons) anyway, would you mind
fixing these stylistic issues as well?".
I'll post a separate mail to the dev@ list, recommending some specific
ways we can make their patch reviews less intimidating, and to focus
more on substantive stuff. I believe yours and Mark's overall point
is a good one. We have been a bit too high-priestish over here in the
temple of Subversion, and that's not good for contributors or for the
project.
Thanks,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jul 25 18:14:43 2005