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

[Patrick Steinhardt <ps@pks.im>]

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(&current_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(&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.

> 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