From: "Omar Polo" <op@omarpolo.com>
Subject: Re: fix serving a branch of unrelated history with gotd
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Fri, 15 May 2026 18:18:09 +0200

Stefan Sperling <stsp@stsp.name> wrote:
> Fix a case where gotd started sending pack progress output in response
> to the client sending 'done', without any ACK or NAK in-between.
> 
> I saw this happen when trying to fetch an unrelated main branch from
> gotd, with completely different history and initial root commit,
> into an existing repository with a different main branch history.
>   
> Add test cases covering got fetch against a git-upload-pack server (is
> always passing), and 'got fetch' and 'git fetch' against gotd (was not
> passing, i.e. both got and git fail to fetch unless this fix is applied).
> 
> ok?

makes sense, okay op@

> M  lib/serve.c                 |    3+  2-
> M  regress/cmdline/fetch.sh    |  102+  0-
> M  regress/gotd/repo_write.sh  |  183+  0-
> 
> 3 files changed, 288 insertions(+), 2 deletions(-)
> 
> commit - 91c7e219d492ac01fd8fb77133a4d5a3d7f712f2
> commit + 66474b7c1b327523f0423ec5f4921d6e9ae6c841
> blob - e36084ae30ef3cd5c8813f82e04ba0c23a52a299
> blob + 5d19ebd9dd67070635de77c325debade916ca4e7
> --- lib/serve.c
> +++ lib/serve.c
> @@ -803,7 +803,7 @@ serve_read(int infd, int outfd, int gotd_sock, const c
>  		STATE_DONE,
>  	};
>  	enum protostate curstate = STATE_EXPECT_WANT;
> -	int have_ack = 0, use_sidebands = 0, seen_have = 0;
> +	int have_ack = 0, use_sidebands = 0, seen_have = 0, sent_nak = 0;
>  	int packfd = -1;
>  	size_t pack_chunksize;
>  
> @@ -864,6 +864,7 @@ serve_read(int infd, int outfd, int gotd_sock, const c
>  				err = send_nak(outfd, chattygot);
>  				if (err)
>  					goto done;
> +				sent_nak = 1;
>  			}
>  			if (curstate == STATE_EXPECT_MORE_WANT)
>  				curstate = STATE_EXPECT_HAVE_OR_DONE;
> @@ -908,7 +909,7 @@ serve_read(int infd, int outfd, int gotd_sock, const c
>  		}
>  	}
>  
> -	if (!seen_have) {
> +	if (!seen_have || (!have_ack && !sent_nak)) {
>  		err = send_nak(outfd, chattygot);
>  		if (err)
>  			goto done;
> blob - aac6a219e2b443b516724c8444a2c326b6bdcf49
> blob + ebc64209e918302ec7f357d2a756cb1ea495c017
> --- regress/cmdline/fetch.sh
> +++ regress/cmdline/fetch.sh
> @@ -2153,6 +2153,107 @@ test_fetch_basic_http() {
>  	test_done "$testroot" "$ret"
>  }
>  
> +test_fetch_unrelated_history() {
> +	local testroot=`test_init fetch_unrelated_history`
> +	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
> +
> +	got ref -l -r $testroot/repo-clone > $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got ref command failed unexpectedly" >&2
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
> +	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
> +	echo "refs/remotes/origin/HEAD: refs/remotes/origin/master" \
> +		>> $testroot/stdout.expected
> +	echo "refs/remotes/origin/master: $commit_id" \
> +		>> $testroot/stdout.expected
> +
> +	rm -rf $testroot/repo
> +	mkdir $testroot/repo
> +	git_init $testroot/repo
> +	echo 'new history with just one file' > $testroot/repo/alpha
> +	git -C $testroot/repo add alpha
> +	git_commit $testroot/repo -m "adding the test tree"
> +	touch $testroot/repo/.git/git-daemon-export-ok
> +	local commit_id2=`git_show_head $testroot/repo`
> +
> +	got fetch -q -r $testroot/repo-clone > $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
> +
> +	echo -n > $testroot/stdout.expected
> +
> +	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
> +
> +	got ref -l -r $testroot/repo > $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got ref command failed unexpectedly" >&2
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
> +	echo "refs/heads/master: $commit_id2" >> $testroot/stdout.expected
> +
> +	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
> +
> +	got ref -l -r $testroot/repo-clone > $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got ref command failed unexpectedly" >&2
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
> +	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
> +	echo "refs/remotes/origin/HEAD: refs/remotes/origin/master" \
> +		>> $testroot/stdout.expected
> +	echo "refs/remotes/origin/master: $commit_id2" \
> +		>> $testroot/stdout.expected
> +
> +	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
> +	test_done "$testroot" "$ret"
> +}
> +
> +
>  test_parseargs "$@"
>  run_test test_fetch_basic			no-sha256
>  run_test test_fetch_list			no-sha256
> @@ -2172,3 +2273,4 @@ run_test test_fetch_delete_remote_refs		no-sha256
>  run_test test_fetch_honor_wt_conf_bflag		no-sha256
>  run_test test_fetch_from_out_of_date_remote	no-sha256
>  run_test test_fetch_basic_http			no-sha256
> +run_test test_fetch_unrelated_history		no-sha256
> blob - b24dd5fd78d5c9570776c47220b98aa18dc4e3f1
> blob + 79fd816a3d8d965fa50236f65eb132af7ec334b3
> --- regress/gotd/repo_write.sh
> +++ regress/gotd/repo_write.sh
> @@ -554,9 +554,192 @@ EOF
>  	test_done "$testroot" 0
>  }
>  
> +test_fetch_unrelated_history() {
> +	local testroot=`test_init fetch_unrelated_history 1`
> +
> +	got clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got clone failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +	local commit_id0=`git_show_head $testroot/repo-clone`
> +	
> +	mkdir $testroot/new-repo
> +	git_init $testroot/new-repo
> +	git -C $testroot/new-repo symbolic-ref HEAD refs/heads/main
> +
> +	echo 'new history with just one file' > $testroot/new-repo/alpha
> +	git -C $testroot/new-repo add alpha
> +	git_commit $testroot/new-repo -m "adding the test tree"
> +	for i in 1 2 3; do
> +		echo 'line $i' >> $testroot/new-repo/alpha
> +		git -C $testroot/new-repo add alpha
> +		git_commit $testroot/new-repo -m "more changes" 
> +	done
> +	got tag -r $testroot/new-repo -m "tagging 1.0" 1.0 >/dev/null
> +	local commit_id2=`git_show_head $testroot/new-repo`
> +	local tag_id=`got ref -r "$testroot/new-repo" -l refs/tags/1.0 | \
> +		awk '{print $2}'`
> +
> +	cp $testroot/repo-clone/got.conf $testroot/new-repo/.git/got.conf
> +
> +	got send -q -b main -t 1.0 -r $testroot/new-repo -f
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got send failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	# Verify that the send operation worked fine.
> +	got clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone2
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got fetch failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	got tree -R -r $testroot/repo-clone2 > $testroot/stdout
> +	cat > $testroot/stdout.expected <<EOF
> +alpha
> +EOF
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	# This test checks for server-side behaviour where progress output
> +	# gets generated before any ACK or NAK arrive.
> +	# To trigger the problem we should not be sending any refs the
> +	# server already knows about. So delete the following:
> +	got ref -d -r $testroot/repo-clone refs/tags/1.0 > /dev/null
> +	got fetch -X -r $testroot/repo-clone origin > /dev/null
> +
> +	# Rewrite main branch hisotry to avoid sending a HAVE with a
> +	# commit ID which the server can find locally.
> +	got ref -r $testroot/repo-clone -d refs/heads/main > /dev/null
> +	mkdir $testroot/import-tree
> +	echo 'parallel universe alpha' > $testroot/import-tree/alpha
> +
> +	got import -r $testroot/repo-clone -b main -m init \
> +		$testroot/import-tree > /dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got import failed unexpectedly" >&2
> +		test_done "$testroot" 1
> +		return 1
> +	fi
> +	local commit_id=`git_show_head $testroot/repo-clone`
> +
> +	cp -r $testroot/repo-clone $testroot/repo-clone3
> +	cp $testroot/repo-clone/config $testroot/repo-clone3/config
> +
> +	# verify that unrelated history can be fetched into the initial clone.
> +	# This used to fail with:
> +	# got-fetch-pack: readpkt: 48:    [0x02]1 commits colored, \
> +	#    0 objects found, deltify 0%[0x0d]
> +	# got-fetch-pack: unexpected message from server
> +	got fetch -q -r $testroot/repo-clone > $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
> +
> +	echo -n > $testroot/stdout.expected
> +
> +	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
> +
> +	got ref -l -r $testroot/repo-clone > $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/main
> +refs/heads/main: $commit_id
> +refs/remotes/origin/HEAD: refs/remotes/origin/main
> +refs/remotes/origin/main: $commit_id2
> +refs/tags/1.0: $tag_id
> +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
> +
> +	# Now try the same fetch operation against gotd with Git.
> +	# This used to fail with:
> +	# fatal: git fetch-pack: expected ACK/NAK, got '?1 commits colored, \
> +	#     0 objects found, deltify 0%?'
> +	git -C $testroot/repo-clone3 fetch > $testroot/stdout \
> +		2> $testroot/stderr
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "git fetch command failed unexpectedly" >&2
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	echo -n > $testroot/stdout.expected
> +
> +	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
> +
> +	got ref -l -r $testroot/repo-clone3 > $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/main
> +refs/heads/main: $commit_id
> +refs/remotes/origin/HEAD: refs/remotes/origin/main
> +refs/remotes/origin/main: $commit_id2
> +refs/tags/1.0: $tag_id
> +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
> +
> +	test_done "$testroot" "$ret"
> +}
> +
>  test_parseargs "$@"
>  run_test test_send_basic
>  run_test test_fetch_more_history
>  run_test test_send_new_empty_branch
>  run_test test_delete_branch
>  run_test test_rewind_branch
> +run_test test_fetch_unrelated_history