* Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs
2025-01-22 10:03 ` [PATCH] refs: fix creation of corrupted reflogs for symrefs Karthik Nayak
@ 2025-01-22 12:04 ` Patrick Steinhardt
2025-01-22 17:56 ` Junio C Hamano
2025-01-23 10:21 ` Karthik Nayak
2025-01-22 15:02 ` Jeff King
2025-01-23 11:29 ` [PATCH v2] " Karthik Nayak
2 siblings, 2 replies; 13+ messages in thread
From: Patrick Steinhardt @ 2025-01-22 12:04 UTC (permalink / raw)
To: Karthik Nayak; +Cc: peff, git, nika, gitster
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
s/reflog only/reflog-only
> 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.
It's a bit curious that you describe the fix here, then in the next
paragraph describe why we have skipped the logic only to reiterate the
fix.
> 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.
[snip]
> 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) {
> /*
Okay, makes sense. The error is specific to the "files" backend, which
might be worth mentioning in the commit message.
One thing that made me a bit curious is that we now end up executing
`check_old_oid()` for symref reflog entries, because we have
`REF_ISSYMREF` and `REF_NO_DEREF` set. But that function should end up
skipping the check because we explicitly unset `REF_HAVE_OLD` when
queueing the update. The remainder should be skipped because we have
`REF_LOG_ONLY` set.
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index e2316f1dd4..59493dd73f 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -4,6 +4,8 @@
> #
>
> 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
>
We could use `git symbolic-ref HEAD` to resolve the branch name instead
of overriding the branch name here.
Patrick
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs
2025-01-22 12:04 ` Patrick Steinhardt
@ 2025-01-22 17:56 ` Junio C Hamano
2025-01-23 10:21 ` Karthik Nayak
1 sibling, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2025-01-22 17:56 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, peff, git, nika
Patrick Steinhardt <ps@pks.im> writes:
>> 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.
>
> It's a bit curious that you describe the fix here, then in the next
> paragraph describe why we have skipped the logic only to reiterate the
> fix.
>
>> 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.
Yeah, that indeed is a "bit" curious. I'd call it confusing, though
;-).
> Okay, makes sense. The error is specific to the "files" backend, which
> might be worth mentioning in the commit message.
Indeed.
>> 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
>>
>
> We could use `git symbolic-ref HEAD` to resolve the branch name instead
> of overriding the branch name here.
I agree. That sounds like a more sensible way to go.
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs
2025-01-22 12:04 ` Patrick Steinhardt
2025-01-22 17:56 ` Junio C Hamano
@ 2025-01-23 10:21 ` Karthik Nayak
1 sibling, 0 replies; 13+ messages in thread
From: Karthik Nayak @ 2025-01-23 10:21 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: peff, git, nika, gitster
[-- Attachment #1: Type: text/plain, Size: 3041 bytes --]
Patrick Steinhardt <ps@pks.im> 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
>
> s/reflog only/reflog-only
>
Will change.
>> 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.
>
> It's a bit curious that you describe the fix here, then in the next
> paragraph describe why we have skipped the logic only to reiterate the
> fix.
>
Let me rephrase that to make it a little clearer.
>
>> 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.
>
> [snip]
>> 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) {
>> /*
>
> Okay, makes sense. The error is specific to the "files" backend, which
> might be worth mentioning in the commit message.
>
Indeed, will add to the commit message.
> One thing that made me a bit curious is that we now end up executing
> `check_old_oid()` for symref reflog entries, because we have
> `REF_ISSYMREF` and `REF_NO_DEREF` set. But that function should end up
> skipping the check because we explicitly unset `REF_HAVE_OLD` when
> queueing the update. The remainder should be skipped because we have
> `REF_LOG_ONLY` set.
>
Correct, the part which is crucial and was skipped was the call to
`refs_resolve_ref_unsafe()` right after the block in discussion.
>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index e2316f1dd4..59493dd73f 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -4,6 +4,8 @@
>> #
>>
>> 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
>>
>
> We could use `git symbolic-ref HEAD` to resolve the branch name instead
> of overriding the branch name here.
>
Yes, will make that change.
> Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs
2025-01-22 10:03 ` [PATCH] refs: fix creation of corrupted reflogs for symrefs Karthik Nayak
2025-01-22 12:04 ` Patrick Steinhardt
@ 2025-01-22 15:02 ` Jeff King
2025-01-23 11:08 ` Karthik Nayak
2025-01-23 11:29 ` [PATCH v2] " Karthik Nayak
2 siblings, 1 reply; 13+ messages in thread
From: Jeff King @ 2025-01-22 15:02 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, nika, gitster, ps
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>
> +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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs
2025-01-22 15:02 ` Jeff King
@ 2025-01-23 11:08 ` Karthik Nayak
2025-01-23 14:34 ` Jeff King
0 siblings, 1 reply; 13+ messages in thread
From: Karthik Nayak @ 2025-01-23 11:08 UTC (permalink / raw)
To: Jeff King; +Cc: git, nika, gitster, ps
[-- Attachment #1: Type: text/plain, Size: 3321 bytes --]
Jeff King <peff@peff.net> 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@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.
>> +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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs
2025-01-23 11:08 ` Karthik Nayak
@ 2025-01-23 14:34 ` Jeff King
0 siblings, 0 replies; 13+ messages in thread
From: Jeff King @ 2025-01-23 14:34 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, nika, gitster, ps
On Thu, Jan 23, 2025 at 03:08:30AM -0800, Karthik Nayak wrote:
> Thanks Peff! I'll add it in! It's now completely your patch :)
:) IMHO the hard part was understanding the implications of the removed
code block and writing the commit message. Any fool can delete a few
lines. ;)
-Peff
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] refs: fix creation of corrupted reflogs for symrefs
2025-01-22 10:03 ` [PATCH] refs: fix creation of corrupted reflogs for symrefs Karthik Nayak
2025-01-22 12:04 ` Patrick Steinhardt
2025-01-22 15:02 ` Jeff King
@ 2025-01-23 11:29 ` Karthik Nayak
2025-01-23 17:55 ` Junio C Hamano
2 siblings, 1 reply; 13+ messages in thread
From: Karthik Nayak @ 2025-01-23 11:29 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, nika, peff, ps
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@thelayzells.com>
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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@gmail.com>
## 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@thelayzells.com>
Co-authored-by: Jeff King <peff@peff.net>
+ Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
## 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2] refs: fix creation of corrupted reflogs for symrefs
2025-01-23 11:29 ` [PATCH v2] " Karthik Nayak
@ 2025-01-23 17:55 ` Junio C Hamano
2025-01-24 10:38 ` Karthik Nayak
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2025-01-23 17:55 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, nika, peff, ps
Karthik Nayak <karthik.188@gmail.com> writes:
> Subject: Re: [PATCH v2] refs: fix creation of corrupted reflogs for symrefs
This may be just me, but every time I see the above title, it read
to me as if we are on purpose doing "creation of corrupted reflogs
for symrefs", but we are failing to do so for some reason, and this
commit is about improving the situation so that we can correctly
create corrupted reflog entries for symbolic ref updates.
And because the only sensible reason why we may on purpose do
"creation of corrupted reflogs" I can think of is perhaps we prepare
such corrupted thing to test how robust the production code is when
seeing such corrupted data, I would expect to see a change to t/
hierarchy.
But the patch touches the code, not just tests, which makes me
doubly puzzled.
It happens every time I see this title and the change. Perhaps drop
"corrupted" from the title?
> 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
I do not think the actor, who "exits early", is not "reflogs".
Should we have "for reflogs" in the above, or perhaps move it to the
end of the sentence (i.e. the required lock gets obtained because we
want to do some operation "for reflogs")?
> 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.
"The early exit skipped this required work"?
> 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.
OK.
> 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.
I am not sure if this is even worth saying, as you already said that
the early return was done incorrectly assuming that the remainder of
the function can be skipped as an optimization. What may help
readers is to state that all the rest of the code path that was
skipped by a mistaken optimization is necessary and would not do
anything unwanted.
> Reported-by: Nika Layzell <nika@thelayzells.com>
> Co-authored-by: Jeff King <peff@peff.net>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> 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
> +'
Nice. We could rename "head" to something more meaningful (like
"current branch") but I can live with the above version. It is much
nicer than assuming on what branch we would be, which was what the
previous iteration did.
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] refs: fix creation of corrupted reflogs for symrefs
2025-01-23 17:55 ` Junio C Hamano
@ 2025-01-24 10:38 ` Karthik Nayak
0 siblings, 0 replies; 13+ messages in thread
From: Karthik Nayak @ 2025-01-24 10:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, nika, peff, ps
[-- Attachment #1: Type: text/plain, Size: 4150 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Subject: Re: [PATCH v2] refs: fix creation of corrupted reflogs for symrefs
>
> This may be just me, but every time I see the above title, it read
> to me as if we are on purpose doing "creation of corrupted reflogs
> for symrefs", but we are failing to do so for some reason, and this
> commit is about improving the situation so that we can correctly
> create corrupted reflog entries for symbolic ref updates.
>
Okay. I see what you mean.
> And because the only sensible reason why we may on purpose do
> "creation of corrupted reflogs" I can think of is perhaps we prepare
> such corrupted thing to test how robust the production code is when
> seeing such corrupted data, I would expect to see a change to t/
> hierarchy.
>
> But the patch touches the code, not just tests, which makes me
> doubly puzzled.
>
> It happens every time I see this title and the change. Perhaps drop
> "corrupted" from the title?
>
Yeah, that would make it much clearer.
>> 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
>
> I do not think the actor, who "exits early", is not "reflogs".
Took me some time to understand this, but I get what you're talking
about. My sentence adds ambiguity on what we're exactly exiting early.
> Should we have "for reflogs" in the above, or perhaps move it to the
> end of the sentence (i.e. the required lock gets obtained because we
> want to do some operation "for reflogs")?
>
Yeah that would make it much clearer.
>> 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.
>
> "The early exit skipped this required work"?
>
Yeah, that works!
>> 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.
>
> OK.
>
>> 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.
>
> I am not sure if this is even worth saying, as you already said that
> the early return was done incorrectly assuming that the remainder of
> the function can be skipped as an optimization. What may help
> readers is to state that all the rest of the code path that was
> skipped by a mistaken optimization is necessary and would not do
> anything unwanted.
>
That was what I was trying to convey.
>> Reported-by: Nika Layzell <nika@thelayzells.com>
>> Co-authored-by: Jeff King <peff@peff.net>
>> Signed-off-by: Jeff King <peff@peff.net>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>
>> 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
>> +'
>
> Nice. We could rename "head" to something more meaningful (like
> "current branch") but I can live with the above version. It is much
> nicer than assuming on what branch we would be, which was what the
> previous iteration did.
>
> Thanks.
I agree, this is much nicer indeed.
Also I just noticed that you have already amended the commit message
and added it to `next`. Thanks for doing that. Happy to re-roll if
needed!
Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread