Rev 3955: Slight refactoring and test fixing. in lp:~bzr/bzr/1.12-lca-deleted

[Vincent Ladeuil]

At lp:~bzr/bzr/1.12-lca-deleted

------------------------------------------------------------
revno: 3955
revision-id: v.ladeuil+lp at free.fr-20090123210648-yfb39g22yyo83d3y
parent: v.ladeuil+lp at free.fr-20090123194502-4i7lbovxs4s2oeh6
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: spurious-conflicts
timestamp: Fri 2009-01-23 22:06:48 +0100
message:
  Slight refactoring and test fixing.
  
  * bzrlib/tests/test_merge.py:
  (TestMergerEntriesLCAOnDisk.test_modified_symlink): Passing now.
  
  * bzrlib/merge.py:
  (Merge3Merger._lca_multi_way): Fix doc reference.
  (Merge3Merger.merge_contents.contents_conflict): Try to delay
  this_pair evaulation to avoid unnecessary sha1 (impyling file read
  from disk) calculation. Also slightly refactor to avoid repeated
  file_id in trees calculations.
-------------- next part --------------
=== modified file 'NEWS'
--- a/NEWS	2009-01-23 07:42:35 +0000
+++ b/NEWS	2009-01-23 21:06:48 +0000
@@ -19,6 +19,11 @@
       merge, we would cause the deletion to conflict a second time.
       (Vincent Ladeuil, John Arbash Meinel)
 
+    * There was another bug in how we chose the correct intermediate LCA in
+      criss-cross merges leading to several kind of changes be incorrectly
+      handled.
+      (John Arbash Meinel, Vincent Ladeuil)
+
 
 NOT RELEASED YET
 ----------------

=== modified file 'bzrlib/merge.py'
--- a/bzrlib/merge.py	2009-01-23 19:45:02 +0000
+++ b/bzrlib/merge.py	2009-01-23 21:06:48 +0000
@@ -977,7 +977,7 @@
         :return: 'this', 'other', or 'conflict' depending on whether an entry
             changed or not.
         """
-        # See doc/developers/lca_merge_resolution.txt for details about this
+        # See doc/developers/lca_tree_merging.txt for details about this
         # algorithm.
         if other == this:
             # Either Ambiguously clean, or nothing was actually changed. We
@@ -1118,40 +1118,43 @@
         # file kind...
         base_pair = contents_pair(self.base_tree)
         other_pair = contents_pair(self.other_tree)
-        # TODO: we could evaluate this only after evaluating base vs other vs
-        #       lcas, as at least some of the time it isn't needed
-        this_pair = contents_pair(self.this_tree)
         if self._lca_trees:
+            this_pair = contents_pair(self.this_tree)
             lca_pairs = [contents_pair(tree) for tree in self._lca_trees]
             winner = self._lca_multi_way((base_pair, lca_pairs), other_pair,
                                          this_pair, allow_overriding_lca=False)
         else:
-            winner = self._three_way(base_pair, other_pair, this_pair)
+            if base_pair == other_pair:
+                winner = 'this'
+            else:
+                # We delayed evaluating this_pair as long as we can to avoid
+                # unnecessary sha1 calculation
+                this_pair = contents_pair(self.this_tree)
+                winner = self._three_way(base_pair, other_pair, this_pair)
         if winner == 'this':
             # No interesting changes introduced by OTHER
             return "unmodified"
         trans_id = self.tt.trans_id_file_id(file_id)
         if winner == 'other':
             # OTHER is a straight winner, so replace this contents with other
-            if file_id in self.this_tree:
+            file_in_this = file_id in self.this_tree
+            if file_in_this:
                 # Remove any existing contents
                 self.tt.delete_contents(trans_id)
-            # XXX: why do we use file_id in self.other_tree, but then use
             if file_id in self.other_tree:
                 # OTHER changed the file
                 create_from_tree(self.tt, trans_id,
                                  self.other_tree, file_id)
-                if file_id not in self.this_tree:
+                if not file_in_this:
                     self.tt.version_file(file_id, trans_id)
                 return "modified"
-            elif file_id in self.this_tree:
+            elif file_in_this:
                 # OTHER deleted the file
                 self.tt.unversion_file(trans_id)
                 return "deleted"
         else:
             # We have a hypothetical conflict, but if we have files, then we
             # can try to merge the content
-            assert winner == 'conflict'
             if this_pair[0] == 'file' and other_pair[0] == 'file':
                 # THIS and OTHER are both files, so text merge.  Either
                 # BASE is a file, or both converted to files, so at least we

=== modified file 'bzrlib/tests/test_merge.py'
--- a/bzrlib/tests/test_merge.py	2009-01-23 19:45:02 +0000
+++ b/bzrlib/tests/test_merge.py	2009-01-23 21:06:48 +0000
@@ -2153,8 +2153,7 @@
         wt.merge_from_branch(wt.branch, 'C-id')
         wt.commit('D merges B & C', rev_id='D-id')
         conflicts = wt.merge_from_branch(wt.branch, to_revision='F-id')
-        self.expectFailure("Merge3Merger doesn't use lcas for symlink content",
-            self.assertEqual, 0, conflicts)
+        self.assertEqual(0, conflicts)
         self.assertEqual('bing', wt.get_symlink_target('foo-id'))
 
     def test_renamed_symlink(self):