One thought:
The path library is all about a certain kind of string manipulation.
Until now, it has not ever talked to the system.
If absolute<->relative path conversion needs to happen, isolate it to
one function, and make sure other functions take paths that have
already been canonicalized (that shouldn't be taken to imply that
svn_path_canonicalize() is necessarily the place to do this, I'm using
"canonicalize" in a loose sense -- just meaning that absolute vs
relative has been resolved, one way or the other, for a given set of
paths).
-K
Kevin Pilch-Bisson <kevin@pilch-bisson.net> writes:
> --DocE+STaALJfprDB
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
>
> 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=20
> 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=20
>
> /* Return the common base path of TARGETS as an absolute path in
> PBASENAME. Do any necessary allocation in POOL.
> =20
> 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.
> >=20
> > 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 *=20
> > > > svn_?_condense_targets(svn_string_t ***pcondensed_targets,
> > > > int *pnum_condensed_targets,
> > > > const svn_string_t **targets,
> > > > int num_targets,
> > > > apr_pool_t *pool);
> > > >=20
> > > > svn_error_t *
> > > > svn_?_get_basepath(svn_string_t **pbasepath,
> > > > const svn_string_t **targets,
> > > > int num_targets,
> > > > apr_pool_t *pool);
> > > >=20
> > > > Now for questions and notes:
> > > >=20
> > > > 1) What library should they be part of?
> > >=20
> > > subr. I see them as part of path processing.
> > >=20
> > > > 2) They currently return svn_error_t *, but nothing they call generat=
> es
> > > > an error, so they could just as well return results directly.
> > >=20
> > > Yup. Return an apr_array_header_t*, containing the svn_string_t* ptrs.
> > >=20
> > > > 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.
> > >=20
> > > Optional return value from condense_targets. If pbasepath !=3D NULL, th=
> en you
> > > get the value returned to you.
> > >=20
> > > > 4) I have been creating a subpool of pool for temporary allocation, a=
> nd
> > > > apr_pool_destroy()'ing it at the end of the functions, is this
> > > > appropriate?
> > >=20
> > > I'd say no.
> > >=20
> > > SVN already spends too much time trying to be "nifty" with its allocati=
> ons.
> > > As a result, we end up with things in the wrong pools, and then we have
> > > lifetime mismatches.
> > >=20
> > > Put your allocations and temporary allocations directly into the provid=
> ed
> > > 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 c=
> aller
> > > can create subpools. Or we can revise the function.
> > >=20
> > > Trying to optimize memory usage now (read: prematurely) can only lead to
> > > problems.
> > >=20
> > > > 5) I could also modify the targets array and num targets directly. T=
> hus
> > >=20
> > > I'm not sure about this one. I'd initially say "no", but defer to Ben on
> > > whether the original set is needed.
> > >=20
> > > >...
> > > > 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=20
> > > > file as a target.
> > > >=20
> > > > What do you all think?
> > >=20
> > > (1) must be fixed. Consider:
> > >
> > > [gstein@kurgan mod_dav_svn]$ svn update . ~/src/svn/notes
> > >=20
> > >=20
> > > (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).
> > >=20
> > > (3) shouldn't be a problem. That is easily discoverable.
> > >=20
> > > These last two points should be noted in the doc, so that callers can
> > > understand the issues.
> > >=20
> > > Cheers,
> > > -g
> > >=20
> > > --=20
> > > Greg Stein, http://www.lyra.org/
>
> --=20
> >~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Kevin Pilch-Bisson
> kevin@pilch-bisson.net
> http://www.pilch-bisson.net
> PGP Public Key At http://pgp.pilch-bisson.net
>
> --DocE+STaALJfprDB
> Content-Type: application/pgp-signature
> Content-Disposition: inline
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.0.4 (GNU/Linux)
> Comment: For info see http://www.gnupg.org
>
> iD8DBQE6hB2lgJlk/lQdbnARAnGlAJ9XXYABq8DNrZYTB5uPX3+Z2pg+0QCghyrC
> 3FIavaVIPwtGCA/klxW1CKw=
> =ayds
> -----END PGP SIGNATURE-----
>
> --DocE+STaALJfprDB--
Received on Sat Oct 21 14:36:21 2006