Justin Erenkrantz <justin@erenkrantz.com> writes:
> Humans are going to miss things, yet that's why we require 3 of them
> to merge. We're not perfect, and we're just starting our review
> process, so it's a little unclear what your conditions for +1ing a
> patch are.
>
> I think it would be extremely helpful if you specifically outline in
> STATUS what additional tools and processes you (as project lead)
> expect must also be satisfied in any +1. Let me reiterate: neither
> documentation nor this tools/examples/ directory can be built via
> 'make.' So, I'm not sure how I was reasonably supposed to detect
> this. And, as I said, I'm not predisposed to checking the doc strings
> manually - I care more about the correctness of code.
I don't understand what you're saying.
Certain minimal conditions for +1'ing a patch are obvious and do not
need to be documented over and over: The patch does what it claims,
doesn't introduce new bugs, and meets the coding guidelines of the
project.
I mean, come on, this stuff is just common sense. You want to
document in STATUS what's already in HACKING?
And why does it matter that the documentation and the tools/examples/
directory don't get built via 'make'? We're talking about parameters
to public API functions here -- a 'may_save' parameter was added to
functions in svn_auth.h, but the documentation for those functions was
not adjusted to describe the new parameter.
> So, I'm not sure how I was reasonably supposed to detect this.
Are you _serious_? This stuff is detected all the time.
You don't need automated tools for it. People catch this sort of
thing constantly in patch review -- I've done it more times than I can
count, as have many others. You probably have as well. To +1 a patch
that contains this problem is a review error. A minor one, we're not
talking mortal sin here.
I'm *not* saying it's unacceptable to make mistakes. Of course we all
make them, it's no big deal, just try to be more careful next time,
etc.
What I'm astonished at now, though, is that you seem to also be
denying that it's fair to expect you to catch such mistakes, that
without an automated tool, you're helpless to notice this kind of doc
error. Yet, you do seem to agree it's the job of humans to catch
these things... which is it?
Here's a thought experiment to clarify what I mean:
If, before you had voted +1, someone had called you and pointed out
this doc bug in the patch, would you still have voted +1 on it? If
yes, then we have a serious disconnect here. If no, then we don't.
> Of course, I agree. But, I just ran "doxygen doc/doxygen.conf" right
> now and I get a lot of missing argument and documentation warnings (at
> least 95). So, even if I had run doxygen to check, it probably would
> have been lost in the noise.
Why do you keep bringing up doxygen? One doesn't need to run it to
catch bugs like this.
> Yes, I noticed the thread. And, it was fixed promptly by others.
> What else would you have liked me to do?
The things I listed in my previous mail (the last time you asked this
question).
> As I see it, the review
> process worked perfectly. Instead of criticizing those who overlooked
> the (minor) problem, I think it'd be better to understand why they
> overlooked it so this same thing doesn't occur in the future. --
Isn't that what I did?
I pointed out to you that you had overlooked something. I can't say
why you overlooked it, but at least I can let you know that you did,
so you can figure out what happened (for example: maybe you'll try to
do more manual doc checking in the future).
Yet your responses imply it's okay to overlook such things because,
hey, someone else will catch it if I don't. Well, no: the review
process is only as solid as the reviewers.
You seem to think it was inappropriate of me to publicly point out
that you +1'd a change with a bug. I disagree. If +1 really means
equal responsibility, then take responsibility.
I said: "Hey, you +1'd a change you shouldn't have."
You said: "It's not right of you to point that out."
I say: "Welcome to the world of peer review."
The time you spend responding to this thread, you could spend
reviewing r8101, fixing the STATUS file, and making your votes
consistent with what you now know. That's not something I can do. I
could review r8101, but I can't change your votes for you -- only you
can do that.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Dec 29 20:40:34 2003