{"thread":{"id":"49896","subject":"[PATCH] pack-protocol.txt: accept error packets in any context","startedAt":"2018-11-27T04:53:06Z","lastAt":"2018-12-03T23:59:34Z","messageCount":7,"participants":["Masaya Suzuki","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"364117","messageId":"20181127045301.103807-1-masayasuzuki@google.com","threadId":"49896","inReplyTo":null,"subject":"[PATCH] pack-protocol.txt: accept error packets in any context","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-11-27T04:53:01Z","receivedAt":"2018-11-27T04:53:06Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"In the Git pack protocol definition, an error packet may appear only in\na certain context. However, servers can face a runtime error (e.g. I/O\nerror) at an arbitrary timing. This patch changes the protocol to allow\nan error packet to be sent instead of any packet.\n\nFollowing this protocol spec change, the error packet handling code is\nmoved to pkt-line.c.\n\nSigned-off-by: Masaya Suzuki \n---\n Documentation/technical/pack-protocol.txt | 20 +++++++++++---------\n builtin/archive.c | 2 --\n connect.c | 2 --\n fetch-pack.c | 2 --\n pkt-line.c | 4 ++++\n 5 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 6ac774d5f..7a2375a55 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate `PKT-LINE(...)`, unless\n otherwise noted the usual pkt-line LF rules apply: the sender SHOULD\n include a LF, but the receiver MUST NOT complain if it is not present.\n \n+An error packet is a special pkt-line that contains an error string.\n+\n+----\n+ error-line = PKT-LINE(\"ERR\" SP explanation-text)\n+----\n+\n+Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY\n+be sent. Once this packet is sent by a client or a server, the data transfer\n+process defined in this protocol is terminated.\n+\n Transports\n ----------\n There are three transports over which the packfile protocol is\n@@ -89,13 +99,6 @@ process on the server side over the Git protocol is this:\n \"0039git-upload-pack /schacon/gitbook.git\\0host=example.com\\0\" |\n nc -v example.com 9418\n \n-If the server refuses the request for some reasons, it could abort\n-gracefully with an error message.\n-\n-----\n- error-line = PKT-LINE(\"ERR\" SP explanation-text)\n-----\n-\n \n SSH Transport\n -------------\n@@ -398,12 +401,11 @@ from the client).\n Then the server will start sending its packfile data.\n \n ----\n- server-response = *ack_multi ack / nak / error-line\n+ server-response = *ack_multi ack / nak\n ack_multi = PKT-LINE(\"ACK\" SP obj-id ack_status)\n ack_status = \"continue\" / \"common\" / \"ready\"\n ack = PKT-LINE(\"ACK\" SP obj-id)\n nak = PKT-LINE(\"NAK\")\n- error-line = PKT-LINE(\"ERR\" SP explanation-text)\n ----\n \n A simple clone may look like this (with no 'have' lines):\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex d2455237c..5d179bbd1 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -59,8 +59,6 @@ static int run_remote_archiver(int argc, const char **argv,\n \tif (strcmp(buf, \"ACK\")) {\n \t\tif (starts_with(buf, \"NACK \"))\n \t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n-\t\tif (starts_with(buf, \"ERR \"))\n-\t\t\tdie(_(\"remote error: %s\"), buf + 4);\n \t\tdie(_(\"git archive: protocol error\"));\n \t}\n \ndiff --git a/connect.c b/connect.c\nindex 24281b608..458906e60 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -306,8 +306,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n \t\t\tdie_initial_contact(1);\n \t\tcase PACKET_READ_NORMAL:\n \t\t\tlen = reader->pktlen;\n-\t\t\tif (len > 4 && skip_prefix(reader->line, \"ERR \", &arg))\n-\t\t\t\tdie(_(\"remote error: %s\"), arg);\n \t\t\tbreak;\n \t\tcase PACKET_READ_FLUSH:\n \t\t\tstate = EXPECTING_DONE;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 9691046e6..e66cd7b71 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -178,8 +178,6 @@ static enum ack_type get_ack(int fd, struct object_id *result_oid)\n \t\t\treturn ACK;\n \t\t}\n \t}\n-\tif (skip_prefix(line, \"ERR \", &arg))\n-\t\tdie(_(\"remote error: %s\"), arg);\n \tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), line);\n }\n \ndiff --git a/pkt-line.c b/pkt-line.c\nindex 04d10bbd0..ce9e42d10 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\treturn PACKET_READ_EOF;\n \t}\n \n+\tif (starts_with(buffer, \"ERR \")) {\n+\t\tdie(_(\"remote error: %s\"), buffer + 4);\n+\t}\n+\n \tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n \t len && buffer[len-1] == '\\n')\n \t\tlen--;\n-- \n2.20.0.rc0.387.gc7a69e6b6c-goog\n\n"},{"id":"364119","messageId":"xmqqh8g3t7gm.fsf@gitster-ct.c.googlers.com","threadId":"49896","inReplyTo":"20181127045301.103807-1-masayasuzuki@google.com","subject":"Re: [PATCH] pack-protocol.txt: accept error packets in any context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-27T05:28:25Z","receivedAt":"2018-11-27T05:28:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Masaya Suzuki writes:\n\n> In the Git pack protocol definition, an error packet may appear only in\n> a certain context. However, servers can face a runtime error (e.g. I/O\n> error) at an arbitrary timing. This patch changes the protocol to allow\n> an error packet to be sent instead of any packet.\n\nMakes perfect sense.\n\n> Following this protocol spec change, the error packet handling code is\n> moved to pkt-line.c.\n>\n> Signed-off-by: Masaya Suzuki \n> ---\n> Documentation/technical/pack-protocol.txt | 20 +++++++++++---------\n> builtin/archive.c | 2 --\n> connect.c | 2 --\n> fetch-pack.c | 2 --\n> pkt-line.c | 4 ++++\n> 5 files changed, 15 insertions(+), 15 deletions(-)\n>\n> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\n> index 6ac774d5f..7a2375a55 100644\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate `PKT-LINE(...)`, unless\n> otherwise noted the usual pkt-line LF rules apply: the sender SHOULD\n> include a LF, but the receiver MUST NOT complain if it is not present.\n> \n> +An error packet is a special pkt-line that contains an error string.\n> +\n> +----\n> + error-line = PKT-LINE(\"ERR\" SP explanation-text)\n> +----\n> +\n> +Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY\n> +be sent. Once this packet is sent by a client or a server, the data transfer\n> +process defined in this protocol is terminated.\n> +\n> Transports\n> ----------\n> There are three transports over which the packfile protocol is\n> @@ -89,13 +99,6 @@ process on the server side over the Git protocol is this:\n> \"0039git-upload-pack /schacon/gitbook.git\\0host=example.com\\0\" |\n> nc -v example.com 9418\n> \n> -If the server refuses the request for some reasons, it could abort\n> -gracefully with an error message.\n> -\n> -----\n> - error-line = PKT-LINE(\"ERR\" SP explanation-text)\n> -----\n> -\n> \n> SSH Transport\n> -------------\n> @@ -398,12 +401,11 @@ from the client).\n> Then the server will start sending its packfile data.\n> \n> ----\n> - server-response = *ack_multi ack / nak / error-line\n> + server-response = *ack_multi ack / nak\n> ack_multi = PKT-LINE(\"ACK\" SP obj-id ack_status)\n> ack_status = \"continue\" / \"common\" / \"ready\"\n> ack = PKT-LINE(\"ACK\" SP obj-id)\n> nak = PKT-LINE(\"NAK\")\n> - error-line = PKT-LINE(\"ERR\" SP explanation-text)\n> ----\n> \n> A simple clone may look like this (with no 'have' lines):\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index d2455237c..5d179bbd1 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -59,8 +59,6 @@ static int run_remote_archiver(int argc, const char **argv,\n> \tif (strcmp(buf, \"ACK\")) {\n> \t\tif (starts_with(buf, \"NACK \"))\n> \t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n> -\t\tif (starts_with(buf, \"ERR \"))\n> -\t\t\tdie(_(\"remote error: %s\"), buf + 4);\n> \t\tdie(_(\"git archive: protocol error\"));\n> \t}\n> \n> diff --git a/connect.c b/connect.c\n> index 24281b608..458906e60 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -306,8 +306,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n> \t\t\tdie_initial_contact(1);\n> \t\tcase PACKET_READ_NORMAL:\n> \t\t\tlen = reader->pktlen;\n> -\t\t\tif (len > 4 && skip_prefix(reader->line, \"ERR \", &arg))\n> -\t\t\t\tdie(_(\"remote error: %s\"), arg);\n> \t\t\tbreak;\n> \t\tcase PACKET_READ_FLUSH:\n> \t\t\tstate = EXPECTING_DONE;\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 9691046e6..e66cd7b71 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -178,8 +178,6 @@ static enum ack_type get_ack(int fd, struct object_id *result_oid)\n> \t\t\treturn ACK;\n> \t\t}\n> \t}\n> -\tif (skip_prefix(line, \"ERR \", &arg))\n> -\t\tdie(_(\"remote error: %s\"), arg);\n> \tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), line);\n> }\n> \n> diff --git a/pkt-line.c b/pkt-line.c\n> index 04d10bbd0..ce9e42d10 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> \t\treturn PACKET_READ_EOF;\n> \t}\n> \n> +\tif (starts_with(buffer, \"ERR \")) {\n> +\t\tdie(_(\"remote error: %s\"), buffer + 4);\n> +\t}\n> +\n> \tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n> \t len && buffer[len-1] == '\\n')\n> \t\tlen--;\n"},{"id":"364296","messageId":"xmqq1s74nx1n.fsf@gitster-ct.c.googlers.com","threadId":"49896","inReplyTo":"xmqqh8g3t7gm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] pack-protocol.txt: accept error packets in any context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-29T07:49:08Z","receivedAt":"2018-11-29T07:49:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> diff --git a/connect.c b/connect.c\n>> index 24281b608..458906e60 100644\n>> --- a/connect.c\n>> +++ b/connect.c\n>> @@ -306,8 +306,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n>> \t\t\tdie_initial_contact(1);\n>> \t\tcase PACKET_READ_NORMAL:\n>> \t\t\tlen = reader->pktlen;\n>> -\t\t\tif (len > 4 && skip_prefix(reader->line, \"ERR \", &arg))\n>> -\t\t\t\tdie(_(\"remote error: %s\"), arg);\n>> \t\t\tbreak;\n>> \t\tcase PACKET_READ_FLUSH:\n>> \t\t\tstate = EXPECTING_DONE;\n\nThis breaks build by not removing the decl of arg while removing the\nlast user of that variable in the function.\n\n connect.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/connect.c b/connect.c\nindex 458906e60d..4813f005ab 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -296,7 +296,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n \tstruct ref **orig_list = list;\n \tint len = 0;\n \tenum get_remote_heads_state state = EXPECTING_FIRST_REF;\n-\tconst char *arg;\n \n \t*list = NULL;\n \n-- \n2.20.0-rc1-10-g7068cbc4ab\n\n"},{"id":"364298","messageId":"xmqqwoowmfzt.fsf@gitster-ct.c.googlers.com","threadId":"49896","inReplyTo":"20181127045301.103807-1-masayasuzuki@google.com","subject":"Re: [PATCH] pack-protocol.txt: accept error packets in any context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-29T08:42:46Z","receivedAt":"2018-11-29T08:42:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Masaya Suzuki writes:\n\n> In the Git pack protocol definition, an error packet may appear only in\n> a certain context. However, servers can face a runtime error (e.g. I/O\n> error) at an arbitrary timing. This patch changes the protocol to allow\n> an error packet to be sent instead of any packet.\n>\n> Following this protocol spec change, the error packet handling code is\n> moved to pkt-line.c.\n>\n> Signed-off-by: Masaya Suzuki \n> ---\n\nHave you run tests with this applied? I noticed 5703.9 now has\nstale expectations, but there may be others.\n"},{"id":"364343","messageId":"CAJB1erVBMq84gXir1YD=3R_RXS+qXYAsPKo6ecEJBBNv-4JOFQ@mail.gmail.com","threadId":"49896","inReplyTo":"xmqqwoowmfzt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] pack-protocol.txt: accept error packets in any context","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-11-29T16:10:27Z","receivedAt":"2018-11-29T16:10:43Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"On Thu, Nov 29, 2018 at 3:42 AM Junio C Hamano wrote:\n>\n> Masaya Suzuki writes:\n>\n> > In the Git pack protocol definition, an error packet may appear only in\n> > a certain context. However, servers can face a runtime error (e.g. I/O\n> > error) at an arbitrary timing. This patch changes the protocol to allow\n> > an error packet to be sent instead of any packet.\n> >\n> > Following this protocol spec change, the error packet handling code is\n> > moved to pkt-line.c.\n> >\n> > Signed-off-by: Masaya Suzuki \n> > ---\n>\n> Have you run tests with this applied? I noticed 5703.9 now has\n> stale expectations, but there may be others.\n\nYes, I did. And it also didn't end up in a build error. Do I have a\ndifferent build option...?\n"},{"id":"364374","messageId":"xmqqsgzjmjho.fsf@gitster-ct.c.googlers.com","threadId":"49896","inReplyTo":"CAJB1erVBMq84gXir1YD=3R_RXS+qXYAsPKo6ecEJBBNv-4JOFQ@mail.gmail.com","subject":"Re: [PATCH] pack-protocol.txt: accept error packets in any context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-30T01:39:31Z","receivedAt":"2018-11-30T01:39:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Masaya Suzuki writes:\n\n> Yes, I did. And it also didn't end up in a build error. Do I have a\n> different build option...?\n\nPassig DEVELOPER=Yes to make turns a bit more warnings on (in this\ncase, I think it was \"unused-variable\") and also uses -Werror to\nturn warnings into errors.\n"},{"id":"364510","messageId":"CAGZ79karTiSh94qwgrL76qOQ_W_bEK=2y3UvSsdz+cHpUXXqeQ@mail.gmail.com","threadId":"49896","inReplyTo":"20181127045301.103807-1-masayasuzuki@google.com","subject":"Re: [PATCH] pack-protocol.txt: accept error packets in any context","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-03T23:59:19Z","receivedAt":"2018-12-03T23:59:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> diff --git a/pkt-line.c b/pkt-line.c\n> index 04d10bbd0..ce9e42d10 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> return PACKET_READ_EOF;\n> }\n>\n> + if (starts_with(buffer, \"ERR \")) {\n> + die(_(\"remote error: %s\"), buffer + 4);\n> + }\n> +\n\nHandling any ERR line in the pkt reader is okay, as\n* we do not pkt-ize pack files and\n* we do not have any other parts of the protocol where user data is in\n the first four bytes, which could randomly match this pattern and\n* the rest of the pkt-ized part of the protocol never sends\n ERR lines.\n\nMakes sense.\n"}]}