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

From: Jeff King <peff@peff.net>
Date: Sep 22, 2025, 17:12 UTC
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:

Show 6 quoted lines

> 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.

Show 13 quoted lines

> 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