Re: [PATCH 4/4] refs: do not clobber dangling symrefs

From: Jeff King <peff@peff.net>
Date: Sep 22, 2025, 17:21 UTC
Message-ID: <20250922172140.GB2202085@coredump.intra.peff.net>
In-Reply-To: <xmqqwm5qv5xh.fsf@gitster.g>

On Mon, Sep 22, 2025 at 08:54:34AM -0700, Junio C Hamano wrote:

Show 17 quoted lines

> Toon Claes <toon@iotcl.com> writes:
> 
> > We use `update FOO_HEAD 000...000 000..000` to delete a symref, if that symref
> > is dangling (otherwise the old oid would have resolved to something). I've
> > attached a patch that would allow this (on top of your patches). Do you think it
> > makes sense to allow this scenario?
> > ...
> > +	test_when_finished "git update-ref -d refs/heads/dangling" &&
> > +	git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
> > +	echo "update refs/heads/dangling $Z $Z" >stdin &&
> > +	git update-ref --no-deref --stdin <stdin &&
> 
> "git update-ref --help" seems to show that the "--stdin" mode has a
> separate command that is designed for exactly the purpose of removing
> a symbolic ref, though.  If you are changing the semantics of "update"
> to make it safer while dealing with a dangling symbolic ref, do you
> also need to touch the code path that handles "symref-delete" command?

I don't think so. Whatever we are trying to write (whether a regular ref, a symref, or a deletion), the "check the old value" code path ends up in the same place.

IMHO the directives for "update-ref --stdin" are a bit mis-designed. All of update/delete/verify should accept either "old-oid" or "old-target" (you do not need it for create, which always implies an old-oid of all-zeroes).

And then symref-* is used when you want the _new_ thing to be a symref. So symref-delete is not needed at all. You just have symref-* directives for create/update/verify. Which almost could be replaced by "ref <new-target>", but IIRC there was some syntactic ambiguity (because we allow new-target to be a ref, so you'd have to pick some invalid name like ":symref").

It is probably too late now to switch from "symref-update foo" to "update :ref foo" (and again, I think that may have even been considered and rejected). But we could add support for "ref <old-target>" to the non-symref commands. That is not just a syntactic weakness, but something you literally _can't_ do now (convert a symref into a regular ref atomically).

Anyway, all very off-topic for Toon's issue, though. I think his patch as-is does the right thing for his case, if we want to loosen it for historical reasons (see my other response).

-Peff