launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> === added directory 'turnip/pack/data'
> === modified file 'turnip/pack/git.py'
> --- turnip/pack/git.py	2015-03-09 11:53:40 +0000
> +++ turnip/pack/git.py	2015-03-30 09:20:00 +0000
> @@ -5,6 +5,7 @@
>      )
>  
>  import sys
> +import uuid
>  
>  from twisted.internet import (
>      defer,
> @@ -24,6 +25,7 @@
>      decode_request,
>      encode_packet,
>      encode_request,
> +    ensure_hooks,
>      INCOMPLETE_PKT,
>      )
>  
> @@ -271,6 +273,8 @@
>      Invokes the reference C Git implementation.
>      """
>  
> +    hookrpc_key = None
> +
>      def requestReceived(self, command, raw_pathname, params):
>          path = compose_path(self.factory.root, raw_pathname)
>          if command == b'git-upload-pack':
> @@ -289,8 +293,19 @@
>              args.append(b'--advertise-refs')
>          args.append(path)
>  
> +        env = {}
> +        if subcmd == b'receive-pack' and self.factory.hookrpc_handler:
> +            # This is a write operation, so prepare hooks, the hook RPC
> +            # server, and the environment variables that link them up.
> +            self.hookrpc_key = str(uuid.uuid4())
> +            self.factory.hookrpc_handler.registerKey(
> +                self.hookrpc_key, raw_pathname, [])
> +            ensure_hooks(path)
> +            env[b'TURNIP_HOOK_RPC_SOCK'] = self.factory.hookrpc_sock
> +            env[b'TURNIP_HOOK_RPC_KEY'] = self.hookrpc_key
> +
>          self.peer = GitProcessProtocol(self)
> -        reactor.spawnProcess(self.peer, cmd, args)
> +        reactor.spawnProcess(self.peer, cmd, args, env=env)
>  
>      def readConnectionLost(self):
>          self.peer.loseWriteConnection()
> @@ -298,13 +313,20 @@
>      def writeConnectionLost(self):
>          self.peer.loseReadConnection()
>  
> +    def connectionLost(self, reason):
> +        if self.hookrpc_key:
> +            self.factory.hookrpc_handler.unregisterKey(self.hookrpc_key)
> +        PackServerProtocol.connectionLost(self, reason)
> +
>  
>  class PackBackendFactory(protocol.Factory):
>  
>      protocol = PackBackendProtocol
>  
> -    def __init__(self, root):
> +    def __init__(self, root, hookrpc_handler=None, hookrpc_sock=None):
>          self.root = root
> +        self.hookrpc_handler = hookrpc_handler
> +        self.hookrpc_sock = hookrpc_sock
>  
>  
>  class PackVirtServerProtocol(PackProxyServerProtocol):
> 
> === modified file 'turnip/pack/helpers.py'
> --- turnip/pack/helpers.py	2015-01-22 06:14:32 +0000
> +++ turnip/pack/helpers.py	2015-03-30 09:20:00 +0000
> @@ -4,6 +4,16 @@
>      unicode_literals,
>      )
>  
> +import hashlib
> +import os.path

Should logically be just os given that you're using os.rename etc.  (Of course it works anyway.)

> +import shutil
> +from tempfile import (
> +    mktemp,
> +    NamedTemporaryFile,
> +    )
> +
> +import turnip.pack.hooks.hook
> +
>  
>  PKT_LEN_SIZE = 4
>  PKT_PAYLOAD_MAX = 65520
> @@ -83,3 +93,48 @@
>              raise ValueError('Metacharacter in arguments')
>          bits.append(name + b'=' + value)
>      return command + b' ' + b'\0'.join(bits) + b'\0'
> +
> +
> +def ensure_hooks(repo_root):
> +    """Put a repository's hooks into the desired state.
> +
> +    Consistency is maintained even if there are multiple invocations
> +    running concurrently. Files starting with tmp* are ignored, and any
> +    directories will cause an exception.
> +    """
> +
> +    wanted_hooks = ('pre-receive', 'post-receive')
> +    target_name = 'hook.py'
> +
> +    def hook_path(name):
> +        return os.path.join(repo_root, 'hooks', name)
> +
> +    if not os.path.exists(hook_path(target_name)):
> +        need_target = True
> +    else:
> +        with open(turnip.pack.hooks.hook.__file__, 'rb') as f:
> +            wanted = hashlib.sha256(f.read()).hexdigest()
> +        with open(hook_path(target_name), 'rb') as f:
> +            have = hashlib.sha256(f.read()).hexdigest()
> +        if wanted != have:
> +            need_target = True
> +        else:
> +            need_target = False

Maybe we should sanity-check permissions here as well.

> +
> +    if need_target:
> +        with open(turnip.pack.hooks.hook.__file__, 'rb') as master:
> +            with NamedTemporaryFile(dir=hook_path('.'), delete=False) as this:
> +                shutil.copyfileobj(master, this)
> +        os.chmod(this.name, 0o755)
> +        os.rename(this.name, hook_path(target_name))
> +
> +    for hook in wanted_hooks:
> +        # Not actually insecure, since os.symlink fails if the file exists.
> +        path = mktemp(dir=hook_path('.'))
> +        os.symlink(target_name, path)
> +        os.rename(path, hook_path(hook))
> +
> +    for name in os.listdir(hook_path('.')):
> +        if (name != target_name and name not in wanted_hooks
> +                and not name.startswith('tmp')):
> +            os.unlink(hook_path(name))

This can raise an exception if two invocations are running concurrently and race between os.listdir and os.unlink.  I expect you could safely ignore errors from os.unlink.

> 
> === added file 'turnip/pack/hookrpc.py'
> --- turnip/pack/hookrpc.py	1970-01-01 00:00:00 +0000
> +++ turnip/pack/hookrpc.py	2015-03-30 09:20:00 +0000
> @@ -0,0 +1,110 @@
> +from __future__ import (
> +    absolute_import,
> +    print_function,
> +    unicode_literals,
> +    )
> +
> +import json
> +import sys
> +
> +from twisted.internet import (
> +    defer,
> +    protocol,
> +    )
> +from twisted.protocols import basic
> +# twisted.web.xmlrpc doesn't exist for Python 3 yet, but the non-XML-RPC
> +# bits of this module work.
> +if sys.version_info.major < 3:
> +    from twisted.web import xmlrpc
> +
> +
> +class JSONNetstringProtocol(basic.NetstringReceiver):
> +
> +    def stringReceived(self, string):
> +        try:
> +            val = json.loads(string.decode('utf-8'))
> +        except (ValueError, UnicodeDecodeError):
> +            self.invalidValueReceived(string)
> +        else:
> +            self.valueReceived(val)
> +
> +    def valueReceived(self, value):
> +        raise NotImplementedError()
> +
> +    def invalidValueReceived(self, string):
> +        raise NotImplementedError()
> +
> +    def sendValue(self, value):
> +        self.sendString(json.dumps(value).encode('utf-8'))

I don't know if this matters here, but json.dumps never returns unicode in Python 2 unless ensure_ascii=False.  Did you mean to pass that?

> +
> +
> +class RPCServerProtocol(JSONNetstringProtocol):
> +
> +    def __init__(self, methods):
> +        self.result_log = []
> +        self.methods = dict(methods)
> +
> +    @defer.inlineCallbacks
> +    def valueReceived(self, val):
> +        if not isinstance(val, dict):
> +            self.sendValue({"error": "Command must be a JSON object"})
> +            return
> +        val = dict(val)
> +        op = val.pop('op', None)
> +        if not op:
> +            self.sendValue({"error": "No op specified"})
> +            return
> +        if op not in self.methods:
> +            self.sendValue({"error": "Unknown op: %s" % op})
> +            return
> +        res = yield self.methods[op](self, val)
> +        self.sendValue({"result": res})
> +
> +    def invalidValueReceived(self, string):
> +        self.sendValue({"error": "Command must be a JSON object"})
> +
> +
> +class RPCServerFactory(protocol.ServerFactory):
> +
> +    protocol = RPCServerProtocol
> +
> +    def __init__(self, methods):
> +        self.methods = dict(methods)
> +
> +    def buildProtocol(self, addr):
> +        return self.protocol(self.methods)
> +
> +
> +class HookRPCHandler(object):
> +
> +    def __init__(self, virtinfo_url):
> +        self.ref_paths = {}
> +        self.ref_rules = {}
> +        self.virtinfo_url = virtinfo_url
> +
> +    def registerKey(self, key, path, ref_rules):
> +        self.ref_paths[key] = path
> +        self.ref_rules[key] = ref_rules
> +
> +    def unregisterKey(self, key):
> +        del self.ref_rules[key]
> +        del self.ref_paths[key]
> +
> +    def listRefRules(self, proto, args):
> +        return self.ref_rules[args['key']]
> +
> +    @defer.inlineCallbacks
> +    def notifyPush(self, proto, args):
> +        path = self.ref_paths[args['key']]
> +        proxy = xmlrpc.Proxy(self.virtinfo_url, allowNone=True)
> +        yield proxy.callRemote(b'notify', path)
> +
> +
> +class HookRPCServerFactory(RPCServerFactory):
> +
> +    def __init__(self, hookrpc_handler):
> +        self.hookrpc_handler = hookrpc_handler
> +        self.methods = {
> +            'list_ref_rules': self.hookrpc_handler.listRefRules,
> +            'notify_push': self.hookrpc_handler.notifyPush,
> +            }
> 
> === added directory 'turnip/pack/hooks'
> === added file 'turnip/pack/hooks/__init__.py'
> === added file 'turnip/pack/hooks/hook.py'
> --- turnip/pack/hooks/hook.py	1970-01-01 00:00:00 +0000
> +++ turnip/pack/hooks/hook.py	2015-03-30 09:20:00 +0000
> @@ -0,0 +1,84 @@
> +#!/usr/bin/python
> +
> +from __future__ import (
> +    absolute_import,
> +    print_function,
> +    unicode_literals,
> +    )
> +
> +import json
> +import os
> +import re
> +import socket
> +import sys
> +
> +
> +def glob_to_re(s):
> +    """Convert a glob to a regular expression.
> +
> +    The only wildcard supported is "*", to match any path segment.
> +    """
> +    return b'^%s\Z' % (
> +        b''.join(b'[^/]*' if c == b'*' else re.escape(c) for c in s))

It might be worth explaining here why fnmatch isn't good enough.  (I infer that that is because we want to treat "/" specially.)

> +
> +
> +def match_rules(rule_lines, ref_lines):
> +    rules = [re.compile(glob_to_re(l.rstrip(b'\n'))) for l in rule_lines]
> +    # Match each ref against each rule.
> +    errors = []
> +    for ref_line in ref_lines:
> +        old, new, ref = ref_line.rstrip(b'\n').split(b' ', 2)
> +        if any(rule.match(ref) for rule in rules):
> +            errors.append(b"You can't push to %s." % ref)
> +    return errors
> +
> +
> +def netstring_send(sock, s):
> +    sock.send(b'%d:%s,' % (len(s), s))
> +
> +
> +def netstring_recv(sock):
> +    c = sock.recv(1)
> +    lengthstr = ''
> +    while c != b':':
> +        assert c.isdigit()
> +        lengthstr += c
> +        c = sock.recv(1)
> +    length = int(lengthstr)
> +    s = sock.recv(length)
> +    assert sock.recv(1) == b','
> +    return s
> +
> +
> +def rpc_invoke(sock, method, args):
> +    msg = dict(args)
> +    assert 'op' not in msg
> +    msg['op'] = method
> +    netstring_send(sock, json.dumps(msg))
> +    res = json.loads(netstring_recv(sock))
> +    if 'error' in res:
> +        raise Exception(res)
> +    return res['result']
> +
> +
> +if __name__ == '__main__':
> +    hook = os.path.basename(sys.argv[0])
> +    if hook == 'pre-receive':
> +        rpc_key = os.environ[b'TURNIP_HOOK_RPC_KEY']

os.environ's keys aren't byte strings, even in Python 3.  os.environb is a separate thing.

> +        sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> +        sock.connect(os.environ[b'TURNIP_HOOK_RPC_SOCK'])
> +        rule_lines = rpc_invoke(sock, b'list_ref_rules', {'key': rpc_key})
> +
> +        errors = match_rules(rule_lines, sys.stdin.readlines())
> +        for error in errors:
> +            sys.stdout.write(error + b'\n')
> +        sys.exit(1 if errors else 0)
> +    elif hook == 'post-receive':
> +        rpc_key = os.environ[b'TURNIP_HOOK_RPC_KEY']
> +        sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> +        sock.connect(os.environ[b'TURNIP_HOOK_RPC_SOCK'])
> +        if sys.stdin.readlines():
> +            rule_lines = rpc_invoke(sock, b'notify_push', {'key': rpc_key})
> +        sys.exit(0)
> +    else:
> +        sys.stderr.write(b'Invalid hook name: %s' % hook)
> 
> === added symlink 'turnip/pack/hooks/post-receive'
> === target is u'hook.py'
> === added symlink 'turnip/pack/hooks/pre-receive'
> === target is u'hook.py'

Why do we need these in the source tree?

> === modified file 'turnip/pack/tests/test_functional.py'
> --- turnip/pack/tests/test_functional.py	2015-03-04 08:02:09 +0000
> +++ turnip/pack/tests/test_functional.py	2015-03-30 09:20:00 +0000
> @@ -34,6 +34,10 @@
>      PackFrontendFactory,
>      PackVirtFactory,
>      )
> +from turnip.pack.hookrpc import (
> +    HookRPCHandler,
> +    HookRPCServerFactory,
> +    )
>  from turnip.pack.http import SmartHTTPFrontendResource
>  from turnip.pack.ssh import SmartSSHService
>  
> @@ -74,6 +78,10 @@
>      path is prefixed with '/+rw'
>      """
>  
> +    def __init__(self, *args, **kwargs):
> +        xmlrpc.XMLRPC.__init__(self, *args, **kwargs)
> +        self.push_notifications = []
> +
>      def xmlrpc_translatePath(self, pathname, permission, authenticated_uid,
>                               can_authenticate):
>          writable = False
> @@ -88,11 +96,32 @@
>      def xmlrpc_authenticateWithPassword(self, username, password):
>          return {'user': username}
>  
> +    def xmlrpc_notify(self, path):
> +        self.push_notifications.append(path)
> +
>  
>  class FunctionalTestMixin(object):
>  
>      run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)
>  
> +    def startVirtInfo(self):
> +        # Set up a fake virt information XML-RPC server which just
> +        # maps paths to their SHA-256 hash.
> +        self.virtinfo = FakeVirtInfoService(allowNone=True)
> +        self.virtinfo_listener = reactor.listenTCP(
> +            0, server.Site(self.virtinfo))
> +        self.virtinfo_port = self.virtinfo_listener.getHost().port
> +        self.virtinfo_url = b'http://localhost:%d/' % self.virtinfo_port
> +        self.addCleanup(self.virtinfo_listener.stopListening)
> +
> +    def startHookRPC(self):
> +        self.hookrpc_handler = HookRPCHandler(self.virtinfo_url)
> +        dir = self.useFixture(TempDir()).path
> +        self.hookrpc_path = os.path.join(dir, 'hookrpc_sock')
> +        self.hookrpc_listener = reactor.listenUNIX(
> +            self.hookrpc_path, HookRPCServerFactory(self.hookrpc_handler))
> +        self.addCleanup(self.hookrpc_listener.stopListening)
> +
>      @defer.inlineCallbacks
>      def assertCommandSuccess(self, command, path='.'):
>          out, err, code = yield utils.getProcessOutputAndValue(
> @@ -175,9 +204,15 @@
>      @defer.inlineCallbacks
>      def setUp(self):
>          super(TestBackendFunctional, self).setUp()
> +
>          # Set up a PackBackendFactory on a free port in a clean repo root.
> +        self.startVirtInfo()
> +        self.startHookRPC()
>          self.root = self.useFixture(TempDir()).path
> -        self.listener = reactor.listenTCP(0, PackBackendFactory(self.root))
> +        self.listener = reactor.listenTCP(
> +            0,
> +            PackBackendFactory(
> +                self.root, self.hookrpc_handler, self.hookrpc_path))
>          self.port = self.listener.getHost().port
>  
>          yield self.assertCommandSuccess(
> @@ -206,21 +241,18 @@
>          self.authserver_port = self.authserver_listener.getHost().port
>          self.authserver_url = b'http://localhost:%d/' % self.authserver_port
>  
> -        # Set up a fake virt information XML-RPC server which just
> -        # maps paths to their SHA-256 hash.
> -        self.virtinfo_listener = reactor.listenTCP(
> -            0, server.Site(FakeVirtInfoService(allowNone=True)))
> -        self.virtinfo_port = self.virtinfo_listener.getHost().port
> -        self.virtinfo_url = b'http://localhost:%d/' % self.virtinfo_port
> -
>          # Run a backend server in a repo root containing an empty repo
>          # for the path '/test'.
> +        self.startVirtInfo()
> +        self.startHookRPC()
>          self.root = self.useFixture(TempDir()).path
> -        internal_name = hashlib.sha256(b'/test').hexdigest()
> +        self.internal_name = hashlib.sha256(b'/test').hexdigest()
>          yield self.assertCommandSuccess(
> -            (b'git', b'init', b'--bare', internal_name), path=self.root)
> +            (b'git', b'init', b'--bare', self.internal_name), path=self.root)
>          self.backend_listener = reactor.listenTCP(
> -            0, PackBackendFactory(self.root))
> +            0,
> +            PackBackendFactory(
> +                self.root, self.hookrpc_handler, self.hookrpc_path))
>          self.backend_port = self.backend_listener.getHost().port
>  
>          self.virt_listener = reactor.listenTCP(
> @@ -234,7 +266,6 @@
>          super(FrontendFunctionalTestMixin, self).tearDown()
>          yield self.virt_listener.stopListening()
>          yield self.backend_listener.stopListening()
> -        yield self.virtinfo_listener.stopListening()
>          yield self.authserver_listener.stopListening()
>  
>      @defer.inlineCallbacks
> @@ -259,6 +290,7 @@
>              env=os.environ, path=clone1, errortoo=True)
>          self.assertThat(
>              out, StartsWith(self.early_error + b'Repository is read-only'))
> +        self.assertEqual([], self.virtinfo.push_notifications)
>  
>          # The remote repository is still empty.
>          out = yield utils.getProcessOutput(
> @@ -271,6 +303,8 @@
>              (b'git', b'remote', b'set-url', b'origin', self.url), path=clone1)
>          yield self.assertCommandSuccess(
>              (b'git', b'push', b'origin', b'master'), path=clone1)
> +        self.assertEqual(
> +            [self.internal_name], self.virtinfo.push_notifications)
>  
>  
>  class TestGitFrontendFunctional(FrontendFunctionalTestMixin, TestCase):
> 
> === modified file 'turnip/pack/tests/test_helpers.py'
> --- turnip/pack/tests/test_helpers.py	2015-01-22 06:14:32 +0000
> +++ turnip/pack/tests/test_helpers.py	2015-03-30 09:20:00 +0000
> @@ -4,9 +4,15 @@
>      unicode_literals,
>      )
>  
> +import hashlib
> +import os.path
> +import stat
> +
> +from fixtures import TempDir
>  from testtools import TestCase
>  
>  from turnip.pack import helpers
> +import turnip.pack.hooks.hook
>  
>  
>  TEST_DATA = b'0123456789abcdef'
> @@ -153,3 +159,52 @@
>          self.assertInvalid(
>              b'git-do-stuff', b'/some/path', {b'host': b'exam\0le.com'},
>              b'Metacharacter in arguments')
> +
> +
> +class TestEnsureHooks(TestCase):
> +    """Test repository hook maintenance."""
> +
> +    def setUp(self):
> +        super(TestEnsureHooks, self).setUp()
> +        self.repo_dir = self.useFixture(TempDir()).path
> +        self.hooks_dir = os.path.join(self.repo_dir, 'hooks')
> +        os.mkdir(self.hooks_dir)
> +
> +    def hook(self, hook):
> +        return os.path.join(self.hooks_dir, hook)
> +
> +    def test_deletes_random(self):
> +        # Unknown files are deleted.
> +        os.symlink('foo', self.hook('bar'))
> +        self.assertIn('bar', os.listdir(self.hooks_dir))
> +        helpers.ensure_hooks(self.repo_dir)
> +        self.assertNotIn('bar', os.listdir(self.hooks_dir))
> +
> +    def test_fixes_symlink(self):
> +        # A symlink with a bad path is fixed.
> +        os.symlink('foo', self.hook('pre-receive'))
> +        self.assertEqual('foo', os.readlink(self.hook('pre-receive')))
> +        helpers.ensure_hooks(self.repo_dir)
> +        self.assertEqual('hook.py', os.readlink(self.hook('pre-receive')))
> +
> +    def test_replaces_regular_file(self):
> +        # A regular file is replaced with a symlink.
> +        with open(self.hook('pre-receive'), 'w') as f:
> +            f.write('garbage')
> +        self.assertRaises(OSError, os.readlink, self.hook('pre-receive'))
> +        helpers.ensure_hooks(self.repo_dir)
> +        self.assertEqual('hook.py', os.readlink(self.hook('pre-receive')))
> +
> +    def test_replaces_hook_py(self):
> +        # The hooks themselves are symlinks to hook.py, which is always
> +        # kept up to date.
> +        with open(self.hook('hook.py'), 'w') as f:
> +            f.write('nothing to see here')
> +        helpers.ensure_hooks(self.repo_dir)
> +        with open(self.hook('hook.py'), 'rb') as actual:
> +            with open(turnip.pack.hooks.hook.__file__, 'rb') as expected:
> +                self.assertEqual(
> +                    hashlib.sha256(expected.read()).hexdigest(),
> +                    hashlib.sha256(actual.read()).hexdigest())
> +        # The hook is executable.
> +        self.assertTrue(os.stat(self.hook('hook.py')).st_mode & stat.S_IXUSR)
> 
> === added file 'turnip/pack/tests/test_hookrpc.py'
> --- turnip/pack/tests/test_hookrpc.py	1970-01-01 00:00:00 +0000
> +++ turnip/pack/tests/test_hookrpc.py	2015-03-30 09:20:00 +0000
> @@ -0,0 +1,126 @@
> +from __future__ import (
> +    absolute_import,
> +    print_function,
> +    unicode_literals,
> +    )
> +
> +from testtools import TestCase
> +from twisted.internet import defer
> +from twisted.test import proto_helpers
> +
> +from turnip.pack import hookrpc
> +
> +
> +class DummyJSONNetstringProtocol(hookrpc.JSONNetstringProtocol):
> +
> +    response_deferred = None
> +
> +    def __init__(self):
> +        self.test_value_log = []
> +        self.test_invalid_log = []
> +
> +    def valueReceived(self, val):
> +        self.test_value_log.append(val)
> +
> +    def invalidValueReceived(self, string):
> +        self.test_invalid_log.append(string)
> +
> +    def sendValue(self, value):
> +        # Hack to allow tests to block until a response is sent, since
> +        # dataReceived can't return a Deferred without breaking things.
> +        hookrpc.JSONNetstringProtocol.sendValue(self, value)
> +        if self.response_deferred is not None:
> +            d = self.response_deferred
> +            self.response_deferred = None
> +            d.callback()
> +
> +
> +class TestJSONNetStringProtocol(TestCase):
> +    """Test the JSON netstring protocol."""
> +
> +    def setUp(self):
> +        super(TestJSONNetStringProtocol, self).setUp()
> +        self.proto = DummyJSONNetstringProtocol()
> +        self.transport = proto_helpers.StringTransportWithDisconnection()
> +        self.transport.protocol = self.proto
> +        self.proto.makeConnection(self.transport)
> +
> +    def test_calls_valueReceived(self):
> +        # A valid netstring containing valid JSON is given to
> +        # valueReceived.
> +        self.proto.dataReceived(b'14:{"foo": "bar"},')
> +        self.proto.dataReceived(b'19:[{"it": ["works"]}],')
> +        self.assertEqual(
> +            [{"foo": "bar"}, [{"it": ["works"]}]],
> +            self.proto.test_value_log)
> +
> +    def test_calls_invalidValueReceived(self):
> +        # A valid nestring containing invalid JSON calls
> +        # invalidValueReceived. Framing is preserved, so the connection
> +        # need not be destroyed.
> +        self.proto.dataReceived(b'12:{"foo": "bar,')
> +        self.proto.dataReceived(b'3:"ga,')
> +        self.assertEqual([], self.proto.test_value_log)
> +        self.assertEqual(
> +            [b'{"foo": "bar', b'"ga'], self.proto.test_invalid_log)
> +
> +    def test_sendValue(self):
> +        # sendValue serialises to JSON and encodes as a netstring.
> +        self.proto.sendValue({"yay": "it works"})
> +        self.assertEqual(b'19:{"yay": "it works"},', self.transport.value())
> +
> +
> +def async_rpc_method(proto, args):
> +    d = defer.Deferred()
> +    d.callback(list(args.items()))
> +    return d
> +
> +
> +def sync_rpc_method(proto, args):
> +    return list(args.items())
> +
> +
> +class TestRPCServerProtocol(TestCase):
> +    """Test the socket server that handles git hook callbacks."""
> +
> +    def setUp(self):
> +        super(TestRPCServerProtocol, self).setUp()
> +        self.proto = hookrpc.RPCServerProtocol({
> +            'sync': sync_rpc_method,
> +            'async': async_rpc_method,
> +            })
> +        self.transport = proto_helpers.StringTransportWithDisconnection()
> +        self.transport.protocol = self.proto
> +        self.proto.makeConnection(self.transport)
> +
> +    def test_call_sync(self):
> +        self.proto.dataReceived(b'28:{"op": "sync", "bar": "baz"},')
> +        self.assertEqual(
> +            b'28:{"result": [["bar", "baz"]]},', self.transport.value())
> +
> +    def test_call_async(self):
> +        self.proto.dataReceived(b'29:{"op": "async", "bar": "baz"},')
> +        self.assertEqual(
> +            b'28:{"result": [["bar", "baz"]]},', self.transport.value())
> +
> +    def test_bad_op(self):
> +        self.proto.dataReceived(b'27:{"op": "bar", "bar": "baz"},')
> +        self.assertEqual(
> +            b'28:{"error": "Unknown op: bar"},', self.transport.value())
> +
> +    def test_no_op(self):
> +        self.proto.dataReceived(b'28:{"nop": "bar", "bar": "baz"},')
> +        self.assertEqual(
> +            b'28:{"error": "No op specified"},', self.transport.value())
> +
> +    def test_bad_value(self):
> +        self.proto.dataReceived(b'14:["foo", "bar"],')
> +        self.assertEqual(
> +            b'42:{"error": "Command must be a JSON object"},',
> +            self.transport.value())
> +
> +    def test_bad_json(self):
> +        self.proto.dataReceived(b'12:["nop", "bar,')
> +        self.assertEqual(
> +            b'42:{"error": "Command must be a JSON object"},',
> +            self.transport.value())
> 
> === added file 'turnip/pack/tests/test_hooks.py'
> --- turnip/pack/tests/test_hooks.py	1970-01-01 00:00:00 +0000
> +++ turnip/pack/tests/test_hooks.py	2015-03-30 09:20:00 +0000
> @@ -0,0 +1,172 @@
> +from __future__ import (
> +    absolute_import,
> +    print_function,
> +    unicode_literals,
> +    )
> +
> +import os.path
> +import uuid
> +
> +from fixtures import TempDir
> +from testtools import TestCase
> +from testtools.deferredruntest import AsynchronousDeferredRunTest
> +from twisted.internet import (
> +    defer,
> +    protocol,
> +    reactor,
> +    )
> +
> +from turnip.pack import hookrpc
> +import turnip.pack.hooks
> +
> +
> +class HookProcessProtocol(protocol.ProcessProtocol):
> +
> +    def __init__(self, deferred, stdin):
> +        self.deferred = deferred
> +        self.stdin = stdin
> +        self.stdout = self.stderr = ''
> +
> +    def connectionMade(self):
> +        self.transport.write(self.stdin)
> +        self.transport.closeStdin()
> +
> +    def outReceived(self, data):
> +        self.stdout += data
> +
> +    def errReceived(self, data):
> +        self.stderr += data
> +
> +    def processEnded(self, status):
> +        self.deferred.callback(
> +            (status.value.exitCode, self.stdout, self.stderr))
> +
> +
> +class MockHookRPCHandler(hookrpc.HookRPCHandler):
> +
> +    def __init__(self):
> +        super(MockHookRPCHandler, self).__init__(None)
> +        self.notifications = []
> +
> +    def notifyPush(self, proto, args):
> +        self.notifications.append(self.ref_paths[args['key']])
> +
> +
> +class HookTestMixin(object):
> +    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=1)
> +
> +    old_sha1 = b'a' * 40
> +    new_sha1 = b'b' * 40
> +
> +    @property
> +    def hook_path(self):
> +        return os.path.join(
> +            os.path.dirname(turnip.pack.hooks.__file__), self.hook_name)
> +
> +    def handlePushNotification(self, path):
> +        self.notifications.append(path)
> +
> +    def setUp(self):
> +        super(HookTestMixin, self).setUp()
> +        self.hookrpc_handler = MockHookRPCHandler()
> +        self.hookrpc = hookrpc.HookRPCServerFactory(self.hookrpc_handler)
> +        dir = self.useFixture(TempDir()).path
> +        self.hookrpc_path = os.path.join(dir, 'hookrpc_sock')
> +        self.hookrpc_port = reactor.listenUNIX(
> +            self.hookrpc_path, self.hookrpc)
> +        self.addCleanup(self.hookrpc_port.stopListening)
> +
> +    def encodeRefs(self, updates):
> +        return b'\n'.join(
> +            b'%s %s %s' % (old, new, ref) for ref, old, new in updates)
> +
> +    @defer.inlineCallbacks
> +    def invokeHook(self, input, rules):
> +        key = str(uuid.uuid4())
> +        self.hookrpc_handler.registerKey(key, '/translated', list(rules))
> +        try:
> +            d = defer.Deferred()
> +            reactor.spawnProcess(
> +                HookProcessProtocol(d, input),
> +                self.hook_path, [self.hook_path],
> +                env={
> +                    b'TURNIP_HOOK_RPC_SOCK': self.hookrpc_path,
> +                    b'TURNIP_HOOK_RPC_KEY': key})
> +            code, stdout, stderr = yield d
> +        finally:
> +            self.hookrpc_handler.unregisterKey(key)
> +        defer.returnValue((code, stdout, stderr))
> +
> +    @defer.inlineCallbacks
> +    def assertAccepted(self, updates, rules):
> +        code, out, err = yield self.invokeHook(self.encodeRefs(updates), rules)
> +        self.assertEqual((0, b'', b''), (code, out, err))
> +
> +    @defer.inlineCallbacks
> +    def assertRejected(self, updates, rules, message):
> +        code, out, err = yield self.invokeHook(self.encodeRefs(updates), rules)
> +        self.assertEqual((1, message, b''), (code, out, err))
> +
> +
> +class TestPreReceiveHook(HookTestMixin, TestCase):
> +    """Tests for the git pre-receive hook."""
> +
> +    hook_name = 'pre-receive'
> +
> +    @defer.inlineCallbacks
> +    def test_accepted(self):
> +        # A single valid ref is accepted.
> +        yield self.assertAccepted(
> +            [(b'refs/heads/master', self.old_sha1, self.new_sha1)],
> +            [])
> +
> +    @defer.inlineCallbacks
> +    def test_rejected(self):
> +        # An invalid ref is rejected.
> +        yield self.assertRejected(
> +            [(b'refs/heads/verboten', self.old_sha1, self.new_sha1)],
> +            [b'refs/heads/verboten'],
> +            b"You can't push to refs/heads/verboten.\n")
> +
> +    @defer.inlineCallbacks
> +    def test_wildcard(self):
> +        # "*" in a rule matches any path segment.
> +        yield self.assertRejected(
> +            [(b'refs/heads/foo', self.old_sha1, self.new_sha1),
> +             (b'refs/tags/bar', self.old_sha1, self.new_sha1),
> +             (b'refs/tags/foo', self.old_sha1, self.new_sha1),
> +             (b'refs/baz/quux', self.old_sha1, self.new_sha1)],
> +            [b'refs/*/foo', b'refs/baz/*'],
> +            b"You can't push to refs/heads/foo.\n"
> +            b"You can't push to refs/tags/foo.\n"
> +            b"You can't push to refs/baz/quux.\n")
> +
> +    @defer.inlineCallbacks
> +    def test_rejected_multiple(self):
> +        # A combination of valid and invalid refs is still rejected.
> +        yield self.assertRejected(
> +            [(b'refs/heads/verboten', self.old_sha1, self.new_sha1),
> +             (b'refs/heads/master', self.old_sha1, self.new_sha1),
> +             (b'refs/heads/super-verboten', self.old_sha1, self.new_sha1)],
> +            [b'refs/heads/verboten', b'refs/heads/super-verboten'],
> +            b"You can't push to refs/heads/verboten.\n"
> +            b"You can't push to refs/heads/super-verboten.\n")
> +
> +
> +class TestPostReceiveHook(HookTestMixin, TestCase):
> +    """Tests for the git post-receive hook."""
> +
> +    hook_name = 'post-receive'
> +
> +    @defer.inlineCallbacks
> +    def test_notifies(self):
> +        # The notification callback is invoked with the storage path.
> +        yield self.assertAccepted(
> +            [(b'refs/heads/foo', self.old_sha1, self.new_sha1)], [])
> +        self.assertEqual(['/translated'], self.hookrpc_handler.notifications)
> +
> +    @defer.inlineCallbacks
> +    def test_does_not_notify_on_empty_push(self):
> +        # No notification is sent for an empty push.
> +        yield self.assertAccepted([], [])
> +        self.assertEqual([], self.hookrpc_handler.notifications)
> 
> === modified file 'turnipserver.py'
> --- turnipserver.py	2015-03-29 09:50:58 +0000
> +++ turnipserver.py	2015-03-30 09:20:00 +0000
> @@ -15,6 +15,10 @@
>      PackFrontendFactory,
>      PackVirtFactory,
>      )
> +from turnip.pack.hookrpc import (
> +    HookRPCHandler,
> +    HookRPCServerFactory,
> +    )
>  from turnip.pack.http import SmartHTTPFrontendResource
>  from turnip.pack.ssh import SmartSSHService
>  
> @@ -34,8 +38,14 @@
>  # Start a pack storage service on 19418, pointed at by a pack frontend
>  # on 9418 (the default git:// port), a smart HTTP frontend on 9419, and
>  # a smart SSH frontend on 9422.
> -reactor.listenTCP(PACK_BACKEND_PORT,
> -                  PackBackendFactory(REPO_STORE))
> +
> +hookrpc_handler = HookRPCHandler(VIRTINFO_ENDPOINT)
> +hookrpc_path = os.path.join(REPO_STORE, 'hookrpc_sock')
> +reactor.listenUNIX(hookrpc_path, HookRPCServerFactory(hookrpc_handler))
> +
> +reactor.listenTCP(
> +    PACK_BACKEND_PORT,
> +    PackBackendFactory(REPO_STORE, hookrpc_handler, hookrpc_path))
>  reactor.listenTCP(PACK_VIRT_PORT,
>                    PackVirtFactory('localhost',
>                                    PACK_BACKEND_PORT,
> 


-- 
https://code.launchpad.net/~wgrant/turnip/pre-receive/+merge/254542
Your team Launchpad code reviewers is subscribed to branch lp:turnip.