Ben Collins-Sussman wrote:
>Here are my notes for David Waite, after reviewing his certs branch
>for the Nth time. I'm about to merge the branch into /trunk, because
>it looks really good. :-)
>
>
>Various cleanups done by me:
>
>* Please read HACKING. There are many project formatting conventions
> you're not following, which I've had to clean up:
>
> * In almost every C file in Subversion, we use the GNU function
> style: foo (blah), not foo(blah). Our rule is: keep the style
> of the C file consistent. This means I reformatted your new
> routines almost everywhere, because they weren't using
> GNU-style. (The exception was libsvn_ra_dav/session.c, which
> consistently uses the latter style.)
>
> * When a function returns svn_error_t *, return SVN_NO_ERROR,
> rather than NULL. We don't want to confuse semantics.
>
> * We write ISO C code, not C++ or C99 code. I got flat-out
> compile errors at first, because in libsvn_ra_dav/session.c, you
> were declaring variables in the middle of blocks all over the
> place!
>
> * We declare pointers like 'svn_foo_t *foo', not 'svn_foo_t*
> foo'. Your variable/function declarations are inconsistent;
> you seem to alternate back and forth between the two styles.
>
> Sorry to nitpick, but the svn community prides itself on having very
> neat, consistent, easy-to-read code. :-)
>
>* svn_auth.h was #including ne_session.h. That's not good; remember
> that we only want libsvn_ra_dav to depend on neon, since the
> compilation of that "pair" of libraries is optional. I think this
> was simply vestigial; I removed the #include, and everything
> compiled fine.
>
Let me just summarize all of these (excluding SVN_NO_ERROR) with a
reserved 'oops..'. I thought I had checked the code for these specific
stylistic problems on every commit, but apparently I let some slip
through. I will try doubly hard in the future.
>* I removed the useless "#ifndef NO_SSL" stuff from main.c. We can
> register all the cert providers uncondititonally. If ra_dav is
> present, or if neon doesn't have SSL support, then the providers
> will simply never be called. Also, I moved your prompt-provider
> creation within the 'non interactive' block; prompt providers aren't
> registered if the user passes --non-interactive.
>
These defines were just markers until I found out if these needed to be
runtime configured or not. Since any setup without a ra_dav using a neon
with ssl will never look for these providers, and since the providers do
not depend on neon or openssl symbols themselves, it turns out it does
not matter.
>Things you still need to fix:
>
>* The runtime parameter SVN_AUTH_PARAM_SSL_SERVER_CERTIFICATE and
> SSL_SERVER_DNAME are still defined in svn_auth.h. None of your
> providers use them. And yet your neon callback is still putting the
> server cert in the parameter hash! You should remove this unused
> stuff.
>
>* Your file-based providers don't implement next_creds() functions,
> which is totally fine. But I think it might be courteous to make
> their first_creds() routines at least return a NULL iter_baton
> anyway. That's what we do for the wc-cache providers.
>
Sure, let me know if you want these fixed in a patch, or on the trunk.
-David Waite
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Mar 21 03:18:52 2003