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 <masayasuzuki@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 6ac774d5f665614848cdaba9e4c97d86a2453913..7a2375a55d074514c5ebd851fe55ff27541c207c 100644 (file)
--- 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 2fe1f05ca2021f76c4388269fccddf31b7e6fc53..45d11669aae459caf95bb6b4737558297debae89 100644 (file)
--- 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 63e69a58011a4d3c774cd3c81ae9f10e3a96efe6..85dbc2af87b9098ef3c37274a6fd3dd033f6cc5f 100644 (file)
--- 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 81cc07370022647c6d08eecb95f50ae35a99526e..d58b7750b6b46565ca4ebb8299e5260943aa364e 100644 (file)
--- 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 8e3c7490f70df79497ee064ab13c0164de11fe9e..098ebf22d0d65a6f98606c7ea567467641976f6b 100644 (file)
--- 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 24281b608284ee74b262237c467ff054874d8a8e..4813f005ab05279a72ef2894cfae8887965d9640 100644 (file)
--- 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 86790b9bbad5a97c2dd4afaa42f9ccfca406b1b9..3f9626dbf70ad126d2ae430d58094987d05886b6 100644 (file)
--- 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 04d10bbd037b393574f8453cbd00f94e0b2e7e99..e70ea6d88ff927e956a1f89941b55f5521d5e3f0 100644 (file)
--- 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 5b28d43472db41a59f0a44845953f163748593b0..d7e1dbc0470088edb94a65bc32749e5e884b6207 100644 (file)
--- 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 bbd9ba0f3586dc19d42298c55a6adbaf25b9edc3..10b50869c5841d8443d4b94ffb3e1e3ce5187bae 100644 (file)
--- 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 9136450464b4c598e229a97419b5be6a55962e46..7b9829f165e7aff7abe804fb780cf7c9d88aaf73 100644 (file)
--- 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 bda085f09c8e10314c2c497dbca978fd9241e7b5..317256c1a493c4b2cc730a6212add4d1ef5696d4 100644 (file)
--- 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 3f58f05cbb49d3b4a7f50e946e0f46bbc953140a..d2a9d0c127d6ce43b0a23fae94df84cdddc72469 100755 (executable)
--- 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 5a74b609ffad51752ec91cc93c602f00292288d1..12db4251c15c088bdad4601418cb59e8e2f03b6d 100644 (file)
--- 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 1638825ee85c6875d7bf60658cea9ea62bec4245..08b547cf01bffe219140a7c3fa5ffb855b82058f 100644 (file)
--- 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) {