Download raw body.
fix serving a branch of unrelated history with gotd
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
fix serving a branch of unrelated history with gotd