launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:create-mp-refs into launchpad:master with ~pappacena/launchpad:githosting-copy-and-delete-refs as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/390581
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:create-mp-refs into launchpad:master.

diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index 344bc43..1251996 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -8,6 +8,7 @@ __metaclass__ = type
 __all__ = [
     'ContributorGitIdentity',
     'GitIdentityMixin',
+    'GIT_MP_VIRTUAL_REF_FORMAT',
     'GIT_REPOSITORY_NAME_VALIDATION_ERROR_MESSAGE',
     'git_repository_name_validator',
     'IGitRepository',
@@ -105,6 +106,10 @@ 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'
+
+
 def valid_git_repository_name(name):
     """Return True iff the name is valid as a Git repository name.
 
@@ -595,11 +600,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 lits of refs copy requested to GitHosting.
         """
 
     def markRecipesStale(paths):
diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index 338cc29..4287885 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
 
@@ -63,6 +64,7 @@ from lp.code.errors import (
     BadStateTransition,
     BranchMergeProposalExists,
     DiffNotFound,
+    GitReferenceDeletionFault,
     UserNotBranchReviewer,
     WrongBranchMergeProposal,
     )
@@ -82,7 +84,9 @@ 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_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
@@ -142,6 +146,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?
 
@@ -958,6 +965,17 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
             branch_merge_proposal=self.id):
             job.destroySelf()
         self._preview_diffs.remove()
+
+        # For git merge proposals, delete the virtual refs
+        if self.source_git_ref is not None:
+            ref = GIT_MP_VIRTUAL_REF_FORMAT.format(mp=self)
+            hosting_client = getUtility(IGitHostingClient)
+            try:
+                hosting_client.deleteRef(
+                    self.target_git_repository.getInternalPath(), ref)
+            except GitReferenceDeletionFault as e:
+                logger.error("Could not delete MP virtual ref: %s", e)
+
         self.destroySelf()
 
     def getUnlandedSourceBranchRevisions(self):
diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
index b957574..5383195 100644
--- a/lib/lp/code/model/githosting.py
+++ b/lib/lp/code/model/githosting.py
@@ -32,7 +32,6 @@ from lp.code.errors import (
     GitRepositoryDeletionFault,
     GitRepositoryScanFault,
     GitTargetError,
-    NoSuchGitReference,
     )
 from lp.code.interfaces.githosting import IGitHostingClient
 from lp.services.config import config
@@ -59,6 +58,17 @@ class RefCopyOperation:
         self.target_repo = target_repo
         self.target_ref = target_ref
 
+    def __eq__(self, other):
+        if not isinstance(other, self.__class__):
+            return False
+        return (self.source_ref == other.source_ref and
+                self.target_repo == other.target_repo and
+                self.target_ref == other.target_ref)
+
+    def __str__(self):
+        return "RefCopyOperation from %s to %s:%s" % (
+            self.source_ref, self.target_repo, self.target_ref)
+
 
 @implementer(IGitHostingClient)
 class GitHostingClient:
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 77a4bbd..2ab7efc 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -123,6 +123,7 @@ from lp.code.interfaces.gitnamespace import (
     IGitNamespacePolicy,
     )
 from lp.code.interfaces.gitrepository import (
+    GIT_MP_VIRTUAL_REF_FORMAT,
     GitIdentityMixin,
     IGitRepository,
     IGitRepositorySet,
@@ -136,6 +137,7 @@ from lp.code.interfaces.revision import IRevisionSet
 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.githosting import RefCopyOperation
 from lp.code.model.gitref import (
     GitRef,
     GitRefDefault,
@@ -1160,9 +1162,19 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
     def updateLandingTargets(self, paths):
         """See `IGitRepository`."""
         jobs = []
+        copy_operations = []
         for merge_proposal in self.getActiveLandingTargets(paths):
             jobs.extend(merge_proposal.scheduleDiffUpdates())
-        return jobs
+            copy = RefCopyOperation(
+                merge_proposal.source_git_commit_sha1,
+                merge_proposal.target_git_repository.getInternalPath(),
+                GIT_MP_VIRTUAL_REF_FORMAT.format(mp=merge_proposal))
+            copy_operations.append(copy)
+
+        if copy_operations:
+            hosting_client = getUtility(IGitHostingClient)
+            hosting_client.copyRefs(self.getInternalPath(), copy_operations)
+        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 0d2457d..729f569 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -1184,6 +1184,7 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
     def test_delete_triggers_webhooks(self):
         # When an existing merge proposal is deleted, any relevant webhooks
         # are triggered.
+        hosting_fixture = self.useFixture(GitHostingFixture())
         logger = self.useFixture(FakeLogger())
         source = self.makeBranch()
         target = self.makeBranch(same_target_as=source)
@@ -1206,6 +1207,8 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
         delivery = hook.deliveries.one()
         self.assertCorrectDelivery(expected_payload, hook, delivery)
         self.assertCorrectLogging(expected_redacted_payload, hook, logger)
+        self.assertEqual(
+            1 if self.git else 0, hosting_fixture.deleteRef.call_count)
 
 
 class TestGetAddress(TestCaseWithFactory):
@@ -1506,6 +1509,17 @@ class TestBranchMergeProposalDeletion(TestCaseWithFactory):
         self.assertRaises(
             SQLObjectNotFound, BranchMergeProposalJob.get, job_id)
 
+    def test_deleteProposal_for_git_removes_virtual_ref(self):
+        hosting_fixture = self.useFixture(GitHostingFixture())
+        proposal = self.factory.makeBranchMergeProposalForGit()
+        proposal.deleteProposal()
+
+        self.assertEqual(1, hosting_fixture.deleteRef.call_count)
+        args = hosting_fixture.deleteRef.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_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index a8b9c35..13e9a5d 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -113,6 +113,7 @@ from lp.code.model.branchmergeproposaljob import (
     )
 from lp.code.model.codereviewcomment import CodeReviewComment
 from lp.code.model.gitactivity import GitActivity
+from lp.code.model.githosting import RefCopyOperation
 from lp.code.model.gitjob import (
     GitJob,
     GitJobType,
@@ -782,6 +783,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.
@@ -823,7 +825,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
@@ -983,6 +984,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.
@@ -1114,7 +1116,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.
@@ -1125,7 +1126,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
@@ -1181,7 +1181,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
@@ -1520,6 +1519,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 = unicode(hashlib.sha1("").hexdigest())
@@ -1788,18 +1791,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
@@ -1871,9 +1873,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
@@ -1916,7 +1917,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,
@@ -1927,22 +1927,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(
@@ -1955,7 +1953,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):
@@ -2576,18 +2574,33 @@ class TestGitRepositoryUpdateLandingTargets(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
+    def setUp(self):
+        super(TestGitRepositoryUpdateLandingTargets, self).setUp()
+        self.hosting_fixture = self.useFixture(GitHostingFixture())
+
     def test_schedules_diff_updates(self):
         """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(
+        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)
 
+        self.assertEqual(1, self.hosting_fixture.copyRefs.call_count)
+        self.assertEqual((
+            (bmp1.source_git_repository.getInternalPath(), [
+                RefCopyOperation(bmp1.source_git_commit_sha1,
+                                 bmp1.target_git_repository.getInternalPath(),
+                                 "refs/merge/%s/head" % bmp1.id),
+                RefCopyOperation(bmp2.source_git_commit_sha1,
+                                 bmp2.target_git_repository.getInternalPath(),
+                                 "refs/merge/%s/head" % bmp2.id),
+            ]), {}), self.hosting_fixture.copyRefs.calls[0])
+
     def test_ignores_final(self):
         """Diffs for proposals in final states aren't updated."""
         [source_ref] = self.factory.makeGitRefs()
@@ -2599,8 +2612,11 @@ 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])
+        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/tests/helpers.py b/lib/lp/code/tests/helpers.py
index 0e784f0..90a7a87 100644
--- a/lib/lp/code/tests/helpers.py
+++ b/lib/lp/code/tests/helpers.py
@@ -368,6 +368,8 @@ class GitHostingFixture(fixtures.Fixture):
         self.getBlob = FakeMethod(result=blob)
         self.delete = FakeMethod()
         self.disable_memcache = disable_memcache
+        self.copyRefs = FakeMethod()
+        self.deleteRef = FakeMethod()
 
     def _setUp(self):
         self.useFixture(ZopeUtilityFixture(self, IGitHostingClient))