[PATCH 0/2] Protocol v2 in_vain fixes

[PATCH 0/2] Protocol v2 in_vain fixes

[Jonathan Tan]

Patch 1 is a fix for what I described in [1].

The fix in patch 1 exposes a bug, which patch 2 fixes. Jonathan Nieder
found the bug [2], and I have written a test covering this case. I've
credited him as "Helped-by".

[1] https://lore.kernel.org/git/20200423213735.242662-1-jonathantanmy@google.com/
[2] https://lore.kernel.org/git/20200422165358.GB140314@google.com/

Jonathan Tan (2):
  fetch-pack: in protocol v2, in_vain only after ACK
  fetch-pack: in protocol v2, reset in_vain upon ACK

 fetch-pack.c          | 14 +++++++++----
 t/t5500-fetch-pack.sh | 49 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 4 deletions(-)

-- 
2.26.2.303.gf8c07b1a785-goog

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

[Jonathan Tan]

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

diff --git a/fetch-pack.c b/fetch-pack.c
index 0b07b3ee73..7d15c7af81 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1143,6 +1143,7 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)
 }
 
 static int add_haves(struct fetch_negotiator *negotiator,
+		     int seen_ack,
 		     struct strbuf *req_buf,
 		     int *haves_to_send, int *in_vain)
 {
@@ -1157,7 +1158,7 @@ static int add_haves(struct fetch_negotiator *negotiator,
 	}
 
 	*in_vain += haves_added;
-	if (!haves_added || *in_vain >= MAX_IN_VAIN) {
+	if (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {
 		/* Send Done */
 		packet_buf_write(req_buf, "done\n");
 		ret = 1;
@@ -1173,7 +1174,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 			      struct fetch_pack_args *args,
 			      const struct ref *wants, struct oidset *common,
 			      int *haves_to_send, int *in_vain,
-			      int sideband_all)
+			      int sideband_all, int seen_ack)
 {
 	int ret = 0;
 	struct strbuf req_buf = STRBUF_INIT;
@@ -1230,7 +1231,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 		add_common(&req_buf, common);
 
 		/* Add initial haves */
-		ret = add_haves(negotiator, &req_buf, haves_to_send, in_vain);
+		ret = add_haves(negotiator, seen_ack, &req_buf,
+				haves_to_send, in_vain);
 	}
 
 	/* Send request */
@@ -1444,6 +1446,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	int haves_to_send = INITIAL_FLUSH;
 	struct fetch_negotiator negotiator_alloc;
 	struct fetch_negotiator *negotiator;
+	int seen_ack = 0;
 
 	if (args->no_dependents) {
 		negotiator = NULL;
@@ -1500,7 +1503,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 			if (send_fetch_request(negotiator, fd[1], args, ref,
 					       &common,
 					       &haves_to_send, &in_vain,
-					       reader.use_sideband))
+					       reader.use_sideband,
+					       seen_ack))
 				state = FETCH_GET_PACK;
 			else
 				state = FETCH_PROCESS_ACKS;
@@ -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
+'
+
 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

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

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

Makes sense.

I think we can instead use in_vain==NULL as a signal that we haven't
seen any ack yet and that shrinks the patch somewhat ([Patch 1/2]
becomes +6/-4 from +9/-4 for fetch-pack.c).  I do not know if the
result is easier to follow (as there is one less variable to keep in
mind), though.  The callee is probably easier to reason about (it
needs to worry about the *in_vain variable only when it is given---
you cannot beat the simplicity of it), but the caller side may
become harder to reason about, especially without any comment where
in_vain_p becomes non-NULL.  Your version has an assignment to make
"seen_ack" to true there, which makes it very clear without any
comment what is going on.

So I dunno.  I've queued your version and discarded the following.

 fetch-pack.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 1734a573b0..7b837b6a76 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1156,8 +1156,9 @@ static int add_haves(struct fetch_negotiator *negotiator,
 			break;
 	}
 
-	*in_vain += haves_added;
-	if (!haves_added || *in_vain >= MAX_IN_VAIN) {
+	if (in_vain)
+		*in_vain += haves_added;
+	if (!haves_added || (in_vain && *in_vain >= MAX_IN_VAIN)) {
 		/* Send Done */
 		packet_buf_write(req_buf, "done\n");
 		ret = 1;
@@ -1444,6 +1445,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	int haves_to_send = INITIAL_FLUSH;
 	struct fetch_negotiator negotiator_alloc;
 	struct fetch_negotiator *negotiator;
+	int *in_vain_p = NULL;
 
 	if (args->no_dependents) {
 		negotiator = NULL;
@@ -1499,7 +1501,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 			}
 			if (send_fetch_request(negotiator, fd[1], args, ref,
 					       &common,
-					       &haves_to_send, &in_vain,
+					       &haves_to_send, in_vain_p,
 					       reader.use_sideband))
 				state = FETCH_GET_PACK;
 			else
@@ -1512,7 +1514,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				state = FETCH_GET_PACK;
 				break;
 			case 1:
-				in_vain = 0;
+				in_vain_p = &in_vain;
 				/* fallthrough */
 			default:
 				state = FETCH_SEND_REQUEST;

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

[Jonathan Nieder]

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.

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

[Jonathan Tan]

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

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

Probably better to have tracing in the client and use that, but this
requires us to add that tracing. But in general I agree.

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

Thanks for your review.

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

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

Well, if I am expecting the "named enumerators" patch anyway, I'd
wait the fix to be done at the source ;-)

[PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK

[Jonathan Tan]

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

diff --git a/fetch-pack.c b/fetch-pack.c
index 7d15c7af81..8551c07ed9 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1287,6 +1287,7 @@ static int process_acks(struct fetch_negotiator *negotiator,
 
 		if (skip_prefix(reader->line, "ACK ", &arg)) {
 			struct object_id oid;
+			received_ack = 1;
 			if (!get_oid_hex(arg, &oid)) {
 				struct commit *commit;
 				oidset_insert(common, &oid);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index fcc5cc8ab0..98e1442cbf 100755
--- 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
+'
+
 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

Re: [PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK

[Jonathan Nieder]

Jonathan Tan wrote:

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

Re: [PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK

[Jonathan Tan]

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

Thanks for your review. Yes they pass with protocol v0 too (checked by
passing "-c protocol.version=0" when fetching).

[PATCH v2 0/3] Protocol v2 in_vain fixes

[Jonathan Tan]

I'm not sure if Jonathan Nieder meant the enum-from-process_acks()
change should be in this patch set, but I've included it here anyway.
Also I've removed an extraneous debug "cp" from patch 2/3 (previously
patch 1/2).

Jonathan Tan (3):
  fetch-pack: return enum from process_acks()
  fetch-pack: in protocol v2, in_vain only after ACK
  fetch-pack: in protocol v2, reset in_vain upon ACK

 fetch-pack.c          | 49 +++++++++++++++++++++++++++++++++----------
 t/t5500-fetch-pack.sh | 48 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 11 deletions(-)

-- 
2.26.2.303.gf8c07b1a785-goog

[PATCH v2 1/3] fetch-pack: return enum from process_acks()

[Jonathan Tan]

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

diff --git a/fetch-pack.c b/fetch-pack.c
index 0b07b3ee73..45547a621e 100644
--- 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;
 			}
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH v2 1/3] fetch-pack: return enum from process_acks()

[Jonathan Nieder]

Jonathan Tan wrote:

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

Yay!

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

Thanks for the clear comments.

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

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>

Thanks.

diff --git i/fetch-pack.c w/fetch-pack.c
index 45547a621e6..7b0eef98db5 100644
--- i/fetch-pack.c
+++ w/fetch-pack.c
@@ -1339,7 +1339,6 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator,
 	if (!received_ready && reader->status != PACKET_READ_FLUSH)
 		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 ? READY :
 		(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);
 }

Re: [PATCH v2 1/3] fetch-pack: return enum from process_acks()

[Junio C Hamano]

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

Yes, the result reads very clearly.

Locally amended.

Thanks.

Re: [PATCH v2 1/3] fetch-pack: return enum from process_acks()

[Michal Suchánek]

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?

Thanks

Michal

Re: [PATCH v2 1/3] fetch-pack: return enum from process_acks()

[Junio C Hamano]

Michal Suchánek <msuchanek@suse.de> writes:

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

Some branch that is pushed out _after_ I push out, perhaps?

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

[Jonathan Tan]

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 | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 45547a621e..76691dc6c0 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1143,6 +1143,7 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)
 }
 
 static int add_haves(struct fetch_negotiator *negotiator,
+		     int seen_ack,
 		     struct strbuf *req_buf,
 		     int *haves_to_send, int *in_vain)
 {
@@ -1157,7 +1158,7 @@ static int add_haves(struct fetch_negotiator *negotiator,
 	}
 
 	*in_vain += haves_added;
-	if (!haves_added || *in_vain >= MAX_IN_VAIN) {
+	if (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {
 		/* Send Done */
 		packet_buf_write(req_buf, "done\n");
 		ret = 1;
@@ -1173,7 +1174,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 			      struct fetch_pack_args *args,
 			      const struct ref *wants, struct oidset *common,
 			      int *haves_to_send, int *in_vain,
-			      int sideband_all)
+			      int sideband_all, int seen_ack)
 {
 	int ret = 0;
 	struct strbuf req_buf = STRBUF_INIT;
@@ -1230,7 +1231,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 		add_common(&req_buf, common);
 
 		/* Add initial haves */
-		ret = add_haves(negotiator, &req_buf, haves_to_send, in_vain);
+		ret = add_haves(negotiator, seen_ack, &req_buf,
+				haves_to_send, in_vain);
 	}
 
 	/* Send request */
@@ -1465,6 +1467,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	int haves_to_send = INITIAL_FLUSH;
 	struct fetch_negotiator negotiator_alloc;
 	struct fetch_negotiator *negotiator;
+	int seen_ack = 0;
 
 	if (args->no_dependents) {
 		negotiator = NULL;
@@ -1521,7 +1524,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 			if (send_fetch_request(negotiator, fd[1], args, ref,
 					       &common,
 					       &haves_to_send, &in_vain,
-					       reader.use_sideband))
+					       reader.use_sideband,
+					       seen_ack))
 				state = FETCH_GET_PACK;
 			else
 				state = FETCH_PROCESS_ACKS;
@@ -1534,6 +1538,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				break;
 			case COMMON_FOUND:
 				in_vain = 0;
+				seen_ack = 1;
 				/* fallthrough */
 			case NO_COMMON_FOUND:
 				state = FETCH_SEND_REQUEST;
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

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

[Jonathan Nieder]

Jonathan Tan wrote:

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

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

[Johannes Schindelin]

Hi Jonathan,

On Mon, 27 Apr 2020, Jonathan Tan wrote:

> 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

This just failed in one of the Pipelines I monitor:
https://github.com/git-for-windows/git-sdk-64/runs/648253955?check_suite_focus=true

The short of it is:

-- snip --
[...]
packet:     sideband< \2Enumerating objects: 4, done.
packet:     sideband< \2Counting objects: 25% (1/4)\15Counting objects:  50% (2/4)\15Counting objects:  75% (3/4)\15Counting objects: 100% (4/4)\15Counting obj
packet:     sideband< \2ects: 100% (4/4), done.Compressing objects:  50% (1/2)\15Compressing objects: 100% (2/2)\15Compressing objects: 100% (2/2), done.T
packet:     sideband< \2otal 3 (delta 0), reused 0 (delta 0), pack-reused 0
packet:     sideband< PACK ...
packet:  upload-pack> 0000
packet:     sideband< 0000
++ return 1
error: last command exited with $?=1
t/t5500-fetch-pack.sh:388: error: not ok 43 - 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
-- snap --

In other words, that `test_i18ngrep` assumes incorrectly that "Total 3"
will be found in the trace, but the sideband is totally allowed to cut
through it and deliver it in different packets.

This makes t5500 flaky.

Ciao,
Johannes

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

[PATCH] t5500: count objects through stderr, not trace

[Jonathan Tan]

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

Re: [PATCH] t5500: count objects through stderr, not trace

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

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

Hmph.  Do we coalesce the payload we receive in adjacent two packet
lines into one before writing it out, or do we know in this codepath
that the progress messages are the only thing we are writing out to
the standard error stream and we do not need to worry about other
stuff getting mixed in?  If that is the case, perhaps "just like we
already do in another test" is a lot weaker justification (it sounds
as if we are saying "another test is doing this thing, so if it is
broken, we are not making things worse") than what you actually
have ("the only thing we are writing out to the standard error
stream in the non-error case is these progress messages, and the
'Total 3 ' string, even if it is carried in two separate fragments,
will be shown without getting mixed with anything else on the
standard error stream").

Assuming that the way I (re)read your justification is correct, the
patch looks good to me.

Thanks, both.

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

Re: [PATCH] t5500: count objects through stderr, not trace

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Thanks, Dscho. The commits introducing the flakiness have made it to
>> master, so this commit is on master.

I'll queue directly on top of jt/v2-fetch-neg-fix as that topic was
designed to be later mergeable down to 'maint' so that the 2.26
track can (1) revert the default flip so protocol v0 is restored as
the default, and (2) breakage of negotiataion in protocol v2 is
fixed.

Thanks.

Re: [PATCH] t5500: count objects through stderr, not trace

[Johannes Schindelin]

Hi Jonathan,

On Wed, 6 May 2020, Jonathan Tan wrote:

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

Thank you for fixing this so quickly. I agree that the patch addresses the
underlying problem.

Thanks!
Dscho

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

Re: [PATCH] t5500: count objects through stderr, not trace

[Johannes Schindelin]

Hi Jonathan,

On Wed, 6 May 2020, Jonathan Tan wrote:

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

Sorry for the blast from the past. Apparently, even with this fix, the
first test is still flaky.

I submitted a patch to work around the bug via
https://github.com/gitgitgadget/git/pull/753; Please review when you have
some time.

Thanks,
Dscho

[PATCH v2 3/3] fetch-pack: in protocol v2, reset in_vain upon ACK

[Jonathan Tan]

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

diff --git a/fetch-pack.c b/fetch-pack.c
index 76691dc6c0..edbfc2af1c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1307,6 +1307,7 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator,
 
 		if (skip_prefix(reader->line, "ACK ", &arg)) {
 			struct object_id oid;
+			received_ack = 1;
 			if (!get_oid_hex(arg, &oid)) {
 				struct commit *commit;
 				oidset_insert(common, &oid);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 961cd6beec..52dd1a688c 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -403,6 +403,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
+'
+
 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

Re: [PATCH v2 3/3] fetch-pack: in protocol v2, reset in_vain upon ACK

[Jonathan Nieder]

Jonathan Tan wrote:

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

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.