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>
Masaya Suzuki committed
Dec 29, 2018 at 13:19 UTC
2d103c31c2cfcf03ff1408d639043469b0c93f70
15 files changed
+54
-34
Documentation/technical/pack-protocol.txt
+11
-9
@@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate `PKT-LINE(...)`, unless
22
otherwise noted the usual pkt-line LF rules apply: the sender SHOULD
23
include a LF, but the receiver MUST NOT complain if it is not present.
24
25
+An error packet is a special pkt-line that contains an error string.
26
+
27
+----
28
+ error-line = PKT-LINE("ERR" SP explanation-text)
29
+----
30
+
31
+Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY
32
+be sent. Once this packet is sent by a client or a server, the data transfer
33
+process defined in this protocol is terminated.
34
+
35
Transports
36
----------
37
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:
99
"0039git-upload-pack /schacon/gitbook.git\0host=example.com\0" |
100
nc -v example.com 9418
101
92
-If the server refuses the request for some reasons, it could abort
93
-gracefully with an error message.
94
-
95
-----
96
- error-line = PKT-LINE("ERR" SP explanation-text)
97
-----
98
-
102
103
SSH Transport
104
-------------
@@ -398,12 +401,11 @@ from the client).
401
Then the server will start sending its packfile data.
402
403
----
401
- server-response = *ack_multi ack / nak / error-line
404
+ server-response = *ack_multi ack / nak
405
ack_multi = PKT-LINE("ACK" SP obj-id ack_status)
406
ack_status = "continue" / "common" / "ready"
407
ack = PKT-LINE("ACK" SP obj-id)
408
nak = PKT-LINE("NAK")
406
- error-line = PKT-LINE("ERR" SP explanation-text)
409
----
410
411
A simple clone may look like this (with no 'have' lines):
builtin/archive.c
+3
-3
@@ -53,15 +53,15 @@ static int run_remote_archiver(int argc, const char **argv,
53
packet_write_fmt(fd[1], "argument %s\n", argv[i]);
54
packet_flush(fd[1]);
55
56
- packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);
56
+ packet_reader_init(&reader, fd[0], NULL, 0,
57
+ PACKET_READ_CHOMP_NEWLINE |
58
+ PACKET_READ_DIE_ON_ERR_PACKET);
59
60
if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
61
die(_("git archive: expected ACK/NAK, got a flush packet"));
62
if (strcmp(reader.line, "ACK")) {
63
if (starts_with(reader.line, "NACK "))
64
die(_("git archive: NACK %s"), reader.line + 5);
63
- if (starts_with(reader.line, "ERR "))
64
- die(_("remote error: %s"), reader.line + 4);
65
die(_("git archive: protocol error"));
66
}
67
builtin/fetch-pack.c
+2
-1
@@ -217,7 +217,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
217
218
packet_reader_init(&reader, fd[0], NULL, 0,
219
PACKET_READ_CHOMP_NEWLINE |
220
- PACKET_READ_GENTLE_ON_EOF);
220
+ PACKET_READ_GENTLE_ON_EOF |
221
+ PACKET_READ_DIE_ON_ERR_PACKET);
222
223
switch (discover_version(&reader)) {
224
case protocol_v2:
builtin/receive-pack.c
+3
-1
@@ -1986,7 +1986,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
1986
if (advertise_refs)
1987
return 0;
1988
1989
- packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);
1989
+ packet_reader_init(&reader, 0, NULL, 0,
1990
+ PACKET_READ_CHOMP_NEWLINE |
1991
+ PACKET_READ_DIE_ON_ERR_PACKET);
1992
1993
if ((commands = read_head_info(&reader, &shallow)) != NULL) {
1994
const char *unpack_status = NULL;
builtin/send-pack.c
+2
-1
@@ -250,7 +250,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
250
251
packet_reader_init(&reader, fd[0], NULL, 0,
252
PACKET_READ_CHOMP_NEWLINE |
253
- PACKET_READ_GENTLE_ON_EOF);
253
+ PACKET_READ_GENTLE_ON_EOF |
254
+ PACKET_READ_DIE_ON_ERR_PACKET);
255
256
switch (discover_version(&reader)) {
257
case protocol_v2:
connect.c
-3
@@ -296,7 +296,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,
296
struct ref **orig_list = list;
297
int len = 0;
298
enum get_remote_heads_state state = EXPECTING_FIRST_REF;
299
- const char *arg;
299
300
*list = NULL;
301
@@ -306,8 +305,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,
305
die_initial_contact(1);
306
case PACKET_READ_NORMAL:
307
len = reader->pktlen;
309
- if (len > 4 && skip_prefix(reader->line, "ERR ", &arg))
310
- die(_("remote error: %s"), arg);
308
break;
309
case PACKET_READ_FLUSH:
310
state = EXPECTING_DONE;
fetch-pack.c
+4
-4
@@ -182,8 +182,6 @@ static enum ack_type get_ack(struct packet_reader *reader,
182
return ACK;
183
}
184
}
185
- if (skip_prefix(reader->line, "ERR ", &arg))
186
- die(_("remote error: %s"), arg);
185
die(_("git fetch-pack: expected ACK/NAK, got '%s'"), reader->line);
186
}
187
@@ -258,7 +256,8 @@ static int find_common(struct fetch_negotiator *negotiator,
256
die(_("--stateless-rpc requires multi_ack_detailed"));
257
258
packet_reader_init(&reader, fd[0], NULL, 0,
261
- PACKET_READ_CHOMP_NEWLINE);
259
+ PACKET_READ_CHOMP_NEWLINE |
260
+ PACKET_READ_DIE_ON_ERR_PACKET);
261
262
if (!args->no_dependents) {
263
mark_tips(negotiator, args->negotiation_tips);
@@ -1358,7 +1357,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
1357
struct fetch_negotiator negotiator;
1358
fetch_negotiator_init(&negotiator, negotiation_algorithm);
1359
packet_reader_init(&reader, fd[0], NULL, 0,
1361
- PACKET_READ_CHOMP_NEWLINE);
1360
+ PACKET_READ_CHOMP_NEWLINE |
1361
+ PACKET_READ_DIE_ON_ERR_PACKET);
1362
1363
while (state != FETCH_DONE) {
1364
switch (state) {
pkt-line.c
+4
@@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
346
return PACKET_READ_EOF;
347
}
348
349
+ if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
350
+ starts_with(buffer, "ERR "))
351
+ die(_("remote error: %s"), buffer + 4);
352
+
353
if ((options & PACKET_READ_CHOMP_NEWLINE) &&
354
len && buffer[len-1] == '\n')
355
len--;
pkt-line.h
+6
-2
@@ -62,9 +62,13 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
62
*
63
* If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if
64
* present) is removed from the buffer before returning.
65
+ *
66
+ * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an
67
+ * ERR packet.
68
*/
66
-#define PACKET_READ_GENTLE_ON_EOF (1u<<0)
67
-#define PACKET_READ_CHOMP_NEWLINE (1u<<1)
69
+#define PACKET_READ_GENTLE_ON_EOF (1u<<0)
70
+#define PACKET_READ_CHOMP_NEWLINE (1u<<1)
71
+#define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)
72
int packet_read(int fd, char **src_buffer, size_t *src_len, char
73
*buffer, unsigned size, int options);
74
remote-curl.c
+6
-3
@@ -204,7 +204,8 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
204
205
packet_reader_init(&reader, -1, heads->buf, heads->len,
206
PACKET_READ_CHOMP_NEWLINE |
207
- PACKET_READ_GENTLE_ON_EOF);
207
+ PACKET_READ_GENTLE_ON_EOF |
208
+ PACKET_READ_DIE_ON_ERR_PACKET);
209
210
heads->version = discover_version(&reader);
211
switch (heads->version) {
@@ -411,7 +412,8 @@ static struct discovery *discover_refs(const char *service, int for_push)
412
!strbuf_cmp(&exp, &type)) {
413
struct packet_reader reader;
414
packet_reader_init(&reader, -1, last->buf, last->len,
414
- PACKET_READ_CHOMP_NEWLINE);
415
+ PACKET_READ_CHOMP_NEWLINE |
416
+ PACKET_READ_DIE_ON_ERR_PACKET);
417
418
/*
419
* smart HTTP response; validate that the service
@@ -1182,7 +1184,8 @@ static void proxy_state_init(struct proxy_state *p, const char *service_name,
1184
p->headers = curl_slist_append(p->headers, buf.buf);
1185
1186
packet_reader_init(&p->reader, p->in, NULL, 0,
1185
- PACKET_READ_GENTLE_ON_EOF);
1187
+ PACKET_READ_GENTLE_ON_EOF |
1188
+ PACKET_READ_DIE_ON_ERR_PACKET);
1189
1190
strbuf_release(&buf);
1191
}
send-pack.c
+3
-1
@@ -558,7 +558,9 @@ int send_pack(struct send_pack_args *args,
558
in = demux.out;
559
}
560
561
- packet_reader_init(&reader, in, NULL, 0, PACKET_READ_CHOMP_NEWLINE);
561
+ packet_reader_init(&reader, in, NULL, 0,
562
+ PACKET_READ_CHOMP_NEWLINE |
563
+ PACKET_READ_DIE_ON_ERR_PACKET);
564
565
if (need_pack_data && cmds_sent) {
566
if (pack_objects(out, remote_refs, extra_have, args) < 0) {
serve.c
+3
-2
@@ -167,7 +167,8 @@ static int process_request(void)
167
168
packet_reader_init(&reader, 0, NULL, 0,
169
PACKET_READ_CHOMP_NEWLINE |
170
- PACKET_READ_GENTLE_ON_EOF);
170
+ PACKET_READ_GENTLE_ON_EOF |
171
+ PACKET_READ_DIE_ON_ERR_PACKET);
172
173
/*
174
* Check to see if the client closed their end before sending another
@@ -175,7 +176,7 @@ static int process_request(void)
176
*/
177
if (packet_reader_peek(&reader) == PACKET_READ_EOF)
178
return 1;
178
- reader.options = PACKET_READ_CHOMP_NEWLINE;
179
+ reader.options &= ~PACKET_READ_GENTLE_ON_EOF;
180
181
while (state != PROCESS_REQUEST_DONE) {
182
switch (packet_reader_peek(&reader)) {
t/t5703-upload-pack-ref-in-want.sh
+2
-2
@@ -208,7 +208,7 @@ test_expect_success 'server is initially ahead - no ref in want' '
208
cp -r "$LOCAL_PRISTINE" local &&
209
inconsistency master 1234567890123456789012345678901234567890 &&
210
test_must_fail git -C local fetch 2>err &&
211
- grep "ERR upload-pack: not our ref" err
211
+ grep "fatal: remote error: upload-pack: not our ref" err
212
'
213
214
test_expect_success 'server is initially ahead - ref in want' '
@@ -254,7 +254,7 @@ test_expect_success 'server loses a ref - ref in want' '
254
echo "s/master/raster/" >"$HTTPD_ROOT_PATH/one-time-sed" &&
255
test_must_fail git -C local fetch 2>err &&
256
257
- grep "ERR unknown ref refs/heads/raster" err
257
+ grep "fatal: remote error: unknown ref refs/heads/raster" err
258
'
259
260
stop_httpd
transport.c
+2
-1
@@ -273,7 +273,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
273
274
packet_reader_init(&reader, data->fd[0], NULL, 0,
275
PACKET_READ_CHOMP_NEWLINE |
276
- PACKET_READ_GENTLE_ON_EOF);
276
+ PACKET_READ_GENTLE_ON_EOF |
277
+ PACKET_READ_DIE_ON_ERR_PACKET);
278
279
data->version = discover_version(&reader);
280
switch (data->version) {
upload-pack.c
+3
-1
@@ -1078,7 +1078,9 @@ void upload_pack(struct upload_pack_options *options)
1078
if (options->advertise_refs)
1079
return;
1080
1081
- packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);
1081
+ packet_reader_init(&reader, 0, NULL, 0,
1082
+ PACKET_READ_CHOMP_NEWLINE |
1083
+ PACKET_READ_DIE_ON_ERR_PACKET);
1084
1085
receive_needs(&reader, &want_obj);
1086
if (want_obj.nr) {