Re: condense_targets and basepath functions

Ben,

What do think about what Greg/Karl said? Also should the targets string
be modified in place or a new one returned? Finally, what names would
like the functions to have? I am thinking:

/* Convert TARGETS into a list of absolute paths, removing any that are
   contained in other entries. Return the results in
   PCONDENSED_TARGETS. Additionally, if PBASENAME is non-NULL, return
   the common part of all the targets in pbasename(suitable to be used
   as the location to call start_edit), as an absolute path. Do any
   necessary allocation in POOL.

   NOTES:
   1) There is no guarantee that pbasename will be within a
   subversion working copy.
   2) If targets consists of a single file entry, then pbasename will be
   set to it, even though it is not a directory. */
svn_error_t *
svn_path_condense_targets(apr_array_header_t **pcondensed_targets,
                          svn_string_t **pbasename,
                          const apr_array_header_t *targets,
                          apr_pool_t *pool);

and

/* Return the common base path of TARGETS as an absolute path in
   PBASENAME. Do any necessary allocation in POOL.
 
 NOTES:
   1) There is no guarantee that pbasename will be within a
   subversion working copy.
   2) If targets consists of a single file entry, then pbasename will be
   set to it, even though it is not a directory. */
svn_error_t *
svn_path_common_base(svn_string_t **pbasename,
                     const apr_array_header_t *targets,
                     apr_pool_t *pool);

for the entries in svn_path.h. What do you think? Anyone else?

One more thought, since I am going to be talking to the system to
convert to absolute paths anyways, I could at the same time ensure that
PBASENAME is in fact a directory, and remove NOTE 2 from both functions.

On Thu, Feb 08, 2001 at 01:18:37PM -0600, Karl Fogel wrote:
> What he said.
>
> Greg Stein <gstein@lyra.org> writes:
> > 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/

-- 
>~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson
kevin@pilch-bisson.net
http://www.pilch-bisson.net
PGP Public Key At http://pgp.pilch-bisson.net