Kamesh Jayachandran wrote:
> Julian Foad wrote:
> > Kamesh Jayachandran wrote:
> >> Justin Erenkrantz wrote:
> >> C. Michael Pilato <cmpilato_at_collab.net> wrote:
> >>>> r34562 (which is proposed for backport to 1.5.x). It adds code that looks
> >>>> like so:
> >>>>
> >>>> /* abs_added_path had better be a child of abs_target_path
> >>>> or something is *really* wrong. */
> >>>> SVN_ERR_ASSERT(svn_path_is_child(abs_target_path,
> >>>> abs_added_path,
> >>>> iterpool));
> >>>> common_ancestor_path =
> >>>> svn_path_get_longest_ancestor(abs_added_path,
> >>>> abs_target_path,
> >>>> iterpool);
> >>>>
> >>>> I'm a little confused by these two statements, though. If ABS_ADDED_PATH is
> >>>> a child of ABS_TARGET_PATH, then isn't the longest ancestor common to the
> >>>> two of them necessarily ABS_TARGET_PATH? Could this be written as:
> >>>>
> >>>> common_ancestor_path =
> >>>> svn_path_get_longest_ancestor(abs_added_path,
> >>>> abs_target_path,
> >>>> iterpool);
> >>>> SVN_ERR_ASSERT(*common_ancestor_path);
> >>>>
> >>>> ?
> >>> Yah, this snippet struck me as being redundnant too, but I don't
> >>> *think* it should harm anything.
> >>>
> >>> +1 if you want to change this and merge it back for 1.5.x. -- justin
> >> Fixed in r34756
> >>
> >> With regards
> >> Kamesh Jayachandran
> >
> >
> > Huh? You are now asserting only that there is some common ancestor.
> > Previously you were asserting that ABS_ADDED_PATH was the common
> > ancestor. That's very different.
>
> My head spins a bit when I think about it many times :),
>
> Hope you meant,
> "Previously you were asserting that *ABS_TARGET_PATH* was the common
> ancestor. That's very different."
Oops, yes, that's what I meant. Sorry. I'll say it a different way as
well:
You are now asserting only that ABS_ADDED_PATH and ABS_TARGET_PATH have
some common ancestor. Previously you were asserting that ABS_ADDED_PATH
was a child of ABS_TARGET_PATH. Those are different assertions.
> > Unless I'm still asleep, this block of code should say:
> >
> > /* abs_added_path had better be a child of abs_target_path
> > or something is *really* wrong. */
> > SVN_ERR_ASSERT(svn_path_is_child(abs_target_path,
> > abs_added_path,
> > iterpool));
> > common_ancestor_path = abs_target_path;
> >
> > - Julian
> >
>
> I think r34756 is another way of doing the same thing(check of paths).
>
> Let me put it with a data I have in my mind, if you disagree please let
> me know with data so that I can understand easily.
>
> let us say
> abs_target_path='/a'
> abs_added_path='/b/c'
>
> common_anc=svn_path_get_longest_ancestor(abs_added_path,
> abs_target_path,iterpool) returns "" so
> SVN_ERR_ASSERT(*common_anc) fails in assertion.
>
> svn_path_is_child(abs_target_path,abs_added_path,iterpool) returns NULL
>
> I *think* both first SVN_ERR_ASSERT() and
> SVN_ERR_ASSERT(svn_path_is_child(...)) yields same results(assertion
> failure).
Sure, yes, both of those cases fail. But if the two paths have a common
ancestor but are NOT parent and child:
abs_target_path='/a/z'
abs_added_path='/a/b/c'
common_anc=svn_path_get_longest_ancestor(abs_added_path,
abs_target_path,iterpool) returns "/a" so
SVN_ERR_ASSERT(*common_anc) passes, which is wrong.
The original check:
SVN_ERR_ASSERT(svn_path_is_child(abs_target_path,
abs_added_path,
iterpool));
would have failed, because '/a/b/c' is not a child of '/a/z'.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=985662
Received on 2008-12-17 13:25:42 CET