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