Re: [PATCH v3 2/5] builtin/refs: add "delete" subcommand

[Patrick Steinhardt <ps@pks.im>]

On Fri, Jul 03, 2026 at 12:54:17PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/builtin/refs.c b/builtin/refs.c
> > index f0faabf45a..edb7d61663 100644
> > --- a/builtin/refs.c
> > +++ b/builtin/refs.c
> > @@ -175,6 +178,52 @@ static int cmd_refs_optimize(int argc, const char **argv, const char *prefix,
> >  	return pack_refs_core(argc, argv, prefix, repo, refs_optimize_usage);
> >  }
> >  
> > +static int cmd_refs_delete(int argc, const char **argv, const char *prefix,
> > +			   struct repository *repo)
> > +{
> > +	static char const * const refs_delete_usage[] = {
> > +		REFS_DELETE_USAGE,
> > +		NULL
> > +	};
> > +	const char *message = NULL;
> > +	unsigned flags = 0;
> > +	struct option opts[] = {
> > +		OPT_STRING(0, "message", &message, N_("reason"),
> > +			   N_("reason of the update")),
> > +		OPT_BIT(0 ,"no-deref", &flags,
> > +			N_("update <refname> not the one it points to"),
> > +			REF_NO_DEREF),
> 
> Would it make sense to allow both --deref and --no-deref? (and --deref
> being the default)

Our "parse-options.h" subsystem is clever enough to make this work
already :)

> > diff --git a/t/t1464-refs-delete.sh b/t/t1464-refs-delete.sh
> > new file mode 100755
> > index 0000000000..efff7d0574
> > --- /dev/null
> > +++ b/t/t1464-refs-delete.sh
> > @@ -0,0 +1,130 @@
> > +#!/bin/sh
> > +
> > +test_description='git refs delete'
> > +
> > +. ./test-lib.sh
> > +
> > +setup_repo () {
> > +	git init "$1" &&
> > +	test_commit -C "$1" A &&
> > +	test_commit -C "$1" B
> > +}
> > +
> > +test_expect_success 'delete without oldvalue verification' '
> > +	test_when_finished "rm -rf repo" &&
> > +	setup_repo repo &&
> > +	A=$(git -C repo rev-parse A) &&
> > +	git -C repo update-ref refs/heads/foo $A &&
> > +	git -C repo refs delete refs/heads/foo &&
> > +	test_must_fail git -C repo show-ref --verify -q refs/heads/foo
> 
> Why not use `git refs exists` here? And why use `git -C repo` in this
> test, and `cd repo` in the other?

Yeah, there isn't really a good reason.

[snip]
> > +test_expect_success 'delete symref with --no-deref leaves target intact' '
> > +	test_when_finished "rm -rf repo" &&
> > +	setup_repo repo &&
> > +	(
> > +		cd repo &&
> > +		A=$(git rev-parse A) &&
> > +		git update-ref refs/heads/foo $A &&
> > +		git symbolic-ref refs/heads/symref refs/heads/foo &&
> > +		git refs delete --no-deref refs/heads/symref &&
> > +		test_must_fail git refs exists refs/heads/symref &&
> > +		git refs exists refs/heads/foo
> > +	)
> 
> What happens if you delete a symref and provide an <old-value>?

That's a good question indeed. It verifies that the symref target
resolves to <old-value>. That's the exact same behaviour as `git
update-ref -d`, even though it may be a bit on the funny side.

I've fixed up the above test and added another test for this behaviour,
but I'm not sure whether it makes sense to send another reroll for this.

Thanks!

Patrick