From: Omar Polo <op@omarpolo.com>
Subject: Re: make rewinding of branches work with gotd
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Mon, 10 Apr 2023 19:32:51 +0200

On 2023/04/10 18:23:39 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> Fix a spurious empty packfile error from gotd when rewinding a branch.
> 
> ok?

ok

> diff 7755e2f80723b22eb4fcc203ffd75608db7055c7 8146f0364982425931d98b5cf31a12252b3dff39
> commit - 7755e2f80723b22eb4fcc203ffd75608db7055c7
> commit + 8146f0364982425931d98b5cf31a12252b3dff39
> blob - 317d11a2dde1181dd8dca488ba5ed7f353895c70
> blob + eefdc7de1087c2a2956a9fdbbd25ad81719a783c
> --- gotd/repo_write.c
> +++ gotd/repo_write.c
> @@ -97,6 +97,7 @@ static struct repo_write_client {
>  	int				 nref_updates;
>  	int				 nref_del;
>  	int				 nref_new;
> +	int				 nref_move;
>  } repo_write_client;
>  
>  static volatile sig_atomic_t sigint_received;
> @@ -265,6 +266,7 @@ list_refs(struct imsg *imsg)
>  	client->nref_updates = 0;
>  	client->nref_del = 0;
>  	client->nref_new = 0;
> +	client->nref_move = 0;
>  
>  	imsg_init(&ibuf, client_fd);
>  
> @@ -971,6 +973,24 @@ recv_packdata(off_t *outsize, uint32_t *nobj, uint8_t 
>  }
>  
>  static const struct got_error *
> +ensure_all_objects_exist_locally(struct gotd_ref_updates *ref_updates)
> +{
> +	const struct got_error *err = NULL;
> +	struct gotd_ref_update *ref_update;
> +	struct got_object *obj;
> +
> +	STAILQ_FOREACH(ref_update, ref_updates, entry) {
> +		err = got_object_open(&obj, repo_write.repo,
> +		    &ref_update->new_id);
> +		if (err)
> +			return err;
> +		got_object_close(obj);
> +	}
> +
> +	return NULL;
> +}
> +
> +static const struct got_error *
>  recv_packdata(off_t *outsize, uint32_t *nobj, uint8_t *sha1,
>      int infd, int outfd)
>  {
> @@ -1019,8 +1039,20 @@ recv_packdata(off_t *outsize, uint32_t *nobj, uint8_t 
>  		    client->nref_updates == client->nref_new)
>  			return NULL;
>  
> -		return got_error_msg(GOT_ERR_BAD_PACKFILE,
> -		    "bad packfile with zero objects");
> +		/*
> +		 * Clients which only move existing refs will send us an empty
> +		 * pack file. All referenced objects must exist locally.
> +		 */
> +		err = ensure_all_objects_exist_locally(&client->ref_updates);
> +		if (err) {
> +			if (err->code != GOT_ERR_NO_OBJ)
> +				return err;
> +			return got_error_msg(GOT_ERR_BAD_PACKFILE,
> +			    "bad packfile with zero objects");
> +		}
> +
> +		client->nref_move = client->nref_updates;
> +		return NULL;
>  	}
>  
>  	log_debug("expecting %d objects", *nobj);
> @@ -1271,6 +1303,16 @@ recv_packfile(int *have_packfile, struct imsg *imsg)
>  	    client->nref_updates == client->nref_del)
>  		goto done;
>  
> +	/*
> +	 * Clients which only move existing refs will send us an empty
> +	 * pack file. All referenced objects must exist locally.
> +	 */
> +	if (nobj == 0 &&
> +	    pack_filesize == sizeof(struct got_packfile_hdr) &&
> +	    client->nref_move > 0 &&
> +	    client->nref_updates == client->nref_move)
> +		goto done;
> +
>  	pack->filesize = pack_filesize;
>  	*have_packfile = 1;
>  
> blob - d75e82812ee4b8e5c04144e1f6f981703b541699
> blob + daff924b47ca360160665215790f700bf0f8bfda
> --- regress/gotd/repo_write.sh
> +++ regress/gotd/repo_write.sh
> @@ -458,8 +458,95 @@ test_parseargs "$@"
>  	test_done "$testroot" 0
>  }
>  
> +test_rewind_branch() {
> +	local testroot=`test_init rewind_branch 1`
> +
> +	got clone -a -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
> +
> +	(cd $testroot/wt && got branch foo) >/dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got branch failed unexpectedly" >&2
> +		test_done "$testroot" 1
> +		return 1
> +	fi
> +
> +	echo modified alpha > $testroot/wt/alpha
> +	(cd $testroot/wt && got commit -m 'edit alpha') >/dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got commit failed unexpectedly" >&2
> +		test_done "$testroot" 1
> +		return 1
> +	fi
> +
> +	if ! got send -q -r $testroot/repo-clone -b foo; then
> +		echo "got send failed unexpectedly" >&2
> +		test_done "$testroot" 1
> +		return 1
> +	fi
> +
> +	local foo_id=`git_show_branch_head "$testroot/repo-clone" foo`
> +	local main_id=`git_show_branch_head "$testroot/repo-clone" main`
> +	local tag_id=`got ref -r "$testroot/repo-clone" -l refs/tags/1.0 | \
> +		awk '{print $2}'`
> +
> +	(cd $testroot/wt && got update -c $main_id) >/dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got update failed unexpectedly" >&2
> +		test_done "$testroot" 1
> +		return 1
> +	fi
> +
> +	(cd $testroot/wt && got histedit -d) >/dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got histedit failed unexpectedly" >&2
> +		test_done "$testroot" 1
> +		return 1
> +	fi
> +
> +	if ! got send -q -r $testroot/repo-clone -f -b foo; then
> +		echo "got send failed unexpectedly" >&2
> +		test_done "$testroot" 1
> +		return 1
> +	fi
> +
> +	got fetch -q -r $testroot/repo-clone -l >$testroot/refs
> +	cat <<EOF >$testroot/refs.expected
> +HEAD: refs/heads/main
> +HEAD: $main_id
> +refs/heads/foo: $main_id
> +refs/heads/main: $main_id
> +refs/tags/1.0: $tag_id
> +EOF
> +	if ! cmp -s $testroot/refs.expected $testroot/refs; then
> +		diff -u $testroot/refs.expected $testroot/refs
> +		test_done "$testroot" 1
> +		return 1
> +	fi
> +
> +	test_done "$testroot" 0
> +}
> +
>  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