commit c12ab9ad7f32b9eb712734e776de2f7b0370e643
from: Stefan Sperling <stsp@stsp.name>
via: Thomas Adam <thomas@xteddy.org>
date: Tue Feb 21 12:31:55 2023 UTC

fix "got fetch" hang against out-of-date remote repositories

Do not assume that remote repositories will always have our objects.
In Git protocol terms: Do not wait for an ACK from the server before
sending the final "done" message. Otherwise servers might be waiting
for more have-lines from us in order to find a common ancestor, which
will never be sent by us.

Problem reported by James Cook who also provided an initial test case

ok op@

commit - 47c7ee21b5f269eed3127db9d3ac7c490d1ecf0a
commit + c12ab9ad7f32b9eb712734e776de2f7b0370e643
blob - 0b5d861489f9276c73e9cf8da77eb3420f732e24
blob + 58acac3d67d7dfc870a3cb123211735a54f6d1ae
--- libexec/got-fetch-pack/got-fetch-pack.c
+++ libexec/got-fetch-pack/got-fetch-pack.c
@@ -588,6 +588,11 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 		nhave++;
 	}
 
+	n = strlcpy(buf, "done\n", sizeof(buf));
+	err = got_pkt_writepkt(fd, buf, n, chattygot);
+	if (err)
+		goto done;
+
 	while (nhave > 0 && !acked) {
 		struct got_object_id common_id;
 
@@ -600,8 +605,12 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 			goto done;
 		}
 		if (n >= 4 && strncmp(buf, "NAK\n", 4) == 0) {
-			/* Server has not located our objects yet. */
-			continue;
+			/*
+			 * Server could not find a common ancestor.
+			 * Perhaps it is an out-of-date mirror, or there
+			 * is a repository with unrelated history.
+			 */
+			break;
 		}
 		if (n < 4 + SHA1_DIGEST_STRING_LENGTH ||
 		    strncmp(buf, "ACK ", 4) != 0) {
@@ -616,11 +625,6 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 		}
 		acked++;
 	}
-
-	n = strlcpy(buf, "done\n", sizeof(buf));
-	err = got_pkt_writepkt(fd, buf, n, chattygot);
-	if (err)
-		goto done;
 
 	if (nhave == 0) {
 		err = got_pkt_readpkt(&n, fd, buf, sizeof(buf), chattygot);
blob - d24224b5bc7fdac0107e60604c32eb162cf6598d
blob + 737b0cb81e316d7acd423ca57c68f090d5f33d9f
--- regress/cmdline/fetch.sh
+++ regress/cmdline/fetch.sh
@@ -1835,7 +1835,97 @@ test_fetch_honor_wt_conf_bflag() {
 	fi
 	test_done "$testroot" "$ret"
 }
+
+test_fetch_from_out_of_date_remote() {
+	local testroot=`test_init fetch_from_out_of_date_remote`
+	local testurl=ssh://127.0.0.1/$testroot
+	local commit_id=`git_show_head $testroot/repo`
+
+	got clone -q $testurl/repo $testroot/repo-clone
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got clone command failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
 
+	echo "modified alpha" > $testroot/repo/alpha
+	git_commit $testroot/repo -m "modified alpha"
+	local commit_id2=`git_show_head $testroot/repo`
+
+	got clone -q $testurl/repo $testroot/repo-clone2 \
+		> $testroot/stdout 2> $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got clone command failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	got ref -l -r $testroot/repo-clone2 > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got ref command failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	cat > $testroot/stdout.expected <<EOF
+HEAD: refs/heads/master
+refs/heads/master: $commit_id2
+refs/remotes/origin/HEAD: refs/remotes/origin/master
+refs/remotes/origin/master: $commit_id2
+EOF
+	cmp -s $testroot/stdout $testroot/stdout.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	cat >> $testroot/repo-clone2/got.conf <<EOF
+remote "other" {
+	server "127.0.0.1"
+	protocol ssh
+	repository "$testroot/repo-clone"
+	branch { "master" }
+}
+EOF
+	got fetch -q -r $testroot/repo-clone2 other \
+		> $testroot/stdout 2> $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got fetch command failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	got ref -l -r $testroot/repo-clone2 > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got ref command failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	cat > $testroot/stdout.expected <<EOF
+HEAD: refs/heads/master
+refs/heads/master: $commit_id2
+refs/remotes/origin/HEAD: refs/remotes/origin/master
+refs/remotes/origin/master: $commit_id2
+refs/remotes/other/HEAD: refs/remotes/other/master
+refs/remotes/other/master: $commit_id
+EOF
+	cmp -s $testroot/stdout $testroot/stdout.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done "$testroot" "$ret"
+
+}
+
 test_parseargs "$@"
 run_test test_fetch_basic
 run_test test_fetch_list
@@ -1852,3 +1942,4 @@ run_test test_fetch_headref_deleted_locally
 run_test test_fetch_gotconfig_remote_repo
 run_test test_fetch_delete_remote_refs
 run_test test_fetch_honor_wt_conf_bflag
+run_test test_fetch_from_out_of_date_remote