launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/codeimport-git-dulwich-web into lp:launchpad.

Commit message:
Refactor code import git-to-git worker tests to push over HTTP.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-dulwich-web/+merge/309969

This will make it easier to add integration test support for turnip-set-symbolic-ref in a later branch.

I considered adding a turnip fixture instead, but this is a *lot* less infrastructure faff given that turnip would involve adding an indirect dependency on pygit2, and even having to write a duplicate turnip-set-symbolic-ref handler on top of dulwich is only about 40 lines of code.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/codeimport-git-dulwich-web into lp:launchpad.

=== modified file 'lib/lp/codehosting/codeimport/tests/servers.py'
--- lib/lp/codehosting/codeimport/tests/servers.py	2016-10-11 15:28:25 +0000
+++ lib/lp/codehosting/codeimport/tests/servers.py	2016-11-03 15:21:21 +0000
@@ -22,6 +22,7 @@
 import tempfile
 import threading
 import time
+from wsgiref.simple_server import make_server
 
 from bzrlib.branch import Branch
 from bzrlib.branchbuilder import BranchBuilder
@@ -37,12 +38,17 @@
     join as urljoin,
     )
 import CVS
+from dulwich.errors import NotGitRepository
 import dulwich.index
 from dulwich.objects import Blob
 from dulwich.repo import Repo as GitRepo
-from dulwich.server import (
-    DictBackend,
-    TCPGitServer,
+from dulwich.server import Backend
+from dulwich.web import (
+    GunzipFilter,
+    HTTPGitApplication,
+    LimitedInputFilter,
+    WSGIRequestHandlerLogger,
+    WSGIServerLogger,
     )
 import subvertpy.ra
 import subvertpy.repos
@@ -221,13 +227,29 @@
         self._repository = self.createRepository(self._repository_path)
 
 
-class TCPGitServerThread(threading.Thread):
-    """Thread that runs a TCP Git server."""
+class GitStoreBackend(Backend):
+    """A backend that looks up repositories under a store directory."""
+
+    def __init__(self, root):
+        self.root = root
+
+    def open_repository(self, path):
+        full_path = os.path.normpath(os.path.join(self.root, path.lstrip("/")))
+        if not full_path.startswith(self.root + "/"):
+            raise NotGitRepository("Repository %s not under store" % path)
+        return GitRepo(full_path)
+
+
+class HTTPGitServerThread(threading.Thread):
+    """Thread that runs an HTTP Git server."""
 
     def __init__(self, backend, address, port=None):
-        super(TCPGitServerThread, self).__init__()
-        self.setName("TCP Git server on %s:%s" % (address, port))
-        self.server = TCPGitServer(backend, address, port)
+        super(HTTPGitServerThread, self).__init__()
+        self.setName("HTTP Git server on %s:%s" % (address, port))
+        app = GunzipFilter(LimitedInputFilter(HTTPGitApplication(backend)))
+        self.server = make_server(
+            address, port, app, handler_class=WSGIRequestHandlerLogger,
+            server_class=WSGIServerLogger)
 
     def run(self):
         self.server.serve_forever()
@@ -241,17 +263,19 @@
 
 class GitServer(Server):
 
-    def __init__(self, repository_path, use_server=False):
+    def __init__(self, repository_store, use_server=False):
         super(GitServer, self).__init__()
-        self.repository_path = repository_path
+        self.repository_store = repository_store
         self._use_server = use_server
 
-    def get_url(self):
+    def get_url(self, repository_name):
         """Return a URL to the Git repository."""
         if self._use_server:
-            return 'git://%s:%d/' % self._server.get_address()
+            host, port = self._server.get_address()
+            return 'http://%s:%d/%s' % (host, port, repository_name)
         else:
-            return local_path_to_url(self.repository_path)
+            return local_path_to_url(
+                os.path.join(self.repository_store, repository_name))
 
     def createRepository(self, path, bare=False):
         if bare:
@@ -259,13 +283,11 @@
         else:
             GitRepo.init(path)
 
-    def start_server(self, bare=False):
+    def start_server(self):
         super(GitServer, self).start_server()
-        self.createRepository(self.repository_path, bare=bare)
         if self._use_server:
-            repo = GitRepo(self.repository_path)
-            self._server = TCPGitServerThread(
-                DictBackend({"/": repo}), "localhost", 0)
+            self._server = HTTPGitServerThread(
+                GitStoreBackend(self.repository_store), "localhost", 0)
             self._server.start()
 
     def stop_server(self):
@@ -273,8 +295,11 @@
         if self._use_server:
             self._server.stop()
 
-    def makeRepo(self, tree_contents):
-        repo = GitRepo(self.repository_path)
+    def makeRepo(self, repository_name, tree_contents):
+        repository_path = os.path.join(self.repository_store, repository_name)
+        os.mkdir(repository_path)
+        self.createRepository(repository_path, bare=self._use_server)
+        repo = GitRepo(repository_path)
         blobs = [
             (Blob.from_string(contents), filename) for (filename, contents)
             in tree_contents]

=== modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
--- lib/lp/codehosting/codeimport/tests/test_worker.py	2016-10-13 12:19:56 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py	2016-11-03 15:21:21 +0000
@@ -1163,16 +1163,16 @@
     def makeSourceDetails(self, branch_name, files, stacked_on_url=None):
         """Make a Git `CodeImportSourceDetails` pointing at a real Git repo.
         """
-        repository_path = self.makeTemporaryDirectory()
-        git_server = GitServer(repository_path)
+        repository_store = self.makeTemporaryDirectory()
+        git_server = GitServer(repository_store)
         git_server.start_server()
         self.addCleanup(git_server.stop_server)
 
-        git_server.makeRepo(files)
+        git_server.makeRepo('source', files)
         self.foreign_commit_count = 1
 
         return self.factory.makeCodeImportSourceDetails(
-            rcstype='git', url=git_server.get_url(),
+            rcstype='git', url=git_server.get_url('source'),
             stacked_on_url=stacked_on_url)
 
     def test_non_master(self):

=== modified file 'lib/lp/codehosting/codeimport/tests/test_workermonitor.py'
--- lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2016-10-12 13:35:48 +0000
+++ lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2016-11-03 15:21:21 +0000
@@ -14,7 +14,6 @@
 import subprocess
 import tempfile
 import urllib
-from urlparse import urlsplit
 
 from bzrlib.branch import Branch
 from bzrlib.tests import (
@@ -739,11 +738,11 @@
         self.git_server.start_server()
         self.addCleanup(self.git_server.stop_server)
 
-        self.git_server.makeRepo([('README', 'contents')])
+        self.git_server.makeRepo('source', [('README', 'contents')])
         self.foreign_commit_count = 1
 
         return self.factory.makeCodeImport(
-            git_repo_url=self.git_server.get_url(),
+            git_repo_url=self.git_server.get_url('source'),
             target_rcs_type=target_rcs_type)
 
     def makeBzrCodeImport(self):
@@ -791,8 +790,7 @@
             commit_count = branch.revno()
         else:
             repo_path = os.path.join(
-                urlsplit(config.codehosting.git_browse_root).path,
-                code_import.target.unique_name)
+                self.target_store, code_import.target.unique_name)
             commit_count = int(subprocess.check_output(
                 ["git", "rev-list", "--count", "HEAD"],
                 cwd=repo_path, universal_newlines=True))
@@ -863,14 +861,17 @@
 
     def test_import_git_to_git(self):
         # Create a Git-to-Git CodeImport and import it.
-        target_store = tempfile.mkdtemp()
-        self.addCleanup(shutil.rmtree, target_store)
+        self.target_store = tempfile.mkdtemp()
+        self.addCleanup(shutil.rmtree, self.target_store)
+        target_git_server = GitServer(self.target_store, use_server=True)
+        target_git_server.start_server()
+        self.addCleanup(target_git_server.stop_server)
         config_name = self.getUniqueString()
         config_fixture = self.useFixture(ConfigFixture(
             config_name, self.layer.config_fixture.instance_name))
         setting_lines = [
             "[codehosting]",
-            "git_browse_root: file://%s" % target_store,
+            "git_browse_root: %s" % target_git_server.get_url(""),
             "",
             "[codeimport]",
             "macaroon_secret_key: some-secret",
@@ -884,11 +885,9 @@
         job_id = job.id
         self.layer.txn.commit()
         target_repo_path = os.path.join(
-            target_store, job.code_import.target.unique_name)
+            self.target_store, job.code_import.target.unique_name)
         os.makedirs(target_repo_path)
-        target_git_server = GitServer(target_repo_path, use_server=False)
-        target_git_server.start_server(bare=True)
-        self.addCleanup(target_git_server.stop_server)
+        target_git_server.createRepository(target_repo_path, bare=True)
         result = self.performImport(job_id)
         return result.addCallback(self.assertImported, code_import_id)
 

=== modified file 'lib/lp/codehosting/codeimport/worker.py'
--- lib/lp/codehosting/codeimport/worker.py	2016-10-14 16:47:33 +0000
+++ lib/lp/codehosting/codeimport/worker.py	2016-11-03 15:21:21 +0000
@@ -1010,13 +1010,10 @@
         unauth_target_url = urljoin(
             config.codehosting.git_browse_root, self.source_details.target_id)
         split = urlsplit(unauth_target_url)
-        if split.hostname:
-            target_netloc = ":%s@%s" % (
-                self.source_details.macaroon.serialize(), split.hostname)
-            if split.port:
-                target_netloc += ":%s" % split.port
-        else:
-            target_netloc = ""
+        target_netloc = ":%s@%s" % (
+            self.source_details.macaroon.serialize(), split.hostname)
+        if split.port:
+            target_netloc += ":%s" % split.port
         target_url = urlunsplit([
             split.scheme, target_netloc, split.path, "", ""])
         # XXX cjwatson 2016-10-11: Ideally we'd put credentials in a