| Message ID | 20181229211915.161686-3-masayasuzuki@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Accept error packets in any context | expand |
First of all, there was some discussion [1] about its relation with js/smart-http-detect-remote-error. I noticed that js/smart-http-detect-remote-error has all tests passing even if I remove Masaya's patch, so I'm reviewing these patches independently, directly on master. [1] https://public-inbox.org/git/CAJB1erU0utjKGtv3LBFT6SEEKCfFRuxGvDtpkKeS3GSC1S89JA@mail.gmail.com/ > 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. The other thing that happens is that servers send "ERR" anyway, even though it is not allowed by the protocol. This overall looks like a good direction - this makes explicit something that is already being done. My remaining concern is if "ERR " could be a non-error packet mistaken for an error one. I glanced through pack-protocol.txt and as far as I can tell, I don't see anything non-error sent by the server that could be prefixed with "ERR". (There are push-option and gpg-signature-lines, but those are sent by the client.) Packfiles can contain anything, of course, but as far as I can tell, they are either sent un-PKT-ed or preceded by a sideband (\1), so they are fine. > diff --git a/serve.c b/serve.c > index bda085f09..317256c1a 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; Here, the old line is meant to remove PACKET_READ_GENTLE_ON_EOF - the new line is both necessary and clearer. > diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh > index 3f58f05cb..d2a9d0c12 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 > ' And this shows that we have tests that exercise the new code. The rest of the diff is just the addition of the new PACKET_READ_DIE_ON_ERR_PACKET, and looks correct.
On 2018.12.29 13:19, Masaya Suzuki wrote: > 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 <masayasuzuki@google.com> > --- > 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 6ac774d5f..7a2375a55 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 2fe1f05ca..45d11669a 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 63e69a580..85dbc2af8 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 81cc07370..d58b7750b 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 8e3c7490f..098ebf22d 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 24281b608..4813f005a 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 86790b9bb..3f9626dbf 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 04d10bbd0..e70ea6d88 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 5b28d4347..d7e1dbc04 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 bbd9ba0f3..10b50869c 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 913645046..7b9829f16 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 bda085f09..317256c1a 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 3f58f05cb..d2a9d0c12 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 5a74b609f..12db4251c 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 1638825ee..08b547cf0 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) { > -- > 2.20.1.415.g653613c723-goog > This all looks good to me. Reviewed-by: Josh Steadmon <steadmon@google.com>
diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt index 6ac774d5f..7a2375a55 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 2fe1f05ca..45d11669a 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 63e69a580..85dbc2af8 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 81cc07370..d58b7750b 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 8e3c7490f..098ebf22d 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 24281b608..4813f005a 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 86790b9bb..3f9626dbf 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 04d10bbd0..e70ea6d88 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 5b28d4347..d7e1dbc04 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 bbd9ba0f3..10b50869c 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 913645046..7b9829f16 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 bda085f09..317256c1a 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 3f58f05cb..d2a9d0c12 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 5a74b609f..12db4251c 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 1638825ee..08b547cf0 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) {
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 <masayasuzuki@google.com> --- 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(-)