You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Collins-Sussman <su...@collab.net> on 2003/12/27 04:52:52 UTC
svn_auth.h API change
Tobias, you changed the auth API in r8013, but didn't update all the
docstrings.
I was trying to compile tools/examples/minimal_client.c today, and
discovered that the prompt_func prototypes had changed, but their
docstrings in svn_auth.h made no mention of the new 'may_save'
parameter. I had to read the r8013 log message to understand it.
Can you look over r8013, and make sure the new parameter is documented
everywhere it's declared?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn_auth.h API change
Posted by Ben Collins-Sussman <su...@collab.net>.
On Sat, 2003-12-27 at 08:15, Tobias Ringström wrote:
> Whoups. The attached patch adds the missing docstrings, and adds more
> documentation. It also changes the way the auth parameters are
> documented. Previously, the providers documented which parameters they
> required. I've changed that so that the parameters that are passed to
> the providers are documented in svn_auth.h where the credential types
> are declared and documented.
>
Yes, this is a good documentation patch .... looks good to me. I'll
even +1 it for svn-1.0.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn_auth.h API change
Posted by Tobias Ringström <to...@ringstrom.mine.nu>.
Ben Collins-Sussman wrote:
> Tobias, you changed the auth API in r8013, but didn't update all the
> docstrings.
Whoups. The attached patch adds the missing docstrings, and adds more
documentation. It also changes the way the auth parameters are
documented. Previously, the providers documented which parameters they
required. I've changed that so that the parameters that are passed to
the providers are documented in svn_auth.h where the credential types
are declared and documented.
It would be great if you could take a quick lock at the patch and
comment on the new auth parameter documentation before I commit it.
By the way, is the following log message too short for a change like
this? It doen't seem useful to list the functions, etc, but I'm not
sure what you usually do in cases like this.
* subversion/include/svn_client.h
* subversion/include/svn_auth.h
Moved the documentation of the auth parameters (SVN_AUTH_PARAM_*)
to where the crential types are declared. Added more documentation
and improved the doxygen comments.
/Tobias
Re: svn_auth.h API change
Posted by kf...@collab.net.
Justin Erenkrantz <ju...@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
Re: svn_auth.h API change
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Monday, December 29, 2003 10:58 AM -0600 kfogel@collab.net wrote:
> Huh? That question seems like a bit of a non sequitur. Just because
> something can't be tested automatically doesn't mean it isn't a bug.
> Isn't that exactly why human review is part of the process?
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.
> standards as well as anyone. A missing parameter is a correctness
> issue in this project, and under other circumstances you'd be the
> first to agree.
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.
(Note: doxygen just SEGVd on me partway through, so it didn't even finish.)
> But my mail wasn't mainly about spurring you to take these specific
> actions. I'm really asking you (and all of us) to be careful with
> future reviews, that's all. If someone votes +1 on a patch that turns
> out to have an unambiguous problem, then they should be made aware of
> that, and I intend to do so whenever it happens, including to myself.
Yes, I noticed the thread. And, it was fixed promptly by others. What else
would you have liked me to do? 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. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn_auth.h API change
Posted by kf...@collab.net.
Justin sent this to me privately, but I thought it was worth
forwarding to the list, and he was okay with that:
Justin Erenkrantz <ju...@erenkrantz.com> writes:
> Let me reiterate a key point of the review process from httpd: you
> *never* explicitly blame anyone if a bug is later found in a review
> that others +1ed.
>
> It is incredibly bad form and only serves to get people riled up. You
> just deal with the fact that the patch is bad and move on.
>
> Do I think it sucks that I missed the doc string? Sure, but I had
> nothing but the best of intentions here (which I think somehow got
> lost here). Yet, that's why we require 3 +1s - to minimize people
> missing things. 3 people giving their best effort to the review the
> changes *are* going to catch things like this.
>
> Yet, I'm a 'big boy' and can take it. But, I wonder if others are
> probably going like, 'Shit, we're not going to review anything now'
> because of your comments.
>
> Just a word to the wise. If we want the review process to succeed,
> we're going to have make this review process friendly as possible,
> not confrontational. If it becomes confrontational, it'll fail
> spectacularly.
Hmm, he's right -- my original mail was not very constructive.
A different sort of post might have been in order, to alert people
(who may not follow every thread on the list) that they might want to
either change their vote, or quickly review the new fixup patch. But
that could be done less confrontationally and more casually than I did
it.
So, sorry, no chilling effect on future reviews intended, and I hope
none taken. If similar situations come up (and they probably will) I
will post more constructively or not at all :-).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn_auth.h API change
Posted by kf...@collab.net.
Justin Erenkrantz <ju...@erenkrantz.com> writes:
> > And Justin Erenkrantz, why did you vote +1 on this change (for 1.0)
> > even though it had doc bugs? :-)
>
> How are you testing for doc bugs? I don't have the tools installed to
> test for this. And, tools/examples isn't even in our build system.
> If you mean for this to be part of our validation, then we should add
> it to our build.
Huh? That question seems like a bit of a non sequitur. Just because
something can't be tested automatically doesn't mean it isn't a bug.
Isn't that exactly why human review is part of the process?
> > These reviews should be serious & thorough. Even Tobias hadn't voted
> > +1 on it yet (I don't know if that was an oversight, or if he had been
> > planning to review his own change again before voting.)
> >
> > That's all. I'm sure I'll make a review mistake at some point too;
> > and I hope I'll be held to account when I do! (By Justin, preferably.)
>
> I care more about correctness bugs. And, I looked at *every* line of
> the patch, confirmed that it was implemented in a sane manner without
> any problems that were evident to me. (I helped with the design of
> the auth system, so yes, I know how it should work.)
Whoa, slow up there, Tex :-). I wasn't accusing you of not knowing
how the auth system works, and you don't need to defend yourself about
that. Your technical knowledge was not in question here.
"I care more about correctness bugs"... I don't understand what that's
supposed to mean. Are you saying it's unfair to expect you to catch
doc bugs? That that's not supposed to be part of review? That people
only have to review for the things they personally care about, and the
project's coding standards don't matter? You know Subversion's doc
standards as well as anyone. A missing parameter is a correctness
issue in this project, and under other circumstances you'd be the
first to agree.
Sheesh, dude, just say "Whups, I made a mistake, sorry", correct it
[see below] and that's all. This isn't the worst thing in the world.
You're going out on a limb to defend the indefensible, for a
relatively minor matter too :-(.
> If Tobias didn't fix the docstrings, because I casted a +1 to it, I
> would fix it myself. But, he already did so in r8101. I honestly
> don't know what more you'd expect from volunteers. -- justin
There is one thing you could do:
Tobias added r8101 to the STATUS file, but not in the same item as
r8013. Meanwhile, your +1 is still on the latter, but not yet on the
former. You could review and vote on r8101, and also move it to be
with r8013, since they go together (though r8101's log message seems
not to have mentioned that it is partially a followup to 8013, not
sure why). Or, maybe undo your +1 on r8013 until you can vote on them
both?
Anyway, at the moment, you still have a +1 vote registered for a
change with doc bugs. (Of course, you're keeping track of the
situation now, so no doubt that's a short term situation -- would be
nice if the STATUS reflected what you know in your head, though.)
But my mail wasn't mainly about spurring you to take these specific
actions. I'm really asking you (and all of us) to be careful with
future reviews, that's all. If someone votes +1 on a patch that turns
out to have an unambiguous problem, then they should be made aware of
that, and I intend to do so whenever it happens, including to myself.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn_auth.h API change
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Sunday, December 28, 2003 11:32 AM -0600 kfogel@collab.net wrote:
> And Justin Erenkrantz, why did you vote +1 on this change (for 1.0)
> even though it had doc bugs? :-)
How are you testing for doc bugs? I don't have the tools installed to test
for this. And, tools/examples isn't even in our build system. If you mean
for this to be part of our validation, then we should add it to our build.
> These reviews should be serious & thorough. Even Tobias hadn't voted
> +1 on it yet (I don't know if that was an oversight, or if he had been
> planning to review his own change again before voting.)
>
> That's all. I'm sure I'll make a review mistake at some point too;
> and I hope I'll be held to account when I do! (By Justin, preferably.)
I care more about correctness bugs. And, I looked at *every* line of the
patch, confirmed that it was implemented in a sane manner without any problems
that were evident to me. (I helped with the design of the auth system, so
yes, I know how it should work.)
If Tobias didn't fix the docstrings, because I casted a +1 to it, I would fix
it myself. But, he already did so in r8101. I honestly don't know what more
you'd expect from volunteers. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn_auth.h API change
Posted by kf...@collab.net.
Ben Collins-Sussman <su...@collab.net> writes:
> Tobias, you changed the auth API in r8013, but didn't update all the
> docstrings.
>
> I was trying to compile tools/examples/minimal_client.c today, and
> discovered that the prompt_func prototypes had changed, but their
> docstrings in svn_auth.h made no mention of the new 'may_save'
> parameter. I had to read the r8013 log message to understand it.
>
> Can you look over r8013, and make sure the new parameter is documented
> everywhere it's declared?
And Justin Erenkrantz, why did you vote +1 on this change (for 1.0)
even though it had doc bugs? :-)
These reviews should be serious & thorough. Even Tobias hadn't voted
+1 on it yet (I don't know if that was an oversight, or if he had been
planning to review his own change again before voting.)
That's all. I'm sure I'll make a review mistake at some point too;
and I hope I'll be held to account when I do! (By Justin, preferably.)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org