git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 1/4] pack-protocol.txt: accept error packets in any context

From
Josh Steadmon <steadmon@google.com>
Date
Dec 12, 2018, 00:25 UTC
Message-ID
<df7d3659ae5f11d163f1e992f3b9403be709ddb7.1544572142.git.steadmon@google.com>
In-Reply-To
<cover.1544572142.git.steadmon@google.com>
From: Masaya Suzuki <masayasuzuki@google.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.

Following this protocol spec change, the error packet handling code is moved to pkt-line.c.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/technical/pack-protocol.txt | 20 +++++++++++---------
 builtin/archive.c                         |  2 --
 connect.c                                 |  3 ---
 fetch-pack.c                              |  2 --
 pkt-line.c                                |  4 ++++
 t/t5703-upload-pack-ref-in-want.sh        |  4 ++--
 6 files changed, 17 insertions(+), 18 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 d2455237ce..5d179bbd16 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -59,8 +59,6 @@ static int run_remote_archiver(int argc, const char **argv,
 	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);
 		die(_("git archive: protocol error"));
 	}
 
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 9691046e64..e66cd7b71b 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -178,8 +178,6 @@ static enum ack_type get_ack(int fd, struct object_id *result_oid)
 			return ACK;
 		}
 	}
-	if (skip_prefix(line, "ERR ", &arg))
-		die(_("remote error: %s"), arg);
 	die(_("git fetch-pack: expected ACK/NAK, got '%s'"), line);
 }
 
diff --git a/pkt-line.c b/pkt-line.c
index 04d10bbd03..ce9e42d10e 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 (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/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

-- 
2.20.0.rc2.403.gdbc3b29805-goog
Previous: Josh SteadmonNext: Jeff King
Message 28 of 41 in “remote-curl: die on server-side errors”
  1. remote-curl: die on server-side errorssteadmon@google.com, Nov 12, 2018
  2. Stefan BellerNov 12, 2018
  3. Junio C HamanoNov 13, 2018
  4. Junio C HamanoNov 13, 2018
  5. Josh SteadmonNov 13, 2018
  6. Jeff KingNov 13, 2018
  7. Josh SteadmonNov 13, 2018
  8. Jeff KingNov 14, 2018
  9. Jeff KingNov 14, 2018
  10. Josh SteadmonNov 15, 2018
  11. 0/3 remote-curl smart-http discovery cleanupJeff King, Nov 16, 2018
  12. 1/3 remote-curl: refactor smart-http discoveryJeff King, Nov 16, 2018
  13. Josh SteadmonNov 16, 2018
  14. Junio C HamanoFeb 5, 2019
  15. Jeff KingFeb 6, 2019
  16. 1/3 remote-curl: refactor smart-http discoveryJeff King, Feb 6, 2019
  17. Josh SteadmonFeb 6, 2019
  18. Junio C HamanoFeb 6, 2019
  19. Jeff KingFeb 6, 2019
  20. 2/3 remote-curl: tighten "version 2" check for smart-httpJeff King, Feb 6, 2019
  21. 3/3 t5551: test server-side ERR packetJeff King, Feb 6, 2019
  22. Junio C HamanoFeb 6, 2019
  23. 2/3 remote-curl: tighten "version 2" check for smart-httpJeff King, Nov 16, 2018
  24. Josh SteadmonNov 16, 2018
  25. 3/3 remote-curl: die on server-side errorsJeff King, Nov 16, 2018
  26. Josh SteadmonNov 16, 2018
  27. 0/4 Unify pkt-line error handling and refactor smart-httpJosh Steadmon, Dec 12, 2018
  28. 1/4 pack-protocol.txt: accept error packets in any contextJosh Steadmon, Dec 12, 2018
  29. Jeff KingDec 12, 2018
  30. Masaya SuzukiDec 13, 2018
  31. Jeff KingDec 13, 2018
  32. Josh SteadmonDec 13, 2018
  33. Jeff KingDec 17, 2018
  34. Josh SteadmonDec 19, 2018
  35. Jeff KingDec 20, 2018
  36. 2/4 remote-curl: refactor smart-http discoveryJosh Steadmon, Dec 12, 2018
  37. 3/4 remote-curl: tighten "version 2" check for smart-httpJosh Steadmon, Dec 12, 2018
  38. 4/4 lib-httpd, t5551: check server-side HTTP errorsJosh Steadmon, Dec 12, 2018
  39. Junio C HamanoDec 12, 2018
  40. Junio C HamanoNov 13, 2018
  41. Josh SteadmonNov 13, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.