launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:merge-diff-prerequisite into launchpad:master.

Commit message:
Send prerequisite repository path for merge diffs

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1861095 in Launchpad itself: "LP doesn't send Turnip the ID of the prerequisite repository"
  https://bugs.launchpad.net/launchpad/+bug/1861095

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/378169

When generating preview diffs for Git-based merge proposals with prerequisites, we send the commit IDs of the target, source, and prerequisite to the hosting service, but the combined path that we sent only included the target and source repository paths.  As a result, if the tip of the prerequisite branch wasn't in either the target or the source repository (perhaps because the prerequisite branch changed after the source branch was created), the hosting service would be unable to find that commit.

To fix this, simply add the prerequisite repository path to the combined path.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:merge-diff-prerequisite into launchpad:master.

diff --git a/lib/lp/code/model/diff.py b/lib/lp/code/model/diff.py
index d3a61d0..b5a0cb1 100644
--- a/lib/lp/code/model/diff.py
+++ b/lib/lp/code/model/diff.py
@@ -431,12 +431,14 @@ class PreviewDiff(Storm):
         else:
             source_repository = bmp.source_git_repository
             target_repository = bmp.target_git_repository
-            if source_repository == target_repository:
-                path = source_repository.getInternalPath()
-            else:
-                path = "%s:%s" % (
-                    target_repository.getInternalPath(),
-                    source_repository.getInternalPath())
+            prerequisite_repository = bmp.prerequisite_git_repository
+            path = target_repository.getInternalPath()
+            if source_repository != target_repository:
+                path += ":%s" % source_repository.getInternalPath()
+            if (prerequisite_repository is not None and
+                    prerequisite_repository != source_repository and
+                    prerequisite_repository != target_repository):
+                path += ":%s" % prerequisite_repository.getInternalPath()
             response = getUtility(IGitHostingClient).getMergeDiff(
                 path, bmp.target_git_commit_sha1, bmp.source_git_commit_sha1,
                 prerequisite=bmp.prerequisite_git_commit_sha1)
diff --git a/lib/lp/code/model/tests/test_diff.py b/lib/lp/code/model/tests/test_diff.py
index f4ec2ce..70f0866 100644
--- a/lib/lp/code/model/tests/test_diff.py
+++ b/lib/lp/code/model/tests/test_diff.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for Diff, etc."""
@@ -18,6 +18,12 @@ from breezy.patches import (
     parse_patches,
     RemoveLine,
     )
+from testtools.matchers import (
+    Equals,
+    Is,
+    MatchesDict,
+    MatchesListwise,
+    )
 import transaction
 from zope.security.proxy import removeSecurityProxy
 
@@ -141,14 +147,18 @@ class DiffTestCase(TestCaseWithFactory):
         return (source_bzr, source_rev_id, target_bzr, prerequisite_bzr,
                 prerequisite)
 
-    def createExampleGitMerge(self):
+    def createExampleGitMerge(self, prerequisite_bmp=None):
         """Create an example Git-based merge scenario.
 
         The actual diff work is done in turnip, and we have no turnip
         fixture as yet, so for the moment we just install a fake hosting
         client that will return a plausible-looking diff.
         """
-        bmp = self.factory.makeBranchMergeProposalForGit()
+        kwargs = {}
+        if prerequisite_bmp is not None:
+            kwargs["target_ref"] = prerequisite_bmp.target_git_ref
+            kwargs["prerequisite_ref"] = prerequisite_bmp.source_git_ref
+        bmp = self.factory.makeBranchMergeProposalForGit(**kwargs)
         source_sha1 = bmp.source_git_ref.commit_sha1
         target_sha1 = bmp.target_git_ref.commit_sha1
         patch = dedent("""\
@@ -586,6 +596,47 @@ class TestPreviewDiff(DiffTestCase):
         transaction.commit()
         self.assertEqual(patch, preview.text)
         self.assertEqual({'foo': (5, 0)}, preview.diffstat)
+        self.assertThat(
+            self.hosting_fixture.getMergeDiff.calls,
+            MatchesListwise([
+                MatchesListwise([
+                    Equals((
+                        "%s:%s" % (
+                            bmp.target_git_repository.getInternalPath(),
+                            bmp.source_git_repository.getInternalPath()),
+                        bmp.target_git_commit_sha1,
+                        bmp.source_git_commit_sha1,
+                        )),
+                    MatchesDict({"prerequisite": Is(None)}),
+                    ])]))
+
+    def test_fromBranchMergeProposalForGit_with_prerequisite(self):
+        # Correctly generates a PreviewDiff from a Git-based
+        # BranchMergeProposal.
+        prerequisite_bmp = self.factory.makeBranchMergeProposalForGit()
+        bmp, source_rev_id, target_rev_id, patch = self.createExampleGitMerge(
+            prerequisite_bmp=prerequisite_bmp)
+        preview = PreviewDiff.fromBranchMergeProposal(bmp)
+        transaction.commit()
+        self.assertEqual(patch, preview.text)
+        self.assertEqual({'foo': (5, 0)}, preview.diffstat)
+        self.assertThat(
+            self.hosting_fixture.getMergeDiff.calls,
+            MatchesListwise([
+                MatchesListwise([
+                    Equals((
+                        "%s:%s:%s" % (
+                            bmp.target_git_repository.getInternalPath(),
+                            bmp.source_git_repository.getInternalPath(),
+                            bmp.prerequisite_git_repository.getInternalPath()),
+                        bmp.target_git_commit_sha1,
+                        bmp.source_git_commit_sha1,
+                        )),
+                    MatchesDict({
+                        "prerequisite": Equals(
+                            bmp.prerequisite_git_commit_sha1),
+                        }),
+                    ])]))
 
     def test_fromBranchMergeProposalForGit_sets_conflicts(self):
         # Conflicts are set on the PreviewDiff.