Senthil, as you know I've been slowly modifying the patch you originally
attached to issue #2489. I think it's done, I just need to test (but
for that, I need a client cert to use with the server you kindly set up,
see http://subversion.tigris.org/issues/show_bug.cgi?id=2489#desc15).
Anyway, as I went along tweaking the patch, I kept notes on what I did,
and I thought you'd want to see them. These notes are basically about
the difference between
http://subversion.tigris.org/nonav/issues/showattachment.cgi/900/2489.patch
and
http://subversion.tigris.org/nonav/issues/showattachment.cgi/907/2489-patch-v5.txt
There is one question for you below, about null providers, by the way.
Here are the notes:
-------------------------------------------------------------------------
In the log message, I rewrote an entry to mention a new parameter
(that's important!). The new version looks like this:
(get_auth_simple_provider): Rename to...
(get_auth_provider): ...this, take new argument provider_type,
and select a provider based on it.
And in the code, I documented that function (it had been mostly
undocumented).
In svn_cmdline_set_up_auth_baton(), I tweaked the comments a bit.
This is just a note for the future: in ensure_auth_dirs(), we should
abstract out the creation code stanzas, they're all exactly the same.
(I realize some of these issues predated your code, and they showed up
in your patch just because you were using existing code as an example.)
I rewrote some of the new documentation output by svn_config_ensure().
In the subversion/libsvn_subr/ssl_client_cert_pw_providers.c log
entry, there was no mention of including svn_private_config.h, only of
including svn_auth_private.h. I added the former.
In ssl_client_cert_pw_file_first_credentials, why change the return
parameter from 'credentials_p' to 'credentials'? The old name was still
good, and it matches the callee that this now wraps anyway. I changed
it back.
As we discussed today in IRC, I changed
SVN_AUTH__AUTHFILE_PASSPHRASE_KEY ==> AUTHN_PASSPHRASE_KEY,
SVN_AUTH__AUTHFILE_PASSTYPE_KEY ==> AUTHN_PASSTYPE_KEY
...and added documentation comments explaining them. The comments also
discuss the broader future change we should make in the auth code to
rename related constants in other files.
Also as per IRC, svn_auth__ssl_client_cert_pw_file_save_creds_helper()
had an "XXX: Hopefully..." comment that I extended. This will affect
simple_providers.c as well, once it's resolved.
Also, I made cached_answer be a regular boolean. (This should probably
also be tweaked in simple_providers.c, but that's a separate commit.)
I documented simple_passphrase_get() and simple_passphrase_set().
In the original log message, there was this:
(svn_auth__ssl_client_cert_pw_file_first_creds_helper): New helper function
for ssl_client_cert_pw_file_first_credentials.
(svn_auth__ssl_client_cert_pw_file_save_creds_helper): New helper function
for ssl_client_cert_pw_file_save_credentials.
But actually, these new functions have ben abstracted out from the old
functions (named above), and then are called not only from the old
functions, but from other functions in other files, specifically, from
gnome_keyring_ssl_client_cert_pw_first_creds()
gnome_keyring_ssl_client_cert_pw_save_creds()
So "helper function for ssl_client_cert_pwd_file_foo()" is not the
right way to describe these (in fact, that description sent me off on
a surprisingly long detour, as I tried to figure out why they had been
abstracted out at all when the log message implied they'd only have
one caller each). I've updated those entries in the log message to
describe what actually happened.
Watch out for documenting things twice. For example, the
svn_auth__ssl_client_cert_pw_file_save_creds_helper() function
definition had a short doc comment above it. This is redundant, because
the function is documented in its .h file. Many functions had redundant
stub doc strings. I got rid of some of them; that way they can't ever
become out-of-date with respect to their official doc strings. I might
have missed some, though.
A couple of places in libsvn_subr/cmdline.c have code like this:
if (non_interactive == FALSE)
{
/* This provider doesn't prompt the user in order to get creds;
it prompts the user regarding the caching of creds. */
svn_auth_get_simple_provider2(&provider,
svn_cmdline_auth_plaintext_prompt,
pb, pool);
}
else
{
svn_auth_get_simple_provider2(&provider, NULL, NULL, pool);
}
APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
svn_auth_get_username_provider(&provider, pool);
APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
But why do we bother to register a null provider in the non_interactive
case? Why not just avoid pushing onto the 'providers' array at all, in
that case?
I changed some symbol names, variables, and strings to make them more
consistent and more self-explanatory:
SVN_CONFIG_OPTION_STORE_SSL_CLIENT_CERT_PP_PLAINTEXT
(was formerly SVN_CONFIG_OPTION_STORE_PLAINTEXT_PASSPHRASE)
SVN_CONFIG_DEFAULT_OPTION_STORE_SSL_CLIENT_CERT_PP
(was formerly SVN_CONFIG_DEFAULT_OPTION_STORE_PASSPHRASE)
SVN_CONFIG_DEFAULT_OPTION_STORE_SSL_CLIENT_CERT_PP_PLAINTEXT
(was formerly SVN_CONFIG_DEFAULT_OPTION_STORE_PLAINTEXT_PASSPHRASE)
SVN_AUTH_PARAM_STORE_SSL_CLIENT_CERT_PP_PLAINTEXT
(was formerly SVN_AUTH_PARAM_STORE_PLAINTEXT_PASSPHRASE)
SVN_AUTH_PARAM_DONT_STORE_SSL_CLIENT_CERT_PP
(was formerly SVN_AUTH_PARAM_DONT_STORE_PASSPHRASE)
store-ssl-client-cert-pp-plaintext
(was formerly store-plaintext-passphrase)
store_pp, store_pp_plaintext
(was formerly local variables with longer names, renamed and
reordered to make their relationship to each other clearer.)
I realize these are getting quite long. We could maybe shorten
"CLIENT_CERT" to "CC" if that's a problem.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-07-15 01:23:59 CEST