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

[Junio C Hamano <gitster@pobox.com>]

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;