On Wed, 2011-06-15 at 12:42 -0400, Greg Stein wrote: > On Wed, Jun 15, 2011 at 12:32, Julian Foad <julian.f...@wandisco.com> wrote: > > On Fri, 2011-06-10 at 09:06 +0100, Julian Foad wrote: > >> I (Julian Foad) wrote: > >> > On Thu, 2011-06-09, I (Julian Foad) wrote: > >> > > svn_dirent_uri.h > >> > > svn_relpath_internal_style() > >> > > svn_relpath_local_style() > >> > > # These two are inappropriate: only dirents have a 'local' style. > >> > > >> > I removed the former and made the latter private in r1133964. > > > > Bert pointed out that there is a desire and precedent for the Windows > > client to display in-repository relpaths using the Windows "\" > > separator, so svn_relpath_local_style() may be wanted. It was currently > > URLs use forward slashes. Backslashes are not proper separators. The > above does not make sense. > > >... > > Many callers of svn_dirent_skip_ancestor(), on the other hand, expect > > the 'child' input to be returned if it is not in fact a child. I am > > splitting the function into two: > > > > svn_dirent_skip_ancestor() - return NULL if not a child > > [some other function name] - return 'child' input if not a child > > PLEASE not another function. There are already too many variants of > the same thing: > > svn_dirent_is_child() > svn_dirent_skip_ancestor() > svn_dirent_is_ancestor() > > These are all trivial variants which leads the reader to say "umm... > which one is this?" and the developer to say "umm... which do I use > here?". Please do not add a fourth... it will only exacerbate an > already awful situation. > > If anything, please reduce the number to ONE.
Absolutely. I intend to reduce the number. But the first and more important thing is to reduce the number of *semantic* variations, rather than strictly the number of exposed API symbols. After this round of changes, the semantics of *_skip_ancestor will match *_is_ancestor exactly, so the latter becomes strictly redundant and I'll certainly consider removing it, and even if I don't then its implementation will be a simple wrapper rather than the current long-winded and separate code. As a second change, I think *_is_child() should go, because that's a semantic variant that can easily be derived from _skip_ancestor. svn_dirent_is_child() was already public in 1.6, though, so that one must stay, but we could deprecate that and prefer using dirent_skip_ancestor(). I guess, to speed this up in readiness for 1.7 branching, the sensible thing would be to make the _is_child functions private at first. Adapting their callers to use _skip_ancestor instead will be a bigger change so that can come later. So that's what I intend to do. And this step of splitting _dirent_skip_ancestor into two variants with the two different semantic meanings which are currently conflated - does that now make sense to you as it does to me? - Julian
