launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:git-delete-self-merges into launchpad:master.

Commit message:
Fix deletion of Git repositories with internal MPs

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

If there exists a merge proposal internal to a repository (that is, both source and target references are branches in that repository), then deleting the repository will fail because it tries to delete the merge proposal twice.  Handle this case.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:git-delete-self-merges into launchpad:master.

diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 416a4f3..bc9785a 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -1496,29 +1496,36 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
         """
         alteration_operations = []
         deletion_operations = []
+        seen_merge_proposal_ids = set()
         # Merge proposals require their source and target repositories to
         # exist.
         for merge_proposal in self.landing_targets:
-            deletion_operations.append(
-                DeletionCallable(
-                    merge_proposal,
-                    msg("This repository is the source repository of this "
-                        "merge proposal."),
-                    merge_proposal.deleteProposal))
+            if merge_proposal.id not in seen_merge_proposal_ids:
+                deletion_operations.append(
+                    DeletionCallable(
+                        merge_proposal,
+                        msg("This repository is the source repository of this "
+                            "merge proposal."),
+                        merge_proposal.deleteProposal))
+                seen_merge_proposal_ids.add(merge_proposal.id)
         # Cannot use self.landing_candidates, because it ignores merged
         # merge proposals.
         for merge_proposal in BranchMergeProposal.selectBy(
-            target_git_repository=self):
-            deletion_operations.append(
-                DeletionCallable(
-                    merge_proposal,
-                    msg("This repository is the target repository of this "
-                        "merge proposal."),
-                    merge_proposal.deleteProposal))
+                target_git_repository=self):
+            if merge_proposal.id not in seen_merge_proposal_ids:
+                deletion_operations.append(
+                    DeletionCallable(
+                        merge_proposal,
+                        msg("This repository is the target repository of this "
+                            "merge proposal."),
+                        merge_proposal.deleteProposal))
+                seen_merge_proposal_ids.add(merge_proposal.id)
         for merge_proposal in BranchMergeProposal.selectBy(
-            prerequisite_git_repository=self):
-            alteration_operations.append(
-                ClearPrerequisiteRepository(merge_proposal))
+                prerequisite_git_repository=self):
+            if merge_proposal.id not in seen_merge_proposal_ids:
+                alteration_operations.append(
+                    ClearPrerequisiteRepository(merge_proposal))
+                seen_merge_proposal_ids.add(merge_proposal.id)
         recipes = self.recipes if eager_load else self._getRecipes()
         deletion_operations.extend(
             DeletionCallable(
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 165980b..2917c4e 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -878,6 +878,17 @@ class TestGitRepositoryDeletion(TestCaseWithFactory):
         )
         self.repository.destroySelf(break_references=True)
 
+    def test_destroySelf_with_merge_proposal_within_self(self):
+        # Deletion works if the repository has a merge proposal from one
+        # branch to another within itself.
+        [source_ref] = self.factory.makeGitRefs(repository=self.repository)
+        [prerequisite_ref] = self.factory.makeGitRefs(
+            repository=self.repository)
+        self.factory.makeBranchMergeProposalForGit(
+            registrant=self.user, target_ref=self.ref,
+            prerequisite_ref=prerequisite_ref, source_ref=source_ref)
+        self.repository.destroySelf(break_references=True)
+
     def test_related_webhooks_deleted(self):
         webhook = self.factory.makeWebhook(target=self.repository)
         webhook.ping()