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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 20, 2025, 07:27 UTC
Message-ID: <aKV44BDyIMyarinZ@pks.im>
In-Reply-To: <20250819192934.GD1059295@coredump.intra.peff.net>

On Tue, Aug 19, 2025 at 03:29:34PM -0400, Jeff King wrote:

Show 6 quoted lines

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

Show 44 quoted lines

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

Show 38 quoted lines

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

Show 27 quoted lines

> 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