Jeff King <peff@xxxxxxxx> 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
>> 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.
>>
>> 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.
>
> Thanks for the explanation.
>
>> Reported-by: Nika Layzell <nika@xxxxxxxxxxxxxxx>
>> Co-authored-by: Jeff King <peff@xxxxxxxx>
>> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
>
> I don't know if we need my s-o-b to delete a few lines of code, but just
> in case:
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
>
I was looking at the docs and it doesn't specify if I should apply this
myself. I will go ahead and apply it for the next version, but it is
worthwhile to add to our documentation.
>> +test_expect_success 'update-ref should also create reflog for HEAD' '
>> + test_when_finished "rm -rf repo" &&
>> + git init repo &&
>> + (
>> + cd repo &&
>> + test_commit A &&
>> + test_commit B &&
>> + git rev-parse HEAD >>expect &&
>
> Using ">>" here is unexpected. It's OK because we are in a new repo (so
> there is no leftover "expect" file from a previous test) but probably
> better to stick to ">" unless we really need to append.
>
Yeah, I agree, this should be `>`
> Plus I don't think there is really any need for a new repo. The
> important thing is just updating the branch via update-ref (it doesn't
> even have to be a rewind, but of course it has to exist already, so a
> rewind is the simplest thing).
>
I agree there is no need for new repo, but the way our tests are written
there is often a lot of context leak between them. This isolates that
behavior.
>> + git update-ref --create-reflog refs/heads/main HEAD~ &&
>
> I agree with Patrick that we are probably better off just getting the
> branch name with symbolic-ref.
>
> So all together, something like:
>
> 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
> +'
> +
> test_done
>
Thanks Peff! I'll add it in! It's now completely your patch :)
> -Peff
Attachment:
signature.asc
Description: PGP signature
