[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r31428 - in trunk/subversion: include libsvn_subr

From: Arfrever Frehtes Taifersar Arahesis <arfrever.fta_at_gmail.com>
Date: Mon, 26 May 2008 12:29:01 +0200

2008/5/26 Daniel Shahaf <d.s_at_daniel.shahaf.co.il>:
> Arfrever Frehtes Taifersar Arahesis wrote on Mon, 26 May 2008 at 11:23 +0200:
>> 2008-05-26 08:06 Daniel Shahaf <d.s_at_daniel.shahaf.co.il> napisał(a):
>> > Karl Fogel wrote on Sun, 25 May 2008 at 22:30 -0400:
>> >> Arfrever Frehtes Taifersar Arahesis <arfrever.fta_at_gmail.com> writes:
>> >> >> Log:
>> >> >> Implement the 'password-stores' config option.
>> >> >>
>> >> >> --- trunk/subversion/libsvn_subr/cmdline.c (r31427)
>> >> >> +++ trunk/subversion/libsvn_subr/cmdline.c (r31428)
>> >> >> @@ -435,17 +435,45 @@ svn_cmdline_setup_auth_baton(svn_auth_ba
>> >> >>
>> >> >> [...]
>> >> >>
>> >> >> #ifdef SVN_HAVE_KWALLET
>> >> >> - if (get_auth_simple_provider(&provider, "kwallet", pool))
>> >> >> - {
>> >> >> - APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
>> >> >> + if (apr_strnatcmp(password_store, "kwallet") == 0)
>> >> >> + {
>> >> >> + if (get_auth_simple_provider(&provider, "kwallet", pool))
>> >> >> + {
>> >> >> + APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
>> >> >> + }
>> >> >> + continue;
>> >> >> + }
>> >> >> +#endif
>> >> >> + /* TODO: Error. */
>> >
>> > Don't you need to also allow the case where password_store == ""?
>>
>> svn_cstring_split() doesn't allow password_store to be "".
>>
>
> Sorry, I misread svn_cstring_split()'s docstring. It says that the
> array will not contain empty elements -- I missed the "not".
>
>> When the 'password-stores' config option is set to "", then
>> password_stores->nelts should be 0, so the code inside the 'for (i =
>> 0; i < password_stores->nelts; i++)' loop shouldn't be used.
>>
>> >> > Which error code should be used for invalid value of DAV-unrelated
>> >> > configuration option?
>> >>
>> >
>> > Nothing? It will fall back to plaintext passwords and get a warning
>> > there...
>>
>> When the 'http-library' option is set to invalid value, then the
>> "Invalid config: unknown HTTP library '%s'" error is printed instead
>> of falling back to Neon.
>> I would prefer consistent behavior.
>>
>
> That error is only printed when both serf and neon are enabled at
> compile time. Therefore, ignoring gnome-keyring when it is not enabled
> at compile time might be considered consistent...
>
> e.g. If I have two clients, one with kwallet and one with both kwallet and
> gnome-keyring, and my password-stores option is 'gnome-keyring,kwallet',
> then I'll want the first client to use kwallet, not to complain that
> 'gnome-keyring' is not a known value...

OK.

I will replace:
<snip>
#ifdef SVN_HAVE_GNOME_KEYRING
      if (apr_strnatcmp(password_store, "gnome-keyring") == 0)
        {
          if (get_auth_simple_provider(&provider, "gnome_keyring", pool))
            {
              APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *)
= provider;
            }
          continue;
        }
#endif
</snip>

With:
<snip>
      if (apr_strnatcmp(password_store, "gnome-keyring") == 0)
        {
#ifdef SVN_HAVE_GNOME_KEYRING
          if (get_auth_simple_provider(&provider, "gnome_keyring", pool))
            {
              APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *)
= provider;
            }
#endif
          continue;
        }
</snip>

(And the same for KWallet-related code.)
Received on 2008-05-26 12:29:18 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.