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@xxxxxxxxxxxxxx/ > 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.
