{"thread":{"id":"62839","subject":"`git update-ref` fails to set reflog old_oid in 2.48","startedAt":"2025-01-21T20:40:17Z","lastAt":"2025-01-24T10:38:11Z","messageCount":13,"participants":["Nika Layzell","Jeff King","Karthik Nayak","Patrick Steinhardt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"511019","messageId":"CACwGqKixQEGau8CZuLwJx02F4h8hxrf9e_7N1tHMU=Wvy8ViyQ@mail.gmail.com","threadId":"62839","inReplyTo":null,"subject":"`git update-ref` fails to set reflog old_oid in 2.48","fromName":"Nika Layzell","fromEmail":"nika@thelayzells.com","sentAt":"2025-01-21T20:40:06Z","receivedAt":"2025-01-21T20:40:17Z","isPatch":false,"sender":{"key":"nika@thelayzells.com","avatar":"https://avatars.githubusercontent.com/u/1261662?v=4"},"body":"In git 2.48.1, the `git update-ref` subcommand no longer correctly\nupdates the reflog in some cases. Specifically, it appears that the\n`old_oid` field will not be updated when modifying a branch referenced\nby another symbolic ref (e.g. HEAD). This doesn't break the `git\nreflog` subcommand, but does break references like `HEAD@{1}`, which\nappear to read the `old_oid` field.\n\nSTR (in a fresh directory):\n```\ngit init -b main\ngit commit --allow-empty -m \"A\"\ngit commit --allow-empty -m \"B\"\ngit update-ref -m \"reason\" refs/heads/main HEAD~ HEAD\n```\n\nExpected Result:\n```\n$ git rev-parse HEAD@{1}\n70d9116663eee9f01065c3a6d8984b1dea661f20\n\n$ cat .git/logs/HEAD\n0000000000000000000000000000000000000000\n17dd31aaf89190a36b8d04136a1a0f83fb37da4c AUTHOR TIME commit\n(initial): A\n17dd31aaf89190a36b8d04136a1a0f83fb37da4c\n70d9116663eee9f01065c3a6d8984b1dea661f20 AUTHOR TIME commit:\nB\n70d9116663eee9f01065c3a6d8984b1dea661f20\n17dd31aaf89190a36b8d04136a1a0f83fb37da4c AUTHOR TIME reason\n```\n\nThe `old_oid` field for the reflog entry added by `git update-ref` is\npresent, and correctly initialized. This was the case prior to git\n2.48.\n\nActual Result:\n```\n$ git rev-parse HEAD@{1}\nwarning: log for ref HEAD unexpectedly ended on TIME\n17dd31aaf89190a36b8d04136a1a0f83fb37da4c\n\n$ cat .git/logs/HEAD\n0000000000000000000000000000000000000000\n17dd31aaf89190a36b8d04136a1a0f83fb37da4c AUTHOR TIME commit\n(initial): A\n17dd31aaf89190a36b8d04136a1a0f83fb37da4c\n70d9116663eee9f01065c3a6d8984b1dea661f20 AUTHOR TIME commit:\nB\n0000000000000000000000000000000000000000\n17dd31aaf89190a36b8d04136a1a0f83fb37da4c AUTHOR TIME reason\n```\n\nThe `old_oid` field is empty (all zeroes). This is only the case in\nderived reflogs (in this case .git/logs/HEAD). The reflog for\nrefs/heads/main appears to be updated correctly.\n"},{"id":"511025","messageId":"20250121215235.GA2753621@coredump.intra.peff.net","threadId":"62839","inReplyTo":"CACwGqKixQEGau8CZuLwJx02F4h8hxrf9e_7N1tHMU=Wvy8ViyQ@mail.gmail.com","subject":"Re: `git update-ref` fails to set reflog old_oid in 2.48","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-21T21:52:35Z","receivedAt":"2025-01-21T21:52:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 21, 2025 at 03:40:06PM -0500, Nika Layzell wrote:\n\n> In git 2.48.1, the `git update-ref` subcommand no longer correctly\n> updates the reflog in some cases. Specifically, it appears that the\n> `old_oid` field will not be updated when modifying a branch referenced\n> by another symbolic ref (e.g. HEAD). This doesn't break the `git\n> reflog` subcommand, but does break references like `HEAD@{1}`, which\n> appear to read the `old_oid` field.\n> \n> STR (in a fresh directory):\n> ```\n> git init -b main\n> git commit --allow-empty -m \"A\"\n> git commit --allow-empty -m \"B\"\n> git update-ref -m \"reason\" refs/heads/main HEAD~ HEAD\n> ```\n> [...]\n> The `old_oid` field is empty (all zeroes). This is only the case in\n> derived reflogs (in this case .git/logs/HEAD). The reflog for\n> refs/heads/main appears to be updated correctly.\n\nThanks for an easy reproduction recipe. Looks like it bisects to\n297c09eabb (refs: allow multiple reflog entries for the same refname,\n2024-12-16). Author cc'd.\n\nJust looking at the diff, the early return from lock_ref_for_update()\nseems suspicious to me. Doing this:\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 8953d1c6d3..1c0e24a855 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2611,9 +2611,6 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \n \tupdate->backend_data = lock;\n \n-\tif (update->flags & REF_LOG_ONLY)\n-\t\tgoto out;\n-\n \tif (update->type & REF_ISSYMREF) {\n \t\tif (update->flags & REF_NO_DEREF) {\n \t\t\t/*\n\nmakes the problem go away, and doesn't fail any tests. But that is just\nme poking at it without understanding why those lines were there in the\nfirst place.\n\n-Peff\n"},{"id":"511040","messageId":"CAOLa=ZQt8GFPpqnzVLNw0EfPOWDi8hZC0QOpNytRwDxYYDW0Ng@mail.gmail.com","threadId":"62839","inReplyTo":"20250121215235.GA2753621@coredump.intra.peff.net","subject":"Re: `git update-ref` fails to set reflog old_oid in 2.48","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-22T06:47:43Z","receivedAt":"2025-01-22T06:47:46Z","isPatch":false,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Jeff King writes:\n>\n> On Tue, Jan 21, 2025 at 03:40:06PM -0500, Nika Layzell wrote:\n>\n>> In git 2.48.1, the `git update-ref` subcommand no longer correctly\n>> updates the reflog in some cases. Specifically, it appears that the\n>> `old_oid` field will not be updated when modifying a branch referenced\n>> by another symbolic ref (e.g. HEAD). This doesn't break the `git\n>> reflog` subcommand, but does break references like `HEAD@{1}`, which\n>> appear to read the `old_oid` field.\n>>\n>> STR (in a fresh directory):\n>> ```\n>> git init -b main\n>> git commit --allow-empty -m \"A\"\n>> git commit --allow-empty -m \"B\"\n>> git update-ref -m \"reason\" refs/heads/main HEAD~ HEAD\n>> ```\n>> [...]\n>> The `old_oid` field is empty (all zeroes). This is only the case in\n>> derived reflogs (in this case .git/logs/HEAD). The reflog for\n>> refs/heads/main appears to be updated correctly.\n>\n> Thanks for an easy reproduction recipe. Looks like it bisects to\n> 297c09eabb (refs: allow multiple reflog entries for the same refname,\n> 2024-12-16). Author cc'd.\n>\n> Just looking at the diff, the early return from lock_ref_for_update()\n> seems suspicious to me. Doing this:\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 8953d1c6d3..1c0e24a855 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2611,9 +2611,6 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n>\n> \tupdate->backend_data = lock;\n>\n> -\tif (update->flags & REF_LOG_ONLY)\n> -\t\tgoto out;\n> -\n> \tif (update->type & REF_ISSYMREF) {\n> \t\tif (update->flags & REF_NO_DEREF) {\n> \t\t\t/*\n>\n> makes the problem go away, and doesn't fail any tests. But that is just\n> me poking at it without understanding why those lines were there in the\n> first place.\n>\n\nThe reason for that addition was that there was assumed the flow of\n`lock_ref_for_update()` for reflog only updates was to capture the lock\nonly [1]. But this is wrong since this misses the old_oid population. As\nsuch your fix should do the trick. Thanks!\n\n[1]: https://lore.kernel.org/git/CAOLa=ZQqQTEquJ0e5rG168-CVADR8K-uYma7Z8yiDCptyPgBQg@mail.gmail.com/\n\n> -Peff\n"},{"id":"511047","messageId":"20250122100319.2280647-1-karthik.188@gmail.com","threadId":"62839","inReplyTo":"20250121215235.GA2753621@coredump.intra.peff.net","subject":"[PATCH] refs: fix creation of corrupted reflogs for symrefs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-22T10:03:19Z","receivedAt":"2025-01-22T10:03:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 297c09eabb (refs: allow multiple reflog entries for the same\nrefname, 2024-12-16) added logic for reflogs to exit early in\n`lock_ref_for_update()` after obtaining the required lock. This was\nadded as a performance optimization as it was assumed that no further\nprocessing was required for reflog only updates. However this was\nincorrect since for a symref's reflog entry, the update needs to be\npopulated with the old_oid value. This is done right after the early\nexit.\n\nThis caused a bug in Git 2.48 where target references of symrefs being\nupdated would create a corrupted reflog entry for the symref since the\nold_oid is not populated. Undo the skip in logic to fix this issue and\nalso add a test to ensure that such an issue doesn't arise in the\nfuture.\n\nThe early exit was added as a performance optimization for reflog-only\nupdates, but this accidentally broke symref reflog handling. Remove the\noptimization since it wasn't essential to the original changes.\n\nReported-by: Nika Layzell \nCo-authored-by: Jeff King \nSigned-off-by: Karthik Nayak \n---\n\nHello,\n\nThis patch is based on top of 'maint' so that it can be easily backported.\nSorry for the inconvenience here. This was a premature optimization which\nwasn't needed, and unfortunately this wasn't captured by any test.\n\nKarthik\n\n---\n refs/files-backend.c | 3 ---\n t/t1400-update-ref.sh | 16 ++++++++++++++++\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5cfb8b7ca8..29f08dced4 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2615,9 +2615,6 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \n \tupdate->backend_data = lock;\n \n-\tif (update->flags & REF_LOG_ONLY)\n-\t\tgoto out;\n-\n \tif (update->type & REF_ISSYMREF) {\n \t\tif (update->flags & REF_NO_DEREF) {\n \t\t\t/*\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex e2316f1dd4..59493dd73f 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -4,6 +4,8 @@\n #\n \n test_description='Test git update-ref and basic ref logging'\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n@@ -2068,4 +2070,18 @@ do\n \n done\n \n+test_expect_success 'update-ref should also create reflog for HEAD' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\tgit rev-parse HEAD >>expect &&\n+\t\tgit update-ref --create-reflog refs/heads/main HEAD~ &&\n+\t\tgit rev-parse HEAD@{1} >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.47.0\n\n"},{"id":"511056","messageId":"Z5De17-Ed3K31kzh@pks.im","threadId":"62839","inReplyTo":"20250122100319.2280647-1-karthik.188@gmail.com","subject":"Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-22T12:04:39Z","receivedAt":"2025-01-22T12:04:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 22, 2025 at 11:03:19AM +0100, Karthik Nayak wrote:\n> The commit 297c09eabb (refs: allow multiple reflog entries for the same\n> refname, 2024-12-16) added logic for reflogs to exit early in\n> `lock_ref_for_update()` after obtaining the required lock. This was\n> added as a performance optimization as it was assumed that no further\n> processing was required for reflog only updates. However this was\n\ns/reflog only/reflog-only\n\n> incorrect since for a symref's reflog entry, the update needs to be\n> populated with the old_oid value. This is done right after the early\n> exit.\n> \n> This caused a bug in Git 2.48 where target references of symrefs being\n> updated would create a corrupted reflog entry for the symref since the\n> old_oid is not populated. Undo the skip in logic to fix this issue and\n> also add a test to ensure that such an issue doesn't arise in the\n> future.\n\nIt's a bit curious that you describe the fix here, then in the next\nparagraph describe why we have skipped the logic only to reiterate the\nfix.\n\n> The early exit was added as a performance optimization for reflog-only\n> updates, but this accidentally broke symref reflog handling. Remove the\n> optimization since it wasn't essential to the original changes.\n\n[snip]\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 5cfb8b7ca8..29f08dced4 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2615,9 +2615,6 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n> \n> \tupdate->backend_data = lock;\n> \n> -\tif (update->flags & REF_LOG_ONLY)\n> -\t\tgoto out;\n> -\n> \tif (update->type & REF_ISSYMREF) {\n> \t\tif (update->flags & REF_NO_DEREF) {\n> \t\t\t/*\n\nOkay, makes sense. The error is specific to the \"files\" backend, which\nmight be worth mentioning in the commit message.\n\nOne thing that made me a bit curious is that we now end up executing\n`check_old_oid()` for symref reflog entries, because we have\n`REF_ISSYMREF` and `REF_NO_DEREF` set. But that function should end up\nskipping the check because we explicitly unset `REF_HAVE_OLD` when\nqueueing the update. The remainder should be skipped because we have\n`REF_LOG_ONLY` set.\n\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index e2316f1dd4..59493dd73f 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -4,6 +4,8 @@\n> #\n> \n> test_description='Test git update-ref and basic ref logging'\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> \n> . ./test-lib.sh\n> \n\nWe could use `git symbolic-ref HEAD` to resolve the branch name instead\nof overriding the branch name here.\n\nPatrick\n"},{"id":"511071","messageId":"20250122150222.GA3348514@coredump.intra.peff.net","threadId":"62839","inReplyTo":"20250122100319.2280647-1-karthik.188@gmail.com","subject":"Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-22T15:02:22Z","receivedAt":"2025-01-22T15:02:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 22, 2025 at 11:03:19AM +0100, Karthik Nayak wrote:\n\n> The commit 297c09eabb (refs: allow multiple reflog entries for the same\n> refname, 2024-12-16) added logic for reflogs to exit early in\n> `lock_ref_for_update()` after obtaining the required lock. This was\n> added as a performance optimization as it was assumed that no further\n> processing was required for reflog only updates. However this was\n> incorrect since for a symref's reflog entry, the update needs to be\n> populated with the old_oid value. This is done right after the early\n> exit.\n> \n> This caused a bug in Git 2.48 where target references of symrefs being\n> updated would create a corrupted reflog entry for the symref since the\n> old_oid is not populated. Undo the skip in logic to fix this issue and\n> also add a test to ensure that such an issue doesn't arise in the\n> future.\n> \n> The early exit was added as a performance optimization for reflog-only\n> updates, but this accidentally broke symref reflog handling. Remove the\n> optimization since it wasn't essential to the original changes.\n\nThanks for the explanation.\n\n> Reported-by: Nika Layzell \n> Co-authored-by: Jeff King \n> Signed-off-by: Karthik Nayak \n\nI don't know if we need my s-o-b to delete a few lines of code, but just\nin case:\n\n Signed-off-by: Jeff King \n\n> +test_expect_success 'update-ref should also create reflog for HEAD' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit A &&\n> +\t\ttest_commit B &&\n> +\t\tgit rev-parse HEAD >>expect &&\n\nUsing \">>\" here is unexpected. It's OK because we are in a new repo (so\nthere is no leftover \"expect\" file from a previous test) but probably\nbetter to stick to \">\" unless we really need to append.\n\nPlus I don't think there is really any need for a new repo. The\nimportant thing is just updating the branch via update-ref (it doesn't\neven have to be a rewind, but of course it has to exist already, so a\nrewind is the simplest thing).\n\n> +\t\tgit update-ref --create-reflog refs/heads/main HEAD~ &&\n\nI agree with Patrick that we are probably better off just getting the\nbranch name with symbolic-ref.\n\nSo all together, something like:\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex e2316f1dd4..29045aad43 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -2068,4 +2068,13 @@ do\n \n done\n \n+test_expect_success 'update-ref should also create reflog for HEAD' '\n+\ttest_commit to-rewind &&\n+\tgit rev-parse HEAD >expect &&\n+\thead=$(git symbolic-ref HEAD) &&\n+\tgit update-ref --create-reflog \"$head\" HEAD~ &&\n+\tgit rev-parse HEAD@{1} >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\n-Peff\n"},{"id":"511079","messageId":"xmqqsepawx4o.fsf@gitster.g","threadId":"62839","inReplyTo":"Z5De17-Ed3K31kzh@pks.im","subject":"Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-22T17:56:23Z","receivedAt":"2025-01-22T17:56:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> This caused a bug in Git 2.48 where target references of symrefs being\n>> updated would create a corrupted reflog entry for the symref since the\n>> old_oid is not populated. Undo the skip in logic to fix this issue and\n>> also add a test to ensure that such an issue doesn't arise in the\n>> future.\n>\n> It's a bit curious that you describe the fix here, then in the next\n> paragraph describe why we have skipped the logic only to reiterate the\n> fix.\n>\n>> The early exit was added as a performance optimization for reflog-only\n>> updates, but this accidentally broke symref reflog handling. Remove the\n>> optimization since it wasn't essential to the original changes.\n\nYeah, that indeed is a \"bit\" curious. I'd call it confusing, though\n;-).\n\n> Okay, makes sense. The error is specific to the \"files\" backend, which\n> might be worth mentioning in the commit message.\n\nIndeed.\n\n>> test_description='Test git update-ref and basic ref logging'\n>> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n>> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>> \n>> . ./test-lib.sh\n>> \n>\n> We could use `git symbolic-ref HEAD` to resolve the branch name instead\n> of overriding the branch name here.\n\nI agree. That sounds like a more sensible way to go.\n\nThanks.\n"},{"id":"511106","messageId":"CAOLa=ZT3ZHAC1mDQj=tezzdnJ_m413Oez9v20oZ4t5Rh3M5k1w@mail.gmail.com","threadId":"62839","inReplyTo":"Z5De17-Ed3K31kzh@pks.im","subject":"Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-23T10:21:27Z","receivedAt":"2025-01-23T10:21:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Wed, Jan 22, 2025 at 11:03:19AM +0100, Karthik Nayak wrote:\n>> The commit 297c09eabb (refs: allow multiple reflog entries for the same\n>> refname, 2024-12-16) added logic for reflogs to exit early in\n>> `lock_ref_for_update()` after obtaining the required lock. This was\n>> added as a performance optimization as it was assumed that no further\n>> processing was required for reflog only updates. However this was\n>\n> s/reflog only/reflog-only\n>\n\nWill change.\n\n>> incorrect since for a symref's reflog entry, the update needs to be\n>> populated with the old_oid value. This is done right after the early\n>> exit.\n>>\n>> This caused a bug in Git 2.48 where target references of symrefs being\n>> updated would create a corrupted reflog entry for the symref since the\n>> old_oid is not populated. Undo the skip in logic to fix this issue and\n>> also add a test to ensure that such an issue doesn't arise in the\n>> future.\n>\n> It's a bit curious that you describe the fix here, then in the next\n> paragraph describe why we have skipped the logic only to reiterate the\n> fix.\n>\n\nLet me rephrase that to make it a little clearer.\n\n>\n>> The early exit was added as a performance optimization for reflog-only\n>> updates, but this accidentally broke symref reflog handling. Remove the\n>> optimization since it wasn't essential to the original changes.\n>\n> [snip]\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index 5cfb8b7ca8..29f08dced4 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -2615,9 +2615,6 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n>>\n>> \tupdate->backend_data = lock;\n>>\n>> -\tif (update->flags & REF_LOG_ONLY)\n>> -\t\tgoto out;\n>> -\n>> \tif (update->type & REF_ISSYMREF) {\n>> \t\tif (update->flags & REF_NO_DEREF) {\n>> \t\t\t/*\n>\n> Okay, makes sense. The error is specific to the \"files\" backend, which\n> might be worth mentioning in the commit message.\n>\n\nIndeed, will add to the commit message.\n\n> One thing that made me a bit curious is that we now end up executing\n> `check_old_oid()` for symref reflog entries, because we have\n> `REF_ISSYMREF` and `REF_NO_DEREF` set. But that function should end up\n> skipping the check because we explicitly unset `REF_HAVE_OLD` when\n> queueing the update. The remainder should be skipped because we have\n> `REF_LOG_ONLY` set.\n>\n\nCorrect, the part which is crucial and was skipped was the call to\n`refs_resolve_ref_unsafe()` right after the block in discussion.\n\n>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n>> index e2316f1dd4..59493dd73f 100755\n>> --- a/t/t1400-update-ref.sh\n>> +++ b/t/t1400-update-ref.sh\n>> @@ -4,6 +4,8 @@\n>> #\n>>\n>> test_description='Test git update-ref and basic ref logging'\n>> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n>> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>>\n>> . ./test-lib.sh\n>>\n>\n> We could use `git symbolic-ref HEAD` to resolve the branch name instead\n> of overriding the branch name here.\n>\n\nYes, will make that change.\n\n> Patrick\n"},{"id":"511107","messageId":"CAOLa=ZQMhhznFj=QdTgq=ydiPVzFO7YsEzk4VZ7fwn-ZZ=W=bQ@mail.gmail.com","threadId":"62839","inReplyTo":"20250122150222.GA3348514@coredump.intra.peff.net","subject":"Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-23T11:08:30Z","receivedAt":"2025-01-23T11:08:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jan 22, 2025 at 11:03:19AM +0100, Karthik Nayak wrote:\n>\n>> The commit 297c09eabb (refs: allow multiple reflog entries for the same\n>> refname, 2024-12-16) added logic for reflogs to exit early in\n>> `lock_ref_for_update()` after obtaining the required lock. This was\n>> added as a performance optimization as it was assumed that no further\n>> processing was required for reflog only updates. However this was\n>> incorrect since for a symref's reflog entry, the update needs to be\n>> populated with the old_oid value. This is done right after the early\n>> exit.\n>>\n>> This caused a bug in Git 2.48 where target references of symrefs being\n>> updated would create a corrupted reflog entry for the symref since the\n>> old_oid is not populated. Undo the skip in logic to fix this issue and\n>> also add a test to ensure that such an issue doesn't arise in the\n>> future.\n>>\n>> The early exit was added as a performance optimization for reflog-only\n>> updates, but this accidentally broke symref reflog handling. Remove the\n>> optimization since it wasn't essential to the original changes.\n>\n> Thanks for the explanation.\n>\n>> Reported-by: Nika Layzell \n>> Co-authored-by: Jeff King \n>> Signed-off-by: Karthik Nayak \n>\n> I don't know if we need my s-o-b to delete a few lines of code, but just\n> in case:\n>\n> Signed-off-by: Jeff King \n>\n\nI was looking at the docs and it doesn't specify if I should apply this\nmyself. I will go ahead and apply it for the next version, but it is\nworthwhile to add to our documentation.\n\n>> +test_expect_success 'update-ref should also create reflog for HEAD' '\n>> +\ttest_when_finished \"rm -rf repo\" &&\n>> +\tgit init repo &&\n>> +\t(\n>> +\t\tcd repo &&\n>> +\t\ttest_commit A &&\n>> +\t\ttest_commit B &&\n>> +\t\tgit rev-parse HEAD >>expect &&\n>\n> Using \">>\" here is unexpected. It's OK because we are in a new repo (so\n> there is no leftover \"expect\" file from a previous test) but probably\n> better to stick to \">\" unless we really need to append.\n>\n\nYeah, I agree, this should be `>`\n\n> Plus I don't think there is really any need for a new repo. The\n> important thing is just updating the branch via update-ref (it doesn't\n> even have to be a rewind, but of course it has to exist already, so a\n> rewind is the simplest thing).\n>\n\nI agree there is no need for new repo, but the way our tests are written\nthere is often a lot of context leak between them. This isolates that\nbehavior.\n\n>> +\t\tgit update-ref --create-reflog refs/heads/main HEAD~ &&\n>\n> I agree with Patrick that we are probably better off just getting the\n> branch name with symbolic-ref.\n>\n> So all together, something like:\n>\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index e2316f1dd4..29045aad43 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -2068,4 +2068,13 @@ do\n>\n> done\n>\n> +test_expect_success 'update-ref should also create reflog for HEAD' '\n> +\ttest_commit to-rewind &&\n> +\tgit rev-parse HEAD >expect &&\n> +\thead=$(git symbolic-ref HEAD) &&\n> +\tgit update-ref --create-reflog \"$head\" HEAD~ &&\n> +\tgit rev-parse HEAD@{1} >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n>\n\nThanks Peff! I'll add it in! It's now completely your patch :)\n\n> -Peff\n"},{"id":"511108","messageId":"20250123112944.3922712-1-karthik.188@gmail.com","threadId":"62839","inReplyTo":"20250122100319.2280647-1-karthik.188@gmail.com","subject":"[PATCH v2] refs: fix creation of corrupted reflogs for symrefs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-23T11:29:44Z","receivedAt":"2025-01-23T11:29:53Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 297c09eabb (refs: allow multiple reflog entries for the same\nrefname, 2024-12-16) added logic for reflogs to exit early in\n`lock_ref_for_update()` after obtaining the required lock. This was\nadded as a performance optimization as it was assumed that no further\nprocessing was required for reflog-only updates. However this was\nincorrect since for a symref's reflog entry, the update needs to be\npopulated with the old_oid value. This is done right after the early\nexit.\n\nThis caused a bug in Git 2.48 in the files backend where target\nreferences of symrefs being updated would create a corrupted reflog\nentry for the symref since the old_oid is not populated. Undo the skip\nin logic to fix this issue and also add a test to ensure that such an\nissue doesn't arise in the future.\n\nThe early exit was added as a performance optimization for reflog-only\nupdates, and it wasn't essential to the original changes. As such,\nreverting it shouldn't cause any further issues.\n\nReported-by: Nika Layzell \nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Karthik Nayak \n---\n\nChanges since the v1:\n- Modified the commit message and subject to make it a little more clearer.\n- Used `git symbolic-ref HEAD` in the test instead of setting\n the default branch.\n\nRange diff:\n\n1: dbb1186512 ! 1: 9326fb08a9 reflog: fix bug which didn't resolve symref reflogs\n @@ Metadata\n Author: Karthik Nayak \n \n ## Commit message ##\n - reflog: fix bug which didn't resolve symref reflogs\n + refs: fix creation of corrupted reflogs for symrefs\n \n The commit 297c09eabb (refs: allow multiple reflog entries for the same\n - refname, 2024-12-16) added logic to skip the flow for reflogs in\n - `lock_ref_for_update()` after obtaining the required lock. This was done\n - because it was assumed that the flow ends there for reflogs. However\n - this was incorrect since for a symref's reflog entry, we need to\n - populate the old_oid value which is done right after.\n + refname, 2024-12-16) added logic for reflogs to exit early in\n + `lock_ref_for_update()` after obtaining the required lock. This was\n + added as a performance optimization as it was assumed that no further\n + processing was required for reflog-only updates. However this was\n + incorrect since for a symref's reflog entry, the update needs to be\n + populated with the old_oid value. This is done right after the early\n + exit.\n \n - This caused a bug in Git 2.48 where target references of symrefs being\n - updated would create a corrupted reflog entry for the symref since the\n - old_oid is not populated. Undo the skip in logic to fix this issue and\n - also add a test to ensure that such an issue doesn't arise in the\n - future.\n + This caused a bug in Git 2.48 in the files backend where target\n + references of symrefs being updated would create a corrupted reflog\n + entry for the symref since the old_oid is not populated. Undo the skip\n + in logic to fix this issue and also add a test to ensure that such an\n + issue doesn't arise in the future.\n +\n + The early exit was added as a performance optimization for reflog-only\n + updates, and it wasn't essential to the original changes. As such,\n + reverting it shouldn't cause any further issues.\n \n Reported-by: Nika Layzell \n Co-authored-by: Jeff King \n + Signed-off-by: Jeff King \n Signed-off-by: Karthik Nayak \n \n ## refs/files-backend.c ##\n @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref\n \t\t\t/*\n \n ## t/t1400-update-ref.sh ##\n -@@\n - #\n - \n - test_description='Test git update-ref and basic ref logging'\n -+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n -+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n - \n - . ./test-lib.sh\n - \n @@ t/t1400-update-ref.sh: do\n \n done\n \n +test_expect_success 'update-ref should also create reflog for HEAD' '\n -+\ttest_when_finished \"rm -rf repo\" &&\n -+\tgit init repo &&\n -+\t(\n -+\t\tcd repo &&\n -+\t\ttest_commit A &&\n -+\t\ttest_commit B &&\n -+\t\tgit rev-parse HEAD >>expect &&\n -+\t\tgit update-ref --create-reflog refs/heads/main HEAD~ &&\n -+\t\tgit rev-parse HEAD@{1} >actual &&\n -+\t\ttest_cmp expect actual\n -+\t)\n ++\ttest_commit to-rewind &&\n ++\tgit rev-parse HEAD >expect &&\n ++\thead=$(git symbolic-ref HEAD) &&\n ++\tgit update-ref --create-reflog \"$head\" HEAD~ &&\n ++\tgit rev-parse HEAD@{1} >actual &&\n ++\ttest_cmp expect actual\n +'\n +\n test_done\n\n---\n refs/files-backend.c | 3 ---\n t/t1400-update-ref.sh | 9 +++++++++\n 2 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5cfb8b7ca8..29f08dced4 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2615,9 +2615,6 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \n \tupdate->backend_data = lock;\n \n-\tif (update->flags & REF_LOG_ONLY)\n-\t\tgoto out;\n-\n \tif (update->type & REF_ISSYMREF) {\n \t\tif (update->flags & REF_NO_DEREF) {\n \t\t\t/*\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex e2316f1dd4..29045aad43 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -2068,4 +2068,13 @@ do\n \n done\n \n+test_expect_success 'update-ref should also create reflog for HEAD' '\n+\ttest_commit to-rewind &&\n+\tgit rev-parse HEAD >expect &&\n+\thead=$(git symbolic-ref HEAD) &&\n+\tgit update-ref --create-reflog \"$head\" HEAD~ &&\n+\tgit rev-parse HEAD@{1} >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.47.0\n\n"},{"id":"511110","messageId":"20250123143412.GA3916082@coredump.intra.peff.net","threadId":"62839","inReplyTo":"CAOLa=ZQMhhznFj=QdTgq=ydiPVzFO7YsEzk4VZ7fwn-ZZ=W=bQ@mail.gmail.com","subject":"Re: [PATCH] refs: fix creation of corrupted reflogs for symrefs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-23T14:34:12Z","receivedAt":"2025-01-23T14:34:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 23, 2025 at 03:08:30AM -0800, Karthik Nayak wrote:\n\n> Thanks Peff! I'll add it in! It's now completely your patch :)\n\n:) IMHO the hard part was understanding the implications of the removed\ncode block and writing the commit message. Any fool can delete a few\nlines. ;)\n\n-Peff\n"},{"id":"511125","messageId":"xmqqv7u5s9cq.fsf@gitster.g","threadId":"62839","inReplyTo":"20250123112944.3922712-1-karthik.188@gmail.com","subject":"Re: [PATCH v2] refs: fix creation of corrupted reflogs for symrefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-23T17:55:49Z","receivedAt":"2025-01-23T17:55:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Subject: Re: [PATCH v2] refs: fix creation of corrupted reflogs for symrefs\n\nThis may be just me, but every time I see the above title, it read\nto me as if we are on purpose doing \"creation of corrupted reflogs\nfor symrefs\", but we are failing to do so for some reason, and this\ncommit is about improving the situation so that we can correctly\ncreate corrupted reflog entries for symbolic ref updates.\n\nAnd because the only sensible reason why we may on purpose do\n\"creation of corrupted reflogs\" I can think of is perhaps we prepare\nsuch corrupted thing to test how robust the production code is when\nseeing such corrupted data, I would expect to see a change to t/\nhierarchy.\n\nBut the patch touches the code, not just tests, which makes me\ndoubly puzzled.\n\nIt happens every time I see this title and the change. Perhaps drop\n\"corrupted\" from the title?\n\n> The commit 297c09eabb (refs: allow multiple reflog entries for the same\n> refname, 2024-12-16) added logic for reflogs to exit early in\n> `lock_ref_for_update()` after obtaining the required lock. This was\n\nI do not think the actor, who \"exits early\", is not \"reflogs\".\nShould we have \"for reflogs\" in the above, or perhaps move it to the\nend of the sentence (i.e. the required lock gets obtained because we\nwant to do some operation \"for reflogs\")?\n\n> added as a performance optimization as it was assumed that no further\n> processing was required for reflog-only updates. However this was\n> incorrect since for a symref's reflog entry, the update needs to be\n> populated with the old_oid value. This is done right after the early\n> exit.\n\n\"The early exit skipped this required work\"?\n\n> This caused a bug in Git 2.48 in the files backend where target\n> references of symrefs being updated would create a corrupted reflog\n> entry for the symref since the old_oid is not populated. Undo the skip\n> in logic to fix this issue and also add a test to ensure that such an\n> issue doesn't arise in the future.\n\nOK.\n\n> The early exit was added as a performance optimization for reflog-only\n> updates, and it wasn't essential to the original changes. As such,\n> reverting it shouldn't cause any further issues.\n\nI am not sure if this is even worth saying, as you already said that\nthe early return was done incorrectly assuming that the remainder of\nthe function can be skipped as an optimization. What may help\nreaders is to state that all the rest of the code path that was\nskipped by a mistaken optimization is necessary and would not do\nanything unwanted.\n\n> Reported-by: Nika Layzell \n> Co-authored-by: Jeff King \n> Signed-off-by: Jeff King \n> Signed-off-by: Karthik Nayak \n\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index e2316f1dd4..29045aad43 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -2068,4 +2068,13 @@ do\n> \n> done\n> \n> +test_expect_success 'update-ref should also create reflog for HEAD' '\n> +\ttest_commit to-rewind &&\n> +\tgit rev-parse HEAD >expect &&\n> +\thead=$(git symbolic-ref HEAD) &&\n> +\tgit update-ref --create-reflog \"$head\" HEAD~ &&\n> +\tgit rev-parse HEAD@{1} >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nNice. We could rename \"head\" to something more meaningful (like\n\"current branch\") but I can live with the above version. It is much\nnicer than assuming on what branch we would be, which was what the\nprevious iteration did.\n\nThanks.\n"},{"id":"511154","messageId":"CAOLa=ZTPy_gMkG0gPeasATdudynDY2uATvHUS3dpiqdgqZtR0g@mail.gmail.com","threadId":"62839","inReplyTo":"xmqqv7u5s9cq.fsf@gitster.g","subject":"Re: [PATCH v2] refs: fix creation of corrupted reflogs for symrefs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-24T10:38:09Z","receivedAt":"2025-01-24T10:38:11Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> Subject: Re: [PATCH v2] refs: fix creation of corrupted reflogs for symrefs\n>\n> This may be just me, but every time I see the above title, it read\n> to me as if we are on purpose doing \"creation of corrupted reflogs\n> for symrefs\", but we are failing to do so for some reason, and this\n> commit is about improving the situation so that we can correctly\n> create corrupted reflog entries for symbolic ref updates.\n>\n\nOkay. I see what you mean.\n\n> And because the only sensible reason why we may on purpose do\n> \"creation of corrupted reflogs\" I can think of is perhaps we prepare\n> such corrupted thing to test how robust the production code is when\n> seeing such corrupted data, I would expect to see a change to t/\n> hierarchy.\n>\n> But the patch touches the code, not just tests, which makes me\n> doubly puzzled.\n>\n> It happens every time I see this title and the change. Perhaps drop\n> \"corrupted\" from the title?\n>\n\nYeah, that would make it much clearer.\n\n>> The commit 297c09eabb (refs: allow multiple reflog entries for the same\n>> refname, 2024-12-16) added logic for reflogs to exit early in\n>> `lock_ref_for_update()` after obtaining the required lock. This was\n>\n> I do not think the actor, who \"exits early\", is not \"reflogs\".\n\nTook me some time to understand this, but I get what you're talking\nabout. My sentence adds ambiguity on what we're exactly exiting early.\n\n> Should we have \"for reflogs\" in the above, or perhaps move it to the\n> end of the sentence (i.e. the required lock gets obtained because we\n> want to do some operation \"for reflogs\")?\n>\n\nYeah that would make it much clearer.\n\n>> added as a performance optimization as it was assumed that no further\n>> processing was required for reflog-only updates. However this was\n>> incorrect since for a symref's reflog entry, the update needs to be\n>> populated with the old_oid value. This is done right after the early\n>> exit.\n>\n> \"The early exit skipped this required work\"?\n>\n\nYeah, that works!\n\n>> This caused a bug in Git 2.48 in the files backend where target\n>> references of symrefs being updated would create a corrupted reflog\n>> entry for the symref since the old_oid is not populated. Undo the skip\n>> in logic to fix this issue and also add a test to ensure that such an\n>> issue doesn't arise in the future.\n>\n> OK.\n>\n>> The early exit was added as a performance optimization for reflog-only\n>> updates, and it wasn't essential to the original changes. As such,\n>> reverting it shouldn't cause any further issues.\n>\n> I am not sure if this is even worth saying, as you already said that\n> the early return was done incorrectly assuming that the remainder of\n> the function can be skipped as an optimization. What may help\n> readers is to state that all the rest of the code path that was\n> skipped by a mistaken optimization is necessary and would not do\n> anything unwanted.\n>\n\nThat was what I was trying to convey.\n\n>> Reported-by: Nika Layzell \n>> Co-authored-by: Jeff King \n>> Signed-off-by: Jeff King \n>> Signed-off-by: Karthik Nayak \n>\n>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n>> index e2316f1dd4..29045aad43 100755\n>> --- a/t/t1400-update-ref.sh\n>> +++ b/t/t1400-update-ref.sh\n>> @@ -2068,4 +2068,13 @@ do\n>>\n>> done\n>>\n>> +test_expect_success 'update-ref should also create reflog for HEAD' '\n>> +\ttest_commit to-rewind &&\n>> +\tgit rev-parse HEAD >expect &&\n>> +\thead=$(git symbolic-ref HEAD) &&\n>> +\tgit update-ref --create-reflog \"$head\" HEAD~ &&\n>> +\tgit rev-parse HEAD@{1} >actual &&\n>> +\ttest_cmp expect actual\n>> +'\n>\n> Nice. We could rename \"head\" to something more meaningful (like\n> \"current branch\") but I can live with the above version. It is much\n> nicer than assuming on what branch we would be, which was what the\n> previous iteration did.\n>\n> Thanks.\n\nI agree, this is much nicer indeed.\n\nAlso I just noticed that you have already amended the commit message\nand added it to `next`. Thanks for doing that. Happy to re-roll if\nneeded!\n\nKarthik\n"}]}