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

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

[Nika Layzell]

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.

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

[Jeff King]

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.

-Peff

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

Jeff King <peff@peff.net> writes:
>
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Karthik Nayak]

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

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

[Patrick Steinhardt]

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

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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

[Karthik Nayak]

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

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

[Karthik Nayak]

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

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

[Karthik Nayak]

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

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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

[Karthik Nayak]

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