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

Re: A new look at the apr_off_t problem

From: <kfogel_at_collab.net>
Date: 2004-01-23 23:15:46 CET

Greg Hudson <ghudson@MIT.EDU> writes:
> Tobias has discovered a twist to the apr_off_t problem which requires
> us to take a step back.
>
> As I've detailed at length, using apr_off_t in our APIs is bad because
> (in decreasing order of importance):
>
> * It interacts badly with perl right now.
> * It interacts badly with a possible future APR change
> (see http://marc.theaimsgroup.com/?l=apr-dev&m=107481332607151&w=20)
> * It interacts badly with callers or callees which might be built
> with _FILE_OFFSET_BITS64.
>
> Unfortunately, eliminating it from our API is hard or worrisome
> because:
>
> * We use it in lots of places in the diff interface, and fixing that
> happens to dovetail into a secondary problem, that the diff
> interface doesn't use pools in a lot of places where it probably
> should.

Ooh.

> * Our svn_io APR wrappers use apr_finfo_t, which contains an
> apr_off_t. We can't reasonably duplicate the apr_finfo_t
> structure to work around this problem.

Double ooh with gut punch, and a twist of lime, yeah.

> So, we need to think about targeted solutions. There are three areas
> of the API in play here:
>
> 1. The blame callback uses an apr_off_t for the line number. It is
> easy to change this to use an apr_uint64_t instead. (The current
> #1710 patch does this.)

Although I'm all for doing less of this change (including none of it),
it'll be kind of strange that 'blame' takes apr_uint64_t while
everything else uses apr_off_t.

Let's remember that (aesthetically displeasing though it may be) we
have the opportunity to fix interfaces post-1.0 by adding new
interfaces, without breaking API compatibility.

> 2. The diff library uses apr_off_t all over the place. We can fix
> that by introducing svn_offset_t, and also introducing pool
> parameters all over the place. This is a good change, but it's
> also a big change, and not necessarily appropriate for 0.37.
> (The current #1710 does this.)

Same comment above about adding new interfaces.

Realistically, I don't think we're going to avoid some of that. This
could be one of those cases, where we keep the current diff
implementations, and add new ones that have the better interfaces.

Greg Hudson has objected to this approach (iirc) on the grounds that
while that's an okay philosophy for later releases, we should still
release 1.0 with the best interfaces we know how. I think he's got a
point. My counter-argument is only that there's no termination
condition for the API-polishing process. Last week we discover the
apr_off_t issue; this week we discover someone needs a pool who
doesn't have it; the week after that we discover who knows what? IOW,
the only way to release 1.0 is to treat it as a real goal, whose
deferment has costs.

This is an age-old tension, depicted in cave paintings and on pottery
shards from all over the world. I don't propose to resolve it here,
just acknowledge it.

> 3. The svn_io wrappers use apr_off_t and apr_finfo_t. The best we
> could do here is to declare those wrappers to be private for now,
> which would be a giant rename. (The current #1710 patch uses
> svn_offset_t in places of apr_off_t, but does not address
> apr_finfo_t.)
>
> Although (2) and (3) may bite us if we don't fix them (particularly if
> APR 1.0 changes the meaning of apr_off_t), the most important thing to
> fix is (1), because without that fix, we can't have working perl
> wrappers for blame. (Without (2), we can't have working perl wrappers
> for libsvn_diff, but I don't know if such a thing is really important,
> and Joe Orton thinks it may be possible to fix perl in the long run.)
> Certainly, we don't care about perl wrappers for (3).

Why is it more important to have Perl wrappers for blame than diff?

Or maybe it's more a cost/benefit thing -- it's easier to fix the
problem in blame, so we do it there, but shy away from diff. (?)

> So, I think the thing to do right now is create a patch for (1) (it
> will be something like a one-line change) and merge it in tonight for
> 0.37. On a slightly more leisurely schedule, we can work out a big
> patch for (2) and (3). Then we can argue about whether it's
> appropriate to roll an 0.38 in order to get that patch in.

I won't veto this, though I'd prefer we fix blame & diff in the same
way and at the same time.

Right now, I'm inclined not to roll a 0.38 for the rest. I'm sorry
about the Perl diff bindings, but there needs to be a limit to how
much trouble the core code goes through for bindings. Originally, the
bindings were supposed to be this lightweight thing that would float
on top of our APIs, and mostly keep themselves up to date. It hasn't
worked out that way, and while I appreciate the work that people do to
maintain them, I still come down on the "nice-to-have" rather than
"must-have" side w.r.t. the bindings.

Also, there is a workaround, albeit a clumsy one: you could recompile
your APR (and all its dependencies) to match Perl. If you really,
really need all the Perl bindings, this is probably doable. But yeah,
for most people it is unrealistic.

Anyway, I'm not trying to end the 0.38 discussion in advance, just
advertising my biases. Do the (1) change if you absolutely must :-),
then let's talk about the rest.

-K

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jan 24 00:13:12 2004

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