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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jan 23, 2025, 11:08 UTC
Message-ID: <CAOLa=ZQMhhznFj=QdTgq=ydiPVzFO7YsEzk4VZ7fwn-ZZ=W=bQ@mail.gmail.com>
In-Reply-To: <20250122150222.GA3348514@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 32 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
>> 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@thelayzells.com>
>> Co-authored-by: Jeff King <peff@peff.net>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>
> 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@peff.net>
>

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.

Show 13 quoted lines

>> +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 `>`

Show 5 quoted lines

> 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.

Show 26 quoted lines

>> +		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