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