From: Jeff King <peff@peff.net>
Date: Mon, 22 Sep 2025 17:12:03 GMT
Subject: Re: [PATCH 4/4] refs: do not clobber dangling symrefs
Message-ID: <20250922171203.GA2202085@coredump.intra.peff.net>
In-Reply-To: <20250922122332.584428-1-toon@iotcl.com>

On Mon, Sep 22, 2025 at 02:23:32PM +0200, Toon Claes wrote:

> At $DAYJOB we hit into an edge-case where this patch breaks our expectancies.
> 
> 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?

Hmm. That's a funny command. You are providing _two_ null oids. The
first one says "this should be a deletion" and the second one says "the
previous state is that this should be deleted". So it should always be a
noop, if we are checking both sides.

I think the "right" way to say that is just:

  update FOO_HEAD 000...000

with no old-oid field at all. Or just:

  delete FOO_HEAD

but the two are internally the same thing.

So I think allowing this is working against what the patch is trying to
do, which is to consistently enforce the old-oid match that the user
asked for. The only thing that makes it an oddball is that it is
inherently a broken thing to ask for in the first place (at least under
the new, enforced regime). So we could perhaps allow it as a special
case for historical reasons without hurting anybody too badly.

I'd prefer not to do that, just because the refs code is already
complicated enough. But whether that's practical would depend on how
widespread this pattern is. Presumably it would not be that big a deal
to fix what you're sending (and assuming this is Gitaly, I'd guess that
it is bundled along with Git, so you are not that worried about people
using new Git with old Gitaly). But I'm not sure how we'd find out if
other people are doing the same thing in the wild.

So I dunno. My inclination is to say that the double-null-oid invocation
is weird and wrong, and callers should update if they need to. But I
could be convinced otherwise.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 1b3bf26add..5e46d3a110 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2537,7 +2537,7 @@ static enum ref_transaction_error check_old_oid(struct ref_update *update,
>  		 * that case to preserve the dangling symref.
>  		 */
>  		if ((update->flags & REF_NO_DEREF) && referent->len &&
> -		    is_null_oid(oid)) {
> +		    is_null_oid(oid) && !is_null_oid(&update->new_oid)) {
>  			strbuf_addf(err, "cannot lock ref '%s': "
>  				    "dangling symref already exists",
>  				    ref_update_original_update_refname(update));

I think the implementation here (and the matching one in the reftable
code) is correct for what you want to do. We should probably note the
special case in the comment above, too.

-Peff