On 2023/01/27 18:09:50 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Jan 27, 2023 at 05:37:26PM +0100, Omar Polo wrote:
> > This makes "got send -d" works against gotd.
> > 
> > The idea is that, if a server supports this capability, clients can
> > delete references by sending a zero-id value as the target value of
> > the update.
> 
> The client actually sends an old-id and a new zero-id.
>  
> > I'm not 100% sure the locking is right, but it resambles the update
> > case so maybe it is.
> 
> After locking, gotd should resolve the ref to obtain its current ID,
> and not delete if current ID != old ID sent by the client.
> Otherwise, during a race, one client would silently win over another.

ahhh right, haven't thought about that case.  it what we do when
updating a ref too, I should have seen it, sorry.

I've stolen the error message from the update case, hope it's fine.

> > There are some improvements that we can do on top of this:
> > 
> >  - the ref_is_new and delete_ref fields of the struct
> >    gotd{,_imsg}_ref_update could be merged in a single field using an
> >    enum, and
> 
> Not worth it, I would say. Just keep two flags.

sure

> >  - as a special case (but it's actually the more common one I guess),
> >    the session process could avoid sending the fds for the pack idx
> >    and data when a client is only deleting references.
> 
> Not sure. Just keep sending them for now, so we have less special cases.

agreed.  I wanted to do it to avoid a few imsg and temp files, but it
was not straightforward so I deferred.  It's also not particularly
performance sensitive so I guess it's fine.

updated diff


(had a weird feeling typing SHA1_DIGEST_LENGTH... ;-)

-----------------------------------------------
commit b219819442474860eb2e49e50f3dcc1f0a8506c0 (main)
from: Omar Polo <op@omarpolo.com>
date: Fri Jan 27 17:16:27 2023 UTC
 
 gotd: implement the delete-refs capability
 
diff 711bec03cb52c156a80885f2ee8ad7fce27dd57e b219819442474860eb2e49e50f3dcc1f0a8506c0
commit - 711bec03cb52c156a80885f2ee8ad7fce27dd57e
commit + b219819442474860eb2e49e50f3dcc1f0a8506c0
blob - de4f9c2c78944e2a095e39c114c19e9baed240f5
blob + e22197aef200b4b37d02bf22b05649f1af0a55a4
--- gotd/gotd.h
+++ gotd/gotd.h
@@ -325,6 +325,7 @@ struct gotd_imsg_ref_update {
 	uint8_t old_id[SHA1_DIGEST_LENGTH];
 	uint8_t new_id[SHA1_DIGEST_LENGTH];
 	int ref_is_new;
+	int delete_ref;
 	uint32_t client_id;
 	size_t name_len;
 
blob - a1082a01690cd74692583d348692d92175800c69
blob + 06a5bdff23de1154538510232e0cf2998523a312
--- gotd/repo_write.c
+++ gotd/repo_write.c
@@ -75,6 +75,7 @@ struct gotd_ref_update {
 	STAILQ_ENTRY(gotd_ref_update) entry;
 	struct got_reference *ref;
 	int ref_is_new;
+	int delete_ref;
 	struct got_object_id old_id;
 	struct got_object_id new_id;
 };
@@ -89,6 +90,7 @@ static struct repo_write_client {
 	int				 packidx_fd;
 	struct gotd_ref_updates		 ref_updates;
 	int				 nref_updates;
+	int				 nref_del;
 	int				 nref_new;
 } repo_write_client;
 
@@ -258,6 +260,7 @@ list_refs(struct imsg *imsg)
 	client->pack_pipe = -1;
 	client->packidx_fd = -1;
 	client->nref_updates = 0;
+	client->nref_del = 0;
 	client->nref_new = 0;
 
 	imsg_init(&ibuf, client_fd);
@@ -330,6 +333,7 @@ recv_ref_update(struct imsg *imsg)
 static const struct got_error *
 recv_ref_update(struct imsg *imsg)
 {
+	static const char zero_id[SHA1_DIGEST_LENGTH];
 	const struct got_error *err = NULL;
 	struct repo_write_client *client = &repo_write_client;
 	struct gotd_imsg_ref_update iref;
@@ -418,6 +422,10 @@ recv_ref_update(struct imsg *imsg)
 	    repo_write.pid);
 
 	ref_update->ref = ref;
+	if (memcmp(ref_update->new_id.sha1, zero_id, sizeof(zero_id)) == 0) {
+		ref_update->delete_ref = 1;
+		client->nref_del++;
+	}
 	STAILQ_INSERT_HEAD(&client->ref_updates, ref_update, entry);
 	client->nref_updates++;
 	ref = NULL;
@@ -705,6 +713,11 @@ recv_packdata(off_t *outsize, uint32_t *nobj, uint8_t 
 
 	*outsize = 0;
 	*nobj = 0;
+
+	/* if only deleting references there's nothing to read */
+	if (client->nref_updates == client->nref_del)
+		return NULL;
+
 	SHA1Init(&ctx);
 
 	err = got_poll_read_full(infd, &have, &hdr, sizeof(hdr), sizeof(hdr));
@@ -972,6 +985,15 @@ recv_packfile(int *have_packfile, struct imsg *imsg)
 	    client->nref_updates == client->nref_new)
 		goto done;
 
+	/*
+	 * Clients which are deleting references only will send
+	 * no pack file.
+	 */
+	if (nobj == 0 &&
+	    client->nref_del > 0 &&
+	    client->nref_updates == client->nref_del)
+		goto done;
+
 	pack->filesize = pack_filesize;
 	*have_packfile = 1;
 
@@ -1048,6 +1070,9 @@ verify_packfile(void)
 		return err;
 
 	STAILQ_FOREACH(ref_update, &client->ref_updates, entry) {
+		if (ref_update->delete_ref)
+			continue;
+
 		err = got_object_id_str(&id_str, &ref_update->new_id);
 		if (err)
 			goto done;
@@ -1121,6 +1146,7 @@ send_ref_update(struct gotd_ref_update *ref_update, st
 	memcpy(iref.old_id, ref_update->old_id.sha1, SHA1_DIGEST_LENGTH);
 	memcpy(iref.new_id, ref_update->new_id.sha1, SHA1_DIGEST_LENGTH);
 	iref.ref_is_new = ref_update->ref_is_new;
+	iref.delete_ref = ref_update->delete_ref;
 	iref.client_id = client->id;
 	iref.name_len = strlen(refname);
 
blob - c2bf744e16922aec4500424898af4499c5240308
blob + bbe5f89f20242a5571907a6f813d02388bacddf4
--- gotd/session.c
+++ gotd/session.c
@@ -432,7 +432,8 @@ update_ref(int *shut, struct gotd_session_client *clie
 
 	memcpy(old_id.sha1, iref.old_id, SHA1_DIGEST_LENGTH);
 	memcpy(new_id.sha1, iref.new_id, SHA1_DIGEST_LENGTH);
-	err = got_object_open(&obj, repo, &new_id);
+	err = got_object_open(&obj, repo,
+	    iref.delete_ref ? &old_id : &new_id);
 	if (err)
 		goto done;
 
@@ -454,6 +455,30 @@ update_ref(int *shut, struct gotd_session_client *clie
 			    got_ref_get_name(ref));
 			goto done;
 		}
+	} else if (iref.delete_ref) {
+		err = got_ref_open(&ref, repo, refname, 1 /* lock */);
+		if (err)
+			goto done;
+		locked = 1;
+
+		err = got_ref_resolve(&id, repo, ref);
+		if (err)
+			goto done;
+
+		if (got_object_id_cmp(id, &old_id) != 0) {
+			err = got_error_fmt(GOT_ERR_REF_BUSY,
+			    "%s has been modified by someone else "
+			    "while transaction was in progress",
+			    got_ref_get_name(ref));
+			goto done;
+		}
+
+		err = got_ref_delete(ref, repo);
+		if (err)
+			goto done;
+
+		free(id);
+		id = NULL;
 	} else {
 		err = got_ref_open(&ref, repo, refname, 1 /* lock */);
 		if (err)
blob - 98f9aeb0236ac62592fd35493c4c5c761a8445d6
blob + d21d2a42eb07a9427920b7bc02e980a4ee9a5a22
--- lib/serve.c
+++ lib/serve.c
@@ -59,9 +59,7 @@ static const struct got_capability write_capabilities[
 	{ GOT_CAPA_OFS_DELTA, NULL },
 	{ GOT_CAPA_REPORT_STATUS, NULL },
 	{ GOT_CAPA_NO_THIN, NULL },
-#if 0
 	{ GOT_CAPA_DELETE_REFS, NULL },
-#endif
 };
 
 const struct got_error *
blob - c9dca58857cb121368aa8d3378609e72787267c1
blob + b48e2b54f360c89a3ce5d4c9e38d4dd5a7a3532f
--- regress/gotd/repo_write.sh
+++ regress/gotd/repo_write.sh
@@ -292,8 +292,98 @@ test_send_new_empty_branch() {
 	test_done "$testroot" "$ret"
 }
 
+test_delete_branch() {
+	local testroot=`test_init delete_branch 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
+
+	(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
+
+	got send -r $testroot/repo-clone -d foo >$testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got send -d failed unexpectedly" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	cat <<EOF >$testroot/stdout.expected
+Connecting to "origin" ${GOTD_TEST_REPO_URL}
+Server has deleted refs/heads/foo
+EOF
+	if ! cmp -s $testroot/stdout.expected $testroot/stdout; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	# now try again but while also updating another branch
+	# other than deleting `foo'.
+
+	(cd $testroot/wt && got up -b main && \
+		echo 'more alpha' > alpha && \
+		got commit -m 'edit alpha on main' && \
+		got send -q -b foo) >/dev/null
+
+	got send -r $testroot/repo-clone -d foo -b main | \
+		grep '^Server has' >$testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got send -d foo -b main failed unexpectedly" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	cat <<EOF >$testroot/stdout.expected
+Server has accepted refs/heads/main
+Server has deleted refs/heads/foo
+EOF
+	if ! cmp -s $testroot/stdout.expected $testroot/stdout; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		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