Re: cvs2git: One possible bug, and two other issues

Hello,

Thanks a lot for your email and the patches.  Now that Xmas chaos is
getting over I finally had the chance to look at them...

On 01/03/2011 03:48 PM, Anders Pilegaard wrote:
> When that is said, it did take me some time to figure out why keywords
> were suddenly not expanded any more in rev 5311.  I finally figured
> out that I had to change the options file to add
> KeywordHandlingPropertySetter('untouched') instead of
> KeywordHandlingPropertySetter('collapsed') to
> ctx.file_property_setters.  I can see that there is a FIXME to improve
> the documentation though ... :-)

I think you want 'expanded' rather than 'untouched'.  'untouched' passes
through the keywords literally as they are expanded in the RCS file,
which is how they were expanded when the revision was checked *in*,
which is typically how they were expanded by CVS when the *previous*
version was checked out for editing.  The net result is that the
keywords expansions will all seem to be off by one revision if you use
'untouched'.  'expanded', on the other hand, causes the keywords to be
re-expanded as they should be for the revision being processed.

> Of these three remaining issues one looks like a regular bug, one is a
> change which I believe would be of general interest - and the last may
> only be interesting to a smaller audience.  I've got patches to all
> three.

Excellent.

> Issue 1 - Branches getting the wrong parent(s)
> ----------------------------------------------
> [...]
> In 2.3.0 I tracked this down to the code in cvs_file_items.py that
> grafts branches onto their preferred parents.  Branch b3 turns out to
> have b1 as preferred parent.  This works out ok for file1, but for
> file2 b1 gets grafted onto b2 *before* b3 is handled.  This causes b1
> to leave the set of possible parents, so b3 now has a preferred parent
> which isn't possible any more.  Thus it chooses another, and we get
> the merge commit.
> 
> One way to solve this could be to make a more extensive search for
> possible parents.  For my patch I chose a simpler way however - store
> the original set of branch_ids in the source commit in a new attribute
> branch_ids_orig - and iterate over that to find a parent.  It works
> well for the tests I've done so far.

This looks like the correct analysis of the problem.

At first I thought that the same problem could occur if b3 were a tag.
But (if I understand correctly) the problem does not arise for tags
because CVSFileItems.adjust_parents() processes all tags on an LOD
before any branches on that LOD are grafted.

In trunk r5313 I committed a test case based on your example repository.

AFAICT your fix will work.  But there might be a simpler way to fix the
problem.

The following code in CVSFileItems._adjust_branch_parents() is there to
prevent "circular grafts", where branch1 is grafted onto branch2 and
branch2 is grafted onto branch1:

      elif possible_parent.symbol == cvs_branch.symbol:
        # Only branches that precede the branch to be adjusted are
        # considered possible parents.  Leave parentage unchanged:
        return

Because of that code, there is never an attempt to graft a branch that
is earlier in the list onto one that is later in the list.  Therefore,
grafting a branch that is later in the list cannot affect how branches
that are earlier in the list are grafted.  Therefore, if we process
branches in *reverse* order, there should be no need for special code.
See move-parents.patch for an example patch.  Does this work for you?

I will reply to your other patches in separate emails.

Michael

-- 
Michael Haggerty
[email protected]
http://softwareswirl.blogspot.com/

------------------------------------------------------
http://cvs2svn.tigris.org/ds/viewMessage.do?dsForumId=1667&dsMessageId=2696752

To unsubscribe from this discussion, e-mail: [[email protected]].

diff --git a/cvs2svn_lib/cvs_file_items.py b/cvs2svn_lib/cvs_file_items.py
index 5e24109..d339047 100644
--- a/cvs2svn_lib/cvs_file_items.py
+++ b/cvs2svn_lib/cvs_file_items.py
@@ -1037,7 +1037,11 @@ class CVSFileItems(object):
       for cvs_tag in lod_items.cvs_tags:
         self._adjust_tag_parent(cvs_tag)
 
-      for cvs_branch in lod_items.cvs_branches:
+      # It is important to process branches in reverse order, so that
+      # a branch graft target (which necessarily occurs earlier in the
+      # list than the branch itself) is not moved before the branch
+      # itself.
+      for cvs_branch in reversed(lod_items.cvs_branches):
         self._adjust_branch_parents(cvs_branch)
 
   def _get_revision_source(self, cvs_symbol):
diff --git a/run-tests.py b/run-tests.py
index b1752a4..e2fad54 100755
--- a/run-tests.py
+++ b/run-tests.py
@@ -4131,7 +4131,7 @@ test_list = [
     add_on_branch2,
     XFail(branch_from_vendor_branch),
     strange_default_branch,
-    XFail(move_parent),
+    move_parent,
     ]
 
 if __name__ == '__main__':