launchpad-reviewers team mailing list archive

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

Thiago F. Pappacena has proposed merging ~pappacena/turnip:protocol-v2-parameters-parsing into turnip:master with ~pappacena/turnip:scenario-tests as a prerequisite.

Commit message:
Adding parsing capability for v2 protocol parameters, and encoding of more data types for our internal protocol

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/turnip/+git/turnip/+merge/389106
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/turnip:protocol-v2-parameters-parsing into turnip:master.

diff --git a/turnip/pack/helpers.py b/turnip/pack/helpers.py
index 9cf411d..3804fde 100644
--- a/turnip/pack/helpers.py
+++ b/turnip/pack/helpers.py
@@ -7,6 +7,7 @@ from __future__ import (
     unicode_literals,
     )
 
+from collections import OrderedDict
 import enum
 import hashlib
 import os.path
@@ -25,6 +26,8 @@ import yaml
 import turnip.pack.hooks
 
 
+FLUSH_PKT = b'0000'
+DELIM_PKT = b'0001'
 PKT_LEN_SIZE = 4
 PKT_PAYLOAD_MAX = 65520
 INCOMPLETE_PKT = object()
@@ -33,7 +36,7 @@ INCOMPLETE_PKT = object()
 def encode_packet(payload):
     if payload is None:
         # flush-pkt.
-        return b'0000'
+        return FLUSH_PKT
     else:
         # data-pkt
         if len(payload) > PKT_PAYLOAD_MAX:
@@ -47,7 +50,7 @@ def decode_packet(input):
     """Consume a packet, returning the payload and any unconsumed tail."""
     if len(input) < PKT_LEN_SIZE:
         return (INCOMPLETE_PKT, input)
-    if input.startswith(b'0000'):
+    if input.startswith(FLUSH_PKT):
         # flush-pkt
         return (None, input[PKT_LEN_SIZE:])
     else:
@@ -64,6 +67,41 @@ def decode_packet(input):
         return (input[PKT_LEN_SIZE:pkt_len], input[pkt_len:])
 
 
+def decode_packet_list(data):
+    remaining = data
+    retval = []
+    while remaining:
+        pkt, remaining = decode_packet(remaining)
+        retval.append(pkt)
+    return retval
+
+
+def decode_protocol_v2_params(data):
+    """Parse the protocol v2 extra parameters hidden behind the end of v1
+    protocol.
+
+    :return: An ordered dict with parsed v2 parameters.
+    """
+    params = OrderedDict()
+    cmd, remaining = decode_packet(data)
+    cmd = cmd.split(b'=', 1)[-1].strip()
+    capabilities, args = remaining.split(DELIM_PKT)
+    params[b"command"] = cmd
+    params[b"capabilities"] = decode_packet_list(capabilities)
+    for arg in decode_packet_list(args):
+        if arg is None:
+            continue
+        arg = arg.strip('\n')
+        if b' ' in arg:
+            k, v = arg.split(b' ', 1)
+            if k not in params:
+                params[k] = []
+            params[k].append(v)
+        else:
+            params[arg] = b""
+    return params
+
+
 def decode_request(data):
     """Decode a turnip-proto-request.
 
@@ -77,10 +115,11 @@ def decode_request(data):
     bits = rest.split(b'\0')
     # Following the command is a pathname, then any number of named
     # parameters. Each of these is NUL-terminated.
-    if len(bits) < 2 or bits[-1] != b'':
+    # After that, v1 should end (v2 might have extra commands).
+    if len(bits) < 2 or (b'version=2' not in bits and bits[-1] != b''):
         raise ValueError('Invalid git-proto-request')
     pathname = bits[0]
-    params = {}
+    params = OrderedDict()
     for index, param in enumerate(bits[1:-1]):
         if param == b'':
             if (index < len(bits) - 1):
@@ -94,7 +133,27 @@ def decode_request(data):
         if name in params:
             raise ValueError('Parameters must not be repeated')
         params[name] = value
-    return (command, pathname, params)
+
+    # If there are remaining bits at the end, we must be dealing with v2
+    # protocol. So, we append v2 parameters at the end of original parameters.
+    if bits[-1]:
+        for k, v in decode_protocol_v2_params(bits[-1]).items():
+            params[k] = v
+
+    return command, pathname, params
+
+
+def get_encoded_value(value):
+    """Encode a value for serialization on encode_request"""
+    if value is None:
+        return b''
+    if isinstance(value, list):
+        if any(b'\n' in i for i in value):
+            raise ValueError('Metacharacter in list argument')
+        return b"\n".join(get_encoded_value(i) for i in value)
+    if isinstance(value, bool):
+        return b'1' if value else b''
+    return six.ensure_binary(value)
 
 
 def encode_request(command, pathname, params):
@@ -105,9 +164,8 @@ def encode_request(command, pathname, params):
     if b' ' in command or b'\0' in pathname:
         raise ValueError('Metacharacter in arguments')
     bits = [pathname]
-    for name in sorted(params):
-        value = params[name]
-        value = six.ensure_binary(value) if value is not None else b''
+    for name in params:
+        value = get_encoded_value(params[name])
         name = six.ensure_binary(name)
         if b'=' in name or b'\0' in name + value:
             raise ValueError('Metacharacter in arguments')
diff --git a/turnip/pack/tests/test_helpers.py b/turnip/pack/tests/test_helpers.py
index a7a3870..bad7117 100644
--- a/turnip/pack/tests/test_helpers.py
+++ b/turnip/pack/tests/test_helpers.py
@@ -9,6 +9,8 @@ from __future__ import (
 
 import os.path
 import re
+from collections import OrderedDict
+
 import stat
 import sys
 from textwrap import dedent
@@ -23,7 +25,7 @@ from testtools import TestCase
 
 from turnip.pack import helpers
 import turnip.pack.hooks
-
+from turnip.pack.helpers import FLUSH_PKT
 
 TEST_DATA = b'0123456789abcdef'
 TEST_PKT = b'00140123456789abcdef'
@@ -173,6 +175,27 @@ class TestDecodeRequest(TestCase):
             b'git-do-stuff /foo\0host=foo\0host=bar\0',
             b'Parameters must not be repeated')
 
+    def test_v2_extra_commands(self):
+        data = (
+            b"git-upload-pack b306d" +
+            b"\x00turnip-x=yes\x00turnip-request-id=123\x00" +
+            b"version=2\x000014command=ls-refs\n0014agent=git/2.25.1" +
+            b'00010009peel\n000csymrefs\n0014ref-prefix HEAD\n' +
+            b'001bref-prefix refs/heads/\n'
+            b'001aref-prefix refs/tags/\n0000' +
+            FLUSH_PKT)
+        decoded = helpers.decode_request(data)
+        self.assertEqual((b'git-upload-pack', b'b306d', OrderedDict([
+            (b'turnip-x', b'yes'),
+            (b'turnip-request-id', b'123'),
+            (b'version', b'2'),
+            (b'command', b'ls-refs'),
+            (b'capabilities', [b'agent=git/2.25.1']),
+            (b'peel', b''),
+            (b'symrefs', b''),
+            (b'ref-prefix', [b'HEAD', b'refs/heads/', b'refs/tags/'])
+            ])), decoded)
+
 
 class TestEncodeRequest(TestCase):
     """Test git-proto-request encoding."""
@@ -194,6 +217,24 @@ class TestEncodeRequest(TestCase):
                 b'git-do-stuff', b'/some/path',
                 {b'host': b'example.com', b'user': b'foo=bar'}))
 
+    def test_with_parameters_respects_order(self):
+        params = OrderedDict([
+            (b'a', b'1'), (b'z', b'2'), (b'b', b'3')])
+        self.assertEqual(
+            b'git-do-stuff /some/path\0a=1\0z=2\0b=3\0',
+            helpers.encode_request(b'git-do-stuff', b'/some/path', params))
+
+    def test_with_special_type_values(self):
+        params = OrderedDict([
+            (b'a', None),
+            (b'b', True),
+            (b'c', False),
+            (b'd', [b'x', b'y', b'z'])])
+        encoded = helpers.encode_request(
+            b'git-do-stuff', b'/some/path', params)
+        self.assertEqual(
+            b'git-do-stuff /some/path\0a=\0b=1\0c=\0d=x\ny\nz\0', encoded)
+
     def test_rejects_meta_in_args(self):
         self.assertInvalid(
             b'git do-stuff', b'/some/path', {b'host': b'example.com'},