Greg Hudson <ghudson@MIT.EDU> writes:
> As it's much easier to make API changes before 1.0 than after, here
> are a list of issues we might want to solve soon. The first three
> seem important; the rest are just unfortunate foibles (except for #6,
> but that would require a serious push to fix). Many of these have
> received attention in the past, but time marched on without anyone
> fixing them.
Wow, nice list! Glad you were keeping track.
I'll make some specific comments below. After a bit of thread
discussion here to reach any obvious conclusions, I'll file issues for
all of these that remain. They'll be a lot easier to manage that way;
we can schedule them individually and have clear record of what we've
decided.
> #1: We define symbols in the APR namespace:
> In svn_sorts.h: apr_hash_sorted_keys
> apr_array_prepend
> In svn_types.h: APR_ARRAY_IDX
> APR_ARRAY_PUSH
> apr_atoui64
> IMPACT OF PROBLEM: We're not very sociable, and might conflict with
> a future version of APR which includes these symbols.
> DIFFICULTY OF FIX: Depends on how we fix them.
On dev@apr, Sander Striker is proposing to get at least some of these
into APR. That may happen quickly, or may not, but in any case it's
too late for us because we need to work with the APR that ships today.
I suggest we simply switch the prefixes (prefices?) to "svn_apr_" and
"SVN_APR_", to avoid any possibility of conflict, then remove our
definitions and switch them back when and if APR gets these functions.
I can do this, if we agree.
(My goal here is: least impact on Subversion while maintaining
compatibility with future APRs.)
> #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.
Agree with solution. Again volunteer, though am happy if someone else
wants to do it.
> #3: Recursion is specified as a boolean in svn_ra operations, but
> we have vague plans to support either tri-state (0/1/infinity)
> recursion or arbitrary recursion depth.
> IMPACT OF PROBLEM: Supporting enhanced recursion will require a
> difficult API change.
> DIFFICULTY OF FIX: Medium; we need to agree on a new API, and then
> all ra layers need a trivial change across half a dozen
> functions.
I think we should punt on this for 1.0; the discussion would be a big
thread, and we're going to have enough of those between now and 1.0.
The API change itself is no more difficult later than it is now. We
will break code & binary compatibility, but we've always accepted that
future versions will do that, and we have a release numbering scheme
to handle it.
> #4: Even though we introduced "" as a no-op path element years ago,
> svn_wc_get_actual_target() returns NULL in *target if anchor is
> the actual target, and various APIs support NULL path elements
> for compatibility with that.
> IMPACT OF PROBLEM: API is somewhat inconsistent and confusing.
> Fixing after 1.0 is unlikely to ever be worth the effort.
> DIFFICULTY OF FIX: Medium, and with some change of destabilization.
> (I had a go at this in issue #999.)
I agree with Greg Stein's comment in the issue, that this isn't
necessary for 1.0. If you have the motivation to finish #999, go for
it, but I think we can live with things as they are for 1.0.
(I don't see why it would be worth it now, but not worth it after
1.0.)
> #5: svn_ra_init_ra_libs and friends use a void * baton instead of an
> opaque type.
> IMPACT OF PROBLEM: The interface is needlessly non-type-safe.
> DIFFICULT OF FIX: Minor.
Sure. Not a tragedy if it doesn't get fixed before 1.0; I'm of a mind
to punt on the 1.0 release line, but maybe get a fix into trunk when
sometime when we're going to break binary compatibility anyway.
Oh wait, this wouldn't even break binary compatibility, would it?
> #6: We still use getdate.y, which implements a bizarre and arbitrary
> set of date formats. (Did you know that "3" is a date?)
> IMPACT OF PROBLEM: We'll be stuck with getdate.y forever.
> DIFFICULTY OF FIX: Major. And we already have people (including
> developers) relying on getdate's foibles. Bleah.
We don't really need to fix this for 1.0, as long as whatever date
formats we extend to in the future are a superset of what we currently
support. (Or a superset of all the ones people actually use, anyway
:-) ).
What are the particular foibles people are relying on already?
I'd like to avoid messing with this, unless the current interface
truly guarantees brokenness down the road.
> #7: Overly defensive programming in:
> svn_path_uri_endode
> svn_path_uri_decode
> svn_cstring_split_append
> The path_uri functions actually document that they will pass
> through NULL, even though there is no use case which makes that
> especially convenient. svn_cstring_split_append accepts a NULL
> input even though it is not documented that it does so.
> IMPACT OF PROBLEM: Minor; we will be trapped into providing
> interface guarantees for these functions which we don't provide
> most places.
> DIFFICULTY OF FIX: Trivial.
I'd be happy with just removing the doc promises now and changing the
code later, or changing both now. What color would we like to paint
the bikeshed today? :-)
> #8: log_message_receiver_t accepts a hash of changed paths instead
> of an array. This appears to be for the convenience of existing
> callers.
> IMPACT OF PROBLEM: Minor; changed_paths is not a pointer to const
> since you can't iterate over a const hash table.
> DIFFICULTY OF FIX: Medium.
IMHO the cost of fixing isn't worth the benefits of const.
> #9: log_message_receiver_t and commit_callback_t pass dates as const
> char * instead of apr_time_t, as a nod to the fact that dates
> are stored as text properties on revisions.
> IMPACT OF PROBLEM: API is not as clean as it could be; users of
> those types must perform extra input validation because of the
> possibility of malformed date strings.
> DIFFICULTY OF FIX: Medium.
Hmm. The current API was a deliberate decision. There are certainly
drawbacks to both ways, but I'm not sure apr_time_t would be a
definite win over the status quo.
By the way, for those that we are fixing, I don't think it matters
much whether the fixes go into the 0.35 branch or wait for the
1.0-stabilization branch. Whenever they're ready, they can go in.
(We should do them on trunk and then merge, though).
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Dec 12 20:59:23 2003