launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:git-fork-non-default-repo into launchpad:master.

Commit message:
Always fork the repository we're told to fork

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1901579 in Launchpad itself: "Forking a non-default repository forks the default repository instead"
  https://bugs.launchpad.net/launchpad/+bug/1901579

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

Also adjust the logic for creating the forked repository as owner-default: this only makes sense if the origin repository was target-default or owner-default to begin with.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:git-fork-non-default-repo into launchpad:master.

diff --git a/lib/lp/code/interfaces/gitnamespace.py b/lib/lp/code/interfaces/gitnamespace.py
index f4ff75a..ed0a4d3 100644
--- a/lib/lp/code/interfaces/gitnamespace.py
+++ b/lib/lp/code/interfaces/gitnamespace.py
@@ -40,7 +40,8 @@ class IGitNamespace(Interface):
     def createRepository(repository_type, registrant, name,
                          information_type=None, date_created=None,
                          target_default=False, owner_default=False,
-                         with_hosting=False, async_hosting=False, status=None):
+                         with_hosting=False, async_hosting=False, status=None,
+                         clone_from_repository=None):
         """Create and return an `IGitRepository` in this namespace.
 
         :param with_hosting: If True, also creates the repository on git
diff --git a/lib/lp/code/model/gitnamespace.py b/lib/lp/code/model/gitnamespace.py
index 6c0a20f..deaa9d9 100644
--- a/lib/lp/code/model/gitnamespace.py
+++ b/lib/lp/code/model/gitnamespace.py
@@ -79,7 +79,8 @@ class _BaseGitNamespace:
                          date_created=DEFAULT, description=None,
                          target_default=False, owner_default=False,
                          with_hosting=False, async_hosting=False,
-                         status=GitRepositoryStatus.AVAILABLE):
+                         status=GitRepositoryStatus.AVAILABLE,
+                         clone_from_repository=None):
         """See `IGitNamespace`."""
         repository_set = getUtility(IGitRepositorySet)
 
@@ -133,7 +134,8 @@ class _BaseGitNamespace:
                 transaction.commit()
             assert repository.id is not None
 
-            clone_from_repository = repository.getClonedFrom()
+            if clone_from_repository is None:
+                clone_from_repository = repository.getClonedFrom()
             repository._createOnHostingService(
                 clone_from_repository=clone_from_repository,
                 async_create=async_hosting)
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index f7a2a0b..fb5e703 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -1753,14 +1753,15 @@ class GitRepositorySet:
     def new(self, repository_type, registrant, owner, target, name,
             information_type=None, date_created=DEFAULT, description=None,
             with_hosting=False, async_hosting=False,
-            status=GitRepositoryStatus.AVAILABLE):
+            status=GitRepositoryStatus.AVAILABLE, clone_from_repository=None):
         """See `IGitRepositorySet`."""
         namespace = get_git_namespace(target, owner)
         return namespace.createRepository(
             repository_type, registrant, name,
             information_type=information_type, date_created=date_created,
             description=description, with_hosting=with_hosting,
-            async_hosting=async_hosting, status=status)
+            async_hosting=async_hosting, status=status,
+            clone_from_repository=clone_from_repository)
 
     def fork(self, origin, requester, new_owner):
         namespace = get_git_namespace(origin.target, new_owner)
@@ -1771,14 +1772,17 @@ class GitRepositorySet:
             name=name, information_type=origin.information_type,
             date_created=UTC_NOW, description=origin.description,
             with_hosting=True, async_hosting=True,
-            status=GitRepositoryStatus.CREATING)
-        try:
-            # Try to set the new repo as owner-default.
-            repository.setOwnerDefault(True)
-        except GitDefaultConflict:
-            # If there is already a owner-default for this owner/target,
-            # just move on.
-            pass
+            status=GitRepositoryStatus.CREATING, clone_from_repository=origin)
+        if origin.target_default or origin.owner_default:
+            try:
+                # If the origin is the default for its target or for its
+                # owner and target, then try to set the new repo as
+                # owner-default.
+                repository.setOwnerDefault(True)
+            except GitDefaultConflict:
+                # If there is already a owner-default for this owner/target,
+                # just move on.
+                pass
         return repository
 
     def getByPath(self, user, path):
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 7d7464e..b83f9da 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -2743,18 +2743,31 @@ class TestGitRepositoryFork(TestCaseWithFactory):
 
     def test_fork(self):
         repo = self.factory.makeGitRepository()
+        with person_logged_in(repo.target.owner):
+            getUtility(IGitRepositorySet).setDefaultRepository(
+                repo.target, repo)
         another_person = self.factory.makePerson()
         another_team = self.factory.makeTeam(members=[another_person])
 
         forked_repo = getUtility(IGitRepositorySet).fork(
             repo, another_person, another_team)
-        self.assertEqual(another_team, forked_repo.owner)
-        self.assertEqual(another_person, forked_repo.registrant)
-        self.assertTrue(forked_repo.owner_default)
-        self.assertEqual(1, self.hosting_fixture.create.call_count)
+        self.assertThat(forked_repo, MatchesStructure(
+            registrant=Equals(another_person),
+            owner=Equals(another_team),
+            target=Equals(repo.target),
+            name=Equals(repo.name),
+            owner_default=Is(True),
+            target_default=Is(False)))
+        self.assertEqual(
+            [((forked_repo.getInternalPath(),),
+              {"clone_from": repo.getInternalPath(), "async_create": True})],
+            self.hosting_fixture.create.calls)
 
     def test_fork_not_owner_default(self):
         repo = self.factory.makeGitRepository()
+        with person_logged_in(repo.target.owner):
+            getUtility(IGitRepositorySet).setDefaultRepository(
+                repo.target, repo)
         # The person forking the repo already has another repo which is the
         # owner-default for that owner & target.
         previous_repo = self.factory.makeGitRepository(target=repo.target)
@@ -2762,23 +2775,64 @@ class TestGitRepositoryFork(TestCaseWithFactory):
 
         forked_repo = getUtility(IGitRepositorySet).fork(
             repo, previous_repo.owner, previous_repo.owner)
-        self.assertEqual(previous_repo.owner, forked_repo.owner)
-        self.assertEqual(previous_repo.owner, forked_repo.registrant)
-        self.assertFalse(forked_repo.owner_default)
-        self.assertEqual(1, self.hosting_fixture.create.call_count)
+        self.assertThat(forked_repo, MatchesStructure(
+            registrant=Equals(previous_repo.owner),
+            owner=Equals(previous_repo.owner),
+            target=Equals(repo.target),
+            name=Equals(repo.name),
+            owner_default=Is(False),
+            target_default=Is(False)))
+        self.assertEqual(
+            [((forked_repo.getInternalPath(),),
+              {"clone_from": repo.getInternalPath(), "async_create": True})],
+            self.hosting_fixture.create.calls)
 
     def test_fork_same_name(self):
         repo = self.factory.makeGitRepository()
+        with person_logged_in(repo.target.owner):
+            getUtility(IGitRepositorySet).setDefaultRepository(
+                repo.target, repo)
 
         person = self.factory.makePerson()
-        same_name_repo = self.factory.makeGitRepository(
+        self.factory.makeGitRepository(
             owner=person, registrant=person,
             name=repo.name, target=repo.target)
 
         forked_repo = getUtility(IGitRepositorySet).fork(repo, person, person)
-        self.assertEqual(forked_repo.target, repo.target)
-        self.assertTrue(forked_repo.owner_default)
-        self.assertEqual(forked_repo.name, "%s-1" % same_name_repo.name)
+        self.assertThat(forked_repo, MatchesStructure(
+            registrant=Equals(person),
+            owner=Equals(person),
+            target=Equals(repo.target),
+            name=Equals("%s-1" % repo.name),
+            owner_default=Is(True),
+            target_default=Is(False)))
+        self.assertEqual(
+            [((forked_repo.getInternalPath(),),
+              {"clone_from": repo.getInternalPath(), "async_create": True})],
+            self.hosting_fixture.create.calls)
+
+    def test_fork_non_default_origin(self):
+        project = self.factory.makeProduct()
+        default_repo = self.factory.makeGitRepository(target=project)
+        non_default_repo = self.factory.makeGitRepository(target=project)
+        with person_logged_in(project.owner):
+            getUtility(IGitRepositorySet).setDefaultRepository(
+                project, default_repo)
+        person = self.factory.makePerson()
+
+        forked_repo = getUtility(IGitRepositorySet).fork(
+            non_default_repo, person, person)
+        self.assertThat(forked_repo, MatchesStructure(
+            registrant=Equals(person),
+            owner=Equals(person),
+            target=Equals(project),
+            owner_default=Is(False),
+            target_default=Is(False)))
+        self.assertEqual(
+            [((forked_repo.getInternalPath(),),
+              {"clone_from": non_default_repo.getInternalPath(),
+               "async_create": True})],
+            self.hosting_fixture.create.calls)
 
 
 class TestGitRepositoryDetectMerges(TestCaseWithFactory):