launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging ~wgrant/turnip:http-shallow-clones into turnip:master with ~wgrant/turnip:half-close-rework as a prerequisite.

Commit message:
Fix shallow clones over HTTP. (LP: #1547141)

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/turnip/+git/turnip/+merge/288618

Fix shallow clones over HTTP. (LP: #1547141)

The details are marginally gory, but basically we need to half-close the connection once the HTTP body has been sent, and then not forward stderr as an ERR packet because nasty stderr occurs as a normal part of git HTTP operations because git. Sigh.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~wgrant/turnip:http-shallow-clones into turnip:master.

diff --git a/turnip/pack/git.py b/turnip/pack/git.py
index 2fbd59f..0c15b30 100644
--- a/turnip/pack/git.py
+++ b/turnip/pack/git.py
@@ -221,6 +221,7 @@ class GitProcessProtocol(protocol.ProcessProtocol):
 
     def __init__(self, peer):
         self.peer = peer
+        self.out_started = False
 
     def connectionMade(self):
         self.peer.setPeer(self)
@@ -230,16 +231,32 @@ class GitProcessProtocol(protocol.ProcessProtocol):
         self.peer.resumeProducing()
 
     def outReceived(self, data):
+        self.out_started = True
         self.peer.sendRawData(data)
 
     def errReceived(self, data):
-        # Just store it up so we can forward it as a single ERR packet
-        # when the process is done.
+        # Just store it up so we can forward and/or log it when the
+        # process is done.
         self._err_buffer += data
 
     def outConnectionLost(self):
         if self._err_buffer:
-            self.peer.die(self._err_buffer)
+            # Originally we'd always return stderr as an ERR packet for
+            # debugging, but it breaks HTTP shallow clones: the second
+            # HTTP request causes the backend to die when it tries to
+            # read more than was sent, but the reference
+            # git-http-backend conveniently just sends the error to the
+            # server log so the client doesn't notice. So now we always
+            # log any stderr, but only forward it to the client if the
+            # subprocess never wrote to stdout.
+            if not self.out_started:
+                self.peer.log.info(
+                    'git wrote to stderr first; returning to client: {buf}',
+                    buf=repr(self._err_buffer))
+                self.peer.sendPacket(ERROR_PREFIX + self._err_buffer + b'\n')
+            else:
+                self.peer.log.info(
+                    "git wrote to stderr: {buf}", buf=repr(self._err_buffer))
 
     def sendPacket(self, data):
         self.sendRawData(encode_packet(data))
diff --git a/turnip/pack/http.py b/turnip/pack/http.py
index 975d9a2..a2d6d97 100644
--- a/turnip/pack/http.py
+++ b/turnip/pack/http.py
@@ -151,6 +151,8 @@ class HTTPPackClientProtocol(PackProtocol):
                 self.factory.command, self.factory.pathname,
                 self.factory.params))
         self.sendRawData(self.factory.body.read())
+        if hasattr(self.transport, 'loseWriteConnection'):
+            self.transport.loseWriteConnection()
 
     def packetReceived(self, data):
         """Check and forward the first packet from the backend.
diff --git a/turnip/pack/tests/test_functional.py b/turnip/pack/tests/test_functional.py
index 8763dcc..2eee919 100644
--- a/turnip/pack/tests/test_functional.py
+++ b/turnip/pack/tests/test_functional.py
@@ -207,6 +207,33 @@ class FunctionalTestMixin(object):
         self.assertIn(b'Another test', out)
 
     @defer.inlineCallbacks
+    def test_clone_shallow(self):
+        # Test a shallow clone. This makes the negotation a little more
+        # complicated, and tests some weird edge cases in the HTTP protocol.
+        test_root = self.useFixture(TempDir()).path
+        clone1 = os.path.join(test_root, 'clone1')
+        clone2 = os.path.join(test_root, 'clone2')
+
+        # Push a commit that we can try to clone.
+        yield self.assertCommandSuccess((b'git', b'clone', self.url, clone1))
+        yield self.assertCommandSuccess(
+            (b'git', b'config', b'user.email', b'test@xxxxxxxxxxx'),
+            path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'commit', b'--allow-empty', b'-m', b'Committed test'),
+            path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'push', b'origin', b'master'), path=clone1)
+
+        # Try a shallow clone. This makes the negotation a little more
+        # complicated, and tests some weird edge cases in the HTTP protocol.
+        yield self.assertCommandSuccess(
+            (b'git', b'clone', '--depth=1', self.url, clone2))
+        out = yield self.assertCommandSuccess(
+            (b'git', b'log', b'--oneline', b'-n', b'1'), path=clone2)
+        self.assertIn(b'Committed test', out)
+
+    @defer.inlineCallbacks
     def test_no_repo(self):
         test_root = self.useFixture(TempDir()).path
         output = yield utils.getProcessOutput(