launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/turnip/merge-prerequisite into lp:turnip.

Commit message:
Add an optional sha1_prerequisite parameter to compare-merge API.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1489839 in turnip: "Prerequisite handling broken for git merge proposals"
  https://bugs.launchpad.net/turnip/+bug/1489839

For more details, see:
https://code.launchpad.net/~cjwatson/turnip/merge-prerequisite/+merge/269512

Add an optional sha1_prerequisite parameter to compare-merge API.

This involves writing a temporary tree object, so I made sure to use an ephemeral repository in this case in order that we aren't relying on GC to clean things up for us later.  The algorithm is the same as that used by Launchpad for Bazaar: diff(merge prerequisite into target, merge source into target).
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/turnip/merge-prerequisite into lp:turnip.

=== modified file 'turnip/api/store.py'
--- turnip/api/store.py	2015-08-28 11:55:50 +0000
+++ turnip/api/store.py	2015-08-28 14:39:09 +0000
@@ -177,14 +177,17 @@
 
 
 @contextmanager
-def open_repo(repo_store, repo_name):
+def open_repo(repo_store, repo_name, force_ephemeral=False):
     """Open an existing git repository. Optionally create an ephemeral
-    repository with alternates if repo_path contains ':'.
+    repository with alternates if repo_name contains ':' or force_ephemeral
+    is True.
 
     :param repo_store: path to repository root.
     :param repo_name: repository name.
+    :param force_ephemeral: create an ephemeral repository even if repo_name
+        does not contain ':'.
     """
-    if ':' in repo_name:
+    if force_ephemeral or ':' in repo_name:
         try:
             # Create ephemeral repo with alternates set from both.
             # Neither git nor libgit2 will respect a relative alternate
@@ -297,28 +300,55 @@
                         sha1_source, context_lines)
 
 
+def _add_conflicted_files(repo, index):
+    """Add flattened versions of conflicted files in an index.
+
+    Any conflicted files will be merged using
+    `pygit2.Repository.merge_file_from_index` (thereby including conflict
+    markers); the resulting files will be added to the index and the
+    conflicts deleted.
+
+    :param repo: a `pygit2.Repository`.
+    :param index: a `pygit2.Index` to modify.
+    :return: a set of files that contain conflicts.
+    """
+    conflicts = set()
+    if index.conflicts is not None:
+        for conflict in list(index.conflicts):
+            path = [entry for entry in conflict
+                    if entry is not None][0].path
+            conflicts.add(path)
+            merged_file = repo.merge_file_from_index(*conflict)
+            blob_oid = repo.create_blob(merged_file)
+            index.add(IndexEntry(path, blob_oid, GIT_FILEMODE_BLOB))
+            del index.conflicts[path]
+    return conflicts
+
+
 def get_merge_diff(repo_store, repo_name, sha1_base,
-                   sha1_head, context_lines=3):
+                   sha1_head, context_lines=3, sha1_prerequisite=None):
     """Get diff of common ancestor and source diff.
 
     :param sha1_base: target sha1 for merge.
     :param sha1_head: source sha1 for merge.
     :param context_lines: num unchanged lines that define a hunk boundary.
+    :param sha1_prerequisite: if not None, sha1 of prerequisite commit to
+        merge into `sha1_target` before computing diff to `sha1_source`.
     """
-    with open_repo(repo_store, repo_name) as repo:
+    with open_repo(
+            repo_store, repo_name,
+            force_ephemeral=(sha1_prerequisite is not None)) as repo:
+        if sha1_prerequisite is not None:
+            prerequisite_index = repo.merge_commits(
+                sha1_base, sha1_prerequisite)
+            _add_conflicted_files(repo, prerequisite_index)
+            from_tree = repo[prerequisite_index.write_tree(repo=repo)]
+        else:
+            from_tree = repo[sha1_base].tree
         merged_index = repo.merge_commits(sha1_base, sha1_head)
-        conflicts = set()
-        if merged_index.conflicts is not None:
-            for conflict in list(merged_index.conflicts):
-                path = [entry for entry in conflict
-                        if entry is not None][0].path
-                conflicts.add(path)
-                merged_file = repo.merge_file_from_index(*conflict)
-                blob_oid = repo.create_blob(merged_file)
-                merged_index.add(IndexEntry(path, blob_oid, GIT_FILEMODE_BLOB))
-                del merged_index.conflicts[path]
+        conflicts = _add_conflicted_files(repo, merged_index)
         patch = merged_index.diff_to_tree(
-            repo[sha1_base].tree, context_lines=context_lines).patch
+            from_tree, context_lines=context_lines).patch
         if patch is None:
             patch = u''
         shas = [sha1_base, sha1_head]

=== modified file 'turnip/api/tests/test_api.py'
--- turnip/api/tests/test_api.py	2015-06-18 00:05:01 +0000
+++ turnip/api/tests/test_api.py	2015-08-28 14:39:09 +0000
@@ -382,6 +382,24 @@
             """), resp.json_body['patch'])
         self.assertEqual(['blah.txt'], resp.json_body['conflicts'])
 
+    def test_repo_diff_merge_with_prerequisite(self):
+        """Ensure that compare-merge handles prerequisites."""
+        repo = RepoFactory(self.repo_store)
+        c1 = repo.add_commit('foo\n', 'blah.txt')
+        c2 = repo.add_commit('foo\nbar\n', 'blah.txt', parents=[c1])
+        c3 = repo.add_commit('foo\nbar\nbaz\n', 'blah.txt', parents=[c2])
+
+        resp = self.app.get(
+            '/repo/{}/compare-merge/{}:{}?sha1_prerequisite={}'.format(
+                self.repo_path, c1, c3, c2))
+        self.assertIn(dedent("""\
+            @@ -1,2 +1,3 @@
+             foo
+             bar
+            +baz
+            """), resp.json_body['patch'])
+        self.assertEqual([], resp.json_body['conflicts'])
+
     def test_repo_diff_merge_empty(self):
         """Ensure that diffing two identical commits returns an empty string
         as the patch, not None."""

=== modified file 'turnip/api/views.py'
--- turnip/api/views.py	2015-06-15 19:15:51 +0000
+++ turnip/api/views.py	2015-08-28 14:39:09 +0000
@@ -230,10 +230,12 @@
     def get(self, repo_store, repo_name):
         """Returns diff of two commits."""
         context_lines = int(self.request.params.get('context_lines', 3))
+        sha1_prerequisite = self.request.params.get('sha1_prerequisite')
         try:
             patch = store.get_merge_diff(
                 repo_store, repo_name, self.request.matchdict['base'],
-                self.request.matchdict['head'], context_lines)
+                self.request.matchdict['head'], context_lines=context_lines,
+                sha1_prerequisite=sha1_prerequisite)
         except (KeyError, ValueError, GitError):
             # invalid pygit2 sha1's return ValueError: 1: Ambiguous lookup
             return exc.HTTPNotFound()