Karl Fogel wrote on Tue, 22 Apr 2008 at 23:28 -0000:
> Daniel Shahaf <d.s_at_daniel.shahaf.co.il> writes:
> > [[[
> > Username/password caching bugfixes.
> >
> > * subversion/libsvn_subr/simple_providers.c
> > (simple_save_creds_helper):
> > Cache the username even if we're not allowed to cache the password,
> > and respect SVN_AUTH_PARAM_NO_AUTH_CACHE.
> >
> > Suggested by: stsp
> > Patch by: Daniel Shahaf <d.s_at_daniel.shahaf.co.il>
> > ]]]
>
> As mentioned in previous mail: clarify that the second part is to fix an
> API violation, and that there is currently no user-visible manifestation
> of the problem.
>
Okay.
> > Index: subversion/libsvn_subr/simple_providers.c
> > ===================================================================
> > --- subversion/libsvn_subr/simple_providers.c (revision 30750)
> > +++ subversion/libsvn_subr/simple_providers.c (working copy)
> > @@ -234,18 +234,25 @@ simple_save_creds_helper(svn_boolean_t *saved,
> > apr_hash_t *creds_hash = NULL;
> > const char *config_dir;
> > svn_error_t *err;
> > - const char *dont_store_passwords =
> > - apr_hash_get(parameters,
> > - SVN_AUTH_PARAM_DONT_STORE_PASSWORDS,
> > - APR_HASH_KEY_STRING);
> > + svn_boolean_t dont_store_passwords =
> > + (NULL != apr_hash_get(parameters,
> > + SVN_AUTH_PARAM_DONT_STORE_PASSWORDS,
> > + APR_HASH_KEY_STRING));
> > svn_boolean_t non_interactive = apr_hash_get(parameters,
> > SVN_AUTH_PARAM_NON_INTERACTIVE,
> > APR_HASH_KEY_STRING) != NULL;
>
> Heh -- I've always used "!!" for that trick, but "NULL !=" works too.
>
Copied existing code.
> > + svn_boolean_t no_auth_cache = apr_hash_get(parameters,
> > + SVN_AUTH_PARAM_NO_AUTH_CACHE,
> > + APR_HASH_KEY_STRING) != NULL;
> > + svn_boolean_t username_stored = FALSE;
> > svn_boolean_t password_stored = TRUE;
>
> For consistency, better to do the boolean-folding the same way for
> both 'dont_store_passwords' and 'no_auth_cache' -- that is, put the
> "NULL !=" in front, or at put it at the end, but whichever way it's
> done, be consistent. And...
>
Okay.
> > *saved = FALSE;
> >
> > if (! creds->may_save)
> > + no_auth_cache = TRUE;
> > +
> > + if (no_auth_cache)
> > return SVN_NO_ERROR;
>
> ...why not just fold this condition into the initialization of
> no_auth_cache in the first place?
>
Because
apr_hash_get(SVN_AUTH....WHATEVER) != NULL
|| ! creds->may_save
would be very long and would break symmetry with the other initialisations
nearby.
> > config_dir = apr_hash_get(parameters,
> > @@ -254,9 +261,17 @@ simple_save_creds_helper(svn_boolean_t *saved,
> >
> > /* Put the credentials in a hash and save it to disk */
> > creds_hash = apr_hash_make(pool);
> > - apr_hash_set(creds_hash, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> > - APR_HASH_KEY_STRING,
> > - svn_string_create(creds->username, pool));
> > +
> > + /* Maybe cache the username. */
> > + if (! no_auth_cache)
> > + {
> > + apr_hash_set(creds_hash, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> > + APR_HASH_KEY_STRING,
> > + svn_string_create(creds->username, pool));
> > + username_stored = TRUE;
> > + }
> > +
> > + /* Maybe cache the password. */
> > if (! dont_store_passwords)
> > {
> > password_stored = password_set(creds_hash, realmstring, creds->username,
> > @@ -276,7 +291,8 @@ simple_save_creds_helper(svn_boolean_t *saved,
> > *saved = FALSE;
> > }
> >
> > - if (password_stored)
> > + /* If we cached anything, write it to disk. */
> > + if (username_stored || password_stored)
> > {
> > err = svn_config_write_auth_data(creds_hash, SVN_AUTH_CRED_SIMPLE,
> > realmstring, config_dir, pool);
>
> Looks good to me, modulo the above stylistic nits. I assume this passes
> 'make check'?
Yes. Passes svnserveautocheck on HEAD as of a few hours ago. (I don't
think username/password caching is well-covered by tests, though?)
> (I'll start a 'make check' run right now, but won't be
> able to see the results till tomorrow morning.)
>
> -Karl
>
Thanks for the review Karl.
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-04-22 22:50:02 CEST