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? >
