On 23-01-18 12:14PM, Stefan Sperling wrote:
> While testing 'git pull' against got.gameoftrees.org I came across
> this error:
> 
>   fetch-pack: protocol error: bad band #69
>   fatal: protocol error: bad pack header
>   gotsh: unexpected 'have' packet
> 
> This occurs if there is a deep enough branch history to trigger
> git into sending lots of have-lines until the server sends an ack.
> Because this communication happens asynchronously we might already
> be expecting a 'done' packet while more have-lines keep arriving.
> 
> Fix this problem and add a test to hopefully trigger the issue reliably.
> Because this problem depends on timing it might not be 100% reproducible.
> But the test kept failing reliably on my laptop at least.
> 
> This might also fix the spurious 'unexpected flush packet' errors seen with
> 'got fetch' while already up-to-date, but I have not yet confirmed this.
> 
> ok?

You never cease to amaze with how quick you resolve these!

I can confirm the test passes with your fix and fails without it, ok

>  fix an issue where gotd fails to accept multiple have-lines from clients
>  
> diff 6da1c69cd7747c70dfbe29c9fb66fa03fa985459 f654fdbfc59bee89b4c114780e28be9aa19757f3
> commit - 6da1c69cd7747c70dfbe29c9fb66fa03fa985459
> commit + f654fdbfc59bee89b4c114780e28be9aa19757f3
> blob - 356daf210a6e33f99485b698040f4993bd37d9d3
> blob + c32e0bf5066a6f1b2b11a59ed1e4200d990d37ba
> --- gotd/session.c
> +++ gotd/session.c
> @@ -1102,7 +1102,7 @@ session_dispatch_listener(int fd, short events, void *
>  				err = ensure_client_is_writing(client);
>  				if (err)
>  					break;
> -			} else {
> +			} else if (client->state != GOTD_STATE_EXPECT_DONE) {
>  				err = got_error_msg(GOT_ERR_BAD_REQUEST,
>  				    "unexpected flush-pkt received");
>  				break;
> @@ -1123,7 +1123,7 @@ session_dispatch_listener(int fd, short events, void *
>  				log_debug("uid %d: expecting packfile",
>  				    client->euid);
>  				err = recv_packfile(client);
> -			} else {
> +			} else if (client->state != GOTD_STATE_EXPECT_DONE) {
>  				/* should not happen, see above */
>  				err = got_error_msg(GOT_ERR_BAD_REQUEST,
>  				    "unexpected client state");
> blob - 6e4c3ef0627fe92f92f4091b2926040aa3d9d9db
> blob + 02c4cb511fe756c9101538c93243233f34941dd2
> --- lib/serve.c
> +++ lib/serve.c
> @@ -899,7 +899,8 @@ serve_read(int infd, int outfd, int gotd_sock, const c
>  			break;
>  		if (n == 0) {
>  			if (curstate != STATE_EXPECT_MORE_WANT &&
> -			    curstate != STATE_EXPECT_HAVE) {
> +			    curstate != STATE_EXPECT_HAVE &&
> +			    curstate != STATE_EXPECT_DONE) {
>  				err = got_error_msg(GOT_ERR_BAD_PACKET,
>  				    "unexpected flush packet received");
>  				goto done;
> @@ -930,16 +931,21 @@ serve_read(int infd, int outfd, int gotd_sock, const c
>  			if (curstate == STATE_EXPECT_WANT)
>  				curstate = STATE_EXPECT_MORE_WANT;
>  		} else if (n >= 5 && strncmp(buf, "have ", 5) == 0) {
> -			if (curstate != STATE_EXPECT_HAVE) {
> +			if (curstate != STATE_EXPECT_HAVE &&
> +			    curstate != STATE_EXPECT_DONE) {
>  				err = got_error_msg(GOT_ERR_BAD_PACKET,
>  				    "unexpected 'have' packet");
>  				goto done;
>  			}
> -			err = recv_have(&have_ack, outfd, &ibuf, buf, n,
> -			    chattygot);
> -			if (err)
> -				goto done;
> -			seen_have = 1;
> +			if (curstate == STATE_EXPECT_HAVE) {
> +				err = recv_have(&have_ack, outfd, &ibuf,
> +				    buf, n, chattygot);
> +				if (err)
> +					goto done;
> +				seen_have = 1;
> +				if (have_ack)
> +					curstate = STATE_EXPECT_DONE;
> +			}
>  		} else if (n == 5 && strncmp(buf, "done\n", 5) == 0) {
>  			if (curstate != STATE_EXPECT_HAVE &&
>  			    curstate != STATE_EXPECT_DONE) {
> blob - cb89c0c20a28919ad7363ff1bba5419fdad3c7f1
> blob + 907efb48181d5468b2ba8bcf55d39ca905be2686
> --- regress/gotd/repo_write.sh
> +++ regress/gotd/repo_write.sh
> @@ -143,5 +143,116 @@ test_parseargs "$@"
>  	test_done "$testroot" "$ret"
>  }
>  
> +test_fetch_more_history() {
> +	local testroot=`test_init fetch_more_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
> +
> +	got checkout -q $testroot/repo-clone $testroot/wt >/dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got checkout failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	# Create some more commit history on the main branch.
> +	# History needs to be deep enough to trick 'git pull' into sending
> +	# a lot of 'have' lines, which triggered a bug in gotd.
> +	for i in `jot 50`; do
> +		echo "more alpha" >> $testroot/wt/alpha
> +		(cd $testroot/wt && got commit -m 'more changes' > /dev/null)
> +	done
> +	got send -b main -q -r $testroot/repo-clone
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got send failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	# create a second clone to test an incremental fetch with later
> +	got clone -q -m ${GOTD_TEST_REPO_URL} $testroot/repo-clone2
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got clone failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +	# same for Git, which used to fail:
> +	# fetch-pack: protocol error: bad band #69
> +	# fatal: protocol error: bad pack header
> +	# gotsh: unexpected 'have' packet
> +	git clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone3 \
> +		>$testroot/stdout 2>$testroot/stderr
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "git clone failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	# Create more commit history on the main branch
> +	echo "more alpha" >> $testroot/wt/alpha
> +	(cd $testroot/wt && got commit -m 'make changes' > /dev/null)
> +	echo "more beta" >> $testroot/wt/beta
> +	(cd $testroot/wt && got commit -m 'more changes' > /dev/null)
> +	(cd $testroot/wt && got rm epsilon/zeta > /dev/null)
> +	(cd $testroot/wt && got commit -m 'rm epsilon/zeta' > /dev/null)
> +	got send -b main -q -r $testroot/repo-clone
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got send failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	# Verify that the new changes can be fetched
> +	got fetch -q -r $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
> +beta
> +gamma/
> +gamma/delta
> +psi/
> +psi/new
> +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
> +
> +	# Verify that git pull works, too
> +	(cd $testroot/repo-clone3 && git pull -q > $testroot/stdout \
> +		2> $testroot/stderr)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "git pull failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	test_done "$testroot" "$ret"
> +}
> +
> +
>  test_parseargs "$@"
>  run_test test_send_basic
> +run_test test_fetch_more_history
> 

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68