Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs
- From
Jeff King <peff@peff.net>
- Date
- Jan 22, 2025, 15:02 UTC
- Message-ID
- <20250122150222.GA3348514@coredump.intra.peff.net>
- In-Reply-To
- <20250122100319.2280647-1-karthik.188@gmail.com>
On Wed, Jan 22, 2025 at 11:03:19AM +0100, Karthik Nayak wrote:
Show 18 quoted lines
> 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>
Show 8 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.
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).
> + 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 -Peff