Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jan 23, 2025, 10:21 UTC
- Message-ID
- <CAOLa=ZT3ZHAC1mDQj=tezzdnJ_m413Oez9v20oZ4t5Rh3M5k1w@mail.gmail.com>
- In-Reply-To
- <Z5De17-Ed3K31kzh@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 9 quoted lines
> 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.
Show 14 quoted lines
>> 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.
Show 24 quoted lines
>
>> 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.
Show 7 quoted lines
> 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.
Show 17 quoted lines
>> 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