From: Jeff King <peff@peff.net>
Date: Mon, 22 Sep 2025 17:21:40 GMT
Subject: Re: [PATCH 4/4] refs: do not clobber dangling symrefs
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:

> 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