git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/2] Protocol v2 in_vain fixes
@ 2020-04-25  0:56 Jonathan Tan
  2020-04-25  0:56 ` [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK Jonathan Tan
                   ` (2 more replies)
  0 siblings, 3 replies; 25+ messages in thread
From: Jonathan Tan @ 2020-04-25  0:56 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, peff, jrnieder, gitster

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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK
  2020-04-25  0:56 [PATCH 0/2] Protocol v2 in_vain fixes Jonathan Tan
@ 2020-04-25  0:56 ` Jonathan Tan
  2020-04-25  5:08   ` Junio C Hamano
  2020-04-26  0:28   ` Jonathan Nieder
  2020-04-25  0:56 ` [PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK Jonathan Tan
  2020-04-28  0:01 ` [PATCH v2 0/3] Protocol v2 in_vain fixes Jonathan Tan
  2 siblings, 2 replies; 25+ messages in thread
From: Jonathan Tan @ 2020-04-25  0:56 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, peff, jrnieder, gitster

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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK
  2020-04-25  0:56 [PATCH 0/2] Protocol v2 in_vain fixes Jonathan Tan
  2020-04-25  0:56 ` [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK Jonathan Tan
@ 2020-04-25  0:56 ` Jonathan Tan
  2020-04-26  1:10   ` Jonathan Nieder
  2020-04-28  0:01 ` [PATCH v2 0/3] Protocol v2 in_vain fixes Jonathan Tan
  2 siblings, 1 reply; 25+ messages in thread
From: Jonathan Tan @ 2020-04-25  0:56 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, peff, jrnieder, gitster

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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK
  2020-04-25  0:56 ` [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK Jonathan Tan
@ 2020-04-25  5:08   ` Junio C Hamano
  2020-04-26  0:28   ` Jonathan Nieder
  1 sibling, 0 replies; 25+ messages in thread
From: Junio C Hamano @ 2020-04-25  5:08 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, peff, jrnieder

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;

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK
  2020-04-25  0:56 ` [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK Jonathan Tan
  2020-04-25  5:08   ` Junio C Hamano
@ 2020-04-26  0:28   ` Jonathan Nieder
  2020-04-27 17:27     ` Jonathan Tan
  1 sibling, 1 reply; 25+ messages in thread
From: Jonathan Nieder @ 2020-04-26  0:28 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, peff, gitster

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.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK
  2020-04-25  0:56 ` [PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK Jonathan Tan
@ 2020-04-26  1:10   ` Jonathan Nieder
  2020-04-27 17:28     ` Jonathan Tan
  0 siblings, 1 reply; 25+ messages in thread
From: Jonathan Nieder @ 2020-04-26  1:10 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, peff, gitster

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK
  2020-04-26  0:28   ` Jonathan Nieder
@ 2020-04-27 17:27     ` Jonathan Tan
  2020-04-27 22:16       ` Junio C Hamano
  0 siblings, 1 reply; 25+ messages in thread
From: Jonathan Tan @ 2020-04-27 17:27 UTC (permalink / raw)
  To: jrnieder; +Cc: jonathantanmy, git, peff, gitster

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK
  2020-04-26  1:10   ` Jonathan Nieder
@ 2020-04-27 17:28     ` Jonathan Tan
  0 siblings, 0 replies; 25+ messages in thread
From: Jonathan Tan @ 2020-04-27 17:28 UTC (permalink / raw)
  To: jrnieder; +Cc: jonathantanmy, git, peff, gitster

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK
  2020-04-27 17:27     ` Jonathan Tan
@ 2020-04-27 22:16       ` Junio C Hamano
  0 siblings, 0 replies; 25+ messages in thread
From: Junio C Hamano @ 2020-04-27 22:16 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: jrnieder, git, peff

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v2 0/3] Protocol v2 in_vain fixes
  2020-04-25  0:56 [PATCH 0/2] Protocol v2 in_vain fixes Jonathan Tan
  2020-04-25  0:56 ` [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK Jonathan Tan
  2020-04-25  0:56 ` [PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK Jonathan Tan
@ 2020-04-28  0:01 ` Jonathan Tan
  2020-04-28  0:01   ` [PATCH v2 1/3] fetch-pack: return enum from process_acks() Jonathan Tan
                     ` (2 more replies)
  2 siblings, 3 replies; 25+ messages in thread
From: Jonathan Tan @ 2020-04-28  0:01 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, gitster, jrnieder

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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v2 1/3] fetch-pack: return enum from process_acks()
  2020-04-28  0:01 ` [PATCH v2 0/3] Protocol v2 in_vain fixes Jonathan Tan
@ 2020-04-28  0:01   ` Jonathan Tan
  2020-04-28  0:53     ` Jonathan Nieder
  2020-04-28  0:01   ` [PATCH v2 2/3] fetch-pack: in protocol v2, in_vain only after ACK Jonathan Tan
  2020-04-28  0:01   ` [PATCH v2 3/3] fetch-pack: in protocol v2, reset in_vain upon ACK Jonathan Tan
  2 siblings, 1 reply; 25+ messages in thread
From: Jonathan Tan @ 2020-04-28  0:01 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, gitster, jrnieder

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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v2 2/3] fetch-pack: in protocol v2, in_vain only after ACK
  2020-04-28  0:01 ` [PATCH v2 0/3] Protocol v2 in_vain fixes Jonathan Tan
  2020-04-28  0:01   ` [PATCH v2 1/3] fetch-pack: return enum from process_acks() Jonathan Tan
@ 2020-04-28  0:01   ` Jonathan Tan
  2020-04-28  0:54     ` Jonathan Nieder
  2020-05-06 21:08     ` Johannes Schindelin
  2020-04-28  0:01   ` [PATCH v2 3/3] fetch-pack: in protocol v2, reset in_vain upon ACK Jonathan Tan
  2 siblings, 2 replies; 25+ messages in thread
From: Jonathan Tan @ 2020-04-28  0:01 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, gitster, jrnieder

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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v2 3/3] fetch-pack: in protocol v2, reset in_vain upon ACK
  2020-04-28  0:01 ` [PATCH v2 0/3] Protocol v2 in_vain fixes Jonathan Tan
  2020-04-28  0:01   ` [PATCH v2 1/3] fetch-pack: return enum from process_acks() Jonathan Tan
  2020-04-28  0:01   ` [PATCH v2 2/3] fetch-pack: in protocol v2, in_vain only after ACK Jonathan Tan
@ 2020-04-28  0:01   ` Jonathan Tan
  2020-04-28  0:55     ` Jonathan Nieder
  2 siblings, 1 reply; 25+ messages in thread
From: Jonathan Tan @ 2020-04-28  0:01 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, gitster, jrnieder

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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/3] fetch-pack: return enum from process_acks()
  2020-04-28  0:01   ` [PATCH v2 1/3] fetch-pack: return enum from process_acks() Jonathan Tan
@ 2020-04-28  0:53     ` Jonathan Nieder
  2020-04-28 16:54       ` Junio C Hamano
  0 siblings, 1 reply; 25+ messages in thread
From: Jonathan Nieder @ 2020-04-28  0:53 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, gitster

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/3] fetch-pack: in protocol v2, in_vain only after ACK
  2020-04-28  0:01   ` [PATCH v2 2/3] fetch-pack: in protocol v2, in_vain only after ACK Jonathan Tan
@ 2020-04-28  0:54     ` Jonathan Nieder
  2020-05-06 21:08     ` Johannes Schindelin
  1 sibling, 0 replies; 25+ messages in thread
From: Jonathan Nieder @ 2020-04-28  0:54 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, gitster

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>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 3/3] fetch-pack: in protocol v2, reset in_vain upon ACK
  2020-04-28  0:01   ` [PATCH v2 3/3] fetch-pack: in protocol v2, reset in_vain upon ACK Jonathan Tan
@ 2020-04-28  0:55     ` Jonathan Nieder
  0 siblings, 0 replies; 25+ messages in thread
From: Jonathan Nieder @ 2020-04-28  0:55 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, gitster

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.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/3] fetch-pack: return enum from process_acks()
  2020-04-28  0:53     ` Jonathan Nieder
@ 2020-04-28 16:54       ` Junio C Hamano
  2020-04-28 18:00         ` Michal Suchánek
  0 siblings, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2020-04-28 16:54 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Jonathan Tan, git

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.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/3] fetch-pack: return enum from process_acks()
  2020-04-28 16:54       ` Junio C Hamano
@ 2020-04-28 18:00         ` Michal Suchánek
  2020-04-28 19:17           ` Junio C Hamano
  0 siblings, 1 reply; 25+ messages in thread
From: Michal Suchánek @ 2020-04-28 18:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jonathan Nieder, Jonathan Tan, git

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/3] fetch-pack: return enum from process_acks()
  2020-04-28 18:00         ` Michal Suchánek
@ 2020-04-28 19:17           ` Junio C Hamano
  0 siblings, 0 replies; 25+ messages in thread
From: Junio C Hamano @ 2020-04-28 19:17 UTC (permalink / raw)
  To: Michal Suchánek; +Cc: Jonathan Nieder, Jonathan Tan, git

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?

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/3] fetch-pack: in protocol v2, in_vain only after ACK
  2020-04-28  0:01   ` [PATCH v2 2/3] fetch-pack: in protocol v2, in_vain only after ACK Jonathan Tan
  2020-04-28  0:54     ` Jonathan Nieder
@ 2020-05-06 21:08     ` Johannes Schindelin
  2020-05-06 22:07       ` [PATCH] t5500: count objects through stderr, not trace Jonathan Tan
  1 sibling, 1 reply; 25+ messages in thread
From: Johannes Schindelin @ 2020-05-06 21:08 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, gitster, jrnieder

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH] t5500: count objects through stderr, not trace
  2020-05-06 21:08     ` Johannes Schindelin
@ 2020-05-06 22:07       ` Jonathan Tan
  2020-05-06 22:28         ` Junio C Hamano
                           ` (2 more replies)
  0 siblings, 3 replies; 25+ messages in thread
From: Jonathan Tan @ 2020-05-06 22:07 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, Johannes.Schindelin

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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] t5500: count objects through stderr, not trace
  2020-05-06 22:07       ` [PATCH] t5500: count objects through stderr, not trace Jonathan Tan
@ 2020-05-06 22:28         ` Junio C Hamano
  2020-05-06 22:40           ` Junio C Hamano
  2020-05-07 14:35         ` Johannes Schindelin
  2020-10-13 14:45         ` Johannes Schindelin
  2 siblings, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2020-05-06 22:28 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, Johannes.Schindelin

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] t5500: count objects through stderr, not trace
  2020-05-06 22:28         ` Junio C Hamano
@ 2020-05-06 22:40           ` Junio C Hamano
  0 siblings, 0 replies; 25+ messages in thread
From: Junio C Hamano @ 2020-05-06 22:40 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, Johannes.Schindelin

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.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] t5500: count objects through stderr, not trace
  2020-05-06 22:07       ` [PATCH] t5500: count objects through stderr, not trace Jonathan Tan
  2020-05-06 22:28         ` Junio C Hamano
@ 2020-05-07 14:35         ` Johannes Schindelin
  2020-10-13 14:45         ` Johannes Schindelin
  2 siblings, 0 replies; 25+ messages in thread
From: Johannes Schindelin @ 2020-05-07 14:35 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] t5500: count objects through stderr, not trace
  2020-05-06 22:07       ` [PATCH] t5500: count objects through stderr, not trace Jonathan Tan
  2020-05-06 22:28         ` Junio C Hamano
  2020-05-07 14:35         ` Johannes Schindelin
@ 2020-10-13 14:45         ` Johannes Schindelin
  2 siblings, 0 replies; 25+ messages in thread
From: Johannes Schindelin @ 2020-10-13 14:45 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2020-10-13 14:45 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-25  0:56 [PATCH 0/2] Protocol v2 in_vain fixes Jonathan Tan
2020-04-25  0:56 ` [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK Jonathan Tan
2020-04-25  5:08   ` Junio C Hamano
2020-04-26  0:28   ` Jonathan Nieder
2020-04-27 17:27     ` Jonathan Tan
2020-04-27 22:16       ` Junio C Hamano
2020-04-25  0:56 ` [PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK Jonathan Tan
2020-04-26  1:10   ` Jonathan Nieder
2020-04-27 17:28     ` Jonathan Tan
2020-04-28  0:01 ` [PATCH v2 0/3] Protocol v2 in_vain fixes Jonathan Tan
2020-04-28  0:01   ` [PATCH v2 1/3] fetch-pack: return enum from process_acks() Jonathan Tan
2020-04-28  0:53     ` Jonathan Nieder
2020-04-28 16:54       ` Junio C Hamano
2020-04-28 18:00         ` Michal Suchánek
2020-04-28 19:17           ` Junio C Hamano
2020-04-28  0:01   ` [PATCH v2 2/3] fetch-pack: in protocol v2, in_vain only after ACK Jonathan Tan
2020-04-28  0:54     ` Jonathan Nieder
2020-05-06 21:08     ` Johannes Schindelin
2020-05-06 22:07       ` [PATCH] t5500: count objects through stderr, not trace Jonathan Tan
2020-05-06 22:28         ` Junio C Hamano
2020-05-06 22:40           ` Junio C Hamano
2020-05-07 14:35         ` Johannes Schindelin
2020-10-13 14:45         ` Johannes Schindelin
2020-04-28  0:01   ` [PATCH v2 3/3] fetch-pack: in protocol v2, reset in_vain upon ACK Jonathan Tan
2020-04-28  0:55     ` Jonathan Nieder

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git