Rev 4889: (jam) Teach bzr merge --weave/--lca how to create a .BASE file (bug in file:///home/pqm/archives/thelove/bzr/%2Btrunk/

Canonical.com Patch Queue Manager pqm at pqm.ubuntu.com
Thu Dec 10 23:57:41 GMT 2009


At file:///home/pqm/archives/thelove/bzr/%2Btrunk/

------------------------------------------------------------
revno: 4889 [merge]
revision-id: pqm at pqm.ubuntu.com-20091210235739-zwgaqsbb2mk57gyv
parent: pqm at pqm.ubuntu.com-20091210215626-qzr3coh4bgh71lb6
parent: john at arbash-meinel.com-20091210231135-1w7w19m19u3j386c
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Thu 2009-12-10 23:57:39 +0000
message:
  (jam) Teach bzr merge --weave/--lca how to create a .BASE file (bug
  	#40412)
modified:
  NEWS                           NEWS-20050323055033-4e00b5db738777ff
  bzrlib/merge.py                merge.py-20050513021216-953b65a438527106
  bzrlib/tests/blackbox/test_merge.py test_merge.py-20060323225809-9bc0459c19917f41
  bzrlib/tests/blackbox/test_remerge.py test_remerge.py-20060629162739-o9m3s6143v8jnr2y-1
  bzrlib/tests/test_merge.py     testmerge.py-20050905070950-c1b5aa49ff911024
  bzrlib/tests/test_merge_core.py test_merge_core.py-20050824132511-eb99b23a0eec641b
  bzrlib/versionedfile.py        versionedfile.py-20060222045106-5039c71ee3b65490
=== modified file 'NEWS'
--- a/NEWS	2009-12-10 01:30:32 +0000
+++ b/NEWS	2009-12-10 21:49:20 +0000
@@ -49,16 +49,21 @@
 * ``bzr log -n0 -rN`` should not return revisions beyond its merged revisions.
   (#325618, #484109, Marius Kruger)
 
+* ``bzr merge --weave`` and ``--lca`` will now create ``.BASE`` files for
+  files with conflicts (similar to ``--merge3``). The contents of the file
+  is a synthesis of all bases used for the merge.
+  (John Arbash Meinel, #40412)
+
 * ``bzr mv --quiet`` really is quiet now.  (Gordon Tyler, #271790)
 
 * ``bzr serve`` is more clear about the risk of supplying --allow-writes.
   (Robert Collins, #84659)
 
+* ``bzr serve --quiet`` really is quiet now.  (Gordon Tyler, #252834)
+
 * Fix bug with redirected URLs over authenticated HTTP.
   (Glen Mailer, Neil Martinsen-Burrell, Vincent Ladeuil, #395714)
 
-* ``bzr serve --quiet`` really is quiet now.  (Gordon Tyler, #252834)
-
 * Interactive merge doesn't leave branch locks behind.  (Aaron Bentley)
 
 * Lots of bugfixes for the test suite on Windows. We should once again

=== modified file 'bzrlib/merge.py'
--- a/bzrlib/merge.py	2009-12-03 02:24:54 +0000
+++ b/bzrlib/merge.py	2009-12-10 17:16:19 +0000
@@ -1407,70 +1407,62 @@
     supports_reverse_cherrypick = False
     history_based = True
 
-    def _merged_lines(self, file_id):
-        """Generate the merged lines.
-        There is no distinction between lines that are meant to contain <<<<<<<
-        and conflicts.
-        """
-        if self.cherrypick:
-            base = self.base_tree
-        else:
-            base = None
-        plan = self.this_tree.plan_file_merge(file_id, self.other_tree,
+    def _generate_merge_plan(self, file_id, base):
+        return self.this_tree.plan_file_merge(file_id, self.other_tree,
                                               base=base)
+
+    def _merged_lines(self, file_id):
+        """Generate the merged lines.
+        There is no distinction between lines that are meant to contain <<<<<<<
+        and conflicts.
+        """
+        if self.cherrypick:
+            base = self.base_tree
+        else:
+            base = None
+        plan = self._generate_merge_plan(file_id, base)
         if 'merge' in debug.debug_flags:
             plan = list(plan)
             trans_id = self.tt.trans_id_file_id(file_id)
             name = self.tt.final_name(trans_id) + '.plan'
-            contents = ('%10s|%s' % l for l in plan)
+            contents = ('%11s|%s' % l for l in plan)
             self.tt.new_file(name, self.tt.final_parent(trans_id), contents)
         textmerge = versionedfile.PlanWeaveMerge(plan, '<<<<<<< TREE\n',
                                                  '>>>>>>> MERGE-SOURCE\n')
-        return textmerge.merge_lines(self.reprocess)
+        lines, conflicts = textmerge.merge_lines(self.reprocess)
+        if conflicts:
+            base_lines = textmerge.base_from_plan()
+        else:
+            base_lines = None
+        return lines, base_lines
 
     def text_merge(self, file_id, trans_id):
         """Perform a (weave) text merge for a given file and file-id.
         If conflicts are encountered, .THIS and .OTHER files will be emitted,
         and a conflict will be noted.
         """
-        lines, conflicts = self._merged_lines(file_id)
+        lines, base_lines = self._merged_lines(file_id)
         lines = list(lines)
         # Note we're checking whether the OUTPUT is binary in this case,
         # because we don't want to get into weave merge guts.
         textfile.check_text_lines(lines)
         self.tt.create_file(lines, trans_id)
-        if conflicts:
+        if base_lines is not None:
+            # Conflict
             self._raw_conflicts.append(('text conflict', trans_id))
             name = self.tt.final_name(trans_id)
             parent_id = self.tt.final_parent(trans_id)
             file_group = self._dump_conflicts(name, parent_id, file_id,
-                                              no_base=True)
+                                              no_base=False,
+                                              base_lines=base_lines)
             file_group.append(trans_id)
 
 
 class LCAMerger(WeaveMerger):
 
-    def _merged_lines(self, file_id):
-        """Generate the merged lines.
-        There is no distinction between lines that are meant to contain <<<<<<<
-        and conflicts.
-        """
-        if self.cherrypick:
-            base = self.base_tree
-        else:
-            base = None
-        plan = self.this_tree.plan_file_lca_merge(file_id, self.other_tree,
+    def _generate_merge_plan(self, file_id, base):
+        return self.this_tree.plan_file_lca_merge(file_id, self.other_tree,
                                                   base=base)
-        if 'merge' in debug.debug_flags:
-            plan = list(plan)
-            trans_id = self.tt.trans_id_file_id(file_id)
-            name = self.tt.final_name(trans_id) + '.plan'
-            contents = ('%10s|%s' % l for l in plan)
-            self.tt.new_file(name, self.tt.final_parent(trans_id), contents)
-        textmerge = versionedfile.PlanWeaveMerge(plan, '<<<<<<< TREE\n',
-                                                 '>>>>>>> MERGE-SOURCE\n')
-        return textmerge.merge_lines(self.reprocess)
-
 
 class Diff3Merger(Merge3Merger):
     """Three-way merger using external diff3 for text merging"""

=== modified file 'bzrlib/tests/blackbox/test_merge.py'
--- a/bzrlib/tests/blackbox/test_merge.py	2009-12-07 21:46:28 +0000
+++ b/bzrlib/tests/blackbox/test_merge.py	2009-12-10 17:16:19 +0000
@@ -211,6 +211,22 @@
         self.failUnlessExists('sub/a.txt.OTHER')
         self.failUnlessExists('sub/a.txt.BASE')
 
+    def test_conflict_leaves_base_this_other_files(self):
+        tree, other = self.create_conflicting_branches()
+        self.run_bzr('merge ../other', working_dir='tree',
+                     retcode=1)
+        self.assertFileEqual('a\nb\nc\n', 'tree/fname.BASE')
+        self.assertFileEqual('a\nB\nD\n', 'tree/fname.OTHER')
+        self.assertFileEqual('a\nB\nC\n', 'tree/fname.THIS')
+
+    def test_weave_conflict_leaves_base_this_other_files(self):
+        tree, other = self.create_conflicting_branches()
+        self.run_bzr('merge ../other --weave', working_dir='tree',
+                     retcode=1)
+        self.assertFileEqual('a\nb\nc\n', 'tree/fname.BASE')
+        self.assertFileEqual('a\nB\nD\n', 'tree/fname.OTHER')
+        self.assertFileEqual('a\nB\nC\n', 'tree/fname.THIS')
+
     def test_merge_remember(self):
         """Merge changes from one branch to another, test submit location."""
         tree_a = self.make_branch_and_tree('branch_a')

=== modified file 'bzrlib/tests/blackbox/test_remerge.py'
--- a/bzrlib/tests/blackbox/test_remerge.py	2009-03-23 14:59:43 +0000
+++ b/bzrlib/tests/blackbox/test_remerge.py	2009-12-10 23:11:35 +0000
@@ -85,7 +85,7 @@
                            'remerge --merge-type weave', retcode=1)
 
         self.failUnlessExists('hello.OTHER')
-        self.failIfExists('hello.BASE')
+        self.failUnless('hello.BASE')
         self.assertFalse('|||||||' in conflict_text)
         self.assertFalse('hi world' in conflict_text)
 

=== modified file 'bzrlib/tests/test_merge.py'
--- a/bzrlib/tests/test_merge.py	2009-09-19 00:32:14 +0000
+++ b/bzrlib/tests/test_merge.py	2009-12-10 17:16:19 +0000
@@ -38,6 +38,7 @@
 from bzrlib.workingtree import WorkingTree
 from bzrlib.transform import TreeTransform
 
+
 class TestMerge(TestCaseWithTransport):
     """Test appending more than one revision"""
 
@@ -524,6 +525,12 @@
         self.add_uncommitted_version(('root', 'C:'), [('root', 'A')], 'fabg')
         return _PlanMerge('B:', 'C:', self.plan_merge_vf, ('root',))
 
+    def test_base_from_plan(self):
+        self.setup_plan_merge()
+        plan = self.plan_merge_vf.plan_merge('B', 'C')
+        pwm = versionedfile.PlanWeaveMerge(plan)
+        self.assertEqual(['a\n', 'b\n', 'c\n'], pwm.base_from_plan())
+
     def test_unique_lines(self):
         plan = self.setup_plan_merge()
         self.assertEqual(plan._unique_lines(
@@ -827,6 +834,111 @@
                           ('unchanged', 'f\n'),
                           ('unchanged', 'g\n')],
                          list(plan))
+        plan = self.plan_merge_vf.plan_lca_merge('F', 'G')
+        # This is one of the main differences between plan_merge and
+        # plan_lca_merge. plan_lca_merge generates a conflict for 'x => z',
+        # because 'x' was not present in one of the bases. However, in this
+        # case it is spurious because 'x' does not exist in the global base A.
+        self.assertEqual([
+                          ('unchanged', 'h\n'),
+                          ('unchanged', 'a\n'),
+                          ('conflicted-a', 'x\n'),
+                          ('new-b', 'z\n'),
+                          ('unchanged', 'c\n'),
+                          ('unchanged', 'd\n'),
+                          ('unchanged', 'y\n'),
+                          ('unchanged', 'f\n'),
+                          ('unchanged', 'g\n')],
+                         list(plan))
+
+    def test_criss_cross_flip_flop(self):
+        # This is specificly trying to trigger problems when using limited
+        # ancestry and weaves. The ancestry graph looks like:
+        #       XX      unused ancestor, should not show up in the weave
+        #       |
+        #       A       Unique LCA
+        #      / \  
+        #     B   C     B & C both introduce a new line
+        #     |\ /|  
+        #     | X |  
+        #     |/ \| 
+        #     D   E     B & C are both merged, so both are common ancestors
+        #               In the process of merging, both sides order the new
+        #               lines differently
+        #
+        self.add_rev('root', 'XX', [], 'qrs')
+        self.add_rev('root', 'A', ['XX'], 'abcdef')
+        self.add_rev('root', 'B', ['A'], 'abcdgef')
+        self.add_rev('root', 'C', ['A'], 'abcdhef')
+        self.add_rev('root', 'D', ['B', 'C'], 'abcdghef')
+        self.add_rev('root', 'E', ['C', 'B'], 'abcdhgef')
+        plan = list(self.plan_merge_vf.plan_merge('D', 'E'))
+        self.assertEqual([
+                          ('unchanged', 'a\n'),
+                          ('unchanged', 'b\n'),
+                          ('unchanged', 'c\n'),
+                          ('unchanged', 'd\n'),
+                          ('new-b', 'h\n'),
+                          ('unchanged', 'g\n'),
+                          ('killed-b', 'h\n'),
+                          ('unchanged', 'e\n'),
+                          ('unchanged', 'f\n'),
+                         ], plan)
+        pwm = versionedfile.PlanWeaveMerge(plan)
+        self.assertEqualDiff('\n'.join('abcdghef') + '\n',
+                             ''.join(pwm.base_from_plan()))
+        # Reversing the order reverses the merge plan, and final order of 'hg'
+        # => 'gh'
+        plan = list(self.plan_merge_vf.plan_merge('E', 'D'))
+        self.assertEqual([
+                          ('unchanged', 'a\n'),
+                          ('unchanged', 'b\n'),
+                          ('unchanged', 'c\n'),
+                          ('unchanged', 'd\n'),
+                          ('new-b', 'g\n'),
+                          ('unchanged', 'h\n'),
+                          ('killed-b', 'g\n'),
+                          ('unchanged', 'e\n'),
+                          ('unchanged', 'f\n'),
+                         ], plan)
+        pwm = versionedfile.PlanWeaveMerge(plan)
+        self.assertEqualDiff('\n'.join('abcdhgef') + '\n',
+                             ''.join(pwm.base_from_plan()))
+        # This is where lca differs, in that it (fairly correctly) determines
+        # that there is a conflict because both sides resolved the merge
+        # differently
+        plan = list(self.plan_merge_vf.plan_lca_merge('D', 'E'))
+        self.assertEqual([
+                          ('unchanged', 'a\n'),
+                          ('unchanged', 'b\n'),
+                          ('unchanged', 'c\n'),
+                          ('unchanged', 'd\n'),
+                          ('conflicted-b', 'h\n'),
+                          ('unchanged', 'g\n'),
+                          ('conflicted-a', 'h\n'),
+                          ('unchanged', 'e\n'),
+                          ('unchanged', 'f\n'),
+                         ], plan)
+        pwm = versionedfile.PlanWeaveMerge(plan)
+        self.assertEqualDiff('\n'.join('abcdgef') + '\n',
+                             ''.join(pwm.base_from_plan()))
+        # Reversing it changes what line is doubled, but still gives a
+        # double-conflict
+        plan = list(self.plan_merge_vf.plan_lca_merge('E', 'D'))
+        self.assertEqual([
+                          ('unchanged', 'a\n'),
+                          ('unchanged', 'b\n'),
+                          ('unchanged', 'c\n'),
+                          ('unchanged', 'd\n'),
+                          ('conflicted-b', 'g\n'),
+                          ('unchanged', 'h\n'),
+                          ('conflicted-a', 'g\n'),
+                          ('unchanged', 'e\n'),
+                          ('unchanged', 'f\n'),
+                         ], plan)
+        pwm = versionedfile.PlanWeaveMerge(plan)
+        self.assertEqualDiff('\n'.join('abcdhef') + '\n',
+                             ''.join(pwm.base_from_plan()))
 
     def assertRemoveExternalReferences(self, filtered_parent_map,
                                        child_map, tails, parent_map):

=== modified file 'bzrlib/tests/test_merge_core.py'
--- a/bzrlib/tests/test_merge_core.py	2009-08-20 04:09:58 +0000
+++ b/bzrlib/tests/test_merge_core.py	2009-12-08 21:04:07 +0000
@@ -434,6 +434,7 @@
         self.assertEqual('text2', builder.this.get_file('1').read())
         builder.cleanup()
 
+
 class FunctionalMergeTest(TestCaseWithTransport):
 
     def test_trivial_star_merge(self):
@@ -467,30 +468,61 @@
         self.assertEqual("Mary\n", open("original/file2", "rt").read())
 
     def test_conflicts(self):
-        os.mkdir('a')
         wta = self.make_branch_and_tree('a')
-        a = wta.branch
-        file('a/file', 'wb').write('contents\n')
+        self.build_tree_contents([('a/file', 'contents\n')])
         wta.add('file')
         wta.commit('base revision', allow_pointless=False)
-        d_b = a.bzrdir.clone('b')
-        b = d_b.open_branch()
-        file('a/file', 'wb').write('other contents\n')
+        d_b = wta.branch.bzrdir.clone('b')
+        self.build_tree_contents([('a/file', 'other contents\n')])
         wta.commit('other revision', allow_pointless=False)
-        file('b/file', 'wb').write('this contents contents\n')
+        self.build_tree_contents([('b/file', 'this contents contents\n')])
         wtb = d_b.open_workingtree()
         wtb.commit('this revision', allow_pointless=False)
         self.assertEqual(1, wtb.merge_from_branch(wta.branch))
-        self.assert_(os.path.lexists('b/file.THIS'))
-        self.assert_(os.path.lexists('b/file.BASE'))
-        self.assert_(os.path.lexists('b/file.OTHER'))
+        self.failUnlessExists('b/file.THIS')
+        self.failUnlessExists('b/file.BASE')
+        self.failUnlessExists('b/file.OTHER')
         wtb.revert()
         self.assertEqual(1, wtb.merge_from_branch(wta.branch,
                                                   merge_type=WeaveMerger))
-        self.assert_(os.path.lexists('b/file'))
-        self.assert_(os.path.lexists('b/file.THIS'))
-        self.assert_(not os.path.lexists('b/file.BASE'))
-        self.assert_(os.path.lexists('b/file.OTHER'))
+        self.failUnlessExists('b/file')
+        self.failUnlessExists('b/file.THIS')
+        self.failUnlessExists('b/file.BASE')
+        self.failUnlessExists('b/file.OTHER')
+
+    def test_weave_conflicts_not_in_base(self):
+        builder = self.make_branch_builder('source')
+        builder.start_series()
+        # See bug #494197
+        #  A        base revision (before criss-cross)
+        #  |\
+        #  B C      B does nothing, C adds 'foo'
+        #  |X|
+        #  D E      D and E modify foo in incompatible ways
+        #
+        # Merging will conflict, with C as a clean base text. However, the
+        # current code uses A as the global base and 'foo' doesn't exist there.
+        # It isn't trivial to create foo.BASE because it tries to look up
+        # attributes like 'executable' in A.
+        builder.build_snapshot('A-id', None, [
+            ('add', ('', 'TREE_ROOT', 'directory', None))])
+        builder.build_snapshot('B-id', ['A-id'], [])
+        builder.build_snapshot('C-id', ['A-id'], [
+            ('add', ('foo', 'foo-id', 'file', 'orig\ncontents\n'))])
+        builder.build_snapshot('D-id', ['B-id', 'C-id'], [
+            ('add', ('foo', 'foo-id', 'file', 'orig\ncontents\nand D\n'))])
+        builder.build_snapshot('E-id', ['C-id', 'B-id'], [
+            ('modify', ('foo-id', 'orig\ncontents\nand E\n'))])
+        builder.finish_series()
+        tree = builder.get_branch().create_checkout('tree', lightweight=True)
+        self.assertEqual(1, tree.merge_from_branch(tree.branch,
+                                                   to_revision='D-id',
+                                                   merge_type=WeaveMerger))
+        self.failUnlessExists('tree/foo.THIS')
+        self.failUnlessExists('tree/foo.OTHER')
+        self.expectFailure('fail to create .BASE in some criss-cross merges',
+            self.failUnlessExists, 'tree/foo.BASE')
+        self.failUnlessExists('tree/foo.BASE')
 
     def test_merge_unrelated(self):
         """Sucessfully merges unrelated branches with no common names"""

=== modified file 'bzrlib/versionedfile.py'
--- a/bzrlib/versionedfile.py	2009-10-19 15:06:58 +0000
+++ b/bzrlib/versionedfile.py	2009-12-10 17:16:19 +0000
@@ -1426,7 +1426,7 @@
     def __init__(self, plan, a_marker=TextMerge.A_MARKER,
                  b_marker=TextMerge.B_MARKER):
         TextMerge.__init__(self, a_marker, b_marker)
-        self.plan = plan
+        self.plan = list(plan)
 
     def _merge_struct(self):
         lines_a = []
@@ -1490,6 +1490,60 @@
         for struct in outstanding_struct():
             yield struct
 
+    def base_from_plan(self):
+        """Construct a BASE file from the plan text."""
+        base_lines = []
+        for state, line in self.plan:
+            if state in ('killed-a', 'killed-b', 'killed-both', 'unchanged'):
+                # If unchanged, then this line is straight from base. If a or b
+                # or both killed the line, then it *used* to be in base.
+                base_lines.append(line)
+            else:
+                if state not in ('killed-base', 'irrelevant',
+                                 'ghost-a', 'ghost-b',
+                                 'new-a', 'new-b',
+                                 'conflicted-a', 'conflicted-b'):
+                    # killed-base, irrelevant means it doesn't apply
+                    # ghost-a/ghost-b are harder to say for sure, but they
+                    # aren't in the 'inc_c' which means they aren't in the
+                    # shared base of a & b. So we don't include them.  And
+                    # obviously if the line is newly inserted, it isn't in base
+
+                    # If 'conflicted-a' or b, then it is new vs one base, but
+                    # old versus another base. However, if we make it present
+                    # in the base, it will be deleted from the target, and it
+                    # seems better to get a line doubled in the merge result,
+                    # rather than have it deleted entirely.
+                    # Example, each node is the 'text' at that point:
+                    #           MN
+                    #          /   \
+                    #        MaN   MbN
+                    #         |  X  |
+                    #        MabN MbaN
+                    #          \   /
+                    #           ???
+                    # There was a criss-cross conflict merge. Both sides
+                    # include the other, but put themselves first.
+                    # Weave marks this as a 'clean' merge, picking OTHER over
+                    # THIS. (Though the details depend on order inserted into
+                    # weave, etc.)
+                    # LCA generates a plan:
+                    # [('unchanged', M),
+                    #  ('conflicted-b', b),
+                    #  ('unchanged', a),
+                    #  ('conflicted-a', b),
+                    #  ('unchanged', N)]
+                    # If you mark 'conflicted-*' as part of BASE, then a 3-way
+                    # merge tool will cleanly generate "MaN" (as BASE vs THIS
+                    # removes one 'b', and BASE vs OTHER removes the other)
+                    # If you include neither, 3-way creates a clean "MbabN" as
+                    # THIS adds one 'b', and OTHER does too.
+                    # It seems that having the line 2 times is better than
+                    # having it omitted. (Easier to manually delete than notice
+                    # it needs to be added.)
+                    raise AssertionError('Unknown state: %s' % (state,))
+        return base_lines
+
 
 class WeaveMerge(PlanWeaveMerge):
     """Weave merge that takes a VersionedFile and two versions as its input."""




More information about the bazaar-commits mailing list