You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kevin Pilch-Bisson <ke...@pilch-bisson.net> on 2001/02/08 16:37:48 UTC
condense_targets and basepath functions
Hi All,
I have completed implementations of the two functions for condensing a
list of targets, and finding the basepath of the targets, but I have a
number of questions about exact semantics that I thought I would ask
you all. If you've no time to think about for now, fine, just don't
respond until you do. If you do, great.
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?
2) They currently return svn_error_t *, but nothing they call generates
an error, so they could just as well return results directly.
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.
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?
5) I could also modify the targets array and num targets directly. Thus
combining points 2, 3, and 5, I could have a single prototype of the
form:
svn_string_t *
svn_?_condense_and_get_basepath(svn_string_t **targets,
int *num_targets,
apr_pool_t * pool);
with the return value being the basepath.
which I think is a better interface.
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?
--
>~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson
kevin@pilch-bisson.net
http://www.pilch-bisson.net
PGP Public Key At http://pgp.pilch-bisson.net
Re: condense_targets and basepath functions
Posted by Ben Collins-Sussman <su...@newton.ch.collab.net>.
Thanks for doing this! I'll process this message later today when my
hands are free...
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> --2fHTh5uZTiUOsy+g
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
>
> Hi All,
>
> I have completed implementations of the two functions for condensing a
> list of targets, and finding the basepath of the targets, but I have a
> number of questions about exact semantics that I thought I would ask
> you all. If you've no time to think about for now, fine, just don't
> respond until you do. If you do, great.
>
> 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);
>
> 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?
> 2) They currently return svn_error_t *, but nothing they call generates
> an error, so they could just as well return results directly.
> 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.
> 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?
> 5) I could also modify the targets array and num targets directly. Thus
> combining points 2, 3, and 5, I could have a single prototype of the
> form:
>
> svn_string_t *
> svn_?_condense_and_get_basepath(svn_string_t **targets,
> int *num_targets,=20
> apr_pool_t * pool);
>
> with the return value being the basepath.
>
> which I think is a better interface.
>
> 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.
>
> What do you all think?
>
> --=20
> >~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Kevin Pilch-Bisson
> kevin@pilch-bisson.net
> http://www.pilch-bisson.net
> PGP Public Key At http://pgp.pilch-bisson.net
>
> --2fHTh5uZTiUOsy+g
> 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
>
> iD8DBQE6gstcgJlk/lQdbnARAviZAKCLt0vu8env9ghDbdTMFwFh3RQmswCggqji
> P394IFjVRLslK23+OExFKQc=
> =g0qJ
> -----END PGP SIGNATURE-----
>
> --2fHTh5uZTiUOsy+g--
Re: condense_targets and basepath functions
Posted by cm...@collab.net.
Greg Stein <gs...@lyra.org> writes:
> > 1) What library should they be part of?
>
> subr. I see them as part of path processing.
+1, same reasoning.
Re: condense_targets and basepath functions
Posted by Karl Fogel <kf...@galois.collab.net>.
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 <ke...@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 <gs...@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--
Re: condense_targets and basepath functions
Posted by Greg Stein <gs...@lyra.org>.
On Fri, Feb 09, 2001 at 05:58:06PM -0600, Ben Collins-Sussman wrote:
> Greg Stein <gs...@lyra.org> writes:
>
> > When we go to actually pass the targets to (say) the RA do_update
> > function, we'll also need to convert them to "URL style".
>
> We do need someone to write a `style-converter' in the path library,
> so we can convert paths from one style to another...
Note that only relative paths can be converted.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: condense_targets and basepath functions
Posted by Ben Collins-Sussman <su...@newton.ch.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> When we go to actually pass the targets to (say) the RA do_update
> function, we'll also need to convert them to "URL style".
We do need someone to write a `style-converter' in the path library,
so we can convert paths from one style to another...
Re: condense_targets and basepath functions
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Mon, Feb 12, 2001 at 03:57:21PM -0600, Karl Fogel wrote:
> Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> > Well I posted something to the APR list, and got a reply that someone
> > else is already working on doing it for APR, so I guess I will just
> > leave it for now, and use the APR version once it is committed.
>
> If you want, follow up and see if that person has resources allocated
> to it. If not, you could be the resources. :-)
>
> (Or maybe he's doing it as we speak.)
>
> -K
I guess I should have cross posted so other could see the replies. Here
is what I have so far.
Date: Mon, 12 Feb 2001 10:49:37 -0800 (PST)
From: rbb@covalent.net
To: Kevin Pilch-Bisson <ke...@pilch-bisson.net>
cc: dev@apr.apache.org
Subject: Re: Feature request realpath(3) wrapper
Hmmmmm........ This could be very interesting. Let's throw this into the
STATUS file, and see who picks it up. I believe this will also be handled
by Will Rowe's canonical filename stuff, but I would need him to reply to
be absolutely sure.
Ryan
On Mon, 12 Feb 2001, Kevin Pilch-Bisson wrote:
> Hi All,
>
> In doing some work for Subversion, I came accross a need to convert
> relative pathnames to absolute pathnames. Now realpath works under
> Linux, but I am told it is broken under some versions of Solaris
> (although it works for 2.5), and doesn't exist under win32(although
> there is _fullpath). I have no idea about BeOS.
>
> Is this something that anyone on the APR team would like to implement in
> a nice portable fashion?
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
And William Rowe replied:
From: "William A. Rowe, Jr." <wr...@rowe-clan.net>
To: "Kevin Pilch-Bisson" <ke...@pilch-bisson.net>, <de...@apr.apache.org>
Subject: Re: Feature request realpath(3) wrapper
Date: Mon, 12 Feb 2001 14:44:17 -0600
In the works :-)
Finally, I also got this one:
Date: Mon, 12 Feb 2001 15:40:44 -0600
From: Sam TH <sa...@uchicago.edu>
To: dev@apr.apache.org
Subject: Re: Feature request realpath(3) wrapper
On Mon, Feb 12, 2001 at 01:38:10PM -0500, Kevin Pilch-Bisson wrote:
> Hi All,
>
> In doing some work for Subversion, I came accross a need to convert
> relative pathnames to absolute pathnames. Now realpath works under
> Linux, but I am told it is broken under some versions of Solaris
> (although it works for 2.5), and doesn't exist under win32(although
> there is _fullpath). I have no idea about BeOS.
Just for the sake of info, it doesn't appear the BeOS provides
anything of this sort at all.
sam th
sam@uchicago.edu
http://www.abisource.com/~sam/
GnuPG Key:
http://www.abisource.com/~sam/key
So for now it looks like it is in good hands, although I think I will
ask William if he needs a hand.
--
>~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson
kevin@pilch-bisson.net
http://www.pilch-bisson.net
PGP Public Key At http://pgp.pilch-bisson.net
Re: condense_targets and basepath functions
Posted by Karl Fogel <kf...@galois.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> We move it to APR after we have a working version. We already have several
> functions that would best be within APR, and are pending a move. This would
> simply be another.
+1
> We can't just say "that's APR's job" and sit back. The people that care
> about something are the people to write the code. That means we write the
> code and move it to the right place later.
Right on!
To expand on what Greg is saying, this has been our practice all
along. For example, the current apr_transfer_file_contents() function
in APR began life as apr_copy_file() in *Subversion*. We had a note
that when it was done and tested, it would get moved into APR. It got
done, and tested, and reviewed, and now it lives in APR.
The important thing is to get the code you need, writing it if
necessary. If you think it's something that belongs in APR, then
write it in the APR-style and assume it will live there when ready.
-K
Re: condense_targets and basepath functions
Posted by Karl Fogel <kf...@galois.collab.net>.
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> Well I posted something to the APR list, and got a reply that someone
> else is already working on doing it for APR, so I guess I will just
> leave it for now, and use the APR version once it is committed.
If you want, follow up and see if that person has resources allocated
to it. If not, you could be the resources. :-)
(Or maybe he's doing it as we speak.)
-K
Re: condense_targets and basepath functions
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Mon, Feb 12, 2001 at 03:45:03PM -0800, Greg Stein wrote:
> On Mon, Feb 12, 2001 at 09:16:29AM -0600, cmpilato@newton.ch.collab.net wrote:
> > Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> > > Alright, that sounds reasonable, but how are we handling portability
> > > stuff. realpath(3) doesn't exist on win32, but _fullpath does. Should
> > > I suggest this to the APR list, since portability stuff is their domain,
> > > or should I hack up the file with #ifdef WIN32. That way APR could in
> > > theory use the system version if it is not broken, or use their own if
> > > it is broken.
>
> Write the code however it is needed, including an #ifdef.
>
> > Nonono. Please don't #ifdef WIN32 if at all possible. I've spent too
> > much time already finding and fixing inconsistencies between the
> > various Win32 platforms in APR...we don't want to fight the same
> > battles on this side of the border. I think APR is indeed the place
> > for such a thing.
>
> We move it to APR after we have a working version. We already have several
> functions that would best be within APR, and are pending a move. This would
> simply be another.
>
> We can't just say "that's APR's job" and sit back. The people that care
> about something are the people to write the code. That means we write the
> code and move it to the right place later.
Well I posted something to the APR list, and got a reply that someone
else is already working on doing it for APR, so I guess I will just
leave it for now, and use the APR version once it is committed.
>
> Cheers,
> -g
>
> --
> Greg Stein, http://www.lyra.org/
--
>~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson, http://www.pilch-bisson.net
Re: condense_targets and basepath functions
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Feb 12, 2001 at 09:16:29AM -0600, cmpilato@newton.ch.collab.net wrote:
> Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> > Alright, that sounds reasonable, but how are we handling portability
> > stuff. realpath(3) doesn't exist on win32, but _fullpath does. Should
> > I suggest this to the APR list, since portability stuff is their domain,
> > or should I hack up the file with #ifdef WIN32. That way APR could in
> > theory use the system version if it is not broken, or use their own if
> > it is broken.
Write the code however it is needed, including an #ifdef.
> Nonono. Please don't #ifdef WIN32 if at all possible. I've spent too
> much time already finding and fixing inconsistencies between the
> various Win32 platforms in APR...we don't want to fight the same
> battles on this side of the border. I think APR is indeed the place
> for such a thing.
We move it to APR after we have a working version. We already have several
functions that would best be within APR, and are pending a move. This would
simply be another.
We can't just say "that's APR's job" and sit back. The people that care
about something are the people to write the code. That means we write the
code and move it to the right place later.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: condense_targets and basepath functions
Posted by cm...@newton.ch.collab.net.
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> Alright, that sounds reasonable, but how are we handling portability
> stuff. realpath(3) doesn't exist on win32, but _fullpath does. Should
> I suggest this to the APR list, since portability stuff is their domain,
> or should I hack up the file with #ifdef WIN32. That way APR could in
> theory use the system version if it is not broken, or use their own if
> it is broken.
Nonono. Please don't #ifdef WIN32 if at all possible. I've spent too
much time already finding and fixing inconsistencies between the
various Win32 platforms in APR...we don't want to fight the same
battles on this side of the border. I think APR is indeed the place
for such a thing.
Re: condense_targets and basepath functions
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Mon, Feb 12, 2001 at 01:48:40PM +0100, Phil Pennock wrote:
> On 2001-02-12 at 07:34 -0500, Kevin Pilch-Bisson gifted us with:
> > Offhand, does anyone know the best
> > way to convert relative paths to absolute. I looked at realpath(3), but
> > it doesn't seem very portable, and says Solaris may give relative paths
> > back. I also looked through APR, and the docs for apr_stat seem to
> > suggest that it converts paths, but the implementation does nothing of
> > the sort. I could obviously do it by walking through the string looking
> > for `.'s and `..'s but I would have thought there would be a standard
> > way of doing it.
> >
> > Any ideas.
>
> realpath(3) is the way to go; if the system one is broken, copy one from
> a 4.4BSD system (or write your own).
>
> realpath(3) _is_ the standard non-self-rolled way.
>
> A BSD system manual-page notes:
> ERRORS
> The function realpath() may fail and set the external variable errno for
> any of the errors specified for the library functions chdir(2), close(2),
> fchdir(2), lstat(2), open(2), readlink(2), and getcwd(3).
> which gives a few clues. ;^)
>
> Solaris is a known problem. It's why so many things like ftp daemons
> carry their own implementation of realpath with them. :^(
Alright, that sounds reasonable, but how are we handling portability
stuff. realpath(3) doesn't exist on win32, but _fullpath does. Should
I suggest this to the APR list, since portability stuff is their domain,
or should I hack up the file with #ifdef WIN32. That way APR could in
theory use the system version if it is not broken, or use their own if
it is broken.
> --
> A formal manipulator in mathematics often experiences the discomforting
> feeling that his pencil surpasses him in intelligence -- Howard W Eves
> ____________________________________________________________
--
>~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson
kevin@pilch-bisson.net
http://www.pilch-bisson.net
PGP Public Key At http://pgp.pilch-bisson.net
Re: condense_targets and basepath functions
Posted by Phil Pennock <Ph...@globnix.org>.
On 2001-02-12 at 07:34 -0500, Kevin Pilch-Bisson gifted us with:
> Offhand, does anyone know the best
> way to convert relative paths to absolute. I looked at realpath(3), but
> it doesn't seem very portable, and says Solaris may give relative paths
> back. I also looked through APR, and the docs for apr_stat seem to
> suggest that it converts paths, but the implementation does nothing of
> the sort. I could obviously do it by walking through the string looking
> for `.'s and `..'s but I would have thought there would be a standard
> way of doing it.
>
> Any ideas.
realpath(3) is the way to go; if the system one is broken, copy one from
a 4.4BSD system (or write your own).
realpath(3) _is_ the standard non-self-rolled way.
A BSD system manual-page notes:
ERRORS
The function realpath() may fail and set the external variable errno for
any of the errors specified for the library functions chdir(2), close(2),
fchdir(2), lstat(2), open(2), readlink(2), and getcwd(3).
which gives a few clues. ;^)
Solaris is a known problem. It's why so many things like ftp daemons
carry their own implementation of realpath with them. :^(
--
A formal manipulator in mathematics often experiences the discomforting
feeling that his pencil surpasses him in intelligence -- Howard W Eves
Re: condense_targets and basepath functions
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Fri, Feb 09, 2001 at 03:49:22PM -0800, Greg Stein wrote:
> On Fri, Feb 09, 2001 at 11:41:09AM -0500, Kevin Pilch-Bisson wrote:
> > 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:
>
> Careful. He is working on other code right now... Don't distract him :-)
>
> >...[ prototypes ]
>
> I'm thinking it might be nice to go ahead and collapse these into one
> function (condense_targets). Further, to make pbasename be required. Then,
> we could do:
>
> *) rename it pbasedir to make it obvious it is always a dir
> *) targets become *relative* to pbasedir
This sounds like a good idea to me. I was thinking that something like
this would have to be done anyway. Offhand, does anyone know the best
way to convert relative paths to absolute. I looked at realpath(3), but
it doesn't seem very portable, and says Solaris may give relative paths
back. I also looked through APR, and the docs for apr_stat seem to
suggest that it converts paths, but the implementation does nothing of
the sort. I could obviously do it by walking through the string looking
for `.'s and `..'s but I would have thought there would be a standard
way of doing it.
Any ideas.
>
> We could actually make the targets-return-value optional if we wanted.
>
Can do.
> Having the targets as relative values is a bit nicer. It means that we don't
> have to chop them up to remove the absolute portion. When we go to actually
> pass the targets to (say) the RA do_update function, we'll also need to
> convert them to "URL style". But I believe condense_targets should just use
> "local style"... hmm. Maybe a passed-in style?
Will do, I am going to follow local style for now, but it if a
conversion function is written, then this function could easily just
call it on all the paths as we go through them.
>
> [ the WC will pass them as local style, but I could see the function being
> used to condense URLs, too ... ]
>
> 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
Re: condense_targets and basepath functions
Posted by Greg Stein <gs...@lyra.org>.
On Fri, Feb 09, 2001 at 11:41:09AM -0500, Kevin Pilch-Bisson wrote:
> 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:
Careful. He is working on other code right now... Don't distract him :-)
>...[ prototypes ]
I'm thinking it might be nice to go ahead and collapse these into one
function (condense_targets). Further, to make pbasename be required. Then,
we could do:
*) rename it pbasedir to make it obvious it is always a dir
*) targets become *relative* to pbasedir
We could actually make the targets-return-value optional if we wanted.
Having the targets as relative values is a bit nicer. It means that we don't
have to chop them up to remove the absolute portion. When we go to actually
pass the targets to (say) the RA do_update function, we'll also need to
convert them to "URL style". But I believe condense_targets should just use
"local style"... hmm. Maybe a passed-in style?
[ the WC will pass them as local style, but I could see the function being
used to condense URLs, too ... ]
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: condense_targets and basepath functions
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
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 <gs...@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
Re: condense_targets and basepath functions
Posted by Karl Fogel <kf...@galois.collab.net>.
What he said.
Greg Stein <gs...@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/
Re: condense_targets and basepath functions
Posted by Karl Fogel <kf...@galois.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> > 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
One thing though:
If it handles both absolute and relative paths, it probably has to
return svn_error_t * after all. Because converting a relative to an
absolute involves talking to the system and could result in an error,
right?
-K
Re: condense_targets and basepath functions
Posted by Greg Stein <gs...@lyra.org>.
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/