On Wed, Aug 20, 2025 at 09:27:28AM +0200, Patrick Steinhardt wrote:
> > (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.
> > + } else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD) {
> > + if (oideq(¤t_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.
> > +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
