{"thread":{"id":"46664","subject":"[RFC 0/7] transitioning to protocol v2","startedAt":"2017-08-24T22:54:01Z","lastAt":"2017-09-01T23:20:21Z","messageCount":31,"participants":["Brandon Williams","Junio C Hamano","Stefan Beller","Jeff King","Jonathan Nieder","Jeff Hostetler","Kevin Daudt","Bryan Turner"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"327163","messageId":"20170824225328.8174-1-bmwill@google.com","threadId":"46664","inReplyTo":null,"subject":"[RFC 0/7] transitioning to protocol v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-24T22:53:21Z","receivedAt":"2017-08-24T22:54:01Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Another version of Git's wire protocol is a topic that has been discussed and\nattempted by many in the community over the years.  The biggest challenge, as\nfar as I understand, has been coming up with a transition plan to using the new\nserver without breaking existing clients and servers.  As such this RFC is\nreally only concerned with solidifying a transition plan.  Once it has been\ndecided how we can transition to a new protocol we can get into decided what\nthis new protocol would look like (though it would obviously eliminate the ref\nadvertisement ;).\n\nThe best way to preserve functionality with old servers and clients would be to\ncommunicate using the same end point but have the client send a bit of extra\ninformation with its initial request.  This extra information would need to be\nsent in such a way that old servers ignore it and operate normally (using\nprotocol v1).  The client would then need to be able to look at a server's\nresponse to determine whether the server understands and is speaking v2 or has\nignored the clients request to use a newer protocol and is speaking v1.\n\nPatches 1-5 enable a client to unconditionally send this back-channel\ninformation to a server.  This is done by sending a version number after a\nsecond NUL byte in git://, in the envvar GIT_PROTOCOL in file:// and ssh://,\nand in an http header in http://, https://.  Patches 6-7 teach a client and\nupload-pack to send and recognize a request to use protocol v2.\n\nThe biggest question I'm trying to answer is if these are reasonable ways with\nwhich to communicate a request to a server to use a newer protocol, without\nbreaking current servers/clients.  As far as I've tested, with patches 1-5\napplied I can still communicate with current servers without causing any\nproblems.\n\nAny comments/discussion is welcome!\n\nBrandon Williams (7):\n  pkt-line: add packet_write function\n  pkt-line: add strbuf_packet_read\n  protocol: tell server that the client understands v2\n  t: fix ssh tests to cope with using '-o SendEnv=GIT_PROTOCOL'\n  http: send Git-Protocol-Version header\n  transport: teach client to recognize v2 server response\n  upload-pack: ack version 2\n\n builtin/fetch-pack.c         |   4 +-\n builtin/send-pack.c          |   5 +-\n connect.c                    | 196 +++++++++++++++++++++++++++----------------\n daemon.c                     |  28 ++++++-\n http.c                       |   7 ++\n pkt-line.c                   |  27 ++++++\n pkt-line.h                   |   2 +\n remote-curl.c                |   7 +-\n remote.h                     |  22 ++++-\n t/lib-proto-disable.sh       |   1 +\n t/t5551-http-fetch-smart.sh  |   2 +\n t/t5601-clone.sh             |  10 +--\n t/t5602-clone-remote-exec.sh |   4 +-\n transport.c                  |  60 +++++++++++--\n upload-pack.c                |  11 +++\n 15 files changed, 286 insertions(+), 100 deletions(-)\n\n-- \n2.14.1.342.g6490525c54-goog\n\n"},{"id":"327164","messageId":"20170824225328.8174-2-bmwill@google.com","threadId":"46664","inReplyTo":"20170824225328.8174-1-bmwill@google.com","subject":"[RFC 1/7] pkt-line: add packet_write function","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-24T22:53:22Z","receivedAt":"2017-08-24T22:54:03Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Add a function which can be used to write the contents of an arbitrary\nbuffer.  This makes it easy to build up data in a strbuf before writing\nthe packet.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n pkt-line.c | 6 ++++++\n pkt-line.h | 1 +\n 2 files changed, 7 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 7db911957..cf98f371b 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -188,6 +188,12 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n \treturn error(\"packet write failed\");\n }\n \n+void packet_write(const int fd_out, const char *buf, size_t size)\n+{\n+\tif (packet_write_gently(fd_out, buf, size))\n+\t\tdie_errno(\"packet write failed\");\n+}\n+\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n {\n \tva_list args;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 66ef610fc..d9e9783b1 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -22,6 +22,7 @@\n void packet_flush(int fd);\n void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n void packet_buf_flush(struct strbuf *buf);\n+void packet_write(const int fd_out, const char *buf, size_t size);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n int packet_flush_gently(int fd);\n int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n-- \n2.14.1.342.g6490525c54-goog\n\n"},{"id":"327165","messageId":"20170824225328.8174-3-bmwill@google.com","threadId":"46664","inReplyTo":"20170824225328.8174-1-bmwill@google.com","subject":"[RFC 2/7] pkt-line: add strbuf_packet_read","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-24T22:53:23Z","receivedAt":"2017-08-24T22:54:05Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Add function which can be used to read the contents of a single pkt-line\ninto a strbuf.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n pkt-line.c | 21 +++++++++++++++++++++\n pkt-line.h |  1 +\n 2 files changed, 22 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex cf98f371b..875524ab8 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -312,6 +312,27 @@ int packet_read(int fd, char **src_buf, size_t *src_len,\n \treturn len;\n }\n \n+int strbuf_packet_read(int fd_in, struct strbuf *sb_out, int options)\n+{\n+\tint packet_len;\n+\tstrbuf_grow(sb_out, LARGE_PACKET_DATA_MAX);\n+\tpacket_len = packet_read(fd_in, NULL, NULL,\n+\t\t\t\t /*\n+\t\t\t\t  * strbuf_grow() above always allocates one extra byte to\n+\t\t\t\t  * store a '\\0' at the end of the string. packet_read()\n+\t\t\t\t  * writes a '\\0' extra byte at the end, too. Let it know\n+\t\t\t\t  * that there is already room for the extra byte.\n+\t\t\t\t  */\n+\t\t\t\t sb_out->buf, LARGE_PACKET_DATA_MAX+1,\n+\t\t\t\t options);\n+\tif (packet_len < 0)\n+\t\treturn packet_len;\n+\n+\tsb_out->len = packet_len;\n+\n+\treturn sb_out->len;\n+}\n+\n static char *packet_read_line_generic(int fd,\n \t\t\t\t      char **src, size_t *src_len,\n \t\t\t\t      int *dst_len)\ndiff --git a/pkt-line.h b/pkt-line.h\nindex d9e9783b1..c24c4f290 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -65,6 +65,7 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n int packet_read(int fd, char **src_buffer, size_t *src_len, char\n \t\t*buffer, unsigned size, int options);\n \n+int strbuf_packet_read(int fd_in, struct strbuf *sb_out, int options);\n /*\n  * Convenience wrapper for packet_read that is not gentle, and sets the\n  * CHOMP_NEWLINE option. The return value is NULL for a flush packet,\n-- \n2.14.1.342.g6490525c54-goog\n\n"},{"id":"327166","messageId":"20170824225328.8174-4-bmwill@google.com","threadId":"46664","inReplyTo":"20170824225328.8174-1-bmwill@google.com","subject":"[RFC 3/7] protocol: tell server that the client understands v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-24T22:53:24Z","receivedAt":"2017-08-24T22:54:07Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Teach the connection logic to tell a serve that it understands protocol\nv2.  This is done in 2 different ways for the built in protocols.\n\n1. git://\n   A normal request is structured as \"command path/to/repo\\0host=..\\0\"\n   and due to a bug in an old version of git-daemon 73bb33a94 (daemon:\n   Strictly parse the \"extra arg\" part of the command, 2009-06-04) we\n   aren't able to place any extra args (separated by NULs) besides the\n   host.  In order to get around this limitation put protocol version\n   information after a second NUL byte so the request is structured\n   like: \"command path/to/repo\\0host=..\\0\\0version=2\\0\".  git-daemon can\n   then parse out the version number and set GIT_PROTOCOL.\n\n2. ssh://, file://\n   Set GIT_PROTOCOL envvar with the desired protocol version.  The\n   envvar can be sent across ssh by using '-o SendEnv=GIT_PROTOCOL' and\n   having the server whitelist this envvar.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n connect.c | 31 ++++++++++++++++++++++++++-----\n daemon.c  | 28 +++++++++++++++++++++++++---\n 2 files changed, 51 insertions(+), 8 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 49b28b83b..d609267be 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -793,6 +793,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n \t\tconn = NULL;\n \t} else if (protocol == PROTO_GIT) {\n+\t\tstruct strbuf request = STRBUF_INIT;\n \t\t/*\n \t\t * Set up virtual host information based on where we will\n \t\t * connect, unless the user has overridden us in\n@@ -820,12 +821,23 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t * Note: Do not add any other headers here!  Doing so\n \t\t * will cause older git-daemon servers to crash.\n \t\t */\n-\t\tpacket_write_fmt(fd[1],\n-\t\t\t     \"%s %s%chost=%s%c\",\n-\t\t\t     prog, path, 0,\n-\t\t\t     target_host, 0);\n+\t\tstrbuf_addf(&request,\n+\t\t\t    \"%s %s%chost=%s%c\",\n+\t\t\t    prog, path, 0,\n+\t\t\t    target_host, 0);\n+\n+\t\t/* If using a new version put that stuff here after a second null byte */\n+\t\tstrbuf_addch(&request, '\\0');\n+\t\tstrbuf_addf(&request, \"version=%d%c\", 2, '\\0');\n+\t\t/* subsequent supported versions can also be added */\n+\t\tstrbuf_addf(&request, \"version=%d%c\", 3, '\\0');\n+\n+\t\tpacket_write(fd[1], request.buf, request.len);\n+\n \t\tfree(target_host);\n+\t\tstrbuf_release(&request);\n \t} else {\n+\t\tconst char *const *var;\n \t\tconn = xmalloc(sizeof(*conn));\n \t\tchild_process_init(conn);\n \n@@ -837,7 +849,9 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tsq_quote_buf(&cmd, path);\n \n \t\t/* remove repo-local variables from the environment */\n-\t\tconn->env = local_repo_env;\n+\t\tfor (var = local_repo_env; *var; var++)\n+\t\t\targv_array_push(&conn->env_array, *var);\n+\n \t\tconn->use_shell = 1;\n \t\tconn->in = conn->out = -1;\n \t\tif (protocol == PROTO_SSH) {\n@@ -890,6 +904,12 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t}\n \n \t\t\targv_array_push(&conn->args, ssh);\n+\n+\t\t\t/* protocol v2! */\n+\t\t\targv_array_push(&conn->args, \"-o\");\n+\t\t\targv_array_push(&conn->args, \"SendEnv=GIT_PROTOCOL\");\n+\t\t\targv_array_push(&conn->env_array, \"GIT_PROTOCOL=2\");\n+\n \t\t\tif (flags & CONNECT_IPV4)\n \t\t\t\targv_array_push(&conn->args, \"-4\");\n \t\t\telse if (flags & CONNECT_IPV6)\n@@ -904,6 +924,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\targv_array_push(&conn->args, ssh_host);\n \t\t} else {\n \t\t\ttransport_check_allowed(\"file\");\n+\t\t\targv_array_push(&conn->env_array, \"GIT_PROTOCOL=2\");\n \t\t}\n \t\targv_array_push(&conn->args, cmd.buf);\n \ndiff --git a/daemon.c b/daemon.c\nindex 30747075f..76a7b2d64 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -574,7 +574,7 @@ static void canonicalize_client(struct strbuf *out, const char *in)\n /*\n  * Read the host as supplied by the client connection.\n  */\n-static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)\n+static char *parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)\n {\n \tchar *val;\n \tint vallen;\n@@ -602,6 +602,22 @@ static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)\n \t\tif (extra_args < end && *extra_args)\n \t\t\tdie(\"Invalid request\");\n \t}\n+\n+\treturn extra_args;\n+}\n+\n+static void parse_extra_args(const char *extra_args, int buflen)\n+{\n+\tconst char *end = extra_args + buflen;\n+\n+\tfor (; extra_args < end; extra_args += strlen(extra_args) + 1) {\n+\t\tconst char *arg = extra_args;\n+\t\tif (skip_prefix(arg, \"version=\", &arg))\n+\t\t\tfprintf(stderr, \"VERSION=%s\\n\", arg);\n+\n+\t\tfprintf(stderr, \"%s\\n\", extra_args);\n+\n+\t}\n }\n \n /*\n@@ -716,8 +732,14 @@ static int execute(void)\n \t\tpktlen--;\n \t}\n \n-\tif (len != pktlen)\n-\t\tparse_host_arg(&hi, line + len + 1, pktlen - len - 1);\n+\tif (len != pktlen) {\n+\t\tconst char *extra_args;\n+\t\t/* retreive host */\n+\t\textra_args = parse_host_arg(&hi, line + len + 1, pktlen - len - 1);\n+\n+\t\t/* determine version */\n+\t\tparse_extra_args(extra_args + 1, pktlen - (extra_args - line) - 1);\n+\t}\n \n \tfor (i = 0; i < ARRAY_SIZE(daemon_service); i++) {\n \t\tstruct daemon_service *s = &(daemon_service[i]);\n-- \n2.14.1.342.g6490525c54-goog\n\n"},{"id":"327167","messageId":"20170824225328.8174-6-bmwill@google.com","threadId":"46664","inReplyTo":"20170824225328.8174-1-bmwill@google.com","subject":"[RFC 5/7] http: send Git-Protocol-Version header","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-24T22:53:26Z","receivedAt":"2017-08-24T22:54:08Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Tell a serve that protocol v2 can be used by sending an http header\nindicating this.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n http.c                      | 7 +++++++\n t/t5551-http-fetch-smart.sh | 2 ++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex fa8666a21..504a14a5a 100644\n--- a/http.c\n+++ b/http.c\n@@ -896,6 +896,11 @@ static void set_from_env(const char **var, const char *envname)\n \t\t*var = val;\n }\n \n+static const char *get_version(void)\n+{\n+\treturn \"Git-Protocol-Version: 2\";\n+}\n+\n void http_init(struct remote *remote, const char *url, int proactive_auth)\n {\n \tchar *low_speed_limit;\n@@ -926,6 +931,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \tif (remote)\n \t\tvar_override(&http_proxy_authmethod, remote->http_proxy_authmethod);\n \n+\textra_http_headers = curl_slist_append(extra_http_headers, get_version());\n+\n \tpragma_header = curl_slist_append(http_copy_default_headers(),\n \t\t\"Pragma: no-cache\");\n \tno_pragma_header = curl_slist_append(http_copy_default_headers(),\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex a51b7e20d..ce13f2425 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -27,6 +27,7 @@ cat >exp <<EOF\n > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n > Accept: */*\n > Accept-Encoding: gzip\n+> Git-Protocol-Version: 2\n > Pragma: no-cache\n < HTTP/1.1 200 OK\n < Pragma: no-cache\n@@ -34,6 +35,7 @@ cat >exp <<EOF\n < Content-Type: application/x-git-upload-pack-advertisement\n > POST /smart/repo.git/git-upload-pack HTTP/1.1\n > Accept-Encoding: gzip\n+> Git-Protocol-Version: 2\n > Content-Type: application/x-git-upload-pack-request\n > Accept: application/x-git-upload-pack-result\n > Content-Length: xxx\n-- \n2.14.1.342.g6490525c54-goog\n\n"},{"id":"327168","messageId":"20170824225328.8174-7-bmwill@google.com","threadId":"46664","inReplyTo":"20170824225328.8174-1-bmwill@google.com","subject":"[RFC 6/7] transport: teach client to recognize v2 server response","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-24T22:53:27Z","receivedAt":"2017-08-24T22:54:12Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Teach a client to recognize that a server understand protocol v2 by\nlooking at the first pkt-line the server sends in response.  This is\ndone by looking for the response \"version 2\" sent by upload-pack.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n builtin/fetch-pack.c |   4 +-\n builtin/send-pack.c  |   5 +-\n connect.c            | 165 ++++++++++++++++++++++++++++++---------------------\n remote-curl.c        |   7 ++-\n remote.h             |  22 ++++++-\n transport.c          |  60 ++++++++++++++++---\n 6 files changed, 178 insertions(+), 85 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 366b9d13f..a2a5e1c73 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -52,6 +52,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tstruct fetch_pack_args args;\n \tstruct oid_array shallow = OID_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n+\tstruct remote_refs_scanner scanner;\n \n \tpacket_trace_identity(\"fetch-pack\");\n \n@@ -193,7 +194,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\tif (!conn)\n \t\t\treturn args.diag_url ? 0 : 1;\n \t}\n-\tget_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);\n+\tremote_refs_scanner_init(&scanner, &ref, 0, NULL, &shallow);\n+\tget_remote_heads(fd[0], NULL, 0, &scanner);\n \n \tref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,\n \t\t\t &shallow, pack_lockfile_ptr);\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex fc4f0bb5f..92ec1f871 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -154,6 +154,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tint progress = -1;\n \tint from_stdin = 0;\n \tstruct push_cas_option cas = {0};\n+\tstruct remote_refs_scanner scanner;\n \n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbose),\n@@ -256,8 +257,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\t\targs.verbose ? CONNECT_VERBOSE : 0);\n \t}\n \n-\tget_remote_heads(fd[0], NULL, 0, &remote_refs, REF_NORMAL,\n-\t\t\t &extra_have, &shallow);\n+\tremote_refs_scanner_init(&scanner, &remote_refs, REF_NORMAL, &extra_have, &shallow);\n+\tget_remote_heads(fd[0], NULL, 0, &scanner);\n \n \ttransport_verify_remote_names(nr_refspecs, refspecs);\n \ndiff --git a/connect.c b/connect.c\nindex d609267be..732b651d9 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -107,97 +107,124 @@ static void annotate_refs_with_symref_info(struct ref *ref)\n \tstring_list_clear(&symref, 0);\n }\n \n-/*\n- * Read all the refs from the other end\n- */\n-struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n+void remote_refs_scanner_init(struct remote_refs_scanner *scanner,\n \t\t\t      struct ref **list, unsigned int flags,\n \t\t\t      struct oid_array *extra_have,\n \t\t\t      struct oid_array *shallow_points)\n {\n-\tstruct ref **orig_list = list;\n+\tmemset(scanner, 0, sizeof(*scanner));\n+\n+\tscanner->orig_list = list;\n+\t*list = NULL;\n+\tscanner->list = list;\n+\tscanner->flags = flags;\n+\tscanner->extra_have = extra_have;\n+\tscanner->shallow_points = shallow_points;\n+}\n+\n+int process_ref(struct remote_refs_scanner *scanner,\n+\t\tconst char *buffer, int len)\n+{\n+\tstruct ref *ref;\n+\tstruct object_id old_oid;\n+\tconst char *name;\n+\tint name_len;\n+\tconst char *arg;\n+\tint ret = 1;\n+\n+\tif (len < 0)\n+\t\tdie_initial_contact(scanner->seen_response);\n+\n+\tif (!len) {\n+\t\tret = 0;\n+\t\tgoto out;\n+\t}\n+\n+\tif (len > 4 && skip_prefix(buffer, \"ERR \", &arg))\n+\t\tdie(\"remote error: %s\", arg);\n+\n+\tif (len == GIT_SHA1_HEXSZ + strlen(\"shallow \") &&\n+\t    skip_prefix(buffer, \"shallow \", &arg)) {\n+\t\tif (get_oid_hex(arg, &old_oid))\n+\t\t\tdie(\"protocol error: expected shallow sha-1, got '%s'\", arg);\n+\t\tif (!scanner->shallow_points)\n+\t\t\tdie(\"repository on the other end cannot be shallow\");\n+\t\toid_array_append(scanner->shallow_points, &old_oid);\n+\t\tgoto out;\n+\t}\n+\n+\tif (len < GIT_SHA1_HEXSZ + 2 || get_oid_hex(buffer, &old_oid) ||\n+\t    buffer[GIT_SHA1_HEXSZ] != ' ')\n+\t\tdie(\"protocol error: expected sha/ref, got '%s'\", buffer);\n+\tname = buffer + GIT_SHA1_HEXSZ + 1;\n+\n+\tname_len = strlen(name);\n+\tif (len != name_len + GIT_SHA1_HEXSZ + 1) {\n+\t\tfree(server_capabilities);\n+\t\tserver_capabilities = xstrdup(name + name_len + 1);\n+\t}\n+\n+\tif (scanner->extra_have && !strcmp(name, \".have\")) {\n+\t\toid_array_append(scanner->extra_have, &old_oid);\n+\t\tgoto out;\n+\t}\n+\n+\tif (!strcmp(name, \"capabilities^{}\")) {\n+\t\tif (scanner->seen_response)\n+\t\t\tdie(\"protocol error: unexpected capabilities^{}\");\n+\t\tif (scanner->recieved_dummy_capabilities_ref)\n+\t\t\tdie(\"protocol error: multiple capabilities^{}\");\n+\t\tscanner->recieved_dummy_capabilities_ref = 1;\n+\t\tret = 1;\n+\t\tgoto out;\n+\t}\n+\n+\tif (!check_ref(name, scanner->flags))\n+\t\tgoto out;\n+\n+\tif (scanner->recieved_dummy_capabilities_ref)\n+\t\tdie(\"protocol error: unexpected ref after capabilities^{}\");\n+\n+\tref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);\n+\toidcpy(&ref->old_oid, &old_oid);\n+\n+\t*scanner->list = ref;\n+\tscanner->list = &ref->next;\n+\n+out:\n+\tscanner->seen_response = 1;\n+\tscanner->done = !ret;\n+\treturn ret;\n+}\n \n+/*\n+ * Read all the refs from the other end\n+ */\n+struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n+\t\t\t      struct remote_refs_scanner *scanner)\n+{\n \t/*\n \t * A hang-up after seeing some response from the other end\n \t * means that it is unexpected, as we know the other end is\n \t * willing to talk to us.  A hang-up before seeing any\n \t * response does not necessarily mean an ACL problem, though.\n \t */\n-\tint saw_response;\n-\tint got_dummy_ref_with_capabilities_declaration = 0;\n-\n-\t*list = NULL;\n-\tfor (saw_response = 0; ; saw_response = 1) {\n-\t\tstruct ref *ref;\n-\t\tstruct object_id old_oid;\n-\t\tchar *name;\n-\t\tint len, name_len;\n+\twhile (!scanner->done) {\n \t\tchar *buffer = packet_buffer;\n-\t\tconst char *arg;\n+\t\tint len;\n \n \t\tlen = packet_read(in, &src_buf, &src_len,\n \t\t\t\t  packet_buffer, sizeof(packet_buffer),\n \t\t\t\t  PACKET_READ_GENTLE_ON_EOF |\n \t\t\t\t  PACKET_READ_CHOMP_NEWLINE);\n-\t\tif (len < 0)\n-\t\t\tdie_initial_contact(saw_response);\n \n-\t\tif (!len)\n+\t\tif (!process_ref(scanner, buffer, len))\n \t\t\tbreak;\n-\n-\t\tif (len > 4 && skip_prefix(buffer, \"ERR \", &arg))\n-\t\t\tdie(\"remote error: %s\", arg);\n-\n-\t\tif (len == GIT_SHA1_HEXSZ + strlen(\"shallow \") &&\n-\t\t\tskip_prefix(buffer, \"shallow \", &arg)) {\n-\t\t\tif (get_oid_hex(arg, &old_oid))\n-\t\t\t\tdie(\"protocol error: expected shallow sha-1, got '%s'\", arg);\n-\t\t\tif (!shallow_points)\n-\t\t\t\tdie(\"repository on the other end cannot be shallow\");\n-\t\t\toid_array_append(shallow_points, &old_oid);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (len < GIT_SHA1_HEXSZ + 2 || get_oid_hex(buffer, &old_oid) ||\n-\t\t\tbuffer[GIT_SHA1_HEXSZ] != ' ')\n-\t\t\tdie(\"protocol error: expected sha/ref, got '%s'\", buffer);\n-\t\tname = buffer + GIT_SHA1_HEXSZ + 1;\n-\n-\t\tname_len = strlen(name);\n-\t\tif (len != name_len + GIT_SHA1_HEXSZ + 1) {\n-\t\t\tfree(server_capabilities);\n-\t\t\tserver_capabilities = xstrdup(name + name_len + 1);\n-\t\t}\n-\n-\t\tif (extra_have && !strcmp(name, \".have\")) {\n-\t\t\toid_array_append(extra_have, &old_oid);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!strcmp(name, \"capabilities^{}\")) {\n-\t\t\tif (saw_response)\n-\t\t\t\tdie(\"protocol error: unexpected capabilities^{}\");\n-\t\t\tif (got_dummy_ref_with_capabilities_declaration)\n-\t\t\t\tdie(\"protocol error: multiple capabilities^{}\");\n-\t\t\tgot_dummy_ref_with_capabilities_declaration = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!check_ref(name, flags))\n-\t\t\tcontinue;\n-\n-\t\tif (got_dummy_ref_with_capabilities_declaration)\n-\t\t\tdie(\"protocol error: unexpected ref after capabilities^{}\");\n-\n-\t\tref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);\n-\t\toidcpy(&ref->old_oid, &old_oid);\n-\t\t*list = ref;\n-\t\tlist = &ref->next;\n \t}\n \n-\tannotate_refs_with_symref_info(*orig_list);\n+\tannotate_refs_with_symref_info(*scanner->orig_list);\n \n-\treturn list;\n+\treturn scanner->list;\n }\n \n static const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0053b0954..04c191493 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -175,9 +175,12 @@ static struct discovery *last_discovery;\n \n static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n {\n+\tstruct remote_refs_scanner scanner;\n \tstruct ref *list = NULL;\n-\tget_remote_heads(-1, heads->buf, heads->len, &list,\n-\t\t\t for_push ? REF_NORMAL : 0, NULL, &heads->shallow);\n+\tremote_refs_scanner_init(&scanner, &list, for_push ? REF_NORMAL : 0,\n+\t\t\t\t NULL, &heads->shallow);\n+\tget_remote_heads(-1, heads->buf, heads->len, &scanner);\n+\n \treturn list;\n }\n \ndiff --git a/remote.h b/remote.h\nindex 2ecf4c8c7..3a60b8fc4 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -150,10 +150,26 @@ int check_ref_type(const struct ref *ref, int flags);\n void free_refs(struct ref *ref);\n \n struct oid_array;\n+\n+struct remote_refs_scanner {\n+\tstruct ref **orig_list;\n+\tstruct ref **list;\n+\tunsigned int flags;\n+\tstruct oid_array *extra_have;\n+\tstruct oid_array *shallow_points;\n+\tunsigned seen_response : 1;\n+\tunsigned recieved_dummy_capabilities_ref : 1;\n+\tunsigned done : 1;\n+};\n+void remote_refs_scanner_init(struct remote_refs_scanner *scanner,\n+\t\t\t      struct ref **list, unsigned int flags,\n+\t\t\t      struct oid_array *extra_have,\n+\t\t\t      struct oid_array *shallow_points);\n+int process_ref(struct remote_refs_scanner *scanner,\n+\t\tconst char *buffer, int len);\n+\n extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n-\t\t\t\t     struct ref **list, unsigned int flags,\n-\t\t\t\t     struct oid_array *extra_have,\n-\t\t\t\t     struct oid_array *shallow);\n+\t\t\t\t     struct remote_refs_scanner *scanner);\n \n int resolve_remote_symref(struct ref *ref, struct ref *list);\n int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid);\ndiff --git a/transport.c b/transport.c\nindex d75ff0514..c05e167d6 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -187,18 +187,59 @@ static int connect_setup(struct transport *transport, int for_push)\n \treturn 0;\n }\n \n+static int determine_version(int fd, struct strbuf *packet)\n+{\n+\tconst char *v;\n+\tint len;\n+\n+\tlen = strbuf_packet_read(fd, packet,\n+\t\t\t\t PACKET_READ_GENTLE_ON_EOF |\n+\t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n+\tif (len < 0)\n+\t\treturn -1;\n+\n+\tif (skip_prefix(packet->buf, \"version \", &v)) {\n+\t\treturn 2;\n+\t}\n+\n+\treturn 1;\n+}\n+\n static struct ref *get_refs_via_connect(struct transport *transport, int for_push)\n {\n \tstruct git_transport_data *data = transport->data;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct remote_refs_scanner scanner;\n \tstruct ref *refs;\n \n+\tremote_refs_scanner_init(&scanner, &refs, for_push ? REF_NORMAL : 0,\n+\t\t\t\t &data->extra_have, &data->shallow);\n+\n \tconnect_setup(transport, for_push);\n-\tget_remote_heads(data->fd[0], NULL, 0, &refs,\n-\t\t\t for_push ? REF_NORMAL : 0,\n-\t\t\t &data->extra_have,\n-\t\t\t &data->shallow);\n+\n+\tif (0)\n+\t\tdetermine_version(-1, NULL);\n+\tswitch(determine_version(data->fd[0], &buf)) {\n+\tcase 2:\n+\t\t/* The server understands Protocol v2 */\n+\t\tfprintf(stderr, \"Server understands Protocol v2!\\n\");\n+\t\tbreak;\n+\tcase 1:\n+\t\t/* Server is speaking Protocol v1 and sent a ref so process it */\n+\t\tprocess_ref(&scanner, buf.buf, buf.len);\n+\t\tbreak;\n+\tdefault:\n+\t\tprocess_ref(&scanner, NULL, -1);\n+\t\tbreak;\n+\n+\t}\n+\n+\tget_remote_heads(data->fd[0], NULL, 0, &scanner);\n+\n \tdata->got_remote_heads = 1;\n \n+\tstrbuf_release(&buf);\n+\n \treturn refs;\n }\n \n@@ -231,9 +272,11 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.update_shallow = data->options.update_shallow;\n \n \tif (!data->got_remote_heads) {\n+\t\tstruct remote_refs_scanner scanner;\n \t\tconnect_setup(transport, 0);\n-\t\tget_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0,\n-\t\t\t\t NULL, &data->shallow);\n+\n+\t\tremote_refs_scanner_init(&scanner, &refs_tmp, 0, NULL, &data->shallow);\n+\t\tget_remote_heads(data->fd[0], NULL, 0, &scanner);\n \t\tdata->got_remote_heads = 1;\n \t}\n \n@@ -544,10 +587,11 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \n \tif (!data->got_remote_heads) {\n \t\tstruct ref *tmp_refs;\n+\t\tstruct remote_refs_scanner scanner;\n \t\tconnect_setup(transport, 1);\n \n-\t\tget_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL,\n-\t\t\t\t NULL, &data->shallow);\n+\t\tremote_refs_scanner_init(&scanner, &tmp_refs, REF_NORMAL, NULL, &data->shallow);\n+\t\tget_remote_heads(data->fd[0], NULL, 0, &scanner);\n \t\tdata->got_remote_heads = 1;\n \t}\n \n-- \n2.14.1.342.g6490525c54-goog\n\n"},{"id":"327169","messageId":"20170824225328.8174-8-bmwill@google.com","threadId":"46664","inReplyTo":"20170824225328.8174-1-bmwill@google.com","subject":"[RFC 7/7] upload-pack: ack version 2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-24T22:53:28Z","receivedAt":"2017-08-24T22:54:14Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Signed-off-by: Brandon Williams <bmwill@google.com>\n---\n upload-pack.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 7efff2fbf..0f853152f 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1032,9 +1032,15 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n \n+void upload_pack_v2(void)\n+{\n+\tpacket_write_fmt(1, \"%s\\n\", \"version 2\");\n+}\n+\n int cmd_main(int argc, const char **argv)\n {\n \tconst char *dir;\n+\tconst char *version;\n \tint strict = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc,\n@@ -1067,6 +1073,11 @@ int cmd_main(int argc, const char **argv)\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n \n \tgit_config(upload_pack_config, NULL);\n+\n+\tversion = getenv(\"GIT_PROTOCOL\");\n+\tif (!strcmp(version, \"2\"))\n+\t\tupload_pack_v2();\n+\n \tupload_pack();\n \treturn 0;\n }\n-- \n2.14.1.342.g6490525c54-goog\n\n"},{"id":"327170","messageId":"20170824225328.8174-5-bmwill@google.com","threadId":"46664","inReplyTo":"20170824225328.8174-1-bmwill@google.com","subject":"[RFC 4/7] t: fix ssh tests to cope with using '-o SendEnv=GIT_PROTOCOL'","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-24T22:53:25Z","receivedAt":"2017-08-24T22:54:19Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Update some of our tests to cope with ssh being launched with the option\nto send the protocol version.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n t/lib-proto-disable.sh       |  1 +\n t/t5601-clone.sh             | 10 +++++-----\n t/t5602-clone-remote-exec.sh |  4 ++--\n 3 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh\nindex 83babe57d..d19c88f96 100644\n--- a/t/lib-proto-disable.sh\n+++ b/t/lib-proto-disable.sh\n@@ -194,6 +194,7 @@ setup_ssh_wrapper () {\n \ttest_expect_success 'setup ssh wrapper' '\n \t\twrite_script ssh-wrapper <<-\\EOF &&\n \t\techo >&2 \"ssh: $*\"\n+\t\tshift; shift\n \t\thost=$1; shift\n \t\tcd \"$TRASH_DIRECTORY/$host\" &&\n \t\teval \"$*\"\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 9c56f771b..7e65013c5 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -332,13 +332,13 @@ expect_ssh () {\n \t\t1)\n \t\t\t;;\n \t\t2)\n-\t\t\techo \"ssh: $1 git-upload-pack '$2'\"\n+\t\t\techo \"ssh: -o SendEnv=GIT_PROTOCOL $1 git-upload-pack '$2'\"\n \t\t\t;;\n \t\t3)\n-\t\t\techo \"ssh: $1 $2 git-upload-pack '$3'\"\n+\t\t\techo \"ssh: -o SendEnv=GIT_PROTOCOL $1 $2 git-upload-pack '$3'\"\n \t\t\t;;\n \t\t*)\n-\t\t\techo \"ssh: $1 $2 git-upload-pack '$3' $4\"\n+\t\t\techo \"ssh: $1 -o SendEnv=GIT_PROTOCOL $2 $3 git-upload-pack '$4'\"\n \t\tesac\n \t} >\"$TRASH_DIRECTORY/ssh-expect\" &&\n \t(cd \"$TRASH_DIRECTORY\" && test_cmp ssh-expect ssh-output)\n@@ -390,7 +390,7 @@ test_expect_success 'double quoted plink.exe in GIT_SSH_COMMAND' '\n \tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink.exe\" &&\n \tGIT_SSH_COMMAND=\"\\\"$TRASH_DIRECTORY/plink.exe\\\" -v\" \\\n \t\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-3 &&\n-\texpect_ssh \"-v -P 123\" myhost src\n+\texpect_ssh \"-v\" \"-P 123\" myhost src\n '\n \n SQ=\"'\"\n@@ -398,7 +398,7 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '\n \tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink.exe\" &&\n \tGIT_SSH_COMMAND=\"$SQ$TRASH_DIRECTORY/plink.exe$SQ -v\" \\\n \t\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-4 &&\n-\texpect_ssh \"-v -P 123\" myhost src\n+\texpect_ssh \"-v\" \"-P 123\" myhost src\n '\n \n test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '\ndiff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh\nindex cbcceab9d..b0d80cadd 100755\n--- a/t/t5602-clone-remote-exec.sh\n+++ b/t/t5602-clone-remote-exec.sh\n@@ -13,14 +13,14 @@ test_expect_success setup '\n \n test_expect_success 'clone calls git upload-pack unqualified with no -u option' '\n \ttest_must_fail env GIT_SSH=./not_ssh git clone localhost:/path/to/repo junk &&\n-\techo \"localhost git-upload-pack '\\''/path/to/repo'\\''\" >expected &&\n+\techo \"-o SendEnv=GIT_PROTOCOL localhost git-upload-pack '\\''/path/to/repo'\\''\" >expected &&\n \ttest_cmp expected not_ssh_output\n '\n \n test_expect_success 'clone calls specified git upload-pack with -u option' '\n \ttest_must_fail env GIT_SSH=./not_ssh \\\n \t\tgit clone -u ./something/bin/git-upload-pack localhost:/path/to/repo junk &&\n-\techo \"localhost ./something/bin/git-upload-pack '\\''/path/to/repo'\\''\" >expected &&\n+\techo \"-o SendEnv=GIT_PROTOCOL localhost ./something/bin/git-upload-pack '\\''/path/to/repo'\\''\" >expected &&\n \ttest_cmp expected not_ssh_output\n '\n \n-- \n2.14.1.342.g6490525c54-goog\n\n"},{"id":"327171","messageId":"xmqq7exsv3yu.fsf@gitster.mtv.corp.google.com","threadId":"46664","inReplyTo":"20170824225328.8174-1-bmwill@google.com","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-25T01:19:37Z","receivedAt":"2017-08-25T01:19:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> The best way to preserve functionality with old servers and clients would be to\n> communicate using the same end point but have the client send a bit of extra\n> information with its initial request.  This extra information would need to be\n> sent in such a way that old servers ignore it and operate normally (using\n> protocol v1).  The client would then need to be able to look at a server's\n> response to determine whether the server understands and is speaking v2 or has\n> ignored the clients request to use a newer protocol and is speaking v1.\n\nGood. I think the idle talk last round was for the server to tell\nthe v1 client \"we are still doing the slow ls-remote comes first\nprotocol with this exchange, but just for future reference, you can\nuse the v2 endpoint with me\", which was way less desirable (even\nthough it may be safer).\n\n> Patches 1-5 enable a client to unconditionally send this back-channel\n> information to a server.  This is done by sending a version number after a\n> second NUL byte in git://, in the envvar GIT_PROTOCOL in file:// and ssh://,\n> and in an http header in http://, https://.  Patches 6-7 teach a client and\n> upload-pack to send and recognize a request to use protocol v2.\n\nAll sounds sensible.\n\n - for git://, if you say you found a hole in the protocol to stuff\n   this information, I simply believe you ;-)  Good job.\n\n - http:// and https:// should be a no-brainer as the HTTP headers\n   give ample room to send information from the initiator side.\n\n - For ssh://, I do not think it is sane to assume that we can\n   convince server operators to allow passing any random environment\n   variable.  If the use of this specific variable turns out to be\n   popular enough, however, and its benefit outweighs administrative\n   burden, perhaps it is not impossible to convince them to allow\n   AcceptEnv on this single variable.\n\nThanks.\n"},{"id":"327194","messageId":"CAGZ79kZ3NDsAQ4zVrUN8Ab3=f5RT63G16a_gCPm3Poe47BCMvg@mail.gmail.com","threadId":"46664","inReplyTo":"xmqq7exsv3yu.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-25T17:07:08Z","receivedAt":"2017-08-25T17:07:15Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 24, 2017 at 6:19 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Brandon Williams <bmwill@google.com> writes:\n>\n>> The best way to preserve functionality with old servers and clients would be to\n>> communicate using the same end point but have the client send a bit of extra\n>> information with its initial request.  This extra information would need to be\n>> sent in such a way that old servers ignore it and operate normally (using\n>> protocol v1).  The client would then need to be able to look at a server's\n>> response to determine whether the server understands and is speaking v2 or has\n>> ignored the clients request to use a newer protocol and is speaking v1.\n>\n> Good. I think the idle talk last round was for the server to tell\n> the v1 client \"we are still doing the slow ls-remote comes first\n> protocol with this exchange, but just for future reference, you can\n> use the v2 endpoint with me\", which was way less desirable (even\n> though it may be safer).\n\nThe idea that this RFC series tries to show case is safer IMO.\n\n>> Patches 1-5 enable a client to unconditionally send this back-channel\n>> information to a server.  This is done by sending a version number after a\n>> second NUL byte in git://, in the envvar GIT_PROTOCOL in file:// and ssh://,\n>> and in an http header in http://, https://.  Patches 6-7 teach a client and\n>> upload-pack to send and recognize a request to use protocol v2.\n>\n> All sounds sensible.\n>\n>  - for git://, if you say you found a hole in the protocol to stuff\n>    this information, I simply believe you ;-)  Good job.\n\nThe hole is incredible funny and sad IMHO, so I would expect that\nthis series (specifically the review once it leaves RFC state) focusses\non how to allow future protocols with no such hacks. So AFAICT\nthe core idea of this series is that we can have an exchange\n\nclient through poked hole>  \"We can speak version 4,3, and 1 as fallback\"\nserver > \"Ok, 3 it is\"\n[ protocol v3 is executed, I don't know what it looks like. ]\n\nAlternatively when the server is old:\n\nclient >  \"We can speak version 4,3, and 1 as fallback\"\nserver > lists refs\nclient continues v1 as usual, because the version announcements\nare so different from the first ref in the refs advertisement of v1,\nsuch that the client knows for sure which version is talked. (despite\nnever getting an explicit \"it is v1\")\n\nOr if the client is old:\n\nclient > (nothing)\nserver > lists refs, current v1 style.\n\n\n>  - http:// and https:// should be a no-brainer as the HTTP headers\n>    give ample room to send information from the initiator side.\n>\n>  - For ssh://, I do not think it is sane to assume that we can\n>    convince server operators to allow passing any random environment\n>    variable.  If the use of this specific variable turns out to be\n>    popular enough, however, and its benefit outweighs administrative\n>    burden, perhaps it is not impossible to convince them to allow\n>    AcceptEnv on this single variable.\n\nOnce the next generation protocol demonstrates it is far superior\nthan the current protocol admins will switch happily. (Some ideas:\n(a) refstable instead packed-refs format, yielding better compression for\nref advertisement, (b) refs-in-want to cut down ref advertisement to\njust the needs, (c) negotiation to draw from the remote reflog)\n\nFor now I would suggest we put a protocol v2 in place that is\nthe current protocol + a version number coming through the\npoked hole at the beginning; the goal and review of this series\nought to focus on getting the version handshake right (and future\nproof for an eventual v3 if needed in another 10 years)\n\nRegarding the patches itself:\n\n patches 1,2 seem ok, no further comment\n patches 3-5 are the client stating that it can understand v2.\n which means that patch 6 (\"actually understand a v2, that\n looks surprisingly similar to v1\") should come before 3-5.\n\n patch 7 can be either before or after 6, the server side\n seems independent of the client side for the sake of\n this patch series.\n\nThanks,\nStefan\n\n>\n> Thanks.\n"},{"id":"327196","messageId":"xmqqbmn3tvru.fsf@gitster.mtv.corp.google.com","threadId":"46664","inReplyTo":"CAGZ79kZ3NDsAQ4zVrUN8Ab3=f5RT63G16a_gCPm3Poe47BCMvg@mail.gmail.com","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-25T17:14:13Z","receivedAt":"2017-08-25T17:14:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> For now I would suggest we put a protocol v2 in place that is\n> the current protocol + a version number coming through the\n> poked hole at the beginning; the goal and review of this series\n> ought to focus on getting the version handshake right...\n\nOh, we are in absolute agreement on that.  It would be nice if we\ncan have new tests to demonostrate three combinations working well\n(i.e. use 'installed git' whose path is given externally on one end\nof the connection, while the just-built binary sits on the other\nend, in addition to making sure just-built binary successfully talks\nwith itself).\n\n"},{"id":"327200","messageId":"20170825172901.kvquxafudhelxqq3@sigill.intra.peff.net","threadId":"46664","inReplyTo":"20170824225328.8174-1-bmwill@google.com","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-25T17:29:02Z","receivedAt":"2017-08-25T17:29:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 24, 2017 at 03:53:21PM -0700, Brandon Williams wrote:\n\n> Another version of Git's wire protocol is a topic that has been discussed and\n> attempted by many in the community over the years.  The biggest challenge, as\n> far as I understand, has been coming up with a transition plan to using the new\n> server without breaking existing clients and servers.  As such this RFC is\n> really only concerned with solidifying a transition plan.  Once it has been\n> decided how we can transition to a new protocol we can get into decided what\n> this new protocol would look like (though it would obviously eliminate the ref\n> advertisement ;).\n\nSadly, while splitting these things apart makes the protocol\nconceptually cleaner, I'm not sure if we can consider them separately\nand avoid adding an extra round-trip to the protocol.\n\nFor instance, let's say as a client that I've communicated \"I would like\nto speak v2\" to the server. I don't immediately know if it was received\nand respected, so I have to wait for the server to say \"OK, I know v2\"\nbefore sending any more data (like my list of capabilities that I'd like\nthe server to know before doing the ref advertisement).\n\nSo what was perhaps:\n\n  C: please run upload-pack\n  S: advertisement + caps\n  C: caps + wants\n  C+S: async have negotiation\n  S: packfile\n\nbecomes:\n\n  C: please run upload-pack (v2 if you support it)\n  S: yes, I speak v2\n  C: caps (including that I'm interested only in refs/heads/foo)\n  S: advertise refs/heads/foo + caps\n  C+S async have negotiation\n  S: packfile\n\nThat extra round-trip is probably tolerable for stateful connections\nlike git:// or ssh. But what about http? We have to add a whole\nrequest/response pair just to find out if v2 is supported.\n\nBut what if we instead think of it not as \"protocol v2\" but as \"can I\ngive the server some hints that it may end up ignoring\", then we end up\nwith something more like:\n\n  C: please run upload-pack (btw, I'm only interested in refs/heads/foo)\n  S: advertisement + caps (hopefully limited to foo, but client is prepared to receive all)\n  ... etc, as before ...\n\nIt's a subtle distinction, but the question becomes not \"can we sneak in\nan extra bit of information\" but \"can we sneak in a reasonable number of\narbitrary key/value pairs\".\n\nGiven the techniques you've used here, I suspect the answer may be\n\"yes\". We could stick arbitrary data in each of those methods (though I\nsuspect our length may be limited to about 1024 bytes if we want\ncompatibility with very old git servers).\n\n> The biggest question I'm trying to answer is if these are reasonable ways with\n> which to communicate a request to a server to use a newer protocol, without\n> breaking current servers/clients.  As far as I've tested, with patches 1-5\n> applied I can still communicate with current servers without causing any\n> problems.\n\nCurrent git.git servers, I assume?. How much do we want to care about\nalternate implementations? I would not be surprised if other git://\nimplementations are more picky about cruft after the virtual-host field\n(though I double-checked GitHub's implementation at least, and it is\nfine).\n\nI don't think libgit2 implements the server side. That leaves probably\nJGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian\nand GitLab use.\n\nThere's not really a spec here.  I'm not entirely opposed to saying \"if\nyour server does not behave like the git.git one it is wrong\". But this\nis awfully quirky behavior to be relying on, and if it behaves badly\nwith those implementations it will create headaches for users. The\ncentralized services I'm not too worried about; they'll upgrade\npromptly. But any deployments of those systems may hang around for\nyears.\n\nI dunno. Maybe it would be enough to have a config to switch off this\nfeature, which would give people using those systems an escape hatch\n(until they upgrade). Or alternatively, I guess make this optional to\nstart with, and let early adopters turn it on and complain to their server\nvendors for a while before flipping the default to on.\n\n-Peff\n"},{"id":"327202","messageId":"20170825173550.GJ13924@aiede.mtv.corp.google.com","threadId":"46664","inReplyTo":"20170825172901.kvquxafudhelxqq3@sigill.intra.peff.net","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-08-25T17:35:50Z","receivedAt":"2017-08-25T17:36:00Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Thu, Aug 24, 2017 at 03:53:21PM -0700, Brandon Williams wrote:\n\n>> Another version of Git's wire protocol is a topic that has been discussed and\n>> attempted by many in the community over the years.  The biggest challenge, as\n>> far as I understand, has been coming up with a transition plan to using the new\n>> server without breaking existing clients and servers.  As such this RFC is\n>> really only concerned with solidifying a transition plan.  Once it has been\n>> decided how we can transition to a new protocol we can get into decided what\n>> this new protocol would look like (though it would obviously eliminate the ref\n>> advertisement ;).\n>\n> Sadly, while splitting these things apart makes the protocol\n> conceptually cleaner, I'm not sure if we can consider them separately\n> and avoid adding an extra round-trip to the protocol.\n\nHow about the idea of using this mechanism to implement a protocol\n\"v1\"?\n\nThe reply would be the same as today, except that it has a \"protocol\nv1\" pkt-line at the beginning.  So this doesn't change the number of\nround-trips --- it just validates the protocol migration approach.\n\nI agree with you that an actual protocol v2 needs to change how the\ncapability exchange etc work.  bmwill@ has mentioned some thoughts about\nthis privately.  Probably he can say more here too.\n\n[...]\n> Given the techniques you've used here, I suspect the answer may be\n> \"yes\". We could stick arbitrary data in each of those methods (though I\n> suspect our length may be limited to about 1024 bytes if we want\n> compatibility with very old git servers).\n\nYes, including arbitrary data to be able to include some kinds of\nrequests inline in the initial request is one of the design goals.\n\n>> The biggest question I'm trying to answer is if these are reasonable ways with\n>> which to communicate a request to a server to use a newer protocol, without\n>> breaking current servers/clients.  As far as I've tested, with patches 1-5\n>> applied I can still communicate with current servers without causing any\n>> problems.\n>\n> Current git.git servers, I assume?. How much do we want to care about\n> alternate implementations? I would not be surprised if other git://\n> implementations are more picky about cruft after the virtual-host field\n> (though I double-checked GitHub's implementation at least, and it is\n> fine).\n\nFWIW JGit copes fine with this.\n\n> I don't think libgit2 implements the server side. That leaves probably\n> JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian\n> and GitLab use.\n\nI'd be happy if someone tests the patches against those. :)\n\n> There's not really a spec here.\n\nTechnically pack-protocol is a spec, just not a very clear one.\n\nIt does say this kind of client request is invalid.  The idea of this\nseries is to change the spec. :)\n\n[...]\n> I dunno. Maybe it would be enough to have a config to switch off this\n> feature, which would give people using those systems an escape hatch\n> (until they upgrade).\n\nI'd rather not.  That means there's less motivation for people to\nreport compatibility problems so we can fix them.\n\nIt might be necessary as a temporary escape hatch, though.\n\n>                        Or alternatively, I guess make this optional to\n> start with, and let early adopters turn it on and complain to their server\n> vendors for a while before flipping the default to on.\n\nCan we do that by making it a patch / letting it cook for a while in\n'next'? :)\n\nThanks,\nJonathan\n"},{"id":"327203","messageId":"20170825173627.gic5gdu7dv7zxucj@sigill.intra.peff.net","threadId":"46664","inReplyTo":"xmqqbmn3tvru.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-25T17:36:27Z","receivedAt":"2017-08-25T17:36:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 25, 2017 at 10:14:13AM -0700, Junio C Hamano wrote:\n\n> Stefan Beller <sbeller@google.com> writes:\n> \n> > For now I would suggest we put a protocol v2 in place that is\n> > the current protocol + a version number coming through the\n> > poked hole at the beginning; the goal and review of this series\n> > ought to focus on getting the version handshake right...\n> \n> Oh, we are in absolute agreement on that.  It would be nice if we\n> can have new tests to demonostrate three combinations working well\n> (i.e. use 'installed git' whose path is given externally on one end\n> of the connection, while the just-built binary sits on the other\n> end, in addition to making sure just-built binary successfully talks\n> with itself).\n\nThe harness in t/interop can perhaps help with that (at least between\nexisting git versions; testing across other implementations makes the\nsetup a lot tricker).\n\n-Peff\n"},{"id":"327204","messageId":"20170825174145.x6oa7btkpy5yii54@sigill.intra.peff.net","threadId":"46664","inReplyTo":"20170825173550.GJ13924@aiede.mtv.corp.google.com","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-25T17:41:46Z","receivedAt":"2017-08-25T17:41:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 25, 2017 at 10:35:50AM -0700, Jonathan Nieder wrote:\n\n> > Sadly, while splitting these things apart makes the protocol\n> > conceptually cleaner, I'm not sure if we can consider them separately\n> > and avoid adding an extra round-trip to the protocol.\n> \n> How about the idea of using this mechanism to implement a protocol\n> \"v1\"?\n> \n> The reply would be the same as today, except that it has a \"protocol\n> v1\" pkt-line at the beginning.  So this doesn't change the number of\n> round-trips --- it just validates the protocol migration approach.\n\nI'm not that worried about validating the ideas here to shoe-horn a\nversion field. I'm worried about what \"step 2\" is going to look like,\nand whether \"we shoe-horned a version field\" can be extended to \"we\nshoe-horned arbitrary data\".\n\nMaybe those are the same thing, and validating the first validates the\nsecond. But I don't think it's a foregone conclusion.\n\n> > Current git.git servers, I assume?. How much do we want to care about\n> > alternate implementations? I would not be surprised if other git://\n> > implementations are more picky about cruft after the virtual-host field\n> > (though I double-checked GitHub's implementation at least, and it is\n> > fine).\n> \n> FWIW JGit copes fine with this.\n\nThanks for checking.\n\n> > I don't think libgit2 implements the server side. That leaves probably\n> > JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian\n> > and GitLab use.\n> \n> I'd be happy if someone tests the patches against those. :)\n\nMe too. I don't have an easy setup for the last 3.\n\n> >                        Or alternatively, I guess make this optional to\n> > start with, and let early adopters turn it on and complain to their server\n> > vendors for a while before flipping the default to on.\n> \n> Can we do that by making it a patch / letting it cook for a while in\n> 'next'? :)\n\nIf people actually ran 'next', that would help. I was hoping that we\ncould get it out to the masses behind a feature flag, and dangle it in\nfront of them with \"this will improve fetch performance if you turn it\non\". But that carrot implies going all the way through the follow-on\nsteps of designing the performance-improving v2 extensions and getting\nthem implemented on the server side.\n\n-Peff\n"},{"id":"327205","messageId":"xmqqtw0vsfrz.fsf@gitster.mtv.corp.google.com","threadId":"46664","inReplyTo":"20170824225328.8174-4-bmwill@google.com","subject":"Re: [RFC 3/7] protocol: tell server that the client understands v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-25T17:45:04Z","receivedAt":"2017-08-25T17:45:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> +\t\t/* If using a new version put that stuff here after a second null byte */\n> +\t\tstrbuf_addch(&request, '\\0');\n> +\t\tstrbuf_addf(&request, \"version=%d%c\", 2, '\\0');\n> +\t\t/* subsequent supported versions can also be added */\n> +\t\tstrbuf_addf(&request, \"version=%d%c\", 3, '\\0');\n\nIsn't this last one meant only as a comment?\n"},{"id":"327206","messageId":"xmqqpobjsflp.fsf@gitster.mtv.corp.google.com","threadId":"46664","inReplyTo":"20170825172901.kvquxafudhelxqq3@sigill.intra.peff.net","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-25T17:48:50Z","receivedAt":"2017-08-25T17:48:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But what if we instead think of it not as \"protocol v2\" but as \"can I\n> give the server some hints that it may end up ignoring\", then we end up\n> with something more like:\n>\n>   C: please run upload-pack (btw, I'm only interested in refs/heads/foo)\n>   S: advertisement + caps (hopefully limited to foo, but client is prepared to receive all)\n>   ... etc, as before ...\n\nNice.  The caps that come back can tell us between the cases where\nthey only had refs/heads/foo and nothing else, or if they limited\ntheir output to it among many others we told them to ignore, so\nthere is no ambiguity.\n\n> Or alternatively, I guess make this optional to start with, and\n> let early adopters turn it on and complain to their server vendors\n> for a while before flipping the default to on.\n\nThat sounds like a safe transition plan.\n"},{"id":"327209","messageId":"20170825185010.GA103659@google.com","threadId":"46664","inReplyTo":"20170825174145.x6oa7btkpy5yii54@sigill.intra.peff.net","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-25T18:50:10Z","receivedAt":"2017-08-25T18:50:18Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/25, Jeff King wrote:\n> On Fri, Aug 25, 2017 at 10:35:50AM -0700, Jonathan Nieder wrote:\n> \n> > > Sadly, while splitting these things apart makes the protocol\n> > > conceptually cleaner, I'm not sure if we can consider them separately\n> > > and avoid adding an extra round-trip to the protocol.\n> > \n> > How about the idea of using this mechanism to implement a protocol\n> > \"v1\"?\n> > \n> > The reply would be the same as today, except that it has a \"protocol\n> > v1\" pkt-line at the beginning.  So this doesn't change the number of\n> > round-trips --- it just validates the protocol migration approach.\n> \n> I'm not that worried about validating the ideas here to shoe-horn a\n> version field. I'm worried about what \"step 2\" is going to look like,\n> and whether \"we shoe-horned a version field\" can be extended to \"we\n> shoe-horned arbitrary data\".\n\nI know that this initial RFC didn't mention adding arbitrary data in the\ninitial request but I fully intend that the final version will allow\nsuch a thing.  One such idea (via the envvar, though the same can be\ndone with git-daemon) would be to have GIT_PROTOCOL hold colon\ndelimited values, versions could be indicated by \"v1\", \"v2\", etc while\narbitrary key/values as \"key=value\" such that the envvar would look\nlike: 'v1:v2:key1=value1:key2=value2'.  This would mean that the client\nunderstands protocol version 1 and 2 (so either are acceptable for the\nserver to select since there is a change the server doesn't understand\nv2 or some other higher version number) and that if supported it wants\nkey1 to have value 'value1' and key2 to have value 'value2'.\n\nAs you said the initial request to git-daemon is limited in length (I\nthink envvars have a length limit too?) so we would still be limited in\nhow much data we can send in that initial request and so we should\ndesign a new protocol in such a way that doesn't hinge on adding extra\ndata in that first request (aside from a version number) but that can\nuse it to speed things up if at all possible.\n\n> > Can we do that by making it a patch / letting it cook for a while in\n> > 'next'? :)\n> \n> If people actually ran 'next', that would help. I was hoping that we\n> could get it out to the masses behind a feature flag, and dangle it in\n> front of them with \"this will improve fetch performance if you turn it\n> on\". But that carrot implies going all the way through the follow-on\n> steps of designing the performance-improving v2 extensions and getting\n> them implemented on the server side.\n\nWe run 'next' here so we will be able to get at least a little bit of\nfeedback from a small subset of users.\n\n-- \nBrandon Williams\n"},{"id":"327211","messageId":"20170825185321.GB103659@google.com","threadId":"46664","inReplyTo":"xmqqtw0vsfrz.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC 3/7] protocol: tell server that the client understands v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-25T18:53:21Z","receivedAt":"2017-08-25T18:53:28Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/25, Junio C Hamano wrote:\n> Brandon Williams <bmwill@google.com> writes:\n> \n> > +\t\t/* If using a new version put that stuff here after a second null byte */\n> > +\t\tstrbuf_addch(&request, '\\0');\n> > +\t\tstrbuf_addf(&request, \"version=%d%c\", 2, '\\0');\n> > +\t\t/* subsequent supported versions can also be added */\n> > +\t\tstrbuf_addf(&request, \"version=%d%c\", 3, '\\0');\n> \n> Isn't this last one meant only as a comment?\n\nSorry since this was structured as an RFC I didn't go back through the\ncode with a fine tooth comb to ensure I removed or commented out any\ndebugging statements.  Stefan also pointed out to me that I left in an\nif (0) statement somewhere haha.\n\n-- \nBrandon Williams\n"},{"id":"327212","messageId":"20170825185506.GC103659@google.com","threadId":"46664","inReplyTo":"20170825185321.GB103659@google.com","subject":"Re: [RFC 3/7] protocol: tell server that the client understands v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-25T18:55:06Z","receivedAt":"2017-08-25T18:55:13Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/25, Brandon Williams wrote:\n> On 08/25, Junio C Hamano wrote:\n> > Brandon Williams <bmwill@google.com> writes:\n> > \n> > > +\t\t/* If using a new version put that stuff here after a second null byte */\n> > > +\t\tstrbuf_addch(&request, '\\0');\n> > > +\t\tstrbuf_addf(&request, \"version=%d%c\", 2, '\\0');\n> > > +\t\t/* subsequent supported versions can also be added */\n> > > +\t\tstrbuf_addf(&request, \"version=%d%c\", 3, '\\0');\n> > \n> > Isn't this last one meant only as a comment?\n> \n> Sorry since this was structured as an RFC I didn't go back through the\n> code with a fine tooth comb to ensure I removed or commented out any\n> debugging statements.  Stefan also pointed out to me that I left in an\n> if (0) statement somewhere haha.\n\nOh one more thing about this line.  I added it to show (and check) that\ngit-daemon would be able to cope with more than one field being added as\nI wanted to avoid the issue we had when adding the 'host' field where\nadditional fields aren't allowed as they aren't gracefully ignored by\nthe server.\n\n-- \nBrandon Williams\n"},{"id":"327339","messageId":"2ff0ead4-e13a-220e-f858-36ab206c1485@jeffhostetler.com","threadId":"46664","inReplyTo":"20170825173550.GJ13924@aiede.mtv.corp.google.com","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-08-29T20:08:25Z","receivedAt":"2017-08-29T20:08:32Z","isPatch":false,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/25/2017 1:35 PM, Jonathan Nieder wrote:\n> Hi,\n> \n> Jeff King wrote:\n>> On Thu, Aug 24, 2017 at 03:53:21PM -0700, Brandon Williams wrote:\n> \n>>> Another version of Git's wire protocol is a topic that has been discussed and\n>>> attempted by many in the community over the years.  The biggest challenge, as\n>>> far as I understand, has been coming up with a transition plan to using the new\n>>> server without breaking existing clients and servers.  As such this RFC is\n>>> really only concerned with solidifying a transition plan.  Once it has been\n>>> decided how we can transition to a new protocol we can get into decided what\n>>> this new protocol would look like (though it would obviously eliminate the ref\n>>> advertisement ;).\n>>\n> \n>> I don't think libgit2 implements the server side. That leaves probably\n>> JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian\n>> and GitLab use.\n> \n> I'd be happy if someone tests the patches against those. :)\n\nI just wanted to jump in here and say I've done some initial\ntesting of this against VSTS and so far it seems fine.  And yes,\nwe have a custom git server.\n\nVSTS doesn't support the \"git://\" protocol, so the double-null trick\nisn't an issue for us.  But \"https://\" worked just fine.  I'm still\nasking around internally whether we support passing SSH environment\nvariables.\n\nJeff\n\n"},{"id":"327340","messageId":"20170829211007.GF131745@google.com","threadId":"46664","inReplyTo":"2ff0ead4-e13a-220e-f858-36ab206c1485@jeffhostetler.com","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-29T21:10:07Z","receivedAt":"2017-08-29T21:10:14Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/29, Jeff Hostetler wrote:\n> \n> \n> On 8/25/2017 1:35 PM, Jonathan Nieder wrote:\n> >Hi,\n> >\n> >Jeff King wrote:\n> >>On Thu, Aug 24, 2017 at 03:53:21PM -0700, Brandon Williams wrote:\n> >\n> >>>Another version of Git's wire protocol is a topic that has been discussed and\n> >>>attempted by many in the community over the years.  The biggest challenge, as\n> >>>far as I understand, has been coming up with a transition plan to using the new\n> >>>server without breaking existing clients and servers.  As such this RFC is\n> >>>really only concerned with solidifying a transition plan.  Once it has been\n> >>>decided how we can transition to a new protocol we can get into decided what\n> >>>this new protocol would look like (though it would obviously eliminate the ref\n> >>>advertisement ;).\n> >>\n> >\n> >>I don't think libgit2 implements the server side. That leaves probably\n> >>JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian\n> >>and GitLab use.\n> >\n> >I'd be happy if someone tests the patches against those. :)\n> \n> I just wanted to jump in here and say I've done some initial\n> testing of this against VSTS and so far it seems fine.  And yes,\n> we have a custom git server.\n> \n> VSTS doesn't support the \"git://\" protocol, so the double-null trick\n> isn't an issue for us.  But \"https://\" worked just fine.  I'm still\n> asking around internally whether we support passing SSH environment\n> variables.\n> \n> Jeff\n> \n\nThanks for checking on this, I really appreciate it.  Please let me know\nif anything I haven't thought of becomes an issue.\n\nI'm currently working on getting these patches into a more polished\nstate to be used (as discussed elsewhere on this thread) as a precursor\nto an actual v2.\n\n-- \nBrandon Williams\n"},{"id":"327344","messageId":"20170830030612.qjss3ygouardxi6k@sigill.intra.peff.net","threadId":"46664","inReplyTo":"2ff0ead4-e13a-220e-f858-36ab206c1485@jeffhostetler.com","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-30T03:06:13Z","receivedAt":"2017-08-30T03:06:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 04:08:25PM -0400, Jeff Hostetler wrote:\n\n> I just wanted to jump in here and say I've done some initial\n> testing of this against VSTS and so far it seems fine.  And yes,\n> we have a custom git server.\n\nGreat, thank you for checking.\n\n> VSTS doesn't support the \"git://\" protocol, so the double-null trick\n> isn't an issue for us.  But \"https://\" worked just fine.  I'm still\n> asking around internally whether we support passing SSH environment\n> variables.\n\nThe key thing for ssh is not whether you support passing environment\nvariables. It's whether you quietly ignore unknown variables rather than\ncutting off the connection.\n\nTo support the v2 protocol you'd need to pass the new variables, but\nyou'd also need to modify your server to actually do something useful\nwith them anyway. At this point we're mostly concerned with whether we\ncan safely pass the variables to current implementations unconditionally\nand get a reasonable outcome.\n\nTo be honest, I'm not all that worried about VSTS either way. It's a\ncentralized service which will likely get v2 extensions implemented in a\ntimely manner (once they exist). I'm much more concerned about\nshrink-wrap implementations deployed in the wild, which may hang around\nwithout being upgraded for years. If v2-aware clients send requests that\ncause those old implementations to choke, users won't be happy.\n\n-Peff\n"},{"id":"327395","messageId":"20170830105519.GH3839@alpha.vpn.ikke.info","threadId":"46664","inReplyTo":"20170824225328.8174-6-bmwill@google.com","subject":"Re: [RFC 5/7] http: send Git-Protocol-Version header","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-08-30T10:55:19Z","receivedAt":"2017-08-30T11:01:33Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Thu, Aug 24, 2017 at 03:53:26PM -0700, Brandon Williams wrote:\n> Tell a serve that protocol v2 can be used by sending an http header\n> indicating this.\n\ns/serve/server/\n"},{"id":"327398","messageId":"3a2694ca-b5e6-7258-de01-c9898cfa60a2@jeffhostetler.com","threadId":"46664","inReplyTo":"20170830030612.qjss3ygouardxi6k@sigill.intra.peff.net","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-08-30T13:30:16Z","receivedAt":"2017-08-30T13:30:26Z","isPatch":false,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/29/2017 11:06 PM, Jeff King wrote:\n> On Tue, Aug 29, 2017 at 04:08:25PM -0400, Jeff Hostetler wrote:\n> \n>> I just wanted to jump in here and say I've done some initial\n>> testing of this against VSTS and so far it seems fine.  And yes,\n>> we have a custom git server.\n> \n> Great, thank you for checking.\n> \n>> VSTS doesn't support the \"git://\" protocol, so the double-null trick\n>> isn't an issue for us.  But \"https://\" worked just fine.  I'm still\n>> asking around internally whether we support passing SSH environment\n>> variables.\n> \n> The key thing for ssh is not whether you support passing environment\n> variables. It's whether you quietly ignore unknown variables rather than\n> cutting off the connection.\n >\n> To support the v2 protocol you'd need to pass the new variables, but\n> you'd also need to modify your server to actually do something useful\n> with them anyway. At this point we're mostly concerned with whether we\n> can safely pass the variables to current implementations unconditionally\n> and get a reasonable outcome.\n\nRight.  I just spoke with our server folks and, currently, our SSH\nsupport quietly eats ALL variables.   So we're safe :-)\n\nI'm starting a conversation with them to pass them thru so we can\nbe ready for this.  (Assuming we choose to go this way.)\n\nThanks,\nJeff\n\n\n"},{"id":"327406","messageId":"20170830165445.GA50018@google.com","threadId":"46664","inReplyTo":"3a2694ca-b5e6-7258-de01-c9898cfa60a2@jeffhostetler.com","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-30T16:54:45Z","receivedAt":"2017-08-30T16:54:52Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/30, Jeff Hostetler wrote:\n> \n> \n> On 8/29/2017 11:06 PM, Jeff King wrote:\n> >On Tue, Aug 29, 2017 at 04:08:25PM -0400, Jeff Hostetler wrote:\n> >\n> >>I just wanted to jump in here and say I've done some initial\n> >>testing of this against VSTS and so far it seems fine.  And yes,\n> >>we have a custom git server.\n> >\n> >Great, thank you for checking.\n> >\n> >>VSTS doesn't support the \"git://\" protocol, so the double-null trick\n> >>isn't an issue for us.  But \"https://\" worked just fine.  I'm still\n> >>asking around internally whether we support passing SSH environment\n> >>variables.\n> >\n> >The key thing for ssh is not whether you support passing environment\n> >variables. It's whether you quietly ignore unknown variables rather than\n> >cutting off the connection.\n> >\n> >To support the v2 protocol you'd need to pass the new variables, but\n> >you'd also need to modify your server to actually do something useful\n> >with them anyway. At this point we're mostly concerned with whether we\n> >can safely pass the variables to current implementations unconditionally\n> >and get a reasonable outcome.\n> \n> Right.  I just spoke with our server folks and, currently, our SSH\n> support quietly eats ALL variables.   So we're safe :-)\n> \n> I'm starting a conversation with them to pass them thru so we can\n> be ready for this.  (Assuming we choose to go this way.)\n\nPerfect! Thanks again.\n\n-- \nBrandon Williams\n"},{"id":"327467","messageId":"CAGyf7-HTuM2kBRYDP8i9B8pUgqFw02pqBdYCVnp9s7v4bDAM0w@mail.gmail.com","threadId":"46664","inReplyTo":"20170825172901.kvquxafudhelxqq3@sigill.intra.peff.net","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2017-08-30T20:38:39Z","receivedAt":"2017-08-30T20:38:45Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Fri, Aug 25, 2017 at 10:29 AM, Jeff King <peff@peff.net> wrote:\n> On Thu, Aug 24, 2017 at 03:53:21PM -0700, Brandon Williams wrote:\n>\n>> The biggest question I'm trying to answer is if these are reasonable ways with\n>> which to communicate a request to a server to use a newer protocol, without\n>> breaking current servers/clients.  As far as I've tested, with patches 1-5\n>> applied I can still communicate with current servers without causing any\n>> problems.\n>\n> Current git.git servers, I assume?. How much do we want to care about\n> alternate implementations? I would not be surprised if other git://\n> implementations are more picky about cruft after the virtual-host field\n> (though I double-checked GitHub's implementation at least, and it is\n> fine).\n>\n> I don't think libgit2 implements the server side. That leaves probably\n> JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian\n> and GitLab use.\n\nBefore I manually apply the patches to test how they work with\nBitbucket Server, are they applied on a branch somewhere where I can\njust fetch them? If not, I'll apply them manually and verify.\n\nJust based on the description, though, I expect no issues. We don't\ncurrently support the git:// protocol. Our HTTP handling passes\nheaders through to the receive-pack and upload-pack processes as\nenvironment variables (with a little massaging), but doesn't consider\nthem itself; it only considers the URL and \"service\" query parameter\nto decide what command to run and to detect \"dumb\" requests. Our SSH\nhandling ignores any environment variables provided and does not\nforward them to the git process, similar to VSTS.\n\nI'll confirm explicitly, to be certain, but just based on reading the\noverview and knowing our code I think the described approaches should\nwork fine.\n\nBest regards,\nBryan Turner\n"},{"id":"327470","messageId":"20170830211245.GD50018@google.com","threadId":"46664","inReplyTo":"CAGyf7-HTuM2kBRYDP8i9B8pUgqFw02pqBdYCVnp9s7v4bDAM0w@mail.gmail.com","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-30T21:12:45Z","receivedAt":"2017-08-30T21:12:53Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/30, Bryan Turner wrote:\n> On Fri, Aug 25, 2017 at 10:29 AM, Jeff King <peff@peff.net> wrote:\n> > On Thu, Aug 24, 2017 at 03:53:21PM -0700, Brandon Williams wrote:\n> >\n> >> The biggest question I'm trying to answer is if these are reasonable ways with\n> >> which to communicate a request to a server to use a newer protocol, without\n> >> breaking current servers/clients.  As far as I've tested, with patches 1-5\n> >> applied I can still communicate with current servers without causing any\n> >> problems.\n> >\n> > Current git.git servers, I assume?. How much do we want to care about\n> > alternate implementations? I would not be surprised if other git://\n> > implementations are more picky about cruft after the virtual-host field\n> > (though I double-checked GitHub's implementation at least, and it is\n> > fine).\n> >\n> > I don't think libgit2 implements the server side. That leaves probably\n> > JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian\n> > and GitLab use.\n> \n> Before I manually apply the patches to test how they work with\n> Bitbucket Server, are they applied on a branch somewhere where I can\n> just fetch them? If not, I'll apply them manually and verify.\n\nI just pushed this set of patches up to: https://github.com/bmwill/git/tree/protocol-v2\nso you should be able to fetch them from there (saves you from having to\nmanually applying the patches).\n\n> Just based on the description, though, I expect no issues. We don't\n> currently support the git:// protocol. Our HTTP handling passes\n> headers through to the receive-pack and upload-pack processes as\n> environment variables (with a little massaging), but doesn't consider\n> them itself; it only considers the URL and \"service\" query parameter\n> to decide what command to run and to detect \"dumb\" requests. Our SSH\n> handling ignores any environment variables provided and does not\n> forward them to the git process, similar to VSTS.\n> \n> I'll confirm explicitly, to be certain, but just based on reading the\n> overview and knowing our code I think the described approaches should\n> work fine.\n\nPerfect!  Thanks for taking the time to verify that this will work.\n\n-- \nBrandon Williams\n"},{"id":"327512","messageId":"CAGyf7-GKAW3Dn_rPKxtidKoXh7a6NmDFYzGrb9tGOszhsMignA@mail.gmail.com","threadId":"46664","inReplyTo":"20170824225328.8174-8-bmwill@google.com","subject":"Re: [RFC 7/7] upload-pack: ack version 2","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2017-09-01T22:02:36Z","receivedAt":"2017-09-01T22:02:43Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Thu, Aug 24, 2017 at 3:53 PM, Brandon Williams <bmwill@google.com> wrote:\n> +\n> +       version = getenv(\"GIT_PROTOCOL\");\n> +       if (!strcmp(version, \"2\"))\n> +               upload_pack_v2();\n> +\n\nI think the \"if\" check here needs some type of NULL check for\n\"version\" before calling \"strcmp\". Without that, if the \"GIT_PROTOCOL\"\nenvironment variable isn't set, git-upload-pack SEGVs.\n\nThis came up when I was testing the \"protocol-v2\" branch with\nBitbucket Server. For performance reasons, we sometimes run ref\nadvertisements as a separate process, when serving fetches, to allow\nthrottling the ref advertisement separately from actually generating\nand sending a packfile. Since CI servers continuously poll for\nchanges, but usually don't find any, we want to be able to serve ref\nadvertisements, which have minimal overhead, with much higher\nparallelism than serving packs. To do that, we run \"git-upload-pack\n--advertize-refs\" directly, and that command has never *required*\n\"GIT_PROTOCOL\" before this change.\n"},{"id":"327517","messageId":"CAGyf7-H9MdSzuU51opj+cA9ZAFSEnt-rvQcLuwY_j7qfn=kNsg@mail.gmail.com","threadId":"46664","inReplyTo":"20170830211245.GD50018@google.com","subject":"Re: [RFC 0/7] transitioning to protocol v2","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2017-09-01T23:06:55Z","receivedAt":"2017-09-01T23:07:05Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Wed, Aug 30, 2017 at 2:12 PM, Brandon Williams <bmwill@google.com> wrote:\n> On 08/30, Bryan Turner wrote:\n>> On Fri, Aug 25, 2017 at 10:29 AM, Jeff King <peff@peff.net> wrote:\n>> > On Thu, Aug 24, 2017 at 03:53:21PM -0700, Brandon Williams wrote:\n>> >\n>> >> The biggest question I'm trying to answer is if these are reasonable ways with\n>> >> which to communicate a request to a server to use a newer protocol, without\n>> >> breaking current servers/clients.  As far as I've tested, with patches 1-5\n>> >> applied I can still communicate with current servers without causing any\n>> >> problems.\n>> >\n>> > Current git.git servers, I assume?. How much do we want to care about\n>> > alternate implementations? I would not be surprised if other git://\n>> > implementations are more picky about cruft after the virtual-host field\n>> > (though I double-checked GitHub's implementation at least, and it is\n>> > fine).\n>> >\n>> > I don't think libgit2 implements the server side. That leaves probably\n>> > JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian\n>> > and GitLab use.\n>>\n>> Before I manually apply the patches to test how they work with\n>> Bitbucket Server, are they applied on a branch somewhere where I can\n>> just fetch them? If not, I'll apply them manually and verify.\n>\n> I just pushed this set of patches up to: https://github.com/bmwill/git/tree/protocol-v2\n> so you should be able to fetch them from there (saves you from having to\n> manually applying the patches).\n\nThanks for that! It made testing a lot simpler.\n\n>\n>> Just based on the description, though, I expect no issues. We don't\n>> currently support the git:// protocol. Our HTTP handling passes\n>> headers through to the receive-pack and upload-pack processes as\n>> environment variables (with a little massaging), but doesn't consider\n>> them itself; it only considers the URL and \"service\" query parameter\n>> to decide what command to run and to detect \"dumb\" requests. Our SSH\n>> handling ignores any environment variables provided and does not\n>> forward them to the git process, similar to VSTS.\n>>\n>> I'll confirm explicitly, to be certain, but just based on reading the\n>> overview and knowing our code I think the described approaches should\n>> work fine.\n>\n> Perfect!  Thanks for taking the time to verify that this will work.\n\nWith 2 small tweaks on top of \"protocol-v2\", I was able to run our\nfull command and hosting (HTTP and SSH) test suites without issues.\n\ndiff --git a/transport.c b/transport.c\nindex c05e167..37b5d83 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -222,7 +222,8 @@ static struct ref *get_refs_via_connect(struct\ntransport *transport, int for_pus\n        switch(determine_version(data->fd[0], &buf)) {\n        case 2:\n                /* The server understands Protocol v2 */\n-               fprintf(stderr, \"Server understands Protocol v2!\\n\");\n+               if (transport->verbose >= 0)\n+                       fprintf(stderr, \"Server understands Protocol v2!\\n\");\n                break;\n        case 1:\n                /* Server is speaking Protocol v1 and sent a ref so\nprocess it */\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 0f85315..7c59495 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1075,7 +1075,7 @@ int cmd_main(int argc, const char **argv)\n        git_config(upload_pack_config, NULL);\n\n        version = getenv(\"GIT_PROTOCOL\");\n-       if (!strcmp(version, \"2\"))\n+       if (version && !strcmp(version, \"2\"))\n                upload_pack_v2();\n\n        upload_pack();\n\nI'd imagine the \"Server understands Protocol v2!\" message won't\nactually *ship* in Git, so it's likely making that honor \"--quiet\"\ndoesn't actually matter; I only adjusted it because we have a test\nthat verifies \"git clone --quiet\" is quiet. The \"if (version\" change I\ncommented on separately in the 7/7 patch that introduced the check.\n\nThanks again for publishing this, and for letting me test it!\n\n>\n> --\n> Brandon Williams\n"},{"id":"327519","messageId":"20170901232012.GA16739@google.com","threadId":"46664","inReplyTo":"CAGyf7-GKAW3Dn_rPKxtidKoXh7a6NmDFYzGrb9tGOszhsMignA@mail.gmail.com","subject":"Re: [RFC 7/7] upload-pack: ack version 2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-09-01T23:20:12Z","receivedAt":"2017-09-01T23:20:21Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 09/01, Bryan Turner wrote:\n> On Thu, Aug 24, 2017 at 3:53 PM, Brandon Williams <bmwill@google.com> wrote:\n> > +\n> > +       version = getenv(\"GIT_PROTOCOL\");\n> > +       if (!strcmp(version, \"2\"))\n> > +               upload_pack_v2();\n> > +\n> \n> I think the \"if\" check here needs some type of NULL check for\n> \"version\" before calling \"strcmp\". Without that, if the \"GIT_PROTOCOL\"\n> environment variable isn't set, git-upload-pack SEGVs.\n> \n> This came up when I was testing the \"protocol-v2\" branch with\n> Bitbucket Server. For performance reasons, we sometimes run ref\n> advertisements as a separate process, when serving fetches, to allow\n> throttling the ref advertisement separately from actually generating\n> and sending a packfile. Since CI servers continuously poll for\n> changes, but usually don't find any, we want to be able to serve ref\n> advertisements, which have minimal overhead, with much higher\n> parallelism than serving packs. To do that, we run \"git-upload-pack\n> --advertize-refs\" directly, and that command has never *required*\n> \"GIT_PROTOCOL\" before this change.\n\nThanks for pointing this out.  Since this was an RFC I wasn't being\ncareful with doing these sorts of checks :).  I'm currently working on\nthe non-RFC version of this series and it is getting close to being in a\nstate where I can send it out for more careful review.\n\n-- \nBrandon Williams\n"}]}