launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/relax-personal-git-mp-restrictions into lp:launchpad.

Commit message:
Allow proposing merges between different branches of the same personal Git repository.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/relax-personal-git-mp-restrictions/+merge/349253
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/relax-personal-git-mp-restrictions into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/gitnamespace.py'
--- lib/lp/code/interfaces/gitnamespace.py	2016-10-14 17:25:51 +0000
+++ lib/lp/code/interfaces/gitnamespace.py	2018-07-10 11:44:04 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interface for a Git repository namespace."""
@@ -178,8 +178,12 @@
             already exists in the namespace.
         """
 
-    def areRepositoriesMergeable(other_namespace):
-        """Are repositories from other_namespace mergeable into this one?"""
+    def areRepositoriesMergeable(this, other):
+        """Is `other` mergeable into `this`?
+
+        :param this: An `IGitRepository` in this namespace.
+        :param other: An `IGitRepository` in either this or another namespace.
+        """
 
     collection = Attribute("An `IGitCollection` for this namespace.")
 

=== modified file 'lib/lp/code/model/gitnamespace.py'
--- lib/lp/code/model/gitnamespace.py	2018-05-14 08:01:56 +0000
+++ lib/lp/code/model/gitnamespace.py	2018-07-10 11:44:04 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementations of `IGitNamespace`."""
@@ -250,7 +250,7 @@
 
     has_defaults = False
     allow_push_to_set_default = False
-    supports_merge_proposals = False
+    supports_merge_proposals = True
     supports_code_imports = False
     allow_recipe_name_from_target = False
 
@@ -303,9 +303,12 @@
         else:
             return InformationType.PUBLIC
 
-    def areRepositoriesMergeable(self, other_namespace):
+    def areRepositoriesMergeable(self, this, other):
         """See `IGitNamespacePolicy`."""
-        return False
+        if this.namespace != self:
+            raise AssertionError(
+                "Namespace of %s is not %s." % (this.unique_name, self.name))
+        return this == other
 
     @property
     def collection(self):
@@ -383,12 +386,16 @@
             return None
         return default_type
 
-    def areRepositoriesMergeable(self, other_namespace):
+    def areRepositoriesMergeable(self, this, other):
         """See `IGitNamespacePolicy`."""
         # Repositories are mergeable into a project repository if the
         # project is the same.
         # XXX cjwatson 2015-04-18: Allow merging from a package repository
         # if any (active?) series is linked to this project.
+        if this.namespace != self:
+            raise AssertionError(
+                "Namespace of %s is not %s." % (this.unique_name, self.name))
+        other_namespace = other.namespace
         if zope_isinstance(other_namespace, ProjectGitNamespace):
             return self.target == other_namespace.target
         else:
@@ -457,12 +464,16 @@
         """See `IGitNamespace`."""
         return InformationType.PUBLIC
 
-    def areRepositoriesMergeable(self, other_namespace):
+    def areRepositoriesMergeable(self, this, other):
         """See `IGitNamespacePolicy`."""
         # Repositories are mergeable into a package repository if the
         # package is the same.
         # XXX cjwatson 2015-04-18: Allow merging from a project repository
         # if any (active?) series links this package to that project.
+        if this.namespace != self:
+            raise AssertionError(
+                "Namespace of %s is not %s." % (this.unique_name, self.name))
+        other_namespace = other.namespace
         if zope_isinstance(other_namespace, PackageGitNamespace):
             return self.target == other_namespace.target
         else:

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2017-11-24 17:22:34 +0000
+++ lib/lp/code/model/gitrepository.py	2018-07-10 11:44:04 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -946,7 +946,7 @@
 
     def isRepositoryMergeable(self, other):
         """See `IGitRepository`."""
-        return self.namespace.areRepositoriesMergeable(other.namespace)
+        return self.namespace.areRepositoriesMergeable(self, other)
 
     @property
     def pending_updates(self):

=== modified file 'lib/lp/code/model/tests/test_gitnamespace.py'
--- lib/lp/code/model/tests/test_gitnamespace.py	2017-10-04 01:29:35 +0000
+++ lib/lp/code/model/tests/test_gitnamespace.py	2018-07-10 11:44:04 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `IGitNamespace` implementations."""
@@ -278,6 +278,53 @@
         namespace = PersonalGitNamespace(person)
         self.assertEqual(person, namespace.target)
 
+    def test_supports_merge_proposals(self):
+        # Personal namespaces support merge proposals.
+        self.assertTrue(self.getNamespace().supports_merge_proposals)
+
+    def test_areRepositoriesMergeable_same_repository(self):
+        # A personal repository is mergeable into itself.
+        owner = self.factory.makePerson()
+        repository = self.factory.makeGitRepository(owner=owner, target=owner)
+        self.assertTrue(
+            repository.namespace.areRepositoriesMergeable(
+                repository, repository))
+
+    def test_areRepositoriesMergeable_same_namespace(self):
+        # A personal repository is not mergeable into another personal
+        # repository, even if they are in the same namespace.
+        owner = self.factory.makePerson()
+        this = self.factory.makeGitRepository(owner=owner, target=owner)
+        other = self.factory.makeGitRepository(owner=owner, target=owner)
+        self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
+
+    def test_areRepositoriesMergeable_different_namespace(self):
+        # A personal repository is not mergeable into another personal
+        # repository with a different namespace.
+        this_owner = self.factory.makePerson()
+        this = self.factory.makeGitRepository(
+            owner=this_owner, target=this_owner)
+        other_owner = self.factory.makePerson()
+        other = self.factory.makeGitRepository(
+            owner=other_owner, target=other_owner)
+        self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
+
+    def test_areRepositoriesMergeable_project(self):
+        # Project repositories are not mergeable into personal repositories.
+        owner = self.factory.makePerson()
+        this = self.factory.makeGitRepository(owner=owner, target=owner)
+        project = self.factory.makeProduct()
+        other = self.factory.makeGitRepository(owner=owner, target=project)
+        self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
+
+    def test_areRepositoriesMergeable_package(self):
+        # Package repositories are not mergeable into personal repositories.
+        owner = self.factory.makePerson()
+        this = self.factory.makeGitRepository(owner=owner, target=owner)
+        dsp = self.factory.makeDistributionSourcePackage()
+        other = self.factory.makeGitRepository(owner=owner, target=dsp)
+        self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
+
 
 class TestProjectGitNamespace(TestCaseWithFactory, NamespaceMixin):
     """Tests for `ProjectGitNamespace`."""
@@ -312,6 +359,50 @@
         namespace = ProjectGitNamespace(person, project)
         self.assertEqual(project, namespace.target)
 
+    def test_supports_merge_proposals(self):
+        # Project namespaces support merge proposals.
+        self.assertTrue(self.getNamespace().supports_merge_proposals)
+
+    def test_areRepositoriesMergeable_same_repository(self):
+        # A project repository is mergeable into itself.
+        project = self.factory.makeProduct()
+        repository = self.factory.makeGitRepository(target=project)
+        self.assertTrue(
+            repository.namespace.areRepositoriesMergeable(
+                repository, repository))
+
+    def test_areRepositoriesMergeable_same_namespace(self):
+        # Repositories of the same project are mergeable.
+        project = self.factory.makeProduct()
+        this = self.factory.makeGitRepository(target=project)
+        other = self.factory.makeGitRepository(target=project)
+        self.assertTrue(this.namespace.areRepositoriesMergeable(this, other))
+
+    def test_areRepositoriesMergeable_different_namespace(self):
+        # Repositories of a different project are not mergeable.
+        this_project = self.factory.makeProduct()
+        this = self.factory.makeGitRepository(target=this_project)
+        other_project = self.factory.makeProduct()
+        other = self.factory.makeGitRepository(target=other_project)
+        self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
+
+    def test_areRepositoriesMergeable_personal(self):
+        # Personal repositories are not mergeable into project repositories.
+        owner = self.factory.makePerson()
+        project = self.factory.makeProduct()
+        this = self.factory.makeGitRepository(owner=owner, target=project)
+        other = self.factory.makeGitRepository(owner=owner, target=owner)
+        self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
+
+    def test_areRepositoriesMergeable_package(self):
+        # Package repositories are not mergeable into project repositories.
+        owner = self.factory.makePerson()
+        project = self.factory.makeProduct()
+        this = self.factory.makeGitRepository(owner=owner, target=project)
+        dsp = self.factory.makeDistributionSourcePackage()
+        other = self.factory.makeGitRepository(owner=owner, target=dsp)
+        self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
+
 
 class TestProjectGitNamespacePrivacyWithInformationType(TestCaseWithFactory):
     """Tests for the privacy aspects of `ProjectGitNamespace`.
@@ -521,6 +612,50 @@
         namespace = PackageGitNamespace(person, dsp)
         self.assertEqual(dsp, namespace.target)
 
+    def test_supports_merge_proposals(self):
+        # Package namespaces support merge proposals.
+        self.assertTrue(self.getNamespace().supports_merge_proposals)
+
+    def test_areRepositoriesMergeable_same_repository(self):
+        # A package repository is mergeable into itself.
+        dsp = self.factory.makeDistributionSourcePackage()
+        repository = self.factory.makeGitRepository(target=dsp)
+        self.assertTrue(
+            repository.namespace.areRepositoriesMergeable(
+                repository, repository))
+
+    def test_areRepositoriesMergeable_same_namespace(self):
+        # Repositories of the same package are mergeable.
+        dsp = self.factory.makeDistributionSourcePackage()
+        this = self.factory.makeGitRepository(target=dsp)
+        other = self.factory.makeGitRepository(target=dsp)
+        self.assertTrue(this.namespace.areRepositoriesMergeable(this, other))
+
+    def test_areRepositoriesMergeable_different_namespace(self):
+        # Repositories of a different package are not mergeable.
+        this_dsp = self.factory.makeDistributionSourcePackage()
+        this = self.factory.makeGitRepository(target=this_dsp)
+        other_dsp = self.factory.makeDistributionSourcePackage()
+        other = self.factory.makeGitRepository(target=other_dsp)
+        self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
+
+    def test_areRepositoriesMergeable_personal(self):
+        # Personal repositories are not mergeable into package repositories.
+        owner = self.factory.makePerson()
+        dsp = self.factory.makeProduct()
+        this = self.factory.makeGitRepository(owner=owner, target=dsp)
+        other = self.factory.makeGitRepository(owner=owner, target=owner)
+        self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
+
+    def test_areRepositoriesMergeable_project(self):
+        # Project repositories are not mergeable into package repositories.
+        owner = self.factory.makePerson()
+        dsp = self.factory.makeDistributionSourcePackage()
+        this = self.factory.makeGitRepository(owner=owner, target=dsp)
+        project = self.factory.makeProduct()
+        other = self.factory.makeGitRepository(owner=owner, target=project)
+        self.assertFalse(this.namespace.areRepositoriesMergeable(this, other))
+
 
 class BaseCanCreateRepositoriesMixin:
     """Common tests for all namespaces."""

=== modified file 'lib/lp/code/model/tests/test_gitref.py'
--- lib/lp/code/model/tests/test_gitref.py	2017-10-04 01:29:35 +0000
+++ lib/lp/code/model/tests/test_gitref.py	2018-07-10 11:44:04 +0000
@@ -301,14 +301,25 @@
         else:
             self.assertEqual(review_type, vote.review_type)
 
-    def test_personal_source(self):
-        """Personal repositories cannot be used as a source for MPs."""
+    def test_personal_source_project_target(self):
+        """Personal repositories cannot be used as a source for MPs to
+        project repositories.
+        """
         self.source.repository.setTarget(
             target=self.source.owner, user=self.source.owner)
         self.assertRaises(
             InvalidBranchMergeProposal, self.source.addLandingTarget,
             self.user, self.target)
 
+    def test_personal_source_personal_target(self):
+        """A branch in a personal repository can be used as a source for MPs
+        to another branch in the same personal repository.
+        """
+        self.target.repository.setTarget(
+            target=self.target.owner, user=self.target.owner)
+        [source] = self.factory.makeGitRefs(repository=self.target.repository)
+        source.addLandingTarget(self.user, self.target)
+
     def test_target_repository_same_target(self):
         """The target repository's target must match that of the source."""
         self.target.repository.setTarget(