Re: condense_targets and basepath functions

On Thu, Feb 08, 2001 at 11:37:48AM -0500, Kevin Pilch-Bisson wrote:
>...
> Here are the prototypes I have come up with so far:
> svn_error_t *
> svn_?_condense_targets(svn_string_t ***pcondensed_targets,
> int *pnum_condensed_targets,
> const svn_string_t **targets,
> int num_targets,
> apr_pool_t *pool);
>
> svn_error_t *
> svn_?_get_basepath(svn_string_t **pbasepath,
> const svn_string_t **targets,
> int num_targets,
> apr_pool_t *pool);
>
> Now for questions and notes:
>
> 1) What library should they be part of?

subr. I see them as part of path processing.

> 2) They currently return svn_error_t *, but nothing they call generates
> an error, so they could just as well return results directly.

Yup. Return an apr_array_header_t*, containing the svn_string_t* ptrs.

> 3) In condense_targets, I can calculate the basepath as a side-effect,
> although it would be (slightly) less efficient than doing it after,
> depending on the order of targets.

Optional return value from condense_targets. If pbasepath != NULL, then you
get the value returned to you.

> 4) I have been creating a subpool of pool for temporary allocation, and
> apr_pool_destroy()'ing it at the end of the functions, is this
> appropriate?

I'd say no.

SVN already spends too much time trying to be "nifty" with its allocations.
As a result, we end up with things in the wrong pools, and then we have
lifetime mismatches.

Put your allocations and temporary allocations directly into the provided
pool. If the *caller* ends up calling these things way too often (e.g.
unbounded due to be proportional to, say, a tree traversal), then the caller
can create subpools. Or we can revise the function.

Trying to optimize memory usage now (read: prematurely) can only lead to
problems.

> 5) I could also modify the targets array and num targets directly. Thus

I'm not sure about this one. I'd initially say "no", but defer to Ben on
whether the original set is needed.

>...
> Problems with the algo's:
> 1) Won't work if some paths are given relatively and some absolutely.
> 2) No garantee that basepath is in a working copy
> 3) No garantee that basepath is a directory if given only a single
> file as a target.
>
> What do you all think?

(1) must be fixed. Consider:

[gstein@kurgan mod_dav_svn]$ svn update . ~/src/svn/notes

(2) is no big deal. The WC has functions to detect if a given dir is a WC
dir or not. Note that a basepath outside of a WC usually means you have
disjoint WC's (and possibly different repositories).

(3) shouldn't be a problem. That is easily discoverable.

These last two points should be noted in the doc, so that callers can
understand the issues.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/