Kamesh Jayachandran wrote:
> Thanks Julian fixed in r34760(revert of r34756) and r34762.
[[[
$ svn diff -c34762
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 34761)
+++ subversion/libsvn_client/merge.c (revision 34762)
@@ -6374,10 +6374,7 @@
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);
+ common_ancestor_path = abs_target_path;
/* Need to +1 to avoid a leading '/'. */
rel_added_path =
abs_added_path + strlen(common_ancestor_path) + 1;
]]]
That looks better, but isn't the next line simply setting rel_added_path
to the result of the svn_path_is_child() call above?
- Julian
> Julian Foad wrote:
> > 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
> >
> >
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.2.6 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFJSQAC3WHvyO0YTCwRAqx2AJ4og17mK3rbAa7AHKlrr5o7W3/gIACgpvtB
> XMpS+Iu+e2IxLkXwogZq9f4=
> =zXbC
> -----END PGP SIGNATURE-----
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=985749
Received on 2008-12-17 15:56:48 CET