-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Karl,
Karl Fogel wrote:
> Oops, sorry Senthil. I owed a response to your response to my response
> to your patch :-).
:) My comments inline.
>> If there is a change in credentials then the changed creds will be cached in
>> the auth area afresh. This is what happens now. This was the behavior
>> previously in older versions, so I didn't want to break it, because it breaks
>> tests.
>
> Hmm. That seems counterintuitive to me, but if it's current behavior,
> let's keep it for now, and raise this as a separate issue (unrelated to
> your patch).
Ok.
> Well, the current caller uses it temporarily, but simple_username_get()
> is providing an interface, and we don't know who will use it in the
> future. It might be good to state explicitly that:
>
> "*USERNAME will have the same lifetime as CREDS."
Documented the above in "simple_username_get()".
>> - svn_boolean_t may_save = username || password;
>> + const char *def_username = NULL;
>> + const char *def_password = NULL;
>> + 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;
>
> I think the subsequent code would become much more comprehensible if you
> document some of these variables here, in particular need_to_save. (For
> example, at the end when you assign "creds->may_save = need_to_save;",
> the semantic transfer from one to the other will be confusing unless the
> reader totally understands what need_to_save's semantics are.)
Added comments for the above variables.
> With def_username and def_password, does "def" mean "default"? If so,
> just write it out -- that way their meaning is instantly clear.
Done this in new patch.
Another instance of the above in "prompt_for_simple_creds()" was corrected in
r32717.
>> + else
>> + {
>> + /* Nothing was present in the auth cache, so save the credentials if
>> + we are asked to do so. */
>> + need_to_save = TRUE;
>> + }
>
> Where's the check for "if we are asked to do so", though? This new
> 'else' is the counter-condition to this 'if':
>
> /* We have something in the auth cache for this realm. */
> if (! err && creds_hash)
> {
> [...]
> }
>
> Or do you mean "... so indicate that these credentials should be saved
> if saving is allowed by the run-time configuration"? (Since the
> run-time config is checked in svn_auth__simple_save_creds_helper(), I
> guess.)
Yes this is checked in svn_auth__simple_save_creds_helper() before saving, so I
changed the comment in the attached patch according to the above (as you have
specified).
[[[
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.
Patch by: stylesen
Found by: arfrever
Review by: kfogel
]]]
Thank You.
- --
Senthil Kumaran S
http://www.stylesen.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFItAQE9o1G+2zNQDgRAt8XAJ4gO2mfjZjtvAikLPxA8rXKTnEXSwCghM/L
xiBs5/tMDZlwMDW7x7UOujc=
=a9mO
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-26 15:24:58 CEST