Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs

[Patrick Steinhardt <ps@pks.im>]

On Wed, Jan 22, 2025 at 11:03:19AM +0100, Karthik Nayak wrote:
> The commit 297c09eabb (refs: allow multiple reflog entries for the same
> refname, 2024-12-16) added logic for reflogs to exit early in
> `lock_ref_for_update()` after obtaining the required lock. This was
> added as a performance optimization as it was assumed that no further
> processing was required for reflog only updates. However this was

s/reflog only/reflog-only

> incorrect since for a symref's reflog entry, the update needs to be
> populated with the old_oid value. This is done right after the early
> exit.
> 
> This caused a bug in Git 2.48 where target references of symrefs being
> updated would create a corrupted reflog entry for the symref since the
> old_oid is not populated. Undo the skip in logic to fix this issue and
> also add a test to ensure that such an issue doesn't arise in the
> future.

It's a bit curious that you describe the fix here, then in the next
paragraph describe why we have skipped the logic only to reiterate the
fix.

> The early exit was added as a performance optimization for reflog-only
> updates, but this accidentally broke symref reflog handling. Remove the
> optimization since it wasn't essential to the original changes.

[snip]
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 5cfb8b7ca8..29f08dced4 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2615,9 +2615,6 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>  
>  	update->backend_data = lock;
>  
> -	if (update->flags & REF_LOG_ONLY)
> -		goto out;
> -
>  	if (update->type & REF_ISSYMREF) {
>  		if (update->flags & REF_NO_DEREF) {
>  			/*

Okay, makes sense. The error is specific to the "files" backend, which
might be worth mentioning in the commit message.

One thing that made me a bit curious is that we now end up executing
`check_old_oid()` for symref reflog entries, because we have
`REF_ISSYMREF` and `REF_NO_DEREF` set. But that function should end up
skipping the check because we explicitly unset `REF_HAVE_OLD` when
queueing the update. The remainder should be skipped because we have
`REF_LOG_ONLY` set.

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index e2316f1dd4..59493dd73f 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -4,6 +4,8 @@
>  #
>  
>  test_description='Test git update-ref and basic ref logging'
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>  
>  . ./test-lib.sh
>  

We could use `git symbolic-ref HEAD` to resolve the branch name instead
of overriding the branch name here.

Patrick