Merge ~pelpsi/launchpad:differentiate-git-push-message-if-mp-exists into launchpad:master

[Simone Pelosi]

Differentiate git push message if the MP exists

[Colin Watson (cjwatson)]

review: Approve

[Simone Pelosi (pelpsi)]

Squashed into one commit

diff --git a/lib/lp/code/interfaces/branchmergeproposal.py b/lib/lp/code/interfaces/branchmergeproposal.py
index e65eef6..1b3c916 100644
--- a/lib/lp/code/interfaces/branchmergeproposal.py
+++ b/lib/lp/code/interfaces/branchmergeproposal.py
@@ -971,6 +971,13 @@ class IBranchMergeProposalGetter(Interface):
     def get(id):
         """Return the BranchMergeProposal with specified id."""
 
+    def activeProposalsForBranches(source, target=None):
+        """Return BranchMergeProposals having a given source and target.
+
+        :param source: source branch
+        :param target: target branch
+        """
+
     def getProposalsForContext(context, status=None, visible_by_user=None):
         """Return BranchMergeProposals associated with the context.
 
diff --git a/lib/lp/code/interfaces/gitapi.py b/lib/lp/code/interfaces/gitapi.py
index 5495944..98a241a 100644
--- a/lib/lp/code/interfaces/gitapi.py
+++ b/lib/lp/code/interfaces/gitapi.py
@@ -95,6 +95,22 @@ class IGitAPI(Interface):
             or an `Unauthorized` fault for unauthorized push attempts.
         """
 
+    def getMergeProposalInfo(translated_path, branch, auth_params):
+        """Return the info (string) for a merge proposal.
+
+        When a `branch` that is not the default branch in a repository
+        is pushed, the URL where the merge proposal for that branch can
+        be opened will be generated and returned if the merge proposal
+        doesn't exist, otherwise the link of the existing merge proposal
+        will be returned.
+
+        :returns: A string explaining how to register a merge proposal
+            for this branch, or pointing to an existing active merge
+            proposal. A `GitRepositoryNotFound` fault is returned
+            if no repository can be found for 'translated_path',
+            or an `Unauthorized` fault for unauthorized push attempts.
+        """
+
     def confirmRepoCreation(repository_id):
         """Confirm that repository creation.
 
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index 7f6053f..4f57972 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -787,6 +787,21 @@ class IGitRepositoryView(IHasRecipes):
             diffs updated.
         """
 
+    def makeFrozenRef(path, commit_sha1):
+        """A frozen Git reference.
+
+        This is like a GitRef, but is frozen at a particular commit, even if
+        the real reference has moved on or has been deleted.
+        It isn't necessarily backed by a real database object,
+        and will retrieve columns from the database when required.
+        Use this when you have a repository/path/commit_sha1 that you want
+        to pass around as a single object,
+        but don't necessarily know that the ref still exists.
+
+        :param path: the repository reference path.
+        :param commit_sha1: the commit sha1 for that repository reference path.
+        """
+
     def markRecipesStale(paths):
         """Mark recipes associated with this repository as stale.
 
diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index 924658c..73cae9a 100644
--- a/lib/lp/code/model/branchmergeproposal.py
+++ b/lib/lp/code/model/branchmergeproposal.py
@@ -220,12 +220,9 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
 
     @property
     def source_git_ref(self):
-        from lp.code.model.gitref import GitRefFrozen
-
         if self.source_git_repository is None:
             return None
-        return GitRefFrozen(
-            self.source_git_repository,
+        return self.source_git_repository.makeFrozenRef(
             self.source_git_path,
             self.source_git_commit_sha1,
         )
@@ -238,12 +235,9 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
 
     @property
     def target_git_ref(self):
-        from lp.code.model.gitref import GitRefFrozen
-
         if self.target_git_repository is None:
             return None
-        return GitRefFrozen(
-            self.target_git_repository,
+        return self.target_git_repository.makeFrozenRef(
             self.target_git_path,
             self.target_git_commit_sha1,
         )
@@ -256,12 +250,9 @@ class BranchMergeProposal(StormBase, BugLinkTargetMixin):
 
     @property
     def prerequisite_git_ref(self):
-        from lp.code.model.gitref import GitRefFrozen
-
         if self.prerequisite_git_repository is None:
             return None
-        return GitRefFrozen(
-            self.prerequisite_git_repository,
+        return self.prerequisite_git_repository.makeFrozenRef(
             self.prerequisite_git_path,
             self.prerequisite_git_commit_sha1,
         )
@@ -1805,7 +1796,7 @@ class BranchMergeProposalGetter:
         return result
 
     @staticmethod
-    def activeProposalsForBranches(source, target):
+    def activeProposalsForBranches(source, target=None):
         clauses = [Not(BranchMergeProposal.queue_status.is_in(FINAL_STATES))]
         if IGitRef.providedBy(source):
             clauses.extend(
@@ -1813,16 +1804,29 @@ class BranchMergeProposalGetter:
                     BranchMergeProposal.source_git_repository
                     == source.repository,
                     BranchMergeProposal.source_git_path == source.path,
-                    BranchMergeProposal.target_git_repository
-                    == target.repository,
-                    BranchMergeProposal.target_git_path == target.path,
                 ]
             )
+
+            if target is not None:
+                clauses.extend(
+                    [
+                        BranchMergeProposal.target_git_repository
+                        == target.repository,
+                        BranchMergeProposal.target_git_path == target.path,
+                    ]
+                )
+
         else:
             clauses.extend(
                 [
                     BranchMergeProposal.source_branch == source,
-                    BranchMergeProposal.target_branch == target,
                 ]
             )
+
+            if target is not None:
+                clauses.extend(
+                    [
+                        BranchMergeProposal.target_branch == target,
+                    ]
+                )
         return IStore(BranchMergeProposal).find(BranchMergeProposal, *clauses)
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 87abb5b..a91df2e 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -98,7 +98,7 @@ from lp.code.interfaces.revisionstatus import (
 from lp.code.mail.branch import send_git_repository_modified_notifications
 from lp.code.model.branchmergeproposal import BranchMergeProposal
 from lp.code.model.gitactivity import GitActivity
-from lp.code.model.gitref import GitRef, GitRefDefault
+from lp.code.model.gitref import GitRef, GitRefDefault, GitRefFrozen
 from lp.code.model.gitrule import GitRule, GitRuleGrant
 from lp.code.model.gitsubscription import GitSubscription
 from lp.code.model.revisionstatus import RevisionStatusReport
@@ -1478,6 +1478,13 @@ class GitRepository(
             jobs.extend(merge_proposal.scheduleDiffUpdates())
         return jobs
 
+    def makeFrozenRef(self, path, commit_sha1):
+        return GitRefFrozen(
+            self,
+            path,
+            commit_sha1,
+        )
+
     def _getRecipes(self, paths=None):
         """Undecorated version of recipes for use by `markRecipesStale`."""
         from lp.code.model.sourcepackagerecipedata import (
diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index 3b97a85..6e39e8c 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -38,6 +38,7 @@ from lp.code.errors import (
     GitRepositoryExists,
     InvalidNamespace,
 )
+from lp.code.interfaces.branchmergeproposal import IBranchMergeProposalGetter
 from lp.code.interfaces.codehosting import (
     LAUNCHPAD_ANONYMOUS,
     LAUNCHPAD_SERVICES,
@@ -699,6 +700,76 @@ class GitAPI(LaunchpadXMLRPCView):
             del result
 
     @return_fault
+    def _getMergeProposalInfo(
+        self, requester, translated_path, ref, auth_params
+    ):
+        if requester == LAUNCHPAD_ANONYMOUS:
+            requester = None
+        repository = removeSecurityProxy(
+            getUtility(IGitLookup).getByHostingPath(translated_path)
+        )
+        if repository is None:
+            raise faults.GitRepositoryNotFound(translated_path)
+
+        verified = self._verifyAuthParams(requester, repository, auth_params)
+        if verified is not None and verified.user is NO_USER:
+            # Showing merge proposal information may be useful to ordinary
+            # users, but it doesn't make sense in the context of
+            # an internal service.
+            return None
+
+        # commit_sha1 isn't used here, but is
+        # needed to satisfy the `IGitRef` interface.
+        frozen_ref = repository.makeFrozenRef(path=ref, commit_sha1="")
+        branch = frozen_ref.name
+        merge_proposals = getUtility(
+            IBranchMergeProposalGetter
+        ).activeProposalsForBranches(source=frozen_ref, target=None)
+
+        merge_proposal = merge_proposals.any()
+        if merge_proposal:
+            return (
+                "Updated existing merge proposal "
+                "for %s on Launchpad:\n      %s"
+                % (quote(branch), merge_proposal.address)
+            )
+        else:
+            base_url = canonical_url(repository, rootsite="code")
+            mp_url = "%s/+ref/%s/+register-merge" % (base_url, quote(branch))
+            return (
+                "Create a merge proposal for '%s' on "
+                "Launchpad by visiting:\n      %s" % (branch, mp_url)
+            )
+
+    def getMergeProposalInfo(self, translated_path, ref, auth_params):
+        """See `IGitAPI`."""
+        logger = self._getLogger(auth_params.get("request-id"))
+        requester_id = _get_requester_id(auth_params)
+        logger.info(
+            "Request received: getMergeProposalInfo('%s, %s') for %s",
+            translated_path,
+            ref,
+            requester_id,
+        )
+
+        result = run_with_login(
+            requester_id,
+            self._getMergeProposalInfo,
+            translated_path,
+            ref,
+            auth_params,
+        )
+        try:
+            if isinstance(result, xmlrpc.client.Fault):
+                logger.error("getMergeProposalInfo failed: %r", result)
+            else:
+                logger.info("getMergeProposalInfo succeeded: %s" % result)
+            return result
+        finally:
+            # Avoid traceback reference cycles.
+            del result
+
+    @return_fault
     def _authenticateWithPassword(self, username, password):
         """Authenticate a user by username and password.
 
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index 1f69b8e..4764319 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -660,6 +660,34 @@ class TestGitAPIMixin:
         )
         self.assertEqual(expected_mp_url, result)
 
+    def assertHasMergeProposalInfo(
+        self, repository, pushed_branch, auth_params, mp=None
+    ):
+        base_url = canonical_url(repository, rootsite="code")
+        expected_mp_url = (
+            "Create a merge proposal for 'branch1' on "
+            "Launchpad by visiting:\n      "
+            "%s/+ref/%s/+register-merge"
+            % (
+                base_url,
+                quote(pushed_branch),
+            )
+        )
+
+        if mp:
+            expected_mp_url = (
+                "Updated existing merge proposal "
+                "for %s on Launchpad:\n      %s"
+                % (quote(pushed_branch), mp.address)
+            )
+
+        result = self.git_api.getMergeProposalInfo(
+            repository.getInternalPath(),
+            "refs/heads/%s" % pushed_branch,
+            auth_params,
+        )
+        self.assertEqual(expected_mp_url, result)
+
     def test_translatePath_private_repository(self):
         requester = self.factory.makePerson()
         repository = removeSecurityProxy(
@@ -2760,6 +2788,35 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
             access_token_id="string",
         )
 
+    # TODO: add more `getMergeProposalInfo` tests
+    def test_getMergeProposalInfo(self):
+        # Merge proposal information is returned by LP for a non-default branch
+        # pushed by a user that has their ordinary privileges on the
+        # corresponding repository.
+        requester_owner = self.factory.makePerson()
+        repository = self.factory.makeGitRepository(owner=requester_owner)
+        [ref] = self.factory.makeGitRefs(
+            repository=repository,
+            paths=["refs/heads/master"],
+        )
+        [source_ref] = self.factory.makeGitRefs(
+            repository=repository,
+            paths=["refs/heads/branch1"],
+        )
+        removeSecurityProxy(repository).default_branch = "refs/heads/master"
+        pushed_branch = "branch1"
+        self.assertHasMergeProposalInfo(
+            repository, pushed_branch, {"uid": requester_owner.id}
+        )
+
+        mp = self.factory.makeBranchMergeProposalForGit(
+            source_ref=source_ref, target_ref=ref
+        )
+
+        self.assertHasMergeProposalInfo(
+            repository, pushed_branch, {"uid": requester_owner.id}, mp
+        )
+
     def test_getMergeProposalURL_user(self):
         # A merge proposal URL is returned by LP for a non-default branch
         # pushed by a user that has their ordinary privileges on the