{"thread":{"id":"62796","subject":"Bug in 2.48 with `git refs migrate`","startedAt":"2025-01-13T13:56:41Z","lastAt":"2025-01-27T16:24:15Z","messageCount":21,"participants":["brian m. carlson","Patrick Steinhardt","Karthik Nayak","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"510412","messageId":"Z4UbkcmJAU1MT-Rs@tapette.crustytoothpaste.net","threadId":"62796","inReplyTo":null,"subject":"Bug in 2.48 with `git refs migrate`","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-01-13T13:56:33Z","receivedAt":"2025-01-13T13:56:41Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Hi,\n\nI noticed that Git 2.48 has support for migrating refs when there are\nreflogs and, as promised at Git Merge, I decided to try it out.\nUnfortunately, I got an error:\n\n----\n% git refs migrate --ref-format=reftable --dry-run\nerror: reftable: transaction failure: corrupt reftable file\n----\n\nHere's a small reproduction case:\n\n----\n#!/bin/sh\n\nrm -fr test-repo\ngit init -b dev test-repo\ncd test-repo\n\n# start first block\ntouch foo.txt\ngit add foo.txt\ngit commit -m +\n\nhead=$(git rev-parse HEAD)\nseq 5000 | sed -Ee \"s!^(.*)\\$!create refs/heads/ref-\\1 $head!\" | git update-ref --stdin\n# end first block\n\n# start second block\necho abc >bar.txt\ngit add bar.txt\ngit commit -m +\nhead=$(git rev-parse HEAD)\nseq 3000 | sed -Ee \"s!^(.*)\\$!update refs/heads/ref-\\1 $head!\" | git update-ref --stdin\n# end second block\n\ngit refs migrate --ref-format=reftable\n----\n\nI can also reproduce this on the latest master.\n\nIf you remove the second block, it does not appear to reproduce. Some\ninvestigation led me to the conclusion that the difference is when\nmax_update_index is not 1, the header has the value 1 for it but the\ntrailer has the correct value, and so we flag the header and trailer as\nmismatching and therefore it gets marked as corrupt. I believe the\nreason things work when removing the second block is because that value\nremains 1, and so it works.\n\nI haven't done anything else to investigate here, for which I apologize,\nbut I just wanted to mention it while it was fresh on my mind.\n\nIn case this is helpful, I did see this when attempting to migrate two\nwork repositories with lots of reflogs and many refs (the smaller has\n2983 and the larger, 44832). I obviously cannot send you these\nrepositories or things in them, but I'm happy to test patches against\nthem.\n\nPlease let me know if I can provide more useful information.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"510423","messageId":"Z4U1H6pe0NL0cOBh@pks.im","threadId":"62796","inReplyTo":"Z4UbkcmJAU1MT-Rs@tapette.crustytoothpaste.net","subject":"Re: Bug in 2.48 with `git refs migrate`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-13T15:45:35Z","receivedAt":"2025-01-13T15:45:40Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 13, 2025 at 01:56:33PM +0000, brian m. carlson wrote:\n> If you remove the second block, it does not appear to reproduce. Some\n> investigation led me to the conclusion that the difference is when\n> max_update_index is not 1, the header has the value 1 for it but the\n> trailer has the correct value, and so we flag the header and trailer as\n> mismatching and therefore it gets marked as corrupt. I believe the\n> reason things work when removing the second block is because that value\n> remains 1, and so it works.\n\nHm. Makes me wonder whether this is caused by the newly added code to\nalso migrate reflog entries, as we'd play around with the update index\nonly when migrating those. Cc'ing Karthik so he can have a look.\n\n> I haven't done anything else to investigate here, for which I apologize,\n> but I just wanted to mention it while it was fresh on my mind.\n> \n> In case this is helpful, I did see this when attempting to migrate two\n> work repositories with lots of reflogs and many refs (the smaller has\n> 2983 and the larger, 44832). I obviously cannot send you these\n> repositories or things in them, but I'm happy to test patches against\n> them.\n> \n> Please let me know if I can provide more useful information.\n\nThanks for your report!\n\nPatrick\n"},{"id":"510567","messageId":"CAOLa=ZTL9n_DPhNr49XAd6bT838kc09oVx_AH7Pb4o8VK_xQ9w@mail.gmail.com","threadId":"62796","inReplyTo":"Z4UbkcmJAU1MT-Rs@tapette.crustytoothpaste.net","subject":"Re: Bug in 2.48 with `git refs migrate`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-15T11:54:51Z","receivedAt":"2025-01-15T11:54:53Z","isPatch":false,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\"brian m. carlson\" writes:\n\nHello brian,\n\n> Hi,\n>\n> I noticed that Git 2.48 has support for migrating refs when there are\n> reflogs and, as promised at Git Merge, I decided to try it out.\n> Unfortunately, I got an error:\n>\n\nThanks for trying it out and reporting.\n\n>\n> ----\n> % git refs migrate --ref-format=reftable --dry-run\n> error: reftable: transaction failure: corrupt reftable file\n> ----\n>\n> Here's a small reproduction case:\n>\n> ----\n> #!/bin/sh\n>\n> rm -fr test-repo\n> git init -b dev test-repo\n> cd test-repo\n>\n> # start first block\n> touch foo.txt\n> git add foo.txt\n> git commit -m +\n>\n> head=$(git rev-parse HEAD)\n> seq 5000 | sed -Ee \"s!^(.*)\\$!create refs/heads/ref-\\1 $head!\" | git update-ref --stdin\n> # end first block\n>\n> # start second block\n> echo abc >bar.txt\n> git add bar.txt\n> git commit -m +\n> head=$(git rev-parse HEAD)\n> seq 3000 | sed -Ee \"s!^(.*)\\$!update refs/heads/ref-\\1 $head!\" | git update-ref --stdin\n> # end second block\n>\n> git refs migrate --ref-format=reftable\n> ----\n>\n> I can also reproduce this on the latest master.\n>\n\nI can also reproduce it using the script provided here.\n\n>\n> If you remove the second block, it does not appear to reproduce. Some\n> investigation led me to the conclusion that the difference is when\n> max_update_index is not 1, the header has the value 1 for it but the\n> trailer has the correct value, and so we flag the header and trailer as\n> mismatching and therefore it gets marked as corrupt. I believe the\n> reason things work when removing the second block is because that value\n> remains 1, and so it works.\n>\n\nIndeed, you're absolutely correct here, this is because the\n`max_update_index` is indeed different between the footer and the\nheader. This flow is triggered when we have multiple reftable blocks\nbeing written with multiple reflog entries. The multiple blocks is\ncrucial because we update the `max_update_index` right before writing\nthe reflogs. So if there are refs being written and they spawn more than\none block, the header is written without the updated `max_update_index`,\ncausing the mismatch.\n\n> I haven't done anything else to investigate here, for which I apologize,\n> but I just wanted to mention it while it was fresh on my mind.\n>\n\nI think you've provided sufficient information and also enough to make\nit easy for me to debug.\n\n> In case this is helpful, I did see this when attempting to migrate two\n> work repositories with lots of reflogs and many refs (the smaller has\n> 2983 and the larger, 44832). I obviously cannot send you these\n> repositories or things in them, but I'm happy to test patches against\n> them.\n>\n\nI'm attaching a patch below which should fixes the issue for me and also\nadding a test to test against the same. I'd be grateful if you could\nalso test the patch against the repositoryies you mention.\n\n> Please let me know if I can provide more useful information.\n> --\n> brian m. carlson (they/them or he/him)\n> Toronto, Ontario, CA\n\n-- >8 --\n\nSubject: [PATCH] reftable: write correct max_update_index to header\n\nIn 297c09eabb (refs: allow multiple reflog entries for the same refname,\n2024-12-16), the reftable backend learned to handle multiple reflog\nentries within the same transaction. This was done modifying the\n`update_index` for reflogs with multiple indices. During writing the\nlogs, the `max_update_index` of the writer was modified to ensure the\nlimits were raised to the modified `update_index`s.\n\nHowever, since ref entries are written before the modification to the\n`max_update_index`, if there are multiple blocks to be written, the\nreftable backend writes the header with the old `max_update_index`. When\nall logs are finally written, the footer will be written with the new\n`min_update_index`. This causes a mismatch between the header and the\nfooter and causes the reftable file to be corrupted. The existing tests\nonly spawn a single block and since headers are lazily written with the\nfirst block, the tests didn't capture this bug.\n\nTo fix the issue, the appropriate `max_update_index` limit must be set\neven before the first block is written. Add a `max_index` field to the\ntransaction which holds the `max_index` within all its updates, then\npropagate this value to the reftable backend, wherein this is used to\nthe set the `max_update_index` correctly.\n\nAdd a test which creates a few thousand reference updates with multiple\nreflog entries, which should trigger the bug.\n\nReported-by: brian m. carlson \nSigned-off-by: Karthik Nayak \n---\n refs.c | 7 +++++++\n refs/refs-internal.h | 1 +\n refs/reftable-backend.c | 20 ++++++++++----------\n t/t1460-refs-migrate.sh | 12 ++++++++++++\n 4 files changed, 30 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0f41b2fd4a..f7b6f0f897 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1345,6 +1345,13 @@ int ref_transaction_update_reflog(struct\nref_transaction *transaction,\n \tupdate->flags &= ~REF_HAVE_OLD;\n \tupdate->index = index;\n\n+\t/*\n+\t * Reference backends may need to know the max index to optimize\n+\t * their writes. So we store the max_index on the transaction level.\n+\t */\n+\tif (index > transaction->max_index)\n+\t\ttransaction->max_index = index;\n+\n \treturn 0;\n }\n\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 16550862d3..aaab711bb9 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -203,6 +203,7 @@ struct ref_transaction {\n \tenum ref_transaction_state state;\n \tvoid *backend_data;\n \tunsigned int flags;\n+\tunsigned int max_index;\n };\n\n /*\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 00d95a9a2f..289496058e 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -942,6 +942,7 @@ struct write_transaction_table_arg {\n \tsize_t updates_nr;\n \tsize_t updates_alloc;\n \tsize_t updates_expected;\n+\tunsigned int max_index;\n };\n\n struct reftable_transaction_data {\n@@ -1428,7 +1429,6 @@ static int write_transaction_table(struct\nreftable_writer *writer, void *cb_data\n \tstruct reftable_log_record *logs = NULL;\n \tstruct ident_split committer_ident = {0};\n \tsize_t logs_nr = 0, logs_alloc = 0, i;\n-\tuint64_t max_update_index = ts;\n \tconst char *committer_info;\n \tint ret = 0;\n\n@@ -1438,7 +1438,12 @@ static int write_transaction_table(struct\nreftable_writer *writer, void *cb_data\n\n \tQSORT(arg->updates, arg->updates_nr, transaction_update_cmp);\n\n-\treftable_writer_set_limits(writer, ts, ts);\n+\t/*\n+\t * During reflog migration, we add indexes for a single reflog with\n+\t * multiple entries. Each entry will contain a different update_index,\n+\t * so set the limits accordingly.\n+\t */\n+\treftable_writer_set_limits(writer, ts, ts + arg->max_index);\n\n \tfor (i = 0; i < arg->updates_nr; i++) {\n \t\tstruct reftable_transaction_update *tx_update = &arg->updates[i];\n@@ -1540,12 +1545,6 @@ static int write_transaction_table(struct\nreftable_writer *writer, void *cb_data\n \t\t\t\t */\n \t\t\t\tlog->update_index = ts + u->index;\n\n-\t\t\t\t/*\n-\t\t\t\t * Note the max update_index so the limit can be set later on.\n-\t\t\t\t */\n-\t\t\t\tif (log->update_index > max_update_index)\n-\t\t\t\t\tmax_update_index = log->update_index;\n-\n \t\t\t\tlog->refname = xstrdup(u->refname);\n \t\t\t\tmemcpy(log->value.update.new_hash,\n \t\t\t\t u->new_oid.hash, GIT_MAX_RAWSZ);\n@@ -1609,8 +1608,6 @@ static int write_transaction_table(struct\nreftable_writer *writer, void *cb_data\n \t * and log blocks.\n \t */\n \tif (logs) {\n-\t\treftable_writer_set_limits(writer, ts, max_update_index);\n-\n \t\tret = reftable_writer_add_logs(writer, logs, logs_nr);\n \t\tif (ret < 0)\n \t\t\tgoto done;\n@@ -1631,6 +1628,9 @@ static int reftable_be_transaction_finish(struct\nref_store *ref_store UNUSED,\n \tstruct reftable_transaction_data *tx_data = transaction->backend_data;\n \tint ret = 0;\n\n+\tif (tx_data->args)\n+\t\ttx_data->args->max_index = transaction->max_index;\n+\n \tfor (size_t i = 0; i < tx_data->args_nr; i++) {\n \t\tret = reftable_addition_add(tx_data->args[i].addition,\n \t\t\t\t\t write_transaction_table, &tx_data->args[i]);\ndiff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh\nindex f59bc4860f..307b2998ef 100755\n--- a/t/t1460-refs-migrate.sh\n+++ b/t/t1460-refs-migrate.sh\n@@ -227,6 +227,18 @@ do\n \tdone\n done\n\n+test_expect_success 'multiple reftable blocks with multiple entries' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init --ref-format=files repo &&\n+\ttest_commit -C repo first &&\n+\tprintf \"create refs/heads/ref-%d HEAD\\n\" $(test_seq 5000) >stdin &&\n+\tgit -C repo update-ref --stdin stdin &&\n+\tgit -C repo update-ref --stdin writes:\n\n> Subject: [PATCH] reftable: write correct max_update_index to header\n>\n> In 297c09eabb (refs: allow multiple reflog entries for the same refname,\n> 2024-12-16), the reftable backend learned to handle multiple reflog\n> entries within the same transaction. This was done modifying the\n\n\"done\" -> \"done by\", I think.\n\n> To fix the issue, the appropriate `max_update_index` limit must be set\n> even before the first block is written. Add a `max_index` field to the\n> transaction which holds the `max_index` within all its updates, then\n> propagate this value to the reftable backend, wherein this is used to\n> the set the `max_update_index` correctly.\n\n\n> diff --git a/refs.c b/refs.c\n> index 0f41b2fd4a..f7b6f0f897 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1345,6 +1345,13 @@ int ref_transaction_update_reflog(struct\n\nNot the focus of this topic/fix, but I notice that the only caller\nof this ref_transaction_update_reflog() function is a static\nfunction migrate_one_reflog_entry() in the same file. Do we expect\nthat we would add more callers? Otherwise we should make it a file\nscope static and remove it from file.\n\n> ref_transaction *transaction,\n> \tupdate->flags &= ~REF_HAVE_OLD;\n> \tupdate->index = index;\n>\n> +\t/*\n> +\t * Reference backends may need to know the max index to optimize\n> +\t * their writes. So we store the max_index on the transaction level.\n> +\t */\n> +\tif (index > transaction->max_index)\n> +\t\ttransaction->max_index = index;\n> +\n> \treturn 0;\n> }\n\nSo from the problem description, whenever we consume an index by\nassigning it to an update that belongs to a transaction, we must\nmake sure that transaction's max_index covers that value of the\nindex? I was wondering if we should have a less error prone way to\ndo that by having a helper function that takes ref_update and\nref_transaction objects to do the above, but this is exclusively\nused by reflog migration code path and nowhere else, so it may\nprobably be fine as-is. I dunno.\n"},{"id":"510632","messageId":"Z4hpXwtv-ChwAGeG@tapette.crustytoothpaste.net","threadId":"62796","inReplyTo":"CAOLa=ZTL9n_DPhNr49XAd6bT838kc09oVx_AH7Pb4o8VK_xQ9w@mail.gmail.com","subject":"Re: Bug in 2.48 with `git refs migrate`","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-01-16T02:05:19Z","receivedAt":"2025-01-16T02:05:22Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-01-15 at 11:54:51, Karthik Nayak wrote:\n> \"brian m. carlson\" writes:\n> \n> I'm attaching a patch below which should fixes the issue for me and also\n> adding a test to test against the same. I'd be grateful if you could\n> also test the patch against the repositoryies you mention.\n\nFantastic, I'll try to do that tomorrow and get back to you. I really\nappreciate such a prompt response.\n\n> diff --git a/refs/refs-internal.h b/refs/refs-internal.h\n> index 16550862d3..aaab711bb9 100644\n> --- a/refs/refs-internal.h\n> +++ b/refs/refs-internal.h\n> @@ -203,6 +203,7 @@ struct ref_transaction {\n> \tenum ref_transaction_state state;\n> \tvoid *backend_data;\n> \tunsigned int flags;\n> +\tunsigned int max_index;\n> };\n> \n> /*\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 00d95a9a2f..289496058e 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -942,6 +942,7 @@ struct write_transaction_table_arg {\n> \tsize_t updates_nr;\n> \tsize_t updates_alloc;\n> \tsize_t updates_expected;\n> +\tunsigned int max_index;\n\nI wonder if this and the above should be `uint64_t` instead of `unsigned\nint`. From the file names and the data format, it looks like we\nintentionally use a 64-bit integer. That's good, because I have\nunfortunately seen some people who have created giant test repositories\nwith really unreasonable numbers of commits and I could see us possibly\nexceeding a 32-bit integer here.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"510635","messageId":"CAOLa=ZR6jbJwav0+3-A+3w=XRU02Lp_9Mm4RXbN6A00-N3Xq-g@mail.gmail.com","threadId":"62796","inReplyTo":"xmqqa5bsypht.fsf@gitster.g","subject":"Re: Bug in 2.48 with `git refs migrate`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-16T06:44:17Z","receivedAt":"2025-01-16T06:44:22Z","isPatch":false,"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: [PATCH] reftable: write correct max_update_index to header\n>>\n>> In 297c09eabb (refs: allow multiple reflog entries for the same refname,\n>> 2024-12-16), the reftable backend learned to handle multiple reflog\n>> entries within the same transaction. This was done modifying the\n>\n> \"done\" -> \"done by\", I think.\n>\n\nYup, thanks!\n\n>> To fix the issue, the appropriate `max_update_index` limit must be set\n>> even before the first block is written. Add a `max_index` field to the\n>> transaction which holds the `max_index` within all its updates, then\n>> propagate this value to the reftable backend, wherein this is used to\n>> the set the `max_update_index` correctly.\n>\n>\n>> diff --git a/refs.c b/refs.c\n>> index 0f41b2fd4a..f7b6f0f897 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -1345,6 +1345,13 @@ int ref_transaction_update_reflog(struct\n>\n> Not the focus of this topic/fix, but I notice that the only caller\n> of this ref_transaction_update_reflog() function is a static\n> function migrate_one_reflog_entry() in the same file. Do we expect\n> that we would add more callers? Otherwise we should make it a file\n> scope static and remove it from file.\n>\n\nI don't think we expect any other callers now. Maybe someday we'd expose\ncreating reflogs to the users. But for now, making it static is more\nworthwhile.\n\nLet's follow the boy scout rule and clean this up, I'll add a commit to\ndo the same in v2 of my patch.\n\n>> ref_transaction *transaction,\n>> \tupdate->flags &= ~REF_HAVE_OLD;\n>> \tupdate->index = index;\n>>\n>> +\t/*\n>> +\t * Reference backends may need to know the max index to optimize\n>> +\t * their writes. So we store the max_index on the transaction level.\n>> +\t */\n>> +\tif (index > transaction->max_index)\n>> +\t\ttransaction->max_index = index;\n>> +\n>> \treturn 0;\n>> }\n>\n> So from the problem description, whenever we consume an index by\n> assigning it to an update that belongs to a transaction, we must\n> make sure that transaction's max_index covers that value of the\n> index? I was wondering if we should have a less error prone way to\n> do that by having a helper function that takes ref_update and\n> ref_transaction objects to do the above, but this is exclusively\n> used by reflog migration code path and nowhere else, so it may\n> probably be fine as-is. I dunno.\n\nYup, that's correct. It should be okay, but let me do it anyways, makes\nit safer for the future.\n"},{"id":"510637","messageId":"CAOLa=ZS2ZQRSbirW1HLYJ1d1HAwmfugKdNGBCNGtS-yS1rcKEA@mail.gmail.com","threadId":"62796","inReplyTo":"Z4hpXwtv-ChwAGeG@tapette.crustytoothpaste.net","subject":"Re: Bug in 2.48 with `git refs migrate`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-16T07:29:07Z","receivedAt":"2025-01-16T07:29:09Z","isPatch":false,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On 2025-01-15 at 11:54:51, Karthik Nayak wrote:\n>> \"brian m. carlson\" writes:\n>>\n>> I'm attaching a patch below which should fixes the issue for me and also\n>> adding a test to test against the same. I'd be grateful if you could\n>> also test the patch against the repositoryies you mention.\n>\n> Fantastic, I'll try to do that tomorrow and get back to you. I really\n> appreciate such a prompt response.\n>\n>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h\n>> index 16550862d3..aaab711bb9 100644\n>> --- a/refs/refs-internal.h\n>> +++ b/refs/refs-internal.h\n>> @@ -203,6 +203,7 @@ struct ref_transaction {\n>> \tenum ref_transaction_state state;\n>> \tvoid *backend_data;\n>> \tunsigned int flags;\n>> +\tunsigned int max_index;\n>> };\n>>\n>> /*\n>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n>> index 00d95a9a2f..289496058e 100644\n>> --- a/refs/reftable-backend.c\n>> +++ b/refs/reftable-backend.c\n>> @@ -942,6 +942,7 @@ struct write_transaction_table_arg {\n>> \tsize_t updates_nr;\n>> \tsize_t updates_alloc;\n>> \tsize_t updates_expected;\n>> +\tunsigned int max_index;\n>\n> I wonder if this and the above should be `uint64_t` instead of `unsigned\n> int`. From the file names and the data format, it looks like we\n> intentionally use a 64-bit integer. That's good, because I have\n> unfortunately seen some people who have created giant test repositories\n> with really unreasonable numbers of commits and I could see us possibly\n> exceeding a 32-bit integer here.\n\nIdeally it should be okay, since this only comes into play when we're\nmigrating reflogs. So the index would be the number of reflog entries\nfor a given ref. I suppose even for large repositories this number\nshould be low. But I'd rather be safe here, so let me modify this in the\nnext version to be 'uint64_t'.\n\n> --\n> brian m. carlson (they/them or he/him)\n> Toronto, Ontario, CA\n"},{"id":"510747","messageId":"Z4mUizLNUdq_1BgY@tapette.crustytoothpaste.net","threadId":"62796","inReplyTo":"CAOLa=ZTL9n_DPhNr49XAd6bT838kc09oVx_AH7Pb4o8VK_xQ9w@mail.gmail.com","subject":"Re: Bug in 2.48 with `git refs migrate`","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-01-16T23:21:47Z","receivedAt":"2025-01-16T23:21:50Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-01-15 at 11:54:51, Karthik Nayak wrote:\n> \"brian m. carlson\" writes:\n> I'm attaching a patch below which should fixes the issue for me and also\n> adding a test to test against the same. I'd be grateful if you could\n> also test the patch against the repositoryies you mention.\n\nI can confirm that the patch did indeed fix the problem. I was able to\nconvert both repositories successfully (and very quickly, no less), and\nthey both work fine (I did normal development activity with them) with\nan unmodified Git 2.48 after the migration process.\n\nThanks again for the quick fix.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"510748","messageId":"xmqqmsfqnw4s.fsf@gitster.g","threadId":"62796","inReplyTo":"Z4mUizLNUdq_1BgY@tapette.crustytoothpaste.net","subject":"Re: Bug in 2.48 with `git refs migrate`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-17T00:04:35Z","receivedAt":"2025-01-17T00:04:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On 2025-01-15 at 11:54:51, Karthik Nayak wrote:\n>> \"brian m. carlson\" writes:\n>> I'm attaching a patch below which should fixes the issue for me and also\n>> adding a test to test against the same. I'd be grateful if you could\n>> also test the patch against the repositoryies you mention.\n>\n> I can confirm that the patch did indeed fix the problem. I was able to\n> convert both repositories successfully (and very quickly, no less), and\n> they both work fine (I did normal development activity with them) with\n> an unmodified Git 2.48 after the migration process.\n>\n> Thanks again for the quick fix.\n\nThanks, both. Let's merge it down to 'next' and then to 'master',\nthen.\n"},{"id":"510756","messageId":"CAOLa=ZQC9E6WnaD-aaNzOJnqjwvdccvopOkagRvKmsz7_U1V6A@mail.gmail.com","threadId":"62796","inReplyTo":"Z4mUizLNUdq_1BgY@tapette.crustytoothpaste.net","subject":"Re: Bug in 2.48 with `git refs migrate`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-17T06:15:18Z","receivedAt":"2025-01-17T06:15:20Z","isPatch":false,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On 2025-01-15 at 11:54:51, Karthik Nayak wrote:\n>> \"brian m. carlson\" writes:\n>> I'm attaching a patch below which should fixes the issue for me and also\n>> adding a test to test against the same. I'd be grateful if you could\n>> also test the patch against the repositoryies you mention.\n>\n> I can confirm that the patch did indeed fix the problem. I was able to\n> convert both repositories successfully (and very quickly, no less), and\n> they both work fine (I did normal development activity with them) with\n> an unmodified Git 2.48 after the migration process.\n>\n\nWohoo!\n\n> Thanks again for the quick fix.\n\nThanks for reporting and testing! I'm glad it fixed the issue.\n\n> --\n> brian m. carlson (they/them or he/him)\n> Toronto, Ontario, CA\n"},{"id":"510757","messageId":"CAOLa=ZTqv4owoj5qFjn9XqKahsmBhY-CynQJstWcP=OBChRn5g@mail.gmail.com","threadId":"62796","inReplyTo":"xmqqmsfqnw4s.fsf@gitster.g","subject":"Re: Bug in 2.48 with `git refs migrate`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-17T06:17:16Z","receivedAt":"2025-01-17T06:17:18Z","isPatch":false,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> \"brian m. carlson\" writes:\n>\n>> On 2025-01-15 at 11:54:51, Karthik Nayak wrote:\n>>> \"brian m. carlson\" writes:\n>>> I'm attaching a patch below which should fixes the issue for me and also\n>>> adding a test to test against the same. I'd be grateful if you could\n>>> also test the patch against the repositoryies you mention.\n>>\n>> I can confirm that the patch did indeed fix the problem. I was able to\n>> convert both repositories successfully (and very quickly, no less), and\n>> they both work fine (I did normal development activity with them) with\n>> an unmodified Git 2.48 after the migration process.\n>>\n>> Thanks again for the quick fix.\n>\n> Thanks, both. Let's merge it down to 'next' and then to 'master',\n> then.\n\nI was going to send some additional commits around the patch as a v2,\nbut I think they can come in as follow ups. This would ensure that the\nfix is already in place and we can discuss over the other commits\nslowly.\n\nThanks\n"},{"id":"511109","messageId":"20250123135613.748916-1-karthik.188@gmail.com","threadId":"62796","inReplyTo":"CAOLa=ZTL9n_DPhNr49XAd6bT838kc09oVx_AH7Pb4o8VK_xQ9w@mail.gmail.com","subject":"[PATCH v2] reftable: write correct max_update_index to header","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-23T13:56:13Z","receivedAt":"2025-01-23T13:56:24Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In 297c09eabb (refs: allow multiple reflog entries for the same refname,\n2024-12-16), the reftable backend learned to handle multiple reflog\nentries within the same transaction. This was done modifying the\n`update_index` for reflogs with multiple indices. During writing the\nlogs, the `max_update_index` of the writer was modified to ensure the\nlimits were raised to the modified `update_index`s.\n\nHowever, since ref entries are written before the modification to the\n`max_update_index`, if there are multiple blocks to be written, the\nreftable backend writes the header with the old `max_update_index`. When\nall logs are finally written, the footer will be written with the new\n`min_update_index`. This causes a mismatch between the header and the\nfooter and causes the reftable file to be corrupted. The existing tests\nonly spawn a single block and since headers are lazily written with the\nfirst block, the tests didn't capture this bug.\n\nTo fix the issue, the appropriate `max_update_index` limit must be set\neven before the first block is written. Add a `max_index` field to the\ntransaction which holds the `max_index` within all its updates, then\npropagate this value to the reftable backend, wherein this is used to\nthe set the `max_update_index` correctly.\n\nAdd a test which creates a few thousand reference updates with multiple\nreflog entries, which should trigger the bug.\n\nReported-by: brian m. carlson \nSigned-off-by: Karthik Nayak \n---\n\nHello,\n\nWhile this patch was merged to next, Dscho reported that it was flaky\non macos pipeline. On further investigation I found this was easily\nreproducible when the leak sanitizer was turned on and the reftable\ntests were run. The fix was simply to add the missing 0 initialization.\n\nI also found that we didn't set the appropriate values for all elements\nof the `reftable_transaction_data`. Which I've done now.\n\nThe patch is based on Maint f93ff170b9 (Git 2.48.1, 2025-01-13). \n\nThanks\n\nChanges from v1:\n1. Correctly intialize the value to `0` for `write_transaction_table_arg.max_index`.\n2. Set the `max_index` for all elements of `reftable_transaction_data`.\n3. The range diff was made against the commit in Junio's branch\n `gitster/kn/reflog-migration-fix`, which included his 'S-o-b', which I've\n removed in this patch, since that is usually applied by Junio. Not sure if\n that was the right move. \n\nRange diff:\n1: bc67b4ab5f ! 1: 5489826b47 reftable: write correct max_update_index to header\n @@ Commit message\n \n Reported-by: brian m. carlson \n Signed-off-by: Karthik Nayak \n - Signed-off-by: Junio C Hamano \n \n ## refs.c ##\n @@ refs.c: int ref_transaction_update_reflog(struct ref_transaction *transaction,\n @@ refs/reftable-backend.c: struct write_transaction_table_arg {\n };\n \n struct reftable_transaction_data {\n +@@ refs/reftable-backend.c: static int prepare_transaction_update(struct write_transaction_table_arg **out,\n + \t\targ->updates_nr = 0;\n + \t\targ->updates_alloc = 0;\n + \t\targ->updates_expected = 0;\n ++\t\targ->max_index = 0;\n + \t}\n + \n + \targ->updates_expected++;\n @@ refs/reftable-backend.c: static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \tstruct reftable_log_record *logs = NULL;\n \tstruct ident_split committer_ident = {0};\n @@ refs/reftable-backend.c: static int write_transaction_table(struct reftable_writ\n \t\tif (ret < 0)\n \t\t\tgoto done;\n @@ refs/reftable-backend.c: static int reftable_be_transaction_finish(struct ref_store *ref_store UNUSED,\n - \tstruct reftable_transaction_data *tx_data = transaction->backend_data;\n \tint ret = 0;\n \n -+\tif (tx_data->args)\n -+\t\ttx_data->args->max_index = transaction->max_index;\n -+\n \tfor (size_t i = 0; i < tx_data->args_nr; i++) {\n ++\t\ttx_data->args[i].max_index = transaction->max_index;\n ++\n \t\tret = reftable_addition_add(tx_data->args[i].addition,\n \t\t\t\t\t write_transaction_table, &tx_data->args[i]);\n + \t\tif (ret < 0)\n \n ## t/t1460-refs-migrate.sh ##\n @@ t/t1460-refs-migrate.sh: do\n---\n refs.c | 7 +++++++\n refs/refs-internal.h | 1 +\n refs/reftable-backend.c | 20 ++++++++++----------\n t/t1460-refs-migrate.sh | 12 ++++++++++++\n 4 files changed, 30 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0f41b2fd4a..f7b6f0f897 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1345,6 +1345,13 @@ int ref_transaction_update_reflog(struct ref_transaction *transaction,\n \tupdate->flags &= ~REF_HAVE_OLD;\n \tupdate->index = index;\n \n+\t/*\n+\t * Reference backends may need to know the max index to optimize\n+\t * their writes. So we store the max_index on the transaction level.\n+\t */\n+\tif (index > transaction->max_index)\n+\t\ttransaction->max_index = index;\n+\n \treturn 0;\n }\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 16550862d3..aaab711bb9 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -203,6 +203,7 @@ struct ref_transaction {\n \tenum ref_transaction_state state;\n \tvoid *backend_data;\n \tunsigned int flags;\n+\tunsigned int max_index;\n };\n \n /*\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 00d95a9a2f..d39a14c5a4 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -942,6 +942,7 @@ struct write_transaction_table_arg {\n \tsize_t updates_nr;\n \tsize_t updates_alloc;\n \tsize_t updates_expected;\n+\tunsigned int max_index;\n };\n \n struct reftable_transaction_data {\n@@ -1019,6 +1020,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \t\targ->updates_nr = 0;\n \t\targ->updates_alloc = 0;\n \t\targ->updates_expected = 0;\n+\t\targ->max_index = 0;\n \t}\n \n \targ->updates_expected++;\n@@ -1428,7 +1430,6 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \tstruct reftable_log_record *logs = NULL;\n \tstruct ident_split committer_ident = {0};\n \tsize_t logs_nr = 0, logs_alloc = 0, i;\n-\tuint64_t max_update_index = ts;\n \tconst char *committer_info;\n \tint ret = 0;\n \n@@ -1438,7 +1439,12 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \n \tQSORT(arg->updates, arg->updates_nr, transaction_update_cmp);\n \n-\treftable_writer_set_limits(writer, ts, ts);\n+\t/*\n+\t * During reflog migration, we add indexes for a single reflog with\n+\t * multiple entries. Each entry will contain a different update_index,\n+\t * so set the limits accordingly.\n+\t */\n+\treftable_writer_set_limits(writer, ts, ts + arg->max_index);\n \n \tfor (i = 0; i < arg->updates_nr; i++) {\n \t\tstruct reftable_transaction_update *tx_update = &arg->updates[i];\n@@ -1540,12 +1546,6 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t\t\t\t */\n \t\t\t\tlog->update_index = ts + u->index;\n \n-\t\t\t\t/*\n-\t\t\t\t * Note the max update_index so the limit can be set later on.\n-\t\t\t\t */\n-\t\t\t\tif (log->update_index > max_update_index)\n-\t\t\t\t\tmax_update_index = log->update_index;\n-\n \t\t\t\tlog->refname = xstrdup(u->refname);\n \t\t\t\tmemcpy(log->value.update.new_hash,\n \t\t\t\t u->new_oid.hash, GIT_MAX_RAWSZ);\n@@ -1609,8 +1609,6 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t * and log blocks.\n \t */\n \tif (logs) {\n-\t\treftable_writer_set_limits(writer, ts, max_update_index);\n-\n \t\tret = reftable_writer_add_logs(writer, logs, logs_nr);\n \t\tif (ret < 0)\n \t\t\tgoto done;\n@@ -1632,6 +1630,8 @@ static int reftable_be_transaction_finish(struct ref_store *ref_store UNUSED,\n \tint ret = 0;\n \n \tfor (size_t i = 0; i < tx_data->args_nr; i++) {\n+\t\ttx_data->args[i].max_index = transaction->max_index;\n+\n \t\tret = reftable_addition_add(tx_data->args[i].addition,\n \t\t\t\t\t write_transaction_table, &tx_data->args[i]);\n \t\tif (ret < 0)\ndiff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh\nindex f59bc4860f..307b2998ef 100755\n--- a/t/t1460-refs-migrate.sh\n+++ b/t/t1460-refs-migrate.sh\n@@ -227,6 +227,18 @@ do\n \tdone\n done\n \n+test_expect_success 'multiple reftable blocks with multiple entries' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init --ref-format=files repo &&\n+\ttest_commit -C repo first &&\n+\tprintf \"create refs/heads/ref-%d HEAD\\n\" $(test_seq 5000) >stdin &&\n+\tgit -C repo update-ref --stdin stdin &&\n+\tgit -C repo update-ref --stdin writes:\n\n> While this patch was merged to next, Dscho reported that it was flaky\n> on macos pipeline. On further investigation I found this was easily\n> reproducible when the leak sanitizer was turned on and the reftable\n> tests were run. The fix was simply to add the missing 0 initialization.\n\nIf it is already _in_ 'next', please turn it into a relative patch\non top of it, instead of replacing it.\n\nThat will give you an opportunity to describe the breakage in the\noriginal version, which everybody missed until it hit 'next'. And\nyou can also credit the folks who reported the breakage, and\ndescribe the fix.\n\nThe reason we do not revert out of 'next' lightly is because the\nchanges we merge to 'next' are supposed to be reviewed well enough,\nwhich means that any bug we discover later is likely to have been\ncaused by mistakes any of us may repeat in the future, and it is\nworth documenting in our history.\n\nIt is quite a different review philosophy if you compare the rules\nwe use for patches that haven't hit 'next'. These uncooked patches\nmay have mistrakes that reviewers can easily spot and get corrected,\nand these easy ones are not worth documenting as much.\n\n> The patch is based on Maint f93ff170b9 (Git 2.48.1, 2025-01-13). \n\nThanks.\n\n"},{"id":"511128","messageId":"xmqq5xm5s80e.fsf@gitster.g","threadId":"62796","inReplyTo":"xmqqbjvxs8me.fsf@gitster.g","subject":"Re: [PATCH v2] reftable: write correct max_update_index to header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-23T18:24:49Z","receivedAt":"2025-01-23T18:24:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> While this patch was merged to next, Dscho reported that it was flaky\n>> on macos pipeline. On further investigation I found this was easily\n>> reproducible when the leak sanitizer was turned on and the reftable\n>> tests were run. The fix was simply to add the missing 0 initialization.\n>\n> If it is already _in_ 'next', please turn it into a relative patch\n> on top of it, instead of replacing it.\n\nFor now, I have tentatively created the following and will queue on\na separate kn/reflog-migration-fix-fix topic (which would be ahead\nof kn/reflog-migration-fix topic by this one commit), in the hope\nthat it can be replaced with a version with proper commit log\nmessage that describes what bugs in the original \"fix\" are\naddressed, how they are caused (e.g., how does it lead to the\nbreakage to forget clearing of arg->max_index in the first hunk\nhad?), and what their fixes are.\n\nThanks.\n\n--- >8 ---\nFrom: Karthik Nayak \nDate: Fri, 20 Dec 2024 13:58:37 +0100\nSubject: [PATCH] SQUASH - needs to describe the breakage and fix in v1\n\n---\n refs/reftable-backend.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 68db2baa8f..bb658826fe 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -920,6 +920,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \t\targ->updates_nr = 0;\n \t\targ->updates_alloc = 0;\n \t\targ->updates_expected = 0;\n+\t\targ->max_index = 0;\n \t}\n \n \targ->updates_expected++;\n@@ -1502,10 +1503,9 @@ static int reftable_be_transaction_finish(struct ref_store *ref_store UNUSED,\n \tstruct reftable_transaction_data *tx_data = transaction->backend_data;\n \tint ret = 0;\n \n-\tif (tx_data->args)\n-\t\ttx_data->args->max_index = transaction->max_index;\n-\n \tfor (size_t i = 0; i < tx_data->args_nr; i++) {\n+\t\ttx_data->args[i].max_index = transaction->max_index;\n+\n \t\tret = reftable_addition_add(tx_data->args[i].addition,\n \t\t\t\t\t write_transaction_table, &tx_data->args[i]);\n \t\tif (ret < 0)\n-- \n2.48.1-259-gf9754493bb\n\n"},{"id":"511138","messageId":"CAOLa=ZSisRwmUyLHAP0e=8U0QjfFbDfXwd2mJXKjVywcmM56Tg@mail.gmail.com","threadId":"62796","inReplyTo":"xmqqbjvxs8me.fsf@gitster.g","subject":"Re: [PATCH v2] reftable: write correct max_update_index to header","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-24T04:06:21Z","receivedAt":"2025-01-24T04:06:22Z","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>> While this patch was merged to next, Dscho reported that it was flaky\n>> on macos pipeline. On further investigation I found this was easily\n>> reproducible when the leak sanitizer was turned on and the reftable\n>> tests were run. The fix was simply to add the missing 0 initialization.\n>\n> If it is already _in_ 'next', please turn it into a relative patch\n> on top of it, instead of replacing it.\n>\n> That will give you an opportunity to describe the breakage in the\n> original version, which everybody missed until it hit 'next'. And\n> you can also credit the folks who reported the breakage, and\n> describe the fix.\n>\n> The reason we do not revert out of 'next' lightly is because the\n> changes we merge to 'next' are supposed to be reviewed well enough,\n> which means that any bug we discover later is likely to have been\n> caused by mistakes any of us may repeat in the future, and it is\n> worth documenting in our history.\n>\n> It is quite a different review philosophy if you compare the rules\n> we use for patches that haven't hit 'next'. These uncooked patches\n> may have mistrakes that reviewers can easily spot and get corrected,\n> and these easy ones are not worth documenting as much.\n>\n\nThanks Junio, I understand your reasoning here and it makes sense to me.\nDo you think it is worthwhile to also add something like this to our\nDocumentation? I couldn't find anything there. I'll add a small patch to\nthe bottom of this mail.\n\n>> The patch is based on Maint f93ff170b9 (Git 2.48.1, 2025-01-13).\n>\n> Thanks.\n\n-- >8 --\n\nSubject: [PATCH] doc: add guideline to tackle bugs in `next`\n\nWhen fixing a bug in a topic already merged into `next`, there are no\nstrict guidelines to follow. While topics in `seen` can be reverted,\ntopics in `next` have undergone thorough review. Documenting fixes for\nsuch topics is valuable, as it helps to clarify the issue and\ncontributes to preventing similar problems in the future.\n\nSigned-off-by: Karthik Nayak \n---\n Documentation/SubmittingPatches | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex 958e3cc3d5..72454acf21 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -115,6 +115,13 @@ latest HEAD commit of `maint` or `master` based\non the following cases:\n new API features on the cutting edge that recently appeared in\n `master` but were not available in the released version).\n\n+* If you're fixing a bug in a topic that's already been merged into\n+ `next`, it's preferable to create a patch relative to that topic.\n+ This approach allows you to describe the issue in the original version\n+ that went unnoticed until it reached next. Additionally, it provides\n+ an opportunity to credit those who reported the issue and document\n+ the details of the fix.\n+\n * Otherwise (such as if you are adding new features) use `master`.\n\n\n-- \n2.47.0\n"},{"id":"511164","messageId":"20250124140203.886324-1-karthik.188@gmail.com","threadId":"62796","inReplyTo":"xmqq5xm5s80e.fsf@gitster.g","subject":"[PATCH v3] refs: fix uninitialized memory access of `max_index`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-24T14:02:03Z","receivedAt":"2025-01-24T14:02:15Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"When migrating reflogs between reference backends, maintaining the\noriginal order of the reflog entries is crucial. To achieve this, an\n`index` field is stored within the `ref_update` struct.\n\nIn the reftable backend, before writing any references, the writer must\nbe configured with the minimum and maximum update index values. The\n`max_update_index` is derived from the maximum `ref_update.index` value\nin a transaction . The commit bc67b4ab5f (reftable: write correct\nmax_update_index to header, 2025-01-15) addressed this by propagating the\n`max_update_index` value from the transaction to\n`write_transaction_table_arg` and, ultimately, to\n`reftable_writer_set_limits()`, which sets the min and max index for the\nreftable writer.\n\nHowever, that commit introduced an issue:\n\n - In `reftable_transaction_data`, which contains an array of\n `write_transaction_table_arg`, only the first element was assigned the\n `max_index` value.\n\nAs a result, any elements beyond the first in the array contained\nuninitialized `max_index`. The writer contains multiple elements of\n`write_transaction_table_arg` to correspond to different worktrees being\nwritten. This uninitialized value was later used to set the\n`max_update_index` for the writer, potentially causing overflow or\nundefined behavior.\n\nFix this by:\n\n - Initializing the `max_index` field to 0.\n - Moving the assignment of `max_index` in\n `reftable_be_transaction_finish()` inside the loop, ensuring all\n elements of the array are correctly initialized.\n\nInitializing `max_index` to `0` is not strictly necessary, as all\nelements of `write_transaction_table_arg.max_index` are now assigned\ncorrectly. However, this initialization is added for consistency and to\nsafeguard against potential future changes that might inadvertently\nintroduce uninitialized memory access.\n\nReported-by: Johannes Schindelin \nSigned-off-by: Karthik Nayak \n---\nHello,\n\nAs suggested, I've redone my patch to make this a relative patch on top of\n'kn/reflog-migration-fix'. \n\nThis is based on top of maint with 'kn/reflog-migration-fix' merged in.\n\nThanks\n---\n refs/reftable-backend.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 289496058e..d39a14c5a4 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1020,6 +1020,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \t\targ->updates_nr = 0;\n \t\targ->updates_alloc = 0;\n \t\targ->updates_expected = 0;\n+\t\targ->max_index = 0;\n \t}\n \n \targ->updates_expected++;\n@@ -1628,10 +1629,9 @@ static int reftable_be_transaction_finish(struct ref_store *ref_store UNUSED,\n \tstruct reftable_transaction_data *tx_data = transaction->backend_data;\n \tint ret = 0;\n \n-\tif (tx_data->args)\n-\t\ttx_data->args->max_index = transaction->max_index;\n-\n \tfor (size_t i = 0; i < tx_data->args_nr; i++) {\n+\t\ttx_data->args[i].max_index = transaction->max_index;\n+\n \t\tret = reftable_addition_add(tx_data->args[i].addition,\n \t\t\t\t\t write_transaction_table, &tx_data->args[i]);\n \t\tif (ret < 0)\n-- \n2.47.0\n\n"},{"id":"511166","messageId":"Z5On1waE-2uwIjS2@pks.im","threadId":"62796","inReplyTo":"20250124140203.886324-1-karthik.188@gmail.com","subject":"Re: [PATCH v3] refs: fix uninitialized memory access of `max_index`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-24T14:46:47Z","receivedAt":"2025-01-24T14:46:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 24, 2025 at 03:02:03PM +0100, Karthik Nayak wrote:\n> When migrating reflogs between reference backends, maintaining the\n> original order of the reflog entries is crucial. To achieve this, an\n> `index` field is stored within the `ref_update` struct.\n> \n> In the reftable backend, before writing any references, the writer must\n> be configured with the minimum and maximum update index values. The\n> `max_update_index` is derived from the maximum `ref_update.index` value\n> in a transaction . The commit bc67b4ab5f (reftable: write correct\n> max_update_index to header, 2025-01-15) addressed this by propagating the\n> `max_update_index` value from the transaction to\n> `write_transaction_table_arg` and, ultimately, to\n> `reftable_writer_set_limits()`, which sets the min and max index for the\n> reftable writer.\n> \n> However, that commit introduced an issue:\n> \n> - In `reftable_transaction_data`, which contains an array of\n> `write_transaction_table_arg`, only the first element was assigned the\n> `max_index` value.\n> \n> As a result, any elements beyond the first in the array contained\n> uninitialized `max_index`. The writer contains multiple elements of\n> `write_transaction_table_arg` to correspond to different worktrees being\n> written. This uninitialized value was later used to set the\n> `max_update_index` for the writer, potentially causing overflow or\n> undefined behavior.\n\nIt reads a bit funny as a bulleted list with a single item, only. A\nsuggestion for the above:\n\n However, we only set the update index for the first\n `write_transaction_table_arg`, even though there can be multiple\n such arguments. This is the case when we write to multiple stacks in\n a single transaction, e.g. when updating references in two different\n worktrees at once. And, if so, we wouldn't have initialized the\n update index for any but the first such argument. This uninitialized\n value was later used to set the `max_update_index` for the writer,\n potentially causing undefined behaviour.\n\nOther than that this is nicely described, and the fix looks reasonable,\ntoo.\n\nPatrick\n"},{"id":"511167","messageId":"xmqqikq4qlmz.fsf@gitster.g","threadId":"62796","inReplyTo":"CAOLa=ZSisRwmUyLHAP0e=8U0QjfFbDfXwd2mJXKjVywcmM56Tg@mail.gmail.com","subject":"Re: [PATCH v2] reftable: write correct max_update_index to header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-24T15:25:40Z","receivedAt":"2025-01-24T15:25:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Thanks Junio, I understand your reasoning here and it makes sense to me.\n> Do you think it is worthwhile to also add something like this to our\n> Documentation? I couldn't find anything there. I'll add a small patch to\n> the bottom of this mail.\n\nPerhaps. I think referring people to \"Notes from the maintainer\"\nmay also work without duplicating information.\n\nThanks.\n\n"},{"id":"511170","messageId":"CAOLa=ZR=Hz+LU0n-uC2dpk8_sLqAyaxO0NswJH8bP_kEdDdbUQ@mail.gmail.com","threadId":"62796","inReplyTo":"Z5On1waE-2uwIjS2@pks.im","subject":"Re: [PATCH v3] refs: fix uninitialized memory access of `max_index`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-24T15:48:43Z","receivedAt":"2025-01-24T15:48:45Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Jan 24, 2025 at 03:02:03PM +0100, Karthik Nayak wrote:\n>> When migrating reflogs between reference backends, maintaining the\n>> original order of the reflog entries is crucial. To achieve this, an\n>> `index` field is stored within the `ref_update` struct.\n>>\n>> In the reftable backend, before writing any references, the writer must\n>> be configured with the minimum and maximum update index values. The\n>> `max_update_index` is derived from the maximum `ref_update.index` value\n>> in a transaction . The commit bc67b4ab5f (reftable: write correct\n>> max_update_index to header, 2025-01-15) addressed this by propagating the\n>> `max_update_index` value from the transaction to\n>> `write_transaction_table_arg` and, ultimately, to\n>> `reftable_writer_set_limits()`, which sets the min and max index for the\n>> reftable writer.\n>>\n>> However, that commit introduced an issue:\n>>\n>> - In `reftable_transaction_data`, which contains an array of\n>> `write_transaction_table_arg`, only the first element was assigned the\n>> `max_index` value.\n>>\n>> As a result, any elements beyond the first in the array contained\n>> uninitialized `max_index`. The writer contains multiple elements of\n>> `write_transaction_table_arg` to correspond to different worktrees being\n>> written. This uninitialized value was later used to set the\n>> `max_update_index` for the writer, potentially causing overflow or\n>> undefined behavior.\n>\n> It reads a bit funny as a bulleted list with a single item, only. A\n> suggestion for the above:\n>\n> However, we only set the update index for the first\n> `write_transaction_table_arg`, even though there can be multiple\n> such arguments. This is the case when we write to multiple stacks in\n> a single transaction, e.g. when updating references in two different\n> worktrees at once. And, if so, we wouldn't have initialized the\n> update index for any but the first such argument. This uninitialized\n> value was later used to set the `max_update_index` for the writer,\n> potentially causing undefined behaviour.\n>\n> Other than that this is nicely described, and the fix looks reasonable,\n> too.\n>\n> Patrick\n\nThanks Patrick for taking the time, this seems much better. Let me add\nthis in for the next version.\n"},{"id":"511234","messageId":"Z5dd1YOYytx7kZee@pks.im","threadId":"62796","inReplyTo":"CAOLa=ZR=Hz+LU0n-uC2dpk8_sLqAyaxO0NswJH8bP_kEdDdbUQ@mail.gmail.com","subject":"Re: [PATCH v3] refs: fix uninitialized memory access of `max_index`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-27T10:20:05Z","receivedAt":"2025-01-27T10:20:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 24, 2025 at 07:48:43AM -0800, Karthik Nayak wrote:\n> Thanks Patrick for taking the time, this seems much better. Let me add\n> this in for the next version.\n\nI've sent a v4 of this patch, but forgot to set the In-reply-to header.\nThe patch can be found at [1].\n\nPatrick\n\n[1]: \n"},{"id":"511280","messageId":"xmqqikq0uswi.fsf@gitster.g","threadId":"62796","inReplyTo":"Z5dd1YOYytx7kZee@pks.im","subject":"Re: [PATCH v3] refs: fix uninitialized memory access of `max_index`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-27T16:24:13Z","receivedAt":"2025-01-27T16:24:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Jan 24, 2025 at 07:48:43AM -0800, Karthik Nayak wrote:\n>> Thanks Patrick for taking the time, this seems much better. Let me add\n>> this in for the next version.\n>\n> I've sent a v4 of this patch, but forgot to set the In-reply-to header.\n> The patch can be found at [1].\n>\n> Patrick\n>\n> [1]: \n\nAh, how very considerate of you. List-archive spelunkers have\nbidirectional links to see (1) from an older discussion, how the\nissues were resolved, and (2) from the final attempt, where the\nissues came from.\n\nThanks.\n"}]}