Merge ~pappacena/launchpad:create-mp-refs into launchpad:master

[Thiago F. Pappacena]

Creating and deleting merge proposal virtual ref for "merge/xxx/head" on git hosting when appropriated.

[William Grant (wgrant)]

How does this interact with privacy?

[Thiago F. Pappacena (pappacena)]

wgrant, good point.

When a user opens a MP#1 targeting RepositoryX's master, for example, RepositoryX itself will have a read-only ref called `refs/merge/1/head`. The idea is that whomever is responsible for RepositoryX will have an easier way to pull locally the changes introduced by MP#1.

Let's assume a RepositoryX is private. In theory, nothing changes for the user opening the MP#1: the privacy checks and requirements to actually open a new MP targeting RepositoryX are still the same.

The only extra security check introduced on RepositoryX will be on Turnip side, to block pushes to `refs/merge/...` namespace: https://code.launchpad.net/~pappacena/turnip/+git/turnip/+merge/390620.

Do you see any specific privacy problem with this scenario?

[William Grant (wgrant)]

On 12/9/20 12:38 am, Thiago F. Pappacena wrote:
> wgrant, good point.
>
> When a user opens a MP#1 targeting RepositoryX's master, for example, RepositoryX itself will have a read-only ref called `refs/merge/1/head`. The idea is that whomever is responsible for RepositoryX will have an easier way to pull locally the changes introduced by MP#1.
>
> Let's assume a RepositoryX is private. In theory, nothing changes for the user opening the MP#1: the privacy checks and requirements to actually open a new MP targeting RepositoryX are still the same.
>
> The only extra security check introduced on RepositoryX will be on Turnip side, to block pushes to `refs/merge/...` namespace: https://code.launchpad.net/~pappacena/turnip/+git/turnip/+merge/390620.
>
> Do you see any specific privacy problem with this scenario?

The problem arises when the *source* repository is private. Consider,
for example, a security fix MP: a user can only view an MP if they can
see both the source and target branches. But this will let anyone who
can see the target repository examine the code in the MP from a
potentially invisible private branch.

[Thiago F. Pappacena (pappacena)]

wgrant, got it.

I guess we have 2 options in this case:

1- We don't add at all the the `refs/merge/<xxx>` if the source repository is private; or
2- We do extra permission checks on refs on Turnip on every operation, and not only pushes as it seems to be implemented today.

The first option brings some complexity in terms of changing a repository to/from private after the virtual ref is already created, but is simple to implement.

I prefer the second option, as it seems cleaner and better for the long term IMHO, although it might require (a small) extra implementation effort now.

Any thoughts on this?

[Thiago F. Pappacena (pappacena)]

Just to keep the record, it turn out that filtering git fetches is not that trivial, since there is no hook for that anymore (http://git.661346.n2.nabble.com/Removal-of-post-upload-hook-td4394312.html).

We will handle private repositories the following way:

1- When opening the MP, if the source is a different private repository, the virtual ref will not be created;
2- When opening the MP, if the source is the same repository (regardless of being private or not), we will create the virtual ref;
3- When opening the MP, if the owner of the source repository is the same as the owner of the target repository, we will creat ethe virtual ref
4- When privacy or the owner of a repository changes, we need to run a job to check again the above conditions and make sure that the virtual ref is created/deleted accordingly

I'll split in some MPs just to make the review easier (this MP is already a bit too big), but I guess all of these rules should be landed at once.

[Thiago F. Pappacena (pappacena)]

The MP with the job that should re-sync the virtual refs is here: https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/390940.

As said before, they are splitted just to make the review process easier, but the functionality should only be landed once both MPs are ready to be landed together.

[Colin Watson (cjwatson)]

review: Approve

[Colin Watson (cjwatson)]

Also please set a commit message.

4fae2e0... by Thiago F. Pappacena on 2020-10-13

Merge branch 'master' into create-mp-refs

4740bf9... by Thiago F. Pappacena on 2020-10-13

Methods renaming and better defining interfaces

297a039... by Thiago F. Pappacena on 2020-10-13

Refactoring

[Thiago F. Pappacena (pappacena)]

Strange that the commit message is empty. I'm adding it now.

Pushed the requested changes. There are a few comments that may worth checking, cjwatson.

b4af659... by Thiago F. Pappacena on 2020-10-29

Merge branch 'master' into create-mp-refs

Unmerged commits

b4af659... by Thiago F. Pappacena on 2020-10-29

Merge branch 'master' into create-mp-refs

297a039... by Thiago F. Pappacena on 2020-10-13

Refactoring

4740bf9... by Thiago F. Pappacena on 2020-10-13

Methods renaming and better defining interfaces

4fae2e0... by Thiago F. Pappacena on 2020-10-13

Merge branch 'master' into create-mp-refs

f3bda00... by Thiago F. Pappacena on 2020-09-17

Fixing test

30c88c7... by Thiago F. Pappacena on 2020-09-16

Merge branch 'githosting-copy-and-delete-refs' into create-mp-refs

7e11cb0... by Thiago F. Pappacena on 2020-09-16

Refactoring virtual refs creation and preparing privacy code

1f7f371... by Thiago F. Pappacena on 2020-09-11

Adding feature flag and triggering copy on MP creation

50eeb2e... by Thiago F. Pappacena on 2020-09-11

Adding feature flag to control mp virtual refs creation

84d5c5f... by Thiago F. Pappacena on 2020-09-10

Logging error

diff --git a/lib/lp/code/interfaces/branchmergeproposal.py b/lib/lp/code/interfaces/branchmergeproposal.py
index bf481c9..449e921 100644
--- a/lib/lp/code/interfaces/branchmergeproposal.py
+++ b/lib/lp/code/interfaces/branchmergeproposal.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 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).
 
 """The interface for branch merge proposals."""
@@ -519,6 +519,31 @@ class IBranchMergeProposalView(Interface):
     def getLatestDiffUpdateJob():
         """Return the latest IUpdatePreviewDiffJob for this MP."""
 
+    def getGitRefCopyOperations():
+        """Gets the list of GitHosting copy operations that should be done
+        in order to keep this MP's virtual refs up-to-date.
+
+        :return: A list of RefCopyOperation objects.
+        """
+
+    def syncGitVirtualRefs(force_delete=False):
+        """Requests all copies and deletion of virtual refs to make git code
+        hosting in sync with this MP.
+
+        :param force_delete: Do not try to copy any new virtual ref. Just
+            delete all virtual refs possibly created.
+        """
+
+    def bulkSyncGitVirtualRefs(git_repository, merge_proposals):
+        """Synchronizes a set of merge proposals' virtual refs.
+
+        :params git_repository: The source git repository of the given merge
+            proposals.
+        :params merge_proposals: The set of merge proposals that should have
+            the virtual refs synced.
+        :return: A list of copy operations executed.
+        """
+
 
 class IBranchMergeProposalEdit(Interface):
 
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index 192133a..48e7dd3 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -8,6 +8,8 @@ __metaclass__ = type
 __all__ = [
     'ContributorGitIdentity',
     'GitIdentityMixin',
+    'GIT_CREATE_MP_VIRTUAL_REF',
+    'GIT_MP_VIRTUAL_REF_FORMAT',
     'GIT_REPOSITORY_NAME_VALIDATION_ERROR_MESSAGE',
     'git_repository_name_validator',
     'IGitRepository',
@@ -105,6 +107,11 @@ valid_git_repository_name_pattern = re.compile(
     r"^(?i)[a-z0-9][a-z0-9+\.\-@_]*\Z")
 
 
+# Virtual ref where we automatically put a copy of any open merge proposal.
+GIT_MP_VIRTUAL_REF_FORMAT = 'refs/merge/{mp.id}/head'
+GIT_CREATE_MP_VIRTUAL_REF = 'git.mergeproposal_virtualref.enabled'
+
+
 def valid_git_repository_name(name):
     """Return True iff the name is valid as a Git repository name.
 
@@ -595,11 +602,15 @@ class IGitRepositoryView(IHasRecipes):
     def updateLandingTargets(paths):
         """Update landing targets (MPs where this repository is the source).
 
-        For each merge proposal, create `UpdatePreviewDiffJob`s.
+        For each merge proposal, create `UpdatePreviewDiffJob`s, and runs
+        the appropriate GitHosting.copyRefs operation to allow having
+        virtual merge refs with the updated branch version.
 
         :param paths: A list of reference paths.  Any merge proposals whose
             source is this repository and one of these paths will have their
             diffs updated.
+        :return: Returns a tuple with the list of background jobs created,
+                 and the list of ref copies requested to GitHosting.
         """
 
     def markRecipesStale(paths):
diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index 061fb1e..ec65e98 100644
--- a/lib/lp/code/model/branchmergeproposal.py
+++ b/lib/lp/code/model/branchmergeproposal.py
@@ -12,6 +12,7 @@ __all__ = [
 
 from collections import defaultdict
 from email.utils import make_msgid
+import logging
 from operator import attrgetter
 import sys
 
@@ -84,7 +85,12 @@ from lp.code.interfaces.codereviewcomment import ICodeReviewComment
 from lp.code.interfaces.codereviewinlinecomment import (
     ICodeReviewInlineCommentSet,
     )
+from lp.code.interfaces.githosting import IGitHostingClient
 from lp.code.interfaces.gitref import IGitRef
+from lp.code.interfaces.gitrepository import (
+    GIT_CREATE_MP_VIRTUAL_REF,
+    GIT_MP_VIRTUAL_REF_FORMAT,
+    )
 from lp.code.mail.branch import RecipientReason
 from lp.code.model.branchrevision import BranchRevision
 from lp.code.model.codereviewcomment import CodeReviewComment
@@ -94,6 +100,7 @@ from lp.code.model.diff import (
     IncrementalDiff,
     PreviewDiff,
     )
+from lp.code.model.githosting import RefCopyOperation
 from lp.registry.interfaces.person import (
     IPerson,
     IPersonSet,
@@ -123,6 +130,7 @@ from lp.services.database.sqlbase import (
     quote,
     SQLBase,
     )
+from lp.services.features import getFeatureFlag
 from lp.services.helpers import shortlist
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
@@ -144,6 +152,9 @@ from lp.soyuz.enums import (
     )
 
 
+logger = logging.getLogger(__name__)
+
+
 def is_valid_transition(proposal, from_state, next_state, user=None):
     """Is it valid for this user to move this proposal to to next_state?
 
@@ -971,6 +982,7 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
             branch_merge_proposal=self.id):
             job.destroySelf()
         self._preview_diffs.remove()
+
         self.destroySelf()
 
     def getUnlandedSourceBranchRevisions(self):
@@ -1217,6 +1229,86 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
             for diff in diffs)
         return [diff_dict.get(revisions) for revisions in revision_list]
 
+    def getGitRefCopyOperations(self):
+        """See `IBranchMergeProposal`"""
+        if (not self.target_git_repository
+                or not getFeatureFlag(GIT_CREATE_MP_VIRTUAL_REF)):
+            return []
+        # If the source repository is private and it's opening a MP to
+        # another repository, we should not risk disclosing the private code to
+        # everyone with permission to see the target repo:
+        private_source = self.source_git_repository.private
+        same_owner = self.source_git_repository.owner.inTeam(
+            self.target_git_repository.owner)
+        if private_source and not same_owner:
+            return []
+        return [RefCopyOperation(
+            self.source_git_commit_sha1,
+            self.target_git_repository.getInternalPath(),
+            GIT_MP_VIRTUAL_REF_FORMAT.format(mp=self))]
+
+    def _copyGitVirtualRefs(self):
+        """Requests virtual refs copy operations on GitHosting in order to
+        keep them up-to-date with current MP's state.
+
+        :return: The list of copied refs.
+        """
+        copy_operations = self.getGitRefCopyOperations()
+        if copy_operations:
+            hosting_client = getUtility(IGitHostingClient)
+            hosting_client.copyRefs(
+                self.source_git_repository.getInternalPath(),
+                copy_operations)
+        return [i.target_ref for i in copy_operations]
+
+    def _deleteGitVirtualRefs(self, except_refs=None):
+        """Deletes on git code hosting service all virtual refs, except
+        those ones in the given list.
+
+        For now, the only existing virtual ref to be deleted is
+        GIT_CREATE_MP_VIRTUAL_REF. So this method will only ever delete at
+        most 1 virtual ref. Once new virtual refs will be created, this method
+        should be extended to delete them also.
+        """
+        if self.source_git_ref is None:
+            return
+        if not getFeatureFlag(GIT_CREATE_MP_VIRTUAL_REF):
+            # XXX pappacena 2020-09-15: Do not try to remove virtual refs if
+            # the feature is disabled. It might be disabled due to bug on
+            # the first versions, for example, and we should have a way to
+            # skip this feature entirely.
+            # Once the feature is stable, we should actually *always* delete
+            # the virtual refs, since we don't know if the feature was
+            # enabled or not when the branch was created.
+            return
+        ref = GIT_MP_VIRTUAL_REF_FORMAT.format(mp=self)
+        if ref not in (except_refs or []):
+            hosting_client = getUtility(IGitHostingClient)
+            hosting_client.deleteRefs(
+                [(self.target_git_repository.getInternalPath(), ref)])
+
+    def syncGitVirtualRefs(self, force_delete=False):
+        """See `IBranchMergeProposal`"""
+        if force_delete:
+            # When force deletion of virtual refs, do not try to copy
+            # anything. Just run the delete step.
+            copied_refs = []
+        else:
+            copied_refs = self._copyGitVirtualRefs()
+        self._deleteGitVirtualRefs(except_refs=copied_refs)
+
+    @classmethod
+    def bulkSyncGitVirtualRefs(cls, git_repository, merge_proposals):
+        """See `IBranchMergeProposal`"""
+        copy_operations = []
+        for merge_proposal in merge_proposals:
+            copy_operations += merge_proposal.getGitRefCopyOperations()
+        if copy_operations:
+            hosting_client = getUtility(IGitHostingClient)
+            hosting_client.copyRefs(
+                git_repository.getInternalPath(), copy_operations)
+        return copy_operations
+
     def scheduleDiffUpdates(self, return_jobs=True):
         """See `IBranchMergeProposal`."""
         from lp.code.model.branchmergeproposaljob import (
diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
index 74f4ec0..5fe859b 100644
--- a/lib/lp/code/model/githosting.py
+++ b/lib/lp/code/model/githosting.py
@@ -33,7 +33,6 @@ from lp.code.errors import (
     GitRepositoryDeletionFault,
     GitRepositoryScanFault,
     GitTargetError,
-    NoSuchGitReference,
     )
 from lp.code.interfaces.githosting import IGitHostingClient
 from lp.services.config import config
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index fb5e703..13d67db 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -1184,9 +1184,12 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
     def updateLandingTargets(self, paths):
         """See `IGitRepository`."""
         jobs = []
-        for merge_proposal in self.getActiveLandingTargets(paths):
+        merge_proposals = self.getActiveLandingTargets(paths)
+        for merge_proposal in merge_proposals:
             jobs.extend(merge_proposal.scheduleDiffUpdates())
-        return jobs
+        copy_operations = BranchMergeProposal.bulkSyncGitVirtualRefs(
+            self, merge_proposals)
+        return jobs, copy_operations
 
     def _getRecipes(self, paths=None):
         """Undecorated version of recipes for use by `markRecipesStale`."""
diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
index 81b94b3..bdedc8b 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -67,6 +67,10 @@ from lp.code.interfaces.branchmergeproposal import (
     IBranchMergeProposalGetter,
     IBranchMergeProposalJobSource,
     )
+from lp.code.interfaces.gitrepository import (
+    GIT_CREATE_MP_VIRTUAL_REF,
+    GIT_MP_VIRTUAL_REF_FORMAT,
+    )
 from lp.code.model.branchmergeproposal import (
     BranchMergeProposal,
     BranchMergeProposalGetter,
@@ -97,6 +101,7 @@ from lp.testing import (
     ExpectedException,
     launchpadlib_for,
     login,
+    login_admin,
     login_person,
     person_logged_in,
     TestCaseWithFactory,
@@ -256,6 +261,137 @@ class TestBranchMergeProposalPrivacy(TestCaseWithFactory):
             self.assertContentEqual([owner, team], subscriptions)
 
 
+class TestGitBranchMergeProposalVirtualRefs(TestCaseWithFactory):
+    """Ensure that BranchMergeProposal creation run the appropriate copy
+    and delete of virtual refs, like ref/merge/<id>/head."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestGitBranchMergeProposalVirtualRefs, self).setUp()
+        self.hosting_fixture = self.useFixture(GitHostingFixture())
+
+    def test_copy_git_merge_virtual_ref(self):
+        self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+        mp = self.factory.makeBranchMergeProposalForGit()
+
+        copy_operations = mp.getGitRefCopyOperations()
+        self.assertEqual(1, len(copy_operations))
+        self.assertThat(copy_operations[0], MatchesStructure(
+            source_ref=Equals(mp.source_git_commit_sha1),
+            target_repo=Equals(mp.target_git_repository.getInternalPath()),
+            target_ref=Equals("refs/merge/%s/head" % mp.id),
+        ))
+
+        self.assertEqual(1, self.hosting_fixture.copyRefs.call_count)
+        args, kwargs = self.hosting_fixture.copyRefs.calls[0]
+        arg_path, arg_copy_operations = args
+        self.assertEqual({}, kwargs)
+        self.assertEqual(mp.source_git_repository.getInternalPath(), arg_path)
+        self.assertEqual(1, len(arg_copy_operations))
+        self.assertThat(arg_copy_operations[0], MatchesStructure(
+            source_ref=Equals(mp.source_git_commit_sha1),
+            target_repo=Equals(mp.target_git_repository.getInternalPath()),
+            target_ref=Equals("refs/merge/%s/head" % mp.id),
+        ))
+
+    def test_getGitRefCopyOperations_private_source(self):
+        self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+        login_admin()
+        source_repo = self.factory.makeGitRepository(
+            information_type=InformationType.PRIVATESECURITY)
+        target_repo = self.factory.makeGitRepository(target=source_repo.target)
+        [source] = self.factory.makeGitRefs(source_repo)
+        [target] = self.factory.makeGitRefs(target_repo)
+        mp = self.factory.makeBranchMergeProposalForGit(
+            source_ref=source, target_ref=target)
+        self.assertEqual([], mp.getGitRefCopyOperations())
+
+    def test_getGitRefCopyOperations_private_source_same_repo(self):
+        self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+        login_admin()
+        repo = self.factory.makeGitRepository(
+            information_type=InformationType.PRIVATESECURITY)
+        [source, target] = self.factory.makeGitRefs(
+            repo, ['refs/heads/bugfix', 'refs/heads/master'])
+        mp = self.factory.makeBranchMergeProposalForGit(
+            source_ref=source, target_ref=target)
+        operations = mp.getGitRefCopyOperations()
+        self.assertEqual(1, len(operations))
+        self.assertThat(operations[0], MatchesStructure(
+            source_ref=Equals(mp.source_git_commit_sha1),
+            target_repo=Equals(mp.target_git_repository.getInternalPath()),
+            target_ref=Equals(GIT_MP_VIRTUAL_REF_FORMAT.format(mp=mp))
+        ))
+
+    def test_getGitRefCopyOperations_private_source_same_owner(self):
+        self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+        login_admin()
+        source_repo = self.factory.makeGitRepository(
+            information_type=InformationType.PRIVATESECURITY)
+        target_repo = self.factory.makeGitRepository(
+            target=source_repo.target, owner=source_repo.owner)
+        [source] = self.factory.makeGitRefs(source_repo)
+        [target] = self.factory.makeGitRefs(target_repo)
+        mp = self.factory.makeBranchMergeProposalForGit(
+            source_ref=source, target_ref=target)
+        operations = mp.getGitRefCopyOperations()
+        self.assertEqual(1, len(operations))
+        self.assertThat(operations[0], MatchesStructure(
+            source_ref=Equals(mp.source_git_commit_sha1),
+            target_repo=Equals(mp.target_git_repository.getInternalPath()),
+            target_ref=Equals(GIT_MP_VIRTUAL_REF_FORMAT.format(mp=mp))
+        ))
+
+    def test_syncGitVirtualRefs(self):
+        self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+        login_admin()
+        login_admin()
+        source_repo = self.factory.makeGitRepository()
+        target_repo = self.factory.makeGitRepository(target=source_repo.target)
+        [source] = self.factory.makeGitRefs(source_repo)
+        [target] = self.factory.makeGitRefs(target_repo)
+        mp = self.factory.makeBranchMergeProposalForGit(
+            source_ref=source, target_ref=target)
+
+        # mp.syncGitVirtualRefs should have been triggered by event.
+        # Check lp.code.subscribers.branchmergeproposal.merge_proposal_created.
+        self.assertEqual(1, self.hosting_fixture.copyRefs.call_count)
+        args, kwargs = self.hosting_fixture.copyRefs.calls[0]
+        self.assertEquals({}, kwargs)
+        self.assertEqual(args[0], source_repo.getInternalPath())
+        self.assertEqual(1, len(args[1]))
+        self.assertThat(args[1][0], MatchesStructure(
+            source_ref=Equals(mp.source_git_commit_sha1),
+            target_repo=Equals(mp.target_git_repository.getInternalPath()),
+            target_ref=Equals("refs/merge/%s/head" % mp.id),
+        ))
+
+        self.assertEqual(0, self.hosting_fixture.deleteRefs.call_count)
+
+    def test_syncGitVirtualRefs_private_source_deletes_ref(self):
+        self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+        login_admin()
+        source_repo = self.factory.makeGitRepository(
+            information_type=InformationType.PRIVATESECURITY)
+        target_repo = self.factory.makeGitRepository(target=source_repo.target)
+        [source] = self.factory.makeGitRefs(source_repo)
+        [target] = self.factory.makeGitRefs(target_repo)
+        mp = self.factory.makeBranchMergeProposalForGit(
+            source_ref=source, target_ref=target)
+
+        # mp.syncGitVirtualRefs should have been triggered by event.
+        # Check lp.code.subscribers.branchmergeproposal.merge_proposal_created.
+        self.assertEqual(0, self.hosting_fixture.copyRefs.call_count)
+        self.assertEqual(1, self.hosting_fixture.deleteRefs.call_count)
+        args, kwargs = self.hosting_fixture.deleteRefs.calls[0]
+        self.assertEqual({}, kwargs)
+        self.assertEqual(
+            ([(target_repo.getInternalPath(),
+               "refs/merge/%s/head" % mp.id)], ),
+            args)
+
+
 class TestBranchMergeProposalTransitions(TestCaseWithFactory):
     """Test the state transitions of branch merge proposals."""
 
@@ -1185,6 +1321,10 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
     def test_delete_triggers_webhooks(self):
         # When an existing merge proposal is deleted, any relevant webhooks
         # are triggered.
+        self.useFixture(FeatureFixture({
+            GIT_CREATE_MP_VIRTUAL_REF: "on",
+            BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG: "on"}))
+        hosting_fixture = self.useFixture(GitHostingFixture())
         logger = self.useFixture(FakeLogger())
         source = self.makeBranch()
         target = self.makeBranch(same_target_as=source)
@@ -1205,8 +1345,11 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
             old=MatchesDict(self.getExpectedPayload(proposal, redact=True)))
         proposal.deleteProposal()
         delivery = hook.deliveries.one()
+        self.assertIsNotNone(delivery)
         self.assertCorrectDelivery(expected_payload, hook, delivery)
         self.assertCorrectLogging(expected_redacted_payload, hook, logger)
+        self.assertEqual(
+            1 if self.git else 0, hosting_fixture.deleteRefs.call_count)
 
 
 class TestGetAddress(TestCaseWithFactory):
@@ -1507,6 +1650,18 @@ class TestBranchMergeProposalDeletion(TestCaseWithFactory):
         self.assertRaises(
             SQLObjectNotFound, BranchMergeProposalJob.get, job_id)
 
+    def test_deleteProposal_for_git_removes_virtual_ref(self):
+        self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+        hosting_fixture = self.useFixture(GitHostingFixture())
+        proposal = self.factory.makeBranchMergeProposalForGit()
+        proposal.deleteProposal()
+
+        self.assertEqual(1, hosting_fixture.deleteRefs.call_count)
+        args = hosting_fixture.deleteRefs.calls[0]
+        self.assertEqual((
+            (([(proposal.target_git_repository.getInternalPath(),
+                'refs/merge/%s/head' % proposal.id)]), ), {}), args)
+
 
 class TestBranchMergeProposalBugs(WithVCSScenarios, TestCaseWithFactory):
 
diff --git a/lib/lp/code/model/tests/test_githosting.py b/lib/lp/code/model/tests/test_githosting.py
index 23d6063..5ca77d3 100644
--- a/lib/lp/code/model/tests/test_githosting.py
+++ b/lib/lp/code/model/tests/test_githosting.py
@@ -42,7 +42,6 @@ from lp.code.errors import (
     GitRepositoryDeletionFault,
     GitRepositoryScanFault,
     GitTargetError,
-    NoSuchGitReference,
     )
 from lp.code.interfaces.githosting import IGitHostingClient
 from lp.code.model.githosting import RefCopyOperation
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index b83f9da..53d9d57 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -97,6 +97,7 @@ from lp.code.interfaces.gitnamespace import (
     IGitNamespaceSet,
     )
 from lp.code.interfaces.gitrepository import (
+    GIT_CREATE_MP_VIRTUAL_REF,
     IGitRepository,
     IGitRepositorySet,
     IGitRepositoryView,
@@ -783,6 +784,7 @@ class TestGitRepositoryDeletion(TestCaseWithFactory):
         # Make sure that the tests all flush the database changes.
         self.addCleanup(Store.of(self.repository).flush)
         login_person(self.user)
+        self.hosting_fixture = self.useFixture(GitHostingFixture())
 
     def test_deletable(self):
         # A newly created repository can be deleted without any problems.
@@ -824,7 +826,6 @@ class TestGitRepositoryDeletion(TestCaseWithFactory):
 
     def test_code_import_does_not_disable_deletion(self):
         # A repository that has an attached code import can be deleted.
-        self.useFixture(GitHostingFixture())
         code_import = self.factory.makeCodeImport(
             target_rcs_type=TargetRevisionControlSystems.GIT)
         repository = code_import.git_repository
@@ -984,6 +985,7 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
         # unsubscribe the repository owner here.
         self.repository.unsubscribe(
             self.repository.owner, self.repository.owner)
+        self.hosting_fixture = self.useFixture(GitHostingFixture())
 
     def test_plain_repository(self):
         # A fresh repository has no deletion requirements.
@@ -1115,7 +1117,6 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
 
     def test_code_import_requirements(self):
         # Code imports are not included explicitly in deletion requirements.
-        self.useFixture(GitHostingFixture())
         code_import = self.factory.makeCodeImport(
             target_rcs_type=TargetRevisionControlSystems.GIT)
         # Remove the implicit repository subscription first.
@@ -1126,7 +1127,6 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
 
     def test_code_import_deletion(self):
         # break_references allows deleting a code import repository.
-        self.useFixture(GitHostingFixture())
         code_import = self.factory.makeCodeImport(
             target_rcs_type=TargetRevisionControlSystems.GIT)
         code_import_id = code_import.id
@@ -1182,7 +1182,6 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
 
     def test_DeleteCodeImport(self):
         # DeleteCodeImport.__call__ must delete the CodeImport.
-        self.useFixture(GitHostingFixture())
         code_import = self.factory.makeCodeImport(
             target_rcs_type=TargetRevisionControlSystems.GIT)
         code_import_id = code_import.id
@@ -1521,6 +1520,10 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
+    def setUp(self):
+        super(TestGitRepositoryRefs, self).setUp()
+        self.hosting_fixture = self.useFixture(GitHostingFixture())
+
     def test__convertRefInfo(self):
         # _convertRefInfo converts a valid info dictionary.
         sha1 = six.ensure_text(hashlib.sha1("").hexdigest())
@@ -1791,18 +1794,17 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
         # planRefChanges excludes some ref prefixes by default, and can be
         # configured otherwise.
         repository = self.factory.makeGitRepository()
-        hosting_fixture = self.useFixture(GitHostingFixture())
         repository.planRefChanges("dummy")
         self.assertEqual(
             [{"exclude_prefixes": ["refs/changes/"]}],
-            hosting_fixture.getRefs.extract_kwargs())
-        hosting_fixture.getRefs.calls = []
+            self.hosting_fixture.getRefs.extract_kwargs())
+        self.hosting_fixture.getRefs.calls = []
         self.pushConfig(
             "codehosting", git_exclude_ref_prefixes="refs/changes/ refs/pull/")
         repository.planRefChanges("dummy")
         self.assertEqual(
             [{"exclude_prefixes": ["refs/changes/", "refs/pull/"]}],
-            hosting_fixture.getRefs.extract_kwargs())
+            self.hosting_fixture.getRefs.extract_kwargs())
 
     def test_fetchRefCommits(self):
         # fetchRefCommits fetches detailed tip commit metadata for the
@@ -1875,9 +1877,8 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
     def test_fetchRefCommits_empty(self):
         # If given an empty refs dictionary, fetchRefCommits returns early
         # without contacting the hosting service.
-        hosting_fixture = self.useFixture(GitHostingFixture())
         GitRepository.fetchRefCommits("dummy", {})
-        self.assertEqual([], hosting_fixture.getCommits.calls)
+        self.assertEqual([], self.hosting_fixture.getCommits.calls)
 
     def test_synchroniseRefs(self):
         # synchroniseRefs copes with synchronising a repository where some
@@ -1920,7 +1921,6 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
         self.assertThat(repository.refs, MatchesSetwise(*matchers))
 
     def test_set_default_branch(self):
-        hosting_fixture = self.useFixture(GitHostingFixture())
         repository = self.factory.makeGitRepository()
         self.factory.makeGitRefs(
             repository=repository,
@@ -1931,22 +1931,20 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
         self.assertEqual(
             [((repository.getInternalPath(),),
              {"default_branch": "refs/heads/new"})],
-            hosting_fixture.setProperties.calls)
+            self.hosting_fixture.setProperties.calls)
         self.assertEqual("refs/heads/new", repository.default_branch)
 
     def test_set_default_branch_unchanged(self):
-        hosting_fixture = self.useFixture(GitHostingFixture())
         repository = self.factory.makeGitRepository()
         self.factory.makeGitRefs(
             repository=repository, paths=["refs/heads/master"])
         removeSecurityProxy(repository)._default_branch = "refs/heads/master"
         with person_logged_in(repository.owner):
             repository.default_branch = "master"
-        self.assertEqual([], hosting_fixture.setProperties.calls)
+        self.assertEqual([], self.hosting_fixture.setProperties.calls)
         self.assertEqual("refs/heads/master", repository.default_branch)
 
     def test_set_default_branch_imported(self):
-        hosting_fixture = self.useFixture(GitHostingFixture())
         repository = self.factory.makeGitRepository(
             repository_type=GitRepositoryType.IMPORTED)
         self.factory.makeGitRefs(
@@ -1959,7 +1957,7 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
                 "Cannot modify non-hosted Git repository %s." %
                     repository.display_name,
                 setattr, repository, "default_branch", "new")
-        self.assertEqual([], hosting_fixture.setProperties.calls)
+        self.assertEqual([], self.hosting_fixture.setProperties.calls)
         self.assertEqual("refs/heads/master", repository.default_branch)
 
     def test_exception_unset_default_branch(self):
@@ -2580,18 +2578,59 @@ class TestGitRepositoryUpdateLandingTargets(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
-    def test_schedules_diff_updates(self):
+    def setUp(self):
+        super(TestGitRepositoryUpdateLandingTargets, self).setUp()
+        self.hosting_fixture = self.useFixture(GitHostingFixture())
+
+    def assertSchedulesDiffUpdate(self, with_mp_virtual_ref):
         """Create jobs for all merge proposals."""
         bmp1 = self.factory.makeBranchMergeProposalForGit()
         bmp2 = self.factory.makeBranchMergeProposalForGit(
             source_ref=bmp1.source_git_ref)
-        jobs = bmp1.source_git_repository.updateLandingTargets(
+
+        # Only enable this virtual ref here, since
+        # self.factory.makeBranchMergeProposalForGit also tries to create
+        # the virtual refs.
+        if with_mp_virtual_ref:
+            self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+        else:
+            self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: ""}))
+        jobs, ref_copies = bmp1.source_git_repository.updateLandingTargets(
             [bmp1.source_git_path])
         self.assertEqual(2, len(jobs))
         bmps_to_update = [
             removeSecurityProxy(job).branch_merge_proposal for job in jobs]
         self.assertContentEqual([bmp1, bmp2], bmps_to_update)
 
+        if not with_mp_virtual_ref:
+            self.assertEqual(0, self.hosting_fixture.copyRefs.call_count)
+        else:
+            self.assertEqual(1, self.hosting_fixture.copyRefs.call_count)
+            args, kwargs = self.hosting_fixture.copyRefs.calls[0]
+            self.assertEqual({}, kwargs)
+            self.assertEqual(2, len(args))
+            path, operations = args
+            self.assertEqual(
+                path, bmp1.source_git_repository.getInternalPath())
+            self.assertThat(operations[0], MatchesStructure(
+                source_ref=Equals(bmp1.source_git_commit_sha1),
+                target_repo=Equals(
+                    bmp1.target_git_repository.getInternalPath()),
+                target_ref=Equals("refs/merge/%s/head" % bmp1.id),
+            ))
+            self.assertThat(operations[1], MatchesStructure(
+                source_ref=Equals(bmp2.source_git_commit_sha1),
+                target_repo=Equals(
+                    bmp2.target_git_repository.getInternalPath()),
+                target_ref=Equals("refs/merge/%s/head" % bmp2.id),
+            ))
+
+    def test_schedules_diff_updates_with_mp_virtual_ref(self):
+        self.assertSchedulesDiffUpdate(with_mp_virtual_ref=True)
+
+    def test_schedules_diff_updates_without_mp_virtual_ref(self):
+        self.assertSchedulesDiffUpdate(with_mp_virtual_ref=False)
+
     def test_ignores_final(self):
         """Diffs for proposals in final states aren't updated."""
         [source_ref] = self.factory.makeGitRefs()
@@ -2603,8 +2642,15 @@ class TestGitRepositoryUpdateLandingTargets(TestCaseWithFactory):
         for bmp in source_ref.landing_targets:
             if bmp.queue_status not in FINAL_STATES:
                 removeSecurityProxy(bmp).deleteProposal()
-        jobs = source_ref.repository.updateLandingTargets([source_ref.path])
+
+        # Enable the feature here, since factory.makeBranchMergeProposalForGit
+        # would also trigger the copy refs call.
+        self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+        jobs, ref_copies = source_ref.repository.updateLandingTargets(
+            [source_ref.path])
         self.assertEqual(0, len(jobs))
+        self.assertEqual(0, len(ref_copies))
+        self.assertEqual(0, self.hosting_fixture.copyRefs.call_count)
 
 
 class TestGitRepositoryMarkRecipesStale(TestCaseWithFactory):
diff --git a/lib/lp/code/subscribers/branchmergeproposal.py b/lib/lp/code/subscribers/branchmergeproposal.py
index a214c8f..a34ca2a 100644
--- a/lib/lp/code/subscribers/branchmergeproposal.py
+++ b/lib/lp/code/subscribers/branchmergeproposal.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Event subscribers for branch merge proposals."""
@@ -63,9 +63,13 @@ def _trigger_webhook(merge_proposal, payload):
 def merge_proposal_created(merge_proposal, event):
     """A new merge proposal has been created.
 
-    Create a job to update the diff for the merge proposal; trigger webhooks.
+    Create a job to update the diff for the merge proposal; trigger webhooks
+    and copy virtual refs as needed.
     """
     getUtility(IUpdatePreviewDiffJobSource).create(merge_proposal)
+
+    merge_proposal.syncGitVirtualRefs()
+
     if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG):
         payload = {
             "action": "created",
@@ -149,3 +153,5 @@ def merge_proposal_deleted(merge_proposal, event):
             "old": _compose_merge_proposal_webhook_payload(merge_proposal),
             }
         _trigger_webhook(merge_proposal, payload)
+
+    merge_proposal.syncGitVirtualRefs(force_delete=True)
diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py
index 4ef843d..e0ab275 100644
--- a/lib/lp/code/tests/helpers.py
+++ b/lib/lp/code/tests/helpers.py
@@ -369,6 +369,8 @@ class GitHostingFixture(fixtures.Fixture):
         self.getBlob = FakeMethod(result=blob)
         self.delete = FakeMethod()
         self.disable_memcache = disable_memcache
+        self.copyRefs = FakeMethod()
+        self.deleteRefs = FakeMethod()
 
     def _setUp(self):
         self.useFixture(ZopeUtilityFixture(self, IGitHostingClient))