{"thread":{"id":"49323","subject":"Add proto v2 archive command with HTTP support","startedAt":"2018-09-12T05:35:32Z","lastAt":"2018-09-28T01:25:40Z","messageCount":33,"participants":["Josh Steadmon","Stefan Beller","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"357946","messageId":"20180912053519.31085-1-steadmon@google.com","threadId":"49323","inReplyTo":null,"subject":"Add proto v2 archive command with HTTP support","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-09-12T05:35:16Z","receivedAt":"2018-09-12T05:35:32Z","isPatch":false,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"This series adds a new protocol v2 command for archiving, and allows\nthis command to work over HTTP(S). This was previously discussed in [1].\nI've CCed everyone who participated in that discussion.\n\n[1]: https://public-inbox.org/git/CANq=j3tK7QeBJOC7VNWkh4+WBNibMJJp5YUkd9te5NaYwukAow@mail.gmail.com/\n\n\n"},{"id":"357947","messageId":"20180912053519.31085-2-steadmon@google.com","threadId":"49323","inReplyTo":"20180912053519.31085-1-steadmon@google.com","subject":"[PATCH 1/3] archive: use packet_reader for communications","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-09-12T05:35:17Z","receivedAt":"2018-09-12T05:35:52Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Using packet_reader will simplify version detection and capability\nhandling, which will make implementation of protocol v2 support in\ngit-archive easier.\n\nSigned-off-by: Josh Steadmon \n---\n builtin/archive.c | 23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex e74f67539..e54fc39ad 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -27,10 +27,11 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\t\t const char *remote, const char *exec,\n \t\t\t const char *name_hint)\n {\n-\tchar *buf;\n \tint fd[2], i, rv;\n \tstruct transport *transport;\n \tstruct remote *_remote;\n+\tstruct packet_reader reader;\n+\tenum packet_read_status status;\n \n \t_remote = remote_get(remote);\n \tif (!_remote->url[0])\n@@ -38,6 +39,8 @@ static int run_remote_archiver(int argc, const char **argv,\n \ttransport = transport_get(_remote, _remote->url[0]);\n \ttransport_connect(transport, \"git-upload-archive\", exec, fd);\n \n+\tpacket_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n+\n \t/*\n \t * Inject a fake --format field at the beginning of the\n \t * arguments, with the format inferred from our output\n@@ -53,18 +56,20 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\tpacket_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n \tpacket_flush(fd[1]);\n \n-\tbuf = packet_read_line(fd[0], NULL);\n-\tif (!buf)\n+\tstatus = packet_reader_read(&reader);\n+\n+\tif (status == PACKET_READ_FLUSH)\n \t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n-\tif (strcmp(buf, \"ACK\")) {\n-\t\tif (starts_with(buf, \"NACK \"))\n-\t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n-\t\tif (starts_with(buf, \"ERR \"))\n-\t\t\tdie(_(\"remote error: %s\"), buf + 4);\n+\tif (strcmp(reader.buffer, \"ACK\")) {\n+\t\tif (starts_with(reader.buffer, \"NACK \"))\n+\t\t\tdie(_(\"git archive: NACK %s\"), reader.buffer + 5);\n+\t\tif (starts_with(reader.buffer, \"ERR \"))\n+\t\t\tdie(_(\"remote error: %s\"), reader.buffer + 4);\n \t\tdie(_(\"git archive: protocol error\"));\n \t}\n \n-\tif (packet_read_line(fd[0], NULL))\n+\tstatus = packet_reader_read(&reader);\n+\tif (status != PACKET_READ_FLUSH)\n \t\tdie(_(\"git archive: expected a flush\"));\n \n \t/* Now, start reading from fd[0] and spit it out to stdout */\n-- \n2.19.0.397.gdd90340f6a-goog\n\n"},{"id":"357948","messageId":"20180912053519.31085-3-steadmon@google.com","threadId":"49323","inReplyTo":"20180912053519.31085-1-steadmon@google.com","subject":"[PATCH 2/3] archive: implement protocol v2 archive command","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-09-12T05:35:18Z","receivedAt":"2018-09-12T05:36:05Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"This adds a new archive command for protocol v2. The command expects\narguments in the form \"argument X\" which are passed unmodified to\ngit-upload-archive--writer.\n\nThis command works over the file://, Git, and SSH transports. HTTP\nsupport will be added in a separate patch.\n\nSigned-off-by: Josh Steadmon \n---\n builtin/archive.c | 45 +++++++++++++++++++++++++++-------------\n builtin/upload-archive.c | 44 ++++++++++++++++++++++++++++++++++++---\n t/t5000-tar-tree.sh | 5 +++++\n 3 files changed, 77 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex e54fc39ad..73831887d 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -5,9 +5,11 @@\n #include \"cache.h\"\n #include \"builtin.h\"\n #include \"archive.h\"\n+#include \"connect.h\"\n #include \"transport.h\"\n #include \"parse-options.h\"\n #include \"pkt-line.h\"\n+#include \"protocol.h\"\n #include \"sideband.h\"\n \n static void create_output_file(const char *output_file)\n@@ -23,6 +25,13 @@ static void create_output_file(const char *output_file)\n \t}\n }\n \n+static int do_v2_command_and_cap(int out)\n+{\n+\tpacket_write_fmt(out, \"command=archive\\n\");\n+\t/* Capability list would go here, if we had any. */\n+\tpacket_delim(out);\n+}\n+\n static int run_remote_archiver(int argc, const char **argv,\n \t\t\t const char *remote, const char *exec,\n \t\t\t const char *name_hint)\n@@ -32,6 +41,7 @@ static int run_remote_archiver(int argc, const char **argv,\n \tstruct remote *_remote;\n \tstruct packet_reader reader;\n \tenum packet_read_status status;\n+\tenum protocol_version version;\n \n \t_remote = remote_get(remote);\n \tif (!_remote->url[0])\n@@ -41,6 +51,11 @@ static int run_remote_archiver(int argc, const char **argv,\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n \n+\tversion = discover_version(&reader);\n+\n+\tif (version == protocol_v2)\n+\t\tdo_v2_command_and_cap(fd[1]);\n+\n \t/*\n \t * Inject a fake --format field at the beginning of the\n \t * arguments, with the format inferred from our output\n@@ -56,22 +71,24 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\tpacket_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n \tpacket_flush(fd[1]);\n \n-\tstatus = packet_reader_read(&reader);\n-\n-\tif (status == PACKET_READ_FLUSH)\n-\t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n-\tif (strcmp(reader.buffer, \"ACK\")) {\n-\t\tif (starts_with(reader.buffer, \"NACK \"))\n-\t\t\tdie(_(\"git archive: NACK %s\"), reader.buffer + 5);\n-\t\tif (starts_with(reader.buffer, \"ERR \"))\n-\t\t\tdie(_(\"remote error: %s\"), reader.buffer + 4);\n-\t\tdie(_(\"git archive: protocol error\"));\n+\tif (version == protocol_v0) {\n+\t\tstatus = packet_reader_read(&reader);\n+\n+\t\tif (status == PACKET_READ_FLUSH)\n+\t\t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n+\t\tif (strcmp(reader.buffer, \"ACK\")) {\n+\t\t\tif (starts_with(reader.buffer, \"NACK \"))\n+\t\t\t\tdie(_(\"git archive: NACK %s\"), reader.buffer + 5);\n+\t\t\tif (starts_with(reader.buffer, \"ERR \"))\n+\t\t\t\tdie(_(\"remote error: %s\"), reader.buffer + 4);\n+\t\t\tdie(_(\"git archive: protocol error\"));\n+\t\t}\n+\n+\t\tstatus = packet_reader_read(&reader);\n+\t\tif (status != PACKET_READ_FLUSH)\n+\t\t\tdie(_(\"git archive: expected a flush\"));\n \t}\n \n-\tstatus = packet_reader_read(&reader);\n-\tif (status != PACKET_READ_FLUSH)\n-\t\tdie(_(\"git archive: expected a flush\"));\n-\n \t/* Now, start reading from fd[0] and spit it out to stdout */\n \trv = recv_sideband(\"archive\", fd[0], 1);\n \trv |= transport_disconnect(transport);\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 25d911635..534e8fd56 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -5,6 +5,7 @@\n #include \"builtin.h\"\n #include \"archive.h\"\n #include \"pkt-line.h\"\n+#include \"protocol.h\"\n #include \"sideband.h\"\n #include \"run-command.h\"\n #include \"argv-array.h\"\n@@ -73,13 +74,53 @@ static ssize_t process_input(int child_fd, int band)\n \treturn sz;\n }\n \n+static int handle_v2_command_and_cap(void)\n+{\n+\tstruct packet_reader reader;\n+\tenum packet_read_status status;\n+\n+\tpacket_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n+\n+\tpacket_write_fmt(1, \"version 2\\n\");\n+\t/*\n+\t * We don't currently send any capabilities, but maybe we could list\n+\t * supported archival formats?\n+\t */\n+\tpacket_flush(1);\n+\n+\tstatus = packet_reader_read(&reader);\n+\tif (status != PACKET_READ_NORMAL ||\n+\t strcmp(reader.buffer, \"command=archive\"))\n+\t\tdie(_(\"upload-archive: expected command=archive\"));\n+\twhile (status == PACKET_READ_NORMAL) {\n+\t\t/* We don't currently expect any client capabilities, but we\n+\t\t * should still read (and ignore) any that happen to get sent.\n+\t\t */\n+\t\tstatus = packet_reader_read(&reader);\n+\t}\n+\tif (status != PACKET_READ_DELIM)\n+\t\tdie(_(\"upload-archive: expected delim packet\"));\n+\n+\t/* Let git-upload-archive--writer handle the arguments. */\n+\n+\treturn 0;\n+}\n+\n int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n {\n \tstruct child_process writer = { argv };\n+\tenum protocol_version version = determine_protocol_version_server();\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(upload_archive_usage);\n \n+\tif (version == protocol_v2)\n+\t\thandle_v2_command_and_cap();\n+\telse {\n+\t\tpacket_write_fmt(1, \"ACK\\n\");\n+\t\tpacket_flush(1);\n+\t}\n+\n \t/*\n \t * Set up sideband subprocess.\n \t *\n@@ -96,9 +137,6 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n \t\tdie(\"upload-archive: %s\", strerror(err));\n \t}\n \n-\tpacket_write_fmt(1, \"ACK\\n\");\n-\tpacket_flush(1);\n-\n \twhile (1) {\n \t\tstruct pollfd pfd[2];\n \ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 2a97b27b0..4be74d6e9 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -145,6 +145,11 @@ test_expect_success \\\n \n check_tar b\n \n+test_expect_success 'protocol v2 for remote' '\n+\tGIT_PROTOCOL=\"version=2\" git archive --remote=. HEAD >v2_remote.tar\n+'\n+check_tar v2_remote\n+\n test_expect_success 'git archive --prefix=prefix/' '\n \tgit archive --prefix=prefix/ HEAD >with_prefix.tar\n '\n-- \n2.19.0.397.gdd90340f6a-goog\n\n"},{"id":"357949","messageId":"20180912053519.31085-4-steadmon@google.com","threadId":"49323","inReplyTo":"20180912053519.31085-1-steadmon@google.com","subject":"[PATCH 3/3] archive: allow archive over HTTP(S) with proto v2","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-09-12T05:35:19Z","receivedAt":"2018-09-12T05:36:05Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Signed-off-by: Josh Steadmon \n---\n builtin/archive.c | 8 +++++++-\n http-backend.c | 10 +++++++++-\n transport-helper.c | 5 +++--\n 3 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 73831887d..5fa75b3f7 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -87,7 +87,13 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\tstatus = packet_reader_read(&reader);\n \t\tif (status != PACKET_READ_FLUSH)\n \t\t\tdie(_(\"git archive: expected a flush\"));\n-\t}\n+\t} else if (version == protocol_v2 &&\n+\t\t starts_with(transport->url, \"http\"))\n+\t\t/*\n+\t\t * Commands over HTTP require two requests, so there's an\n+\t\t * additional server response to parse.\n+\t\t */\n+\t\tdiscover_version(&reader);\n \n \t/* Now, start reading from fd[0] and spit it out to stdout */\n \trv = recv_sideband(\"archive\", fd[0], 1);\ndiff --git a/http-backend.c b/http-backend.c\nindex 458642ef7..d62d583c7 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -32,6 +32,7 @@ struct rpc_service {\n static struct rpc_service rpc_service[] = {\n \t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n \t{ \"receive-pack\", \"receivepack\", 0, -1 },\n+\t{ \"upload-archive\", \"uploadarchive\", 1, 1 },\n };\n \n static struct string_list *get_parameters(void)\n@@ -637,6 +638,12 @@ static void service_rpc(struct strbuf *hdr, char *service_name)\n \tstruct rpc_service *svc = select_service(hdr, service_name);\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tif (!strcmp(service_name, \"git-upload-archive\")) {\n+\t\t/* git-upload-archive doesn't need --stateless-rpc */\n+\t\targv[1] = \".\";\n+\t\targv[2] = NULL;\n+\t}\n+\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"application/x-git-%s-request\", svc->name);\n \tcheck_content_type(hdr, buf.buf);\n@@ -713,7 +720,8 @@ static struct service_cmd {\n \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{40}\\\\.idx$\", get_idx_file},\n \n \t{\"POST\", \"/git-upload-pack$\", service_rpc},\n-\t{\"POST\", \"/git-receive-pack$\", service_rpc}\n+\t{\"POST\", \"/git-receive-pack$\", service_rpc},\n+\t{\"POST\", \"/git-upload-archive$\", service_rpc},\n };\n \n static int bad_request(struct strbuf *hdr, const struct service_cmd *c)\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 143ca008c..b4b96fc89 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -605,7 +605,8 @@ static int process_connect_service(struct transport *transport,\n \t\tret = run_connect(transport, &cmdbuf);\n \t} else if (data->stateless_connect &&\n \t\t (get_protocol_version_config() == protocol_v2) &&\n-\t\t !strcmp(\"git-upload-pack\", name)) {\n+\t\t (!strcmp(\"git-upload-pack\", name) ||\n+\t\t !strcmp(\"git-upload-archive\", name))) {\n \t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n \t\tret = run_connect(transport, &cmdbuf);\n \t\tif (ret)\n@@ -639,7 +640,7 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \t/* Get_helper so connect is inited. */\n \tget_helper(transport);\n-\tif (!data->connect)\n+\tif (!data->connect && !data->stateless_connect)\n \t\tdie(_(\"operation not supported by protocol\"));\n \n \tif (!process_connect_service(transport, name, exec))\n-- \n2.19.0.397.gdd90340f6a-goog\n\n"},{"id":"358014","messageId":"CAGZ79kb2Z71YL0o06eFC_POH0DWKOc2r=xZ1Mjc7Mxg-3OsHDw@mail.gmail.com","threadId":"49323","inReplyTo":"20180912053519.31085-2-steadmon@google.com","subject":"Re: [PATCH 1/3] archive: use packet_reader for communications","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-12T22:01:37Z","receivedAt":"2018-09-12T22:01:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 11, 2018 at 10:35 PM Josh Steadmon wrote:\n>\n> Using packet_reader will simplify version detection and capability\n> handling, which will make implementation of protocol v2 support in\n> git-archive easier.\n>\n> Signed-off-by: Josh Steadmon \n\nReviewed-by: Stefan Beller \n"},{"id":"358017","messageId":"CAGZ79kZOTsUH=zQX3rLXvuSOx1vp8C98maSn47ssfca8c-BrBQ@mail.gmail.com","threadId":"49323","inReplyTo":"20180912053519.31085-3-steadmon@google.com","subject":"Re: [PATCH 2/3] archive: implement protocol v2 archive command","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-12T22:28:22Z","receivedAt":"2018-09-12T22:28:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 11, 2018 at 10:36 PM Josh Steadmon wrote:\n>\n> This adds a new archive command for protocol v2. The command expects\n> arguments in the form \"argument X\" which are passed unmodified to\n> git-upload-archive--writer.\n>\n> This command works over the file://, Git, and SSH transports. HTTP\n> support will be added in a separate patch.\n>\n> Signed-off-by: Josh Steadmon \n> ---\n> builtin/archive.c | 45 +++++++++++++++++++++++++++-------------\n> builtin/upload-archive.c | 44 ++++++++++++++++++++++++++++++++++++---\n> t/t5000-tar-tree.sh | 5 +++++\n> 3 files changed, 77 insertions(+), 17 deletions(-)\n>\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index e54fc39ad..73831887d 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -5,9 +5,11 @@\n> #include \"cache.h\"\n> #include \"builtin.h\"\n> #include \"archive.h\"\n> +#include \"connect.h\"\n> #include \"transport.h\"\n> #include \"parse-options.h\"\n> #include \"pkt-line.h\"\n> +#include \"protocol.h\"\n> #include \"sideband.h\"\n>\n> static void create_output_file(const char *output_file)\n> @@ -23,6 +25,13 @@ static void create_output_file(const char *output_file)\n> }\n> }\n>\n> +static int do_v2_command_and_cap(int out)\n> +{\n> + packet_write_fmt(out, \"command=archive\\n\");\n> + /* Capability list would go here, if we had any. */\n> + packet_delim(out);\n> +}\n> +\n> static int run_remote_archiver(int argc, const char **argv,\n> const char *remote, const char *exec,\n> const char *name_hint)\n> @@ -32,6 +41,7 @@ static int run_remote_archiver(int argc, const char **argv,\n> struct remote *_remote;\n> struct packet_reader reader;\n> enum packet_read_status status;\n> + enum protocol_version version;\n>\n> _remote = remote_get(remote);\n> if (!_remote->url[0])\n> @@ -41,6 +51,11 @@ static int run_remote_archiver(int argc, const char **argv,\n>\n> packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n>\n> + version = discover_version(&reader);\n> +\n> + if (version == protocol_v2)\n> + do_v2_command_and_cap(fd[1]);\n> +\n> /*\n> * Inject a fake --format field at the beginning of the\n> * arguments, with the format inferred from our output\n> @@ -56,22 +71,24 @@ static int run_remote_archiver(int argc, const char **argv,\n> packet_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n> packet_flush(fd[1]);\n>\n> - status = packet_reader_read(&reader);\n> -\n> - if (status == PACKET_READ_FLUSH)\n> - die(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n> - if (strcmp(reader.buffer, \"ACK\")) {\n> - if (starts_with(reader.buffer, \"NACK \"))\n> - die(_(\"git archive: NACK %s\"), reader.buffer + 5);\n> - if (starts_with(reader.buffer, \"ERR \"))\n> - die(_(\"remote error: %s\"), reader.buffer + 4);\n> - die(_(\"git archive: protocol error\"));\n\nMaybe we also want to support v1\n(which is v0 prefixed with one pkt_line saying it is v1).\n\n If (version == protocol_v1)\n /* drop version v1 line, and then follow v0 logic. */\n packet_reader_read(&reader);\n\nDo we care about v1, or do we just ignore it here? why?\n(Don't answer me here, but rather put it in the commit message)\n\n> + if (version == protocol_v0) {\n> + status = packet_reader_read(&reader);\n> +\n> + if (status == PACKET_READ_FLUSH)\n> + die(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n> + if (strcmp(reader.buffer, \"ACK\")) {\n> + if (starts_with(reader.buffer, \"NACK \"))\n> + die(_(\"git archive: NACK %s\"), reader.buffer + 5);\n> + if (starts_with(reader.buffer, \"ERR \"))\n> + die(_(\"remote error: %s\"), reader.buffer + 4);\n> + die(_(\"git archive: protocol error\"));\n> + }\n> +\n> + status = packet_reader_read(&reader);\n> + if (status != PACKET_READ_FLUSH)\n> + die(_(\"git archive: expected a flush\"));\n> }\n>\n> - status = packet_reader_read(&reader);\n> - if (status != PACKET_READ_FLUSH)\n> - die(_(\"git archive: expected a flush\"));\n> -\n> /* Now, start reading from fd[0] and spit it out to stdout */\n> rv = recv_sideband(\"archive\", fd[0], 1);\n> rv |= transport_disconnect(transport);\n> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\n> index 25d911635..534e8fd56 100644\n> --- a/builtin/upload-archive.c\n> +++ b/builtin/upload-archive.c\n> @@ -5,6 +5,7 @@\n> #include \"builtin.h\"\n> #include \"archive.h\"\n> #include \"pkt-line.h\"\n> +#include \"protocol.h\"\n> #include \"sideband.h\"\n> #include \"run-command.h\"\n> #include \"argv-array.h\"\n> @@ -73,13 +74,53 @@ static ssize_t process_input(int child_fd, int band)\n> return sz;\n> }\n>\n> +static int handle_v2_command_and_cap(void)\n> +{\n> + struct packet_reader reader;\n> + enum packet_read_status status;\n> +\n> + packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> +\n> + packet_write_fmt(1, \"version 2\\n\");\n> + /*\n> + * We don't currently send any capabilities, but maybe we could list\n> + * supported archival formats?\n> + */\n> + packet_flush(1);\n> +\n> + status = packet_reader_read(&reader);\n> + if (status != PACKET_READ_NORMAL ||\n> + strcmp(reader.buffer, \"command=archive\"))\n> + die(_(\"upload-archive: expected command=archive\"));\n> + while (status == PACKET_READ_NORMAL) {\n> + /* We don't currently expect any client capabilities, but we\n> + * should still read (and ignore) any that happen to get sent.\n\n/*\n * Makes sense to ignore the client capabilities here,\n * but the multi line comments take their opening\n * and closing line on a separate line. just like above.\n */\n\n> + */\n> + status = packet_reader_read(&reader);\n> + }\n> + if (status != PACKET_READ_DELIM)\n> + die(_(\"upload-archive: expected delim packet\"));\n\nThis is upload-archive, which is a low level plumbing command\n(see the main man page of git for an explanation of that category),\nso we do not translate the error/die() calls. Besides, this is executed\non the server, which might have a different locale than the requesting\nclient?\n\nWould asking for a setlocale() on the server side be an unreasonable\nfeature request for the capabilities (in a follow up patch, and then not\njust for archive but also fetch/push, etc.)?\n\n> int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n> {\n> struct child_process writer = { argv };\n> + enum protocol_version version = determine_protocol_version_server();\n>\n> if (argc == 2 && !strcmp(argv[1], \"-h\"))\n> usage(upload_archive_usage);\n>\n> + if (version == protocol_v2)\n> + handle_v2_command_and_cap();\n> + else {\n\nSo if the client asked for v1, we still fall back to v0 here,\nwhich answers my question above.\n\n> + packet_write_fmt(1, \"ACK\\n\");\n> + packet_flush(1);\n> + }\n> +\n> /*\n> * Set up sideband subprocess.\n> *\n> @@ -96,9 +137,6 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n> die(\"upload-archive: %s\", strerror(err));\n> }\n>\n> - packet_write_fmt(1, \"ACK\\n\");\n> - packet_flush(1);\n> -\n> while (1) {\n> struct pollfd pfd[2];\n>\n> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n> index 2a97b27b0..4be74d6e9 100755\n> --- a/t/t5000-tar-tree.sh\n> +++ b/t/t5000-tar-tree.sh\n> @@ -145,6 +145,11 @@ test_expect_success \\\n>\n> check_tar b\n>\n> +test_expect_success 'protocol v2 for remote' '\n> + GIT_PROTOCOL=\"version=2\" git archive --remote=. HEAD >v2_remote.tar\n> +'\n> +check_tar v2_remote\n\nOur current standard is to keep all executions inside\na test_expect_* block, but here it is hard to comply with\nthat as the check_tar function contains test_expect_*\nand calling test_expect_* from within itself doesn't work\nwith our test suite.\n\nSo bonus points for a refactoring to bring t5000 up to\nour current standard (c.f. t0020 for a reasonable new\ncode, and t2002 for older code, though that only covers\nsyntax, not functions)\n\nThe check itself is just testing that giving GIT_PROTOCOL=2\nin the environment also let's you obtain an archive. It doesn't\ntest if the actual communication *is* v2.\nSee 5e3548ef161 (fetch: send server options when using\nprotocol v2, 2018-04-23) for an example how to sniff on the\nnetwork traffic in tests, i.e. use GIT_TRACE_PACKET=...\nand grep on that?\n\nThanks,\nStefan\n"},{"id":"358019","messageId":"CAGZ79kaXF96QKm6g2kUj+C0-ebhrNLY_BVJWhj_xYeritjPkvw@mail.gmail.com","threadId":"49323","inReplyTo":"20180912053519.31085-4-steadmon@google.com","subject":"Re: [PATCH 3/3] archive: allow archive over HTTP(S) with proto v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-12T22:38:30Z","receivedAt":"2018-09-12T22:38:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 11, 2018 at 10:36 PM Josh Steadmon wrote:\n>\n> Signed-off-by: Josh Steadmon \n> ---\n> builtin/archive.c | 8 +++++++-\n> http-backend.c | 10 +++++++++-\n> transport-helper.c | 5 +++--\n> 3 files changed, 19 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index 73831887d..5fa75b3f7 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -87,7 +87,13 @@ static int run_remote_archiver(int argc, const char **argv,\n> status = packet_reader_read(&reader);\n> if (status != PACKET_READ_FLUSH)\n> die(_(\"git archive: expected a flush\"));\n> - }\n> + } else if (version == protocol_v2 &&\n> + starts_with(transport->url, \"http\"))\n\nI assume this is a smart way to cover both http and https\nas the latter is prefixed by the former.\n\nHow does this interact with remote helpers\n(See gitremote-helpers(1), which doesn't mention archives,\nbut I would imagine that one would be able to use a remote\nhelper eventually, too?\n\n git archive --remote=abc://example.org/test ....\n\n> + /*\n> + * Commands over HTTP require two requests, so there's an\n> + * additional server response to parse.\n> + */\n> + discover_version(&reader);\n>\n> /* Now, start reading from fd[0] and spit it out to stdout */\n> rv = recv_sideband(\"archive\", fd[0], 1);\n> diff --git a/http-backend.c b/http-backend.c\n> index 458642ef7..d62d583c7 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -32,6 +32,7 @@ struct rpc_service {\n> static struct rpc_service rpc_service[] = {\n> { \"upload-pack\", \"uploadpack\", 1, 1 },\n> { \"receive-pack\", \"receivepack\", 0, -1 },\n> + { \"upload-archive\", \"uploadarchive\", 1, 1 },\n\nSo git archives are not supported in protocol v0 via http?\nThen it makes sense to see not mention of archives in\nthe remote helpers as well.\n\nThe rest of the code is a surprisingly small patch.\n\nStefan\n"},{"id":"358050","messageId":"xmqqr2hxigw2.fsf@gitster-ct.c.googlers.com","threadId":"49323","inReplyTo":"20180912053519.31085-2-steadmon@google.com","subject":"Re: [PATCH 1/3] archive: use packet_reader for communications","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-13T14:58:05Z","receivedAt":"2018-09-13T14:58:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Using packet_reader will simplify version detection and capability\n> handling, which will make implementation of protocol v2 support in\n> git-archive easier.\n\nIs this meant as a change in implementation without any change in\nbehaviour?\n\n> Signed-off-by: Josh Steadmon \n> ---\n> builtin/archive.c | 23 ++++++++++++++---------\n> 1 file changed, 14 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index e74f67539..e54fc39ad 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -27,10 +27,11 @@ static int run_remote_archiver(int argc, const char **argv,\n> \t\t\t const char *remote, const char *exec,\n> \t\t\t const char *name_hint)\n> {\n> -\tchar *buf;\n> \tint fd[2], i, rv;\n> \tstruct transport *transport;\n> \tstruct remote *_remote;\n> +\tstruct packet_reader reader;\n> +\tenum packet_read_status status;\n> \n> \t_remote = remote_get(remote);\n> \tif (!_remote->url[0])\n> @@ -38,6 +39,8 @@ static int run_remote_archiver(int argc, const char **argv,\n> \ttransport = transport_get(_remote, _remote->url[0]);\n> \ttransport_connect(transport, \"git-upload-archive\", exec, fd);\n> \n> +\tpacket_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> +\n> \t/*\n> \t * Inject a fake --format field at the beginning of the\n> \t * arguments, with the format inferred from our output\n> @@ -53,18 +56,20 @@ static int run_remote_archiver(int argc, const char **argv,\n> \t\tpacket_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n> \tpacket_flush(fd[1]);\n> \n> -\tbuf = packet_read_line(fd[0], NULL);\n> -\tif (!buf)\n> +\tstatus = packet_reader_read(&reader);\n> +\n> +\tif (status == PACKET_READ_FLUSH)\n> \t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n\nIt is true that packet_read_line() returns a NULL on flush, but the\nfunction also returns NULL on other conditions for which underlying\npacket_read() returns 0 (or negative) length. EOF, normal data with\nzero length (i.e. packet length itself is 4), and DELIM packets\nwould all have led to this die() in the original code. We fail to\nnotice that we got something unexpected when we were expecting to\nget a normal packet with ACK or NACK on it.\n\n> -\tif (strcmp(buf, \"ACK\")) {\n> -\t\tif (starts_with(buf, \"NACK \"))\n> -\t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n> -\t\tif (starts_with(buf, \"ERR \"))\n> -\t\t\tdie(_(\"remote error: %s\"), buf + 4);\n> +\tif (strcmp(reader.buffer, \"ACK\")) {\n\nThe way I read packet_reader_read()'s implementation and existing\ncallers (like the ones in fetch-pack.c) tells me that consumers\nshould not be looking at \"reader.buffer\"; they should instead be\nreading from \"reader.line\".\n\n> +\t\tif (starts_with(reader.buffer, \"NACK \"))\n> +\t\t\tdie(_(\"git archive: NACK %s\"), reader.buffer + 5);\n> +\t\tif (starts_with(reader.buffer, \"ERR \"))\n> +\t\t\tdie(_(\"remote error: %s\"), reader.buffer + 4);\n> \t\tdie(_(\"git archive: protocol error\"));\n> \t}\n> \n> -\tif (packet_read_line(fd[0], NULL))\n> +\tstatus = packet_reader_read(&reader);\n> +\tif (status != PACKET_READ_FLUSH)\n> \t\tdie(_(\"git archive: expected a flush\"));\n\nThis makes me wonder what happens if we got an EOF instead. We fail\nto notice protocol error here, but do the code after this part\ncorrectly handle the situation?\n\n> \t/* Now, start reading from fd[0] and spit it out to stdout */\n"},{"id":"358053","messageId":"xmqqa7olif76.fsf@gitster-ct.c.googlers.com","threadId":"49323","inReplyTo":"xmqqr2hxigw2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] archive: use packet_reader for communications","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-13T15:34:37Z","receivedAt":"2018-09-13T15:34:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> -\tif (packet_read_line(fd[0], NULL))\n>> +\tstatus = packet_reader_read(&reader);\n>> +\tif (status != PACKET_READ_FLUSH)\n>> \t\tdie(_(\"git archive: expected a flush\"));\n>\n> This makes me wonder what happens if we got an EOF instead. We fail\n> to notice protocol error here, but do the code after this part\n> correctly handle the situation?\n\nSorry, this part of my comment is completely backwards.\n\nWe require they send a flush, not a 0-length data packet of length\n4, and otherwise we die, even though we used to treate 0-length data\npacket of length 4 just like a flush.\n\nSo this is making the code more strict than the original. As long\nas all the existing implementations correctly use flush here, there\nwould be no unintended regression, but it bothers me that we have to\neven worry if these behaviour changes affect the already deployed\nsoftware negatively.\n\n>\n>> \t/* Now, start reading from fd[0] and spit it out to stdout */\n"},{"id":"358057","messageId":"xmqq1s9xicku.fsf@gitster-ct.c.googlers.com","threadId":"49323","inReplyTo":"20180912053519.31085-3-steadmon@google.com","subject":"Re: [PATCH 2/3] archive: implement protocol v2 archive command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-13T16:31:13Z","receivedAt":"2018-09-13T16:31:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> +static int do_v2_command_and_cap(int out)\n> +{\n> +\tpacket_write_fmt(out, \"command=archive\\n\");\n> +\t/* Capability list would go here, if we had any. */\n> +\tpacket_delim(out);\n> +}\n> +\n> static int run_remote_archiver(int argc, const char **argv,\n> \t\t\t const char *remote, const char *exec,\n> \t\t\t const char *name_hint)\n> @@ -32,6 +41,7 @@ static int run_remote_archiver(int argc, const char **argv,\n> \tstruct remote *_remote;\n> \tstruct packet_reader reader;\n> \tenum packet_read_status status;\n> +\tenum protocol_version version;\n> \n> \t_remote = remote_get(remote);\n> \tif (!_remote->url[0])\n> @@ -41,6 +51,11 @@ static int run_remote_archiver(int argc, const char **argv,\n> \n> \tpacket_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> \n> +\tversion = discover_version(&reader);\n\nThe original version of upload-archive that is correctly running on\nthe other end sends either NACK (unable to spawn) or ACK (ready to\nserve) to us without waiting for us to speak first, so peeking that\nwith this discover_version() is a safe thing to do.\n\n> +\tif (version == protocol_v2)\n> +\t\tdo_v2_command_and_cap(fd[1]);\n> +\n\nWith proto v2, \"server capabilities\" have already been collected in\nserver_capabilities_v2 array in discover_version(). We are to pick\nand ask the capabilities in that function and respond. Right now we\ndo not need to do much, as we saw that very thin implementation of\nthat function above.\n\n> \t/*\n> \t * Inject a fake --format field at the beginning of the\n> \t * arguments, with the format inferred from our output\n\nAnd then after that, both the original and updated protocol lets us\nsend the archive format and arguments (like revs and pathspecs),\nfollowed by a flush packet...\n\n> @@ -56,22 +71,24 @@ static int run_remote_archiver(int argc, const char **argv,\n> \t\tpacket_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n> \tpacket_flush(fd[1]);\n\n... which is a piece of code shared between the protocol versions\nthat ends here.\n\n> -\tstatus = packet_reader_read(&reader);\n> -\n> -\tif (status == PACKET_READ_FLUSH)\n> -\t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n> -\tif (strcmp(reader.buffer, \"ACK\")) {\n> -\t\tif (starts_with(reader.buffer, \"NACK \"))\n> -\t\t\tdie(_(\"git archive: NACK %s\"), reader.buffer + 5);\n> -\t\tif (starts_with(reader.buffer, \"ERR \"))\n> -\t\t\tdie(_(\"remote error: %s\"), reader.buffer + 4);\n> -\t\tdie(_(\"git archive: protocol error\"));\n> +\tif (version == protocol_v0) {\n> +\t\tstatus = packet_reader_read(&reader);\n> +\n> +\t\tif (status == PACKET_READ_FLUSH)\n> +\t\t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n> +\t\tif (strcmp(reader.buffer, \"ACK\")) {\n> +\t\t\tif (starts_with(reader.buffer, \"NACK \"))\n> +\t\t\t\tdie(_(\"git archive: NACK %s\"), reader.buffer + 5);\n> +\t\t\tif (starts_with(reader.buffer, \"ERR \"))\n> +\t\t\t\tdie(_(\"remote error: %s\"), reader.buffer + 4);\n> +\t\t\tdie(_(\"git archive: protocol error\"));\n> +\t\t}\n> +\n> +\t\tstatus = packet_reader_read(&reader);\n> +\t\tif (status != PACKET_READ_FLUSH)\n> +\t\t\tdie(_(\"git archive: expected a flush\"));\n> \t}\n\nThe original protocol lets upload-archive to report failure to spawn\nthe writer backend process and lets us act on it. We do not need a\nsimilar support in the updated protocol and instead can jump right\ninto receiving the archive stream because...?\n\n> -\tstatus = packet_reader_read(&reader);\n> -\tif (status != PACKET_READ_FLUSH)\n> -\t\tdie(_(\"git archive: expected a flush\"));\n> -\n\n> \t/* Now, start reading from fd[0] and spit it out to stdout */\n> \trv = recv_sideband(\"archive\", fd[0], 1);\n> \trv |= transport_disconnect(transport);\n\n\n\n> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\n> index 25d911635..534e8fd56 100644\n> --- a/builtin/upload-archive.c\n> +++ b/builtin/upload-archive.c\n> @@ -5,6 +5,7 @@\n> #include \"builtin.h\"\n> #include \"archive.h\"\n> #include \"pkt-line.h\"\n> +#include \"protocol.h\"\n> #include \"sideband.h\"\n> #include \"run-command.h\"\n> #include \"argv-array.h\"\n> @@ -73,13 +74,53 @@ static ssize_t process_input(int child_fd, int band)\n> \treturn sz;\n> }\n> \n> +static int handle_v2_command_and_cap(void)\n> +{\n> +\tstruct packet_reader reader;\n> +\tenum packet_read_status status;\n> +\n> +\tpacket_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> +\n> +\tpacket_write_fmt(1, \"version 2\\n\");\n\nThis lets the discover_version() on the other side notice that we\nare speaking version 2.\n\n> +\t/*\n> +\t * We don't currently send any capabilities, but maybe we could list\n> +\t * supported archival formats?\n> +\t */\n> +\tpacket_flush(1);\n\nprocess_capabilities_v2() expects the list of caps ends with a\nflush, which is given here.\n\n> +\tstatus = packet_reader_read(&reader);\n> +\tif (status != PACKET_READ_NORMAL ||\n> +\t strcmp(reader.buffer, \"command=archive\"))\n> +\t\tdie(_(\"upload-archive: expected command=archive\"));\n\nThe other side in do_v2_command_and_cap() would ask command=archive\nand that is verified. _() is unwanted, I suppose, by the way, as\nyou do not know what language the other side wants anyway.\n\n> +\twhile (status == PACKET_READ_NORMAL) {\n> +\t\t/* We don't currently expect any client capabilities, but we\n> +\t\t * should still read (and ignore) any that happen to get sent.\n> +\t\t */\n> +\t\tstatus = packet_reader_read(&reader);\n\nIt is wrong to say we should \"ignore\". If you are asked to behave\nin a certain way by a capability that is not understood, the other\nside expects you to honor that request and you have no idea how to\ncomply. At least you should make sure that what is asked is among\nthe capabilities you offered (or you understand), and you should\nerror out when you see an unknown one, no?\n\n> +\t}\n> +\tif (status != PACKET_READ_DELIM)\n> +\t\tdie(_(\"upload-archive: expected delim packet\"));\n> +\n> +\t/* Let git-upload-archive--writer handle the arguments. */\n\nThe choice of DELIM here over FLUSH is a bit curious, but it is\nconsistent between upload-archive and run-remote-archiver.\n\n> +\treturn 0;\n> +}\n> +\n> int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n> {\n> \tstruct child_process writer = { argv };\n> +\tenum protocol_version version = determine_protocol_version_server();\n> \n> \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n> \t\tusage(upload_archive_usage);\n> \n> +\tif (version == protocol_v2)\n> +\t\thandle_v2_command_and_cap();\n> +\telse {\n> +\t\tpacket_write_fmt(1, \"ACK\\n\");\n> +\t\tpacket_flush(1);\n> +\t}\n> +\n\nThis breaks the original protocol, no? At this point we haven't\neven tried to start the writer process, and letting the other side\ngo by giving ACK + flush prematurely. After start_command() fails,\nwe may say NACK, but the other side is no longer listening to it.\n\n> \t/*\n> \t * Set up sideband subprocess.\n> \t *\n> @@ -96,9 +137,6 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n> \t\tdie(\"upload-archive: %s\", strerror(err));\n> \t}\n> \n> -\tpacket_write_fmt(1, \"ACK\\n\");\n> -\tpacket_flush(1);\n> -\n> \twhile (1) {\n> \t\tstruct pollfd pfd[2];\n> \n> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n> index 2a97b27b0..4be74d6e9 100755\n> --- a/t/t5000-tar-tree.sh\n> +++ b/t/t5000-tar-tree.sh\n> @@ -145,6 +145,11 @@ test_expect_success \\\n> \n> check_tar b\n> \n> +test_expect_success 'protocol v2 for remote' '\n> +\tGIT_PROTOCOL=\"version=2\" git archive --remote=. HEAD >v2_remote.tar\n> +'\n> +check_tar v2_remote\n> +\n> test_expect_success 'git archive --prefix=prefix/' '\n> \tgit archive --prefix=prefix/ HEAD >with_prefix.tar\n> '\n"},{"id":"358059","messageId":"xmqqworpgx99.fsf@gitster-ct.c.googlers.com","threadId":"49323","inReplyTo":"20180912053519.31085-4-steadmon@google.com","subject":"Re: [PATCH 3/3] archive: allow archive over HTTP(S) with proto v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-13T16:47:30Z","receivedAt":"2018-09-13T16:47:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Signed-off-by: Josh Steadmon \n> ---\n> builtin/archive.c | 8 +++++++-\n> http-backend.c | 10 +++++++++-\n> transport-helper.c | 5 +++--\n> 3 files changed, 19 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index 73831887d..5fa75b3f7 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -87,7 +87,13 @@ static int run_remote_archiver(int argc, const char **argv,\n> \t\tstatus = packet_reader_read(&reader);\n> \t\tif (status != PACKET_READ_FLUSH)\n> \t\t\tdie(_(\"git archive: expected a flush\"));\n> -\t}\n> +\t} else if (version == protocol_v2 &&\n> +\t\t starts_with(transport->url, \"http\"))\n> +\t\t/*\n> +\t\t * Commands over HTTP require two requests, so there's an\n> +\t\t * additional server response to parse.\n> +\t\t */\n> +\t\tdiscover_version(&reader);\n\nWhat should happen if the version discovered here is different from\nv2 or the capabilities offered are different from what we saw\nbefore? Perhaps we need some sanity checks, as we on this side of\nthe connection know we are making two requests, and may even end up\ntalking with an instance of \"upload-archive\" that is different from\nthe one we talked with earlier.\n\n> diff --git a/http-backend.c b/http-backend.c\n> index 458642ef7..d62d583c7 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -32,6 +32,7 @@ struct rpc_service {\n> static struct rpc_service rpc_service[] = {\n> \t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n> \t{ \"receive-pack\", \"receivepack\", 0, -1 },\n> +\t{ \"upload-archive\", \"uploadarchive\", 1, 1 },\n> };\n> \n> static struct string_list *get_parameters(void)\n> @@ -637,6 +638,12 @@ static void service_rpc(struct strbuf *hdr, char *service_name)\n> \tstruct rpc_service *svc = select_service(hdr, service_name);\n> \tstruct strbuf buf = STRBUF_INIT;\n> \n> +\tif (!strcmp(service_name, \"git-upload-archive\")) {\n> +\t\t/* git-upload-archive doesn't need --stateless-rpc */\n> +\t\targv[1] = \".\";\n> +\t\targv[2] = NULL;\n> +\t}\n> +\n> \tstrbuf_reset(&buf);\n> \tstrbuf_addf(&buf, \"application/x-git-%s-request\", svc->name);\n> \tcheck_content_type(hdr, buf.buf);\n> @@ -713,7 +720,8 @@ static struct service_cmd {\n> \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{40}\\\\.idx$\", get_idx_file},\n> \n> \t{\"POST\", \"/git-upload-pack$\", service_rpc},\n> -\t{\"POST\", \"/git-receive-pack$\", service_rpc}\n> +\t{\"POST\", \"/git-receive-pack$\", service_rpc},\n> +\t{\"POST\", \"/git-upload-archive$\", service_rpc},\n> };\n> \n> static int bad_request(struct strbuf *hdr, const struct service_cmd *c)\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 143ca008c..b4b96fc89 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -605,7 +605,8 @@ static int process_connect_service(struct transport *transport,\n> \t\tret = run_connect(transport, &cmdbuf);\n> \t} else if (data->stateless_connect &&\n> \t\t (get_protocol_version_config() == protocol_v2) &&\n> -\t\t !strcmp(\"git-upload-pack\", name)) {\n> +\t\t (!strcmp(\"git-upload-pack\", name) ||\n> +\t\t !strcmp(\"git-upload-archive\", name))) {\n> \t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n> \t\tret = run_connect(transport, &cmdbuf);\n> \t\tif (ret)\n> @@ -639,7 +640,7 @@ static int connect_helper(struct transport *transport, const char *name,\n> \n> \t/* Get_helper so connect is inited. */\n> \tget_helper(transport);\n> -\tif (!data->connect)\n> +\tif (!data->connect && !data->stateless_connect)\n> \t\tdie(_(\"operation not supported by protocol\"));\n\nThis is somewhat curious. The upload-pack going over HTTP also is\ntriggered by the same \"stateless-connect\" remote helper command, as\nwe just saw in the previous hunk, and that support is not new. Why\ndo we need this change then? What's different between the \"data\"\nthat is obtained by get_helper(transport) for driving upload-pack\nand upload-archive? Presumably upload-pack was working without this\nchange because it also sets the connect bit on, and upload-archive\ndoes not work without this change because it does not? Why do these\ntwo behave differently?\n\n> \tif (!process_connect_service(transport, name, exec))\n"},{"id":"358083","messageId":"87k1npkzh5.fsf@evledraar.gmail.com","threadId":"49323","inReplyTo":"CAGZ79kZOTsUH=zQX3rLXvuSOx1vp8C98maSn47ssfca8c-BrBQ@mail.gmail.com","subject":"Re: [PATCH 2/3] archive: implement protocol v2 archive command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-13T18:45:58Z","receivedAt":"2018-09-13T18:46:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 12 2018, Stefan Beller wrote:\n\n> On Tue, Sep 11, 2018 at 10:36 PM Josh Steadmon wrote:\n>> + */\n>> + status = packet_reader_read(&reader);\n>> + }\n>> + if (status != PACKET_READ_DELIM)\n>> + die(_(\"upload-archive: expected delim packet\"));\n>\n> This is upload-archive, which is a low level plumbing command\n> (see the main man page of git for an explanation of that category),\n> so we do not translate the error/die() calls. Besides, this is executed\n> on the server, which might have a different locale than the requesting\n> client?\n>\n> Would asking for a setlocale() on the server side be an unreasonable\n> feature request for the capabilities (in a follow up patch, and then not\n> just for archive but also fetch/push, etc.)?\n\nThis would be very nice to have, but as you suggest in some follow-up\nchange.\n\nI think though that instead of doing setlocale() it would be better to\npass some flag saying we're operating in a machine-readable mode, and\nthen we'd (as part of the protocol defintion) say we're going to emit\nGIT_ERR_UPLOAD_ARCHIVE_EXPECTED_DELIM_PACKET or whatever.\n\nAdvantages of doing that over a server-side setlocale():\n\n 1) Purely for translation purposes, users can update to a newer client\n to get new translations, even though they're talking to an old\n server.\n\n 2) Again, only for translation purposes, servers may not have the\n appropriate locales generated and/or linked to libgettext.\n\n 3) Ditto, some clients that aren't git.git may want/need to emit\n different translation messages to their consumers than what we have,\n think some GUI client / Emacs magit etc. whose UI is different from\n ours.\n\n 4) Aside from translation purposes, getting a machine-readable\n \"push/pull\" etc. mode would be very handy. E.g. now you need to\n parse stderr to see why exactly your push failed (hook denied, or\n non-fast-forward, or non-fast-forward where there was a lock race\n condition? ...).\n\nI also wonder if something like #4 wouldn't compliment something like\nthe proposed structured logging[1]. I.e. even though we'd like to run\ngit.git, and present exactly the message to the user we do now, we might\nwant to run in such a machine-readable mode under the hood when talking\nto the server so we can log exactly how the push went / how it failed\nfor the purposes of aggregation.\n\n1. https://public-inbox.org/git/20180713165621.52017-2-git@jeffhostetler.com/\n"},{"id":"358111","messageId":"20180914053650.GA219147@aiede.svl.corp.google.com","threadId":"49323","inReplyTo":"20180912053519.31085-1-steadmon@google.com","subject":"Re: Add proto v2 archive command with HTTP support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-14T05:36:51Z","receivedAt":"2018-09-14T05:36:56Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJosh Steadmon wrote:\n\n> This series adds a new protocol v2 command for archiving, and allows\n> this command to work over HTTP(S). This was previously discussed in [1].\n> I've CCed everyone who participated in that discussion.\n\nYay! Getting ready to read it now.\n\nFor the future, \"git format-patch --cover-letter\" does a few things\nthat can be nice for this kind of opening message:\n\n- it lists the patches in the series, and a diffstat\n- it puts [PATCH 0/3] in the subject line so people know what to expect\n\nThanks,\nJonathan\n\n> [1]: https://public-inbox.org/git/CANq=j3tK7QeBJOC7VNWkh4+WBNibMJJp5YUkd9te5NaYwukAow@mail.gmail.com/\n"},{"id":"358112","messageId":"20180914053944.GB219147@aiede.svl.corp.google.com","threadId":"49323","inReplyTo":"20180912053519.31085-3-steadmon@google.com","subject":"Re: [PATCH 2/3] archive: implement protocol v2 archive command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-14T05:39:44Z","receivedAt":"2018-09-14T05:39:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJosh Steadmon wrote:\n\n> This adds a new archive command for protocol v2. The command expects\n> arguments in the form \"argument X\" which are passed unmodified to\n> git-upload-archive--writer.\n>\n> This command works over the file://, Git, and SSH transports. HTTP\n> support will be added in a separate patch.\n>\n> Signed-off-by: Josh Steadmon \n> ---\n> builtin/archive.c | 45 +++++++++++++++++++++++++++-------------\n> builtin/upload-archive.c | 44 ++++++++++++++++++++++++++++++++++++---\n> t/t5000-tar-tree.sh | 5 +++++\n> 3 files changed, 77 insertions(+), 17 deletions(-)\n\nI like the diffstat. :)\n\nCan this include some docs in Documentation/technical/ as well, to\nhelp other implementors to understand the protocol so they can\ninteroperate?\n\nThanks,\nJonathan\n"},{"id":"358113","messageId":"20180914055703.GC219147@aiede.svl.corp.google.com","threadId":"49323","inReplyTo":"20180912053519.31085-4-steadmon@google.com","subject":"Re: [PATCH 3/3] archive: allow archive over HTTP(S) with proto v2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-14T05:57:03Z","receivedAt":"2018-09-14T05:57:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJosh Steadmon wrote:\n\n> Subject: archive: allow archive over HTTP(S) with proto v2\n\nIt's interesting how little this has to touch the client.\n\n> Signed-off-by: Josh Steadmon \n> ---\n> builtin/archive.c | 8 +++++++-\n> http-backend.c | 10 +++++++++-\n> transport-helper.c | 5 +++--\n> 3 files changed, 19 insertions(+), 4 deletions(-)\n[....]\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -87,7 +87,13 @@ static int run_remote_archiver(int argc, const char **argv,\n> \t\tstatus = packet_reader_read(&reader);\n> \t\tif (status != PACKET_READ_FLUSH)\n> \t\t\tdie(_(\"git archive: expected a flush\"));\n> -\t}\n> +\t} else if (version == protocol_v2 &&\n> +\t\t starts_with(transport->url, \"http\"))\n\nAs Stefan noticed, this starts_with test seems a bit too loose. For\nexample, what happens if I try an scp-style SSH URL like\nhttp.example.com:path/to/repo, a local path like http/foo/bar, or a\ncustom protocol like httplikebutbetter://path/to/repo (honest\nquestion: I haven't tried)?\n\n> +\t\t/*\n> +\t\t * Commands over HTTP require two requests, so there's an\n> +\t\t * additional server response to parse.\n> +\t\t */\n> +\t\tdiscover_version(&reader);\n\nCan this be made consistent with the non-http case? The original\ncapabilities (/info/refs) response told us what protocol version the\nserver wants to use, which means that a hypothetical protocol v3 could\nuse a completely different request format for the followup commands:\nso could the server omit the protocol version in the v2\n/git-upload-archive response? Alternatively, if we want to include\nthe protocol version again, could we do that in stateful protocols as\nwell?\n\nRelated question: what should happen if the two responses declare\ndifferent protocol versions? Should we diagnose that as a protocol\nerror?\n\n[...]\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -32,6 +32,7 @@ struct rpc_service {\n> static struct rpc_service rpc_service[] = {\n> \t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n> \t{ \"receive-pack\", \"receivepack\", 0, -1 },\n> +\t{ \"upload-archive\", \"uploadarchive\", 1, 1 },\n\nshell.c orders these in almost-alphabetical order (receive-pack,\nupload-pack, upload-archive). I guess they should both use actual\nalphabetical order? (If you agree, then please feel free to do that\nin a separate patch.)\n\n[...]\n> @@ -637,6 +638,12 @@ static void service_rpc(struct strbuf *hdr, char *service_name)\n> \tstruct rpc_service *svc = select_service(hdr, service_name);\n> \tstruct strbuf buf = STRBUF_INIT;\n> \n> +\tif (!strcmp(service_name, \"git-upload-archive\")) {\n> +\t\t/* git-upload-archive doesn't need --stateless-rpc */\n\nThis comment doesn't seem actionable. Can it say why? E.g. \"[...]\nbecause an upload-archive command always involves a single\nround-trip\". Or alternatively, I think it's fine to omit the comment.\n\n> +\t\targv[1] = \".\";\n> +\t\targv[2] = NULL;\n> +\t}\n[...]\n> @@ -713,7 +720,8 @@ static struct service_cmd {\n> \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{40}\\\\.idx$\", get_idx_file},\n> \n> \t{\"POST\", \"/git-upload-pack$\", service_rpc},\n> -\t{\"POST\", \"/git-receive-pack$\", service_rpc}\n> +\t{\"POST\", \"/git-receive-pack$\", service_rpc},\n> +\t{\"POST\", \"/git-upload-archive$\", service_rpc},\n\nSame comment about services seeming to be in a randomish order.\n\n[...]\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -605,7 +605,8 @@ static int process_connect_service(struct transport *transport,\n> \t\tret = run_connect(transport, &cmdbuf);\n> \t} else if (data->stateless_connect &&\n> \t\t (get_protocol_version_config() == protocol_v2) &&\n\n(not about this patch) These parens don't help --- they make it harder\nfor me to read, especially with the new parens to try to match them up\nwith.\n\n> -\t\t !strcmp(\"git-upload-pack\", name)) {\n> +\t\t (!strcmp(\"git-upload-pack\", name) ||\n> +\t\t !strcmp(\"git-upload-archive\", name))) {\n\nA part of me wonders about the wasted cycles comparing to\n\"git-upload-\" twice, but (1) it is tiny relative to actually serving\nthe request and (2) if we're lucky, the compiler (or a compiler of the\nfuture) inlines the strcmp call and could optimize it out.\n\n[...]\n> @@ -639,7 +640,7 @@ static int connect_helper(struct transport *transport, const char *name,\n> \n> \t/* Get_helper so connect is inited. */\n> \tget_helper(transport);\n> -\tif (!data->connect)\n> +\tif (!data->connect && !data->stateless_connect)\n> \t\tdie(_(\"operation not supported by protocol\"));\n\nI don't understand this part. Can you explain it further (possibly by\nputting it in its own patch)?\n\nThanks for a pleasant read,\nJonathan\n"},{"id":"358114","messageId":"20180914060552.GD219147@aiede.svl.corp.google.com","threadId":"49323","inReplyTo":"87k1npkzh5.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/3] archive: implement protocol v2 archive command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-14T06:05:52Z","receivedAt":"2018-09-14T06:05:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Wed, Sep 12 2018, Stefan Beller wrote:\n\n>> Would asking for a setlocale() on the server side be an unreasonable\n>> feature request for the capabilities (in a follow up patch, and then not\n>> just for archive but also fetch/push, etc.)?\n>\n> This would be very nice to have, but as you suggest in some follow-up\n> change.\n\nIndeed, I think we've gone pretty far afield from the goal of this\npatch series.\n\n> I think though that instead of doing setlocale() it would be better to\n> pass some flag saying we're operating in a machine-readable mode, and\n> then we'd (as part of the protocol defintion) say we're going to emit\n> GIT_ERR_UPLOAD_ARCHIVE_EXPECTED_DELIM_PACKET or whatever.\n\nI think you're suggesting client-side message generation, and that is\none way to handle internationalization of server output.\n\nThe main downside is when the server really does want to provide a\ncustom error message. For that, we'd need\n\n 1. To propagate LANG to the server, so it knows what human language\n to generate messages in.\n\n 2. On the server side, to produce messages in that language if\n available, with an appropriate fallback if not.\n\nWe've been thinking of doing at least (1) using the same trick as\nserver-options use (cramming it into client capabilities).\n\nIt is difficult to use setlocale for this because it affects the whole\nprogram (problematic for a threaded server) and affects features like\ncollation order instead of just message generation (problematic for\nmany things). Does gettext have a variant that takes a locale_t\nargument?\n\n[...]\n> 4) Aside from translation purposes, getting a machine-readable\n> \"push/pull\" etc. mode would be very handy. E.g. now you need to\n> parse stderr to see why exactly your push failed (hook denied, or\n> non-fast-forward, or non-fast-forward where there was a lock race\n> condition? ...).\n\nIndeed, this is a good reason to provide error codes instead of (in\nthe case where the message doesn't add anything to it) or alongside\n(in case the error message is more specialized) human-oriented error\nmessages.\n\nThanks,\nJonathan\n"},{"id":"358120","messageId":"87d0tgkv55.fsf@evledraar.gmail.com","threadId":"49323","inReplyTo":"20180914060552.GD219147@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/3] archive: implement protocol v2 archive command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-14T14:31:50Z","receivedAt":"2018-09-14T14:31:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 14 2018, Jonathan Nieder wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>> On Wed, Sep 12 2018, Stefan Beller wrote:\n>\n>>> Would asking for a setlocale() on the server side be an unreasonable\n>>> feature request for the capabilities (in a follow up patch, and then not\n>>> just for archive but also fetch/push, etc.)?\n>>\n>> This would be very nice to have, but as you suggest in some follow-up\n>> change.\n>\n> Indeed, I think we've gone pretty far afield from the goal of this\n> patch series.\n>\n>> I think though that instead of doing setlocale() it would be better to\n>> pass some flag saying we're operating in a machine-readable mode, and\n>> then we'd (as part of the protocol defintion) say we're going to emit\n>> GIT_ERR_UPLOAD_ARCHIVE_EXPECTED_DELIM_PACKET or whatever.\n>\n> I think you're suggesting client-side message generation, and that is\n> one way to handle internationalization of server output.\n>\n> The main downside is when the server really does want to provide a\n> custom error message. For that, we'd need\n\nYeah you can't do it for everything. E.g. hooks will want to spew out\ncustom messages, and this hypothetical protocol extension won't have\ncodes for those. So you'll still need to pass $LANG along.\n\n> 1. To propagate LANG to the server, so it knows what human language\n> to generate messages in.\n>\n> 2. On the server side, to produce messages in that language if\n> available, with an appropriate fallback if not.\n>\n> We've been thinking of doing at least (1) using the same trick as\n> server-options use (cramming it into client capabilities).\n>\n> It is difficult to use setlocale for this because it affects the whole\n> program (problematic for a threaded server) and affects features like\n> collation order instead of just message generation (problematic for\n> many things). Does gettext have a variant that takes a locale_t\n> argument?\n\nNo, its API is fairly crappy and depends on setlocale().\n\nKeep in mind though that we're not tied to that API. E.g. one way to\nwork around this problem is to simply loop through all the languages we\nhave translations for at server startup, for each one call setlocale()\nand gettext(), and save the result in a hash table for runtime lookup,\nthen you'd just call sprintf(hash[language][message_id], ...) at\nruntime.\n\nThat's all libintl is really doing under the hood, in a roundabout way\nwhere calls to setlocale() determine what table we're looking things up\nin.\n\nThe reason I opted to go for gettext to begin with was mainly a) it was\nthere b) the ubiquitous availability of tooling for translators when it\ncomes to the *.po files.\n\nBut the API for looking things up at runtime is fairly small, and easy\nto replace. We could e.g. replace all of our own gettext.[ch] wrapper\nwith something that works somewhat like what I described above, with\nsome extra build step to extract the relevant data from the *.mo files\n(or parse the *.po directly).\n\n> [...]\n>> 4) Aside from translation purposes, getting a machine-readable\n>> \"push/pull\" etc. mode would be very handy. E.g. now you need to\n>> parse stderr to see why exactly your push failed (hook denied, or\n>> non-fast-forward, or non-fast-forward where there was a lock race\n>> condition? ...).\n>\n> Indeed, this is a good reason to provide error codes instead of (in\n> the case where the message doesn't add anything to it) or alongside\n> (in case the error message is more specialized) human-oriented error\n> messages.\n"},{"id":"358130","messageId":"xmqq4lesdpk4.fsf@gitster-ct.c.googlers.com","threadId":"49323","inReplyTo":"20180914060552.GD219147@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/3] archive: implement protocol v2 archive command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-14T16:14:19Z","receivedAt":"2018-09-14T16:14:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> I think though that instead of doing setlocale() it would be better to\n>> pass some flag saying we're operating in a machine-readable mode, and\n>> then we'd (as part of the protocol defintion) say we're going to emit\n>> GIT_ERR_UPLOAD_ARCHIVE_EXPECTED_DELIM_PACKET or whatever.\n>\n> I think you're suggesting client-side message generation, and that is\n> one way to handle internationalization of server output.\n>\n> The main downside is when the server really does want to provide a\n> custom error message. For that, we'd need\n>\n> 1. To propagate LANG to the server, so it knows what human language\n> to generate messages in.\n>\n> 2. On the server side, to produce messages in that language if\n> available, with an appropriate fallback if not.\n\nThat is one way to do so, but it does not have to be the only way, I\nwould think. You can send a machine parsable message in pieces, and\nassemble the parts of speech into a message at the receiving end.\nLike sending a msgid to identify an entry in the .pot file, and\nvalues to be filled in.\n\n"},{"id":"358131","messageId":"20180914161956.GA133420@aiede.svl.corp.google.com","threadId":"49323","inReplyTo":"xmqq4lesdpk4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] archive: implement protocol v2 archive command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-14T16:19:56Z","receivedAt":"2018-09-14T16:20:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>>> I think though that instead of doing setlocale() it would be better to\n>>> pass some flag saying we're operating in a machine-readable mode, and\n>>> then we'd (as part of the protocol defintion) say we're going to emit\n>>> GIT_ERR_UPLOAD_ARCHIVE_EXPECTED_DELIM_PACKET or whatever.\n>>\n>> I think you're suggesting client-side message generation, and that is\n>> one way to handle internationalization of server output.\n>>\n>> The main downside is when the server really does want to provide a\n>> custom error message. For that, we'd need\n>>\n>> 1. To propagate LANG to the server, so it knows what human language\n>> to generate messages in.\n>>\n>> 2. On the server side, to produce messages in that language if\n>> available, with an appropriate fallback if not.\n>\n> That is one way to do so, but it does not have to be the only way, I\n> would think. You can send a machine parsable message in pieces, and\n> assemble the parts of speech into a message at the receiving end.\n> Like sending a msgid to identify an entry in the .pot file, and\n> values to be filled in.\n\nThat works if the same party controls the client and server and the\nclient is up to date enough to know about every message the server\nwould want to send.\n\nIt doesn't work for\n- hooks\n- alternate server implementations\n- messages involved in an emergency fix\n- ... etc ...\n\nDon't get me wrong: for messages with a machine as an audience, error\ncodes or similar structured errors are a great way to go, and getting\nclient-side generation of messages for humans (not to mention styling,\netc) are a nice bonus there. I stand by what's in the message you're\nreplying to, though: if we actually want to be able to consistently\nprovide useful messages to people who do not like to read English,\nthen client-side generation won't get us all the way there.\n\nJonathan\n"},{"id":"359022","messageId":"20180927012455.234876-1-steadmon@google.com","threadId":"49323","inReplyTo":"20180912053519.31085-1-steadmon@google.com","subject":"[PATCH v2 0/4] Add proto v2 archive command with HTTP support","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-09-27T01:24:51Z","receivedAt":"2018-09-27T01:25:26Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"This is the second version of my series to add a new protocol v2 command\nfor archiving, with support for HTTP(S).\n\nNEEDSWORK: a server built with this series is not backwards-compatible\nwith clients that set GIT_PROTOCOL=version=2 or configure\nprotocol.version=2. The old client will unconditionally send \"argument\n...\" packet lines, which breaks the server's expectations of a\n\"command=archive\" request, while the server's capability advertisement\nin turn breaks the clients expectation of either an ACK or NACK.\n\nI've been discussing workarounds for this with Jonathan Nieder, but\nplease let me know if you have any suggestions for v3 of this series.\n\n\nJosh Steadmon (4):\n archive: follow test standards around assertions\n archive: use packet_reader for communications\n archive: implement protocol v2 archive command\n archive: allow archive over HTTP(S) with proto v2\n\n Documentation/technical/protocol-v2.txt | 21 ++++++++-\n builtin/archive.c | 58 +++++++++++++++++++------\n builtin/upload-archive.c | 27 ++++++++++--\n http-backend.c | 13 +++++-\n serve.c | 7 +++\n t/t5000-tar-tree.sh | 33 +++++++-------\n t/t5701-git-serve.sh | 1 +\n transport-helper.c | 7 +--\n 8 files changed, 130 insertions(+), 37 deletions(-)\n\nRange-diff against v1:\n-: ---------- > 1: c2e371ad24 archive: follow test standards around assertions\n1: b514184273 ! 2: a65f73f627 archive: use packet_reader for communications\n @@ -6,7 +6,10 @@\n handling, which will make implementation of protocol v2 support in\n git-archive easier.\n \n + This refactoring does not change the behavior of \"git archive\".\n +\n Signed-off-by: Josh Steadmon \n + Reviewed-by: Stefan Beller \n \n diff --git a/builtin/archive.c b/builtin/archive.c\n @@ -42,24 +45,24 @@\n -\tif (!buf)\n +\tstatus = packet_reader_read(&reader);\n +\n -+\tif (status == PACKET_READ_FLUSH)\n ++\tif (status != PACKET_READ_NORMAL || reader.pktlen <= 0)\n \t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n -\tif (strcmp(buf, \"ACK\")) {\n -\t\tif (starts_with(buf, \"NACK \"))\n -\t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n -\t\tif (starts_with(buf, \"ERR \"))\n -\t\t\tdie(_(\"remote error: %s\"), buf + 4);\n -+\tif (strcmp(reader.buffer, \"ACK\")) {\n -+\t\tif (starts_with(reader.buffer, \"NACK \"))\n -+\t\t\tdie(_(\"git archive: NACK %s\"), reader.buffer + 5);\n -+\t\tif (starts_with(reader.buffer, \"ERR \"))\n -+\t\t\tdie(_(\"remote error: %s\"), reader.buffer + 4);\n ++\tif (strcmp(reader.line, \"ACK\")) {\n ++\t\tif (starts_with(reader.line, \"NACK \"))\n ++\t\t\tdie(_(\"git archive: NACK %s\"), reader.line + 5);\n ++\t\tif (starts_with(reader.line, \"ERR \"))\n ++\t\t\tdie(_(\"remote error: %s\"), reader.line + 4);\n \t\tdie(_(\"git archive: protocol error\"));\n \t}\n \n -\tif (packet_read_line(fd[0], NULL))\n +\tstatus = packet_reader_read(&reader);\n -+\tif (status != PACKET_READ_FLUSH)\n ++\tif (status == PACKET_READ_NORMAL && reader.pktlen > 0)\n \t\tdie(_(\"git archive: expected a flush\"));\n \n \t/* Now, start reading from fd[0] and spit it out to stdout */\n2: 1518c15dc1 < -: ---------- archive: implement protocol v2 archive command\n-: ---------- > 3: 0a8cc5e331 archive: implement protocol v2 archive command\n3: 1b7ad8d8f6 ! 4: 97a1424f32 archive: allow archive over HTTP(S) with proto v2\n @@ -10,16 +10,20 @@\n +++ b/builtin/archive.c\n @@\n \t\tstatus = packet_reader_read(&reader);\n - \t\tif (status != PACKET_READ_FLUSH)\n + \t\tif (status == PACKET_READ_NORMAL && reader.pktlen > 0)\n \t\t\tdie(_(\"git archive: expected a flush\"));\n -\t}\n +\t} else if (version == protocol_v2 &&\n -+\t\t starts_with(transport->url, \"http\"))\n ++\t\t (starts_with(transport->url, \"http://\") ||\n ++\t\t starts_with(transport->url, \"https://\")))\n +\t\t/*\n +\t\t * Commands over HTTP require two requests, so there's an\n -+\t\t * additional server response to parse.\n ++\t\t * additional server response to parse. We do only basic sanity\n ++\t\t * checking here that the versions presented match across\n ++\t\t * requests.\n +\t\t */\n -+\t\tdiscover_version(&reader);\n ++\t\tif (version != discover_version(&reader))\n ++\t\t\tdie(_(\"git archive: received different protocol versions in subsequent requests\"));\n \n \t/* Now, start reading from fd[0] and spit it out to stdout */\n \trv = recv_sideband(\"archive\", fd[0], 1);\n @@ -40,7 +44,10 @@\n \tstruct strbuf buf = STRBUF_INIT;\n \n +\tif (!strcmp(service_name, \"git-upload-archive\")) {\n -+\t\t/* git-upload-archive doesn't need --stateless-rpc */\n ++\t\t/*\n ++\t\t * git-upload-archive doesn't need --stateless-rpc, because it\n ++\t\t * always handles only a single request.\n ++\t\t */\n +\t\targv[1] = \".\";\n +\t\targv[2] = NULL;\n +\t}\n @@ -63,10 +70,12 @@\n --- a/transport-helper.c\n +++ b/transport-helper.c\n @@\n + \t\tstrbuf_addf(&cmdbuf, \"connect %s\\n\", name);\n \t\tret = run_connect(transport, &cmdbuf);\n \t} else if (data->stateless_connect &&\n - \t\t (get_protocol_version_config() == protocol_v2) &&\n +-\t\t (get_protocol_version_config() == protocol_v2) &&\n -\t\t !strcmp(\"git-upload-pack\", name)) {\n ++\t\t get_protocol_version_config() == protocol_v2 &&\n +\t\t (!strcmp(\"git-upload-pack\", name) ||\n +\t\t !strcmp(\"git-upload-archive\", name))) {\n \t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359023","messageId":"20180927012455.234876-2-steadmon@google.com","threadId":"49323","inReplyTo":"20180927012455.234876-1-steadmon@google.com","subject":"[PATCH v2 1/4] archive: follow test standards around assertions","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-09-27T01:24:52Z","receivedAt":"2018-09-27T01:25:37Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Move assertions outside of the check_tar function so that all top-level\ncode is wrapped in a test_expect_* assertion.\n\nSigned-off-by: Josh Steadmon \n---\n t/t5000-tar-tree.sh | 26 ++++++++++----------------\n 1 file changed, 10 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 2a97b27b0a..c408e3a23d 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -62,11 +62,9 @@ check_tar() {\n \tdir=$1\n \tdir_with_prefix=$dir/$2\n \n-\ttest_expect_success ' extract tar archive' '\n-\t\t(mkdir $dir && cd $dir && \"$TAR\" xf -) <$tarfile\n-\t'\n+\t(mkdir $dir && cd $dir && \"$TAR\" xf -) <$tarfile &&\n \n-\ttest_expect_success TAR_NEEDS_PAX_FALLBACK ' interpret pax headers' '\n+\tif test_have_prereq TAR_NEEDS_PAX_FALLBACK ; then\n \t\t(\n \t\t\tcd $dir &&\n \t\t\tfor header in *.paxheader\n@@ -82,16 +80,11 @@ check_tar() {\n \t\t\t\tfi\n \t\t\tdone\n \t\t)\n-\t'\n+\tfi &&\n \n-\ttest_expect_success ' validate filenames' '\n-\t\t(cd ${dir_with_prefix}a && find .) | sort >$listfile &&\n-\t\ttest_cmp a.lst $listfile\n-\t'\n-\n-\ttest_expect_success ' validate file contents' '\n-\t\tdiff -r a ${dir_with_prefix}a\n-\t'\n+\t(cd ${dir_with_prefix}a && find .) | sort >$listfile &&\n+\ttest_cmp a.lst $listfile &&\n+\tdiff -r a ${dir_with_prefix}a\n }\n \n test_expect_success \\\n@@ -143,19 +136,20 @@ test_expect_success \\\n 'git archive' \\\n 'git archive HEAD >b.tar'\n \n-check_tar b\n+test_expect_success 'extract archive' 'check_tar b'\n \n test_expect_success 'git archive --prefix=prefix/' '\n \tgit archive --prefix=prefix/ HEAD >with_prefix.tar\n '\n \n-check_tar with_prefix prefix/\n+test_expect_success 'extract with prefix' 'check_tar with_prefix prefix/'\n \n test_expect_success 'git-archive --prefix=olde-' '\n \tgit archive --prefix=olde- HEAD >with_olde-prefix.tar\n '\n \n-check_tar with_olde-prefix olde-\n+test_expect_success 'extract with olde- prefix' \\\n+\t'check_tar with_olde-prefix olde-'\n \n test_expect_success 'git archive on large files' '\n test_config core.bigfilethreshold 1 &&\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359024","messageId":"20180927012455.234876-3-steadmon@google.com","threadId":"49323","inReplyTo":"20180927012455.234876-1-steadmon@google.com","subject":"[PATCH v2 2/4] archive: use packet_reader for communications","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-09-27T01:24:53Z","receivedAt":"2018-09-27T01:25:39Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Using packet_reader will simplify version detection and capability\nhandling, which will make implementation of protocol v2 support in\ngit-archive easier.\n\nThis refactoring does not change the behavior of \"git archive\".\n\nSigned-off-by: Josh Steadmon \n---\n builtin/archive.c | 23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex e74f675390..4eb547c5b7 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -27,10 +27,11 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\t\t const char *remote, const char *exec,\n \t\t\t const char *name_hint)\n {\n-\tchar *buf;\n \tint fd[2], i, rv;\n \tstruct transport *transport;\n \tstruct remote *_remote;\n+\tstruct packet_reader reader;\n+\tenum packet_read_status status;\n \n \t_remote = remote_get(remote);\n \tif (!_remote->url[0])\n@@ -38,6 +39,8 @@ static int run_remote_archiver(int argc, const char **argv,\n \ttransport = transport_get(_remote, _remote->url[0]);\n \ttransport_connect(transport, \"git-upload-archive\", exec, fd);\n \n+\tpacket_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n+\n \t/*\n \t * Inject a fake --format field at the beginning of the\n \t * arguments, with the format inferred from our output\n@@ -53,18 +56,20 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\tpacket_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n \tpacket_flush(fd[1]);\n \n-\tbuf = packet_read_line(fd[0], NULL);\n-\tif (!buf)\n+\tstatus = packet_reader_read(&reader);\n+\n+\tif (status != PACKET_READ_NORMAL || reader.pktlen <= 0)\n \t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n-\tif (strcmp(buf, \"ACK\")) {\n-\t\tif (starts_with(buf, \"NACK \"))\n-\t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n-\t\tif (starts_with(buf, \"ERR \"))\n-\t\t\tdie(_(\"remote error: %s\"), buf + 4);\n+\tif (strcmp(reader.line, \"ACK\")) {\n+\t\tif (starts_with(reader.line, \"NACK \"))\n+\t\t\tdie(_(\"git archive: NACK %s\"), reader.line + 5);\n+\t\tif (starts_with(reader.line, \"ERR \"))\n+\t\t\tdie(_(\"remote error: %s\"), reader.line + 4);\n \t\tdie(_(\"git archive: protocol error\"));\n \t}\n \n-\tif (packet_read_line(fd[0], NULL))\n+\tstatus = packet_reader_read(&reader);\n+\tif (status == PACKET_READ_NORMAL && reader.pktlen > 0)\n \t\tdie(_(\"git archive: expected a flush\"));\n \n \t/* Now, start reading from fd[0] and spit it out to stdout */\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359025","messageId":"20180927012455.234876-4-steadmon@google.com","threadId":"49323","inReplyTo":"20180927012455.234876-1-steadmon@google.com","subject":"[PATCH v2 3/4] archive: implement protocol v2 archive command","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-09-27T01:24:54Z","receivedAt":"2018-09-27T01:25:43Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"This adds a new archive command for protocol v2. The command expects\narguments in the form \"argument X\" which are passed unmodified to\ngit-upload-archive--writer.\n\nThis command works over the file://, Git, and SSH transports. HTTP\nsupport will be added in a separate patch.\n\nNEEDSWORK: this is not backwards-compatible with older clients that set\nGIT_PROTOCOL=version=2 or configure protocol.version=2.\n\nSigned-off-by: Josh Steadmon \n---\n Documentation/technical/protocol-v2.txt | 21 +++++++++++-\n builtin/archive.c | 45 +++++++++++++++++--------\n builtin/upload-archive.c | 27 +++++++++++++--\n serve.c | 7 ++++\n t/t5000-tar-tree.sh | 7 ++++\n t/t5701-git-serve.sh | 1 +\n 6 files changed, 90 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 09e4e0273f..6126fc5738 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -77,7 +77,8 @@ A v2 server would reply:\n S: \n \n Subsequent requests are then made directly to the service\n-`$GIT_URL/git-upload-pack`. (This works the same for git-receive-pack).\n+`$GIT_URL/git-upload-pack`. (This works the same for git-receive-pack or\n+git-upload-archive).\n \n Capability Advertisement\n --------------------------\n@@ -168,6 +169,24 @@ printable ASCII characters except space (i.e., the byte range 32 < x <\n and debugging purposes, and MUST NOT be used to programmatically assume\n the presence or absence of particular features.\n \n+ archive\n+~~~~~~~~~\n+\n+`archive` is the command to request an archive (such as a tarball) from a server\n+over v2.\n+\n+archive takes the following optional arguments:\n+\n+ argument \n+ This specifies that should be passed as an argument to the\n+ underlying archive writer. This option can be repeated to pass\n+ additional arguments to the archive writer.\n+\n+See linkgit:git-archive[1] for allowed values of ``.\n+\n+The output of archive is a packet-line stream of the resulting archive, with\n+error messages transferred over the sideband channel.\n+\n ls-refs\n ~~~~~~~~~\n \ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 4eb547c5b7..f91d222677 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -5,9 +5,11 @@\n #include \"cache.h\"\n #include \"builtin.h\"\n #include \"archive.h\"\n+#include \"connect.h\"\n #include \"transport.h\"\n #include \"parse-options.h\"\n #include \"pkt-line.h\"\n+#include \"protocol.h\"\n #include \"sideband.h\"\n \n static void create_output_file(const char *output_file)\n@@ -23,6 +25,13 @@ static void create_output_file(const char *output_file)\n \t}\n }\n \n+static void do_v2_command_and_cap(int out)\n+{\n+\tpacket_write_fmt(out, \"command=archive\\n\");\n+\t/* Capability list would go here, if we wanted to request any. */\n+\tpacket_delim(out);\n+}\n+\n static int run_remote_archiver(int argc, const char **argv,\n \t\t\t const char *remote, const char *exec,\n \t\t\t const char *name_hint)\n@@ -32,6 +41,7 @@ static int run_remote_archiver(int argc, const char **argv,\n \tstruct remote *_remote;\n \tstruct packet_reader reader;\n \tenum packet_read_status status;\n+\tenum protocol_version version;\n \n \t_remote = remote_get(remote);\n \tif (!_remote->url[0])\n@@ -41,6 +51,11 @@ static int run_remote_archiver(int argc, const char **argv,\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n \n+\tversion = discover_version(&reader);\n+\n+\tif (version == protocol_v2 && server_supports_v2(\"archive\", 0))\n+\t\tdo_v2_command_and_cap(fd[1]);\n+\n \t/*\n \t * Inject a fake --format field at the beginning of the\n \t * arguments, with the format inferred from our output\n@@ -56,22 +71,24 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\tpacket_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n \tpacket_flush(fd[1]);\n \n-\tstatus = packet_reader_read(&reader);\n-\n-\tif (status != PACKET_READ_NORMAL || reader.pktlen <= 0)\n-\t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n-\tif (strcmp(reader.line, \"ACK\")) {\n-\t\tif (starts_with(reader.line, \"NACK \"))\n-\t\t\tdie(_(\"git archive: NACK %s\"), reader.line + 5);\n-\t\tif (starts_with(reader.line, \"ERR \"))\n-\t\t\tdie(_(\"remote error: %s\"), reader.line + 4);\n-\t\tdie(_(\"git archive: protocol error\"));\n+\tif (version == protocol_v0) {\n+\t\tstatus = packet_reader_read(&reader);\n+\n+\t\tif (status != PACKET_READ_NORMAL || reader.pktlen <= 0)\n+\t\t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n+\t\tif (strcmp(reader.line, \"ACK\")) {\n+\t\t\tif (starts_with(reader.line, \"NACK \"))\n+\t\t\t\tdie(_(\"git archive: NACK %s\"), reader.line + 5);\n+\t\t\tif (starts_with(reader.line, \"ERR \"))\n+\t\t\t\tdie(_(\"remote error: %s\"), reader.line + 4);\n+\t\t\tdie(_(\"git archive: protocol error\"));\n+\t\t}\n+\n+\t\tstatus = packet_reader_read(&reader);\n+\t\tif (status == PACKET_READ_NORMAL && reader.pktlen > 0)\n+\t\t\tdie(_(\"git archive: expected a flush\"));\n \t}\n \n-\tstatus = packet_reader_read(&reader);\n-\tif (status == PACKET_READ_NORMAL && reader.pktlen > 0)\n-\t\tdie(_(\"git archive: expected a flush\"));\n-\n \t/* Now, start reading from fd[0] and spit it out to stdout */\n \trv = recv_sideband(\"archive\", fd[0], 1);\n \trv |= transport_disconnect(transport);\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 25d9116356..b8a8fb256f 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -5,6 +5,8 @@\n #include \"builtin.h\"\n #include \"archive.h\"\n #include \"pkt-line.h\"\n+#include \"protocol.h\"\n+#include \"serve.h\"\n #include \"sideband.h\"\n #include \"run-command.h\"\n #include \"argv-array.h\"\n@@ -76,10 +78,24 @@ static ssize_t process_input(int child_fd, int band)\n int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n {\n \tstruct child_process writer = { argv };\n+\tenum protocol_version version = determine_protocol_version_server();\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(upload_archive_usage);\n \n+\tif (version == protocol_v2) {\n+\t\tstruct serve_options options = SERVE_OPTIONS_INIT;\n+\n+\t\t/* Send \"version 2\" message and capabilities. */\n+\t\toptions.advertise_capabilities = 1;\n+\t\tserve(&options);\n+\n+\t\t/* Process command request and validate client capabilities. */\n+\t\toptions.advertise_capabilities = 0;\n+\t\toptions.stateless_rpc = 1;\n+\t\tserve(&options);\n+\t}\n+\n \t/*\n \t * Set up sideband subprocess.\n \t *\n@@ -92,12 +108,17 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n \twriter.git_cmd = 1;\n \tif (start_command(&writer)) {\n \t\tint err = errno;\n-\t\tpacket_write_fmt(1, \"NACK unable to spawn subprocess\\n\");\n+\t\tif (version == protocol_v0 || version == protocol_v1)\n+\t\t\tpacket_write_fmt(1, \"NACK unable to spawn subprocess\\n\");\n+\t\telse if (version == protocol_v2)\n+\t\t\terror_clnt(\"unable to spawn subprocess\\n\");\n \t\tdie(\"upload-archive: %s\", strerror(err));\n \t}\n \n-\tpacket_write_fmt(1, \"ACK\\n\");\n-\tpacket_flush(1);\n+\tif (version == protocol_v0 || version == protocol_v1) {\n+\t\tpacket_write_fmt(1, \"ACK\\n\");\n+\t\tpacket_flush(1);\n+\t}\n \n \twhile (1) {\n \t\tstruct pollfd pfd[2];\ndiff --git a/serve.c b/serve.c\nindex bda085f09c..cef575086f 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -52,8 +52,15 @@ struct protocol_capability {\n \t\t struct packet_reader *request);\n };\n \n+static int noop(struct repository *r, struct argv_array *keys,\n+\t\tstruct packet_reader *request)\n+{\n+\treturn 0;\n+}\n+\n static struct protocol_capability capabilities[] = {\n \t{ \"agent\", agent_advertise, NULL },\n+\t{ \"archive\", always_advertise, noop },\n \t{ \"ls-refs\", always_advertise, ls_refs },\n \t{ \"fetch\", upload_pack_advertise, upload_pack_v2 },\n \t{ \"server-option\", always_advertise, NULL },\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex c408e3a23d..4a76354021 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -138,6 +138,13 @@ test_expect_success \\\n \n test_expect_success 'extract archive' 'check_tar b'\n \n+test_expect_success 'protocol v2 for remote' '\n+\tGIT_PROTOCOL=\"version=2\" GIT_TRACE_PACKET=\"$(pwd)/log\" git archive \\\n+\t\t--remote=. HEAD >v2_remote.tar &&\n+\tgrep \"version 2\" log\n+'\n+test_expect_success 'extract v2 archive' 'check_tar v2_remote'\n+\n test_expect_success 'git archive --prefix=prefix/' '\n \tgit archive --prefix=prefix/ HEAD >with_prefix.tar\n '\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex 75ec79e6cb..f47a14660b 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -8,6 +8,7 @@ test_expect_success 'test capability advertisement' '\n \tcat >expect <<-EOF &&\n \tversion 2\n \tagent=git/$(git version | cut -d\" \" -f3)\n+\tarchive\n \tls-refs\n \tfetch=shallow\n \tserver-option\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359026","messageId":"20180927012455.234876-5-steadmon@google.com","threadId":"49323","inReplyTo":"20180927012455.234876-1-steadmon@google.com","subject":"[PATCH v2 4/4] archive: allow archive over HTTP(S) with proto v2","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-09-27T01:24:55Z","receivedAt":"2018-09-27T01:25:46Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Signed-off-by: Josh Steadmon \n---\n builtin/archive.c | 12 +++++++++++-\n http-backend.c | 13 ++++++++++++-\n transport-helper.c | 7 ++++---\n 3 files changed, 27 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex f91d222677..78a259518d 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -87,7 +87,17 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\tstatus = packet_reader_read(&reader);\n \t\tif (status == PACKET_READ_NORMAL && reader.pktlen > 0)\n \t\t\tdie(_(\"git archive: expected a flush\"));\n-\t}\n+\t} else if (version == protocol_v2 &&\n+\t\t (starts_with(transport->url, \"http://\") ||\n+\t\t starts_with(transport->url, \"https://\")))\n+\t\t/*\n+\t\t * Commands over HTTP require two requests, so there's an\n+\t\t * additional server response to parse. We do only basic sanity\n+\t\t * checking here that the versions presented match across\n+\t\t * requests.\n+\t\t */\n+\t\tif (version != discover_version(&reader))\n+\t\t\tdie(_(\"git archive: received different protocol versions in subsequent requests\"));\n \n \t/* Now, start reading from fd[0] and spit it out to stdout */\n \trv = recv_sideband(\"archive\", fd[0], 1);\ndiff --git a/http-backend.c b/http-backend.c\nindex 9e894f197f..8e262d50e0 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -32,6 +32,7 @@ struct rpc_service {\n static struct rpc_service rpc_service[] = {\n \t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n \t{ \"receive-pack\", \"receivepack\", 0, -1 },\n+\t{ \"upload-archive\", \"uploadarchive\", 1, 1 },\n };\n \n static struct string_list *get_parameters(void)\n@@ -637,6 +638,15 @@ static void service_rpc(struct strbuf *hdr, char *service_name)\n \tstruct rpc_service *svc = select_service(hdr, service_name);\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tif (!strcmp(service_name, \"git-upload-archive\")) {\n+\t\t/*\n+\t\t * git-upload-archive doesn't need --stateless-rpc, because it\n+\t\t * always handles only a single request.\n+\t\t */\n+\t\targv[1] = \".\";\n+\t\targv[2] = NULL;\n+\t}\n+\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"application/x-git-%s-request\", svc->name);\n \tcheck_content_type(hdr, buf.buf);\n@@ -713,7 +723,8 @@ static struct service_cmd {\n \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{40}\\\\.idx$\", get_idx_file},\n \n \t{\"POST\", \"/git-upload-pack$\", service_rpc},\n-\t{\"POST\", \"/git-receive-pack$\", service_rpc}\n+\t{\"POST\", \"/git-receive-pack$\", service_rpc},\n+\t{\"POST\", \"/git-upload-archive$\", service_rpc},\n };\n \n static int bad_request(struct strbuf *hdr, const struct service_cmd *c)\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 143ca008c8..c41c3dfcff 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -604,8 +604,9 @@ static int process_connect_service(struct transport *transport,\n \t\tstrbuf_addf(&cmdbuf, \"connect %s\\n\", name);\n \t\tret = run_connect(transport, &cmdbuf);\n \t} else if (data->stateless_connect &&\n-\t\t (get_protocol_version_config() == protocol_v2) &&\n-\t\t !strcmp(\"git-upload-pack\", name)) {\n+\t\t get_protocol_version_config() == protocol_v2 &&\n+\t\t (!strcmp(\"git-upload-pack\", name) ||\n+\t\t !strcmp(\"git-upload-archive\", name))) {\n \t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n \t\tret = run_connect(transport, &cmdbuf);\n \t\tif (ret)\n@@ -639,7 +640,7 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \t/* Get_helper so connect is inited. */\n \tget_helper(transport);\n-\tif (!data->connect)\n+\tif (!data->connect && !data->stateless_connect)\n \t\tdie(_(\"operation not supported by protocol\"));\n \n \tif (!process_connect_service(transport, name, exec))\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359082","messageId":"CAGZ79kaBvHwUUf0rXeAwBY-M+Oi9JjsQnLs4v3FAvx4a9ZRbSA@mail.gmail.com","threadId":"49323","inReplyTo":"20180927012455.234876-1-steadmon@google.com","subject":"Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-27T18:20:13Z","receivedAt":"2018-09-27T18:20:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 26, 2018 at 6:25 PM Josh Steadmon wrote:\n>\n> This is the second version of my series to add a new protocol v2 command\n> for archiving, with support for HTTP(S).\n>\n> NEEDSWORK: a server built with this series is not backwards-compatible\n> with clients that set GIT_PROTOCOL=version=2 or configure\n> protocol.version=2. The old client will unconditionally send \"argument\n> ...\" packet lines, which breaks the server's expectations of a\n> \"command=archive\" request,\n\nSo if an old client sets protocol to v2, it would only apply that\nprotocol version\nto fetch, not archive, so it would start following a v0 conversation, but\nas the protocol version is set, it would be transmitted to the server.\nThis sounds like a bug in the client?\n\n> while the server's capability advertisement\n> in turn breaks the clients expectation of either an ACK or NACK.\n\nCould a modern client send either another protocol version (3?)\nor a special capability along the protocol version (\"fixed_archive\")\n\n> I've been discussing workarounds for this with Jonathan Nieder, but\n> please let me know if you have any suggestions for v3 of this series.\n\nCare to open the discussion to the list? What are the different\napproaches, what are the pros/cons?\n"},{"id":"359084","messageId":"20180927183017.GD112066@aiede.svl.corp.google.com","threadId":"49323","inReplyTo":"CAGZ79kaBvHwUUf0rXeAwBY-M+Oi9JjsQnLs4v3FAvx4a9ZRbSA@mail.gmail.com","subject":"Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-27T18:30:17Z","receivedAt":"2018-09-27T18:30:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Wed, Sep 26, 2018 at 6:25 PM Josh Steadmon wrote:\n\n>> I've been discussing workarounds for this with Jonathan Nieder, but\n>> please let me know if you have any suggestions for v3 of this series.\n>\n> Care to open the discussion to the list? What are the different\n> approaches, what are the pros/cons?\n\nDo you mean sending video of chatting in the office?\n\nJosh and I discussed that\n\n 1. Clients sending version=2 when they do not, in fact, speak protocol\n v2 for a service is a (serious) bug. (Separately from this\n series) we should fix it.\n\n 2. That bug is already in the wild, alas. Fortunately the semantics of\n GIT_PROTOCOL as a list of key/value pairs is well defined. So we\n have choices of (a) bump version to version=3 (b) pass another\n value 'version=2:yesreallyversion=2' (c) etc.\n\n 3. This is likely to affect push, too.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"359085","messageId":"20180927183036.GA55647@google.com","threadId":"49323","inReplyTo":"CAGZ79kaBvHwUUf0rXeAwBY-M+Oi9JjsQnLs4v3FAvx4a9ZRbSA@mail.gmail.com","subject":"Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-09-27T18:30:36Z","receivedAt":"2018-09-27T18:30:45Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.09.27 11:20, Stefan Beller wrote:\n> On Wed, Sep 26, 2018 at 6:25 PM Josh Steadmon wrote:\n> >\n> > This is the second version of my series to add a new protocol v2 command\n> > for archiving, with support for HTTP(S).\n> >\n> > NEEDSWORK: a server built with this series is not backwards-compatible\n> > with clients that set GIT_PROTOCOL=version=2 or configure\n> > protocol.version=2. The old client will unconditionally send \"argument\n> > ...\" packet lines, which breaks the server's expectations of a\n> > \"command=archive\" request,\n> \n> So if an old client sets protocol to v2, it would only apply that\n> protocol version\n> to fetch, not archive, so it would start following a v0 conversation, but\n> as the protocol version is set, it would be transmitted to the server.\n> This sounds like a bug in the client?\n\nYeah, basically. We're telling the server we support v2, even if the\nspecific operation we're trying to do doesn't have a v2 implementation\non the client. So this is going to make it ugly to replace existing\ncommands.\n\n> > while the server's capability advertisement\n> > in turn breaks the clients expectation of either an ACK or NACK.\n> \n> Could a modern client send either another protocol version (3?)\n> or a special capability along the protocol version (\"fixed_archive\")\n> \n> > I've been discussing workarounds for this with Jonathan Nieder, but\n> > please let me know if you have any suggestions for v3 of this series.\n> \n> Care to open the discussion to the list? What are the different\n> approaches, what are the pros/cons?\n\nJonathan suggested something along the lines of what you said above,\nadding a new field in GIT_PROTOCOL. So we'd send something like\n\"version=2:archive_version=2\" and have the server detect the latter.\n\nI'm not sure if that's the best way to go about this since I'm not\nfamiliar with the version detection code for other parts of the system.\nI worry that it will lead us down the path of having to specify a\nversion for every command that we eventually convert to protocol v2. On\nthe other hand, I don't see any other way to work around this, at least\nin the archive case. We can't peek at the client's transmissions on the\nserver, because v2 requires that the server speaks first...\n"},{"id":"359088","messageId":"CAGZ79kZ2aqW7X57KHPtjQiyWtwfk4SGZYRqwccgy9ji0s+sYYQ@mail.gmail.com","threadId":"49323","inReplyTo":"20180927012455.234876-2-steadmon@google.com","subject":"Re: [PATCH v2 1/4] archive: follow test standards around assertions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-27T18:38:04Z","receivedAt":"2018-09-27T18:38:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 26, 2018 at 6:25 PM Josh Steadmon wrote:\n>\n> Move assertions outside of the check_tar function so that all top-level\n> code is wrapped in a test_expect_* assertion.\n\nCool, I'll file this under modernizing the test infrastructure ;-)\n\n> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n> index 2a97b27b0a..c408e3a23d 100755\n> --- a/t/t5000-tar-tree.sh\n> +++ b/t/t5000-tar-tree.sh\n> @@ -62,11 +62,9 @@ check_tar() {\n> dir=$1\n> dir_with_prefix=$dir/$2\n>\n> - test_expect_success ' extract tar archive' '\n> - (mkdir $dir && cd $dir && \"$TAR\" xf -) <$tarfile\n> - '\n> + (mkdir $dir && cd $dir && \"$TAR\" xf -) <$tarfile &&\n>\n> - test_expect_success TAR_NEEDS_PAX_FALLBACK ' interpret pax headers' '\n> + if test_have_prereq TAR_NEEDS_PAX_FALLBACK ; then\n> (\n> cd $dir &&\n> for header in *.paxheader\n> @@ -82,16 +80,11 @@ check_tar() {\n> fi\n> done\n> )\n> - '\n> + fi &&\n>\n> - test_expect_success ' validate filenames' '\n> - (cd ${dir_with_prefix}a && find .) | sort >$listfile &&\n> - test_cmp a.lst $listfile\n> - '\n> -\n> - test_expect_success ' validate file contents' '\n> - diff -r a ${dir_with_prefix}a\n> - '\n> + (cd ${dir_with_prefix}a && find .) | sort >$listfile &&\n> + test_cmp a.lst $listfile &&\n> + diff -r a ${dir_with_prefix}a\n\nUp to here we unwrapped code and removed test_expect_success\nand just executed the code as is, so later callers would need to encapsulate\nthe call to check_tar with test_expect_success.\n\nHowever as we are touching the code here, we can go further than just\nunwrapping it, usually we'd format one command a line,\n\n (\n cd ${dir_with_prefix}a &&\n find .\n ) | sort >$listfile &&\n test_cmp ...\n\nI am not sure if that standard style is more legible in this case though.\n\n> }\n>\n> test_expect_success \\\n> @@ -143,19 +136,20 @@ test_expect_success \\\n> 'git archive' \\\n> 'git archive HEAD >b.tar'\n>\n> -check_tar b\n> +test_expect_success 'extract archive' 'check_tar b'\n\nHeh. Just looked into the file and the surrounding code is\na wild mixture of the old style\n\ntest_expect_success \\\n 'git archive' \\\n 'git archive HEAD >b.tar'\n\ncheck_tar b\n\nand the new style\n\ntest_expect_success 'test name' '\n command &&\n command2\n'\n\nMaybe we could cleanup that file to look more like\none of the newer tests (e.g. t3206, t0410) ?\n\nBut I guess for the purpose of getting the check_tar\nfunction usable inside a test, this would do enough.\n\n>\n> test_expect_success 'git archive --prefix=prefix/' '\n> git archive --prefix=prefix/ HEAD >with_prefix.tar\n> '\n>\n> -check_tar with_prefix prefix/\n> +test_expect_success 'extract with prefix' 'check_tar with_prefix prefix/'\n>\n> test_expect_success 'git-archive --prefix=olde-' '\n> git archive --prefix=olde- HEAD >with_olde-prefix.tar\n> '\n>\n> -check_tar with_olde-prefix olde-\n> +test_expect_success 'extract with olde- prefix' \\\n> + 'check_tar with_olde-prefix olde-'\n\nIn new style this would look like\n\n test_expect_success 'extract with olde- prefix' '\n check_tar with_olde-prefix olde-\n '\n"},{"id":"359090","messageId":"CAGZ79kZuJr35b4ui3J_sMkscQDt+tx88jFgKY9CKn5CtHnN7tA@mail.gmail.com","threadId":"49323","inReplyTo":"20180927012455.234876-3-steadmon@google.com","subject":"Re: [PATCH v2 2/4] archive: use packet_reader for communications","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-27T18:42:03Z","receivedAt":"2018-09-27T18:42:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 26, 2018 at 6:25 PM Josh Steadmon wrote:\n>\n> Using packet_reader will simplify version detection and capability\n> handling, which will make implementation of protocol v2 support in\n> git-archive easier.\n>\n> This refactoring does not change the behavior of \"git archive\".\n>\n> Signed-off-by: Josh Steadmon \n\nThis patch is\nReviewed-by: Stefan Beller \n\nThanks!\n\n> ---\n> builtin/archive.c | 23 ++++++++++++++---------\n> 1 file changed, 14 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index e74f675390..4eb547c5b7 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -27,10 +27,11 @@ static int run_remote_archiver(int argc, const char **argv,\n> const char *remote, const char *exec,\n> const char *name_hint)\n> {\n> - char *buf;\n> int fd[2], i, rv;\n> struct transport *transport;\n> struct remote *_remote;\n> + struct packet_reader reader;\n> + enum packet_read_status status;\n>\n> _remote = remote_get(remote);\n> if (!_remote->url[0])\n> @@ -38,6 +39,8 @@ static int run_remote_archiver(int argc, const char **argv,\n> transport = transport_get(_remote, _remote->url[0]);\n> transport_connect(transport, \"git-upload-archive\", exec, fd);\n>\n> + packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> +\n> /*\n> * Inject a fake --format field at the beginning of the\n> * arguments, with the format inferred from our output\n> @@ -53,18 +56,20 @@ static int run_remote_archiver(int argc, const char **argv,\n> packet_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n> packet_flush(fd[1]);\n>\n> - buf = packet_read_line(fd[0], NULL);\n> - if (!buf)\n> + status = packet_reader_read(&reader);\n> +\n> + if (status != PACKET_READ_NORMAL || reader.pktlen <= 0)\n> die(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n> - if (strcmp(buf, \"ACK\")) {\n> - if (starts_with(buf, \"NACK \"))\n> - die(_(\"git archive: NACK %s\"), buf + 5);\n> - if (starts_with(buf, \"ERR \"))\n> - die(_(\"remote error: %s\"), buf + 4);\n> + if (strcmp(reader.line, \"ACK\")) {\n> + if (starts_with(reader.line, \"NACK \"))\n> + die(_(\"git archive: NACK %s\"), reader.line + 5);\n> + if (starts_with(reader.line, \"ERR \"))\n> + die(_(\"remote error: %s\"), reader.line + 4);\n> die(_(\"git archive: protocol error\"));\n> }\n>\n> - if (packet_read_line(fd[0], NULL))\n> + status = packet_reader_read(&reader);\n> + if (status == PACKET_READ_NORMAL && reader.pktlen > 0)\n> die(_(\"git archive: expected a flush\"));\n>\n> /* Now, start reading from fd[0] and spit it out to stdout */\n> --\n> 2.19.0.605.g01d371f741-goog\n>\n"},{"id":"359114","messageId":"20180927202815.GB55647@google.com","threadId":"49323","inReplyTo":"xmqqworpgx99.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] archive: allow archive over HTTP(S) with proto v2","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-09-27T20:28:15Z","receivedAt":"2018-09-27T20:28:32Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.09.13 09:47, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > Signed-off-by: Josh Steadmon \n> > ---\n> > builtin/archive.c | 8 +++++++-\n> > http-backend.c | 10 +++++++++-\n> > transport-helper.c | 5 +++--\n> > 3 files changed, 19 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/builtin/archive.c b/builtin/archive.c\n> > index 73831887d..5fa75b3f7 100644\n> > --- a/builtin/archive.c\n> > +++ b/builtin/archive.c\n> > @@ -87,7 +87,13 @@ static int run_remote_archiver(int argc, const char **argv,\n> > \t\tstatus = packet_reader_read(&reader);\n> > \t\tif (status != PACKET_READ_FLUSH)\n> > \t\t\tdie(_(\"git archive: expected a flush\"));\n> > -\t}\n> > +\t} else if (version == protocol_v2 &&\n> > +\t\t starts_with(transport->url, \"http\"))\n> > +\t\t/*\n> > +\t\t * Commands over HTTP require two requests, so there's an\n> > +\t\t * additional server response to parse.\n> > +\t\t */\n> > +\t\tdiscover_version(&reader);\n> \n> What should happen if the version discovered here is different from\n> v2 or the capabilities offered are different from what we saw\n> before? Perhaps we need some sanity checks, as we on this side of\n> the connection know we are making two requests, and may even end up\n> talking with an instance of \"upload-archive\" that is different from\n> the one we talked with earlier.\n> \n> > diff --git a/http-backend.c b/http-backend.c\n> > index 458642ef7..d62d583c7 100644\n> > --- a/http-backend.c\n> > +++ b/http-backend.c\n> > @@ -32,6 +32,7 @@ struct rpc_service {\n> > static struct rpc_service rpc_service[] = {\n> > \t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n> > \t{ \"receive-pack\", \"receivepack\", 0, -1 },\n> > +\t{ \"upload-archive\", \"uploadarchive\", 1, 1 },\n> > };\n> > \n> > static struct string_list *get_parameters(void)\n> > @@ -637,6 +638,12 @@ static void service_rpc(struct strbuf *hdr, char *service_name)\n> > \tstruct rpc_service *svc = select_service(hdr, service_name);\n> > \tstruct strbuf buf = STRBUF_INIT;\n> > \n> > +\tif (!strcmp(service_name, \"git-upload-archive\")) {\n> > +\t\t/* git-upload-archive doesn't need --stateless-rpc */\n> > +\t\targv[1] = \".\";\n> > +\t\targv[2] = NULL;\n> > +\t}\n> > +\n> > \tstrbuf_reset(&buf);\n> > \tstrbuf_addf(&buf, \"application/x-git-%s-request\", svc->name);\n> > \tcheck_content_type(hdr, buf.buf);\n> > @@ -713,7 +720,8 @@ static struct service_cmd {\n> > \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{40}\\\\.idx$\", get_idx_file},\n> > \n> > \t{\"POST\", \"/git-upload-pack$\", service_rpc},\n> > -\t{\"POST\", \"/git-receive-pack$\", service_rpc}\n> > +\t{\"POST\", \"/git-receive-pack$\", service_rpc},\n> > +\t{\"POST\", \"/git-upload-archive$\", service_rpc},\n> > };\n> > \n> > static int bad_request(struct strbuf *hdr, const struct service_cmd *c)\n> > diff --git a/transport-helper.c b/transport-helper.c\n> > index 143ca008c..b4b96fc89 100644\n> > --- a/transport-helper.c\n> > +++ b/transport-helper.c\n> > @@ -605,7 +605,8 @@ static int process_connect_service(struct transport *transport,\n> > \t\tret = run_connect(transport, &cmdbuf);\n> > \t} else if (data->stateless_connect &&\n> > \t\t (get_protocol_version_config() == protocol_v2) &&\n> > -\t\t !strcmp(\"git-upload-pack\", name)) {\n> > +\t\t (!strcmp(\"git-upload-pack\", name) ||\n> > +\t\t !strcmp(\"git-upload-archive\", name))) {\n> > \t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n> > \t\tret = run_connect(transport, &cmdbuf);\n> > \t\tif (ret)\n> > @@ -639,7 +640,7 @@ static int connect_helper(struct transport *transport, const char *name,\n> > \n> > \t/* Get_helper so connect is inited. */\n> > \tget_helper(transport);\n> > -\tif (!data->connect)\n> > +\tif (!data->connect && !data->stateless_connect)\n> > \t\tdie(_(\"operation not supported by protocol\"));\n> \n> This is somewhat curious. The upload-pack going over HTTP also is\n> triggered by the same \"stateless-connect\" remote helper command, as\n> we just saw in the previous hunk, and that support is not new. Why\n> do we need this change then? What's different between the \"data\"\n> that is obtained by get_helper(transport) for driving upload-pack\n> and upload-archive? Presumably upload-pack was working without this\n> change because it also sets the connect bit on, and upload-archive\n> does not work without this change because it does not? Why do these\n> two behave differently?\n\nThe data struct is not different between upload-pack vs. upload-archive.\nNeither of them set the connect bit. The difference is that upload-pack\ndoesn't need to call transport_connect(), so it never hits\nconnect_helper().\n"},{"id":"359151","messageId":"xmqq1s9efuqc.fsf@gitster-ct.c.googlers.com","threadId":"49323","inReplyTo":"20180927183017.GD112066@aiede.svl.corp.google.com","subject":"Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-27T22:20:27Z","receivedAt":"2018-09-27T22:20:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> 1. Clients sending version=2 when they do not, in fact, speak protocol\n> v2 for a service is a (serious) bug. (Separately from this\n> series) we should fix it.\n>\n> 2. That bug is already in the wild, alas. Fortunately the semantics of\n> GIT_PROTOCOL as a list of key/value pairs is well defined. So we\n> have choices of (a) bump version to version=3 (b) pass another\n> value 'version=2:yesreallyversion=2' (c) etc.\n>\n> 3. This is likely to affect push, too.\n\nDo you mean that existing \"git push\", \"git fetch\" and \"git archive\"\nsends version=2 even when they are not capable of speaking protocol\nv2? I thought that \"git archive [--remote]\" was left outside of the\nprotocol update (that was the reason why the earlier attempt took a\nhacky route of \"shallow clone followed by local archive\"), so there\nis no \"git archive\" in the wild that can even say \"version=$n\"\n(which requires you to be at least version=1)?\n"},{"id":"359156","messageId":"20180927223314.GA230445@google.com","threadId":"49323","inReplyTo":"xmqq1s9efuqc.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-09-27T22:33:14Z","receivedAt":"2018-09-27T22:33:27Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.09.27 15:20, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n> \n> > 1. Clients sending version=2 when they do not, in fact, speak protocol\n> > v2 for a service is a (serious) bug. (Separately from this\n> > series) we should fix it.\n> >\n> > 2. That bug is already in the wild, alas. Fortunately the semantics of\n> > GIT_PROTOCOL as a list of key/value pairs is well defined. So we\n> > have choices of (a) bump version to version=3 (b) pass another\n> > value 'version=2:yesreallyversion=2' (c) etc.\n> >\n> > 3. This is likely to affect push, too.\n> \n> Do you mean that existing \"git push\", \"git fetch\" and \"git archive\"\n> sends version=2 even when they are not capable of speaking protocol\n> v2? I thought that \"git archive [--remote]\" was left outside of the\n> protocol update (that was the reason why the earlier attempt took a\n> hacky route of \"shallow clone followed by local archive\"), so there\n> is no \"git archive\" in the wild that can even say \"version=$n\"\n> (which requires you to be at least version=1)?\n\nYes, the version on my desktop sends version=2 when archiving:\n\n∫ which git\n/usr/bin/git\n∫ git --version\ngit version 2.19.0.605.g01d371f741-goog\n∫ GIT_TRACE_PACKET=${HOME}/server_trace git daemon \\\n --enable=upload-archive \\\n --base-path=${HOME}/src/bare-repos &\n[1] 258496\n∫ git archive --remote git://localhost/test-repo.git HEAD >! test.tar\n∫ grep version ~/server_trace\n15:31:22.377869 pkt-line.c:80 packet: git< git-upload-archive /test-repo.git\\0host=localhost\\0\\0version=2\\0\n"},{"id":"359172","messageId":"xmqqwor6e7lc.fsf@gitster-ct.c.googlers.com","threadId":"49323","inReplyTo":"20180927223314.GA230445@google.com","subject":"Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-28T01:25:35Z","receivedAt":"2018-09-28T01:25:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Yes, the version on my desktop sends version=2 when archiving:\n>\n> ∫ which git\n> /usr/bin/git\n> ∫ git --version\n> git version 2.19.0.605.g01d371f741-goog\n> ∫ GIT_TRACE_PACKET=${HOME}/server_trace git daemon \\\n> --enable=upload-archive \\\n> --base-path=${HOME}/src/bare-repos &\n> [1] 258496\n> ∫ git archive --remote git://localhost/test-repo.git HEAD >! test.tar\n> ∫ grep version ~/server_trace\n> 15:31:22.377869 pkt-line.c:80 packet: git< git-upload-archive /test-repo.git\\0host=localhost\\0\\0version=2\\0\n\nAh, that's truly broken.\n\nCome to think of it, do we need to be using uniform versions across\ndifferent endpoints? The archive request could be at v3 while fetch\nrequest could still be at v2, in which case the design to use a\nsingle protocol.version variable is probably the root cause of the\nconfusion? Perhaps like protocol..allow, we would want\nprotocol..version or something like that (and no\nprotocol.version) to make it clear that protocol v2 used for\nfetching has nothing to do with protocol v1 or v2 or v3 used for\narchiving?\n\nLuckily, protocol.version is still marked as experimental so it is\nnot too bad that we caught the design mistake (if it is one) and can\nnow correct it before the damage spreads too widely.\n\n\n"}]}