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

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Subject: Re: [PATCH v2] refs: fix creation of corrupted reflogs for symrefs
>
> This may be just me, but every time I see the above title, it read
> to me as if we are on purpose doing "creation of corrupted reflogs
> for symrefs", but we are failing to do so for some reason, and this
> commit is about improving the situation so that we can correctly
> create corrupted reflog entries for symbolic ref updates.
>

> And because the only sensible reason why we may on purpose do
> "creation of corrupted reflogs" I can think of is perhaps we prepare
> such corrupted thing to test how robust the production code is when
> seeing such corrupted data, I would expect to see a change to t/
> hierarchy.
>
> But the patch touches the code, not just tests, which makes me
> doubly puzzled.
>
> It happens every time I see this title and the change.  Perhaps drop
> "corrupted" from the title?
>

>> 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
>
> I do not think the actor, who "exits early", is not "reflogs".

>> added as a performance optimization as it was assumed that no further
>> processing was required for reflog-only updates. However this was
>> 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.
>
> "The early exit skipped this required work"?
>

>> This caused a bug in Git 2.48 in the files backend 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.
>
> OK.
>
>> The early exit was added as a performance optimization for reflog-only
>> updates, and it wasn't essential to the original changes. As such,
>> reverting it shouldn't cause any further issues.
>
> I am not sure if this is even worth saying, as you already said that
> the early return was done incorrectly assuming that the remainder of
> the function can be skipped as an optimization.  What may help
> readers is to state that all the rest of the code path that was
> skipped by a mistaken optimization is necessary and would not do
> anything unwanted.
>

>> Reported-by: Nika Layzell <nika@thelayzells.com>
>> Co-authored-by: Jeff King <peff@peff.net>
>> Signed-off-by: Jeff King <peff@peff.net>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>
>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index e2316f1dd4..29045aad43 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -2068,4 +2068,13 @@ do
>>
>>  done
>>
>> +test_expect_success 'update-ref should also create reflog for HEAD' '
>> +	test_commit to-rewind &&
>> +	git rev-parse HEAD >expect &&
>> +	head=$(git symbolic-ref HEAD) &&
>> +	git update-ref --create-reflog "$head" HEAD~ &&
>> +	git rev-parse HEAD@{1} >actual &&
>> +	test_cmp expect actual
>> +'
>
> Nice.  We could rename "head" to something more meaningful (like
> "current branch") but I can live with the above version.  It is much
> nicer than assuming on what branch we would be, which was what the
> previous iteration did.
>
> Thanks.