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

Re: svn commit: rev 4621 - in trunk/subversion: include libsvn_client clients/cmdline svnversion

From: <rooneg_at_electricjellyfish.net>
Date: 2003-01-30 02:22:26 CET

On Wednesday, January 29, 2003, at 07:53 PM, Greg Stein wrote:

 On Mon, Jan 27, 2003 at 06:13:01PM -0600, rooneg@tigris.org wrote:
 ...
 +++ trunk/subversion/libsvn_client/switch.c Mon Jan 27 18:12:38 2003
 ...
 @@ -133,7 +134,9 @@
    /* Get the RA vtable that matches working copy's current URL. */
    SVN_ERR (svn_ra_init_ra_libs (ra_baton, pool));
    SVN_ERR (svn_ra_get_ra_library (ra_lib, ra_baton, URL, pool));
 -
 +
 + SVN_ERR (svn_client_ctx_get_auth_baton (ctx, auth_baton));

 Eek. This code is *inside* libsvn_client. Our typical pattern is that a
 structure is opaque outside the library, but completely visible inside
 the
 library. Thus, we don't need to use getters/setters within the
 library. The
 code can simply refer to ctx-auth_baton.

i could go either way on this. keeping it opaque seemed like a
reasonable idea so that we could easily add new things to it in the
future (post-1.0) while still remaining binary compatability, but
realistically, if we're adding new callbacks or something keeping
binary compatability might be pointless if we're breaking semantic
compatability.

 ...
    else if ((src_is_url) (! dst_is_url))
 - SVN_ERR (repos_to_wc_copy (src_path, src_revision,
 - dst_path, optional_adm_access,
 auth_baton,
 - notify_func, notify_baton,
 - pool));
 + {
 + svn_client_auth_baton_t *auth_baton;
 +
 + SVN_ERR (svn_client_ctx_get_auth_baton (ctx, auth_baton));

 + SVN_ERR (repos_to_wc_copy (src_path, src_revision,
 + dst_path, optional_adm_access,
 auth_baton,
 + notify_func, notify_baton,
 + pool));
 + }

 From a simple line to a big block. Keeping the structure visible
 within the
 library would avoid this kind of stuff.
 ...
 +++ trunk/subversion/libsvn_client/context.c Mon Jan 27 18:12:46 2003
 ...
 +svn_error_t *
 +svn_client_ctx_get_auth_baton (svn_client_ctx_t *ctx,
 + svn_client_auth_baton_t **auth_baton)
 +{
 + *auth_baton = ctx-auth_baton;
 +
 + if (! *auth_baton)
 + return svn_error_create (SVN_ERR_CLIENT_CTX_NOT_FOUND, NULL,
 + no authentication baton found);

 Why the whole error thing? Just return NULL. It could be quite a valid
 approach for the caller to want to check if auth stuff has been
 installed or
 not. They shouldn't have to deal with error returns.

as i start playing with the notification baton, it is getting more and
more annoying having the error return, since having a null notification
function/baton is completely valid. i'm definately leaning towards
forgetting about the error and just returning null if there isn't
anything set.

 Basically, it seems like there is too much engineering around what
 could
 be a simple structure.

i'm still not sure if it's worth leaving this opaque or not. is their
any advantage in being able to change this later while retaining binary
compatability?

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 14 02:24:52 2006

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