{"thread":{"id":"50205","subject":"[PATCH 0/4] Sideband the whole fetch v2 response","startedAt":"2019-01-11T22:18:38Z","lastAt":"2019-02-13T06:49:10Z","messageCount":41,"participants":["Jonathan Tan","Junio C Hamano","SZEDER Gábor","Jonathan Nieder","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"366584","messageId":"cover.1547244620.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":null,"subject":"[PATCH 0/4] Sideband the whole fetch v2 response","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-11T22:18:13Z","receivedAt":"2019-01-11T22:18:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is on ms/packet-err-check.\n\nThis is a follow-up of my earlier work to allow partial CDN offloading\nof fetch v2 response packfiles, which had the issue of having to buffer\n(or suspend) progress and keepalive messages because we didn't have\nsideband until the packfile section started (as I wrote here [1]). These\npatches expand the sideband to the whole response, eliminating that\nproblem.\n\nThere are 8 patches total. I'm most interested in review of the first 4\npatches for inclusion into Git - I think it would be useful for servers\nto be allowed to send progress messages and other notices whenever they\nneed to, especially if other sections are added to the response (like in\nthe case of CDN offloading).\n\nThe other 4 patches are from my CDN offloading work, included here to\nshow how this sideband-all feature could be used. But take note that\nthey are all WIP and that I still haven't incorporated some of the\ndesign considerations from the earlier review [2].\n\n[1] https://public-inbox.org/git/20181206232538.141378-1-jonathantanmy@google.com/\n[2] https://public-inbox.org/git/cover.1543879256.git.jonathantanmy@google.com/\n\nJonathan Tan (8):\n pkt-line: introduce struct packet_writer\n sideband: reverse its dependency on pkt-line\n {fetch,upload}-pack: sideband v2 fetch response\n tests: define GIT_TEST_SIDEBAND_ALL\n Documentation: order protocol v2 sections\n Documentation: add Packfile URIs design doc\n upload-pack: refactor reading of pack-objects out\n upload-pack: send part of packfile response as uri\n\n Documentation/technical/packfile-uri.txt | 83 ++++++++\n Documentation/technical/protocol-v2.txt | 32 ++-\n builtin/pack-objects.c | 48 +++++\n fetch-pack.c | 22 +-\n pkt-line.c | 114 ++++++++--\n pkt-line.h | 34 +++\n sideband.c | 161 +++++++-------\n sideband.h | 15 +-\n t/README | 5 +\n t/lib-httpd/apache.conf | 1 +\n t/t5537-fetch-shallow.sh | 3 +-\n t/t5701-git-serve.sh | 2 +-\n t/t5702-protocol-v2.sh | 31 ++-\n upload-pack.c | 259 +++++++++++++++--------\n 14 files changed, 607 insertions(+), 203 deletions(-)\n create mode 100644 Documentation/technical/packfile-uri.txt\n\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366585","messageId":"9b37e68e2f0d83ea133f6662efb780466c518170.1547244620.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547244620.git.jonathantanmy@google.com","subject":"[PATCH 1/4] pkt-line: introduce struct packet_writer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-11T22:18:14Z","receivedAt":"2019-01-11T22:18:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"It will be convenient for a future patch if writing options\n(specifically, whether the written data is to be multiplexed) could be\ncontrolled from a single place, so create struct packet_writer to serve\nas that place, and modify upload-pack to use it.\n\nCurrently, it only stores the output fd, but a subsequent patch will (as\ndescribed above) introduce an option to determine if the written data is\nto be multiplexed.\n\nSigned-off-by: Jonathan Tan \n---\n pkt-line.c | 47 ++++++++++++++++++---\n pkt-line.h | 14 +++++++\n upload-pack.c | 112 +++++++++++++++++++++++++++-----------------------\n 3 files changed, 115 insertions(+), 58 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex e70ea6d88f..9d3e402d41 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -129,12 +129,14 @@ static void set_packet_header(char *buf, const int size)\n \t#undef hex\n }\n \n-static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n+static void format_packet(struct strbuf *out, const char *prefix,\n+\t\t\t const char *fmt, va_list args)\n {\n \tsize_t orig_len, n;\n \n \torig_len = out->len;\n \tstrbuf_addstr(out, \"0000\");\n+\tstrbuf_addstr(out, prefix);\n \tstrbuf_vaddf(out, fmt, args);\n \tn = out->len - orig_len;\n \n@@ -145,13 +147,13 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n \tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n }\n \n-static int packet_write_fmt_1(int fd, int gently,\n+static int packet_write_fmt_1(int fd, int gently, const char *prefix,\n \t\t\t const char *fmt, va_list args)\n {\n \tstatic struct strbuf buf = STRBUF_INIT;\n \n \tstrbuf_reset(&buf);\n-\tformat_packet(&buf, fmt, args);\n+\tformat_packet(&buf, prefix, fmt, args);\n \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n \t\tif (!gently) {\n \t\t\tcheck_pipe(errno);\n@@ -168,7 +170,7 @@ void packet_write_fmt(int fd, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tpacket_write_fmt_1(fd, 0, fmt, args);\n+\tpacket_write_fmt_1(fd, 0, \"\", fmt, args);\n \tva_end(args);\n }\n \n@@ -178,7 +180,7 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tstatus = packet_write_fmt_1(fd, 1, fmt, args);\n+\tstatus = packet_write_fmt_1(fd, 1, \"\", fmt, args);\n \tva_end(args);\n \treturn status;\n }\n@@ -211,7 +213,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tformat_packet(buf, fmt, args);\n+\tformat_packet(buf, \"\", fmt, args);\n \tva_end(args);\n }\n \n@@ -486,3 +488,36 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)\n \treader->line_peeked = 1;\n \treturn reader->status;\n }\n+\n+void packet_writer_init(struct packet_writer *writer, int dest_fd)\n+{\n+\twriter->dest_fd = dest_fd;\n+}\n+\n+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n+{\n+\tva_list args;\n+\n+\tva_start(args, fmt);\n+\tpacket_write_fmt_1(writer->dest_fd, 0, \"\", fmt, args);\n+\tva_end(args);\n+}\n+\n+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)\n+{\n+\tva_list args;\n+\n+\tva_start(args, fmt);\n+\tpacket_write_fmt_1(writer->dest_fd, 0, \"ERR \", fmt, args);\n+\tva_end(args);\n+}\n+\n+void packet_writer_delim(struct packet_writer *writer)\n+{\n+\tpacket_delim(writer->dest_fd);\n+}\n+\n+void packet_writer_flush(struct packet_writer *writer)\n+{\n+\tpacket_flush(writer->dest_fd);\n+}\ndiff --git a/pkt-line.h b/pkt-line.h\nindex d7e1dbc047..023ad2951d 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -183,4 +183,18 @@ extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);\n #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)\n extern char packet_buffer[LARGE_PACKET_MAX];\n \n+struct packet_writer {\n+\tint dest_fd;\n+};\n+\n+void packet_writer_init(struct packet_writer *writer, int dest_fd);\n+\n+/* These functions die upon failure. */\n+__attribute__((format (printf, 2, 3)))\n+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...);\n+__attribute__((format (printf, 2, 3)))\n+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...);\n+void packet_writer_delim(struct packet_writer *writer);\n+void packet_writer_flush(struct packet_writer *writer);\n+\n #endif\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 08b547cf01..60a26bbbfd 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -613,13 +613,14 @@ static void check_non_tip(struct object_array *want_obj)\n \t}\n }\n \n-static void send_shallow(struct commit_list *result)\n+static void send_shallow(struct packet_writer *writer,\n+\t\t\t struct commit_list *result)\n {\n \twhile (result) {\n \t\tstruct object *object = &result->item->object;\n \t\tif (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {\n-\t\t\tpacket_write_fmt(1, \"shallow %s\",\n-\t\t\t\t\t oid_to_hex(&object->oid));\n+\t\t\tpacket_writer_write(writer, \"shallow %s\",\n+\t\t\t\t\t oid_to_hex(&object->oid));\n \t\t\tregister_shallow(the_repository, &object->oid);\n \t\t\tshallow_nr++;\n \t\t}\n@@ -627,7 +628,8 @@ static void send_shallow(struct commit_list *result)\n \t}\n }\n \n-static void send_unshallow(const struct object_array *shallows,\n+static void send_unshallow(struct packet_writer *writer,\n+\t\t\t const struct object_array *shallows,\n \t\t\t struct object_array *want_obj)\n {\n \tint i;\n@@ -636,8 +638,8 @@ static void send_unshallow(const struct object_array *shallows,\n \t\tstruct object *object = shallows->objects[i].item;\n \t\tif (object->flags & NOT_SHALLOW) {\n \t\t\tstruct commit_list *parents;\n-\t\t\tpacket_write_fmt(1, \"unshallow %s\",\n-\t\t\t\t\t oid_to_hex(&object->oid));\n+\t\t\tpacket_writer_write(writer, \"unshallow %s\",\n+\t\t\t\t\t oid_to_hex(&object->oid));\n \t\t\tobject->flags &= ~CLIENT_SHALLOW;\n \t\t\t/*\n \t\t\t * We want to _register_ \"object\" as shallow, but we\n@@ -662,7 +664,7 @@ static void send_unshallow(const struct object_array *shallows,\n \t}\n }\n \n-static void deepen(int depth, int deepen_relative,\n+static void deepen(struct packet_writer *writer, int depth, int deepen_relative,\n \t\t struct object_array *shallows, struct object_array *want_obj)\n {\n \tif (depth == INFINITE_DEPTH && !is_repository_shallow(the_repository)) {\n@@ -680,7 +682,7 @@ static void deepen(int depth, int deepen_relative,\n \t\tresult = get_shallow_commits(&reachable_shallows,\n \t\t\t\t\t depth + 1,\n \t\t\t\t\t SHALLOW, NOT_SHALLOW);\n-\t\tsend_shallow(result);\n+\t\tsend_shallow(writer, result);\n \t\tfree_commit_list(result);\n \t\tobject_array_clear(&reachable_shallows);\n \t} else {\n@@ -688,14 +690,15 @@ static void deepen(int depth, int deepen_relative,\n \n \t\tresult = get_shallow_commits(want_obj, depth,\n \t\t\t\t\t SHALLOW, NOT_SHALLOW);\n-\t\tsend_shallow(result);\n+\t\tsend_shallow(writer, result);\n \t\tfree_commit_list(result);\n \t}\n \n-\tsend_unshallow(shallows, want_obj);\n+\tsend_unshallow(writer, shallows, want_obj);\n }\n \n-static void deepen_by_rev_list(int ac, const char **av,\n+static void deepen_by_rev_list(struct packet_writer *writer, int ac,\n+\t\t\t const char **av,\n \t\t\t struct object_array *shallows,\n \t\t\t struct object_array *want_obj)\n {\n@@ -703,13 +706,14 @@ static void deepen_by_rev_list(int ac, const char **av,\n \n \tclose_commit_graph(the_repository);\n \tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n-\tsend_shallow(result);\n+\tsend_shallow(writer, result);\n \tfree_commit_list(result);\n-\tsend_unshallow(shallows, want_obj);\n+\tsend_unshallow(writer, shallows, want_obj);\n }\n \n /* Returns 1 if a shallow list is sent or 0 otherwise */\n-static int send_shallow_list(int depth, int deepen_rev_list,\n+static int send_shallow_list(struct packet_writer *writer,\n+\t\t\t int depth, int deepen_rev_list,\n \t\t\t timestamp_t deepen_since,\n \t\t\t struct string_list *deepen_not,\n \t\t\t struct object_array *shallows,\n@@ -720,7 +724,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,\n \tif (depth > 0 && deepen_rev_list)\n \t\tdie(\"git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together\");\n \tif (depth > 0) {\n-\t\tdeepen(depth, deepen_relative, shallows, want_obj);\n+\t\tdeepen(writer, depth, deepen_relative, shallows, want_obj);\n \t\tret = 1;\n \t} else if (deepen_rev_list) {\n \t\tstruct argv_array av = ARGV_ARRAY_INIT;\n@@ -741,7 +745,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,\n \t\t\tstruct object *o = want_obj->objects[i].item;\n \t\t\targv_array_push(&av, oid_to_hex(&o->oid));\n \t\t}\n-\t\tdeepen_by_rev_list(av.argc, av.argv, shallows, want_obj);\n+\t\tdeepen_by_rev_list(writer, av.argc, av.argv, shallows, want_obj);\n \t\targv_array_clear(&av);\n \t\tret = 1;\n \t} else {\n@@ -834,8 +838,10 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \tint has_non_tip = 0;\n \ttimestamp_t deepen_since = 0;\n \tint deepen_rev_list = 0;\n+\tstruct packet_writer writer;\n \n \tshallow_nr = 0;\n+\tpacket_writer_init(&writer, 1);\n \tfor (;;) {\n \t\tstruct object *o;\n \t\tconst char *features;\n@@ -892,9 +898,9 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \n \t\to = parse_object(the_repository, &oid_buf);\n \t\tif (!o) {\n-\t\t\tpacket_write_fmt(1,\n-\t\t\t\t\t \"ERR upload-pack: not our ref %s\",\n-\t\t\t\t\t oid_to_hex(&oid_buf));\n+\t\t\tpacket_writer_error(&writer,\n+\t\t\t\t\t \"upload-pack: not our ref %s\",\n+\t\t\t\t\t oid_to_hex(&oid_buf));\n \t\t\tdie(\"git upload-pack: not our ref %s\",\n \t\t\t oid_to_hex(&oid_buf));\n \t\t}\n@@ -923,7 +929,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \tif (depth == 0 && !deepen_rev_list && shallows.nr == 0)\n \t\treturn;\n \n-\tif (send_shallow_list(depth, deepen_rev_list, deepen_since,\n+\tif (send_shallow_list(&writer, depth, deepen_rev_list, deepen_since,\n \t\t\t &deepen_not, &shallows, want_obj))\n \t\tpacket_flush(1);\n \tobject_array_clear(&shallows);\n@@ -1102,6 +1108,8 @@ struct upload_pack_data {\n \tint deepen_rev_list;\n \tint deepen_relative;\n \n+\tstruct packet_writer writer;\n+\n \tunsigned stateless_rpc : 1;\n \n \tunsigned use_thin_pack : 1;\n@@ -1125,6 +1133,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tdata->haves = haves;\n \tdata->shallows = shallows;\n \tdata->deepen_not = deepen_not;\n+\tpacket_writer_init(&data->writer, 1);\n }\n \n static void upload_pack_data_clear(struct upload_pack_data *data)\n@@ -1136,7 +1145,8 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \tstring_list_clear(&data->deepen_not, 0);\n }\n \n-static int parse_want(const char *line, struct object_array *want_obj)\n+static int parse_want(struct packet_writer *writer, const char *line,\n+\t\t struct object_array *want_obj)\n {\n \tconst char *arg;\n \tif (skip_prefix(line, \"want \", &arg)) {\n@@ -1149,9 +1159,9 @@ static int parse_want(const char *line, struct object_array *want_obj)\n \n \t\to = parse_object(the_repository, &oid);\n \t\tif (!o) {\n-\t\t\tpacket_write_fmt(1,\n-\t\t\t\t\t \"ERR upload-pack: not our ref %s\",\n-\t\t\t\t\t oid_to_hex(&oid));\n+\t\t\tpacket_writer_error(writer,\n+\t\t\t\t\t \"upload-pack: not our ref %s\",\n+\t\t\t\t\t oid_to_hex(&oid));\n \t\t\tdie(\"git upload-pack: not our ref %s\",\n \t\t\t oid_to_hex(&oid));\n \t\t}\n@@ -1167,7 +1177,8 @@ static int parse_want(const char *line, struct object_array *want_obj)\n \treturn 0;\n }\n \n-static int parse_want_ref(const char *line, struct string_list *wanted_refs,\n+static int parse_want_ref(struct packet_writer *writer, const char *line,\n+\t\t\t struct string_list *wanted_refs,\n \t\t\t struct object_array *want_obj)\n {\n \tconst char *arg;\n@@ -1177,7 +1188,7 @@ static int parse_want_ref(const char *line, struct string_list *wanted_refs,\n \t\tstruct object *o;\n \n \t\tif (read_ref(arg, &oid)) {\n-\t\t\tpacket_write_fmt(1, \"ERR unknown ref %s\", arg);\n+\t\t\tpacket_writer_error(writer, \"unknown ref %s\", arg);\n \t\t\tdie(\"unknown ref %s\", arg);\n \t\t}\n \n@@ -1220,10 +1231,11 @@ static void process_args(struct packet_reader *request,\n \t\tconst char *p;\n \n \t\t/* process want */\n-\t\tif (parse_want(arg, want_obj))\n+\t\tif (parse_want(&data->writer, arg, want_obj))\n \t\t\tcontinue;\n \t\tif (allow_ref_in_want &&\n-\t\t parse_want_ref(arg, &data->wanted_refs, want_obj))\n+\t\t parse_want_ref(&data->writer, arg, &data->wanted_refs,\n+\t\t\t\t want_obj))\n \t\t\tcontinue;\n \t\t/* process have line */\n \t\tif (parse_have(arg, &data->haves))\n@@ -1317,26 +1329,26 @@ static int process_haves(struct oid_array *haves, struct oid_array *common,\n \treturn 0;\n }\n \n-static int send_acks(struct oid_array *acks, struct strbuf *response,\n+static int send_acks(struct packet_writer *writer, struct oid_array *acks,\n \t\t const struct object_array *have_obj,\n \t\t struct object_array *want_obj)\n {\n \tint i;\n \n-\tpacket_buf_write(response, \"acknowledgments\\n\");\n+\tpacket_writer_write(writer, \"acknowledgments\\n\");\n \n \t/* Send Acks */\n \tif (!acks->nr)\n-\t\tpacket_buf_write(response, \"NAK\\n\");\n+\t\tpacket_writer_write(writer, \"NAK\\n\");\n \n \tfor (i = 0; i < acks->nr; i++) {\n-\t\tpacket_buf_write(response, \"ACK %s\\n\",\n-\t\t\t\t oid_to_hex(&acks->oid[i]));\n+\t\tpacket_writer_write(writer, \"ACK %s\\n\",\n+\t\t\t\t oid_to_hex(&acks->oid[i]));\n \t}\n \n \tif (ok_to_give_up(have_obj, want_obj)) {\n \t\t/* Send Ready */\n-\t\tpacket_buf_write(response, \"ready\\n\");\n+\t\tpacket_writer_write(writer, \"ready\\n\");\n \t\treturn 1;\n \t}\n \n@@ -1348,25 +1360,20 @@ static int process_haves_and_send_acks(struct upload_pack_data *data,\n \t\t\t\t struct object_array *want_obj)\n {\n \tstruct oid_array common = OID_ARRAY_INIT;\n-\tstruct strbuf response = STRBUF_INIT;\n \tint ret = 0;\n \n \tprocess_haves(&data->haves, &common, have_obj);\n \tif (data->done) {\n \t\tret = 1;\n-\t} else if (send_acks(&common, &response, have_obj, want_obj)) {\n-\t\tpacket_buf_delim(&response);\n+\t} else if (send_acks(&data->writer, &common, have_obj, want_obj)) {\n+\t\tpacket_writer_delim(&data->writer);\n \t\tret = 1;\n \t} else {\n \t\t/* Add Flush */\n-\t\tpacket_buf_flush(&response);\n+\t\tpacket_writer_flush(&data->writer);\n \t\tret = 0;\n \t}\n \n-\t/* Send response */\n-\twrite_or_die(1, response.buf, response.len);\n-\tstrbuf_release(&response);\n-\n \toid_array_clear(&data->haves);\n \toid_array_clear(&common);\n \treturn ret;\n@@ -1379,15 +1386,15 @@ static void send_wanted_ref_info(struct upload_pack_data *data)\n \tif (!data->wanted_refs.nr)\n \t\treturn;\n \n-\tpacket_write_fmt(1, \"wanted-refs\\n\");\n+\tpacket_writer_write(&data->writer, \"wanted-refs\\n\");\n \n \tfor_each_string_list_item(item, &data->wanted_refs) {\n-\t\tpacket_write_fmt(1, \"%s %s\\n\",\n-\t\t\t\t oid_to_hex(item->util),\n-\t\t\t\t item->string);\n+\t\tpacket_writer_write(&data->writer, \"%s %s\\n\",\n+\t\t\t\t oid_to_hex(item->util),\n+\t\t\t\t item->string);\n \t}\n \n-\tpacket_delim(1);\n+\tpacket_writer_delim(&data->writer);\n }\n \n static void send_shallow_info(struct upload_pack_data *data,\n@@ -1398,14 +1405,15 @@ static void send_shallow_info(struct upload_pack_data *data,\n \t !is_repository_shallow(the_repository))\n \t\treturn;\n \n-\tpacket_write_fmt(1, \"shallow-info\\n\");\n+\tpacket_writer_write(&data->writer, \"shallow-info\\n\");\n \n-\tif (!send_shallow_list(data->depth, data->deepen_rev_list,\n+\tif (!send_shallow_list(&data->writer, data->depth,\n+\t\t\t data->deepen_rev_list,\n \t\t\t data->deepen_since, &data->deepen_not,\n \t\t\t &data->shallows, want_obj) &&\n \t is_repository_shallow(the_repository))\n-\t\tdeepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,\n-\t\t want_obj);\n+\t\tdeepen(&data->writer, INFINITE_DEPTH, data->deepen_relative,\n+\t\t &data->shallows, want_obj);\n \n \tpacket_delim(1);\n }\n@@ -1467,7 +1475,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \t\t\tsend_wanted_ref_info(&data);\n \t\t\tsend_shallow_info(&data, &want_obj);\n \n-\t\t\tpacket_write_fmt(1, \"packfile\\n\");\n+\t\t\tpacket_writer_write(&data.writer, \"packfile\\n\");\n \t\t\tcreate_pack_file(&have_obj, &want_obj);\n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366586","messageId":"4079a1f110086bd412fb4f56bc8b7f8a6a7a8f9f.1547244620.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547244620.git.jonathantanmy@google.com","subject":"[PATCH 2/4] sideband: reverse its dependency on pkt-line","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-11T22:18:15Z","receivedAt":"2019-01-11T22:18:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent patch will teach struct packet_reader a new field that, if\nset, instructs it to interpret read data as multiplexed. This will\ncreate a dependency from pkt-line to sideband.\n\nTo avoid a circular dependency, split recv_sideband() into 2 parts: the\nreading loop (left in recv_sideband()) and the processing of the\ncontents (in diagnose_sideband()), and move the former into pkt-line.\nThis reverses the direction of dependency: sideband no longer depends on\npkt-line, and pkt-line now depends on sideband.\n\nSigned-off-by: Jonathan Tan \n---\n pkt-line.c | 22 ++++++++\n pkt-line.h | 16 ++++++\n sideband.c | 156 +++++++++++++++++++++++++----------------------------\n sideband.h | 15 +++++-\n 4 files changed, 125 insertions(+), 84 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 9d3e402d41..ebdc6c2530 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -439,6 +439,28 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)\n \treturn sb_out->len - orig_len;\n }\n \n+int recv_sideband(const char *me, int in_stream, int out)\n+{\n+\tchar buf[LARGE_PACKET_MAX + 1];\n+\tint retval = 0;\n+\tint len;\n+\n+\twhile (1) {\n+\t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);\n+\t\tretval = diagnose_sideband(me, buf, len);\n+\t\tswitch (retval) {\n+\t\t\tcase SIDEBAND_PRIMARY:\n+\t\t\t\twrite_or_die(out, buf + 1, len - 1);\n+\t\t\t\tbreak;\n+\t\t\tcase SIDEBAND_PROGRESS:\n+\t\t\t\t/* already written by diagnose_sideband() */\n+\t\t\t\tbreak;\n+\t\t\tdefault: /* flush or error */\n+\t\t\t\treturn retval;\n+\t\t}\n+\t}\n+}\n+\n /* Packet Reader Functions */\n void packet_reader_init(struct packet_reader *reader, int fd,\n \t\t\tchar *src_buffer, size_t src_len,\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 023ad2951d..a8e92a4b63 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -3,6 +3,7 @@\n \n #include \"git-compat-util.h\"\n #include \"strbuf.h\"\n+#include \"sideband.h\"\n \n /*\n * Write a packetized stream, where each line is preceded by\n@@ -120,6 +121,21 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);\n */\n ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);\n \n+/*\n+ * Receive multiplexed output stream over git native protocol.\n+ * in_stream is the input stream from the remote, which carries data\n+ * in pkt_line format with band designator. Demultiplex it into out\n+ * and err and return error appropriately. Band #1 carries the\n+ * primary payload. Things coming over band #2 is not necessarily\n+ * error; they are usually informative message on the standard error\n+ * stream, aka \"verbose\"). A message over band #3 is a signal that\n+ * the remote died unexpectedly. A flush() concludes the stream.\n+ *\n+ * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR\n+ * or SIDEBAND_REMOTE_ERROR if an error occurred.\n+ */\n+int recv_sideband(const char *me, int in_stream, int out);\n+\n struct packet_reader {\n \t/* source file descriptor */\n \tint fd;\ndiff --git a/sideband.c b/sideband.c\nindex 368647acf8..842a92e975 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -1,7 +1,6 @@\n #include \"cache.h\"\n #include \"color.h\"\n #include \"config.h\"\n-#include \"pkt-line.h\"\n #include \"sideband.h\"\n #include \"help.h\"\n \n@@ -109,103 +108,94 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n }\n \n \n-/*\n- * Receive multiplexed output stream over git native protocol.\n- * in_stream is the input stream from the remote, which carries data\n- * in pkt_line format with band designator. Demultiplex it into out\n- * and err and return error appropriately. Band #1 carries the\n- * primary payload. Things coming over band #2 is not necessarily\n- * error; they are usually informative message on the standard error\n- * stream, aka \"verbose\"). A message over band #3 is a signal that\n- * the remote died unexpectedly. A flush() concludes the stream.\n- */\n-\n #define DISPLAY_PREFIX \"remote: \"\n \n #define ANSI_SUFFIX \"\\033[K\"\n #define DUMB_SUFFIX \" \"\n \n-int recv_sideband(const char *me, int in_stream, int out)\n+int diagnose_sideband(const char *me, char *buf, int len)\n {\n-\tconst char *suffix;\n-\tchar buf[LARGE_PACKET_MAX + 1];\n+\tstatic const char *suffix;\n \tstruct strbuf outbuf = STRBUF_INIT;\n \tint retval = 0;\n+\tconst char *b, *brk;\n+\tint band;\n+\n+\tif (!suffix) {\n+\t\tif (isatty(2) && !is_terminal_dumb())\n+\t\t\tsuffix = ANSI_SUFFIX;\n+\t\telse\n+\t\t\tsuffix = DUMB_SUFFIX;\n+\t}\n \n-\tif (isatty(2) && !is_terminal_dumb())\n-\t\tsuffix = ANSI_SUFFIX;\n-\telse\n-\t\tsuffix = DUMB_SUFFIX;\n+\tif (len == 0) {\n+\t\tretval = SIDEBAND_FLUSH;\n+\t\tgoto cleanup;\n+\t}\n+\tif (len < 1) {\n+\t\tstrbuf_addf(&outbuf,\n+\t\t\t \"%s%s: protocol error: no band designator\",\n+\t\t\t outbuf.len ? \"\\n\" : \"\", me);\n+\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n+\t\tgoto cleanup;\n+\t}\n+\tband = buf[0] & 0xff;\n+\tbuf[len] = '\\0';\n+\tlen--;\n+\tswitch (band) {\n+\tcase 3:\n+\t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ? \"\\n\" : \"\",\n+\t\t\t DISPLAY_PREFIX);\n+\t\tmaybe_colorize_sideband(&outbuf, buf + 1, len);\n+\n+\t\tretval = SIDEBAND_REMOTE_ERROR;\n+\t\tbreak;\n+\tcase 2:\n+\t\tb = buf + 1;\n \n-\twhile (!retval) {\n-\t\tconst char *b, *brk;\n-\t\tint band, len;\n-\t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);\n-\t\tif (len == 0)\n-\t\t\tbreak;\n-\t\tif (len < 1) {\n-\t\t\tstrbuf_addf(&outbuf,\n-\t\t\t\t \"%s%s: protocol error: no band designator\",\n-\t\t\t\t outbuf.len ? \"\\n\" : \"\", me);\n-\t\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n-\t\t\tbreak;\n-\t\t}\n-\t\tband = buf[0] & 0xff;\n-\t\tbuf[len] = '\\0';\n-\t\tlen--;\n-\t\tswitch (band) {\n-\t\tcase 3:\n-\t\t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ? \"\\n\" : \"\",\n-\t\t\t\t DISPLAY_PREFIX);\n-\t\t\tmaybe_colorize_sideband(&outbuf, buf + 1, len);\n-\n-\t\t\tretval = SIDEBAND_REMOTE_ERROR;\n-\t\t\tbreak;\n-\t\tcase 2:\n-\t\t\tb = buf + 1;\n-\n-\t\t\t/*\n-\t\t\t * Append a suffix to each nonempty line to clear the\n-\t\t\t * end of the screen line.\n-\t\t\t *\n-\t\t\t * The output is accumulated in a buffer and\n-\t\t\t * each line is printed to stderr using\n-\t\t\t * write(2) to ensure inter-process atomicity.\n-\t\t\t */\n-\t\t\twhile ((brk = strpbrk(b, \"\\n\\r\"))) {\n-\t\t\t\tint linelen = brk - b;\n-\n-\t\t\t\tif (!outbuf.len)\n-\t\t\t\t\tstrbuf_addstr(&outbuf, DISPLAY_PREFIX);\n-\t\t\t\tif (linelen > 0) {\n-\t\t\t\t\tmaybe_colorize_sideband(&outbuf, b, linelen);\n-\t\t\t\t\tstrbuf_addstr(&outbuf, suffix);\n-\t\t\t\t}\n-\n-\t\t\t\tstrbuf_addch(&outbuf, *brk);\n-\t\t\t\txwrite(2, outbuf.buf, outbuf.len);\n-\t\t\t\tstrbuf_reset(&outbuf);\n-\n-\t\t\t\tb = brk + 1;\n+\t\t/*\n+\t\t * Append a suffix to each nonempty line to clear the\n+\t\t * end of the screen line.\n+\t\t *\n+\t\t * The output is accumulated in a buffer and\n+\t\t * each line is printed to stderr using\n+\t\t * write(2) to ensure inter-process atomicity.\n+\t\t */\n+\t\twhile ((brk = strpbrk(b, \"\\n\\r\"))) {\n+\t\t\tint linelen = brk - b;\n+\n+\t\t\tif (!outbuf.len)\n+\t\t\t\tstrbuf_addstr(&outbuf, DISPLAY_PREFIX);\n+\t\t\tif (linelen > 0) {\n+\t\t\t\tmaybe_colorize_sideband(&outbuf, b, linelen);\n+\t\t\t\tstrbuf_addstr(&outbuf, suffix);\n \t\t\t}\n \n-\t\t\tif (*b) {\n-\t\t\t\tstrbuf_addstr(&outbuf, outbuf.len ?\n-\t\t\t\t\t \"\" : DISPLAY_PREFIX);\n-\t\t\t\tmaybe_colorize_sideband(&outbuf, b, strlen(b));\n-\t\t\t}\n-\t\t\tbreak;\n-\t\tcase 1:\n-\t\t\twrite_or_die(out, buf + 1, len);\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tstrbuf_addf(&outbuf, \"%s%s: protocol error: bad band #%d\",\n-\t\t\t\t outbuf.len ? \"\\n\" : \"\", me, band);\n-\t\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n-\t\t\tbreak;\n+\t\t\tstrbuf_addch(&outbuf, *brk);\n+\t\t\txwrite(2, outbuf.buf, outbuf.len);\n+\t\t\tstrbuf_reset(&outbuf);\n+\n+\t\t\tb = brk + 1;\n+\t\t}\n+\n+\t\tif (*b) {\n+\t\t\tstrbuf_addstr(&outbuf, outbuf.len ?\n+\t\t\t\t \"\" : DISPLAY_PREFIX);\n+\t\t\tmaybe_colorize_sideband(&outbuf, b, strlen(b));\n \t\t}\n+\t\tretval = SIDEBAND_PROGRESS;\n+\t\tbreak;\n+\tcase 1:\n+\t\tretval = SIDEBAND_PRIMARY;\n+\t\tbreak;\n+\tdefault:\n+\t\tstrbuf_addf(&outbuf, \"%s%s: protocol error: bad band #%d\",\n+\t\t\t outbuf.len ? \"\\n\" : \"\", me, band);\n+\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n+\t\tbreak;\n \t}\n \n+cleanup:\n \tif (outbuf.len) {\n \t\tstrbuf_addch(&outbuf, '\\n');\n \t\txwrite(2, outbuf.buf, outbuf.len);\ndiff --git a/sideband.h b/sideband.h\nindex 7a8146f161..a56cd86287 100644\n--- a/sideband.h\n+++ b/sideband.h\n@@ -3,8 +3,21 @@\n \n #define SIDEBAND_PROTOCOL_ERROR -2\n #define SIDEBAND_REMOTE_ERROR -1\n+#define SIDEBAND_FLUSH 0\n+#define SIDEBAND_PRIMARY 1\n+#define SIDEBAND_PROGRESS 2\n+\n+/*\n+ * buf and len should be the result of reading a line from a remote sending\n+ * multiplexed data.\n+ *\n+ * Determines the nature of the result and returns it. If\n+ * SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS, also\n+ * prints a message (or the formatted contents of the notice in the case of\n+ * SIDEBAND_PROGRESS) to stderr.\n+ */\n+int diagnose_sideband(const char *me, char *buf, int len);\n \n-int recv_sideband(const char *me, int in_stream, int out);\n void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);\n \n #endif\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366587","messageId":"673b928b4158b0aecca843302483b48da5ed8d35.1547244620.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547244620.git.jonathantanmy@google.com","subject":"[PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-11T22:18:16Z","receivedAt":"2019-01-11T22:18:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, a response to a fetch request has sideband support only while\nthe packfile is being sent, meaning that the server cannot send notices\nuntil the start of the packfile.\n\nExtend sideband support in protocol v2 fetch responses to the whole\nresponse. upload-pack will advertise it if the\nuploadpack.allowsidebandall configuration variable is set, and\nfetch-pack will automatically request it if advertised.\n\nIf the sideband is to be used throughout the whole response, upload-pack\nwill use it to send errors instead of prefixing a PKT-LINE payload with\n\"ERR \".\n\nThis will be tested in a subsequent patch.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/protocol-v2.txt | 10 +++++\n fetch-pack.c | 12 +++++-\n pkt-line.c | 51 +++++++++++++++++++------\n pkt-line.h | 4 ++\n sideband.c | 7 +++-\n sideband.h | 2 +-\n upload-pack.c | 16 ++++++++\n 7 files changed, 87 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 09e4e0273f..1b0633f59f 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -307,6 +307,16 @@ the 'wanted-refs' section in the server's response as explained below.\n \tparticular ref, where is the full name of a ref on the\n \tserver.\n \n+If the 'sideband-all' feature is advertised, the following argument can be\n+included in the client's request:\n+\n+ sideband-all\n+\tInstruct the server to send the whole response multiplexed, not just\n+\tthe packfile section. All non-flush and non-delim PKT-LINE in the\n+\tresponse (not only in the packfile section) will then start with a byte\n+\tindicating its sideband (1, 2, or 3), and the server may send \"0005\\1\"\n+\t(a PKT-LINE of sideband 1 with no payload) as a keepalive packet.\n+\n The response of `fetch` is broken into a number of sections separated by\n delimiter packets (0001), with each section beginning with its section\n header.\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 3f9626dbf7..b89199891d 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1125,7 +1125,8 @@ static int add_haves(struct fetch_negotiator *negotiator,\n static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\t\t const struct fetch_pack_args *args,\n \t\t\t const struct ref *wants, struct oidset *common,\n-\t\t\t int *haves_to_send, int *in_vain)\n+\t\t\t int *haves_to_send, int *in_vain,\n+\t\t\t int sideband_all)\n {\n \tint ret = 0;\n \tstruct strbuf req_buf = STRBUF_INIT;\n@@ -1151,6 +1152,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tpacket_buf_write(&req_buf, \"include-tag\");\n \tif (prefer_ofs_delta)\n \t\tpacket_buf_write(&req_buf, \"ofs-delta\");\n+\tif (sideband_all)\n+\t\tpacket_buf_write(&req_buf, \"sideband-all\");\n \n \t/* Add shallow-info and deepen request */\n \tif (server_supports_feature(\"fetch\", \"shallow\", 0))\n@@ -1359,6 +1362,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n+\tif (server_supports_feature(\"fetch\", \"sideband-all\", 0)) {\n+\t\treader.use_sideband = 1;\n+\t\treader.me = \"fetch-pack\";\n+\t}\n \n \twhile (state != FETCH_DONE) {\n \t\tswitch (state) {\n@@ -1392,7 +1399,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\tcase FETCH_SEND_REQUEST:\n \t\t\tif (send_fetch_request(&negotiator, fd[1], args, ref,\n \t\t\t\t\t &common,\n-\t\t\t\t\t &haves_to_send, &in_vain))\n+\t\t\t\t\t &haves_to_send, &in_vain,\n+\t\t\t\t\t reader.use_sideband))\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\telse\n \t\t\t\tstate = FETCH_PROCESS_ACKS;\ndiff --git a/pkt-line.c b/pkt-line.c\nindex ebdc6c2530..71b17e6b93 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -447,7 +447,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n \n \twhile (1) {\n \t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);\n-\t\tretval = diagnose_sideband(me, buf, len);\n+\t\tretval = diagnose_sideband(me, buf, len, 0);\n \t\tswitch (retval) {\n \t\t\tcase SIDEBAND_PRIMARY:\n \t\t\t\twrite_or_die(out, buf + 1, len - 1);\n@@ -483,16 +483,42 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\treturn reader->status;\n \t}\n \n-\treader->status = packet_read_with_status(reader->fd,\n-\t\t\t\t\t\t &reader->src_buffer,\n-\t\t\t\t\t\t &reader->src_len,\n-\t\t\t\t\t\t reader->buffer,\n-\t\t\t\t\t\t reader->buffer_size,\n-\t\t\t\t\t\t &reader->pktlen,\n-\t\t\t\t\t\t reader->options);\n+\twhile (1) {\n+\t\tint retval;\n+\t\treader->status = packet_read_with_status(reader->fd,\n+\t\t\t\t\t\t\t &reader->src_buffer,\n+\t\t\t\t\t\t\t &reader->src_len,\n+\t\t\t\t\t\t\t reader->buffer,\n+\t\t\t\t\t\t\t reader->buffer_size,\n+\t\t\t\t\t\t\t &reader->pktlen,\n+\t\t\t\t\t\t\t reader->options);\n+\t\tif (!reader->use_sideband)\n+\t\t\tbreak;\n+\t\tretval = diagnose_sideband(reader->me, reader->buffer,\n+\t\t\t\t\t reader->pktlen, 1);\n+\t\tswitch (retval) {\n+\t\t\tcase SIDEBAND_PROTOCOL_ERROR:\n+\t\t\tcase SIDEBAND_REMOTE_ERROR:\n+\t\t\t\tBUG(\"should have died in diagnose_sideband\");\n+\t\t\tcase SIDEBAND_FLUSH:\n+\t\t\t\tgoto nonprogress;\n+\t\t\tcase SIDEBAND_PRIMARY:\n+\t\t\t\tif (reader->pktlen != 1)\n+\t\t\t\t\tgoto nonprogress;\n+\t\t\t\t/*\n+\t\t\t\t * Since pktlen is 1, this is a keepalive\n+\t\t\t\t * packet. Wait for the next one.\n+\t\t\t\t */\n+\t\t\t\tbreak;\n+\t\t\tdefault: /* SIDEBAND_PROGRESS */\n+\t\t\t\t;\n+\t\t}\n+\t}\n \n+nonprogress:\n \tif (reader->status == PACKET_READ_NORMAL)\n-\t\treader->line = reader->buffer;\n+\t\treader->line = reader->use_sideband ?\n+\t\t\treader->buffer + 1 : reader->buffer;\n \telse\n \t\treader->line = NULL;\n \n@@ -514,6 +540,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)\n void packet_writer_init(struct packet_writer *writer, int dest_fd)\n {\n \twriter->dest_fd = dest_fd;\n+\twriter->use_sideband = 0;\n }\n \n void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n@@ -521,7 +548,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tpacket_write_fmt_1(writer->dest_fd, 0, \"\", fmt, args);\n+\tpacket_write_fmt_1(writer->dest_fd, 0,\n+\t\t\t writer->use_sideband ? \"\\1\" : \"\", fmt, args);\n \tva_end(args);\n }\n \n@@ -530,7 +558,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tpacket_write_fmt_1(writer->dest_fd, 0, \"ERR \", fmt, args);\n+\tpacket_write_fmt_1(writer->dest_fd, 0,\n+\t\t\t writer->use_sideband ? \"\\3\" : \"ERR \", fmt, args);\n \tva_end(args);\n }\n \ndiff --git a/pkt-line.h b/pkt-line.h\nindex a8e92a4b63..ad9a4a2cd7 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -162,6 +162,9 @@ struct packet_reader {\n \n \t/* indicates if a line has been peeked */\n \tint line_peeked;\n+\n+\tunsigned use_sideband : 1;\n+\tconst char *me;\n };\n \n /*\n@@ -201,6 +204,7 @@ extern char packet_buffer[LARGE_PACKET_MAX];\n \n struct packet_writer {\n \tint dest_fd;\n+\tunsigned use_sideband : 1;\n };\n \n void packet_writer_init(struct packet_writer *writer, int dest_fd);\ndiff --git a/sideband.c b/sideband.c\nindex 842a92e975..cbeab380ae 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -113,7 +113,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n #define ANSI_SUFFIX \"\\033[K\"\n #define DUMB_SUFFIX \" \"\n \n-int diagnose_sideband(const char *me, char *buf, int len)\n+int diagnose_sideband(const char *me, char *buf, int len, int die_on_error)\n {\n \tstatic const char *suffix;\n \tstruct strbuf outbuf = STRBUF_INIT;\n@@ -144,6 +144,9 @@ int diagnose_sideband(const char *me, char *buf, int len)\n \tlen--;\n \tswitch (band) {\n \tcase 3:\n+\t\tif (die_on_error)\n+\t\t\tdie(\"remote error: %s\", buf + 1);\n+\n \t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ? \"\\n\" : \"\",\n \t\t\t DISPLAY_PREFIX);\n \t\tmaybe_colorize_sideband(&outbuf, buf + 1, len);\n@@ -196,6 +199,8 @@ int diagnose_sideband(const char *me, char *buf, int len)\n \t}\n \n cleanup:\n+\tif (die_on_error && retval == SIDEBAND_PROTOCOL_ERROR)\n+\t\tdie(\"%s\", outbuf.buf);\n \tif (outbuf.len) {\n \t\tstrbuf_addch(&outbuf, '\\n');\n \t\txwrite(2, outbuf.buf, outbuf.len);\ndiff --git a/sideband.h b/sideband.h\nindex a56cd86287..3786670694 100644\n--- a/sideband.h\n+++ b/sideband.h\n@@ -16,7 +16,7 @@\n * prints a message (or the formatted contents of the notice in the case of\n * SIDEBAND_PROGRESS) to stderr.\n */\n-int diagnose_sideband(const char *me, char *buf, int len);\n+int diagnose_sideband(const char *me, char *buf, int len, int die_on_error);\n \n void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 60a26bbbfd..765b7695d2 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -71,6 +71,8 @@ static int allow_filter;\n static int allow_ref_in_want;\n static struct list_objects_filter_options filter_options;\n \n+static int allow_sideband_all;\n+\n static void reset_timeout(void)\n {\n \talarm(timeout);\n@@ -1046,6 +1048,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \t\tallow_filter = git_config_bool(var, value);\n \t} else if (!strcmp(\"uploadpack.allowrefinwant\", var)) {\n \t\tallow_ref_in_want = git_config_bool(var, value);\n+\t} else if (!strcmp(\"uploadpack.allowsidebandall\", var)) {\n+\t\tallow_sideband_all = git_config_bool(var, value);\n \t}\n \n \tif (current_config_scope() != CONFIG_SCOPE_REPO) {\n@@ -1284,6 +1288,11 @@ static void process_args(struct packet_reader *request,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (allow_sideband_all && !strcmp(arg, \"sideband-all\")) {\n+\t\t\tdata->writer.use_sideband = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\t/* ignore unknown lines maybe? */\n \t\tdie(\"unexpected line: '%s'\", arg);\n \t}\n@@ -1496,6 +1505,7 @@ int upload_pack_advertise(struct repository *r,\n \tif (value) {\n \t\tint allow_filter_value;\n \t\tint allow_ref_in_want;\n+\t\tint allow_sideband_all_value;\n \n \t\tstrbuf_addstr(value, \"shallow\");\n \n@@ -1510,6 +1520,12 @@ int upload_pack_advertise(struct repository *r,\n \t\t\t\t\t &allow_ref_in_want) &&\n \t\t allow_ref_in_want)\n \t\t\tstrbuf_addstr(value, \" ref-in-want\");\n+\n+\t\tif (!repo_config_get_bool(the_repository,\n+\t\t\t\t\t \"uploadpack.allowsidebandall\",\n+\t\t\t\t\t &allow_sideband_all_value) &&\n+\t\t allow_sideband_all_value)\n+\t\t\tstrbuf_addstr(value, \" sideband-all\");\n \t}\n \n \treturn 1;\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366588","messageId":"e91db1276b5e838faa7060ba9f3fe23acbb5e3a7.1547244620.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547244620.git.jonathantanmy@google.com","subject":"[PATCH 4/4] tests: define GIT_TEST_SIDEBAND_ALL","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-11T22:18:17Z","receivedAt":"2019-01-11T22:18:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Define a GIT_TEST_SIDEBAND_ALL environment variable meant to be used\nfrom tests. When set to true, this overrides uploadpack.allowsidebandall\nto true, allowing the entire test suite to be run as if this\nconfiguration is in place for all repositories.\n\nAs of this patch, all tests pass whether GIT_TEST_SIDEBAND_ALL is unset\nor set to 1.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 3 ++-\n t/README | 5 +++++\n t/lib-httpd/apache.conf | 1 +\n t/t5537-fetch-shallow.sh | 3 ++-\n t/t5701-git-serve.sh | 2 +-\n t/t5702-protocol-v2.sh | 4 ++--\n upload-pack.c | 13 ++++++++-----\n 7 files changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b89199891d..4618568fee 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1362,7 +1362,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n-\tif (server_supports_feature(\"fetch\", \"sideband-all\", 0)) {\n+\tif (git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 1) &&\n+\t server_supports_feature(\"fetch\", \"sideband-all\", 0)) {\n \t\treader.use_sideband = 1;\n \t\treader.me = \"fetch-pack\";\n \t}\ndiff --git a/t/README b/t/README\nindex 28711cc508..b275c883b8 100644\n--- a/t/README\n+++ b/t/README\n@@ -358,6 +358,11 @@ GIT_TEST_MULTI_PACK_INDEX=, when true, forces the multi-pack-\n index to be written after every 'git repack' command, and overrides the\n 'core.multiPackIndex' setting to true.\n \n+GIT_TEST_SIDEBAND_ALL=, when true, overrides the\n+'uploadpack.allowSidebandAll' setting to true, and when false, forces\n+fetch-pack to not request sideband-all (even if the server advertises\n+sideband-all).\n+\n Naming Tests\n ------------\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 581c010d8f..9a6d368247 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -78,6 +78,7 @@ PassEnv GNUPGHOME\n PassEnv ASAN_OPTIONS\n PassEnv GIT_TRACE\n PassEnv GIT_CONFIG_NOSYSTEM\n+PassEnv GIT_TEST_SIDEBAND_ALL\n \n SetEnvIf Git-Protocol \".*\" GIT_PROTOCOL=$0\n \ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 6faf17e17a..6caf628efa 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -243,7 +243,8 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f\n \t \"$(git -C \"$REPO\" rev-parse HEAD)\" \\\n \t \"$(git -C \"$REPO\" rev-parse HEAD^)\" \\\n \t >\"$HTTPD_ROOT_PATH/one-time-sed\" &&\n-\ttest_must_fail git -C client fetch --depth=1 \"$HTTPD_URL/one_time_sed/repo\" \\\n+\ttest_must_fail env GIT_TEST_SIDEBAND_ALL=0 git -C client \\\n+\t\tfetch --depth=1 \"$HTTPD_URL/one_time_sed/repo\" \\\n \t\tmaster:a_branch &&\n \n \t# Ensure that the one-time-sed script was used.\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex ae79c6bbc0..fe45bf828d 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -14,7 +14,7 @@ test_expect_success 'test capability advertisement' '\n \t0000\n \tEOF\n \n-\tgit serve --advertise-capabilities >out &&\n+\tGIT_TEST_SIDEBAND_ALL=0 git serve --advertise-capabilities >out &&\n \ttest-tool pkt-line unpack actual &&\n \ttest_cmp expect actual\n '\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 0f2b09ebb8..b491c62e3e 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -583,8 +583,8 @@ test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n \ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" git -C http_child \\\n \t\t-c protocol.version=2 \\\n \t\tfetch \"$HTTPD_URL/one_time_sed/http_parent\" 2> err &&\n-\tgrep \"fetch< acknowledgments\" log &&\n-\t! grep \"fetch< ready\" log &&\n+\tgrep \"fetch< .*acknowledgments\" log &&\n+\t! grep \"fetch< .*ready\" log &&\n \ttest_i18ngrep \"expected no other sections to be sent after no .ready.\" err\n '\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 765b7695d2..0c1feccaab 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1288,7 +1288,9 @@ static void process_args(struct packet_reader *request,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (allow_sideband_all && !strcmp(arg, \"sideband-all\")) {\n+\t\tif ((git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 0) ||\n+\t\t allow_sideband_all) &&\n+\t\t !strcmp(arg, \"sideband-all\")) {\n \t\t\tdata->writer.use_sideband = 1;\n \t\t\tcontinue;\n \t\t}\n@@ -1521,10 +1523,11 @@ int upload_pack_advertise(struct repository *r,\n \t\t allow_ref_in_want)\n \t\t\tstrbuf_addstr(value, \" ref-in-want\");\n \n-\t\tif (!repo_config_get_bool(the_repository,\n-\t\t\t\t\t \"uploadpack.allowsidebandall\",\n-\t\t\t\t\t &allow_sideband_all_value) &&\n-\t\t allow_sideband_all_value)\n+\t\tif (git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 0) ||\n+\t\t (!repo_config_get_bool(the_repository,\n+\t\t\t\t\t \"uploadpack.allowsidebandall\",\n+\t\t\t\t\t &allow_sideband_all_value) &&\n+\t\t allow_sideband_all_value))\n \t\t\tstrbuf_addstr(value, \" sideband-all\");\n \t}\n \n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366589","messageId":"7e5d8a6ba3ae4211e18973f1549986c2e7640c82.1547244620.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547244620.git.jonathantanmy@google.com","subject":"[WIP 5/4] Documentation: order protocol v2 sections","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-11T22:18:18Z","receivedAt":"2019-01-11T22:18:50Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The current C Git implementation expects Git servers to follow a\nspecific order of sections when transmitting protocol v2 responses, but\nthis is not explicit in the documentation. Make the order explicit.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/protocol-v2.txt | 18 ++++++++----------\n 1 file changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 1b0633f59f..c76f2311c2 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -319,11 +319,11 @@ included in the client's request:\n \n The response of `fetch` is broken into a number of sections separated by\n delimiter packets (0001), with each section beginning with its section\n-header.\n+header. Most sections are sent only when the packfile is sent.\n \n- output = *section\n- section = (acknowledgments | shallow-info | wanted-refs | packfile)\n-\t (flush-pkt | delim-pkt)\n+ output = acknowledgements flush-pkt |\n+\t [acknowledgments delim-pkt] [shallow-info delim-pkt]\n+\t [wanted-refs delim-pkt] packfile flush-pkt\n \n acknowledgments = PKT-LINE(\"acknowledgments\" LF)\n \t\t (nak | *ack)\n@@ -345,9 +345,10 @@ header.\n \t *PKT-LINE(%x01-03 *%x00-ff)\n \n acknowledgments section\n-\t* If the client determines that it is finished with negotiations\n-\t by sending a \"done\" line, the acknowledgments sections MUST be\n-\t omitted from the server's response.\n+\t* If the client determines that it is finished with negotiations by\n+\t sending a \"done\" line (thus requiring the server to send a packfile),\n+\t the acknowledgments sections MUST be omitted from the server's\n+\t response.\n \n \t* Always begins with the section header \"acknowledgments\"\n \n@@ -398,9 +399,6 @@ header.\n \t which the client has not indicated was shallow as a part of\n \t its request.\n \n-\t* This section is only included if a packfile section is also\n-\t included in the response.\n-\n wanted-refs section\n \t* This section is only included if the client has requested a\n \t ref using a 'want-ref' line and if a packfile section is also\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366590","messageId":"e621cd3fb84a8d6865b5d8fe7f14e253a62af936.1547244620.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547244620.git.jonathantanmy@google.com","subject":"[WIP 6/4] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-11T22:18:19Z","receivedAt":"2019-01-11T22:18:53Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n Documentation/technical/packfile-uri.txt | 83 ++++++++++++++++++++++++\n Documentation/technical/protocol-v2.txt | 6 +-\n 2 files changed, 88 insertions(+), 1 deletion(-)\n create mode 100644 Documentation/technical/packfile-uri.txt\n\ndiff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt\nnew file mode 100644\nindex 0000000000..6535801486\n--- /dev/null\n+++ b/Documentation/technical/packfile-uri.txt\n@@ -0,0 +1,83 @@\n+Packfile URIs\n+=============\n+\n+This feature allows servers to serve part of their packfile response as URIs.\n+This allows server designs that improve scalability in bandwidth and CPU usage\n+(for example, by serving some data through a CDN), and (in the future) provides\n+some measure of resumability to clients.\n+\n+This feature is available only in protocol version 2.\n+\n+Protocol\n+--------\n+\n+The server advertises `packfile-uris`.\n+\n+If the client replies with the following arguments:\n+\n+ * packfile-uris\n+ * thin-pack\n+ * ofs-delta\n+\n+when the server sends the packfile, it MAY send a `packfile-uris` section\n+directly before the `packfile` section (right after `wanted-refs` if it is\n+sent) containing HTTP(S) URIs. See protocol-v2.txt for the documentation of\n+this section.\n+\n+Clients then should understand that the returned packfile could be incomplete,\n+and that it needs to download all the given URIs before the fetch or clone is\n+complete. Each URI should point to a Git packfile (which may be a thin pack and\n+which may contain offset deltas).\n+\n+Server design\n+-------------\n+\n+The server can be trivially made compatible with the proposed protocol by\n+having it advertise `packfile-uris`, tolerating the client sending\n+`packfile-uris`, and never sending any `packfile-uris` section. But we should\n+include some sort of non-trivial implementation in the Minimum Viable Product,\n+at least so that we can test the client.\n+\n+This is the implementation: a feature, marked experimental, that allows the\n+server to be configured by one or more `uploadpack.blobPackfileUri=\n+` entries. Whenever the list of objects to be sent is assembled, a blob\n+with the given sha1 can be replaced by the given URI. This allows, for example,\n+servers to delegate serving of large blobs to CDNs.\n+\n+Client design\n+-------------\n+\n+While fetching, the client needs to remember the list of URIs and cannot\n+declare that the fetch is complete until all URIs have been downloaded as\n+packfiles.\n+\n+The division of work (initial fetch + additional URIs) introduces convenient\n+points for resumption of an interrupted clone - such resumption can be done\n+after the Minimum Viable Product (see \"Future work\").\n+\n+The client can inhibit this feature (i.e. refrain from sending the\n+`packfile-urls` parameter) by passing --no-packfile-urls to `git fetch`.\n+\n+Future work\n+-----------\n+\n+The protocol design allows some evolution of the server and client without any\n+need for protocol changes, so only a small-scoped design is included here to\n+form the MVP. For example, the following can be done:\n+\n+ * On the server, a long-running process that takes in entire requests and\n+ outputs a list of URIs and the corresponding inclusion and exclusion sets of\n+ objects. This allows, e.g., signed URIs to be used and packfiles for common\n+ requests to be cached.\n+ * On the client, resumption of clone. If a clone is interrupted, information\n+ could be recorded in the repository's config and a \"clone-resume\" command\n+ can resume the clone in progress. (Resumption of subsequent fetches is more\n+ difficult because that must deal with the user wanting to use the repository\n+ even after the fetch was interrupted.)\n+\n+There are some possible features that will require a change in protocol:\n+\n+ * Additional HTTP headers (e.g. authentication)\n+ * Byte range support\n+ * Different file formats referenced by URIs (e.g. raw object)\n+\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex c76f2311c2..de5c63bc9b 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -323,7 +323,8 @@ header. Most sections are sent only when the packfile is sent.\n \n output = acknowledgements flush-pkt |\n \t [acknowledgments delim-pkt] [shallow-info delim-pkt]\n-\t [wanted-refs delim-pkt] packfile flush-pkt\n+\t [wanted-refs delim-pkt] [packfile-uris delim-pkt]\n+\t packfile flush-pkt\n \n acknowledgments = PKT-LINE(\"acknowledgments\" LF)\n \t\t (nak | *ack)\n@@ -341,6 +342,9 @@ header. Most sections are sent only when the packfile is sent.\n \t\t *PKT-LINE(wanted-ref LF)\n wanted-ref = obj-id SP refname\n \n+ packfile-uris = PKT-LINE(\"packfile-uris\" LF) *packfile-uri\n+ packfile-uri = PKT-LINE(\"uri\" SP *%x20-ff LF)\n+\n packfile = PKT-LINE(\"packfile\" LF)\n \t *PKT-LINE(%x01-03 *%x00-ff)\n \n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366591","messageId":"e87b45637a9773f0aaf1a04e97c0939e45a0cf80.1547244620.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547244620.git.jonathantanmy@google.com","subject":"[WIP 7/4] upload-pack: refactor reading of pack-objects out","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-11T22:18:20Z","receivedAt":"2019-01-11T22:18:56Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Subsequent patches will change how the output of pack-objects is\nprocessed, so extract that processing into its own function.\n\nCurrently, at most 1 character can be buffered (in the \"buffered\" local\nvariable). One of those patches will require a larger buffer, so replace\nthat \"buffered\" local variable with a buffer array.\n\nSigned-off-by: Jonathan Tan \n---\n upload-pack.c | 80 +++++++++++++++++++++++++++++----------------------\n 1 file changed, 46 insertions(+), 34 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 0c1feccaab..c87b752550 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -103,14 +103,51 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n \treturn 0;\n }\n \n+struct output_state {\n+\tchar buffer[8193];\n+\tint used;\n+};\n+\n+static int read_pack_objects_stdout(int outfd, struct output_state *os)\n+{\n+\t/* Data ready; we keep the last byte to ourselves\n+\t * in case we detect broken rev-list, so that we\n+\t * can leave the stream corrupted. This is\n+\t * unfortunate -- unpack-objects would happily\n+\t * accept a valid packdata with trailing garbage,\n+\t * so appending garbage after we pass all the\n+\t * pack data is not good enough to signal\n+\t * breakage to downstream.\n+\t */\n+\tssize_t readsz;\n+\n+\treadsz = xread(outfd, os->buffer + os->used,\n+\t\t sizeof(os->buffer) - os->used);\n+\tif (readsz < 0) {\n+\t\treturn readsz;\n+\t}\n+\tos->used += readsz;\n+\n+\tif (os->used > 1) {\n+\t\tsend_client_data(1, os->buffer, os->used - 1);\n+\t\tos->buffer[0] = os->buffer[os->used - 1];\n+\t\tos->used = 1;\n+\t} else {\n+\t\tsend_client_data(1, os->buffer, os->used);\n+\t\tos->used = 0;\n+\t}\n+\n+\treturn readsz;\n+}\n+\n static void create_pack_file(const struct object_array *have_obj,\n \t\t\t const struct object_array *want_obj)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n-\tchar data[8193], progress[128];\n+\tstruct output_state output_state = {0};\n+\tchar progress[128];\n \tchar abort_msg[] = \"aborting due to possible repository \"\n \t\t\"corruption on the remote side.\";\n-\tint buffered = -1;\n \tssize_t sz;\n \tint i;\n \tFILE *pipe_fd;\n@@ -237,39 +274,15 @@ static void create_pack_file(const struct object_array *have_obj,\n \t\t\tcontinue;\n \t\t}\n \t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n-\t\t\t/* Data ready; we keep the last byte to ourselves\n-\t\t\t * in case we detect broken rev-list, so that we\n-\t\t\t * can leave the stream corrupted. This is\n-\t\t\t * unfortunate -- unpack-objects would happily\n-\t\t\t * accept a valid packdata with trailing garbage,\n-\t\t\t * so appending garbage after we pass all the\n-\t\t\t * pack data is not good enough to signal\n-\t\t\t * breakage to downstream.\n-\t\t\t */\n-\t\t\tchar *cp = data;\n-\t\t\tssize_t outsz = 0;\n-\t\t\tif (0 <= buffered) {\n-\t\t\t\t*cp++ = buffered;\n-\t\t\t\toutsz++;\n-\t\t\t}\n-\t\t\tsz = xread(pack_objects.out, cp,\n-\t\t\t\t sizeof(data) - outsz);\n-\t\t\tif (0 < sz)\n-\t\t\t\t;\n-\t\t\telse if (sz == 0) {\n+\t\t\tint result = read_pack_objects_stdout(pack_objects.out,\n+\t\t\t\t\t\t\t &output_state);\n+\n+\t\t\tif (result == 0) {\n \t\t\t\tclose(pack_objects.out);\n \t\t\t\tpack_objects.out = -1;\n-\t\t\t}\n-\t\t\telse\n+\t\t\t} else if (result < 0) {\n \t\t\t\tgoto fail;\n-\t\t\tsz += outsz;\n-\t\t\tif (1 < sz) {\n-\t\t\t\tbuffered = data[sz-1] & 0xFF;\n-\t\t\t\tsz--;\n \t\t\t}\n-\t\t\telse\n-\t\t\t\tbuffered = -1;\n-\t\t\tsend_client_data(1, data, sz);\n \t\t}\n \n \t\t/*\n@@ -294,9 +307,8 @@ static void create_pack_file(const struct object_array *have_obj,\n \t}\n \n \t/* flush the data */\n-\tif (0 <= buffered) {\n-\t\tdata[0] = buffered;\n-\t\tsend_client_data(1, data, 1);\n+\tif (output_state.used > 0) {\n+\t\tsend_client_data(1, output_state.buffer, output_state.used);\n \t\tfprintf(stderr, \"flushed.\\n\");\n \t}\n \tif (use_sideband)\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366592","messageId":"7a6c111a9ad1c808fb3f0b562ac8b351e3fb1832.1547244620.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547244620.git.jonathantanmy@google.com","subject":"[WIP 8/4] upload-pack: send part of packfile response as uri","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-11T22:18:21Z","receivedAt":"2019-01-11T22:18:59Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is a partial implementation of upload-pack sending part of its\npackfile response as URIs.\n\nThe client is not fully implemented - it knows to ignore the\n\"packfile-uris\" section, but because it does not actually fetch those\nURIs, the returned packfile is incomplete. A test is included to show\nthat the appropriate URI is indeed transmitted, and that the returned\npackfile is lacking exactly the expected object.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/pack-objects.c | 48 ++++++++++++++++++++++++++++++++++++\n fetch-pack.c | 9 +++++++\n t/t5702-protocol-v2.sh | 27 ++++++++++++++++++++\n upload-pack.c | 56 ++++++++++++++++++++++++++++++++++++------\n 4 files changed, 133 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 411aefd687..448c42a666 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -117,6 +117,15 @@ enum missing_action {\n static enum missing_action arg_missing_action;\n static show_object_fn fn_show_object;\n \n+struct configured_exclusion {\n+\tstruct oidmap_entry e;\n+\tchar *uri;\n+};\n+static struct oidmap configured_exclusions;\n+\n+static int exclude_configured_blobs;\n+static struct oidset excluded_by_config;\n+\n /*\n * stats\n */\n@@ -831,6 +840,23 @@ static off_t write_reused_pack(struct hashfile *f)\n \treturn reuse_packfile_offset - sizeof(struct pack_header);\n }\n \n+static void write_excluded_by_configs(void)\n+{\n+\tstruct oidset_iter iter;\n+\tconst struct object_id *oid;\n+\n+\toidset_iter_init(&excluded_by_config, &iter);\n+\twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tstruct configured_exclusion *ex =\n+\t\t\toidmap_get(&configured_exclusions, oid);\n+\n+\t\tif (!ex)\n+\t\t\tBUG(\"configured exclusion wasn't configured\");\n+\t\twrite_in_full(1, ex->uri, strlen(ex->uri));\n+\t\twrite_in_full(1, \"\\n\", 1);\n+\t}\n+}\n+\n static const char no_split_warning[] = N_(\n \"disabling bitmap writing, packs are split due to pack.packSizeLimit\"\n );\n@@ -1124,6 +1150,12 @@ static int want_object_in_pack(const struct object_id *oid,\n \t\t}\n \t}\n \n+\tif (exclude_configured_blobs &&\n+\t oidmap_get(&configured_exclusions, oid)) {\n+\t\toidset_insert(&excluded_by_config, oid);\n+\t\treturn 0;\n+\t}\n+\n \treturn 1;\n }\n \n@@ -2728,6 +2760,19 @@ static int git_pack_config(const char *k, const char *v, void *cb)\n \t\t\t pack_idx_opts.version);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"uploadpack.blobpackfileuri\")) {\n+\t\tstruct configured_exclusion *ex = xmalloc(sizeof(*ex));\n+\t\tconst char *end;\n+\n+\t\tif (parse_oid_hex(v, &ex->e.oid, &end) || *end != ' ')\n+\t\t\tdie(_(\"value of uploadpack.blobpackfileuri must be \"\n+\t\t\t \"of the form ' ' (got '%s')\"), v);\n+\t\tif (oidmap_get(&configured_exclusions, &ex->e.oid))\n+\t\t\tdie(_(\"object already configured in another \"\n+\t\t\t \"uploadpack.blobpackfileuri (got '%s')\"), v);\n+\t\tex->uri = xstrdup(end + 1);\n+\t\toidmap_put(&configured_exclusions, ex);\n+\t}\n \treturn git_default_config(k, v, cb);\n }\n \n@@ -3316,6 +3361,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"do not pack objects in promisor packfiles\")),\n \t\tOPT_BOOL(0, \"delta-islands\", &use_delta_islands,\n \t\t\t N_(\"respect islands during delta compression\")),\n+\t\tOPT_BOOL(0, \"exclude-configured-blobs\", &exclude_configured_blobs,\n+\t\t\t N_(\"respect uploadpack.blobpackfileuri\")),\n \t\tOPT_END(),\n \t};\n \n@@ -3489,6 +3536,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \tif (nr_result)\n \t\tprepare_pack(window, depth);\n+\twrite_excluded_by_configs();\n \twrite_pack_file();\n \tif (progress)\n \t\tfprintf_ln(stderr,\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 4618568fee..79af87b2cf 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1429,6 +1429,15 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n \n \t\t\t/* get the pack */\n+\t\t\tif (process_section_header(&reader, \"packfile-uris\", 1)) {\n+\t\t\t\t/* skip the whole section */\n+\t\t\t\tprocess_section_header(&reader, \"packfile-uris\", 0);\n+\t\t\t\twhile (packet_reader_read(&reader) == PACKET_READ_NORMAL) {\n+\t\t\t\t\t/* do nothing */\n+\t\t\t\t}\n+\t\t\t\tif (reader.status != PACKET_READ_DELIM)\n+\t\t\t\t\tdie(\"expected DELIM\");\n+\t\t\t}\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n \t\t\tif (get_pack(args, fd, pack_lockfile))\n \t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex b491c62e3e..ba85ee4dd9 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -588,6 +588,33 @@ test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n \ttest_i18ngrep \"expected no other sections to be sent after no .ready.\" err\n '\n \n+test_expect_success 'part of packfile response provided as URI' '\n+\trm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child log &&\n+\n+\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\techo my-blob >\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/my-blob\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/\" add my-blob &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/\" commit -m x &&\n+\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/\" hash-object my-blob >h &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/\" config \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat h) https://example.com/a-uri\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/\" config \\\n+\t\t\"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\t# NEEDSWORK: \"git clone\" fails here because it ignores the URI provided\n+\t# instead of fetching it.\n+\ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" \\\n+\t\tgit -c protocol.version=2 clone \\\n+\t\t\"$HTTPD_URL/smart/http_parent\" http_child 2>err &&\n+\t# Although \"git clone\" fails, we can still check that the server\n+\t# provided the URI we requested and that the error message pinpoints\n+\t# the object that is missing.\n+\tgrep \"clone< .*uri https://example.com/a-uri\" log &&\n+\ttest_i18ngrep \"did not receive expected object $(cat h)\" err\n+'\n+\n stop_httpd\n \n test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c87b752550..fe6e2300e3 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -106,9 +106,12 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n struct output_state {\n \tchar buffer[8193];\n \tint used;\n+\tunsigned packfile_uris_started : 1;\n+\tunsigned packfile_started : 1;\n };\n \n-static int read_pack_objects_stdout(int outfd, struct output_state *os)\n+static int read_pack_objects_stdout(int outfd, struct output_state *os,\n+\t\t\t\t int write_packfile_line)\n {\n \t/* Data ready; we keep the last byte to ourselves\n \t * in case we detect broken rev-list, so that we\n@@ -128,6 +131,37 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os)\n \t}\n \tos->used += readsz;\n \n+\twhile (!os->packfile_started) {\n+\t\tchar *p;\n+\t\tif (os->used >= 4 && !memcmp(os->buffer, \"PACK\", 4)) {\n+\t\t\tos->packfile_started = 1;\n+\t\t\tif (write_packfile_line) {\n+\t\t\t\tif (os->packfile_uris_started)\n+\t\t\t\t\tpacket_delim(1);\n+\t\t\t\tpacket_write_fmt(1, \"\\1packfile\\n\");\n+\t\t\t}\n+\t\t\tbreak;\n+\t\t}\n+\t\tif ((p = memchr(os->buffer, '\\n', os->used))) {\n+\t\t\tif (!os->packfile_uris_started) {\n+\t\t\t\tos->packfile_uris_started = 1;\n+\t\t\t\tif (!write_packfile_line)\n+\t\t\t\t\tBUG(\"packfile_uris requires sideband-all\");\n+\t\t\t\tpacket_write_fmt(1, \"\\1packfile-uris\\n\");\n+\t\t\t}\n+\t\t\t*p = '\\0';\n+\t\t\tpacket_write_fmt(1, \"\\1uri %s\\n\", os->buffer);\n+\n+\t\t\tos->used -= p - os->buffer + 1;\n+\t\t\tmemmove(os->buffer, p, os->used);\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * Incomplete line.\n+\t\t\t */\n+\t\t\treturn readsz;\n+\t\t}\n+\t}\n+\n \tif (os->used > 1) {\n \t\tsend_client_data(1, os->buffer, os->used - 1);\n \t\tos->buffer[0] = os->buffer[os->used - 1];\n@@ -141,7 +175,8 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os)\n }\n \n static void create_pack_file(const struct object_array *have_obj,\n-\t\t\t const struct object_array *want_obj)\n+\t\t\t const struct object_array *want_obj,\n+\t\t\t int write_packfile_line)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n \tstruct output_state output_state = {0};\n@@ -189,6 +224,9 @@ static void create_pack_file(const struct object_array *have_obj,\n \t\t\t\t\t filter_options.filter_spec);\n \t\t}\n \t}\n+\tif (write_packfile_line)\n+\t\targv_array_push(&pack_objects.args,\n+\t\t\t\t\"--exclude-configured-blobs\");\n \n \tpack_objects.in = -1;\n \tpack_objects.out = -1;\n@@ -275,8 +313,8 @@ static void create_pack_file(const struct object_array *have_obj,\n \t\t}\n \t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n \t\t\tint result = read_pack_objects_stdout(pack_objects.out,\n-\t\t\t\t\t\t\t &output_state);\n-\n+\t\t\t\t\t\t\t &output_state,\n+\t\t\t\t\t\t\t write_packfile_line);\n \t\t\tif (result == 0) {\n \t\t\t\tclose(pack_objects.out);\n \t\t\t\tpack_objects.out = -1;\n@@ -1108,7 +1146,7 @@ void upload_pack(struct upload_pack_options *options)\n \tif (want_obj.nr) {\n \t\tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n \t\tget_common_commits(&reader, &have_obj, &want_obj);\n-\t\tcreate_pack_file(&have_obj, &want_obj);\n+\t\tcreate_pack_file(&have_obj, &want_obj, 0);\n \t}\n }\n \n@@ -1498,8 +1536,12 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \t\t\tsend_wanted_ref_info(&data);\n \t\t\tsend_shallow_info(&data, &want_obj);\n \n-\t\t\tpacket_writer_write(&data.writer, \"packfile\\n\");\n-\t\t\tcreate_pack_file(&have_obj, &want_obj);\n+\t\t\tif (data.writer.use_sideband) {\n+\t\t\t\tcreate_pack_file(&have_obj, &want_obj, 1);\n+\t\t\t} else {\n+\t\t\t\tpacket_write_fmt(1, \"packfile\\n\");\n+\t\t\t\tcreate_pack_file(&have_obj, &want_obj, 0);\n+\t\t\t}\n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n \t\tcase FETCH_DONE:\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366675","messageId":"xmqqbm4juhoc.fsf@gitster-ct.c.googlers.com","threadId":"50205","inReplyTo":"9b37e68e2f0d83ea133f6662efb780466c518170.1547244620.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/4] pkt-line: introduce struct packet_writer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-14T20:07:31Z","receivedAt":"2019-01-14T20:07:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\nThe patch itself look quite noisy, but in esssense, at the lowest\nlevel we used to have a single format_packet() that was used to\nwrite out normal payload and an error message prefixed with \"ERR \";\nnow the users of the function are updated to call one of the two\nhelper functions, packet_writer_write() or packet_writer_error().\nMost of the patch noise comes from the helper functions at higher\nlevels getting updated to pass the packet_writer struct through the\ncallchain.\n\nWhich makes tons of sense.\n\n> It will be convenient for a future patch if writing options\n> (specifically, whether the written data is to be multiplexed) could be\n> controlled from a single place, so create struct packet_writer to serve\n> as that place, and modify upload-pack to use it.\n\nI've singled out \"ERR \" in my comment above, but this only refers to\n\"multiplexed\". Are there reasons why we want multiplexing other\nthan the \"are we sending payload, or an error message\"?\n\n> Currently, it only stores the output fd, but a subsequent patch will (as\n> described above) introduce an option to determine if the written data is\n> to be multiplexed.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n\n"},{"id":"366676","messageId":"xmqq4labuho5.fsf@gitster-ct.c.googlers.com","threadId":"50205","inReplyTo":"4079a1f110086bd412fb4f56bc8b7f8a6a7a8f9f.1547244620.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-14T20:07:38Z","receivedAt":"2019-01-14T20:07:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> +int recv_sideband(const char *me, int in_stream, int out)\n> +{\n> +\tchar buf[LARGE_PACKET_MAX + 1];\n> +\tint retval = 0;\n> +\tint len;\n> +\n> +\twhile (1) {\n> +\t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);\n> +\t\tretval = diagnose_sideband(me, buf, len);\n\nThis name of the helper does not convey much useful information.\nLet's think about it more later.\n\n> +\t\tswitch (retval) {\n> +\t\t\tcase SIDEBAND_PRIMARY:\n\nDedent; case/default arms align with 's' in switch in our codebase.\n\n> +\t\t\t\twrite_or_die(out, buf + 1, len - 1);\n> +\t\t\t\tbreak;\n\nOK, so PRIMARY return (band#1) gives us the payload unwrapped back\nin \"out\" and we are responsible for writing it out here.\n\n> +\t\t\tcase SIDEBAND_PROGRESS:\n> +\t\t\t\t/* already written by diagnose_sideband() */\n\nTrue, that happens in \"switch(band) { case 2: }\" in the other helper.\n\n> +\t\t\t\tbreak;\n> +\t\t\tdefault: /* flush or error */\n> +\t\t\t\treturn retval;\n\nLack of corresponding comment bothers readers. In all of\nREMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper\nstuffs the message in outbuf in \"switch (band) { ... }\" and writes\nit out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see\nthere is no need for us to write anything out here. Perhaps\n\n\t\tcase SIDEBAND_FLUSH:\n\t\tdefault: /* errors: message already written */\n\t\t\treturn retval;\n\nor something to clarify?\n\n\n> +/*\n> + * Receive multiplexed output stream over git native protocol.\n> + * in_stream is the input stream from the remote, which carries data\n> + * in pkt_line format with band designator. Demultiplex it into out\n> + * and err and return error appropriately. Band #1 carries the\n> + * primary payload. Things coming over band #2 is not necessarily\n> + * error; they are usually informative message on the standard error\n> + * stream, aka \"verbose\"). A message over band #3 is a signal that\n> + * the remote died unexpectedly. A flush() concludes the stream.\n> + *\n> + * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR\n> + * or SIDEBAND_REMOTE_ERROR if an error occurred.\n> + */\n> +int recv_sideband(const char *me, int in_stream, int out);\n\nThis is well described.\n\n> diff --git a/sideband.c b/sideband.c\n> index 368647acf8..842a92e975 100644\n> --- a/sideband.c\n> +++ b/sideband.c\n> ...\n> +int diagnose_sideband(const char *me, char *buf, int len)\n> {\n> +\tstatic const char *suffix;\n> \tstruct strbuf outbuf = STRBUF_INIT;\n> \tint retval = 0;\n> +\tconst char *b, *brk;\n> +\tint band;\n> +\n> +\tif (!suffix) {\n> +\t\tif (isatty(2) && !is_terminal_dumb())\n> +\t\t\tsuffix = ANSI_SUFFIX;\n> +\t\telse\n> +\t\t\tsuffix = DUMB_SUFFIX;\n> +\t}\n\nIt may be worth considering a further clean-up for the progress\ncode, that consumes lines in the \"switch(band)\" below that are\ndisproportionate to what it does in this function by introducing\nanother helper function that is called from here. When it happens,\nthe above \"suffix\" thing should move to the helper function, too.\n\n> +\tif (len == 0) {\n> +\t\tretval = SIDEBAND_FLUSH;\n> +\t\tgoto cleanup;\n> +\t}\n> +\tif (len < 1) {\n> +\t\tstrbuf_addf(&outbuf,\n> +\t\t\t \"%s%s: protocol error: no band designator\",\n> +\t\t\t outbuf.len ? \"\\n\" : \"\", me);\n> +\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n> +\t\tgoto cleanup;\n> +\t}\n> +\tband = buf[0] & 0xff;\n> +\tbuf[len] = '\\0';\n> +\tlen--;\n> +\tswitch (band) {\n> +\tcase 3:\n> +...\n> +\tcase 2:\n> +...\n> +\tcase 1:\n> +\t\tretval = SIDEBAND_PRIMARY;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tstrbuf_addf(&outbuf, \"%s%s: protocol error: bad band #%d\",\n> +\t\t\t outbuf.len ? \"\\n\" : \"\", me, band);\n> +\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n> +\t\tbreak;\n\nSo, the point of this helper is to inspect a single packet-line data\nand dispatch the payload of the packet based on which band it is\nsent to. Perhaps call it with a name with demultiplex or dispatch\nin it? \"diagnose\" is a bit unclear in that what trait you are\ndiagnosing and for what purpose.\n\n> diff --git a/sideband.h b/sideband.h\n> index 7a8146f161..a56cd86287 100644\n> --- a/sideband.h\n> +++ b/sideband.h\n> @@ -3,8 +3,21 @@\n> \n> #define SIDEBAND_PROTOCOL_ERROR -2\n> #define SIDEBAND_REMOTE_ERROR -1\n> +#define SIDEBAND_FLUSH 0\n> +#define SIDEBAND_PRIMARY 1\n> +#define SIDEBAND_PROGRESS 2\n> +\n> +/*\n> + * buf and len should be the result of reading a line from a remote sending\n> + * multiplexed data.\n> + *\n> + * Determines the nature of the result and returns it. If\n\n\"the result\" may be from the point of view of the implementor or the\n\"recv_sideband()\" function who called packet_read(), but a casual\nreader would perceive it more natural if you referred it as \"a\npacket read from a remote\". \"Inspect a packet read from the remote\nand returns which sideband it is for\", perhaps?\n\n\n> + * SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS, also\n> + * prints a message (or the formatted contents of the notice in the case of\n> + * SIDEBAND_PROGRESS) to stderr.\n> + */\n> +int diagnose_sideband(const char *me, char *buf, int len);\n> \n> -int recv_sideband(const char *me, int in_stream, int out);\n> void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);\n> \n> #endif\n"},{"id":"366682","messageId":"xmqqlg3mubxt.fsf@gitster-ct.c.googlers.com","threadId":"50205","inReplyTo":"xmqq4labuho5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-14T22:11:26Z","receivedAt":"2019-01-14T22:11:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Lack of corresponding comment bothers readers. In all of\n> REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper\n> stuffs the message in outbuf in \"switch (band) { ... }\" and writes\n> it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see\n> there is no need for us to write anything out here. Perhaps\n>\n> \t\tcase SIDEBAND_FLUSH:\n> \t\tdefault: /* errors: message already written */\n> \t\t\treturn retval;\n>\n> or something to clarify?\n\nForgot a footnote for *1* above. I was wondering if xwrite is what\nwe really want, rather than write_in_full().\n"},{"id":"366683","messageId":"xmqqk1j6ub7t.fsf@gitster-ct.c.googlers.com","threadId":"50205","inReplyTo":"673b928b4158b0aecca843302483b48da5ed8d35.1547244620.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-14T22:27:02Z","receivedAt":"2019-01-14T22:27:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Currently, a response to a fetch request has sideband support only while\n> the packfile is being sent, meaning that the server cannot send notices\n> until the start of the packfile.\n>\n> Extend sideband support in protocol v2 fetch responses to the whole\n> response. upload-pack will advertise it if the\n> uploadpack.allowsidebandall configuration variable is set, and\n> fetch-pack will automatically request it if advertised.\n>\n> If the sideband is to be used throughout the whole response, upload-pack\n> will use it to send errors instead of prefixing a PKT-LINE payload with\n> \"ERR \".\n\nMakes sense.\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 3f9626dbf7..b89199891d 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1125,7 +1125,8 @@ static int add_haves(struct fetch_negotiator *negotiator,\n> static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n> \t\t\t const struct fetch_pack_args *args,\n> \t\t\t const struct ref *wants, struct oidset *common,\n> -\t\t\t int *haves_to_send, int *in_vain)\n> +\t\t\t int *haves_to_send, int *in_vain,\n> +\t\t\t int sideband_all)\n> {\n> \tint ret = 0;\n> \tstruct strbuf req_buf = STRBUF_INIT;\n> @@ -1151,6 +1152,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n> \t\tpacket_buf_write(&req_buf, \"include-tag\");\n> \tif (prefer_ofs_delta)\n> \t\tpacket_buf_write(&req_buf, \"ofs-delta\");\n> +\tif (sideband_all)\n> +\t\tpacket_buf_write(&req_buf, \"sideband-all\");\n> \n> \t/* Add shallow-info and deepen request */\n> \tif (server_supports_feature(\"fetch\", \"shallow\", 0))\n> @@ -1359,6 +1362,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n> \tpacket_reader_init(&reader, fd[0], NULL, 0,\n> \t\t\t PACKET_READ_CHOMP_NEWLINE |\n> \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> +\tif (server_supports_feature(\"fetch\", \"sideband-all\", 0)) {\n> +\t\treader.use_sideband = 1;\n> +\t\treader.me = \"fetch-pack\";\n> +\t}\n> \n> \twhile (state != FETCH_DONE) {\n> \t\tswitch (state) {\n> @@ -1392,7 +1399,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n> \t\tcase FETCH_SEND_REQUEST:\n> \t\t\tif (send_fetch_request(&negotiator, fd[1], args, ref,\n> \t\t\t\t\t &common,\n> -\t\t\t\t\t &haves_to_send, &in_vain))\n> +\t\t\t\t\t &haves_to_send, &in_vain,\n> +\t\t\t\t\t reader.use_sideband))\n> \t\t\t\tstate = FETCH_GET_PACK;\n> \t\t\telse\n> \t\t\t\tstate = FETCH_PROCESS_ACKS;\n\nOK, so the \"fetch\" side enables sideband-all any time it knows that\nthe other side supports it.\n\nIt feels a bit strange at the logical level that reader.me is set\nonly when we are going to talk over \"sideband-all\". I know that at\nthe implementation level, reader.me is only looked at when sideband\nis use, but it still feels somewhat funny. Future developers may\nwant to use reader.me to identify the entity that found some error\nduring a read, even when the sideband is not yet in use, and at that\npoint, uninitialized .me would be a source of an error. IOW, that\nassignment smells like a timebomb waiting to go off.\n\n> @@ -483,16 +483,42 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n> \t\treturn reader->status;\n> \t}\n> \n> -\treader->status = packet_read_with_status(reader->fd,\n> -\t\t\t\t\t\t &reader->src_buffer,\n> -\t\t\t\t\t\t &reader->src_len,\n> -\t\t\t\t\t\t reader->buffer,\n> -\t\t\t\t\t\t reader->buffer_size,\n> -\t\t\t\t\t\t &reader->pktlen,\n> -\t\t\t\t\t\t reader->options);\n> +\twhile (1) {\n> +\t\tint retval;\n> +\t\treader->status = packet_read_with_status(reader->fd,\n> +\t\t\t\t\t\t\t &reader->src_buffer,\n> +\t\t\t\t\t\t\t &reader->src_len,\n> +\t\t\t\t\t\t\t reader->buffer,\n> +\t\t\t\t\t\t\t reader->buffer_size,\n> +\t\t\t\t\t\t\t &reader->pktlen,\n> +\t\t\t\t\t\t\t reader->options);\n> +\t\tif (!reader->use_sideband)\n> +\t\t\tbreak;\n> +\t\tretval = diagnose_sideband(reader->me, reader->buffer,\n> +\t\t\t\t\t reader->pktlen, 1);\n> +\t\tswitch (retval) {\n> +\t\t\tcase SIDEBAND_PROTOCOL_ERROR:\n\nDedent (see previous step).\n\n> +\t\t\tcase SIDEBAND_REMOTE_ERROR:\n> +\t\t\t\tBUG(\"should have died in diagnose_sideband\");\n> +\t\t\tcase SIDEBAND_FLUSH:\n> +\t\t\t\tgoto nonprogress;\n> +\t\t\tcase SIDEBAND_PRIMARY:\n> +\t\t\t\tif (reader->pktlen != 1)\n> +\t\t\t\t\tgoto nonprogress;\n> +\t\t\t\t/*\n> +\t\t\t\t * Since pktlen is 1, this is a keepalive\n> +\t\t\t\t * packet. Wait for the next one.\n> +\t\t\t\t */\n\nWhat guarantees that a normal payload packet is never of length==1?\n\n> +\t\t\t\tbreak;\n> +\t\t\tdefault: /* SIDEBAND_PROGRESS */\n> +\t\t\t\t;\n> +\t\t}\n> +\t}\n> \n> +nonprogress:\n\nIt is totally unclear why this label is called nonprogress. Is it\nthat the primary purpose of the while loop above is to spin and eat\nthe keep-alive packets from the other side? If so, then it sort-of\nmakes sense (i.e. \"we are looking for progress-meter related\npackets, but now we found something else, so let's leave the loop\").\n\nIt would have greatly helped reviewers to have a comment in front of\nthat infinite loop, perhaps like\n\n\t/*\n\t * Spin and consume the keep-alive packets\n\t */\n\twhile (1) {\n\t\tint keepalive = 0;\n\n\t\tget_packet();\n\n\t\t/* decide if we got a keepalive here */\n\t\t...\n\t\tkeepalive = ...;\n\n\t\tif (keepalive) {\n\t\t\t/* do the keep-alive thing here */\n\t\t\t...\n\t\t\tcontinue;\n\t\t}\n\n\t\t/* do one-off thing for non-keepalive packet if any */\n\t\t...\n\t\tbreak;\n\t}\n\n> \tif (reader->status == PACKET_READ_NORMAL)\n> -\t\treader->line = reader->buffer;\n> +\t\treader->line = reader->use_sideband ?\n> +\t\t\treader->buffer + 1 : reader->buffer;\n\nIs the one byte the sideband designator?\n\n> void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n> @@ -521,7 +548,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n> \tva_list args;\n> \n> \tva_start(args, fmt);\n> -\tpacket_write_fmt_1(writer->dest_fd, 0, \"\", fmt, args);\n> +\tpacket_write_fmt_1(writer->dest_fd, 0,\n> +\t\t\t writer->use_sideband ? \"\\1\" : \"\", fmt, args);\n> \tva_end(args);\n> }\n\nAs I am superstitious, I'd prefer to see octal literals to be fully\nspelled out as 3-digit sequence, i.e. \"\\001\". Likewise for \"\\003\".\n\n> @@ -530,7 +558,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)\n> \tva_list args;\n> \n> \tva_start(args, fmt);\n> -\tpacket_write_fmt_1(writer->dest_fd, 0, \"ERR \", fmt, args);\n> +\tpacket_write_fmt_1(writer->dest_fd, 0,\n> +\t\t\t writer->use_sideband ? \"\\3\" : \"ERR \", fmt, args);\n\nOK, band#3 is an error message for emergency exit. It is a bit\ncurious that this patch does not show any line from the existing\ncode in the context that reacts to the ERR string, but that is\nbecause the errors are noticed at a lot lower level when the\nsideband is in use than the code that currently checks for errors?\n\n"},{"id":"366726","messageId":"20190115181810.249142-1-jonathantanmy@google.com","threadId":"50205","inReplyTo":"xmqqbm4juhoc.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/4] pkt-line: introduce struct packet_writer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-15T18:18:10Z","receivedAt":"2019-01-15T18:18:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> The patch itself look quite noisy, but in esssense, at the lowest\n> level we used to have a single format_packet() that was used to\n> write out normal payload and an error message prefixed with \"ERR \";\n> now the users of the function are updated to call one of the two\n> helper functions, packet_writer_write() or packet_writer_error().\n> Most of the patch noise comes from the helper functions at higher\n> levels getting updated to pass the packet_writer struct through the\n> callchain.\n> \n> Which makes tons of sense.\n\nYes, that's right. Thanks.\n\n> > It will be convenient for a future patch if writing options\n> > (specifically, whether the written data is to be multiplexed) could be\n> > controlled from a single place, so create struct packet_writer to serve\n> > as that place, and modify upload-pack to use it.\n> \n> I've singled out \"ERR \" in my comment above, but this only refers to\n> \"multiplexed\". Are there reasons why we want multiplexing other\n> than the \"are we sending payload, or an error message\"?\n\nThe main reason is to allow progress and keepalive messages at any time,\nactually. I'll add this paragraph at the start of the commit message:\n\n A future patch will allow the client to request multiplexing of the\n entire fetch response (and not only during packfile transmission), which\n in turn allows the server to send progress and keepalive messages at any\n time during the response.\n"},{"id":"366729","messageId":"20190115183642.253201-1-jonathantanmy@google.com","threadId":"50205","inReplyTo":"xmqq4labuho5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-15T18:36:42Z","receivedAt":"2019-01-15T18:36:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > +int recv_sideband(const char *me, int in_stream, int out)\n> > +{\n> > +\tchar buf[LARGE_PACKET_MAX + 1];\n> > +\tint retval = 0;\n> > +\tint len;\n> > +\n> > +\twhile (1) {\n> > +\t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);\n> > +\t\tretval = diagnose_sideband(me, buf, len);\n> \n> This name of the helper does not convey much useful information.\n> Let's think about it more later.\n\nOK - after reading the later comments, switched to demultiplex_sideband.\n\n> > +\t\tswitch (retval) {\n> > +\t\t\tcase SIDEBAND_PRIMARY:\n> \n> Dedent; case/default arms align with 's' in switch in our codebase.\n\nDone.\n\n> > +\t\t\t\tbreak;\n> > +\t\t\tdefault: /* flush or error */\n> > +\t\t\t\treturn retval;\n> \n> Lack of corresponding comment bothers readers. In all of\n> REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper\n> stuffs the message in outbuf in \"switch (band) { ... }\" and writes\n> it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see\n> there is no need for us to write anything out here. Perhaps\n> \n> \t\tcase SIDEBAND_FLUSH:\n> \t\tdefault: /* errors: message already written */\n> \t\t\treturn retval;\n> \n> or something to clarify?\n\nDone.\n\n> > +/*\n> > + * Receive multiplexed output stream over git native protocol.\n> > + * in_stream is the input stream from the remote, which carries data\n> > + * in pkt_line format with band designator. Demultiplex it into out\n> > + * and err and return error appropriately. Band #1 carries the\n> > + * primary payload. Things coming over band #2 is not necessarily\n> > + * error; they are usually informative message on the standard error\n> > + * stream, aka \"verbose\"). A message over band #3 is a signal that\n> > + * the remote died unexpectedly. A flush() concludes the stream.\n> > + *\n> > + * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR\n> > + * or SIDEBAND_REMOTE_ERROR if an error occurred.\n> > + */\n> > +int recv_sideband(const char *me, int in_stream, int out);\n> \n> This is well described.\n\nThanks, although the credit should be given to the original author - most of\nthis was moved.\n\n> > diff --git a/sideband.c b/sideband.c\n> > index 368647acf8..842a92e975 100644\n> > --- a/sideband.c\n> > +++ b/sideband.c\n> > ...\n> > +int diagnose_sideband(const char *me, char *buf, int len)\n> > {\n> > +\tstatic const char *suffix;\n> > \tstruct strbuf outbuf = STRBUF_INIT;\n> > \tint retval = 0;\n> > +\tconst char *b, *brk;\n> > +\tint band;\n> > +\n> > +\tif (!suffix) {\n> > +\t\tif (isatty(2) && !is_terminal_dumb())\n> > +\t\t\tsuffix = ANSI_SUFFIX;\n> > +\t\telse\n> > +\t\t\tsuffix = DUMB_SUFFIX;\n> > +\t}\n> \n> It may be worth considering a further clean-up for the progress\n> code, that consumes lines in the \"switch(band)\" below that are\n> disproportionate to what it does in this function by introducing\n> another helper function that is called from here. When it happens,\n> the above \"suffix\" thing should move to the helper function, too.\n\nThat's reasonable. If it's OK, I would like to limit the scope of this\npatch to splitting the existing recv_sideband() into recv_sideband() and\ndemultiplex_sideband() (the latter is quite close to the old\nrecv_sideband(), and the diff looks larger only because most of it is a\ndedent).\n\n> So, the point of this helper is to inspect a single packet-line data\n> and dispatch the payload of the packet based on which band it is\n> sent to. Perhaps call it with a name with demultiplex or dispatch\n> in it? \"diagnose\" is a bit unclear in that what trait you are\n> diagnosing and for what purpose.\n\nChanged to demultiplex_sideband.\n\n> > +/*\n> > + * buf and len should be the result of reading a line from a remote sending\n> > + * multiplexed data.\n> > + *\n> > + * Determines the nature of the result and returns it. If\n> \n> \"the result\" may be from the point of view of the implementor or the\n> \"recv_sideband()\" function who called packet_read(), but a casual\n> reader would perceive it more natural if you referred it as \"a\n> packet read from a remote\". \"Inspect a packet read from the remote\n> and returns which sideband it is for\", perhaps?\n\nDone.\n"},{"id":"366730","messageId":"20190115183807.255640-1-jonathantanmy@google.com","threadId":"50205","inReplyTo":"xmqqlg3mubxt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-15T18:38:07Z","receivedAt":"2019-01-15T18:38:14Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Junio C Hamano writes:\n> \n> > Lack of corresponding comment bothers readers. In all of\n> > REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper\n> > stuffs the message in outbuf in \"switch (band) { ... }\" and writes\n> > it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see\n> > there is no need for us to write anything out here. Perhaps\n> >\n> > \t\tcase SIDEBAND_FLUSH:\n> > \t\tdefault: /* errors: message already written */\n> > \t\t\treturn retval;\n> >\n> > or something to clarify?\n> \n> Forgot a footnote for *1* above. I was wondering if xwrite is what\n> we really want, rather than write_in_full().\n\nThere is an existing comment explaining this:\n\n\t * The output is accumulated in a buffer and\n\t * each line is printed to stderr using\n\t * write(2) to ensure inter-process atomicity.\n\nwhich seems reasonable to me (I know that write(2) does not guarantee to\nwrite everything, but write_in_full() does not guarantee atomicity, as\nfar as I know).\n"},{"id":"366739","messageId":"20190115191842.123188-1-jonathantanmy@google.com","threadId":"50205","inReplyTo":"xmqqk1j6ub7t.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-15T19:18:42Z","receivedAt":"2019-01-15T19:18:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> OK, so the \"fetch\" side enables sideband-all any time it knows that\n> the other side supports it.\n> \n> It feels a bit strange at the logical level that reader.me is set\n> only when we are going to talk over \"sideband-all\". I know that at\n> the implementation level, reader.me is only looked at when sideband\n> is use, but it still feels somewhat funny. Future developers may\n> want to use reader.me to identify the entity that found some error\n> during a read, even when the sideband is not yet in use, and at that\n> point, uninitialized .me would be a source of an error. IOW, that\n> assignment smells like a timebomb waiting to go off.\n\nI think the best solution here is to initialize .me to \"git\", like how\nwe do it for trace. I've done that for now.\n\n> > +\t\tswitch (retval) {\n> > +\t\t\tcase SIDEBAND_PROTOCOL_ERROR:\n> \n> Dedent (see previous step).\n\nDone.\n\n> > +\t\t\tcase SIDEBAND_PRIMARY:\n> > +\t\t\t\tif (reader->pktlen != 1)\n> > +\t\t\t\t\tgoto nonprogress;\n> > +\t\t\t\t/*\n> > +\t\t\t\t * Since pktlen is 1, this is a keepalive\n> > +\t\t\t\t * packet. Wait for the next one.\n> > +\t\t\t\t */\n> \n> What guarantees that a normal payload packet is never of length==1?\n\nThis is indeed assuming that we currently don't send 0004 (the\nequivalent of 0005\\1 without the sideband). I chose this because it is\ncurrently what we use in create_pack_file() in upload-pack.c, but I'm OK\nwith alternatives (e.g. if we use 0005\\2 instead).\n\n> > +\t\t\t\tbreak;\n> > +\t\t\tdefault: /* SIDEBAND_PROGRESS */\n> > +\t\t\t\t;\n> > +\t\t}\n> > +\t}\n> > \n> > +nonprogress:\n> \n> It is totally unclear why this label is called nonprogress. Is it\n> that the primary purpose of the while loop above is to spin and eat\n> the keep-alive packets from the other side? If so, then it sort-of\n> makes sense (i.e. \"we are looking for progress-meter related\n> packets, but now we found something else, so let's leave the loop\").\n> \n> It would have greatly helped reviewers to have a comment in front of\n> that infinite loop, perhaps like\n> \n> \t/*\n> \t * Spin and consume the keep-alive packets\n> \t */\n\n[snip]\n\nYes, it's meant to spin and consume the progress and keepalive packets.\nI've added a comment at the top of the loop and renamed the jump label\nto \"nonprogress_received\".\n\n> > \tif (reader->status == PACKET_READ_NORMAL)\n> > -\t\treader->line = reader->buffer;\n> > +\t\treader->line = reader->use_sideband ?\n> > +\t\t\treader->buffer + 1 : reader->buffer;\n> \n> Is the one byte the sideband designator?\n\nYes. Added comment to clarify that.\n\n> > void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n> > @@ -521,7 +548,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n> > \tva_list args;\n> > \n> > \tva_start(args, fmt);\n> > -\tpacket_write_fmt_1(writer->dest_fd, 0, \"\", fmt, args);\n> > +\tpacket_write_fmt_1(writer->dest_fd, 0,\n> > +\t\t\t writer->use_sideband ? \"\\1\" : \"\", fmt, args);\n> > \tva_end(args);\n> > }\n> \n> As I am superstitious, I'd prefer to see octal literals to be fully\n> spelled out as 3-digit sequence, i.e. \"\\001\". Likewise for \"\\003\".\n\nDone.\n\n> > @@ -530,7 +558,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)\n> > \tva_list args;\n> > \n> > \tva_start(args, fmt);\n> > -\tpacket_write_fmt_1(writer->dest_fd, 0, \"ERR \", fmt, args);\n> > +\tpacket_write_fmt_1(writer->dest_fd, 0,\n> > +\t\t\t writer->use_sideband ? \"\\3\" : \"ERR \", fmt, args);\n> \n> OK, band#3 is an error message for emergency exit. It is a bit\n> curious that this patch does not show any line from the existing\n> code in the context that reacts to the ERR string, but that is\n> because the errors are noticed at a lot lower level when the\n> sideband is in use than the code that currently checks for errors?\n\nYes - the branch that this patch set is on (ms/packet-err-check) handles\nthis. I have taken care in this patch to ensure that the error message\nprinted matches what Masaya used, so that in a future patch, when we\nforce sideband-all (using GIT_TEST_SIDEBAND_ALL), the same message is\nprinted so that the existing tests still pass.\n"},{"id":"366747","messageId":"cover.1547581039.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547244620.git.jonathantanmy@google.com","subject":"[PATCH v2 0/4] Sideband the whole fetch v2 response","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-15T19:40:26Z","receivedAt":"2019-01-15T19:40:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Like v1, this is on origin/ms/packet-err-check.\n\nThanks, Junio, for your reviews.\n\nJonathan Tan (4):\n pkt-line: introduce struct packet_writer\n sideband: reverse its dependency on pkt-line\n {fetch,upload}-pack: sideband v2 fetch response\n tests: define GIT_TEST_SIDEBAND_ALL\n\n Documentation/technical/protocol-v2.txt | 10 ++\n fetch-pack.c | 13 +-\n pkt-line.c | 121 +++++++++++++++---\n pkt-line.h | 34 +++++\n sideband.c | 161 ++++++++++++------------\n sideband.h | 14 ++-\n t/README | 5 +\n t/lib-httpd/apache.conf | 1 +\n t/t5537-fetch-shallow.sh | 3 +-\n t/t5701-git-serve.sh | 2 +-\n t/t5702-protocol-v2.sh | 4 +-\n upload-pack.c | 131 +++++++++++--------\n 12 files changed, 343 insertions(+), 156 deletions(-)\n\nInterdiff against v1:\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 71b17e6b93..69162f3990 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -447,16 +447,16 @@ int recv_sideband(const char *me, int in_stream, int out)\n \n \twhile (1) {\n \t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);\n-\t\tretval = diagnose_sideband(me, buf, len, 0);\n+\t\tretval = demultiplex_sideband(me, buf, len, 0);\n \t\tswitch (retval) {\n-\t\t\tcase SIDEBAND_PRIMARY:\n-\t\t\t\twrite_or_die(out, buf + 1, len - 1);\n-\t\t\t\tbreak;\n-\t\t\tcase SIDEBAND_PROGRESS:\n-\t\t\t\t/* already written by diagnose_sideband() */\n-\t\t\t\tbreak;\n-\t\t\tdefault: /* flush or error */\n-\t\t\t\treturn retval;\n+\t\tcase SIDEBAND_PRIMARY:\n+\t\t\twrite_or_die(out, buf + 1, len - 1);\n+\t\t\tbreak;\n+\t\tcase SIDEBAND_PROGRESS:\n+\t\t\t/* already written by demultiplex_sideband() */\n+\t\t\tbreak;\n+\t\tdefault: /* errors: message already written */\n+\t\t\treturn retval;\n \t\t}\n \t}\n }\n@@ -474,6 +474,7 @@ void packet_reader_init(struct packet_reader *reader, int fd,\n \treader->buffer = packet_buffer;\n \treader->buffer_size = sizeof(packet_buffer);\n \treader->options = options;\n+\treader->me = \"git\";\n }\n \n enum packet_read_status packet_reader_read(struct packet_reader *reader)\n@@ -483,6 +484,10 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\treturn reader->status;\n \t}\n \n+\t/*\n+\t * Consume all progress and keepalive packets until a primary payload\n+\t * packet is received\n+\t */\n \twhile (1) {\n \t\tint retval;\n \t\treader->status = packet_read_with_status(reader->fd,\n@@ -494,29 +499,31 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\t\t\t\t\t\t reader->options);\n \t\tif (!reader->use_sideband)\n \t\t\tbreak;\n-\t\tretval = diagnose_sideband(reader->me, reader->buffer,\n-\t\t\t\t\t reader->pktlen, 1);\n+\t\tretval = demultiplex_sideband(reader->me, reader->buffer,\n+\t\t\t\t\t reader->pktlen, 1);\n \t\tswitch (retval) {\n-\t\t\tcase SIDEBAND_PROTOCOL_ERROR:\n-\t\t\tcase SIDEBAND_REMOTE_ERROR:\n-\t\t\t\tBUG(\"should have died in diagnose_sideband\");\n-\t\t\tcase SIDEBAND_FLUSH:\n-\t\t\t\tgoto nonprogress;\n-\t\t\tcase SIDEBAND_PRIMARY:\n-\t\t\t\tif (reader->pktlen != 1)\n-\t\t\t\t\tgoto nonprogress;\n-\t\t\t\t/*\n-\t\t\t\t * Since pktlen is 1, this is a keepalive\n-\t\t\t\t * packet. Wait for the next one.\n-\t\t\t\t */\n-\t\t\t\tbreak;\n-\t\t\tdefault: /* SIDEBAND_PROGRESS */\n-\t\t\t\t;\n+\t\tcase SIDEBAND_PROTOCOL_ERROR:\n+\t\tcase SIDEBAND_REMOTE_ERROR:\n+\t\t\tBUG(\"should have died in diagnose_sideband\");\n+\t\tcase SIDEBAND_FLUSH:\n+\t\t\tgoto nonprogress_received;\n+\t\tcase SIDEBAND_PRIMARY:\n+\t\t\tif (reader->pktlen != 1)\n+\t\t\t\tgoto nonprogress_received;\n+\t\t\t/*\n+\t\t\t * Since the packet contains nothing but the sideband\n+\t\t\t * designator, this is a keepalive packet. Wait for the\n+\t\t\t * next one.\n+\t\t\t */\n+\t\t\tbreak;\n+\t\tdefault: /* SIDEBAND_PROGRESS */\n+\t\t\t;\n \t\t}\n \t}\n \n-nonprogress:\n+nonprogress_received:\n \tif (reader->status == PACKET_READ_NORMAL)\n+\t\t/* Skip the sideband designator if sideband is used */\n \t\treader->line = reader->use_sideband ?\n \t\t\treader->buffer + 1 : reader->buffer;\n \telse\n@@ -549,7 +556,7 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n \n \tva_start(args, fmt);\n \tpacket_write_fmt_1(writer->dest_fd, 0,\n-\t\t\t writer->use_sideband ? \"\\1\" : \"\", fmt, args);\n+\t\t\t writer->use_sideband ? \"\\001\" : \"\", fmt, args);\n \tva_end(args);\n }\n \n@@ -559,7 +566,7 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)\n \n \tva_start(args, fmt);\n \tpacket_write_fmt_1(writer->dest_fd, 0,\n-\t\t\t writer->use_sideband ? \"\\3\" : \"ERR \", fmt, args);\n+\t\t\t writer->use_sideband ? \"\\003\" : \"ERR \", fmt, args);\n \tva_end(args);\n }\n \ndiff --git a/sideband.c b/sideband.c\nindex cbeab380ae..9d3051e3fe 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -113,7 +113,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n #define ANSI_SUFFIX \"\\033[K\"\n #define DUMB_SUFFIX \" \"\n \n-int diagnose_sideband(const char *me, char *buf, int len, int die_on_error)\n+int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)\n {\n \tstatic const char *suffix;\n \tstruct strbuf outbuf = STRBUF_INIT;\ndiff --git a/sideband.h b/sideband.h\nindex 3786670694..f75c4fde2a 100644\n--- a/sideband.h\n+++ b/sideband.h\n@@ -8,15 +8,14 @@\n #define SIDEBAND_PROGRESS 2\n \n /*\n- * buf and len should be the result of reading a line from a remote sending\n- * multiplexed data.\n+ * Inspects a multiplexed packet read from the remote and returns which\n+ * sideband it is for.\n *\n- * Determines the nature of the result and returns it. If\n- * SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS, also\n- * prints a message (or the formatted contents of the notice in the case of\n- * SIDEBAND_PROGRESS) to stderr.\n+ * If SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS,\n+ * also prints a message (or the formatted contents of the notice in the case\n+ * of SIDEBAND_PROGRESS) to stderr.\n */\n-int diagnose_sideband(const char *me, char *buf, int len, int die_on_error);\n+int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error);\n \n void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);\n \n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366748","messageId":"68f9883a5a0042aee5932a007e573fe9a7d01273.1547581039.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547581039.git.jonathantanmy@google.com","subject":"[PATCH v2 1/4] pkt-line: introduce struct packet_writer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-15T19:40:27Z","receivedAt":"2019-01-15T19:40:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A future patch will allow the client to request multiplexing of the\nentire fetch response (and not only during packfile transmission), which\nin turn allows the server to send progress and keepalive messages at any\ntime during the response.\n\nIt will be convenient for a future patch if writing options\n(specifically, whether the written data is to be multiplexed) could be\ncontrolled from a single place, so create struct packet_writer to serve\nas that place, and modify upload-pack to use it.\n\nCurrently, it only stores the output fd, but a subsequent patch will (as\ndescribed above) introduce an option to determine if the written data is\nto be multiplexed.\n\nSigned-off-by: Jonathan Tan \n---\n pkt-line.c | 47 ++++++++++++++++++---\n pkt-line.h | 14 +++++++\n upload-pack.c | 112 +++++++++++++++++++++++++++-----------------------\n 3 files changed, 115 insertions(+), 58 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex e70ea6d88f..9d3e402d41 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -129,12 +129,14 @@ static void set_packet_header(char *buf, const int size)\n \t#undef hex\n }\n \n-static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n+static void format_packet(struct strbuf *out, const char *prefix,\n+\t\t\t const char *fmt, va_list args)\n {\n \tsize_t orig_len, n;\n \n \torig_len = out->len;\n \tstrbuf_addstr(out, \"0000\");\n+\tstrbuf_addstr(out, prefix);\n \tstrbuf_vaddf(out, fmt, args);\n \tn = out->len - orig_len;\n \n@@ -145,13 +147,13 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n \tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n }\n \n-static int packet_write_fmt_1(int fd, int gently,\n+static int packet_write_fmt_1(int fd, int gently, const char *prefix,\n \t\t\t const char *fmt, va_list args)\n {\n \tstatic struct strbuf buf = STRBUF_INIT;\n \n \tstrbuf_reset(&buf);\n-\tformat_packet(&buf, fmt, args);\n+\tformat_packet(&buf, prefix, fmt, args);\n \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n \t\tif (!gently) {\n \t\t\tcheck_pipe(errno);\n@@ -168,7 +170,7 @@ void packet_write_fmt(int fd, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tpacket_write_fmt_1(fd, 0, fmt, args);\n+\tpacket_write_fmt_1(fd, 0, \"\", fmt, args);\n \tva_end(args);\n }\n \n@@ -178,7 +180,7 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tstatus = packet_write_fmt_1(fd, 1, fmt, args);\n+\tstatus = packet_write_fmt_1(fd, 1, \"\", fmt, args);\n \tva_end(args);\n \treturn status;\n }\n@@ -211,7 +213,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tformat_packet(buf, fmt, args);\n+\tformat_packet(buf, \"\", fmt, args);\n \tva_end(args);\n }\n \n@@ -486,3 +488,36 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)\n \treader->line_peeked = 1;\n \treturn reader->status;\n }\n+\n+void packet_writer_init(struct packet_writer *writer, int dest_fd)\n+{\n+\twriter->dest_fd = dest_fd;\n+}\n+\n+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n+{\n+\tva_list args;\n+\n+\tva_start(args, fmt);\n+\tpacket_write_fmt_1(writer->dest_fd, 0, \"\", fmt, args);\n+\tva_end(args);\n+}\n+\n+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)\n+{\n+\tva_list args;\n+\n+\tva_start(args, fmt);\n+\tpacket_write_fmt_1(writer->dest_fd, 0, \"ERR \", fmt, args);\n+\tva_end(args);\n+}\n+\n+void packet_writer_delim(struct packet_writer *writer)\n+{\n+\tpacket_delim(writer->dest_fd);\n+}\n+\n+void packet_writer_flush(struct packet_writer *writer)\n+{\n+\tpacket_flush(writer->dest_fd);\n+}\ndiff --git a/pkt-line.h b/pkt-line.h\nindex d7e1dbc047..023ad2951d 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -183,4 +183,18 @@ extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);\n #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)\n extern char packet_buffer[LARGE_PACKET_MAX];\n \n+struct packet_writer {\n+\tint dest_fd;\n+};\n+\n+void packet_writer_init(struct packet_writer *writer, int dest_fd);\n+\n+/* These functions die upon failure. */\n+__attribute__((format (printf, 2, 3)))\n+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...);\n+__attribute__((format (printf, 2, 3)))\n+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...);\n+void packet_writer_delim(struct packet_writer *writer);\n+void packet_writer_flush(struct packet_writer *writer);\n+\n #endif\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 08b547cf01..60a26bbbfd 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -613,13 +613,14 @@ static void check_non_tip(struct object_array *want_obj)\n \t}\n }\n \n-static void send_shallow(struct commit_list *result)\n+static void send_shallow(struct packet_writer *writer,\n+\t\t\t struct commit_list *result)\n {\n \twhile (result) {\n \t\tstruct object *object = &result->item->object;\n \t\tif (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {\n-\t\t\tpacket_write_fmt(1, \"shallow %s\",\n-\t\t\t\t\t oid_to_hex(&object->oid));\n+\t\t\tpacket_writer_write(writer, \"shallow %s\",\n+\t\t\t\t\t oid_to_hex(&object->oid));\n \t\t\tregister_shallow(the_repository, &object->oid);\n \t\t\tshallow_nr++;\n \t\t}\n@@ -627,7 +628,8 @@ static void send_shallow(struct commit_list *result)\n \t}\n }\n \n-static void send_unshallow(const struct object_array *shallows,\n+static void send_unshallow(struct packet_writer *writer,\n+\t\t\t const struct object_array *shallows,\n \t\t\t struct object_array *want_obj)\n {\n \tint i;\n@@ -636,8 +638,8 @@ static void send_unshallow(const struct object_array *shallows,\n \t\tstruct object *object = shallows->objects[i].item;\n \t\tif (object->flags & NOT_SHALLOW) {\n \t\t\tstruct commit_list *parents;\n-\t\t\tpacket_write_fmt(1, \"unshallow %s\",\n-\t\t\t\t\t oid_to_hex(&object->oid));\n+\t\t\tpacket_writer_write(writer, \"unshallow %s\",\n+\t\t\t\t\t oid_to_hex(&object->oid));\n \t\t\tobject->flags &= ~CLIENT_SHALLOW;\n \t\t\t/*\n \t\t\t * We want to _register_ \"object\" as shallow, but we\n@@ -662,7 +664,7 @@ static void send_unshallow(const struct object_array *shallows,\n \t}\n }\n \n-static void deepen(int depth, int deepen_relative,\n+static void deepen(struct packet_writer *writer, int depth, int deepen_relative,\n \t\t struct object_array *shallows, struct object_array *want_obj)\n {\n \tif (depth == INFINITE_DEPTH && !is_repository_shallow(the_repository)) {\n@@ -680,7 +682,7 @@ static void deepen(int depth, int deepen_relative,\n \t\tresult = get_shallow_commits(&reachable_shallows,\n \t\t\t\t\t depth + 1,\n \t\t\t\t\t SHALLOW, NOT_SHALLOW);\n-\t\tsend_shallow(result);\n+\t\tsend_shallow(writer, result);\n \t\tfree_commit_list(result);\n \t\tobject_array_clear(&reachable_shallows);\n \t} else {\n@@ -688,14 +690,15 @@ static void deepen(int depth, int deepen_relative,\n \n \t\tresult = get_shallow_commits(want_obj, depth,\n \t\t\t\t\t SHALLOW, NOT_SHALLOW);\n-\t\tsend_shallow(result);\n+\t\tsend_shallow(writer, result);\n \t\tfree_commit_list(result);\n \t}\n \n-\tsend_unshallow(shallows, want_obj);\n+\tsend_unshallow(writer, shallows, want_obj);\n }\n \n-static void deepen_by_rev_list(int ac, const char **av,\n+static void deepen_by_rev_list(struct packet_writer *writer, int ac,\n+\t\t\t const char **av,\n \t\t\t struct object_array *shallows,\n \t\t\t struct object_array *want_obj)\n {\n@@ -703,13 +706,14 @@ static void deepen_by_rev_list(int ac, const char **av,\n \n \tclose_commit_graph(the_repository);\n \tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n-\tsend_shallow(result);\n+\tsend_shallow(writer, result);\n \tfree_commit_list(result);\n-\tsend_unshallow(shallows, want_obj);\n+\tsend_unshallow(writer, shallows, want_obj);\n }\n \n /* Returns 1 if a shallow list is sent or 0 otherwise */\n-static int send_shallow_list(int depth, int deepen_rev_list,\n+static int send_shallow_list(struct packet_writer *writer,\n+\t\t\t int depth, int deepen_rev_list,\n \t\t\t timestamp_t deepen_since,\n \t\t\t struct string_list *deepen_not,\n \t\t\t struct object_array *shallows,\n@@ -720,7 +724,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,\n \tif (depth > 0 && deepen_rev_list)\n \t\tdie(\"git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together\");\n \tif (depth > 0) {\n-\t\tdeepen(depth, deepen_relative, shallows, want_obj);\n+\t\tdeepen(writer, depth, deepen_relative, shallows, want_obj);\n \t\tret = 1;\n \t} else if (deepen_rev_list) {\n \t\tstruct argv_array av = ARGV_ARRAY_INIT;\n@@ -741,7 +745,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,\n \t\t\tstruct object *o = want_obj->objects[i].item;\n \t\t\targv_array_push(&av, oid_to_hex(&o->oid));\n \t\t}\n-\t\tdeepen_by_rev_list(av.argc, av.argv, shallows, want_obj);\n+\t\tdeepen_by_rev_list(writer, av.argc, av.argv, shallows, want_obj);\n \t\targv_array_clear(&av);\n \t\tret = 1;\n \t} else {\n@@ -834,8 +838,10 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \tint has_non_tip = 0;\n \ttimestamp_t deepen_since = 0;\n \tint deepen_rev_list = 0;\n+\tstruct packet_writer writer;\n \n \tshallow_nr = 0;\n+\tpacket_writer_init(&writer, 1);\n \tfor (;;) {\n \t\tstruct object *o;\n \t\tconst char *features;\n@@ -892,9 +898,9 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \n \t\to = parse_object(the_repository, &oid_buf);\n \t\tif (!o) {\n-\t\t\tpacket_write_fmt(1,\n-\t\t\t\t\t \"ERR upload-pack: not our ref %s\",\n-\t\t\t\t\t oid_to_hex(&oid_buf));\n+\t\t\tpacket_writer_error(&writer,\n+\t\t\t\t\t \"upload-pack: not our ref %s\",\n+\t\t\t\t\t oid_to_hex(&oid_buf));\n \t\t\tdie(\"git upload-pack: not our ref %s\",\n \t\t\t oid_to_hex(&oid_buf));\n \t\t}\n@@ -923,7 +929,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \tif (depth == 0 && !deepen_rev_list && shallows.nr == 0)\n \t\treturn;\n \n-\tif (send_shallow_list(depth, deepen_rev_list, deepen_since,\n+\tif (send_shallow_list(&writer, depth, deepen_rev_list, deepen_since,\n \t\t\t &deepen_not, &shallows, want_obj))\n \t\tpacket_flush(1);\n \tobject_array_clear(&shallows);\n@@ -1102,6 +1108,8 @@ struct upload_pack_data {\n \tint deepen_rev_list;\n \tint deepen_relative;\n \n+\tstruct packet_writer writer;\n+\n \tunsigned stateless_rpc : 1;\n \n \tunsigned use_thin_pack : 1;\n@@ -1125,6 +1133,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tdata->haves = haves;\n \tdata->shallows = shallows;\n \tdata->deepen_not = deepen_not;\n+\tpacket_writer_init(&data->writer, 1);\n }\n \n static void upload_pack_data_clear(struct upload_pack_data *data)\n@@ -1136,7 +1145,8 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \tstring_list_clear(&data->deepen_not, 0);\n }\n \n-static int parse_want(const char *line, struct object_array *want_obj)\n+static int parse_want(struct packet_writer *writer, const char *line,\n+\t\t struct object_array *want_obj)\n {\n \tconst char *arg;\n \tif (skip_prefix(line, \"want \", &arg)) {\n@@ -1149,9 +1159,9 @@ static int parse_want(const char *line, struct object_array *want_obj)\n \n \t\to = parse_object(the_repository, &oid);\n \t\tif (!o) {\n-\t\t\tpacket_write_fmt(1,\n-\t\t\t\t\t \"ERR upload-pack: not our ref %s\",\n-\t\t\t\t\t oid_to_hex(&oid));\n+\t\t\tpacket_writer_error(writer,\n+\t\t\t\t\t \"upload-pack: not our ref %s\",\n+\t\t\t\t\t oid_to_hex(&oid));\n \t\t\tdie(\"git upload-pack: not our ref %s\",\n \t\t\t oid_to_hex(&oid));\n \t\t}\n@@ -1167,7 +1177,8 @@ static int parse_want(const char *line, struct object_array *want_obj)\n \treturn 0;\n }\n \n-static int parse_want_ref(const char *line, struct string_list *wanted_refs,\n+static int parse_want_ref(struct packet_writer *writer, const char *line,\n+\t\t\t struct string_list *wanted_refs,\n \t\t\t struct object_array *want_obj)\n {\n \tconst char *arg;\n@@ -1177,7 +1188,7 @@ static int parse_want_ref(const char *line, struct string_list *wanted_refs,\n \t\tstruct object *o;\n \n \t\tif (read_ref(arg, &oid)) {\n-\t\t\tpacket_write_fmt(1, \"ERR unknown ref %s\", arg);\n+\t\t\tpacket_writer_error(writer, \"unknown ref %s\", arg);\n \t\t\tdie(\"unknown ref %s\", arg);\n \t\t}\n \n@@ -1220,10 +1231,11 @@ static void process_args(struct packet_reader *request,\n \t\tconst char *p;\n \n \t\t/* process want */\n-\t\tif (parse_want(arg, want_obj))\n+\t\tif (parse_want(&data->writer, arg, want_obj))\n \t\t\tcontinue;\n \t\tif (allow_ref_in_want &&\n-\t\t parse_want_ref(arg, &data->wanted_refs, want_obj))\n+\t\t parse_want_ref(&data->writer, arg, &data->wanted_refs,\n+\t\t\t\t want_obj))\n \t\t\tcontinue;\n \t\t/* process have line */\n \t\tif (parse_have(arg, &data->haves))\n@@ -1317,26 +1329,26 @@ static int process_haves(struct oid_array *haves, struct oid_array *common,\n \treturn 0;\n }\n \n-static int send_acks(struct oid_array *acks, struct strbuf *response,\n+static int send_acks(struct packet_writer *writer, struct oid_array *acks,\n \t\t const struct object_array *have_obj,\n \t\t struct object_array *want_obj)\n {\n \tint i;\n \n-\tpacket_buf_write(response, \"acknowledgments\\n\");\n+\tpacket_writer_write(writer, \"acknowledgments\\n\");\n \n \t/* Send Acks */\n \tif (!acks->nr)\n-\t\tpacket_buf_write(response, \"NAK\\n\");\n+\t\tpacket_writer_write(writer, \"NAK\\n\");\n \n \tfor (i = 0; i < acks->nr; i++) {\n-\t\tpacket_buf_write(response, \"ACK %s\\n\",\n-\t\t\t\t oid_to_hex(&acks->oid[i]));\n+\t\tpacket_writer_write(writer, \"ACK %s\\n\",\n+\t\t\t\t oid_to_hex(&acks->oid[i]));\n \t}\n \n \tif (ok_to_give_up(have_obj, want_obj)) {\n \t\t/* Send Ready */\n-\t\tpacket_buf_write(response, \"ready\\n\");\n+\t\tpacket_writer_write(writer, \"ready\\n\");\n \t\treturn 1;\n \t}\n \n@@ -1348,25 +1360,20 @@ static int process_haves_and_send_acks(struct upload_pack_data *data,\n \t\t\t\t struct object_array *want_obj)\n {\n \tstruct oid_array common = OID_ARRAY_INIT;\n-\tstruct strbuf response = STRBUF_INIT;\n \tint ret = 0;\n \n \tprocess_haves(&data->haves, &common, have_obj);\n \tif (data->done) {\n \t\tret = 1;\n-\t} else if (send_acks(&common, &response, have_obj, want_obj)) {\n-\t\tpacket_buf_delim(&response);\n+\t} else if (send_acks(&data->writer, &common, have_obj, want_obj)) {\n+\t\tpacket_writer_delim(&data->writer);\n \t\tret = 1;\n \t} else {\n \t\t/* Add Flush */\n-\t\tpacket_buf_flush(&response);\n+\t\tpacket_writer_flush(&data->writer);\n \t\tret = 0;\n \t}\n \n-\t/* Send response */\n-\twrite_or_die(1, response.buf, response.len);\n-\tstrbuf_release(&response);\n-\n \toid_array_clear(&data->haves);\n \toid_array_clear(&common);\n \treturn ret;\n@@ -1379,15 +1386,15 @@ static void send_wanted_ref_info(struct upload_pack_data *data)\n \tif (!data->wanted_refs.nr)\n \t\treturn;\n \n-\tpacket_write_fmt(1, \"wanted-refs\\n\");\n+\tpacket_writer_write(&data->writer, \"wanted-refs\\n\");\n \n \tfor_each_string_list_item(item, &data->wanted_refs) {\n-\t\tpacket_write_fmt(1, \"%s %s\\n\",\n-\t\t\t\t oid_to_hex(item->util),\n-\t\t\t\t item->string);\n+\t\tpacket_writer_write(&data->writer, \"%s %s\\n\",\n+\t\t\t\t oid_to_hex(item->util),\n+\t\t\t\t item->string);\n \t}\n \n-\tpacket_delim(1);\n+\tpacket_writer_delim(&data->writer);\n }\n \n static void send_shallow_info(struct upload_pack_data *data,\n@@ -1398,14 +1405,15 @@ static void send_shallow_info(struct upload_pack_data *data,\n \t !is_repository_shallow(the_repository))\n \t\treturn;\n \n-\tpacket_write_fmt(1, \"shallow-info\\n\");\n+\tpacket_writer_write(&data->writer, \"shallow-info\\n\");\n \n-\tif (!send_shallow_list(data->depth, data->deepen_rev_list,\n+\tif (!send_shallow_list(&data->writer, data->depth,\n+\t\t\t data->deepen_rev_list,\n \t\t\t data->deepen_since, &data->deepen_not,\n \t\t\t &data->shallows, want_obj) &&\n \t is_repository_shallow(the_repository))\n-\t\tdeepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,\n-\t\t want_obj);\n+\t\tdeepen(&data->writer, INFINITE_DEPTH, data->deepen_relative,\n+\t\t &data->shallows, want_obj);\n \n \tpacket_delim(1);\n }\n@@ -1467,7 +1475,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \t\t\tsend_wanted_ref_info(&data);\n \t\t\tsend_shallow_info(&data, &want_obj);\n \n-\t\t\tpacket_write_fmt(1, \"packfile\\n\");\n+\t\t\tpacket_writer_write(&data.writer, \"packfile\\n\");\n \t\t\tcreate_pack_file(&have_obj, &want_obj);\n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366749","messageId":"5aa5d047c828a0db10544c706dd595f09db3215d.1547581039.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547581039.git.jonathantanmy@google.com","subject":"[PATCH v2 2/4] sideband: reverse its dependency on pkt-line","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-15T19:40:28Z","receivedAt":"2019-01-15T19:40:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent patch will teach struct packet_reader a new field that, if\nset, instructs it to interpret read data as multiplexed. This will\ncreate a dependency from pkt-line to sideband.\n\nTo avoid a circular dependency, split recv_sideband() into 2 parts: the\nreading loop (left in recv_sideband()) and the processing of the\ncontents (in demultiplex_sideband()), and move the former into pkt-line.\nThis reverses the direction of dependency: sideband no longer depends on\npkt-line, and pkt-line now depends on sideband.\n\nSigned-off-by: Jonathan Tan \n---\n pkt-line.c | 22 ++++++++\n pkt-line.h | 16 ++++++\n sideband.c | 156 +++++++++++++++++++++++++----------------------------\n sideband.h | 14 ++++-\n 4 files changed, 124 insertions(+), 84 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 9d3e402d41..5feb73ebec 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -439,6 +439,28 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)\n \treturn sb_out->len - orig_len;\n }\n \n+int recv_sideband(const char *me, int in_stream, int out)\n+{\n+\tchar buf[LARGE_PACKET_MAX + 1];\n+\tint retval = 0;\n+\tint len;\n+\n+\twhile (1) {\n+\t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);\n+\t\tretval = demultiplex_sideband(me, buf, len);\n+\t\tswitch (retval) {\n+\t\tcase SIDEBAND_PRIMARY:\n+\t\t\twrite_or_die(out, buf + 1, len - 1);\n+\t\t\tbreak;\n+\t\tcase SIDEBAND_PROGRESS:\n+\t\t\t/* already written by demultiplex_sideband() */\n+\t\t\tbreak;\n+\t\tdefault: /* errors: message already written */\n+\t\t\treturn retval;\n+\t\t}\n+\t}\n+}\n+\n /* Packet Reader Functions */\n void packet_reader_init(struct packet_reader *reader, int fd,\n \t\t\tchar *src_buffer, size_t src_len,\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 023ad2951d..a8e92a4b63 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -3,6 +3,7 @@\n \n #include \"git-compat-util.h\"\n #include \"strbuf.h\"\n+#include \"sideband.h\"\n \n /*\n * Write a packetized stream, where each line is preceded by\n@@ -120,6 +121,21 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);\n */\n ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);\n \n+/*\n+ * Receive multiplexed output stream over git native protocol.\n+ * in_stream is the input stream from the remote, which carries data\n+ * in pkt_line format with band designator. Demultiplex it into out\n+ * and err and return error appropriately. Band #1 carries the\n+ * primary payload. Things coming over band #2 is not necessarily\n+ * error; they are usually informative message on the standard error\n+ * stream, aka \"verbose\"). A message over band #3 is a signal that\n+ * the remote died unexpectedly. A flush() concludes the stream.\n+ *\n+ * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR\n+ * or SIDEBAND_REMOTE_ERROR if an error occurred.\n+ */\n+int recv_sideband(const char *me, int in_stream, int out);\n+\n struct packet_reader {\n \t/* source file descriptor */\n \tint fd;\ndiff --git a/sideband.c b/sideband.c\nindex 368647acf8..dce22d20b1 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -1,7 +1,6 @@\n #include \"cache.h\"\n #include \"color.h\"\n #include \"config.h\"\n-#include \"pkt-line.h\"\n #include \"sideband.h\"\n #include \"help.h\"\n \n@@ -109,103 +108,94 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n }\n \n \n-/*\n- * Receive multiplexed output stream over git native protocol.\n- * in_stream is the input stream from the remote, which carries data\n- * in pkt_line format with band designator. Demultiplex it into out\n- * and err and return error appropriately. Band #1 carries the\n- * primary payload. Things coming over band #2 is not necessarily\n- * error; they are usually informative message on the standard error\n- * stream, aka \"verbose\"). A message over band #3 is a signal that\n- * the remote died unexpectedly. A flush() concludes the stream.\n- */\n-\n #define DISPLAY_PREFIX \"remote: \"\n \n #define ANSI_SUFFIX \"\\033[K\"\n #define DUMB_SUFFIX \" \"\n \n-int recv_sideband(const char *me, int in_stream, int out)\n+int demultiplex_sideband(const char *me, char *buf, int len)\n {\n-\tconst char *suffix;\n-\tchar buf[LARGE_PACKET_MAX + 1];\n+\tstatic const char *suffix;\n \tstruct strbuf outbuf = STRBUF_INIT;\n \tint retval = 0;\n+\tconst char *b, *brk;\n+\tint band;\n+\n+\tif (!suffix) {\n+\t\tif (isatty(2) && !is_terminal_dumb())\n+\t\t\tsuffix = ANSI_SUFFIX;\n+\t\telse\n+\t\t\tsuffix = DUMB_SUFFIX;\n+\t}\n \n-\tif (isatty(2) && !is_terminal_dumb())\n-\t\tsuffix = ANSI_SUFFIX;\n-\telse\n-\t\tsuffix = DUMB_SUFFIX;\n+\tif (len == 0) {\n+\t\tretval = SIDEBAND_FLUSH;\n+\t\tgoto cleanup;\n+\t}\n+\tif (len < 1) {\n+\t\tstrbuf_addf(&outbuf,\n+\t\t\t \"%s%s: protocol error: no band designator\",\n+\t\t\t outbuf.len ? \"\\n\" : \"\", me);\n+\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n+\t\tgoto cleanup;\n+\t}\n+\tband = buf[0] & 0xff;\n+\tbuf[len] = '\\0';\n+\tlen--;\n+\tswitch (band) {\n+\tcase 3:\n+\t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ? \"\\n\" : \"\",\n+\t\t\t DISPLAY_PREFIX);\n+\t\tmaybe_colorize_sideband(&outbuf, buf + 1, len);\n+\n+\t\tretval = SIDEBAND_REMOTE_ERROR;\n+\t\tbreak;\n+\tcase 2:\n+\t\tb = buf + 1;\n \n-\twhile (!retval) {\n-\t\tconst char *b, *brk;\n-\t\tint band, len;\n-\t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);\n-\t\tif (len == 0)\n-\t\t\tbreak;\n-\t\tif (len < 1) {\n-\t\t\tstrbuf_addf(&outbuf,\n-\t\t\t\t \"%s%s: protocol error: no band designator\",\n-\t\t\t\t outbuf.len ? \"\\n\" : \"\", me);\n-\t\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n-\t\t\tbreak;\n-\t\t}\n-\t\tband = buf[0] & 0xff;\n-\t\tbuf[len] = '\\0';\n-\t\tlen--;\n-\t\tswitch (band) {\n-\t\tcase 3:\n-\t\t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ? \"\\n\" : \"\",\n-\t\t\t\t DISPLAY_PREFIX);\n-\t\t\tmaybe_colorize_sideband(&outbuf, buf + 1, len);\n-\n-\t\t\tretval = SIDEBAND_REMOTE_ERROR;\n-\t\t\tbreak;\n-\t\tcase 2:\n-\t\t\tb = buf + 1;\n-\n-\t\t\t/*\n-\t\t\t * Append a suffix to each nonempty line to clear the\n-\t\t\t * end of the screen line.\n-\t\t\t *\n-\t\t\t * The output is accumulated in a buffer and\n-\t\t\t * each line is printed to stderr using\n-\t\t\t * write(2) to ensure inter-process atomicity.\n-\t\t\t */\n-\t\t\twhile ((brk = strpbrk(b, \"\\n\\r\"))) {\n-\t\t\t\tint linelen = brk - b;\n-\n-\t\t\t\tif (!outbuf.len)\n-\t\t\t\t\tstrbuf_addstr(&outbuf, DISPLAY_PREFIX);\n-\t\t\t\tif (linelen > 0) {\n-\t\t\t\t\tmaybe_colorize_sideband(&outbuf, b, linelen);\n-\t\t\t\t\tstrbuf_addstr(&outbuf, suffix);\n-\t\t\t\t}\n-\n-\t\t\t\tstrbuf_addch(&outbuf, *brk);\n-\t\t\t\txwrite(2, outbuf.buf, outbuf.len);\n-\t\t\t\tstrbuf_reset(&outbuf);\n-\n-\t\t\t\tb = brk + 1;\n+\t\t/*\n+\t\t * Append a suffix to each nonempty line to clear the\n+\t\t * end of the screen line.\n+\t\t *\n+\t\t * The output is accumulated in a buffer and\n+\t\t * each line is printed to stderr using\n+\t\t * write(2) to ensure inter-process atomicity.\n+\t\t */\n+\t\twhile ((brk = strpbrk(b, \"\\n\\r\"))) {\n+\t\t\tint linelen = brk - b;\n+\n+\t\t\tif (!outbuf.len)\n+\t\t\t\tstrbuf_addstr(&outbuf, DISPLAY_PREFIX);\n+\t\t\tif (linelen > 0) {\n+\t\t\t\tmaybe_colorize_sideband(&outbuf, b, linelen);\n+\t\t\t\tstrbuf_addstr(&outbuf, suffix);\n \t\t\t}\n \n-\t\t\tif (*b) {\n-\t\t\t\tstrbuf_addstr(&outbuf, outbuf.len ?\n-\t\t\t\t\t \"\" : DISPLAY_PREFIX);\n-\t\t\t\tmaybe_colorize_sideband(&outbuf, b, strlen(b));\n-\t\t\t}\n-\t\t\tbreak;\n-\t\tcase 1:\n-\t\t\twrite_or_die(out, buf + 1, len);\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tstrbuf_addf(&outbuf, \"%s%s: protocol error: bad band #%d\",\n-\t\t\t\t outbuf.len ? \"\\n\" : \"\", me, band);\n-\t\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n-\t\t\tbreak;\n+\t\t\tstrbuf_addch(&outbuf, *brk);\n+\t\t\txwrite(2, outbuf.buf, outbuf.len);\n+\t\t\tstrbuf_reset(&outbuf);\n+\n+\t\t\tb = brk + 1;\n+\t\t}\n+\n+\t\tif (*b) {\n+\t\t\tstrbuf_addstr(&outbuf, outbuf.len ?\n+\t\t\t\t \"\" : DISPLAY_PREFIX);\n+\t\t\tmaybe_colorize_sideband(&outbuf, b, strlen(b));\n \t\t}\n+\t\tretval = SIDEBAND_PROGRESS;\n+\t\tbreak;\n+\tcase 1:\n+\t\tretval = SIDEBAND_PRIMARY;\n+\t\tbreak;\n+\tdefault:\n+\t\tstrbuf_addf(&outbuf, \"%s%s: protocol error: bad band #%d\",\n+\t\t\t outbuf.len ? \"\\n\" : \"\", me, band);\n+\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n+\t\tbreak;\n \t}\n \n+cleanup:\n \tif (outbuf.len) {\n \t\tstrbuf_addch(&outbuf, '\\n');\n \t\txwrite(2, outbuf.buf, outbuf.len);\ndiff --git a/sideband.h b/sideband.h\nindex 7a8146f161..7244971231 100644\n--- a/sideband.h\n+++ b/sideband.h\n@@ -3,8 +3,20 @@\n \n #define SIDEBAND_PROTOCOL_ERROR -2\n #define SIDEBAND_REMOTE_ERROR -1\n+#define SIDEBAND_FLUSH 0\n+#define SIDEBAND_PRIMARY 1\n+#define SIDEBAND_PROGRESS 2\n+\n+/*\n+ * Inspects a multiplexed packet read from the remote and returns which\n+ * sideband it is for.\n+ *\n+ * If SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS,\n+ * also prints a message (or the formatted contents of the notice in the case\n+ * of SIDEBAND_PROGRESS) to stderr.\n+ */\n+int demultiplex_sideband(const char *me, char *buf, int len);\n \n-int recv_sideband(const char *me, int in_stream, int out);\n void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);\n \n #endif\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366750","messageId":"d6eefeb9c7e21191223ee6944d87972549fa6515.1547581039.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547581039.git.jonathantanmy@google.com","subject":"[PATCH v2 3/4] {fetch,upload}-pack: sideband v2 fetch response","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-15T19:40:29Z","receivedAt":"2019-01-15T19:40:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, a response to a fetch request has sideband support only while\nthe packfile is being sent, meaning that the server cannot send notices\nuntil the start of the packfile.\n\nExtend sideband support in protocol v2 fetch responses to the whole\nresponse. upload-pack will advertise it if the\nuploadpack.allowsidebandall configuration variable is set, and\nfetch-pack will automatically request it if advertised.\n\nIf the sideband is to be used throughout the whole response, upload-pack\nwill use it to send errors instead of prefixing a PKT-LINE payload with\n\"ERR \".\n\nThis will be tested in a subsequent patch.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/protocol-v2.txt | 10 +++++\n fetch-pack.c | 12 ++++-\n pkt-line.c | 58 ++++++++++++++++++++-----\n pkt-line.h | 4 ++\n sideband.c | 7 ++-\n sideband.h | 2 +-\n upload-pack.c | 16 +++++++\n 7 files changed, 94 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 09e4e0273f..1b0633f59f 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -307,6 +307,16 @@ the 'wanted-refs' section in the server's response as explained below.\n \tparticular ref, where is the full name of a ref on the\n \tserver.\n \n+If the 'sideband-all' feature is advertised, the following argument can be\n+included in the client's request:\n+\n+ sideband-all\n+\tInstruct the server to send the whole response multiplexed, not just\n+\tthe packfile section. All non-flush and non-delim PKT-LINE in the\n+\tresponse (not only in the packfile section) will then start with a byte\n+\tindicating its sideband (1, 2, or 3), and the server may send \"0005\\1\"\n+\t(a PKT-LINE of sideband 1 with no payload) as a keepalive packet.\n+\n The response of `fetch` is broken into a number of sections separated by\n delimiter packets (0001), with each section beginning with its section\n header.\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 3f9626dbf7..b89199891d 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1125,7 +1125,8 @@ static int add_haves(struct fetch_negotiator *negotiator,\n static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\t\t const struct fetch_pack_args *args,\n \t\t\t const struct ref *wants, struct oidset *common,\n-\t\t\t int *haves_to_send, int *in_vain)\n+\t\t\t int *haves_to_send, int *in_vain,\n+\t\t\t int sideband_all)\n {\n \tint ret = 0;\n \tstruct strbuf req_buf = STRBUF_INIT;\n@@ -1151,6 +1152,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tpacket_buf_write(&req_buf, \"include-tag\");\n \tif (prefer_ofs_delta)\n \t\tpacket_buf_write(&req_buf, \"ofs-delta\");\n+\tif (sideband_all)\n+\t\tpacket_buf_write(&req_buf, \"sideband-all\");\n \n \t/* Add shallow-info and deepen request */\n \tif (server_supports_feature(\"fetch\", \"shallow\", 0))\n@@ -1359,6 +1362,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n+\tif (server_supports_feature(\"fetch\", \"sideband-all\", 0)) {\n+\t\treader.use_sideband = 1;\n+\t\treader.me = \"fetch-pack\";\n+\t}\n \n \twhile (state != FETCH_DONE) {\n \t\tswitch (state) {\n@@ -1392,7 +1399,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\tcase FETCH_SEND_REQUEST:\n \t\t\tif (send_fetch_request(&negotiator, fd[1], args, ref,\n \t\t\t\t\t &common,\n-\t\t\t\t\t &haves_to_send, &in_vain))\n+\t\t\t\t\t &haves_to_send, &in_vain,\n+\t\t\t\t\t reader.use_sideband))\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\telse\n \t\t\t\tstate = FETCH_PROCESS_ACKS;\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 5feb73ebec..69162f3990 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -447,7 +447,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n \n \twhile (1) {\n \t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);\n-\t\tretval = demultiplex_sideband(me, buf, len);\n+\t\tretval = demultiplex_sideband(me, buf, len, 0);\n \t\tswitch (retval) {\n \t\tcase SIDEBAND_PRIMARY:\n \t\t\twrite_or_die(out, buf + 1, len - 1);\n@@ -474,6 +474,7 @@ void packet_reader_init(struct packet_reader *reader, int fd,\n \treader->buffer = packet_buffer;\n \treader->buffer_size = sizeof(packet_buffer);\n \treader->options = options;\n+\treader->me = \"git\";\n }\n \n enum packet_read_status packet_reader_read(struct packet_reader *reader)\n@@ -483,16 +484,48 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\treturn reader->status;\n \t}\n \n-\treader->status = packet_read_with_status(reader->fd,\n-\t\t\t\t\t\t &reader->src_buffer,\n-\t\t\t\t\t\t &reader->src_len,\n-\t\t\t\t\t\t reader->buffer,\n-\t\t\t\t\t\t reader->buffer_size,\n-\t\t\t\t\t\t &reader->pktlen,\n-\t\t\t\t\t\t reader->options);\n+\t/*\n+\t * Consume all progress and keepalive packets until a primary payload\n+\t * packet is received\n+\t */\n+\twhile (1) {\n+\t\tint retval;\n+\t\treader->status = packet_read_with_status(reader->fd,\n+\t\t\t\t\t\t\t &reader->src_buffer,\n+\t\t\t\t\t\t\t &reader->src_len,\n+\t\t\t\t\t\t\t reader->buffer,\n+\t\t\t\t\t\t\t reader->buffer_size,\n+\t\t\t\t\t\t\t &reader->pktlen,\n+\t\t\t\t\t\t\t reader->options);\n+\t\tif (!reader->use_sideband)\n+\t\t\tbreak;\n+\t\tretval = demultiplex_sideband(reader->me, reader->buffer,\n+\t\t\t\t\t reader->pktlen, 1);\n+\t\tswitch (retval) {\n+\t\tcase SIDEBAND_PROTOCOL_ERROR:\n+\t\tcase SIDEBAND_REMOTE_ERROR:\n+\t\t\tBUG(\"should have died in diagnose_sideband\");\n+\t\tcase SIDEBAND_FLUSH:\n+\t\t\tgoto nonprogress_received;\n+\t\tcase SIDEBAND_PRIMARY:\n+\t\t\tif (reader->pktlen != 1)\n+\t\t\t\tgoto nonprogress_received;\n+\t\t\t/*\n+\t\t\t * Since the packet contains nothing but the sideband\n+\t\t\t * designator, this is a keepalive packet. Wait for the\n+\t\t\t * next one.\n+\t\t\t */\n+\t\t\tbreak;\n+\t\tdefault: /* SIDEBAND_PROGRESS */\n+\t\t\t;\n+\t\t}\n+\t}\n \n+nonprogress_received:\n \tif (reader->status == PACKET_READ_NORMAL)\n-\t\treader->line = reader->buffer;\n+\t\t/* Skip the sideband designator if sideband is used */\n+\t\treader->line = reader->use_sideband ?\n+\t\t\treader->buffer + 1 : reader->buffer;\n \telse\n \t\treader->line = NULL;\n \n@@ -514,6 +547,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)\n void packet_writer_init(struct packet_writer *writer, int dest_fd)\n {\n \twriter->dest_fd = dest_fd;\n+\twriter->use_sideband = 0;\n }\n \n void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n@@ -521,7 +555,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tpacket_write_fmt_1(writer->dest_fd, 0, \"\", fmt, args);\n+\tpacket_write_fmt_1(writer->dest_fd, 0,\n+\t\t\t writer->use_sideband ? \"\\001\" : \"\", fmt, args);\n \tva_end(args);\n }\n \n@@ -530,7 +565,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tpacket_write_fmt_1(writer->dest_fd, 0, \"ERR \", fmt, args);\n+\tpacket_write_fmt_1(writer->dest_fd, 0,\n+\t\t\t writer->use_sideband ? \"\\003\" : \"ERR \", fmt, args);\n \tva_end(args);\n }\n \ndiff --git a/pkt-line.h b/pkt-line.h\nindex a8e92a4b63..ad9a4a2cd7 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -162,6 +162,9 @@ struct packet_reader {\n \n \t/* indicates if a line has been peeked */\n \tint line_peeked;\n+\n+\tunsigned use_sideband : 1;\n+\tconst char *me;\n };\n \n /*\n@@ -201,6 +204,7 @@ extern char packet_buffer[LARGE_PACKET_MAX];\n \n struct packet_writer {\n \tint dest_fd;\n+\tunsigned use_sideband : 1;\n };\n \n void packet_writer_init(struct packet_writer *writer, int dest_fd);\ndiff --git a/sideband.c b/sideband.c\nindex dce22d20b1..9d3051e3fe 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -113,7 +113,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n #define ANSI_SUFFIX \"\\033[K\"\n #define DUMB_SUFFIX \" \"\n \n-int demultiplex_sideband(const char *me, char *buf, int len)\n+int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)\n {\n \tstatic const char *suffix;\n \tstruct strbuf outbuf = STRBUF_INIT;\n@@ -144,6 +144,9 @@ int demultiplex_sideband(const char *me, char *buf, int len)\n \tlen--;\n \tswitch (band) {\n \tcase 3:\n+\t\tif (die_on_error)\n+\t\t\tdie(\"remote error: %s\", buf + 1);\n+\n \t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ? \"\\n\" : \"\",\n \t\t\t DISPLAY_PREFIX);\n \t\tmaybe_colorize_sideband(&outbuf, buf + 1, len);\n@@ -196,6 +199,8 @@ int demultiplex_sideband(const char *me, char *buf, int len)\n \t}\n \n cleanup:\n+\tif (die_on_error && retval == SIDEBAND_PROTOCOL_ERROR)\n+\t\tdie(\"%s\", outbuf.buf);\n \tif (outbuf.len) {\n \t\tstrbuf_addch(&outbuf, '\\n');\n \t\txwrite(2, outbuf.buf, outbuf.len);\ndiff --git a/sideband.h b/sideband.h\nindex 7244971231..f75c4fde2a 100644\n--- a/sideband.h\n+++ b/sideband.h\n@@ -15,7 +15,7 @@\n * also prints a message (or the formatted contents of the notice in the case\n * of SIDEBAND_PROGRESS) to stderr.\n */\n-int demultiplex_sideband(const char *me, char *buf, int len);\n+int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error);\n \n void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 60a26bbbfd..765b7695d2 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -71,6 +71,8 @@ static int allow_filter;\n static int allow_ref_in_want;\n static struct list_objects_filter_options filter_options;\n \n+static int allow_sideband_all;\n+\n static void reset_timeout(void)\n {\n \talarm(timeout);\n@@ -1046,6 +1048,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \t\tallow_filter = git_config_bool(var, value);\n \t} else if (!strcmp(\"uploadpack.allowrefinwant\", var)) {\n \t\tallow_ref_in_want = git_config_bool(var, value);\n+\t} else if (!strcmp(\"uploadpack.allowsidebandall\", var)) {\n+\t\tallow_sideband_all = git_config_bool(var, value);\n \t}\n \n \tif (current_config_scope() != CONFIG_SCOPE_REPO) {\n@@ -1284,6 +1288,11 @@ static void process_args(struct packet_reader *request,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (allow_sideband_all && !strcmp(arg, \"sideband-all\")) {\n+\t\t\tdata->writer.use_sideband = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\t/* ignore unknown lines maybe? */\n \t\tdie(\"unexpected line: '%s'\", arg);\n \t}\n@@ -1496,6 +1505,7 @@ int upload_pack_advertise(struct repository *r,\n \tif (value) {\n \t\tint allow_filter_value;\n \t\tint allow_ref_in_want;\n+\t\tint allow_sideband_all_value;\n \n \t\tstrbuf_addstr(value, \"shallow\");\n \n@@ -1510,6 +1520,12 @@ int upload_pack_advertise(struct repository *r,\n \t\t\t\t\t &allow_ref_in_want) &&\n \t\t allow_ref_in_want)\n \t\t\tstrbuf_addstr(value, \" ref-in-want\");\n+\n+\t\tif (!repo_config_get_bool(the_repository,\n+\t\t\t\t\t \"uploadpack.allowsidebandall\",\n+\t\t\t\t\t &allow_sideband_all_value) &&\n+\t\t allow_sideband_all_value)\n+\t\t\tstrbuf_addstr(value, \" sideband-all\");\n \t}\n \n \treturn 1;\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366751","messageId":"ae4866adace97d96fb312df792ab1e18fc3dfa9c.1547581039.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547581039.git.jonathantanmy@google.com","subject":"[PATCH v2 4/4] tests: define GIT_TEST_SIDEBAND_ALL","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-15T19:40:30Z","receivedAt":"2019-01-15T19:40:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Define a GIT_TEST_SIDEBAND_ALL environment variable meant to be used\nfrom tests. When set to true, this overrides uploadpack.allowsidebandall\nto true, allowing the entire test suite to be run as if this\nconfiguration is in place for all repositories.\n\nAs of this patch, all tests pass whether GIT_TEST_SIDEBAND_ALL is unset\nor set to 1.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 3 ++-\n t/README | 5 +++++\n t/lib-httpd/apache.conf | 1 +\n t/t5537-fetch-shallow.sh | 3 ++-\n t/t5701-git-serve.sh | 2 +-\n t/t5702-protocol-v2.sh | 4 ++--\n upload-pack.c | 13 ++++++++-----\n 7 files changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b89199891d..4618568fee 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1362,7 +1362,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n-\tif (server_supports_feature(\"fetch\", \"sideband-all\", 0)) {\n+\tif (git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 1) &&\n+\t server_supports_feature(\"fetch\", \"sideband-all\", 0)) {\n \t\treader.use_sideband = 1;\n \t\treader.me = \"fetch-pack\";\n \t}\ndiff --git a/t/README b/t/README\nindex 28711cc508..b275c883b8 100644\n--- a/t/README\n+++ b/t/README\n@@ -358,6 +358,11 @@ GIT_TEST_MULTI_PACK_INDEX=, when true, forces the multi-pack-\n index to be written after every 'git repack' command, and overrides the\n 'core.multiPackIndex' setting to true.\n \n+GIT_TEST_SIDEBAND_ALL=, when true, overrides the\n+'uploadpack.allowSidebandAll' setting to true, and when false, forces\n+fetch-pack to not request sideband-all (even if the server advertises\n+sideband-all).\n+\n Naming Tests\n ------------\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 581c010d8f..9a6d368247 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -78,6 +78,7 @@ PassEnv GNUPGHOME\n PassEnv ASAN_OPTIONS\n PassEnv GIT_TRACE\n PassEnv GIT_CONFIG_NOSYSTEM\n+PassEnv GIT_TEST_SIDEBAND_ALL\n \n SetEnvIf Git-Protocol \".*\" GIT_PROTOCOL=$0\n \ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 6faf17e17a..6caf628efa 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -243,7 +243,8 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f\n \t \"$(git -C \"$REPO\" rev-parse HEAD)\" \\\n \t \"$(git -C \"$REPO\" rev-parse HEAD^)\" \\\n \t >\"$HTTPD_ROOT_PATH/one-time-sed\" &&\n-\ttest_must_fail git -C client fetch --depth=1 \"$HTTPD_URL/one_time_sed/repo\" \\\n+\ttest_must_fail env GIT_TEST_SIDEBAND_ALL=0 git -C client \\\n+\t\tfetch --depth=1 \"$HTTPD_URL/one_time_sed/repo\" \\\n \t\tmaster:a_branch &&\n \n \t# Ensure that the one-time-sed script was used.\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex ae79c6bbc0..fe45bf828d 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -14,7 +14,7 @@ test_expect_success 'test capability advertisement' '\n \t0000\n \tEOF\n \n-\tgit serve --advertise-capabilities >out &&\n+\tGIT_TEST_SIDEBAND_ALL=0 git serve --advertise-capabilities >out &&\n \ttest-tool pkt-line unpack actual &&\n \ttest_cmp expect actual\n '\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 0f2b09ebb8..b491c62e3e 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -583,8 +583,8 @@ test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n \ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" git -C http_child \\\n \t\t-c protocol.version=2 \\\n \t\tfetch \"$HTTPD_URL/one_time_sed/http_parent\" 2> err &&\n-\tgrep \"fetch< acknowledgments\" log &&\n-\t! grep \"fetch< ready\" log &&\n+\tgrep \"fetch< .*acknowledgments\" log &&\n+\t! grep \"fetch< .*ready\" log &&\n \ttest_i18ngrep \"expected no other sections to be sent after no .ready.\" err\n '\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 765b7695d2..0c1feccaab 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1288,7 +1288,9 @@ static void process_args(struct packet_reader *request,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (allow_sideband_all && !strcmp(arg, \"sideband-all\")) {\n+\t\tif ((git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 0) ||\n+\t\t allow_sideband_all) &&\n+\t\t !strcmp(arg, \"sideband-all\")) {\n \t\t\tdata->writer.use_sideband = 1;\n \t\t\tcontinue;\n \t\t}\n@@ -1521,10 +1523,11 @@ int upload_pack_advertise(struct repository *r,\n \t\t allow_ref_in_want)\n \t\t\tstrbuf_addstr(value, \" ref-in-want\");\n \n-\t\tif (!repo_config_get_bool(the_repository,\n-\t\t\t\t\t \"uploadpack.allowsidebandall\",\n-\t\t\t\t\t &allow_sideband_all_value) &&\n-\t\t allow_sideband_all_value)\n+\t\tif (git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 0) ||\n+\t\t (!repo_config_get_bool(the_repository,\n+\t\t\t\t\t \"uploadpack.allowsidebandall\",\n+\t\t\t\t\t &allow_sideband_all_value) &&\n+\t\t allow_sideband_all_value))\n \t\t\tstrbuf_addstr(value, \" sideband-all\");\n \t}\n \n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366755","messageId":"xmqqtvi9r98l.fsf@gitster-ct.c.googlers.com","threadId":"50205","inReplyTo":"cover.1547581039.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/4] Sideband the whole fetch v2 response","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-15T19:50:18Z","receivedAt":"2019-01-15T19:50:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> @@ -474,6 +474,7 @@ void packet_reader_init(struct packet_reader *reader, int fd,\n> \treader->buffer = packet_buffer;\n> \treader->buffer_size = sizeof(packet_buffer);\n> \treader->options = options;\n> +\treader->me = \"git\";\n> }\n\nThis was somewhat unexpected. I would have thought that an\ninterdiff would be more like\n\n +\treader.me = \"fetch-pack\";\n if (using sideband-all) {\n reader.use_sideband = 1;\n -\t\treader.me = \"fetch-pack\";\n }\n\n> +\t\tcase SIDEBAND_PRIMARY:\n> +\t\t\tif (reader->pktlen != 1)\n> +\t\t\t\tgoto nonprogress_received;\n> +\t\t\t/*\n> +\t\t\t * Since the packet contains nothing but the sideband\n> +\t\t\t * designator, this is a keepalive packet. Wait for the\n> +\t\t\t * next one.\n> +\t\t\t */\n> +\t\t\tbreak;\n> +\t\tdefault: /* SIDEBAND_PROGRESS */\n> +\t\t\t;\n\nOK.\n\nWill replace. Thanks.\n"},{"id":"366767","messageId":"xmqqa7k1r5h9.fsf@gitster-ct.c.googlers.com","threadId":"50205","inReplyTo":"cover.1547581039.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/4] Sideband the whole fetch v2 response","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-15T21:11:30Z","receivedAt":"2019-01-15T21:11:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Like v1, this is on origin/ms/packet-err-check.\n\nBy the way, when merged to 'pu' as one of the earlier topic, t5409\nstarts to fail under --stress.\n\n\t$ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'\n\t$ make\n\t$ cd t && sh ./t5409-col*.sh --stress\n\nThis is not new to this round; v1 exhibited the same symptom.\n\nThanks.\n\n\n"},{"id":"366770","messageId":"20190115220835.61384-1-jonathantanmy@google.com","threadId":"50205","inReplyTo":"xmqqa7k1r5h9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/4] Sideband the whole fetch v2 response","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-15T22:08:35Z","receivedAt":"2019-01-15T22:08:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > Like v1, this is on origin/ms/packet-err-check.\n> \n> By the way, when merged to 'pu' as one of the earlier topic, t5409\n> starts to fail under --stress.\n> \n> \t$ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'\n> \t$ make\n> \t$ cd t && sh ./t5409-col*.sh --stress\n> \n> This is not new to this round; v1 exhibited the same symptom.\n> \n> Thanks.\n\nThanks for checking. I don't think this branch is the cause of this\nissue, though. I ran the same stress test on both:\n\n - f3035d003e (\"Merge branch 'sg/stress-test' into jch\", 2019-01-14) and\n - the result of merging sg/stress-test into master,\n\nand the test fails with the same result.\n"},{"id":"366778","messageId":"xmqqva2ppn17.fsf@gitster-ct.c.googlers.com","threadId":"50205","inReplyTo":"20190115220835.61384-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 0/4] Sideband the whole fetch v2 response","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-15T22:35:16Z","receivedAt":"2019-01-15T22:35:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> Jonathan Tan writes:\n>> \n>> > Like v1, this is on origin/ms/packet-err-check.\n>> \n>> By the way, when merged to 'pu' as one of the earlier topic, t5409\n>> starts to fail under --stress.\n>> \n>> \t$ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'\n>> \t$ make\n>> \t$ cd t && sh ./t5409-col*.sh --stress\n>> \n>> This is not new to this round; v1 exhibited the same symptom.\n>> \n>> Thanks.\n>\n> Thanks for checking. I don't think this branch is the cause of this\n> issue, though. I ran the same stress test on both:\n>\n> - f3035d003e (\"Merge branch 'sg/stress-test' into jch\", 2019-01-14) and\n> - the result of merging sg/stress-test into master,\n>\n> and the test fails with the same result.\n\nInteresting. That is not what I am seeing (as I manually bisected\nthe first-parent chain between f3035d003e and the tip of pu).\n"},{"id":"366780","messageId":"20190115230239.57954-1-jonathantanmy@google.com","threadId":"50205","inReplyTo":"xmqqva2ppn17.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/4] Sideband the whole fetch v2 response","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-15T23:02:39Z","receivedAt":"2019-01-15T23:02:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> >> Jonathan Tan writes:\n> >> \n> >> > Like v1, this is on origin/ms/packet-err-check.\n> >> \n> >> By the way, when merged to 'pu' as one of the earlier topic, t5409\n> >> starts to fail under --stress.\n> >> \n> >> \t$ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'\n> >> \t$ make\n> >> \t$ cd t && sh ./t5409-col*.sh --stress\n> >> \n> >> This is not new to this round; v1 exhibited the same symptom.\n> >> \n> >> Thanks.\n> >\n> > Thanks for checking. I don't think this branch is the cause of this\n> > issue, though. I ran the same stress test on both:\n> >\n> > - f3035d003e (\"Merge branch 'sg/stress-test' into jch\", 2019-01-14) and\n> > - the result of merging sg/stress-test into master,\n> >\n> > and the test fails with the same result.\n> \n> Interesting. That is not what I am seeing (as I manually bisected\n> the first-parent chain between f3035d003e and the tip of pu).\n\nAh...yes, you're right. I forgot to build before running the tests. I'll\ntake a look.\n"},{"id":"366781","messageId":"xmqqr2ddpl8q.fsf@gitster-ct.c.googlers.com","threadId":"50205","inReplyTo":"20190115230239.57954-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 0/4] Sideband the whole fetch v2 response","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-15T23:13:57Z","receivedAt":"2019-01-15T23:14:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> Jonathan Tan writes:\n>> \n>> >> Jonathan Tan writes:\n>> >> \n>> >> > Like v1, this is on origin/ms/packet-err-check.\n>> >> \n>> >> By the way, when merged to 'pu' as one of the earlier topic, t5409\n>> >> starts to fail under --stress.\n>> >> \n>> >> \t$ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'\n>> >> \t$ make\n>> >> \t$ cd t && sh ./t5409-col*.sh --stress\n>> >> \n>> >> This is not new to this round; v1 exhibited the same symptom.\n>> >> \n>> >> Thanks.\n>> >\n>> > Thanks for checking. I don't think this branch is the cause of this\n>> > issue, though. I ran the same stress test on both:\n>> >\n>> > - f3035d003e (\"Merge branch 'sg/stress-test' into jch\", 2019-01-14) and\n>> > - the result of merging sg/stress-test into master,\n>> >\n>> > and the test fails with the same result.\n>> \n>> Interesting. That is not what I am seeing (as I manually bisected\n>> the first-parent chain between f3035d003e and the tip of pu).\n>\n> Ah...yes, you're right. I forgot to build before running the tests. I'll\n> take a look.\n\nThanks.\n"},{"id":"366787","messageId":"20190116003828.64889-1-jonathantanmy@google.com","threadId":"50205","inReplyTo":"xmqqr2ddpl8q.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/4] Sideband the whole fetch v2 response","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-16T00:38:28Z","receivedAt":"2019-01-16T00:38:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Ah...yes, you're right. I forgot to build before running the tests. I'll\n> > take a look.\n> \n> Thanks.\n\nThanks once again for taking a look. It turns out that it's because\nprogress messages are sometimes split across PKT-LINEs depending on your\nluck, and we need to retain the \"leftover\" on a \\2 sideband in order to\ncombine it with the next one if necessary. So, for example, the\nfollowing fixup works:\n\n\tdiff --git a/sideband.c b/sideband.c\n\tindex c185c38637..d5da587d68 100644\n\t--- a/sideband.c\n\t+++ b/sideband.c\n\t@@ -117,7 +117,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n\t int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)\n\t {\n\t static const char *suffix;\n\t- struct strbuf outbuf = STRBUF_INIT;\n\t+ static struct strbuf outbuf = STRBUF_INIT;\n\t int retval = 0;\n\t const char *b, *brk;\n\t int band;\n\t@@ -187,8 +187,7 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)\n\t \"\" : DISPLAY_PREFIX);\n\t maybe_colorize_sideband(&outbuf, b, strlen(b));\n\t }\n\t- retval = SIDEBAND_PROGRESS;\n\t- break;\n\t+ return SIDEBAND_PROGRESS; /* skip cleanup */\n\t case 1:\n\t retval = SIDEBAND_PRIMARY;\n\t break;\n\nWe could make the caller of demultiplex_sideband() store the outbuf, but\nat this point, it might be better to refactor packet_reader into its own\nfile and have it depend on both pkt-line.h and sideband.h. If you (or\nanyone else) have any ideas, let me know what you think. I'll think\nfurther about this too.\n"},{"id":"366795","messageId":"xmqqy37lnsuz.fsf@gitster-ct.c.googlers.com","threadId":"50205","inReplyTo":"20190116003828.64889-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 0/4] Sideband the whole fetch v2 response","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-16T04:12:20Z","receivedAt":"2019-01-16T04:12:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> We could make the caller of demultiplex_sideband() store the outbuf, but\n> at this point, it might be better to refactor packet_reader into its own\n> file and have it depend on both pkt-line.h and sideband.h.\n\nI do not know or too deeply care about the pkt-line / sideband\ndistinction, as they are logically one thing. With a packet-reader\nabstraction in place, doesn't this \"here is the remainder of the\npacket from the previous round\" belong to each reader instance?\n"},{"id":"366813","messageId":"20190116103438.GG840@szeder.dev","threadId":"50205","inReplyTo":"5aa5d047c828a0db10544c706dd595f09db3215d.1547581039.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/4] sideband: reverse its dependency on pkt-line","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-16T10:34:38Z","receivedAt":"2019-01-16T10:34:44Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jan 15, 2019 at 11:40:28AM -0800, Jonathan Tan wrote:\n> A subsequent patch will teach struct packet_reader a new field that, if\n> set, instructs it to interpret read data as multiplexed. This will\n> create a dependency from pkt-line to sideband.\n> \n> To avoid a circular dependency, split recv_sideband() into 2 parts: the\n> reading loop (left in recv_sideband()) and the processing of the\n> contents (in demultiplex_sideband()), and move the former into pkt-line.\n> This reverses the direction of dependency: sideband no longer depends on\n> pkt-line, and pkt-line now depends on sideband.\n\nIn the last couple of days I noticed occasional but frequent failures\nin the test 'leading space' in 't5409-colorize-remote-messas.sh' on\n'pu' and on this topic. Bisect suggests that this patch is the\nculprit, but of course bisecting an occasional test failure can't be\ncompletely trusted.\n\nThe trace output of the failing test looks like this:\n\n + git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space\n + cat output\n remote: error: error \n remote: ERROR: also highlighted \n remote: hint: hint \n remote: hinting: not highlighted \n remote: success: success \n remote: warning: warning \n remote: prefixerror: error \n remote: \n remote: error: leading space \n remote: \n remote: Err \n To /<...>/trash directory.t5409-colorize-remote-messages.stress-4/.\n * [new branch] HEAD -> leading-space\n + test_decode_color\n + awk \n < ... snip enormous awk script ... >\n + grep error: leading space decoded\n error: last command exited with $?=1\n not ok 6 - leading space\n\nNotice how that \"error: leading space\" lines up with the other\nmessages. On 'master' that line looks like this:\n\n remote: error: leading space \n\n\n"},{"id":"366858","messageId":"20190116174319.251432-1-jonathantanmy@google.com","threadId":"50205","inReplyTo":"20190116103438.GG840@szeder.dev","subject":"Re: [PATCH v2 2/4] sideband: reverse its dependency on pkt-line","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-16T17:43:19Z","receivedAt":"2019-01-16T17:43:25Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Tue, Jan 15, 2019 at 11:40:28AM -0800, Jonathan Tan wrote:\n> > A subsequent patch will teach struct packet_reader a new field that, if\n> > set, instructs it to interpret read data as multiplexed. This will\n> > create a dependency from pkt-line to sideband.\n> > \n> > To avoid a circular dependency, split recv_sideband() into 2 parts: the\n> > reading loop (left in recv_sideband()) and the processing of the\n> > contents (in demultiplex_sideband()), and move the former into pkt-line.\n> > This reverses the direction of dependency: sideband no longer depends on\n> > pkt-line, and pkt-line now depends on sideband.\n> \n> In the last couple of days I noticed occasional but frequent failures\n> in the test 'leading space' in 't5409-colorize-remote-messas.sh' on\n> 'pu' and on this topic. Bisect suggests that this patch is the\n> culprit, but of course bisecting an occasional test failure can't be\n> completely trusted.\n> \n> The trace output of the failing test looks like this:\n\nThanks for checking. Junio noticed a similar thing and I have replied to\nit here:\n\nhttps://public-inbox.org/git/20190116003828.64889-1-jonathantanmy@google.com/\n\nI'll try to ensure that this issue is fixed in the subsequent version.\n"},{"id":"366867","messageId":"20190116191721.GH840@szeder.dev","threadId":"50205","inReplyTo":"20190116174319.251432-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 2/4] sideband: reverse its dependency on pkt-line","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-16T19:17:21Z","receivedAt":"2019-01-16T19:17:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jan 16, 2019 at 09:43:19AM -0800, Jonathan Tan wrote:\n> > On Tue, Jan 15, 2019 at 11:40:28AM -0800, Jonathan Tan wrote:\n> > In the last couple of days I noticed occasional but frequent failures\n> > in the test 'leading space' in 't5409-colorize-remote-messas.sh' on\n \n> Thanks for checking. Junio noticed a similar thing and I have replied to\n> it here:\n> \n> https://public-inbox.org/git/20190116003828.64889-1-jonathantanmy@google.com/\n> \n> I'll try to ensure that this issue is fixed in the subsequent version.\n\nOh, indeed, that must be the same issue. After I bisected the issue I\nonly paid attention to the discussion of patch 2/4 in v1, and\ncompletely ignored the replies to the cover letter.\n\nBut I'm glad that --stress makes itself useful :)\n\n"},{"id":"366868","messageId":"cover.1547666330.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547244620.git.jonathantanmy@google.com","subject":"[PATCH v3 0/4] Sideband the whole fetch v2 response","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-16T19:28:11Z","receivedAt":"2019-01-16T19:28:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Like previous versions, this is on origin/ms/packet-err-check.\n\nFirst of all, thanks to those who noticed the issue with t5409. I have\nmerged sg/stress-test and ran:\n\n make DEVELOPER=1 && (cd t && sh ./t5409-col*.sh --stress)\n\nwith no issues.\n\ndemultiplex_sideband() now requires its caller to maintain a strbuf,\nsince progress messages can be split across packets. I have also changed\nthe API of demultiplex_sideband() to hopefully make things clearer.\n\nAs for keepalive packets, I think it's best if we state that the remote\nshould use 0005\\2 instead of 0005\\1, and have made the change\naccordingly. This does not need to be treated specially by the client.\n(I shouldn't have a problem updating my CDN offloading patches [1] to\nuse 0005\\2 before the packfile starts and 0005\\1 after the packfile\nstarts.)\n\n[1] Patches 5-8 of https://public-inbox.org/git/cover.1547244620.git.jonathantanmy@google.com/\n\nJonathan Tan (4):\n pkt-line: introduce struct packet_writer\n sideband: reverse its dependency on pkt-line\n {fetch,upload}-pack: sideband v2 fetch response\n tests: define GIT_TEST_SIDEBAND_ALL\n\n Documentation/technical/protocol-v2.txt | 10 ++\n fetch-pack.c | 13 +-\n pkt-line.c | 107 ++++++++++++--\n pkt-line.h | 34 +++++\n sideband.c | 178 ++++++++++++------------\n sideband.h | 25 +++-\n t/README | 5 +\n t/lib-httpd/apache.conf | 1 +\n t/t5537-fetch-shallow.sh | 3 +-\n t/t5701-git-serve.sh | 2 +-\n t/t5702-protocol-v2.sh | 4 +-\n upload-pack.c | 131 ++++++++++-------\n 12 files changed, 346 insertions(+), 167 deletions(-)\n\nInterdiff against v2:\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 1b0633f59f..39b40c0dc1 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -314,8 +314,8 @@ included in the client's request:\n \tInstruct the server to send the whole response multiplexed, not just\n \tthe packfile section. All non-flush and non-delim PKT-LINE in the\n \tresponse (not only in the packfile section) will then start with a byte\n-\tindicating its sideband (1, 2, or 3), and the server may send \"0005\\1\"\n-\t(a PKT-LINE of sideband 1 with no payload) as a keepalive packet.\n+\tindicating its sideband (1, 2, or 3), and the server may send \"0005\\2\"\n+\t(a PKT-LINE of sideband 2 with no payload) as a keepalive packet.\n \n The response of `fetch` is broken into a number of sections separated by\n delimiter packets (0001), with each section beginning with its section\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 69162f3990..d4b71d3e82 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -442,21 +442,22 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)\n int recv_sideband(const char *me, int in_stream, int out)\n {\n \tchar buf[LARGE_PACKET_MAX + 1];\n-\tint retval = 0;\n \tint len;\n+\tstruct strbuf scratch = STRBUF_INIT;\n+\tenum sideband_type sideband_type;\n \n \twhile (1) {\n-\t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);\n-\t\tretval = demultiplex_sideband(me, buf, len, 0);\n-\t\tswitch (retval) {\n+\t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX,\n+\t\t\t\t 0);\n+\t\tif (!demultiplex_sideband(me, buf, len, 0, &scratch,\n+\t\t\t\t\t &sideband_type))\n+\t\t\tcontinue;\n+\t\tswitch (sideband_type) {\n \t\tcase SIDEBAND_PRIMARY:\n \t\t\twrite_or_die(out, buf + 1, len - 1);\n \t\t\tbreak;\n-\t\tcase SIDEBAND_PROGRESS:\n-\t\t\t/* already written by demultiplex_sideband() */\n-\t\t\tbreak;\n \t\tdefault: /* errors: message already written */\n-\t\t\treturn retval;\n+\t\t\treturn sideband_type;\n \t\t}\n \t}\n }\n@@ -479,17 +480,19 @@ void packet_reader_init(struct packet_reader *reader, int fd,\n \n enum packet_read_status packet_reader_read(struct packet_reader *reader)\n {\n+\tstruct strbuf scratch = STRBUF_INIT;\n+\n \tif (reader->line_peeked) {\n \t\treader->line_peeked = 0;\n \t\treturn reader->status;\n \t}\n \n \t/*\n-\t * Consume all progress and keepalive packets until a primary payload\n-\t * packet is received\n+\t * Consume all progress packets until a primary payload packet is\n+\t * received\n \t */\n \twhile (1) {\n-\t\tint retval;\n+\t\tenum sideband_type sideband_type;\n \t\treader->status = packet_read_with_status(reader->fd,\n \t\t\t\t\t\t\t &reader->src_buffer,\n \t\t\t\t\t\t\t &reader->src_len,\n@@ -499,29 +502,12 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\t\t\t\t\t\t reader->options);\n \t\tif (!reader->use_sideband)\n \t\t\tbreak;\n-\t\tretval = demultiplex_sideband(reader->me, reader->buffer,\n-\t\t\t\t\t reader->pktlen, 1);\n-\t\tswitch (retval) {\n-\t\tcase SIDEBAND_PROTOCOL_ERROR:\n-\t\tcase SIDEBAND_REMOTE_ERROR:\n-\t\t\tBUG(\"should have died in diagnose_sideband\");\n-\t\tcase SIDEBAND_FLUSH:\n-\t\t\tgoto nonprogress_received;\n-\t\tcase SIDEBAND_PRIMARY:\n-\t\t\tif (reader->pktlen != 1)\n-\t\t\t\tgoto nonprogress_received;\n-\t\t\t/*\n-\t\t\t * Since the packet contains nothing but the sideband\n-\t\t\t * designator, this is a keepalive packet. Wait for the\n-\t\t\t * next one.\n-\t\t\t */\n+\t\tif (demultiplex_sideband(reader->me, reader->buffer,\n+\t\t\t\t\t reader->pktlen, 1, &scratch,\n+\t\t\t\t\t &sideband_type))\n \t\t\tbreak;\n-\t\tdefault: /* SIDEBAND_PROGRESS */\n-\t\t\t;\n-\t\t}\n \t}\n \n-nonprogress_received:\n \tif (reader->status == PACKET_READ_NORMAL)\n \t\t/* Skip the sideband designator if sideband is used */\n \t\treader->line = reader->use_sideband ?\ndiff --git a/sideband.c b/sideband.c\nindex 9d3051e3fe..6a16feb262 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -113,11 +113,12 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n #define ANSI_SUFFIX \"\\033[K\"\n #define DUMB_SUFFIX \" \"\n \n-int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)\n+int demultiplex_sideband(const char *me, char *buf, int len,\n+\t\t\t int die_on_error,\n+\t\t\t struct strbuf *scratch,\n+\t\t\t enum sideband_type *sideband_type)\n {\n \tstatic const char *suffix;\n-\tstruct strbuf outbuf = STRBUF_INIT;\n-\tint retval = 0;\n \tconst char *b, *brk;\n \tint band;\n \n@@ -129,14 +130,14 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)\n \t}\n \n \tif (len == 0) {\n-\t\tretval = SIDEBAND_FLUSH;\n+\t\t*sideband_type = SIDEBAND_FLUSH;\n \t\tgoto cleanup;\n \t}\n \tif (len < 1) {\n-\t\tstrbuf_addf(&outbuf,\n+\t\tstrbuf_addf(scratch,\n \t\t\t \"%s%s: protocol error: no band designator\",\n-\t\t\t outbuf.len ? \"\\n\" : \"\", me);\n-\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n+\t\t\t scratch->len ? \"\\n\" : \"\", me);\n+\t\t*sideband_type = SIDEBAND_PROTOCOL_ERROR;\n \t\tgoto cleanup;\n \t}\n \tband = buf[0] & 0xff;\n@@ -146,12 +147,11 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)\n \tcase 3:\n \t\tif (die_on_error)\n \t\t\tdie(\"remote error: %s\", buf + 1);\n-\n-\t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ? \"\\n\" : \"\",\n+\t\tstrbuf_addf(scratch, \"%s%s\", scratch->len ? \"\\n\" : \"\",\n \t\t\t DISPLAY_PREFIX);\n-\t\tmaybe_colorize_sideband(&outbuf, buf + 1, len);\n+\t\tmaybe_colorize_sideband(scratch, buf + 1, len);\n \n-\t\tretval = SIDEBAND_REMOTE_ERROR;\n+\t\t*sideband_type = SIDEBAND_REMOTE_ERROR;\n \t\tbreak;\n \tcase 2:\n \t\tb = buf + 1;\n@@ -167,46 +167,45 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)\n \t\twhile ((brk = strpbrk(b, \"\\n\\r\"))) {\n \t\t\tint linelen = brk - b;\n \n-\t\t\tif (!outbuf.len)\n-\t\t\t\tstrbuf_addstr(&outbuf, DISPLAY_PREFIX);\n+\t\t\tif (!scratch->len)\n+\t\t\t\tstrbuf_addstr(scratch, DISPLAY_PREFIX);\n \t\t\tif (linelen > 0) {\n-\t\t\t\tmaybe_colorize_sideband(&outbuf, b, linelen);\n-\t\t\t\tstrbuf_addstr(&outbuf, suffix);\n+\t\t\t\tmaybe_colorize_sideband(scratch, b, linelen);\n+\t\t\t\tstrbuf_addstr(scratch, suffix);\n \t\t\t}\n \n-\t\t\tstrbuf_addch(&outbuf, *brk);\n-\t\t\txwrite(2, outbuf.buf, outbuf.len);\n-\t\t\tstrbuf_reset(&outbuf);\n+\t\t\tstrbuf_addch(scratch, *brk);\n+\t\t\txwrite(2, scratch->buf, scratch->len);\n+\t\t\tstrbuf_reset(scratch);\n \n \t\t\tb = brk + 1;\n \t\t}\n \n \t\tif (*b) {\n-\t\t\tstrbuf_addstr(&outbuf, outbuf.len ?\n+\t\t\tstrbuf_addstr(scratch, scratch->len ?\n \t\t\t\t \"\" : DISPLAY_PREFIX);\n-\t\t\tmaybe_colorize_sideband(&outbuf, b, strlen(b));\n+\t\t\tmaybe_colorize_sideband(scratch, b, strlen(b));\n \t\t}\n-\t\tretval = SIDEBAND_PROGRESS;\n-\t\tbreak;\n+\t\treturn 0;\n \tcase 1:\n-\t\tretval = SIDEBAND_PRIMARY;\n+\t\t*sideband_type = SIDEBAND_PRIMARY;\n \t\tbreak;\n \tdefault:\n-\t\tstrbuf_addf(&outbuf, \"%s%s: protocol error: bad band #%d\",\n-\t\t\t outbuf.len ? \"\\n\" : \"\", me, band);\n-\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n+\t\tstrbuf_addf(scratch, \"%s%s: protocol error: bad band #%d\",\n+\t\t\t scratch->len ? \"\\n\" : \"\", me, band);\n+\t\t*sideband_type = SIDEBAND_PROTOCOL_ERROR;\n \t\tbreak;\n \t}\n \n cleanup:\n-\tif (die_on_error && retval == SIDEBAND_PROTOCOL_ERROR)\n-\t\tdie(\"%s\", outbuf.buf);\n-\tif (outbuf.len) {\n-\t\tstrbuf_addch(&outbuf, '\\n');\n-\t\txwrite(2, outbuf.buf, outbuf.len);\n+\tif (die_on_error && *sideband_type == SIDEBAND_PROTOCOL_ERROR)\n+\t\tdie(\"%s\", scratch->buf);\n+\tif (scratch->len) {\n+\t\tstrbuf_addch(scratch, '\\n');\n+\t\txwrite(2, scratch->buf, scratch->len);\n \t}\n-\tstrbuf_release(&outbuf);\n-\treturn retval;\n+\tstrbuf_release(scratch);\n+\treturn 1;\n }\n \n /*\ndiff --git a/sideband.h b/sideband.h\nindex f75c4fde2a..227740a58e 100644\n--- a/sideband.h\n+++ b/sideband.h\n@@ -1,21 +1,28 @@\n #ifndef SIDEBAND_H\n #define SIDEBAND_H\n \n-#define SIDEBAND_PROTOCOL_ERROR -2\n-#define SIDEBAND_REMOTE_ERROR -1\n-#define SIDEBAND_FLUSH 0\n-#define SIDEBAND_PRIMARY 1\n-#define SIDEBAND_PROGRESS 2\n+enum sideband_type {\n+\tSIDEBAND_PROTOCOL_ERROR = -2,\n+\tSIDEBAND_REMOTE_ERROR = -1,\n+\tSIDEBAND_FLUSH = 0,\n+\tSIDEBAND_PRIMARY = 1\n+};\n \n /*\n- * Inspects a multiplexed packet read from the remote and returns which\n- * sideband it is for.\n+ * Inspects a multiplexed packet read from the remote. If this packet is a\n+ * progress packet and thus should not be processed by the caller, returns 0.\n+ * Otherwise, returns 1, releases scratch, and sets sideband_type.\n *\n- * If SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS,\n- * also prints a message (or the formatted contents of the notice in the case\n- * of SIDEBAND_PROGRESS) to stderr.\n+ * If this packet is SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or a\n+ * progress packet, also prints a message to stderr.\n+ *\n+ * scratch must be a struct strbuf allocated by the caller. It is used to store\n+ * progress messages split across multiple packets.\n */\n-int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error);\n+int demultiplex_sideband(const char *me, char *buf, int len,\n+\t\t\t int die_on_error,\n+\t\t\t struct strbuf *scratch,\n+\t\t\t enum sideband_type *sideband_type);\n \n void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);\n \n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366869","messageId":"bc2e795cea607e444a9f985b0dbed5a4d25921c8.1547666330.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547666330.git.jonathantanmy@google.com","subject":"[PATCH v3 1/4] pkt-line: introduce struct packet_writer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-16T19:28:12Z","receivedAt":"2019-01-16T19:28:24Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A future patch will allow the client to request multiplexing of the\nentire fetch response (and not only during packfile transmission), which\nin turn allows the server to send progress and keepalive messages at any\ntime during the response.\n\nIt will be convenient for a future patch if writing options\n(specifically, whether the written data is to be multiplexed) could be\ncontrolled from a single place, so create struct packet_writer to serve\nas that place, and modify upload-pack to use it.\n\nCurrently, it only stores the output fd, but a subsequent patch will (as\ndescribed above) introduce an option to determine if the written data is\nto be multiplexed.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n pkt-line.c | 47 ++++++++++++++++++---\n pkt-line.h | 14 +++++++\n upload-pack.c | 112 +++++++++++++++++++++++++++-----------------------\n 3 files changed, 115 insertions(+), 58 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex e70ea6d88f..9d3e402d41 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -129,12 +129,14 @@ static void set_packet_header(char *buf, const int size)\n \t#undef hex\n }\n \n-static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n+static void format_packet(struct strbuf *out, const char *prefix,\n+\t\t\t const char *fmt, va_list args)\n {\n \tsize_t orig_len, n;\n \n \torig_len = out->len;\n \tstrbuf_addstr(out, \"0000\");\n+\tstrbuf_addstr(out, prefix);\n \tstrbuf_vaddf(out, fmt, args);\n \tn = out->len - orig_len;\n \n@@ -145,13 +147,13 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n \tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n }\n \n-static int packet_write_fmt_1(int fd, int gently,\n+static int packet_write_fmt_1(int fd, int gently, const char *prefix,\n \t\t\t const char *fmt, va_list args)\n {\n \tstatic struct strbuf buf = STRBUF_INIT;\n \n \tstrbuf_reset(&buf);\n-\tformat_packet(&buf, fmt, args);\n+\tformat_packet(&buf, prefix, fmt, args);\n \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n \t\tif (!gently) {\n \t\t\tcheck_pipe(errno);\n@@ -168,7 +170,7 @@ void packet_write_fmt(int fd, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tpacket_write_fmt_1(fd, 0, fmt, args);\n+\tpacket_write_fmt_1(fd, 0, \"\", fmt, args);\n \tva_end(args);\n }\n \n@@ -178,7 +180,7 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tstatus = packet_write_fmt_1(fd, 1, fmt, args);\n+\tstatus = packet_write_fmt_1(fd, 1, \"\", fmt, args);\n \tva_end(args);\n \treturn status;\n }\n@@ -211,7 +213,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tformat_packet(buf, fmt, args);\n+\tformat_packet(buf, \"\", fmt, args);\n \tva_end(args);\n }\n \n@@ -486,3 +488,36 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)\n \treader->line_peeked = 1;\n \treturn reader->status;\n }\n+\n+void packet_writer_init(struct packet_writer *writer, int dest_fd)\n+{\n+\twriter->dest_fd = dest_fd;\n+}\n+\n+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n+{\n+\tva_list args;\n+\n+\tva_start(args, fmt);\n+\tpacket_write_fmt_1(writer->dest_fd, 0, \"\", fmt, args);\n+\tva_end(args);\n+}\n+\n+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)\n+{\n+\tva_list args;\n+\n+\tva_start(args, fmt);\n+\tpacket_write_fmt_1(writer->dest_fd, 0, \"ERR \", fmt, args);\n+\tva_end(args);\n+}\n+\n+void packet_writer_delim(struct packet_writer *writer)\n+{\n+\tpacket_delim(writer->dest_fd);\n+}\n+\n+void packet_writer_flush(struct packet_writer *writer)\n+{\n+\tpacket_flush(writer->dest_fd);\n+}\ndiff --git a/pkt-line.h b/pkt-line.h\nindex d7e1dbc047..023ad2951d 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -183,4 +183,18 @@ extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);\n #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)\n extern char packet_buffer[LARGE_PACKET_MAX];\n \n+struct packet_writer {\n+\tint dest_fd;\n+};\n+\n+void packet_writer_init(struct packet_writer *writer, int dest_fd);\n+\n+/* These functions die upon failure. */\n+__attribute__((format (printf, 2, 3)))\n+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...);\n+__attribute__((format (printf, 2, 3)))\n+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...);\n+void packet_writer_delim(struct packet_writer *writer);\n+void packet_writer_flush(struct packet_writer *writer);\n+\n #endif\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 08b547cf01..60a26bbbfd 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -613,13 +613,14 @@ static void check_non_tip(struct object_array *want_obj)\n \t}\n }\n \n-static void send_shallow(struct commit_list *result)\n+static void send_shallow(struct packet_writer *writer,\n+\t\t\t struct commit_list *result)\n {\n \twhile (result) {\n \t\tstruct object *object = &result->item->object;\n \t\tif (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {\n-\t\t\tpacket_write_fmt(1, \"shallow %s\",\n-\t\t\t\t\t oid_to_hex(&object->oid));\n+\t\t\tpacket_writer_write(writer, \"shallow %s\",\n+\t\t\t\t\t oid_to_hex(&object->oid));\n \t\t\tregister_shallow(the_repository, &object->oid);\n \t\t\tshallow_nr++;\n \t\t}\n@@ -627,7 +628,8 @@ static void send_shallow(struct commit_list *result)\n \t}\n }\n \n-static void send_unshallow(const struct object_array *shallows,\n+static void send_unshallow(struct packet_writer *writer,\n+\t\t\t const struct object_array *shallows,\n \t\t\t struct object_array *want_obj)\n {\n \tint i;\n@@ -636,8 +638,8 @@ static void send_unshallow(const struct object_array *shallows,\n \t\tstruct object *object = shallows->objects[i].item;\n \t\tif (object->flags & NOT_SHALLOW) {\n \t\t\tstruct commit_list *parents;\n-\t\t\tpacket_write_fmt(1, \"unshallow %s\",\n-\t\t\t\t\t oid_to_hex(&object->oid));\n+\t\t\tpacket_writer_write(writer, \"unshallow %s\",\n+\t\t\t\t\t oid_to_hex(&object->oid));\n \t\t\tobject->flags &= ~CLIENT_SHALLOW;\n \t\t\t/*\n \t\t\t * We want to _register_ \"object\" as shallow, but we\n@@ -662,7 +664,7 @@ static void send_unshallow(const struct object_array *shallows,\n \t}\n }\n \n-static void deepen(int depth, int deepen_relative,\n+static void deepen(struct packet_writer *writer, int depth, int deepen_relative,\n \t\t struct object_array *shallows, struct object_array *want_obj)\n {\n \tif (depth == INFINITE_DEPTH && !is_repository_shallow(the_repository)) {\n@@ -680,7 +682,7 @@ static void deepen(int depth, int deepen_relative,\n \t\tresult = get_shallow_commits(&reachable_shallows,\n \t\t\t\t\t depth + 1,\n \t\t\t\t\t SHALLOW, NOT_SHALLOW);\n-\t\tsend_shallow(result);\n+\t\tsend_shallow(writer, result);\n \t\tfree_commit_list(result);\n \t\tobject_array_clear(&reachable_shallows);\n \t} else {\n@@ -688,14 +690,15 @@ static void deepen(int depth, int deepen_relative,\n \n \t\tresult = get_shallow_commits(want_obj, depth,\n \t\t\t\t\t SHALLOW, NOT_SHALLOW);\n-\t\tsend_shallow(result);\n+\t\tsend_shallow(writer, result);\n \t\tfree_commit_list(result);\n \t}\n \n-\tsend_unshallow(shallows, want_obj);\n+\tsend_unshallow(writer, shallows, want_obj);\n }\n \n-static void deepen_by_rev_list(int ac, const char **av,\n+static void deepen_by_rev_list(struct packet_writer *writer, int ac,\n+\t\t\t const char **av,\n \t\t\t struct object_array *shallows,\n \t\t\t struct object_array *want_obj)\n {\n@@ -703,13 +706,14 @@ static void deepen_by_rev_list(int ac, const char **av,\n \n \tclose_commit_graph(the_repository);\n \tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n-\tsend_shallow(result);\n+\tsend_shallow(writer, result);\n \tfree_commit_list(result);\n-\tsend_unshallow(shallows, want_obj);\n+\tsend_unshallow(writer, shallows, want_obj);\n }\n \n /* Returns 1 if a shallow list is sent or 0 otherwise */\n-static int send_shallow_list(int depth, int deepen_rev_list,\n+static int send_shallow_list(struct packet_writer *writer,\n+\t\t\t int depth, int deepen_rev_list,\n \t\t\t timestamp_t deepen_since,\n \t\t\t struct string_list *deepen_not,\n \t\t\t struct object_array *shallows,\n@@ -720,7 +724,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,\n \tif (depth > 0 && deepen_rev_list)\n \t\tdie(\"git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together\");\n \tif (depth > 0) {\n-\t\tdeepen(depth, deepen_relative, shallows, want_obj);\n+\t\tdeepen(writer, depth, deepen_relative, shallows, want_obj);\n \t\tret = 1;\n \t} else if (deepen_rev_list) {\n \t\tstruct argv_array av = ARGV_ARRAY_INIT;\n@@ -741,7 +745,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,\n \t\t\tstruct object *o = want_obj->objects[i].item;\n \t\t\targv_array_push(&av, oid_to_hex(&o->oid));\n \t\t}\n-\t\tdeepen_by_rev_list(av.argc, av.argv, shallows, want_obj);\n+\t\tdeepen_by_rev_list(writer, av.argc, av.argv, shallows, want_obj);\n \t\targv_array_clear(&av);\n \t\tret = 1;\n \t} else {\n@@ -834,8 +838,10 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \tint has_non_tip = 0;\n \ttimestamp_t deepen_since = 0;\n \tint deepen_rev_list = 0;\n+\tstruct packet_writer writer;\n \n \tshallow_nr = 0;\n+\tpacket_writer_init(&writer, 1);\n \tfor (;;) {\n \t\tstruct object *o;\n \t\tconst char *features;\n@@ -892,9 +898,9 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \n \t\to = parse_object(the_repository, &oid_buf);\n \t\tif (!o) {\n-\t\t\tpacket_write_fmt(1,\n-\t\t\t\t\t \"ERR upload-pack: not our ref %s\",\n-\t\t\t\t\t oid_to_hex(&oid_buf));\n+\t\t\tpacket_writer_error(&writer,\n+\t\t\t\t\t \"upload-pack: not our ref %s\",\n+\t\t\t\t\t oid_to_hex(&oid_buf));\n \t\t\tdie(\"git upload-pack: not our ref %s\",\n \t\t\t oid_to_hex(&oid_buf));\n \t\t}\n@@ -923,7 +929,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \tif (depth == 0 && !deepen_rev_list && shallows.nr == 0)\n \t\treturn;\n \n-\tif (send_shallow_list(depth, deepen_rev_list, deepen_since,\n+\tif (send_shallow_list(&writer, depth, deepen_rev_list, deepen_since,\n \t\t\t &deepen_not, &shallows, want_obj))\n \t\tpacket_flush(1);\n \tobject_array_clear(&shallows);\n@@ -1102,6 +1108,8 @@ struct upload_pack_data {\n \tint deepen_rev_list;\n \tint deepen_relative;\n \n+\tstruct packet_writer writer;\n+\n \tunsigned stateless_rpc : 1;\n \n \tunsigned use_thin_pack : 1;\n@@ -1125,6 +1133,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tdata->haves = haves;\n \tdata->shallows = shallows;\n \tdata->deepen_not = deepen_not;\n+\tpacket_writer_init(&data->writer, 1);\n }\n \n static void upload_pack_data_clear(struct upload_pack_data *data)\n@@ -1136,7 +1145,8 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \tstring_list_clear(&data->deepen_not, 0);\n }\n \n-static int parse_want(const char *line, struct object_array *want_obj)\n+static int parse_want(struct packet_writer *writer, const char *line,\n+\t\t struct object_array *want_obj)\n {\n \tconst char *arg;\n \tif (skip_prefix(line, \"want \", &arg)) {\n@@ -1149,9 +1159,9 @@ static int parse_want(const char *line, struct object_array *want_obj)\n \n \t\to = parse_object(the_repository, &oid);\n \t\tif (!o) {\n-\t\t\tpacket_write_fmt(1,\n-\t\t\t\t\t \"ERR upload-pack: not our ref %s\",\n-\t\t\t\t\t oid_to_hex(&oid));\n+\t\t\tpacket_writer_error(writer,\n+\t\t\t\t\t \"upload-pack: not our ref %s\",\n+\t\t\t\t\t oid_to_hex(&oid));\n \t\t\tdie(\"git upload-pack: not our ref %s\",\n \t\t\t oid_to_hex(&oid));\n \t\t}\n@@ -1167,7 +1177,8 @@ static int parse_want(const char *line, struct object_array *want_obj)\n \treturn 0;\n }\n \n-static int parse_want_ref(const char *line, struct string_list *wanted_refs,\n+static int parse_want_ref(struct packet_writer *writer, const char *line,\n+\t\t\t struct string_list *wanted_refs,\n \t\t\t struct object_array *want_obj)\n {\n \tconst char *arg;\n@@ -1177,7 +1188,7 @@ static int parse_want_ref(const char *line, struct string_list *wanted_refs,\n \t\tstruct object *o;\n \n \t\tif (read_ref(arg, &oid)) {\n-\t\t\tpacket_write_fmt(1, \"ERR unknown ref %s\", arg);\n+\t\t\tpacket_writer_error(writer, \"unknown ref %s\", arg);\n \t\t\tdie(\"unknown ref %s\", arg);\n \t\t}\n \n@@ -1220,10 +1231,11 @@ static void process_args(struct packet_reader *request,\n \t\tconst char *p;\n \n \t\t/* process want */\n-\t\tif (parse_want(arg, want_obj))\n+\t\tif (parse_want(&data->writer, arg, want_obj))\n \t\t\tcontinue;\n \t\tif (allow_ref_in_want &&\n-\t\t parse_want_ref(arg, &data->wanted_refs, want_obj))\n+\t\t parse_want_ref(&data->writer, arg, &data->wanted_refs,\n+\t\t\t\t want_obj))\n \t\t\tcontinue;\n \t\t/* process have line */\n \t\tif (parse_have(arg, &data->haves))\n@@ -1317,26 +1329,26 @@ static int process_haves(struct oid_array *haves, struct oid_array *common,\n \treturn 0;\n }\n \n-static int send_acks(struct oid_array *acks, struct strbuf *response,\n+static int send_acks(struct packet_writer *writer, struct oid_array *acks,\n \t\t const struct object_array *have_obj,\n \t\t struct object_array *want_obj)\n {\n \tint i;\n \n-\tpacket_buf_write(response, \"acknowledgments\\n\");\n+\tpacket_writer_write(writer, \"acknowledgments\\n\");\n \n \t/* Send Acks */\n \tif (!acks->nr)\n-\t\tpacket_buf_write(response, \"NAK\\n\");\n+\t\tpacket_writer_write(writer, \"NAK\\n\");\n \n \tfor (i = 0; i < acks->nr; i++) {\n-\t\tpacket_buf_write(response, \"ACK %s\\n\",\n-\t\t\t\t oid_to_hex(&acks->oid[i]));\n+\t\tpacket_writer_write(writer, \"ACK %s\\n\",\n+\t\t\t\t oid_to_hex(&acks->oid[i]));\n \t}\n \n \tif (ok_to_give_up(have_obj, want_obj)) {\n \t\t/* Send Ready */\n-\t\tpacket_buf_write(response, \"ready\\n\");\n+\t\tpacket_writer_write(writer, \"ready\\n\");\n \t\treturn 1;\n \t}\n \n@@ -1348,25 +1360,20 @@ static int process_haves_and_send_acks(struct upload_pack_data *data,\n \t\t\t\t struct object_array *want_obj)\n {\n \tstruct oid_array common = OID_ARRAY_INIT;\n-\tstruct strbuf response = STRBUF_INIT;\n \tint ret = 0;\n \n \tprocess_haves(&data->haves, &common, have_obj);\n \tif (data->done) {\n \t\tret = 1;\n-\t} else if (send_acks(&common, &response, have_obj, want_obj)) {\n-\t\tpacket_buf_delim(&response);\n+\t} else if (send_acks(&data->writer, &common, have_obj, want_obj)) {\n+\t\tpacket_writer_delim(&data->writer);\n \t\tret = 1;\n \t} else {\n \t\t/* Add Flush */\n-\t\tpacket_buf_flush(&response);\n+\t\tpacket_writer_flush(&data->writer);\n \t\tret = 0;\n \t}\n \n-\t/* Send response */\n-\twrite_or_die(1, response.buf, response.len);\n-\tstrbuf_release(&response);\n-\n \toid_array_clear(&data->haves);\n \toid_array_clear(&common);\n \treturn ret;\n@@ -1379,15 +1386,15 @@ static void send_wanted_ref_info(struct upload_pack_data *data)\n \tif (!data->wanted_refs.nr)\n \t\treturn;\n \n-\tpacket_write_fmt(1, \"wanted-refs\\n\");\n+\tpacket_writer_write(&data->writer, \"wanted-refs\\n\");\n \n \tfor_each_string_list_item(item, &data->wanted_refs) {\n-\t\tpacket_write_fmt(1, \"%s %s\\n\",\n-\t\t\t\t oid_to_hex(item->util),\n-\t\t\t\t item->string);\n+\t\tpacket_writer_write(&data->writer, \"%s %s\\n\",\n+\t\t\t\t oid_to_hex(item->util),\n+\t\t\t\t item->string);\n \t}\n \n-\tpacket_delim(1);\n+\tpacket_writer_delim(&data->writer);\n }\n \n static void send_shallow_info(struct upload_pack_data *data,\n@@ -1398,14 +1405,15 @@ static void send_shallow_info(struct upload_pack_data *data,\n \t !is_repository_shallow(the_repository))\n \t\treturn;\n \n-\tpacket_write_fmt(1, \"shallow-info\\n\");\n+\tpacket_writer_write(&data->writer, \"shallow-info\\n\");\n \n-\tif (!send_shallow_list(data->depth, data->deepen_rev_list,\n+\tif (!send_shallow_list(&data->writer, data->depth,\n+\t\t\t data->deepen_rev_list,\n \t\t\t data->deepen_since, &data->deepen_not,\n \t\t\t &data->shallows, want_obj) &&\n \t is_repository_shallow(the_repository))\n-\t\tdeepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,\n-\t\t want_obj);\n+\t\tdeepen(&data->writer, INFINITE_DEPTH, data->deepen_relative,\n+\t\t &data->shallows, want_obj);\n \n \tpacket_delim(1);\n }\n@@ -1467,7 +1475,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \t\t\tsend_wanted_ref_info(&data);\n \t\t\tsend_shallow_info(&data, &want_obj);\n \n-\t\t\tpacket_write_fmt(1, \"packfile\\n\");\n+\t\t\tpacket_writer_write(&data.writer, \"packfile\\n\");\n \t\t\tcreate_pack_file(&have_obj, &want_obj);\n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366870","messageId":"1a8f645c19fffd2ecd53c5c15be48a1e436729ba.1547666330.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547666330.git.jonathantanmy@google.com","subject":"[PATCH v3 2/4] sideband: reverse its dependency on pkt-line","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-16T19:28:13Z","receivedAt":"2019-01-16T19:28:28Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent patch will teach struct packet_reader a new field that, if\nset, instructs it to interpret read data as multiplexed. This will\ncreate a dependency from pkt-line to sideband.\n\nTo avoid a circular dependency, split recv_sideband() into 2 parts: the\nreading loop (left in recv_sideband()) and the processing of the\ncontents (in demultiplex_sideband()), and move the former into pkt-line.\nThis reverses the direction of dependency: sideband no longer depends on\npkt-line, and pkt-line now depends on sideband.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n pkt-line.c | 23 +++++++\n pkt-line.h | 16 +++++\n sideband.c | 173 +++++++++++++++++++++++++----------------------------\n sideband.h | 24 +++++++-\n 4 files changed, 141 insertions(+), 95 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 9d3e402d41..321ff632a5 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -439,6 +439,29 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)\n \treturn sb_out->len - orig_len;\n }\n \n+int recv_sideband(const char *me, int in_stream, int out)\n+{\n+\tchar buf[LARGE_PACKET_MAX + 1];\n+\tint len;\n+\tstruct strbuf scratch = STRBUF_INIT;\n+\tenum sideband_type sideband_type;\n+\n+\twhile (1) {\n+\t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX,\n+\t\t\t\t 0);\n+\t\tif (!demultiplex_sideband(me, buf, len, &scratch,\n+\t\t\t\t\t &sideband_type))\n+\t\t\tcontinue;\n+\t\tswitch (sideband_type) {\n+\t\tcase SIDEBAND_PRIMARY:\n+\t\t\twrite_or_die(out, buf + 1, len - 1);\n+\t\t\tbreak;\n+\t\tdefault: /* errors: message already written */\n+\t\t\treturn sideband_type;\n+\t\t}\n+\t}\n+}\n+\n /* Packet Reader Functions */\n void packet_reader_init(struct packet_reader *reader, int fd,\n \t\t\tchar *src_buffer, size_t src_len,\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 023ad2951d..a8e92a4b63 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -3,6 +3,7 @@\n \n #include \"git-compat-util.h\"\n #include \"strbuf.h\"\n+#include \"sideband.h\"\n \n /*\n * Write a packetized stream, where each line is preceded by\n@@ -120,6 +121,21 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);\n */\n ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);\n \n+/*\n+ * Receive multiplexed output stream over git native protocol.\n+ * in_stream is the input stream from the remote, which carries data\n+ * in pkt_line format with band designator. Demultiplex it into out\n+ * and err and return error appropriately. Band #1 carries the\n+ * primary payload. Things coming over band #2 is not necessarily\n+ * error; they are usually informative message on the standard error\n+ * stream, aka \"verbose\"). A message over band #3 is a signal that\n+ * the remote died unexpectedly. A flush() concludes the stream.\n+ *\n+ * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR\n+ * or SIDEBAND_REMOTE_ERROR if an error occurred.\n+ */\n+int recv_sideband(const char *me, int in_stream, int out);\n+\n struct packet_reader {\n \t/* source file descriptor */\n \tint fd;\ndiff --git a/sideband.c b/sideband.c\nindex 368647acf8..e89d677626 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -1,7 +1,6 @@\n #include \"cache.h\"\n #include \"color.h\"\n #include \"config.h\"\n-#include \"pkt-line.h\"\n #include \"sideband.h\"\n #include \"help.h\"\n \n@@ -109,109 +108,99 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n }\n \n \n-/*\n- * Receive multiplexed output stream over git native protocol.\n- * in_stream is the input stream from the remote, which carries data\n- * in pkt_line format with band designator. Demultiplex it into out\n- * and err and return error appropriately. Band #1 carries the\n- * primary payload. Things coming over band #2 is not necessarily\n- * error; they are usually informative message on the standard error\n- * stream, aka \"verbose\"). A message over band #3 is a signal that\n- * the remote died unexpectedly. A flush() concludes the stream.\n- */\n-\n #define DISPLAY_PREFIX \"remote: \"\n \n #define ANSI_SUFFIX \"\\033[K\"\n #define DUMB_SUFFIX \" \"\n \n-int recv_sideband(const char *me, int in_stream, int out)\n+int demultiplex_sideband(const char *me, char *buf, int len,\n+\t\t\t struct strbuf *scratch,\n+\t\t\t enum sideband_type *sideband_type)\n {\n-\tconst char *suffix;\n-\tchar buf[LARGE_PACKET_MAX + 1];\n-\tstruct strbuf outbuf = STRBUF_INIT;\n-\tint retval = 0;\n-\n-\tif (isatty(2) && !is_terminal_dumb())\n-\t\tsuffix = ANSI_SUFFIX;\n-\telse\n-\t\tsuffix = DUMB_SUFFIX;\n-\n-\twhile (!retval) {\n-\t\tconst char *b, *brk;\n-\t\tint band, len;\n-\t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);\n-\t\tif (len == 0)\n-\t\t\tbreak;\n-\t\tif (len < 1) {\n-\t\t\tstrbuf_addf(&outbuf,\n-\t\t\t\t \"%s%s: protocol error: no band designator\",\n-\t\t\t\t outbuf.len ? \"\\n\" : \"\", me);\n-\t\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n-\t\t\tbreak;\n-\t\t}\n-\t\tband = buf[0] & 0xff;\n-\t\tbuf[len] = '\\0';\n-\t\tlen--;\n-\t\tswitch (band) {\n-\t\tcase 3:\n-\t\t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ? \"\\n\" : \"\",\n-\t\t\t\t DISPLAY_PREFIX);\n-\t\t\tmaybe_colorize_sideband(&outbuf, buf + 1, len);\n-\n-\t\t\tretval = SIDEBAND_REMOTE_ERROR;\n-\t\t\tbreak;\n-\t\tcase 2:\n-\t\t\tb = buf + 1;\n-\n-\t\t\t/*\n-\t\t\t * Append a suffix to each nonempty line to clear the\n-\t\t\t * end of the screen line.\n-\t\t\t *\n-\t\t\t * The output is accumulated in a buffer and\n-\t\t\t * each line is printed to stderr using\n-\t\t\t * write(2) to ensure inter-process atomicity.\n-\t\t\t */\n-\t\t\twhile ((brk = strpbrk(b, \"\\n\\r\"))) {\n-\t\t\t\tint linelen = brk - b;\n-\n-\t\t\t\tif (!outbuf.len)\n-\t\t\t\t\tstrbuf_addstr(&outbuf, DISPLAY_PREFIX);\n-\t\t\t\tif (linelen > 0) {\n-\t\t\t\t\tmaybe_colorize_sideband(&outbuf, b, linelen);\n-\t\t\t\t\tstrbuf_addstr(&outbuf, suffix);\n-\t\t\t\t}\n-\n-\t\t\t\tstrbuf_addch(&outbuf, *brk);\n-\t\t\t\txwrite(2, outbuf.buf, outbuf.len);\n-\t\t\t\tstrbuf_reset(&outbuf);\n-\n-\t\t\t\tb = brk + 1;\n-\t\t\t}\n+\tstatic const char *suffix;\n+\tconst char *b, *brk;\n+\tint band;\n+\n+\tif (!suffix) {\n+\t\tif (isatty(2) && !is_terminal_dumb())\n+\t\t\tsuffix = ANSI_SUFFIX;\n+\t\telse\n+\t\t\tsuffix = DUMB_SUFFIX;\n+\t}\n \n-\t\t\tif (*b) {\n-\t\t\t\tstrbuf_addstr(&outbuf, outbuf.len ?\n-\t\t\t\t\t \"\" : DISPLAY_PREFIX);\n-\t\t\t\tmaybe_colorize_sideband(&outbuf, b, strlen(b));\n+\tif (len == 0) {\n+\t\t*sideband_type = SIDEBAND_FLUSH;\n+\t\tgoto cleanup;\n+\t}\n+\tif (len < 1) {\n+\t\tstrbuf_addf(scratch,\n+\t\t\t \"%s%s: protocol error: no band designator\",\n+\t\t\t scratch->len ? \"\\n\" : \"\", me);\n+\t\t*sideband_type = SIDEBAND_PROTOCOL_ERROR;\n+\t\tgoto cleanup;\n+\t}\n+\tband = buf[0] & 0xff;\n+\tbuf[len] = '\\0';\n+\tlen--;\n+\tswitch (band) {\n+\tcase 3:\n+\t\tstrbuf_addf(scratch, \"%s%s\", scratch->len ? \"\\n\" : \"\",\n+\t\t\t DISPLAY_PREFIX);\n+\t\tmaybe_colorize_sideband(scratch, buf + 1, len);\n+\n+\t\t*sideband_type = SIDEBAND_REMOTE_ERROR;\n+\t\tbreak;\n+\tcase 2:\n+\t\tb = buf + 1;\n+\n+\t\t/*\n+\t\t * Append a suffix to each nonempty line to clear the\n+\t\t * end of the screen line.\n+\t\t *\n+\t\t * The output is accumulated in a buffer and\n+\t\t * each line is printed to stderr using\n+\t\t * write(2) to ensure inter-process atomicity.\n+\t\t */\n+\t\twhile ((brk = strpbrk(b, \"\\n\\r\"))) {\n+\t\t\tint linelen = brk - b;\n+\n+\t\t\tif (!scratch->len)\n+\t\t\t\tstrbuf_addstr(scratch, DISPLAY_PREFIX);\n+\t\t\tif (linelen > 0) {\n+\t\t\t\tmaybe_colorize_sideband(scratch, b, linelen);\n+\t\t\t\tstrbuf_addstr(scratch, suffix);\n \t\t\t}\n-\t\t\tbreak;\n-\t\tcase 1:\n-\t\t\twrite_or_die(out, buf + 1, len);\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tstrbuf_addf(&outbuf, \"%s%s: protocol error: bad band #%d\",\n-\t\t\t\t outbuf.len ? \"\\n\" : \"\", me, band);\n-\t\t\tretval = SIDEBAND_PROTOCOL_ERROR;\n-\t\t\tbreak;\n+\n+\t\t\tstrbuf_addch(scratch, *brk);\n+\t\t\txwrite(2, scratch->buf, scratch->len);\n+\t\t\tstrbuf_reset(scratch);\n+\n+\t\t\tb = brk + 1;\n+\t\t}\n+\n+\t\tif (*b) {\n+\t\t\tstrbuf_addstr(scratch, scratch->len ?\n+\t\t\t\t \"\" : DISPLAY_PREFIX);\n+\t\t\tmaybe_colorize_sideband(scratch, b, strlen(b));\n \t\t}\n+\t\treturn 0;\n+\tcase 1:\n+\t\t*sideband_type = SIDEBAND_PRIMARY;\n+\t\tbreak;\n+\tdefault:\n+\t\tstrbuf_addf(scratch, \"%s%s: protocol error: bad band #%d\",\n+\t\t\t scratch->len ? \"\\n\" : \"\", me, band);\n+\t\t*sideband_type = SIDEBAND_PROTOCOL_ERROR;\n+\t\tbreak;\n \t}\n \n-\tif (outbuf.len) {\n-\t\tstrbuf_addch(&outbuf, '\\n');\n-\t\txwrite(2, outbuf.buf, outbuf.len);\n+cleanup:\n+\tif (scratch->len) {\n+\t\tstrbuf_addch(scratch, '\\n');\n+\t\txwrite(2, scratch->buf, scratch->len);\n \t}\n-\tstrbuf_release(&outbuf);\n-\treturn retval;\n+\tstrbuf_release(scratch);\n+\treturn 1;\n }\n \n /*\ndiff --git a/sideband.h b/sideband.h\nindex 7a8146f161..2c4f021645 100644\n--- a/sideband.h\n+++ b/sideband.h\n@@ -1,10 +1,28 @@\n #ifndef SIDEBAND_H\n #define SIDEBAND_H\n \n-#define SIDEBAND_PROTOCOL_ERROR -2\n-#define SIDEBAND_REMOTE_ERROR -1\n+enum sideband_type {\n+\tSIDEBAND_PROTOCOL_ERROR = -2,\n+\tSIDEBAND_REMOTE_ERROR = -1,\n+\tSIDEBAND_FLUSH = 0,\n+\tSIDEBAND_PRIMARY = 1\n+};\n+\n+/*\n+ * Inspects a multiplexed packet read from the remote. If this packet is a\n+ * progress packet and thus should not be processed by the caller, returns 0.\n+ * Otherwise, returns 1, releases scratch, and sets sideband_type.\n+ *\n+ * If this packet is SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or a\n+ * progress packet, also prints a message to stderr.\n+ *\n+ * scratch must be a struct strbuf allocated by the caller. It is used to store\n+ * progress messages split across multiple packets.\n+ */\n+int demultiplex_sideband(const char *me, char *buf, int len,\n+\t\t\t struct strbuf *scratch,\n+\t\t\t enum sideband_type *sideband_type);\n \n-int recv_sideband(const char *me, int in_stream, int out);\n void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);\n \n #endif\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366871","messageId":"d5b7ea8ecee9c0110756d8d8a0060fc2df195ee7.1547666330.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547666330.git.jonathantanmy@google.com","subject":"[PATCH v3 3/4] {fetch,upload}-pack: sideband v2 fetch response","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-16T19:28:14Z","receivedAt":"2019-01-16T19:28:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, a response to a fetch request has sideband support only while\nthe packfile is being sent, meaning that the server cannot send notices\nuntil the start of the packfile.\n\nExtend sideband support in protocol v2 fetch responses to the whole\nresponse. upload-pack will advertise it if the\nuploadpack.allowsidebandall configuration variable is set, and\nfetch-pack will automatically request it if advertised.\n\nIf the sideband is to be used throughout the whole response, upload-pack\nwill use it to send errors instead of prefixing a PKT-LINE payload with\n\"ERR \".\n\nThis will be tested in a subsequent patch.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n Documentation/technical/protocol-v2.txt | 10 ++++++\n fetch-pack.c | 12 +++++--\n pkt-line.c | 43 ++++++++++++++++++-------\n pkt-line.h | 4 +++\n sideband.c | 5 +++\n sideband.h | 1 +\n upload-pack.c | 16 +++++++++\n 7 files changed, 78 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 09e4e0273f..39b40c0dc1 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -307,6 +307,16 @@ the 'wanted-refs' section in the server's response as explained below.\n \tparticular ref, where is the full name of a ref on the\n \tserver.\n \n+If the 'sideband-all' feature is advertised, the following argument can be\n+included in the client's request:\n+\n+ sideband-all\n+\tInstruct the server to send the whole response multiplexed, not just\n+\tthe packfile section. All non-flush and non-delim PKT-LINE in the\n+\tresponse (not only in the packfile section) will then start with a byte\n+\tindicating its sideband (1, 2, or 3), and the server may send \"0005\\2\"\n+\t(a PKT-LINE of sideband 2 with no payload) as a keepalive packet.\n+\n The response of `fetch` is broken into a number of sections separated by\n delimiter packets (0001), with each section beginning with its section\n header.\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex ee0847e6ae..86e9e18901 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1090,7 +1090,8 @@ static int add_haves(struct fetch_negotiator *negotiator,\n static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\t\t const struct fetch_pack_args *args,\n \t\t\t const struct ref *wants, struct oidset *common,\n-\t\t\t int *haves_to_send, int *in_vain)\n+\t\t\t int *haves_to_send, int *in_vain,\n+\t\t\t int sideband_all)\n {\n \tint ret = 0;\n \tstruct strbuf req_buf = STRBUF_INIT;\n@@ -1116,6 +1117,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tpacket_buf_write(&req_buf, \"include-tag\");\n \tif (prefer_ofs_delta)\n \t\tpacket_buf_write(&req_buf, \"ofs-delta\");\n+\tif (sideband_all)\n+\t\tpacket_buf_write(&req_buf, \"sideband-all\");\n \n \t/* Add shallow-info and deepen request */\n \tif (server_supports_feature(\"fetch\", \"shallow\", 0))\n@@ -1324,6 +1327,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n+\tif (server_supports_feature(\"fetch\", \"sideband-all\", 0)) {\n+\t\treader.use_sideband = 1;\n+\t\treader.me = \"fetch-pack\";\n+\t}\n \n \twhile (state != FETCH_DONE) {\n \t\tswitch (state) {\n@@ -1357,7 +1364,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\tcase FETCH_SEND_REQUEST:\n \t\t\tif (send_fetch_request(&negotiator, fd[1], args, ref,\n \t\t\t\t\t &common,\n-\t\t\t\t\t &haves_to_send, &in_vain))\n+\t\t\t\t\t &haves_to_send, &in_vain,\n+\t\t\t\t\t reader.use_sideband))\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\telse\n \t\t\t\tstate = FETCH_PROCESS_ACKS;\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 321ff632a5..d4b71d3e82 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -449,7 +449,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n \twhile (1) {\n \t\tlen = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX,\n \t\t\t\t 0);\n-\t\tif (!demultiplex_sideband(me, buf, len, &scratch,\n+\t\tif (!demultiplex_sideband(me, buf, len, 0, &scratch,\n \t\t\t\t\t &sideband_type))\n \t\t\tcontinue;\n \t\tswitch (sideband_type) {\n@@ -475,25 +475,43 @@ void packet_reader_init(struct packet_reader *reader, int fd,\n \treader->buffer = packet_buffer;\n \treader->buffer_size = sizeof(packet_buffer);\n \treader->options = options;\n+\treader->me = \"git\";\n }\n \n enum packet_read_status packet_reader_read(struct packet_reader *reader)\n {\n+\tstruct strbuf scratch = STRBUF_INIT;\n+\n \tif (reader->line_peeked) {\n \t\treader->line_peeked = 0;\n \t\treturn reader->status;\n \t}\n \n-\treader->status = packet_read_with_status(reader->fd,\n-\t\t\t\t\t\t &reader->src_buffer,\n-\t\t\t\t\t\t &reader->src_len,\n-\t\t\t\t\t\t reader->buffer,\n-\t\t\t\t\t\t reader->buffer_size,\n-\t\t\t\t\t\t &reader->pktlen,\n-\t\t\t\t\t\t reader->options);\n+\t/*\n+\t * Consume all progress packets until a primary payload packet is\n+\t * received\n+\t */\n+\twhile (1) {\n+\t\tenum sideband_type sideband_type;\n+\t\treader->status = packet_read_with_status(reader->fd,\n+\t\t\t\t\t\t\t &reader->src_buffer,\n+\t\t\t\t\t\t\t &reader->src_len,\n+\t\t\t\t\t\t\t reader->buffer,\n+\t\t\t\t\t\t\t reader->buffer_size,\n+\t\t\t\t\t\t\t &reader->pktlen,\n+\t\t\t\t\t\t\t reader->options);\n+\t\tif (!reader->use_sideband)\n+\t\t\tbreak;\n+\t\tif (demultiplex_sideband(reader->me, reader->buffer,\n+\t\t\t\t\t reader->pktlen, 1, &scratch,\n+\t\t\t\t\t &sideband_type))\n+\t\t\tbreak;\n+\t}\n \n \tif (reader->status == PACKET_READ_NORMAL)\n-\t\treader->line = reader->buffer;\n+\t\t/* Skip the sideband designator if sideband is used */\n+\t\treader->line = reader->use_sideband ?\n+\t\t\treader->buffer + 1 : reader->buffer;\n \telse\n \t\treader->line = NULL;\n \n@@ -515,6 +533,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)\n void packet_writer_init(struct packet_writer *writer, int dest_fd)\n {\n \twriter->dest_fd = dest_fd;\n+\twriter->use_sideband = 0;\n }\n \n void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n@@ -522,7 +541,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tpacket_write_fmt_1(writer->dest_fd, 0, \"\", fmt, args);\n+\tpacket_write_fmt_1(writer->dest_fd, 0,\n+\t\t\t writer->use_sideband ? \"\\001\" : \"\", fmt, args);\n \tva_end(args);\n }\n \n@@ -531,7 +551,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tpacket_write_fmt_1(writer->dest_fd, 0, \"ERR \", fmt, args);\n+\tpacket_write_fmt_1(writer->dest_fd, 0,\n+\t\t\t writer->use_sideband ? \"\\003\" : \"ERR \", fmt, args);\n \tva_end(args);\n }\n \ndiff --git a/pkt-line.h b/pkt-line.h\nindex a8e92a4b63..ad9a4a2cd7 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -162,6 +162,9 @@ struct packet_reader {\n \n \t/* indicates if a line has been peeked */\n \tint line_peeked;\n+\n+\tunsigned use_sideband : 1;\n+\tconst char *me;\n };\n \n /*\n@@ -201,6 +204,7 @@ extern char packet_buffer[LARGE_PACKET_MAX];\n \n struct packet_writer {\n \tint dest_fd;\n+\tunsigned use_sideband : 1;\n };\n \n void packet_writer_init(struct packet_writer *writer, int dest_fd);\ndiff --git a/sideband.c b/sideband.c\nindex e89d677626..6a16feb262 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -114,6 +114,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n #define DUMB_SUFFIX \" \"\n \n int demultiplex_sideband(const char *me, char *buf, int len,\n+\t\t\t int die_on_error,\n \t\t\t struct strbuf *scratch,\n \t\t\t enum sideband_type *sideband_type)\n {\n@@ -144,6 +145,8 @@ int demultiplex_sideband(const char *me, char *buf, int len,\n \tlen--;\n \tswitch (band) {\n \tcase 3:\n+\t\tif (die_on_error)\n+\t\t\tdie(\"remote error: %s\", buf + 1);\n \t\tstrbuf_addf(scratch, \"%s%s\", scratch->len ? \"\\n\" : \"\",\n \t\t\t DISPLAY_PREFIX);\n \t\tmaybe_colorize_sideband(scratch, buf + 1, len);\n@@ -195,6 +198,8 @@ int demultiplex_sideband(const char *me, char *buf, int len,\n \t}\n \n cleanup:\n+\tif (die_on_error && *sideband_type == SIDEBAND_PROTOCOL_ERROR)\n+\t\tdie(\"%s\", scratch->buf);\n \tif (scratch->len) {\n \t\tstrbuf_addch(scratch, '\\n');\n \t\txwrite(2, scratch->buf, scratch->len);\ndiff --git a/sideband.h b/sideband.h\nindex 2c4f021645..227740a58e 100644\n--- a/sideband.h\n+++ b/sideband.h\n@@ -20,6 +20,7 @@ enum sideband_type {\n * progress messages split across multiple packets.\n */\n int demultiplex_sideband(const char *me, char *buf, int len,\n+\t\t\t int die_on_error,\n \t\t\t struct strbuf *scratch,\n \t\t\t enum sideband_type *sideband_type);\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 60a26bbbfd..765b7695d2 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -71,6 +71,8 @@ static int allow_filter;\n static int allow_ref_in_want;\n static struct list_objects_filter_options filter_options;\n \n+static int allow_sideband_all;\n+\n static void reset_timeout(void)\n {\n \talarm(timeout);\n@@ -1046,6 +1048,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \t\tallow_filter = git_config_bool(var, value);\n \t} else if (!strcmp(\"uploadpack.allowrefinwant\", var)) {\n \t\tallow_ref_in_want = git_config_bool(var, value);\n+\t} else if (!strcmp(\"uploadpack.allowsidebandall\", var)) {\n+\t\tallow_sideband_all = git_config_bool(var, value);\n \t}\n \n \tif (current_config_scope() != CONFIG_SCOPE_REPO) {\n@@ -1284,6 +1288,11 @@ static void process_args(struct packet_reader *request,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (allow_sideband_all && !strcmp(arg, \"sideband-all\")) {\n+\t\t\tdata->writer.use_sideband = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\t/* ignore unknown lines maybe? */\n \t\tdie(\"unexpected line: '%s'\", arg);\n \t}\n@@ -1496,6 +1505,7 @@ int upload_pack_advertise(struct repository *r,\n \tif (value) {\n \t\tint allow_filter_value;\n \t\tint allow_ref_in_want;\n+\t\tint allow_sideband_all_value;\n \n \t\tstrbuf_addstr(value, \"shallow\");\n \n@@ -1510,6 +1520,12 @@ int upload_pack_advertise(struct repository *r,\n \t\t\t\t\t &allow_ref_in_want) &&\n \t\t allow_ref_in_want)\n \t\t\tstrbuf_addstr(value, \" ref-in-want\");\n+\n+\t\tif (!repo_config_get_bool(the_repository,\n+\t\t\t\t\t \"uploadpack.allowsidebandall\",\n+\t\t\t\t\t &allow_sideband_all_value) &&\n+\t\t allow_sideband_all_value)\n+\t\t\tstrbuf_addstr(value, \" sideband-all\");\n \t}\n \n \treturn 1;\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366872","messageId":"47a98b67113869aa6a887ced52560c8306e55bc0.1547666330.git.jonathantanmy@google.com","threadId":"50205","inReplyTo":"cover.1547666330.git.jonathantanmy@google.com","subject":"[PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-16T19:28:15Z","receivedAt":"2019-01-16T19:28:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Define a GIT_TEST_SIDEBAND_ALL environment variable meant to be used\nfrom tests. When set to true, this overrides uploadpack.allowsidebandall\nto true, allowing the entire test suite to be run as if this\nconfiguration is in place for all repositories.\n\nAs of this patch, all tests pass whether GIT_TEST_SIDEBAND_ALL is unset\nor set to 1.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n fetch-pack.c | 3 ++-\n t/README | 5 +++++\n t/lib-httpd/apache.conf | 1 +\n t/t5537-fetch-shallow.sh | 3 ++-\n t/t5701-git-serve.sh | 2 +-\n t/t5702-protocol-v2.sh | 4 ++--\n upload-pack.c | 13 ++++++++-----\n 7 files changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 86e9e18901..e98294d918 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1327,7 +1327,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n-\tif (server_supports_feature(\"fetch\", \"sideband-all\", 0)) {\n+\tif (git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 1) &&\n+\t server_supports_feature(\"fetch\", \"sideband-all\", 0)) {\n \t\treader.use_sideband = 1;\n \t\treader.me = \"fetch-pack\";\n \t}\ndiff --git a/t/README b/t/README\nindex 28711cc508..b275c883b8 100644\n--- a/t/README\n+++ b/t/README\n@@ -358,6 +358,11 @@ GIT_TEST_MULTI_PACK_INDEX=, when true, forces the multi-pack-\n index to be written after every 'git repack' command, and overrides the\n 'core.multiPackIndex' setting to true.\n \n+GIT_TEST_SIDEBAND_ALL=, when true, overrides the\n+'uploadpack.allowSidebandAll' setting to true, and when false, forces\n+fetch-pack to not request sideband-all (even if the server advertises\n+sideband-all).\n+\n Naming Tests\n ------------\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 581c010d8f..9a6d368247 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -78,6 +78,7 @@ PassEnv GNUPGHOME\n PassEnv ASAN_OPTIONS\n PassEnv GIT_TRACE\n PassEnv GIT_CONFIG_NOSYSTEM\n+PassEnv GIT_TEST_SIDEBAND_ALL\n \n SetEnvIf Git-Protocol \".*\" GIT_PROTOCOL=$0\n \ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 6faf17e17a..6caf628efa 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -243,7 +243,8 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f\n \t \"$(git -C \"$REPO\" rev-parse HEAD)\" \\\n \t \"$(git -C \"$REPO\" rev-parse HEAD^)\" \\\n \t >\"$HTTPD_ROOT_PATH/one-time-sed\" &&\n-\ttest_must_fail git -C client fetch --depth=1 \"$HTTPD_URL/one_time_sed/repo\" \\\n+\ttest_must_fail env GIT_TEST_SIDEBAND_ALL=0 git -C client \\\n+\t\tfetch --depth=1 \"$HTTPD_URL/one_time_sed/repo\" \\\n \t\tmaster:a_branch &&\n \n \t# Ensure that the one-time-sed script was used.\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex ae79c6bbc0..fe45bf828d 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -14,7 +14,7 @@ test_expect_success 'test capability advertisement' '\n \t0000\n \tEOF\n \n-\tgit serve --advertise-capabilities >out &&\n+\tGIT_TEST_SIDEBAND_ALL=0 git serve --advertise-capabilities >out &&\n \ttest-tool pkt-line unpack actual &&\n \ttest_cmp expect actual\n '\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 0f2b09ebb8..b491c62e3e 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -583,8 +583,8 @@ test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n \ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" git -C http_child \\\n \t\t-c protocol.version=2 \\\n \t\tfetch \"$HTTPD_URL/one_time_sed/http_parent\" 2> err &&\n-\tgrep \"fetch< acknowledgments\" log &&\n-\t! grep \"fetch< ready\" log &&\n+\tgrep \"fetch< .*acknowledgments\" log &&\n+\t! grep \"fetch< .*ready\" log &&\n \ttest_i18ngrep \"expected no other sections to be sent after no .ready.\" err\n '\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 765b7695d2..0c1feccaab 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1288,7 +1288,9 @@ static void process_args(struct packet_reader *request,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (allow_sideband_all && !strcmp(arg, \"sideband-all\")) {\n+\t\tif ((git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 0) ||\n+\t\t allow_sideband_all) &&\n+\t\t !strcmp(arg, \"sideband-all\")) {\n \t\t\tdata->writer.use_sideband = 1;\n \t\t\tcontinue;\n \t\t}\n@@ -1521,10 +1523,11 @@ int upload_pack_advertise(struct repository *r,\n \t\t allow_ref_in_want)\n \t\t\tstrbuf_addstr(value, \" ref-in-want\");\n \n-\t\tif (!repo_config_get_bool(the_repository,\n-\t\t\t\t\t \"uploadpack.allowsidebandall\",\n-\t\t\t\t\t &allow_sideband_all_value) &&\n-\t\t allow_sideband_all_value)\n+\t\tif (git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 0) ||\n+\t\t (!repo_config_get_bool(the_repository,\n+\t\t\t\t\t \"uploadpack.allowsidebandall\",\n+\t\t\t\t\t &allow_sideband_all_value) &&\n+\t\t allow_sideband_all_value))\n \t\t\tstrbuf_addstr(value, \" sideband-all\");\n \t}\n \n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"367013","messageId":"xmqq5zunnk8b.fsf@gitster-ct.c.googlers.com","threadId":"50205","inReplyTo":"cover.1547666330.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 0/4] Sideband the whole fetch v2 response","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-17T19:43:16Z","receivedAt":"2019-01-17T19:43:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> /*\n> + * Inspects a multiplexed packet read from the remote. If this packet is a\n> + * progress packet and thus should not be processed by the caller, returns 0.\n> + * Otherwise, returns 1, releases scratch, and sets sideband_type.\n> *\n> + * If this packet is SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or a\n> + * progress packet, also prints a message to stderr.\n> + *\n> + * scratch must be a struct strbuf allocated by the caller. It is used to store\n> + * progress messages split across multiple packets.\n> */\n> +int demultiplex_sideband(const char *me, char *buf, int len,\n> +\t\t\t int die_on_error,\n> +\t\t\t struct strbuf *scratch,\n> +\t\t\t enum sideband_type *sideband_type);\n\nOK. That sounds like a sensible way to keep the leftover across\ncalls.\n\nWill queue.\n"},{"id":"368070","messageId":"20190129232732.GB218214@google.com","threadId":"50205","inReplyTo":"47a98b67113869aa6a887ced52560c8306e55bc0.1547666330.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-01-29T23:27:32Z","receivedAt":"2019-01-29T23:27:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> --- a/t/lib-httpd/apache.conf\n> +++ b/t/lib-httpd/apache.conf\n> @@ -78,6 +78,7 @@ PassEnv GNUPGHOME\n> PassEnv ASAN_OPTIONS\n> PassEnv GIT_TRACE\n> PassEnv GIT_CONFIG_NOSYSTEM\n> +PassEnv GIT_TEST_SIDEBAND_ALL\n\nThis is producing\n\n [env:warn] [pid 248632] AH01506: PassEnv variable GIT_TEST_SIDEBAND_ALL was undefined\n\nwhen tests are run with \"prove\".\n\nShould we set the envvar unconditionally in lib-httpd.sh?\n\nThanks,\nJonathan\n"},{"id":"369208","messageId":"20190213064906.GA29560@sigill.intra.peff.net","threadId":"50205","inReplyTo":"20190129232732.GB218214@google.com","subject":"Re: [PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-13T06:49:06Z","receivedAt":"2019-02-13T06:49:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 29, 2019 at 03:27:32PM -0800, Jonathan Nieder wrote:\n\n> Jonathan Tan wrote:\n> \n> > --- a/t/lib-httpd/apache.conf\n> > +++ b/t/lib-httpd/apache.conf\n> > @@ -78,6 +78,7 @@ PassEnv GNUPGHOME\n> > PassEnv ASAN_OPTIONS\n> > PassEnv GIT_TRACE\n> > PassEnv GIT_CONFIG_NOSYSTEM\n> > +PassEnv GIT_TEST_SIDEBAND_ALL\n> \n> This is producing\n> \n> [env:warn] [pid 248632] AH01506: PassEnv variable GIT_TEST_SIDEBAND_ALL was undefined\n> \n> when tests are run with \"prove\".\n> \n> Should we set the envvar unconditionally in lib-httpd.sh?\n\nYes, that's the same solution we've had to use for GIT_VALGRIND, etc. I\ndon't think there's an easier way.\n\n-Peff\n"}]}