[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

corrupted mergeinfo when do_directory_merge adds a file

From: Mark Eichin <eichin_at_gmail.com>
Date: Wed, 3 Dec 2008 19:00:20 -0500

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"

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.)

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

(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...)

-- 
_Mark_ <eichin_at_thok.org> <eichin_at_gmail.com>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=979211
Received on 2008-12-04 01:00:43 CET

This is an archived mail posted to the Subversion Dev mailing list.