{"thread":{"id":"42183","subject":"[WIP PATCH 00/14] Protocol v2 patches","startedAt":"2016-04-29T23:34:33Z","lastAt":"2016-05-25T21:29:09Z","messageCount":40,"participants":["Stefan Beller","David Turner","Duy Nguyen","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":14},"messages":[{"id":"284992","messageId":"1461972887-22100-1-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":null,"subject":"[WIP PATCH 00/14] Protocol v2 patches","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:33Z","receivedAt":"2016-04-29T23:34:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi David,\n\nhere are my patches for a protocol v2.\n\n(\"Negotiate capabilities before doing anything else\", or as code:\n\n static void upload_pack_version_2(void)\n {\n send_capabilities_version_2();\n receive_capabilities_version_2();\n\n /* The rest of the protocol stays the same, capabilities advertising\n is disabled though. */\n advertise_capabilities = 0;\n upload_pack();\n }\n)\n\nThey are rough and unfinished as you can see by the tailing WIPs.\nHowever the plumbing (upload-pack and fetch-pack) works and we'd need to\nintegrate that into user porcelains, i.e. fetch, clone, push.\n\nAlso we need to add tests for all the options again, so we'd need to be smart\nabout testing that.\n\nI am not sure if it makes sense to integrate that with the http series, though.\n\nThanks,\nStefan\n\nNguyễn Thái Ngọc Duy (1):\n upload-pack: make client capability parsing code a separate function\n\nStefan Beller (13):\n upload-pack.c: Refactor capability advertising\n upload-pack-2: Implement the version 2 of upload-pack\n connect: rewrite feature parsing to work on string_list\n transport: add infrastructure to support a protocol version number\n remote.h: add get_remote_capabilities, request_capabilities\n fetch-pack: move capability selection out of do_fetch_pack\n fetch-pack: factor out get_selected_capabilities_list\n fetch-pack: Add negotiate_capabilities\n do_fetch_pack: select capabilities for transport version 1 only\n builtin/fetch-pack: add argument for transport version\n Add test for fetch-pack\n WIP add test for git pull\n WIP test git fetch\n\n .gitignore | 1 +\n Makefile | 4 ++\n builtin/fetch-pack.c | 20 ++++++-\n builtin/receive-pack.c | 15 +++--\n connect.c | 141 +++++++++++++++++++++++++++++--------------\n connect.h | 2 +-\n fetch-pack.c | 102 ++++++++++++++++++++++++-------\n fetch-pack.h | 7 +++\n remote.c | 2 +\n remote.h | 5 ++\n t/t5500-fetch-pack.sh | 21 +++++++\n t/t5510-fetch.sh | 5 ++\n t/t5520-pull.sh | 6 ++\n transport-helper.c | 1 +\n transport.c | 20 ++++++-\n transport.h | 8 +++\n upload-pack-2.c | 1 +\n upload-pack.c | 159 ++++++++++++++++++++++++++++++++++++-------------\n 18 files changed, 403 insertions(+), 117 deletions(-)\n create mode 120000 upload-pack-2.c\n\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"284993","messageId":"1461972887-22100-2-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 01/14] upload-pack: make client capability parsing code a separate function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:34Z","receivedAt":"2016-04-29T23:34:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Stefan Beller \n---\n upload-pack.c | 44 +++++++++++++++++++++++---------------------\n 1 file changed, 23 insertions(+), 21 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex dc802a0..aaaf883 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -535,6 +535,28 @@ error:\n \t}\n }\n \n+static void parse_features(const char *features)\n+{\n+\tif (parse_feature_request(features, \"multi_ack_detailed\"))\n+\t\tmulti_ack = 2;\n+\telse if (parse_feature_request(features, \"multi_ack\"))\n+\t\tmulti_ack = 1;\n+\tif (parse_feature_request(features, \"no-done\"))\n+\t\tno_done = 1;\n+\tif (parse_feature_request(features, \"thin-pack\"))\n+\t\tuse_thin_pack = 1;\n+\tif (parse_feature_request(features, \"ofs-delta\"))\n+\t\tuse_ofs_delta = 1;\n+\tif (parse_feature_request(features, \"side-band-64k\"))\n+\t\tuse_sideband = LARGE_PACKET_MAX;\n+\telse if (parse_feature_request(features, \"side-band\"))\n+\t\tuse_sideband = DEFAULT_PACKET_MAX;\n+\tif (parse_feature_request(features, \"no-progress\"))\n+\t\tno_progress = 1;\n+\tif (parse_feature_request(features, \"include-tag\"))\n+\t\tuse_include_tag = 1;\n+}\n+\n static void receive_needs(void)\n {\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n@@ -544,7 +566,6 @@ static void receive_needs(void)\n \tshallow_nr = 0;\n \tfor (;;) {\n \t\tstruct object *o;\n-\t\tconst char *features;\n \t\tunsigned char sha1_buf[20];\n \t\tchar *line = packet_read_line(0, NULL);\n \t\treset_timeout();\n@@ -579,26 +600,7 @@ static void receive_needs(void)\n \t\t\tdie(\"git upload-pack: protocol error, \"\n \t\t\t \"expected to get sha, not '%s'\", line);\n \n-\t\tfeatures = line + 45;\n-\n-\t\tif (parse_feature_request(features, \"multi_ack_detailed\"))\n-\t\t\tmulti_ack = 2;\n-\t\telse if (parse_feature_request(features, \"multi_ack\"))\n-\t\t\tmulti_ack = 1;\n-\t\tif (parse_feature_request(features, \"no-done\"))\n-\t\t\tno_done = 1;\n-\t\tif (parse_feature_request(features, \"thin-pack\"))\n-\t\t\tuse_thin_pack = 1;\n-\t\tif (parse_feature_request(features, \"ofs-delta\"))\n-\t\t\tuse_ofs_delta = 1;\n-\t\tif (parse_feature_request(features, \"side-band-64k\"))\n-\t\t\tuse_sideband = LARGE_PACKET_MAX;\n-\t\telse if (parse_feature_request(features, \"side-band\"))\n-\t\t\tuse_sideband = DEFAULT_PACKET_MAX;\n-\t\tif (parse_feature_request(features, \"no-progress\"))\n-\t\t\tno_progress = 1;\n-\t\tif (parse_feature_request(features, \"include-tag\"))\n-\t\t\tuse_include_tag = 1;\n+\t\tparse_features(line + 45);\n \n \t\to = parse_object(sha1_buf);\n \t\tif (!o)\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"284994","messageId":"1461972887-22100-3-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 02/14] upload-pack.c: Refactor capability advertising","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:35Z","receivedAt":"2016-04-29T23:34:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Instead of having the capabilities in a local string, keep them\nin a struct outside the function. This will allow us in a later patch\nto easily reuse the capabilities in version 2 of the protocol.\n\nSigned-off-by: Stefan Beller \n---\n upload-pack.c | 59 ++++++++++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 40 insertions(+), 19 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex aaaf883..85381d5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -719,37 +719,58 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)\n \t\tstrbuf_addf(buf, \" symref=%s:%s\", item->string, (char *)item->util);\n }\n \n+static int advertise_capabilities = 1;\n+const char *all_capabilities[] = {\n+\t\"multi_ack\",\n+\t\"thin-pack\",\n+\t\"side-band\",\n+\t\"side-band-64k\",\n+\t\"ofs-delta\",\n+\t\"shallow\",\n+\t\"no-progress\",\n+\t\"include-tag\",\n+\t\"multi_ack_detailed\",\n+\t\"allow-tip-sha1-in-want\",\n+\t\"allow-reachable-sha1-in-want\",\n+\t\"no-done\",\n+};\n+\n static int send_ref(const char *refname, const struct object_id *oid,\n \t\t int flag, void *cb_data)\n {\n-\tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n-\t\t\" side-band-64k ofs-delta shallow no-progress\"\n-\t\t\" include-tag multi_ack_detailed\";\n \tconst char *refname_nons = strip_namespace(refname);\n \tstruct object_id peeled;\n \n \tif (mark_our_ref(refname_nons, refname, oid))\n \t\treturn 0;\n \n-\tif (capabilities) {\n-\t\tstruct strbuf symref_info = STRBUF_INIT;\n-\n-\t\tformat_symref_info(&symref_info, cb_data);\n-\t\tpacket_write(1, \"%s %s%c%s%s%s%s%s agent=%s\\n\",\n-\t\t\t oid_to_hex(oid), refname_nons,\n-\t\t\t 0, capabilities,\n-\t\t\t (allow_unadvertised_object_request & ALLOW_TIP_SHA1) ?\n-\t\t\t\t \" allow-tip-sha1-in-want\" : \"\",\n-\t\t\t (allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1) ?\n-\t\t\t\t \" allow-reachable-sha1-in-want\" : \"\",\n-\t\t\t stateless_rpc ? \" no-done\" : \"\",\n-\t\t\t symref_info.buf,\n-\t\t\t git_user_agent_sanitized());\n-\t\tstrbuf_release(&symref_info);\n+\tif (advertise_capabilities) {\n+\t\tint i;\n+\t\tstruct strbuf capabilities = STRBUF_INIT;\n+\n+\t\tfor (i = 0; i < ARRAY_SIZE(all_capabilities); i++) {\n+\t\t\tconst char *cap = all_capabilities[i];\n+\t\t\tif (!strcmp(cap, \"allow-tip-sha1-in-want\")\n+\t\t\t && !(allow_unadvertised_object_request & ALLOW_TIP_SHA1))\n+\t\t\t\tcontinue;\n+\t\t\tif (!strcmp(cap, \"allow-reachable-sha1-in-want\")\n+\t\t\t && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))\n+\t\t\t\tcontinue;\n+\t\t\tif (!strcmp(cap, \"no-done\") && !stateless_rpc)\n+\t\t\t\tcontinue;\n+\t\t\tstrbuf_addf(&capabilities, \" %s\", cap);\n+\t\t}\n+\n+\t\tformat_symref_info(&capabilities, cb_data);\n+\t\tstrbuf_addf(&capabilities, \" agent=%s\", git_user_agent_sanitized());\n+\n+\t\tpacket_write(1, \"%s %s%c%s\\n\", oid_to_hex(oid), refname_nons,\n+\t\t\t 0, capabilities.buf);\n+\t\tstrbuf_release(&capabilities);\n+\t\tadvertise_capabilities = 0;\n \t} else {\n \t\tpacket_write(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n-\tcapabilities = NULL;\n \tif (!peel_ref(refname, peeled.hash))\n \t\tpacket_write(1, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n \treturn 0;\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"284995","messageId":"1461972887-22100-4-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 03/14] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:36Z","receivedAt":"2016-04-29T23:34:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In upload-pack-2 we send each capability in its own packet buffer.\nThe construction of upload-pack-2 is a bit unfortunate as I would like\nit to not be depending on a symlink linking to upload-pack.c, but I did\nnot find another easy way to do it. I would like it to generate\nupload-pack-2.o from upload-pack.c but with '-DTRANSPORT_VERSION=2' set.\n\nSigned-off-by: Stefan Beller \n---\n .gitignore | 1 +\n Makefile | 4 ++++\n upload-pack-2.c | 1 +\n upload-pack.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-\n 4 files changed, 54 insertions(+), 1 deletion(-)\n create mode 120000 upload-pack-2.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 5087ce1..9a6ea8c 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -166,6 +166,7 @@\n /git-update-server-info\n /git-upload-archive\n /git-upload-pack\n+/git-upload-pack-2\n /git-var\n /git-verify-commit\n /git-verify-pack\ndiff --git a/Makefile b/Makefile\nindex a83e322..3d96126 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -580,6 +580,7 @@ PROGRAM_OBJS += sh-i18n--envsubst.o\n PROGRAM_OBJS += shell.o\n PROGRAM_OBJS += show-index.o\n PROGRAM_OBJS += upload-pack.o\n+PROGRAM_OBJS += upload-pack-2.o\n PROGRAM_OBJS += remote-testsvn.o\n \n # Binary suffix, set to .exe for Windows builds\n@@ -648,6 +649,7 @@ OTHER_PROGRAMS = git$X\n # what test wrappers are needed and 'install' will install, in bindir\n BINDIR_PROGRAMS_NEED_X += git\n BINDIR_PROGRAMS_NEED_X += git-upload-pack\n+BINDIR_PROGRAMS_NEED_X += git-upload-pack-2\n BINDIR_PROGRAMS_NEED_X += git-receive-pack\n BINDIR_PROGRAMS_NEED_X += git-upload-archive\n BINDIR_PROGRAMS_NEED_X += git-shell\n@@ -1730,6 +1732,8 @@ $(BUILT_INS): git$X\n \tln -s $< $@ 2>/dev/null || \\\n \tcp $< $@\n \n+$X%-2.o: EXTRA_CPPFLAGS = '-DTRANSPORT_VERSION=2'\n+\n common-cmds.h: generate-cmdlist.sh command-list.txt\n \n common-cmds.h: $(wildcard Documentation/git-*.txt)\ndiff --git a/upload-pack-2.c b/upload-pack-2.c\nnew file mode 120000\nindex 0000000..e30a871\n--- /dev/null\n+++ b/upload-pack-2.c\n@@ -0,0 +1 @@\n+upload-pack.c\n\\ No newline at end of file\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 85381d5..edfd417 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -820,6 +820,45 @@ static void upload_pack(void)\n \t}\n }\n \n+#if (TRANSPORT_VERSION == 2)\n+static void send_capabilities_version_2(void)\n+{\n+\tint i;\n+\tfor (i = 0; i < ARRAY_SIZE(all_capabilities); i++) {\n+\t\tconst char *cap = all_capabilities[i];\n+\t\tif (!strcmp(cap, \"allow-tip-sha1-in-want\")\n+\t\t && !(allow_unadvertised_object_request & ALLOW_TIP_SHA1))\n+\t\t\tcontinue;\n+\t\tif (!strcmp(cap, \"no-done\") && !stateless_rpc)\n+\t\t\tcontinue;\n+\t\tpacket_write(1, \"%s\\n\", cap);\n+\t}\n+\n+\tpacket_write(1, \"agent=%s\\n\", git_user_agent_sanitized());\n+\tpacket_flush(1);\n+}\n+\n+static void receive_capabilities_version_2(void)\n+{\n+\tchar *line = packet_read_line(0, NULL);\n+\twhile (line) {\n+\t\tparse_features(line);\n+\t\tline = packet_read_line(0, NULL);\n+\t}\n+}\n+\n+static void upload_pack_version_2(void)\n+{\n+\tsend_capabilities_version_2();\n+\treceive_capabilities_version_2();\n+\n+\t/* The rest of the protocol stays the same, capabilities advertising\n+\t is disabled though. */\n+\tadvertise_capabilities = 0;\n+\tupload_pack();\n+}\n+#endif\n+\n static int upload_pack_config(const char *var, const char *value, void *unused)\n {\n \tif (!strcmp(\"uploadpack.allowtipsha1inwant\", var)) {\n@@ -847,8 +886,11 @@ int main(int argc, char **argv)\n \tint strict = 0;\n \n \tgit_setup_gettext();\n-\n+#if TRANSPORT_VERSION == 2\n+\tpacket_trace_identity(\"upload-pack-2\");\n+#else\n \tpacket_trace_identity(\"upload-pack\");\n+#endif\n \tgit_extract_argv0_path(argv[0]);\n \tcheck_replace_refs = 0;\n \n@@ -891,6 +933,11 @@ int main(int argc, 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+#if TRANSPORT_VERSION == 2\n+\tupload_pack_version_2();\n+#else\n \tupload_pack();\n+#endif\n \treturn 0;\n }\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"285003","messageId":"1461972887-22100-5-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 04/14] connect: rewrite feature parsing to work on string_list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:37Z","receivedAt":"2016-04-29T23:34:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Later on when we introduce the version 2 transport protocol, the\ncapabilities will not be transported in one lone string but each\ncapability will be carried in its own pkt line.\n\nTo reuse existing infrastructure we would either need to join the\ncapabilities into a single string again later or refactor the current\ncapability parsing to be using a data structure which fits both\nversions of the transport protocol. We chose to implement the later.\n\nSigned-off-by: Stefan Beller \n---\n builtin/receive-pack.c | 15 ++++++---\n connect.c | 82 +++++++++++++++++++++++---------------------------\n connect.h | 2 +-\n upload-pack.c | 13 ++++++--\n 4 files changed, 58 insertions(+), 54 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex a744437..4e98514 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1429,16 +1429,21 @@ static struct command *read_head_info(struct sha1_array *shallow)\n \n \t\tlinelen = strlen(line);\n \t\tif (linelen < len) {\n-\t\t\tconst char *feature_list = line + linelen + 1;\n-\t\t\tif (parse_feature_request(feature_list, \"report-status\"))\n+\t\t\tstruct string_list feature_list = STRING_LIST_INIT_DUP;\n+\t\t\tstring_list_split(&feature_list,\n+\t\t\t\t\t line + linelen + 1,\n+\t\t\t\t\t ' ', -1);\n+\t\t\tif (parse_feature_request(&feature_list, \"report-status\"))\n \t\t\t\treport_status = 1;\n-\t\t\tif (parse_feature_request(feature_list, \"side-band-64k\"))\n+\t\t\tif (parse_feature_request(&feature_list, \"side-band-64k\"))\n \t\t\t\tuse_sideband = LARGE_PACKET_MAX;\n-\t\t\tif (parse_feature_request(feature_list, \"quiet\"))\n+\t\t\tif (parse_feature_request(&feature_list, \"quiet\"))\n \t\t\t\tquiet = 1;\n \t\t\tif (advertise_atomic_push\n-\t\t\t && parse_feature_request(feature_list, \"atomic\"))\n+\t\t\t && parse_feature_request(&feature_list, \"atomic\"))\n \t\t\t\tuse_atomic = 1;\n+\n+\t\t\tstring_list_clear(&feature_list, 1);\n \t\t}\n \n \t\tif (!strcmp(line, \"push-cert\")) {\ndiff --git a/connect.c b/connect.c\nindex c53f3f1..79505fb 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -11,8 +11,8 @@\n #include \"sha1-array.h\"\n #include \"transport.h\"\n \n-static char *server_capabilities;\n-static const char *parse_feature_value(const char *, const char *, int *);\n+struct string_list server_capabilities = STRING_LIST_INIT_DUP;\n+static const char *parse_feature_value(struct string_list *, const char *, int *);\n \n static int check_ref(const char *name, unsigned int flags)\n {\n@@ -81,18 +81,18 @@ reject:\n \n static void annotate_refs_with_symref_info(struct ref *ref)\n {\n+\tstruct string_list_item *item;\n \tstruct string_list symref = STRING_LIST_INIT_DUP;\n-\tconst char *feature_list = server_capabilities;\n \n-\twhile (feature_list) {\n-\t\tint len;\n+\tfor_each_string_list_item(item, &server_capabilities) {\n \t\tconst char *val;\n \n-\t\tval = parse_feature_value(feature_list, \"symref\", &len);\n-\t\tif (!val)\n-\t\t\tbreak;\n-\t\tparse_one_symref_info(&symref, val, len);\n-\t\tfeature_list = val + 1;\n+\t\tif (skip_prefix(item->string, \"symref\", &val)) {\n+\t\t\tif (!val)\n+\t\t\t\tcontinue;\n+\t\t\tval++; /* skip the = */\n+\t\t\tparse_one_symref_info(&symref, val, strlen(val));\n+\t\t}\n \t}\n \tstring_list_sort(&symref);\n \n@@ -155,9 +155,12 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\tname = buffer + GIT_SHA1_HEXSZ + 1;\n \n \t\tname_len = strlen(name);\n+\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\tstring_list_clear(&server_capabilities, 1);\n+\t\t\tstring_list_split(&server_capabilities,\n+\t\t\t\t\t name + name_len + 1,\n+\t\t\t\t\t ' ', -1);\n \t\t}\n \n \t\tif (extra_have && !strcmp(name, \".have\")) {\n@@ -179,51 +182,40 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \treturn list;\n }\n \n-static const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp)\n+static const char *parse_feature_value(struct string_list *feature_list, const char *feature, int *lenp)\n {\n-\tint len;\n-\n-\tif (!feature_list)\n-\t\treturn NULL;\n-\n-\tlen = strlen(feature);\n-\twhile (*feature_list) {\n-\t\tconst char *found = strstr(feature_list, feature);\n-\t\tif (!found)\n-\t\t\treturn NULL;\n-\t\tif (feature_list == found || isspace(found[-1])) {\n-\t\t\tconst char *value = found + len;\n-\t\t\t/* feature with no value (e.g., \"thin-pack\") */\n-\t\t\tif (!*value || isspace(*value)) {\n-\t\t\t\tif (lenp)\n-\t\t\t\t\t*lenp = 0;\n-\t\t\t\treturn value;\n-\t\t\t}\n-\t\t\t/* feature with a value (e.g., \"agent=git/1.2.3\") */\n-\t\t\telse if (*value == '=') {\n-\t\t\t\tvalue++;\n-\t\t\t\tif (lenp)\n-\t\t\t\t\t*lenp = strcspn(value, \" \\t\\n\");\n-\t\t\t\treturn value;\n-\t\t\t}\n-\t\t\t/*\n-\t\t\t * otherwise we matched a substring of another feature;\n-\t\t\t * keep looking\n-\t\t\t */\n+\tconst char *value;\n+\tstruct string_list_item *item;\n+\n+\tfor_each_string_list_item(item, feature_list) {\n+\t\tif (!skip_prefix(item->string, feature, &value))\n+\t\t\tcontinue;\n+\n+\t\t/* feature with no value (e.g., \"thin-pack\") */\n+\t\tif (!*value) {\n+\t\t\tif (lenp)\n+\t\t\t\t*lenp = 0;\n+\t\t\treturn value;\n+\t\t}\n+\t\t/* feature with a value (e.g., \"agent=git/1.2.3\") */\n+\t\telse if (*value == '=') {\n+\t\t\tvalue++;\n+\t\t\tif (lenp)\n+\t\t\t\t*lenp = strlen(value);\n+\t\t\treturn value;\n \t\t}\n-\t\tfeature_list = found + 1;\n \t}\n \treturn NULL;\n }\n \n-int parse_feature_request(const char *feature_list, const char *feature)\n+int parse_feature_request(struct string_list *feature_list, const char *feature)\n {\n \treturn !!parse_feature_value(feature_list, feature, NULL);\n }\n \n const char *server_feature_value(const char *feature, int *len)\n {\n-\treturn parse_feature_value(server_capabilities, feature, len);\n+\treturn parse_feature_value(&server_capabilities, feature, len);\n }\n \n int server_supports(const char *feature)\ndiff --git a/connect.h b/connect.h\nindex 01f14cd..eaf21c2 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -9,7 +9,7 @@ extern struct child_process *git_connect(int fd[2], const char *url, const char\n extern int finish_connect(struct child_process *conn);\n extern int git_connection_is_socket(struct child_process *conn);\n extern int server_supports(const char *feature);\n-extern int parse_feature_request(const char *features, const char *feature);\n+extern int parse_feature_request(struct string_list *, const char *feature);\n extern const char *server_feature_value(const char *feature, int *len_ret);\n extern int url_is_local_not_ssh(const char *url);\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex edfd417..4e69b8e 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -535,7 +535,7 @@ error:\n \t}\n }\n \n-static void parse_features(const char *features)\n+static void parse_features(struct string_list *features)\n {\n \tif (parse_feature_request(features, \"multi_ack_detailed\"))\n \t\tmulti_ack = 2;\n@@ -567,7 +567,9 @@ static void receive_needs(void)\n \tfor (;;) {\n \t\tstruct object *o;\n \t\tunsigned char sha1_buf[20];\n+\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n \t\tchar *line = packet_read_line(0, NULL);\n+\n \t\treset_timeout();\n \t\tif (!line)\n \t\t\tbreak;\n@@ -600,7 +602,9 @@ static void receive_needs(void)\n \t\t\tdie(\"git upload-pack: protocol error, \"\n \t\t\t \"expected to get sha, not '%s'\", line);\n \n-\t\tparse_features(line + 45);\n+\t\tstring_list_split(&list, line + 45, ' ', -1);\n+\t\tparse_features(&list);\n+\t\tstring_list_clear(&list, 1);\n \n \t\to = parse_object(sha1_buf);\n \t\tif (!o)\n@@ -840,9 +844,12 @@ static void send_capabilities_version_2(void)\n \n static void receive_capabilities_version_2(void)\n {\n+\tstruct string_list list = STRING_LIST_INIT_NODUP;\n \tchar *line = packet_read_line(0, NULL);\n \twhile (line) {\n-\t\tparse_features(line);\n+\t\tstring_list_append(&list, line);\n+\t\tparse_features(&list);\n+\t\tstring_list_clear(&list, 1);\n \t\tline = packet_read_line(0, NULL);\n \t}\n }\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"284996","messageId":"1461972887-22100-6-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 05/14] transport: add infrastructure to support a protocol version number","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:38Z","receivedAt":"2016-04-29T23:34:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n fetch-pack.h | 1 +\n remote.c | 2 ++\n remote.h | 2 ++\n transport-helper.c | 1 +\n transport.c | 20 ++++++++++++++++++--\n transport.h | 8 ++++++++\n 6 files changed, 32 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex bb7fd76..3314362 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -8,6 +8,7 @@ struct sha1_array;\n \n struct fetch_pack_args {\n \tconst char *uploadpack;\n+\tint transport_version;\n \tint unpacklimit;\n \tint depth;\n \tunsigned quiet:1;\ndiff --git a/remote.c b/remote.c\nindex 28fd676..760611d 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -9,6 +9,7 @@\n #include \"string-list.h\"\n #include \"mergesort.h\"\n #include \"argv-array.h\"\n+#include \"transport.h\"\n \n enum map_direction { FROM_SRC, FROM_DST };\n \n@@ -164,6 +165,7 @@ static struct remote *make_remote(const char *name, int len)\n \t\treturn ret;\n \n \tret = xcalloc(1, sizeof(struct remote));\n+\tret->transport_version = DEFAULT_TRANSPORT_VERSION;\n \tret->prune = -1; /* unspecified */\n \tALLOC_GROW(remotes, remotes_nr + 1, remotes_alloc);\n \tremotes[remotes_nr++] = ret;\ndiff --git a/remote.h b/remote.h\nindex c21fd37..cdb25d0 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -51,6 +51,8 @@ struct remote {\n \tconst char *receivepack;\n \tconst char *uploadpack;\n \n+\tint transport_version;\n+\n \t/*\n \t * for curl remotes only\n \t */\ndiff --git a/transport-helper.c b/transport-helper.c\nindex b934183..3cb386f 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -248,6 +248,7 @@ static int disconnect_helper(struct transport *transport)\n }\n \n static const char *unsupported_options[] = {\n+\tTRANS_OPT_TRANSPORTVERSION,\n \tTRANS_OPT_UPLOADPACK,\n \tTRANS_OPT_RECEIVEPACK,\n \tTRANS_OPT_THIN,\ndiff --git a/transport.c b/transport.c\nindex 095e61f..608b92c 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -151,6 +151,16 @@ static int set_git_option(struct git_transport_options *opts,\n \t\t\t\tdie(\"transport: invalid depth option '%s'\", value);\n \t\t}\n \t\treturn 0;\n+\t} else if (!strcmp(name, TRANS_OPT_TRANSPORTVERSION)) {\n+\t\tif (!value)\n+\t\t\topts->transport_version = DEFAULT_TRANSPORT_VERSION;\n+\t\telse {\n+\t\t\tchar *end;\n+\t\t\topts->transport_version = strtol(value, &end, 0);\n+\t\t\tif (*end)\n+\t\t\t\tdie(\"transport: invalid transport version option '%s'\", value);\n+\t\t}\n+\t\treturn 0;\n \t}\n \treturn 1;\n }\n@@ -203,6 +213,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \n \tmemset(&args, 0, sizeof(args));\n \targs.uploadpack = data->options.uploadpack;\n+\targs.transport_version = data->options.transport_version;\n \targs.keep_pack = data->options.keep;\n \targs.lock_pack = 1;\n \targs.use_thin_pack = data->options.thin;\n@@ -694,6 +705,8 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tret->connect = connect_git;\n \t\tret->disconnect = disconnect_git;\n \t\tret->smart_options = &(data->options);\n+\t\tret->smart_options->transport_version =\n+\t\t\tDEFAULT_TRANSPORT_VERSION;\n \n \t\tdata->conn = NULL;\n \t\tdata->got_remote_heads = 0;\n@@ -706,12 +719,15 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \n \tif (ret->smart_options) {\n \t\tret->smart_options->thin = 1;\n-\t\tret->smart_options->uploadpack = \"git-upload-pack\";\n+\t\tret->smart_options->uploadpack = DEFAULT_TRANSPORT_UPLOAD_PACK;\n \t\tif (remote->uploadpack)\n \t\t\tret->smart_options->uploadpack = remote->uploadpack;\n-\t\tret->smart_options->receivepack = \"git-receive-pack\";\n+\t\tret->smart_options->receivepack = DEFAULT_TRANSPORT_RECEIVE_PACK;\n \t\tif (remote->receivepack)\n \t\t\tret->smart_options->receivepack = remote->receivepack;\n+\t\tif (remote->transport_version)\n+\t\t\tret->smart_options->transport_version =\n+\t\t\t\tremote->transport_version;\n \t}\n \n \treturn ret;\ndiff --git a/transport.h b/transport.h\nindex c681408..af90529 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -13,6 +13,7 @@ struct git_transport_options {\n \tunsigned self_contained_and_connected : 1;\n \tunsigned update_shallow : 1;\n \tint depth;\n+\tint transport_version;\n \tconst char *uploadpack;\n \tconst char *receivepack;\n \tstruct push_cas_option *cas;\n@@ -188,6 +189,13 @@ int transport_restrict_protocols(void);\n /* Send push certificates */\n #define TRANS_OPT_PUSH_CERT \"pushcert\"\n \n+/* Use a new version of the git protocol */\n+#define TRANS_OPT_TRANSPORTVERSION \"transportversion\"\n+\n+#define DEFAULT_TRANSPORT_VERSION 1\n+#define DEFAULT_TRANSPORT_UPLOAD_PACK \"git-upload-pack\"\n+#define DEFAULT_TRANSPORT_RECEIVE_PACK \"git-receive-pack\"\n+\n /**\n * Returns 0 if the option was used, non-zero otherwise. Prints a\n * message to stderr if the option is not used.\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"285005","messageId":"1461972887-22100-7-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 06/14] remote.h: add get_remote_capabilities, request_capabilities","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:39Z","receivedAt":"2016-04-29T23:34:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Instead of calling get_remote_heads as a first command during the\nprotocol exchange, we need to have fine grained control over the\ncapability negotiation in version 2 of the protocol.\n\nIntroduce get_remote_capabilities, which will just listen to\ncapabilities of the remote and request_capabilities which will\ntell the selection of capabilities to the remote.\n\nSigned-off-by: Stefan Beller \n---\n connect.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n remote.h | 3 +++\n 2 files changed, 62 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 79505fb..1ba9a0f 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -10,6 +10,7 @@\n #include \"string-list.h\"\n #include \"sha1-array.h\"\n #include \"transport.h\"\n+#include \"version.h\"\n \n struct string_list server_capabilities = STRING_LIST_INIT_DUP;\n static const char *parse_feature_value(struct string_list *, const char *, int *);\n@@ -106,6 +107,64 @@ static void annotate_refs_with_symref_info(struct ref *ref)\n \tstring_list_clear(&symref, 0);\n }\n \n+const char *known_capabilities[] = {\n+\t\"multi_ack\",\n+\t\"thin-pack\",\n+\t\"side-band\",\n+\t\"side-band-64k\",\n+\t\"ofs-delta\",\n+\t\"shallow\",\n+\t\"no-progress\",\n+\t\"include-tag\",\n+\t\"multi_ack_detailed\",\n+\t\"allow-tip-sha1-in-want\",\n+\t\"allow-reachable-sha1-in-want\",\n+\t\"no-done\",\n+};\n+\n+static int keep_capability(char *line)\n+{\n+\tint i;\n+\tfor (i = 0; i < ARRAY_SIZE(known_capabilities); i++)\n+\t\tif (starts_with(line, known_capabilities[i]))\n+\t\t\treturn 1;\n+\treturn 0;\n+}\n+\n+void get_remote_capabilities(int in, char *src_buf, size_t src_len)\n+{\n+\tfor (;;) {\n+\t\tint len;\n+\t\tchar *line = packet_buffer;\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(0);\n+\n+\t\tif (!len)\n+\t\t\tbreak;\n+\n+\t\t/*\n+\t\t * We need to ignore and drop unknown capabilities as they\n+\t\t * may be huge.\n+\t\t */\n+\t\tif (keep_capability(line))\n+\t\t\tstring_list_append(&server_capabilities, line);\n+\t}\n+}\n+\n+\n+void request_capabilities(int out, struct string_list *list)\n+{\n+\tstruct string_list_item *item;\n+\tfor_each_string_list_item(item, list)\n+\t\tpacket_write(out, \"%s\\n\", item->string);\n+\tpacket_flush(out);\n+}\n+\n /*\n * Read all the refs from the other end\n */\ndiff --git a/remote.h b/remote.h\nindex cdb25d0..534282b 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -153,6 +153,9 @@ extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\t\t struct sha1_array *extra_have,\n \t\t\t\t struct sha1_array *shallow);\n \n+extern void get_remote_capabilities(int in, char *src_buf, size_t src_len);\n+extern void request_capabilities(int out, struct string_list*);\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);\n \n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"285006","messageId":"1461972887-22100-8-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 07/14] fetch-pack: move capability selection out of do_fetch_pack","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:40Z","receivedAt":"2016-04-29T23:34:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Later in version 2 of the pack protocol the selection of capabilities\nhappens at another step of the protocol, so move out the current\ncapability selection, so we can reuse it later more easily.\n\nSigned-off-by: Stefan Beller \n---\n fetch-pack.c | 28 +++++++++++++++++-----------\n 1 file changed, 17 insertions(+), 11 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f96f6df..53f6384 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -796,21 +796,11 @@ static int cmp_ref_by_name(const void *a_, const void *b_)\n \treturn strcmp(a->name, b->name);\n }\n \n-static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n-\t\t\t\t int fd[2],\n-\t\t\t\t const struct ref *orig_ref,\n-\t\t\t\t struct ref **sought, int nr_sought,\n-\t\t\t\t struct shallow_info *si,\n-\t\t\t\t char **pack_lockfile)\n+static void select_capabilities(struct fetch_pack_args *args)\n {\n-\tstruct ref *ref = copy_ref_list(orig_ref);\n-\tunsigned char sha1[20];\n \tconst char *agent_feature;\n \tint agent_len;\n \n-\tsort_ref_list(&ref, ref_compare_name);\n-\tqsort(sought, nr_sought, sizeof(*sought), cmp_ref_by_name);\n-\n \tif ((args->depth > 0 || is_repository_shallow()) && !server_supports(\"shallow\"))\n \t\tdie(\"Server does not support shallow clients\");\n \tif (server_supports(\"multi_ack_detailed\")) {\n@@ -867,6 +857,22 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\tfprintf(stderr, \"Server version is %.*s\\n\",\n \t\t\t\tagent_len, agent_feature);\n \t}\n+}\n+\n+static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n+\t\t\t\t int fd[2],\n+\t\t\t\t const struct ref *orig_ref,\n+\t\t\t\t struct ref **sought, int nr_sought,\n+\t\t\t\t struct shallow_info *si,\n+\t\t\t\t char **pack_lockfile)\n+{\n+\tstruct ref *ref = copy_ref_list(orig_ref);\n+\tunsigned char sha1[20];\n+\n+\tsort_ref_list(&ref, ref_compare_name);\n+\tqsort(sought, nr_sought, sizeof(*sought), cmp_ref_by_name);\n+\n+\tselect_capabilities(args);\n \n \tif (everything_local(args, &ref, sought, nr_sought)) {\n \t\tpacket_flush(fd[1]);\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"285004","messageId":"1461972887-22100-9-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 08/14] fetch-pack: factor out get_selected_capabilities_list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:41Z","receivedAt":"2016-04-29T23:34:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This will be used later by both versions of the transport protocol.\n\nSigned-off-by: Stefan Beller \n---\n fetch-pack.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 40 insertions(+), 11 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 53f6384..b43490f 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -257,6 +257,9 @@ static int next_flush(struct fetch_pack_args *args, int count)\n \treturn count;\n }\n \n+static void get_selected_capabilities_list(struct string_list *list,\n+\t\t\t\t\t struct fetch_pack_args *args);\n+\n static int find_common(struct fetch_pack_args *args,\n \t\t int fd[2], unsigned char *result_sha1,\n \t\t struct ref *refs)\n@@ -302,18 +305,15 @@ static int find_common(struct fetch_pack_args *args,\n \n \t\tremote_hex = sha1_to_hex(remote);\n \t\tif (!fetching) {\n+\t\t\tstruct string_list list = STRING_LIST_INIT_NODUP;\n \t\t\tstruct strbuf c = STRBUF_INIT;\n-\t\t\tif (multi_ack == 2) strbuf_addstr(&c, \" multi_ack_detailed\");\n-\t\t\tif (multi_ack == 1) strbuf_addstr(&c, \" multi_ack\");\n-\t\t\tif (no_done) strbuf_addstr(&c, \" no-done\");\n-\t\t\tif (use_sideband == 2) strbuf_addstr(&c, \" side-band-64k\");\n-\t\t\tif (use_sideband == 1) strbuf_addstr(&c, \" side-band\");\n-\t\t\tif (args->use_thin_pack) strbuf_addstr(&c, \" thin-pack\");\n-\t\t\tif (args->no_progress) strbuf_addstr(&c, \" no-progress\");\n-\t\t\tif (args->include_tag) strbuf_addstr(&c, \" include-tag\");\n-\t\t\tif (prefer_ofs_delta) strbuf_addstr(&c, \" ofs-delta\");\n-\t\t\tif (agent_supported) strbuf_addf(&c, \" agent=%s\",\n-\t\t\t\t\t\t\t git_user_agent_sanitized());\n+\t\t\tstruct string_list_item *item;\n+\t\t\tget_selected_capabilities_list(&list, args);\n+\t\t\tfor_each_string_list_item(item, &list) {\n+\t\t\t\tstrbuf_addstr(&c, \" \");\n+\t\t\t\tstrbuf_addstr(&c, item->string);\n+\t\t\t}\n+\n \t\t\tpacket_buf_write(&req_buf, \"want %s%s\\n\", remote_hex, c.buf);\n \t\t\tstrbuf_release(&c);\n \t\t} else\n@@ -859,6 +859,35 @@ static void select_capabilities(struct fetch_pack_args *args)\n \t}\n }\n \n+static void get_selected_capabilities_list(struct string_list *list,\n+\t\t\t\t\t struct fetch_pack_args *args)\n+{\n+\tif (multi_ack == 2)\n+\t\tstring_list_append(list, \"multi_ack_detailed\");\n+\tif (multi_ack == 1)\n+\t\tstring_list_append(list, \"multi_ack\");\n+\tif (no_done)\n+\t\tstring_list_append(list, \"no-done\");\n+\tif (use_sideband == 2)\n+\t\tstring_list_append(list, \"side-band-64k\");\n+\tif (use_sideband == 1)\n+\t\tstring_list_append(list, \"side-band\");\n+\tif (args->use_thin_pack)\n+\t\tstring_list_append(list, \"thin-pack\");\n+\tif (args->no_progress)\n+\t\tstring_list_append(list, \"no-progress\");\n+\tif (args->include_tag)\n+\t\tstring_list_append(list, \"include-tag\");\n+\tif (prefer_ofs_delta)\n+\t\tstring_list_append(list, \"ofs-delta\");\n+\tif (agent_supported) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tstrbuf_addf(&buf, \"agent=%s\", git_user_agent_sanitized());\n+\t\tstring_list_append(list, buf.buf);\n+\t\tstrbuf_release(&buf);\n+\t}\n+}\n+\n static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\t\t int fd[2],\n \t\t\t\t const struct ref *orig_ref,\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"284997","messageId":"1461972887-22100-10-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 09/14] fetch-pack: Add negotiate_capabilities","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:42Z","receivedAt":"2016-04-29T23:34:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As in the pack protocol version 2 we want to\nnegotiate the capabilities before any other exchange\nwe will have a function which will take care of the\nwhole negotiation process.\n\nIt will be placed in fetch-pack.c for now as there\nwe have access to its internal variables and we'll\nwork on a `struct fetch_pack_args`. Eventually we\nwant to move it to a better place such as transport.c\n\nSigned-off-by: Stefan Beller \n---\n fetch-pack.c | 14 ++++++++++++++\n fetch-pack.h | 6 ++++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b43490f..1544629 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -888,6 +888,20 @@ static void get_selected_capabilities_list(struct string_list *list,\n \t}\n }\n \n+void negotiate_capabilities(int fd[2], struct fetch_pack_args *args)\n+{\n+\tstruct string_list list = STRING_LIST_INIT_NODUP;\n+\n+\tget_remote_capabilities(fd[0], NULL, 0);\n+\n+\tselect_capabilities(args);\n+\tget_selected_capabilities_list(&list, args);\n+\n+\trequest_capabilities(fd[1], &list);\n+\n+\tstring_list_clear(&list, 1);\n+}\n+\n static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\t\t int fd[2],\n \t\t\t\t const struct ref *orig_ref,\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 3314362..198498a 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -29,6 +29,12 @@ struct fetch_pack_args {\n };\n \n /*\n+ * In version 2 of the pack protocol we negotiate the capabilities\n+ * before the actual transfer of refs and packs.\n+ */\n+void negotiate_capabilities(int fd[2], struct fetch_pack_args *args);\n+\n+/*\n * sought represents remote references that should be updated from.\n * On return, the names that were found on the remote will have been\n * marked as such.\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"285002","messageId":"1461972887-22100-11-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 10/14] do_fetch_pack: select capabilities for transport version 1 only","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:43Z","receivedAt":"2016-04-29T23:34:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`select_capabilities` would set local variables depending on the\nuser selection provided by `args` and the server advertisement which\nis kept in a list in connect.c\n\nWhen talking pack protocol version 2 however this has already happend\nbefore during 'negotiate_capabilities'\n\nSigned-off-by: Stefan Beller \n---\n fetch-pack.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 1544629..5ca1e97 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -915,7 +915,16 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tsort_ref_list(&ref, ref_compare_name);\n \tqsort(sought, nr_sought, sizeof(*sought), cmp_ref_by_name);\n \n-\tselect_capabilities(args);\n+\tswitch (args->transport_version) {\n+\tcase 2:\n+\t\t/* capability selection already happend */\n+\t\tbreak;\n+\tcase 1:\n+\t\tselect_capabilities(args);\n+\t\tbreak;\n+\tdefault:\n+\t\tdie (\"transport version %d not supported\", args->transport_version);\n+\t}\n \n \tif (everything_local(args, &ref, sought, nr_sought)) {\n \t\tpacket_flush(fd[1]);\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"284998","messageId":"1461972887-22100-12-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 11/14] builtin/fetch-pack: add argument for transport version","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:44Z","receivedAt":"2016-04-29T23:34:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n builtin/fetch-pack.c | 20 +++++++++++++++++++-\n 1 file changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex bfd0be4..afb614b 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -4,6 +4,7 @@\n #include \"remote.h\"\n #include \"connect.h\"\n #include \"sha1-array.h\"\n+#include \"transport.h\"\n \n static const char fetch_pack_usage[] =\n \"git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] \"\n@@ -56,6 +57,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \n \tmemset(&args, 0, sizeof(args));\n \targs.uploadpack = \"git-upload-pack\";\n+\targs.transport_version = DEFAULT_TRANSPORT_VERSION;\n \n \tfor (i = 1; i < argc && *argv[i] == '-'; i++) {\n \t\tconst char *arg = argv[i];\n@@ -130,6 +132,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\targs.update_shallow = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (starts_with(arg, \"--transport-version=\")) {\n+\t\t\targs.transport_version = strtol(arg + 20, NULL, 0);\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(fetch_pack_usage);\n \t}\n \n@@ -178,7 +184,19 @@ 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+\n+\tswitch (args.transport_version) {\n+\tcase 2: /* first talk about capabilities, then get the refs */\n+\t\tnegotiate_capabilities(fd, &args);\n+\t\t/* fall through */\n+\tcase 1:\n+\t\tget_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: Transport version %d not supported\",\n+\t\t\targs.transport_version);\n+\t\tbreak;\n+\t}\n \n \tref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,\n \t\t\t &shallow, pack_lockfile_ptr);\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"285001","messageId":"1461972887-22100-13-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 12/14] Add test for fetch-pack","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:45Z","receivedAt":"2016-04-29T23:34:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n t/t5500-fetch-pack.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 91a69fc..2c704ef 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -519,6 +519,7 @@ test_expect_success 'test --all, --depth, and explicit tag' '\n '\n \n test_expect_success 'shallow fetch with tags does not break the repository' '\n+\ttest_when_finished \"rm -rf repo1\" &&\n \tmkdir repo1 &&\n \t(\n \t\tcd repo1 &&\n@@ -547,6 +548,26 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n '\n \n+test_expect_success 'fetch-pack with protocol version 2' '\n+\ttest_when_finished \"rm -rf repo1\" &&\n+\tmkdir repo1 &&\n+\t(\n+\t\tcd repo1 &&\n+\t\tgit init &&\n+\t\ttest_commit 1 &&\n+\t\ttest_commit 2 &&\n+\t\ttest_commit 3 &&\n+\t\techo \"$(git rev-parse master) refs/heads/master\" >expected &&\n+\t\tmkdir repo2 &&\n+\t\t(\n+\t\t\tcd repo2 &&\n+\t\t\tgit init &&\n+\t\t\tgit fetch-pack --transport-version=2 --upload-pack=git-upload-pack-2 ../.git refs/heads/master >../actual\n+\t\t) &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"285000","messageId":"1461972887-22100-14-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 13/14] WIP add test for git pull","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:46Z","receivedAt":"2016-04-29T23:34:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n t/t5520-pull.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 739c089..9bd1feb 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -599,4 +599,10 @@ test_expect_success 'git pull --rebase against local branch' '\n \ttest file = \"$(cat file2)\"\n '\n \n+test_expect_success 'git pull with protocol version 2' '\n+\ttest_pause &&\n+\tgit pull --transport-version=2\n+\n+'\n+\n test_done\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"284999","messageId":"1461972887-22100-15-git-send-email-sbeller@google.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"[PATCH 14/14] WIP test git fetch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-29T23:34:47Z","receivedAt":"2016-04-29T23:34:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n t/t5510-fetch.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 38321d1..ab5d3da 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -687,4 +687,9 @@ test_expect_success 'fetching with auto-gc does not lock up' '\n \t)\n '\n \n+test_expect_success 'fetching with transport protocol 2 works' '\n+\ttest_pause\n+\tgit fetch --transport-protocol=2\n+'\n+\n test_done\n-- \n2.8.0.32.g71f8beb.dirty\n"},{"id":"285040","messageId":"1461978246.4123.45.camel@twopensource.com","threadId":"42183","inReplyTo":"1461972887-22100-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 02/14] upload-pack.c: Refactor capability advertising","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-30T01:04:06Z","receivedAt":"2016-04-30T01:04:06Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n> Instead of having the capabilities in a local string, keep them\n> in a struct outside the function. This will allow us in a later patch\n\nnit: s/struct/array/\n"},{"id":"285170","messageId":"1462210997.4123.49.camel@twopensource.com","threadId":"42183","inReplyTo":"1461972887-22100-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 03/14] upload-pack-2: Implement the version 2 of upload-pack","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-02T17:43:17Z","receivedAt":"2016-05-02T17:43:17Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n> In upload-pack-2 we send each capability in its own packet buffer.\n> The construction of upload-pack-2 is a bit unfortunate as I would\n> like\n> it to not be depending on a symlink linking to upload-pack.c, but I\n> did\n> not find another easy way to do it. I would like it to generate\n> upload-pack-2.o from upload-pack.c but with '-DTRANSPORT_VERSION=2'\n> set.\n\nCouldn't we check argv[0] and use that to determine protocol? Then we\ncould symlink executables rather than source code.\n"},{"id":"285173","messageId":"CAGZ79kavBUohoMcC3iEws5mVBQ6HZsR=HyBoxM1jxUVJ6Z1jkA@mail.gmail.com","threadId":"42183","inReplyTo":"1462210997.4123.49.camel@twopensource.com","subject":"Re: [PATCH 03/14] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-02T17:51:36Z","receivedAt":"2016-05-02T17:51:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 2, 2016 at 10:43 AM, David Turner wrote:\n> On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n>> In upload-pack-2 we send each capability in its own packet buffer.\n>> The construction of upload-pack-2 is a bit unfortunate as I would\n>> like\n>> it to not be depending on a symlink linking to upload-pack.c, but I\n>> did\n>> not find another easy way to do it. I would like it to generate\n>> upload-pack-2.o from upload-pack.c but with '-DTRANSPORT_VERSION=2'\n>> set.\n>\n> Couldn't we check argv[0] and use that to determine protocol? Then we\n> could symlink executables rather than source code.\n\nIIRC I proposed a similar thing earlier, i.e.\n\n if (argv[0] ends with 2)\n do_protocol_v_2(...)\n\nbut that may break (and confuse a lot!) some use cases.\n`git fetch` has the documented --upload-pack switch, so as a server-admin\nyou are free to have git-upload-pack linking to\n\"git-upload-pack-2.8\" but additionally you still have\n\"git-upload-pack-1.7\" or \"git-upload-pack-custom-2\".\n\nso I think we should not do that :(\nI do like to symlink the executables though.\n"},{"id":"285183","messageId":"1462213121.4123.58.camel@twopensource.com","threadId":"42183","inReplyTo":"1461972887-22100-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 04/14] connect: rewrite feature parsing to work on string_list","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-02T18:18:41Z","receivedAt":"2016-05-02T18:18:41Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n> +\t\tif (skip_prefix(item->string, \"symref\", &val)) {\n> +\t\t\tif (!val)\n> +\t\t\t\tcontinue;\n\nThis if should never happen (skip_prefix returns 0 in that case). You\nprobably meant !*val -- but:\n\n> +\t\t\tval++; /* skip the = */\n\nI think you should instead skip_prefix \"symref=\" because:\n(a) it saves some code.\n(b) it allows for capabilities like symref_foo to later be added.\n\n> +\tstruct string_list list = STRING_LIST_INIT_NODUP;\n\nMaybe move the scope of list into the while loop below?\n\n> \tchar *line = packet_read_line(0, NULL);\n> \twhile (line) {\n> -\t\tparse_features(line);\n> +\t\tstring_list_append(&list, line);\n> +\t\tparse_features(&list);\n> +\t\tstring_list_clear(&list, 1);\n> \t\tline = packet_read_line(0, NULL);\n\nThis is a bit convoluted in the one-feature-per-line case, but I guess\nI understand that for the sake of generality it's useful.\n"},{"id":"285187","messageId":"CAGZ79kbUekUtfzmW00UbCy-Qpu5jVM0gE-6n=4N5++-T+Ed8+g@mail.gmail.com","threadId":"42183","inReplyTo":"1462213121.4123.58.camel@twopensource.com","subject":"Re: [PATCH 04/14] connect: rewrite feature parsing to work on string_list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-02T18:46:41Z","receivedAt":"2016-05-02T18:46:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 2, 2016 at 11:18 AM, David Turner wrote:\n> On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n>> + if (skip_prefix(item->string, \"symref\", &val)) {\n>> + if (!val)\n>> + continue;\n>\n> This if should never happen (skip_prefix returns 0 in that case). You\n> probably meant !*val -- but:\n>\n>> + val++; /* skip the = */\n>\n> I think you should instead skip_prefix \"symref=\" because:\n> (a) it saves some code.\n> (b) it allows for capabilities like symref_foo to later be added.\n>\n>> + struct string_list list = STRING_LIST_INIT_NODUP;\n>\n> Maybe move the scope of list into the while loop below?\n>\n>> char *line = packet_read_line(0, NULL);\n>> while (line) {\n>> - parse_features(line);\n>> + string_list_append(&list, line);\n>> + parse_features(&list);\n>> + string_list_clear(&list, 1);\n>> line = packet_read_line(0, NULL);\n>\n> This is a bit convoluted in the one-feature-per-line case, but I guess\n> I understand that for the sake of generality it's useful.\n\nThanks for the review,\nStefan\n"},{"id":"285189","messageId":"1462215375.4123.69.camel@twopensource.com","threadId":"42183","inReplyTo":"CAGZ79kavBUohoMcC3iEws5mVBQ6HZsR=HyBoxM1jxUVJ6Z1jkA@mail.gmail.com","subject":"Re: [PATCH 03/14] upload-pack-2: Implement the version 2 of upload-pack","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-02T18:56:15Z","receivedAt":"2016-05-02T18:56:15Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Mon, 2016-05-02 at 10:51 -0700, Stefan Beller wrote:\n> On Mon, May 2, 2016 at 10:43 AM, David Turner <\n> dturner@twopensource.com> wrote:\n> > On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n> > > In upload-pack-2 we send each capability in its own packet\n> > > buffer.\n> > > The construction of upload-pack-2 is a bit unfortunate as I would\n> > > like\n> > > it to not be depending on a symlink linking to upload-pack.c, but\n> > > I\n> > > did\n> > > not find another easy way to do it. I would like it to generate\n> > > upload-pack-2.o from upload-pack.c but with '\n> > > -DTRANSPORT_VERSION=2'\n> > > set.\n> > \n> > Couldn't we check argv[0] and use that to determine protocol? Then\n> > we\n> > could symlink executables rather than source code.\n> \n> IIRC I proposed a similar thing earlier, i.e.\n> \n> if (argv[0] ends with 2)\n> do_protocol_v_2(...)\n> \n> but that may break (and confuse a lot!) some use cases.\n> `git fetch` has the documented --upload-pack switch, so as a server\n> -admin\n> you are free to have git-upload-pack linking to\n> \"git-upload-pack-2.8\" but additionally you still have\n> \"git-upload-pack-1.7\" or \"git-upload-pack-custom-2\".\n> \n> so I think we should not do that :(\n> I do like to symlink the executables though.\n\nI think it would probably not break anyone if the new executable name\nwere sufficiently distinctive -- e.g. starts_with (strrchr(argv[0],\n'/'), \"git-upload-pack-protocol-v2\"). But it would make custom\nexecutables a bit more complicated for the future.\n\nI guess it is better to have silly source code but clean binaries than\nclean source code but silly user-visible rules.\n"},{"id":"285190","messageId":"1462215463.4123.70.camel@twopensource.com","threadId":"42183","inReplyTo":"1461972887-22100-7-git-send-email-sbeller@google.com","subject":"Re: [PATCH 06/14] remote.h: add get_remote_capabilities, request_capabilities","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-02T18:57:43Z","receivedAt":"2016-05-02T18:57:43Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n \n> +const char *known_capabilities[] = {\n> +\t\"multi_ack\",\n> +\t\"thin-pack\",\n> +\t\"side-band\",\n> +\t\"side-band-64k\",\n> +\t\"ofs-delta\",\n> +\t\"shallow\",\n> +\t\"no-progress\",\n> +\t\"include-tag\",\n> +\t\"multi_ack_detailed\",\n> +\t\"allow-tip-sha1-in-want\",\n> +\t\"allow-reachable-sha1-in-want\",\n> +\t\"no-done\",\n> +};\n\nI wonder if it is possible to not repeat the list from upload-pack.c?\nIt seems unfortunate to have to add the same string in two places\nwhenever you add a capability.\n\n> +static int keep_capability(char *line)\n\ns/keep_/is_known_/ ? Also it would be good to handle capabilities that\nare prefixes of others correctly.\n\n> +{\n> +\tint i;\n> +\tfor (i = 0; i < ARRAY_SIZE(known_capabilities); i++)\n> +\t\tif (starts_with(line, known_capabilities[i]))\n> +\t\t\treturn 1;\n> +\treturn 0;\n> +}\n> +\n> +void get_remote_capabilities(int in, char *src_buf, size_t src_len)\n\nmaybe rename \"in\" to \"fd\" or \"in_fd\"? I don't immediately know what\n\"in\" is supposed to be when I just look at this signature.\n\n> +void request_capabilities(int out, struct string_list *list)\n\nMaybe name this \"send_capabilities_request\"?\n"},{"id":"285192","messageId":"1462216172.4123.71.camel@twopensource.com","threadId":"42183","inReplyTo":"1461972887-22100-9-git-send-email-sbeller@google.com","subject":"Re: [PATCH 08/14] fetch-pack: factor out get_selected_capabilities_list","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-02T19:09:32Z","receivedAt":"2016-05-02T19:09:32Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n> +\t\t\tstruct string_list_item *item;\n> +\t\t\tget_selected_capabilities_list(&list, args);\n> +\t\t\tfor_each_string_list_item(item, &list) {\n> +\t\t\t\tstrbuf_addstr(&c, \" \");\n> +\t\t\t\tstrbuf_addstr(&c, item->string);\n> +\t\t\t}\n> +\n\nI think the contents of list leak here -- you need a string_list_clear.\n"},{"id":"285201","messageId":"1462218350.4123.74.camel@twopensource.com","threadId":"42183","inReplyTo":"1461972887-22100-13-git-send-email-sbeller@google.com","subject":"Re: [PATCH 12/14] Add test for fetch-pack","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-02T19:45:50Z","receivedAt":"2016-05-02T19:45:50Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n> +test_expect_success 'fetch-pack with protocol version 2' '\n> +\ttest_when_finished \"rm -rf repo1\" &&\n> +\tmkdir repo1 &&\n> +\t(\n> +\t\tcd repo1 &&\n> +\t\tgit init &&\n> +\t\ttest_commit 1 &&\n> +\t\ttest_commit 2 &&\n> +\t\ttest_commit 3 &&\n> +\t\techo \"$(git rev-parse master) refs/heads/master\"\n> >expected &&\n> +\t\tmkdir repo2 &&\n> +\t\t(\n> +\t\t\tcd repo2 &&\n> +\t\t\tgit init &&\n> +\t\t\tgit fetch-pack --transport-version=2 -\n> -upload-pack=git-upload-pack-2 ../.git refs/heads/master >../actual\n> +\t\t) &&\n> +\t\ttest_cmp expected actual\n> +\t)\n> +'\n\nThis doesn't actually test that protocol v2 is in fact used (it just\ntests that --transport-version=2 doesn't crash). It would be nice to\nactually test the version in-use.\n"},{"id":"285208","messageId":"1462221687.4123.79.camel@twopensource.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"Re: [WIP PATCH 00/14] Protocol v2 patches","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-02T20:41:27Z","receivedAt":"2016-05-02T20:41:27Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n> Hi David,\n> \n> here are my patches for a protocol v2.\n> \n> (\"Negotiate capabilities before doing anything else\", or as code:\n> \n> static void upload_pack_version_2(void)\n> {\n> send_capabilities_version_2();\n> receive_capabilities_version_2();\n> \n> /* The rest of the protocol stays the same,\n> capabilities advertising\n> is disabled though. */\n> advertise_capabilities = 0;\n> upload_pack();\n> }\n> )\n\nOverall, except for the comments I made, these patches seem sensible. \n\nWould it be possible to add some docs on the new protocol when you re\n-roll? (I know these are just the initial patches, but it really helps\nme to see an explanation along with the code).\n"},{"id":"285209","messageId":"CAGZ79kZVbK+bS_is-sbfMAhj0hPqpKUcFXAq0womwSXeWPKsnw@mail.gmail.com","threadId":"42183","inReplyTo":"1462221687.4123.79.camel@twopensource.com","subject":"Re: [WIP PATCH 00/14] Protocol v2 patches","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-02T20:43:44Z","receivedAt":"2016-05-02T20:43:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 2, 2016 at 1:41 PM, David Turner wrote:\n> On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n>> Hi David,\n>>\n>> here are my patches for a protocol v2.\n>>\n>> (\"Negotiate capabilities before doing anything else\", or as code:\n>>\n>> static void upload_pack_version_2(void)\n>> {\n>> send_capabilities_version_2();\n>> receive_capabilities_version_2();\n>>\n>> /* The rest of the protocol stays the same,\n>> capabilities advertising\n>> is disabled though. */\n>> advertise_capabilities = 0;\n>> upload_pack();\n>> }\n>> )\n>\n> Overall, except for the comments I made, these patches seem sensible.\n>\n> Would it be possible to add some docs on the new protocol when you re\n> -roll? (I know these are just the initial patches, but it really helps\n> me to see an explanation along with the code).\n>\n\nThanks for the review :) I'll fix the issues and add some docs, though\nthe reroll\nmay take some time. I really want to get the submodule groups stuff\ndone as well.\n"},{"id":"285235","messageId":"CACsJy8DxwQKcno0ikZCDZUQ3xAd-iWw9mqSGnzgb5GqdRwHsYA@mail.gmail.com","threadId":"42183","inReplyTo":"1462215375.4123.69.camel@twopensource.com","subject":"Re: [PATCH 03/14] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-03T00:31:02Z","receivedAt":"2016-05-03T00:31:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 3, 2016 at 1:56 AM, David Turner wrote:\n> On Mon, 2016-05-02 at 10:51 -0700, Stefan Beller wrote:\n>> On Mon, May 2, 2016 at 10:43 AM, David Turner <\n>> dturner@twopensource.com> wrote:\n>> > On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n>> > > In upload-pack-2 we send each capability in its own packet\n>> > > buffer.\n>> > > The construction of upload-pack-2 is a bit unfortunate as I would\n>> > > like\n>> > > it to not be depending on a symlink linking to upload-pack.c, but\n>> > > I\n>> > > did\n>> > > not find another easy way to do it. I would like it to generate\n>> > > upload-pack-2.o from upload-pack.c but with '\n>> > > -DTRANSPORT_VERSION=2'\n>> > > set.\n>> >\n>> > Couldn't we check argv[0] and use that to determine protocol? Then\n>> > we\n>> > could symlink executables rather than source code.\n>>\n>> IIRC I proposed a similar thing earlier, i.e.\n>>\n>> if (argv[0] ends with 2)\n>> do_protocol_v_2(...)\n>>\n>> but that may break (and confuse a lot!) some use cases.\n>> `git fetch` has the documented --upload-pack switch, so as a server\n>> -admin\n>> you are free to have git-upload-pack linking to\n>> \"git-upload-pack-2.8\" but additionally you still have\n>> \"git-upload-pack-1.7\" or \"git-upload-pack-custom-2\".\n>>\n>> so I think we should not do that :(\n>> I do like to symlink the executables though.\n>\n> I think it would probably not break anyone if the new executable name\n> were sufficiently distinctive -- e.g. starts_with (strrchr(argv[0],\n> '/'), \"git-upload-pack-protocol-v2\"). But it would make custom\n> executables a bit more complicated for the future.\n>\n> I guess it is better to have silly source code but clean binaries than\n> clean source code but silly user-visible rules.\n\nMaybe add --version to upload-pack? Then we can have a script\ngit-upload-pack-v2 that does \"exec git upload-pack --version=2\"\n-- \nDuy\n"},{"id":"285250","messageId":"20160503053337.GA17986@sigill.intra.peff.net","threadId":"42183","inReplyTo":"1462215463.4123.70.camel@twopensource.com","subject":"Re: [PATCH 06/14] remote.h: add get_remote_capabilities, request_capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-03T05:33:38Z","receivedAt":"2016-05-03T05:33:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 02, 2016 at 02:57:43PM -0400, David Turner wrote:\n\n> On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n> \n> > +const char *known_capabilities[] = {\n> > +\t\"multi_ack\",\n> > +\t\"thin-pack\",\n> > +\t\"side-band\",\n> > +\t\"side-band-64k\",\n> > +\t\"ofs-delta\",\n> > +\t\"shallow\",\n> > +\t\"no-progress\",\n> > +\t\"include-tag\",\n> > +\t\"multi_ack_detailed\",\n> > +\t\"allow-tip-sha1-in-want\",\n> > +\t\"allow-reachable-sha1-in-want\",\n> > +\t\"no-done\",\n> > +};\n> \n> I wonder if it is possible to not repeat the list from upload-pack.c?\n> It seems unfortunate to have to add the same string in two places\n> whenever you add a capability.\n\nI think that in general, we'd stop adding capabilities to v1. If you\nhave a client which speaks the new capability, then it should also be\nspeaking the new protocol. That's not strictly true if other non-git.git\nimplementations want to learn capability X but not protocol v2, but I\nthink in practice it's not an unreasonable world view.\n\nI guess there may be a grey area for a while, though, where even\nv2-capable clients don't end up speaking it, because they don't yet know\nthat a particular server can handle it. So any capabilities added in\nthat grey area may want to go to both v1 and v2.\n\n-Peff\n"},{"id":"285380","messageId":"1462310486.4123.81.camel@twopensource.com","threadId":"42183","inReplyTo":"20160503053337.GA17986@sigill.intra.peff.net","subject":"Re: [PATCH 06/14] remote.h: add get_remote_capabilities, request_capabilities","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-03T21:21:26Z","receivedAt":"2016-05-03T21:21:26Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2016-05-03 at 01:33 -0400, Jeff King wrote:\n> On Mon, May 02, 2016 at 02:57:43PM -0400, David Turner wrote:\n> \n> > On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n> > \n> > > +const char *known_capabilities[] = {\n> > > +\t\"multi_ack\",\n> > > +\t\"thin-pack\",\n> > > +\t\"side-band\",\n> > > +\t\"side-band-64k\",\n> > > +\t\"ofs-delta\",\n> > > +\t\"shallow\",\n> > > +\t\"no-progress\",\n> > > +\t\"include-tag\",\n> > > +\t\"multi_ack_detailed\",\n> > > +\t\"allow-tip-sha1-in-want\",\n> > > +\t\"allow-reachable-sha1-in-want\",\n> > > +\t\"no-done\",\n> > > +};\n> > \n> > I wonder if it is possible to not repeat the list from upload\n> > -pack.c?\n> > It seems unfortunate to have to add the same string in two places\n> > whenever you add a capability.\n> \n> I think that in general, we'd stop adding capabilities to v1. If you\n> have a client which speaks the new capability, then it should also be\n> speaking the new protocol. That's not strictly true if other non\n> -git.git\n> implementations want to learn capability X but not protocol v2, but I\n> think in practice it's not an unreasonable world view.\n> \n> I guess there may be a grey area for a while, though, where even\n> v2-capable clients don't end up speaking it, because they don't yet\n> know\n> that a particular server can handle it. So any capabilities added in\n> that grey area may want to go to both v1 and v2.\n\nOK, but then there should be one list per protocol version rather than\ntwo copies of the same list. \n"},{"id":"285475","messageId":"CAGZ79kbi+g2EnZosyYpAMfj8rFFV45vtgyeEJR3Fe8LG+zut7Q@mail.gmail.com","threadId":"42183","inReplyTo":"1462310486.4123.81.camel@twopensource.com","subject":"Re: [PATCH 06/14] remote.h: add get_remote_capabilities, request_capabilities","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-04T16:44:20Z","receivedAt":"2016-05-04T16:44:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 3, 2016 at 2:21 PM, David Turner wrote:\n> On Tue, 2016-05-03 at 01:33 -0400, Jeff King wrote:\n>> On Mon, May 02, 2016 at 02:57:43PM -0400, David Turner wrote:\n>>\n>> > On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n>> >\n>> > > +const char *known_capabilities[] = {\n>> > > + \"multi_ack\",\n>> > > + \"thin-pack\",\n>> > > + \"side-band\",\n>> > > + \"side-band-64k\",\n>> > > + \"ofs-delta\",\n>> > > + \"shallow\",\n>> > > + \"no-progress\",\n>> > > + \"include-tag\",\n>> > > + \"multi_ack_detailed\",\n>> > > + \"allow-tip-sha1-in-want\",\n>> > > + \"allow-reachable-sha1-in-want\",\n>> > > + \"no-done\",\n>> > > +};\n>> >\n>> > I wonder if it is possible to not repeat the list from upload\n>> > -pack.c?\n>> > It seems unfortunate to have to add the same string in two places\n>> > whenever you add a capability.\n>>\n>> I think that in general, we'd stop adding capabilities to v1. If you\n>> have a client which speaks the new capability, then it should also be\n>> speaking the new protocol. That's not strictly true if other non\n>> -git.git\n>> implementations want to learn capability X but not protocol v2, but I\n>> think in practice it's not an unreasonable world view.\n>>\n>> I guess there may be a grey area for a while, though, where even\n>> v2-capable clients don't end up speaking it, because they don't yet\n>> know\n>> that a particular server can handle it. So any capabilities added in\n>> that grey area may want to go to both v1 and v2.\n>\n> OK, but then there should be one list per protocol version rather than\n> two copies of the same list.\n>\n\nI thought this is by design as upload-pack is a different program, i.e. it\ncould be developed out of sync with the client, adding/removing\ncapabilities there but not in fetch-pack. That doesn't make sense though.\n\nWe could introduce known_capabilities_v1 and _v2 respectively in shared\nheader files, though.\n"},{"id":"285501","messageId":"xmqq37pxh9ll.fsf@gitster.mtv.corp.google.com","threadId":"42183","inReplyTo":"1461972887-22100-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 02/14] upload-pack.c: Refactor capability advertising","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-04T20:05:58Z","receivedAt":"2016-05-04T20:05:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Instead of having the capabilities in a local string, keep them\n> in a struct outside the function. This will allow us in a later patch\n> to easily reuse the capabilities in version 2 of the protocol.\n>\n> Signed-off-by: Stefan Beller \n> ---\n\nBetween a flat string and a list, you may be able to enumerate the\nelements in a list more easily, but my knee-jerk reaction is that\nthis does not go far enough, if you are to introduce a table. It\nfor example does not allow us to lose the conditional writing of\nsome capabilties based on allow_*_request and replace that with a\nmore table-driven approach.\n\nPerhaps that can come in a later step (in other words, the above is\nnot a basis for rejecting this change; just pointing it out that\nthis does not have to be the end of the story).\n\n> upload-pack.c | 59 ++++++++++++++++++++++++++++++++++++++++-------------------\n> 1 file changed, 40 insertions(+), 19 deletions(-)\n>\n> diff --git a/upload-pack.c b/upload-pack.c\n> index aaaf883..85381d5 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -719,37 +719,58 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)\n> \t\tstrbuf_addf(buf, \" symref=%s:%s\", item->string, (char *)item->util);\n> }\n> \n> +static int advertise_capabilities = 1;\n> +const char *all_capabilities[] = {\n> +\t\"multi_ack\",\n> +\t\"thin-pack\",\n> +\t\"side-band\",\n> +\t\"side-band-64k\",\n> +\t\"ofs-delta\",\n> +\t\"shallow\",\n> +\t\"no-progress\",\n> +\t\"include-tag\",\n> +\t\"multi_ack_detailed\",\n> +\t\"allow-tip-sha1-in-want\",\n> +\t\"allow-reachable-sha1-in-want\",\n> +\t\"no-done\",\n> +};\n> +\n> static int send_ref(const char *refname, const struct object_id *oid,\n> \t\t int flag, void *cb_data)\n> {\n> -\tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n> -\t\t\" side-band-64k ofs-delta shallow no-progress\"\n> -\t\t\" include-tag multi_ack_detailed\";\n> \tconst char *refname_nons = strip_namespace(refname);\n> \tstruct object_id peeled;\n> \n> \tif (mark_our_ref(refname_nons, refname, oid))\n> \t\treturn 0;\n> \n> -\tif (capabilities) {\n> -\t\tstruct strbuf symref_info = STRBUF_INIT;\n> -\n> -\t\tformat_symref_info(&symref_info, cb_data);\n> -\t\tpacket_write(1, \"%s %s%c%s%s%s%s%s agent=%s\\n\",\n> -\t\t\t oid_to_hex(oid), refname_nons,\n> -\t\t\t 0, capabilities,\n> -\t\t\t (allow_unadvertised_object_request & ALLOW_TIP_SHA1) ?\n> -\t\t\t\t \" allow-tip-sha1-in-want\" : \"\",\n> -\t\t\t (allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1) ?\n> -\t\t\t\t \" allow-reachable-sha1-in-want\" : \"\",\n> -\t\t\t stateless_rpc ? \" no-done\" : \"\",\n> -\t\t\t symref_info.buf,\n> -\t\t\t git_user_agent_sanitized());\n> -\t\tstrbuf_release(&symref_info);\n> +\tif (advertise_capabilities) {\n> +\t\tint i;\n> +\t\tstruct strbuf capabilities = STRBUF_INIT;\n> +\n> +\t\tfor (i = 0; i < ARRAY_SIZE(all_capabilities); i++) {\n> +\t\t\tconst char *cap = all_capabilities[i];\n> +\t\t\tif (!strcmp(cap, \"allow-tip-sha1-in-want\")\n> +\t\t\t && !(allow_unadvertised_object_request & ALLOW_TIP_SHA1))\n> +\t\t\t\tcontinue;\n> +\t\t\tif (!strcmp(cap, \"allow-reachable-sha1-in-want\")\n> +\t\t\t && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))\n> +\t\t\t\tcontinue;\n> +\t\t\tif (!strcmp(cap, \"no-done\") && !stateless_rpc)\n> +\t\t\t\tcontinue;\n> +\t\t\tstrbuf_addf(&capabilities, \" %s\", cap);\n> +\t\t}\n> +\n> +\t\tformat_symref_info(&capabilities, cb_data);\n> +\t\tstrbuf_addf(&capabilities, \" agent=%s\", git_user_agent_sanitized());\n> +\n> +\t\tpacket_write(1, \"%s %s%c%s\\n\", oid_to_hex(oid), refname_nons,\n> +\t\t\t 0, capabilities.buf);\n> +\t\tstrbuf_release(&capabilities);\n> +\t\tadvertise_capabilities = 0;\n> \t} else {\n> \t\tpacket_write(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n> \t}\n> -\tcapabilities = NULL;\n> \tif (!peel_ref(refname, peeled.hash))\n> \t\tpacket_write(1, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n> \treturn 0;\n"},{"id":"285503","messageId":"xmqqvb2tfusk.fsf@gitster.mtv.corp.google.com","threadId":"42183","inReplyTo":"1462210997.4123.49.camel@twopensource.com","subject":"Re: [PATCH 03/14] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-04T20:11:07Z","receivedAt":"2016-05-04T20:11:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:\n>> In upload-pack-2 we send each capability in its own packet buffer.\n>> The construction of upload-pack-2 is a bit unfortunate as I would\n>> like\n>> it to not be depending on a symlink linking to upload-pack.c, but I\n>> did\n>> not find another easy way to do it. I would like it to generate\n>> upload-pack-2.o from upload-pack.c but with '-DTRANSPORT_VERSION=2'\n>> set.\n>\n> Couldn't we check argv[0] and use that to determine protocol? Then we\n> could symlink executables rather than source code.\n\nYeah, I do not have a good suggestion on the mechanism to actually\nswitch between behaviours other than what was already been discussed\nin the thread, but the code resulting from the patch proposed is too\nugly with full of #ifdef for it to be the final form.\n\nJust to make sure nobody gets me wrong; it is OK as a POC to move\nthe discussion forward.\n\nA production quality implemention however would need to either be a\nsingle executable that switches behaviour at runtime, or two\nexecutables, each with its own *.c file with its own main(), that\nshare code in another common *.c file, I would think.\n"},{"id":"285504","messageId":"xmqqr3dhfuo9.fsf@gitster.mtv.corp.google.com","threadId":"42183","inReplyTo":"1461972887-22100-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 04/14] connect: rewrite feature parsing to work on string_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-04T20:13:42Z","receivedAt":"2016-05-04T20:13:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Later on when we introduce the version 2 transport protocol, the\n> capabilities will not be transported in one lone string but each\n\ns/lone/long/, I think.\n\n> capability will be carried in its own pkt line.\n>\n> To reuse existing infrastructure we would either need to join the\n> capabilities into a single string again later or refactor the current\n> capability parsing to be using a data structure which fits both\n> versions of the transport protocol. We chose to implement the later.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/receive-pack.c | 15 ++++++---\n> connect.c | 82 +++++++++++++++++++++++---------------------------\n> connect.h | 2 +-\n> upload-pack.c | 13 ++++++--\n> 4 files changed, 58 insertions(+), 54 deletions(-)\n\nI am not sure if the churn is make a right tradeoff here.\n\nA loop to concatenate each segment into a strbuf before passing it\nto parse_feature_request would be at most 5 lines long, no?\n"},{"id":"286841","messageId":"1463523829.24478.78.camel@twopensource.com","threadId":"42183","inReplyTo":"xmqqr3dhfuo9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 04/14] connect: rewrite feature parsing to work on string_list","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-17T22:23:49Z","receivedAt":"2016-05-17T22:23:49Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-05-04 at 13:13 -0700, Junio C Hamano wrote:\n> Stefan Beller writes:\n> \n> > Later on when we introduce the version 2 transport protocol, the\n> > capabilities will not be transported in one lone string but each\n> \n> s/lone/long/, I think.\n> \n> > capability will be carried in its own pkt line.\n> > \n> > To reuse existing infrastructure we would either need to join the\n> > capabilities into a single string again later or refactor the\n> > current\n> > capability parsing to be using a data structure which fits both\n> > versions of the transport protocol. We chose to implement the\n> > later.\n> > \n> > Signed-off-by: Stefan Beller \n> > ---\n> > builtin/receive-pack.c | 15 ++++++---\n> > connect.c | 82 +++++++++++++++++++++++---------------\n> > ------------\n> > connect.h | 2 +-\n> > upload-pack.c | 13 ++++++--\n> > 4 files changed, 58 insertions(+), 54 deletions(-)\n> \n> I am not sure if the churn is make a right tradeoff here.\n> \n> A loop to concatenate each segment into a strbuf before passing it\n> to parse_feature_request would be at most 5 lines long, no?\n\nI started looking at this again briefly today, and I actually think\nthat string manipulation of this sort is a mistake. String\nmanipulation is error-prone, and having an interface defined in terms\nof strings makes it hard for a reader to see what sorts of input are\nvalid and invalid. (It's also often somewhat less efficient). So I\nthink it is a good idea to improve the interface while we're already\nchanging code in this area.\n"},{"id":"287462","messageId":"1464130008.24478.134.camel@twopensource.com","threadId":"42183","inReplyTo":"1461972887-22100-1-git-send-email-sbeller@google.com","subject":"Re: [WIP PATCH 00/14] Protocol v2 patches","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-24T22:46:48Z","receivedAt":"2016-05-24T22:46:48Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"I was looking at this again today, and noticed that it doesn't really\naddress the HTTP case.\n\nThe central problem is that protocol v2 goes like this:\nserver: I have capabilities w,x,y, and z\nclient: I want capabilities x and z.\n\nBut HTTP goes like this:\nclient: [request]\nserver: [response]\n\nI tried to make libcurl do the receive-before-sending thing, but it\ndoesn't seem to be designed for it (even if you prime things by sending\na \"hello\" from the client first). My thought was to hook up\nCURLOPT_READFUNCTION and CURLOPT_WRITEFUNCTION, and have the read\nfunction return CURL_READFUNC_PAUSE and then have the write (=client\nreceiving data ) function unpause the reader (= client sending data)\nonce it gets the capabilities. But apparently pausing only works with\nchunked encoding, which seems to cause Apache's mod_cgi to fail.\n\nMaybe I'm missing something. Has anyone else ever made something like\nthis work?\n\nOf course, I could always use CURLOPT_CONNECT_ONLY to write my own HTTP\nclient, but that seems pretty unreasonable.\n\nI also looked to see if libcurl had websockets support, since that's\none kind of bidirectional conversation over HTTP, but it doesn't seem\nto.\n\nAnother choice is to make a separate /capabilities endpoint that gets\nhit before /info/refs. This is a bit bad because:\n(a) it's another HTTP request\n(b) it adds implicit state to the HTTP conversation. If multiple git\nservers were behind a load balancer, you might end up getting server A\nfor /capabilities and server B for /info/refs, and those servers might\nhave different capabilities. This is not impossible when testing a git\nserver upgrade on one machine before rolling it out to a whole fleet. \n Maybe the rule for clients re capabilities is that they can request\nwhatever capabilities they want, but the server is free to ignore that\nrequest and send whatever data it feels like. That's not great, but it\nshould work (I think).\n\nDoes anyone else have any thoughts on how this ought to work?\n"},{"id":"287464","messageId":"CACsJy8DkMDF85NMS8wS3CsNdt-vDpu6WajY1snxWV0GyeUJ7Vw@mail.gmail.com","threadId":"42183","inReplyTo":"1464130008.24478.134.camel@twopensource.com","subject":"Re: [WIP PATCH 00/14] Protocol v2 patches","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-24T23:03:29Z","receivedAt":"2016-05-24T23:03:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 25, 2016 at 5:46 AM, David Turner wrote:\n> I was looking at this again today, and noticed that it doesn't really\n> address the HTTP case.\n>\n> The central problem is that protocol v2 goes like this:\n> server: I have capabilities w,x,y, and z\n> client: I want capabilities x and z.\n>\n> But HTTP goes like this:\n> client: [request]\n> server: [response]\n>\n> I tried to make libcurl do the receive-before-sending thing, but it\n> doesn't seem to be designed for it (even if you prime things by sending\n> a \"hello\" from the client first). My thought was to hook up\n> CURLOPT_READFUNCTION and CURLOPT_WRITEFUNCTION, and have the read\n> function return CURL_READFUNC_PAUSE and then have the write (=client\n> receiving data ) function unpause the reader (= client sending data)\n> once it gets the capabilities. But apparently pausing only works with\n> chunked encoding, which seems to cause Apache's mod_cgi to fail.\n>\n> Maybe I'm missing something. Has anyone else ever made something like\n> this work?\n\nIt simply takes one more round-trip to negotiate. Not the best thing, but...\n\n> I also looked to see if libcurl had websockets support, since that's\n> one kind of bidirectional conversation over HTTP, but it doesn't seem\n> to.\n\nYeah. And libcurl probably will not support websockets even in long\nrun. I've been searching for a websocket implementation for git and\nfinally settled for netcat-like programs, sitting in front of git and\ndealing with network just like ssh. It will be the simplest way to add\neither websocket or http/2 support. If either protocol gets popular\nenough, smart-http can become a fall-back mechanism where performance\ndoes not matter much.\n-- \nDuy\n"},{"id":"287499","messageId":"xmqqfut6150a.fsf@gitster.mtv.corp.google.com","threadId":"42183","inReplyTo":"1464130008.24478.134.camel@twopensource.com","subject":"Re: [WIP PATCH 00/14] Protocol v2 patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-25T16:23:01Z","receivedAt":"2016-05-25T16:23:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> I was looking at this again today, and noticed that it doesn't really\n> address the HTTP case.\n>\n> The central problem is that protocol v2 goes like this:\n> server: I have capabilities w,x,y, and z\n> client: I want capabilities x and z.\n>\n> But HTTP goes like this:\n> client: [request]\n> server: [response]\n\nI wonder if that can be solved by speculative request?\n\nLet the connection initiator say \"If you can do x and z, please do\nso\", and allow the responder to say either \"OK, I can do x and z; by\nthe way the full capabilites I support are w, x, y and z\", or\n\"Sorry, can't do that; I have capabilities w, x, and y\".\n"},{"id":"287500","messageId":"1464194746.24478.135.camel@twopensource.com","threadId":"42183","inReplyTo":"CACsJy8DkMDF85NMS8wS3CsNdt-vDpu6WajY1snxWV0GyeUJ7Vw@mail.gmail.com","subject":"Re: [WIP PATCH 00/14] Protocol v2 patches","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-25T16:45:46Z","receivedAt":"2016-05-25T16:45:46Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-05-25 at 06:03 +0700, Duy Nguyen wrote:\n> On Wed, May 25, 2016 at 5:46 AM, David Turner <\n> dturner@twopensource.com> wrote:\n> > I was looking at this again today, and noticed that it doesn't\n> > really\n> > address the HTTP case.\n> > \n> > The central problem is that protocol v2 goes like this:\n> > server: I have capabilities w,x,y, and z\n> > client: I want capabilities x and z.\n> > \n> > But HTTP goes like this:\n> > client: [request]\n> > server: [response]\n> > \n> > I tried to make libcurl do the receive-before-sending thing, but it\n> > doesn't seem to be designed for it (even if you prime things by\n> > sending\n> > a \"hello\" from the client first). My thought was to hook up\n> > CURLOPT_READFUNCTION and CURLOPT_WRITEFUNCTION, and have the read\n> > function return CURL_READFUNC_PAUSE and then have the write\n> > (=client\n> > receiving data ) function unpause the reader (= client sending\n> > data)\n> > once it gets the capabilities. But apparently pausing only works\n> > with\n> > chunked encoding, which seems to cause Apache's mod_cgi to fail.\n> > \n> > Maybe I'm missing something. Has anyone else ever made something\n> > like\n> > this work?\n> \n> It simply takes one more round-trip to negotiate. Not the best thing,\n> but...\n\nDo you mean that it can be done with libcurl? Or do you mean that I\nshould go with the /capabilities endpoint?\n"},{"id":"287507","messageId":"1464204699.24478.144.camel@twopensource.com","threadId":"42183","inReplyTo":"xmqqfut6150a.fsf@gitster.mtv.corp.google.com","subject":"Re: [WIP PATCH 00/14] Protocol v2 patches","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-25T19:31:39Z","receivedAt":"2016-05-25T19:31:39Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-05-25 at 09:23 -0700, Junio C Hamano wrote:\n> David Turner writes:\n> \n> > I was looking at this again today, and noticed that it doesn't\n> > really\n> > address the HTTP case.\n> > \n> > The central problem is that protocol v2 goes like this:\n> > server: I have capabilities w,x,y, and z\n> > client: I want capabilities x and z.\n> > \n> > But HTTP goes like this:\n> > client: [request]\n> > server: [response]\n> \n> I wonder if that can be solved by speculative request?\n> \n> Let the connection initiator say \"If you can do x and z, please do\n> so\", and allow the responder to say either \"OK, I can do x and z; by\n> the way the full capabilites I support are w, x, y and z\", or\n> \"Sorry, can't do that; I have capabilities w, x, and y\".\n\nThat protocol is somewhat more complicated. And it's different than\nthe v2 protocol for the other transports (unless you are thinking that\nwe should change those too?). It's actually a tiny bit like what I\noriginally proposed for HTTP. \n\nIt sounds OK to me, but I want to know what others think before I start\nimplementing.\n"},{"id":"287510","messageId":"20160525212909.GA13776@sigill.intra.peff.net","threadId":"42183","inReplyTo":"1464130008.24478.134.camel@twopensource.com","subject":"Re: [WIP PATCH 00/14] Protocol v2 patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-25T21:29:09Z","receivedAt":"2016-05-25T21:29:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 24, 2016 at 06:46:48PM -0400, David Turner wrote:\n\n> I tried to make libcurl do the receive-before-sending thing, but it\n> doesn't seem to be designed for it (even if you prime things by sending\n> a \"hello\" from the client first). My thought was to hook up\n> CURLOPT_READFUNCTION and CURLOPT_WRITEFUNCTION, and have the read\n> function return CURL_READFUNC_PAUSE and then have the write (=client\n> receiving data ) function unpause the reader (= client sending data)\n> once it gets the capabilities. But apparently pausing only works with\n> chunked encoding, which seems to cause Apache's mod_cgi to fail.\n> \n> Maybe I'm missing something. Has anyone else ever made something like\n> this work?\n\nI don't think it can work in the general case. HTTP is not full-duplex,\nand you have to send off the request and wait for the response. Even if\nyou could convince the client and git-http-backend to do it, you're\ngoing to get foiled by proxies, web server implementations, and other\nmiddle-men.\n\n> Of course, I could always use CURLOPT_CONNECT_ONLY to write my own HTTP\n> client, but that seems pretty unreasonable.\n> \n> I also looked to see if libcurl had websockets support, since that's\n> one kind of bidirectional conversation over HTTP, but it doesn't seem\n> to.\n\nI would love to see us move to a true bidirectional HTTP-based protocol.\nIt would clear up all of the drawbacks that the current HTTP protocol\nhas, and I think we could generally recommend it entirely over using\ngit://. But like you, I haven't figured out an easy way to do it.\n\nI hoped that maybe HTTP/2 would solve some of that if we waited long\nenough for it to be adopted, but it doesn't look like there's anything\nout of the box. It seems like the recommended solutions still involve\nwebsockets. I might be wrong, though; this is very much outside my area\nof expertise.\n\n> Another choice is to make a separate /capabilities endpoint that gets\n> hit before /info/refs. This is a bit bad because:\n> (a) it's another HTTP request\n\nRight, this is the extra round-trip I mentioned in:\n\n http://thread.gmane.org/gmane.comp.version-control.git/291640/focus=291951\n\nI think you could get rid of it by making protocol v2 a true \"client\nspeaks first\" protocol, which aligns better with how HTTP works (but if\nwe do that, it would be nice to do it for _all_ of the transports, so\nthey stay closer to each other). But...\n\n> (b) it adds implicit state to the HTTP conversation. If multiple git\n> servers were behind a load balancer, you might end up getting server A\n> for /capabilities and server B for /info/refs, and those servers might\n> have different capabilities. This is not impossible when testing a git\n> server upgrade on one machine before rolling it out to a whole fleet. \n> Maybe the rule for clients re capabilities is that they can request\n> whatever capabilities they want, but the server is free to ignore that\n> request and send whatever data it feels like. That's not great, but it\n> should work (I think).\n\nI think this is already the case today. Every non-trivial git-over-http\nrequest requires at least two HTTP requests: one to receive the server\nfetch advertisement, and the second to actually do the work (and in the\nfetch case, the have/want negotiation in the second one may actually\nspan several requests).\n\nThe capabilities from the server come in the first request, and then the\nclient sends back its capabilities in the second one. So if you are\nhitting multiple incompatible servers, the server may not understand\nyour request. Likewise, if an upload-pack request takes multiple hits,\nwe send up the client capabilities in each request.\n\nI don't think quietly ignoring unknown capabilities is a good idea. The\nresults would range from confusing breakages (e.g., ignored multi-ack or\nno-done capabilities) to subtly wrong behavior (e.g., a server which\nignores \"atomic\" and proceeds with a half-failed push anyway). Given\nthe rarity of the situation, it's probably better for the server to barf\nwith an appropriate error message. That sucks for the user, but it's\nprobably better than the alternatives.\n\n-Peff\n"}]}