launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/fix-git-prerequisite into lp:launchpad.

Commit message:
Handle prerequisites in Git-based merge proposals.

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

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/fix-git-prerequisite/+merge/277281

Handle prerequisites in Git-based merge proposals.

The corresponding turnip change is now live on qastaging, and there's what should be an XS ticket (RT#86272) to upgrade production.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/fix-git-prerequisite into lp:launchpad.

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2015-10-05 17:02:20 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2015-11-11 18:26:03 +0000
@@ -11,6 +11,10 @@
     timedelta,
     )
 from difflib import unified_diff
+<<<<<<< TREE
+=======
+import re
+>>>>>>> MERGE-SOURCE
 
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.restful.interfaces import IJSONRequestCache
@@ -1484,8 +1488,36 @@
         self.assertThat(browser.contents, HTMLContains(expected_meta))
 
 
+<<<<<<< TREE
 class TestBranchMergeProposalChangeStatusView(TestCaseWithFactory):
     """Test the +edit-status view."""
+=======
+class TestBranchMergeProposalBrowserView(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_prerequisite_bzr(self):
+        # A prerequisite branch is rendered in the Bazaar case.
+        branch = self.factory.makeProductBranch()
+        identity = branch.identity
+        bmp = self.factory.makeBranchMergeProposal(prerequisite_branch=branch)
+        text = self.getMainText(bmp, '+index')
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            'Prerequisite: ' + re.escape(identity), text)
+
+    def test_prerequisite_git(self):
+        # A prerequisite reference is rendered in the Git case.
+        [ref] = self.factory.makeGitRefs()
+        identity = ref.identity
+        bmp = self.factory.makeBranchMergeProposalForGit(prerequisite_ref=ref)
+        text = self.getMainText(bmp, '+index')
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            'Prerequisite: ' + re.escape(identity), text)
+
+
+class TestBranchMergeProposalChangeStatusOptions(TestCaseWithFactory):
+    """Test the status vocabulary generated for then +edit-status view."""
+>>>>>>> MERGE-SOURCE
 
     layer = DatabaseFunctionalLayer
 

=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py	2015-07-09 20:06:17 +0000
+++ lib/lp/code/model/diff.py	2015-11-11 18:26:03 +0000
@@ -450,22 +450,15 @@
                 path = "%s:%s" % (
                     target_repository.getInternalPath(),
                     source_repository.getInternalPath())
-            # XXX cjwatson 2015-04-30: Add prerequisite handling once turnip
-            # supports it.
             response = getUtility(IGitHostingClient).getMergeDiff(
-                path, bmp.target_git_ref.commit_sha1,
-                bmp.source_git_ref.commit_sha1)
-            source_revision_id = bmp.source_git_ref.commit_sha1
-            target_revision_id = bmp.target_git_ref.commit_sha1
-            if bmp.prerequisite_git_ref is not None:
-                prerequisite_revision_id = bmp.prerequisite_git_ref.commit_sha1
-            else:
-                prerequisite_revision_id = None
+                path, bmp.target_git_commit_sha1, bmp.source_git_commit_sha1,
+                prerequisite=bmp.prerequisite_git_commit_sha1)
             conflicts = u"".join(
                 u"Conflict in %s\n" % path for path in response['conflicts'])
             preview = cls.create(
-                bmp, response['patch'].encode('utf-8'), source_revision_id,
-                target_revision_id, prerequisite_revision_id, conflicts,
+                bmp, response['patch'].encode('utf-8'),
+                bmp.source_git_commit_sha1, bmp.target_git_commit_sha1,
+                bmp.prerequisite_git_commit_sha1, conflicts,
                 strip_prefix_segments=1)
         del get_property_cache(bmp).preview_diffs
         del get_property_cache(bmp).preview_diff

=== modified file 'lib/lp/code/model/githosting.py'
--- lib/lp/code/model/githosting.py	2015-10-05 04:57:16 +0000
+++ lib/lp/code/model/githosting.py	2015-11-11 18:26:03 +0000
@@ -125,15 +125,17 @@
                 "Failed to get commit details from Git repository: %s" %
                 unicode(e))
 
-    def getMergeDiff(self, path, base, head, logger=None):
+    def getMergeDiff(self, path, base, head, prerequisite=None, logger=None):
         """See `IGitHostingClient`."""
         try:
             if logger is not None:
                 logger.info(
                     "Requesting merge diff for %s from %s to %s" % (
                         path, base, head))
-            return self._get(
-                "/repo/%s/compare-merge/%s:%s" % (path, base, head))
+            url = "/repo/%s/compare-merge/%s:%s" % (path, base, head)
+            if prerequisite is not None:
+                url += "?sha1_prerequisite=%s" % prerequisite
+            return self._get(url)
         except Exception as e:
             raise GitRepositoryScanFault(
                 "Failed to get merge diff from Git repository: %s" %

=== modified file 'lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt'
--- lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt	2015-06-10 10:25:28 +0000
+++ lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt	2015-11-11 18:26:03 +0000
@@ -110,7 +110,7 @@
       <td tal:content="structure context/merge_target/fmt:link">lp:~foo/bar/baz</td>
     </tr>
     <tr id="summary-row-a-prerequisite-branch"
-        tal:condition="context/prerequisite_branch">
+        tal:condition="context/merge_prerequisite">
       <th>Prerequisite:</th>
       <td tal:content="structure context/merge_prerequisite/fmt:link">lp:~foo/bar/baz</td>
     </tr>