launchpad-reviewers team mailing list archive

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

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:scan-forked-repository into launchpad:master.

Commit message:
Scan the repository when confirming its creation

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

When we ask a fork and Turnip finishes doing its job, we confirm the repository creation at Launchpad side, but we are not scanning it. Without the scan, Launchpad reports to the user in the web interface that the repository finished creating,  but no branch is shown.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:scan-forked-repository into launchpad:master.

diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index 201c9a4..a4af806 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -771,10 +771,17 @@ class IGitRepositoryEdit(IWebhookTarget):
     def setTarget(target, user):
         """Set the target of the repository."""
 
+    def scan(log=None):
+        """
+        Executes a synchronous scan of this repository.
+
+        :return: A tuple with (updated_refs, deleted_refs).
+        """
+
     @export_write_operation()
     @operation_for_version("devel")
     def rescan():
-        """Force a rescan of this repository.
+        """Force a rescan of this repository as a celery task.
 
         This may be helpful in cases where a previous scan crashed.
         """
diff --git a/lib/lp/code/model/gitjob.py b/lib/lp/code/model/gitjob.py
index ab37d2b..8328e0b 100644
--- a/lib/lp/code/model/gitjob.py
+++ b/lib/lp/code/model/gitjob.py
@@ -32,7 +32,6 @@ from zope.interface import (
     implementer,
     provider,
     )
-from zope.security.proxy import removeSecurityProxy
 
 from lp.app.errors import NotFoundError
 from lp.code.enums import (
@@ -220,13 +219,13 @@ class GitRefScanJob(GitJobDerived):
         return job
 
     @staticmethod
-    def composeWebhookPayload(repository, refs_to_upsert, refs_to_remove):
-        old_refs = {ref.path: ref for ref in repository.refs}
+    def composeWebhookPayload(repository, old_refs_commits, refs_to_upsert,
+                              refs_to_remove):
         ref_changes = {}
         for ref in refs_to_upsert.keys() + list(refs_to_remove):
             old = (
-                {"commit_sha1": old_refs[ref].commit_sha1}
-                if ref in old_refs else None)
+                {"commit_sha1": old_refs_commits[ref]}
+                if ref in old_refs_commits else None)
             new = (
                 {"commit_sha1": refs_to_upsert[ref]['sha1']}
                 if ref in refs_to_upsert else None)
@@ -246,26 +245,17 @@ class GitRefScanJob(GitJobDerived):
             with try_advisory_lock(
                     LockType.GIT_REF_SCAN, self.repository.id,
                     Store.of(self.repository)):
-                hosting_path = self.repository.getInternalPath()
-                refs_to_upsert, refs_to_remove = (
-                    self.repository.planRefChanges(hosting_path, logger=log))
-                self.repository.fetchRefCommits(
-                    hosting_path, refs_to_upsert, logger=log)
+                old_refs_commits = {
+                    ref.path: ref.commit_sha1 for ref in self.repository.refs}
+                upserted_refs, removed_refs = self.repository.scan(log=log)
                 # The webhook delivery includes old ref information, so
                 # prepare it before we actually execute the changes.
                 if getFeatureFlag('code.git.webhooks.enabled'):
                     payload = self.composeWebhookPayload(
-                        self.repository, refs_to_upsert, refs_to_remove)
+                        self.repository, old_refs_commits,
+                        upserted_refs, removed_refs)
                     getUtility(IWebhookSet).trigger(
                         self.repository, 'git:push:0.1', payload)
-                self.repository.synchroniseRefs(
-                    refs_to_upsert, refs_to_remove, logger=log)
-                props = getUtility(IGitHostingClient).getProperties(
-                    hosting_path)
-                # We don't want ref canonicalisation, nor do we want to send
-                # this change back to the hosting service.
-                removeSecurityProxy(self.repository)._default_branch = (
-                    props["default_branch"])
         except LostObjectError:
             log.info(
                 "Skipping repository %s because it has been deleted." %
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 726e9f1..44b9315 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -24,6 +24,7 @@ from itertools import (
     chain,
     groupby,
     )
+import logging
 from operator import attrgetter
 
 from breezy import urlutils
@@ -208,6 +209,9 @@ from lp.services.webhooks.model import WebhookTargetMixin
 from lp.snappy.interfaces.snap import ISnapSet
 
 
+logger = logging.getLogger(__name__)
+
+
 object_type_map = {
     "commit": GitObjectType.COMMIT,
     "tree": GitObjectType.TREE,
@@ -806,6 +810,23 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
         if refs_to_remove:
             self.removeRefs(refs_to_remove)
 
+    def scan(self, log=None):
+        """See `IGitRepository`"""
+        log = log if log is not None else logger
+        hosting_path = self.getInternalPath()
+        refs_to_upsert, refs_to_remove = (
+            self.planRefChanges(hosting_path, logger=log))
+        self.fetchRefCommits(
+            hosting_path, refs_to_upsert, logger=log)
+        self.synchroniseRefs(
+            refs_to_upsert, refs_to_remove, logger=log)
+        props = getUtility(IGitHostingClient).getProperties(
+            hosting_path)
+        # We don't want ref canonicalisation, nor do we want to send
+        # this change back to the hosting service.
+        self._default_branch = props["default_branch"]
+        return refs_to_upsert, refs_to_remove
+
     def rescan(self):
         """See `IGitRepository`."""
         getUtility(IGitRefScanJobSource).create(self)
diff --git a/lib/lp/code/model/tests/test_gitjob.py b/lib/lp/code/model/tests/test_gitjob.py
index e671c42..3f606c0 100644
--- a/lib/lp/code/model/tests/test_gitjob.py
+++ b/lib/lp/code/model/tests/test_gitjob.py
@@ -283,8 +283,10 @@ class TestGitRefScanJob(TestCaseWithFactory):
                 'type': 'commit'},
             }
         removed_refs = ['refs/tags/1.0']
+        old_refs_commits = {
+            ref.path: ref.commit_sha1 for ref in repository.refs}
         payload = GitRefScanJob.composeWebhookPayload(
-            repository, new_refs, removed_refs)
+            repository, old_refs_commits, new_refs, removed_refs)
         self.assertEqual(
             {'git_repository': '/' + repository.unique_name,
              'git_repository_path': repository.unique_name,
diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index 9dc6175..9bc5788 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -658,6 +658,7 @@ class GitAPI(LaunchpadXMLRPCView):
             raise faults.GitRepositoryNotFound(translated_path)
         self._validateRequesterCanManageRepoCreation(
             requester, naked_repo, auth_params)
+        naked_repo.scan()
         naked_repo.status = GitRepositoryStatus.AVAILABLE
 
     def confirmRepoCreation(self, translated_path, auth_params):
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index 9fb9b2e..9024a25 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -3,8 +3,11 @@
 
 """Tests for the internal Git API."""
 
+from __future__ import absolute_import, print_function, unicode_literals
+
 __metaclass__ = type
 
+import hashlib
 import uuid
 
 from fixtures import FakeLogger
@@ -287,6 +290,22 @@ class TestGitAPIMixin:
 
     def assertConfirmsRepoCreation(self, requester, git_repository,
                                    can_authenticate=True, macaroon_raw=None):
+        # Puts some refs in git hosting, to make sure we scanned them.
+        sha1 = lambda x: hashlib.sha1(x).hexdigest()
+        self.hosting_fixture = self.useFixture(
+            GitHostingFixture(refs={
+                'refs/heads/master': {
+                    "object": {
+                        "sha1": sha1('master-branch'),
+                        "type": "commit",
+                    },
+                },
+                'refs/heads/foo': {
+                    "object": {
+                        "sha1": sha1('foo-branch'),
+                        "type": "commit",
+                    },
+                }}))
         translated_path = git_repository.getInternalPath()
         auth_params = _make_auth_params(
             requester, can_authenticate=can_authenticate,
@@ -298,6 +317,19 @@ class TestGitAPIMixin:
         self.assertIsNone(result)
         Store.of(git_repository).invalidate(git_repository)
         self.assertEqual(GitRepositoryStatus.AVAILABLE, git_repository.status)
+        # Should have checked the refs at some point.
+        excluded_prefixes = config.codehosting.git_exclude_ref_prefixes
+        self.assertEqual(
+            [(tuple(git_repository.getInternalPath(), ),
+              dict(exclude_prefixes=excluded_prefixes.split(",")))],
+            self.hosting_fixture.getRefs.calls)
+        self.assertEqual(2, git_repository.refs.count())
+        self.assertEqual(
+            {'refs/heads/foo', 'refs/heads/master'},
+            {i.path for i in git_repository.refs})
+        self.assertEqual(
+            {sha1('foo-branch'), sha1('master-branch')},
+            {i.commit_sha1 for i in git_repository.refs})
 
     def assertConfirmRepoCreationFails(
             self, failure, requester, git_repository, can_authenticate=True,
@@ -325,7 +357,7 @@ class TestGitAPIMixin:
             macaroon_raw)
 
     def assertAbortsRepoCreation(self, requester, git_repository,
-                                   can_authenticate=True, macaroon_raw=None):
+                                 can_authenticate=True, macaroon_raw=None):
         translated_path = git_repository.getInternalPath()
         auth_params = _make_auth_params(
             requester, can_authenticate=can_authenticate,
@@ -403,7 +435,7 @@ class TestGitAPIMixin:
             expected_hosting_call_args = [(repository.getInternalPath(),)]
             expected_hosting_call_kwargs = [{
                 "clone_from": (cloned_from.getInternalPath()
-                                if cloned_from else None),
+                               if cloned_from else None),
                 "async_create": False}]
 
         self.assertEqual(GitRepositoryType.HOSTED, repository.repository_type)