Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 30, 2020, 18:42 UTC
Message-ID: <xmqq7dr7369a.fsf@gitster.c.googlers.com>
In-Reply-To: <1604029402-19593-1-git-send-email-dan@mutual.io>

Daniel Duvall <dan@mutual.io> writes:

Show 21 quoted lines

> diff --git a/upload-pack.c b/upload-pack.c
> index 3b858eb..2b128e4 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1344,7 +1344,18 @@ void upload_pack(struct upload_pack_options *options)
>  				   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  		receive_needs(&data, &reader);
> -		if (data.want_obj.nr) {
> +
> +		/*
> +		 * An EOF at this exact point in negotiation should be
> +		 * acceptable from stateless clients as they will consume the
> +		 * shallow list before doing subsequent rpc with haves/etc.
> +		 */
> +		if (data.stateless_rpc)
> +			reader.options |= PACKET_READ_GENTLE_ON_EOF;
> +
> +		if (data.want_obj.nr &&
> +		    packet_reader_peek(&reader) != PACKET_READ_EOF) {
> +			reader.options ^= PACKET_READ_GENTLE_ON_EOF;

I am a bit puzzled why it is sensible to

  (1) unconditionally
  (2) toggle

the GENTLE bit.

>  			get_common_commits(&data, &reader);
>  			create_pack_file(&data, NULL);
>  		}

If we are not doing stateless_rpc, we call get_common_commits() with a reader that is gentle on eof, which is not an intended behaviour change, no?

I would have understood if this were more like

	if (data.stateless_rpc)
		reader.options |= PACKET_READ_GENTLE_ON_EOF;

	if (data.want_obj.nr &&
	    packet_reader_peek(&reader) != PACKET_READ_EOF) {
		if (data.stateless_rpc)
			reader.options &= ~PACKET_READ_GENTLE_ON_EOF;

i.e. only when we know we set the bit when the bit was originally clear, revert to the original state.

	Note. initially I thought we may need to check the original
	value of the bit in reader.options before flipping it on,
	but this packet_reader has freshly been initialized in the
	inner block we see here, so we know that nobody other than
	this new code would have set the bit.

Or for that matter, just unconditionally turn it off, e.g.

	if (data.want_obj.nr &&
	    packet_reader_peek(&reader) != PACKET_READ_EOF) {
		reader.options &= ~PACKET_READ_GENTLE_ON_EOF;

Puzzled...