From 01f9ec64c8a82a05ba7e5a17b292ede037a469ea Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Sat, 29 Dec 2018 13:19:14 -0800 Subject: [PATCH 1/6] Use packet_reader instead of packet_read_line 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 Signed-off-by: Junio C Hamano --- 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 d2455237ce..2fe1f05ca2 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 33187bd8e9..81cc073700 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 9691046e64..86790b9bba 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 @@ done: 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 1220dffcdc..bbd9ba0f35 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 f692686770..9136450464 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 5e81f1ff24..1638825ee8 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); } } From 2d103c31c2cfcf03ff1408d639043469b0c93f70 Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Sat, 29 Dec 2018 13:19:15 -0800 Subject: [PATCH 2/6] pack-protocol.txt: accept error packets in any context In the Git pack protocol definition, an error packet may appear only in a certain context. However, servers can face a runtime error (e.g. I/O error) at an arbitrary timing. This patch changes the protocol to allow an error packet to be sent instead of any packet. Without this protocol spec change, when a server cannot process a request, there's no way to tell that to a client. Since the server cannot produce a valid response, it would be forced to cut a connection without telling why. With this protocol spec change, the server can be more gentle in this situation. An old client may see these error packets as an unexpected packet, but this is not worse than having an unexpected EOF. Following this protocol spec change, the error packet handling code is moved to pkt-line.c. Implementation wise, this implementation uses pkt-line to communicate with a subprocess. Since this is not a part of Git protocol, it's possible that a packet that is not supposed to be an error packet is mistakenly parsed as an error packet. This error packet handling is enabled only for the Git pack protocol parsing code considering this. Signed-off-by: Masaya Suzuki Signed-off-by: Junio C Hamano --- Documentation/technical/pack-protocol.txt | 20 +++++++++++--------- builtin/archive.c | 6 +++--- builtin/fetch-pack.c | 3 ++- builtin/receive-pack.c | 4 +++- builtin/send-pack.c | 3 ++- connect.c | 3 --- fetch-pack.c | 8 ++++---- pkt-line.c | 4 ++++ pkt-line.h | 8 ++++++-- remote-curl.c | 9 ++++++--- send-pack.c | 4 +++- serve.c | 5 +++-- t/t5703-upload-pack-ref-in-want.sh | 4 ++-- transport.c | 3 ++- upload-pack.c | 4 +++- 15 files changed, 54 insertions(+), 34 deletions(-) diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt index 6ac774d5f6..7a2375a55d 100644 --- a/Documentation/technical/pack-protocol.txt +++ b/Documentation/technical/pack-protocol.txt @@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate `PKT-LINE(...)`, unless otherwise noted the usual pkt-line LF rules apply: the sender SHOULD include a LF, but the receiver MUST NOT complain if it is not present. +An error packet is a special pkt-line that contains an error string. + +---- + error-line = PKT-LINE("ERR" SP explanation-text) +---- + +Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY +be sent. Once this packet is sent by a client or a server, the data transfer +process defined in this protocol is terminated. + Transports ---------- There are three transports over which the packfile protocol is @@ -89,13 +99,6 @@ process on the server side over the Git protocol is this: "0039git-upload-pack /schacon/gitbook.git\0host=example.com\0" | nc -v example.com 9418 -If the server refuses the request for some reasons, it could abort -gracefully with an error message. - ----- - error-line = PKT-LINE("ERR" SP explanation-text) ----- - SSH Transport ------------- @@ -398,12 +401,11 @@ from the client). Then the server will start sending its packfile data. ---- - server-response = *ack_multi ack / nak / error-line + server-response = *ack_multi ack / nak ack_multi = PKT-LINE("ACK" SP obj-id ack_status) ack_status = "continue" / "common" / "ready" ack = PKT-LINE("ACK" SP obj-id) nak = PKT-LINE("NAK") - error-line = PKT-LINE("ERR" SP explanation-text) ---- A simple clone may look like this (with no 'have' lines): diff --git a/builtin/archive.c b/builtin/archive.c index 2fe1f05ca2..45d11669aa 100644 --- a/builtin/archive.c +++ b/builtin/archive.c @@ -53,15 +53,15 @@ static int run_remote_archiver(int argc, const char **argv, packet_write_fmt(fd[1], "argument %s\n", argv[i]); packet_flush(fd[1]); - packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE); + packet_reader_init(&reader, fd[0], NULL, 0, + PACKET_READ_CHOMP_NEWLINE | + PACKET_READ_DIE_ON_ERR_PACKET); if (packet_reader_read(&reader) != PACKET_READ_NORMAL) die(_("git archive: expected ACK/NAK, got a flush packet")); 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")); } diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 63e69a5801..85dbc2af87 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -217,7 +217,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE | - PACKET_READ_GENTLE_ON_EOF); + PACKET_READ_GENTLE_ON_EOF | + PACKET_READ_DIE_ON_ERR_PACKET); switch (discover_version(&reader)) { case protocol_v2: diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 81cc073700..d58b7750b6 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1986,7 +1986,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) if (advertise_refs) return 0; - packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE); + packet_reader_init(&reader, 0, NULL, 0, + PACKET_READ_CHOMP_NEWLINE | + PACKET_READ_DIE_ON_ERR_PACKET); if ((commands = read_head_info(&reader, &shallow)) != NULL) { const char *unpack_status = NULL; diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 8e3c7490f7..098ebf22d0 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -250,7 +250,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE | - PACKET_READ_GENTLE_ON_EOF); + PACKET_READ_GENTLE_ON_EOF | + PACKET_READ_DIE_ON_ERR_PACKET); switch (discover_version(&reader)) { case protocol_v2: diff --git a/connect.c b/connect.c index 24281b6082..4813f005ab 100644 --- a/connect.c +++ b/connect.c @@ -296,7 +296,6 @@ struct ref **get_remote_heads(struct packet_reader *reader, struct ref **orig_list = list; int len = 0; enum get_remote_heads_state state = EXPECTING_FIRST_REF; - const char *arg; *list = NULL; @@ -306,8 +305,6 @@ struct ref **get_remote_heads(struct packet_reader *reader, die_initial_contact(1); case PACKET_READ_NORMAL: len = reader->pktlen; - if (len > 4 && skip_prefix(reader->line, "ERR ", &arg)) - die(_("remote error: %s"), arg); break; case PACKET_READ_FLUSH: state = EXPECTING_DONE; diff --git a/fetch-pack.c b/fetch-pack.c index 86790b9bba..3f9626dbf7 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -182,8 +182,6 @@ static enum ack_type get_ack(struct packet_reader *reader, return ACK; } } - if (skip_prefix(reader->line, "ERR ", &arg)) - die(_("remote error: %s"), arg); die(_("git fetch-pack: expected ACK/NAK, got '%s'"), reader->line); } @@ -258,7 +256,8 @@ static int find_common(struct fetch_negotiator *negotiator, die(_("--stateless-rpc requires multi_ack_detailed")); packet_reader_init(&reader, fd[0], NULL, 0, - PACKET_READ_CHOMP_NEWLINE); + PACKET_READ_CHOMP_NEWLINE | + PACKET_READ_DIE_ON_ERR_PACKET); if (!args->no_dependents) { mark_tips(negotiator, args->negotiation_tips); @@ -1358,7 +1357,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, struct fetch_negotiator negotiator; fetch_negotiator_init(&negotiator, negotiation_algorithm); packet_reader_init(&reader, fd[0], NULL, 0, - PACKET_READ_CHOMP_NEWLINE); + PACKET_READ_CHOMP_NEWLINE | + PACKET_READ_DIE_ON_ERR_PACKET); while (state != FETCH_DONE) { switch (state) { diff --git a/pkt-line.c b/pkt-line.c index 04d10bbd03..e70ea6d88f 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer, return PACKET_READ_EOF; } + if ((options & PACKET_READ_DIE_ON_ERR_PACKET) && + starts_with(buffer, "ERR ")) + die(_("remote error: %s"), buffer + 4); + if ((options & PACKET_READ_CHOMP_NEWLINE) && len && buffer[len-1] == '\n') len--; diff --git a/pkt-line.h b/pkt-line.h index 5b28d43472..d7e1dbc047 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -62,9 +62,13 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); * * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if * present) is removed from the buffer before returning. + * + * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an + * ERR packet. */ -#define PACKET_READ_GENTLE_ON_EOF (1u<<0) -#define PACKET_READ_CHOMP_NEWLINE (1u<<1) +#define PACKET_READ_GENTLE_ON_EOF (1u<<0) +#define PACKET_READ_CHOMP_NEWLINE (1u<<1) +#define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2) int packet_read(int fd, char **src_buffer, size_t *src_len, char *buffer, unsigned size, int options); diff --git a/remote-curl.c b/remote-curl.c index bbd9ba0f35..10b50869c5 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -204,7 +204,8 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push) packet_reader_init(&reader, -1, heads->buf, heads->len, PACKET_READ_CHOMP_NEWLINE | - PACKET_READ_GENTLE_ON_EOF); + PACKET_READ_GENTLE_ON_EOF | + PACKET_READ_DIE_ON_ERR_PACKET); heads->version = discover_version(&reader); switch (heads->version) { @@ -411,7 +412,8 @@ static struct discovery *discover_refs(const char *service, int for_push) !strbuf_cmp(&exp, &type)) { struct packet_reader reader; packet_reader_init(&reader, -1, last->buf, last->len, - PACKET_READ_CHOMP_NEWLINE); + PACKET_READ_CHOMP_NEWLINE | + PACKET_READ_DIE_ON_ERR_PACKET); /* * smart HTTP response; validate that the service @@ -1182,7 +1184,8 @@ static void proxy_state_init(struct proxy_state *p, const char *service_name, p->headers = curl_slist_append(p->headers, buf.buf); packet_reader_init(&p->reader, p->in, NULL, 0, - PACKET_READ_GENTLE_ON_EOF); + PACKET_READ_GENTLE_ON_EOF | + PACKET_READ_DIE_ON_ERR_PACKET); strbuf_release(&buf); } diff --git a/send-pack.c b/send-pack.c index 9136450464..7b9829f165 100644 --- a/send-pack.c +++ b/send-pack.c @@ -558,7 +558,9 @@ int send_pack(struct send_pack_args *args, in = demux.out; } - packet_reader_init(&reader, in, NULL, 0, PACKET_READ_CHOMP_NEWLINE); + packet_reader_init(&reader, in, NULL, 0, + PACKET_READ_CHOMP_NEWLINE | + PACKET_READ_DIE_ON_ERR_PACKET); if (need_pack_data && cmds_sent) { if (pack_objects(out, remote_refs, extra_have, args) < 0) { diff --git a/serve.c b/serve.c index bda085f09c..317256c1a4 100644 --- a/serve.c +++ b/serve.c @@ -167,7 +167,8 @@ static int process_request(void) packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE | - PACKET_READ_GENTLE_ON_EOF); + PACKET_READ_GENTLE_ON_EOF | + PACKET_READ_DIE_ON_ERR_PACKET); /* * Check to see if the client closed their end before sending another @@ -175,7 +176,7 @@ static int process_request(void) */ if (packet_reader_peek(&reader) == PACKET_READ_EOF) return 1; - reader.options = PACKET_READ_CHOMP_NEWLINE; + reader.options &= ~PACKET_READ_GENTLE_ON_EOF; while (state != PROCESS_REQUEST_DONE) { switch (packet_reader_peek(&reader)) { diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh index 3f58f05cbb..d2a9d0c127 100755 --- a/t/t5703-upload-pack-ref-in-want.sh +++ b/t/t5703-upload-pack-ref-in-want.sh @@ -208,7 +208,7 @@ test_expect_success 'server is initially ahead - no ref in want' ' cp -r "$LOCAL_PRISTINE" local && inconsistency master 1234567890123456789012345678901234567890 && test_must_fail git -C local fetch 2>err && - grep "ERR upload-pack: not our ref" err + grep "fatal: remote error: upload-pack: not our ref" err ' test_expect_success 'server is initially ahead - ref in want' ' @@ -254,7 +254,7 @@ test_expect_success 'server loses a ref - ref in want' ' echo "s/master/raster/" >"$HTTPD_ROOT_PATH/one-time-sed" && test_must_fail git -C local fetch 2>err && - grep "ERR unknown ref refs/heads/raster" err + grep "fatal: remote error: unknown ref refs/heads/raster" err ' stop_httpd diff --git a/transport.c b/transport.c index 5a74b609ff..12db4251c1 100644 --- a/transport.c +++ b/transport.c @@ -273,7 +273,8 @@ static struct ref *handshake(struct transport *transport, int for_push, packet_reader_init(&reader, data->fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE | - PACKET_READ_GENTLE_ON_EOF); + PACKET_READ_GENTLE_ON_EOF | + PACKET_READ_DIE_ON_ERR_PACKET); data->version = discover_version(&reader); switch (data->version) { diff --git a/upload-pack.c b/upload-pack.c index 1638825ee8..08b547cf01 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1078,7 +1078,9 @@ void upload_pack(struct upload_pack_options *options) if (options->advertise_refs) return; - packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE); + packet_reader_init(&reader, 0, NULL, 0, + PACKET_READ_CHOMP_NEWLINE | + PACKET_READ_DIE_ON_ERR_PACKET); receive_needs(&reader, &want_obj); if (want_obj.nr) { From bc2e795cea607e444a9f985b0dbed5a4d25921c8 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Tue, 15 Jan 2019 11:40:27 -0800 Subject: [PATCH 3/6] pkt-line: introduce struct packet_writer A future patch will allow the client to request multiplexing of the entire fetch response (and not only during packfile transmission), which in turn allows the server to send progress and keepalive messages at any time during the response. It will be convenient for a future patch if writing options (specifically, whether the written data is to be multiplexed) could be controlled from a single place, so create struct packet_writer to serve as that place, and modify upload-pack to use it. Currently, it only stores the output fd, but a subsequent patch will (as described above) introduce an option to determine if the written data is to be multiplexed. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- pkt-line.c | 47 ++++++++++++++++++--- pkt-line.h | 14 +++++++ upload-pack.c | 112 +++++++++++++++++++++++++++----------------------- 3 files changed, 115 insertions(+), 58 deletions(-) diff --git a/pkt-line.c b/pkt-line.c index e70ea6d88f..9d3e402d41 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -129,12 +129,14 @@ static void set_packet_header(char *buf, const int size) #undef hex } -static void format_packet(struct strbuf *out, const char *fmt, va_list args) +static void format_packet(struct strbuf *out, const char *prefix, + const char *fmt, va_list args) { size_t orig_len, n; orig_len = out->len; strbuf_addstr(out, "0000"); + strbuf_addstr(out, prefix); strbuf_vaddf(out, fmt, args); n = out->len - orig_len; @@ -145,13 +147,13 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args) packet_trace(out->buf + orig_len + 4, n - 4, 1); } -static int packet_write_fmt_1(int fd, int gently, +static int packet_write_fmt_1(int fd, int gently, const char *prefix, const char *fmt, va_list args) { static struct strbuf buf = STRBUF_INIT; strbuf_reset(&buf); - format_packet(&buf, fmt, args); + format_packet(&buf, prefix, fmt, args); if (write_in_full(fd, buf.buf, buf.len) < 0) { if (!gently) { check_pipe(errno); @@ -168,7 +170,7 @@ void packet_write_fmt(int fd, const char *fmt, ...) va_list args; va_start(args, fmt); - packet_write_fmt_1(fd, 0, fmt, args); + packet_write_fmt_1(fd, 0, "", fmt, args); va_end(args); } @@ -178,7 +180,7 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...) va_list args; va_start(args, fmt); - status = packet_write_fmt_1(fd, 1, fmt, args); + status = packet_write_fmt_1(fd, 1, "", fmt, args); va_end(args); return status; } @@ -211,7 +213,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) va_list args; va_start(args, fmt); - format_packet(buf, fmt, args); + format_packet(buf, "", fmt, args); va_end(args); } @@ -486,3 +488,36 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader) reader->line_peeked = 1; return reader->status; } + +void packet_writer_init(struct packet_writer *writer, int dest_fd) +{ + writer->dest_fd = dest_fd; +} + +void packet_writer_write(struct packet_writer *writer, const char *fmt, ...) +{ + va_list args; + + va_start(args, fmt); + packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args); + va_end(args); +} + +void packet_writer_error(struct packet_writer *writer, const char *fmt, ...) +{ + va_list args; + + va_start(args, fmt); + packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args); + va_end(args); +} + +void packet_writer_delim(struct packet_writer *writer) +{ + packet_delim(writer->dest_fd); +} + +void packet_writer_flush(struct packet_writer *writer) +{ + packet_flush(writer->dest_fd); +} diff --git a/pkt-line.h b/pkt-line.h index d7e1dbc047..023ad2951d 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -183,4 +183,18 @@ extern enum packet_read_status packet_reader_peek(struct packet_reader *reader); #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4) extern char packet_buffer[LARGE_PACKET_MAX]; +struct packet_writer { + int dest_fd; +}; + +void packet_writer_init(struct packet_writer *writer, int dest_fd); + +/* These functions die upon failure. */ +__attribute__((format (printf, 2, 3))) +void packet_writer_write(struct packet_writer *writer, const char *fmt, ...); +__attribute__((format (printf, 2, 3))) +void packet_writer_error(struct packet_writer *writer, const char *fmt, ...); +void packet_writer_delim(struct packet_writer *writer); +void packet_writer_flush(struct packet_writer *writer); + #endif diff --git a/upload-pack.c b/upload-pack.c index 08b547cf01..60a26bbbfd 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -613,13 +613,14 @@ error: } } -static void send_shallow(struct commit_list *result) +static void send_shallow(struct packet_writer *writer, + struct commit_list *result) { while (result) { struct object *object = &result->item->object; if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) { - packet_write_fmt(1, "shallow %s", - oid_to_hex(&object->oid)); + packet_writer_write(writer, "shallow %s", + oid_to_hex(&object->oid)); register_shallow(the_repository, &object->oid); shallow_nr++; } @@ -627,7 +628,8 @@ static void send_shallow(struct commit_list *result) } } -static void send_unshallow(const struct object_array *shallows, +static void send_unshallow(struct packet_writer *writer, + const struct object_array *shallows, struct object_array *want_obj) { int i; @@ -636,8 +638,8 @@ static void send_unshallow(const struct object_array *shallows, struct object *object = shallows->objects[i].item; if (object->flags & NOT_SHALLOW) { struct commit_list *parents; - packet_write_fmt(1, "unshallow %s", - oid_to_hex(&object->oid)); + packet_writer_write(writer, "unshallow %s", + oid_to_hex(&object->oid)); object->flags &= ~CLIENT_SHALLOW; /* * We want to _register_ "object" as shallow, but we @@ -662,7 +664,7 @@ static void send_unshallow(const struct object_array *shallows, } } -static void deepen(int depth, int deepen_relative, +static void deepen(struct packet_writer *writer, int depth, int deepen_relative, struct object_array *shallows, struct object_array *want_obj) { if (depth == INFINITE_DEPTH && !is_repository_shallow(the_repository)) { @@ -680,7 +682,7 @@ static void deepen(int depth, int deepen_relative, result = get_shallow_commits(&reachable_shallows, depth + 1, SHALLOW, NOT_SHALLOW); - send_shallow(result); + send_shallow(writer, result); free_commit_list(result); object_array_clear(&reachable_shallows); } else { @@ -688,14 +690,15 @@ static void deepen(int depth, int deepen_relative, result = get_shallow_commits(want_obj, depth, SHALLOW, NOT_SHALLOW); - send_shallow(result); + send_shallow(writer, result); free_commit_list(result); } - send_unshallow(shallows, want_obj); + send_unshallow(writer, shallows, want_obj); } -static void deepen_by_rev_list(int ac, const char **av, +static void deepen_by_rev_list(struct packet_writer *writer, int ac, + const char **av, struct object_array *shallows, struct object_array *want_obj) { @@ -703,13 +706,14 @@ static void deepen_by_rev_list(int ac, const char **av, close_commit_graph(the_repository); result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW); - send_shallow(result); + send_shallow(writer, result); free_commit_list(result); - send_unshallow(shallows, want_obj); + send_unshallow(writer, shallows, want_obj); } /* Returns 1 if a shallow list is sent or 0 otherwise */ -static int send_shallow_list(int depth, int deepen_rev_list, +static int send_shallow_list(struct packet_writer *writer, + int depth, int deepen_rev_list, timestamp_t deepen_since, struct string_list *deepen_not, struct object_array *shallows, @@ -720,7 +724,7 @@ static int send_shallow_list(int depth, int deepen_rev_list, if (depth > 0 && deepen_rev_list) die("git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together"); if (depth > 0) { - deepen(depth, deepen_relative, shallows, want_obj); + deepen(writer, depth, deepen_relative, shallows, want_obj); ret = 1; } else if (deepen_rev_list) { struct argv_array av = ARGV_ARRAY_INIT; @@ -741,7 +745,7 @@ static int send_shallow_list(int depth, int deepen_rev_list, struct object *o = want_obj->objects[i].item; argv_array_push(&av, oid_to_hex(&o->oid)); } - deepen_by_rev_list(av.argc, av.argv, shallows, want_obj); + deepen_by_rev_list(writer, av.argc, av.argv, shallows, want_obj); argv_array_clear(&av); ret = 1; } else { @@ -834,8 +838,10 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan int has_non_tip = 0; timestamp_t deepen_since = 0; int deepen_rev_list = 0; + struct packet_writer writer; shallow_nr = 0; + packet_writer_init(&writer, 1); for (;;) { struct object *o; const char *features; @@ -892,9 +898,9 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan o = parse_object(the_repository, &oid_buf); if (!o) { - packet_write_fmt(1, - "ERR upload-pack: not our ref %s", - oid_to_hex(&oid_buf)); + packet_writer_error(&writer, + "upload-pack: not our ref %s", + oid_to_hex(&oid_buf)); die("git upload-pack: not our ref %s", oid_to_hex(&oid_buf)); } @@ -923,7 +929,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan if (depth == 0 && !deepen_rev_list && shallows.nr == 0) return; - if (send_shallow_list(depth, deepen_rev_list, deepen_since, + if (send_shallow_list(&writer, depth, deepen_rev_list, deepen_since, &deepen_not, &shallows, want_obj)) packet_flush(1); object_array_clear(&shallows); @@ -1102,6 +1108,8 @@ struct upload_pack_data { int deepen_rev_list; int deepen_relative; + struct packet_writer writer; + unsigned stateless_rpc : 1; unsigned use_thin_pack : 1; @@ -1125,6 +1133,7 @@ static void upload_pack_data_init(struct upload_pack_data *data) data->haves = haves; data->shallows = shallows; data->deepen_not = deepen_not; + packet_writer_init(&data->writer, 1); } static void upload_pack_data_clear(struct upload_pack_data *data) @@ -1136,7 +1145,8 @@ static void upload_pack_data_clear(struct upload_pack_data *data) string_list_clear(&data->deepen_not, 0); } -static int parse_want(const char *line, struct object_array *want_obj) +static int parse_want(struct packet_writer *writer, const char *line, + struct object_array *want_obj) { const char *arg; if (skip_prefix(line, "want ", &arg)) { @@ -1149,9 +1159,9 @@ static int parse_want(const char *line, struct object_array *want_obj) o = parse_object(the_repository, &oid); if (!o) { - packet_write_fmt(1, - "ERR upload-pack: not our ref %s", - oid_to_hex(&oid)); + packet_writer_error(writer, + "upload-pack: not our ref %s", + oid_to_hex(&oid)); die("git upload-pack: not our ref %s", oid_to_hex(&oid)); } @@ -1167,7 +1177,8 @@ static int parse_want(const char *line, struct object_array *want_obj) return 0; } -static int parse_want_ref(const char *line, struct string_list *wanted_refs, +static int parse_want_ref(struct packet_writer *writer, const char *line, + struct string_list *wanted_refs, struct object_array *want_obj) { const char *arg; @@ -1177,7 +1188,7 @@ static int parse_want_ref(const char *line, struct string_list *wanted_refs, struct object *o; if (read_ref(arg, &oid)) { - packet_write_fmt(1, "ERR unknown ref %s", arg); + packet_writer_error(writer, "unknown ref %s", arg); die("unknown ref %s", arg); } @@ -1220,10 +1231,11 @@ static void process_args(struct packet_reader *request, const char *p; /* process want */ - if (parse_want(arg, want_obj)) + if (parse_want(&data->writer, arg, want_obj)) continue; if (allow_ref_in_want && - parse_want_ref(arg, &data->wanted_refs, want_obj)) + parse_want_ref(&data->writer, arg, &data->wanted_refs, + want_obj)) continue; /* process have line */ if (parse_have(arg, &data->haves)) @@ -1317,26 +1329,26 @@ static int process_haves(struct oid_array *haves, struct oid_array *common, return 0; } -static int send_acks(struct oid_array *acks, struct strbuf *response, +static int send_acks(struct packet_writer *writer, struct oid_array *acks, const struct object_array *have_obj, struct object_array *want_obj) { int i; - packet_buf_write(response, "acknowledgments\n"); + packet_writer_write(writer, "acknowledgments\n"); /* Send Acks */ if (!acks->nr) - packet_buf_write(response, "NAK\n"); + packet_writer_write(writer, "NAK\n"); for (i = 0; i < acks->nr; i++) { - packet_buf_write(response, "ACK %s\n", - oid_to_hex(&acks->oid[i])); + packet_writer_write(writer, "ACK %s\n", + oid_to_hex(&acks->oid[i])); } if (ok_to_give_up(have_obj, want_obj)) { /* Send Ready */ - packet_buf_write(response, "ready\n"); + packet_writer_write(writer, "ready\n"); return 1; } @@ -1348,25 +1360,20 @@ static int process_haves_and_send_acks(struct upload_pack_data *data, struct object_array *want_obj) { struct oid_array common = OID_ARRAY_INIT; - struct strbuf response = STRBUF_INIT; int ret = 0; process_haves(&data->haves, &common, have_obj); if (data->done) { ret = 1; - } else if (send_acks(&common, &response, have_obj, want_obj)) { - packet_buf_delim(&response); + } else if (send_acks(&data->writer, &common, have_obj, want_obj)) { + packet_writer_delim(&data->writer); ret = 1; } else { /* Add Flush */ - packet_buf_flush(&response); + packet_writer_flush(&data->writer); ret = 0; } - /* Send response */ - write_or_die(1, response.buf, response.len); - strbuf_release(&response); - oid_array_clear(&data->haves); oid_array_clear(&common); return ret; @@ -1379,15 +1386,15 @@ static void send_wanted_ref_info(struct upload_pack_data *data) if (!data->wanted_refs.nr) return; - packet_write_fmt(1, "wanted-refs\n"); + packet_writer_write(&data->writer, "wanted-refs\n"); for_each_string_list_item(item, &data->wanted_refs) { - packet_write_fmt(1, "%s %s\n", - oid_to_hex(item->util), - item->string); + packet_writer_write(&data->writer, "%s %s\n", + oid_to_hex(item->util), + item->string); } - packet_delim(1); + packet_writer_delim(&data->writer); } static void send_shallow_info(struct upload_pack_data *data, @@ -1398,14 +1405,15 @@ static void send_shallow_info(struct upload_pack_data *data, !is_repository_shallow(the_repository)) return; - packet_write_fmt(1, "shallow-info\n"); + packet_writer_write(&data->writer, "shallow-info\n"); - if (!send_shallow_list(data->depth, data->deepen_rev_list, + if (!send_shallow_list(&data->writer, data->depth, + data->deepen_rev_list, data->deepen_since, &data->deepen_not, &data->shallows, want_obj) && is_repository_shallow(the_repository)) - deepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows, - want_obj); + deepen(&data->writer, INFINITE_DEPTH, data->deepen_relative, + &data->shallows, want_obj); packet_delim(1); } @@ -1467,7 +1475,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys, send_wanted_ref_info(&data); send_shallow_info(&data, &want_obj); - packet_write_fmt(1, "packfile\n"); + packet_writer_write(&data.writer, "packfile\n"); create_pack_file(&have_obj, &want_obj); state = FETCH_DONE; break; From fbd76cd450e6675cbd5d48da3c53fa446b776475 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Wed, 16 Jan 2019 11:28:13 -0800 Subject: [PATCH 4/6] sideband: reverse its dependency on pkt-line A subsequent patch will teach struct packet_reader a new field that, if set, instructs it to interpret read data as multiplexed. This will create a dependency from pkt-line to sideband. To avoid a circular dependency, split recv_sideband() into 2 parts: the reading loop (left in recv_sideband()) and the processing of the contents (in demultiplex_sideband()), and move the former into pkt-line. This reverses the direction of dependency: sideband no longer depends on pkt-line, and pkt-line now depends on sideband. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- pkt-line.c | 23 +++++++ pkt-line.h | 16 +++++ sideband.c | 177 +++++++++++++++++++++++++---------------------------- sideband.h | 24 +++++++- 4 files changed, 143 insertions(+), 97 deletions(-) diff --git a/pkt-line.c b/pkt-line.c index 9d3e402d41..321ff632a5 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -439,6 +439,29 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out) return sb_out->len - orig_len; } +int recv_sideband(const char *me, int in_stream, int out) +{ + char buf[LARGE_PACKET_MAX + 1]; + int len; + struct strbuf scratch = STRBUF_INIT; + enum sideband_type sideband_type; + + while (1) { + len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, + 0); + if (!demultiplex_sideband(me, buf, len, &scratch, + &sideband_type)) + continue; + switch (sideband_type) { + case SIDEBAND_PRIMARY: + write_or_die(out, buf + 1, len - 1); + break; + default: /* errors: message already written */ + return sideband_type; + } + } +} + /* Packet Reader Functions */ void packet_reader_init(struct packet_reader *reader, int fd, char *src_buffer, size_t src_len, diff --git a/pkt-line.h b/pkt-line.h index 023ad2951d..a8e92a4b63 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -3,6 +3,7 @@ #include "git-compat-util.h" #include "strbuf.h" +#include "sideband.h" /* * Write a packetized stream, where each line is preceded by @@ -120,6 +121,21 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size); */ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out); +/* + * Receive multiplexed output stream over git native protocol. + * in_stream is the input stream from the remote, which carries data + * in pkt_line format with band designator. Demultiplex it into out + * and err and return error appropriately. Band #1 carries the + * primary payload. Things coming over band #2 is not necessarily + * error; they are usually informative message on the standard error + * stream, aka "verbose"). A message over band #3 is a signal that + * the remote died unexpectedly. A flush() concludes the stream. + * + * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR + * or SIDEBAND_REMOTE_ERROR if an error occurred. + */ +int recv_sideband(const char *me, int in_stream, int out); + struct packet_reader { /* source file descriptor */ int fd; diff --git a/sideband.c b/sideband.c index 368647acf8..e89d677626 100644 --- a/sideband.c +++ b/sideband.c @@ -1,7 +1,6 @@ #include "cache.h" #include "color.h" #include "config.h" -#include "pkt-line.h" #include "sideband.h" #include "help.h" @@ -109,109 +108,99 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) } -/* - * Receive multiplexed output stream over git native protocol. - * in_stream is the input stream from the remote, which carries data - * in pkt_line format with band designator. Demultiplex it into out - * and err and return error appropriately. Band #1 carries the - * primary payload. Things coming over band #2 is not necessarily - * error; they are usually informative message on the standard error - * stream, aka "verbose"). A message over band #3 is a signal that - * the remote died unexpectedly. A flush() concludes the stream. - */ - #define DISPLAY_PREFIX "remote: " #define ANSI_SUFFIX "\033[K" #define DUMB_SUFFIX " " -int recv_sideband(const char *me, int in_stream, int out) +int demultiplex_sideband(const char *me, char *buf, int len, + struct strbuf *scratch, + enum sideband_type *sideband_type) { - const char *suffix; - char buf[LARGE_PACKET_MAX + 1]; - struct strbuf outbuf = STRBUF_INIT; - int retval = 0; + static const char *suffix; + const char *b, *brk; + int band; - if (isatty(2) && !is_terminal_dumb()) - suffix = ANSI_SUFFIX; - else - suffix = DUMB_SUFFIX; - - while (!retval) { - const char *b, *brk; - int band, len; - len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0); - if (len == 0) - break; - if (len < 1) { - strbuf_addf(&outbuf, - "%s%s: protocol error: no band designator", - outbuf.len ? "\n" : "", me); - retval = SIDEBAND_PROTOCOL_ERROR; - break; - } - band = buf[0] & 0xff; - buf[len] = '\0'; - len--; - switch (band) { - case 3: - strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "", - DISPLAY_PREFIX); - maybe_colorize_sideband(&outbuf, buf + 1, len); - - retval = SIDEBAND_REMOTE_ERROR; - break; - case 2: - b = buf + 1; - - /* - * Append a suffix to each nonempty line to clear the - * end of the screen line. - * - * The output is accumulated in a buffer and - * each line is printed to stderr using - * write(2) to ensure inter-process atomicity. - */ - while ((brk = strpbrk(b, "\n\r"))) { - int linelen = brk - b; - - if (!outbuf.len) - strbuf_addstr(&outbuf, DISPLAY_PREFIX); - if (linelen > 0) { - maybe_colorize_sideband(&outbuf, b, linelen); - strbuf_addstr(&outbuf, suffix); - } - - strbuf_addch(&outbuf, *brk); - xwrite(2, outbuf.buf, outbuf.len); - strbuf_reset(&outbuf); - - b = brk + 1; - } - - if (*b) { - strbuf_addstr(&outbuf, outbuf.len ? - "" : DISPLAY_PREFIX); - maybe_colorize_sideband(&outbuf, b, strlen(b)); - } - break; - case 1: - write_or_die(out, buf + 1, len); - break; - default: - strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d", - outbuf.len ? "\n" : "", me, band); - retval = SIDEBAND_PROTOCOL_ERROR; - break; - } + if (!suffix) { + if (isatty(2) && !is_terminal_dumb()) + suffix = ANSI_SUFFIX; + else + suffix = DUMB_SUFFIX; } - if (outbuf.len) { - strbuf_addch(&outbuf, '\n'); - xwrite(2, outbuf.buf, outbuf.len); + if (len == 0) { + *sideband_type = SIDEBAND_FLUSH; + goto cleanup; } - strbuf_release(&outbuf); - return retval; + if (len < 1) { + strbuf_addf(scratch, + "%s%s: protocol error: no band designator", + scratch->len ? "\n" : "", me); + *sideband_type = SIDEBAND_PROTOCOL_ERROR; + goto cleanup; + } + band = buf[0] & 0xff; + buf[len] = '\0'; + len--; + switch (band) { + case 3: + strbuf_addf(scratch, "%s%s", scratch->len ? "\n" : "", + DISPLAY_PREFIX); + maybe_colorize_sideband(scratch, buf + 1, len); + + *sideband_type = SIDEBAND_REMOTE_ERROR; + break; + case 2: + b = buf + 1; + + /* + * Append a suffix to each nonempty line to clear the + * end of the screen line. + * + * The output is accumulated in a buffer and + * each line is printed to stderr using + * write(2) to ensure inter-process atomicity. + */ + while ((brk = strpbrk(b, "\n\r"))) { + int linelen = brk - b; + + if (!scratch->len) + strbuf_addstr(scratch, DISPLAY_PREFIX); + if (linelen > 0) { + maybe_colorize_sideband(scratch, b, linelen); + strbuf_addstr(scratch, suffix); + } + + strbuf_addch(scratch, *brk); + xwrite(2, scratch->buf, scratch->len); + strbuf_reset(scratch); + + b = brk + 1; + } + + if (*b) { + strbuf_addstr(scratch, scratch->len ? + "" : DISPLAY_PREFIX); + maybe_colorize_sideband(scratch, b, strlen(b)); + } + return 0; + case 1: + *sideband_type = SIDEBAND_PRIMARY; + break; + default: + strbuf_addf(scratch, "%s%s: protocol error: bad band #%d", + scratch->len ? "\n" : "", me, band); + *sideband_type = SIDEBAND_PROTOCOL_ERROR; + break; + } + +cleanup: + if (scratch->len) { + strbuf_addch(scratch, '\n'); + xwrite(2, scratch->buf, scratch->len); + } + strbuf_release(scratch); + return 1; } /* diff --git a/sideband.h b/sideband.h index 7a8146f161..2c4f021645 100644 --- a/sideband.h +++ b/sideband.h @@ -1,10 +1,28 @@ #ifndef SIDEBAND_H #define SIDEBAND_H -#define SIDEBAND_PROTOCOL_ERROR -2 -#define SIDEBAND_REMOTE_ERROR -1 +enum sideband_type { + SIDEBAND_PROTOCOL_ERROR = -2, + SIDEBAND_REMOTE_ERROR = -1, + SIDEBAND_FLUSH = 0, + SIDEBAND_PRIMARY = 1 +}; + +/* + * Inspects a multiplexed packet read from the remote. If this packet is a + * progress packet and thus should not be processed by the caller, returns 0. + * Otherwise, returns 1, releases scratch, and sets sideband_type. + * + * If this packet is SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or a + * progress packet, also prints a message to stderr. + * + * scratch must be a struct strbuf allocated by the caller. It is used to store + * progress messages split across multiple packets. + */ +int demultiplex_sideband(const char *me, char *buf, int len, + struct strbuf *scratch, + enum sideband_type *sideband_type); -int recv_sideband(const char *me, int in_stream, int out); void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max); #endif From 0bbc0bc5745ab8b294a5faf8c3b1d939ae8b6d10 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Wed, 16 Jan 2019 11:28:14 -0800 Subject: [PATCH 5/6] {fetch,upload}-pack: sideband v2 fetch response Currently, a response to a fetch request has sideband support only while the packfile is being sent, meaning that the server cannot send notices until the start of the packfile. Extend sideband support in protocol v2 fetch responses to the whole response. upload-pack will advertise it if the uploadpack.allowsidebandall configuration variable is set, and fetch-pack will automatically request it if advertised. If the sideband is to be used throughout the whole response, upload-pack will use it to send errors instead of prefixing a PKT-LINE payload with "ERR ". This will be tested in a subsequent patch. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- Documentation/technical/protocol-v2.txt | 10 ++++++ fetch-pack.c | 12 +++++-- pkt-line.c | 43 ++++++++++++++++++------- pkt-line.h | 4 +++ sideband.c | 5 +++ sideband.h | 1 + upload-pack.c | 16 +++++++++ 7 files changed, 78 insertions(+), 13 deletions(-) diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt index 09e4e0273f..39b40c0dc1 100644 --- a/Documentation/technical/protocol-v2.txt +++ b/Documentation/technical/protocol-v2.txt @@ -307,6 +307,16 @@ the 'wanted-refs' section in the server's response as explained below. particular ref, where is the full name of a ref on the server. +If the 'sideband-all' feature is advertised, the following argument can be +included in the client's request: + + sideband-all + Instruct the server to send the whole response multiplexed, not just + the packfile section. All non-flush and non-delim PKT-LINE in the + response (not only in the packfile section) will then start with a byte + indicating its sideband (1, 2, or 3), and the server may send "0005\2" + (a PKT-LINE of sideband 2 with no payload) as a keepalive packet. + The response of `fetch` is broken into a number of sections separated by delimiter packets (0001), with each section beginning with its section header. diff --git a/fetch-pack.c b/fetch-pack.c index ee0847e6ae..86e9e18901 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1090,7 +1090,8 @@ static int add_haves(struct fetch_negotiator *negotiator, static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, const struct fetch_pack_args *args, const struct ref *wants, struct oidset *common, - int *haves_to_send, int *in_vain) + int *haves_to_send, int *in_vain, + int sideband_all) { int ret = 0; struct strbuf req_buf = STRBUF_INIT; @@ -1116,6 +1117,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, packet_buf_write(&req_buf, "include-tag"); if (prefer_ofs_delta) packet_buf_write(&req_buf, "ofs-delta"); + if (sideband_all) + packet_buf_write(&req_buf, "sideband-all"); /* Add shallow-info and deepen request */ if (server_supports_feature("fetch", "shallow", 0)) @@ -1324,6 +1327,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE | PACKET_READ_DIE_ON_ERR_PACKET); + if (server_supports_feature("fetch", "sideband-all", 0)) { + reader.use_sideband = 1; + reader.me = "fetch-pack"; + } while (state != FETCH_DONE) { switch (state) { @@ -1357,7 +1364,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, case FETCH_SEND_REQUEST: if (send_fetch_request(&negotiator, fd[1], args, ref, &common, - &haves_to_send, &in_vain)) + &haves_to_send, &in_vain, + reader.use_sideband)) state = FETCH_GET_PACK; else state = FETCH_PROCESS_ACKS; diff --git a/pkt-line.c b/pkt-line.c index 321ff632a5..d4b71d3e82 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -449,7 +449,7 @@ int recv_sideband(const char *me, int in_stream, int out) while (1) { len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0); - if (!demultiplex_sideband(me, buf, len, &scratch, + if (!demultiplex_sideband(me, buf, len, 0, &scratch, &sideband_type)) continue; switch (sideband_type) { @@ -475,25 +475,43 @@ void packet_reader_init(struct packet_reader *reader, int fd, reader->buffer = packet_buffer; reader->buffer_size = sizeof(packet_buffer); reader->options = options; + reader->me = "git"; } enum packet_read_status packet_reader_read(struct packet_reader *reader) { + struct strbuf scratch = STRBUF_INIT; + if (reader->line_peeked) { reader->line_peeked = 0; return reader->status; } - reader->status = packet_read_with_status(reader->fd, - &reader->src_buffer, - &reader->src_len, - reader->buffer, - reader->buffer_size, - &reader->pktlen, - reader->options); + /* + * Consume all progress packets until a primary payload packet is + * received + */ + while (1) { + enum sideband_type sideband_type; + reader->status = packet_read_with_status(reader->fd, + &reader->src_buffer, + &reader->src_len, + reader->buffer, + reader->buffer_size, + &reader->pktlen, + reader->options); + if (!reader->use_sideband) + break; + if (demultiplex_sideband(reader->me, reader->buffer, + reader->pktlen, 1, &scratch, + &sideband_type)) + break; + } if (reader->status == PACKET_READ_NORMAL) - reader->line = reader->buffer; + /* Skip the sideband designator if sideband is used */ + reader->line = reader->use_sideband ? + reader->buffer + 1 : reader->buffer; else reader->line = NULL; @@ -515,6 +533,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader) void packet_writer_init(struct packet_writer *writer, int dest_fd) { writer->dest_fd = dest_fd; + writer->use_sideband = 0; } void packet_writer_write(struct packet_writer *writer, const char *fmt, ...) @@ -522,7 +541,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...) va_list args; va_start(args, fmt); - packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args); + packet_write_fmt_1(writer->dest_fd, 0, + writer->use_sideband ? "\001" : "", fmt, args); va_end(args); } @@ -531,7 +551,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...) va_list args; va_start(args, fmt); - packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args); + packet_write_fmt_1(writer->dest_fd, 0, + writer->use_sideband ? "\003" : "ERR ", fmt, args); va_end(args); } diff --git a/pkt-line.h b/pkt-line.h index a8e92a4b63..ad9a4a2cd7 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -162,6 +162,9 @@ struct packet_reader { /* indicates if a line has been peeked */ int line_peeked; + + unsigned use_sideband : 1; + const char *me; }; /* @@ -201,6 +204,7 @@ extern char packet_buffer[LARGE_PACKET_MAX]; struct packet_writer { int dest_fd; + unsigned use_sideband : 1; }; void packet_writer_init(struct packet_writer *writer, int dest_fd); diff --git a/sideband.c b/sideband.c index e89d677626..6a16feb262 100644 --- a/sideband.c +++ b/sideband.c @@ -114,6 +114,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) #define DUMB_SUFFIX " " int demultiplex_sideband(const char *me, char *buf, int len, + int die_on_error, struct strbuf *scratch, enum sideband_type *sideband_type) { @@ -144,6 +145,8 @@ int demultiplex_sideband(const char *me, char *buf, int len, len--; switch (band) { case 3: + if (die_on_error) + die("remote error: %s", buf + 1); strbuf_addf(scratch, "%s%s", scratch->len ? "\n" : "", DISPLAY_PREFIX); maybe_colorize_sideband(scratch, buf + 1, len); @@ -195,6 +198,8 @@ int demultiplex_sideband(const char *me, char *buf, int len, } cleanup: + if (die_on_error && *sideband_type == SIDEBAND_PROTOCOL_ERROR) + die("%s", scratch->buf); if (scratch->len) { strbuf_addch(scratch, '\n'); xwrite(2, scratch->buf, scratch->len); diff --git a/sideband.h b/sideband.h index 2c4f021645..227740a58e 100644 --- a/sideband.h +++ b/sideband.h @@ -20,6 +20,7 @@ enum sideband_type { * progress messages split across multiple packets. */ int demultiplex_sideband(const char *me, char *buf, int len, + int die_on_error, struct strbuf *scratch, enum sideband_type *sideband_type); diff --git a/upload-pack.c b/upload-pack.c index 60a26bbbfd..765b7695d2 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -71,6 +71,8 @@ static int allow_filter; static int allow_ref_in_want; static struct list_objects_filter_options filter_options; +static int allow_sideband_all; + static void reset_timeout(void) { alarm(timeout); @@ -1046,6 +1048,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused) allow_filter = git_config_bool(var, value); } else if (!strcmp("uploadpack.allowrefinwant", var)) { allow_ref_in_want = git_config_bool(var, value); + } else if (!strcmp("uploadpack.allowsidebandall", var)) { + allow_sideband_all = git_config_bool(var, value); } if (current_config_scope() != CONFIG_SCOPE_REPO) { @@ -1284,6 +1288,11 @@ static void process_args(struct packet_reader *request, continue; } + if (allow_sideband_all && !strcmp(arg, "sideband-all")) { + data->writer.use_sideband = 1; + continue; + } + /* ignore unknown lines maybe? */ die("unexpected line: '%s'", arg); } @@ -1496,6 +1505,7 @@ int upload_pack_advertise(struct repository *r, if (value) { int allow_filter_value; int allow_ref_in_want; + int allow_sideband_all_value; strbuf_addstr(value, "shallow"); @@ -1510,6 +1520,12 @@ int upload_pack_advertise(struct repository *r, &allow_ref_in_want) && allow_ref_in_want) strbuf_addstr(value, " ref-in-want"); + + if (!repo_config_get_bool(the_repository, + "uploadpack.allowsidebandall", + &allow_sideband_all_value) && + allow_sideband_all_value) + strbuf_addstr(value, " sideband-all"); } return 1; From 07c3c2aa16370fd97551b7d1aa6af3d051e7cf8f Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Wed, 16 Jan 2019 11:28:15 -0800 Subject: [PATCH 6/6] tests: define GIT_TEST_SIDEBAND_ALL Define a GIT_TEST_SIDEBAND_ALL environment variable meant to be used from tests. When set to true, this overrides uploadpack.allowsidebandall to true, allowing the entire test suite to be run as if this configuration is in place for all repositories. As of this patch, all tests pass whether GIT_TEST_SIDEBAND_ALL is unset or set to 1. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- fetch-pack.c | 3 ++- t/README | 5 +++++ t/lib-httpd/apache.conf | 1 + t/t5537-fetch-shallow.sh | 3 ++- t/t5701-git-serve.sh | 2 +- t/t5702-protocol-v2.sh | 4 ++-- upload-pack.c | 13 ++++++++----- 7 files changed, 21 insertions(+), 10 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 86e9e18901..e98294d918 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1327,7 +1327,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE | PACKET_READ_DIE_ON_ERR_PACKET); - if (server_supports_feature("fetch", "sideband-all", 0)) { + if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 1) && + server_supports_feature("fetch", "sideband-all", 0)) { reader.use_sideband = 1; reader.me = "fetch-pack"; } diff --git a/t/README b/t/README index 28711cc508..b275c883b8 100644 --- a/t/README +++ b/t/README @@ -358,6 +358,11 @@ GIT_TEST_MULTI_PACK_INDEX=, when true, forces the multi-pack- index to be written after every 'git repack' command, and overrides the 'core.multiPackIndex' setting to true. +GIT_TEST_SIDEBAND_ALL=, when true, overrides the +'uploadpack.allowSidebandAll' setting to true, and when false, forces +fetch-pack to not request sideband-all (even if the server advertises +sideband-all). + Naming Tests ------------ diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 581c010d8f..9a6d368247 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -78,6 +78,7 @@ PassEnv GNUPGHOME PassEnv ASAN_OPTIONS PassEnv GIT_TRACE PassEnv GIT_CONFIG_NOSYSTEM +PassEnv GIT_TEST_SIDEBAND_ALL SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0 diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index 6faf17e17a..6caf628efa 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -243,7 +243,8 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f "$(git -C "$REPO" rev-parse HEAD)" \ "$(git -C "$REPO" rev-parse HEAD^)" \ >"$HTTPD_ROOT_PATH/one-time-sed" && - test_must_fail git -C client fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \ + test_must_fail env GIT_TEST_SIDEBAND_ALL=0 git -C client \ + fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \ master:a_branch && # Ensure that the one-time-sed script was used. diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh index ae79c6bbc0..fe45bf828d 100755 --- a/t/t5701-git-serve.sh +++ b/t/t5701-git-serve.sh @@ -14,7 +14,7 @@ test_expect_success 'test capability advertisement' ' 0000 EOF - git serve --advertise-capabilities >out && + GIT_TEST_SIDEBAND_ALL=0 git serve --advertise-capabilities >out && test-tool pkt-line unpack actual && test_cmp expect actual ' diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 0f2b09ebb8..b491c62e3e 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -583,8 +583,8 @@ test_expect_success 'when server does not send "ready", expect FLUSH' ' test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" git -C http_child \ -c protocol.version=2 \ fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err && - grep "fetch< acknowledgments" log && - ! grep "fetch< ready" log && + grep "fetch< .*acknowledgments" log && + ! grep "fetch< .*ready" log && test_i18ngrep "expected no other sections to be sent after no .ready." err ' diff --git a/upload-pack.c b/upload-pack.c index 765b7695d2..0c1feccaab 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1288,7 +1288,9 @@ static void process_args(struct packet_reader *request, continue; } - if (allow_sideband_all && !strcmp(arg, "sideband-all")) { + if ((git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) || + allow_sideband_all) && + !strcmp(arg, "sideband-all")) { data->writer.use_sideband = 1; continue; } @@ -1521,10 +1523,11 @@ int upload_pack_advertise(struct repository *r, allow_ref_in_want) strbuf_addstr(value, " ref-in-want"); - if (!repo_config_get_bool(the_repository, - "uploadpack.allowsidebandall", - &allow_sideband_all_value) && - allow_sideband_all_value) + if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) || + (!repo_config_get_bool(the_repository, + "uploadpack.allowsidebandall", + &allow_sideband_all_value) && + allow_sideband_all_value)) strbuf_addstr(value, " sideband-all"); }