Re: [PATCH v2 2/2] pack-protocol.txt: accept error packets in any context

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jan 7, 2019, 19:41 UTC
Message-ID: <20190107194150.58700-1-jonathantanmy@google.com>
In-Reply-To: <20181229211915.161686-3-masayasuzuki@google.com>

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@mail.gmail.com/

Show 12 quoted lines

> 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.

Show 20 quoted lines

> 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.

Show 20 quoted lines

> 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.