From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got fetch hang
To: James Cook <falsifian@falsifian.org>
Cc: gameoftrees@openbsd.org
Date: Mon, 20 Feb 2023 17:17:12 +0100

On Mon, Feb 20, 2023 at 03:08:02PM +0000, James Cook wrote:
> I've found a situation where "got fetch" hangs. Here are instructions to
> reproduce. I'm using got build at the main branch (commit 45e6b2f4).
> 
> Summary: clone a repo with two commits, then try to fetch from a repo
> with only the first commit.
> 
> Detailed steps:

Thank you, James. The patch below fixes the problem and adds
a test case based on your reproduction recipe.

-----------------------------------------------
 
 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
 
diff 45e6b2f427b11e0fc760c10ee96eae3a6a5f06e7 fe78ddb153c30329fc2090d4a1c72cf5015c5a98
commit - 45e6b2f427b11e0fc760c10ee96eae3a6a5f06e7
commit + fe78ddb153c30329fc2090d4a1c72cf5015c5a98
blob - 83de1f2f31433cea0f0bf579b0a90680763cd8e1
blob + bb635d09467269b207dc6d95439e5c975526a940
--- 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) {
@@ -617,11 +626,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);
 		if (err)
blob - d24224b5bc7fdac0107e60604c32eb162cf6598d
blob + 737b0cb81e316d7acd423ca57c68f090d5f33d9f
--- regress/cmdline/fetch.sh
+++ regress/cmdline/fetch.sh
@@ -1836,6 +1836,96 @@ test_parseargs "$@"
 	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_honor_wt_conf_bflag
 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