Patrick Steinhardt <ps@xxxxxx> writes:
> 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
>
Will change.
>> 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.
>
Let me rephrase that to make it a little clearer.
>
>> 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.
>
Indeed, will add to 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.
>
Correct, the part which is crucial and was skipped was the call to
`refs_resolve_ref_unsafe()` right after the block in discussion.
>> 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.
>
Yes, will make that change.
> Patrick
Attachment:
signature.asc
Description: PGP signature
