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

From: Jeff King <peff@peff.net>
Date: Aug 20, 2025, 19:14 UTC
Message-ID: <20250820191441.GA1661980@coredump.intra.peff.net>
In-Reply-To: <aKV44BDyIMyarinZ@pks.im>

On Wed, Aug 20, 2025 at 09:27:28AM +0200, Patrick Steinhardt wrote:

Show 8 quoted lines

> > (Another curiosity is that the messages in the reftable code are
> > marked for translation, but the ones in the files backend are not. I
> > followed local convention in each case, but we may want to harmonize
> > this at some point).
> 
> Oh, interesting. I guess translating these messages is the right thing
> to do, as the messages are user facing. But this definitely does not
> have to be part of this patch series.

I know we left some unpack-trees messages untranslated because we thought users might depend on them (see unpack_plumbing_errors). I wondered if we might have done the same for the ref messages, but there's certainly no infrastructure around it. So it may just have been the case that nobody (yet) bothered to mark them.

Show 34 quoted lines

> > +	} else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD) {
> > +		if (oideq(&current_oid, &u->old_oid)) {
> > +			/*
> > +			 * Normally matching the expected old oid is enough. Either we
> > +			 * found the ref at the expected state, or we are creating and
> > +			 * expect the null oid (and likewise found nothing).
> > +			 *
> > +			 * But there is one exception for the null oid: if we found a
> > +			 * symref pointing to nothing we'll also get the null oid. In
> > +			 * regular recursive mode, that's good (we'll write to what the
> > +			 * symref points to, which doesn't exist). But in no-deref
> > +			 * mode, it means we'll clobber the symref, even though the
> > +			 * caller asked for this to be a creation event. So flag
> > +			 * that case to preserve the dangling symref.
> > +			 *
> > +			 * Everything else is OK and we can fall through to the
> > +			 * end of the conditional chain.
> > +			 */
> > +			if ((u->flags & REF_NO_DEREF) &&
> > +			    referent->len &&
> > +			    is_null_oid(&u->old_oid)) {
> > +				strbuf_addf(err, _("cannot lock ref '%s': "
> > +					    "dangling symref already exists"),
> > +					    ref_update_original_update_refname(u));
> > +				return REF_TRANSACTION_ERROR_CREATE_EXISTS;
> > +			}
> > +		} else if (is_null_oid(&u->old_oid)) {
> 
> Wouldn't it be more natural to put the new check into this `if
> (is_null_oid(&u->old_oid))` branch? Makes it a bit more explicit that we
> really only care about the case where we expect the ref to not exist.
> 
> Ah, no. I missed that you also change the original condition and move
> the `oideq()` call into the whole thing. Makes sense.

Yep, exactly. If we did the is_null_oid() check first then we'd have to check oideq() again inside that block, duplicating that logic. So there is no winning. :) I tried to keep the logic as close to the original as possible.

Show 11 quoted lines

> > +test_expect_success 'dangling symref overwritten without old oid' '
> > +	test_when_finished "git update-ref -d refs/heads/dangling" &&
> > +	git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
> > +	git update-ref --no-deref --stdin <<-\EOF &&
> > +	update refs/heads/dangling HEAD
> > +	EOF
> > +	git rev-parse --verify refs/heads/dangling &&
> > +	test_must_fail git rev-parse --verify refs/heads/does-not-exist
> 
> Do we also want to verify that the dangling symref got converted into a
> normal ref? Or do we already have other tests that do so?

My intent was that the above test does so: we know that "dangling" now points to a valid oid and that "does-not-exist" was not written to. Ergo, "dangling" is now a normal ref (the only other option is that it remained a symref and was pointed somewhere else entirely, but that seems like an unlikely bug to have).

-Peff