threads / discuss / 62839

`git update-ref` fails to set reflog old_oid in 2.48

Subject: `git update-ref` fails to set reflog old_oid in 2.48

## tl;dr

13 messages between Jan 21, 2025 and Jan 24, 2025.

replies: 12people: 5as markdown or json

Nika Layzell· Jan 21, 2025, 20:40 UTC · lore

In git 2.48.1, the `git update-ref` subcommand no longer correctly updates the reflog in some cases. Specifically, it appears that the `old_oid` field will not be updated when modifying a branch referenced by another symbolic ref (e.g. HEAD). This doesn't break the `git reflog` subcommand, but does break references like `HEAD@{1}`, which appear to read the `old_oid` field.

STR (in a fresh directory): ``` git init -b main git commit --allow-empty -m "A" git commit --allow-empty -m "B" git update-ref -m "reason" refs/heads/main HEAD~ HEAD ```

Expected Result: ``` $ git rev-parse HEAD@{1} 70d9116663eee9f01065c3a6d8984b1dea661f20

$ cat .git/logs/HEAD 0000000000000000000000000000000000000000 17dd31aaf89190a36b8d04136a1a0f83fb37da4c AUTHOR <EMAIL> TIME commit (initial): A 17dd31aaf89190a36b8d04136a1a0f83fb37da4c 70d9116663eee9f01065c3a6d8984b1dea661f20 AUTHOR <EMAIL> TIME commit: B 70d9116663eee9f01065c3a6d8984b1dea661f20 17dd31aaf89190a36b8d04136a1a0f83fb37da4c AUTHOR <EMAIL> TIME reason ```

The `old_oid` field for the reflog entry added by `git update-ref` is present, and correctly initialized. This was the case prior to git 2.48.

Actual Result: ``` $ git rev-parse HEAD@{1} warning: log for ref HEAD unexpectedly ended on TIME 17dd31aaf89190a36b8d04136a1a0f83fb37da4c

$ cat .git/logs/HEAD 0000000000000000000000000000000000000000 17dd31aaf89190a36b8d04136a1a0f83fb37da4c AUTHOR <EMAIL> TIME commit (initial): A 17dd31aaf89190a36b8d04136a1a0f83fb37da4c 70d9116663eee9f01065c3a6d8984b1dea661f20 AUTHOR <EMAIL> TIME commit: B 0000000000000000000000000000000000000000 17dd31aaf89190a36b8d04136a1a0f83fb37da4c AUTHOR <EMAIL> TIME reason ```

The `old_oid` field is empty (all zeroes). This is only the case in derived reflogs (in this case .git/logs/HEAD). The reflog for refs/heads/main appears to be updated correctly.

Jeff King· Jan 21, 2025, 21:52 UTC · re: Nika Layzell · lore

Re: `git update-ref` fails to set reflog old_oid in 2.48

On Tue, Jan 21, 2025 at 03:40:06PM -0500, Nika Layzell wrote:
Show 18 quoted lines
> In git 2.48.1, the `git update-ref` subcommand no longer correctly
> updates the reflog in some cases. Specifically, it appears that the
> `old_oid` field will not be updated when modifying a branch referenced
> by another symbolic ref (e.g. HEAD). This doesn't break the `git
> reflog` subcommand, but does break references like `HEAD@{1}`, which
> appear to read the `old_oid` field.
> 
> STR (in a fresh directory):
> ```
> git init -b main
> git commit --allow-empty -m "A"
> git commit --allow-empty -m "B"
> git update-ref -m "reason" refs/heads/main HEAD~ HEAD
> ```
> [...]
> The `old_oid` field is empty (all zeroes). This is only the case in
> derived reflogs (in this case .git/logs/HEAD). The reflog for
> refs/heads/main appears to be updated correctly.

Thanks for an easy reproduction recipe. Looks like it bisects to 297c09eabb (refs: allow multiple reflog entries for the same refname, 2024-12-16). Author cc'd.

Just looking at the diff, the early return from lock_ref_for_update() seems suspicious to me. Doing this:

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8953d1c6d3..1c0e24a855 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2611,9 +2611,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) {
 			/*

makes the problem go away, and doesn't fail any tests. But that is just
me poking at it without understanding why those lines were there in the
first place.

-Peff
Karthik Nayak· Jan 22, 2025, 06:47 UTC · re: Jeff King · lore

Re: `git update-ref` fails to set reflog old_oid in 2.48

Jeff King <peff@peff.net> writes:
Show 48 quoted lines
>
> On Tue, Jan 21, 2025 at 03:40:06PM -0500, Nika Layzell wrote:
>
>> In git 2.48.1, the `git update-ref` subcommand no longer correctly
>> updates the reflog in some cases. Specifically, it appears that the
>> `old_oid` field will not be updated when modifying a branch referenced
>> by another symbolic ref (e.g. HEAD). This doesn't break the `git
>> reflog` subcommand, but does break references like `HEAD@{1}`, which
>> appear to read the `old_oid` field.
>>
>> STR (in a fresh directory):
>> ```
>> git init -b main
>> git commit --allow-empty -m "A"
>> git commit --allow-empty -m "B"
>> git update-ref -m "reason" refs/heads/main HEAD~ HEAD
>> ```
>> [...]
>> The `old_oid` field is empty (all zeroes). This is only the case in
>> derived reflogs (in this case .git/logs/HEAD). The reflog for
>> refs/heads/main appears to be updated correctly.
>
> Thanks for an easy reproduction recipe. Looks like it bisects to
> 297c09eabb (refs: allow multiple reflog entries for the same refname,
> 2024-12-16). Author cc'd.
>
> Just looking at the diff, the early return from lock_ref_for_update()
> seems suspicious to me. Doing this:
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 8953d1c6d3..1c0e24a855 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2611,9 +2611,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) {
>  			/*
>
> makes the problem go away, and doesn't fail any tests. But that is just
> me poking at it without understanding why those lines were there in the
> first place.
>

The reason for that addition was that there was assumed the flow of `lock_ref_for_update()` for reflog only updates was to capture the lock only [1]. But this is wrong since this misses the old_oid population. As such your fix should do the trick. Thanks!

[1]: https://lore.kernel.org/git/CAOLa=ZQqQTEquJ0e5rG168-CVADR8K-uYma7Z8yiDCptyPgBQg@mail.gmail.com/
> -Peff
Karthik Nayak· Jan 22, 2025, 10:03 UTC · re: Jeff King · lore

[PATCH] 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 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.

Reported-by: Nika Layzell <nika@thelayzells.com>
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Hello,

This patch is based on top of 'maint' so that it can be easily backported. Sorry for the inconvenience here. This was a premature optimization which wasn't needed, and unfortunately this wasn't captured by any test.

Karthik
---
 refs/files-backend.c  |  3 ---
 t/t1400-update-ref.sh | 16 ++++++++++++++++
 2 files changed, 16 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..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
 
@@ -2068,4 +2070,18 @@ 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_done

-- 
2.47.0
Patrick Steinhardt· Jan 22, 2025, 12:04 UTC · re: Karthik Nayak · lore

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

On Wed, Jan 22, 2025 at 11:03:19AM +0100, Karthik Nayak wrote:
Show 5 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
s/reflog only/reflog-only
Show 9 quoted lines
> 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]
Show 14 quoted lines
> 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.

Show 13 quoted lines
> 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
Junio C Hamano· Jan 22, 2025, 17:56 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 13 quoted lines
>> 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.
Show 9 quoted lines
>>  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.
Karthik Nayak· Jan 23, 2025, 10:21 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 9 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
>
> s/reflog only/reflog-only
>
Will change.
Show 14 quoted lines
>> 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.
Show 24 quoted lines
>
>> 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.
Show 7 quoted lines
> 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.

Show 17 quoted lines
>> 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
Jeff King· Jan 22, 2025, 15:02 UTC · re: Karthik Nayak · lore

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

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
Karthik Nayak· Jan 23, 2025, 11:08 UTC · re: Jeff King · lore

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

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
Jeff King· Jan 23, 2025, 14:34 UTC · re: Karthik Nayak · lore

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

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
Karthik Nayak· Jan 23, 2025, 11:29 UTC · re: Karthik Nayak · lore

[PATCH v2] 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 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
Junio C Hamano· Jan 23, 2025, 17:55 UTC · re: Karthik Nayak · lore

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

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")?

Show 5 quoted lines
> 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"?
Show 5 quoted lines
> 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>
Show 16 quoted lines
> 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.
Karthik Nayak· Jan 24, 2025, 10:38 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> 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.
Show 12 quoted lines
> 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.
Show 5 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
>
> 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.
Show 8 quoted lines
>> 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!
Show 19 quoted lines
>> 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.
Show 28 quoted lines
>> 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

← back to recent threads