sOn Wed, Dec 3, 2008 at 7:00 PM, Mark Eichin <eichin_at_gmail.com> wrote:
> libsvn_client/merge.c, do_directory_merge, in the "if
> (notify_b->added_paths)" block that handles propagating inheritable
> mergeinfo down to newly added file, you have this block of code:
>
> const char *common_ancestor_path =
> svn_path_get_longest_ancestor(added_path,
> target_merge_path->path,
> iterpool);
> const char *relative_added_path =
> added_path + strlen(common_ancestor_path) + 1;
> SVN_ERR(svn_wc__entry_versioned(&entry, added_path,
> adm_access, FALSE,
> iterpool));
>
> (lines 6337-6345, on trunk, as of r34547, but the code appears
> unchanged from 1.5 which I'm actually working with.)
>
> The problem I'm seeing is that
> added_path == "TestFoo.py"
> target_merge_path->path == ""
> thus
> common_ancestor_path == ""
> thus
> strlen(common_ancestor_path) == 0
> thus
> relative_added_path == "estFoo.py"
>
> The setup for this is a merge -c 88093 of a single revision which
> creates TestFoo.py on trunk, into a directory which is a checkout of
> an earlier branch; the directory has both inheritable and
> non-inheritable mergeinfo on it already. relative_added_path gets
> joined with mergeinfo_path and applied to the newly created
> TestFoo.py, producing an entirely garbage mergeinfo record of
> "/trunk/tests/estFoo.py:88093"
Hi Mark,
I expanded an existing merge test in r34560 to cover this bug.
> My first thought is that it simply needs to check if
> common_ancestor_path is empty and not add the +1 in that case. That
> would at least make the path not meaningless. Some variant on this:
>
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 34552)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -6339,7 +6339,7 @@
> target_merge_path->path,
> iterpool);
> const char *relative_added_path =
> - added_path + strlen(common_ancestor_path) + 1;
> + added_path + strlen(common_ancestor_path) +
> (common_ancestor_path[0] ? 1 : 0);
> SVN_ERR(svn_wc__entry_versioned(&entry, added_path,
> adm_access, FALSE,
> iterpool));
>
> would cover that (or whatever your house style suggests for testing
> that common_ancestor_path is non-empty.)
What you suggest is completely adequate for this particular bug. But
while looking at this I started to wonder if there was any way we
could end up with target_merge_path->path being absolute and
added_path relative, or vice-versa. It doesn't appear that this is
possbile, but there's no reason not to make this completely
bomb-proof, which I did in r34562.
> However, I'm not sure that the mergeinfo applied is right either -
> since with the correct path, since it's using the mergeinfo from the
> current directory *after* applying this merge, it just describes the
> "natural history" of the change anyway, right? (Or does that count as
> "redundant but not wrong" and will get optimized out in the future?)
Agreed, in all cases there will be *some* natural history made
explicit in the added subtree's mergeinfo. This is a known problem,
see http://subversion.tigris.org/issues/show_bug.cgi?id=3157#desc8.
"Problem" may be too strong a term, since natural history made
explicit is, AFAICT, "redundant but not wrong".
I emphasized "some" above because it might also be the case where we
merge -r4:10 from 'trunk' to 'branch' and r5 adds the subtree
'branch/new_child'. Let's assume we have a situation similar to the
bug you found and we set explicit mergeinfo on 'branch/new_child'
describing the merge, that is, '/trunk/new_child:5-10'. The
'/trunk/new_child:5' portion of this mergeinfo does describe natural
history, but '/trunk/new_child:6-10' is legitimate. We could clean
this up, but imagine that instead of -r5:10 we had a much larger range
and instead of one added subtree we had scores. There would be some
not insignificant communication with the repository necessary to
figure out what mergeinfo is natural history and what is not...so my
gut says that until this proves to be a more serious problem we are
safe leaving it as is (I'm open to arguments the other way of course).
> (Should have a reduced demonstration script soon, but since the code
> is wrong by inspection, I figured I'd give people (especially Paul
> Burba :-) a chance to look at the problem...)
Thanks again for the detailed description and reproduction, you're
becoming exhibit 'A' on how to submit a bug report!
I'll nominate this for backport to 1.5.x too.
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=979779
Received on 2008-12-04 20:14:37 CET