On Sat, Dec 25, 2021 at 08:17:44PM -0500, ori@eigenstate.org wrote:
> 
> I was recently trying get to a problem with
> multiple acks getting sent in git9, and noticed
> you'd run into the same problem:
> 
> libexec/got-fetch-pack/got-fetch-pack.c:659
> 
> 	/*
> 	 * Git server responds with ACK after 'done'
> 	 * even though multi_ack is disabled?!?
> 	 */
> 
> I think I have a theory for this, though the
> misbehavior is still puzzling.
> 
> It seems that there's one ack sent *per duplicate
> hash*; if all "have" lines are unique, it seems
> that only one ACK is sent back.
> 
> currently experimenting on my end, let me know
> if this seems to solve the problem.
> 
> (otherwise, I'm also looking at doing mutli-ack
> support; it seems like it'll be easy enough)

Unless I misunderstood something, single-ACK mode in git-server is
broken, or their spec is wrong when it claims that an ACK will only
be sent for the first common object found unless multi_ack is enabled.

We can indeed avoid multiple ACKs of the same hash by sending unique
"have" ID lines only, as done in the patch below.

Regardless, we may still receive an "ACK" message from the server after
sending our "done" message. I found that "test_fetch_branch" triggers
this on my system with the patch below.

A Git server that runs a multithreaded search of its local refs and
returns ACK whenever some thread finds a common ancestor commit might
trigger this. I have not looked at Git's code but I would expect their
server to be doing something like this and violate their own spec.
I tried to make "test_fetch_branch" pass but it seems impossible to tell
how many ACKs we should be expecting before sending our "done" message.
During test_fetch_branch the server ACKs two out of our three "have"
lines and then goes silent waiting for our "done" message. The server
does not "NAK" anything either in that case, so there does not seem to
be a 1:1 correspondence between the messages sent and received.

Given this, I am not sure if the below patch is really worth it, as we
would have to keep the chunk which handles "ACK" after "done" in any case
to keep test_fetch_branch happy. In which case this patch provides no
benefit as it only adds more lines of code to achieve the same thing.

test_fetch_basic ok
test_fetch_list ok
test_fetch_branch got-fetch-pack: unknown side-band received from server
got: bad packet received
got fetch command failed unexpectedly
test failed; leaving test data in /tmp/got-test-fetch_branch-mYDCrMMI
test_fetch_all ok
test_fetch_empty_packfile ok
test_fetch_delete_branch ok
test_fetch_delete_branch_mirror ok
test_fetch_update_tag ok
test_fetch_reference ok
test_fetch_replace_symref ok
test_fetch_update_headref ok
test_fetch_headref_deleted_locally ok
test_fetch_gotconfig_remote_repo ok
test_fetch_delete_remote_refs ok

diff fd7fa26b794c3d1ee1cbe7baa15ecfff535c172f 0815f88c00ad38a3aa7dea3f2c416dec9c2df4a7
blob - 7df6b00656bdfc711605a13a29e30f4b21aa2f3a
blob + 15d6a0743d79cb9feafda1bbc8058159495b7619
--- libexec/got-fetch-pack/Makefile
+++ libexec/got-fetch-pack/Makefile
@@ -4,7 +4,7 @@
 
 PROG=		got-fetch-pack
 SRCS=		got-fetch-pack.c error.c inflate.c object_parse.c \
-		path.c privsep.c sha1.c pkt.c gitproto.c
+		object_idset.c path.c privsep.c sha1.c pkt.c gitproto.c
 
 CPPFLAGS = -I${.CURDIR}/../../include -I${.CURDIR}/../../lib
 LDADD = -lutil -lz
blob - b85f024e4b7e5925391dbd4ff5c8959ddb3234bc
blob + 6e4fdbc7793864161bfc5769f1eb111278644ad7
--- libexec/got-fetch-pack/got-fetch-pack.c
+++ libexec/got-fetch-pack/got-fetch-pack.c
@@ -46,6 +46,7 @@
 #include "got_lib_delta.h"
 #include "got_lib_object.h"
 #include "got_lib_object_parse.h"
+#include "got_lib_object_idset.h"
 #include "got_lib_privsep.h"
 #include "got_lib_pack.h"
 #include "got_lib_pkt.h"
@@ -310,6 +311,7 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 	char buf[GOT_PKT_MAX];
 	char hashstr[SHA1_DIGEST_STRING_LENGTH];
 	struct got_object_id *have, *want;
+	struct got_object_idset *ids_announced;
 	int is_firstpkt = 1, nref = 0, refsz = 16;
 	int i, n, nwant = 0, nhave = 0, acked = 0;
 	off_t packsz = 0, last_reported_packsz = 0;
@@ -328,6 +330,9 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 	TAILQ_INIT(&symrefs);
 	SHA1Init(&sha1_ctx);
 
+	ids_announced = got_object_idset_alloc();
+	if (ids_announced == NULL)
+		return got_error_from_errno("got_object_idset_alloc");
 	have = malloc(refsz * sizeof(have[0]));
 	if (have == NULL)
 		return got_error_from_errno("malloc");
@@ -519,6 +524,15 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 
 	TAILQ_FOREACH(pe, have_refs, entry) {
 		struct got_object_id *id = pe->data;
+		/*
+		 * Avoid duplicated "have" announcements because they
+		 * would trigger duplicate ACKs from Git servers.
+		 */
+		if (got_object_idset_contains(ids_announced, id))
+			continue;
+		err = got_object_idset_add(ids_announced, id, NULL);
+		if (err)
+			goto done;
 		got_sha1_digest_to_str(id->sha1, hashstr, sizeof(hashstr));
 		n = snprintf(buf, sizeof(buf), "have %s\n", hashstr);
 		if (n >= sizeof(buf)) {
@@ -645,25 +659,6 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 				}
 				err = fetch_error(buf, r);
 				goto done;
-			} else if (buf[0] == 'A') {
-				err = got_pkt_readn(&r, fd, buf, datalen);
-				if (err)
-					goto done;
-				if (r != datalen) {
-					err = got_error_msg(GOT_ERR_BAD_PACKET,
-					    "packet too short");
-					goto done;
-				}
-				/*
-				 * Git server responds with ACK after 'done'
-				 * even though multi_ack is disabled?!?
-				 */
-				buf[r] = '\0';
-				if (strncmp(buf, "CK ", 3) == 0)
-					continue; /* ignore */
-				err = got_error_msg(GOT_ERR_BAD_PACKET,
-				    "unexpected message from server");
-				goto done;
 			} else {
 				err = got_error_msg(GOT_ERR_BAD_PACKET,
 				    "unknown side-band received from server");
@@ -761,6 +756,7 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 		    "pack file checksum mismatch");
 	}
 done:
+	got_object_idset_free(ids_announced);
 	TAILQ_FOREACH(pe, &symrefs, entry) {
 		free((void *)pe->path);
 		free(pe->data);