On Fri, Dec 12, 2003 at 11:34:10PM -0800, Ben Reser wrote:
> On Fri, Dec 12, 2003 at 11:02:08AM -0500, Greg Hudson wrote:
> > #2: The svn_client interface accepts a pointer to a context
> > structure which is allocated by the caller and filled in by the
> > caller.
> > IMPACT OF PROBLEM: We have no way of extending this structure with
> > new fields. We should provide a function to create and
> > initialize a context structure and mandate that the caller use
> > it.
> > DIFFICULTY OF FIX: Minor.
>
> +1
>
> The current situation is ugly IMHO. If nobody else wants to do this I'd
> be happy to do this. I was going to do something along this lines for
> the perl bindings anyway. So if we want it in the main API, I might as
> well just do it there instead.
Done and attached. Some comments on this.
* svn_client_open may not be the best name. I used that becasue we
have svn_auth_open which roughly serves the same purpose. Ohter
possible names that may be better are:
svn_client_init (suggested in the comment above the svn_config_ensure
call in main.c of the cmdline client).
svn_client_create_context (suggested by ghudson, but seems long IMHO).
svn_client_context_create (suggested by mbk, also long).
I already despise the long function names we have like:
svn_client_get_ssl_client_cert_file_provider()
Though granted svn_client_(create_context|context_create) aren't nearly
as bad. So if svn_auth_open is a bad example that we don't want to
follow here then I'd say svn_client_init is best.
* The function was added to a new file ctx.c because I didn't see any
other obvious place to put it in the client lib.
* In order to achieve presurvation of the ABI this assumes that the size
of existing members never changes and that their order stays the same in
the struct. If we only add members and always add them at the end then
this should keep us compatable.
The only way we might break that is by adding a member that causes the
compiler to pad the struct. Future changes might add more members that
then remove the need for the pad. We are currently only storing
pointers in the struct and as long as we don't forsee wanting to store
anything else other than pointers in the future then this should not be
an issue.
Further, the only way to resolve the possible problem of a padding
change would be to write and require clients to use accessor functions.
While this would provide a higher level of ABI protection, it's probably
not worth the complexity for the gain. The cases where we would need to
break this ABI should be far and few between and can be scheduled with
other changes that would necessitate an ABI change in spite of using
accessor functions.
* Per the comment in main.c of the cmdline client it'd be nice if there
was a place to put svn_config_ensure. svn_client_open is of course just
such a place now. If were were to set a hard limit that only
initialization that is *REQUIRED* for the client to do (as
svn_config_ensure is in this case) could go in the svn_client_open
function then it probably would be okay. New requirements that would
get added would likely be pretty rare and would of course necessitate
breaking the ABI because the client would have something new to do. In
these cases I think it would make sense moving those initalizations to
the svn_client_open function. However, to keep the impact of this patch
law I did not make such a change yet. But thought I would raise the
issue now.
* This patch will not break any existing clients. Clients that are
doing their own allocation now will continue to build and work despite
this patch going in. However, clients that don't change to use
svn_client_open would do so at the risk of breaking compatability at
some point down the line. This is good and bad. The good is that it
is unlikely to break anything. The bad is that client authors who
aren't paying attention might not make the appropriate change and then
break far down the line. I do not think this is a compelling argument
against implementing this. It's just something to note and that we
should loudly advertise this in the first release it goes out in.
Now that my wordy commentary is out of the way, here's the patch:
[[[
Future proof the ABI by providing an allocator/initializer function for
the client context.
* subversion/libsvn_client/ctx.c
* subversion/include/svn_client.h
(svn_client_open): New function
* subversion/clients/cmdline/main.c
* subversion/tools/examples/minimal_client.c
Use svn_client_open().
]]]
Attached due to existing lines that were too long and cause wrap.
--
Ben Reser <ben@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Dec 14 23:36:44 2003