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 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.
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.
Reported-by: Nika Layzell <nika@xxxxxxxxxxxxxxx>
Co-authored-by: Jeff King <peff@xxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
---
Changes since the v1:
- Modified the commit message and subject to make it a little more clearer.
- Used `git symbolic-ref HEAD` in the test instead of setting
the default branch.
Range diff:
1: dbb1186512 ! 1: 9326fb08a9 reflog: fix bug which didn't resolve symref reflogs
@@ Metadata
Author: Karthik Nayak <karthik.188@xxxxxxxxx>
## Commit message ##
- reflog: fix bug which didn't resolve symref reflogs
+ refs: fix creation of corrupted reflogs for symrefs
The commit 297c09eabb (refs: allow multiple reflog entries for the same
- refname, 2024-12-16) added logic to skip the flow for reflogs in
- `lock_ref_for_update()` after obtaining the required lock. This was done
- because it was assumed that the flow ends there for reflogs. However
- this was incorrect since for a symref's reflog entry, we need to
- populate the old_oid value which is done right after.
+ 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.
+ 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.
+
+ 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.
Reported-by: Nika Layzell <nika@xxxxxxxxxxxxxxx>
Co-authored-by: Jeff King <peff@xxxxxxxx>
+ Signed-off-by: Jeff King <peff@xxxxxxxx>
Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
## refs/files-backend.c ##
@@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref
/*
## t/t1400-update-ref.sh ##
-@@
- #
-
- test_description='Test git update-ref and basic ref logging'
-+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
-+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
-
- . ./test-lib.sh
-
@@ t/t1400-update-ref.sh: do
done
+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 &&
-+ git update-ref --create-reflog refs/heads/main HEAD~ &&
-+ git rev-parse HEAD@{1} >actual &&
-+ test_cmp expect actual
-+ )
++ 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
---
refs/files-backend.c | 3 ---
t/t1400-update-ref.sh | 9 +++++++++
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5cfb8b7ca8..29f08dced4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2615,9 +2615,6 @@ static int lock_ref_for_update(struct files_ref_store *refs,
update->backend_data = lock;
- if (update->flags & REF_LOG_ONLY)
- goto out;
-
if (update->type & REF_ISSYMREF) {
if (update->flags & REF_NO_DEREF) {
/*
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
--
2.47.0
