{"thread":{"id":"50129","subject":"[PATCH v2 0/2] Accept error packets in any context","startedAt":"2018-12-29T21:19:29Z","lastAt":"2019-01-15T18:44:40Z","messageCount":13,"participants":["Masaya Suzuki","Junio C Hamano","Jonathan Tan","Josh Steadmon","Jonathan Nieder"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"365972","messageId":"20181229211915.161686-1-masayasuzuki@google.com","threadId":"50129","inReplyTo":"reply-to=20181127045301.103807-1-masayasuzuki@google.com","subject":"[PATCH v2 0/2] Accept error packets in any context","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-29T21:19:13Z","receivedAt":"2018-12-29T21:19:29Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"This makes it possible for servers to send an error message back to clients in\nan arbitrary situation.\n\nThe first patch was originally sent in [1]. This version includes some fix.\n\nThe second patch was originally sent in [2]. Later, this was cherry-picked in\n[3]. In the discussion in [3], we agreed that this error packet handling should\nbe done only against the Git pack protocol handling code. With this agreement,\nthe patch series sent in [3] is abandoned (according to [4]). This is a patch\nseries based on that agreement on limiting the error packet handling.\n\n[1]: https://public-inbox.org/git/20181227065210.60817-1-masayasuzuki@google.com/\n[2]: https://public-inbox.org/git/20181127045301.103807-1-masayasuzuki@google.com/\n[3]: https://public-inbox.org/git/df7d3659ae5f11d163f1e992f3b9403be709ddb7.1544572142.git.steadmon@google.com/\n[4]: https://public-inbox.org/git/20181213221826.GE37614@google.com/\n\nMasaya Suzuki (2):\n Use packet_reader instead of packet_read_line\n pack-protocol.txt: accept error packets in any context\n\n Documentation/technical/pack-protocol.txt | 20 +++----\n builtin/archive.c | 19 +++----\n builtin/fetch-pack.c | 3 +-\n builtin/receive-pack.c | 62 +++++++++++----------\n builtin/send-pack.c | 3 +-\n connect.c | 3 --\n fetch-pack.c | 65 +++++++++++++----------\n pkt-line.c | 4 ++\n pkt-line.h | 8 ++-\n remote-curl.c | 29 ++++++----\n send-pack.c | 39 +++++++-------\n serve.c | 5 +-\n t/t5703-upload-pack-ref-in-want.sh | 4 +-\n transport.c | 3 +-\n upload-pack.c | 40 +++++++-------\n 15 files changed, 174 insertions(+), 133 deletions(-)\n\n-- \n2.20.1.415.g653613c723-goog\n\n"},{"id":"365973","messageId":"20181229211915.161686-2-masayasuzuki@google.com","threadId":"50129","inReplyTo":"20181229211915.161686-1-masayasuzuki@google.com","subject":"[PATCH v2 1/2] Use packet_reader instead of packet_read_line","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-29T21:19:14Z","receivedAt":"2018-12-29T21:19:40Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"By using and sharing a packet_reader while handling a Git pack protocol\nrequest, the same reader option is used throughout the code. This makes\nit easy to set a reader option to the request parsing code.\n\nSigned-off-by: Masaya Suzuki \n---\n builtin/archive.c | 19 ++++++-------\n builtin/receive-pack.c | 60 +++++++++++++++++++++--------------------\n fetch-pack.c | 61 +++++++++++++++++++++++-------------------\n remote-curl.c | 22 ++++++++++-----\n send-pack.c | 37 ++++++++++++-------------\n upload-pack.c | 38 +++++++++++++-------------\n 6 files changed, 129 insertions(+), 108 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex d2455237c..2fe1f05ca 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -27,10 +27,10 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\t\t const char *remote, const char *exec,\n \t\t\t const char *name_hint)\n {\n-\tchar *buf;\n \tint fd[2], i, rv;\n \tstruct transport *transport;\n \tstruct remote *_remote;\n+\tstruct packet_reader reader;\n \n \t_remote = remote_get(remote);\n \tif (!_remote->url[0])\n@@ -53,18 +53,19 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\tpacket_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n \tpacket_flush(fd[1]);\n \n-\tbuf = packet_read_line(fd[0], NULL);\n-\tif (!buf)\n+\tpacket_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n+\n+\tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n \t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n-\tif (strcmp(buf, \"ACK\")) {\n-\t\tif (starts_with(buf, \"NACK \"))\n-\t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n-\t\tif (starts_with(buf, \"ERR \"))\n-\t\t\tdie(_(\"remote error: %s\"), buf + 4);\n+\tif (strcmp(reader.line, \"ACK\")) {\n+\t\tif (starts_with(reader.line, \"NACK \"))\n+\t\t\tdie(_(\"git archive: NACK %s\"), reader.line + 5);\n+\t\tif (starts_with(reader.line, \"ERR \"))\n+\t\t\tdie(_(\"remote error: %s\"), reader.line + 4);\n \t\tdie(_(\"git archive: protocol error\"));\n \t}\n \n-\tif (packet_read_line(fd[0], NULL))\n+\tif (packet_reader_read(&reader) != PACKET_READ_FLUSH)\n \t\tdie(_(\"git archive: expected a flush\"));\n \n \t/* Now, start reading from fd[0] and spit it out to stdout */\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 33187bd8e..81cc07370 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1569,30 +1569,29 @@ static void queue_commands_from_cert(struct command **tail,\n \t}\n }\n \n-static struct command *read_head_info(struct oid_array *shallow)\n+static struct command *read_head_info(struct packet_reader *reader,\n+\t\t\t\t struct oid_array *shallow)\n {\n \tstruct command *commands = NULL;\n \tstruct command **p = &commands;\n \tfor (;;) {\n-\t\tchar *line;\n-\t\tint len, linelen;\n+\t\tint linelen;\n \n-\t\tline = packet_read_line(0, &len);\n-\t\tif (!line)\n+\t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n \t\t\tbreak;\n \n-\t\tif (len > 8 && starts_with(line, \"shallow \")) {\n+\t\tif (reader->pktlen > 8 && starts_with(reader->line, \"shallow \")) {\n \t\t\tstruct object_id oid;\n-\t\t\tif (get_oid_hex(line + 8, &oid))\n+\t\t\tif (get_oid_hex(reader->line + 8, &oid))\n \t\t\t\tdie(\"protocol error: expected shallow sha, got '%s'\",\n-\t\t\t\t line + 8);\n+\t\t\t\t reader->line + 8);\n \t\t\toid_array_append(shallow, &oid);\n \t\t\tcontinue;\n \t\t}\n \n-\t\tlinelen = strlen(line);\n-\t\tif (linelen < len) {\n-\t\t\tconst char *feature_list = line + linelen + 1;\n+\t\tlinelen = strlen(reader->line);\n+\t\tif (linelen < reader->pktlen) {\n+\t\t\tconst char *feature_list = reader->line + linelen + 1;\n \t\t\tif (parse_feature_request(feature_list, \"report-status\"))\n \t\t\t\treport_status = 1;\n \t\t\tif (parse_feature_request(feature_list, \"side-band-64k\"))\n@@ -1607,28 +1606,32 @@ static struct command *read_head_info(struct oid_array *shallow)\n \t\t\t\tuse_push_options = 1;\n \t\t}\n \n-\t\tif (!strcmp(line, \"push-cert\")) {\n+\t\tif (!strcmp(reader->line, \"push-cert\")) {\n \t\t\tint true_flush = 0;\n-\t\t\tchar certbuf[1024];\n+\t\t\tint saved_options = reader->options;\n+\t\t\treader->options &= ~PACKET_READ_CHOMP_NEWLINE;\n \n \t\t\tfor (;;) {\n-\t\t\t\tlen = packet_read(0, NULL, NULL,\n-\t\t\t\t\t\t certbuf, sizeof(certbuf), 0);\n-\t\t\t\tif (!len) {\n+\t\t\t\tpacket_reader_read(reader);\n+\t\t\t\tif (reader->status == PACKET_READ_FLUSH) {\n \t\t\t\t\ttrue_flush = 1;\n \t\t\t\t\tbreak;\n \t\t\t\t}\n-\t\t\t\tif (!strcmp(certbuf, \"push-cert-end\\n\"))\n+\t\t\t\tif (reader->status != PACKET_READ_NORMAL) {\n+\t\t\t\t\tdie(\"protocol error: got an unexpected packet\");\n+\t\t\t\t}\n+\t\t\t\tif (!strcmp(reader->line, \"push-cert-end\\n\"))\n \t\t\t\t\tbreak; /* end of cert */\n-\t\t\t\tstrbuf_addstr(&push_cert, certbuf);\n+\t\t\t\tstrbuf_addstr(&push_cert, reader->line);\n \t\t\t}\n+\t\t\treader->options = saved_options;\n \n \t\t\tif (true_flush)\n \t\t\t\tbreak;\n \t\t\tcontinue;\n \t\t}\n \n-\t\tp = queue_command(p, line, linelen);\n+\t\tp = queue_command(p, reader->line, linelen);\n \t}\n \n \tif (push_cert.len)\n@@ -1637,18 +1640,14 @@ static struct command *read_head_info(struct oid_array *shallow)\n \treturn commands;\n }\n \n-static void read_push_options(struct string_list *options)\n+static void read_push_options(struct packet_reader *reader,\n+\t\t\t struct string_list *options)\n {\n \twhile (1) {\n-\t\tchar *line;\n-\t\tint len;\n-\n-\t\tline = packet_read_line(0, &len);\n-\n-\t\tif (!line)\n+\t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n \t\t\tbreak;\n \n-\t\tstring_list_append(options, line);\n+\t\tstring_list_append(options, reader->line);\n \t}\n }\n \n@@ -1924,6 +1923,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \tstruct oid_array shallow = OID_ARRAY_INIT;\n \tstruct oid_array ref = OID_ARRAY_INIT;\n \tstruct shallow_info si;\n+\tstruct packet_reader reader;\n \n \tstruct option options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"quiet\")),\n@@ -1986,12 +1986,14 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \tif (advertise_refs)\n \t\treturn 0;\n \n-\tif ((commands = read_head_info(&shallow)) != NULL) {\n+\tpacket_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n+\n+\tif ((commands = read_head_info(&reader, &shallow)) != NULL) {\n \t\tconst char *unpack_status = NULL;\n \t\tstruct string_list push_options = STRING_LIST_INIT_DUP;\n \n \t\tif (use_push_options)\n-\t\t\tread_push_options(&push_options);\n+\t\t\tread_push_options(&reader, &push_options);\n \t\tif (!check_cert_push_options(&push_options)) {\n \t\t\tstruct command *cmd;\n \t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 9691046e6..86790b9bb 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -135,38 +135,42 @@ enum ack_type {\n \tACK_ready\n };\n \n-static void consume_shallow_list(struct fetch_pack_args *args, int fd)\n+static void consume_shallow_list(struct fetch_pack_args *args,\n+\t\t\t\t struct packet_reader *reader)\n {\n \tif (args->stateless_rpc && args->deepen) {\n \t\t/* If we sent a depth we will get back \"duplicate\"\n \t\t * shallow and unshallow commands every time there\n \t\t * is a block of have lines exchanged.\n \t\t */\n-\t\tchar *line;\n-\t\twhile ((line = packet_read_line(fd, NULL))) {\n-\t\t\tif (starts_with(line, \"shallow \"))\n+\t\twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n+\t\t\tif (starts_with(reader->line, \"shallow \"))\n \t\t\t\tcontinue;\n-\t\t\tif (starts_with(line, \"unshallow \"))\n+\t\t\tif (starts_with(reader->line, \"unshallow \"))\n \t\t\t\tcontinue;\n \t\t\tdie(_(\"git fetch-pack: expected shallow list\"));\n \t\t}\n+\t\tif (reader->status != PACKET_READ_FLUSH)\n+\t\t\tdie(_(\"git fetch-pack: expected a flush packet after shallow list\"));\n \t}\n }\n \n-static enum ack_type get_ack(int fd, struct object_id *result_oid)\n+static enum ack_type get_ack(struct packet_reader *reader,\n+\t\t\t struct object_id *result_oid)\n {\n \tint len;\n-\tchar *line = packet_read_line(fd, &len);\n \tconst char *arg;\n \n-\tif (!line)\n+\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n \t\tdie(_(\"git fetch-pack: expected ACK/NAK, got a flush packet\"));\n-\tif (!strcmp(line, \"NAK\"))\n+\tlen = reader->pktlen;\n+\n+\tif (!strcmp(reader->line, \"NAK\"))\n \t\treturn NAK;\n-\tif (skip_prefix(line, \"ACK \", &arg)) {\n+\tif (skip_prefix(reader->line, \"ACK \", &arg)) {\n \t\tif (!get_oid_hex(arg, result_oid)) {\n \t\t\targ += 40;\n-\t\t\tlen -= arg - line;\n+\t\t\tlen -= arg - reader->line;\n \t\t\tif (len < 1)\n \t\t\t\treturn ACK;\n \t\t\tif (strstr(arg, \"continue\"))\n@@ -178,9 +182,9 @@ static enum ack_type get_ack(int fd, struct object_id *result_oid)\n \t\t\treturn ACK;\n \t\t}\n \t}\n-\tif (skip_prefix(line, \"ERR \", &arg))\n+\tif (skip_prefix(reader->line, \"ERR \", &arg))\n \t\tdie(_(\"remote error: %s\"), arg);\n-\tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), line);\n+\tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), reader->line);\n }\n \n static void send_request(struct fetch_pack_args *args,\n@@ -248,10 +252,14 @@ static int find_common(struct fetch_negotiator *negotiator,\n \tint got_ready = 0;\n \tstruct strbuf req_buf = STRBUF_INIT;\n \tsize_t state_len = 0;\n+\tstruct packet_reader reader;\n \n \tif (args->stateless_rpc && multi_ack == 1)\n \t\tdie(_(\"--stateless-rpc requires multi_ack_detailed\"));\n \n+\tpacket_reader_init(&reader, fd[0], NULL, 0,\n+\t\t\t PACKET_READ_CHOMP_NEWLINE);\n+\n \tif (!args->no_dependents) {\n \t\tmark_tips(negotiator, args->negotiation_tips);\n \t\tfor_each_cached_alternate(negotiator, insert_one_alternate_object);\n@@ -336,31 +344,30 @@ static int find_common(struct fetch_negotiator *negotiator,\n \tstate_len = req_buf.len;\n \n \tif (args->deepen) {\n-\t\tchar *line;\n \t\tconst char *arg;\n \t\tstruct object_id oid;\n \n \t\tsend_request(args, fd[1], &req_buf);\n-\t\twhile ((line = packet_read_line(fd[0], NULL))) {\n-\t\t\tif (skip_prefix(line, \"shallow \", &arg)) {\n+\t\twhile (packet_reader_read(&reader) == PACKET_READ_NORMAL) {\n+\t\t\tif (skip_prefix(reader.line, \"shallow \", &arg)) {\n \t\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\t\tdie(_(\"invalid shallow line: %s\"), line);\n+\t\t\t\t\tdie(_(\"invalid shallow line: %s\"), reader.line);\n \t\t\t\tregister_shallow(the_repository, &oid);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (skip_prefix(line, \"unshallow \", &arg)) {\n+\t\t\tif (skip_prefix(reader.line, \"unshallow \", &arg)) {\n \t\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\t\tdie(_(\"invalid unshallow line: %s\"), line);\n+\t\t\t\t\tdie(_(\"invalid unshallow line: %s\"), reader.line);\n \t\t\t\tif (!lookup_object(the_repository, oid.hash))\n-\t\t\t\t\tdie(_(\"object not found: %s\"), line);\n+\t\t\t\t\tdie(_(\"object not found: %s\"), reader.line);\n \t\t\t\t/* make sure that it is parsed as shallow */\n \t\t\t\tif (!parse_object(the_repository, &oid))\n-\t\t\t\t\tdie(_(\"error in object: %s\"), line);\n+\t\t\t\t\tdie(_(\"error in object: %s\"), reader.line);\n \t\t\t\tif (unregister_shallow(&oid))\n-\t\t\t\t\tdie(_(\"no shallow found: %s\"), line);\n+\t\t\t\t\tdie(_(\"no shallow found: %s\"), reader.line);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tdie(_(\"expected shallow/unshallow, got %s\"), line);\n+\t\t\tdie(_(\"expected shallow/unshallow, got %s\"), reader.line);\n \t\t}\n \t} else if (!args->stateless_rpc)\n \t\tsend_request(args, fd[1], &req_buf);\n@@ -397,9 +404,9 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t\tif (!args->stateless_rpc && count == INITIAL_FLUSH)\n \t\t\t\tcontinue;\n \n-\t\t\tconsume_shallow_list(args, fd[0]);\n+\t\t\tconsume_shallow_list(args, &reader);\n \t\t\tdo {\n-\t\t\t\tack = get_ack(fd[0], result_oid);\n+\t\t\t\tack = get_ack(&reader, result_oid);\n \t\t\t\tif (ack)\n \t\t\t\t\tprint_verbose(args, _(\"got %s %d %s\"), \"ack\",\n \t\t\t\t\t\t ack, oid_to_hex(result_oid));\n@@ -469,9 +476,9 @@ static int find_common(struct fetch_negotiator *negotiator,\n \tstrbuf_release(&req_buf);\n \n \tif (!got_ready || !no_done)\n-\t\tconsume_shallow_list(args, fd[0]);\n+\t\tconsume_shallow_list(args, &reader);\n \twhile (flushes || multi_ack) {\n-\t\tint ack = get_ack(fd[0], result_oid);\n+\t\tint ack = get_ack(&reader, result_oid);\n \t\tif (ack) {\n \t\t\tprint_verbose(args, _(\"got %s (%d) %s\"), \"ack\",\n \t\t\t\t ack, oid_to_hex(result_oid));\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1220dffcd..bbd9ba0f3 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -409,28 +409,36 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \tif (maybe_smart &&\n \t (5 <= last->len && last->buf[4] == '#') &&\n \t !strbuf_cmp(&exp, &type)) {\n-\t\tchar *line;\n+\t\tstruct packet_reader reader;\n+\t\tpacket_reader_init(&reader, -1, last->buf, last->len,\n+\t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n \n \t\t/*\n \t\t * smart HTTP response; validate that the service\n \t\t * pkt-line matches our request.\n \t\t */\n-\t\tline = packet_read_line_buf(&last->buf, &last->len, NULL);\n-\t\tif (!line)\n+\t\tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n \t\t\tdie(\"invalid server response; expected service, got flush packet\");\n \n \t\tstrbuf_reset(&exp);\n \t\tstrbuf_addf(&exp, \"# service=%s\", service);\n-\t\tif (strcmp(line, exp.buf))\n-\t\t\tdie(\"invalid server response; got '%s'\", line);\n+\t\tif (strcmp(reader.line, exp.buf))\n+\t\t\tdie(\"invalid server response; got '%s'\", reader.line);\n \t\tstrbuf_release(&exp);\n \n \t\t/* The header can include additional metadata lines, up\n \t\t * until a packet flush marker. Ignore these now, but\n \t\t * in the future we might start to scan them.\n \t\t */\n-\t\twhile (packet_read_line_buf(&last->buf, &last->len, NULL))\n-\t\t\t;\n+\t\tfor (;;) {\n+\t\t\tpacket_reader_read(&reader);\n+\t\t\tif (reader.pktlen <= 0) {\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tlast->buf = reader.src_buffer;\n+\t\tlast->len = reader.src_len;\n \n \t\tlast->proto_git = 1;\n \t} else if (maybe_smart &&\ndiff --git a/send-pack.c b/send-pack.c\nindex f69268677..913645046 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -135,38 +135,36 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc\n \treturn 0;\n }\n \n-static int receive_unpack_status(int in)\n+static int receive_unpack_status(struct packet_reader *reader)\n {\n-\tconst char *line = packet_read_line(in, NULL);\n-\tif (!line)\n+\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n \t\treturn error(_(\"unexpected flush packet while reading remote unpack status\"));\n-\tif (!skip_prefix(line, \"unpack \", &line))\n-\t\treturn error(_(\"unable to parse remote unpack status: %s\"), line);\n-\tif (strcmp(line, \"ok\"))\n-\t\treturn error(_(\"remote unpack failed: %s\"), line);\n+\tif (!skip_prefix(reader->line, \"unpack \", &reader->line))\n+\t\treturn error(_(\"unable to parse remote unpack status: %s\"), reader->line);\n+\tif (strcmp(reader->line, \"ok\"))\n+\t\treturn error(_(\"remote unpack failed: %s\"), reader->line);\n \treturn 0;\n }\n \n-static int receive_status(int in, struct ref *refs)\n+static int receive_status(struct packet_reader *reader, struct ref *refs)\n {\n \tstruct ref *hint;\n \tint ret;\n \n \thint = NULL;\n-\tret = receive_unpack_status(in);\n+\tret = receive_unpack_status(reader);\n \twhile (1) {\n-\t\tchar *refname;\n+\t\tconst char *refname;\n \t\tchar *msg;\n-\t\tchar *line = packet_read_line(in, NULL);\n-\t\tif (!line)\n+\t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n \t\t\tbreak;\n-\t\tif (!starts_with(line, \"ok \") && !starts_with(line, \"ng \")) {\n-\t\t\terror(\"invalid ref status from remote: %s\", line);\n+\t\tif (!starts_with(reader->line, \"ok \") && !starts_with(reader->line, \"ng \")) {\n+\t\t\terror(\"invalid ref status from remote: %s\", reader->line);\n \t\t\tret = -1;\n \t\t\tbreak;\n \t\t}\n \n-\t\trefname = line + 3;\n+\t\trefname = reader->line + 3;\n \t\tmsg = strchr(refname, ' ');\n \t\tif (msg)\n \t\t\t*msg++ = '\\0';\n@@ -187,7 +185,7 @@ static int receive_status(int in, struct ref *refs)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (line[0] == 'o' && line[1] == 'k')\n+\t\tif (reader->line[0] == 'o' && reader->line[1] == 'k')\n \t\t\thint->status = REF_STATUS_OK;\n \t\telse {\n \t\t\thint->status = REF_STATUS_REMOTE_REJECT;\n@@ -390,6 +388,7 @@ int send_pack(struct send_pack_args *args,\n \tint ret;\n \tstruct async demux;\n \tconst char *push_cert_nonce = NULL;\n+\tstruct packet_reader reader;\n \n \t/* Does the other end support the reporting? */\n \tif (server_supports(\"report-status\"))\n@@ -559,6 +558,8 @@ int send_pack(struct send_pack_args *args,\n \t\tin = demux.out;\n \t}\n \n+\tpacket_reader_init(&reader, in, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n+\n \tif (need_pack_data && cmds_sent) {\n \t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n \t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n@@ -573,7 +574,7 @@ int send_pack(struct send_pack_args *args,\n \t\t\t * are failing, and just want the error() side effects.\n \t\t\t */\n \t\t\tif (status_report)\n-\t\t\t\treceive_unpack_status(in);\n+\t\t\t\treceive_unpack_status(&reader);\n \n \t\t\tif (use_sideband) {\n \t\t\t\tclose(demux.out);\n@@ -590,7 +591,7 @@ int send_pack(struct send_pack_args *args,\n \t\tpacket_flush(out);\n \n \tif (status_report && cmds_sent)\n-\t\tret = receive_status(in, remote_refs);\n+\t\tret = receive_status(&reader, remote_refs);\n \telse\n \t\tret = 0;\n \tif (args->stateless_rpc)\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 5e81f1ff2..1638825ee 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -354,7 +354,8 @@ static int ok_to_give_up(const struct object_array *have_obj,\n \t\t\t\t\t min_generation);\n }\n \n-static int get_common_commits(struct object_array *have_obj,\n+static int get_common_commits(struct packet_reader *reader,\n+\t\t\t struct object_array *have_obj,\n \t\t\t struct object_array *want_obj)\n {\n \tstruct object_id oid;\n@@ -366,12 +367,11 @@ static int get_common_commits(struct object_array *have_obj,\n \tsave_commit_buffer = 0;\n \n \tfor (;;) {\n-\t\tchar *line = packet_read_line(0, NULL);\n \t\tconst char *arg;\n \n \t\treset_timeout();\n \n-\t\tif (!line) {\n+\t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL) {\n \t\t\tif (multi_ack == 2 && got_common\n \t\t\t && !got_other && ok_to_give_up(have_obj, want_obj)) {\n \t\t\t\tsent_ready = 1;\n@@ -390,7 +390,7 @@ static int get_common_commits(struct object_array *have_obj,\n \t\t\tgot_other = 0;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (skip_prefix(line, \"have \", &arg)) {\n+\t\tif (skip_prefix(reader->line, \"have \", &arg)) {\n \t\t\tswitch (got_oid(arg, &oid, have_obj)) {\n \t\t\tcase -1: /* they have what we do not */\n \t\t\t\tgot_other = 1;\n@@ -416,7 +416,7 @@ static int get_common_commits(struct object_array *have_obj,\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!strcmp(line, \"done\")) {\n+\t\tif (!strcmp(reader->line, \"done\")) {\n \t\t\tif (have_obj->nr > 0) {\n \t\t\t\tif (multi_ack)\n \t\t\t\t\tpacket_write_fmt(1, \"ACK %s\\n\", last_hex);\n@@ -425,7 +425,7 @@ static int get_common_commits(struct object_array *have_obj,\n \t\t\tpacket_write_fmt(1, \"NAK\\n\");\n \t\t\treturn -1;\n \t\t}\n-\t\tdie(\"git upload-pack: expected SHA1 list, got '%s'\", line);\n+\t\tdie(\"git upload-pack: expected SHA1 list, got '%s'\", reader->line);\n \t}\n }\n \n@@ -826,7 +826,7 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,\n \treturn 0;\n }\n \n-static void receive_needs(struct object_array *want_obj)\n+static void receive_needs(struct packet_reader *reader, struct object_array *want_obj)\n {\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n@@ -840,33 +840,32 @@ static void receive_needs(struct object_array *want_obj)\n \t\tstruct object *o;\n \t\tconst char *features;\n \t\tstruct object_id oid_buf;\n-\t\tchar *line = packet_read_line(0, NULL);\n \t\tconst char *arg;\n \n \t\treset_timeout();\n-\t\tif (!line)\n+\t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n \t\t\tbreak;\n \n-\t\tif (process_shallow(line, &shallows))\n+\t\tif (process_shallow(reader->line, &shallows))\n \t\t\tcontinue;\n-\t\tif (process_deepen(line, &depth))\n+\t\tif (process_deepen(reader->line, &depth))\n \t\t\tcontinue;\n-\t\tif (process_deepen_since(line, &deepen_since, &deepen_rev_list))\n+\t\tif (process_deepen_since(reader->line, &deepen_since, &deepen_rev_list))\n \t\t\tcontinue;\n-\t\tif (process_deepen_not(line, &deepen_not, &deepen_rev_list))\n+\t\tif (process_deepen_not(reader->line, &deepen_not, &deepen_rev_list))\n \t\t\tcontinue;\n \n-\t\tif (skip_prefix(line, \"filter \", &arg)) {\n+\t\tif (skip_prefix(reader->line, \"filter \", &arg)) {\n \t\t\tif (!filter_capability_requested)\n \t\t\t\tdie(\"git upload-pack: filtering capability not negotiated\");\n \t\t\tparse_list_objects_filter(&filter_options, arg);\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (!skip_prefix(line, \"want \", &arg) ||\n+\t\tif (!skip_prefix(reader->line, \"want \", &arg) ||\n \t\t parse_oid_hex(arg, &oid_buf, &features))\n \t\t\tdie(\"git upload-pack: protocol error, \"\n-\t\t\t \"expected to get object ID, not '%s'\", line);\n+\t\t\t \"expected to get object ID, not '%s'\", reader->line);\n \n \t\tif (parse_feature_request(features, \"deepen-relative\"))\n \t\t\tdeepen_relative = 1;\n@@ -1055,6 +1054,7 @@ void upload_pack(struct upload_pack_options *options)\n {\n \tstruct string_list symref = STRING_LIST_INIT_DUP;\n \tstruct object_array want_obj = OBJECT_ARRAY_INIT;\n+\tstruct packet_reader reader;\n \n \tstateless_rpc = options->stateless_rpc;\n \ttimeout = options->timeout;\n@@ -1078,10 +1078,12 @@ void upload_pack(struct upload_pack_options *options)\n \tif (options->advertise_refs)\n \t\treturn;\n \n-\treceive_needs(&want_obj);\n+\tpacket_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n+\n+\treceive_needs(&reader, &want_obj);\n \tif (want_obj.nr) {\n \t\tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n-\t\tget_common_commits(&have_obj, &want_obj);\n+\t\tget_common_commits(&reader, &have_obj, &want_obj);\n \t\tcreate_pack_file(&have_obj, &want_obj);\n \t}\n }\n-- \n2.20.1.415.g653613c723-goog\n\n"},{"id":"365974","messageId":"20181229211915.161686-3-masayasuzuki@google.com","threadId":"50129","inReplyTo":"20181229211915.161686-1-masayasuzuki@google.com","subject":"[PATCH v2 2/2] pack-protocol.txt: accept error packets in any context","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-29T21:19:15Z","receivedAt":"2018-12-29T21:19:40Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"In the Git pack protocol definition, an error packet may appear only in\na certain context. However, servers can face a runtime error (e.g. I/O\nerror) at an arbitrary timing. This patch changes the protocol to allow\nan error packet to be sent instead of any packet.\n\nWithout this protocol spec change, when a server cannot process a\nrequest, there's no way to tell that to a client. Since the server\ncannot produce a valid response, it would be forced to cut a connection\nwithout telling why. With this protocol spec change, the server can be\nmore gentle in this situation. An old client may see these error packets\nas an unexpected packet, but this is not worse than having an unexpected\nEOF.\n\nFollowing this protocol spec change, the error packet handling code is\nmoved to pkt-line.c. Implementation wise, this implementation uses\npkt-line to communicate with a subprocess. Since this is not a part of\nGit protocol, it's possible that a packet that is not supposed to be an\nerror packet is mistakenly parsed as an error packet. This error packet\nhandling is enabled only for the Git pack protocol parsing code\nconsidering this.\n\nSigned-off-by: Masaya Suzuki \n---\n Documentation/technical/pack-protocol.txt | 20 +++++++++++---------\n builtin/archive.c | 6 +++---\n builtin/fetch-pack.c | 3 ++-\n builtin/receive-pack.c | 4 +++-\n builtin/send-pack.c | 3 ++-\n connect.c | 3 ---\n fetch-pack.c | 8 ++++----\n pkt-line.c | 4 ++++\n pkt-line.h | 8 ++++++--\n remote-curl.c | 9 ++++++---\n send-pack.c | 4 +++-\n serve.c | 5 +++--\n t/t5703-upload-pack-ref-in-want.sh | 4 ++--\n transport.c | 3 ++-\n upload-pack.c | 4 +++-\n 15 files changed, 54 insertions(+), 34 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 6ac774d5f..7a2375a55 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate `PKT-LINE(...)`, unless\n otherwise noted the usual pkt-line LF rules apply: the sender SHOULD\n include a LF, but the receiver MUST NOT complain if it is not present.\n \n+An error packet is a special pkt-line that contains an error string.\n+\n+----\n+ error-line = PKT-LINE(\"ERR\" SP explanation-text)\n+----\n+\n+Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY\n+be sent. Once this packet is sent by a client or a server, the data transfer\n+process defined in this protocol is terminated.\n+\n Transports\n ----------\n There are three transports over which the packfile protocol is\n@@ -89,13 +99,6 @@ process on the server side over the Git protocol is this:\n \"0039git-upload-pack /schacon/gitbook.git\\0host=example.com\\0\" |\n nc -v example.com 9418\n \n-If the server refuses the request for some reasons, it could abort\n-gracefully with an error message.\n-\n-----\n- error-line = PKT-LINE(\"ERR\" SP explanation-text)\n-----\n-\n \n SSH Transport\n -------------\n@@ -398,12 +401,11 @@ from the client).\n Then the server will start sending its packfile data.\n \n ----\n- server-response = *ack_multi ack / nak / error-line\n+ server-response = *ack_multi ack / nak\n ack_multi = PKT-LINE(\"ACK\" SP obj-id ack_status)\n ack_status = \"continue\" / \"common\" / \"ready\"\n ack = PKT-LINE(\"ACK\" SP obj-id)\n nak = PKT-LINE(\"NAK\")\n- error-line = PKT-LINE(\"ERR\" SP explanation-text)\n ----\n \n A simple clone may look like this (with no 'have' lines):\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 2fe1f05ca..45d11669a 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -53,15 +53,15 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\tpacket_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n \tpacket_flush(fd[1]);\n \n-\tpacket_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\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 \n \tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n \t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n \tif (strcmp(reader.line, \"ACK\")) {\n \t\tif (starts_with(reader.line, \"NACK \"))\n \t\t\tdie(_(\"git archive: NACK %s\"), reader.line + 5);\n-\t\tif (starts_with(reader.line, \"ERR \"))\n-\t\t\tdie(_(\"remote error: %s\"), reader.line + 4);\n \t\tdie(_(\"git archive: protocol error\"));\n \t}\n \ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 63e69a580..85dbc2af8 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -217,7 +217,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n-\t\t\t PACKET_READ_GENTLE_ON_EOF);\n+\t\t\t PACKET_READ_GENTLE_ON_EOF |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \tswitch (discover_version(&reader)) {\n \tcase protocol_v2:\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 81cc07370..d58b7750b 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1986,7 +1986,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \tif (advertise_refs)\n \t\treturn 0;\n \n-\tpacket_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n+\tpacket_reader_init(&reader, 0, NULL, 0,\n+\t\t\t PACKET_READ_CHOMP_NEWLINE |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \tif ((commands = read_head_info(&reader, &shallow)) != NULL) {\n \t\tconst char *unpack_status = NULL;\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 8e3c7490f..098ebf22d 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -250,7 +250,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n-\t\t\t PACKET_READ_GENTLE_ON_EOF);\n+\t\t\t PACKET_READ_GENTLE_ON_EOF |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \tswitch (discover_version(&reader)) {\n \tcase protocol_v2:\ndiff --git a/connect.c b/connect.c\nindex 24281b608..4813f005a 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -296,7 +296,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n \tstruct ref **orig_list = list;\n \tint len = 0;\n \tenum get_remote_heads_state state = EXPECTING_FIRST_REF;\n-\tconst char *arg;\n \n \t*list = NULL;\n \n@@ -306,8 +305,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n \t\t\tdie_initial_contact(1);\n \t\tcase PACKET_READ_NORMAL:\n \t\t\tlen = reader->pktlen;\n-\t\t\tif (len > 4 && skip_prefix(reader->line, \"ERR \", &arg))\n-\t\t\t\tdie(_(\"remote error: %s\"), arg);\n \t\t\tbreak;\n \t\tcase PACKET_READ_FLUSH:\n \t\t\tstate = EXPECTING_DONE;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 86790b9bb..3f9626dbf 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -182,8 +182,6 @@ static enum ack_type get_ack(struct packet_reader *reader,\n \t\t\treturn ACK;\n \t\t}\n \t}\n-\tif (skip_prefix(reader->line, \"ERR \", &arg))\n-\t\tdie(_(\"remote error: %s\"), arg);\n \tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), reader->line);\n }\n \n@@ -258,7 +256,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\tdie(_(\"--stateless-rpc requires multi_ack_detailed\"));\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n-\t\t\t PACKET_READ_CHOMP_NEWLINE);\n+\t\t\t PACKET_READ_CHOMP_NEWLINE |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \tif (!args->no_dependents) {\n \t\tmark_tips(negotiator, args->negotiation_tips);\n@@ -1358,7 +1357,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct fetch_negotiator negotiator;\n \tfetch_negotiator_init(&negotiator, negotiation_algorithm);\n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n-\t\t\t PACKET_READ_CHOMP_NEWLINE);\n+\t\t\t PACKET_READ_CHOMP_NEWLINE |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \twhile (state != FETCH_DONE) {\n \t\tswitch (state) {\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 04d10bbd0..e70ea6d88 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\treturn PACKET_READ_EOF;\n \t}\n \n+\tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n+\t starts_with(buffer, \"ERR \"))\n+\t\tdie(_(\"remote error: %s\"), buffer + 4);\n+\n \tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n \t len && buffer[len-1] == '\\n')\n \t\tlen--;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 5b28d4347..d7e1dbc04 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -62,9 +62,13 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n *\n * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if\n * present) is removed from the buffer before returning.\n+ *\n+ * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an\n+ * ERR packet.\n */\n-#define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n-#define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n+#define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n+#define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n+#define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n int packet_read(int fd, char **src_buffer, size_t *src_len, char\n \t\t*buffer, unsigned size, int options);\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex bbd9ba0f3..10b50869c 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -204,7 +204,8 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n \n \tpacket_reader_init(&reader, -1, heads->buf, heads->len,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n-\t\t\t PACKET_READ_GENTLE_ON_EOF);\n+\t\t\t PACKET_READ_GENTLE_ON_EOF |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \theads->version = discover_version(&reader);\n \tswitch (heads->version) {\n@@ -411,7 +412,8 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \t !strbuf_cmp(&exp, &type)) {\n \t\tstruct packet_reader reader;\n \t\tpacket_reader_init(&reader, -1, last->buf, last->len,\n-\t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n+\t\t\t\t PACKET_READ_CHOMP_NEWLINE |\n+\t\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \t\t/*\n \t\t * smart HTTP response; validate that the service\n@@ -1182,7 +1184,8 @@ static void proxy_state_init(struct proxy_state *p, const char *service_name,\n \t\tp->headers = curl_slist_append(p->headers, buf.buf);\n \n \tpacket_reader_init(&p->reader, p->in, NULL, 0,\n-\t\t\t PACKET_READ_GENTLE_ON_EOF);\n+\t\t\t PACKET_READ_GENTLE_ON_EOF |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \tstrbuf_release(&buf);\n }\ndiff --git a/send-pack.c b/send-pack.c\nindex 913645046..7b9829f16 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -558,7 +558,9 @@ int send_pack(struct send_pack_args *args,\n \t\tin = demux.out;\n \t}\n \n-\tpacket_reader_init(&reader, in, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n+\tpacket_reader_init(&reader, in, NULL, 0,\n+\t\t\t PACKET_READ_CHOMP_NEWLINE |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \tif (need_pack_data && cmds_sent) {\n \t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\ndiff --git a/serve.c b/serve.c\nindex bda085f09..317256c1a 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -167,7 +167,8 @@ static int process_request(void)\n \n \tpacket_reader_init(&reader, 0, NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n-\t\t\t PACKET_READ_GENTLE_ON_EOF);\n+\t\t\t PACKET_READ_GENTLE_ON_EOF |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \t/*\n \t * Check to see if the client closed their end before sending another\n@@ -175,7 +176,7 @@ static int process_request(void)\n \t */\n \tif (packet_reader_peek(&reader) == PACKET_READ_EOF)\n \t\treturn 1;\n-\treader.options = PACKET_READ_CHOMP_NEWLINE;\n+\treader.options &= ~PACKET_READ_GENTLE_ON_EOF;\n \n \twhile (state != PROCESS_REQUEST_DONE) {\n \t\tswitch (packet_reader_peek(&reader)) {\ndiff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\nindex 3f58f05cb..d2a9d0c12 100755\n--- a/t/t5703-upload-pack-ref-in-want.sh\n+++ b/t/t5703-upload-pack-ref-in-want.sh\n@@ -208,7 +208,7 @@ test_expect_success 'server is initially ahead - no ref in want' '\n \tcp -r \"$LOCAL_PRISTINE\" local &&\n \tinconsistency master 1234567890123456789012345678901234567890 &&\n \ttest_must_fail git -C local fetch 2>err &&\n-\tgrep \"ERR upload-pack: not our ref\" err\n+\tgrep \"fatal: remote error: upload-pack: not our ref\" err\n '\n \n test_expect_success 'server is initially ahead - ref in want' '\n@@ -254,7 +254,7 @@ test_expect_success 'server loses a ref - ref in want' '\n \techo \"s/master/raster/\" >\"$HTTPD_ROOT_PATH/one-time-sed\" &&\n \ttest_must_fail git -C local fetch 2>err &&\n \n-\tgrep \"ERR unknown ref refs/heads/raster\" err\n+\tgrep \"fatal: remote error: unknown ref refs/heads/raster\" err\n '\n \n stop_httpd\ndiff --git a/transport.c b/transport.c\nindex 5a74b609f..12db4251c 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -273,7 +273,8 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \n \tpacket_reader_init(&reader, data->fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n-\t\t\t PACKET_READ_GENTLE_ON_EOF);\n+\t\t\t PACKET_READ_GENTLE_ON_EOF |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \tdata->version = discover_version(&reader);\n \tswitch (data->version) {\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 1638825ee..08b547cf0 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1078,7 +1078,9 @@ void upload_pack(struct upload_pack_options *options)\n \tif (options->advertise_refs)\n \t\treturn;\n \n-\tpacket_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n+\tpacket_reader_init(&reader, 0, NULL, 0,\n+\t\t\t PACKET_READ_CHOMP_NEWLINE |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \treceive_needs(&reader, &want_obj);\n \tif (want_obj.nr) {\n-- \n2.20.1.415.g653613c723-goog\n\n"},{"id":"366121","messageId":"xmqqef9tfikg.fsf@gitster-ct.c.googlers.com","threadId":"50129","inReplyTo":"20181229211915.161686-1-masayasuzuki@google.com","subject":"Re: [PATCH v2 0/2] Accept error packets in any context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-03T23:05:35Z","receivedAt":"2019-01-03T23:05:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Masaya Suzuki writes:\n\n> This makes it possible for servers to send an error message back to clients in\n> an arbitrary situation.\n>\n> The first patch was originally sent in [1]. This version includes some fix.\n>\n> The second patch was originally sent in [2]. Later, this was cherry-picked in\n> [3]. In the discussion in [3], we agreed that this error packet handling should\n> be done only against the Git pack protocol handling code. With this agreement,\n> the patch series sent in [3] is abandoned (according to [4]). This is a patch\n> series based on that agreement on limiting the error packet handling.\n\nIn short, are you shooting js/smart-http-detect-remote-error topic\ndown and replacing it with this one?\n\nAs that topic is not yet in 'next', I am perfectly fine doing that.\nI just want to make sure that is what you meant, as my reading of\n[4] was a bit fuzzy.\n\nThanks.\n\n>\n> [1]: https://public-inbox.org/git/20181227065210.60817-1-masayasuzuki@google.com/\n> [2]: https://public-inbox.org/git/20181127045301.103807-1-masayasuzuki@google.com/\n> [3]: https://public-inbox.org/git/df7d3659ae5f11d163f1e992f3b9403be709ddb7.1544572142.git.steadmon@google.com/\n> [4]: https://public-inbox.org/git/20181213221826.GE37614@google.com/\n>\n> Masaya Suzuki (2):\n> Use packet_reader instead of packet_read_line\n> pack-protocol.txt: accept error packets in any context\n\n"},{"id":"366124","messageId":"CAJB1erU0utjKGtv3LBFT6SEEKCfFRuxGvDtpkKeS3GSC1S89JA@mail.gmail.com","threadId":"50129","inReplyTo":"xmqqef9tfikg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/2] Accept error packets in any context","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-04T00:20:16Z","receivedAt":"2019-01-04T00:20:31Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"On Thu, Jan 3, 2019 at 3:05 PM Junio C Hamano wrote:\n>\n> Masaya Suzuki writes:\n>\n> > This makes it possible for servers to send an error message back to clients in\n> > an arbitrary situation.\n> >\n> > The first patch was originally sent in [1]. This version includes some fix.\n> >\n> > The second patch was originally sent in [2]. Later, this was cherry-picked in\n> > [3]. In the discussion in [3], we agreed that this error packet handling should\n> > be done only against the Git pack protocol handling code. With this agreement,\n> > the patch series sent in [3] is abandoned (according to [4]). This is a patch\n> > series based on that agreement on limiting the error packet handling.\n>\n> In short, are you shooting js/smart-http-detect-remote-error topic\n> down and replacing it with this one?\n>\n> As that topic is not yet in 'next', I am perfectly fine doing that.\n> I just want to make sure that is what you meant, as my reading of\n> [4] was a bit fuzzy.\n\nSorry, I think I referenced a wrong email. [4] was meant to be\nhttps://public-inbox.org/git/20181219233005.GI37614@google.com/. I\nthink he wants to have\nhttps://public-inbox.org/git/20181116084725.GA31603@sigill.intra.peff.net/,\nhttps://public-inbox.org/git/20181116084838.GB31603@sigill.intra.peff.net/,\nand https://public-inbox.org/git/20181116084951.GC31603@sigill.intra.peff.net/\nfor js/smart-http-detect-remote-error.\n"},{"id":"366273","messageId":"20190107190111.227874-1-jonathantanmy@google.com","threadId":"50129","inReplyTo":"20181229211915.161686-2-masayasuzuki@google.com","subject":"Re: [PATCH v2 1/2] Use packet_reader instead of packet_read_line","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-07T19:01:11Z","receivedAt":"2019-01-07T19:01:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> By using and sharing a packet_reader while handling a Git pack protocol\n> request, the same reader option is used throughout the code. This makes\n> it easy to set a reader option to the request parsing code.\n\nI see that packet_read() and packet_read_line_buf() invocations are also\nremoved, so it might be better to use \"Use packet_reader instead of\npacket_read.*\" as the commit title.\n\nThe code looks correct to me - most of the changes are removals of\npacket_read_line(), replaced with a packet_reader that has\nPACKET_READ_CHOMP_NEWLINE. One instance is packet_read(), for which the\ncorresponding reader does not have PACKET_READ_CHOMP_NEWLINE (noted\nbelow); and another instance is packet_read_line_buf(), for which the\ncorresponding reader is instantiated accordingly with the buffer (also\nnoted below).\n\n> -\t\tif (!strcmp(line, \"push-cert\")) {\n> +\t\tif (!strcmp(reader->line, \"push-cert\")) {\n> \t\t\tint true_flush = 0;\n> -\t\t\tchar certbuf[1024];\n> +\t\t\tint saved_options = reader->options;\n> +\t\t\treader->options &= ~PACKET_READ_CHOMP_NEWLINE;\n> \n> \t\t\tfor (;;) {\n> -\t\t\t\tlen = packet_read(0, NULL, NULL,\n> -\t\t\t\t\t\t certbuf, sizeof(certbuf), 0);\n> -\t\t\t\tif (!len) {\n> +\t\t\t\tpacket_reader_read(reader);\n> +\t\t\t\tif (reader->status == PACKET_READ_FLUSH) {\n> \t\t\t\t\ttrue_flush = 1;\n> \t\t\t\t\tbreak;\n> \t\t\t\t}\n> -\t\t\t\tif (!strcmp(certbuf, \"push-cert-end\\n\"))\n> +\t\t\t\tif (reader->status != PACKET_READ_NORMAL) {\n> +\t\t\t\t\tdie(\"protocol error: got an unexpected packet\");\n> +\t\t\t\t}\n> +\t\t\t\tif (!strcmp(reader->line, \"push-cert-end\\n\"))\n> \t\t\t\t\tbreak; /* end of cert */\n> -\t\t\t\tstrbuf_addstr(&push_cert, certbuf);\n> +\t\t\t\tstrbuf_addstr(&push_cert, reader->line);\n> \t\t\t}\n> +\t\t\treader->options = saved_options;\n\nHere, packet_read() is used, so we shouldn't chomp the newline, so this\nis correct.\n\n> -\t\tchar *line;\n> +\t\tstruct packet_reader reader;\n> +\t\tpacket_reader_init(&reader, -1, last->buf, last->len,\n> +\t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n> \n> \t\t/*\n> \t\t * smart HTTP response; validate that the service\n> \t\t * pkt-line matches our request.\n> \t\t */\n> -\t\tline = packet_read_line_buf(&last->buf, &last->len, NULL);\n> -\t\tif (!line)\n> +\t\tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n> \t\t\tdie(\"invalid server response; expected service, got flush packet\");\n> \n> \t\tstrbuf_reset(&exp);\n> \t\tstrbuf_addf(&exp, \"# service=%s\", service);\n> -\t\tif (strcmp(line, exp.buf))\n> -\t\t\tdie(\"invalid server response; got '%s'\", line);\n> +\t\tif (strcmp(reader.line, exp.buf))\n> +\t\t\tdie(\"invalid server response; got '%s'\", reader.line);\n> \t\tstrbuf_release(&exp);\n> \n> \t\t/* The header can include additional metadata lines, up\n> \t\t * until a packet flush marker. Ignore these now, but\n> \t\t * in the future we might start to scan them.\n> \t\t */\n> -\t\twhile (packet_read_line_buf(&last->buf, &last->len, NULL))\n> -\t\t\t;\n> +\t\tfor (;;) {\n> +\t\t\tpacket_reader_read(&reader);\n> +\t\t\tif (reader.pktlen <= 0) {\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tlast->buf = reader.src_buffer;\n> +\t\tlast->len = reader.src_len;\n\nAnd here, packet_reader_init() correctly initializes the packet_reader\nwith the buffer, and we need to know where in the buffer to continue\nafter parsing the additional metadata lines and the packet flush, so we\nassign the state of the reader to last->buf and last->len.\n\n(Incidentally, with this change, there is no longer any usage of\npacket_read_line_buf(), but we can remove that in a subsequent patch.)\n\nIn summary, this looks like a good change. Configuration of reader\nmetadata (file descriptors, input buffers, and flags) is now more\ncentralized, and there are fewer file descriptor constants and variables\n(which all look like ints) strewn around.\n"},{"id":"366281","messageId":"20190107194150.58700-1-jonathantanmy@google.com","threadId":"50129","inReplyTo":"20181229211915.161686-3-masayasuzuki@google.com","subject":"Re: [PATCH v2 2/2] pack-protocol.txt: accept error packets in any context","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-07T19:41:50Z","receivedAt":"2019-01-07T19:41:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"First of all, there was some discussion [1] about its relation with\njs/smart-http-detect-remote-error. I noticed that\njs/smart-http-detect-remote-error has all tests passing even if I remove\nMasaya's patch, so I'm reviewing these patches independently, directly on\nmaster.\n\n[1] https://public-inbox.org/git/CAJB1erU0utjKGtv3LBFT6SEEKCfFRuxGvDtpkKeS3GSC1S89JA@mail.gmail.com/\n\n> In the Git pack protocol definition, an error packet may appear only in\n> a certain context. However, servers can face a runtime error (e.g. I/O\n> error) at an arbitrary timing. This patch changes the protocol to allow\n> an error packet to be sent instead of any packet.\n> \n> Without this protocol spec change, when a server cannot process a\n> request, there's no way to tell that to a client. Since the server\n> cannot produce a valid response, it would be forced to cut a connection\n> without telling why. With this protocol spec change, the server can be\n> more gentle in this situation. An old client may see these error packets\n> as an unexpected packet, but this is not worse than having an unexpected\n> EOF.\n\nThe other thing that happens is that servers send \"ERR\" anyway, even\nthough it is not allowed by the protocol.\n\nThis overall looks like a good direction - this makes explicit something\nthat is already being done.\n\nMy remaining concern is if \"ERR \" could be a non-error packet mistaken\nfor an error one. I glanced through pack-protocol.txt and as far as I\ncan tell, I don't see anything non-error sent by the server that could\nbe prefixed with \"ERR\". (There are push-option and gpg-signature-lines,\nbut those are sent by the client.) Packfiles can contain anything, of\ncourse, but as far as I can tell, they are either sent un-PKT-ed or\npreceded by a sideband (\\1), so they are fine.\n\n> diff --git a/serve.c b/serve.c\n> index bda085f09..317256c1a 100644\n> --- a/serve.c\n> +++ b/serve.c\n> @@ -167,7 +167,8 @@ static int process_request(void)\n> \n> \tpacket_reader_init(&reader, 0, NULL, 0,\n> \t\t\t PACKET_READ_CHOMP_NEWLINE |\n> -\t\t\t PACKET_READ_GENTLE_ON_EOF);\n> +\t\t\t PACKET_READ_GENTLE_ON_EOF |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \t/*\n> \t * Check to see if the client closed their end before sending another\n> @@ -175,7 +176,7 @@ static int process_request(void)\n> \t */\n> \tif (packet_reader_peek(&reader) == PACKET_READ_EOF)\n> \t\treturn 1;\n> -\treader.options = PACKET_READ_CHOMP_NEWLINE;\n> +\treader.options &= ~PACKET_READ_GENTLE_ON_EOF;\n\nHere, the old line is meant to remove PACKET_READ_GENTLE_ON_EOF - the\nnew line is both necessary and clearer.\n\n> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\n> index 3f58f05cb..d2a9d0c12 100755\n> --- a/t/t5703-upload-pack-ref-in-want.sh\n> +++ b/t/t5703-upload-pack-ref-in-want.sh\n> @@ -208,7 +208,7 @@ test_expect_success 'server is initially ahead - no ref in want' '\n> \tcp -r \"$LOCAL_PRISTINE\" local &&\n> \tinconsistency master 1234567890123456789012345678901234567890 &&\n> \ttest_must_fail git -C local fetch 2>err &&\n> -\tgrep \"ERR upload-pack: not our ref\" err\n> +\tgrep \"fatal: remote error: upload-pack: not our ref\" err\n> '\n> \n> test_expect_success 'server is initially ahead - ref in want' '\n> @@ -254,7 +254,7 @@ test_expect_success 'server loses a ref - ref in want' '\n> \techo \"s/master/raster/\" >\"$HTTPD_ROOT_PATH/one-time-sed\" &&\n> \ttest_must_fail git -C local fetch 2>err &&\n> \n> -\tgrep \"ERR unknown ref refs/heads/raster\" err\n> +\tgrep \"fatal: remote error: unknown ref refs/heads/raster\" err\n> '\n\nAnd this shows that we have tests that exercise the new code.\n\nThe rest of the diff is just the addition of the new\nPACKET_READ_DIE_ON_ERR_PACKET, and looks correct.\n"},{"id":"366297","messageId":"20190107223324.GB54613@google.com","threadId":"50129","inReplyTo":"20181229211915.161686-2-masayasuzuki@google.com","subject":"Re: [PATCH v2 1/2] Use packet_reader instead of packet_read_line","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-07T22:33:24Z","receivedAt":"2019-01-07T22:33:34Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.12.29 13:19, Masaya Suzuki wrote:\n> By using and sharing a packet_reader while handling a Git pack protocol\n> request, the same reader option is used throughout the code. This makes\n> it easy to set a reader option to the request parsing code.\n> \n> Signed-off-by: Masaya Suzuki \n> ---\n> builtin/archive.c | 19 ++++++-------\n> builtin/receive-pack.c | 60 +++++++++++++++++++++--------------------\n> fetch-pack.c | 61 +++++++++++++++++++++++-------------------\n> remote-curl.c | 22 ++++++++++-----\n> send-pack.c | 37 ++++++++++++-------------\n> upload-pack.c | 38 +++++++++++++-------------\n> 6 files changed, 129 insertions(+), 108 deletions(-)\n> \n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index d2455237c..2fe1f05ca 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -27,10 +27,10 @@ static int run_remote_archiver(int argc, const char **argv,\n> \t\t\t const char *remote, const char *exec,\n> \t\t\t const char *name_hint)\n> {\n> -\tchar *buf;\n> \tint fd[2], i, rv;\n> \tstruct transport *transport;\n> \tstruct remote *_remote;\n> +\tstruct packet_reader reader;\n> \n> \t_remote = remote_get(remote);\n> \tif (!_remote->url[0])\n> @@ -53,18 +53,19 @@ static int run_remote_archiver(int argc, const char **argv,\n> \t\tpacket_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n> \tpacket_flush(fd[1]);\n> \n> -\tbuf = packet_read_line(fd[0], NULL);\n> -\tif (!buf)\n> +\tpacket_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> +\n> +\tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n\npacket_read_line() can also return NULL if the packet is zero-length, so\nyou may want to add a \"|| reader.pktlen <= 0\" to the condition here (and\nin other places where we were checking that packet_read_line() != NULL)\nto make sure the behavior doesn't change. See discussion on my previous\nattempt[1] to refactor this in builtin/archive.c.\n\n[1]: https://public-inbox.org/git/20180912053519.31085-1-steadmon@google.com/\n\n> \t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n> -\tif (strcmp(buf, \"ACK\")) {\n> -\t\tif (starts_with(buf, \"NACK \"))\n> -\t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n> -\t\tif (starts_with(buf, \"ERR \"))\n> -\t\t\tdie(_(\"remote error: %s\"), buf + 4);\n> +\tif (strcmp(reader.line, \"ACK\")) {\n> +\t\tif (starts_with(reader.line, \"NACK \"))\n> +\t\t\tdie(_(\"git archive: NACK %s\"), reader.line + 5);\n> +\t\tif (starts_with(reader.line, \"ERR \"))\n> +\t\t\tdie(_(\"remote error: %s\"), reader.line + 4);\n> \t\tdie(_(\"git archive: protocol error\"));\n> \t}\n> \n> -\tif (packet_read_line(fd[0], NULL))\n> +\tif (packet_reader_read(&reader) != PACKET_READ_FLUSH)\n\nAnd here you'd want to add \"&& reader.pktlen > 0\". I'll skip pointing\nout further instances of this issue.\n\n\n> \t\tdie(_(\"git archive: expected a flush\"));\n> \n> \t/* Now, start reading from fd[0] and spit it out to stdout */\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 33187bd8e..81cc07370 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1569,30 +1569,29 @@ static void queue_commands_from_cert(struct command **tail,\n> \t}\n> }\n> \n> -static struct command *read_head_info(struct oid_array *shallow)\n> +static struct command *read_head_info(struct packet_reader *reader,\n> +\t\t\t\t struct oid_array *shallow)\n> {\n> \tstruct command *commands = NULL;\n> \tstruct command **p = &commands;\n> \tfor (;;) {\n> -\t\tchar *line;\n> -\t\tint len, linelen;\n> +\t\tint linelen;\n> \n> -\t\tline = packet_read_line(0, &len);\n> -\t\tif (!line)\n> +\t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n> \t\t\tbreak;\n> \n> -\t\tif (len > 8 && starts_with(line, \"shallow \")) {\n> +\t\tif (reader->pktlen > 8 && starts_with(reader->line, \"shallow \")) {\n> \t\t\tstruct object_id oid;\n> -\t\t\tif (get_oid_hex(line + 8, &oid))\n> +\t\t\tif (get_oid_hex(reader->line + 8, &oid))\n> \t\t\t\tdie(\"protocol error: expected shallow sha, got '%s'\",\n> -\t\t\t\t line + 8);\n> +\t\t\t\t reader->line + 8);\n> \t\t\toid_array_append(shallow, &oid);\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tlinelen = strlen(line);\n> -\t\tif (linelen < len) {\n> -\t\t\tconst char *feature_list = line + linelen + 1;\n> +\t\tlinelen = strlen(reader->line);\n> +\t\tif (linelen < reader->pktlen) {\n> +\t\t\tconst char *feature_list = reader->line + linelen + 1;\n> \t\t\tif (parse_feature_request(feature_list, \"report-status\"))\n> \t\t\t\treport_status = 1;\n> \t\t\tif (parse_feature_request(feature_list, \"side-band-64k\"))\n> @@ -1607,28 +1606,32 @@ static struct command *read_head_info(struct oid_array *shallow)\n> \t\t\t\tuse_push_options = 1;\n> \t\t}\n> \n> -\t\tif (!strcmp(line, \"push-cert\")) {\n> +\t\tif (!strcmp(reader->line, \"push-cert\")) {\n> \t\t\tint true_flush = 0;\n> -\t\t\tchar certbuf[1024];\n> +\t\t\tint saved_options = reader->options;\n> +\t\t\treader->options &= ~PACKET_READ_CHOMP_NEWLINE;\n> \n> \t\t\tfor (;;) {\n> -\t\t\t\tlen = packet_read(0, NULL, NULL,\n> -\t\t\t\t\t\t certbuf, sizeof(certbuf), 0);\n> -\t\t\t\tif (!len) {\n> +\t\t\t\tpacket_reader_read(reader);\n> +\t\t\t\tif (reader->status == PACKET_READ_FLUSH) {\n\nSimilarly, here a delim packet would previously have been caught here as\nwell. So it might be better to just check \"reader->pktlen == 0\" rather\nthan looking at the status. But I see in the next \"if\" you're adding a\nnew check with a die(), so I guess you're not intending to preserve the\noriginal behavior here.\n\n\n> \t\t\t\t\ttrue_flush = 1;\n> \t\t\t\t\tbreak;\n> \t\t\t\t}\n> -\t\t\t\tif (!strcmp(certbuf, \"push-cert-end\\n\"))\n> +\t\t\t\tif (reader->status != PACKET_READ_NORMAL) {\n> +\t\t\t\t\tdie(\"protocol error: got an unexpected packet\");\n> +\t\t\t\t}\n> +\t\t\t\tif (!strcmp(reader->line, \"push-cert-end\\n\"))\n> \t\t\t\t\tbreak; /* end of cert */\n> -\t\t\t\tstrbuf_addstr(&push_cert, certbuf);\n> +\t\t\t\tstrbuf_addstr(&push_cert, reader->line);\n> \t\t\t}\n> +\t\t\treader->options = saved_options;\n> \n> \t\t\tif (true_flush)\n> \t\t\t\tbreak;\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tp = queue_command(p, line, linelen);\n> +\t\tp = queue_command(p, reader->line, linelen);\n> \t}\n> \n> \tif (push_cert.len)\n> @@ -1637,18 +1640,14 @@ static struct command *read_head_info(struct oid_array *shallow)\n> \treturn commands;\n> }\n> \n> -static void read_push_options(struct string_list *options)\n> +static void read_push_options(struct packet_reader *reader,\n> +\t\t\t struct string_list *options)\n> {\n> \twhile (1) {\n> -\t\tchar *line;\n> -\t\tint len;\n> -\n> -\t\tline = packet_read_line(0, &len);\n> -\n> -\t\tif (!line)\n> +\t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n> \t\t\tbreak;\n> \n> -\t\tstring_list_append(options, line);\n> +\t\tstring_list_append(options, reader->line);\n> \t}\n> }\n> \n> @@ -1924,6 +1923,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n> \tstruct oid_array shallow = OID_ARRAY_INIT;\n> \tstruct oid_array ref = OID_ARRAY_INIT;\n> \tstruct shallow_info si;\n> +\tstruct packet_reader reader;\n> \n> \tstruct option options[] = {\n> \t\tOPT__QUIET(&quiet, N_(\"quiet\")),\n> @@ -1986,12 +1986,14 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n> \tif (advertise_refs)\n> \t\treturn 0;\n> \n> -\tif ((commands = read_head_info(&shallow)) != NULL) {\n> +\tpacket_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> +\n> +\tif ((commands = read_head_info(&reader, &shallow)) != NULL) {\n> \t\tconst char *unpack_status = NULL;\n> \t\tstruct string_list push_options = STRING_LIST_INIT_DUP;\n> \n> \t\tif (use_push_options)\n> -\t\t\tread_push_options(&push_options);\n> +\t\t\tread_push_options(&reader, &push_options);\n> \t\tif (!check_cert_push_options(&push_options)) {\n> \t\t\tstruct command *cmd;\n> \t\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 9691046e6..86790b9bb 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -135,38 +135,42 @@ enum ack_type {\n> \tACK_ready\n> };\n> \n> -static void consume_shallow_list(struct fetch_pack_args *args, int fd)\n> +static void consume_shallow_list(struct fetch_pack_args *args,\n> +\t\t\t\t struct packet_reader *reader)\n> {\n> \tif (args->stateless_rpc && args->deepen) {\n> \t\t/* If we sent a depth we will get back \"duplicate\"\n> \t\t * shallow and unshallow commands every time there\n> \t\t * is a block of have lines exchanged.\n> \t\t */\n> -\t\tchar *line;\n> -\t\twhile ((line = packet_read_line(fd, NULL))) {\n> -\t\t\tif (starts_with(line, \"shallow \"))\n> +\t\twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n> +\t\t\tif (starts_with(reader->line, \"shallow \"))\n> \t\t\t\tcontinue;\n> -\t\t\tif (starts_with(line, \"unshallow \"))\n> +\t\t\tif (starts_with(reader->line, \"unshallow \"))\n> \t\t\t\tcontinue;\n> \t\t\tdie(_(\"git fetch-pack: expected shallow list\"));\n> \t\t}\n> +\t\tif (reader->status != PACKET_READ_FLUSH)\n> +\t\t\tdie(_(\"git fetch-pack: expected a flush packet after shallow list\"));\n\nAnother behavior change here, as previously we didn't check for a final\nflush packet (unless I'm missing something).\n\n\n> \t}\n> }\n> \n> -static enum ack_type get_ack(int fd, struct object_id *result_oid)\n> +static enum ack_type get_ack(struct packet_reader *reader,\n> +\t\t\t struct object_id *result_oid)\n> {\n> \tint len;\n> -\tchar *line = packet_read_line(fd, &len);\n> \tconst char *arg;\n> \n> -\tif (!line)\n> +\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n> \t\tdie(_(\"git fetch-pack: expected ACK/NAK, got a flush packet\"));\n> -\tif (!strcmp(line, \"NAK\"))\n> +\tlen = reader->pktlen;\n> +\n> +\tif (!strcmp(reader->line, \"NAK\"))\n> \t\treturn NAK;\n> -\tif (skip_prefix(line, \"ACK \", &arg)) {\n> +\tif (skip_prefix(reader->line, \"ACK \", &arg)) {\n> \t\tif (!get_oid_hex(arg, result_oid)) {\n> \t\t\targ += 40;\n> -\t\t\tlen -= arg - line;\n> +\t\t\tlen -= arg - reader->line;\n> \t\t\tif (len < 1)\n> \t\t\t\treturn ACK;\n> \t\t\tif (strstr(arg, \"continue\"))\n> @@ -178,9 +182,9 @@ static enum ack_type get_ack(int fd, struct object_id *result_oid)\n> \t\t\treturn ACK;\n> \t\t}\n> \t}\n> -\tif (skip_prefix(line, \"ERR \", &arg))\n> +\tif (skip_prefix(reader->line, \"ERR \", &arg))\n> \t\tdie(_(\"remote error: %s\"), arg);\n> -\tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), line);\n> +\tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), reader->line);\n> }\n> \n> static void send_request(struct fetch_pack_args *args,\n> @@ -248,10 +252,14 @@ static int find_common(struct fetch_negotiator *negotiator,\n> \tint got_ready = 0;\n> \tstruct strbuf req_buf = STRBUF_INIT;\n> \tsize_t state_len = 0;\n> +\tstruct packet_reader reader;\n> \n> \tif (args->stateless_rpc && multi_ack == 1)\n> \t\tdie(_(\"--stateless-rpc requires multi_ack_detailed\"));\n> \n> +\tpacket_reader_init(&reader, fd[0], NULL, 0,\n> +\t\t\t PACKET_READ_CHOMP_NEWLINE);\n> +\n> \tif (!args->no_dependents) {\n> \t\tmark_tips(negotiator, args->negotiation_tips);\n> \t\tfor_each_cached_alternate(negotiator, insert_one_alternate_object);\n> @@ -336,31 +344,30 @@ static int find_common(struct fetch_negotiator *negotiator,\n> \tstate_len = req_buf.len;\n> \n> \tif (args->deepen) {\n> -\t\tchar *line;\n> \t\tconst char *arg;\n> \t\tstruct object_id oid;\n> \n> \t\tsend_request(args, fd[1], &req_buf);\n> -\t\twhile ((line = packet_read_line(fd[0], NULL))) {\n> -\t\t\tif (skip_prefix(line, \"shallow \", &arg)) {\n> +\t\twhile (packet_reader_read(&reader) == PACKET_READ_NORMAL) {\n> +\t\t\tif (skip_prefix(reader.line, \"shallow \", &arg)) {\n> \t\t\t\tif (get_oid_hex(arg, &oid))\n> -\t\t\t\t\tdie(_(\"invalid shallow line: %s\"), line);\n> +\t\t\t\t\tdie(_(\"invalid shallow line: %s\"), reader.line);\n> \t\t\t\tregister_shallow(the_repository, &oid);\n> \t\t\t\tcontinue;\n> \t\t\t}\n> -\t\t\tif (skip_prefix(line, \"unshallow \", &arg)) {\n> +\t\t\tif (skip_prefix(reader.line, \"unshallow \", &arg)) {\n> \t\t\t\tif (get_oid_hex(arg, &oid))\n> -\t\t\t\t\tdie(_(\"invalid unshallow line: %s\"), line);\n> +\t\t\t\t\tdie(_(\"invalid unshallow line: %s\"), reader.line);\n> \t\t\t\tif (!lookup_object(the_repository, oid.hash))\n> -\t\t\t\t\tdie(_(\"object not found: %s\"), line);\n> +\t\t\t\t\tdie(_(\"object not found: %s\"), reader.line);\n> \t\t\t\t/* make sure that it is parsed as shallow */\n> \t\t\t\tif (!parse_object(the_repository, &oid))\n> -\t\t\t\t\tdie(_(\"error in object: %s\"), line);\n> +\t\t\t\t\tdie(_(\"error in object: %s\"), reader.line);\n> \t\t\t\tif (unregister_shallow(&oid))\n> -\t\t\t\t\tdie(_(\"no shallow found: %s\"), line);\n> +\t\t\t\t\tdie(_(\"no shallow found: %s\"), reader.line);\n> \t\t\t\tcontinue;\n> \t\t\t}\n> -\t\t\tdie(_(\"expected shallow/unshallow, got %s\"), line);\n> +\t\t\tdie(_(\"expected shallow/unshallow, got %s\"), reader.line);\n> \t\t}\n> \t} else if (!args->stateless_rpc)\n> \t\tsend_request(args, fd[1], &req_buf);\n> @@ -397,9 +404,9 @@ static int find_common(struct fetch_negotiator *negotiator,\n> \t\t\tif (!args->stateless_rpc && count == INITIAL_FLUSH)\n> \t\t\t\tcontinue;\n> \n> -\t\t\tconsume_shallow_list(args, fd[0]);\n> +\t\t\tconsume_shallow_list(args, &reader);\n> \t\t\tdo {\n> -\t\t\t\tack = get_ack(fd[0], result_oid);\n> +\t\t\t\tack = get_ack(&reader, result_oid);\n> \t\t\t\tif (ack)\n> \t\t\t\t\tprint_verbose(args, _(\"got %s %d %s\"), \"ack\",\n> \t\t\t\t\t\t ack, oid_to_hex(result_oid));\n> @@ -469,9 +476,9 @@ static int find_common(struct fetch_negotiator *negotiator,\n> \tstrbuf_release(&req_buf);\n> \n> \tif (!got_ready || !no_done)\n> -\t\tconsume_shallow_list(args, fd[0]);\n> +\t\tconsume_shallow_list(args, &reader);\n> \twhile (flushes || multi_ack) {\n> -\t\tint ack = get_ack(fd[0], result_oid);\n> +\t\tint ack = get_ack(&reader, result_oid);\n> \t\tif (ack) {\n> \t\t\tprint_verbose(args, _(\"got %s (%d) %s\"), \"ack\",\n> \t\t\t\t ack, oid_to_hex(result_oid));\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 1220dffcd..bbd9ba0f3 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -409,28 +409,36 @@ static struct discovery *discover_refs(const char *service, int for_push)\n> \tif (maybe_smart &&\n> \t (5 <= last->len && last->buf[4] == '#') &&\n> \t !strbuf_cmp(&exp, &type)) {\n> -\t\tchar *line;\n> +\t\tstruct packet_reader reader;\n> +\t\tpacket_reader_init(&reader, -1, last->buf, last->len,\n> +\t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n> \n> \t\t/*\n> \t\t * smart HTTP response; validate that the service\n> \t\t * pkt-line matches our request.\n> \t\t */\n> -\t\tline = packet_read_line_buf(&last->buf, &last->len, NULL);\n> -\t\tif (!line)\n> +\t\tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n> \t\t\tdie(\"invalid server response; expected service, got flush packet\");\n> \n> \t\tstrbuf_reset(&exp);\n> \t\tstrbuf_addf(&exp, \"# service=%s\", service);\n> -\t\tif (strcmp(line, exp.buf))\n> -\t\t\tdie(\"invalid server response; got '%s'\", line);\n> +\t\tif (strcmp(reader.line, exp.buf))\n> +\t\t\tdie(\"invalid server response; got '%s'\", reader.line);\n> \t\tstrbuf_release(&exp);\n> \n> \t\t/* The header can include additional metadata lines, up\n> \t\t * until a packet flush marker. Ignore these now, but\n> \t\t * in the future we might start to scan them.\n> \t\t */\n> -\t\twhile (packet_read_line_buf(&last->buf, &last->len, NULL))\n> -\t\t\t;\n> +\t\tfor (;;) {\n> +\t\t\tpacket_reader_read(&reader);\n> +\t\t\tif (reader.pktlen <= 0) {\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tlast->buf = reader.src_buffer;\n> +\t\tlast->len = reader.src_len;\n> \n> \t\tlast->proto_git = 1;\n> \t} else if (maybe_smart &&\n> diff --git a/send-pack.c b/send-pack.c\n> index f69268677..913645046 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -135,38 +135,36 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc\n> \treturn 0;\n> }\n> \n> -static int receive_unpack_status(int in)\n> +static int receive_unpack_status(struct packet_reader *reader)\n> {\n> -\tconst char *line = packet_read_line(in, NULL);\n> -\tif (!line)\n> +\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n> \t\treturn error(_(\"unexpected flush packet while reading remote unpack status\"));\n> -\tif (!skip_prefix(line, \"unpack \", &line))\n> -\t\treturn error(_(\"unable to parse remote unpack status: %s\"), line);\n> -\tif (strcmp(line, \"ok\"))\n> -\t\treturn error(_(\"remote unpack failed: %s\"), line);\n> +\tif (!skip_prefix(reader->line, \"unpack \", &reader->line))\n> +\t\treturn error(_(\"unable to parse remote unpack status: %s\"), reader->line);\n> +\tif (strcmp(reader->line, \"ok\"))\n> +\t\treturn error(_(\"remote unpack failed: %s\"), reader->line);\n> \treturn 0;\n> }\n> \n> -static int receive_status(int in, struct ref *refs)\n> +static int receive_status(struct packet_reader *reader, struct ref *refs)\n> {\n> \tstruct ref *hint;\n> \tint ret;\n> \n> \thint = NULL;\n> -\tret = receive_unpack_status(in);\n> +\tret = receive_unpack_status(reader);\n> \twhile (1) {\n> -\t\tchar *refname;\n> +\t\tconst char *refname;\n> \t\tchar *msg;\n> -\t\tchar *line = packet_read_line(in, NULL);\n> -\t\tif (!line)\n> +\t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n> \t\t\tbreak;\n> -\t\tif (!starts_with(line, \"ok \") && !starts_with(line, \"ng \")) {\n> -\t\t\terror(\"invalid ref status from remote: %s\", line);\n> +\t\tif (!starts_with(reader->line, \"ok \") && !starts_with(reader->line, \"ng \")) {\n> +\t\t\terror(\"invalid ref status from remote: %s\", reader->line);\n> \t\t\tret = -1;\n> \t\t\tbreak;\n> \t\t}\n> \n> -\t\trefname = line + 3;\n> +\t\trefname = reader->line + 3;\n> \t\tmsg = strchr(refname, ' ');\n> \t\tif (msg)\n> \t\t\t*msg++ = '\\0';\n> @@ -187,7 +185,7 @@ static int receive_status(int in, struct ref *refs)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tif (line[0] == 'o' && line[1] == 'k')\n> +\t\tif (reader->line[0] == 'o' && reader->line[1] == 'k')\n> \t\t\thint->status = REF_STATUS_OK;\n> \t\telse {\n> \t\t\thint->status = REF_STATUS_REMOTE_REJECT;\n> @@ -390,6 +388,7 @@ int send_pack(struct send_pack_args *args,\n> \tint ret;\n> \tstruct async demux;\n> \tconst char *push_cert_nonce = NULL;\n> +\tstruct packet_reader reader;\n> \n> \t/* Does the other end support the reporting? */\n> \tif (server_supports(\"report-status\"))\n> @@ -559,6 +558,8 @@ int send_pack(struct send_pack_args *args,\n> \t\tin = demux.out;\n> \t}\n> \n> +\tpacket_reader_init(&reader, in, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> +\n> \tif (need_pack_data && cmds_sent) {\n> \t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n> \t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n> @@ -573,7 +574,7 @@ int send_pack(struct send_pack_args *args,\n> \t\t\t * are failing, and just want the error() side effects.\n> \t\t\t */\n> \t\t\tif (status_report)\n> -\t\t\t\treceive_unpack_status(in);\n> +\t\t\t\treceive_unpack_status(&reader);\n> \n> \t\t\tif (use_sideband) {\n> \t\t\t\tclose(demux.out);\n> @@ -590,7 +591,7 @@ int send_pack(struct send_pack_args *args,\n> \t\tpacket_flush(out);\n> \n> \tif (status_report && cmds_sent)\n> -\t\tret = receive_status(in, remote_refs);\n> +\t\tret = receive_status(&reader, remote_refs);\n> \telse\n> \t\tret = 0;\n> \tif (args->stateless_rpc)\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 5e81f1ff2..1638825ee 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -354,7 +354,8 @@ static int ok_to_give_up(const struct object_array *have_obj,\n> \t\t\t\t\t min_generation);\n> }\n> \n> -static int get_common_commits(struct object_array *have_obj,\n> +static int get_common_commits(struct packet_reader *reader,\n> +\t\t\t struct object_array *have_obj,\n> \t\t\t struct object_array *want_obj)\n> {\n> \tstruct object_id oid;\n> @@ -366,12 +367,11 @@ static int get_common_commits(struct object_array *have_obj,\n> \tsave_commit_buffer = 0;\n> \n> \tfor (;;) {\n> -\t\tchar *line = packet_read_line(0, NULL);\n> \t\tconst char *arg;\n> \n> \t\treset_timeout();\n> \n> -\t\tif (!line) {\n> +\t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL) {\n> \t\t\tif (multi_ack == 2 && got_common\n> \t\t\t && !got_other && ok_to_give_up(have_obj, want_obj)) {\n> \t\t\t\tsent_ready = 1;\n> @@ -390,7 +390,7 @@ static int get_common_commits(struct object_array *have_obj,\n> \t\t\tgot_other = 0;\n> \t\t\tcontinue;\n> \t\t}\n> -\t\tif (skip_prefix(line, \"have \", &arg)) {\n> +\t\tif (skip_prefix(reader->line, \"have \", &arg)) {\n> \t\t\tswitch (got_oid(arg, &oid, have_obj)) {\n> \t\t\tcase -1: /* they have what we do not */\n> \t\t\t\tgot_other = 1;\n> @@ -416,7 +416,7 @@ static int get_common_commits(struct object_array *have_obj,\n> \t\t\t}\n> \t\t\tcontinue;\n> \t\t}\n> -\t\tif (!strcmp(line, \"done\")) {\n> +\t\tif (!strcmp(reader->line, \"done\")) {\n> \t\t\tif (have_obj->nr > 0) {\n> \t\t\t\tif (multi_ack)\n> \t\t\t\t\tpacket_write_fmt(1, \"ACK %s\\n\", last_hex);\n> @@ -425,7 +425,7 @@ static int get_common_commits(struct object_array *have_obj,\n> \t\t\tpacket_write_fmt(1, \"NAK\\n\");\n> \t\t\treturn -1;\n> \t\t}\n> -\t\tdie(\"git upload-pack: expected SHA1 list, got '%s'\", line);\n> +\t\tdie(\"git upload-pack: expected SHA1 list, got '%s'\", reader->line);\n> \t}\n> }\n> \n> @@ -826,7 +826,7 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,\n> \treturn 0;\n> }\n> \n> -static void receive_needs(struct object_array *want_obj)\n> +static void receive_needs(struct packet_reader *reader, struct object_array *want_obj)\n> {\n> \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n> \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n> @@ -840,33 +840,32 @@ static void receive_needs(struct object_array *want_obj)\n> \t\tstruct object *o;\n> \t\tconst char *features;\n> \t\tstruct object_id oid_buf;\n> -\t\tchar *line = packet_read_line(0, NULL);\n> \t\tconst char *arg;\n> \n> \t\treset_timeout();\n> -\t\tif (!line)\n> +\t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n> \t\t\tbreak;\n> \n> -\t\tif (process_shallow(line, &shallows))\n> +\t\tif (process_shallow(reader->line, &shallows))\n> \t\t\tcontinue;\n> -\t\tif (process_deepen(line, &depth))\n> +\t\tif (process_deepen(reader->line, &depth))\n> \t\t\tcontinue;\n> -\t\tif (process_deepen_since(line, &deepen_since, &deepen_rev_list))\n> +\t\tif (process_deepen_since(reader->line, &deepen_since, &deepen_rev_list))\n> \t\t\tcontinue;\n> -\t\tif (process_deepen_not(line, &deepen_not, &deepen_rev_list))\n> +\t\tif (process_deepen_not(reader->line, &deepen_not, &deepen_rev_list))\n> \t\t\tcontinue;\n> \n> -\t\tif (skip_prefix(line, \"filter \", &arg)) {\n> +\t\tif (skip_prefix(reader->line, \"filter \", &arg)) {\n> \t\t\tif (!filter_capability_requested)\n> \t\t\t\tdie(\"git upload-pack: filtering capability not negotiated\");\n> \t\t\tparse_list_objects_filter(&filter_options, arg);\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tif (!skip_prefix(line, \"want \", &arg) ||\n> +\t\tif (!skip_prefix(reader->line, \"want \", &arg) ||\n> \t\t parse_oid_hex(arg, &oid_buf, &features))\n> \t\t\tdie(\"git upload-pack: protocol error, \"\n> -\t\t\t \"expected to get object ID, not '%s'\", line);\n> +\t\t\t \"expected to get object ID, not '%s'\", reader->line);\n> \n> \t\tif (parse_feature_request(features, \"deepen-relative\"))\n> \t\t\tdeepen_relative = 1;\n> @@ -1055,6 +1054,7 @@ void upload_pack(struct upload_pack_options *options)\n> {\n> \tstruct string_list symref = STRING_LIST_INIT_DUP;\n> \tstruct object_array want_obj = OBJECT_ARRAY_INIT;\n> +\tstruct packet_reader reader;\n> \n> \tstateless_rpc = options->stateless_rpc;\n> \ttimeout = options->timeout;\n> @@ -1078,10 +1078,12 @@ void upload_pack(struct upload_pack_options *options)\n> \tif (options->advertise_refs)\n> \t\treturn;\n> \n> -\treceive_needs(&want_obj);\n> +\tpacket_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> +\n> +\treceive_needs(&reader, &want_obj);\n> \tif (want_obj.nr) {\n> \t\tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n> -\t\tget_common_commits(&have_obj, &want_obj);\n> +\t\tget_common_commits(&reader, &have_obj, &want_obj);\n> \t\tcreate_pack_file(&have_obj, &want_obj);\n> \t}\n> }\n> -- \n> 2.20.1.415.g653613c723-goog\n> \n\nIn general I think this looks good. If you want this to be a strict\nrefactor with no behavior changes, you'll want to address the comments\nabove. Otherwise, I'd prefer if you note in the commit message where/how\nthe behavior is changing.\n"},{"id":"366298","messageId":"20190107225348.GC54613@google.com","threadId":"50129","inReplyTo":"20181229211915.161686-3-masayasuzuki@google.com","subject":"Re: [PATCH v2 2/2] pack-protocol.txt: accept error packets in any context","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-07T22:53:48Z","receivedAt":"2019-01-07T22:53:57Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.12.29 13:19, Masaya Suzuki wrote:\n> In the Git pack protocol definition, an error packet may appear only in\n> a certain context. However, servers can face a runtime error (e.g. I/O\n> error) at an arbitrary timing. This patch changes the protocol to allow\n> an error packet to be sent instead of any packet.\n> \n> Without this protocol spec change, when a server cannot process a\n> request, there's no way to tell that to a client. Since the server\n> cannot produce a valid response, it would be forced to cut a connection\n> without telling why. With this protocol spec change, the server can be\n> more gentle in this situation. An old client may see these error packets\n> as an unexpected packet, but this is not worse than having an unexpected\n> EOF.\n> \n> Following this protocol spec change, the error packet handling code is\n> moved to pkt-line.c. Implementation wise, this implementation uses\n> pkt-line to communicate with a subprocess. Since this is not a part of\n> Git protocol, it's possible that a packet that is not supposed to be an\n> error packet is mistakenly parsed as an error packet. This error packet\n> handling is enabled only for the Git pack protocol parsing code\n> considering this.\n> \n> Signed-off-by: Masaya Suzuki \n> ---\n> Documentation/technical/pack-protocol.txt | 20 +++++++++++---------\n> builtin/archive.c | 6 +++---\n> builtin/fetch-pack.c | 3 ++-\n> builtin/receive-pack.c | 4 +++-\n> builtin/send-pack.c | 3 ++-\n> connect.c | 3 ---\n> fetch-pack.c | 8 ++++----\n> pkt-line.c | 4 ++++\n> pkt-line.h | 8 ++++++--\n> remote-curl.c | 9 ++++++---\n> send-pack.c | 4 +++-\n> serve.c | 5 +++--\n> t/t5703-upload-pack-ref-in-want.sh | 4 ++--\n> transport.c | 3 ++-\n> upload-pack.c | 4 +++-\n> 15 files changed, 54 insertions(+), 34 deletions(-)\n> \n> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\n> index 6ac774d5f..7a2375a55 100644\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate `PKT-LINE(...)`, unless\n> otherwise noted the usual pkt-line LF rules apply: the sender SHOULD\n> include a LF, but the receiver MUST NOT complain if it is not present.\n> \n> +An error packet is a special pkt-line that contains an error string.\n> +\n> +----\n> + error-line = PKT-LINE(\"ERR\" SP explanation-text)\n> +----\n> +\n> +Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY\n> +be sent. Once this packet is sent by a client or a server, the data transfer\n> +process defined in this protocol is terminated.\n> +\n> Transports\n> ----------\n> There are three transports over which the packfile protocol is\n> @@ -89,13 +99,6 @@ process on the server side over the Git protocol is this:\n> \"0039git-upload-pack /schacon/gitbook.git\\0host=example.com\\0\" |\n> nc -v example.com 9418\n> \n> -If the server refuses the request for some reasons, it could abort\n> -gracefully with an error message.\n> -\n> -----\n> - error-line = PKT-LINE(\"ERR\" SP explanation-text)\n> -----\n> -\n> \n> SSH Transport\n> -------------\n> @@ -398,12 +401,11 @@ from the client).\n> Then the server will start sending its packfile data.\n> \n> ----\n> - server-response = *ack_multi ack / nak / error-line\n> + server-response = *ack_multi ack / nak\n> ack_multi = PKT-LINE(\"ACK\" SP obj-id ack_status)\n> ack_status = \"continue\" / \"common\" / \"ready\"\n> ack = PKT-LINE(\"ACK\" SP obj-id)\n> nak = PKT-LINE(\"NAK\")\n> - error-line = PKT-LINE(\"ERR\" SP explanation-text)\n> ----\n> \n> A simple clone may look like this (with no 'have' lines):\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index 2fe1f05ca..45d11669a 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -53,15 +53,15 @@ static int run_remote_archiver(int argc, const char **argv,\n> \t\tpacket_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n> \tpacket_flush(fd[1]);\n> \n> -\tpacket_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\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> \n> \tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n> \t\tdie(_(\"git archive: expected ACK/NAK, got a flush packet\"));\n> \tif (strcmp(reader.line, \"ACK\")) {\n> \t\tif (starts_with(reader.line, \"NACK \"))\n> \t\t\tdie(_(\"git archive: NACK %s\"), reader.line + 5);\n> -\t\tif (starts_with(reader.line, \"ERR \"))\n> -\t\t\tdie(_(\"remote error: %s\"), reader.line + 4);\n> \t\tdie(_(\"git archive: protocol error\"));\n> \t}\n> \n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index 63e69a580..85dbc2af8 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -217,7 +217,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n> \n> \tpacket_reader_init(&reader, fd[0], NULL, 0,\n> \t\t\t PACKET_READ_CHOMP_NEWLINE |\n> -\t\t\t PACKET_READ_GENTLE_ON_EOF);\n> +\t\t\t PACKET_READ_GENTLE_ON_EOF |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \tswitch (discover_version(&reader)) {\n> \tcase protocol_v2:\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 81cc07370..d58b7750b 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1986,7 +1986,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n> \tif (advertise_refs)\n> \t\treturn 0;\n> \n> -\tpacket_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> +\tpacket_reader_init(&reader, 0, NULL, 0,\n> +\t\t\t PACKET_READ_CHOMP_NEWLINE |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \tif ((commands = read_head_info(&reader, &shallow)) != NULL) {\n> \t\tconst char *unpack_status = NULL;\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index 8e3c7490f..098ebf22d 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -250,7 +250,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n> \n> \tpacket_reader_init(&reader, fd[0], NULL, 0,\n> \t\t\t PACKET_READ_CHOMP_NEWLINE |\n> -\t\t\t PACKET_READ_GENTLE_ON_EOF);\n> +\t\t\t PACKET_READ_GENTLE_ON_EOF |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \tswitch (discover_version(&reader)) {\n> \tcase protocol_v2:\n> diff --git a/connect.c b/connect.c\n> index 24281b608..4813f005a 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -296,7 +296,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n> \tstruct ref **orig_list = list;\n> \tint len = 0;\n> \tenum get_remote_heads_state state = EXPECTING_FIRST_REF;\n> -\tconst char *arg;\n> \n> \t*list = NULL;\n> \n> @@ -306,8 +305,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n> \t\t\tdie_initial_contact(1);\n> \t\tcase PACKET_READ_NORMAL:\n> \t\t\tlen = reader->pktlen;\n> -\t\t\tif (len > 4 && skip_prefix(reader->line, \"ERR \", &arg))\n> -\t\t\t\tdie(_(\"remote error: %s\"), arg);\n> \t\t\tbreak;\n> \t\tcase PACKET_READ_FLUSH:\n> \t\t\tstate = EXPECTING_DONE;\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 86790b9bb..3f9626dbf 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -182,8 +182,6 @@ static enum ack_type get_ack(struct packet_reader *reader,\n> \t\t\treturn ACK;\n> \t\t}\n> \t}\n> -\tif (skip_prefix(reader->line, \"ERR \", &arg))\n> -\t\tdie(_(\"remote error: %s\"), arg);\n> \tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), reader->line);\n> }\n> \n> @@ -258,7 +256,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n> \t\tdie(_(\"--stateless-rpc requires multi_ack_detailed\"));\n> \n> \tpacket_reader_init(&reader, fd[0], NULL, 0,\n> -\t\t\t PACKET_READ_CHOMP_NEWLINE);\n> +\t\t\t PACKET_READ_CHOMP_NEWLINE |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \tif (!args->no_dependents) {\n> \t\tmark_tips(negotiator, args->negotiation_tips);\n> @@ -1358,7 +1357,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n> \tstruct fetch_negotiator negotiator;\n> \tfetch_negotiator_init(&negotiator, negotiation_algorithm);\n> \tpacket_reader_init(&reader, fd[0], NULL, 0,\n> -\t\t\t PACKET_READ_CHOMP_NEWLINE);\n> +\t\t\t PACKET_READ_CHOMP_NEWLINE |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \twhile (state != FETCH_DONE) {\n> \t\tswitch (state) {\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 04d10bbd0..e70ea6d88 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> \t\treturn PACKET_READ_EOF;\n> \t}\n> \n> +\tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n> +\t starts_with(buffer, \"ERR \"))\n> +\t\tdie(_(\"remote error: %s\"), buffer + 4);\n> +\n> \tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n> \t len && buffer[len-1] == '\\n')\n> \t\tlen--;\n> diff --git a/pkt-line.h b/pkt-line.h\n> index 5b28d4347..d7e1dbc04 100644\n> --- a/pkt-line.h\n> +++ b/pkt-line.h\n> @@ -62,9 +62,13 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n> *\n> * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if\n> * present) is removed from the buffer before returning.\n> + *\n> + * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an\n> + * ERR packet.\n> */\n> -#define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n> -#define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n> +#define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n> +#define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n> +#define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n> int packet_read(int fd, char **src_buffer, size_t *src_len, char\n> \t\t*buffer, unsigned size, int options);\n> \n> diff --git a/remote-curl.c b/remote-curl.c\n> index bbd9ba0f3..10b50869c 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -204,7 +204,8 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n> \n> \tpacket_reader_init(&reader, -1, heads->buf, heads->len,\n> \t\t\t PACKET_READ_CHOMP_NEWLINE |\n> -\t\t\t PACKET_READ_GENTLE_ON_EOF);\n> +\t\t\t PACKET_READ_GENTLE_ON_EOF |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \theads->version = discover_version(&reader);\n> \tswitch (heads->version) {\n> @@ -411,7 +412,8 @@ static struct discovery *discover_refs(const char *service, int for_push)\n> \t !strbuf_cmp(&exp, &type)) {\n> \t\tstruct packet_reader reader;\n> \t\tpacket_reader_init(&reader, -1, last->buf, last->len,\n> -\t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n> +\t\t\t\t PACKET_READ_CHOMP_NEWLINE |\n> +\t\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \t\t/*\n> \t\t * smart HTTP response; validate that the service\n> @@ -1182,7 +1184,8 @@ static void proxy_state_init(struct proxy_state *p, const char *service_name,\n> \t\tp->headers = curl_slist_append(p->headers, buf.buf);\n> \n> \tpacket_reader_init(&p->reader, p->in, NULL, 0,\n> -\t\t\t PACKET_READ_GENTLE_ON_EOF);\n> +\t\t\t PACKET_READ_GENTLE_ON_EOF |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \tstrbuf_release(&buf);\n> }\n> diff --git a/send-pack.c b/send-pack.c\n> index 913645046..7b9829f16 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -558,7 +558,9 @@ int send_pack(struct send_pack_args *args,\n> \t\tin = demux.out;\n> \t}\n> \n> -\tpacket_reader_init(&reader, in, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> +\tpacket_reader_init(&reader, in, NULL, 0,\n> +\t\t\t PACKET_READ_CHOMP_NEWLINE |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \tif (need_pack_data && cmds_sent) {\n> \t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n> diff --git a/serve.c b/serve.c\n> index bda085f09..317256c1a 100644\n> --- a/serve.c\n> +++ b/serve.c\n> @@ -167,7 +167,8 @@ static int process_request(void)\n> \n> \tpacket_reader_init(&reader, 0, NULL, 0,\n> \t\t\t PACKET_READ_CHOMP_NEWLINE |\n> -\t\t\t PACKET_READ_GENTLE_ON_EOF);\n> +\t\t\t PACKET_READ_GENTLE_ON_EOF |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \t/*\n> \t * Check to see if the client closed their end before sending another\n> @@ -175,7 +176,7 @@ static int process_request(void)\n> \t */\n> \tif (packet_reader_peek(&reader) == PACKET_READ_EOF)\n> \t\treturn 1;\n> -\treader.options = PACKET_READ_CHOMP_NEWLINE;\n> +\treader.options &= ~PACKET_READ_GENTLE_ON_EOF;\n> \n> \twhile (state != PROCESS_REQUEST_DONE) {\n> \t\tswitch (packet_reader_peek(&reader)) {\n> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\n> index 3f58f05cb..d2a9d0c12 100755\n> --- a/t/t5703-upload-pack-ref-in-want.sh\n> +++ b/t/t5703-upload-pack-ref-in-want.sh\n> @@ -208,7 +208,7 @@ test_expect_success 'server is initially ahead - no ref in want' '\n> \tcp -r \"$LOCAL_PRISTINE\" local &&\n> \tinconsistency master 1234567890123456789012345678901234567890 &&\n> \ttest_must_fail git -C local fetch 2>err &&\n> -\tgrep \"ERR upload-pack: not our ref\" err\n> +\tgrep \"fatal: remote error: upload-pack: not our ref\" err\n> '\n> \n> test_expect_success 'server is initially ahead - ref in want' '\n> @@ -254,7 +254,7 @@ test_expect_success 'server loses a ref - ref in want' '\n> \techo \"s/master/raster/\" >\"$HTTPD_ROOT_PATH/one-time-sed\" &&\n> \ttest_must_fail git -C local fetch 2>err &&\n> \n> -\tgrep \"ERR unknown ref refs/heads/raster\" err\n> +\tgrep \"fatal: remote error: unknown ref refs/heads/raster\" err\n> '\n> \n> stop_httpd\n> diff --git a/transport.c b/transport.c\n> index 5a74b609f..12db4251c 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -273,7 +273,8 @@ static struct ref *handshake(struct transport *transport, int for_push,\n> \n> \tpacket_reader_init(&reader, data->fd[0], NULL, 0,\n> \t\t\t PACKET_READ_CHOMP_NEWLINE |\n> -\t\t\t PACKET_READ_GENTLE_ON_EOF);\n> +\t\t\t PACKET_READ_GENTLE_ON_EOF |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \tdata->version = discover_version(&reader);\n> \tswitch (data->version) {\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 1638825ee..08b547cf0 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -1078,7 +1078,9 @@ void upload_pack(struct upload_pack_options *options)\n> \tif (options->advertise_refs)\n> \t\treturn;\n> \n> -\tpacket_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> +\tpacket_reader_init(&reader, 0, NULL, 0,\n> +\t\t\t PACKET_READ_CHOMP_NEWLINE |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \treceive_needs(&reader, &want_obj);\n> \tif (want_obj.nr) {\n> -- \n> 2.20.1.415.g653613c723-goog\n> \n\nThis all looks good to me.\n\nReviewed-by: Josh Steadmon \n"},{"id":"366385","messageId":"CAJB1erXk8xTCw0McdD-J=kz3s4fnpmhEEUBdzgTbMgQ-ry0wLw@mail.gmail.com","threadId":"50129","inReplyTo":"20190107223324.GB54613@google.com","subject":"Re: [PATCH v2 1/2] Use packet_reader instead of packet_read_line","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-08T23:27:02Z","receivedAt":"2019-01-08T23:27:18Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"On Mon, Jan 7, 2019 at 2:33 PM Josh Steadmon wrote:\n>\n> On 2018.12.29 13:19, Masaya Suzuki wrote:\n> > By using and sharing a packet_reader while handling a Git pack protocol\n> > request, the same reader option is used throughout the code. This makes\n> > it easy to set a reader option to the request parsing code.\n> >\n> > Signed-off-by: Masaya Suzuki \n> > ---\n> > builtin/archive.c | 19 ++++++-------\n> > builtin/receive-pack.c | 60 +++++++++++++++++++++--------------------\n> > fetch-pack.c | 61 +++++++++++++++++++++++-------------------\n> > remote-curl.c | 22 ++++++++++-----\n> > send-pack.c | 37 ++++++++++++-------------\n> > upload-pack.c | 38 +++++++++++++-------------\n> > 6 files changed, 129 insertions(+), 108 deletions(-)\n> >\n> > diff --git a/builtin/archive.c b/builtin/archive.c\n> > index d2455237c..2fe1f05ca 100644\n> > --- a/builtin/archive.c\n> > +++ b/builtin/archive.c\n> > @@ -27,10 +27,10 @@ static int run_remote_archiver(int argc, const char **argv,\n> > const char *remote, const char *exec,\n> > const char *name_hint)\n> > {\n> > - char *buf;\n> > int fd[2], i, rv;\n> > struct transport *transport;\n> > struct remote *_remote;\n> > + struct packet_reader reader;\n> >\n> > _remote = remote_get(remote);\n> > if (!_remote->url[0])\n> > @@ -53,18 +53,19 @@ static int run_remote_archiver(int argc, const char **argv,\n> > packet_write_fmt(fd[1], \"argument %s\\n\", argv[i]);\n> > packet_flush(fd[1]);\n> >\n> > - buf = packet_read_line(fd[0], NULL);\n> > - if (!buf)\n> > + packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);\n> > +\n> > + if (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n>\n> packet_read_line() can also return NULL if the packet is zero-length, so\n> you may want to add a \"|| reader.pktlen <= 0\" to the condition here (and\n> in other places where we were checking that packet_read_line() != NULL)\n> to make sure the behavior doesn't change. See discussion on my previous\n> attempt[1] to refactor this in builtin/archive.c.\n>\n> [1]: https://public-inbox.org/git/20180912053519.31085-1-steadmon@google.com/\n\nThat is interesting. In Documentation/technical/protocol-common.txt,\nit says \"Implementations SHOULD NOT send an empty pkt-line (\"0004\").\".\nThe existing code won't distinguish \"0000\" and \"0004\", while \"0004\" is\nactually not a valid pkt-line. I'll make this patch with no behavior\nchange, but I think we can make that behavior change to stop accepting\n0004 as 0000, and remove the pktlen checks.\n"},{"id":"366700","messageId":"20190115014348.GM162110@google.com","threadId":"50129","inReplyTo":"xmqqef9tfikg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/2] Accept error packets in any context","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-01-15T01:43:48Z","receivedAt":"2019-01-15T01:43:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n> Masaya Suzuki writes:\n\n>> This makes it possible for servers to send an error message back to clients in\n>> an arbitrary situation.\n\nYay! Yes, this should simplify server implementations and user support.\n\n[...]\n> In short, are you shooting js/smart-http-detect-remote-error topic\n> down and replacing it with this one?\n>\n> As that topic is not yet in 'next', I am perfectly fine doing that.\n> I just want to make sure that is what you meant, as my reading of\n> [4] was a bit fuzzy.\n\nJosh, looking at that branch, I see:\n\n remote-curl: die on server-side errors\n\n\tIncludes a test illustrating error handling in the ref\n\tadvertisement. Should that be revived as a standalone patch,\n\twithout the remote-curl.c change?\n\n remote-curl: tighten \"version 2\" check for smart-http\n\n\tA bug fix. We had an analogous bug in the .googlesource.com\n\tservers and it was problematic when experimenting with\n\tprotocol changes using placeholder version numbers YYYYmmdd\n\t(since that starts with a \"2\").\n\n remote-curl: refactor smart-http discovery\n\n\tA nice cleanup.\n\nThanks,\nJonathan\n"},{"id":"366701","messageId":"20190115014950.GN162110@google.com","threadId":"50129","inReplyTo":"20190115014348.GM162110@google.com","subject":"Re: [PATCH v2 0/2] Accept error packets in any context","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-01-15T01:49:50Z","receivedAt":"2019-01-15T01:49:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Junio C Hamano wrote:\n\n>> In short, are you shooting js/smart-http-detect-remote-error topic\n>> down and replacing it with this one?\n>>\n>> As that topic is not yet in 'next', I am perfectly fine doing that.\n>> I just want to make sure that is what you meant, as my reading of\n>> [4] was a bit fuzzy.\n>\n> Josh, looking at that branch, I see:\n>\n> remote-curl: die on server-side errors\n>\n> \tIncludes a test illustrating error handling in the ref\n> \tadvertisement. Should that be revived as a standalone patch,\n> \twithout the remote-curl.c change?\n\nIn fact, it appears I have that locally:\n\n commit 7fe6abcd775dbffd919891d5838f8d125e41f160\n Author: Josh Steadmon \n Date: Tue Dec 11 16:25:18 2018 -0800\n\n lib-httpd, t5551: check server-side HTTP errors\n\n Add an HTTP path to the test server config that returns an ERR pkt-line\n unconditionally. Verify in t5551 that remote-curl properly detects this\n ERR message and aborts.\n\n Signed-off-by: Josh Steadmon \n Signed-off-by: Jeff King \n Signed-off-by: Junio C Hamano \n\nIt's handled fine by the merge 7be333a6362882e8ffceef3de830dbbfafe99995\n(Merge branch 'js/smart-http-detect-remote-error' into pu, 2019-01-11),\nthough. So I think what is in \"pu\" is okay, without shooting any series\ndown. (Alternatively we can combine them into a single five-patch\nseries, if the maintainer prefers.)\n\nThanks,\nJonathan\n"},{"id":"366732","messageId":"xmqqimypsquk.fsf@gitster-ct.c.googlers.com","threadId":"50129","inReplyTo":"20190115014950.GN162110@google.com","subject":"Re: [PATCH v2 0/2] Accept error packets in any context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-15T18:44:35Z","receivedAt":"2019-01-15T18:44:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> It's handled fine by the merge 7be333a6362882e8ffceef3de830dbbfafe99995\n> (Merge branch 'js/smart-http-detect-remote-error' into pu, 2019-01-11),\n> though. So I think what is in \"pu\" is okay, without shooting any series\n> down.\n\nYeah, I think I wiggled that topic branch after getting a\nclarification from Masaya on what is going on, so what's on 'pu'\nshould be in a good shape.\n\n\n"}]}