patch, 2 partsProtocol v2 in_vain fixes

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

> 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(-)

> @@ -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;

>  				/* 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 &&

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

>> 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.
>
> That sounds reasonable to me.
>
>> > +	cp trace /tmp/x &&
>> 
>> Leftover debugging line?
>
> Ah, yes. If Junio can't or won't do it locally then I'll send out
> another set with this changed.

> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -404,6 +404,36 @@ test_expect_success 'in_vain not triggered before first ACK' '
>  	test_i18ngrep "Total 3 " trace
>  '
>  
> +test_expect_success 'in_vain resetted upon ACK' '
> +	rm -rf myserver myclient trace &&
> +	git init myserver &&
> +
> +	# Linked list of commits on master. The first is common; the rest are
> +	# not.
> +	test_commit -C myserver first_master_commit &&
> +	git clone "file://$(pwd)/myserver" myclient &&
> +	test_commit_bulk -C myclient 255 &&
> +
> +	# Another linked list of commits on anotherbranch with no connection to
> +	# master. The first is common; the rest are not.
> +	git -C myserver checkout --orphan anotherbranch &&
> +	test_commit -C myserver first_anotherbranch_commit &&
> +	git -C myclient fetch origin anotherbranch:refs/heads/anotherbranch &&
> +	git -C myclient checkout anotherbranch &&
> +	test_commit_bulk -C myclient 255 &&
> +
> +	# The new commit that the client wants to fetch.
> +	git -C myserver checkout master &&
> +	test_commit -C myserver to_fetch &&
> +
> +	# The client will send (as "have"s) all 256 commits in anotherbranch
> +	# first. The 256th commit is common between the client and the server,
> +	# and should reset in_vain. This allows negotiation to continue until
> +	# the client reports that first_anotherbranch_commit is common.
> +	GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin master &&
> +	test_i18ngrep "Total 3 " trace

> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> Thanks.
> 
> As with the other patch, trace2 output from the server might be more
> stable.
> 
> Do these tests pass with protocol v0 as well?  (Forgive my laziness in
> not checking yet.)

> process_acks() returns 0, 1, or 2, depending on whether "ready" was
> received and if not, whether at least one commit was found to be common.
> Replace these magic numbers with a documented enum.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)

> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1268,9 +1268,29 @@ static int process_section_header(struct packet_reader *reader,
>  	return ret;
>  }
>  
> -static int process_acks(struct fetch_negotiator *negotiator,
> -			struct packet_reader *reader,
> -			struct oidset *common)
> +enum common_found {
> +	/*
> +	 * No commit was found to be possessed by both the client and the
> +	 * server, and "ready" was not received.
> +	 */
> +	NO_COMMON_FOUND,
> +
> +	/*
> +	 * At least one commit was found to be possessed by both the client and
> +	 * the server, and "ready" was not received.
> +	 */
> +	COMMON_FOUND,
> +
> +	/*
> +	 * "ready" was received, indicating that the server is ready to send
> +	 * the packfile without any further negotiation.
> +	 */
> +	READY
> +};
> +
> +static enum common_found process_acks(struct fetch_negotiator *negotiator,
> +				      struct packet_reader *reader,
> +				      struct oidset *common)

>  {
>  	/* received */
>  	int received_ready = 0;
> @@ -1320,7 +1340,8 @@ static int process_acks(struct fetch_negotiator *negotiator,
>  		die(_("expected no other sections to be sent after no 'ready'"));
>  
>  	/* return 0 if no common, 1 if there are common, or 2 if ready */

> -	return received_ready ? 2 : (received_ack ? 1 : 0);
> +	return received_ready ? READY :
> +		(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);
>  }
>  
>  static void receive_shallow_info(struct fetch_pack_args *args,
> @@ -1508,13 +1529,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  		case FETCH_PROCESS_ACKS:
>  			/* Process ACKs/NAKs */
>  			switch (process_acks(negotiator, &reader, &common)) {
> -			case 2:
> +			case READY:
>  				state = FETCH_GET_PACK;
>  				break;
> -			case 1:
> +			case COMMON_FOUND:
>  				in_vain = 0;
>  				/* fallthrough */
> -			default:
> +			case NO_COMMON_FOUND:
>  				state = FETCH_SEND_REQUEST;
>  				break;
>  			}

>>  	/* return 0 if no common, 1 if there are common, or 2 if ready */
>
> This comment can be removed now, which is a nice bonus.
>
>> -	return received_ready ? 2 : (received_ack ? 1 : 0);
>> +	return received_ready ? READY :
>> +		(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);
>>  }
>>  
>>  static void receive_shallow_info(struct fetch_pack_args *args,
>> @@ -1508,13 +1529,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>>  		case FETCH_PROCESS_ACKS:
>>  			/* Process ACKs/NAKs */
>>  			switch (process_acks(negotiator, &reader, &common)) {
>> -			case 2:
>> +			case READY:
>>  				state = FETCH_GET_PACK;
>>  				break;
>> -			case 1:
>> +			case COMMON_FOUND:
>>  				in_vain = 0;
>>  				/* fallthrough */
>> -			default:
>> +			case NO_COMMON_FOUND:
>>  				state = FETCH_SEND_REQUEST;
>>  				break;
>>  			}
>
> With the extraneous comment removed,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> >>  	/* return 0 if no common, 1 if there are common, or 2 if ready */  
> >
> > This comment can be removed now, which is a nice bonus.

> On Tue, 28 Apr 2020 09:54:19 -0700
> Junio C Hamano <gitster@pobox.com> wrote:
>
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>> 
>> >>  	/* return 0 if no common, 1 if there are common, or 2 if ready */  
>> >
>> > This comment can be removed now, which is a nice bonus.
> It is still present in the pu branch.
>
> Is there other branch where I should look?

> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c          | 13 +++++++++----
>  t/t5500-fetch-pack.sh | 18 ++++++++++++++++++
>  2 files changed, 27 insertions(+), 4 deletions(-)

> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index baa1a99f45..961cd6beec 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -385,6 +385,24 @@ 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 &&
> +	test_i18ngrep "Total 3 " trace

> +'
> +
>  test_expect_success 'fetch in shallow repo unreachable shallow objects' '
>  	(
>  		git clone --bare --branch B --single-branch "file://$(pwd)/." no-reflog &&
> --
> 2.26.2.303.gf8c07b1a785-goog
>
>
>

> In two tests introduced by 4fa3f00abb ("fetch-pack: in protocol v2,
> in_vain only after ACK", 2020-04-28) and 2f0a093dd6 ("fetch-pack: in
> protocol v2, reset in_vain upon ACK", 2020-04-28), the count of objects
> downloaded is checked by grepping for a specific message in the packet
> trace. However, this is flaky as that specific message may be delivered
> over 2 or more packet lines.
>
> Instead, grep over stderr, just like the "fetch creating new shallow
> root" test in the same file.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> Thanks, Dscho. The commits introducing the flakiness have made it to
> master, so this commit is on master.

>  t/t5500-fetch-pack.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 52dd1a688c..8c54e34ef1 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -386,7 +386,7 @@ test_expect_success 'clone shallow with packed refs' '
>  '
>  
>  test_expect_success 'in_vain not triggered before first ACK' '
> -	rm -rf myserver myclient trace &&
> +	rm -rf myserver myclient &&
>  	git init myserver &&
>  	test_commit -C myserver foo &&
>  	git clone "file://$(pwd)/myserver" myclient &&
> @@ -399,12 +399,12 @@ test_expect_success 'in_vain not triggered before first ACK' '
>  	# 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 &&
> -	test_i18ngrep "Total 3 " trace
> +	git -C myclient fetch --progress origin 2>log &&
> +	test_i18ngrep "remote: Total 3 " log
>  '
>  
>  test_expect_success 'in_vain resetted upon ACK' '
> -	rm -rf myserver myclient trace &&
> +	rm -rf myserver myclient &&
>  	git init myserver &&
>  
>  	# Linked list of commits on master. The first is common; the rest are
> @@ -429,8 +429,8 @@ test_expect_success 'in_vain resetted upon ACK' '
>  	# first. The 256th commit is common between the client and the server,
>  	# and should reset in_vain. This allows negotiation to continue until
>  	# the client reports that first_anotherbranch_commit is common.
> -	GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin master &&
> -	test_i18ngrep "Total 3 " trace
> +	git -C myclient fetch --progress origin master 2>log &&
> +	test_i18ngrep "Total 3 " log
>  '
>  
>  test_expect_success 'fetch in shallow repo unreachable shallow objects' '

> In two tests introduced by 4fa3f00abb ("fetch-pack: in protocol v2,
> in_vain only after ACK", 2020-04-28) and 2f0a093dd6 ("fetch-pack: in
> protocol v2, reset in_vain upon ACK", 2020-04-28), the count of objects
> downloaded is checked by grepping for a specific message in the packet
> trace. However, this is flaky as that specific message may be delivered
> over 2 or more packet lines.
>
> Instead, grep over stderr, just like the "fetch creating new shallow
> root" test in the same file.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> Thanks, Dscho. The commits introducing the flakiness have made it to
> master, so this commit is on master.

> ---
>  t/t5500-fetch-pack.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 52dd1a688c..8c54e34ef1 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -386,7 +386,7 @@ test_expect_success 'clone shallow with packed refs' '
>  '
>
>  test_expect_success 'in_vain not triggered before first ACK' '
> -	rm -rf myserver myclient trace &&
> +	rm -rf myserver myclient &&
>  	git init myserver &&
>  	test_commit -C myserver foo &&
>  	git clone "file://$(pwd)/myserver" myclient &&
> @@ -399,12 +399,12 @@ test_expect_success 'in_vain not triggered before first ACK' '
>  	# 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 &&
> -	test_i18ngrep "Total 3 " trace
> +	git -C myclient fetch --progress origin 2>log &&
> +	test_i18ngrep "remote: Total 3 " log
>  '
>
>  test_expect_success 'in_vain resetted upon ACK' '
> -	rm -rf myserver myclient trace &&
> +	rm -rf myserver myclient &&
>  	git init myserver &&
>
>  	# Linked list of commits on master. The first is common; the rest are
> @@ -429,8 +429,8 @@ test_expect_success 'in_vain resetted upon ACK' '
>  	# first. The 256th commit is common between the client and the server,
>  	# and should reset in_vain. This allows negotiation to continue until
>  	# the client reports that first_anotherbranch_commit is common.
> -	GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin master &&
> -	test_i18ngrep "Total 3 " trace
> +	git -C myclient fetch --progress origin master 2>log &&
> +	test_i18ngrep "Total 3 " log
>  '
>
>  test_expect_success 'fetch in shallow repo unreachable shallow objects' '
> --
> 2.26.2.526.g744177e7f7-goog
>
>

> In two tests introduced by 4fa3f00abb ("fetch-pack: in protocol v2,
> in_vain only after ACK", 2020-04-28) and 2f0a093dd6 ("fetch-pack: in
> protocol v2, reset in_vain upon ACK", 2020-04-28), the count of objects
> downloaded is checked by grepping for a specific message in the packet
> trace. However, this is flaky as that specific message may be delivered
> over 2 or more packet lines.
>
> Instead, grep over stderr, just like the "fetch creating new shallow
> root" test in the same file.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> Thanks, Dscho. The commits introducing the flakiness have made it to
> master, so this commit is on master.

> In the function process_acks() in fetch-pack.c, the variable
> received_ack is meant to track that an ACK was received, but it was
> never set. This results in negotiation terminating prematurely through
> the in_vain counter, when the counter should have been reset upon every
> ACK.
>
> Therefore, reset the in_vain counter upon every ACK.
>
> Helped-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c          |  1 +
>  t/t5500-fetch-pack.sh | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)