Re: [PATCH] Remove platform visibility from Subversion auth functions (Was: Python bindings build seems broken)

> When the platform-specific providers aren't available, Subversion will
> fall back to the basic provider (store the user/pass in plaintext in
> ~/.subversion). The fallback will be silent (since the tests disable
> all interactive prompts, including the 'plaintext passwords' prompt).
>
> Therefore, I suspect your patch may be passing the tests but doesn't use
> keychain at all. Partially because your patch #if's away the
> declaration of svn_auth_get_keychain_simple_provider()...
>
> More below.

I tested the Keychain stuff and it worked as expected. Whenever I
used the trunk build, with patch, to access something with cached
credentials stored in the Keychain, the Keychain was accessed. (On
OSX, I get a prompt asking if it's okay to use the new version of
Subversion to access the keychain since the cached credentials were
stored by an older version of Subversion.) Seems to be working fine
on OSX.

>
>> [[[
>> Remove platform visibility for Subversion auth functions.
> ...
>> ]]]
>
>> Index: subversion/libsvn_subr/cmdline.c
>> ===================================================================
>> --- subversion/libsvn_subr/cmdline.c (revision 33596)
>> +++ subversion/libsvn_subr/cmdline.c (working copy)
>> @@ -530,10 +530,18 @@
>> {
>> #ifdef SVN_HAVE_KEYCHAIN_SERVICES
>> svn_auth_get_keychain_simple_provider(&provider, pool);
>
> I'm surprised this call compiled, since your patch #if'd away the
> declaration of svn_auth_get_keychain_simple_provider().
>
>> - APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
>>
>> + if (provider)
>> + {
>> + APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
>
> Nit: over 80 characters.

Sorry about that. Somehow missed that one.

> Also, when I said on IRC that 'individual providers can promise more' -- we
> could make the individual providers we declare promise to return non-NULL on
> the appropriate platforms.
>
> For example,
>
> @@ svn_auth.h @@
> - * @note This function is only available on Mac OS 10.2 and higher.
> + * @note This function returns non-NULL iff the platform is Mac OS ™10.2.
> */
> void
> svn_auth_get_keychain_simple_provider(svn_auth_provider_object_t **provider,
>
> and then we don't need to touch this code in cmdline.c. (But presumably
> the bindings will be happier because the functions are defined on all
> platforms.)

Better documentation makes sense. Are you sure with the "promise" to
return non-NULL is enough? It just seemed like a good idea although
the only feasible way to get a NULL was on an unsupported platform.

> ...
>> Index: subversion/libsvn_subr/win32_crypto.c
>> ===================================================================
>> --- subversion/libsvn_subr/win32_crypto.c (revision 33596)
>> +++ subversion/libsvn_subr/win32_crypto.c (working copy)
>> @@ -18,8 +18,6 @@
>>
>> /* ==================================================================== */
>>
>> -#if defined(WIN32) && !defined(__MINGW32__)
>> -
>> /*** Includes. ***/
>>
>> #include <apr_pools.h>
>> @@ -33,8 +31,11 @@
>>
>> #include "svn_private_config.h"
>>
>> +#include <apr_base64.h>
>> +
>> +#if defined(WIN32) && !defined(__MINGW32__)
>> +
>> #include <wincrypt.h>
>> -#include <apr_base64.h>
>>
>
> So the #if and its #endif are on opposite side of the section delimiter (^L).

I can fix that.

>> /*-----------------------------------------------------------------------*/
>> /* Windows simple provider, encrypts the password on Win2k and later. */
>> @@ -158,18 +159,6 @@
>> windows_simple_save_creds
>> };
>>
>> -
>> -/* Public API */
>> -void
>> -svn_auth_get_windows_simple_provider(svn_auth_provider_object_t **provider,
>> - apr_pool_t *pool)
>> -{
>> - svn_auth_provider_object_t *po = apr_pcalloc(pool, sizeof(*po));
>> -
>> - po->vtable = &windows_simple_provider;
>> - *provider = po;
>> -}
>> -
>>
>> /*-----------------------------------------------------------------------*/
>> /* Windows SSL server trust provider, validates ssl certificate using */
>> @@ -281,16 +270,36 @@
>> NULL,
>> NULL,
>> };
>> +#endif /* WIN32 */
>>
>> /* Public API */
>> void
>> +svn_auth_get_windows_simple_provider(svn_auth_provider_object_t **provider,
>> + apr_pool_t *pool)
>> +{
>> + svn_auth_provider_object_t *po = apr_pcalloc(pool, sizeof(*po));
>> +
>> +#if defined(WIN32) && !defined(__MINGW32__)
>> + po->vtable = &windows_simple_provider;
>> +#else
>> + po->vtable = NULL;
>> +#endif
>> +
>> + *provider = po;
>> +}
>> +
>
> What I had in mind (and what other parts of your diff expect) is
>
> {
> #if defined(WIN32)
> /* body of function */
> #else
> *provider = NULL;
> #endif
> }
>
> but your version could work too. Either way, would be nice to
> explicitly document it.

Yeah. Better documentation here will help. A for who's way is ideal,
I like yours better. It's a little easier to identify what you're
trying to do.

>> Index: subversion/include/svn_client.h
>> ===================================================================
>> --- subversion/include/svn_client.h (revision 33596)
>> +++ subversion/include/svn_client.h (working copy)
>> @@ -160,7 +160,7 @@
>> apr_pool_t *pool);
>>
>>
>> -#if (defined(WIN32) && !defined(__MINGW32__)) || defined(DOXYGEN) || defined(CTYPESGEN)
>> +#if defined(DOXYGEN) || defined(CTYPESGEN)
>> /**
>> * Create and return @a *provider, an authentication provider of type @c
>> * svn_auth_cred_simple_t that gets/sets information from the user's
>> @@ -187,7 +187,7 @@
>> void
>> svn_client_get_windows_simple_provider(svn_auth_provider_object_t **provider,
>> apr_pool_t *pool);
>> -#endif /* WIN32 && !__MINGW32__ || DOXYGEN || CTYPESGEN */
>> +#endif /* DOXYGEN || CTYPESGEN */
>>
>
> This preprocessor condition will not compile the function on *any*
> platform. Presumably not what you intended.

Yeah, that wasn't intended.

Thanks for looking into this. I'll get these fixed and ready for commit.

-- 
Take care,
Jeremy Whitlock
http://www.thoughtspark.org