{"thread":{"id":"39434","subject":"[RFC/WIP PATCH 01/11] upload-pack: make client capability parsing code a separate function","startedAt":"2015-05-26T22:01:04Z","lastAt":"2015-06-04T17:01:12Z","messageCount":61,"participants":["Stefan Beller","Junio C Hamano","Eric Sunshine","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"262172","messageId":"1432677675-5118-1-git-send-email-sbeller@google.com","threadId":"39434","inReplyTo":null,"subject":"[RFC/WIP PATCH 00/11] Protocol version 2, again!","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:01:04Z","receivedAt":"2015-05-26T22:01:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":" \"Just give us something to play around with\" - Peff at GitMerge 2015\n\nThis is another approach for updating the pack protocol of Git.\nWhile in the previous attempts I tried to come up with the perfect\nspecification of the new protocol I realized that such an approach\ndoesn't lead anywhere. So this time actual working code is included!\n\nThe included code is not complete, but a minimal example of\n\n git config remote.origin.transportversion 2\n git fetch origin master # uses the new protocol!\n\nworks.\n\nIn a previous discussion[1] towards version 2 of the pack protocol I wanted\nto come up with a protocol which even includes negotiating what is done\nin the protocol exchange, such as having a command \"push, fetch, ls-remote\"\nbeing part of the protocol. This is not a very good approach as it's too much\nwork to be done at the same time.\n\nThis patch series focusses on just the fetching side, so the first four patches\nare teaching git-upload-pack about the new pack protocol. The next five patches\nwill teach git fetch and fetch-pack how to use the version 2 protocol. Then there\nwill be a small test and a documentation update.\n\nThe new protocol works just like the old protocol, except for\nthe capabilities negotiation being before any exchange of real data.\n\nThis solves the problem of having a first huge chunk of data (the refs\nadvertisement) sent over the wire and just realizing in between that we\ndon't need these data for that operation and no way to cancel.\n\nBy having a capabilites negotiation first the new protocol may be even\nmore future-proof than the current one, as the capabilites will hopefully\nbe kept small and all larger data transfers will get their own later stage\nin the protocol.\n\nTo determine the protocol version we check the ending of the\ninvoked program for an appended version number to see which protocol\nversion we're using in an exchange. At first I thought we should append\na unique suffix instead of a version number to make a distinction easier\nfor the kind of protocol we want to talk (There may be the traditional\nprotocol with no suffix, or the \"upload-pack-capabilities-first\" protocol\nwhich will transmit the capabilities first).\n\nMy preference for a string suffix instead of a sequential number is\nmotiviated by the discussion of sha1 vs sequential numbers to describe\na state of a repository. The main difference here is however how often\nwe expect changes. Version 1 of the protocol is current for 10 years by\nnow, so I do not changes to the protocol number often, which makes it\nsuitable for just having a counter appended to the binary.\n\nThe advantage of just a counting number is its small size,\nand I think this advantage outweights the disadvantage\nof having named protocol versions.\n\nThis series doesn't include an automatic upgrade of the protocol version\nfor later changes if the server supports it, so for now the use of the new\nprotocol needs to be activated manually via setting remote.origin.transportversion.\n\nAny comments welcome!\n\nThanks,\nStefan\n\n[1] http://permalink.gmane.org/gmane.comp.version-control.git/267572\n\nNguyễn Thái Ngọc Duy (2):\n upload-pack: make client capability parsing code a separate function\n upload-pack: only accept capabilities on the first \"want\" line\n\nStefan Beller (9):\n upload-pack: move capabilities out of send_ref\n upload-pack-2: Implement the version 2 of upload-pack\n transport: add infrastructure to support a protocol version number\n remote.h: add get_remote_capabilities, request_capabilities\n fetch-pack: use the configured transport protocol\n transport: connect_setup appends protocol version number\n transport: get_refs_via_connect exchanges capabilities before refs.\n t5544: add a test case for the new protocol\n Document protocol version 2\n\n .gitignore | 1 +\n Documentation/technical/pack-protocol.txt | 86 ++++++++++++--\n Documentation/technical/protocol-capabilities.txt | 15 ---\n Makefile | 2 +\n builtin/fetch-pack.c | 17 ++-\n builtin/fetch.c | 6 +\n connect.c | 48 +++++++-\n fetch-pack.h | 1 +\n remote.c | 2 +\n remote.h | 5 +\n t/t5544-fetch-2.sh | 40 +++++++\n transport-helper.c | 1 +\n transport.c | 60 +++++++++-\n transport.h | 4 +\n upload-pack-2.c | 1 +\n upload-pack.c | 138 +++++++++++++++++-----\n 16 files changed, 366 insertions(+), 61 deletions(-)\n create mode 100755 t/t5544-fetch-2.sh\n create mode 120000 upload-pack-2.c\n\n-- \n2.4.1.345.gab207b6.dirty\n"},{"id":"262170","messageId":"1432677675-5118-2-git-send-email-sbeller@google.com","threadId":"39434","inReplyTo":"1432677675-5118-1-git-send-email-sbeller@google.com","subject":"[RFC/WIP PATCH 01/11] upload-pack: make client capability parsing code a separate function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:01:05Z","receivedAt":"2015-05-26T22:01:05Z","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 745fda8..5449ff7 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -531,6 +531,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@@ -540,7 +562,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@@ -575,26 +596,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.4.1.345.gab207b6.dirty\n"},{"id":"262171","messageId":"1432677675-5118-3-git-send-email-sbeller@google.com","threadId":"39434","inReplyTo":"1432677675-5118-1-git-send-email-sbeller@google.com","subject":"[RFC/WIP PATCH 02/11] upload-pack: only accept capabilities on the first \"want\" line","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:01:06Z","receivedAt":"2015-05-26T22:01:06Z","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\npack-protocol.txt says so and fetch-pack also follows it even though\nupload-pack is a bit lax. Fix it.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Stefan Beller \n---\n upload-pack.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 5449ff7..6734777 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -558,6 +558,7 @@ static void receive_needs(void)\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n \tint depth = 0;\n \tint has_non_tip = 0;\n+\tint first_want = 1;\n \n \tshallow_nr = 0;\n \tfor (;;) {\n@@ -596,7 +597,11 @@ 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\tif (first_want) {\n+\t\t\tparse_features(line + 45);\n+\t\t\tfirst_want = 0;\n+\t\t} else if (line[45])\n+\t\t\tdie(\"garbage at the end of 'want' line %s\", line + 45);\n \n \t\to = parse_object(sha1_buf);\n \t\tif (!o)\n-- \n2.4.1.345.gab207b6.dirty\n"},{"id":"262181","messageId":"1432677675-5118-4-git-send-email-sbeller@google.com","threadId":"39434","inReplyTo":"1432677675-5118-1-git-send-email-sbeller@google.com","subject":"[RFC/WIP PATCH 03/11] upload-pack: move capabilities out of send_ref","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:01:07Z","receivedAt":"2015-05-26T22:01:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This will make it easier to reuse the capabilities in a later patch.\n\nSigned-off-by: Stefan Beller \n---\n upload-pack.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 6734777..a5f75b7 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -716,33 +716,35 @@ 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 send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n-{\n-\tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n+static const char *advertise_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+\n+static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n+{\n+\n \tconst char *refname_nons = strip_namespace(refname);\n \tunsigned char peeled[20];\n \n \tif (mark_our_ref(refname, sha1))\n \t\treturn 0;\n \n-\tif (capabilities) {\n+\tif (advertise_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 agent=%s\\n\",\n \t\t\t sha1_to_hex(sha1), refname_nons,\n-\t\t\t 0, capabilities,\n+\t\t\t 0, advertise_capabilities,\n \t\t\t allow_tip_sha1_in_want ? \" allow-tip-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+\t\tadvertise_capabilities = NULL;\n \t} else {\n \t\tpacket_write(1, \"%s %s\\n\", sha1_to_hex(sha1), refname_nons);\n \t}\n-\tcapabilities = NULL;\n \tif (!peel_ref(refname, peeled))\n \t\tpacket_write(1, \"%s %s^{}\\n\", sha1_to_hex(peeled), refname_nons);\n \treturn 0;\n-- \n2.4.1.345.gab207b6.dirty\n"},{"id":"262173","messageId":"1432677675-5118-5-git-send-email-sbeller@google.com","threadId":"39434","inReplyTo":"1432677675-5118-1-git-send-email-sbeller@google.com","subject":"[RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:01:08Z","receivedAt":"2015-05-26T22:01:08Z","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.\nBy reusing the advertise_capabilities and eventually setting it to\nNULL we will be able to reuse the methods for refs advertisement.\n\nSigned-off-by: Stefan Beller \n---\n .gitignore | 1 +\n Makefile | 2 ++\n upload-pack-2.c | 1 +\n upload-pack.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---\n 4 files changed, 78 insertions(+), 3 deletions(-)\n create mode 120000 upload-pack-2.c\n\ndiff --git a/.gitignore b/.gitignore\nindex a052419..a3c8ab9 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -165,6 +165,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 25a453b..0f3ee41 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -560,6 +560,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@@ -625,6 +626,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\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 a5f75b7..84f9ee3 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -716,10 +716,47 @@ 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 const char *advertise_capabilities = \"multi_ack thin-pack side-band\"\n+static char *advertise_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 \n+/*\n+ * Reads the next capability and puts it into dst as a null terminated string.\n+ * Returns true if more capabilities can be read.\n+ * */\n+static int next_capability(char *dst)\n+{\n+\tint len = 0;\n+\tif (!*advertise_capabilities) {\n+\t\t/* make sure to not advertise capabilities afterwards */\n+\t\tadvertise_capabilities = NULL;\n+\t\treturn 0;\n+\t}\n+\n+\twhile (advertise_capabilities[len] != '\\0' &&\n+\t advertise_capabilities[len] != ' ')\n+\t\tlen ++;\n+\tstrncpy(dst, advertise_capabilities, len);\n+\tdst[len] = '\\0';\n+\n+\tadvertise_capabilities += len;\n+\tif (*advertise_capabilities == ' ')\n+\t\tadvertise_capabilities++;\n+\n+\treturn 1;\n+}\n+\n+static void send_capabilities(void)\n+{\n+\tchar buf[100];\n+\n+\twhile (next_capability(buf))\n+\t\tpacket_write(1, \"capability:%s\\n\", buf);\n+\n+\tpacket_write(1, \"agent:%s\\n\", git_user_agent_sanitized());\n+\tpacket_flush(1);\n+}\n+\n static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n {\n \n@@ -794,6 +831,28 @@ static void upload_pack(void)\n \t}\n }\n \n+static void receive_capabilities(void)\n+{\n+\tint done = 0;\n+\twhile (1) {\n+\t\tchar *line = packet_read_line(0, NULL);\n+\t\tif (!line)\n+\t\t\tbreak;\n+\t\tif (starts_with(line, \"capability:\"))\n+\t\t\tparse_features(line + strlen(\"capability:\"));\n+\t}\n+}\n+\n+static void upload_pack_version_2(void)\n+{\n+\tsend_capabilities();\n+\treceive_capabilities();\n+\n+\t/* The rest of the protocol stays the same, capabilities advertising\n+\t is disabled though. */\n+\tupload_pack();\n+}\n+\n static int upload_pack_config(const char *var, const char *value, void *unused)\n {\n \tif (!strcmp(\"uploadpack.allowtipsha1inwant\", var))\n@@ -806,16 +865,24 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n \n+static int endswith(const char *teststring, const char *ending)\n+{\n+\tint slen = strlen(teststring);\n+\tint elen = strlen(ending);\n+\treturn !strcmp(teststring + slen - elen, ending);\n+}\n+\n int main(int argc, char **argv)\n {\n \tchar *dir;\n+\tconst char *cmd;\n \tint i;\n \tint strict = 0;\n \n \tgit_setup_gettext();\n \n \tpacket_trace_identity(\"upload-pack\");\n-\tgit_extract_argv0_path(argv[0]);\n+\tcmd = git_extract_argv0_path(argv[0]);\n \tcheck_replace_refs = 0;\n \n \tfor (i = 1; i < argc; i++) {\n@@ -857,6 +924,10 @@ 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-\tupload_pack();\n+\n+\tif (endswith(cmd, \"-2\"))\n+\t\tupload_pack_version_2();\n+\telse\n+\t\tupload_pack();\n \treturn 0;\n }\n-- \n2.4.1.345.gab207b6.dirty\n"},{"id":"262174","messageId":"1432677675-5118-6-git-send-email-sbeller@google.com","threadId":"39434","inReplyTo":"1432677675-5118-1-git-send-email-sbeller@google.com","subject":"[RFC/WIP PATCH 05/11] transport: add infrastructure to support a protocol version number","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:01:09Z","receivedAt":"2015-05-26T22:01:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The transport version set via command line argument in\ngit fetch takes precedence over the configured version.\n\nSigned-off-by: Stefan Beller \n---\n builtin/fetch.c | 6 ++++++\n remote.c | 2 ++\n remote.h | 2 ++\n transport-helper.c | 1 +\n transport.c | 13 +++++++++++++\n transport.h | 4 ++++\n 6 files changed, 28 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7910419..d2e1828 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -46,6 +46,7 @@ static const char *recurse_submodules_default;\n static int shown_url = 0;\n static int refmap_alloc, refmap_nr;\n static const char **refmap_array;\n+static const char *transport_version;\n \n static int option_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n@@ -121,6 +122,9 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"default mode for recursion\"), PARSE_OPT_HIDDEN },\n \tOPT_BOOL(0, \"update-shallow\", &update_shallow,\n \t\t N_(\"accept refs that update .git/shallow\")),\n+\tOPT_STRING('y', \"transport-version\", &transport_version,\n+\t\t N_(\"transport-version\"),\n+\t\t N_(\"specify transport version to be used\")),\n \t{ OPTION_CALLBACK, 0, \"refmap\", NULL, N_(\"refmap\"),\n \t N_(\"specify fetch refmap\"), PARSE_OPT_NONEG, parse_refmap_arg },\n \tOPT_END()\n@@ -848,6 +852,8 @@ static struct transport *prepare_transport(struct remote *remote)\n \tstruct transport *transport;\n \ttransport = transport_get(remote, NULL);\n \ttransport_set_verbosity(transport, verbosity, progress);\n+\tif (transport_version)\n+\t\tset_option(transport, TRANS_OPT_TRANSPORTVERSION, transport_version);\n \tif (upload_pack)\n \t\tset_option(transport, TRANS_OPT_UPLOADPACK, upload_pack);\n \tif (keep)\ndiff --git a/remote.c b/remote.c\nindex 68901b0..2914d9d 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -476,6 +476,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\t\t\t key, value);\n \t} else if (!strcmp(subkey, \".vcs\")) {\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n+\t} else if (!strcmp(subkey, \".transportversion\")) {\n+\t\tremote->transport_version = git_config_int(key, value);\n \t}\n \treturn 0;\n }\ndiff --git a/remote.h b/remote.h\nindex 02d66ce..04e2310 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -50,6 +50,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 5d99a6b..ab3cd5b 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -247,6 +247,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 f080e93..3ef15f6 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -479,6 +479,16 @@ static int set_git_option(struct git_transport_options *opts,\n \t} else if (!strcmp(name, TRANS_OPT_PUSH_CERT)) {\n \t\topts->push_cert = !!value;\n \t\treturn 0;\n+\t} else if (!strcmp(name, TRANS_OPT_TRANSPORTVERSION)) {\n+\t\tif (!value)\n+\t\t\topts->transport_version = 0;\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@@ -988,6 +998,9 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tret->smart_options->receivepack = \"git-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 18d2cf8..e07d356 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -14,6 +14,7 @@ struct git_transport_options {\n \tunsigned update_shallow : 1;\n \tunsigned push_cert : 1;\n \tint depth;\n+\tint transport_version;\n \tconst char *uploadpack;\n \tconst char *receivepack;\n \tstruct push_cas_option *cas;\n@@ -162,6 +163,9 @@ struct transport *transport_get(struct remote *, const char *);\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 /**\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.4.1.345.gab207b6.dirty\n"},{"id":"262180","messageId":"1432677675-5118-7-git-send-email-sbeller@google.com","threadId":"39434","inReplyTo":"1432677675-5118-1-git-send-email-sbeller@google.com","subject":"[RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:01:10Z","receivedAt":"2015-05-26T22:01:10Z","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 | 48 +++++++++++++++++++++++++++++++++++++++++++++++-\n remote.h | 3 +++\n 2 files changed, 50 insertions(+), 1 deletion(-)\n\ndiff --git a/connect.c b/connect.c\nindex c0144d8..bb17618 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -105,8 +105,54 @@ static void annotate_refs_with_symref_info(struct ref *ref)\n \tstring_list_clear(&symref, 0);\n }\n \n+void get_remote_capabilities(int in, char *src_buf, size_t src_len)\n+{\n+\tstruct strbuf capabilities_string = STRBUF_INIT;\n+\tfor (;;) {\n+\t\tint len;\n+\t\tchar *line = packet_buffer;\n+\t\tconst char *arg;\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\tif (len > 4 && skip_prefix(line, \"ERR \", &arg))\n+\t\t\tdie(\"remote error: %s\", arg);\n+\n+\t\tif (starts_with(line, \"capability:\")) {\n+\t\t\tstrbuf_addstr(&capabilities_string, line + strlen(\"capability:\"));\n+\t\t\tstrbuf_addch(&capabilities_string, ' ');\n+\t\t}\n+\t}\n+\tfree(server_capabilities);\n+\tserver_capabilities = xmalloc(capabilities_string.len + 1);\n+\tserver_capabilities = strbuf_detach(&capabilities_string, NULL);\n+\n+\tstrbuf_release(&capabilities_string);\n+}\n+\n+int request_capabilities(int out)\n+{\n+\tfprintf(stderr, \"request_capabilities\\n\");\n+\t// todo: send our capabilities\n+\tpacket_write(out, \"capability:foo\");\n+\tpacket_flush(out);\n+}\n+\n /*\n- * Read all the refs from the other end\n+ * Read all the refs from the other end,\n+ * in is a file descriptor input stream\n+ * src_buf and src_len may be an alternative way to specify the input.\n+ * list is the output of refs\n+ * extra_have is a list to store information learned about sha1 the other side has.\n+ * shallow_points\n */\n struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\t struct ref **list, unsigned int flags,\ndiff --git a/remote.h b/remote.h\nindex 04e2310..7381ddf 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -146,6 +146,9 @@ int check_ref_type(const struct ref *ref, int flags);\n void free_refs(struct ref *ref);\n \n struct sha1_array;\n+\n+extern void get_remote_capabilities(int in, char *src_buf, size_t src_len);\n+extern int request_capabilities(int out);\n extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\t\t struct ref **list, unsigned int flags,\n \t\t\t\t struct sha1_array *extra_have,\n-- \n2.4.1.345.gab207b6.dirty\n"},{"id":"262175","messageId":"1432677675-5118-8-git-send-email-sbeller@google.com","threadId":"39434","inReplyTo":"1432677675-5118-1-git-send-email-sbeller@google.com","subject":"[RFC/WIP PATCH 07/11] fetch-pack: use the configured transport protocol","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:01:11Z","receivedAt":"2015-05-26T22:01:11Z","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 | 17 ++++++++++++++++-\n fetch-pack.h | 1 +\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 4a6b340..32dc8b0 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -127,6 +127,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 (!strcmp(\"--transport-version\", arg)) {\n+\t\t\targs.version = strtol(arg + strlen(\"--transport-version\"), NULL, 0);\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(fetch_pack_usage);\n \t}\n \n@@ -175,7 +179,18 @@ 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.version) {\n+\tdefault:\n+\tcase 2:\n+\t\tget_remote_capabilities(fd[0], NULL, 0);\n+\t\trequest_capabilities(fd[1]);\n+\t\tbreak;\n+\tcase 1: /* fall through */\n+\tcase 0:\n+\t\tget_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);\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);\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex bb7fd76..b48b4f5 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -10,6 +10,7 @@ struct fetch_pack_args {\n \tconst char *uploadpack;\n \tint unpacklimit;\n \tint depth;\n+\tint version;\n \tunsigned quiet:1;\n \tunsigned keep_pack:1;\n \tunsigned lock_pack:1;\n-- \n2.4.1.345.gab207b6.dirty\n"},{"id":"262176","messageId":"1432677675-5118-9-git-send-email-sbeller@google.com","threadId":"39434","inReplyTo":"1432677675-5118-1-git-send-email-sbeller@google.com","subject":"[RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:01:12Z","receivedAt":"2015-05-26T22:01:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n transport.c | 18 ++++++++++++++++--\n 1 file changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 3ef15f6..33644a6 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -496,15 +496,29 @@ static int set_git_option(struct git_transport_options *opts,\n static int connect_setup(struct transport *transport, int for_push, int verbose)\n {\n \tstruct git_transport_data *data = transport->data;\n+\tconst char *remote_program;\n+\tchar *buf = 0;\n \n \tif (data->conn)\n \t\treturn 0;\n \n+\tremote_program = (for_push ? data->options.receivepack\n+\t\t\t\t : data->options.uploadpack);\n+\n+\tif (transport->smart_options\n+\t && transport->smart_options->transport_version) {\n+\t\tbuf = xmalloc(strlen(remote_program) + 12);\n+\t\tsprintf(buf, \"%s-%d\", remote_program,\n+\t\t\ttransport->smart_options->transport_version);\n+\t\tremote_program = buf;\n+\t}\n+\n \tdata->conn = git_connect(data->fd, transport->url,\n-\t\t\t\t for_push ? data->options.receivepack :\n-\t\t\t\t data->options.uploadpack,\n+\t\t\t\t remote_program,\n \t\t\t\t verbose ? CONNECT_VERBOSE : 0);\n \n+\tfree(buf);\n+\n \treturn 0;\n }\n \n-- \n2.4.1.345.gab207b6.dirty\n"},{"id":"262177","messageId":"1432677675-5118-10-git-send-email-sbeller@google.com","threadId":"39434","inReplyTo":"1432677675-5118-1-git-send-email-sbeller@google.com","subject":"[RFC/WIP PATCH 09/11] transport: get_refs_via_connect exchanges capabilities before refs.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:01:13Z","receivedAt":"2015-05-26T22:01:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n transport.c | 29 +++++++++++++++++++++++++----\n 1 file changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 33644a6..1cd9b77 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -526,12 +526,33 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus\n {\n \tstruct git_transport_data *data = transport->data;\n \tstruct ref *refs;\n+\tint version = 0;\n \n+\tif (transport->smart_options)\n+\t\tversion = transport->smart_options->transport_version;\n \tconnect_setup(transport, for_push, 0);\n-\tget_remote_heads(data->fd[0], NULL, 0, &refs,\n-\t\t\t for_push ? REF_NORMAL : 0,\n-\t\t\t &data->extra_have,\n-\t\t\t &data->shallow);\n+\tswitch (version) {\n+\t\tdefault: /*\n+\t\t\t * Configured a protocol version > 2?\n+\t\t\t * Try version 2 as it's the most future proof.\n+\t\t\t */\n+\t\t\t/* fall through */\n+\t\tcase 2: /* first talk about capabilities, then get the heads */\n+\t\t\tget_remote_capabilities(data->fd[0], NULL, 0);\n+\t\t\trequest_capabilities(data->fd[1]);\n+\t\t\tget_remote_heads(data->fd[0], NULL, 0, &refs,\n+\t\t\t\t\t for_push ? REF_NORMAL : 0,\n+\t\t\t\t\t &data->extra_have,\n+\t\t\t\t\t &data->shallow);\n+\t\t\tbreak;\n+\t\tcase 1: /* configured version 1, fall through */\n+\t\tcase 0: /* unconfigured, use first protocol */\n+\t\t\tget_remote_heads(data->fd[0], NULL, 0, &refs,\n+\t\t\t\t\t for_push ? REF_NORMAL : 0,\n+\t\t\t\t\t &data->extra_have,\n+\t\t\t\t\t &data->shallow);\n+\t\t\tbreak;\n+\t}\n \tdata->got_remote_heads = 1;\n \n \treturn refs;\n-- \n2.4.1.345.gab207b6.dirty\n"},{"id":"262179","messageId":"1432677675-5118-11-git-send-email-sbeller@google.com","threadId":"39434","inReplyTo":"1432677675-5118-1-git-send-email-sbeller@google.com","subject":"[RFC/WIP PATCH 10/11] t5544: add a test case for the new protocol","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:01:14Z","receivedAt":"2015-05-26T22:01:14Z","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/t5544-fetch-2.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n create mode 100755 t/t5544-fetch-2.sh\n\ndiff --git a/t/t5544-fetch-2.sh b/t/t5544-fetch-2.sh\nnew file mode 100755\nindex 0000000..beee46c\n--- /dev/null\n+++ b/t/t5544-fetch-2.sh\n@@ -0,0 +1,40 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2015 Stefan Beller\n+#\n+\n+test_description='Testing version 2 of the fetch protocol'\n+\n+. ./test-lib.sh\n+\n+mk_repo_pair () {\n+\trm -rf client server &&\n+\ttest_create_repo client &&\n+\ttest_create_repo server &&\n+\t(\n+\t\tcd server &&\n+\t\tgit config receive.denyCurrentBranch warn\n+\t) &&\n+\t(\n+\t\tcd client &&\n+\t\tgit remote add origin ../server\n+\t\tgit config remote.origin.transportversion 2\n+\t)\n+}\n+\n+test_expect_success 'setup' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd server &&\n+\t\ttest_commit one\n+\t) &&\n+\t(\n+\t\tcd client &&\n+\t\tgit fetch origin master\n+\t)\n+'\n+\n+# More to come here, similar to t5515 having a sub directory full of expected\n+# data going over the wire.\n+\n+test_done\n-- \n2.4.1.345.gab207b6.dirty\n"},{"id":"262178","messageId":"1432677675-5118-12-git-send-email-sbeller@google.com","threadId":"39434","inReplyTo":"1432677675-5118-1-git-send-email-sbeller@google.com","subject":"[RFC/WIP PATCH 11/11] Document protocol version 2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:01:15Z","receivedAt":"2015-05-26T22:01:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n Documentation/technical/pack-protocol.txt | 86 ++++++++++++++++++++---\n Documentation/technical/protocol-capabilities.txt | 15 ----\n 2 files changed, 77 insertions(+), 24 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex fc09c63..4e1c82e 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -1,11 +1,11 @@\n Packfile transfer protocols\n ===========================\n \n-Git supports transferring data in packfiles over the ssh://, git:// and\n+Git supports transferring data in packfiles over the ssh://, git://, http:// and\n file:// transports. There exist two sets of protocols, one for pushing\n data from a client to a server and another for fetching data from a\n-server to a client. All three transports (ssh, git, file) use the same\n-protocol to transfer data.\n+server to a client. The three transports (ssh, git, file) use the same\n+protocol to transfer data. http is documented in http-protocol.txt.\n \n The processes invoked in the canonical Git implementation are 'upload-pack'\n on the server side and 'fetch-pack' on the client side for fetching data;\n@@ -14,6 +14,12 @@ data. The protocol functions to have a server tell a client what is\n currently on the server, then for the two to negotiate the smallest amount\n of data to send in order to fully update one or the other.\n \n+\"upload-pack-2\" and \"receive-pack-2\" are the next generation of\n+\"upload-pack\" and \"receive-pack\" respectively. The first two are\n+referred as \"version 2\" in this document and pack-capabilities.txt\n+while the last two are \"version 1\". Unless stated otherwise, \"version 1\"\n+is implied.\n+\n Transports\n ----------\n There are three transports over which the packfile protocol is\n@@ -42,7 +48,8 @@ hostname parameter, terminated by a NUL byte.\n \n --\n git-proto-request = request-command SP pathname NUL [ host-parameter NUL ]\n- request-command = \"git-upload-pack\" / \"git-receive-pack\" /\n+ request-command = \"git-upload-pack\" / \"git-upload-pack-2\" /\n+\t\t \"git-receive-pack\" / \"git-receive-pack-2\" /\n \t\t \"git-upload-archive\" ; case sensitive\n pathname = *( %x01-ff ) ; exclude NUL\n host-parameter = \"host=\" hostname [ \":\" port ]\n@@ -124,6 +131,48 @@ has, the first can 'fetch' from the second. This operation determines\n what data the server has that the client does not then streams that\n data down to the client in packfile format.\n \n+Capability discovery (v2)\n+-------------------------\n+\n+In version 1, capability discovery is part of reference discovery and\n+covered in reference discovery section.\n+\n+In version 2, when the client initially connects, the server\n+immediately sends its capabilities to the client followed by a flush.\n+Then the client must send the list of server capabilities it wants to\n+use to the server.\n+\n+ S: 00XXcapability:lang\\n\n+ S: 00XXcapability:thin-pack\\n\n+ S: 00XXcapability:ofs-delta\\n\n+ S: 00XXagent:agent=git/2:3.4.5+custom-739-gb850f98\\n\n+ S: 0000\n+\n+ C: 00XXcap:thin-pack\\n\n+ C: 00XXcap:ofs-delta\\n\n+ C: 00XXcap:lang=en\\n\n+ C: 00XXagent:agent=git/custom_string\\n\n+ C: 0000\n+\n+----\n+ capability-list = *(capability) [agent LF] flush-pkt\n+ capability = PKT-LINE(\"capability:\" keyvaluepair LF)\n+ agent = keyvaluepair LF\n+ keyvaluepair = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n+ LC_ALPHA = %x61-7A\n+----\n+\n+The client MUST ignore any data on pkt-lines starting with anything\n+different than \"capability\" for future ease of extension.\n+\n+The client MUST NOT ask for capabilities the server did not say it\n+supports. The server MUST diagnose and abort if capabilities it does\n+not understand was requested. The server MUST NOT ignore capabilities\n+that client requested and server advertised. As a consequence of these\n+rules, server MUST NOT advertise capabilities it does not understand.\n+\n+See protocol-capabilities.txt for a list of allowed server and client\n+capabilities and descriptions.\n \n Reference Discovery\n -------------------\n@@ -154,10 +203,14 @@ If HEAD is a valid ref, HEAD MUST appear as the first advertised\n ref. If HEAD is not a valid ref, HEAD MUST NOT appear in the\n advertisement list at all, but other refs may still appear.\n \n-The stream MUST include capability declarations behind a NUL on the\n-first ref. The peeled value of a ref (that is \"ref^{}\") MUST be\n-immediately after the ref itself, if presented. A conforming server\n-MUST peel the ref if it's an annotated tag.\n+In version 1 the stream MUST include capability declarations behind\n+a NUL on the first ref. The peeled value of a ref (that is \"ref^{}\")\n+MUST be immediately after the ref itself, if presented. A conforming\n+server MUST peel the ref if it's an annotated tag.\n+\n+In version 2 the capabilities are already negotiated, so the first ref\n+MUST NOT be followed by any capability advertisement, but it should be\n+treated as any other refs advertising line.\n \n ----\n advertised-refs = (no-refs / list-of-refs)\n@@ -178,13 +231,28 @@ MUST peel the ref if it's an annotated tag.\n shallow = PKT-LINE(\"shallow\" SP obj-id)\n \n capability-list = capability *(SP capability)\n- capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\")\n+ capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n LC_ALPHA = %x61-7A\n ----\n \n Server and client MUST use lowercase for obj-id, both MUST treat obj-id\n as case-insensitive.\n \n+On the very first line of the initial server response of either\n+receive-pack and upload-pack the first reference is followed by\n+a NUL byte and then a list of space delimited server capabilities.\n+These allow the server to declare what it can and cannot support\n+to the client.\n+\n+Client will then send a space separated list of capabilities it wants\n+to be in effect. The client MUST NOT ask for capabilities the server\n+did not say it supports.\n+\n+Server MUST diagnose and abort if capabilities it does not understand\n+was sent. Server MUST NOT ignore capabilities that client requested\n+and server advertised. As a consequence of these rules, server MUST\n+NOT advertise capabilities it does not understand.\n+\n See protocol-capabilities.txt for a list of allowed server capabilities\n and descriptions.\n \ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex 4f8a7bf..a6241d8 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -3,21 +3,6 @@ Git Protocol Capabilities\n \n Servers SHOULD support all capabilities defined in this document.\n \n-On the very first line of the initial server response of either\n-receive-pack and upload-pack the first reference is followed by\n-a NUL byte and then a list of space delimited server capabilities.\n-These allow the server to declare what it can and cannot support\n-to the client.\n-\n-Client will then send a space separated list of capabilities it wants\n-to be in effect. The client MUST NOT ask for capabilities the server\n-did not say it supports.\n-\n-Server MUST diagnose and abort if capabilities it does not understand\n-was sent. Server MUST NOT ignore capabilities that client requested\n-and server advertised. As a consequence of these rules, server MUST\n-NOT advertise capabilities it does not understand.\n-\n The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'\n capabilities are sent and recognized by the receive-pack (push to server)\n process.\n-- \n2.4.1.345.gab207b6.dirty\n"},{"id":"262183","messageId":"xmqqa8wrj85e.fsf@gitster.dls.corp.google.com","threadId":"39434","inReplyTo":"1432677675-5118-3-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 02/11] upload-pack: only accept capabilities on the first \"want\" line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-26T22:17:33Z","receivedAt":"2015-05-26T22:17:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> From: Nguyễn Thái Ngọc Duy \n>\n> pack-protocol.txt says so and fetch-pack also follows it even though\n> upload-pack is a bit lax. Fix it.\n\nHmm, I actually think the .txt file unsuccessfully tried to close\nthe barn door after horse has long left. The existing clients that\nread protocol capabilities keep the last-read copy and then\noverwrite it when a new one came, which is why we couldn't update\nthe protocol by sending only incremental changes, etc. without\nbreaking existing clients.\n\nI somehow thought that we already discussed why this was bad the\nfirst time Duy's patch was posted, but apparently we didn't.\n\nX-<.\n"},{"id":"293763","messageId":"xmqq617fj81l.fsf@gitster.dls.corp.google.com","threadId":"39434","inReplyTo":"1432677675-5118-8-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 07/11] fetch-pack: use the configured transport protocol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-26T22:19:50Z","receivedAt":"2015-05-26T22:19:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> @@ -175,7 +179,18 @@ 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.version) {\n> +\tdefault:\n> +\tcase 2:\n> +\t\tget_remote_capabilities(fd[0], NULL, 0);\n> +\t\trequest_capabilities(fd[1]);\n> +\t\tbreak;\n> +\tcase 1: /* fall through */\n> +\tcase 0:\n> +\t\tget_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);\n> +\t\tbreak;\n> +\t}\n\n"},{"id":"262184","messageId":"CAGZ79ka6VOHpOcJ8Y_3DHApJoK=3msc6DigA6aOVxXYVHdB2Sg@mail.gmail.com","threadId":"39434","inReplyTo":"xmqqa8wrj85e.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/WIP PATCH 02/11] upload-pack: only accept capabilities on the first \"want\" line","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:20:25Z","receivedAt":"2015-05-26T22:20:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 26, 2015 at 3:17 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> From: Nguyễn Thái Ngọc Duy \n>>\n>> pack-protocol.txt says so and fetch-pack also follows it even though\n>> upload-pack is a bit lax. Fix it.\n>\n> Hmm, I actually think the .txt file unsuccessfully tried to close\n> the barn door after horse has long left. The existing clients that\n> read protocol capabilities keep the last-read copy and then\n> overwrite it when a new one came, which is why we couldn't update\n> the protocol by sending only incremental changes, etc. without\n> breaking existing clients.\n\nOn the other hand I am not aware of any remote implementation\nhanding out capabilities after the first line, so this would not break\nanything to my knowledge.\n\n>\n> I somehow thought that we already discussed why this was bad the\n> first time Duy's patch was posted, but apparently we didn't.\n\n/me checks the archive. I may have overlooked that part. :(\n\n>\n> X-<.\n"},{"id":"262185","messageId":"xmqq1ti3j7ym.fsf@gitster.dls.corp.google.com","threadId":"39434","inReplyTo":"1432677675-5118-9-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-26T22:21:37Z","receivedAt":"2015-05-26T22:21:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +\tif (transport->smart_options\n> +\t && transport->smart_options->transport_version) {\n> +\t\tbuf = xmalloc(strlen(remote_program) + 12);\n> +\t\tsprintf(buf, \"%s-%d\", remote_program,\n> +\t\t\ttransport->smart_options->transport_version);\n> +\t\tremote_program = buf;\n> +\t}\n\nBikeshedding: so the versioning scheme is that the current one is\nzero, and the next one is two? I would have expected that there\nwould be something like\n\n\tif (...->version < 2) {\n \t... append \"-%d\" ...\n\t}\n\ninvolved.\n"},{"id":"262186","messageId":"CAGZ79kbuU1YwhxCnT7YRN=-cBsY0R820Mm2QTHJuMBju6r8Zew@mail.gmail.com","threadId":"39434","inReplyTo":"xmqq617fj81l.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/WIP PATCH 07/11] fetch-pack: use the configured transport protocol","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:23:10Z","receivedAt":"2015-05-26T22:23:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 26, 2015 at 3:19 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> @@ -175,7 +179,18 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n>> if (!conn)\n>> return args.diag_url ? 0 : 1;\n>> }\n>> - get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);\n>> +\n>> + switch (args.version) {\n>> + default:\n>> + case 2:\n>> + get_remote_capabilities(fd[0], NULL, 0);\n>> + request_capabilities(fd[1]);\n>> + break;\n\nActually this is wrong, we need to actually fall through from here as well,\nso we not only talk capabilities negotiation, but then continue\nwith get_remote_heads.\n\n>> + case 1: /* fall through */\n>> + case 0:\n>> + get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);\n>> + break;\n>> + }\n>\n> Nice ;-)\n"},{"id":"262187","messageId":"CAGZ79kY=hZ=6pUbpVHUW8rU3AEMx2TwkBYDsDsZ+bPH_QnsYzA@mail.gmail.com","threadId":"39434","inReplyTo":"xmqq1ti3j7ym.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-26T22:31:12Z","receivedAt":"2015-05-26T22:31:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> + if (transport->smart_options\n>> + && transport->smart_options->transport_version) {\n>> + buf = xmalloc(strlen(remote_program) + 12);\n>> + sprintf(buf, \"%s-%d\", remote_program,\n>> + transport->smart_options->transport_version);\n>> + remote_program = buf;\n>> + }\n>\n> Bikeshedding: so the versioning scheme is that the current one is\n> zero, and the next one is two? I would have expected that there\n> would be something like\n\nI think currently we have version 1. But we don't advertise it, so\nI'll call it 0\n(the default or unadvertised or however you name it. 0 as in \"unsure\" maybe)\n\nI thought about future proofing this version a bit. Say version 2 is bad because\nI don't have the experience of 10 years Git nor of maintaining large\nprojects and\nyou want to make a version 3 soon. And this would support that just fine.\n\nThe meaning being: Any version except 0 should have a dedicated\nextension -${version}\nThe 0 is left out for backwards compatibility.\n\nSo in a later patch where we want to introduce force-using of old\nversions you could\nconfigure upload-pack to be explicit upload-pack-1. The upload-pack-1\nversion is not\nyet there with this series though.\n\n>\n> if (...->version < 2) {\n> ... append \"-%d\" ...\n> }\n>\n> involved.\n\nOh! I see here you would count the current one as 1, which has no\nnumber extension,\nand any further would have a -${version}. That would transport the\nintention much better\nI guess.\n"},{"id":"262191","messageId":"CAPig+cQHQFrabqkXUaRHw7HU+URL4QBzzLZYko5GrRHFpYctng@mail.gmail.com","threadId":"39434","inReplyTo":"1432677675-5118-5-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-27T02:30:09Z","receivedAt":"2015-05-27T02:30:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 26, 2015 at 6:01 PM, Stefan Beller wrote:\n> In upload-pack-2 we send each capability in its own packet.\n> By reusing the advertise_capabilities and eventually setting it to\n> NULL we will be able to reuse the methods for refs advertisement.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/upload-pack-2.c b/upload-pack-2.c\n> new file mode 120000\n> index 0000000..e30a871\n> --- /dev/null\n> +++ b/upload-pack-2.c\n> @@ -716,10 +716,47 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)\n> strbuf_addf(buf, \" symref=%s:%s\", item->string, (char *)item->util);\n> }\n>\n> -static const char *advertise_capabilities = \"multi_ack thin-pack side-band\"\n> +static char *advertise_capabilities = \"multi_ack thin-pack side-band\"\n> \" side-band-64k ofs-delta shallow no-progress\"\n> \" include-tag multi_ack_detailed\";\n>\n> +/*\n> + * Reads the next capability and puts it into dst as a null terminated string.\n> + * Returns true if more capabilities can be read.\n> + * */\n> +static int next_capability(char *dst)\n> +{\n> + int len = 0;\n> + if (!*advertise_capabilities) {\n> + /* make sure to not advertise capabilities afterwards */\n> + advertise_capabilities = NULL;\n\nYou set advertise_capabilities to NULL here but then unconditionally\ndereference that NULL in the if-statement just above (if someone calls\nnext_capability() again). Seems fishy (and crashy) to me. Either don't\ndereference the NULL or don't bother setting it to NULL (in which\ncase, you'll dereference and find '\\0', which should serve the same\npurpose).\n\n> + return 0;\n> + }\n> +\n> + while (advertise_capabilities[len] != '\\0' &&\n> + advertise_capabilities[len] != ' ')\n> + len ++;\n\nStyle: len++\n\n> + strncpy(dst, advertise_capabilities, len);\n> + dst[len] = '\\0';\n> +\n> + advertise_capabilities += len;\n> + if (*advertise_capabilities == ' ')\n> + advertise_capabilities++;\n\nIf for some reason, someone happens to add an extra space between\ncapabilities in advertise_capabilities, then the capability returned\nby the next invocation of next_capability() be zero-length, which is\nprobably undesirable, and certainly not expected by the caller.\nSkipping whitespace in a loop would be more robust.\n\n> + return 1;\n> +}\n\nI realize that this is RFC, but my overall reaction to\nnext_capability() in its current form is that it's ugly . A somewhat\ncleaner approach would be to pass some state into next_capability()\nand have it modify that state rather than the global\nadvertise_capabilities. The passed-in state could be as simple as a\n'const char *' which initially points at the start of\nadvertise_capabilities and then gets advanced; until, finally, it\npoints at the '\\0' at the end of advertise_capabilities.\n\n> +static void send_capabilities(void)\n> +{\n> + char buf[100];\n> +\n> + while (next_capability(buf))\n> + packet_write(1, \"capability:%s\\n\", buf);\n> +\n> + packet_write(1, \"agent:%s\\n\", git_user_agent_sanitized());\n> + packet_flush(1);\n> +}\n> +\n> static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n> {\n>\n> @@ -794,6 +831,28 @@ static void upload_pack(void)\n> }\n> }\n>\n> +static void receive_capabilities(void)\n> +{\n> + int done = 0;\n> + while (1) {\n> + char *line = packet_read_line(0, NULL);\n\nIf you declare this 'const char *', then it becomes much more obvious\nthat it's not your responsibility to free() the result (and,\ntangentially, that you have no intention of modifying the content).\n\n> + if (!line)\n> + break;\n> + if (starts_with(line, \"capability:\"))\n> + parse_features(line + strlen(\"capability:\"));\n\nSee skip_prefix().\n\n> + }\n> +}\n> +\n> +static void upload_pack_version_2(void)\n> +{\n> + send_capabilities();\n> + receive_capabilities();\n> +\n> + /* The rest of the protocol stays the same, capabilities advertising\n> + is disabled though. */\n\n /*\n * This is a multi-line\n * comment.\n */\n\n> + upload_pack();\n> +}\n> +\n> static int upload_pack_config(const char *var, const char *value, void *unused)\n> {\n> if (!strcmp(\"uploadpack.allowtipsha1inwant\", var))\n> @@ -806,16 +865,24 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n> return parse_hide_refs_config(var, value, \"uploadpack\");\n> }\n>\n> +static int endswith(const char *teststring, const char *ending)\n> +{\n> + int slen = strlen(teststring);\n> + int elen = strlen(ending);\n\nYou may be confident today that no callers are supplying an 'ending'\nwhich is longer than 'teststring', but someone may some day break that\nassumption. At the very least, for robustness, add an assert() or\ndie() if 'elen' is greater than 'slen'.\n\n> + return !strcmp(teststring + slen - elen, ending);\n> +}\n> +\n> int main(int argc, char **argv)\n> {\n> char *dir;\n> + const char *cmd;\n> int i;\n> int strict = 0;\n>\n> git_setup_gettext();\n>\n> packet_trace_identity(\"upload-pack\");\n> - git_extract_argv0_path(argv[0]);\n> + cmd = git_extract_argv0_path(argv[0]);\n> check_replace_refs = 0;\n>\n> for (i = 1; i < argc; i++) {\n> @@ -857,6 +924,10 @@ int main(int argc, char **argv)\n> die(\"'%s' does not appear to be a git repository\", dir);\n>\n> git_config(upload_pack_config, NULL);\n> - upload_pack();\n> +\n> + if (endswith(cmd, \"-2\"))\n> + upload_pack_version_2();\n> + else\n> + upload_pack();\n> return 0;\n> }\n> --\n> 2.4.1.345.gab207b6.dirty\n"},{"id":"262192","messageId":"CAPig+cRfJKAQ8Q5PF1VfTAGA1njXAshC0RbnMv9cEp4bH_MN7A@mail.gmail.com","threadId":"39434","inReplyTo":"1432677675-5118-7-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-27T03:25:05Z","receivedAt":"2015-05-27T03:25:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 26, 2015 at 6:01 PM, Stefan Beller wrote:\n> Instead of calling get_remote_heads as a first command during the\n> protocol exchange, we need to have fine grained control over the\n> capability negotiation in version 2 of the protocol.\n>\n> Introduce get_remote_capabilities, which will just listen to\n> capabilities of the remote and request_capabilities which will\n> tell the selection of capabilities to the remote.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/connect.c b/connect.c\n> index c0144d8..bb17618 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -105,8 +105,54 @@ static void annotate_refs_with_symref_info(struct ref *ref)\n> string_list_clear(&symref, 0);\n> }\n>\n> +void get_remote_capabilities(int in, char *src_buf, size_t src_len)\n> +{\n> + struct strbuf capabilities_string = STRBUF_INIT;\n> + for (;;) {\n> + int len;\n> + char *line = packet_buffer;\n> + const char *arg;\n> +\n> + len = packet_read(in, &src_buf, &src_len,\n> + packet_buffer, sizeof(packet_buffer),\n> + PACKET_READ_GENTLE_ON_EOF |\n> + PACKET_READ_CHOMP_NEWLINE);\n> + if (len < 0)\n> + die_initial_contact(0);\n> +\n> + if (!len)\n> + break;\n> +\n> + if (len > 4 && skip_prefix(line, \"ERR \", &arg))\n\nThe 'len > 4' check is needed because there's no guarantee that 'line'\nis NUL-terminated. Correct?\n\n> + die(\"remote error: %s\", arg);\n> +\n> + if (starts_with(line, \"capability:\")) {\n> + strbuf_addstr(&capabilities_string, line + strlen(\"capability:\"));\n\nskip_prefix() maybe?\n\n> + strbuf_addch(&capabilities_string, ' ');\n> + }\n> + }\n> + free(server_capabilities);\n> + server_capabilities = xmalloc(capabilities_string.len + 1);\n> + server_capabilities = strbuf_detach(&capabilities_string, NULL);\n\nSo, you're allocating a buffer for server_capabilities and then\nimmediately throwing away (leaking) that buffer. Seems fishy.\n\n> + strbuf_release(&capabilities_string);\n\nNot outright incorrect, but you've just detached capabilities_string's\nbuffer, so releasing it doesn't buy anything.\n\n> +}\n> +\n> +int request_capabilities(int out)\n> +{\n> + fprintf(stderr, \"request_capabilities\\n\");\n> + // todo: send our capabilities\n> + packet_write(out, \"capability:foo\");\n> + packet_flush(out);\n> +}\n> +\n> /*\n> - * Read all the refs from the other end\n> + * Read all the refs from the other end,\n> + * in is a file descriptor input stream\n> + * src_buf and src_len may be an alternative way to specify the input.\n\nThe bit about src_buf and src_len conveys little or nothing. After\nreading it, I'm as clueless to their purpose as I would be if they\nwere undocumented.\n\n> + * list is the output of refs\n> + * extra_have is a list to store information learned about sha1 the other side has.\n> + * shallow_points\n\n'shallow_points' what?\n\n> */\n> struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n> struct ref **list, unsigned int flags,\n> diff --git a/remote.h b/remote.h\n> index 04e2310..7381ddf 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -146,6 +146,9 @@ int check_ref_type(const struct ref *ref, int flags);\n> void free_refs(struct ref *ref);\n>\n> struct sha1_array;\n> +\n> +extern void get_remote_capabilities(int in, char *src_buf, size_t src_len);\n> +extern int request_capabilities(int out);\n> extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n> struct ref **list, unsigned int flags,\n> struct sha1_array *extra_have,\n> --\n> 2.4.1.345.gab207b6.dirty\n"},{"id":"262193","messageId":"CAPig+cQAYKH-3Kr4h+Mhag0J8XC+9qDPfyLZD9FfTXOYmUuDLQ@mail.gmail.com","threadId":"39434","inReplyTo":"1432677675-5118-9-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-27T03:33:48Z","receivedAt":"2015-05-27T03:33:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 26, 2015 at 6:01 PM, Stefan Beller wrote:\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/transport.c b/transport.c\n> index 3ef15f6..33644a6 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -496,15 +496,29 @@ static int set_git_option(struct git_transport_options *opts,\n> static int connect_setup(struct transport *transport, int for_push, int verbose)\n> {\n> struct git_transport_data *data = transport->data;\n> + const char *remote_program;\n> + char *buf = 0;\n\nNaming this 'to_free' would make its purpose more obvious[1], and be\nmore consistent with code elsewhere in the project.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/256125/\n\n> if (data->conn)\n> return 0;\n>\n> + remote_program = (for_push ? data->options.receivepack\n> + : data->options.uploadpack);\n> +\n> + if (transport->smart_options\n> + && transport->smart_options->transport_version) {\n> + buf = xmalloc(strlen(remote_program) + 12);\n> + sprintf(buf, \"%s-%d\", remote_program,\n> + transport->smart_options->transport_version);\n> + remote_program = buf;\n> + }\n> +\n> data->conn = git_connect(data->fd, transport->url,\n> - for_push ? data->options.receivepack :\n> - data->options.uploadpack,\n> + remote_program,\n> verbose ? CONNECT_VERBOSE : 0);\n>\n> + free(buf);\n> +\n> return 0;\n> }\n>\n> --\n> 2.4.1.345.gab207b6.dirty\n"},{"id":"262194","messageId":"xmqqwpzuip2e.fsf@gitster.dls.corp.google.com","threadId":"39434","inReplyTo":"CAGZ79kY=hZ=6pUbpVHUW8rU3AEMx2TwkBYDsDsZ+bPH_QnsYzA@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-27T05:09:45Z","receivedAt":"2015-05-27T05:09:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano wrote:\n>\n>>\n>> if (...->version < 2) {\n>> ... append \"-%d\" ...\n>> }\n>>\n>> involved.\n>\n> Oh! I see here you would count the current one as 1, which has no\n> number extension, and any further would have a -${version}. That\n> would transport the intention much better I guess.\n\nYeah, except that I screwed up my comparison. Obviously, I should\nhave said \"If version is 2 or later, then append -%d to the name,\notherwise use the name as-is\".\n"},{"id":"262196","messageId":"CAPig+cS3ySggj48o0SK3wNROjBJgohLScG3Xtp2aaR9OXpoPiw@mail.gmail.com","threadId":"39434","inReplyTo":"1432677675-5118-11-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 10/11] t5544: add a test case for the new protocol","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-27T05:34:39Z","receivedAt":"2015-05-27T05:34:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 26, 2015 at 6:01 PM, Stefan Beller wrote:\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/t/t5544-fetch-2.sh b/t/t5544-fetch-2.sh\n> new file mode 100755\n> index 0000000..beee46c\n> --- /dev/null\n> +++ b/t/t5544-fetch-2.sh\n> @@ -0,0 +1,40 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2015 Stefan Beller\n> +#\n> +\n> +test_description='Testing version 2 of the fetch protocol'\n> +\n> +. ./test-lib.sh\n> +\n> +mk_repo_pair () {\n> + rm -rf client server &&\n> + test_create_repo client &&\n> + test_create_repo server &&\n> + (\n> + cd server &&\n> + git config receive.denyCurrentBranch warn\n> + ) &&\n> + (\n> + cd client &&\n> + git remote add origin ../server\n\nBroken &&-chain.\n\n> + git config remote.origin.transportversion 2\n> + )\n> +}\n> +\n> +test_expect_success 'setup' '\n> + mk_repo_pair &&\n> + (\n> + cd server &&\n> + test_commit one\n> + ) &&\n> + (\n> + cd client &&\n> + git fetch origin master\n> + )\n> +'\n> +\n> +# More to come here, similar to t5515 having a sub directory full of expected\n> +# data going over the wire.\n> +\n> +test_done\n> --\n> 2.4.1.345.gab207b6.dirty\n"},{"id":"262197","messageId":"CAPig+cQ-qUZs+KfOCXvRwTBWJe7Kx6RiEmiPphQVyx3=mxpVQw@mail.gmail.com","threadId":"39434","inReplyTo":"1432677675-5118-10-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 09/11] transport: get_refs_via_connect exchanges capabilities before refs.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-27T05:37:24Z","receivedAt":"2015-05-27T05:37:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 26, 2015 at 6:01 PM, Stefan Beller wrote:\n> transport: get_refs_via_connect exchanges capabilities before refs.\n\ns/exchanges/exchange/\ns/\\.$//\n\n> Signed-off-by: Stefan Beller \n> ---\n> transport.c | 29 +++++++++++++++++++++++++----\n> 1 file changed, 25 insertions(+), 4 deletions(-)\n>\n> diff --git a/transport.c b/transport.c\n> index 33644a6..1cd9b77 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -526,12 +526,33 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus\n> {\n> struct git_transport_data *data = transport->data;\n> struct ref *refs;\n> + int version = 0;\n>\n> + if (transport->smart_options)\n> + version = transport->smart_options->transport_version;\n> connect_setup(transport, for_push, 0);\n> - get_remote_heads(data->fd[0], NULL, 0, &refs,\n> - for_push ? REF_NORMAL : 0,\n> - &data->extra_have,\n> - &data->shallow);\n> + switch (version) {\n> + default: /*\n> + * Configured a protocol version > 2?\n> + * Try version 2 as it's the most future proof.\n> + */\n> + /* fall through */\n> + case 2: /* first talk about capabilities, then get the heads */\n> + get_remote_capabilities(data->fd[0], NULL, 0);\n> + request_capabilities(data->fd[1]);\n> + get_remote_heads(data->fd[0], NULL, 0, &refs,\n> + for_push ? REF_NORMAL : 0,\n> + &data->extra_have,\n> + &data->shallow);\n> + break;\n> + case 1: /* configured version 1, fall through */\n> + case 0: /* unconfigured, use first protocol */\n> + get_remote_heads(data->fd[0], NULL, 0, &refs,\n> + for_push ? REF_NORMAL : 0,\n> + &data->extra_have,\n> + &data->shallow);\n> + break;\n> + }\n> data->got_remote_heads = 1;\n>\n> return refs;\n> --\n> 2.4.1.345.gab207b6.dirty\n"},{"id":"262198","messageId":"20150527061818.GA885@peff.net","threadId":"39434","inReplyTo":"1432677675-5118-1-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 00/11] Protocol version 2, again!","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T06:18:19Z","receivedAt":"2015-05-27T06:18:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 26, 2015 at 03:01:04PM -0700, Stefan Beller wrote:\n\n> \"Just give us something to play around with\" - Peff at GitMerge 2015\n\nSounds like something I would say.\n\n> The new protocol works just like the old protocol, except for\n> the capabilities negotiation being before any exchange of real data.\n\nI like this approach. We all know that one next step is going to be a\n\"show me only these refs\" capability negotiation of some kind. But it's\ngood to keep the version-breaking changes separated from that, and this\nshould be enough to bootstrap that conversation later.\n\nIf I understand correctly, your proposed protocol allows for a single\nback-and-forth of capabilities followed by the server speaking the ref\nadvertisement. So it is worth thinking a moment how we might have a more\ninvolved conversation before the advertisement if we want to do so in\nthe future.\n\nI think in the simplest case, the server claims to understand the \"foo\"\nextension, and then the client says \"I wish to use foo\". And then a rule\nof \"foo\" might be that the conversation continues in some way before\nsending the advertisement. Like (each line is a pkt-line):\n\n ...\n S: foo\n S: flush\n ...\n C: foo\n S: Here's some extra foo data.\n C: Now I respond to that foo data.\n S: Now the foo conversation is done. Here's the ref advertisement.\n\nWhat if there are multiple such extensions? E.g., if the client asks for\nboth \"foo\" and \"bar\", and both require extra back-and-forth messages?\nWhich conversation happens first?\n\nMaybe the rule is just \"whichever one the client asked for first in the\ncapabilities list\". And I think in general we want to avoid protocol\nround-trips if we can (so we'd prefer the client say\n\"refspec=refs/heads/*\", and not \"I understand refspecs, too! Let's have\na conversation about which ones I'm interested in.\"). But I think it's\nworth giving it a little thought to make sure we don't paint ourselves\nin a corner.\n\n> My preference for a string suffix instead of a sequential number is\n> motiviated by the discussion of sha1 vs sequential numbers to describe\n> a state of a repository. The main difference here is however how often\n> we expect changes. Version 1 of the protocol is current for 10 years by\n> now, so I do not changes to the protocol number often, which makes it\n> suitable for just having a counter appended to the binary.\n\nI think I prefer a number. I'm really hoping that v2 lasts even longer\nthan v1 has, because the capability negotiation should allow us to\nextend it from within rather than breaking compatibility.\n\nAs a minor nit, I think I like \"upload-pack-v2\" better than\n\"upload-pack-2\". But I can live with it either way. :)\n\n> This series doesn't include an automatic upgrade of the protocol version\n> for later changes if the server supports it, so for now the use of the new\n> protocol needs to be activated manually via setting remote.origin.transportversion.\n\nI think that's a good start. Last time we discussed it, I think\neverybody was more or less on board with client probing (so v1 would\nstart to say \"btw, I speak v2\", and then client would set its\nremote.origin.transportversion flag). That can come later, and we are\nnot painting ourselves into a corner.\n\nI think we also discussed passing the version information out-of-band.\nSo over git-daemon, as \"git-upload-pack repo\\0host=...\\0\\0version=2\",\nfor example. I _think_ we are also fine to build that on top of what you\nhave here. If the version information makes it through to upload-pack,\nthen we can do v2, and if not, we are no worse off than we were. HTTP\ncan do a similar out-of-band thing, but I think ssh is mostly out of\nluck. The best I could think of was passing an environment variable, but\nssh typically only lets through a few variables. We could abuse PATH or\nsomething, but that's getting pretty nasty.\n\nAnyway, that is all for the future. The only reason I mention it is to\nmake sure that we are not closing any future doors, and I don't think we\nare.\n\n-Peff\n"},{"id":"262205","messageId":"20150527063558.GB885@peff.net","threadId":"39434","inReplyTo":"1432677675-5118-5-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T06:35:58Z","receivedAt":"2015-05-27T06:35:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 26, 2015 at 03:01:08PM -0700, Stefan Beller wrote:\n\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -716,10 +716,47 @@ 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 const char *advertise_capabilities = \"multi_ack thin-pack side-band\"\n> +static char *advertise_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\nIf we are upload-pack-2, should we advertise that in the capabilities? I\nthink it may make things easier later if we try to provide some\nopportunistic out-of-band data. E.g., if see tell git-daemon:\n\n git-upload-pack repo\\0host=whatever\\0\\0version=2\n\nhow do we know whether version=2 was understood and kicked us into v2\nmode, versus an old server that ignored it?\n\nIt also just makes the protocol more self-describing; from the\nconversation you can see which version is in use.\n\n> +static void send_capabilities(void)\n> +{\n> +\tchar buf[100];\n> +\n> +\twhile (next_capability(buf))\n> +\t\tpacket_write(1, \"capability:%s\\n\", buf);\n\nHaving a static-sized buffer and then passing it to a function which has\nno idea of the buffer size seems like a recipe for a buffer overflow. :)\n\nYou are fine here because you are parsing the hard-coded capabilities\nstring, and we know 100 is enough there. But it's nice to avoid such\nmagic.\n\nLike Eric, I find the whole next_capability thing a little ugly. His\nsuggestion to pass in the parsing state is an improvement, but I wonder\nwhy we need to parse at all. Can we keep the capabilities as:\n\n const char *capabilities[] = {\n\t\"multi_ack\",\n\t\"thin-pack\",\n\t... etc ...\n };\n\nand then loop over the array? The v1 writer will need to be modified,\nbut it should be fairly straightforward (loop and add them to the buffer\nrather than dumping the whole string).\n\nAlso, do we need the capability noise-word? They all have it, except\nfor:\n\n> +\tpacket_write(1, \"agent:%s\\n\", git_user_agent_sanitized());\n\nBut isn't that basically a capability, too (I agree it is stretching the\ndefinition of \"capability\", but I think that ship has sailed; the\nclient's response is not \"I support this, too\" but \"I want to enable\nthis\").\n\nIOW, is there a reason that the initial conversation is not just:\n\n pkt-line(\"multi_ack\");\n pkt-line(\"thin-pack\");\n ...\n pkt-line(\"agent=v2.5.0\");\n pkt-line(0000);\n\nWe already have framing for each capability due to the use of pkt-line.\nThe \"capability:\" is just one more thing the client has to parse past.\nThe keys are already unique up to any \"=\", so I don't think there is any\nambiguity (e.g., we don't care about \"capability:agent\"; we have already\nassigned a meaning to the term \"agent\", and will never introduce a\nstandalone capability with the same name).\n\nLikewise, if we introduce new protocol items here, the client should\neither ignore them (if it does not understand them) or know what to do\nwith them.\n\n> +static void receive_capabilities(void)\n> +{\n> +\tint done = 0;\n> +\twhile (1) {\n> +\t\tchar *line = packet_read_line(0, NULL);\n> +\t\tif (!line)\n> +\t\t\tbreak;\n> +\t\tif (starts_with(line, \"capability:\"))\n> +\t\t\tparse_features(line + strlen(\"capability:\"));\n> +\t}\n\nUse:\n\n const char *cap;\n if (skip_prefix(line, \"capability:\", &cap))\n\tparse_features(cap);\n\nOr better yet, if you take my suggestion above:\n\n parse_features(line);\n\n:)\n\n> +static int endswith(const char *teststring, const char *ending)\n> +{\n> +\tint slen = strlen(teststring);\n> +\tint elen = strlen(ending);\n> +\treturn !strcmp(teststring + slen - elen, ending);\n> +}\n\nEric mentioned the underflow problems here, but I wonder even more:\nwhat's wrong with the global ends_with() that we already provide?\n\n-Peff\n"},{"id":"262206","messageId":"20150527063925.GC885@peff.net","threadId":"39434","inReplyTo":"1432677675-5118-6-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 05/11] transport: add infrastructure to support a protocol version number","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T06:39:25Z","receivedAt":"2015-05-27T06:39:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 26, 2015 at 03:01:09PM -0700, Stefan Beller wrote:\n\n> +\tOPT_STRING('y', \"transport-version\", &transport_version,\n> +\t\t N_(\"transport-version\"),\n> +\t\t N_(\"specify transport version to be used\")),\n\nInteresting choice for the short option (\"-v\" would be nice, but\nobviously it is taken). Do we want to delay on claiming the\nshort-and-sweet 'y' until we are sure this is something people will use\na lot? In an ideal world, it is not (i.e., auto-upgrade and other tricks\nbecome good enough that nobody bothers to specify it manually).\n\n-Peff\n"},{"id":"262207","messageId":"20150527064521.GD885@peff.net","threadId":"39434","inReplyTo":"1432677675-5118-7-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T06:45:21Z","receivedAt":"2015-05-27T06:45:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 26, 2015 at 03:01:10PM -0700, Stefan Beller wrote:\n\n> +void get_remote_capabilities(int in, char *src_buf, size_t src_len)\n> +{\n> +\tstruct strbuf capabilities_string = STRBUF_INIT;\n> +\tfor (;;) {\n> +\t\tint len;\n> +\t\tchar *line = packet_buffer;\n> +\t\tconst char *arg;\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\tif (len > 4 && skip_prefix(line, \"ERR \", &arg))\n> +\t\t\tdie(\"remote error: %s\", arg);\n> +\n> +\t\tif (starts_with(line, \"capability:\")) {\n> +\t\t\tstrbuf_addstr(&capabilities_string, line + strlen(\"capability:\"));\n> +\t\t\tstrbuf_addch(&capabilities_string, ' ');\n> +\t\t}\n> +\t}\n\nI think this is the reverse case of next_capabilities in the upload-pack\nside, so I'll make the reverse suggestion. :) Would it make things nicer\nif both v1 and v2 parsed the capabilities into a string_list?\n\n> +\tfree(server_capabilities);\n> +\tserver_capabilities = xmalloc(capabilities_string.len + 1);\n> +\tserver_capabilities = strbuf_detach(&capabilities_string, NULL);\n\nIs this xmalloc line left over? The strbuf_detach will write over it.\n\n> +\tstrbuf_release(&capabilities_string);\n\nNo need to release if we just detached.\n\n> +int request_capabilities(int out)\n> +{\n> +\tfprintf(stderr, \"request_capabilities\\n\");\n\nDebug cruft, I presume.\n\n> +\t// todo: send our capabilities\n> +\tpacket_write(out, \"capability:foo\");\n\nNo C99 comments. But I think this is just a placeholder. :)\n\n-Peff\n"},{"id":"262208","messageId":"20150527065030.GE885@peff.net","threadId":"39434","inReplyTo":"CAPig+cRfJKAQ8Q5PF1VfTAGA1njXAshC0RbnMv9cEp4bH_MN7A@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T06:50:31Z","receivedAt":"2015-05-27T06:50:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 26, 2015 at 11:25:05PM -0400, Eric Sunshine wrote:\n\n> > + len = packet_read(in, &src_buf, &src_len,\n> > + packet_buffer, sizeof(packet_buffer),\n> > + PACKET_READ_GENTLE_ON_EOF |\n> > + PACKET_READ_CHOMP_NEWLINE);\n> > + if (len < 0)\n> > + die_initial_contact(0);\n> > +\n> > + if (!len)\n> > + break;\n> > +\n> > + if (len > 4 && skip_prefix(line, \"ERR \", &arg))\n> \n> The 'len > 4' check is needed because there's no guarantee that 'line'\n> is NUL-terminated. Correct?\n\nI think this was just blindly copied from get_remote_heads(). And I\nthink that code was being overly paranoid. Ever since f3a3214 (Make\nsend/receive-pack be closer to doing something interesting, 2005-06-29),\nthe pkt-line reader will add an extra NUL to the buffer to ease cases\nlike this.\n\n-Peff\n"},{"id":"262209","messageId":"20150527065342.GA1756@peff.net","threadId":"39434","inReplyTo":"1432677675-5118-8-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 07/11] fetch-pack: use the configured transport protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T06:53:42Z","receivedAt":"2015-05-27T06:53:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 26, 2015 at 03:01:11PM -0700, Stefan Beller wrote:\n\n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index 4a6b340..32dc8b0 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -127,6 +127,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 (!strcmp(\"--transport-version\", arg)) {\n> +\t\t\targs.version = strtol(arg + strlen(\"--transport-version\"), NULL, 0);\n> +\t\t\tcontinue;\n> +\t\t}\n\nYou strcmp() the arg here, so there can't be anything at arg +\nstrlen(...), can there? Did you want:\n\n starts_with(arg, \"--transports-version=\")\n\nhere? Or better yet, skip_prefix().\n\n> -\tget_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);\n> +\n> +\tswitch (args.version) {\n> +\tdefault:\n> +\tcase 2:\n> +\t\tget_remote_capabilities(fd[0], NULL, 0);\n> +\t\trequest_capabilities(fd[1]);\n> +\t\tbreak;\n\nShould the \"default\" case be to complain about an unknown version,\nrather than fall-through to 2?\n\n-Peff\n"},{"id":"262210","messageId":"20150527065617.GF885@peff.net","threadId":"39434","inReplyTo":"xmqqwpzuip2e.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T06:56:17Z","receivedAt":"2015-05-27T06:56:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 26, 2015 at 10:09:45PM -0700, Junio C Hamano wrote:\n\n> Stefan Beller writes:\n> \n> > On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano wrote:\n> >\n> >>\n> >> if (...->version < 2) {\n> >> ... append \"-%d\" ...\n> >> }\n> >>\n> >> involved.\n> >\n> > Oh! I see here you would count the current one as 1, which has no\n> > number extension, and any further would have a -${version}. That\n> > would transport the intention much better I guess.\n> \n> Yeah, except that I screwed up my comparison. Obviously, I should\n> have said \"If version is 2 or later, then append -%d to the name,\n> otherwise use the name as-is\".\n\nFWIW, I had similar head-scratching over Stefan's original. I think it's\nOK to say \"version 1 is magical, and for historical reasons does not\nhave its number appended\". There's no need for us ever to make\n\"upload-pack-1\"; it just introduces more headaches.\n\n-Peff\n"},{"id":"262211","messageId":"20150527070202.GG885@peff.net","threadId":"39434","inReplyTo":"1432677675-5118-9-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T07:02:02Z","receivedAt":"2015-05-27T07:02:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 26, 2015 at 03:01:12PM -0700, Stefan Beller wrote:\n\n> Signed-off-by: Stefan Beller \n> ---\n> transport.c | 18 ++++++++++++++++--\n> 1 file changed, 16 insertions(+), 2 deletions(-)\n> \n> diff --git a/transport.c b/transport.c\n> index 3ef15f6..33644a6 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -496,15 +496,29 @@ static int set_git_option(struct git_transport_options *opts,\n> static int connect_setup(struct transport *transport, int for_push, int verbose)\n> {\n> \tstruct git_transport_data *data = transport->data;\n> +\tconst char *remote_program;\n> +\tchar *buf = 0;\n\nUse NULL when you mean a NULL pointer (they're equivalent to the\ncompiler, but the word is easier to read).\n\nI agree on Eric's naming this \"to_free\" (and I consider it idiomatic to\nassign them in a chain, like \"foo = to_free = xmalloc(...)\", but we\ndon't always do that).\n\n> +\tif (transport->smart_options\n> +\t && transport->smart_options->transport_version) {\n> +\t\tbuf = xmalloc(strlen(remote_program) + 12);\n> +\t\tsprintf(buf, \"%s-%d\", remote_program,\n> +\t\t\ttransport->smart_options->transport_version);\n> +\t\tremote_program = buf;\n> +\t}\n\nUsing xstrfmt can help you avoid magic numbers and repetition,\nlike:\n\n to_free = xstrfmt(\"%s-%d\",\n remote_program,\n\t\t transport->smart_options->transport_version);\n\n-Peff\n"},{"id":"262212","messageId":"20150527070603.GH885@peff.net","threadId":"39434","inReplyTo":"1432677675-5118-10-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 09/11] transport: get_refs_via_connect exchanges capabilities before refs.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T07:06:03Z","receivedAt":"2015-05-27T07:06:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 26, 2015 at 03:01:13PM -0700, Stefan Beller wrote:\n\n> +\tswitch (version) {\n> +\t\tdefault: /*\n> +\t\t\t * Configured a protocol version > 2?\n> +\t\t\t * Try version 2 as it's the most future proof.\n> +\t\t\t */\n> +\t\t\t/* fall through */\n\nSame comment here as earlier. If we do a v3, it might not be compatible\nwith v2. Shouldn't we bail if the user asked for it?\n\n> +\t\tcase 2: /* first talk about capabilities, then get the heads */\n> +\t\t\tget_remote_capabilities(data->fd[0], NULL, 0);\n> +\t\t\trequest_capabilities(data->fd[1]);\n> +\t\t\tget_remote_heads(data->fd[0], NULL, 0, &refs,\n> +\t\t\t\t\t for_push ? REF_NORMAL : 0,\n> +\t\t\t\t\t &data->extra_have,\n> +\t\t\t\t\t &data->shallow);\n> +\t\t\tbreak;\n> +\t\tcase 1: /* configured version 1, fall through */\n> +\t\tcase 0: /* unconfigured, use first protocol */\n> +\t\t\tget_remote_heads(data->fd[0], NULL, 0, &refs,\n> +\t\t\t\t\t for_push ? REF_NORMAL : 0,\n> +\t\t\t\t\t &data->extra_have,\n> +\t\t\t\t\t &data->shallow);\n> +\t\t\tbreak;\n> +\t}\n\nI actually kind of wonder if we should just die(\"BUG\") here on seeing\n\"0\". Is this low level the right place to do the \"default to v1\"?\nBecause eventually we're going to want to default to v2, I would think\n(after a few years have passed, at least). It seems like we should be\nmaking that decision centrally when we init the transport options.\n\n-Peff\n"},{"id":"262213","messageId":"20150527070838.GA6873@peff.net","threadId":"39434","inReplyTo":"20150527061818.GA885@peff.net","subject":"Re: [RFC/WIP PATCH 00/11] Protocol version 2, again!","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T07:08:38Z","receivedAt":"2015-05-27T07:08:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2015 at 02:18:18AM -0400, Jeff King wrote:\n\n> > The new protocol works just like the old protocol, except for\n> > the capabilities negotiation being before any exchange of real data.\n> \n> I like this approach. [...]\n\nSo now I've read through all the patches. I still like it. :)\n\nThere's a lot of work to be done still, but I think this is a great\nstart. Thanks for getting the ball rolling.\n\n-Peff\n"},{"id":"262214","messageId":"20150527071214.GA6898@peff.net","threadId":"39434","inReplyTo":"1432677675-5118-11-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 10/11] t5544: add a test case for the new protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T07:12:15Z","receivedAt":"2015-05-27T07:12:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 26, 2015 at 03:01:14PM -0700, Stefan Beller wrote:\n\n> Signed-off-by: Stefan Beller \n> ---\n> t/t5544-fetch-2.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 40 insertions(+)\n> create mode 100755 t/t5544-fetch-2.sh\n\nObviously we are not there yet, but a fun test will be to globally bump\nthe transport version to \"2\" and try to run the test suite.\n\nWe will also want to test interoperation between v1 and v2. Though the\n_best_ test of that is not hitting a v2 server configured for v1, but\nhitting an actual older version of git. Which is outside the scope of\nthe current test suite, as it always operates on a single version.\n\nIt might be nice to have a separate test suite for doing\ninteroperability, that knows how to build various versions (there's\nalready some prior art in t/perf). I know this isn't the first time this\nconcept has come up.\n\n-Peff\n"},{"id":"262240","messageId":"CAPig+cQUtjVbdF5eEo0EH-QXz4q_UqOtoFZ5DcTNzceCxWLS=A@mail.gmail.com","threadId":"39434","inReplyTo":"20150527065030.GE885@peff.net","subject":"Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-27T17:19:39Z","receivedAt":"2015-05-27T17:19:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 27, 2015 at 2:50 AM, Jeff King wrote:\n> On Tue, May 26, 2015 at 11:25:05PM -0400, Eric Sunshine wrote:\n>\n>> > + len = packet_read(in, &src_buf, &src_len,\n>> > + packet_buffer, sizeof(packet_buffer),\n>> > + PACKET_READ_GENTLE_ON_EOF |\n>> > + PACKET_READ_CHOMP_NEWLINE);\n>> > + if (len < 0)\n>> > + die_initial_contact(0);\n>> > +\n>> > + if (!len)\n>> > + break;\n>> > +\n>> > + if (len > 4 && skip_prefix(line, \"ERR \", &arg))\n>>\n>> The 'len > 4' check is needed because there's no guarantee that 'line'\n>> is NUL-terminated. Correct?\n>\n> I think this was just blindly copied from get_remote_heads(). And I\n> think that code was being overly paranoid. Ever since f3a3214 (Make\n> send/receive-pack be closer to doing something interesting, 2005-06-29),\n> the pkt-line reader will add an extra NUL to the buffer to ease cases\n> like this.\n\nThanks. I had started digging into packet_read() to determine whether\nit guaranteed NUL-termination, but didn't get far enough to decide. I\nagree that if NUL-termination is guaranteed, then the 'len > 4' check\nis superfluous (and confusing, which is why it caught my attention in\nthe first place).\n"},{"id":"262241","messageId":"CAPig+cSFEN+V0668FPDM1jY2KdW_nVaEn7+AOWJj_KwUU_UVPw@mail.gmail.com","threadId":"39434","inReplyTo":"20150527063558.GB885@peff.net","subject":"Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-27T17:30:28Z","receivedAt":"2015-05-27T17:30:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 27, 2015 at 2:35 AM, Jeff King wrote:\n> On Tue, May 26, 2015 at 03:01:08PM -0700, Stefan Beller wrote:\n>> +static void send_capabilities(void)\n>> +{\n>> + char buf[100];\n>> +\n>> + while (next_capability(buf))\n>> + packet_write(1, \"capability:%s\\n\", buf);\n>\n> Like Eric, I find the whole next_capability thing a little ugly. His\n> suggestion to pass in the parsing state is an improvement, but I wonder\n> why we need to parse at all. Can we keep the capabilities as:\n>\n> const char *capabilities[] = {\n> \"multi_ack\",\n> \"thin-pack\",\n> ... etc ...\n> };\n>\n> and then loop over the array?\n\nYes, that would be much nicer. I also had this in mind but didn't know\nif there was a strong reason for the capabilities to be shoehorned\ninto a single string as they are currently.\n"},{"id":"262242","messageId":"CAGZ79kYaoViDrz9TKXWebif4mfyUjzJ6b3id8ozTqtwsmjAC1A@mail.gmail.com","threadId":"39434","inReplyTo":"20150527063558.GB885@peff.net","subject":"Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-27T17:40:37Z","receivedAt":"2015-05-27T17:40:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 26, 2015 at 11:35 PM, Jeff King wrote:\n> On Tue, May 26, 2015 at 03:01:08PM -0700, Stefan Beller wrote:\n>\n>> --- a/upload-pack.c\n>> +++ b/upload-pack.c\n>> @@ -716,10 +716,47 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)\n>> strbuf_addf(buf, \" symref=%s:%s\", item->string, (char *)item->util);\n>> }\n>>\n>> -static const char *advertise_capabilities = \"multi_ack thin-pack side-band\"\n>> +static char *advertise_capabilities = \"multi_ack thin-pack side-band\"\n>> \" side-band-64k ofs-delta shallow no-progress\"\n>> \" include-tag multi_ack_detailed\";\n>\n> If we are upload-pack-2, should we advertise that in the capabilities? I\n> think it may make things easier later if we try to provide some\n> opportunistic out-of-band data. E.g., if see tell git-daemon:\n>\n> git-upload-pack repo\\0host=whatever\\0\\0version=2\n>\n> how do we know whether version=2 was understood and kicked us into v2\n> mode, versus an old server that ignored it?\n\nSo in my vision we would call git-upload-pack-2 instead of having a \"version=2\".\nand if git-upload-pack-2 doesn't exist, the whole conversation is\nover, the client\nit is left to make up some good error message or retry version 1.\n\nBut I think advertising both which versions the server could deal\nwith, as well as\nthe currently expected version is a good thing.\n\ncapability: can_speak=1,2\ncapability: speaking_now=2\n\n>\n> It also just makes the protocol more self-describing; from the\n> conversation you can see which version is in use.\n>\n>> +static void send_capabilities(void)\n>> +{\n>> + char buf[100];\n>> +\n>> + while (next_capability(buf))\n>> + packet_write(1, \"capability:%s\\n\", buf);\n>\n> Having a static-sized buffer and then passing it to a function which has\n> no idea of the buffer size seems like a recipe for a buffer overflow. :)\n\nYes. All patches I proposed are very brittle. My first goal was to have the\nlast test passing (an actual fetch with version 2). Now I will start looking at\nmaking things robust, as by now you all seem to not disagree totally.\n\n>\n> You are fine here because you are parsing the hard-coded capabilities\n> string, and we know 100 is enough there. But it's nice to avoid such\n> magic.\n>\n> Like Eric, I find the whole next_capability thing a little ugly. His\n> suggestion to pass in the parsing state is an improvement, but I wonder\n> why we need to parse at all. Can we keep the capabilities as:\n>\n> const char *capabilities[] = {\n> \"multi_ack\",\n> \"thin-pack\",\n> ... etc ...\n> };\n>\n> and then loop over the array? The v1 writer will need to be modified,\n> but it should be fairly straightforward (loop and add them to the buffer\n> rather than dumping the whole string).\n\nThanks for the design guidance! I'll change that.\n\n>\n> Also, do we need the capability noise-word?\n\nI thought it opens up a new possible door in the future.\nAs we ignore anything not starting with \"capability\" for now, you\ncould introduce\nyour foo and bar ping pong easily and still be version 2 compatible.\n\nS: capability: thin\nS: capability: another-capability\nS: ping-pong foo\nS: done\nC: (not having understood ping-pong) just answering with capability: thin\nC: done, let's proceed to refs advertisement\n\nThe alternative client would do:\n\nC: ping-pong: foo-data1a\nS: ping-pong: foo-data1b\nC: ping-pong: foo-data2a\nC: capability: thin\n...\n\n> They all have it, except\n> for:\n>\n>> + packet_write(1, \"agent:%s\\n\", git_user_agent_sanitized());\n>\n> But isn't that basically a capability, too (I agree it is stretching the\n> definition of \"capability\", but I think that ship has sailed; the\n> client's response is not \"I support this, too\" but \"I want to enable\n> this\").\n>\n> IOW, is there a reason that the initial conversation is not just:\n>\n> pkt-line(\"multi_ack\");\n> pkt-line(\"thin-pack\");\n> ...\n> pkt-line(\"agent=v2.5.0\");\n> pkt-line(0000);\n>\n> We already have framing for each capability due to the use of pkt-line.\n> The \"capability:\" is just one more thing the client has to parse past.\n> The keys are already unique up to any \"=\", so I don't think there is any\n> ambiguity (e.g., we don't care about \"capability:agent\"; we have already\n> assigned a meaning to the term \"agent\", and will never introduce a\n> standalone capability with the same name).\n\nIt looks clearer to me (we're not wasting band-width), maybe this ping pong\nthing can be part of the capabilities announcement too, so we're good this way.\n\n>\n> Likewise, if we introduce new protocol items here, the client should\n> either ignore them (if it does not understand them) or know what to do\n> with them.\n>\n>> +static void receive_capabilities(void)\n>> +{\n>> + int done = 0;\n>> + while (1) {\n>> + char *line = packet_read_line(0, NULL);\n>> + if (!line)\n>> + break;\n>> + if (starts_with(line, \"capability:\"))\n>> + parse_features(line + strlen(\"capability:\"));\n>> + }\n>\n> Use:\n>\n> const char *cap;\n> if (skip_prefix(line, \"capability:\", &cap))\n> parse_features(cap);\n>\n> Or better yet, if you take my suggestion above:\n>\n> parse_features(line);\n\nwill do.\n\n>\n> :)\n>\n>> +static int endswith(const char *teststring, const char *ending)\n>> +{\n>> + int slen = strlen(teststring);\n>> + int elen = strlen(ending);\n>> + return !strcmp(teststring + slen - elen, ending);\n>> +}\n>\n> Eric mentioned the underflow problems here, but I wonder even more:\n> what's wrong with the global ends_with() that we already provide?\n\nI was missing knowledge we have that, and apparently I was thinking it's\nfaster to come up with my own version than to look for it. :)\n\n>\n> -Peff\n"},{"id":"262248","messageId":"CAGZ79kZosnaNv-FccG2pni2JkQdJ-QNtfHz82U+4FyYKrVACqQ@mail.gmail.com","threadId":"39434","inReplyTo":"20150527063925.GC885@peff.net","subject":"Re: [RFC/WIP PATCH 05/11] transport: add infrastructure to support a protocol version number","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-27T19:01:50Z","receivedAt":"2015-05-27T19:01:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 26, 2015 at 11:39 PM, Jeff King wrote:\n> On Tue, May 26, 2015 at 03:01:09PM -0700, Stefan Beller wrote:\n>\n>> + OPT_STRING('y', \"transport-version\", &transport_version,\n>> + N_(\"transport-version\"),\n>> + N_(\"specify transport version to be used\")),\n>\n> Interesting choice for the short option (\"-v\" would be nice, but\n> obviously it is taken). Do we want to delay on claiming the\n> short-and-sweet 'y' until we are sure this is something people will use\n> a lot? In an ideal world, it is not (i.e., auto-upgrade and other tricks\n> become good enough that nobody bothers to specify it manually).\n\nI made the choice this way:\n\"Oh crap! -v is taken. so which letter is most likely not taken, so I\ncan move on?\"\n\nSo if you have any other proposal with actual reasons I'd switch in a\nheart beat.\nI figured the -y is good to testing and debugging, but in an ideal\nworld we don't\nwant people messing with the transport option because of automatic upgrades\nas you said.\n\nOr do you rather hint on dropping the short option at all, and just having NULL\nin the field?\n\n>\n> -Peff\n"},{"id":"262247","messageId":"xmqq617dn8eq.fsf@gitster.dls.corp.google.com","threadId":"39434","inReplyTo":"20150527063925.GC885@peff.net","subject":"Re: [RFC/WIP PATCH 05/11] transport: add infrastructure to support a protocol version number","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-27T19:10:37Z","receivedAt":"2015-05-27T19:10:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, May 26, 2015 at 03:01:09PM -0700, Stefan Beller wrote:\n>\n>> +\tOPT_STRING('y', \"transport-version\", &transport_version,\n>> +\t\t N_(\"transport-version\"),\n>> +\t\t N_(\"specify transport version to be used\")),\n>\n> Interesting choice for the short option (\"-v\" would be nice, but\n> obviously it is taken). Do we want to delay on claiming the\n> short-and-sweet 'y' until we are sure this is something people will use\n> a lot? In an ideal world, it is not (i.e., auto-upgrade and other tricks\n> become good enough that nobody bothers to specify it manually).\n\nYes, just stuff 0 (not NULL but NUL) there; unless we have a very\ngood reason to believe that the option will be used every day to\ntoggle per invocation settings, we shouldn't squat on a short and\nsweet single letter.\n"},{"id":"262260","messageId":"20150527200924.GA14309@peff.net","threadId":"39434","inReplyTo":"CAPig+cQUtjVbdF5eEo0EH-QXz4q_UqOtoFZ5DcTNzceCxWLS=A@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T20:09:25Z","receivedAt":"2015-05-27T20:09:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2015 at 01:19:39PM -0400, Eric Sunshine wrote:\n\n> >> The 'len > 4' check is needed because there's no guarantee that 'line'\n> >> is NUL-terminated. Correct?\n> >\n> > I think this was just blindly copied from get_remote_heads(). And I\n> > think that code was being overly paranoid. Ever since f3a3214 (Make\n> > send/receive-pack be closer to doing something interesting, 2005-06-29),\n> > the pkt-line reader will add an extra NUL to the buffer to ease cases\n> > like this.\n> \n> Thanks. I had started digging into packet_read() to determine whether\n> it guaranteed NUL-termination, but didn't get far enough to decide. I\n> agree that if NUL-termination is guaranteed, then the 'len > 4' check\n> is superfluous (and confusing, which is why it caught my attention in\n> the first place).\n\nYeah, agreed that it should be cleaned up. Interestingly, if you dig on\nthat line, I've touched it several times myself and never noticed this.\n:)\n\n-Peff\n"},{"id":"262262","messageId":"20150527201433.GB14309@peff.net","threadId":"39434","inReplyTo":"CAPig+cSFEN+V0668FPDM1jY2KdW_nVaEn7+AOWJj_KwUU_UVPw@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T20:14:33Z","receivedAt":"2015-05-27T20:14:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2015 at 01:30:28PM -0400, Eric Sunshine wrote:\n\n> > Like Eric, I find the whole next_capability thing a little ugly. His\n> > suggestion to pass in the parsing state is an improvement, but I wonder\n> > why we need to parse at all. Can we keep the capabilities as:\n> >\n> > const char *capabilities[] = {\n> > \"multi_ack\",\n> > \"thin-pack\",\n> > ... etc ...\n> > };\n> >\n> > and then loop over the array?\n> \n> Yes, that would be much nicer. I also had this in mind but didn't know\n> if there was a strong reason for the capabilities to be shoehorned\n> into a single string as they are currently.\n\nI don't think there is a good reason, beyond it being the simplest thing\nfor the current code to work. But as you can see from the existing\npacket_write() in upload-pack, it's already going through some\ncontortions to handle optional capabilities (i.e., \"capabilities\" is by\nno means the full list anymore).\n\nDoing it item by item will mean we can't use a single packet_write() in\nthe v1 code, but it's OK to format into a buffer here (we already need\nsuch a buffer for format_symref_info anyway).\n\n-Peff\n"},{"id":"262263","messageId":"20150527201748.GC14309@peff.net","threadId":"39434","inReplyTo":"CAGZ79kZosnaNv-FccG2pni2JkQdJ-QNtfHz82U+4FyYKrVACqQ@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 05/11] transport: add infrastructure to support a protocol version number","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T20:17:48Z","receivedAt":"2015-05-27T20:17:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2015 at 12:01:50PM -0700, Stefan Beller wrote:\n\n> > Interesting choice for the short option (\"-v\" would be nice, but\n> > obviously it is taken). Do we want to delay on claiming the\n> > short-and-sweet 'y' until we are sure this is something people will use\n> > a lot? In an ideal world, it is not (i.e., auto-upgrade and other tricks\n> > become good enough that nobody bothers to specify it manually).\n> [...]\n> Or do you rather hint on dropping the short option at all, and just having NULL\n> in the field?\n\nYes, that's what I was hinting.\n\n-Peff\n"},{"id":"262266","messageId":"20150527203451.GD14309@peff.net","threadId":"39434","inReplyTo":"CAGZ79kYaoViDrz9TKXWebif4mfyUjzJ6b3id8ozTqtwsmjAC1A@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T20:34:52Z","receivedAt":"2015-05-27T20:34:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2015 at 10:40:37AM -0700, Stefan Beller wrote:\n\n> > If we are upload-pack-2, should we advertise that in the capabilities? I\n> > think it may make things easier later if we try to provide some\n> > opportunistic out-of-band data. E.g., if see tell git-daemon:\n> >\n> > git-upload-pack repo\\0host=whatever\\0\\0version=2\n> >\n> > how do we know whether version=2 was understood and kicked us into v2\n> > mode, versus an old server that ignored it?\n> \n> So in my vision we would call git-upload-pack-2 instead of having a \"version=2\".\n> and if git-upload-pack-2 doesn't exist, the whole conversation is\n> over, the client\n> it is left to make up some good error message or retry version 1.\n\nI'd like for that to be a starting point for us, and then to be able to\nadd-on \"hints\" to ease the transition path in whatever way we want. We\nmay even not do that in the long run, but I want to leave the door open\nif we can.\n\n> But I think advertising both which versions the server could deal\n> with, as well as\n> the currently expected version is a good thing.\n> \n> capability: can_speak=1,2\n> capability: speaking_now=2\n\nI was thinking just \"speaking_now=2\", but it probably makes sense to do\nboth. I do not think using it to \"downgrade\" will ever be particularly\nuseful (certainly not from v2 to v1, since to understand the flag both\nsides must be v2 in the first place). But advertising that via the v1\nconversation will be a good way to tell the other side that upgrade is\npossible.\n\n> > Also, do we need the capability noise-word?\n> \n> I thought it opens up a new possible door in the future.\n> As we ignore anything not starting with \"capability\" for now, you\n> could introduce\n> your foo and bar ping pong easily and still be version 2 compatible.\n> \n> S: capability: thin\n> S: capability: another-capability\n> S: ping-pong foo\n> S: done\n> C: (not having understood ping-pong) just answering with capability: thin\n> C: done, let's proceed to refs advertisement\n> \n> The alternative client would do:\n> \n> C: ping-pong: foo-data1a\n> S: ping-pong: foo-data1b\n> C: ping-pong: foo-data2a\n> C: capability: thin\n> ...\n\nRight, but I think (and please correct me if there's a case I'm missing)\nthat the behavior is the same whether it is spelled \"ping-pong\" or\n\"capability:ping-pong\". That is, the rule for \"capability:\" is \"if you\ndo not understand it, ignore it and do not mention it in your\ncapabilities; the server is required to assume you were written before\nthat capability was invented\". But that is _also_ the rule for\nping-pong, I think.\n\n> > Eric mentioned the underflow problems here, but I wonder even more:\n> > what's wrong with the global ends_with() that we already provide?\n> \n> I was missing knowledge we have that, and apparently I was thinking it's\n> faster to come up with my own version than to look for it. :)\n\nMakes sense. :)\n\n-Peff\n"},{"id":"262271","messageId":"CAGZ79kb6rsxpiXKg+eaVc+5bgUgPSaq+ocd-UHa=_K+rLxreTQ@mail.gmail.com","threadId":"39434","inReplyTo":"20150527203451.GD14309@peff.net","subject":"Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-27T20:45:55Z","receivedAt":"2015-05-27T20:45:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 27, 2015 at 1:34 PM, Jeff King wrote:\n> On Wed, May 27, 2015 at 10:40:37AM -0700, Stefan Beller wrote:\n>\n>> > If we are upload-pack-2, should we advertise that in the capabilities? I\n>> > think it may make things easier later if we try to provide some\n>> > opportunistic out-of-band data. E.g., if see tell git-daemon:\n>> >\n>> > git-upload-pack repo\\0host=whatever\\0\\0version=2\n>> >\n>> > how do we know whether version=2 was understood and kicked us into v2\n>> > mode, versus an old server that ignored it?\n>>\n>> So in my vision we would call git-upload-pack-2 instead of having a \"version=2\".\n>> and if git-upload-pack-2 doesn't exist, the whole conversation is\n>> over, the client\n>> it is left to make up some good error message or retry version 1.\n>\n> I'd like for that to be a starting point for us, and then to be able to\n> add-on \"hints\" to ease the transition path in whatever way we want. We\n> may even not do that in the long run, but I want to leave the door open\n> if we can.\n>\n>> But I think advertising both which versions the server could deal\n>> with, as well as\n>> the currently expected version is a good thing.\n>>\n>> capability: can_speak=1,2\n>> capability: speaking_now=2\n>\n> I was thinking just \"speaking_now=2\", but it probably makes sense to do\n> both. I do not think using it to \"downgrade\" will ever be particularly\n> useful (certainly not from v2 to v1, since to understand the flag both\n> sides must be v2 in the first place). But advertising that via the v1\n> conversation will be a good way to tell the other side that upgrade is\n> possible.\n\nIf for some reason we discover a flaw in the current version, which\nmakes it unusable\n(a buffer overflow?, some stupid abuse which makes the capability list huge),\nyou may want to force downgrading (and in the very distant future when we are\ncurrent on version 4 and have dropped version 1 already, you can only downgrade\nto 2 and 3, so I can see value in it.\n\nAnother idea to make it all more future proof:\n\"capability: speaking_now=2\" must be sent as the first line, so then\nyou can adapt\non the client side easily for which version you are listening.\n\n>\n>> > Also, do we need the capability noise-word?\n>>\n>> I thought it opens up a new possible door in the future.\n>> As we ignore anything not starting with \"capability\" for now, you\n>> could introduce\n>> your foo and bar ping pong easily and still be version 2 compatible.\n>>\n>> S: capability: thin\n>> S: capability: another-capability\n>> S: ping-pong foo\n>> S: done\n>> C: (not having understood ping-pong) just answering with capability: thin\n>> C: done, let's proceed to refs advertisement\n>>\n>> The alternative client would do:\n>>\n>> C: ping-pong: foo-data1a\n>> S: ping-pong: foo-data1b\n>> C: ping-pong: foo-data2a\n>> C: capability: thin\n>> ...\n>\n> Right, but I think (and please correct me if there's a case I'm missing)\n> that the behavior is the same whether it is spelled \"ping-pong\" or\n> \"capability:ping-pong\". That is, the rule for \"capability:\" is \"if you\n> do not understand it, ignore it and do not mention it in your\n> capabilities; the server is required to assume you were written before\n> that capability was invented\". But that is _also_ the rule for\n> ping-pong, I think.\n\nThe rules are the same, right. But the allowed characters are limited\n(in theory)\nas the regular expressions given for the capabilities don't allow for\nbinary data\nfor example, but only well formed ASCII text, space separated.\nThe \"ping-pong\" keyword could introduce a binary stream there\nincluding line feeds. (Today it sounds like a stupid idea though)\n\n>\n>> > Eric mentioned the underflow problems here, but I wonder even more:\n>> > what's wrong with the global ends_with() that we already provide?\n>>\n>> I was missing knowledge we have that, and apparently I was thinking it's\n>> faster to come up with my own version than to look for it. :)\n>\n> Makes sense. :)\n>\n> -Peff\n"},{"id":"262282","messageId":"20150527214616.GA23259@peff.net","threadId":"39434","inReplyTo":"CAGZ79kb6rsxpiXKg+eaVc+5bgUgPSaq+ocd-UHa=_K+rLxreTQ@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T21:46:17Z","receivedAt":"2015-05-27T21:46:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2015 at 01:45:55PM -0700, Stefan Beller wrote:\n\n> > Right, but I think (and please correct me if there's a case I'm\n> > missing) that the behavior is the same whether it is spelled\n> > \"ping-pong\" or \"capability:ping-pong\". That is, the rule for\n> > \"capability:\" is \"if you do not understand it, ignore it and do not\n> > mention it in your capabilities; the server is required to assume\n> > you were written before that capability was invented\". But that is\n> > _also_ the rule for ping-pong, I think.\n> \n> The rules are the same, right. But the allowed characters are limited\n> (in theory) as the regular expressions given for the capabilities\n> don't allow for binary data for example, but only well formed ASCII\n> text, space separated. The \"ping-pong\" keyword could introduce a\n> binary stream there including line feeds. (Today it sounds like a\n> stupid idea though)\n\nDo we need that restriction? IOW, as long as we parse from the start of\nthe packet and give up as soon as we realize it is not a thing we\nunderstand, I do not think anybody is hurt by the contents of the item.\n\nE.g., if an old client sees:\n\n 00XXping-pong=\n\nIt knows:\n\n 1. The item starts with ping-pong; we don't know what that is, so we\n never even bother looking at the binary goo.\n\n 2. It's in a pkt-line. We read to the end of the packet line and throw\n the rest of the data away. Now we're synchronized to read the next\n item.\n\nOf course, \"ping-pong\" may also introduce another phase to the protocol\nwhich is not encapsulated in pkt-lines (e.g., if the data is too big to\nfit right inside the capability pkt-line, or if it needs a lot of back\nand forth like ref negotiation). But then we only proceed to that\nphase if both sides have said \"I understand ping-pong\".\n\nSo I think we are capable of boot-strapping just about anything using\ncapability negotiation (with the exception of fixing the capability\nnegotiation itself; but even that, we can bootstrap a second more\nintensive capability negotiation using a capability in the initial\nlist).\n\n-Peff\n"},{"id":"262448","messageId":"CAGZ79kbm5FCjMH-vV+_ByNvAdOD2idVtd8-bizEJpZpdE9cGjQ@mail.gmail.com","threadId":"39434","inReplyTo":"20150527064521.GD885@peff.net","subject":"Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-29T19:39:35Z","receivedAt":"2015-05-29T19:39:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 26, 2015 at 11:45 PM, Jeff King wrote:\n> On Tue, May 26, 2015 at 03:01:10PM -0700, Stefan Beller wrote:\n>\n>> +void get_remote_capabilities(int in, char *src_buf, size_t src_len)\n>> +{\n>> + struct strbuf capabilities_string = STRBUF_INIT;\n>> + for (;;) {\n>> + int len;\n>> + char *line = packet_buffer;\n>> + const char *arg;\n>> +\n>> + len = packet_read(in, &src_buf, &src_len,\n>> + packet_buffer, sizeof(packet_buffer),\n>> + PACKET_READ_GENTLE_ON_EOF |\n>> + PACKET_READ_CHOMP_NEWLINE);\n>> + if (len < 0)\n>> + die_initial_contact(0);\n>> +\n>> + if (!len)\n>> + break;\n>> +\n>> + if (len > 4 && skip_prefix(line, \"ERR \", &arg))\n>> + die(\"remote error: %s\", arg);\n>> +\n>> + if (starts_with(line, \"capability:\")) {\n>> + strbuf_addstr(&capabilities_string, line + strlen(\"capability:\"));\n>> + strbuf_addch(&capabilities_string, ' ');\n>> + }\n>> + }\n>\n> I think this is the reverse case of next_capabilities in the upload-pack\n> side, so I'll make the reverse suggestion. :) Would it make things nicer\n> if both v1 and v2 parsed the capabilities into a string_list?\n\nOk, I'll do that. Though this makes future enhancements a bit uneasy.\nSay we want to transport a message by the server admins, this might be\nthe right place to do.\n\n if (starts_with(\"message\"))\n fprintf(stderr, ....\n\nOf course we can later add this in the future, but it would break older\nclients (clients as of this patch series). That's why I like the idea of\nadding a prefix here. Maybe just a \"c:\" as an abbreviation for capability.\nBut now making it short and concise makes it painful in the future when\nwe want to transport anything else apart from capabilities in this phase\nof the protocol exchange. Of course we could have a capability \"server-message\"\nindicating that after the capabilities we'll have a dedicated message to be\ndisplayed to the user.\n\nYeah well that should do.\n\nI'll just parse in a string_list for now.\n\n>\n>> + free(server_capabilities);\n>> + server_capabilities = xmalloc(capabilities_string.len + 1);\n>> + server_capabilities = strbuf_detach(&capabilities_string, NULL);\n>\n> Is this xmalloc line left over? The strbuf_detach will write over it.\n\nNo, I wasn't reading the fine documentation and assuming things I\nshould not.\n\n>\n>> + strbuf_release(&capabilities_string);\n>\n> No need to release if we just detached.\n>\n>> +int request_capabilities(int out)\n>> +{\n>> + fprintf(stderr, \"request_capabilities\\n\");\n>\n> Debug cruft, I presume.\n>\n>> + // todo: send our capabilities\n>> + packet_write(out, \"capability:foo\");\n>\n> No C99 comments. But I think this is just a placeholder. :)\n>\n> -Peff\n"},{"id":"262452","messageId":"xmqqsiafazr7.fsf@gitster.dls.corp.google.com","threadId":"39434","inReplyTo":"1432677675-5118-12-git-send-email-sbeller@google.com","subject":"Re: [RFC/WIP PATCH 11/11] Document protocol version 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-29T20:35:08Z","receivedAt":"2015-05-29T20:35:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> @@ -1,11 +1,11 @@\n> Packfile transfer protocols\n> ===========================\n> \n> -Git supports transferring data in packfiles over the ssh://, git:// and\n> +Git supports transferring data in packfiles over the ssh://, git://, http:// and\n\nWhen you have chance, can you do things like this, which is a clear\nimprovement of the current document even if we never had v2, as\nseparate patches?\n\n> +Capability discovery (v2)\n> +-------------------------\n> ...\n> + capability-list = *(capability) [agent LF] flush-pkt\n> + capability = PKT-LINE(\"capability:\" keyvaluepair LF)\n> + agent = keyvaluepair LF\n> + keyvaluepair = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n\nWhat is the \"=\" doing there? If you meant to cover things like\n\"lang=en\" with this, I do not think it is a good idea. Rather, it\nshould be more like this:\n\n\tcapability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\") [ \"=\" value ]\n\tvalue = 0*( any octet other than LF, NUL )\n\nin order to leave us wiggle room to have more than very limited\nsubset of US-ASCII in 'value'. I suspect that we may want to allow\nanything other than LF (unlike v1 that allowed anything other than\nSP and LF).\n\n> + LC_ALPHA = %x61-7A\n> +----\n> +\n> +The client MUST ignore any data on pkt-lines starting with anything\n> +different than \"capability\" for future ease of extension.\n> +\n> +The client MUST NOT ask for capabilities the server did not say it\n> +supports. The server MUST diagnose and abort if capabilities it does\n> +not understand was requested. The server MUST NOT ignore capabilities\n> +that client requested and server advertised. As a consequence of these\n> +rules, server MUST NOT advertise capabilities it does not understand.\n\nI think it was already discussed that we shouldn't do the\n\"capability:\" and \"cap:\" prefixes in reviews of earlier parts, so\nthe details of this part would be updated?\n\n> @@ -154,10 +203,14 @@ If HEAD is a valid ref, HEAD MUST appear as the first advertised\n> ref. If HEAD is not a valid ref, HEAD MUST NOT appear in the\n> advertisement list at all, but other refs may still appear.\n> \n> -The stream MUST include capability declarations behind a NUL on the\n> -first ref. The peeled value of a ref (that is \"ref^{}\") MUST be\n> -immediately after the ref itself, if presented. A conforming server\n> -MUST peel the ref if it's an annotated tag.\n> +In version 1 the stream MUST include capability declarations behind\n> +a NUL on the first ref. The peeled value of a ref (that is \"ref^{}\")\n> +MUST be immediately after the ref itself, if presented. A conforming\n> +server MUST peel the ref if it's an annotated tag.\n> +\n> +In version 2 the capabilities are already negotiated, so the first ref\n> +MUST NOT be followed by any capability advertisement, but it should be\n> +treated as any other refs advertising line.\n\nSensible.\n\n> @@ -178,13 +231,28 @@ MUST peel the ref if it's an annotated tag.\n> shallow = PKT-LINE(\"shallow\" SP obj-id)\n> \n> capability-list = capability *(SP capability)\n> - capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\")\n> + capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n\nDitto.\n\nThanks.\n"},{"id":"262454","messageId":"CAGZ79kbELvZ6otnNwGHsgiC9EjgS2vrDGU2KCgF2c_Azm=-rWg@mail.gmail.com","threadId":"39434","inReplyTo":"xmqqsiafazr7.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/WIP PATCH 11/11] Document protocol version 2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-29T21:36:26Z","receivedAt":"2015-05-29T21:36:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 29, 2015 at 1:35 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> @@ -1,11 +1,11 @@\n>> Packfile transfer protocols\n>> ===========================\n>>\n>> -Git supports transferring data in packfiles over the ssh://, git:// and\n>> +Git supports transferring data in packfiles over the ssh://, git://, http:// and\n>\n> When you have chance, can you do things like this, which is a clear\n> improvement of the current document even if we never had v2, as\n> separate patches?\n\nwill do.\n\n>\n>> +Capability discovery (v2)\n>> +-------------------------\n>> ...\n>> + capability-list = *(capability) [agent LF] flush-pkt\n>> + capability = PKT-LINE(\"capability:\" keyvaluepair LF)\n>> + agent = keyvaluepair LF\n>> + keyvaluepair = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n>\n> What is the \"=\" doing there? If you meant to cover things like\n> \"lang=en\" with this, I do not think it is a good idea. Rather, it\n> should be more like this:\n>\n> capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\") [ \"=\" value ]\n> value = 0*( any octet other than LF, NUL )\n>\n> in order to leave us wiggle room to have more than very limited\n> subset of US-ASCII in 'value'. I suspect that we may want to allow\n> anything other than LF (unlike v1 that allowed anything other than\n> SP and LF).\n\nCurrently we can do a = as part of the line after the first ref, such as\n\n symref=HEAD:refs/heads/master agent=git/2:2.4.0\n\nso I thought we want to keep this. And below I just corrected what I thought\nwas a difference between documentation and implementation.\n\n>\n>> + LC_ALPHA = %x61-7A\n>> +----\n>> +\n>> +The client MUST ignore any data on pkt-lines starting with anything\n>> +different than \"capability\" for future ease of extension.\n>> +\n>> +The client MUST NOT ask for capabilities the server did not say it\n>> +supports. The server MUST diagnose and abort if capabilities it does\n>> +not understand was requested. The server MUST NOT ignore capabilities\n>> +that client requested and server advertised. As a consequence of these\n>> +rules, server MUST NOT advertise capabilities it does not understand.\n>\n> I think it was already discussed that we shouldn't do the\n> \"capability:\" and \"cap:\" prefixes in reviews of earlier parts, so\n> the details of this part would be updated?\n\nsure\n\n>\n>> @@ -154,10 +203,14 @@ If HEAD is a valid ref, HEAD MUST appear as the first advertised\n>> ref. If HEAD is not a valid ref, HEAD MUST NOT appear in the\n>> advertisement list at all, but other refs may still appear.\n>>\n>> -The stream MUST include capability declarations behind a NUL on the\n>> -first ref. The peeled value of a ref (that is \"ref^{}\") MUST be\n>> -immediately after the ref itself, if presented. A conforming server\n>> -MUST peel the ref if it's an annotated tag.\n>> +In version 1 the stream MUST include capability declarations behind\n>> +a NUL on the first ref. The peeled value of a ref (that is \"ref^{}\")\n>> +MUST be immediately after the ref itself, if presented. A conforming\n>> +server MUST peel the ref if it's an annotated tag.\n>> +\n>> +In version 2 the capabilities are already negotiated, so the first ref\n>> +MUST NOT be followed by any capability advertisement, but it should be\n>> +treated as any other refs advertising line.\n>\n> Sensible.\n>\n>> @@ -178,13 +231,28 @@ MUST peel the ref if it's an annotated tag.\n>> shallow = PKT-LINE(\"shallow\" SP obj-id)\n>>\n>> capability-list = capability *(SP capability)\n>> - capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\")\n>> + capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n>\n> Ditto.\n>\n> Thanks.\n"},{"id":"262455","messageId":"xmqqk2vraw6p.fsf@gitster.dls.corp.google.com","threadId":"39434","inReplyTo":"CAGZ79kbELvZ6otnNwGHsgiC9EjgS2vrDGU2KCgF2c_Azm=-rWg@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 11/11] Document protocol version 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-29T21:52:14Z","receivedAt":"2015-05-29T21:52:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>>> +Capability discovery (v2)\n>>> +-------------------------\n>>> ...\n>>> + capability-list = *(capability) [agent LF] flush-pkt\n>>> + capability = PKT-LINE(\"capability:\" keyvaluepair LF)\n>>> + agent = keyvaluepair LF\n>>> + keyvaluepair = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n>>\n>> What is the \"=\" doing there? If you meant to cover things like\n>> \"lang=en\" with this, I do not think it is a good idea. Rather, it\n>> should be more like this:\n>>\n>> capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\") [ \"=\" value ]\n>> value = 0*( any octet other than LF, NUL )\n>>\n>> in order to leave us wiggle room to have more than very limited\n>> subset of US-ASCII in 'value'. I suspect that we may want to allow\n>> anything other than LF (unlike v1 that allowed anything other than\n>> SP and LF).\n>\n> Currently we can do a = as part of the line after the first ref, such as\n>\n> symref=HEAD:refs/heads/master agent=git/2:2.4.0\n>\n> so I thought we want to keep this.\n\nI do not understand that statement.\n\nCapability exchange in v2 is one packet per cap, so the above\nexample would be expressed as:\n\n\tsymref=HEAD:refs/heads/master\n agent=git/2:2.4.0\n\nright? Your \"keyvaluepair\" is limited to [a-z0-9-_=]*, and neither\nof the above two can be expressed with that, which was why I said\nyou need two different set of characters before and after \"=\". Left\nhand side of \"=\" is tightly limited and that is OK. Right hand side\nmay contain characters like ':', '.' and '/', so your alphabet need\nto be more lenient, even in v1 (which I would imagine would be \"any\noctet other than SP, LF and NUL\").\n"},{"id":"262456","messageId":"20150529220811.GA15678@peff.net","threadId":"39434","inReplyTo":"CAGZ79kbm5FCjMH-vV+_ByNvAdOD2idVtd8-bizEJpZpdE9cGjQ@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-29T22:08:11Z","receivedAt":"2015-05-29T22:08:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 29, 2015 at 12:39:35PM -0700, Stefan Beller wrote:\n\n> > I think this is the reverse case of next_capabilities in the upload-pack\n> > side, so I'll make the reverse suggestion. :) Would it make things nicer\n> > if both v1 and v2 parsed the capabilities into a string_list?\n> \n> Ok, I'll do that. Though this makes future enhancements a bit uneasy.\n> Say we want to transport a message by the server admins, this might be\n> the right place to do.\n> \n> if (starts_with(\"message\"))\n> fprintf(stderr, ....\n> \n> Of course we can later add this in the future, but it would break older\n> clients (clients as of this patch series). That's why I like the idea of\n> adding a prefix here. Maybe just a \"c:\" as an abbreviation for capability.\n\nI don't understand how that breaks existing clients. Under your scheme,\nthe older client says \"message? That does not start with capability:, so\nI must ignore it\". Without the \"capability:\" flag, it becomes \"message?\nI do not know that type, so I must ignore it\".\n\n-Peff\n"},{"id":"262457","messageId":"20150529222120.GB15678@peff.net","threadId":"39434","inReplyTo":"xmqqk2vraw6p.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/WIP PATCH 11/11] Document protocol version 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-29T22:21:20Z","receivedAt":"2015-05-29T22:21:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 29, 2015 at 02:52:14PM -0700, Junio C Hamano wrote:\n\n> > Currently we can do a = as part of the line after the first ref, such as\n> >\n> > symref=HEAD:refs/heads/master agent=git/2:2.4.0\n> >\n> > so I thought we want to keep this.\n> \n> I do not understand that statement.\n> \n> Capability exchange in v2 is one packet per cap, so the above\n> example would be expressed as:\n> \n> \tsymref=HEAD:refs/heads/master\n> agent=git/2:2.4.0\n> \n> right? Your \"keyvaluepair\" is limited to [a-z0-9-_=]*, and neither\n> of the above two can be expressed with that, which was why I said\n> you need two different set of characters before and after \"=\". Left\n> hand side of \"=\" is tightly limited and that is OK. Right hand side\n> may contain characters like ':', '.' and '/', so your alphabet need\n> to be more lenient, even in v1 (which I would imagine would be \"any\n> octet other than SP, LF and NUL\").\n\nYes. See git_user_agent_sanitized(), for example, which allows basically\nany printable ASCII except for SP.\n\nI think the v2 capabilities do not even need to have that restriction.\nIt can allow arbitrary binary data, because it has an 8bit-clean framing\nmechanism (pkt-lines). Of course, that means such capabilities cannot be\nrepresented in a v1 conversation (whose framing mechanism involves SP\nand NUL). But it's probably acceptable to introduce new capabilities\nwhich are only available in a v2 conversation. Old clients that do not\nunderstand v2 would not understand the capability either. It does\nrequire new clients implementing the capability to _also_ implement v2\nif they have not done so, but I do not mind pushing people in that\ndirection.\n\nThe initial v2 client implementation should probably do a few cautionary\nthings, then:\n\n 1. Do _not_ fold the per-pkt capabilities into a v1 string; that loses\n the robust framing. I suggested string_list earlier, but probably\n we want a list of ptr/len pair, so that it can remain NUL-clean.\n\n 2. Avoid holding on to unknown packets longer than necessary. Some\n capability pkt-lines may be arbitrarily large (up to 64K). If we do\n not understand them during the v2 read of the capabilities, there\n is no point hanging on to them. It's not _wrong_ to do so, but just\n inefficient; if we know that clients will just throw away unknown\n packets, then we can later introduce new packets with large data,\n without worrying about wasting the client's resources.\n\n I suspect it's not that big a deal either way, though. I have no\n plans for sending a bunch of large packets, and anyway network\n bandwidth is probably more precious than client memory.\n\n-Peff\n"},{"id":"262629","messageId":"CAGZ79kZxFnkneixquUijd2yfKBh6+XnYiYzCh8L9Mkourh64Fw@mail.gmail.com","threadId":"39434","inReplyTo":"20150527070838.GA6873@peff.net","subject":"Re: [RFC/WIP PATCH 00/11] Protocol version 2, again!","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-01T17:49:45Z","receivedAt":"2015-06-01T17:49:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 27, 2015 at 12:08 AM, Jeff King wrote:\n> On Wed, May 27, 2015 at 02:18:18AM -0400, Jeff King wrote:\n>\n>> > The new protocol works just like the old protocol, except for\n>> > the capabilities negotiation being before any exchange of real data.\n>>\n>> I like this approach. [...]\n>\n> So now I've read through all the patches. I still like it. :)\n>\n> There's a lot of work to be done still, but I think this is a great\n> start. Thanks for getting the ball rolling.\n>\n> -Peff\n\nThanks for the reviews. I think I got them all covered by now and\nI am pretty happy about the upload-pack part for now.\n\nHowever the client side with builtin/fetch, builtin/fetch-pack, fetch-pack\nis a bit of a mystery to me, as I cannot fully grasp the difference between\n * connect.{h,c}\n * remote.{h.c}\n * transport.{h.c}\nthere. All of it seems to be doing network related stuff, but I have trouble\ngetting the big picture. I am assuming all of these 3 are rather a low level,\nused like a library, though there must be even more hierarchy in there,\nconnect is most low level judging from the header file and used by\nthe other two.\ntransport.h seems to provide the most toplevel library stuff as it includes\nremote.h in its header?\n\nThe problem I am currently trying to tackle, is passing the options through all\nthe layers early on. so in a few places we have code like\n\n switch (version) {\n case 2: /* first talk about capabilities, then get the heads */\n get_remote_capabilities(data->fd[0], NULL, 0);\n select_capabilities();\n request_capabilities(data->fd[1]);\n /* fall through */\n case 1:\n get_remote_heads(data->fd[0], NULL, 0, &refs,\n for_push ? REF_NORMAL : 0,\n &data->extra_have,\n &data->shallow);\n break;\n default:\n die(\"BUG: Transport version %d not supported\", version);\n break;\n }\n\nand the issue I am concerned about is the select_capabilities as well as\nthe request_capabilities function here. The select_capabilities functionality\nis currently residing in the high level parts of the code as it both depends on\nthe advertised server capabilities and on the user input (--use-frotz or config\noptions), so the capability selection is done in fetchpack.c\n\nSo there are 2 routes to go: Either we leave the select_capabilities in the\nupper layers (near the actual high level command, fetch, fetchpack) or we put\nit into the transport layer and just passing in a struct what the user desires.\nAnd when the users desire doesn't meet the server capabilities we die deep down\nin the transport layer.\n\nAny advice on how to proceed there welcome!\n\nThanks,\nStefan\n"},{"id":"262660","messageId":"CAGZ79kaRTLX7eBCOA=yQHVwcN-H-o_aZFfQ1gw7Nx-NC82pbag@mail.gmail.com","threadId":"39434","inReplyTo":"20150529222120.GB15678@peff.net","subject":"Re: [RFC/WIP PATCH 11/11] Document protocol version 2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-01T23:14:15Z","receivedAt":"2015-06-01T23:14:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 29, 2015 at 3:21 PM, Jeff King wrote:\n> On Fri, May 29, 2015 at 02:52:14PM -0700, Junio C Hamano wrote:\n>\n>> > Currently we can do a = as part of the line after the first ref, such as\n>> >\n>> > symref=HEAD:refs/heads/master agent=git/2:2.4.0\n>> >\n>> > so I thought we want to keep this.\n>>\n>> I do not understand that statement.\n>>\n>> Capability exchange in v2 is one packet per cap, so the above\n>> example would be expressed as:\n>>\n>> symref=HEAD:refs/heads/master\n>> agent=git/2:2.4.0\n>>\n>> right? Your \"keyvaluepair\" is limited to [a-z0-9-_=]*, and neither\n>> of the above two can be expressed with that, which was why I said\n>> you need two different set of characters before and after \"=\". Left\n>> hand side of \"=\" is tightly limited and that is OK. Right hand side\n>> may contain characters like ':', '.' and '/', so your alphabet need\n>> to be more lenient, even in v1 (which I would imagine would be \"any\n>> octet other than SP, LF and NUL\").\n\nI think the recent issue with the push certificates shows that having arbitrary\ndata after the = is a bad idea. So we need to be very cautious when to allow\nwhich data after the =.\n\nI'll try split up the patch.\n\n>\n> Yes. See git_user_agent_sanitized(), for example, which allows basically\n> any printable ASCII except for SP.\n>\n> I think the v2 capabilities do not even need to have that restriction.\n> It can allow arbitrary binary data, because it has an 8bit-clean framing\n> mechanism (pkt-lines). Of course, that means such capabilities cannot be\n> represented in a v1 conversation (whose framing mechanism involves SP\n> and NUL). But it's probably acceptable to introduce new capabilities\n> which are only available in a v2 conversation. Old clients that do not\n> understand v2 would not understand the capability either. It does\n> require new clients implementing the capability to _also_ implement v2\n> if they have not done so, but I do not mind pushing people in that\n> direction.\n>\n> The initial v2 client implementation should probably do a few cautionary\n> things, then:\n>\n> 1. Do _not_ fold the per-pkt capabilities into a v1 string; that loses\n> the robust framing. I suggested string_list earlier, but probably\n> we want a list of ptr/len pair, so that it can remain NUL-clean.\n>\n> 2. Avoid holding on to unknown packets longer than necessary. Some\n> capability pkt-lines may be arbitrarily large (up to 64K). If we do\n> not understand them during the v2 read of the capabilities, there\n> is no point hanging on to them. It's not _wrong_ to do so, but just\n> inefficient; if we know that clients will just throw away unknown\n> packets, then we can later introduce new packets with large data,\n> without worrying about wasting the client's resources.\n>\n> I suspect it's not that big a deal either way, though. I have no\n> plans for sending a bunch of large packets, and anyway network\n> bandwidth is probably more precious than client memory.\n\nThat's very sensible thoughts after rereading this email. The version\nI'll be sending out today will not follow those suggestions though. :(\n\n>\n> -Peff\n"},{"id":"262663","messageId":"CAGZ79kYD--dZ_V=_X_Eo31KYTKXt2njuf56XqRRdaTJeLhDjaQ@mail.gmail.com","threadId":"39434","inReplyTo":"CAGZ79kaRTLX7eBCOA=yQHVwcN-H-o_aZFfQ1gw7Nx-NC82pbag@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 11/11] Document protocol version 2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-01T23:40:54Z","receivedAt":"2015-06-01T23:40:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jun 1, 2015 at 4:14 PM, Stefan Beller wrote:\n> On Fri, May 29, 2015 at 3:21 PM, Jeff King wrote:\n>> On Fri, May 29, 2015 at 02:52:14PM -0700, Junio C Hamano wrote:\n>>\n>>> > Currently we can do a = as part of the line after the first ref, such as\n>>> >\n>>> > symref=HEAD:refs/heads/master agent=git/2:2.4.0\n>>> >\n>>> > so I thought we want to keep this.\n>>>\n>>> I do not understand that statement.\n>>>\n>>> Capability exchange in v2 is one packet per cap, so the above\n>>> example would be expressed as:\n>>>\n>>> symref=HEAD:refs/heads/master\n>>> agent=git/2:2.4.0\n>>>\n>>> right? Your \"keyvaluepair\" is limited to [a-z0-9-_=]*, and neither\n>>> of the above two can be expressed with that, which was why I said\n>>> you need two different set of characters before and after \"=\". Left\n>>> hand side of \"=\" is tightly limited and that is OK. Right hand side\n>>> may contain characters like ':', '.' and '/', so your alphabet need\n>>> to be more lenient, even in v1 (which I would imagine would be \"any\n>>> octet other than SP, LF and NUL\").\n>\n> I think the recent issue with the push certificates shows that having arbitrary\n> data after the = is a bad idea. So we need to be very cautious when to allow\n> which data after the =.\n>\n> I'll try split up the patch.\n>\n>>\n>> Yes. See git_user_agent_sanitized(), for example, which allows basically\n>> any printable ASCII except for SP.\n>>\n>> I think the v2 capabilities do not even need to have that restriction.\n>> It can allow arbitrary binary data, because it has an 8bit-clean framing\n>> mechanism (pkt-lines). Of course, that means such capabilities cannot be\n>> represented in a v1 conversation (whose framing mechanism involves SP\n>> and NUL). But it's probably acceptable to introduce new capabilities\n>> which are only available in a v2 conversation. Old clients that do not\n>> understand v2 would not understand the capability either. It does\n>> require new clients implementing the capability to _also_ implement v2\n>> if they have not done so, but I do not mind pushing people in that\n>> direction.\n>>\n>> The initial v2 client implementation should probably do a few cautionary\n>> things, then:\n>>\n>> 1. Do _not_ fold the per-pkt capabilities into a v1 string; that loses\n>> the robust framing. I suggested string_list earlier, but probably\n>> we want a list of ptr/len pair, so that it can remain NUL-clean.\n>>\n>> 2. Avoid holding on to unknown packets longer than necessary. Some\n>> capability pkt-lines may be arbitrarily large (up to 64K). If we do\n>> not understand them during the v2 read of the capabilities, there\n>> is no point hanging on to them. It's not _wrong_ to do so, but just\n>> inefficient; if we know that clients will just throw away unknown\n>> packets, then we can later introduce new packets with large data,\n>> without worrying about wasting the client's resources.\n>>\n>> I suspect it's not that big a deal either way, though. I have no\n>> plans for sending a bunch of large packets, and anyway network\n>> bandwidth is probably more precious than client memory.\n>\n> That's very sensible thoughts after rereading this email. The version\n> I'll be sending out today will not follow those suggestions though. :(\n\nThinking about this further, maybe it is a good idea to restrict the\ncapabilities\nadvertising to alphabetical order?\n\nThe exchange would look like this:\n\nserver:\n for capability in list:\n pkt_write(capability)\n pkt_flush\n\nclient:\n do\n line = recv_pkt()\n parse_capability(line)\n while line != flush\n\nwith parse_capability checking if we know the capability and maybe setting some\ninternal field if we know this capability.\n\nNow if we assume the number of capabilities grows over time a lot (someone may\n\"abuse\" it for a cool feature, similar to the refs currently. Nobody\nthought about\nhaving so many refs in advance)\n\nSo how does parse_capability scale w.r.t the number of capabilities?\nIf parse_capability is just a linear search then it is O(n) and with n\ncapabilities\nthe client faces an O(n^2) computation which is bad. So if we were to require\nalphabetic capabilities, you could internally keep track and the whole operation\nis O(n). I just wonder if this is premature optimization or some thought we need\nto think of.\n\nTo prevent this problem from popping up, it must be easier to\nintroduce a new phase\nafter the capabilities exchange than to just abuse the capabilities\nphase for whatever\nyou plan on doing.\n\nThanks,\nStefan\n\n>\n>>\n>> -Peff\n"},{"id":"262698","messageId":"CACsJy8CGW-Kaq6hkox03w5bQS2=tVNMzShKUBgC6wKX9dkMQGw@mail.gmail.com","threadId":"39434","inReplyTo":"CAGZ79kZxFnkneixquUijd2yfKBh6+XnYiYzCh8L9Mkourh64Fw@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 00/11] Protocol version 2, again!","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-02T10:10:41Z","receivedAt":"2015-06-02T10:10:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jun 2, 2015 at 12:49 AM, Stefan Beller wrote:\n> However the client side with builtin/fetch, builtin/fetch-pack, fetch-pack\n> is a bit of a mystery to me, as I cannot fully grasp the difference between\n> * connect.{h,c}\n> * remote.{h.c}\n> * transport.{h.c}\n> there. All of it seems to be doing network related stuff, but I have trouble\n> getting the big picture. I am assuming all of these 3 are rather a low level,\n> used like a library, though there must be even more hierarchy in there,\n> connect is most low level judging from the header file and used by\n> the other two.\n> transport.h seems to provide the most toplevel library stuff as it includes\n> remote.h in its header?\n\nI think transport.c is there to support non-native protocols (and\nlater on, smart http). So yeah it's basically the API for fetches and\npushes. git-log over those files may reveal their purposes, especially\nthe few first versions of them.\n\n> The problem I am currently trying to tackle, is passing the options through all\n> the layers early on. so in a few places we have code like\n>\n> switch (version) {\n> case 2: /* first talk about capabilities, then get the heads */\n> get_remote_capabilities(data->fd[0], NULL, 0);\n> select_capabilities();\n> request_capabilities(data->fd[1]);\n> /* fall through */\n> case 1:\n> get_remote_heads(data->fd[0], NULL, 0, &refs,\n> for_push ? REF_NORMAL : 0,\n> &data->extra_have,\n> &data->shallow);\n> break;\n> default:\n> die(\"BUG: Transport version %d not supported\", version);\n> break;\n> }\n>\n> and the issue I am concerned about is the select_capabilities as well as\n> the request_capabilities function here. The select_capabilities functionality\n> is currently residing in the high level parts of the code as it both depends on\n> the advertised server capabilities and on the user input (--use-frotz or config\n> options), so the capability selection is done in fetchpack.c\n>\n> So there are 2 routes to go: Either we leave the select_capabilities in the\n> upper layers (near the actual high level command, fetch, fetchpack) or we put\n> it into the transport layer and just passing in a struct what the user desires.\n> And when the users desire doesn't meet the server capabilities we die deep down\n> in the transport layer.\n\nI read the latest re-roll and I think the placement makes sense. You\ncan't put protocol specific at transport level because \"pack protocol\"\nis just one of the supported protocols. There is smart-http (which\nshares a bunch of code, but from transport perspective is a separate\nprotocol), and then user-defined protocols that know nothing about\nthis v2.\n-- \nDuy\n"},{"id":"262744","messageId":"xmqqoaky3ur9.fsf@gitster.dls.corp.google.com","threadId":"39434","inReplyTo":"CAGZ79kaRTLX7eBCOA=yQHVwcN-H-o_aZFfQ1gw7Nx-NC82pbag@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 11/11] Document protocol version 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-02T17:06:18Z","receivedAt":"2015-06-02T17:06:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I think the recent issue with the push certificates shows that having arbitrary\n> data after the = is a bad idea.\n\nI do not think push certificate episode tells any such thing.\n\nIt was about not carefully using cryptography with arbitrary data.\nHow that arbitrary data came to the machinery is irrelevant. We\ncould have used base-64 to encode the server nonce when transferring\nit to the machinery via capability, but then decode it in order to\nplace it in the cerficiate.\n\nDo not restrict transport for such a reason and make legitimate uses\nof the transport unnecessarily harder for later users. What needs\nto be done is to think how the data that was transport was used.\n"},{"id":"262952","messageId":"20150604130902.GA12404@peff.net","threadId":"39434","inReplyTo":"CAGZ79kZxFnkneixquUijd2yfKBh6+XnYiYzCh8L9Mkourh64Fw@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 00/11] Protocol version 2, again!","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-04T13:09:02Z","receivedAt":"2015-06-04T13:09:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 01, 2015 at 10:49:45AM -0700, Stefan Beller wrote:\n\n> However the client side with builtin/fetch, builtin/fetch-pack, fetch-pack\n> is a bit of a mystery to me, as I cannot fully grasp the difference between\n> * connect.{h,c}\n> * remote.{h.c}\n> * transport.{h.c}\n> there. All of it seems to be doing network related stuff, but I have trouble\n> getting the big picture. I am assuming all of these 3 are rather a low level,\n> used like a library, though there must be even more hierarchy in there,\n> connect is most low level judging from the header file and used by\n> the other two.\n> transport.h seems to provide the most toplevel library stuff as it includes\n> remote.h in its header?\n\nconnect.c was originally \"the git protocol\", and was used by send-pack\nand fetch-pack. Other individual programs implemented other transports.\nLater, as the interface moved towards everybody running \"fetch\" and\n\"push\", and those delegating work to the individual transports, we got\ntransport.c, which is an abstract interface for all transports. It\ndelegates actual git-protocol work to the functions in connect.c (or\nbundle work elsewhere, or handles remote-helpers itself).\n\nAnd then remote.c contains routines for dealing with the remotes at a\nlogical level. E.g., which refs to fetch or push, etc.\n\nSo in theory, the flow is something like:\n\n - fetch.c knows \"the user wants to fetch from 'foo'\"\n\n - fetch asks remote.c: \"who is remote 'foo'\"; we get back a URL\n\n - fetch asks transport.c: \"what are the refs for $URL\"\n\n - it turns out to be a git URL. transport.c calls into connect.c to\n implement get_refs_via_connect.\n\n - after fetch gets back the list of refs, it uses routines in remote.c\n to figure out which refs we are interested in, handle refspecs, etc\n\n - now fetch asks transport.c: \"OK, fetch just these refs\"\n\n - transport.c again calls into connect.c to handle the actual fetch\n\nOf course over the years a lot of cruft has grown around all of them. I\nwouldn't be surprised if there are functions which cross these\nabstractions, or other random functions inside each file that do not\nbelong.\n\n> and the issue I am concerned about is the select_capabilities as well as\n> the request_capabilities function here. The select_capabilities functionality\n> is currently residing in the high level parts of the code as it both depends on\n> the advertised server capabilities and on the user input (--use-frotz or config\n> options), so the capability selection is done in fetchpack.c\n> \n> So there are 2 routes to go: Either we leave the select_capabilities in the\n> upper layers (near the actual high level command, fetch, fetchpack) or we put\n> it into the transport layer and just passing in a struct what the user desires.\n> And when the users desire doesn't meet the server capabilities we die deep down\n> in the transport layer.\n\nI think you have to leave it in the fetch-pack code. As you note, it's\nthe place where we know about what the user is asking for and can\nmanipulate the list. And not all transports even support capabilities\nlike this.\n\n-Peff\n"},{"id":"262954","messageId":"20150604131852.GB12404@peff.net","threadId":"39434","inReplyTo":"CAGZ79kYD--dZ_V=_X_Eo31KYTKXt2njuf56XqRRdaTJeLhDjaQ@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 11/11] Document protocol version 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-04T13:18:52Z","receivedAt":"2015-06-04T13:18:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 01, 2015 at 04:40:54PM -0700, Stefan Beller wrote:\n\n> Thinking about this further, maybe it is a good idea to restrict the\n> capabilities advertising to alphabetical order?\n\nThis seems like an unnecessary restriction. The main impetus seems to\nbe:\n\n> So how does parse_capability scale w.r.t the number of capabilities?\n> If parse_capability is just a linear search then it is O(n) and with n\n> capabilities the client faces an O(n^2) computation which is bad. So\n> if we were to require alphabetic capabilities, you could internally\n> keep track and the whole operation is O(n). I just wonder if this is\n> premature optimization or some thought we need to think of.\n\nbut that is an implementation problem, not a protocol problem. The\nparsing side can easily use something better than O(n) lookups (e.g., a\nbinary search). I think we can live with O(n lg n) to parse the whole\nlist. A true in-order merge would be O(n), but the difference is\nprobably negligible here, because...\n\n> Now if we assume the number of capabilities grows over time a lot\n> (someone may \"abuse\" it for a cool feature, similar to the refs\n> currently. Nobody thought about having so many refs in advance)\n\nI think if it grows without bound, we are doing it wrong. We should\ngenerally only be adding capabilities that require a single short tag in\nthe list (server supports \"foo\", client asks for \"foo\"). I'd find it\nacceptable to add ones that repeat, as long as we are very sure that the\nrepetition is going to be small, or scales with the size of the config\n(e.g., servers says \"here is a mirror you can seed from; here is another\nmirror you can seed from\").\n\nWe should definitely _not_ add anything that scales with the repository\nsize. For instance, the \"symref\" field only shows the \"HEAD\" now. That's\nOK, as it's constant size. We do not show _all_ symrefs right now\nbecause of pkt-line length limitations. With v2, we could in theory\nstart doing so (one per pkt-line). But that does not make it a good\nidea. The right way to implement that is:\n\n 1. the server says \"I can tell you about symrefs\"\n\n 2. client says \"OK, I understand how to parse your symref data\"\n\n 3. for each ref in the advertisement, tack on \"\\0symref=...\" (or\n whatever).\n\nThe capability portion of the conversation remains constant-sized, and\nthe O(# of refs) portion is part of the ref advertisement, which is\ninherently of that magnitude.\n\n-Peff\n"},{"id":"262962","messageId":"CAGZ79ka1qjspbbE4ZKHrys9TOmKbS30KA1PJkwzA47ptjxQfSA@mail.gmail.com","threadId":"39434","inReplyTo":"20150604130902.GA12404@peff.net","subject":"Re: [RFC/WIP PATCH 00/11] Protocol version 2, again!","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-04T16:44:49Z","receivedAt":"2015-06-04T16:44:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jun 4, 2015 at 6:09 AM, Jeff King wrote:\n> On Mon, Jun 01, 2015 at 10:49:45AM -0700, Stefan Beller wrote:\n>\n>> However the client side with builtin/fetch, builtin/fetch-pack, fetch-pack\n>> is a bit of a mystery to me, as I cannot fully grasp the difference between\n>> * connect.{h,c}\n>> * remote.{h.c}\n>> * transport.{h.c}\n>> there. All of it seems to be doing network related stuff, but I have trouble\n>> getting the big picture. I am assuming all of these 3 are rather a low level,\n>> used like a library, though there must be even more hierarchy in there,\n>> connect is most low level judging from the header file and used by\n>> the other two.\n>> transport.h seems to provide the most toplevel library stuff as it includes\n>> remote.h in its header?\n>\n> connect.c was originally \"the git protocol\", and was used by send-pack\n> and fetch-pack. Other individual programs implemented other transports.\n> Later, as the interface moved towards everybody running \"fetch\" and\n> \"push\", and those delegating work to the individual transports, we got\n> transport.c, which is an abstract interface for all transports. It\n> delegates actual git-protocol work to the functions in connect.c (or\n> bundle work elsewhere, or handles remote-helpers itself).\n>\n> And then remote.c contains routines for dealing with the remotes at a\n> logical level. E.g., which refs to fetch or push, etc.\n>\n> So in theory, the flow is something like:\n>\n> - fetch.c knows \"the user wants to fetch from 'foo'\"\n>\n> - fetch asks remote.c: \"who is remote 'foo'\"; we get back a URL\n>\n> - fetch asks transport.c: \"what are the refs for $URL\"\n>\n> - it turns out to be a git URL. transport.c calls into connect.c to\n> implement get_refs_via_connect.\n\nCurrently the distinction which protocol to speak is made\nhere (in get_refs_via_connect), which may be a bit late. Though I updating\nthe git protocol only first would also be feasible.\n\nSo for the next git protocol\n\n - get_refs_via_connect first asks for the capabilities and gets an answer from\n upload-pack-2. Now what?\n\n - we could have a callback in struct transport, which must be set\naccordingly by\n fetch in step 4 (it turns out to be a git URL. transport.c ...)\n This callback is called with each pkt-line such as\n\n void parse-capability(char *line, struct\n*transport_capabilities, void *cdata);\n\nThe line would contain the pkt-line, while the transport_capabilities\nwould be a struct\nsimilar as in \"[RFCv2 06/16] remote.h: add new struct for options\",\nwhere the fetch\nimplementation must select the right bits. Looking at fetch-pack.{c,h}\nwe only expose\none do-it-all method there, so we currently don't have file wide\neasily accessible variables,\nbut rather all in a struct fetch_pack_args, which carries important\ninformation for the\nselection process such as verbosity or desired options. This is why a\nvoid* comes in\nhandy as well. (It will be easy later to adapt that to the sending\nside as well).\n\nInstead of a full grown line by line callback we could also just\ncollect all the capabilities\nfirst in a string list and then only call back once into a\n\n void select_capabilities(struct string_list *available, struct\nstring_list *selected);\n\nI think I'd find this second approach more handy as there are subtle\ndetails you'd miss in\nthe first approach. Looking at fetch-pack.c, do_fetch_pack (line 790),\nwe have one\nselection (no_done) conditioned on another (multi_ack_detailed), so\nhaving the full list\nthere makes the code easier.\n\nThis second approach however might not be as future proof as the\nfirst, because we store\nall received capabilities (which may grow large in the future) and not\nthrow unknowns away\nimmediately.\n\nI tend to rather implement the second one (easier to read/maintain trumps a\nmaybe-performance-problem-in-the-future).\n\nThis performance-problem-in-the-future could be mitigated easily by having a\npreselection in transport.c get_capabilities, which ignores any capabilities not\nwhite listed there (harder to maintain though, as we have a more than one spot\nwhere to put a list)\n\nBy writing this mail I realized another thing. I have had the patch\n \"[RFCv2 09/16] remote.h: add get_remote_capabilities, request_capabilities\"\nwhich has request_capabilities just translating from a struct\ncontaining some bits\ninto a sequence of pkt-lines containing the actual protocol\ncapabilities. Maybe we\nshould not have that in the connect file, but rather as proposed in\nthis email, the\nhigh level command directly selects the strings to put back on the\nwire. (By having\n\"struct string_list *selected\" as part of the select_capabilities arguments.)\nthen the request_capabilities in connect.c would be dumbed down to just:\n\n void request_capabilities(int out, struct string_list *list)\n {\n struct string_list_item *item;\n for_each_string_list_item(item, list) {\n packet_write(out, item->string);\n }\n packet_flush(out);\n }\n\nI think that would be reasonable?\n\n>\n> - after fetch gets back the list of refs, it uses routines in remote.c\n> to figure out which refs we are interested in, handle refspecs, etc\n>\n> - now fetch asks transport.c: \"OK, fetch just these refs\"\n>\n> - transport.c again calls into connect.c to handle the actual fetch\n>\n> Of course over the years a lot of cruft has grown around all of them. I\n> wouldn't be surprised if there are functions which cross these\n> abstractions, or other random functions inside each file that do not\n> belong.\n>\n>> and the issue I am concerned about is the select_capabilities as well as\n>> the request_capabilities function here. The select_capabilities functionality\n>> is currently residing in the high level parts of the code as it both depends on\n>> the advertised server capabilities and on the user input (--use-frotz or config\n>> options), so the capability selection is done in fetchpack.c\n>>\n>> So there are 2 routes to go: Either we leave the select_capabilities in the\n>> upper layers (near the actual high level command, fetch, fetchpack) or we put\n>> it into the transport layer and just passing in a struct what the user desires.\n>> And when the users desire doesn't meet the server capabilities we die deep down\n>> in the transport layer.\n>\n> I think you have to leave it in the fetch-pack code. As you note, it's\n> the place where we know about what the user is asking for and can\n> manipulate the list. And not all transports even support capabilities\n> like this.\n>\n> -Peff\n\nOkay\n"},{"id":"262963","messageId":"xmqq3827wgpz.fsf@gitster.dls.corp.google.com","threadId":"39434","inReplyTo":"20150604131852.GB12404@peff.net","subject":"Re: [RFC/WIP PATCH 11/11] Document protocol version 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-04T17:01:12Z","receivedAt":"2015-06-04T17:01:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We should definitely _not_ add anything that scales with the repository\n> size. For instance, the \"symref\" field only shows the \"HEAD\" now. That's\n> OK, as it's constant size.\n\nI agree that this is an easy-to-explain rule to keep the design\nsensible.\n\n> We do not show _all_ symrefs right now\n> because of pkt-line length limitations. With v2, we could in theory\n> start doing so (one per pkt-line). But that does not make it a good\n> idea.\n\nVery true. If we want symref information conveyed, then we should\neither make a new \"symref advertisement\" available (and it is OK to\nuse a single-bit capability to enable or disable that new section),\nor make the \"ref advertisement\" natively capable of always doing so\n(i.e. if there is no need to make this optional).\n"}]}