{"thread":{"id":"50111","subject":"[PATCH] Use packet_reader instead of packet_read_line","startedAt":"2018-12-27T06:53:46Z","lastAt":"2018-12-29T20:05:40Z","messageCount":2,"participants":["Masaya Suzuki"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"365832","messageId":"20181227065210.60817-1-masayasuzuki@google.com","threadId":"50111","inReplyTo":null,"subject":"[PATCH] Use packet_reader instead of packet_read_line","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-27T06:52:10Z","receivedAt":"2018-12-27T06:53:46Z","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 <masayasuzuki@google.com>\n---\n builtin/archive.c      | 19 ++++++-------\n builtin/receive-pack.c | 60 +++++++++++++++++++++--------------------\n fetch-pack.c           | 61 +++++++++++++++++++++++-------------------\n remote-curl.c          | 20 ++++++++------\n send-pack.c            | 37 ++++++++++++-------------\n upload-pack.c          | 38 +++++++++++++-------------\n 6 files changed, 126 insertions(+), 109 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..db1937c14 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -409,29 +409,33 @@ 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-\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 \t\tlast->proto_git = 1;\n \t} else if (maybe_smart &&\n \t\t   last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\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":"365971","messageId":"CAJB1erUeus4p6YK5zrAmP45bDPmfvBGN6PUG3Dh+UwPZ-3AjKQ@mail.gmail.com","threadId":"50111","inReplyTo":"20181227065210.60817-1-masayasuzuki@google.com","subject":"Re: [PATCH] Use packet_reader instead of packet_read_line","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-29T20:05:24Z","receivedAt":"2018-12-29T20:05:40Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"Sorry. This is really broken. I'll fix the tests and send another version.\n"}]}