Launchpad itself

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

  1. Git
  2. lp:~pappacena/launchpad
  3. create-mp-refs
  4. Merge into master
Proposed by Thiago F. Pappacena
Status: Needs review
Proposed branch: ~pappacena/launchpad:create-mp-refs
Merge into: launchpad:master
Prerequisite: ~pappacena/launchpad:githosting-copy-and-delete-refs
Diff against target: 745 lines (+365/-27)
10 files modified
lib/lp/code/interfaces/branchmergeproposal.py (+26/-1)
lib/lp/code/interfaces/gitrepository.py (+12/-1)
lib/lp/code/model/branchmergeproposal.py (+92/-0)
lib/lp/code/model/githosting.py (+0/-1)
lib/lp/code/model/gitrepository.py (+5/-2)
lib/lp/code/model/tests/test_branchmergeproposal.py (+155/-0)
lib/lp/code/model/tests/test_githosting.py (+0/-1)
lib/lp/code/model/tests/test_gitrepository.py (+65/-19)
lib/lp/code/subscribers/branchmergeproposal.py (+8/-2)
lib/lp/code/tests/helpers.py (+2/-0)
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Review via email: mp+390581@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
William Grant (wgrant) wrote :

How does this interact with privacy?

Reply
Revision history for this message
Thiago F. Pappacena (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?

Reply
Revision history for this message
William Grant (wgrant) wrote :

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.

Reply
Revision history for this message
Thiago F. Pappacena (pappacena) wrote :

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?

Reply
Revision history for this message
Thiago F. Pappacena (pappacena) wrote :

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.

Reply
Revision history for this message
Thiago F. Pappacena (pappacena) wrote :

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.

Reply
Revision history for this message
Colin Watson (cjwatson) :
review: Approve
Reply
Revision history for this message
Colin Watson (cjwatson) wrote :

Also please set a commit message.

Reply
~pappacena/launchpad:create-mp-refs updated
4fae2e0... by Thiago F. Pappacena

Merge branch 'master' into create-mp-refs

4740bf9... by Thiago F. Pappacena

Methods renaming and better defining interfaces

297a039... by Thiago F. Pappacena

Refactoring

Revision history for this message
Thiago F. Pappacena (pappacena) wrote :

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.

Reply
~pappacena/launchpad:create-mp-refs updated
b4af659... by Thiago F. Pappacena

Merge branch 'master' into create-mp-refs

Unmerged commits

b4af659... by Thiago F. Pappacena

Merge branch 'master' into create-mp-refs

297a039... by Thiago F. Pappacena

Refactoring

4740bf9... by Thiago F. Pappacena

Methods renaming and better defining interfaces

4fae2e0... by Thiago F. Pappacena

Merge branch 'master' into create-mp-refs

f3bda00... by Thiago F. Pappacena

Fixing test

30c88c7... by Thiago F. Pappacena

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

7e11cb0... by Thiago F. Pappacena

Refactoring virtual refs creation and preparing privacy code

1f7f371... by Thiago F. Pappacena

Adding feature flag and triggering copy on MP creation

50eeb2e... by Thiago F. Pappacena

Adding feature flag to control mp virtual refs creation

84d5c5f... by Thiago F. Pappacena

Logging error

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/code/interfaces/branchmergeproposal.py b/lib/lp/code/interfaces/branchmergeproposal.py
2index bf481c9..449e921 100644
3--- a/lib/lp/code/interfaces/branchmergeproposal.py
4+++ b/lib/lp/code/interfaces/branchmergeproposal.py
5@@ -1,4 +1,4 @@
6-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
7+# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
8 # GNU Affero General Public License version 3 (see the file LICENSE).
9
10 """The interface for branch merge proposals."""
11@@ -519,6 +519,31 @@ class IBranchMergeProposalView(Interface):
12 def getLatestDiffUpdateJob():
13 """Return the latest IUpdatePreviewDiffJob for this MP."""
14
15+ def getGitRefCopyOperations():
16+ """Gets the list of GitHosting copy operations that should be done
17+ in order to keep this MP's virtual refs up-to-date.
18+
19+ :return: A list of RefCopyOperation objects.
20+ """
21+
22+ def syncGitVirtualRefs(force_delete=False):
23+ """Requests all copies and deletion of virtual refs to make git code
24+ hosting in sync with this MP.
25+
26+ :param force_delete: Do not try to copy any new virtual ref. Just
27+ delete all virtual refs possibly created.
28+ """
29+
30+ def bulkSyncGitVirtualRefs(git_repository, merge_proposals):
31+ """Synchronizes a set of merge proposals' virtual refs.
32+
33+ :params git_repository: The source git repository of the given merge
34+ proposals.
35+ :params merge_proposals: The set of merge proposals that should have
36+ the virtual refs synced.
37+ :return: A list of copy operations executed.
38+ """
39+
40
41 class IBranchMergeProposalEdit(Interface):
42
43diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
44index 192133a..48e7dd3 100644
45--- a/lib/lp/code/interfaces/gitrepository.py
46+++ b/lib/lp/code/interfaces/gitrepository.py
47@@ -8,6 +8,8 @@ __metaclass__ = type
48 __all__ = [
49 'ContributorGitIdentity',
50 'GitIdentityMixin',
51+ 'GIT_CREATE_MP_VIRTUAL_REF',
52+ 'GIT_MP_VIRTUAL_REF_FORMAT',
53 'GIT_REPOSITORY_NAME_VALIDATION_ERROR_MESSAGE',
54 'git_repository_name_validator',
55 'IGitRepository',
56@@ -105,6 +107,11 @@ valid_git_repository_name_pattern = re.compile(
57 r"^(?i)[a-z0-9][a-z0-9+\.\-@_]*\Z")
58
59
60+# Virtual ref where we automatically put a copy of any open merge proposal.
61+GIT_MP_VIRTUAL_REF_FORMAT = 'refs/merge/{mp.id}/head'
62+GIT_CREATE_MP_VIRTUAL_REF = 'git.mergeproposal_virtualref.enabled'
63+
64+
65 def valid_git_repository_name(name):
66 """Return True iff the name is valid as a Git repository name.
67
68@@ -595,11 +602,15 @@ class IGitRepositoryView(IHasRecipes):
69 def updateLandingTargets(paths):
70 """Update landing targets (MPs where this repository is the source).
71
72- For each merge proposal, create `UpdatePreviewDiffJob`s.
73+ For each merge proposal, create `UpdatePreviewDiffJob`s, and runs
74+ the appropriate GitHosting.copyRefs operation to allow having
75+ virtual merge refs with the updated branch version.
76
77 :param paths: A list of reference paths. Any merge proposals whose
78 source is this repository and one of these paths will have their
79 diffs updated.
80+ :return: Returns a tuple with the list of background jobs created,
81+ and the list of ref copies requested to GitHosting.
82 """
83
84 def markRecipesStale(paths):
85diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
86index 061fb1e..ec65e98 100644
87--- a/lib/lp/code/model/branchmergeproposal.py
88+++ b/lib/lp/code/model/branchmergeproposal.py
89@@ -12,6 +12,7 @@ __all__ = [
90
91 from collections import defaultdict
92 from email.utils import make_msgid
93+import logging
94 from operator import attrgetter
95 import sys
96
97@@ -84,7 +85,12 @@ from lp.code.interfaces.codereviewcomment import ICodeReviewComment
98 from lp.code.interfaces.codereviewinlinecomment import (
99 ICodeReviewInlineCommentSet,
100 )
101+from lp.code.interfaces.githosting import IGitHostingClient
102 from lp.code.interfaces.gitref import IGitRef
103+from lp.code.interfaces.gitrepository import (
104+ GIT_CREATE_MP_VIRTUAL_REF,
105+ GIT_MP_VIRTUAL_REF_FORMAT,
106+ )
107 from lp.code.mail.branch import RecipientReason
108 from lp.code.model.branchrevision import BranchRevision
109 from lp.code.model.codereviewcomment import CodeReviewComment
110@@ -94,6 +100,7 @@ from lp.code.model.diff import (
111 IncrementalDiff,
112 PreviewDiff,
113 )
114+from lp.code.model.githosting import RefCopyOperation
115 from lp.registry.interfaces.person import (
116 IPerson,
117 IPersonSet,
118@@ -123,6 +130,7 @@ from lp.services.database.sqlbase import (
119 quote,
120 SQLBase,
121 )
122+from lp.services.features import getFeatureFlag
123 from lp.services.helpers import shortlist
124 from lp.services.job.interfaces.job import JobStatus
125 from lp.services.job.model.job import Job
126@@ -144,6 +152,9 @@ from lp.soyuz.enums import (
127 )
128
129
130+logger = logging.getLogger(__name__)
131+
132+
133 def is_valid_transition(proposal, from_state, next_state, user=None):
134 """Is it valid for this user to move this proposal to to next_state?
135
136@@ -971,6 +982,7 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
137 branch_merge_proposal=self.id):
138 job.destroySelf()
139 self._preview_diffs.remove()
140+
141 self.destroySelf()
142
143 def getUnlandedSourceBranchRevisions(self):
144@@ -1217,6 +1229,86 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
145 for diff in diffs)
146 return [diff_dict.get(revisions) for revisions in revision_list]
147
148+ def getGitRefCopyOperations(self):
149+ """See `IBranchMergeProposal`"""
150+ if (not self.target_git_repository
151+ or not getFeatureFlag(GIT_CREATE_MP_VIRTUAL_REF)):
152+ return []
153+ # If the source repository is private and it's opening a MP to
154+ # another repository, we should not risk disclosing the private code to
155+ # everyone with permission to see the target repo:
156+ private_source = self.source_git_repository.private
157+ same_owner = self.source_git_repository.owner.inTeam(
158+ self.target_git_repository.owner)
159+ if private_source and not same_owner:
160+ return []
161+ return [RefCopyOperation(
162+ self.source_git_commit_sha1,
163+ self.target_git_repository.getInternalPath(),
164+ GIT_MP_VIRTUAL_REF_FORMAT.format(mp=self))]
165+
166+ def _copyGitVirtualRefs(self):
167+ """Requests virtual refs copy operations on GitHosting in order to
168+ keep them up-to-date with current MP's state.
169+
170+ :return: The list of copied refs.
171+ """
172+ copy_operations = self.getGitRefCopyOperations()
173+ if copy_operations:
174+ hosting_client = getUtility(IGitHostingClient)
175+ hosting_client.copyRefs(
176+ self.source_git_repository.getInternalPath(),
177+ copy_operations)
178+ return [i.target_ref for i in copy_operations]
179+
180+ def _deleteGitVirtualRefs(self, except_refs=None):
181+ """Deletes on git code hosting service all virtual refs, except
182+ those ones in the given list.
183+
184+ For now, the only existing virtual ref to be deleted is
185+ GIT_CREATE_MP_VIRTUAL_REF. So this method will only ever delete at
186+ most 1 virtual ref. Once new virtual refs will be created, this method
187+ should be extended to delete them also.
188+ """
189+ if self.source_git_ref is None:
190+ return
191+ if not getFeatureFlag(GIT_CREATE_MP_VIRTUAL_REF):
192+ # XXX pappacena 2020-09-15: Do not try to remove virtual refs if
193+ # the feature is disabled. It might be disabled due to bug on
194+ # the first versions, for example, and we should have a way to
195+ # skip this feature entirely.
196+ # Once the feature is stable, we should actually *always* delete
197+ # the virtual refs, since we don't know if the feature was
198+ # enabled or not when the branch was created.
199+ return
200+ ref = GIT_MP_VIRTUAL_REF_FORMAT.format(mp=self)
201+ if ref not in (except_refs or []):
202+ hosting_client = getUtility(IGitHostingClient)
203+ hosting_client.deleteRefs(
204+ [(self.target_git_repository.getInternalPath(), ref)])
205+
206+ def syncGitVirtualRefs(self, force_delete=False):
207+ """See `IBranchMergeProposal`"""
208+ if force_delete:
209+ # When force deletion of virtual refs, do not try to copy
210+ # anything. Just run the delete step.
211+ copied_refs = []
212+ else:
213+ copied_refs = self._copyGitVirtualRefs()
214+ self._deleteGitVirtualRefs(except_refs=copied_refs)
215+
216+ @classmethod
217+ def bulkSyncGitVirtualRefs(cls, git_repository, merge_proposals):
218+ """See `IBranchMergeProposal`"""
219+ copy_operations = []
220+ for merge_proposal in merge_proposals:
221+ copy_operations += merge_proposal.getGitRefCopyOperations()
222+ if copy_operations:
223+ hosting_client = getUtility(IGitHostingClient)
224+ hosting_client.copyRefs(
225+ git_repository.getInternalPath(), copy_operations)
226+ return copy_operations
227+
228 def scheduleDiffUpdates(self, return_jobs=True):
229 """See `IBranchMergeProposal`."""
230 from lp.code.model.branchmergeproposaljob import (
231diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
232index 74f4ec0..5fe859b 100644
233--- a/lib/lp/code/model/githosting.py
234+++ b/lib/lp/code/model/githosting.py
235@@ -33,7 +33,6 @@ from lp.code.errors import (
236 GitRepositoryDeletionFault,
237 GitRepositoryScanFault,
238 GitTargetError,
239- NoSuchGitReference,
240 )
241 from lp.code.interfaces.githosting import IGitHostingClient
242 from lp.services.config import config
243diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
244index fb5e703..13d67db 100644
245--- a/lib/lp/code/model/gitrepository.py
246+++ b/lib/lp/code/model/gitrepository.py
247@@ -1184,9 +1184,12 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
248 def updateLandingTargets(self, paths):
249 """See `IGitRepository`."""
250 jobs = []
251- for merge_proposal in self.getActiveLandingTargets(paths):
252+ merge_proposals = self.getActiveLandingTargets(paths)
253+ for merge_proposal in merge_proposals:
254 jobs.extend(merge_proposal.scheduleDiffUpdates())
255- return jobs
256+ copy_operations = BranchMergeProposal.bulkSyncGitVirtualRefs(
257+ self, merge_proposals)
258+ return jobs, copy_operations
259
260 def _getRecipes(self, paths=None):
261 """Undecorated version of recipes for use by `markRecipesStale`."""
262diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
263index 81b94b3..bdedc8b 100644
264--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
265+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
266@@ -67,6 +67,10 @@ from lp.code.interfaces.branchmergeproposal import (
267 IBranchMergeProposalGetter,
268 IBranchMergeProposalJobSource,
269 )
270+from lp.code.interfaces.gitrepository import (
271+ GIT_CREATE_MP_VIRTUAL_REF,
272+ GIT_MP_VIRTUAL_REF_FORMAT,
273+ )
274 from lp.code.model.branchmergeproposal import (
275 BranchMergeProposal,
276 BranchMergeProposalGetter,
277@@ -97,6 +101,7 @@ from lp.testing import (
278 ExpectedException,
279 launchpadlib_for,
280 login,
281+ login_admin,
282 login_person,
283 person_logged_in,
284 TestCaseWithFactory,
285@@ -256,6 +261,137 @@ class TestBranchMergeProposalPrivacy(TestCaseWithFactory):
286 self.assertContentEqual([owner, team], subscriptions)
287
288
289+class TestGitBranchMergeProposalVirtualRefs(TestCaseWithFactory):
290+ """Ensure that BranchMergeProposal creation run the appropriate copy
291+ and delete of virtual refs, like ref/merge/<id>/head."""
292+
293+ layer = DatabaseFunctionalLayer
294+
295+ def setUp(self):
296+ super(TestGitBranchMergeProposalVirtualRefs, self).setUp()
297+ self.hosting_fixture = self.useFixture(GitHostingFixture())
298+
299+ def test_copy_git_merge_virtual_ref(self):
300+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
301+ mp = self.factory.makeBranchMergeProposalForGit()
302+
303+ copy_operations = mp.getGitRefCopyOperations()
304+ self.assertEqual(1, len(copy_operations))
305+ self.assertThat(copy_operations[0], MatchesStructure(
306+ source_ref=Equals(mp.source_git_commit_sha1),
307+ target_repo=Equals(mp.target_git_repository.getInternalPath()),
308+ target_ref=Equals("refs/merge/%s/head" % mp.id),
309+ ))
310+
311+ self.assertEqual(1, self.hosting_fixture.copyRefs.call_count)
312+ args, kwargs = self.hosting_fixture.copyRefs.calls[0]
313+ arg_path, arg_copy_operations = args
314+ self.assertEqual({}, kwargs)
315+ self.assertEqual(mp.source_git_repository.getInternalPath(), arg_path)
316+ self.assertEqual(1, len(arg_copy_operations))
317+ self.assertThat(arg_copy_operations[0], MatchesStructure(
318+ source_ref=Equals(mp.source_git_commit_sha1),
319+ target_repo=Equals(mp.target_git_repository.getInternalPath()),
320+ target_ref=Equals("refs/merge/%s/head" % mp.id),
321+ ))
322+
323+ def test_getGitRefCopyOperations_private_source(self):
324+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
325+ login_admin()
326+ source_repo = self.factory.makeGitRepository(
327+ information_type=InformationType.PRIVATESECURITY)
328+ target_repo = self.factory.makeGitRepository(target=source_repo.target)
329+ [source] = self.factory.makeGitRefs(source_repo)
330+ [target] = self.factory.makeGitRefs(target_repo)
331+ mp = self.factory.makeBranchMergeProposalForGit(
332+ source_ref=source, target_ref=target)
333+ self.assertEqual([], mp.getGitRefCopyOperations())
334+
335+ def test_getGitRefCopyOperations_private_source_same_repo(self):
336+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
337+ login_admin()
338+ repo = self.factory.makeGitRepository(
339+ information_type=InformationType.PRIVATESECURITY)
340+ [source, target] = self.factory.makeGitRefs(
341+ repo, ['refs/heads/bugfix', 'refs/heads/master'])
342+ mp = self.factory.makeBranchMergeProposalForGit(
343+ source_ref=source, target_ref=target)
344+ operations = mp.getGitRefCopyOperations()
345+ self.assertEqual(1, len(operations))
346+ self.assertThat(operations[0], MatchesStructure(
347+ source_ref=Equals(mp.source_git_commit_sha1),
348+ target_repo=Equals(mp.target_git_repository.getInternalPath()),
349+ target_ref=Equals(GIT_MP_VIRTUAL_REF_FORMAT.format(mp=mp))
350+ ))
351+
352+ def test_getGitRefCopyOperations_private_source_same_owner(self):
353+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
354+ login_admin()
355+ source_repo = self.factory.makeGitRepository(
356+ information_type=InformationType.PRIVATESECURITY)
357+ target_repo = self.factory.makeGitRepository(
358+ target=source_repo.target, owner=source_repo.owner)
359+ [source] = self.factory.makeGitRefs(source_repo)
360+ [target] = self.factory.makeGitRefs(target_repo)
361+ mp = self.factory.makeBranchMergeProposalForGit(
362+ source_ref=source, target_ref=target)
363+ operations = mp.getGitRefCopyOperations()
364+ self.assertEqual(1, len(operations))
365+ self.assertThat(operations[0], MatchesStructure(
366+ source_ref=Equals(mp.source_git_commit_sha1),
367+ target_repo=Equals(mp.target_git_repository.getInternalPath()),
368+ target_ref=Equals(GIT_MP_VIRTUAL_REF_FORMAT.format(mp=mp))
369+ ))
370+
371+ def test_syncGitVirtualRefs(self):
372+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
373+ login_admin()
374+ login_admin()
375+ source_repo = self.factory.makeGitRepository()
376+ target_repo = self.factory.makeGitRepository(target=source_repo.target)
377+ [source] = self.factory.makeGitRefs(source_repo)
378+ [target] = self.factory.makeGitRefs(target_repo)
379+ mp = self.factory.makeBranchMergeProposalForGit(
380+ source_ref=source, target_ref=target)
381+
382+ # mp.syncGitVirtualRefs should have been triggered by event.
383+ # Check lp.code.subscribers.branchmergeproposal.merge_proposal_created.
384+ self.assertEqual(1, self.hosting_fixture.copyRefs.call_count)
385+ args, kwargs = self.hosting_fixture.copyRefs.calls[0]
386+ self.assertEquals({}, kwargs)
387+ self.assertEqual(args[0], source_repo.getInternalPath())
388+ self.assertEqual(1, len(args[1]))
389+ self.assertThat(args[1][0], MatchesStructure(
390+ source_ref=Equals(mp.source_git_commit_sha1),
391+ target_repo=Equals(mp.target_git_repository.getInternalPath()),
392+ target_ref=Equals("refs/merge/%s/head" % mp.id),
393+ ))
394+
395+ self.assertEqual(0, self.hosting_fixture.deleteRefs.call_count)
396+
397+ def test_syncGitVirtualRefs_private_source_deletes_ref(self):
398+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
399+ login_admin()
400+ source_repo = self.factory.makeGitRepository(
401+ information_type=InformationType.PRIVATESECURITY)
402+ target_repo = self.factory.makeGitRepository(target=source_repo.target)
403+ [source] = self.factory.makeGitRefs(source_repo)
404+ [target] = self.factory.makeGitRefs(target_repo)
405+ mp = self.factory.makeBranchMergeProposalForGit(
406+ source_ref=source, target_ref=target)
407+
408+ # mp.syncGitVirtualRefs should have been triggered by event.
409+ # Check lp.code.subscribers.branchmergeproposal.merge_proposal_created.
410+ self.assertEqual(0, self.hosting_fixture.copyRefs.call_count)
411+ self.assertEqual(1, self.hosting_fixture.deleteRefs.call_count)
412+ args, kwargs = self.hosting_fixture.deleteRefs.calls[0]
413+ self.assertEqual({}, kwargs)
414+ self.assertEqual(
415+ ([(target_repo.getInternalPath(),
416+ "refs/merge/%s/head" % mp.id)], ),
417+ args)
418+
419+
420 class TestBranchMergeProposalTransitions(TestCaseWithFactory):
421 """Test the state transitions of branch merge proposals."""
422
423@@ -1185,6 +1321,10 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
424 def test_delete_triggers_webhooks(self):
425 # When an existing merge proposal is deleted, any relevant webhooks
426 # are triggered.
427+ self.useFixture(FeatureFixture({
428+ GIT_CREATE_MP_VIRTUAL_REF: "on",
429+ BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG: "on"}))
430+ hosting_fixture = self.useFixture(GitHostingFixture())
431 logger = self.useFixture(FakeLogger())
432 source = self.makeBranch()
433 target = self.makeBranch(same_target_as=source)
434@@ -1205,8 +1345,11 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
435 old=MatchesDict(self.getExpectedPayload(proposal, redact=True)))
436 proposal.deleteProposal()
437 delivery = hook.deliveries.one()
438+ self.assertIsNotNone(delivery)
439 self.assertCorrectDelivery(expected_payload, hook, delivery)
440 self.assertCorrectLogging(expected_redacted_payload, hook, logger)
441+ self.assertEqual(
442+ 1 if self.git else 0, hosting_fixture.deleteRefs.call_count)
443
444
445 class TestGetAddress(TestCaseWithFactory):
446@@ -1507,6 +1650,18 @@ class TestBranchMergeProposalDeletion(TestCaseWithFactory):
447 self.assertRaises(
448 SQLObjectNotFound, BranchMergeProposalJob.get, job_id)
449
450+ def test_deleteProposal_for_git_removes_virtual_ref(self):
451+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
452+ hosting_fixture = self.useFixture(GitHostingFixture())
453+ proposal = self.factory.makeBranchMergeProposalForGit()
454+ proposal.deleteProposal()
455+
456+ self.assertEqual(1, hosting_fixture.deleteRefs.call_count)
457+ args = hosting_fixture.deleteRefs.calls[0]
458+ self.assertEqual((
459+ (([(proposal.target_git_repository.getInternalPath(),
460+ 'refs/merge/%s/head' % proposal.id)]), ), {}), args)
461+
462
463 class TestBranchMergeProposalBugs(WithVCSScenarios, TestCaseWithFactory):
464
465diff --git a/lib/lp/code/model/tests/test_githosting.py b/lib/lp/code/model/tests/test_githosting.py
466index 23d6063..5ca77d3 100644
467--- a/lib/lp/code/model/tests/test_githosting.py
468+++ b/lib/lp/code/model/tests/test_githosting.py
469@@ -42,7 +42,6 @@ from lp.code.errors import (
470 GitRepositoryDeletionFault,
471 GitRepositoryScanFault,
472 GitTargetError,
473- NoSuchGitReference,
474 )
475 from lp.code.interfaces.githosting import IGitHostingClient
476 from lp.code.model.githosting import RefCopyOperation
477diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
478index b83f9da..53d9d57 100644
479--- a/lib/lp/code/model/tests/test_gitrepository.py
480+++ b/lib/lp/code/model/tests/test_gitrepository.py
481@@ -97,6 +97,7 @@ from lp.code.interfaces.gitnamespace import (
482 IGitNamespaceSet,
483 )
484 from lp.code.interfaces.gitrepository import (
485+ GIT_CREATE_MP_VIRTUAL_REF,
486 IGitRepository,
487 IGitRepositorySet,
488 IGitRepositoryView,
489@@ -783,6 +784,7 @@ class TestGitRepositoryDeletion(TestCaseWithFactory):
490 # Make sure that the tests all flush the database changes.
491 self.addCleanup(Store.of(self.repository).flush)
492 login_person(self.user)
493+ self.hosting_fixture = self.useFixture(GitHostingFixture())
494
495 def test_deletable(self):
496 # A newly created repository can be deleted without any problems.
497@@ -824,7 +826,6 @@ class TestGitRepositoryDeletion(TestCaseWithFactory):
498
499 def test_code_import_does_not_disable_deletion(self):
500 # A repository that has an attached code import can be deleted.
501- self.useFixture(GitHostingFixture())
502 code_import = self.factory.makeCodeImport(
503 target_rcs_type=TargetRevisionControlSystems.GIT)
504 repository = code_import.git_repository
505@@ -984,6 +985,7 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
506 # unsubscribe the repository owner here.
507 self.repository.unsubscribe(
508 self.repository.owner, self.repository.owner)
509+ self.hosting_fixture = self.useFixture(GitHostingFixture())
510
511 def test_plain_repository(self):
512 # A fresh repository has no deletion requirements.
513@@ -1115,7 +1117,6 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
514
515 def test_code_import_requirements(self):
516 # Code imports are not included explicitly in deletion requirements.
517- self.useFixture(GitHostingFixture())
518 code_import = self.factory.makeCodeImport(
519 target_rcs_type=TargetRevisionControlSystems.GIT)
520 # Remove the implicit repository subscription first.
521@@ -1126,7 +1127,6 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
522
523 def test_code_import_deletion(self):
524 # break_references allows deleting a code import repository.
525- self.useFixture(GitHostingFixture())
526 code_import = self.factory.makeCodeImport(
527 target_rcs_type=TargetRevisionControlSystems.GIT)
528 code_import_id = code_import.id
529@@ -1182,7 +1182,6 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
530
531 def test_DeleteCodeImport(self):
532 # DeleteCodeImport.__call__ must delete the CodeImport.
533- self.useFixture(GitHostingFixture())
534 code_import = self.factory.makeCodeImport(
535 target_rcs_type=TargetRevisionControlSystems.GIT)
536 code_import_id = code_import.id
537@@ -1521,6 +1520,10 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
538
539 layer = DatabaseFunctionalLayer
540
541+ def setUp(self):
542+ super(TestGitRepositoryRefs, self).setUp()
543+ self.hosting_fixture = self.useFixture(GitHostingFixture())
544+
545 def test__convertRefInfo(self):
546 # _convertRefInfo converts a valid info dictionary.
547 sha1 = six.ensure_text(hashlib.sha1("").hexdigest())
548@@ -1791,18 +1794,17 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
549 # planRefChanges excludes some ref prefixes by default, and can be
550 # configured otherwise.
551 repository = self.factory.makeGitRepository()
552- hosting_fixture = self.useFixture(GitHostingFixture())
553 repository.planRefChanges("dummy")
554 self.assertEqual(
555 [{"exclude_prefixes": ["refs/changes/"]}],
556- hosting_fixture.getRefs.extract_kwargs())
557- hosting_fixture.getRefs.calls = []
558+ self.hosting_fixture.getRefs.extract_kwargs())
559+ self.hosting_fixture.getRefs.calls = []
560 self.pushConfig(
561 "codehosting", git_exclude_ref_prefixes="refs/changes/ refs/pull/")
562 repository.planRefChanges("dummy")
563 self.assertEqual(
564 [{"exclude_prefixes": ["refs/changes/", "refs/pull/"]}],
565- hosting_fixture.getRefs.extract_kwargs())
566+ self.hosting_fixture.getRefs.extract_kwargs())
567
568 def test_fetchRefCommits(self):
569 # fetchRefCommits fetches detailed tip commit metadata for the
570@@ -1875,9 +1877,8 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
571 def test_fetchRefCommits_empty(self):
572 # If given an empty refs dictionary, fetchRefCommits returns early
573 # without contacting the hosting service.
574- hosting_fixture = self.useFixture(GitHostingFixture())
575 GitRepository.fetchRefCommits("dummy", {})
576- self.assertEqual([], hosting_fixture.getCommits.calls)
577+ self.assertEqual([], self.hosting_fixture.getCommits.calls)
578
579 def test_synchroniseRefs(self):
580 # synchroniseRefs copes with synchronising a repository where some
581@@ -1920,7 +1921,6 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
582 self.assertThat(repository.refs, MatchesSetwise(*matchers))
583
584 def test_set_default_branch(self):
585- hosting_fixture = self.useFixture(GitHostingFixture())
586 repository = self.factory.makeGitRepository()
587 self.factory.makeGitRefs(
588 repository=repository,
589@@ -1931,22 +1931,20 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
590 self.assertEqual(
591 [((repository.getInternalPath(),),
592 {"default_branch": "refs/heads/new"})],
593- hosting_fixture.setProperties.calls)
594+ self.hosting_fixture.setProperties.calls)
595 self.assertEqual("refs/heads/new", repository.default_branch)
596
597 def test_set_default_branch_unchanged(self):
598- hosting_fixture = self.useFixture(GitHostingFixture())
599 repository = self.factory.makeGitRepository()
600 self.factory.makeGitRefs(
601 repository=repository, paths=["refs/heads/master"])
602 removeSecurityProxy(repository)._default_branch = "refs/heads/master"
603 with person_logged_in(repository.owner):
604 repository.default_branch = "master"
605- self.assertEqual([], hosting_fixture.setProperties.calls)
606+ self.assertEqual([], self.hosting_fixture.setProperties.calls)
607 self.assertEqual("refs/heads/master", repository.default_branch)
608
609 def test_set_default_branch_imported(self):
610- hosting_fixture = self.useFixture(GitHostingFixture())
611 repository = self.factory.makeGitRepository(
612 repository_type=GitRepositoryType.IMPORTED)
613 self.factory.makeGitRefs(
614@@ -1959,7 +1957,7 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
615 "Cannot modify non-hosted Git repository %s." %
616 repository.display_name,
617 setattr, repository, "default_branch", "new")
618- self.assertEqual([], hosting_fixture.setProperties.calls)
619+ self.assertEqual([], self.hosting_fixture.setProperties.calls)
620 self.assertEqual("refs/heads/master", repository.default_branch)
621
622 def test_exception_unset_default_branch(self):
623@@ -2580,18 +2578,59 @@ class TestGitRepositoryUpdateLandingTargets(TestCaseWithFactory):
624
625 layer = DatabaseFunctionalLayer
626
627- def test_schedules_diff_updates(self):
628+ def setUp(self):
629+ super(TestGitRepositoryUpdateLandingTargets, self).setUp()
630+ self.hosting_fixture = self.useFixture(GitHostingFixture())
631+
632+ def assertSchedulesDiffUpdate(self, with_mp_virtual_ref):
633 """Create jobs for all merge proposals."""
634 bmp1 = self.factory.makeBranchMergeProposalForGit()
635 bmp2 = self.factory.makeBranchMergeProposalForGit(
636 source_ref=bmp1.source_git_ref)
637- jobs = bmp1.source_git_repository.updateLandingTargets(
638+
639+ # Only enable this virtual ref here, since
640+ # self.factory.makeBranchMergeProposalForGit also tries to create
641+ # the virtual refs.
642+ if with_mp_virtual_ref:
643+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
644+ else:
645+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: ""}))
646+ jobs, ref_copies = bmp1.source_git_repository.updateLandingTargets(
647 [bmp1.source_git_path])
648 self.assertEqual(2, len(jobs))
649 bmps_to_update = [
650 removeSecurityProxy(job).branch_merge_proposal for job in jobs]
651 self.assertContentEqual([bmp1, bmp2], bmps_to_update)
652
653+ if not with_mp_virtual_ref:
654+ self.assertEqual(0, self.hosting_fixture.copyRefs.call_count)
655+ else:
656+ self.assertEqual(1, self.hosting_fixture.copyRefs.call_count)
657+ args, kwargs = self.hosting_fixture.copyRefs.calls[0]
658+ self.assertEqual({}, kwargs)
659+ self.assertEqual(2, len(args))
660+ path, operations = args
661+ self.assertEqual(
662+ path, bmp1.source_git_repository.getInternalPath())
663+ self.assertThat(operations[0], MatchesStructure(
664+ source_ref=Equals(bmp1.source_git_commit_sha1),
665+ target_repo=Equals(
666+ bmp1.target_git_repository.getInternalPath()),
667+ target_ref=Equals("refs/merge/%s/head" % bmp1.id),
668+ ))
669+ self.assertThat(operations[1], MatchesStructure(
670+ source_ref=Equals(bmp2.source_git_commit_sha1),
671+ target_repo=Equals(
672+ bmp2.target_git_repository.getInternalPath()),
673+ target_ref=Equals("refs/merge/%s/head" % bmp2.id),
674+ ))
675+
676+ def test_schedules_diff_updates_with_mp_virtual_ref(self):
677+ self.assertSchedulesDiffUpdate(with_mp_virtual_ref=True)
678+
679+ def test_schedules_diff_updates_without_mp_virtual_ref(self):
680+ self.assertSchedulesDiffUpdate(with_mp_virtual_ref=False)
681+
682 def test_ignores_final(self):
683 """Diffs for proposals in final states aren't updated."""
684 [source_ref] = self.factory.makeGitRefs()
685@@ -2603,8 +2642,15 @@ class TestGitRepositoryUpdateLandingTargets(TestCaseWithFactory):
686 for bmp in source_ref.landing_targets:
687 if bmp.queue_status not in FINAL_STATES:
688 removeSecurityProxy(bmp).deleteProposal()
689- jobs = source_ref.repository.updateLandingTargets([source_ref.path])
690+
691+ # Enable the feature here, since factory.makeBranchMergeProposalForGit
692+ # would also trigger the copy refs call.
693+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
694+ jobs, ref_copies = source_ref.repository.updateLandingTargets(
695+ [source_ref.path])
696 self.assertEqual(0, len(jobs))
697+ self.assertEqual(0, len(ref_copies))
698+ self.assertEqual(0, self.hosting_fixture.copyRefs.call_count)
699
700
701 class TestGitRepositoryMarkRecipesStale(TestCaseWithFactory):
702diff --git a/lib/lp/code/subscribers/branchmergeproposal.py b/lib/lp/code/subscribers/branchmergeproposal.py
703index a214c8f..a34ca2a 100644
704--- a/lib/lp/code/subscribers/branchmergeproposal.py
705+++ b/lib/lp/code/subscribers/branchmergeproposal.py
706@@ -1,4 +1,4 @@
707-# Copyright 2010-2016 Canonical Ltd. This software is licensed under the
708+# Copyright 2010-2020 Canonical Ltd. This software is licensed under the
709 # GNU Affero General Public License version 3 (see the file LICENSE).
710
711 """Event subscribers for branch merge proposals."""
712@@ -63,9 +63,13 @@ def _trigger_webhook(merge_proposal, payload):
713 def merge_proposal_created(merge_proposal, event):
714 """A new merge proposal has been created.
715
716- Create a job to update the diff for the merge proposal; trigger webhooks.
717+ Create a job to update the diff for the merge proposal; trigger webhooks
718+ and copy virtual refs as needed.
719 """
720 getUtility(IUpdatePreviewDiffJobSource).create(merge_proposal)
721+
722+ merge_proposal.syncGitVirtualRefs()
723+
724 if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG):
725 payload = {
726 "action": "created",
727@@ -149,3 +153,5 @@ def merge_proposal_deleted(merge_proposal, event):
728 "old": _compose_merge_proposal_webhook_payload(merge_proposal),
729 }
730 _trigger_webhook(merge_proposal, payload)
731+
732+ merge_proposal.syncGitVirtualRefs(force_delete=True)
733diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py
734index 4ef843d..e0ab275 100644
735--- a/lib/lp/code/tests/helpers.py
736+++ b/lib/lp/code/tests/helpers.py
737@@ -369,6 +369,8 @@ class GitHostingFixture(fixtures.Fixture):
738 self.getBlob = FakeMethod(result=blob)
739 self.delete = FakeMethod()
740 self.disable_memcache = disable_memcache
741+ self.copyRefs = FakeMethod()
742+ self.deleteRefs = FakeMethod()
743
744 def _setUp(self):
745 self.useFixture(ZopeUtilityFixture(self, IGitHostingClient))