> -----Original Message-----
> From: kfogel@newton.ch.collab.net [mailto:kfogel@newton.ch.collab.net]
>
> 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 :-).
It was intentionally overboard. The best way to emphasize the point was
to take the current limit and multiply by a factor of 5, ergo,
overboard. Incidentally, I'm prone to tirades and this was actually a
rather minor one since I'm not directly involved but just an outsider.
> 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.
As I said before, if you really can't read code that's not formatted a
certain way, use a tidy program on it. That probably won't fix name
issues but as far as spaces and tabs and things of that nature go, it
could clean all that right up. It seems like that is the largest
problem that I see critiqued on. Particularly the space before ( in
function calls. A simple regular expression should be able to fix that
up real quick (No, I don't have one off the top of my head).
The current perception that the submitter needs to fix stylistic issues
is akin to sitting on a metal bench and complaining it's too hot but not
actually getting up off of it. There are ways to be pro-active about
the situation instead of complaining.
I think using a tidy program (and recommending submitters to use one)
would reduce the amount of "review spam" which is the term I would use
for populating a review with stylistic issues.
> Second, you exaggerate the situation quite a bit, see below.
>
> 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.
Perhaps it seems begrudging to me because I've read the entire thread in
one sitting as opposed to have it spaced out over a few days as new
posts come in live. Seeing a bunch of review comments critiquing style
issues and then seeing more comments about style when the commit is
finally made seems just a bit ill-mannered to me.
I don't think its so much a problem with mentioned changes to the patch
in the commit, that is perfectly legitimate, I think its all the nagging
(Yes, nagging) I see about style issues in the reviews for the patches
that are the problem. The commit log is the catalyst that makes it all
even worse. The comment needs to be there, yes, no argument. But I
don't think all the critiques leading up to that do. That's what needs
to be avoided in order to "soften the blow" that comes when the patch is
finally put to bed via a commit.
> 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.)
>
Perceptions; it's all in what's seen, not in what's there. Although the
prospect of monetary gain is intriguing, I'd rather not waste my time or
your time trying this. Judging by the sheer number and redundancy
there-of of stylistic issues, the perception I get is that pretty code
is more important than code that does what it says and what the log
says. That is the source of my comment.
> 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.
Yes, but it's a bit obscured by all the stylistic remarks. Particularly
when a reviewer points out a styling error not once, nor twice, but
thrice (assuming the victim... er... submitter is lucky). There are
content related issues but trying to find them in the forest of style
issues is a bit difficult.
> 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
One out of three comments is content related. The comment is that the
code doesn't belong in the file specified. Well, given the direction
the thread has now gone, I think that's a big of a wash as far as
content comments go, wouldn't you agree? Surely Brane could have formed
the conclusion that there were problems with this patch, regardless of
the file it was located in? I will acknowledge that it is a
content-related issue with the patch, but I will still argue it is not
an acceptable critique given that much more serious technical issues
have been raised with the patch in general.
And not to put too much stock in the order of the list, but the content
related is the third item so Stefan still had to wade through the style
comments first.
> 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?
I don't need any background material to make sweeping generalizations
;). That said, if the stylistic comments have only recently become a
problem, then yes, my view is skewed because I've only recently been
reading the list. Keep in mind, though, that any potential new patch
writers will be in the same boat, so what you did in the past is not
relevant if it's not seen.
> 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.
I'm not being fooled by anything. If I see more of something, there is
more of it there. I don't care how you want to justify the existence of
there being a large number of style comments, the simple fact remains,
finding those nuggets of content critiques amongst the myriad style
problems are just a lot of work. Every time you break the quoting
mechanism to insert comments about style you make it that much harder to
find a useful content related comment. I also see a lot of redundancy,
especially with the space before ( thing (Haven't I said this before?).
Keep in mind, style issues are quick to describe. Content related
issues are not. There may be a 50/50 split between space used to detail
style issues versus content. However, there may be one or two content
issues but 10 or 15 style issues. Looking at numbers alone, it becomes
overwhelming. 2 complex content problems that will need hours of
thought/work to fix seems a bit less intimidating than 15 style issues
to me. That's because perception is quantitative, not qualitative.
This is all about perception.
> 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.
It would be even easier if you put just a couple style remarks. One or
more of:
"Your code doesn't conform to our formatting conventions, could you
please run <insert tidy program> before you submit a patch next time?"
"Please try to rename your variables/functions using a naming scheme
consistent with the rest of Subversion"
That's it. That's all you guys should have to say. If they do it, then
great for you; if they don't, then it shouldn't be too hard to do it.
The key here is stop the whining and be pro-active about it. Find or
write a tidy program. I'm sure somebody could whip up a Perl or Python
script to write a code tidier that conforms to the Subversion
conventions. I bet you'd find you'd spend less time writing such a
thing than you would reviewing stylistic issues. Writing one also
assumes you can't find an acceptable (multi-platform) alternative.
> 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 think you need to "get with the boys" on this one. Branko clearly
states, "...because even a technically perfect patch wouldn't be
accepted if style wasn't correct" which is contrast to what you are
saying above. I think one of the minor problems is there isn't an
official stance on the matter and it's up to an individual committer as
to how much effort they are willing to expend.
Incidentally, the inverse of that comment could be, "a stylistically
correct patch would be accepted even with technical problems". I would
say that a do-nothing patch with misleading log message would fall into
the "stylistically correct but with technical problems". Therefore,
don't count your $100 and $50 respectively as being safe just yet.
I am, of course, trying to demonstrate a point. It's completely
irrelevant that my inverted quote above will never happen. With the
current perception, it seems feasible that it could. Its all about
perception (Have I mentioned that yet?).
> 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
Then I've done all I set out to do. My goal was to make you think.
I've tried to keep things light-hearted enough so as not to piss anybody
off but to also get your wheels to turning to find a solution. I've
also tried to use redundancy to repeat myself (!) to point out to things
I think are most important. Just to sum up real quick (for another
added layer of redundancy), a tidy program is a good idea to invest some
time in acquiring and working on the perception to be less stylistically
focused. If you do the former, then the perception will be changed by
default since the quantity of stylistic comments will decrease
substantially.
And again, as with before, I'm not trying to call anybody out, not even
Branko. I'm only taking what I see and interpreting it (And trying not
to be a spin-doctor in the process). I have to emphasize things a bit
because the intended audience is so close to the problem that the best
way to get them to see it is to make it really big. That's all I'm
trying to do.
- Valik
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jul 25 19:27:25 2005