Re: multiple password prompts

On Tue, Jun 17, 2008 at 04:30:27PM +0530, Senthil Kumaran S wrote:
> Index: subversion/libsvn_subr/simple_providers.c
> ===================================================================
> --- subversion/libsvn_subr/simple_providers.c (revision 31760)
> +++ subversion/libsvn_subr/simple_providers.c (working copy)
> @@ -311,7 +311,7 @@
> * apr_palloc().
> */
> cached_answer = apr_palloc(pool, sizeof(svn_boolean_t));
> - *cached_answer = may_save_password;
> + *cached_answer = TRUE;
> apr_hash_set(b->plaintext_answers, realmstring,
> APR_HASH_KEY_STRING, cached_answer);
> }

Hey Senthil,

I don't think this patch is correct.

If you hard code *cached_answer to TRUE, you are overriding whatever
answer the user gave you. After all, the user may have said 'no',
in which case may_save_password is FALSE.

The else condition at line number 297 you are referring to
is not triggered based on whether *cached_answer is TRUE
or FALSE. It's based on whether cached_answer is NULL or not.
It is NULL if we cannot find an answer in the cache. The value
of the answer (TRUE or FALSE) does not matter.

Maybe the check should be rewritten to make this more clear,
like this:

Index: subversion/libsvn_subr/simple_providers.c
===================================================================
--- subversion/libsvn_subr/simple_providers.c (revision 31725)
+++ subversion/libsvn_subr/simple_providers.c (working copy)
@@ -286,7 +286,7 @@ svn_auth__simple_save_creds_helper(svn_boolean_t *
                   cached_answer = apr_hash_get(b->plaintext_answers,
                                                realmstring,
                                                APR_HASH_KEY_STRING);
- if (cached_answer)
+ if (cached_answer != NULL)
                     may_save_password = *cached_answer;
                   else
                     {

Do you agree or have I misunderstood you?

Stefan