{"thread":{"id":"50510","subject":"[PATCH 0/5] Protocol v2 fix: http and auth","startedAt":"2019-02-14T19:06:48Z","lastAt":"2019-03-04T03:45:47Z","messageCount":24,"participants":["Jonathan Tan","Junio C Hamano","Jeff King","Eric Sunshine","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"369324","messageId":"cover.1550170980.git.jonathantanmy@google.com","threadId":"50510","inReplyTo":null,"subject":"[PATCH 0/5] Protocol v2 fix: http and auth","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-14T19:06:34Z","receivedAt":"2019-02-14T19:06:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Peff noticed an issue with my http auth in protocol v2 patch earlier\n[1], and in the ensuing discussion, I thought that it would be best to\nmake v2 use post_rpc() as well (to be the same as v0/v1) instead of\nusing its own functions, to fix this issue and try to avoid a similar\nissue in the future. Besides that, there is also a net reduction in\nlines of code.\n\nSo here are the patches. First 4 are refactoring - the last one has the\nactual change. This is on the master branch.\n\n[1] https://public-inbox.org/git/20190206212928.GB12737@sigill.intra.peff.net/\n\nJonathan Tan (5):\n remote-curl: reduce scope of rpc_state.argv\n remote-curl: reduce scope of rpc_state.stdin_preamble\n remote-curl: reduce scope of rpc_state.result\n remote-curl: refactor reading into rpc_state's buf\n remote-curl: use post_rpc() for protocol v2 also\n\n pkt-line.c | 2 +-\n pkt-line.h | 1 +\n remote-curl.c | 351 ++++++++++++++++++-----------------------\n t/t5702-protocol-v2.sh | 26 ++-\n 4 files changed, 181 insertions(+), 199 deletions(-)\n\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369325","messageId":"a8bc5a256b5f4c4cba02342961d8e787bdf30abe.1550170980.git.jonathantanmy@google.com","threadId":"50510","inReplyTo":"cover.1550170980.git.jonathantanmy@google.com","subject":"[PATCH 1/5] remote-curl: reduce scope of rpc_state.argv","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-14T19:06:35Z","receivedAt":"2019-02-14T19:06:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The argv field in struct rpc_state is only used in rpc_service(), and\nnot in any functions it directly or indirectly calls. Refactor it to\nbecome an argument of rpc_service() instead.\n\nSigned-off-by: Jonathan Tan \n---\n remote-curl.c | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 2e04d53ac8..3bc5055da6 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -491,7 +491,6 @@ static void output_refs(struct ref *refs)\n \n struct rpc_state {\n \tconst char *service_name;\n-\tconst char **argv;\n \tstruct strbuf *stdin_preamble;\n \tchar *service_url;\n \tchar *hdr_content_type;\n@@ -815,7 +814,8 @@ static int post_rpc(struct rpc_state *rpc)\n \treturn err;\n }\n \n-static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n+static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n+\t\t const char **client_argv)\n {\n \tconst char *svc = rpc->service_name;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -826,7 +826,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n \tclient.in = -1;\n \tclient.out = -1;\n \tclient.git_cmd = 1;\n-\tclient.argv = rpc->argv;\n+\tclient.argv = client_argv;\n \tif (start_command(&client))\n \t\texit(1);\n \tif (preamble)\n@@ -964,11 +964,10 @@ static int fetch_git(struct discovery *heads,\n \n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-upload-pack\",\n-\trpc.argv = args.argv;\n \trpc.stdin_preamble = &preamble;\n \trpc.gzip_request = 1;\n \n-\terr = rpc_service(&rpc, heads);\n+\terr = rpc_service(&rpc, heads, args.argv);\n \tif (rpc.result.len)\n \t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n@@ -1098,10 +1097,9 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-receive-pack\",\n-\trpc.argv = args.argv;\n \trpc.stdin_preamble = &preamble;\n \n-\terr = rpc_service(&rpc, heads);\n+\terr = rpc_service(&rpc, heads, args.argv);\n \tif (rpc.result.len)\n \t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369326","messageId":"0a7cc5f6aa531a3bcee571b08d2eee4715773510.1550170980.git.jonathantanmy@google.com","threadId":"50510","inReplyTo":"cover.1550170980.git.jonathantanmy@google.com","subject":"[PATCH 2/5] remote-curl: reduce scope of rpc_state.stdin_preamble","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-14T19:06:36Z","receivedAt":"2019-02-14T19:06:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The stdin_preamble field in struct rpc_state is only used in\nrpc_service(), and not in any functions it directly or indirectly calls.\nRefactor it to become an argument of rpc_service() instead.\n\nAn observation of all callers of rpc_service() shows that the preamble\nis always set, so we no longer need the \"if (preamble)\" check.\n\nSigned-off-by: Jonathan Tan \n---\n remote-curl.c | 13 ++++---------\n 1 file changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 3bc5055da6..bff0bb9af6 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -491,7 +491,6 @@ static void output_refs(struct ref *refs)\n \n struct rpc_state {\n \tconst char *service_name;\n-\tstruct strbuf *stdin_preamble;\n \tchar *service_url;\n \tchar *hdr_content_type;\n \tchar *hdr_accept;\n@@ -815,11 +814,10 @@ static int post_rpc(struct rpc_state *rpc)\n }\n \n static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n-\t\t const char **client_argv)\n+\t\t const char **client_argv, const struct strbuf *preamble)\n {\n \tconst char *svc = rpc->service_name;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct strbuf *preamble = rpc->stdin_preamble;\n \tstruct child_process client = CHILD_PROCESS_INIT;\n \tint err = 0;\n \n@@ -829,8 +827,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n \tclient.argv = client_argv;\n \tif (start_command(&client))\n \t\texit(1);\n-\tif (preamble)\n-\t\twrite_or_die(client.in, preamble->buf, preamble->len);\n+\twrite_or_die(client.in, preamble->buf, preamble->len);\n \tif (heads)\n \t\twrite_or_die(client.in, heads->buf, heads->len);\n \n@@ -964,10 +961,9 @@ static int fetch_git(struct discovery *heads,\n \n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-upload-pack\",\n-\trpc.stdin_preamble = &preamble;\n \trpc.gzip_request = 1;\n \n-\terr = rpc_service(&rpc, heads, args.argv);\n+\terr = rpc_service(&rpc, heads, args.argv, &preamble);\n \tif (rpc.result.len)\n \t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n@@ -1097,9 +1093,8 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-receive-pack\",\n-\trpc.stdin_preamble = &preamble;\n \n-\terr = rpc_service(&rpc, heads, args.argv);\n+\terr = rpc_service(&rpc, heads, args.argv, &preamble);\n \tif (rpc.result.len)\n \t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369327","messageId":"db10ed20ff11eda56b14ca0667a0b42ab651b0b5.1550170980.git.jonathantanmy@google.com","threadId":"50510","inReplyTo":"cover.1550170980.git.jonathantanmy@google.com","subject":"[PATCH 3/5] remote-curl: reduce scope of rpc_state.result","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-14T19:06:37Z","receivedAt":"2019-02-14T19:07:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The result field in struct rpc_state is only used in rpc_service(), and\nnot in any functions it directly or indirectly calls. Refactor it to\nbecome an argument of rpc_service() instead.\n\nSigned-off-by: Jonathan Tan \n---\n remote-curl.c | 25 +++++++++++++------------\n 1 file changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex bff0bb9af6..2eb39774d0 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -502,7 +502,6 @@ struct rpc_state {\n \tint in;\n \tint out;\n \tint any_written;\n-\tstruct strbuf result;\n \tunsigned gzip_request : 1;\n \tunsigned initial_buffer : 1;\n };\n@@ -814,7 +813,8 @@ static int post_rpc(struct rpc_state *rpc)\n }\n \n static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n-\t\t const char **client_argv, const struct strbuf *preamble)\n+\t\t const char **client_argv, const struct strbuf *preamble,\n+\t\t struct strbuf *rpc_result)\n {\n \tconst char *svc = rpc->service_name;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -835,7 +835,6 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n \trpc->buf = xmalloc(rpc->alloc);\n \trpc->in = client.in;\n \trpc->out = client.out;\n-\tstrbuf_init(&rpc->result, 0);\n \n \tstrbuf_addf(&buf, \"%s%s\", url.buf, svc);\n \trpc->service_url = strbuf_detach(&buf, NULL);\n@@ -863,7 +862,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n \tclose(client.in);\n \tclient.in = -1;\n \tif (!err) {\n-\t\tstrbuf_read(&rpc->result, client.out, 0);\n+\t\tstrbuf_read(rpc_result, client.out, 0);\n \t} else {\n \t\tchar buf[4096];\n \t\tfor (;;)\n@@ -916,6 +915,7 @@ static int fetch_git(struct discovery *heads,\n \tstruct strbuf preamble = STRBUF_INIT;\n \tint i, err;\n \tstruct argv_array args = ARGV_ARRAY_INIT;\n+\tstruct strbuf rpc_result = STRBUF_INIT;\n \n \targv_array_pushl(&args, \"fetch-pack\", \"--stateless-rpc\",\n \t\t\t \"--stdin\", \"--lock-pack\", NULL);\n@@ -963,10 +963,10 @@ static int fetch_git(struct discovery *heads,\n \trpc.service_name = \"git-upload-pack\",\n \trpc.gzip_request = 1;\n \n-\terr = rpc_service(&rpc, heads, args.argv, &preamble);\n-\tif (rpc.result.len)\n-\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n-\tstrbuf_release(&rpc.result);\n+\terr = rpc_service(&rpc, heads, args.argv, &preamble, &rpc_result);\n+\tif (rpc_result.len)\n+\t\twrite_or_die(1, rpc_result.buf, rpc_result.len);\n+\tstrbuf_release(&rpc_result);\n \tstrbuf_release(&preamble);\n \targv_array_clear(&args);\n \treturn err;\n@@ -1061,6 +1061,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \tstruct argv_array args;\n \tstruct string_list_item *cas_option;\n \tstruct strbuf preamble = STRBUF_INIT;\n+\tstruct strbuf rpc_result = STRBUF_INIT;\n \n \targv_array_init(&args);\n \targv_array_pushl(&args, \"send-pack\", \"--stateless-rpc\", \"--helper-status\",\n@@ -1094,10 +1095,10 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-receive-pack\",\n \n-\terr = rpc_service(&rpc, heads, args.argv, &preamble);\n-\tif (rpc.result.len)\n-\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n-\tstrbuf_release(&rpc.result);\n+\terr = rpc_service(&rpc, heads, args.argv, &preamble, &rpc_result);\n+\tif (rpc_result.len)\n+\t\twrite_or_die(1, rpc_result.buf, rpc_result.len);\n+\tstrbuf_release(&rpc_result);\n \tstrbuf_release(&preamble);\n \targv_array_clear(&args);\n \treturn err;\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369328","messageId":"2357bf6af7798e8e400599d47c89dc92ecd2eb8d.1550170980.git.jonathantanmy@google.com","threadId":"50510","inReplyTo":"cover.1550170980.git.jonathantanmy@google.com","subject":"[PATCH 4/5] remote-curl: refactor reading into rpc_state's buf","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-14T19:06:38Z","receivedAt":"2019-02-14T19:07:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, whenever remote-curl reads pkt-lines from its response file\ndescriptor, only the payload is written to its buf, not the 4 characters\ndenoting the length. A future patch will require the ability to also\nwrite those 4 characters, so in preparation for that, refactor this read\ninto its own function.\n\nSigned-off-by: Jonathan Tan \n---\n remote-curl.c | 33 ++++++++++++++++++++++++---------\n 1 file changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 2eb39774d0..32c133f636 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -506,6 +506,25 @@ struct rpc_state {\n \tunsigned initial_buffer : 1;\n };\n \n+/*\n+ * Appends the result of reading from rpc->out to the string represented by\n+ * rpc->buf and rpc->len if there is enough space. Returns 1 if there was\n+ * enough space, 0 otherwise.\n+ *\n+ * Writes the number of bytes appended into appended.\n+ */\n+static int rpc_read_from_out(struct rpc_state *rpc, size_t *appended) {\n+\tsize_t left = rpc->alloc - rpc->len;\n+\tchar *buf = rpc->buf + rpc->len;\n+\n+\tif (left < LARGE_PACKET_MAX)\n+\t\treturn 0;\n+\n+\t*appended = packet_read(rpc->out, NULL, NULL, buf, left, 0);\n+\trpc->len += *appended;\n+\treturn 1;\n+}\n+\n static size_t rpc_out(void *ptr, size_t eltsize,\n \t\tsize_t nmemb, void *buffer_)\n {\n@@ -515,11 +534,12 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \n \tif (!avail) {\n \t\trpc->initial_buffer = 0;\n-\t\tavail = packet_read(rpc->out, NULL, NULL, rpc->buf, rpc->alloc, 0);\n+\t\trpc->len = 0;\n+\t\tif (!rpc_read_from_out(rpc, &avail))\n+\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n \t\tif (!avail)\n \t\t\treturn 0;\n \t\trpc->pos = 0;\n-\t\trpc->len = avail;\n \t}\n \n \tif (max < avail)\n@@ -663,20 +683,15 @@ static int post_rpc(struct rpc_state *rpc)\n \t * chunked encoding mess.\n \t */\n \twhile (1) {\n-\t\tsize_t left = rpc->alloc - rpc->len;\n-\t\tchar *buf = rpc->buf + rpc->len;\n-\t\tint n;\n+\t\tsize_t n;\n \n-\t\tif (left < LARGE_PACKET_MAX) {\n+\t\tif (!rpc_read_from_out(rpc, &n)) {\n \t\t\tlarge_request = 1;\n \t\t\tuse_gzip = 0;\n \t\t\tbreak;\n \t\t}\n-\n-\t\tn = packet_read(rpc->out, NULL, NULL, buf, left, 0);\n \t\tif (!n)\n \t\t\tbreak;\n-\t\trpc->len += n;\n \t}\n \n \tif (large_request) {\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369329","messageId":"25ea75eb435ed8fed759b30a4c362a68818a8905.1550170980.git.jonathantanmy@google.com","threadId":"50510","inReplyTo":"cover.1550170980.git.jonathantanmy@google.com","subject":"[PATCH 5/5] remote-curl: use post_rpc() for protocol v2 also","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-14T19:06:39Z","receivedAt":"2019-02-14T19:07:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When transmitting and receiving POSTs for protocol v0 and v1,\nremote-curl uses post_rpc() (and associated functions), but when doing\nthe same for protocol v2, it uses a separate set of functions\n(proxy_rpc() and others). Besides duplication of code, this has caused\nat least one bug: the auth retry mechanism that was implemented in v0/v1\nwas not implemented in v2.\n\nTo fix this issue and avoid it in the future, make remote-curl also use\npost_rpc() when handling protocol v2. Because line lengths are written\nto the HTTP request in protocol v2 (unlike in protocol v0/v1), this\nnecessitates changes in post_rpc() and some of the functions it uses;\nperform these changes too.\n\nA test has been included to ensure that the code for both the unchunked\nand chunked variants of the HTTP request is exercised.\n\nNote: stateless_connect() has been updated to use the lower-level packet\nreading functions instead of struct packet_reader. The low-level control\nis necessary here because we cannot change the destination buffer of\nstruct packet_reader while it is being used; struct packet_buffer has a\npeeking mechanism which relies on the destination buffer being present\nin between a peek and a read.\n\nSigned-off-by: Jonathan Tan \n---\n pkt-line.c | 2 +-\n pkt-line.h | 1 +\n remote-curl.c | 298 +++++++++++++++++------------------------\n t/t5702-protocol-v2.sh | 26 +++-\n 4 files changed, 150 insertions(+), 177 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex d4b71d3e82..60329b301b 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -117,7 +117,7 @@ void packet_buf_delim(struct strbuf *buf)\n \tstrbuf_add(buf, \"0001\", 4);\n }\n \n-static void set_packet_header(char *buf, const int size)\n+void set_packet_header(char *buf, const int size)\n {\n \tstatic char hexchar[] = \"0123456789abcdef\";\n \ndiff --git a/pkt-line.h b/pkt-line.h\nindex ad9a4a2cd7..c36cb788ed 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -25,6 +25,7 @@ void packet_delim(int fd);\n void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n void packet_buf_flush(struct strbuf *buf);\n void packet_buf_delim(struct strbuf *buf);\n+void set_packet_header(char *buf, int size);\n void packet_write(int fd_out, const char *buf, size_t size);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 32c133f636..13836e4c28 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -504,6 +504,18 @@ struct rpc_state {\n \tint any_written;\n \tunsigned gzip_request : 1;\n \tunsigned initial_buffer : 1;\n+\n+\t/*\n+\t * Whenever a pkt-line is read into buf, append the 4 characters\n+\t * denoting its length before appending the payload.\n+\t */\n+\tunsigned write_line_lengths : 1;\n+\n+\t/*\n+\t * rpc_out uses this to keep track of whether it should continue\n+\t * reading to populate the current request. Initialize to 0.\n+\t */\n+\tunsigned stop_reading : 1;\n };\n \n /*\n@@ -511,17 +523,54 @@ struct rpc_state {\n * rpc->buf and rpc->len if there is enough space. Returns 1 if there was\n * enough space, 0 otherwise.\n *\n- * Writes the number of bytes appended into appended.\n+ * If rpc->write_line_lengths is true, appends the line length as a 4-byte\n+ * hexadecimal string before appending the result described above.\n+ *\n+ * Writes the total number of bytes appended into appended.\n */\n-static int rpc_read_from_out(struct rpc_state *rpc, size_t *appended) {\n-\tsize_t left = rpc->alloc - rpc->len;\n-\tchar *buf = rpc->buf + rpc->len;\n+static int rpc_read_from_out(struct rpc_state *rpc, int options,\n+\t\t\t size_t *appended,\n+\t\t\t enum packet_read_status *status) {\n+\tsize_t left;\n+\tchar *buf;\n+\tint pktlen_raw;\n+\n+\tif (rpc->write_line_lengths) {\n+\t\tleft = rpc->alloc - rpc->len - 4;\n+\t\tbuf = rpc->buf + rpc->len + 4;\n+\t} else {\n+\t\tleft = rpc->alloc - rpc->len;\n+\t\tbuf = rpc->buf + rpc->len;\n+\t}\n \n \tif (left < LARGE_PACKET_MAX)\n \t\treturn 0;\n \n-\t*appended = packet_read(rpc->out, NULL, NULL, buf, left, 0);\n-\trpc->len += *appended;\n+\t*status = packet_read_with_status(rpc->out, NULL, NULL, buf,\n+\t\t\tleft, &pktlen_raw, options);\n+\tif (*status != PACKET_READ_EOF) {\n+\t\t*appended = pktlen_raw + (rpc->write_line_lengths ? 4 : 0);\n+\t\trpc->len += *appended;\n+\t}\n+\n+\tif (rpc->write_line_lengths) {\n+\t\tswitch (*status) {\n+\t\tcase PACKET_READ_EOF:\n+\t\t\tif (!(options & PACKET_READ_GENTLE_ON_EOF))\n+\t\t\t\tdie(\"shouldn't have EOF when not gentle on EOF\");\n+\t\t\tbreak;\n+\t\tcase PACKET_READ_NORMAL:\n+\t\t\tset_packet_header(buf - 4, *appended);\n+\t\t\tbreak;\n+\t\tcase PACKET_READ_DELIM:\n+\t\t\tmemcpy(buf - 4, \"0001\", 4);\n+\t\t\tbreak;\n+\t\tcase PACKET_READ_FLUSH:\n+\t\t\tmemcpy(buf - 4, \"0000\", 4);\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \treturn 1;\n }\n \n@@ -531,15 +580,32 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \tsize_t max = eltsize * nmemb;\n \tstruct rpc_state *rpc = buffer_;\n \tsize_t avail = rpc->len - rpc->pos;\n+\tenum packet_read_status status;\n \n \tif (!avail) {\n \t\trpc->initial_buffer = 0;\n \t\trpc->len = 0;\n-\t\tif (!rpc_read_from_out(rpc, &avail))\n-\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n-\t\tif (!avail)\n-\t\t\treturn 0;\n \t\trpc->pos = 0;\n+\t\tif (!rpc->stop_reading) {\n+\t\t\tif (!rpc_read_from_out(rpc, 0, &avail, &status))\n+\t\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n+\t\t\tif (status == PACKET_READ_FLUSH)\n+\t\t\t\t/*\n+\t\t\t\t * We are done reading for this request, but we\n+\t\t\t\t * still need to send this line out (if\n+\t\t\t\t * rpc->write_line_lengths is true) so do not\n+\t\t\t\t * return yet.\n+\t\t\t\t */\n+\t\t\t\trpc->stop_reading = 1;\n+\t\t}\n+\t}\n+\tif (!avail && rpc->stop_reading) {\n+\t\t/*\n+\t\t * \"return 0\" will notify Curl that this RPC request is done,\n+\t\t * so reset stop_reading back to 0 for the next request.\n+\t\t */\n+\t\trpc->stop_reading = 0;\n+\t\treturn 0;\n \t}\n \n \tif (max < avail)\n@@ -684,13 +750,14 @@ static int post_rpc(struct rpc_state *rpc)\n \t */\n \twhile (1) {\n \t\tsize_t n;\n+\t\tenum packet_read_status status;\n \n-\t\tif (!rpc_read_from_out(rpc, &n)) {\n+\t\tif (!rpc_read_from_out(rpc, 0, &n, &status)) {\n \t\t\tlarge_request = 1;\n \t\t\tuse_gzip = 0;\n \t\t\tbreak;\n \t\t}\n-\t\tif (!n)\n+\t\tif (status == PACKET_READ_FLUSH)\n \t\t\tbreak;\n \t}\n \n@@ -1165,165 +1232,11 @@ static void parse_push(struct strbuf *buf)\n \tfree(specs);\n }\n \n-/*\n- * Used to represent the state of a connection to an HTTP server when\n- * communicating using git's wire-protocol version 2.\n- */\n-struct proxy_state {\n-\tchar *service_name;\n-\tchar *service_url;\n-\tstruct curl_slist *headers;\n-\tstruct strbuf request_buffer;\n-\tint in;\n-\tint out;\n-\tstruct packet_reader reader;\n-\tsize_t pos;\n-\tint seen_flush;\n-};\n-\n-static void proxy_state_init(struct proxy_state *p, const char *service_name,\n-\t\t\t enum protocol_version version)\n-{\n-\tstruct strbuf buf = STRBUF_INIT;\n-\n-\tmemset(p, 0, sizeof(*p));\n-\tp->service_name = xstrdup(service_name);\n-\n-\tp->in = 0;\n-\tp->out = 1;\n-\tstrbuf_init(&p->request_buffer, 0);\n-\n-\tstrbuf_addf(&buf, \"%s%s\", url.buf, p->service_name);\n-\tp->service_url = strbuf_detach(&buf, NULL);\n-\n-\tp->headers = http_copy_default_headers();\n-\n-\tstrbuf_addf(&buf, \"Content-Type: application/x-%s-request\", p->service_name);\n-\tp->headers = curl_slist_append(p->headers, buf.buf);\n-\tstrbuf_reset(&buf);\n-\n-\tstrbuf_addf(&buf, \"Accept: application/x-%s-result\", p->service_name);\n-\tp->headers = curl_slist_append(p->headers, buf.buf);\n-\tstrbuf_reset(&buf);\n-\n-\tp->headers = curl_slist_append(p->headers, \"Transfer-Encoding: chunked\");\n-\n-\t/* Add the Git-Protocol header */\n-\tif (get_protocol_http_header(version, &buf))\n-\t\tp->headers = curl_slist_append(p->headers, buf.buf);\n-\n-\tpacket_reader_init(&p->reader, p->in, NULL, 0,\n-\t\t\t PACKET_READ_GENTLE_ON_EOF |\n-\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n-\n-\tstrbuf_release(&buf);\n-}\n-\n-static void proxy_state_clear(struct proxy_state *p)\n-{\n-\tfree(p->service_name);\n-\tfree(p->service_url);\n-\tcurl_slist_free_all(p->headers);\n-\tstrbuf_release(&p->request_buffer);\n-}\n-\n-/*\n- * CURLOPT_READFUNCTION callback function.\n- * Attempts to copy over a single packet-line at a time into the\n- * curl provided buffer.\n- */\n-static size_t proxy_in(char *buffer, size_t eltsize,\n-\t\t size_t nmemb, void *userdata)\n-{\n-\tsize_t max;\n-\tstruct proxy_state *p = userdata;\n-\tsize_t avail = p->request_buffer.len - p->pos;\n-\n-\n-\tif (eltsize != 1)\n-\t\tBUG(\"curl read callback called with size = %\"PRIuMAX\" != 1\",\n-\t\t (uintmax_t)eltsize);\n-\tmax = nmemb;\n-\n-\tif (!avail) {\n-\t\tif (p->seen_flush) {\n-\t\t\tp->seen_flush = 0;\n-\t\t\treturn 0;\n-\t\t}\n-\n-\t\tstrbuf_reset(&p->request_buffer);\n-\t\tswitch (packet_reader_read(&p->reader)) {\n-\t\tcase PACKET_READ_EOF:\n-\t\t\tdie(\"unexpected EOF when reading from parent process\");\n-\t\tcase PACKET_READ_NORMAL:\n-\t\t\tpacket_buf_write_len(&p->request_buffer, p->reader.line,\n-\t\t\t\t\t p->reader.pktlen);\n-\t\t\tbreak;\n-\t\tcase PACKET_READ_DELIM:\n-\t\t\tpacket_buf_delim(&p->request_buffer);\n-\t\t\tbreak;\n-\t\tcase PACKET_READ_FLUSH:\n-\t\t\tpacket_buf_flush(&p->request_buffer);\n-\t\t\tp->seen_flush = 1;\n-\t\t\tbreak;\n-\t\t}\n-\t\tp->pos = 0;\n-\t\tavail = p->request_buffer.len;\n-\t}\n-\n-\tif (max < avail)\n-\t\tavail = max;\n-\tmemcpy(buffer, p->request_buffer.buf + p->pos, avail);\n-\tp->pos += avail;\n-\treturn avail;\n-}\n-\n-static size_t proxy_out(char *buffer, size_t eltsize,\n-\t\t\tsize_t nmemb, void *userdata)\n-{\n-\tsize_t size;\n-\tstruct proxy_state *p = userdata;\n-\n-\tif (eltsize != 1)\n-\t\tBUG(\"curl read callback called with size = %\"PRIuMAX\" != 1\",\n-\t\t (uintmax_t)eltsize);\n-\tsize = nmemb;\n-\n-\twrite_or_die(p->out, buffer, size);\n-\treturn size;\n-}\n-\n-/* Issues a request to the HTTP server configured in `p` */\n-static int proxy_request(struct proxy_state *p)\n-{\n-\tstruct active_request_slot *slot;\n-\n-\tslot = get_active_slot();\n-\n-\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n-\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_POST, 1);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, p->headers);\n-\n-\t/* Setup function to read request from client */\n-\tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, proxy_in);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_READDATA, p);\n-\n-\t/* Setup function to write server response to client */\n-\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, proxy_out);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, p);\n-\n-\tif (run_slot(slot, NULL) != HTTP_OK)\n-\t\treturn -1;\n-\n-\treturn 0;\n-}\n-\n static int stateless_connect(const char *service_name)\n {\n \tstruct discovery *discover;\n-\tstruct proxy_state p;\n+\tstruct rpc_state rpc;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \t/*\n \t * Run the info/refs request and see if the server supports protocol\n@@ -1343,23 +1256,58 @@ static int stateless_connect(const char *service_name)\n \t\tfflush(stdout);\n \t}\n \n-\tproxy_state_init(&p, service_name, discover->version);\n+\trpc.service_name = service_name;\n+\trpc.service_url = xstrfmt(\"%s%s\", url.buf, rpc.service_name);\n+\trpc.hdr_content_type = xstrfmt(\"Content-Type: application/x-%s-request\", rpc.service_name);\n+\trpc.hdr_accept = xstrfmt(\"Accept: application/x-%s-result\", rpc.service_name);\n+\tif (get_protocol_http_header(discover->version, &buf)) {\n+\t\trpc.protocol_header = strbuf_detach(&buf, NULL);\n+\t} else {\n+\t\trpc.protocol_header = NULL;\n+\t\tstrbuf_release(&buf);\n+\t}\n+\trpc.buf = xmalloc(http_post_buffer);\n+\trpc.alloc = http_post_buffer;\n+\trpc.len = 0;\n+\trpc.pos = 0;\n+\trpc.in = 1;\n+\trpc.out = 0;\n+\trpc.any_written = 0;\n+\trpc.gzip_request = 1;\n+\trpc.initial_buffer = 0;\n+\trpc.write_line_lengths = 1;\n+\trpc.stop_reading = 0;\n \n \t/*\n \t * Dump the capability listing that we got from the server earlier\n \t * during the info/refs request.\n \t */\n-\twrite_or_die(p.out, discover->buf, discover->len);\n+\twrite_or_die(rpc.in, discover->buf, discover->len);\n \n-\t/* Peek the next packet line. Until we see EOF keep sending POSTs */\n-\twhile (packet_reader_peek(&p.reader) != PACKET_READ_EOF) {\n-\t\tif (proxy_request(&p)) {\n+\t/* Until we see EOF keep sending POSTs */\n+\twhile (1) {\n+\t\tsize_t avail;\n+\t\tenum packet_read_status status;\n+\n+\t\tif (!rpc_read_from_out(&rpc, PACKET_READ_GENTLE_ON_EOF, &avail,\n+\t\t\t\t &status))\n+\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n+\t\tif (status == PACKET_READ_EOF)\n+\t\t\tbreak;\n+\t\tif (post_rpc(&rpc))\n \t\t\t/* We would have an err here */\n \t\t\tbreak;\n-\t\t}\n+\t\t/* Reset the buffer for next request */\n+\t\trpc.len = 0;\n \t}\n \n-\tproxy_state_clear(&p);\n+\tfree(rpc.service_url);\n+\tfree(rpc.hdr_content_type);\n+\tfree(rpc.hdr_accept);\n+\tfree(rpc.protocol_header);\n+\tfree(rpc.buf);\n+\tstrbuf_release(&buf);\n+\n \treturn 0;\n }\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex db4ae09f2f..61acf99d80 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -542,7 +542,31 @@ test_expect_success 'clone with http:// using protocol v2' '\n \t# Client requested to use protocol v2\n \tgrep \"Git-Protocol: version=2\" log &&\n \t# Server responded using protocol v2\n-\tgrep \"git< version 2\" log\n+\tgrep \"git< version 2\" log &&\n+\t# Verify that the chunked encoding sending codepath is NOT exercised\n+\t! grep \"Send header: Transfer-Encoding: chunked\" log\n+'\n+\n+test_expect_success 'clone big repository with http:// using protocol v2' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/big\" &&\n+\t# Ensure that the list of wants is greater than http.postbuffer below\n+\tfor i in $(seq 1 1500)\n+\tdo\n+\t\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/big\" \"commit$i\"\n+\tdone &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TRACE_CURL=\"$(pwd)/log\" git \\\n+\t\t-c protocol.version=2 -c http.postbuffer=65536 \\\n+\t\tclone \"$HTTPD_URL/smart/big\" big_child &&\n+\n+\t# Client requested to use protocol v2\n+\tgrep \"Git-Protocol: version=2\" log &&\n+\t# Server responded using protocol v2\n+\tgrep \"git< version 2\" log &&\n+\t# Verify that the chunked encoding sending codepath is exercised\n+\tgrep \"Send header: Transfer-Encoding: chunked\" log\n '\n \n test_expect_success 'fetch with http:// using protocol v2' '\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369358","messageId":"xmqqa7iy6l8j.fsf@gitster-ct.c.googlers.com","threadId":"50510","inReplyTo":"25ea75eb435ed8fed759b30a4c362a68818a8905.1550170980.git.jonathantanmy@google.com","subject":"Re: [PATCH 5/5] remote-curl: use post_rpc() for protocol v2 also","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-14T22:47:24Z","receivedAt":"2019-02-14T22:47:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> +\t# Ensure that the list of wants is greater than http.postbuffer below\n> +\tfor i in $(seq 1 1500)\n\ntest_seq.\n\n> +\tdo\n> +\t\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/big\" \"commit$i\"\n> +\tdone &&\n> +\n> +\tGIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TRACE_CURL=\"$(pwd)/log\" git \\\n> +\t\t-c protocol.version=2 -c http.postbuffer=65536 \\\n> +\t\tclone \"$HTTPD_URL/smart/big\" big_child &&\n> +\n> +\t# Client requested to use protocol v2\n> +\tgrep \"Git-Protocol: version=2\" log &&\n> +\t# Server responded using protocol v2\n> +\tgrep \"git< version 2\" log &&\n> +\t# Verify that the chunked encoding sending codepath is exercised\n> +\tgrep \"Send header: Transfer-Encoding: chunked\" log\n> '\n> \n> test_expect_success 'fetch with http:// using protocol v2' '\n"},{"id":"369807","messageId":"20190221134609.GA21406@sigill.intra.peff.net","threadId":"50510","inReplyTo":"25ea75eb435ed8fed759b30a4c362a68818a8905.1550170980.git.jonathantanmy@google.com","subject":"Re: [PATCH 5/5] remote-curl: use post_rpc() for protocol v2 also","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-21T13:46:10Z","receivedAt":"2019-02-21T13:46:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 14, 2019 at 11:06:39AM -0800, Jonathan Tan wrote:\n\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 32c133f636..13836e4c28 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -504,6 +504,18 @@ struct rpc_state {\n> \tint any_written;\n> \tunsigned gzip_request : 1;\n> \tunsigned initial_buffer : 1;\n> +\n> +\t/*\n> +\t * Whenever a pkt-line is read into buf, append the 4 characters\n> +\t * denoting its length before appending the payload.\n> +\t */\n> +\tunsigned write_line_lengths : 1;\n\nHmm, so we read a packet, and then we \"append its length\" before\nappending the contents. But that would always be the length we just\nread, right? I wonder if it would be simpler to just call this option\nsomething like \"proxy_packets\" or \"full_packets\", teach the packet code\nto give us the full packets, and then just treat that whole buffer as a\nunit. I dunno. There might be some gotchas in practice, and it's not\nlike it's that much simpler. Just a thought.\n\n> +\t/*\n> +\t * rpc_out uses this to keep track of whether it should continue\n> +\t * reading to populate the current request. Initialize to 0.\n> +\t */\n> +\tunsigned stop_reading : 1;\n\nOK, so we need this because the v2 proxying will require us to stop\nreading but keep the channel open? Kind of awkward, but I don't see a\nway around it.\n\n> +static int rpc_read_from_out(struct rpc_state *rpc, int options,\n> +\t\t\t size_t *appended,\n> +\t\t\t enum packet_read_status *status) {\n> +\tsize_t left;\n> +\tchar *buf;\n> +\tint pktlen_raw;\n> +\n> +\tif (rpc->write_line_lengths) {\n> +\t\tleft = rpc->alloc - rpc->len - 4;\n> +\t\tbuf = rpc->buf + rpc->len + 4;\n> +\t} else {\n> +\t\tleft = rpc->alloc - rpc->len;\n> +\t\tbuf = rpc->buf + rpc->len;\n> +\t}\n\nOK, so we push the packets 4 bytes further into the buffer in that case,\nleaving room for the header. Makes sense.\n\n> \tif (left < LARGE_PACKET_MAX)\n> \t\treturn 0;\n> \n> -\t*appended = packet_read(rpc->out, NULL, NULL, buf, left, 0);\n> -\trpc->len += *appended;\n> +\t*status = packet_read_with_status(rpc->out, NULL, NULL, buf,\n> +\t\t\tleft, &pktlen_raw, options);\n> +\tif (*status != PACKET_READ_EOF) {\n> +\t\t*appended = pktlen_raw + (rpc->write_line_lengths ? 4 : 0);\n> +\t\trpc->len += *appended;\n> +\t}\n> +\n> +\tif (rpc->write_line_lengths) {\n> +\t\tswitch (*status) {\n> +\t\tcase PACKET_READ_EOF:\n> +\t\t\tif (!(options & PACKET_READ_GENTLE_ON_EOF))\n> +\t\t\t\tdie(\"shouldn't have EOF when not gentle on EOF\");\n> +\t\t\tbreak;\n> +\t\tcase PACKET_READ_NORMAL:\n> +\t\t\tset_packet_header(buf - 4, *appended);\n> +\t\t\tbreak;\n> +\t\tcase PACKET_READ_DELIM:\n> +\t\t\tmemcpy(buf - 4, \"0001\", 4);\n> +\t\t\tbreak;\n> +\t\tcase PACKET_READ_FLUSH:\n> +\t\t\tmemcpy(buf - 4, \"0000\", 4);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n\nAnd here we fill it in. Make sense. It's a little awkward that we have\nto re-translate READ_DELIM, etc, back into their headers.\n\n> @@ -531,15 +580,32 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n> \tsize_t max = eltsize * nmemb;\n> \tstruct rpc_state *rpc = buffer_;\n> \tsize_t avail = rpc->len - rpc->pos;\n> +\tenum packet_read_status status;\n> \n> \tif (!avail) {\n> \t\trpc->initial_buffer = 0;\n> \t\trpc->len = 0;\n> -\t\tif (!rpc_read_from_out(rpc, &avail))\n> -\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n> -\t\tif (!avail)\n> -\t\t\treturn 0;\n> \t\trpc->pos = 0;\n> +\t\tif (!rpc->stop_reading) {\n> +\t\t\tif (!rpc_read_from_out(rpc, 0, &avail, &status))\n> +\t\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n\nDo we actually need it to be LARGE_PACKET_MAX+4 here? I guess not,\nbecause LARGE_PACKET_DATA_MAX is the \"-4\" version. So I think this BUG()\nwas perhaps already wrong?\n\n> +\t\t\tif (status == PACKET_READ_FLUSH)\n> +\t\t\t\t/*\n> +\t\t\t\t * We are done reading for this request, but we\n> +\t\t\t\t * still need to send this line out (if\n> +\t\t\t\t * rpc->write_line_lengths is true) so do not\n> +\t\t\t\t * return yet.\n> +\t\t\t\t */\n> +\t\t\t\trpc->stop_reading = 1;\n> +\t\t}\n> +\t}\n> +\tif (!avail && rpc->stop_reading) {\n> +\t\t/*\n> +\t\t * \"return 0\" will notify Curl that this RPC request is done,\n> +\t\t * so reset stop_reading back to 0 for the next request.\n> +\t\t */\n> +\t\trpc->stop_reading = 0;\n> +\t\treturn 0;\n\nOK, and here's where we handle the stop_reading thing. It is indeed\nawkward, but I think your comments make it clear what's going on.\n\nIf we get stop_reading, do we care about \"avail\"? I.e., shouldn't we be\nable to return non-zero to say \"we got the whole input, this is not a\ntoo-large request\"?\n\n> +test_expect_success 'clone big repository with http:// using protocol v2' '\n> +\ttest_when_finished \"rm -f log\" &&\n> +\n> +\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/big\" &&\n> +\t# Ensure that the list of wants is greater than http.postbuffer below\n> +\tfor i in $(seq 1 1500)\n> +\tdo\n> +\t\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/big\" \"commit$i\"\n> +\tdone &&\n\nAs Junio noted, this should be test_seq. But I think it would be nice to\navoid looping on test_commit here at all. It kicks off at least 3\nprocesses; multiplying that by 1500 is going to be slow.\n\nMaking a big input is often much faster by generating a fast-import\nstream (which can often be done entirely in-shell). There's some prior\nart in t3302, t5551, t5608, and others.\n\n-Peff\n"},{"id":"369808","messageId":"20190221134758.GB21406@sigill.intra.peff.net","threadId":"50510","inReplyTo":"cover.1550170980.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/5] Protocol v2 fix: http and auth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-21T13:47:58Z","receivedAt":"2019-02-21T13:48:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 14, 2019 at 11:06:34AM -0800, Jonathan Tan wrote:\n\n> Peff noticed an issue with my http auth in protocol v2 patch earlier\n> [1], and in the ensuing discussion, I thought that it would be best to\n> make v2 use post_rpc() as well (to be the same as v0/v1) instead of\n> using its own functions, to fix this issue and try to avoid a similar\n> issue in the future. Besides that, there is also a net reduction in\n> lines of code.\n> \n> So here are the patches. First 4 are refactoring - the last one has the\n> actual change. This is on the master branch.\n\nI think this is the right approach. Patches 1-4 look pretty nice. I\ndidn't see anything wrong with the code in the 5th one, but I did have\nsome thoughts about how we might be able to do it a bit more cleanly\n(though I will not at all be surprised if they turn out to be bad\nsuggestions).\n\n-Peff\n"},{"id":"369834","messageId":"20190221192655.106489-1-jonathantanmy@google.com","threadId":"50510","inReplyTo":"20190221134609.GA21406@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] remote-curl: use post_rpc() for protocol v2 also","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-21T19:26:55Z","receivedAt":"2019-02-21T19:27:02Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > diff --git a/remote-curl.c b/remote-curl.c\n> > index 32c133f636..13836e4c28 100644\n> > --- a/remote-curl.c\n> > +++ b/remote-curl.c\n> > @@ -504,6 +504,18 @@ struct rpc_state {\n> > \tint any_written;\n> > \tunsigned gzip_request : 1;\n> > \tunsigned initial_buffer : 1;\n> > +\n> > +\t/*\n> > +\t * Whenever a pkt-line is read into buf, append the 4 characters\n> > +\t * denoting its length before appending the payload.\n> > +\t */\n> > +\tunsigned write_line_lengths : 1;\n> \n> Hmm, so we read a packet, and then we \"append its length\" before\n> appending the contents. But that would always be the length we just\n> read, right? I wonder if it would be simpler to just call this option\n> something like \"proxy_packets\" or \"full_packets\", teach the packet code\n> to give us the full packets, and then just treat that whole buffer as a\n> unit. I dunno. There might be some gotchas in practice, and it's not\n> like it's that much simpler. Just a thought.\n\nYes, the length is the length we just read. And it might not even be\nsimpler - this shifts the complexity to the code that does not use the 4\ncharacters (unless we always return 2 pointers, which seems redundant,\nand which now changes the problem of ensuring that the correct pointer\nis used). I think that this is a good default - proxying still seems\nrarer than just consuming payloads.\n\nI'm OK with changing the name, although I think that both \"proxy\" and\n\"full\" are less clear than \"write_line_lengths\" - aren't you still\nproxying even if you're changing the format a little, and isn't a packet\n\"full\" even without the line lengths?\n\n> > +\t/*\n> > +\t * rpc_out uses this to keep track of whether it should continue\n> > +\t * reading to populate the current request. Initialize to 0.\n> > +\t */\n> > +\tunsigned stop_reading : 1;\n> \n> OK, so we need this because the v2 proxying will require us to stop\n> reading but keep the channel open? Kind of awkward, but I don't see a\n> way around it.\n\nI'll improve the comments here and elsewhere in the next version. This\nbasically means that \"we read a flush but we haven't sent the 0000 to\nlibcurl yet, so don't read anything more until we have sent the 0000\",\nand \"flush_read_but_not_sent\" is probably a better name.\n\n> > @@ -531,15 +580,32 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n> > \tsize_t max = eltsize * nmemb;\n> > \tstruct rpc_state *rpc = buffer_;\n> > \tsize_t avail = rpc->len - rpc->pos;\n> > +\tenum packet_read_status status;\n> > \n> > \tif (!avail) {\n> > \t\trpc->initial_buffer = 0;\n> > \t\trpc->len = 0;\n> > -\t\tif (!rpc_read_from_out(rpc, &avail))\n> > -\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n> > -\t\tif (!avail)\n> > -\t\t\treturn 0;\n> > \t\trpc->pos = 0;\n> > +\t\tif (!rpc->stop_reading) {\n> > +\t\t\tif (!rpc_read_from_out(rpc, 0, &avail, &status))\n> > +\t\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n> \n> Do we actually need it to be LARGE_PACKET_MAX+4 here? I guess not,\n> because LARGE_PACKET_DATA_MAX is the \"-4\" version. So I think this BUG()\n> was perhaps already wrong?\n\nIn this patch, yes (if not, the non-chunked code is useless).\nPreviously, the BUG should have been LARGE_PACKET_DATA_MAX, yes. The\nBUG() was introduced in patch 4 of this set - I'll update that one to\nLARGE_PACKET_DATA_MAX and this one to LARGE_PACKET_MAX.\n\n> > +\t\t\tif (status == PACKET_READ_FLUSH)\n> > +\t\t\t\t/*\n> > +\t\t\t\t * We are done reading for this request, but we\n> > +\t\t\t\t * still need to send this line out (if\n> > +\t\t\t\t * rpc->write_line_lengths is true) so do not\n> > +\t\t\t\t * return yet.\n> > +\t\t\t\t */\n> > +\t\t\t\trpc->stop_reading = 1;\n> > +\t\t}\n> > +\t}\n> > +\tif (!avail && rpc->stop_reading) {\n> > +\t\t/*\n> > +\t\t * \"return 0\" will notify Curl that this RPC request is done,\n> > +\t\t * so reset stop_reading back to 0 for the next request.\n> > +\t\t */\n> > +\t\trpc->stop_reading = 0;\n> > +\t\treturn 0;\n> \n> OK, and here's where we handle the stop_reading thing. It is indeed\n> awkward, but I think your comments make it clear what's going on.\n> \n> If we get stop_reading, do we care about \"avail\"? I.e., shouldn't we be\n> able to return non-zero to say \"we got the whole input, this is not a\n> too-large request\"?\n\nThis code is in rpc_out(), which is a callback passed as\nCURLOPT_READFUNCTION. So returning non-zero means \"send these bytes\",\nand returning zero means EOF.\n\nWe set stop_reading when we receive a flush, as you can see from the\nquoted code snippet. But this does not mean that the buffer is empty -\nthe buffer may contain \"0000\" (if rpc->write_line_lengths is true, as\nthe comment states). We have to let libcurl repeatedly call this\nfunction until all of the \"0000\" is sent (and return non-zero each\ntime). But once all of the \"0000\" is sent - we know this by avail being\nzero - and libcurl calls this function once more, we have to remember to\ndo nothing except to reset stop_reading and return 0 to indicate EOF.\n\n> > +test_expect_success 'clone big repository with http:// using protocol v2' '\n> > +\ttest_when_finished \"rm -f log\" &&\n> > +\n> > +\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/big\" &&\n> > +\t# Ensure that the list of wants is greater than http.postbuffer below\n> > +\tfor i in $(seq 1 1500)\n> > +\tdo\n> > +\t\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/big\" \"commit$i\"\n> > +\tdone &&\n> \n> As Junio noted, this should be test_seq. But I think it would be nice to\n> avoid looping on test_commit here at all. It kicks off at least 3\n> processes; multiplying that by 1500 is going to be slow.\n> \n> Making a big input is often much faster by generating a fast-import\n> stream (which can often be done entirely in-shell). There's some prior\n> art in t3302, t5551, t5608, and others.\n\nOK - I'll look at generating a fast-import stream. Thanks for all your\ncomments.\n"},{"id":"369841","messageId":"cover.1550780213.git.jonathantanmy@google.com","threadId":"50510","inReplyTo":"cover.1550170980.git.jonathantanmy@google.com","subject":"[PATCH v2 0/5] Protocol v2 fix: http and auth","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-21T20:24:36Z","receivedAt":"2019-02-21T20:24:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks to Junio and Peff for their reviews.\n\nThe main changes are the name and comments around the stop_reading part.\nI have renamed it to flush_read_but_not_sent and have added a lot of\ncomments - I tried to err on the side of overcommenting here, since it\nis rather complicated.\n\nOther changes:\n - used test_seq instead of seq\n - used fast_import instead of test_commit 1500 times\n - in patch 4, the BUG statement now reads \"...larger than\n LARGE_PACKET_DATA_MAX; the corresponding BUG in patch 5 is\n LARGE_PACKET_MAX (this means that the total diff does not change, and\n is thus not visible in the interdiff below)\n\nI discovered a slight issue in which the full http.postBuffer is not\nused, because Git immediately switches to chunked mode if the buffer\ncannot contain a maximally sized pkt-line (without first reading the\npkt-line to see if it fits). This means that I could replace \"test-seq 1\n1500\" with \"test-seq 1 2\" and the test would still pass, but I'm still\nusing 1500 in this patch set so that the test will pass if we ever\ndecide to use the http.postBuffer slightly more efficiently.\n\nJonathan Tan (5):\n remote-curl: reduce scope of rpc_state.argv\n remote-curl: reduce scope of rpc_state.stdin_preamble\n remote-curl: reduce scope of rpc_state.result\n remote-curl: refactor reading into rpc_state's buf\n remote-curl: use post_rpc() for protocol v2 also\n\n pkt-line.c | 2 +-\n pkt-line.h | 1 +\n remote-curl.c | 362 +++++++++++++++++++----------------------\n t/t5702-protocol-v2.sh | 33 +++-\n 4 files changed, 199 insertions(+), 199 deletions(-)\n\nInterdiff against v1:\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 2394a00650..8c03c78fc6 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -526,10 +526,13 @@ struct rpc_state {\n \tunsigned write_line_lengths : 1;\n \n \t/*\n-\t * rpc_out uses this to keep track of whether it should continue\n-\t * reading to populate the current request. Initialize to 0.\n+\t * Used by rpc_out; initialize to 0. This is true if a flush has been\n+\t * read, but the corresponding line length (if write_line_lengths is\n+\t * true) and EOF have not been sent to libcurl. Since each flush marks\n+\t * the end of a request, each flush must be completely sent before any\n+\t * further reading occurs.\n \t */\n-\tunsigned stop_reading : 1;\n+\tunsigned flush_read_but_not_sent : 1;\n };\n \n /*\n@@ -600,26 +603,34 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \t\trpc->initial_buffer = 0;\n \t\trpc->len = 0;\n \t\trpc->pos = 0;\n-\t\tif (!rpc->stop_reading) {\n+\t\tif (!rpc->flush_read_but_not_sent) {\n \t\t\tif (!rpc_read_from_out(rpc, 0, &avail, &status))\n \t\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n \t\t\tif (status == PACKET_READ_FLUSH)\n-\t\t\t\t/*\n-\t\t\t\t * We are done reading for this request, but we\n-\t\t\t\t * still need to send this line out (if\n-\t\t\t\t * rpc->write_line_lengths is true) so do not\n-\t\t\t\t * return yet.\n-\t\t\t\t */\n-\t\t\t\trpc->stop_reading = 1;\n+\t\t\t\trpc->flush_read_but_not_sent = 1;\n \t\t}\n+\t\t/*\n+\t\t * If flush_read_but_not_sent is true, we have already read one\n+\t\t * full request but have not fully sent it + EOF, which is why\n+\t\t * we need to refrain from reading.\n+\t\t */\n \t}\n-\tif (!avail && rpc->stop_reading) {\n+\tif (rpc->flush_read_but_not_sent) {\n+\t\tif (!avail) {\n+\t\t\t/*\n+\t\t\t * The line length either does not need to be sent at\n+\t\t\t * all or has already been completely sent. Now we can\n+\t\t\t * return 0, indicating EOF, meaning that the flush has\n+\t\t\t * been fully sent.\n+\t\t\t */\n+\t\t\trpc->flush_read_but_not_sent = 0;\n+\t\t\treturn 0;\n+\t\t}\n \t\t/*\n-\t\t * \"return 0\" will notify Curl that this RPC request is done,\n-\t\t * so reset stop_reading back to 0 for the next request.\n+\t\t * If avail is non-zerp, the line length for the flush still\n+\t\t * hasn't been fully sent. Proceed with sending the line\n+\t\t * length.\n \t\t */\n-\t\trpc->stop_reading = 0;\n-\t\treturn 0;\n \t}\n \n \tif (max < avail)\n@@ -1290,7 +1301,7 @@ static int stateless_connect(const char *service_name)\n \trpc.gzip_request = 1;\n \trpc.initial_buffer = 0;\n \trpc.write_line_lengths = 1;\n-\trpc.stop_reading = 0;\n+\trpc.flush_read_but_not_sent = 0;\n \n \t/*\n \t * Dump the capability listing that we got from the server earlier\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 61acf99d80..e112b6086c 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -552,10 +552,17 @@ test_expect_success 'clone big repository with http:// using protocol v2' '\n \n \tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/big\" &&\n \t# Ensure that the list of wants is greater than http.postbuffer below\n-\tfor i in $(seq 1 1500)\n+\tfor i in $(test_seq 1 1500)\n \tdo\n-\t\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/big\" \"commit$i\"\n-\tdone &&\n+\t\t# do not use here-doc, because it requires a process\n+\t\t# per loop iteration\n+\t\techo \"commit refs/heads/too-many-refs-$i\" &&\n+\t\techo \"committer git $i +0000\" &&\n+\t\techo \"data 0\" &&\n+\t\techo \"M 644 inline bla.txt\" &&\n+\t\techo \"data 4\" &&\n+\t\techo \"bla\"\n+\tdone | git -C \"$HTTPD_DOCUMENT_ROOT_PATH/big\" fast-import &&\n \n \tGIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TRACE_CURL=\"$(pwd)/log\" git \\\n \t\t-c protocol.version=2 -c http.postbuffer=65536 \\\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369842","messageId":"8062cf9ce3d35bab974e745ef85ee4149619773d.1550780213.git.jonathantanmy@google.com","threadId":"50510","inReplyTo":"cover.1550780213.git.jonathantanmy@google.com","subject":"[PATCH v2 1/5] remote-curl: reduce scope of rpc_state.argv","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-21T20:24:37Z","receivedAt":"2019-02-21T20:24:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The argv field in struct rpc_state is only used in rpc_service(), and\nnot in any functions it directly or indirectly calls. Refactor it to\nbecome an argument of rpc_service() instead.\n\nSigned-off-by: Jonathan Tan \n---\n remote-curl.c | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex bb7421023b..8b7baf6298 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -505,7 +505,6 @@ static void output_refs(struct ref *refs)\n \n struct rpc_state {\n \tconst char *service_name;\n-\tconst char **argv;\n \tstruct strbuf *stdin_preamble;\n \tchar *service_url;\n \tchar *hdr_content_type;\n@@ -829,7 +828,8 @@ static int post_rpc(struct rpc_state *rpc)\n \treturn err;\n }\n \n-static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n+static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n+\t\t const char **client_argv)\n {\n \tconst char *svc = rpc->service_name;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -840,7 +840,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n \tclient.in = -1;\n \tclient.out = -1;\n \tclient.git_cmd = 1;\n-\tclient.argv = rpc->argv;\n+\tclient.argv = client_argv;\n \tif (start_command(&client))\n \t\texit(1);\n \tif (preamble)\n@@ -978,11 +978,10 @@ static int fetch_git(struct discovery *heads,\n \n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-upload-pack\",\n-\trpc.argv = args.argv;\n \trpc.stdin_preamble = &preamble;\n \trpc.gzip_request = 1;\n \n-\terr = rpc_service(&rpc, heads);\n+\terr = rpc_service(&rpc, heads, args.argv);\n \tif (rpc.result.len)\n \t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n@@ -1112,10 +1111,9 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-receive-pack\",\n-\trpc.argv = args.argv;\n \trpc.stdin_preamble = &preamble;\n \n-\terr = rpc_service(&rpc, heads);\n+\terr = rpc_service(&rpc, heads, args.argv);\n \tif (rpc.result.len)\n \t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369843","messageId":"23d2414994e8ca2559940ccd43df0e2b2ceb712b.1550780213.git.jonathantanmy@google.com","threadId":"50510","inReplyTo":"cover.1550780213.git.jonathantanmy@google.com","subject":"[PATCH v2 2/5] remote-curl: reduce scope of rpc_state.stdin_preamble","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-21T20:24:38Z","receivedAt":"2019-02-21T20:24:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The stdin_preamble field in struct rpc_state is only used in\nrpc_service(), and not in any functions it directly or indirectly calls.\nRefactor it to become an argument of rpc_service() instead.\n\nAn observation of all callers of rpc_service() shows that the preamble\nis always set, so we no longer need the \"if (preamble)\" check.\n\nSigned-off-by: Jonathan Tan \n---\n remote-curl.c | 13 ++++---------\n 1 file changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 8b7baf6298..d33d5bbfa8 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -505,7 +505,6 @@ static void output_refs(struct ref *refs)\n \n struct rpc_state {\n \tconst char *service_name;\n-\tstruct strbuf *stdin_preamble;\n \tchar *service_url;\n \tchar *hdr_content_type;\n \tchar *hdr_accept;\n@@ -829,11 +828,10 @@ static int post_rpc(struct rpc_state *rpc)\n }\n \n static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n-\t\t const char **client_argv)\n+\t\t const char **client_argv, const struct strbuf *preamble)\n {\n \tconst char *svc = rpc->service_name;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct strbuf *preamble = rpc->stdin_preamble;\n \tstruct child_process client = CHILD_PROCESS_INIT;\n \tint err = 0;\n \n@@ -843,8 +841,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n \tclient.argv = client_argv;\n \tif (start_command(&client))\n \t\texit(1);\n-\tif (preamble)\n-\t\twrite_or_die(client.in, preamble->buf, preamble->len);\n+\twrite_or_die(client.in, preamble->buf, preamble->len);\n \tif (heads)\n \t\twrite_or_die(client.in, heads->buf, heads->len);\n \n@@ -978,10 +975,9 @@ static int fetch_git(struct discovery *heads,\n \n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-upload-pack\",\n-\trpc.stdin_preamble = &preamble;\n \trpc.gzip_request = 1;\n \n-\terr = rpc_service(&rpc, heads, args.argv);\n+\terr = rpc_service(&rpc, heads, args.argv, &preamble);\n \tif (rpc.result.len)\n \t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n@@ -1111,9 +1107,8 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-receive-pack\",\n-\trpc.stdin_preamble = &preamble;\n \n-\terr = rpc_service(&rpc, heads, args.argv);\n+\terr = rpc_service(&rpc, heads, args.argv, &preamble);\n \tif (rpc.result.len)\n \t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369844","messageId":"19ccc0324803f07f3ceee2f47a998779fe656c0c.1550780213.git.jonathantanmy@google.com","threadId":"50510","inReplyTo":"cover.1550780213.git.jonathantanmy@google.com","subject":"[PATCH v2 3/5] remote-curl: reduce scope of rpc_state.result","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-21T20:24:39Z","receivedAt":"2019-02-21T20:24:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The result field in struct rpc_state is only used in rpc_service(), and\nnot in any functions it directly or indirectly calls. Refactor it to\nbecome an argument of rpc_service() instead.\n\nSigned-off-by: Jonathan Tan \n---\n remote-curl.c | 25 +++++++++++++------------\n 1 file changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex d33d5bbfa8..8e0e37ed3d 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -516,7 +516,6 @@ struct rpc_state {\n \tint in;\n \tint out;\n \tint any_written;\n-\tstruct strbuf result;\n \tunsigned gzip_request : 1;\n \tunsigned initial_buffer : 1;\n };\n@@ -828,7 +827,8 @@ static int post_rpc(struct rpc_state *rpc)\n }\n \n static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n-\t\t const char **client_argv, const struct strbuf *preamble)\n+\t\t const char **client_argv, const struct strbuf *preamble,\n+\t\t struct strbuf *rpc_result)\n {\n \tconst char *svc = rpc->service_name;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -849,7 +849,6 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n \trpc->buf = xmalloc(rpc->alloc);\n \trpc->in = client.in;\n \trpc->out = client.out;\n-\tstrbuf_init(&rpc->result, 0);\n \n \tstrbuf_addf(&buf, \"%s%s\", url.buf, svc);\n \trpc->service_url = strbuf_detach(&buf, NULL);\n@@ -877,7 +876,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n \tclose(client.in);\n \tclient.in = -1;\n \tif (!err) {\n-\t\tstrbuf_read(&rpc->result, client.out, 0);\n+\t\tstrbuf_read(rpc_result, client.out, 0);\n \t} else {\n \t\tchar buf[4096];\n \t\tfor (;;)\n@@ -930,6 +929,7 @@ static int fetch_git(struct discovery *heads,\n \tstruct strbuf preamble = STRBUF_INIT;\n \tint i, err;\n \tstruct argv_array args = ARGV_ARRAY_INIT;\n+\tstruct strbuf rpc_result = STRBUF_INIT;\n \n \targv_array_pushl(&args, \"fetch-pack\", \"--stateless-rpc\",\n \t\t\t \"--stdin\", \"--lock-pack\", NULL);\n@@ -977,10 +977,10 @@ static int fetch_git(struct discovery *heads,\n \trpc.service_name = \"git-upload-pack\",\n \trpc.gzip_request = 1;\n \n-\terr = rpc_service(&rpc, heads, args.argv, &preamble);\n-\tif (rpc.result.len)\n-\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n-\tstrbuf_release(&rpc.result);\n+\terr = rpc_service(&rpc, heads, args.argv, &preamble, &rpc_result);\n+\tif (rpc_result.len)\n+\t\twrite_or_die(1, rpc_result.buf, rpc_result.len);\n+\tstrbuf_release(&rpc_result);\n \tstrbuf_release(&preamble);\n \targv_array_clear(&args);\n \treturn err;\n@@ -1075,6 +1075,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \tstruct argv_array args;\n \tstruct string_list_item *cas_option;\n \tstruct strbuf preamble = STRBUF_INIT;\n+\tstruct strbuf rpc_result = STRBUF_INIT;\n \n \targv_array_init(&args);\n \targv_array_pushl(&args, \"send-pack\", \"--stateless-rpc\", \"--helper-status\",\n@@ -1108,10 +1109,10 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-receive-pack\",\n \n-\terr = rpc_service(&rpc, heads, args.argv, &preamble);\n-\tif (rpc.result.len)\n-\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n-\tstrbuf_release(&rpc.result);\n+\terr = rpc_service(&rpc, heads, args.argv, &preamble, &rpc_result);\n+\tif (rpc_result.len)\n+\t\twrite_or_die(1, rpc_result.buf, rpc_result.len);\n+\tstrbuf_release(&rpc_result);\n \tstrbuf_release(&preamble);\n \targv_array_clear(&args);\n \treturn err;\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369845","messageId":"2ecf63458740c631f0a1eb2acdfc877d7d06c278.1550780213.git.jonathantanmy@google.com","threadId":"50510","inReplyTo":"cover.1550780213.git.jonathantanmy@google.com","subject":"[PATCH v2 4/5] remote-curl: refactor reading into rpc_state's buf","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-21T20:24:40Z","receivedAt":"2019-02-21T20:24:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, whenever remote-curl reads pkt-lines from its response file\ndescriptor, only the payload is written to its buf, not the 4 characters\ndenoting the length. A future patch will require the ability to also\nwrite those 4 characters, so in preparation for that, refactor this read\ninto its own function.\n\nSigned-off-by: Jonathan Tan \n---\n remote-curl.c | 33 ++++++++++++++++++++++++---------\n 1 file changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 8e0e37ed3d..1f0161475d 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -520,6 +520,25 @@ struct rpc_state {\n \tunsigned initial_buffer : 1;\n };\n \n+/*\n+ * Appends the result of reading from rpc->out to the string represented by\n+ * rpc->buf and rpc->len if there is enough space. Returns 1 if there was\n+ * enough space, 0 otherwise.\n+ *\n+ * Writes the number of bytes appended into appended.\n+ */\n+static int rpc_read_from_out(struct rpc_state *rpc, size_t *appended) {\n+\tsize_t left = rpc->alloc - rpc->len;\n+\tchar *buf = rpc->buf + rpc->len;\n+\n+\tif (left < LARGE_PACKET_MAX)\n+\t\treturn 0;\n+\n+\t*appended = packet_read(rpc->out, NULL, NULL, buf, left, 0);\n+\trpc->len += *appended;\n+\treturn 1;\n+}\n+\n static size_t rpc_out(void *ptr, size_t eltsize,\n \t\tsize_t nmemb, void *buffer_)\n {\n@@ -529,11 +548,12 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \n \tif (!avail) {\n \t\trpc->initial_buffer = 0;\n-\t\tavail = packet_read(rpc->out, NULL, NULL, rpc->buf, rpc->alloc, 0);\n+\t\trpc->len = 0;\n+\t\tif (!rpc_read_from_out(rpc, &avail))\n+\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_DATA_MAX\");\n \t\tif (!avail)\n \t\t\treturn 0;\n \t\trpc->pos = 0;\n-\t\trpc->len = avail;\n \t}\n \n \tif (max < avail)\n@@ -677,20 +697,15 @@ static int post_rpc(struct rpc_state *rpc)\n \t * chunked encoding mess.\n \t */\n \twhile (1) {\n-\t\tsize_t left = rpc->alloc - rpc->len;\n-\t\tchar *buf = rpc->buf + rpc->len;\n-\t\tint n;\n+\t\tsize_t n;\n \n-\t\tif (left < LARGE_PACKET_MAX) {\n+\t\tif (!rpc_read_from_out(rpc, &n)) {\n \t\t\tlarge_request = 1;\n \t\t\tuse_gzip = 0;\n \t\t\tbreak;\n \t\t}\n-\n-\t\tn = packet_read(rpc->out, NULL, NULL, buf, left, 0);\n \t\tif (!n)\n \t\t\tbreak;\n-\t\trpc->len += n;\n \t}\n \n \tif (large_request) {\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369846","messageId":"b0ff17d324d46822da9db898d187dcc5fb0467ca.1550780213.git.jonathantanmy@google.com","threadId":"50510","inReplyTo":"cover.1550780213.git.jonathantanmy@google.com","subject":"[PATCH v2 5/5] remote-curl: use post_rpc() for protocol v2 also","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-21T20:24:41Z","receivedAt":"2019-02-21T20:24:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When transmitting and receiving POSTs for protocol v0 and v1,\nremote-curl uses post_rpc() (and associated functions), but when doing\nthe same for protocol v2, it uses a separate set of functions\n(proxy_rpc() and others). Besides duplication of code, this has caused\nat least one bug: the auth retry mechanism that was implemented in v0/v1\nwas not implemented in v2.\n\nTo fix this issue and avoid it in the future, make remote-curl also use\npost_rpc() when handling protocol v2. Because line lengths are written\nto the HTTP request in protocol v2 (unlike in protocol v0/v1), this\nnecessitates changes in post_rpc() and some of the functions it uses;\nperform these changes too.\n\nA test has been included to ensure that the code for both the unchunked\nand chunked variants of the HTTP request is exercised.\n\nNote: stateless_connect() has been updated to use the lower-level packet\nreading functions instead of struct packet_reader. The low-level control\nis necessary here because we cannot change the destination buffer of\nstruct packet_reader while it is being used; struct packet_buffer has a\npeeking mechanism which relies on the destination buffer being present\nin between a peek and a read.\n\nSigned-off-by: Jonathan Tan \n---\n pkt-line.c | 2 +-\n pkt-line.h | 1 +\n remote-curl.c | 309 ++++++++++++++++++-----------------------\n t/t5702-protocol-v2.sh | 33 ++++-\n 4 files changed, 168 insertions(+), 177 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex d4b71d3e82..60329b301b 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -117,7 +117,7 @@ void packet_buf_delim(struct strbuf *buf)\n \tstrbuf_add(buf, \"0001\", 4);\n }\n \n-static void set_packet_header(char *buf, const int size)\n+void set_packet_header(char *buf, const int size)\n {\n \tstatic char hexchar[] = \"0123456789abcdef\";\n \ndiff --git a/pkt-line.h b/pkt-line.h\nindex ad9a4a2cd7..c36cb788ed 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -25,6 +25,7 @@ void packet_delim(int fd);\n void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n void packet_buf_flush(struct strbuf *buf);\n void packet_buf_delim(struct strbuf *buf);\n+void set_packet_header(char *buf, int size);\n void packet_write(int fd_out, const char *buf, size_t size);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1f0161475d..8c03c78fc6 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -518,6 +518,21 @@ struct rpc_state {\n \tint any_written;\n \tunsigned gzip_request : 1;\n \tunsigned initial_buffer : 1;\n+\n+\t/*\n+\t * Whenever a pkt-line is read into buf, append the 4 characters\n+\t * denoting its length before appending the payload.\n+\t */\n+\tunsigned write_line_lengths : 1;\n+\n+\t/*\n+\t * Used by rpc_out; initialize to 0. This is true if a flush has been\n+\t * read, but the corresponding line length (if write_line_lengths is\n+\t * true) and EOF have not been sent to libcurl. Since each flush marks\n+\t * the end of a request, each flush must be completely sent before any\n+\t * further reading occurs.\n+\t */\n+\tunsigned flush_read_but_not_sent : 1;\n };\n \n /*\n@@ -525,17 +540,54 @@ struct rpc_state {\n * rpc->buf and rpc->len if there is enough space. Returns 1 if there was\n * enough space, 0 otherwise.\n *\n- * Writes the number of bytes appended into appended.\n+ * If rpc->write_line_lengths is true, appends the line length as a 4-byte\n+ * hexadecimal string before appending the result described above.\n+ *\n+ * Writes the total number of bytes appended into appended.\n */\n-static int rpc_read_from_out(struct rpc_state *rpc, size_t *appended) {\n-\tsize_t left = rpc->alloc - rpc->len;\n-\tchar *buf = rpc->buf + rpc->len;\n+static int rpc_read_from_out(struct rpc_state *rpc, int options,\n+\t\t\t size_t *appended,\n+\t\t\t enum packet_read_status *status) {\n+\tsize_t left;\n+\tchar *buf;\n+\tint pktlen_raw;\n+\n+\tif (rpc->write_line_lengths) {\n+\t\tleft = rpc->alloc - rpc->len - 4;\n+\t\tbuf = rpc->buf + rpc->len + 4;\n+\t} else {\n+\t\tleft = rpc->alloc - rpc->len;\n+\t\tbuf = rpc->buf + rpc->len;\n+\t}\n \n \tif (left < LARGE_PACKET_MAX)\n \t\treturn 0;\n \n-\t*appended = packet_read(rpc->out, NULL, NULL, buf, left, 0);\n-\trpc->len += *appended;\n+\t*status = packet_read_with_status(rpc->out, NULL, NULL, buf,\n+\t\t\tleft, &pktlen_raw, options);\n+\tif (*status != PACKET_READ_EOF) {\n+\t\t*appended = pktlen_raw + (rpc->write_line_lengths ? 4 : 0);\n+\t\trpc->len += *appended;\n+\t}\n+\n+\tif (rpc->write_line_lengths) {\n+\t\tswitch (*status) {\n+\t\tcase PACKET_READ_EOF:\n+\t\t\tif (!(options & PACKET_READ_GENTLE_ON_EOF))\n+\t\t\t\tdie(\"shouldn't have EOF when not gentle on EOF\");\n+\t\t\tbreak;\n+\t\tcase PACKET_READ_NORMAL:\n+\t\t\tset_packet_header(buf - 4, *appended);\n+\t\t\tbreak;\n+\t\tcase PACKET_READ_DELIM:\n+\t\t\tmemcpy(buf - 4, \"0001\", 4);\n+\t\t\tbreak;\n+\t\tcase PACKET_READ_FLUSH:\n+\t\t\tmemcpy(buf - 4, \"0000\", 4);\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \treturn 1;\n }\n \n@@ -545,15 +597,40 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \tsize_t max = eltsize * nmemb;\n \tstruct rpc_state *rpc = buffer_;\n \tsize_t avail = rpc->len - rpc->pos;\n+\tenum packet_read_status status;\n \n \tif (!avail) {\n \t\trpc->initial_buffer = 0;\n \t\trpc->len = 0;\n-\t\tif (!rpc_read_from_out(rpc, &avail))\n-\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_DATA_MAX\");\n-\t\tif (!avail)\n-\t\t\treturn 0;\n \t\trpc->pos = 0;\n+\t\tif (!rpc->flush_read_but_not_sent) {\n+\t\t\tif (!rpc_read_from_out(rpc, 0, &avail, &status))\n+\t\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n+\t\t\tif (status == PACKET_READ_FLUSH)\n+\t\t\t\trpc->flush_read_but_not_sent = 1;\n+\t\t}\n+\t\t/*\n+\t\t * If flush_read_but_not_sent is true, we have already read one\n+\t\t * full request but have not fully sent it + EOF, which is why\n+\t\t * we need to refrain from reading.\n+\t\t */\n+\t}\n+\tif (rpc->flush_read_but_not_sent) {\n+\t\tif (!avail) {\n+\t\t\t/*\n+\t\t\t * The line length either does not need to be sent at\n+\t\t\t * all or has already been completely sent. Now we can\n+\t\t\t * return 0, indicating EOF, meaning that the flush has\n+\t\t\t * been fully sent.\n+\t\t\t */\n+\t\t\trpc->flush_read_but_not_sent = 0;\n+\t\t\treturn 0;\n+\t\t}\n+\t\t/*\n+\t\t * If avail is non-zerp, the line length for the flush still\n+\t\t * hasn't been fully sent. Proceed with sending the line\n+\t\t * length.\n+\t\t */\n \t}\n \n \tif (max < avail)\n@@ -698,13 +775,14 @@ static int post_rpc(struct rpc_state *rpc)\n \t */\n \twhile (1) {\n \t\tsize_t n;\n+\t\tenum packet_read_status status;\n \n-\t\tif (!rpc_read_from_out(rpc, &n)) {\n+\t\tif (!rpc_read_from_out(rpc, 0, &n, &status)) {\n \t\t\tlarge_request = 1;\n \t\t\tuse_gzip = 0;\n \t\t\tbreak;\n \t\t}\n-\t\tif (!n)\n+\t\tif (status == PACKET_READ_FLUSH)\n \t\t\tbreak;\n \t}\n \n@@ -1179,165 +1257,11 @@ static void parse_push(struct strbuf *buf)\n \tfree(specs);\n }\n \n-/*\n- * Used to represent the state of a connection to an HTTP server when\n- * communicating using git's wire-protocol version 2.\n- */\n-struct proxy_state {\n-\tchar *service_name;\n-\tchar *service_url;\n-\tstruct curl_slist *headers;\n-\tstruct strbuf request_buffer;\n-\tint in;\n-\tint out;\n-\tstruct packet_reader reader;\n-\tsize_t pos;\n-\tint seen_flush;\n-};\n-\n-static void proxy_state_init(struct proxy_state *p, const char *service_name,\n-\t\t\t enum protocol_version version)\n-{\n-\tstruct strbuf buf = STRBUF_INIT;\n-\n-\tmemset(p, 0, sizeof(*p));\n-\tp->service_name = xstrdup(service_name);\n-\n-\tp->in = 0;\n-\tp->out = 1;\n-\tstrbuf_init(&p->request_buffer, 0);\n-\n-\tstrbuf_addf(&buf, \"%s%s\", url.buf, p->service_name);\n-\tp->service_url = strbuf_detach(&buf, NULL);\n-\n-\tp->headers = http_copy_default_headers();\n-\n-\tstrbuf_addf(&buf, \"Content-Type: application/x-%s-request\", p->service_name);\n-\tp->headers = curl_slist_append(p->headers, buf.buf);\n-\tstrbuf_reset(&buf);\n-\n-\tstrbuf_addf(&buf, \"Accept: application/x-%s-result\", p->service_name);\n-\tp->headers = curl_slist_append(p->headers, buf.buf);\n-\tstrbuf_reset(&buf);\n-\n-\tp->headers = curl_slist_append(p->headers, \"Transfer-Encoding: chunked\");\n-\n-\t/* Add the Git-Protocol header */\n-\tif (get_protocol_http_header(version, &buf))\n-\t\tp->headers = curl_slist_append(p->headers, buf.buf);\n-\n-\tpacket_reader_init(&p->reader, p->in, NULL, 0,\n-\t\t\t PACKET_READ_GENTLE_ON_EOF |\n-\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n-\n-\tstrbuf_release(&buf);\n-}\n-\n-static void proxy_state_clear(struct proxy_state *p)\n-{\n-\tfree(p->service_name);\n-\tfree(p->service_url);\n-\tcurl_slist_free_all(p->headers);\n-\tstrbuf_release(&p->request_buffer);\n-}\n-\n-/*\n- * CURLOPT_READFUNCTION callback function.\n- * Attempts to copy over a single packet-line at a time into the\n- * curl provided buffer.\n- */\n-static size_t proxy_in(char *buffer, size_t eltsize,\n-\t\t size_t nmemb, void *userdata)\n-{\n-\tsize_t max;\n-\tstruct proxy_state *p = userdata;\n-\tsize_t avail = p->request_buffer.len - p->pos;\n-\n-\n-\tif (eltsize != 1)\n-\t\tBUG(\"curl read callback called with size = %\"PRIuMAX\" != 1\",\n-\t\t (uintmax_t)eltsize);\n-\tmax = nmemb;\n-\n-\tif (!avail) {\n-\t\tif (p->seen_flush) {\n-\t\t\tp->seen_flush = 0;\n-\t\t\treturn 0;\n-\t\t}\n-\n-\t\tstrbuf_reset(&p->request_buffer);\n-\t\tswitch (packet_reader_read(&p->reader)) {\n-\t\tcase PACKET_READ_EOF:\n-\t\t\tdie(\"unexpected EOF when reading from parent process\");\n-\t\tcase PACKET_READ_NORMAL:\n-\t\t\tpacket_buf_write_len(&p->request_buffer, p->reader.line,\n-\t\t\t\t\t p->reader.pktlen);\n-\t\t\tbreak;\n-\t\tcase PACKET_READ_DELIM:\n-\t\t\tpacket_buf_delim(&p->request_buffer);\n-\t\t\tbreak;\n-\t\tcase PACKET_READ_FLUSH:\n-\t\t\tpacket_buf_flush(&p->request_buffer);\n-\t\t\tp->seen_flush = 1;\n-\t\t\tbreak;\n-\t\t}\n-\t\tp->pos = 0;\n-\t\tavail = p->request_buffer.len;\n-\t}\n-\n-\tif (max < avail)\n-\t\tavail = max;\n-\tmemcpy(buffer, p->request_buffer.buf + p->pos, avail);\n-\tp->pos += avail;\n-\treturn avail;\n-}\n-\n-static size_t proxy_out(char *buffer, size_t eltsize,\n-\t\t\tsize_t nmemb, void *userdata)\n-{\n-\tsize_t size;\n-\tstruct proxy_state *p = userdata;\n-\n-\tif (eltsize != 1)\n-\t\tBUG(\"curl read callback called with size = %\"PRIuMAX\" != 1\",\n-\t\t (uintmax_t)eltsize);\n-\tsize = nmemb;\n-\n-\twrite_or_die(p->out, buffer, size);\n-\treturn size;\n-}\n-\n-/* Issues a request to the HTTP server configured in `p` */\n-static int proxy_request(struct proxy_state *p)\n-{\n-\tstruct active_request_slot *slot;\n-\n-\tslot = get_active_slot();\n-\n-\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n-\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_POST, 1);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, p->headers);\n-\n-\t/* Setup function to read request from client */\n-\tcurl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, proxy_in);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_READDATA, p);\n-\n-\t/* Setup function to write server response to client */\n-\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, proxy_out);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, p);\n-\n-\tif (run_slot(slot, NULL) != HTTP_OK)\n-\t\treturn -1;\n-\n-\treturn 0;\n-}\n-\n static int stateless_connect(const char *service_name)\n {\n \tstruct discovery *discover;\n-\tstruct proxy_state p;\n+\tstruct rpc_state rpc;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \t/*\n \t * Run the info/refs request and see if the server supports protocol\n@@ -1357,23 +1281,58 @@ static int stateless_connect(const char *service_name)\n \t\tfflush(stdout);\n \t}\n \n-\tproxy_state_init(&p, service_name, discover->version);\n+\trpc.service_name = service_name;\n+\trpc.service_url = xstrfmt(\"%s%s\", url.buf, rpc.service_name);\n+\trpc.hdr_content_type = xstrfmt(\"Content-Type: application/x-%s-request\", rpc.service_name);\n+\trpc.hdr_accept = xstrfmt(\"Accept: application/x-%s-result\", rpc.service_name);\n+\tif (get_protocol_http_header(discover->version, &buf)) {\n+\t\trpc.protocol_header = strbuf_detach(&buf, NULL);\n+\t} else {\n+\t\trpc.protocol_header = NULL;\n+\t\tstrbuf_release(&buf);\n+\t}\n+\trpc.buf = xmalloc(http_post_buffer);\n+\trpc.alloc = http_post_buffer;\n+\trpc.len = 0;\n+\trpc.pos = 0;\n+\trpc.in = 1;\n+\trpc.out = 0;\n+\trpc.any_written = 0;\n+\trpc.gzip_request = 1;\n+\trpc.initial_buffer = 0;\n+\trpc.write_line_lengths = 1;\n+\trpc.flush_read_but_not_sent = 0;\n \n \t/*\n \t * Dump the capability listing that we got from the server earlier\n \t * during the info/refs request.\n \t */\n-\twrite_or_die(p.out, discover->buf, discover->len);\n+\twrite_or_die(rpc.in, discover->buf, discover->len);\n+\n+\t/* Until we see EOF keep sending POSTs */\n+\twhile (1) {\n+\t\tsize_t avail;\n+\t\tenum packet_read_status status;\n \n-\t/* Peek the next packet line. Until we see EOF keep sending POSTs */\n-\twhile (packet_reader_peek(&p.reader) != PACKET_READ_EOF) {\n-\t\tif (proxy_request(&p)) {\n+\t\tif (!rpc_read_from_out(&rpc, PACKET_READ_GENTLE_ON_EOF, &avail,\n+\t\t\t\t &status))\n+\t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n+\t\tif (status == PACKET_READ_EOF)\n+\t\t\tbreak;\n+\t\tif (post_rpc(&rpc))\n \t\t\t/* We would have an err here */\n \t\t\tbreak;\n-\t\t}\n+\t\t/* Reset the buffer for next request */\n+\t\trpc.len = 0;\n \t}\n \n-\tproxy_state_clear(&p);\n+\tfree(rpc.service_url);\n+\tfree(rpc.hdr_content_type);\n+\tfree(rpc.hdr_accept);\n+\tfree(rpc.protocol_header);\n+\tfree(rpc.buf);\n+\tstrbuf_release(&buf);\n+\n \treturn 0;\n }\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex db4ae09f2f..e112b6086c 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -542,7 +542,38 @@ test_expect_success 'clone with http:// using protocol v2' '\n \t# Client requested to use protocol v2\n \tgrep \"Git-Protocol: version=2\" log &&\n \t# Server responded using protocol v2\n-\tgrep \"git< version 2\" log\n+\tgrep \"git< version 2\" log &&\n+\t# Verify that the chunked encoding sending codepath is NOT exercised\n+\t! grep \"Send header: Transfer-Encoding: chunked\" log\n+'\n+\n+test_expect_success 'clone big repository with http:// using protocol v2' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/big\" &&\n+\t# Ensure that the list of wants is greater than http.postbuffer below\n+\tfor i in $(test_seq 1 1500)\n+\tdo\n+\t\t# do not use here-doc, because it requires a process\n+\t\t# per loop iteration\n+\t\techo \"commit refs/heads/too-many-refs-$i\" &&\n+\t\techo \"committer git $i +0000\" &&\n+\t\techo \"data 0\" &&\n+\t\techo \"M 644 inline bla.txt\" &&\n+\t\techo \"data 4\" &&\n+\t\techo \"bla\"\n+\tdone | git -C \"$HTTPD_DOCUMENT_ROOT_PATH/big\" fast-import &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TRACE_CURL=\"$(pwd)/log\" git \\\n+\t\t-c protocol.version=2 -c http.postbuffer=65536 \\\n+\t\tclone \"$HTTPD_URL/smart/big\" big_child &&\n+\n+\t# Client requested to use protocol v2\n+\tgrep \"Git-Protocol: version=2\" log &&\n+\t# Server responded using protocol v2\n+\tgrep \"git< version 2\" log &&\n+\t# Verify that the chunked encoding sending codepath is exercised\n+\tgrep \"Send header: Transfer-Encoding: chunked\" log\n '\n \n test_expect_success 'fetch with http:// using protocol v2' '\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369909","messageId":"CAPig+cQNg26zHm=ns3PsmPHEsNo4M7hTSG8=oO_uKqEa4wf3Vw@mail.gmail.com","threadId":"50510","inReplyTo":"b0ff17d324d46822da9db898d187dcc5fb0467ca.1550780213.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 5/5] remote-curl: use post_rpc() for protocol v2 also","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-22T13:18:06Z","receivedAt":"2019-02-22T13:18:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 21, 2019 at 3:25 PM Jonathan Tan wrote:\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> @@ -542,7 +542,38 @@ test_expect_success 'clone with http:// using protocol v2' '\n> +test_expect_success 'clone big repository with http:// using protocol v2' '\n> + test_when_finished \"rm -f log\" &&\n> +\n> + git init \"$HTTPD_DOCUMENT_ROOT_PATH/big\" &&\n> + # Ensure that the list of wants is greater than http.postbuffer below\n> + for i in $(test_seq 1 1500)\n> + do\n> + # do not use here-doc, because it requires a process\n> + # per loop iteration\n> + echo \"commit refs/heads/too-many-refs-$i\" &&\n> + echo \"committer git $i +0000\" &&\n> + echo \"data 0\" &&\n> + echo \"M 644 inline bla.txt\" &&\n> + echo \"data 4\" &&\n> + echo \"bla\"\n> + done | git -C \"$HTTPD_DOCUMENT_ROOT_PATH/big\" fast-import &&\n\nAlthough not outright wrong, maintaining an &&-chain in the loop body\nis a bit misleading here for a couple reasons. First, without \"|| exit\n1\" at the end of the body, neither the loop nor the test itself will\nabort early if some command within the body fails. Second, the exit\nstatus of the loop is lost anyhow due to being upstream of a pipe.\n\nBy the way, I see that this same script already contains a couple\ntests with loops neglecting the trailing \"|| exit 1\", hence they too\nfail to abort early if something goes wrong inside the body (which, in\nboth cases, contains a test_commit() invocation). Fixing those might\nbe fodder as a follow-up patch for someone.\n"},{"id":"369967","messageId":"CAPig+cS02Vr9Wwz2+UWcm1f1oNO6hcGe35VpM5RNdc=fTHJhxQ@mail.gmail.com","threadId":"50510","inReplyTo":"CAPig+cQNg26zHm=ns3PsmPHEsNo4M7hTSG8=oO_uKqEa4wf3Vw@mail.gmail.com","subject":"Re: [PATCH v2 5/5] remote-curl: use post_rpc() for protocol v2 also","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-22T19:15:02Z","receivedAt":"2019-02-22T19:15:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 22, 2019 at 8:18 AM Eric Sunshine wrote:\n> On Thu, Feb 21, 2019 at 3:25 PM Jonathan Tan wrote:\n> > + for i in $(test_seq 1 1500)\n> > + do\n> > + # do not use here-doc, because it requires a process\n> > + # per loop iteration\n> > + echo \"commit refs/heads/too-many-refs-$i\" &&\n> > + echo \"committer git $i +0000\" &&\n> > + echo \"data 0\" &&\n> > + echo \"M 644 inline bla.txt\" &&\n> > + echo \"data 4\" &&\n> > + echo \"bla\"\n> > + done | git -C \"$HTTPD_DOCUMENT_ROOT_PATH/big\" fast-import &&\n>\n> Although not outright wrong, maintaining an &&-chain in the loop body\n> is a bit misleading here for a couple reasons. First, without \"|| exit\n> 1\" at the end of the body, neither the loop nor the test itself will\n> abort early if some command within the body fails. [...]\n\nI meant \"|| return 1\", not \"|| exit 1\".\n\n> By the way, I see that this same script already contains a couple\n> tests with loops neglecting the trailing \"|| exit 1\", hence they too\n> fail to abort early if something goes wrong inside the body [...]\n\nDitto.\n"},{"id":"370155","messageId":"20190225220841.GA3248@sigill.intra.peff.net","threadId":"50510","inReplyTo":"b0ff17d324d46822da9db898d187dcc5fb0467ca.1550780213.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 5/5] remote-curl: use post_rpc() for protocol v2 also","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-25T22:08:41Z","receivedAt":"2019-02-25T22:08:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 21, 2019 at 12:24:41PM -0800, Jonathan Tan wrote:\n\n> When transmitting and receiving POSTs for protocol v0 and v1,\n> remote-curl uses post_rpc() (and associated functions), but when doing\n> the same for protocol v2, it uses a separate set of functions\n> (proxy_rpc() and others). Besides duplication of code, this has caused\n> at least one bug: the auth retry mechanism that was implemented in v0/v1\n> was not implemented in v2.\n> \n> To fix this issue and avoid it in the future, make remote-curl also use\n> post_rpc() when handling protocol v2. Because line lengths are written\n> to the HTTP request in protocol v2 (unlike in protocol v0/v1), this\n> necessitates changes in post_rpc() and some of the functions it uses;\n> perform these changes too.\n\nThis patch seems to cause noop fetches from GitHub to report \"the remote\nend hung up unexpectedly\" at the end of the fetch (I have\nprotocol.version=2 set in the examples below):\n\n [parent is good]\n $ git checkout 0cdb2a12ad0300b5d0cb5bb6e8999034ae4b9bef^\n $ make\n $ bin-wrappers/git fetch https://github.com/git/git\n From https://github.com/git/git\n * branch HEAD -> FETCH_HEAD\n\n [this patch is bad]\n $ git checkout 0cdb2a12ad0300b5d0cb5bb6e8999034ae4b9bef\n $ make\n $ bin-wrappers/git fetch https://github.com/git/git\n From https://github.com/git/git\n * branch HEAD -> FETCH_HEAD\n fatal: the remote end hung up unexpectedly\n\nI haven't dug, so it's possible that GitHub's server-side is doing\nsomething slightly odd or wrong. But given the nature of the patch, I'd\nsuspect we just aren't noticing EOF correctly.\n\n-Peff\n"},{"id":"370186","messageId":"20190225234901.65277-1-jonathantanmy@google.com","threadId":"50510","inReplyTo":"20190225220841.GA3248@sigill.intra.peff.net","subject":"[FIXUP] Fixup on tip of jt/http-auth-proto-v2-fix","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-25T23:49:01Z","receivedAt":"2019-02-25T23:49:07Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks, Peff, for noticing this. It's because the client sometimes sends\n\"0000\" as a single request (that is, it flushes, and then before it\nsends any data, it flushes again). And post_rpc() assumes that it can\nalways read something - which is usually correct, but not in this case;\nwe read in stateless_connect() first, and if we read \"0000\", we need to\ntell post_rpc() to not read at all.\n\nThis is a fixup on the tip of jt/http-auth-proto-v2-fix that fixes that.\n\nAs for why the client sends a lone \"0000\", I'm not sure, but that's\noutside the scope of this patch set, I think.\n\nSigned-off-by: Jonathan Tan \n---\n remote-curl.c | 32 +++++++++++++++++++-------------\n 1 file changed, 19 insertions(+), 13 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 2394a00650..6c0207f4f9 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -747,7 +747,11 @@ static curl_off_t xcurl_off_t(size_t len)\n \treturn (curl_off_t)size;\n }\n \n-static int post_rpc(struct rpc_state *rpc)\n+/*\n+ * If flush_received is true, do not attempt to read any more; just use what's\n+ * in rpc->buf.\n+ */\n+static int post_rpc(struct rpc_state *rpc, int flush_received)\n {\n \tstruct active_request_slot *slot;\n \tstruct curl_slist *headers = http_copy_default_headers();\n@@ -762,17 +766,19 @@ static int post_rpc(struct rpc_state *rpc)\n \t * allocated buffer space we can use HTTP/1.0 and avoid the\n \t * chunked encoding mess.\n \t */\n-\twhile (1) {\n-\t\tsize_t n;\n-\t\tenum packet_read_status status;\n-\n-\t\tif (!rpc_read_from_out(rpc, 0, &n, &status)) {\n-\t\t\tlarge_request = 1;\n-\t\t\tuse_gzip = 0;\n-\t\t\tbreak;\n+\tif (!flush_received) {\n+\t\twhile (1) {\n+\t\t\tsize_t n;\n+\t\t\tenum packet_read_status status;\n+\n+\t\t\tif (!rpc_read_from_out(rpc, 0, &n, &status)) {\n+\t\t\t\tlarge_request = 1;\n+\t\t\t\tuse_gzip = 0;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (status == PACKET_READ_FLUSH)\n+\t\t\t\tbreak;\n \t\t}\n-\t\tif (status == PACKET_READ_FLUSH)\n-\t\t\tbreak;\n \t}\n \n \tif (large_request) {\n@@ -952,7 +958,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n \t\t\tbreak;\n \t\trpc->pos = 0;\n \t\trpc->len = n;\n-\t\terr |= post_rpc(rpc);\n+\t\terr |= post_rpc(rpc, 0);\n \t}\n \n \tclose(client.in);\n@@ -1308,7 +1314,7 @@ static int stateless_connect(const char *service_name)\n \t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n \t\tif (status == PACKET_READ_EOF)\n \t\t\tbreak;\n-\t\tif (post_rpc(&rpc))\n+\t\tif (post_rpc(&rpc, status == PACKET_READ_FLUSH))\n \t\t\t/* We would have an err here */\n \t\t\tbreak;\n \t\t/* Reset the buffer for next request */\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"370209","messageId":"20190226070408.GA117495@google.com","threadId":"50510","inReplyTo":"20190225234901.65277-1-jonathantanmy@google.com","subject":"Re: [FIXUP] Fixup on tip of jt/http-auth-proto-v2-fix","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-02-26T07:04:08Z","receivedAt":"2019-02-26T07:04:14Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> Thanks, Peff, for noticing this. It's because the client sometimes sends\n> \"0000\" as a single request (that is, it flushes, and then before it\n> sends any data, it flushes again). And post_rpc() assumes that it can\n> always read something - which is usually correct, but not in this case;\n> we read in stateless_connect() first, and if we read \"0000\", we need to\n> tell post_rpc() to not read at all.\n>\n> This is a fixup on the tip of jt/http-auth-proto-v2-fix that fixes that.\n>\n> As for why the client sends a lone \"0000\", I'm not sure, but that's\n> outside the scope of this patch set, I think.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> remote-curl.c | 32 +++++++++++++++++++-------------\n> 1 file changed, 19 insertions(+), 13 deletions(-)\n\nTested-by: Jonathan Nieder \n\nThanks for fixing it.\n\nIs there a particular patch this should be squashed into, or does it\nstand alone? It the latter, mind writing a commit message for it?\n\nThanks,\nJonathan\n"},{"id":"370259","messageId":"20190226181859.65894-1-jonathantanmy@google.com","threadId":"50510","inReplyTo":"20190226070408.GA117495@google.com","subject":"Re: [FIXUP] Fixup on tip of jt/http-auth-proto-v2-fix","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-26T18:18:59Z","receivedAt":"2019-02-26T18:19:05Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Thanks for fixing it.\n> \n> Is there a particular patch this should be squashed into, or does it\n> stand alone? It the latter, mind writing a commit message for it?\n\nNot sure if I'm using \"fixup\" correctly in the subject, but this is\nmeant to be squashed onto the tip of jt/http-auth-proto-v2-fix -\nspecifically, deb7d2094a (\"remote-curl: use post_rpc() for protocol v2\nalso\", 2019-02-14).\n\nI don't think it should be a standalone commit, as the tip is buggy -\nwhich might break bisect. But if we really want a standalone commit,\nthis commit message should work:\n\nremote-curl: handle consecutive flushes correctly\n\nWhen the client, using protocol v2, sends two consecutive flushes and\nthen an EOF, remote-curl dies. This is because, at the start of a\nnew request, stateless_connect() reads, and if no EOF is found, then\nstateless_connect() calls post_rpc() which reads until a flush is\nencountered. This is a problem when stateless_connect() reads the second\nconsecutive flush (hence, no EOF), and then post_rpc() reads, not\nexpecting an EOF at all.\n\nTeach stateless_connect() to inform post_rpc() to read only if what it\nread isn't a flush.\n"},{"id":"370298","messageId":"20190227120219.GA10305@sigill.intra.peff.net","threadId":"50510","inReplyTo":"20190225234901.65277-1-jonathantanmy@google.com","subject":"Re: [FIXUP] Fixup on tip of jt/http-auth-proto-v2-fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-27T12:02:19Z","receivedAt":"2019-02-27T12:02:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 25, 2019 at 03:49:01PM -0800, Jonathan Tan wrote:\n\n> Thanks, Peff, for noticing this. It's because the client sometimes sends\n> \"0000\" as a single request (that is, it flushes, and then before it\n> sends any data, it flushes again). And post_rpc() assumes that it can\n> always read something - which is usually correct, but not in this case;\n> we read in stateless_connect() first, and if we read \"0000\", we need to\n> tell post_rpc() to not read at all.\n> \n> This is a fixup on the tip of jt/http-auth-proto-v2-fix that fixes that.\n\nThanks, I can confirm that this makes the problem go away (and your\nexplanation makes perfect sense to me).\n\n> As for why the client sends a lone \"0000\", I'm not sure, but that's\n> outside the scope of this patch set, I think.\n\nYeah, that does seem odd. I noticed it on noop fetches. So after we've\ndone \"ls-refs\", would \"fetch\" need to send a flush to say \"I don't want\nanything?\" I guess not, since we're stateless, and it is literally\nmaking a new HTTP request just to say nothing.\n\nIt does seem unique to protocol v2.\n\n-Peff\n"},{"id":"370569","messageId":"xmqqh8cjxpy1.fsf@gitster-ct.c.googlers.com","threadId":"50510","inReplyTo":"20190226181859.65894-1-jonathantanmy@google.com","subject":"Re: [FIXUP] Fixup on tip of jt/http-auth-proto-v2-fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-04T03:45:42Z","receivedAt":"2019-03-04T03:45:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> Thanks for fixing it.\n>> \n>> Is there a particular patch this should be squashed into, or does it\n>> stand alone? It the latter, mind writing a commit message for it?\n>\n> Not sure if I'm using \"fixup\" correctly in the subject, but this is\n> meant to be squashed onto the tip of jt/http-auth-proto-v2-fix -\n> specifically, deb7d2094a (\"remote-curl: use post_rpc() for protocol v2\n> also\", 2019-02-14).\n\nSaying \"Fixup on tip of\" to mean that you are fixing the 5th one in\na 5-patch series is sufficient to convey what you want between\nhumans, but that does not help the machine to help humans reduce\nmistakes. It would have been even more helpful if you created this\ncommit with\n\n\tgit commit --fixup 0cdb2a12ad\n\nwhich would have produced something like the attached patch,\nand that would have allowed me to do\n\n\tgit checkout jt/http-auth-proto-v2-fix\n\tgit am ./+jt-http-auth-proto-v2-fix-fixup\n\tgit rebase -i --autosquash HEAD~5\n\nor somesuch.\n\nThanks, will queue.\n\n-- >8 --\nSubject: [PATCH] fixup! remote-curl: use post_rpc() for protocol v2 also\n\nSigned-off-by: Jonathan Tan \n---\n remote-curl.c | 32 +++++++++++++++++++-------------\n 1 file changed, 19 insertions(+), 13 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 8c03c78fc6..3c3872cca6 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -758,7 +758,11 @@ static curl_off_t xcurl_off_t(size_t len)\n \treturn (curl_off_t)size;\n }\n \n-static int post_rpc(struct rpc_state *rpc)\n+/*\n+ * If flush_received is true, do not attempt to read any more; just use what's\n+ * in rpc->buf.\n+ */\n+static int post_rpc(struct rpc_state *rpc, int flush_received)\n {\n \tstruct active_request_slot *slot;\n \tstruct curl_slist *headers = http_copy_default_headers();\n@@ -773,17 +777,19 @@ static int post_rpc(struct rpc_state *rpc)\n \t * allocated buffer space we can use HTTP/1.0 and avoid the\n \t * chunked encoding mess.\n \t */\n-\twhile (1) {\n-\t\tsize_t n;\n-\t\tenum packet_read_status status;\n-\n-\t\tif (!rpc_read_from_out(rpc, 0, &n, &status)) {\n-\t\t\tlarge_request = 1;\n-\t\t\tuse_gzip = 0;\n-\t\t\tbreak;\n+\tif (!flush_received) {\n+\t\twhile (1) {\n+\t\t\tsize_t n;\n+\t\t\tenum packet_read_status status;\n+\n+\t\t\tif (!rpc_read_from_out(rpc, 0, &n, &status)) {\n+\t\t\t\tlarge_request = 1;\n+\t\t\t\tuse_gzip = 0;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (status == PACKET_READ_FLUSH)\n+\t\t\t\tbreak;\n \t\t}\n-\t\tif (status == PACKET_READ_FLUSH)\n-\t\t\tbreak;\n \t}\n \n \tif (large_request) {\n@@ -963,7 +969,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n \t\t\tbreak;\n \t\trpc->pos = 0;\n \t\trpc->len = n;\n-\t\terr |= post_rpc(rpc);\n+\t\terr |= post_rpc(rpc, 0);\n \t}\n \n \tclose(client.in);\n@@ -1319,7 +1325,7 @@ static int stateless_connect(const char *service_name)\n \t\t\tBUG(\"The entire rpc->buf should be larger than LARGE_PACKET_MAX\");\n \t\tif (status == PACKET_READ_EOF)\n \t\t\tbreak;\n-\t\tif (post_rpc(&rpc))\n+\t\tif (post_rpc(&rpc, status == PACKET_READ_FLUSH))\n \t\t\t/* We would have an err here */\n \t\t\tbreak;\n \t\t/* Reset the buffer for next request */\n-- \n2.21.0\n\n\n"}]}