Senthil Kumaran S <senthil_at_collab.net> writes:
> The updated patch has more clarification in the doc string for
> 'need_to_save' variable. Please let me know if the patch is good so
> that I can commit it to trunk.
>
> [[[
> Fix unnecessary plaintext password saving prompt when the username
> is supplied on the command line and the password is already cached.
>
> * subversion/libsvn_subr/simple_providers.c
> (simple_username_get): New function to get username for simple provider.
> (svn_auth__simple_first_creds_helper): Start out with need_to_save = FALSE.
> The old code did set creds->may_save to TRUE whenever a username
> was supplied on the command line, regardless of whether a password
> was already cached or not. Here we check for different combinations
> of username and password either supplied in the command line or
> already cached in the auth area, based on that we toggle creds->may_save
> through need_to_save.
>
> Found by: arfrever
> Review by: kfogel
> ]]]
I like the log message -- it explains the core meaning of the change
very clearly.
> --- subversion/libsvn_subr/simple_providers.c (revision 32779)
> +++ subversion/libsvn_subr/simple_providers.c (working copy)
> @@ -127,45 +145,88 @@
> svn_boolean_t non_interactive = apr_hash_get(parameters,
> SVN_AUTH_PARAM_NON_INTERACTIVE,
> APR_HASH_KEY_STRING) != NULL;
> + const char *default_username = NULL; /* Default username from cache. */
> + const char *default_password = NULL; /* Default password from cache. */
>
> - svn_boolean_t may_save = username || password;
> + /* This checks if we should save the CREDS, iff saving the credentials is
> + allowed by the run-time configuration. */
> + svn_boolean_t need_to_save = FALSE;
> + apr_hash_t *creds_hash = NULL;
> svn_error_t *err;
> + svn_string_t *str;
> + svn_boolean_t have_passtype = FALSE;
>
> - /* If we don't have a username and a password yet, we try the auth cache */
> - if (! (username && password))
> + /* Try to load credentials from a file on disk, based on the
> + realmstring. Don't throw an error, though: if something went
> + wrong reading the file, no big deal. What really matters is that
> + we failed to get the creds, so allow the auth system to try the
> + next provider. */
> + err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
> + realmstring, config_dir, pool);
> + svn_error_clear(err);
> +
> + /* We have something in the auth cache for this realm. */
> + if (! err && creds_hash)
> {
This conditional structure is a bit odd, because you check 'err' (the
pointer) after clearing 'err' (the value).
Usually we set errors to NULL after clearing them, so that we're not
keeping a pointer that points to garbage. Thus, it would be better to
do something like this:
err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
realmstring, config_dir, pool);
if (err)
{
svn_error_clear(err);
err = NULL;
}
else if (creds_hash)
{
/* We have something in the auth cache for this realm. */
[...]
}
Rest looks good to me!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-03 16:54:08 CEST