| Message ID | 20181229211915.161686-2-masayasuzuki@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Accept error packets in any context | expand |
> By using and sharing a packet_reader while handling a Git pack protocol > request, the same reader option is used throughout the code. This makes > it easy to set a reader option to the request parsing code. I see that packet_read() and packet_read_line_buf() invocations are also removed, so it might be better to use "Use packet_reader instead of packet_read.*" as the commit title. The code looks correct to me - most of the changes are removals of packet_read_line(), replaced with a packet_reader that has PACKET_READ_CHOMP_NEWLINE. One instance is packet_read(), for which the corresponding reader does not have PACKET_READ_CHOMP_NEWLINE (noted below); and another instance is packet_read_line_buf(), for which the corresponding reader is instantiated accordingly with the buffer (also noted below). > - if (!strcmp(line, "push-cert")) { > + if (!strcmp(reader->line, "push-cert")) { > int true_flush = 0; > - char certbuf[1024]; > + int saved_options = reader->options; > + reader->options &= ~PACKET_READ_CHOMP_NEWLINE; > > for (;;) { > - len = packet_read(0, NULL, NULL, > - certbuf, sizeof(certbuf), 0); > - if (!len) { > + packet_reader_read(reader); > + if (reader->status == PACKET_READ_FLUSH) { > true_flush = 1; > break; > } > - if (!strcmp(certbuf, "push-cert-end\n")) > + if (reader->status != PACKET_READ_NORMAL) { > + die("protocol error: got an unexpected packet"); > + } > + if (!strcmp(reader->line, "push-cert-end\n")) > break; /* end of cert */ > - strbuf_addstr(&push_cert, certbuf); > + strbuf_addstr(&push_cert, reader->line); > } > + reader->options = saved_options; Here, packet_read() is used, so we shouldn't chomp the newline, so this is correct. > - char *line; > + struct packet_reader reader; > + packet_reader_init(&reader, -1, last->buf, last->len, > + PACKET_READ_CHOMP_NEWLINE); > > /* > * smart HTTP response; validate that the service > * pkt-line matches our request. > */ > - line = packet_read_line_buf(&last->buf, &last->len, NULL); > - if (!line) > + if (packet_reader_read(&reader) != PACKET_READ_NORMAL) > die("invalid server response; expected service, got flush packet"); > > strbuf_reset(&exp); > strbuf_addf(&exp, "# service=%s", service); > - if (strcmp(line, exp.buf)) > - die("invalid server response; got '%s'", line); > + if (strcmp(reader.line, exp.buf)) > + die("invalid server response; got '%s'", reader.line); > strbuf_release(&exp); > > /* The header can include additional metadata lines, up > * until a packet flush marker. Ignore these now, but > * in the future we might start to scan them. > */ > - while (packet_read_line_buf(&last->buf, &last->len, NULL)) > - ; > + for (;;) { > + packet_reader_read(&reader); > + if (reader.pktlen <= 0) { > + break; > + } > + } > + > + last->buf = reader.src_buffer; > + last->len = reader.src_len; And here, packet_reader_init() correctly initializes the packet_reader with the buffer, and we need to know where in the buffer to continue after parsing the additional metadata lines and the packet flush, so we assign the state of the reader to last->buf and last->len. (Incidentally, with this change, there is no longer any usage of packet_read_line_buf(), but we can remove that in a subsequent patch.) In summary, this looks like a good change. Configuration of reader metadata (file descriptors, input buffers, and flags) is now more centralized, and there are fewer file descriptor constants and variables (which all look like ints) strewn around.
On 2018.12.29 13:19, Masaya Suzuki wrote: > By using and sharing a packet_reader while handling a Git pack protocol > request, the same reader option is used throughout the code. This makes > it easy to set a reader option to the request parsing code. > > Signed-off-by: Masaya Suzuki <masayasuzuki@google.com> > --- > builtin/archive.c | 19 ++++++------- > builtin/receive-pack.c | 60 +++++++++++++++++++++-------------------- > fetch-pack.c | 61 +++++++++++++++++++++++------------------- > remote-curl.c | 22 ++++++++++----- > send-pack.c | 37 ++++++++++++------------- > upload-pack.c | 38 +++++++++++++------------- > 6 files changed, 129 insertions(+), 108 deletions(-) > > diff --git a/builtin/archive.c b/builtin/archive.c > index d2455237c..2fe1f05ca 100644 > --- a/builtin/archive.c > +++ b/builtin/archive.c > @@ -27,10 +27,10 @@ static int run_remote_archiver(int argc, const char **argv, > const char *remote, const char *exec, > const char *name_hint) > { > - char *buf; > int fd[2], i, rv; > struct transport *transport; > struct remote *_remote; > + struct packet_reader reader; > > _remote = remote_get(remote); > if (!_remote->url[0]) > @@ -53,18 +53,19 @@ static int run_remote_archiver(int argc, const char **argv, > packet_write_fmt(fd[1], "argument %s\n", argv[i]); > packet_flush(fd[1]); > > - buf = packet_read_line(fd[0], NULL); > - if (!buf) > + packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE); > + > + if (packet_reader_read(&reader) != PACKET_READ_NORMAL) packet_read_line() can also return NULL if the packet is zero-length, so you may want to add a "|| reader.pktlen <= 0" to the condition here (and in other places where we were checking that packet_read_line() != NULL) to make sure the behavior doesn't change. See discussion on my previous attempt[1] to refactor this in builtin/archive.c. [1]: https://public-inbox.org/git/20180912053519.31085-1-steadmon@google.com/ > die(_("git archive: expected ACK/NAK, got a flush packet")); > - if (strcmp(buf, "ACK")) { > - if (starts_with(buf, "NACK ")) > - die(_("git archive: NACK %s"), buf + 5); > - if (starts_with(buf, "ERR ")) > - die(_("remote error: %s"), buf + 4); > + if (strcmp(reader.line, "ACK")) { > + if (starts_with(reader.line, "NACK ")) > + die(_("git archive: NACK %s"), reader.line + 5); > + if (starts_with(reader.line, "ERR ")) > + die(_("remote error: %s"), reader.line + 4); > die(_("git archive: protocol error")); > } > > - if (packet_read_line(fd[0], NULL)) > + if (packet_reader_read(&reader) != PACKET_READ_FLUSH) And here you'd want to add "&& reader.pktlen > 0". I'll skip pointing out further instances of this issue. > die(_("git archive: expected a flush")); > > /* Now, start reading from fd[0] and spit it out to stdout */ > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index 33187bd8e..81cc07370 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -1569,30 +1569,29 @@ static void queue_commands_from_cert(struct command **tail, > } > } > > -static struct command *read_head_info(struct oid_array *shallow) > +static struct command *read_head_info(struct packet_reader *reader, > + struct oid_array *shallow) > { > struct command *commands = NULL; > struct command **p = &commands; > for (;;) { > - char *line; > - int len, linelen; > + int linelen; > > - line = packet_read_line(0, &len); > - if (!line) > + if (packet_reader_read(reader) != PACKET_READ_NORMAL) > break; > > - if (len > 8 && starts_with(line, "shallow ")) { > + if (reader->pktlen > 8 && starts_with(reader->line, "shallow ")) { > struct object_id oid; > - if (get_oid_hex(line + 8, &oid)) > + if (get_oid_hex(reader->line + 8, &oid)) > die("protocol error: expected shallow sha, got '%s'", > - line + 8); > + reader->line + 8); > oid_array_append(shallow, &oid); > continue; > } > > - linelen = strlen(line); > - if (linelen < len) { > - const char *feature_list = line + linelen + 1; > + linelen = strlen(reader->line); > + if (linelen < reader->pktlen) { > + const char *feature_list = reader->line + linelen + 1; > if (parse_feature_request(feature_list, "report-status")) > report_status = 1; > if (parse_feature_request(feature_list, "side-band-64k")) > @@ -1607,28 +1606,32 @@ static struct command *read_head_info(struct oid_array *shallow) > use_push_options = 1; > } > > - if (!strcmp(line, "push-cert")) { > + if (!strcmp(reader->line, "push-cert")) { > int true_flush = 0; > - char certbuf[1024]; > + int saved_options = reader->options; > + reader->options &= ~PACKET_READ_CHOMP_NEWLINE; > > for (;;) { > - len = packet_read(0, NULL, NULL, > - certbuf, sizeof(certbuf), 0); > - if (!len) { > + packet_reader_read(reader); > + if (reader->status == PACKET_READ_FLUSH) { Similarly, here a delim packet would previously have been caught here as well. So it might be better to just check "reader->pktlen == 0" rather than looking at the status. But I see in the next "if" you're adding a new check with a die(), so I guess you're not intending to preserve the original behavior here. > true_flush = 1; > break; > } > - if (!strcmp(certbuf, "push-cert-end\n")) > + if (reader->status != PACKET_READ_NORMAL) { > + die("protocol error: got an unexpected packet"); > + } > + if (!strcmp(reader->line, "push-cert-end\n")) > break; /* end of cert */ > - strbuf_addstr(&push_cert, certbuf); > + strbuf_addstr(&push_cert, reader->line); > } > + reader->options = saved_options; > > if (true_flush) > break; > continue; > } > > - p = queue_command(p, line, linelen); > + p = queue_command(p, reader->line, linelen); > } > > if (push_cert.len) > @@ -1637,18 +1640,14 @@ static struct command *read_head_info(struct oid_array *shallow) > return commands; > } > > -static void read_push_options(struct string_list *options) > +static void read_push_options(struct packet_reader *reader, > + struct string_list *options) > { > while (1) { > - char *line; > - int len; > - > - line = packet_read_line(0, &len); > - > - if (!line) > + if (packet_reader_read(reader) != PACKET_READ_NORMAL) > break; > > - string_list_append(options, line); > + string_list_append(options, reader->line); > } > } > > @@ -1924,6 +1923,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) > struct oid_array shallow = OID_ARRAY_INIT; > struct oid_array ref = OID_ARRAY_INIT; > struct shallow_info si; > + struct packet_reader reader; > > struct option options[] = { > OPT__QUIET(&quiet, N_("quiet")), > @@ -1986,12 +1986,14 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) > if (advertise_refs) > return 0; > > - if ((commands = read_head_info(&shallow)) != NULL) { > + packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE); > + > + if ((commands = read_head_info(&reader, &shallow)) != NULL) { > const char *unpack_status = NULL; > struct string_list push_options = STRING_LIST_INIT_DUP; > > if (use_push_options) > - read_push_options(&push_options); > + read_push_options(&reader, &push_options); > if (!check_cert_push_options(&push_options)) { > struct command *cmd; > for (cmd = commands; cmd; cmd = cmd->next) > diff --git a/fetch-pack.c b/fetch-pack.c > index 9691046e6..86790b9bb 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -135,38 +135,42 @@ enum ack_type { > ACK_ready > }; > > -static void consume_shallow_list(struct fetch_pack_args *args, int fd) > +static void consume_shallow_list(struct fetch_pack_args *args, > + struct packet_reader *reader) > { > if (args->stateless_rpc && args->deepen) { > /* If we sent a depth we will get back "duplicate" > * shallow and unshallow commands every time there > * is a block of have lines exchanged. > */ > - char *line; > - while ((line = packet_read_line(fd, NULL))) { > - if (starts_with(line, "shallow ")) > + while (packet_reader_read(reader) == PACKET_READ_NORMAL) { > + if (starts_with(reader->line, "shallow ")) > continue; > - if (starts_with(line, "unshallow ")) > + if (starts_with(reader->line, "unshallow ")) > continue; > die(_("git fetch-pack: expected shallow list")); > } > + if (reader->status != PACKET_READ_FLUSH) > + die(_("git fetch-pack: expected a flush packet after shallow list")); Another behavior change here, as previously we didn't check for a final flush packet (unless I'm missing something). > } > } > > -static enum ack_type get_ack(int fd, struct object_id *result_oid) > +static enum ack_type get_ack(struct packet_reader *reader, > + struct object_id *result_oid) > { > int len; > - char *line = packet_read_line(fd, &len); > const char *arg; > > - if (!line) > + if (packet_reader_read(reader) != PACKET_READ_NORMAL) > die(_("git fetch-pack: expected ACK/NAK, got a flush packet")); > - if (!strcmp(line, "NAK")) > + len = reader->pktlen; > + > + if (!strcmp(reader->line, "NAK")) > return NAK; > - if (skip_prefix(line, "ACK ", &arg)) { > + if (skip_prefix(reader->line, "ACK ", &arg)) { > if (!get_oid_hex(arg, result_oid)) { > arg += 40; > - len -= arg - line; > + len -= arg - reader->line; > if (len < 1) > return ACK; > if (strstr(arg, "continue")) > @@ -178,9 +182,9 @@ static enum ack_type get_ack(int fd, struct object_id *result_oid) > return ACK; > } > } > - if (skip_prefix(line, "ERR ", &arg)) > + if (skip_prefix(reader->line, "ERR ", &arg)) > die(_("remote error: %s"), arg); > - die(_("git fetch-pack: expected ACK/NAK, got '%s'"), line); > + die(_("git fetch-pack: expected ACK/NAK, got '%s'"), reader->line); > } > > static void send_request(struct fetch_pack_args *args, > @@ -248,10 +252,14 @@ static int find_common(struct fetch_negotiator *negotiator, > int got_ready = 0; > struct strbuf req_buf = STRBUF_INIT; > size_t state_len = 0; > + struct packet_reader reader; > > if (args->stateless_rpc && multi_ack == 1) > die(_("--stateless-rpc requires multi_ack_detailed")); > > + packet_reader_init(&reader, fd[0], NULL, 0, > + PACKET_READ_CHOMP_NEWLINE); > + > if (!args->no_dependents) { > mark_tips(negotiator, args->negotiation_tips); > for_each_cached_alternate(negotiator, insert_one_alternate_object); > @@ -336,31 +344,30 @@ static int find_common(struct fetch_negotiator *negotiator, > state_len = req_buf.len; > > if (args->deepen) { > - char *line; > const char *arg; > struct object_id oid; > > send_request(args, fd[1], &req_buf); > - while ((line = packet_read_line(fd[0], NULL))) { > - if (skip_prefix(line, "shallow ", &arg)) { > + while (packet_reader_read(&reader) == PACKET_READ_NORMAL) { > + if (skip_prefix(reader.line, "shallow ", &arg)) { > if (get_oid_hex(arg, &oid)) > - die(_("invalid shallow line: %s"), line); > + die(_("invalid shallow line: %s"), reader.line); > register_shallow(the_repository, &oid); > continue; > } > - if (skip_prefix(line, "unshallow ", &arg)) { > + if (skip_prefix(reader.line, "unshallow ", &arg)) { > if (get_oid_hex(arg, &oid)) > - die(_("invalid unshallow line: %s"), line); > + die(_("invalid unshallow line: %s"), reader.line); > if (!lookup_object(the_repository, oid.hash)) > - die(_("object not found: %s"), line); > + die(_("object not found: %s"), reader.line); > /* make sure that it is parsed as shallow */ > if (!parse_object(the_repository, &oid)) > - die(_("error in object: %s"), line); > + die(_("error in object: %s"), reader.line); > if (unregister_shallow(&oid)) > - die(_("no shallow found: %s"), line); > + die(_("no shallow found: %s"), reader.line); > continue; > } > - die(_("expected shallow/unshallow, got %s"), line); > + die(_("expected shallow/unshallow, got %s"), reader.line); > } > } else if (!args->stateless_rpc) > send_request(args, fd[1], &req_buf); > @@ -397,9 +404,9 @@ static int find_common(struct fetch_negotiator *negotiator, > if (!args->stateless_rpc && count == INITIAL_FLUSH) > continue; > > - consume_shallow_list(args, fd[0]); > + consume_shallow_list(args, &reader); > do { > - ack = get_ack(fd[0], result_oid); > + ack = get_ack(&reader, result_oid); > if (ack) > print_verbose(args, _("got %s %d %s"), "ack", > ack, oid_to_hex(result_oid)); > @@ -469,9 +476,9 @@ static int find_common(struct fetch_negotiator *negotiator, > strbuf_release(&req_buf); > > if (!got_ready || !no_done) > - consume_shallow_list(args, fd[0]); > + consume_shallow_list(args, &reader); > while (flushes || multi_ack) { > - int ack = get_ack(fd[0], result_oid); > + int ack = get_ack(&reader, result_oid); > if (ack) { > print_verbose(args, _("got %s (%d) %s"), "ack", > ack, oid_to_hex(result_oid)); > diff --git a/remote-curl.c b/remote-curl.c > index 1220dffcd..bbd9ba0f3 100644 > --- a/remote-curl.c > +++ b/remote-curl.c > @@ -409,28 +409,36 @@ static struct discovery *discover_refs(const char *service, int for_push) > if (maybe_smart && > (5 <= last->len && last->buf[4] == '#') && > !strbuf_cmp(&exp, &type)) { > - char *line; > + struct packet_reader reader; > + packet_reader_init(&reader, -1, last->buf, last->len, > + PACKET_READ_CHOMP_NEWLINE); > > /* > * smart HTTP response; validate that the service > * pkt-line matches our request. > */ > - line = packet_read_line_buf(&last->buf, &last->len, NULL); > - if (!line) > + if (packet_reader_read(&reader) != PACKET_READ_NORMAL) > die("invalid server response; expected service, got flush packet"); > > strbuf_reset(&exp); > strbuf_addf(&exp, "# service=%s", service); > - if (strcmp(line, exp.buf)) > - die("invalid server response; got '%s'", line); > + if (strcmp(reader.line, exp.buf)) > + die("invalid server response; got '%s'", reader.line); > strbuf_release(&exp); > > /* The header can include additional metadata lines, up > * until a packet flush marker. Ignore these now, but > * in the future we might start to scan them. > */ > - while (packet_read_line_buf(&last->buf, &last->len, NULL)) > - ; > + for (;;) { > + packet_reader_read(&reader); > + if (reader.pktlen <= 0) { > + break; > + } > + } > + > + last->buf = reader.src_buffer; > + last->len = reader.src_len; > > last->proto_git = 1; > } else if (maybe_smart && > diff --git a/send-pack.c b/send-pack.c > index f69268677..913645046 100644 > --- a/send-pack.c > +++ b/send-pack.c > @@ -135,38 +135,36 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc > return 0; > } > > -static int receive_unpack_status(int in) > +static int receive_unpack_status(struct packet_reader *reader) > { > - const char *line = packet_read_line(in, NULL); > - if (!line) > + if (packet_reader_read(reader) != PACKET_READ_NORMAL) > return error(_("unexpected flush packet while reading remote unpack status")); > - if (!skip_prefix(line, "unpack ", &line)) > - return error(_("unable to parse remote unpack status: %s"), line); > - if (strcmp(line, "ok")) > - return error(_("remote unpack failed: %s"), line); > + if (!skip_prefix(reader->line, "unpack ", &reader->line)) > + return error(_("unable to parse remote unpack status: %s"), reader->line); > + if (strcmp(reader->line, "ok")) > + return error(_("remote unpack failed: %s"), reader->line); > return 0; > } > > -static int receive_status(int in, struct ref *refs) > +static int receive_status(struct packet_reader *reader, struct ref *refs) > { > struct ref *hint; > int ret; > > hint = NULL; > - ret = receive_unpack_status(in); > + ret = receive_unpack_status(reader); > while (1) { > - char *refname; > + const char *refname; > char *msg; > - char *line = packet_read_line(in, NULL); > - if (!line) > + if (packet_reader_read(reader) != PACKET_READ_NORMAL) > break; > - if (!starts_with(line, "ok ") && !starts_with(line, "ng ")) { > - error("invalid ref status from remote: %s", line); > + if (!starts_with(reader->line, "ok ") && !starts_with(reader->line, "ng ")) { > + error("invalid ref status from remote: %s", reader->line); > ret = -1; > break; > } > > - refname = line + 3; > + refname = reader->line + 3; > msg = strchr(refname, ' '); > if (msg) > *msg++ = '\0'; > @@ -187,7 +185,7 @@ static int receive_status(int in, struct ref *refs) > continue; > } > > - if (line[0] == 'o' && line[1] == 'k') > + if (reader->line[0] == 'o' && reader->line[1] == 'k') > hint->status = REF_STATUS_OK; > else { > hint->status = REF_STATUS_REMOTE_REJECT; > @@ -390,6 +388,7 @@ int send_pack(struct send_pack_args *args, > int ret; > struct async demux; > const char *push_cert_nonce = NULL; > + struct packet_reader reader; > > /* Does the other end support the reporting? */ > if (server_supports("report-status")) > @@ -559,6 +558,8 @@ int send_pack(struct send_pack_args *args, > in = demux.out; > } > > + packet_reader_init(&reader, in, NULL, 0, PACKET_READ_CHOMP_NEWLINE); > + > if (need_pack_data && cmds_sent) { > if (pack_objects(out, remote_refs, extra_have, args) < 0) { > for (ref = remote_refs; ref; ref = ref->next) > @@ -573,7 +574,7 @@ int send_pack(struct send_pack_args *args, > * are failing, and just want the error() side effects. > */ > if (status_report) > - receive_unpack_status(in); > + receive_unpack_status(&reader); > > if (use_sideband) { > close(demux.out); > @@ -590,7 +591,7 @@ int send_pack(struct send_pack_args *args, > packet_flush(out); > > if (status_report && cmds_sent) > - ret = receive_status(in, remote_refs); > + ret = receive_status(&reader, remote_refs); > else > ret = 0; > if (args->stateless_rpc) > diff --git a/upload-pack.c b/upload-pack.c > index 5e81f1ff2..1638825ee 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -354,7 +354,8 @@ static int ok_to_give_up(const struct object_array *have_obj, > min_generation); > } > > -static int get_common_commits(struct object_array *have_obj, > +static int get_common_commits(struct packet_reader *reader, > + struct object_array *have_obj, > struct object_array *want_obj) > { > struct object_id oid; > @@ -366,12 +367,11 @@ static int get_common_commits(struct object_array *have_obj, > save_commit_buffer = 0; > > for (;;) { > - char *line = packet_read_line(0, NULL); > const char *arg; > > reset_timeout(); > > - if (!line) { > + if (packet_reader_read(reader) != PACKET_READ_NORMAL) { > if (multi_ack == 2 && got_common > && !got_other && ok_to_give_up(have_obj, want_obj)) { > sent_ready = 1; > @@ -390,7 +390,7 @@ static int get_common_commits(struct object_array *have_obj, > got_other = 0; > continue; > } > - if (skip_prefix(line, "have ", &arg)) { > + if (skip_prefix(reader->line, "have ", &arg)) { > switch (got_oid(arg, &oid, have_obj)) { > case -1: /* they have what we do not */ > got_other = 1; > @@ -416,7 +416,7 @@ static int get_common_commits(struct object_array *have_obj, > } > continue; > } > - if (!strcmp(line, "done")) { > + if (!strcmp(reader->line, "done")) { > if (have_obj->nr > 0) { > if (multi_ack) > packet_write_fmt(1, "ACK %s\n", last_hex); > @@ -425,7 +425,7 @@ static int get_common_commits(struct object_array *have_obj, > packet_write_fmt(1, "NAK\n"); > return -1; > } > - die("git upload-pack: expected SHA1 list, got '%s'", line); > + die("git upload-pack: expected SHA1 list, got '%s'", reader->line); > } > } > > @@ -826,7 +826,7 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not, > return 0; > } > > -static void receive_needs(struct object_array *want_obj) > +static void receive_needs(struct packet_reader *reader, struct object_array *want_obj) > { > struct object_array shallows = OBJECT_ARRAY_INIT; > struct string_list deepen_not = STRING_LIST_INIT_DUP; > @@ -840,33 +840,32 @@ static void receive_needs(struct object_array *want_obj) > struct object *o; > const char *features; > struct object_id oid_buf; > - char *line = packet_read_line(0, NULL); > const char *arg; > > reset_timeout(); > - if (!line) > + if (packet_reader_read(reader) != PACKET_READ_NORMAL) > break; > > - if (process_shallow(line, &shallows)) > + if (process_shallow(reader->line, &shallows)) > continue; > - if (process_deepen(line, &depth)) > + if (process_deepen(reader->line, &depth)) > continue; > - if (process_deepen_since(line, &deepen_since, &deepen_rev_list)) > + if (process_deepen_since(reader->line, &deepen_since, &deepen_rev_list)) > continue; > - if (process_deepen_not(line, &deepen_not, &deepen_rev_list)) > + if (process_deepen_not(reader->line, &deepen_not, &deepen_rev_list)) > continue; > > - if (skip_prefix(line, "filter ", &arg)) { > + if (skip_prefix(reader->line, "filter ", &arg)) { > if (!filter_capability_requested) > die("git upload-pack: filtering capability not negotiated"); > parse_list_objects_filter(&filter_options, arg); > continue; > } > > - if (!skip_prefix(line, "want ", &arg) || > + if (!skip_prefix(reader->line, "want ", &arg) || > parse_oid_hex(arg, &oid_buf, &features)) > die("git upload-pack: protocol error, " > - "expected to get object ID, not '%s'", line); > + "expected to get object ID, not '%s'", reader->line); > > if (parse_feature_request(features, "deepen-relative")) > deepen_relative = 1; > @@ -1055,6 +1054,7 @@ void upload_pack(struct upload_pack_options *options) > { > struct string_list symref = STRING_LIST_INIT_DUP; > struct object_array want_obj = OBJECT_ARRAY_INIT; > + struct packet_reader reader; > > stateless_rpc = options->stateless_rpc; > timeout = options->timeout; > @@ -1078,10 +1078,12 @@ void upload_pack(struct upload_pack_options *options) > if (options->advertise_refs) > return; > > - receive_needs(&want_obj); > + packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE); > + > + receive_needs(&reader, &want_obj); > if (want_obj.nr) { > struct object_array have_obj = OBJECT_ARRAY_INIT; > - get_common_commits(&have_obj, &want_obj); > + get_common_commits(&reader, &have_obj, &want_obj); > create_pack_file(&have_obj, &want_obj); > } > } > -- > 2.20.1.415.g653613c723-goog > In general I think this looks good. If you want this to be a strict refactor with no behavior changes, you'll want to address the comments above. Otherwise, I'd prefer if you note in the commit message where/how the behavior is changing.
On Mon, Jan 7, 2019 at 2:33 PM Josh Steadmon <steadmon@google.com> wrote: > > On 2018.12.29 13:19, Masaya Suzuki wrote: > > By using and sharing a packet_reader while handling a Git pack protocol > > request, the same reader option is used throughout the code. This makes > > it easy to set a reader option to the request parsing code. > > > > Signed-off-by: Masaya Suzuki <masayasuzuki@google.com> > > --- > > builtin/archive.c | 19 ++++++------- > > builtin/receive-pack.c | 60 +++++++++++++++++++++-------------------- > > fetch-pack.c | 61 +++++++++++++++++++++++------------------- > > remote-curl.c | 22 ++++++++++----- > > send-pack.c | 37 ++++++++++++------------- > > upload-pack.c | 38 +++++++++++++------------- > > 6 files changed, 129 insertions(+), 108 deletions(-) > > > > diff --git a/builtin/archive.c b/builtin/archive.c > > index d2455237c..2fe1f05ca 100644 > > --- a/builtin/archive.c > > +++ b/builtin/archive.c > > @@ -27,10 +27,10 @@ static int run_remote_archiver(int argc, const char **argv, > > const char *remote, const char *exec, > > const char *name_hint) > > { > > - char *buf; > > int fd[2], i, rv; > > struct transport *transport; > > struct remote *_remote; > > + struct packet_reader reader; > > > > _remote = remote_get(remote); > > if (!_remote->url[0]) > > @@ -53,18 +53,19 @@ static int run_remote_archiver(int argc, const char **argv, > > packet_write_fmt(fd[1], "argument %s\n", argv[i]); > > packet_flush(fd[1]); > > > > - buf = packet_read_line(fd[0], NULL); > > - if (!buf) > > + packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE); > > + > > + if (packet_reader_read(&reader) != PACKET_READ_NORMAL) > > packet_read_line() can also return NULL if the packet is zero-length, so > you may want to add a "|| reader.pktlen <= 0" to the condition here (and > in other places where we were checking that packet_read_line() != NULL) > to make sure the behavior doesn't change. See discussion on my previous > attempt[1] to refactor this in builtin/archive.c. > > [1]: https://public-inbox.org/git/20180912053519.31085-1-steadmon@google.com/ That is interesting. In Documentation/technical/protocol-common.txt, it says "Implementations SHOULD NOT send an empty pkt-line ("0004").". The existing code won't distinguish "0000" and "0004", while "0004" is actually not a valid pkt-line. I'll make this patch with no behavior change, but I think we can make that behavior change to stop accepting 0004 as 0000, and remove the pktlen checks.
diff --git a/builtin/archive.c b/builtin/archive.c index d2455237c..2fe1f05ca 100644 --- a/builtin/archive.c +++ b/builtin/archive.c @@ -27,10 +27,10 @@ static int run_remote_archiver(int argc, const char **argv, const char *remote, const char *exec, const char *name_hint) { - char *buf; int fd[2], i, rv; struct transport *transport; struct remote *_remote; + struct packet_reader reader; _remote = remote_get(remote); if (!_remote->url[0]) @@ -53,18 +53,19 @@ static int run_remote_archiver(int argc, const char **argv, packet_write_fmt(fd[1], "argument %s\n", argv[i]); packet_flush(fd[1]); - buf = packet_read_line(fd[0], NULL); - if (!buf) + packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE); + + if (packet_reader_read(&reader) != PACKET_READ_NORMAL) die(_("git archive: expected ACK/NAK, got a flush packet")); - if (strcmp(buf, "ACK")) { - if (starts_with(buf, "NACK ")) - die(_("git archive: NACK %s"), buf + 5); - if (starts_with(buf, "ERR ")) - die(_("remote error: %s"), buf + 4); + if (strcmp(reader.line, "ACK")) { + if (starts_with(reader.line, "NACK ")) + die(_("git archive: NACK %s"), reader.line + 5); + if (starts_with(reader.line, "ERR ")) + die(_("remote error: %s"), reader.line + 4); die(_("git archive: protocol error")); } - if (packet_read_line(fd[0], NULL)) + if (packet_reader_read(&reader) != PACKET_READ_FLUSH) die(_("git archive: expected a flush")); /* Now, start reading from fd[0] and spit it out to stdout */ diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 33187bd8e..81cc07370 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1569,30 +1569,29 @@ static void queue_commands_from_cert(struct command **tail, } } -static struct command *read_head_info(struct oid_array *shallow) +static struct command *read_head_info(struct packet_reader *reader, + struct oid_array *shallow) { struct command *commands = NULL; struct command **p = &commands; for (;;) { - char *line; - int len, linelen; + int linelen; - line = packet_read_line(0, &len); - if (!line) + if (packet_reader_read(reader) != PACKET_READ_NORMAL) break; - if (len > 8 && starts_with(line, "shallow ")) { + if (reader->pktlen > 8 && starts_with(reader->line, "shallow ")) { struct object_id oid; - if (get_oid_hex(line + 8, &oid)) + if (get_oid_hex(reader->line + 8, &oid)) die("protocol error: expected shallow sha, got '%s'", - line + 8); + reader->line + 8); oid_array_append(shallow, &oid); continue; } - linelen = strlen(line); - if (linelen < len) { - const char *feature_list = line + linelen + 1; + linelen = strlen(reader->line); + if (linelen < reader->pktlen) { + const char *feature_list = reader->line + linelen + 1; if (parse_feature_request(feature_list, "report-status")) report_status = 1; if (parse_feature_request(feature_list, "side-band-64k")) @@ -1607,28 +1606,32 @@ static struct command *read_head_info(struct oid_array *shallow) use_push_options = 1; } - if (!strcmp(line, "push-cert")) { + if (!strcmp(reader->line, "push-cert")) { int true_flush = 0; - char certbuf[1024]; + int saved_options = reader->options; + reader->options &= ~PACKET_READ_CHOMP_NEWLINE; for (;;) { - len = packet_read(0, NULL, NULL, - certbuf, sizeof(certbuf), 0); - if (!len) { + packet_reader_read(reader); + if (reader->status == PACKET_READ_FLUSH) { true_flush = 1; break; } - if (!strcmp(certbuf, "push-cert-end\n")) + if (reader->status != PACKET_READ_NORMAL) { + die("protocol error: got an unexpected packet"); + } + if (!strcmp(reader->line, "push-cert-end\n")) break; /* end of cert */ - strbuf_addstr(&push_cert, certbuf); + strbuf_addstr(&push_cert, reader->line); } + reader->options = saved_options; if (true_flush) break; continue; } - p = queue_command(p, line, linelen); + p = queue_command(p, reader->line, linelen); } if (push_cert.len) @@ -1637,18 +1640,14 @@ static struct command *read_head_info(struct oid_array *shallow) return commands; } -static void read_push_options(struct string_list *options) +static void read_push_options(struct packet_reader *reader, + struct string_list *options) { while (1) { - char *line; - int len; - - line = packet_read_line(0, &len); - - if (!line) + if (packet_reader_read(reader) != PACKET_READ_NORMAL) break; - string_list_append(options, line); + string_list_append(options, reader->line); } } @@ -1924,6 +1923,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) struct oid_array shallow = OID_ARRAY_INIT; struct oid_array ref = OID_ARRAY_INIT; struct shallow_info si; + struct packet_reader reader; struct option options[] = { OPT__QUIET(&quiet, N_("quiet")), @@ -1986,12 +1986,14 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) if (advertise_refs) return 0; - if ((commands = read_head_info(&shallow)) != NULL) { + packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE); + + if ((commands = read_head_info(&reader, &shallow)) != NULL) { const char *unpack_status = NULL; struct string_list push_options = STRING_LIST_INIT_DUP; if (use_push_options) - read_push_options(&push_options); + read_push_options(&reader, &push_options); if (!check_cert_push_options(&push_options)) { struct command *cmd; for (cmd = commands; cmd; cmd = cmd->next) diff --git a/fetch-pack.c b/fetch-pack.c index 9691046e6..86790b9bb 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -135,38 +135,42 @@ enum ack_type { ACK_ready }; -static void consume_shallow_list(struct fetch_pack_args *args, int fd) +static void consume_shallow_list(struct fetch_pack_args *args, + struct packet_reader *reader) { if (args->stateless_rpc && args->deepen) { /* If we sent a depth we will get back "duplicate" * shallow and unshallow commands every time there * is a block of have lines exchanged. */ - char *line; - while ((line = packet_read_line(fd, NULL))) { - if (starts_with(line, "shallow ")) + while (packet_reader_read(reader) == PACKET_READ_NORMAL) { + if (starts_with(reader->line, "shallow ")) continue; - if (starts_with(line, "unshallow ")) + if (starts_with(reader->line, "unshallow ")) continue; die(_("git fetch-pack: expected shallow list")); } + if (reader->status != PACKET_READ_FLUSH) + die(_("git fetch-pack: expected a flush packet after shallow list")); } } -static enum ack_type get_ack(int fd, struct object_id *result_oid) +static enum ack_type get_ack(struct packet_reader *reader, + struct object_id *result_oid) { int len; - char *line = packet_read_line(fd, &len); const char *arg; - if (!line) + if (packet_reader_read(reader) != PACKET_READ_NORMAL) die(_("git fetch-pack: expected ACK/NAK, got a flush packet")); - if (!strcmp(line, "NAK")) + len = reader->pktlen; + + if (!strcmp(reader->line, "NAK")) return NAK; - if (skip_prefix(line, "ACK ", &arg)) { + if (skip_prefix(reader->line, "ACK ", &arg)) { if (!get_oid_hex(arg, result_oid)) { arg += 40; - len -= arg - line; + len -= arg - reader->line; if (len < 1) return ACK; if (strstr(arg, "continue")) @@ -178,9 +182,9 @@ static enum ack_type get_ack(int fd, struct object_id *result_oid) return ACK; } } - if (skip_prefix(line, "ERR ", &arg)) + if (skip_prefix(reader->line, "ERR ", &arg)) die(_("remote error: %s"), arg); - die(_("git fetch-pack: expected ACK/NAK, got '%s'"), line); + die(_("git fetch-pack: expected ACK/NAK, got '%s'"), reader->line); } static void send_request(struct fetch_pack_args *args, @@ -248,10 +252,14 @@ static int find_common(struct fetch_negotiator *negotiator, int got_ready = 0; struct strbuf req_buf = STRBUF_INIT; size_t state_len = 0; + struct packet_reader reader; if (args->stateless_rpc && multi_ack == 1) die(_("--stateless-rpc requires multi_ack_detailed")); + packet_reader_init(&reader, fd[0], NULL, 0, + PACKET_READ_CHOMP_NEWLINE); + if (!args->no_dependents) { mark_tips(negotiator, args->negotiation_tips); for_each_cached_alternate(negotiator, insert_one_alternate_object); @@ -336,31 +344,30 @@ static int find_common(struct fetch_negotiator *negotiator, state_len = req_buf.len; if (args->deepen) { - char *line; const char *arg; struct object_id oid; send_request(args, fd[1], &req_buf); - while ((line = packet_read_line(fd[0], NULL))) { - if (skip_prefix(line, "shallow ", &arg)) { + while (packet_reader_read(&reader) == PACKET_READ_NORMAL) { + if (skip_prefix(reader.line, "shallow ", &arg)) { if (get_oid_hex(arg, &oid)) - die(_("invalid shallow line: %s"), line); + die(_("invalid shallow line: %s"), reader.line); register_shallow(the_repository, &oid); continue; } - if (skip_prefix(line, "unshallow ", &arg)) { + if (skip_prefix(reader.line, "unshallow ", &arg)) { if (get_oid_hex(arg, &oid)) - die(_("invalid unshallow line: %s"), line); + die(_("invalid unshallow line: %s"), reader.line); if (!lookup_object(the_repository, oid.hash)) - die(_("object not found: %s"), line); + die(_("object not found: %s"), reader.line); /* make sure that it is parsed as shallow */ if (!parse_object(the_repository, &oid)) - die(_("error in object: %s"), line); + die(_("error in object: %s"), reader.line); if (unregister_shallow(&oid)) - die(_("no shallow found: %s"), line); + die(_("no shallow found: %s"), reader.line); continue; } - die(_("expected shallow/unshallow, got %s"), line); + die(_("expected shallow/unshallow, got %s"), reader.line); } } else if (!args->stateless_rpc) send_request(args, fd[1], &req_buf); @@ -397,9 +404,9 @@ static int find_common(struct fetch_negotiator *negotiator, if (!args->stateless_rpc && count == INITIAL_FLUSH) continue; - consume_shallow_list(args, fd[0]); + consume_shallow_list(args, &reader); do { - ack = get_ack(fd[0], result_oid); + ack = get_ack(&reader, result_oid); if (ack) print_verbose(args, _("got %s %d %s"), "ack", ack, oid_to_hex(result_oid)); @@ -469,9 +476,9 @@ static int find_common(struct fetch_negotiator *negotiator, strbuf_release(&req_buf); if (!got_ready || !no_done) - consume_shallow_list(args, fd[0]); + consume_shallow_list(args, &reader); while (flushes || multi_ack) { - int ack = get_ack(fd[0], result_oid); + int ack = get_ack(&reader, result_oid); if (ack) { print_verbose(args, _("got %s (%d) %s"), "ack", ack, oid_to_hex(result_oid)); diff --git a/remote-curl.c b/remote-curl.c index 1220dffcd..bbd9ba0f3 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -409,28 +409,36 @@ static struct discovery *discover_refs(const char *service, int for_push) if (maybe_smart && (5 <= last->len && last->buf[4] == '#') && !strbuf_cmp(&exp, &type)) { - char *line; + struct packet_reader reader; + packet_reader_init(&reader, -1, last->buf, last->len, + PACKET_READ_CHOMP_NEWLINE); /* * smart HTTP response; validate that the service * pkt-line matches our request. */ - line = packet_read_line_buf(&last->buf, &last->len, NULL); - if (!line) + if (packet_reader_read(&reader) != PACKET_READ_NORMAL) die("invalid server response; expected service, got flush packet"); strbuf_reset(&exp); strbuf_addf(&exp, "# service=%s", service); - if (strcmp(line, exp.buf)) - die("invalid server response; got '%s'", line); + if (strcmp(reader.line, exp.buf)) + die("invalid server response; got '%s'", reader.line); strbuf_release(&exp); /* The header can include additional metadata lines, up * until a packet flush marker. Ignore these now, but * in the future we might start to scan them. */ - while (packet_read_line_buf(&last->buf, &last->len, NULL)) - ; + for (;;) { + packet_reader_read(&reader); + if (reader.pktlen <= 0) { + break; + } + } + + last->buf = reader.src_buffer; + last->len = reader.src_len; last->proto_git = 1; } else if (maybe_smart && diff --git a/send-pack.c b/send-pack.c index f69268677..913645046 100644 --- a/send-pack.c +++ b/send-pack.c @@ -135,38 +135,36 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc return 0; } -static int receive_unpack_status(int in) +static int receive_unpack_status(struct packet_reader *reader) { - const char *line = packet_read_line(in, NULL); - if (!line) + if (packet_reader_read(reader) != PACKET_READ_NORMAL) return error(_("unexpected flush packet while reading remote unpack status")); - if (!skip_prefix(line, "unpack ", &line)) - return error(_("unable to parse remote unpack status: %s"), line); - if (strcmp(line, "ok")) - return error(_("remote unpack failed: %s"), line); + if (!skip_prefix(reader->line, "unpack ", &reader->line)) + return error(_("unable to parse remote unpack status: %s"), reader->line); + if (strcmp(reader->line, "ok")) + return error(_("remote unpack failed: %s"), reader->line); return 0; } -static int receive_status(int in, struct ref *refs) +static int receive_status(struct packet_reader *reader, struct ref *refs) { struct ref *hint; int ret; hint = NULL; - ret = receive_unpack_status(in); + ret = receive_unpack_status(reader); while (1) { - char *refname; + const char *refname; char *msg; - char *line = packet_read_line(in, NULL); - if (!line) + if (packet_reader_read(reader) != PACKET_READ_NORMAL) break; - if (!starts_with(line, "ok ") && !starts_with(line, "ng ")) { - error("invalid ref status from remote: %s", line); + if (!starts_with(reader->line, "ok ") && !starts_with(reader->line, "ng ")) { + error("invalid ref status from remote: %s", reader->line); ret = -1; break; } - refname = line + 3; + refname = reader->line + 3; msg = strchr(refname, ' '); if (msg) *msg++ = '\0'; @@ -187,7 +185,7 @@ static int receive_status(int in, struct ref *refs) continue; } - if (line[0] == 'o' && line[1] == 'k') + if (reader->line[0] == 'o' && reader->line[1] == 'k') hint->status = REF_STATUS_OK; else { hint->status = REF_STATUS_REMOTE_REJECT; @@ -390,6 +388,7 @@ int send_pack(struct send_pack_args *args, int ret; struct async demux; const char *push_cert_nonce = NULL; + struct packet_reader reader; /* Does the other end support the reporting? */ if (server_supports("report-status")) @@ -559,6 +558,8 @@ int send_pack(struct send_pack_args *args, in = demux.out; } + packet_reader_init(&reader, in, NULL, 0, PACKET_READ_CHOMP_NEWLINE); + if (need_pack_data && cmds_sent) { if (pack_objects(out, remote_refs, extra_have, args) < 0) { for (ref = remote_refs; ref; ref = ref->next) @@ -573,7 +574,7 @@ int send_pack(struct send_pack_args *args, * are failing, and just want the error() side effects. */ if (status_report) - receive_unpack_status(in); + receive_unpack_status(&reader); if (use_sideband) { close(demux.out); @@ -590,7 +591,7 @@ int send_pack(struct send_pack_args *args, packet_flush(out); if (status_report && cmds_sent) - ret = receive_status(in, remote_refs); + ret = receive_status(&reader, remote_refs); else ret = 0; if (args->stateless_rpc) diff --git a/upload-pack.c b/upload-pack.c index 5e81f1ff2..1638825ee 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -354,7 +354,8 @@ static int ok_to_give_up(const struct object_array *have_obj, min_generation); } -static int get_common_commits(struct object_array *have_obj, +static int get_common_commits(struct packet_reader *reader, + struct object_array *have_obj, struct object_array *want_obj) { struct object_id oid; @@ -366,12 +367,11 @@ static int get_common_commits(struct object_array *have_obj, save_commit_buffer = 0; for (;;) { - char *line = packet_read_line(0, NULL); const char *arg; reset_timeout(); - if (!line) { + if (packet_reader_read(reader) != PACKET_READ_NORMAL) { if (multi_ack == 2 && got_common && !got_other && ok_to_give_up(have_obj, want_obj)) { sent_ready = 1; @@ -390,7 +390,7 @@ static int get_common_commits(struct object_array *have_obj, got_other = 0; continue; } - if (skip_prefix(line, "have ", &arg)) { + if (skip_prefix(reader->line, "have ", &arg)) { switch (got_oid(arg, &oid, have_obj)) { case -1: /* they have what we do not */ got_other = 1; @@ -416,7 +416,7 @@ static int get_common_commits(struct object_array *have_obj, } continue; } - if (!strcmp(line, "done")) { + if (!strcmp(reader->line, "done")) { if (have_obj->nr > 0) { if (multi_ack) packet_write_fmt(1, "ACK %s\n", last_hex); @@ -425,7 +425,7 @@ static int get_common_commits(struct object_array *have_obj, packet_write_fmt(1, "NAK\n"); return -1; } - die("git upload-pack: expected SHA1 list, got '%s'", line); + die("git upload-pack: expected SHA1 list, got '%s'", reader->line); } } @@ -826,7 +826,7 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not, return 0; } -static void receive_needs(struct object_array *want_obj) +static void receive_needs(struct packet_reader *reader, struct object_array *want_obj) { struct object_array shallows = OBJECT_ARRAY_INIT; struct string_list deepen_not = STRING_LIST_INIT_DUP; @@ -840,33 +840,32 @@ static void receive_needs(struct object_array *want_obj) struct object *o; const char *features; struct object_id oid_buf; - char *line = packet_read_line(0, NULL); const char *arg; reset_timeout(); - if (!line) + if (packet_reader_read(reader) != PACKET_READ_NORMAL) break; - if (process_shallow(line, &shallows)) + if (process_shallow(reader->line, &shallows)) continue; - if (process_deepen(line, &depth)) + if (process_deepen(reader->line, &depth)) continue; - if (process_deepen_since(line, &deepen_since, &deepen_rev_list)) + if (process_deepen_since(reader->line, &deepen_since, &deepen_rev_list)) continue; - if (process_deepen_not(line, &deepen_not, &deepen_rev_list)) + if (process_deepen_not(reader->line, &deepen_not, &deepen_rev_list)) continue; - if (skip_prefix(line, "filter ", &arg)) { + if (skip_prefix(reader->line, "filter ", &arg)) { if (!filter_capability_requested) die("git upload-pack: filtering capability not negotiated"); parse_list_objects_filter(&filter_options, arg); continue; } - if (!skip_prefix(line, "want ", &arg) || + if (!skip_prefix(reader->line, "want ", &arg) || parse_oid_hex(arg, &oid_buf, &features)) die("git upload-pack: protocol error, " - "expected to get object ID, not '%s'", line); + "expected to get object ID, not '%s'", reader->line); if (parse_feature_request(features, "deepen-relative")) deepen_relative = 1; @@ -1055,6 +1054,7 @@ void upload_pack(struct upload_pack_options *options) { struct string_list symref = STRING_LIST_INIT_DUP; struct object_array want_obj = OBJECT_ARRAY_INIT; + struct packet_reader reader; stateless_rpc = options->stateless_rpc; timeout = options->timeout; @@ -1078,10 +1078,12 @@ void upload_pack(struct upload_pack_options *options) if (options->advertise_refs) return; - receive_needs(&want_obj); + packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE); + + receive_needs(&reader, &want_obj); if (want_obj.nr) { struct object_array have_obj = OBJECT_ARRAY_INIT; - get_common_commits(&have_obj, &want_obj); + get_common_commits(&reader, &have_obj, &want_obj); create_pack_file(&have_obj, &want_obj); } }
By using and sharing a packet_reader while handling a Git pack protocol request, the same reader option is used throughout the code. This makes it easy to set a reader option to the request parsing code. Signed-off-by: Masaya Suzuki <masayasuzuki@google.com> --- builtin/archive.c | 19 ++++++------- builtin/receive-pack.c | 60 +++++++++++++++++++++-------------------- fetch-pack.c | 61 +++++++++++++++++++++++------------------- remote-curl.c | 22 ++++++++++----- send-pack.c | 37 ++++++++++++------------- upload-pack.c | 38 +++++++++++++------------- 6 files changed, 129 insertions(+), 108 deletions(-)