launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/turnip:modify-delete-conflicts into turnip:master.

Commit message:
Fix compare-merge crash on modify/delete conflicts

We have to go to some work to synthesise semi-reasonable conflict diffs.
I'm not sure they're quite perfect, but they're close enough to give the
user a hint as to what's going on.

LP: #1609972

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1609972 in turnip: "Git merge proposals silently show no diff if there are conflicts?"
  https://bugs.launchpad.net/turnip/+bug/1609972

For more details, see:
https://code.launchpad.net/~cjwatson/turnip/+git/turnip/+merge/331347
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/turnip:modify-delete-conflicts into turnip:master.

diff --git a/turnip/api/store.py b/turnip/api/store.py
index 5767958..2e53e5a 100644
--- a/turnip/api/store.py
+++ b/turnip/api/store.py
@@ -331,11 +331,29 @@ def _add_conflicted_files(repo, index):
                 entry for entry in conflict if entry is not None][0]
             path = conflict_entry.path
             conflicts.add(path)
-            merged_file = repo.merge_file_from_index(*conflict)
-            # merge_file_from_index gratuitously decodes as UTF-8, so
-            # encode it back again.
-            blob_oid = repo.create_blob(merged_file.encode('utf-8'))
-            index.add(IndexEntry(path, blob_oid, conflict_entry.mode))
+            ancestor, ours, theirs = conflict
+            if ours is None and theirs is None:
+                # A delete/delete conflict?  We probably shouldn't get here,
+                # but if we do then the resolution is obvious.
+                index.remove(path)
+            else:
+                if ours is None or theirs is None:
+                    # A modify/delete conflict.  Turn the "delete" side into
+                    # a fake empty file so that we can generate a useful
+                    # conflict diff.
+                    empty_oid = repo.create_blob(b'')
+                    if ours is None:
+                        ours = IndexEntry(
+                            path, empty_oid, conflict_entry.mode)
+                    if theirs is None:
+                        theirs = IndexEntry(
+                            path, empty_oid, conflict_entry.mode)
+                merged_file = repo.merge_file_from_index(
+                    ancestor, ours, theirs)
+                # merge_file_from_index gratuitously decodes as UTF-8, so
+                # encode it back again.
+                blob_oid = repo.create_blob(merged_file.encode('utf-8'))
+                index.add(IndexEntry(path, blob_oid, conflict_entry.mode))
             del index.conflicts[path]
     return conflicts
 
diff --git a/turnip/api/tests/test_api.py b/turnip/api/tests/test_api.py
index 4e3c808..1bbfb60 100644
--- a/turnip/api/tests/test_api.py
+++ b/turnip/api/tests/test_api.py
@@ -414,6 +414,51 @@ class ApiTestCase(TestCase):
             """), resp.json_body['patch'])
         self.assertEqual(['blah.txt'], resp.json_body['conflicts'])
 
+    def test_repo_diff_merge_with_modify_delete_conflict(self):
+        """compare-merge handles modify/delete conflicts."""
+        repo = RepoFactory(self.repo_store)
+        c1 = repo.add_commit('foo\n', 'foo.txt')
+        c2_left = repo.add_commit('foo\nbar\n', 'foo.txt', parents=[c1])
+        repo.repo.index.remove('foo.txt')
+        c2_right = repo.add_commit('', 'bar.txt', parents=[c1])
+
+        resp = self.app.get('/repo/{}/compare-merge/{}:{}'.format(
+            self.repo_path, c2_left, c2_right))
+        self.assertIn(dedent(u"""\
+            --- a/foo.txt
+            +++ b/foo.txt
+            @@ -1,2 +1,5 @@
+            +<<<<<<< foo.txt
+             foo
+             bar
+            +=======
+            +>>>>>>> foo.txt
+            """), resp.json_body['patch'])
+        self.assertEqual(['foo.txt'], resp.json_body['conflicts'])
+
+    def test_repo_diff_merge_with_delete_modify_conflict(self):
+        """compare-merge handles delete/modify conflicts."""
+        repo = RepoFactory(self.repo_store)
+        c1 = repo.add_commit('foo\n', 'foo.txt')
+        repo.repo.index.remove('foo.txt')
+        c2_left = repo.add_commit('', 'bar.txt', parents=[c1])
+        repo.repo.index.remove('bar.txt')
+        c2_right = repo.add_commit('foo\nbar\n', 'foo.txt', parents=[c1])
+
+        resp = self.app.get('/repo/{}/compare-merge/{}:{}'.format(
+            self.repo_path, c2_left, c2_right))
+        self.assertIn(dedent(u"""\
+            --- /dev/null
+            +++ b/foo.txt
+            @@ -0,0 +1,5 @@
+            +<<<<<<< foo.txt
+            +=======
+            +foo
+            +bar
+            +>>>>>>> foo.txt
+            """), resp.json_body['patch'])
+        self.assertEqual(['foo.txt'], resp.json_body['conflicts'])
+
     def test_repo_diff_merge_with_prerequisite(self):
         """Ensure that compare-merge handles prerequisites."""
         repo = RepoFactory(self.repo_store)