Re: [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Jonathan Tan wrote:

> When fetching, Git stops negotiation when it has sent at least
> MAX_IN_VAIN (which is 256) "have" lines without having any of them
> ACK-ed. But this is supposed to trigger only after the first ACK, as
> pack-protocol.txt says:
> 
>   However, the 256 limit *only* turns on in the canonical client
>   implementation if we have received at least one "ACK %s continue"
>   during a prior round.  This helps to ensure that at least one common
>   ancestor is found before we give up entirely.
> 
> The code path for protocol v0 observes this, but not protocol v2,
> resulting in shorter negotiation rounds but significantly larger
> packfiles. Teach the code path for protocol v2 to check this criterion
> only after at least one ACK was received.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c          | 13 +++++++++----
>  t/t5500-fetch-pack.sh | 19 +++++++++++++++++++
>  2 files changed, 28 insertions(+), 4 deletions(-)

Makes a lot of sense.  Good find.

[...]
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
[...]
> @@ -1513,6 +1517,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  				break;
>  			case 1:
>  				in_vain = 0;
> +				seen_ack = 1;

not about this patch: can these return values from process_acks be made
into an enum with named enumerators?  That would make what's happening
in the call site more obvious.

>  				/* fallthrough */
>  			default:
>  				state = FETCH_SEND_REQUEST;
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index baa1a99f45..fcc5cc8ab0 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -385,6 +385,25 @@ test_expect_success 'clone shallow with packed refs' '
>  	test_cmp count8.expected count8.actual
>  '
>  
> +test_expect_success 'in_vain not triggered before first ACK' '
> +	rm -rf myserver myclient trace &&
> +	git init myserver &&
> +	test_commit -C myserver foo &&
> +	git clone "file://$(pwd)/myserver" myclient &&
> +
> +	# MAX_IN_VAIN is 256. Because of batching, the client will send 496
> +	# (16+32+64+128+256) commits, not 256, before giving up. So create 496
> +	# irrelevant commits.
> +	test_commit_bulk -C myclient 496 &&
> +
> +	# The new commit that the client wants to fetch.
> +	test_commit -C myserver bar &&
> +
> +	GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin &&
> +	cp trace /tmp/x &&

Leftover debugging line?

> +	test_i18ngrep "Total 3 " trace

Clever.

In some sense this is a fragile test, since the server could change
how it reports progress some day.  Would it make sense (perhaps as a
followup patch) for this to use a trace2 log instead?  For example,
if we turn on tracing in the server, then since 9ed8790282
(pack-objects: write objects packed to trace2, 2019-04-11) it will
report how many objects were in the pack it wrote.

After removing the "cp trace /tmp/x" line,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.