You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2010/03/31 00:46:51 UTC
redundant path functions
The following functions seem very redundant. Because each is slightly
different, I always have to compare/contrast them to isolate their
differences. It would be much better if we could pick JUST ONE, and
run with that:
svn_*_is_child()
svn_*_is_ancestor()
svn_*_skip_ancestor()
I do realize that we published the svn_dirent_* functions in 1.6, but
we don't have to carry their analogues to the new functions. (and
possibly even deprecate the 1.6 funcs)
It seems that the is_child variant is the only function needed. The
is_ancestor() is merely is_child() != NULL, and the skip_ancestor is
simply pool=NULL.
Am I missing something, and/or can/should we remove this redundancy?
Cheers,
-g
Re: redundant path functions
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-03-31 at 08:15 -0400, C. Michael Pilato wrote:
> Stefan Sperling wrote:
> > I too remember being confused as to which interface I should be using.
> > And I think having is_ancestor not be the reverse of is_child is a bad idea.
> > We'll likely find mis-use of either interface in the code base, where
> > the author didn't realise the subtle semantics of the interface.
> >
> > +1 on settling on one variant (I don't really care which), deprecating
> > the others, and updating callers.
>
> We had one variant before -- svn_path_is_child(). But I think people got
> tired of having to implement _is_ancestor() as a pool-requiring wrapper
> around that function and a new public API was born.
>
> The is_child() variant is clearly the most powerful one, having not just the
> ability to answer the question but also to return the path relative to the
> ancestor (which is used in various places in our codebase). But perhaps
> there's a unified API that do this for us:
>
> svn_*_check_ancestry(svn_boolean_t *is_ancestor,
> const char **child_relpath, /* may be NULL */
> const char *path1,
> const char *path2,
> apr_pool_t *result_pool) /* may be NULL */
Ew, ugly! There's no problem having multiple functions in the API (one
to ask the question, one to return the relpath) if their semantics are
identical. It's the differing semantics and poor naming (is_xxx for a
non-bool) that was the main problem.
Plus: let's call the potential-parent and potential-child args 'parent'
and 'child' rather than '1' and '2', as that's impossible to infer
otherwise.
- Julian
> The function always answers the is-ancestor question, and -- if you provide
> a non-NULL child_relpath and result_pool, it will fill in the child path
> relative to the ancestor for you.
>
> Is that sane?
>
Re: redundant path functions
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-03-31 at 08:15 -0400, C. Michael Pilato wrote:
> Stefan Sperling wrote:
> > I too remember being confused as to which interface I should be using.
> > And I think having is_ancestor not be the reverse of is_child is a bad idea.
> > We'll likely find mis-use of either interface in the code base, where
> > the author didn't realise the subtle semantics of the interface.
> >
> > +1 on settling on one variant (I don't really care which), deprecating
> > the others, and updating callers.
>
> We had one variant before -- svn_path_is_child(). But I think people got
> tired of having to implement _is_ancestor() as a pool-requiring wrapper
> around that function and a new public API was born.
>
> The is_child() variant is clearly the most powerful one, having not just the
> ability to answer the question but also to return the path relative to the
> ancestor (which is used in various places in our codebase). But perhaps
> there's a unified API that do this for us:
>
> svn_*_check_ancestry(svn_boolean_t *is_ancestor,
> const char **child_relpath, /* may be NULL */
> const char *path1,
> const char *path2,
> apr_pool_t *result_pool) /* may be NULL */
Ew, ugly! There's no problem having multiple functions in the API (one
to ask the question, one to return the relpath) if their semantics are
identical. It's the differing semantics and poor naming (is_xxx for a
non-bool) that was the main problem.
Plus: let's call the potential-parent and potential-child args 'parent'
and 'child' rather than '1' and '2', as that's impossible to infer
otherwise.
- Julian
> The function always answers the is-ancestor question, and -- if you provide
> a non-NULL child_relpath and result_pool, it will fill in the child path
> relative to the ancestor for you.
>
> Is that sane?
>
Re: redundant path functions
Posted by "C. Michael Pilato" <cm...@collab.net>.
Stefan Sperling wrote:
> I too remember being confused as to which interface I should be using.
> And I think having is_ancestor not be the reverse of is_child is a bad idea.
> We'll likely find mis-use of either interface in the code base, where
> the author didn't realise the subtle semantics of the interface.
>
> +1 on settling on one variant (I don't really care which), deprecating
> the others, and updating callers.
We had one variant before -- svn_path_is_child(). But I think people got
tired of having to implement _is_ancestor() as a pool-requiring wrapper
around that function and a new public API was born.
The is_child() variant is clearly the most powerful one, having not just the
ability to answer the question but also to return the path relative to the
ancestor (which is used in various places in our codebase). But perhaps
there's a unified API that do this for us:
svn_*_check_ancestry(svn_boolean_t *is_ancestor,
const char **child_relpath, /* may be NULL */
const char *path1,
const char *path2,
apr_pool_t *result_pool) /* may be NULL */
The function always answers the is-ancestor question, and -- if you provide
a non-NULL child_relpath and result_pool, it will fill in the child path
relative to the ancestor for you.
Is that sane?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: redundant path functions
Posted by "C. Michael Pilato" <cm...@collab.net>.
Stefan Sperling wrote:
> I too remember being confused as to which interface I should be using.
> And I think having is_ancestor not be the reverse of is_child is a bad idea.
> We'll likely find mis-use of either interface in the code base, where
> the author didn't realise the subtle semantics of the interface.
>
> +1 on settling on one variant (I don't really care which), deprecating
> the others, and updating callers.
We had one variant before -- svn_path_is_child(). But I think people got
tired of having to implement _is_ancestor() as a pool-requiring wrapper
around that function and a new public API was born.
The is_child() variant is clearly the most powerful one, having not just the
ability to answer the question but also to return the path relative to the
ancestor (which is used in various places in our codebase). But perhaps
there's a unified API that do this for us:
svn_*_check_ancestry(svn_boolean_t *is_ancestor,
const char **child_relpath, /* may be NULL */
const char *path1,
const char *path2,
apr_pool_t *result_pool) /* may be NULL */
The function always answers the is-ancestor question, and -- if you provide
a non-NULL child_relpath and result_pool, it will fill in the child path
relative to the ancestor for you.
Is that sane?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: redundant path functions
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 31, 2010 at 11:34:35AM +0200, Bert Huijben wrote:
>
>
> > -----Original Message-----
> > From: Greg Stein [mailto:gstein@gmail.com]
> > Sent: woensdag 31 maart 2010 2:47
> > To: dev@subversion.apache.org
> > Subject: redundant path functions
> >
> > The following functions seem very redundant. Because each is slightly
> > different, I always have to compare/contrast them to isolate their
> > differences. It would be much better if we could pick JUST ONE, and
> > run with that:
> >
> > svn_*_is_child()
> > svn_*_is_ancestor()
> > svn_*_skip_ancestor()
> >
> > I do realize that we published the svn_dirent_* functions in 1.6, but
> > we don't have to carry their analogues to the new functions. (and
> > possibly even deprecate the 1.6 funcs)
> >
> > It seems that the is_child variant is the only function needed. The
> > is_ancestor() is merely is_child() != NULL, and the skip_ancestor is
> > simply pool=NULL.
> >
> > Am I missing something, and/or can/should we remove this redundancy?
>
> There is one difference between is_child and the ancestors functions: How
> they handle the case where the path is the ancestor itself.
>
> The is_child function returns NULL, while is_ancestor returns true.
> Skip ancestor removes the ancestor (prefix) and returns "" for the ancestor
> itself and leaves paths that don't have the ancestor as-is.
I too remember being confused as to which interface I should be using.
And I think having is_ancestor not be the reverse of is_child is a bad idea.
We'll likely find mis-use of either interface in the code base, where
the author didn't realise the subtle semantics of the interface.
+1 on settling on one variant (I don't really care which), deprecating
the others, and updating callers.
Stefan
RE: redundant path functions
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com]
> Sent: woensdag 31 maart 2010 2:47
> To: dev@subversion.apache.org
> Subject: redundant path functions
>
> The following functions seem very redundant. Because each is slightly
> different, I always have to compare/contrast them to isolate their
> differences. It would be much better if we could pick JUST ONE, and
> run with that:
>
> svn_*_is_child()
> svn_*_is_ancestor()
> svn_*_skip_ancestor()
>
> I do realize that we published the svn_dirent_* functions in 1.6, but
> we don't have to carry their analogues to the new functions. (and
> possibly even deprecate the 1.6 funcs)
>
> It seems that the is_child variant is the only function needed. The
> is_ancestor() is merely is_child() != NULL, and the skip_ancestor is
> simply pool=NULL.
>
> Am I missing something, and/or can/should we remove this redundancy?
There is one difference between is_child and the ancestors functions: How
they handle the case where the path is the ancestor itself.
The is_child function returns NULL, while is_ancestor returns true.
Skip ancestor removes the ancestor (prefix) and returns "" for the ancestor
itself and leaves paths that don't have the ancestor as-is.
Bert