On Tue, Aug 19, 2025 at 03:29:34PM -0400, Jeff King wrote:
> The code for the fix is relatively straight-forward given the discussion
> above. But note that we have to implement it independently for the files
> and reftable backends. The "old oid" checks happen as part of the
> locking process, which is implemented separately for each system. We may
> want to factor this out somehow, but it's beyond the scope of this
> patch.
Yeah, there's a bunch of duplication here in general. I originally
wanted to refactor this at some point in time, but I never got around to
it. Also because I kind of shied away from it: the logic to lock and
check refs is quite intertwined with one another in both backends, so I
was afraid that this would ulmitately lead to splitting hairs.
> (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.
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 905555365b..a4419ef62d 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2512,13 +2512,37 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,
> */
> static enum ref_transaction_error check_old_oid(struct ref_update *update,
> struct object_id *oid,
> + struct strbuf *referent,
> struct strbuf *err)
> {
> if (update->flags & REF_LOG_ONLY ||
> - !(update->flags & REF_HAVE_OLD) ||
> - oideq(oid, &update->old_oid))
> + !(update->flags & REF_HAVE_OLD))
> return 0;
>
> + if (oideq(oid, &update->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.
> + */
> + if ((update->flags & REF_NO_DEREF) && referent->len &&
> + is_null_oid(oid)) {
> + strbuf_addf(err, "cannot lock ref '%s': "
> + "dangling symref already exists",
> + ref_update_original_update_refname(update));
> + return REF_TRANSACTION_ERROR_CREATE_EXISTS;
> + }
> + return 0;
> + }
> +
> if (is_null_oid(&update->old_oid)) {
> strbuf_addf(err, "cannot lock ref '%s': "
> "reference already exists",
Makes sense. If we've got an all-zero old object ID _but_ the locked
reference points to a nonexistet ref we refuse the update.
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 99fafd75eb..ef98584bf9 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1272,9 +1272,33 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
> ret = ref_update_check_old_target(referent->buf, u, err);
> if (ret)
> return ret;
> - } else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD &&
> - !oideq(¤t_oid, &u->old_oid)) {
> - if (is_null_oid(&u->old_oid)) {
> + } 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.
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index d29d23cb89..29b31e3b9b 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -2310,4 +2310,25 @@ test_expect_success 'update-ref should also create reflog for HEAD' '
> test_cmp expect actual
> '
>
> +test_expect_success 'dangling symref not overwritten by creation' '
> + test_when_finished "git update-ref -d refs/heads/dangling" &&
> + git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
> + test_must_fail git update-ref --no-deref --stdin 2>err <<-\EOF &&
> + create refs/heads/dangling HEAD
> + EOF
> + test_grep "cannot lock.*dangling symref already exists" err &&
> + test_must_fail git rev-parse --verify refs/heads/dangling &&
> + test_must_fail git rev-parse --verify refs/heads/does-not-exist
> +'
> +
> +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?
Patrick
