You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by cm...@apache.org on 2012/08/20 17:19:06 UTC
svn commit: r1375052 - /subversion/trunk/subversion/libsvn_subr/auth.c
Author: cmpilato
Date: Mon Aug 20 15:19:06 2012
New Revision: 1375052
URL: http://svn.apache.org/viewvc?rev=1375052&view=rev
Log:
Minor code simplifications. No logical changes.
* subversion/libsvn_subr/auth.c
(SVN__MAYBE_ADD_PROVIDER): New macro.
(svn_auth_get_platform_specific_client_providers): Avoid logic
branch by recognizing that svn_config_get() returns the default
value when the passed-in config object is NULL. Also, replace a
bunch of "if-provider-isn't-NULL-then-add-the-array" logic with
SVN__MAYBE_ADD_PROVIDER() macro calls.
Modified:
subversion/trunk/subversion/libsvn_subr/auth.c
Modified: subversion/trunk/subversion/libsvn_subr/auth.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/auth.c?rev=1375052&r1=1375051&r2=1375052&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/auth.c (original)
+++ subversion/trunk/subversion/libsvn_subr/auth.c Mon Aug 20 15:19:06 2012
@@ -523,34 +523,29 @@ svn_auth_get_platform_specific_client_pr
apr_array_header_t *password_stores;
int i;
+#define SVN__MAYBE_ADD_PROVIDER(list, p) \
+ { if (p) APR_ARRAY_PUSH(list, svn_auth_provider_object_t *) = p; }
+
#define SVN__DEFAULT_AUTH_PROVIDER_LIST \
"gnome-keyring,kwallet,keychain,gpg-agent,windows-cryptoapi"
- if (config)
- {
- svn_config_get
- (config,
- &password_stores_config_option,
- SVN_CONFIG_SECTION_AUTH,
- SVN_CONFIG_OPTION_PASSWORD_STORES,
- SVN__DEFAULT_AUTH_PROVIDER_LIST);
- }
- else
- {
- password_stores_config_option = SVN__DEFAULT_AUTH_PROVIDER_LIST;
- }
-
*providers = apr_array_make(pool, 12, sizeof(svn_auth_provider_object_t *));
- password_stores
- = svn_cstring_split(password_stores_config_option, " ,", TRUE, pool);
+ /* Fetch the configured list of password stores, and split them into
+ an array. */
+ svn_config_get(config,
+ &password_stores_config_option,
+ SVN_CONFIG_SECTION_AUTH,
+ SVN_CONFIG_OPTION_PASSWORD_STORES,
+ SVN__DEFAULT_AUTH_PROVIDER_LIST);
+ password_stores = svn_cstring_split(password_stores_config_option,
+ " ,", TRUE, pool);
for (i = 0; i < password_stores->nelts; i++)
{
const char *password_store = APR_ARRAY_IDX(password_stores, i,
const char *);
-
/* GNOME Keyring */
if (apr_strnatcmp(password_store, "gnome-keyring") == 0)
{
@@ -558,104 +553,68 @@ svn_auth_get_platform_specific_client_pr
"gnome_keyring",
"simple",
pool));
-
- if (provider)
- APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) = provider;
+ SVN__MAYBE_ADD_PROVIDER(*providers, provider);
SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
"gnome_keyring",
"ssl_client_cert_pw",
pool));
-
- if (provider)
- APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) = provider;
-
- continue;
+ SVN__MAYBE_ADD_PROVIDER(*providers, provider);
}
-
/* GPG-AGENT */
- if (apr_strnatcmp(password_store, "gpg-agent") == 0)
+ else if (apr_strnatcmp(password_store, "gpg-agent") == 0)
{
SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
"gpg_agent",
"simple",
pool));
-
- if (provider)
- APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) = provider;
-
- continue;
+ SVN__MAYBE_ADD_PROVIDER(*providers, provider);
}
-
/* KWallet */
- if (apr_strnatcmp(password_store, "kwallet") == 0)
+ else if (apr_strnatcmp(password_store, "kwallet") == 0)
{
SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
"kwallet",
"simple",
pool));
-
- if (provider)
- APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) = provider;
+ SVN__MAYBE_ADD_PROVIDER(*providers, provider);
SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
"kwallet",
"ssl_client_cert_pw",
pool));
- if (provider)
- APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) = provider;
-
- continue;
+ SVN__MAYBE_ADD_PROVIDER(*providers, provider);
}
-
/* Keychain */
- if (apr_strnatcmp(password_store, "keychain") == 0)
+ else if (apr_strnatcmp(password_store, "keychain") == 0)
{
SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
"keychain",
"simple",
pool));
-
- if (provider)
- APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) = provider;
+ SVN__MAYBE_ADD_PROVIDER(*providers, provider);
SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
"keychain",
"ssl_client_cert_pw",
pool));
-
- if (provider)
- APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) = provider;
-
- continue;
+ SVN__MAYBE_ADD_PROVIDER(*providers, provider);
}
-
/* Windows */
- if (apr_strnatcmp(password_store, "windows-cryptoapi") == 0)
+ else if (apr_strnatcmp(password_store, "windows-cryptoapi") == 0)
{
SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
"windows",
"simple",
pool));
-
- if (provider)
- APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) = provider;
+ SVN__MAYBE_ADD_PROVIDER(*providers, provider);
SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
"windows",
"ssl_client_cert_pw",
pool));
-
- if (provider)
- APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) = provider;
-
- continue;
+ SVN__MAYBE_ADD_PROVIDER(*providers, provider);
}
-
- return svn_error_createf(SVN_ERR_BAD_CONFIG_VALUE, NULL,
- _("Invalid config: unknown password store "
- "'%s'"),
- password_store);
}
return SVN_NO_ERROR;
Re: svn commit: r1375052 -
/subversion/trunk/subversion/libsvn_subr/auth.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Backport review:
cmpilato@apache.org wrote on Mon, Aug 20, 2012 at 15:19:06 -0000:
> Author: cmpilato
> Date: Mon Aug 20 15:19:06 2012
> New Revision: 1375052
>
> URL: http://svn.apache.org/viewvc?rev=1375052&view=rev
> Log:
> Minor code simplifications. No logical changes.
>
> * subversion/libsvn_subr/auth.c
> (SVN__MAYBE_ADD_PROVIDER): New macro.
> (svn_auth_get_platform_specific_client_providers): Avoid logic
> branch by recognizing that svn_config_get() returns the default
> value when the passed-in config object is NULL. Also, replace a
> bunch of "if-provider-isn't-NULL-then-add-the-array" logic with
> SVN__MAYBE_ADD_PROVIDER() macro calls.
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/auth.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/auth.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/auth.c?rev=1375052&r1=1375051&r2=1375052&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/auth.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/auth.c Mon Aug 20 15:19:06 2012
> @@ -523,34 +523,29 @@ svn_auth_get_platform_specific_client_pr
> apr_array_header_t *password_stores;
> int i;
>
> +#define SVN__MAYBE_ADD_PROVIDER(list, p) \
> + { if (p) APR_ARRAY_PUSH(list, svn_auth_provider_object_t *) = p; }
> +
Parentheses around uses of macro parameters?
+ { if ((p)) APR_ARRAY_PUSH((list), svn_auth_provider_object_t *) = (p); }
> - return svn_error_createf(SVN_ERR_BAD_CONFIG_VALUE, NULL,
> - _("Invalid config: unknown password store "
> - "'%s'"),
> - password_store);
I think this is the only operational change in this revision. It would
have been easier to review this revision if it consisted of a single
hunk removing the above four lines.
> }
>
> return SVN_NO_ERROR;
>
>
Re: svn commit: r1375052 -
/subversion/trunk/subversion/libsvn_subr/auth.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Backport review:
cmpilato@apache.org wrote on Mon, Aug 20, 2012 at 15:19:06 -0000:
> Author: cmpilato
> Date: Mon Aug 20 15:19:06 2012
> New Revision: 1375052
>
> URL: http://svn.apache.org/viewvc?rev=1375052&view=rev
> Log:
> Minor code simplifications. No logical changes.
>
> * subversion/libsvn_subr/auth.c
> (SVN__MAYBE_ADD_PROVIDER): New macro.
> (svn_auth_get_platform_specific_client_providers): Avoid logic
> branch by recognizing that svn_config_get() returns the default
> value when the passed-in config object is NULL. Also, replace a
> bunch of "if-provider-isn't-NULL-then-add-the-array" logic with
> SVN__MAYBE_ADD_PROVIDER() macro calls.
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/auth.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/auth.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/auth.c?rev=1375052&r1=1375051&r2=1375052&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/auth.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/auth.c Mon Aug 20 15:19:06 2012
> @@ -523,34 +523,29 @@ svn_auth_get_platform_specific_client_pr
> apr_array_header_t *password_stores;
> int i;
>
> +#define SVN__MAYBE_ADD_PROVIDER(list, p) \
> + { if (p) APR_ARRAY_PUSH(list, svn_auth_provider_object_t *) = p; }
> +
Parentheses around uses of macro parameters?
+ { if ((p)) APR_ARRAY_PUSH((list), svn_auth_provider_object_t *) = (p); }
> - return svn_error_createf(SVN_ERR_BAD_CONFIG_VALUE, NULL,
> - _("Invalid config: unknown password store "
> - "'%s'"),
> - password_store);
I think this is the only operational change in this revision. It would
have been easier to review this revision if it consisted of a single
hunk removing the above four lines.
> }
>
> return SVN_NO_ERROR;
>
>
Re: svn commit: r1375052 - /subversion/trunk/subversion/libsvn_subr/auth.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/20/2012 12:00 PM, Bert Huijben wrote:
> I like that change too. Maybe we should backport it to 1.7.x in some way?
Agreed! I'll propose it in a bit if no one beats me to it.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
RE: svn commit: r1375052 - /subversion/trunk/subversion/libsvn_subr/auth.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: C. Michael Pilato [mailto:cmpilato@collab.net]
> Sent: maandag 20 augustus 2012 17:44
> To: Bert Huijben
> Cc: cmpilato@apache.org; dev@subversion.apache.org
> Subject: Re: svn commit: r1375052 -
> /subversion/trunk/subversion/libsvn_subr/auth.c
>
> On 08/20/2012 11:22 AM, Bert Huijben wrote:
> > Your log message says no 'logical changes', while this seems to remove
> > the error for unknown password stores?
>
> Doh! Good catch, Bert. Well, as it turns out, I like that change, so I'll
> just fix the log message.
I like that change too. Maybe we should backport it to 1.7.x in some way?
Bert
Re: svn commit: r1375052 - /subversion/trunk/subversion/libsvn_subr/auth.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/20/2012 11:22 AM, Bert Huijben wrote:
> Your log message says no 'logical changes', while this seems to remove
> the error for unknown password stores?
Doh! Good catch, Bert. Well, as it turns out, I like that change, so I'll
just fix the log message.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
RE: svn commit: r1375052 - /subversion/trunk/subversion/libsvn_subr/auth.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: cmpilato@apache.org [mailto:cmpilato@apache.org]
> Sent: maandag 20 augustus 2012 17:19
> To: commits@subversion.apache.org
> Subject: svn commit: r1375052 -
> /subversion/trunk/subversion/libsvn_subr/auth.c
>
> Author: cmpilato
> Date: Mon Aug 20 15:19:06 2012
> New Revision: 1375052
>
> URL: http://svn.apache.org/viewvc?rev=1375052&view=rev
> Log:
> Minor code simplifications. No logical changes.
>
> * subversion/libsvn_subr/auth.c
> (SVN__MAYBE_ADD_PROVIDER): New macro.
> (svn_auth_get_platform_specific_client_providers): Avoid logic
> branch by recognizing that svn_config_get() returns the default
> value when the passed-in config object is NULL. Also, replace a
> bunch of "if-provider-isn't-NULL-then-add-the-array" logic with
> SVN__MAYBE_ADD_PROVIDER() macro calls.
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/auth.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/auth.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/aut
> h.c?rev=1375052&r1=1375051&r2=1375052&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_subr/auth.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/auth.c Mon Aug 20 15:19:06
> 2012
> @@ -523,34 +523,29 @@ svn_auth_get_platform_specific_client_pr
> apr_array_header_t *password_stores;
> int i;
>
> +#define SVN__MAYBE_ADD_PROVIDER(list, p) \
> + { if (p) APR_ARRAY_PUSH(list, svn_auth_provider_object_t *) = p; }
> +
> #define SVN__DEFAULT_AUTH_PROVIDER_LIST \
> "gnome-keyring,kwallet,keychain,gpg-agent,windows-cryptoapi"
>
> - if (config)
> - {
> - svn_config_get
> - (config,
> - &password_stores_config_option,
> - SVN_CONFIG_SECTION_AUTH,
> - SVN_CONFIG_OPTION_PASSWORD_STORES,
> - SVN__DEFAULT_AUTH_PROVIDER_LIST);
> - }
> - else
> - {
> - password_stores_config_option =
> SVN__DEFAULT_AUTH_PROVIDER_LIST;
> - }
> -
> *providers = apr_array_make(pool, 12, sizeof(svn_auth_provider_object_t
> *));
>
> - password_stores
> - = svn_cstring_split(password_stores_config_option, " ,", TRUE, pool);
> + /* Fetch the configured list of password stores, and split them into
> + an array. */
> + svn_config_get(config,
> + &password_stores_config_option,
> + SVN_CONFIG_SECTION_AUTH,
> + SVN_CONFIG_OPTION_PASSWORD_STORES,
> + SVN__DEFAULT_AUTH_PROVIDER_LIST);
> + password_stores = svn_cstring_split(password_stores_config_option,
> + " ,", TRUE, pool);
>
> for (i = 0; i < password_stores->nelts; i++)
> {
> const char *password_store = APR_ARRAY_IDX(password_stores, i,
> const char *);
>
> -
> /* GNOME Keyring */
> if (apr_strnatcmp(password_store, "gnome-keyring") == 0)
> {
> @@ -558,104 +553,68 @@ svn_auth_get_platform_specific_client_pr
> "gnome_keyring",
> "simple",
> pool));
> -
> - if (provider)
> - APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) =
> provider;
> + SVN__MAYBE_ADD_PROVIDER(*providers, provider);
>
> SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
> "gnome_keyring",
> "ssl_client_cert_pw",
> pool));
> -
> - if (provider)
> - APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) =
> provider;
> -
> - continue;
> + SVN__MAYBE_ADD_PROVIDER(*providers, provider);
> }
> -
> /* GPG-AGENT */
> - if (apr_strnatcmp(password_store, "gpg-agent") == 0)
> + else if (apr_strnatcmp(password_store, "gpg-agent") == 0)
> {
> SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
> "gpg_agent",
> "simple",
> pool));
> -
> - if (provider)
> - APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) =
> provider;
> -
> - continue;
> + SVN__MAYBE_ADD_PROVIDER(*providers, provider);
> }
> -
> /* KWallet */
> - if (apr_strnatcmp(password_store, "kwallet") == 0)
> + else if (apr_strnatcmp(password_store, "kwallet") == 0)
> {
> SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
> "kwallet",
> "simple",
> pool));
> -
> - if (provider)
> - APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) =
> provider;
> + SVN__MAYBE_ADD_PROVIDER(*providers, provider);
>
> SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
> "kwallet",
> "ssl_client_cert_pw",
> pool));
> - if (provider)
> - APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) =
> provider;
> -
> - continue;
> + SVN__MAYBE_ADD_PROVIDER(*providers, provider);
> }
> -
> /* Keychain */
> - if (apr_strnatcmp(password_store, "keychain") == 0)
> + else if (apr_strnatcmp(password_store, "keychain") == 0)
> {
> SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
> "keychain",
> "simple",
> pool));
> -
> - if (provider)
> - APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) =
> provider;
> + SVN__MAYBE_ADD_PROVIDER(*providers, provider);
>
> SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
> "keychain",
> "ssl_client_cert_pw",
> pool));
> -
> - if (provider)
> - APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) =
> provider;
> -
> - continue;
> + SVN__MAYBE_ADD_PROVIDER(*providers, provider);
> }
> -
> /* Windows */
> - if (apr_strnatcmp(password_store, "windows-cryptoapi") == 0)
> + else if (apr_strnatcmp(password_store, "windows-cryptoapi") == 0)
> {
> SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
> "windows",
> "simple",
> pool));
> -
> - if (provider)
> - APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) =
> provider;
> + SVN__MAYBE_ADD_PROVIDER(*providers, provider);
>
> SVN_ERR(svn_auth_get_platform_specific_provider(&provider,
> "windows",
> "ssl_client_cert_pw",
> pool));
> -
> - if (provider)
> - APR_ARRAY_PUSH(*providers, svn_auth_provider_object_t *) =
> provider;
> -
> - continue;
> + SVN__MAYBE_ADD_PROVIDER(*providers, provider);
> }
> -
> - return svn_error_createf(SVN_ERR_BAD_CONFIG_VALUE, NULL,
> - _("Invalid config: unknown password store "
> - "'%s'"),
> - password_store);
Your log message says no 'logical changes', while this seems to remove the error for unknown password stores?
Bert
> }
>
> return SVN_NO_ERROR;
>