[MERGE] remerge doesn't clobber unrelated conflicts

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Aaron Bentley wrote:
> Hi all,
> 
> This patch updates remerge so that remerging a file doesn't affect the
> conflicts for other files.
> 
> Aaron

Thanks for doing this. I'm have a little bit of trouble understanding
what the actual fix is, because some of your comments seem to conflict
with the code. Can you do a brief overview of what has changed?

...

@@ -2186,6 +2187,8 @@
             base_tree = repository.revision_tree(base_revision)
             other_tree = repository.revision_tree(pending_merges[0])
             interesting_ids = None
+            new_conflicts = []
+            conflicts = tree.conflicts()

^- What are these for? In case nothing else happens? They just don't
seem to be used.


             if file_list is not None:
                 interesting_ids = set()
                 for filename in file_list:
@@ -2198,7 +2201,9 @@

                     for name, ie in tree.inventory.iter_entries(file_id):
                         interesting_ids.add(ie.file_id)
+                new_conflicts = conflicts.select_conflicts(tree,
file_list)[0]
             transform_tree(tree, tree.basis_tree(), interesting_ids)
+            tree.set_conflicts(ConflictList(new_conflicts))
             if file_list is None:
                 restore_files = list(tree.iter_conflicts())
             else:
@@ -2208,13 +2213,13 @@
                     restore(tree.abspath(filename))
                 except NotConflicted:
                     pass
- -            conflicts =  merge_inner(tree.branch, other_tree, base_tree,
- -                                     this_tree=tree,
- -                                     interesting_ids = interesting_ids,
- -                                     other_rev_id=pending_merges[0],
- -                                     merge_type=merge_type,
- -                                     show_base=show_base,
- -                                     reprocess=reprocess)
+            conflicts = merge_inner(tree.branch, other_tree, base_tree,
+                                    this_tree=tree,
+                                    interesting_ids = interesting_ids,
+                                    other_rev_id=pending_merges[0],
+                                    merge_type=merge_type,
+                                    show_base=show_base,
+                                    reprocess=reprocess)

A little bit of PEP8 here, arguments aren't supposed to have spaces for
parameters, so:
interesting_ids=interesting_ids
not
interesting_ids = interesting_ids.

Am I correct in saying that the only thing that changed was the spacing?



         finally:
             tree.unlock()
         if conflicts > 0:

=== modified file bzrlib/conflicts.py
- --- bzrlib/conflicts.py
+++ bzrlib/conflicts.py
@@ -207,6 +207,7 @@
         """Select the conflicts associated with paths in a tree.

         File-ids are also used for this.
+        :return: a pair of ConflictLists: (not_selected, selected)
         """
         path_set = set(paths)
         ids = {}

^- You change the comment, but I don't see any 'return' section being
changed. I think you didn't actually change it in this way.


=== modified file bzrlib/merge.py
- --- bzrlib/merge.py
+++ bzrlib/merge.py
@@ -390,7 +390,9 @@
             results = self.tt.apply()
             self.write_modified(results)
             try:
- -
working_tree.set_conflicts(ConflictList(self.cooked_conflicts))
+                new_conflicts = (self.cooked_conflicts +
+                                 list(working_tree.conflicts()))
+                working_tree.set_conflicts(ConflictList(new_conflicts))
             except UnsupportedOperation:
                 pass
         finally:

^- And this seems to indicated that you are combining the conflicts, not
returning 2 sets. I'm a little concerned that you would get double
entries for any files that you remerge. Though somehow you must be
working around this.


=== modified file bzrlib/tests/blackbox/test_remerge.py
- --- bzrlib/tests/blackbox/test_remerge.py
+++ bzrlib/tests/blackbox/test_remerge.py
@@ -25,8 +25,10 @@

     def make_file(self, name, contents):
         f = open(name, 'wb')
- -        f.write(contents)
- -        f.close()
+        try:
+            f.write(contents)
+        finally:
+            f.close()

     def create_conflicts(self):
         """Create a conflicted tree"""
@@ -106,3 +108,14 @@
         self.run_bzr_error(['remerge only works after normal merges',
                             'Not cherrypicking or multi-merges'],
                            'remerge')
+
+    def test_conflicts(self):
+        self.create_conflicts()
+        self.run_bzr('merge', '../other', retcode=1)
+        wt = WorkingTree.open('.')
+        self.assertEqual(len(wt.conflicts()), 2)
+        self.run_bzr('remerge', retcode=1)
+        wt = WorkingTree.open('.')
+        self.assertEqual(len(wt.conflicts()), 2)
+        self.run_bzr('remerge', 'hello', retcode=1)
+        self.assertEqual(len(wt.conflicts()), 2)

The test looks good.


=== modified file bzrlib/tests/test_merge.py
- --- bzrlib/tests/test_merge.py
+++ bzrlib/tests/test_merge.py
@@ -19,6 +19,7 @@

 from bzrlib.branch import Branch
 from bzrlib.builtins import merge
+from bzrlib.conflicts import ConflictList, TextConflict
 from bzrlib.errors import UnrelatedBranches, NoCommits, BzrCommandError
 from bzrlib.merge import transform_tree, merge_inner
 from bzrlib.osutils import pathjoin
@@ -139,3 +140,9 @@
                     this_tree=tree_b, ignore_zero=False)
         log = self._get_log()
         self.failUnless('All changes applied successfully.\n' in log)
+
+    def test_merge_inner_conflicts(self):
+        tree_a = self.make_branch_and_tree('a')
+        tree_a.set_conflicts(ConflictList([TextConflict('patha')]))
+        merge_inner(tree_a.branch, tree_a, tree_a, tree_a)
+        self.assertEqual(len(tree_a.conflicts()), 1)

^- What are you testing here. That merge_inner() doesn't effect the
conflict list?

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFEpxn7JdeBCYSNAAMRAgsiAJ9s2U0JTYeqvnweloR15ZSMBZBQXgCgkJlC
48VFMFj3QVY8K4qTRfVp1Fk=
=oNc5
-----END PGP SIGNATURE-----