{"thread":{"id":"58331","subject":"[PATCH 0/6] midx: permit changing the preferred pack when reusing the MIDX","startedAt":"2022-08-19T21:30:14Z","lastAt":"2022-08-23T18:12:37Z","messageCount":29,"participants":["Taylor Blau","Abhradeep Chakraborty","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"461624","messageId":"cover.1660944574.git.me@ttaylorr.com","threadId":"58331","inReplyTo":null,"subject":"[PATCH 0/6] midx: permit changing the preferred pack when reusing the MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-19T21:30:02Z","receivedAt":"2022-08-19T21:30:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series resolves a bug that was reported[1] by Johannes, and\ninvestigated by him, Abhradeep, and Stolee in that same sub-thread.\n\nThe crux of the issue is that a MIDX bitmap can enter a corrupt state\nwhen changing the preferred pack from its value in an existing MIDX in\ncertain circumstances as described in the first and final patches.\n\nThis series is structured as follows:\n\n - The first patch of this series adds a test which demonstrates the\n problem. (This is an improvement from the debugging in [1], where we\n only noticed the problem racily in an existing test, and only in\n SHA-256 mode).\n\n - The next small handful of patches refactor midx.c's\n `get_sorted_entries()` function to make fixing this bug more\n straightforward.\n\n - The final patch resolves the bug and updates the test to no longer\n expect failure.\n\nA couple of meta-notes:\n\n - This bug has existed since the introduction of MIDX bitmaps, but\n probably wasn't noticed until now since it is only triggerable when\n the `--stdin-packs` mode *isn't* passed, so it never occurs when\n invoked via `git repack`.\n\n - We could likely change the behavior of 56d863e979 (midx: expose\n `write_midx_file_only()` publicly, 2021-09-28), which explicitly\n disables reusing the existing MIDX (by avoiding loading it\n altogether) when `--stdin-packs` is given.\n\n The rationale in the comment added by 56d863e979 is somewhat unclear,\n but I have a vague recollection of running into a bug that was\n squashed by avoiding reusing an existing MIDX to write one with\n bitmaps. This was likely that bug.\n\nThanks in advance for your review, and thanks to Johannes, Abhradeep,\nand Stolee for investigating this bug while I was on vacation.\n\n[1]: https://lore.kernel.org/git/p3r70610-8n52-s8q0-n641-onp4ps01330n@tzk.qr/\n\nTaylor Blau (6):\n t5326: demonstrate potential bitmap corruption\n t/lib-bitmap.sh: avoid silencing stderr\n midx.c: extract `struct midx_fanout`\n midx.c: extract `midx_fanout_add_midx_fanout()`\n midx.c: extract `midx_fanout_add_pack_fanout()`\n midx.c: include preferred pack correctly with existing MIDX\n\n midx.c | 128 ++++++++++++++++++++++------------\n t/lib-bitmap.sh | 2 +-\n t/t5326-multi-pack-bitmaps.sh | 43 ++++++++++++\n 3 files changed, 127 insertions(+), 46 deletions(-)\n\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"461625","messageId":"3e30ab1a19115107fc24a25118f2417319bd1b0d.1660944574.git.me@ttaylorr.com","threadId":"58331","inReplyTo":"cover.1660944574.git.me@ttaylorr.com","subject":"[PATCH 1/6] t5326: demonstrate potential bitmap corruption","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-19T21:30:10Z","receivedAt":"2022-08-19T21:30:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"It is possible to generate a corrupt MIDX bitmap when certain conditions\nare met. This happens when the preferred pack \"P\" changes to one (say,\n\"Q\") that:\n\n - \"Q\" has objects included in an existing MIDX,\n - but \"Q\" is different than \"P\",\n - and \"Q\" and \"P\" have some objects in common\n\nWhen this is the case, not all objects from \"Q\" will be selected from\n\"Q\" (ie., the generated MIDX will represent them as coming from a\ndifferent pack), despite \"Q\" being preferred.\n\nThis is an invariant violation, since all objects contained in the\nMIDX's preferred pack are supposed to originate from the preferred pack.\nIn other words, all duplicate objects are resolved in favor of the copy\nthat comes from the MIDX's preferred pack, if any.\n\nThis violation results in a corrupt object order, which cannot be\ninterpreted by the pack-bitmap code, leading to broken clones and other\ndefects.\n\nThis test demonstrates the above problem by constructing a minimal\nreproduction, and showing that the final `git clone` invocation fails.\n\nThe reproduction is mostly straightforward, except that the new pack\ngenerated between MIDX writes (which is necessary in order to prevent\nthat operation from being a noop) must sort ahead of all existing packs\nin order to prevent a different pack (neither \"P\" nor \"Q\") from\nappearing as preferred (meaning all its objects appear in order at the\nbeginning of the pseudo-pack order).\n\nSubsequent commits will first refactor the midx.c::get_sorted_entries()\nfunction, and then fix this bug.\n\nReported-by: Abhradeep Chakraborty \nSigned-off-by: Taylor Blau \n---\n t/t5326-multi-pack-bitmaps.sh | 43 +++++++++++++++++++++++++++++++++++\n 1 file changed, 43 insertions(+)\n\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 4fe57414c1..a60cec5cab 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -307,4 +307,47 @@ test_expect_success 'graceful fallback when missing reverse index' '\n \t)\n '\n \n+test_expect_success 'preferred pack change with existing MIDX bitmap' '\n+\trm -fr repo &&\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit base &&\n+\t\ttest_commit other &&\n+\n+\t\tgit rev-list --objects --no-object-names base >p1.objects &&\n+\t\tgit rev-list --objects --no-object-names other >p2.objects &&\n+\n+\t\tp1=\"$(git pack-objects \"$objdir/pack/pack\" \\\n+\t\t\t--delta-base-offset \n---\n t/lib-bitmap.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-bitmap.sh b/t/lib-bitmap.sh\nindex a95537e759..f595937094 100644\n--- a/t/lib-bitmap.sh\n+++ b/t/lib-bitmap.sh\n@@ -440,7 +440,7 @@ midx_bitmap_partial_tests () {\n \t\ttest_commit packed &&\n \t\tgit repack &&\n \t\ttest_commit loose &&\n-\t\tgit multi-pack-index write --bitmap 2>err &&\n+\t\tgit multi-pack-index write --bitmap &&\n \t\ttest_path_is_file $midx &&\n \t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap\n \t'\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"461627","messageId":"2df8f1e8843ff7b53f45d99da5f8dbbbd3415b9e.1660944574.git.me@ttaylorr.com","threadId":"58331","inReplyTo":"cover.1660944574.git.me@ttaylorr.com","subject":"[PATCH 3/6] midx.c: extract `struct midx_fanout`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-19T21:30:15Z","receivedAt":"2022-08-19T21:30:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To build up a list of objects (along with their packs, and the offsets\nwithin those packs that each object appears at), the MIDX code\nimplements `get_sorted_entries()` which builds up a list of candidates,\nsorts them, and then removes duplicate entries.\n\nTo do this, it keeps an array of `pack_midx_entry` structures that it\nbuilds up once for each fanout level (ie., for all possible values of\nthe first byte of each object's ID).\n\nThis array is a function-local variable of `get_sorted_entries()`. Since\nit uses the ALLOC_GROW() macro, having the `alloc_fanout` variable also\nbe local to that function, and only modified within that function is\nconvenient.\n\nHowever, subsequent changes will extract the two ways this array is\nfilled (from a pack at some fanout value, and from an existing MIDX at\nsome fanout value) into separate functions. Instead of passing around\npointers to the entries array, along with `nr_fanout` and\n`alloc_fanout`, encapsulate these three into a structure instead. Then\npass around a pointer to this structure instead.\n\nThis patch does not yet extract the above two functions, but sets us up\nto begin doing so in the following commit. For now, the implementation\nof get_sorted_entries() is only modified to replace `entries_by_fanout`\nwith `fanout.entries`, `nr_fanout` with `fanout.nr`, and so on.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 54 +++++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 35 insertions(+), 19 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 4e956cacb7..cdb6c481c7 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -577,6 +577,22 @@ static void fill_pack_entry(uint32_t pack_int_id,\n \tentry->preferred = !!preferred;\n }\n \n+struct midx_fanout {\n+\tstruct pack_midx_entry *entries;\n+\tuint32_t nr;\n+\tuint32_t alloc;\n+};\n+\n+static void midx_fanout_grow(struct midx_fanout *fanout, uint32_t nr)\n+{\n+\tALLOC_GROW(fanout->entries, nr, fanout->alloc);\n+}\n+\n+static void midx_fanout_sort(struct midx_fanout *fanout)\n+{\n+\tQSORT(fanout->entries, fanout->nr, midx_oid_compare);\n+}\n+\n /*\n * It is possible to artificially get into a state where there are many\n * duplicate copies of objects. That can create high memory pressure if\n@@ -595,8 +611,8 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\t\t\t\t int preferred_pack)\n {\n \tuint32_t cur_fanout, cur_pack, cur_object;\n-\tuint32_t alloc_fanout, alloc_objects, total_objects = 0;\n-\tstruct pack_midx_entry *entries_by_fanout = NULL;\n+\tuint32_t alloc_objects, total_objects = 0;\n+\tstruct midx_fanout fanout = { 0 };\n \tstruct pack_midx_entry *deduplicated_entries = NULL;\n \tuint32_t start_pack = m ? m->num_packs : 0;\n \n@@ -608,14 +624,14 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t * slices to be evenly distributed, with some noise. Hence,\n \t * allocate slightly more than one 256th.\n \t */\n-\talloc_objects = alloc_fanout = total_objects > 3200 ? total_objects / 200 : 16;\n+\talloc_objects = fanout.alloc = total_objects > 3200 ? total_objects / 200 : 16;\n \n-\tALLOC_ARRAY(entries_by_fanout, alloc_fanout);\n+\tALLOC_ARRAY(fanout.entries, fanout.alloc);\n \tALLOC_ARRAY(deduplicated_entries, alloc_objects);\n \t*nr_objects = 0;\n \n \tfor (cur_fanout = 0; cur_fanout < 256; cur_fanout++) {\n-\t\tuint32_t nr_fanout = 0;\n+\t\tfanout.nr = 0;\n \n \t\tif (m) {\n \t\t\tuint32_t start = 0, end;\n@@ -625,15 +641,15 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\tend = ntohl(m->chunk_oid_fanout[cur_fanout]);\n \n \t\t\tfor (cur_object = start; cur_object < end; cur_object++) {\n-\t\t\t\tALLOC_GROW(entries_by_fanout, nr_fanout + 1, alloc_fanout);\n+\t\t\t\tmidx_fanout_grow(&fanout, fanout.nr + 1);\n \t\t\t\tnth_midxed_pack_midx_entry(m,\n-\t\t\t\t\t\t\t &entries_by_fanout[nr_fanout],\n+\t\t\t\t\t\t\t &fanout.entries[fanout.nr],\n \t\t\t\t\t\t\t cur_object);\n \t\t\t\tif (nth_midxed_pack_int_id(m, cur_object) == preferred_pack)\n-\t\t\t\t\tentries_by_fanout[nr_fanout].preferred = 1;\n+\t\t\t\t\tfanout.entries[fanout.nr].preferred = 1;\n \t\t\t\telse\n-\t\t\t\t\tentries_by_fanout[nr_fanout].preferred = 0;\n-\t\t\t\tnr_fanout++;\n+\t\t\t\t\tfanout.entries[fanout.nr].preferred = 0;\n+\t\t\t\tfanout.nr++;\n \t\t\t}\n \t\t}\n \n@@ -646,36 +662,36 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\tend = get_pack_fanout(info[cur_pack].p, cur_fanout);\n \n \t\t\tfor (cur_object = start; cur_object < end; cur_object++) {\n-\t\t\t\tALLOC_GROW(entries_by_fanout, nr_fanout + 1, alloc_fanout);\n+\t\t\t\tmidx_fanout_grow(&fanout, fanout.nr + 1);\n \t\t\t\tfill_pack_entry(cur_pack,\n \t\t\t\t\t\tinfo[cur_pack].p,\n \t\t\t\t\t\tcur_object,\n-\t\t\t\t\t\t&entries_by_fanout[nr_fanout],\n+\t\t\t\t\t\t&fanout.entries[fanout.nr],\n \t\t\t\t\t\tpreferred);\n-\t\t\t\tnr_fanout++;\n+\t\t\t\tfanout.nr++;\n \t\t\t}\n \t\t}\n \n-\t\tQSORT(entries_by_fanout, nr_fanout, midx_oid_compare);\n+\t\tmidx_fanout_sort(&fanout);\n \n \t\t/*\n \t\t * The batch is now sorted by OID and then mtime (descending).\n \t\t * Take only the first duplicate.\n \t\t */\n-\t\tfor (cur_object = 0; cur_object < nr_fanout; cur_object++) {\n-\t\t\tif (cur_object && oideq(&entries_by_fanout[cur_object - 1].oid,\n-\t\t\t\t\t\t&entries_by_fanout[cur_object].oid))\n+\t\tfor (cur_object = 0; cur_object < fanout.nr; cur_object++) {\n+\t\t\tif (cur_object && oideq(&fanout.entries[cur_object - 1].oid,\n+\t\t\t\t\t\t&fanout.entries[cur_object].oid))\n \t\t\t\tcontinue;\n \n \t\t\tALLOC_GROW(deduplicated_entries, *nr_objects + 1, alloc_objects);\n \t\t\tmemcpy(&deduplicated_entries[*nr_objects],\n-\t\t\t &entries_by_fanout[cur_object],\n+\t\t\t &fanout.entries[cur_object],\n \t\t\t sizeof(struct pack_midx_entry));\n \t\t\t(*nr_objects)++;\n \t\t}\n \t}\n \n-\tfree(entries_by_fanout);\n+\tfree(fanout.entries);\n \treturn deduplicated_entries;\n }\n \n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"461628","messageId":"92b82c83ea31a7453be7a3414c725b2fda13065b.1660944574.git.me@ttaylorr.com","threadId":"58331","inReplyTo":"cover.1660944574.git.me@ttaylorr.com","subject":"[PATCH 4/6] midx.c: extract `midx_fanout_add_midx_fanout()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-19T21:30:18Z","receivedAt":"2022-08-19T21:30:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Extract a routine to add all objects whose object ID's first byte is\n`cur_fanout` from an existing MIDX.\n\nThis function will only be called once, so extracting it is purely\ncosmetic to improve the readability of `get_sorted_entries()` (its sole\ncaller) below.\n\nThe functionality is unchanged in this commit.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 47 ++++++++++++++++++++++++++++-------------------\n 1 file changed, 28 insertions(+), 19 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex cdb6c481c7..0d40089c4d 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -593,6 +593,31 @@ static void midx_fanout_sort(struct midx_fanout *fanout)\n \tQSORT(fanout->entries, fanout->nr, midx_oid_compare);\n }\n \n+static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n+\t\t\t\t\tstruct multi_pack_index *m,\n+\t\t\t\t\tint preferred_pack,\n+\t\t\t\t\tuint32_t cur_fanout)\n+{\n+\tuint32_t start = 0, end;\n+\tuint32_t cur_object;\n+\n+\tif (cur_fanout)\n+\t\tstart = ntohl(m->chunk_oid_fanout[cur_fanout - 1]);\n+\tend = ntohl(m->chunk_oid_fanout[cur_fanout]);\n+\n+\tfor (cur_object = start; cur_object < end; cur_object++) {\n+\t\tmidx_fanout_grow(fanout, fanout->nr + 1);\n+\t\tnth_midxed_pack_midx_entry(m,\n+\t\t\t\t\t &fanout->entries[fanout->nr],\n+\t\t\t\t\t cur_object);\n+\t\tif (nth_midxed_pack_int_id(m, cur_object) == preferred_pack)\n+\t\t\tfanout->entries[fanout->nr].preferred = 1;\n+\t\telse\n+\t\t\tfanout->entries[fanout->nr].preferred = 0;\n+\t\tfanout->nr++;\n+\t}\n+}\n+\n /*\n * It is possible to artificially get into a state where there are many\n * duplicate copies of objects. That can create high memory pressure if\n@@ -633,25 +658,9 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \tfor (cur_fanout = 0; cur_fanout < 256; cur_fanout++) {\n \t\tfanout.nr = 0;\n \n-\t\tif (m) {\n-\t\t\tuint32_t start = 0, end;\n-\n-\t\t\tif (cur_fanout)\n-\t\t\t\tstart = ntohl(m->chunk_oid_fanout[cur_fanout - 1]);\n-\t\t\tend = ntohl(m->chunk_oid_fanout[cur_fanout]);\n-\n-\t\t\tfor (cur_object = start; cur_object < end; cur_object++) {\n-\t\t\t\tmidx_fanout_grow(&fanout, fanout.nr + 1);\n-\t\t\t\tnth_midxed_pack_midx_entry(m,\n-\t\t\t\t\t\t\t &fanout.entries[fanout.nr],\n-\t\t\t\t\t\t\t cur_object);\n-\t\t\t\tif (nth_midxed_pack_int_id(m, cur_object) == preferred_pack)\n-\t\t\t\t\tfanout.entries[fanout.nr].preferred = 1;\n-\t\t\t\telse\n-\t\t\t\t\tfanout.entries[fanout.nr].preferred = 0;\n-\t\t\t\tfanout.nr++;\n-\t\t\t}\n-\t\t}\n+\t\tif (m)\n+\t\t\tmidx_fanout_add_midx_fanout(&fanout, m, preferred_pack,\n+\t\t\t\t\t\t cur_fanout);\n \n \t\tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++) {\n \t\t\tuint32_t start = 0, end;\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"461629","messageId":"db1c6ea8e5e9280db478f7452f725029fec747e8.1660944574.git.me@ttaylorr.com","threadId":"58331","inReplyTo":"cover.1660944574.git.me@ttaylorr.com","subject":"[PATCH 5/6] midx.c: extract `midx_fanout_add_pack_fanout()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-19T21:30:21Z","receivedAt":"2022-08-19T21:30:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Extract a routine to add all objects whose object ID's first byte is\n`cur_fanout` from a given pack (identified by its index into the `struct\npack_info` array maintained by the MIDX writing routine).\n\nUnlike the previous extraction (for `midx_fanout_add_midx_fanout()`),\nthis function will be called twice, once for all new packs, and again\nfor the preferred pack (if it appears in an existing MIDX). The latter\nchange is to resolve the bug described a few patches ago, and will be\nmade in the subsequent commit.\n\nSimilar to the previous refactoring, this function also enhances the\nreadability of its caller in `get_sorted_entries()`.\n\nIts functionality is unchanged in this commit.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 43 ++++++++++++++++++++++++++++---------------\n 1 file changed, 28 insertions(+), 15 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 0d40089c4d..be8186eec2 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -618,6 +618,31 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \t}\n }\n \n+static void midx_fanout_add_pack_fanout(struct midx_fanout *fanout,\n+\t\t\t\t\tstruct pack_info *info,\n+\t\t\t\t\tuint32_t cur_pack,\n+\t\t\t\t\tint preferred,\n+\t\t\t\t\tuint32_t cur_fanout)\n+{\n+\tstruct packed_git *pack = info[cur_pack].p;\n+\tuint32_t start = 0, end;\n+\tuint32_t cur_object;\n+\n+\tif (cur_fanout)\n+\t\tstart = get_pack_fanout(pack, cur_fanout - 1);\n+\tend = get_pack_fanout(pack, cur_fanout);\n+\n+\tfor (cur_object = start; cur_object < end; cur_object++) {\n+\t\tmidx_fanout_grow(fanout, fanout->nr + 1);\n+\t\tfill_pack_entry(cur_pack,\n+\t\t\t\tinfo[cur_pack].p,\n+\t\t\t\tcur_object,\n+\t\t\t\t&fanout->entries[fanout->nr],\n+\t\t\t\tpreferred);\n+\t\tfanout->nr++;\n+\t}\n+}\n+\n /*\n * It is possible to artificially get into a state where there are many\n * duplicate copies of objects. That can create high memory pressure if\n@@ -663,22 +688,10 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\t\t\t\t cur_fanout);\n \n \t\tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++) {\n-\t\t\tuint32_t start = 0, end;\n \t\t\tint preferred = cur_pack == preferred_pack;\n-\n-\t\t\tif (cur_fanout)\n-\t\t\t\tstart = get_pack_fanout(info[cur_pack].p, cur_fanout - 1);\n-\t\t\tend = get_pack_fanout(info[cur_pack].p, cur_fanout);\n-\n-\t\t\tfor (cur_object = start; cur_object < end; cur_object++) {\n-\t\t\t\tmidx_fanout_grow(&fanout, fanout.nr + 1);\n-\t\t\t\tfill_pack_entry(cur_pack,\n-\t\t\t\t\t\tinfo[cur_pack].p,\n-\t\t\t\t\t\tcur_object,\n-\t\t\t\t\t\t&fanout.entries[fanout.nr],\n-\t\t\t\t\t\tpreferred);\n-\t\t\t\tfanout.nr++;\n-\t\t\t}\n+\t\t\tmidx_fanout_add_pack_fanout(&fanout,\n+\t\t\t\t\t\t info, cur_pack,\n+\t\t\t\t\t\t preferred, cur_fanout);\n \t\t}\n \n \t\tmidx_fanout_sort(&fanout);\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"461630","messageId":"4ddddc959b042faf7ae98a8e8eaa05e77f9ea23e.1660944574.git.me@ttaylorr.com","threadId":"58331","inReplyTo":"cover.1660944574.git.me@ttaylorr.com","subject":"[PATCH 6/6] midx.c: include preferred pack correctly with existing MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-19T21:30:24Z","receivedAt":"2022-08-19T21:30:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This patch resolves an issue where the object order used to generate a\nMIDX bitmap would violate an invariant that all of the preferred pack's\nobjects are represented by that pack in the MIDX.\n\nThe problem arises when reusing an existing MIDX while generating a new\none, and occurs specifically when the identity of the preferred pack\nchanges from one MIDX to another, along with a few other conditions:\n\n - the new preferred pack must also be present in the existing MIDX\n\n - the new preferred pack must *not* have been the preferred pack in\n the existing MIDX\n\n - most importantly, there must be at least one object present in the\n physical preferred pack (ie., it shows up in that pack's index)\n but was selected from a *different* pack when the previous MIDX\n was generated\n\nWhen the above conditions are all met, we end up (incorrectly)\ndiscarding copies of some objects in the pack selected as the preferred\npack. This is because `get_sorted_entries()` adds objects to its list\nby doing the following at each fanout level:\n\n - first, adding all objects from that fanout level from an existing\n MIDX\n\n - then, adding all objects from that fanout level in each pack *not*\n included in the existing MIDX\n\nSo if some object was not selected from the to-be-preferred pack when\nwriting the previous MIDX, then we will never consider it as a candidate\nwhen generating the new MIDX. This means that it's possible for the\npreferred pack to not include all of its objects in the MIDX's\npseudo-pack object order, which is an invariant violation of that order.\n\nResolve this by adding all objects from the preferred pack separately\nwhen it appears in the existing MIDX (if one was present). This will\nduplicate objects from that pack that *did* appear in the MIDX, but this\nis fine, since get_sorted_entries() already handles duplicates. (A\nfuture optimization in this area could avoid adding copies of objects\nthat we know already existing in the MIDX.)\n\nNote that we no longer need to compute the preferred-ness of objects\nadded from the MIDX, since we only want to select the preferred objects\nfrom a single source. (We could still mark these preferred bits, but\ndoing so is redundant and unnecessary).\n\nThis resolves the bug described in the first patch of this series.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 14 +++++++-------\n t/t5326-multi-pack-bitmaps.sh | 2 +-\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex be8186eec2..bd1d27090e 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -595,7 +595,6 @@ static void midx_fanout_sort(struct midx_fanout *fanout)\n \n static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \t\t\t\t\tstruct multi_pack_index *m,\n-\t\t\t\t\tint preferred_pack,\n \t\t\t\t\tuint32_t cur_fanout)\n {\n \tuint32_t start = 0, end;\n@@ -610,10 +609,7 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \t\tnth_midxed_pack_midx_entry(m,\n \t\t\t\t\t &fanout->entries[fanout->nr],\n \t\t\t\t\t cur_object);\n-\t\tif (nth_midxed_pack_int_id(m, cur_object) == preferred_pack)\n-\t\t\tfanout->entries[fanout->nr].preferred = 1;\n-\t\telse\n-\t\t\tfanout->entries[fanout->nr].preferred = 0;\n+\t\tfanout->entries[fanout->nr].preferred = 0;\n \t\tfanout->nr++;\n \t}\n }\n@@ -684,8 +680,7 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\tfanout.nr = 0;\n \n \t\tif (m)\n-\t\t\tmidx_fanout_add_midx_fanout(&fanout, m, preferred_pack,\n-\t\t\t\t\t\t cur_fanout);\n+\t\t\tmidx_fanout_add_midx_fanout(&fanout, m, cur_fanout);\n \n \t\tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++) {\n \t\t\tint preferred = cur_pack == preferred_pack;\n@@ -694,6 +689,11 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\t\t\t\t preferred, cur_fanout);\n \t\t}\n \n+\t\tif (-1 < preferred_pack && preferred_pack < start_pack)\n+\t\t\tmidx_fanout_add_pack_fanout(&fanout, info,\n+\t\t\t\t\t\t preferred_pack, 1,\n+\t\t\t\t\t\t cur_fanout);\n+\n \t\tmidx_fanout_sort(&fanout);\n \n \t\t/*\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex a60cec5cab..4f3841661a 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -346,7 +346,7 @@ test_expect_success 'preferred pack change with existing MIDX bitmap' '\n \t\ttest_path_is_file $midx &&\n \t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap &&\n \n-\t\ttest_must_fail git clone --no-local . clone2\n+\t\tgit clone --no-local . clone2\n \t)\n '\n \n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"461660","messageId":"CAPOJW5zSGOY4zryCiMc02HW73nyzzKVopuHgPevgSjeKkt+6zg@mail.gmail.com","threadId":"58331","inReplyTo":"053045db1459812a1baec8771ff22dcac6f9ad47.1660944574.git.me@ttaylorr.com","subject":"Re: [PATCH 2/6] t/lib-bitmap.sh: avoid silencing stderr","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-08-20T16:44:03Z","receivedAt":"2022-08-20T16:44:20Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"On Sat, Aug 20, 2022 at 3:00 AM Taylor Blau wrote:\n>\n> The midx_bitmap_partial_tests() function is responsible for setting up a\n> state where some (but not all) packs in the repository are covered by a\n> MIDX (and bitmap).\n>\n> This function has redirected the `git multi-pack-index write --bitmap`'s\n> stderr to a file \"err\" since its introduction back in c51f5a6437 (t5326:\n> test multi-pack bitmap behavior, 2021-08-31).\n>\n> This was likely a stray change left over from a slightly different\n> version of this test, since the file \"err\" is never read after being\n> written. This leads to confusingly-missing output, especially when the\n> contents of stderr are important.\n>\n> Resolve this confusion by avoiding silencing stderr in this case.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> t/lib-bitmap.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/lib-bitmap.sh b/t/lib-bitmap.sh\n> index a95537e759..f595937094 100644\n> --- a/t/lib-bitmap.sh\n> +++ b/t/lib-bitmap.sh\n> @@ -440,7 +440,7 @@ midx_bitmap_partial_tests () {\n> test_commit packed &&\n> git repack &&\n> test_commit loose &&\n> - git multi-pack-index write --bitmap 2>err &&\n> + git multi-pack-index write --bitmap &&\n> test_path_is_file $midx &&\n> test_path_is_file $midx-$(midx_checksum $objdir).bitmap\n> '\n\nThanks Taylor! I would say this is a very good change. It might have\nbeen there for some reason when it was written, but that was resisting\nus to debug what was going on ;-)\n"},{"id":"461661","messageId":"CAPOJW5zmbQ966KXjaEvxk-oHu01BsxwszUTu3et4SYGFCAegCA@mail.gmail.com","threadId":"58331","inReplyTo":"4ddddc959b042faf7ae98a8e8eaa05e77f9ea23e.1660944574.git.me@ttaylorr.com","subject":"Re: [PATCH 6/6] midx.c: include preferred pack correctly with existing MIDX","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-08-20T18:40:42Z","receivedAt":"2022-08-20T18:40:59Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"On Sat, Aug 20, 2022 at 3:00 AM Taylor Blau wrote:\n>\n> + if (-1 < preferred_pack && preferred_pack < start_pack)\n> + midx_fanout_add_pack_fanout(&fanout, info,\n> + preferred_pack, 1,\n> + cur_fanout);\n> +\n\nAll the other changes make sense to me but I have a question about\nthis particular change. Instead of adding all the preferred objects\nagain (but in this case these are being added from preferred pack) in\n`fanout->entries`, will it be better if we call\n`midx_fanout_add_pack_fanout()` function from\n`midx_fanout_add_midx_fanout()` when above conditions are met?\nSomething like this -\n\n static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n struct multi_pack_index *m,\n struct pack_info *info,\n uint32_t cur_pack,\n int preferred,\n uint32_t cur_fanout)\n {\n ...\n if (cur_fanout)\n start = ntohl(m->chunk_oid_fanout[cur_fanout - 1]);\n end = ntohl(m->chunk_oid_fanout[cur_fanout]);\n if (preferred) {\n midx_fanout_add_pack_fanout(&fanout, info, cur_pack,\n\npreferred, cur_fanout);\n return;\n }\n\n for (.....) {\n ........\n }\n\nIt may reduce some extra code work but also could decrease readability\nof the function(may be). What's your thought here?\n\nThanks :)\n"},{"id":"461773","messageId":"3525f8bd-31af-181d-b7a5-6e8a453bbba7@github.com","threadId":"58331","inReplyTo":"3e30ab1a19115107fc24a25118f2417319bd1b0d.1660944574.git.me@ttaylorr.com","subject":"Re: [PATCH 1/6] t5326: demonstrate potential bitmap corruption","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-22T16:09:55Z","receivedAt":"2022-08-22T16:10:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/19/2022 5:30 PM, Taylor Blau wrote:\n\n> +test_expect_success 'preferred pack change with existing MIDX bitmap' '\n> +\trm -fr repo &&\n\nDoes a previous test not delete 'repo' when necessary?\n\nOr, do previous tests re-use 'repo' and now we are in a region\nwhere we can safely clear that directory? Should we use a\ndifferent name?\n\n> +\tgit init repo &&\n> +\ttest_when_finished \"rm -fr repo\" &&\n\nnit: test_when_finished should be the first line of the test.\n\n> +\t\t# Generate a new MIDX which changes the preferred pack to a pack\n> +\t\t# contained in the existing MIDX, such that not all objects from\n> +\t\t# p2 that appear in the MIDX had their copy selected from p2.\n> +\t\tgit multi-pack-index write --bitmap \\\n> +\t\t\t--preferred-pack=\"pack-$p2.pack\" &&\n> +\t\ttest_path_is_file $midx &&\n> +\t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap &&\n> +\n> +\t\ttest_must_fail git clone --no-local . clone2\n\nThis section is demonstrating the bug. Perhaps we should have\ncomments indicating that this is not desired behavior, but is\nbeing demonstrated so the bug can be fixed by a later change?\n\nThanks,\n-Stolee\n\n"},{"id":"461778","messageId":"3cecd058-aec2-d5f9-ef79-58cc10ce14fb@github.com","threadId":"58331","inReplyTo":"4ddddc959b042faf7ae98a8e8eaa05e77f9ea23e.1660944574.git.me@ttaylorr.com","subject":"Re: [PATCH 6/6] midx.c: include preferred pack correctly with existing MIDX","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-22T17:03:11Z","receivedAt":"2022-08-22T17:03:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/19/2022 5:30 PM, Taylor Blau wrote:\n\n> Resolve this by adding all objects from the preferred pack separately\n> when it appears in the existing MIDX (if one was present). This will\n> duplicate objects from that pack that *did* appear in the MIDX, but this\n> is fine, since get_sorted_entries() already handles duplicates. (A\n> future optimization in this area could avoid adding copies of objects\n> that we know already existing in the MIDX.)\n\n...\n\n> This resolves the bug described in the first patch of this series.\n\nThinking ahead to when this is a commit, perhaps this could instead\nrefer to the 'preferred pack change with existing MIDX bitmap' test\ncase?\n\n> @@ -610,10 +609,7 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n> \t\tnth_midxed_pack_midx_entry(m,\n> \t\t\t\t\t &fanout->entries[fanout->nr],\n> \t\t\t\t\t cur_object);\n> -\t\tif (nth_midxed_pack_int_id(m, cur_object) == preferred_pack)\n> -\t\t\tfanout->entries[fanout->nr].preferred = 1;\n> -\t\telse\n> -\t\t\tfanout->entries[fanout->nr].preferred = 0;\n> +\t\tfanout->entries[fanout->nr].preferred = 0;\n> \t\tfanout->nr++;\n\nHere, we have lost the ability to set the 'preferred' bit from the\nprevious MIDX. Good.\n\n> @@ -694,6 +689,11 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n> \t\t\t\t\t\t preferred, cur_fanout);\n> \t\t}\n> \n> +\t\tif (-1 < preferred_pack && preferred_pack < start_pack)\n> +\t\t\tmidx_fanout_add_pack_fanout(&fanout, info,\n> +\t\t\t\t\t\t preferred_pack, 1,\n> +\t\t\t\t\t\t cur_fanout);\n> +\n\nAnd here, when there is a preferred pack _in the previous MIDX_,\nwe add its objects a second time, but now with the preferred bit\non. If the preferred pack is _not_ in the previous MIDX, then the\n'preferred_pack < start_pack' condition will fail and the bits\nwould have been set within the for loop.\n\n> @@ -346,7 +346,7 @@ test_expect_success 'preferred pack change with existing MIDX bitmap' '\n> \t\ttest_path_is_file $midx &&\n> \t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap &&\n> \n> -\t\ttest_must_fail git clone --no-local . clone2\n> +\t\tgit clone --no-local . clone2\n\nI mentioned in patch 1 that this test could use some comments about\nwhat is unexpected and what _is_ expected. I think this comment needs\nan update in this patch:\n\n\t# Generate a new MIDX which changes the preferred pack to a pack\n\t# contained in the existing MIDX, such that not all objects from\n\t# p2 that appear in the MIDX had their copy selected from p2.\n\nThanks,\n-Stolee\n"},{"id":"461779","messageId":"6c146fa9-48da-5f74-c91a-29c54e1da6ce@github.com","threadId":"58331","inReplyTo":"cover.1660944574.git.me@ttaylorr.com","subject":"Re: [PATCH 0/6] midx: permit changing the preferred pack when reusing the MIDX","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-22T17:04:29Z","receivedAt":"2022-08-22T17:04:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/19/2022 5:30 PM, Taylor Blau wrote:\n> This series resolves a bug that was reported[1] by Johannes, and\n> investigated by him, Abhradeep, and Stolee in that same sub-thread.\n> \n> The crux of the issue is that a MIDX bitmap can enter a corrupt state\n> when changing the preferred pack from its value in an existing MIDX in\n> certain circumstances as described in the first and final patches.\n> \n> This series is structured as follows:\n> \n> - The first patch of this series adds a test which demonstrates the\n> problem. (This is an improvement from the debugging in [1], where we\n> only noticed the problem racily in an existing test, and only in\n> SHA-256 mode).\n> \n> - The next small handful of patches refactor midx.c's\n> `get_sorted_entries()` function to make fixing this bug more\n> straightforward.\n> \n> - The final patch resolves the bug and updates the test to no longer\n> expect failure.\n\nThanks for putting this together. Definitely not an easy bug to find\nand fix.\n\nI mostly have nitpicks, but the overall structure is sound.\n\nThanks,\n-Stolee\n"},{"id":"461780","messageId":"YwPDkW8KemC5Hs/C@nand.local","threadId":"58331","inReplyTo":"3525f8bd-31af-181d-b7a5-6e8a453bbba7@github.com","subject":"Re: [PATCH 1/6] t5326: demonstrate potential bitmap corruption","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T17:57:37Z","receivedAt":"2022-08-22T17:57:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 22, 2022 at 12:09:55PM -0400, Derrick Stolee wrote:\n> On 8/19/2022 5:30 PM, Taylor Blau wrote:\n>\n> > +test_expect_success 'preferred pack change with existing MIDX bitmap' '\n> > +\trm -fr repo &&\n>\n> Does a previous test not delete 'repo' when necessary?\n>\n> Or, do previous tests re-use 'repo' and now we are in a region\n> where we can safely clear that directory? Should we use a\n> different name?\n>\n> > +\tgit init repo &&\n> > +\ttest_when_finished \"rm -fr repo\" &&\n>\n> nit: test_when_finished should be the first line of the test.\n\nThe \"rm-then-init-then-test_when_finished\" is an (unfortunate) pattern\nextended throughout t5326, mostly that some tests don't clean up \"repo\"\nafter deleting and recreating it.\n\nBut it's easy enough to just use a separate repository, and avoid\nremoving it altogether. Thanks for the suggestion!\n\n> > +\t\t# Generate a new MIDX which changes the preferred pack to a pack\n> > +\t\t# contained in the existing MIDX, such that not all objects from\n> > +\t\t# p2 that appear in the MIDX had their copy selected from p2.\n> > +\t\tgit multi-pack-index write --bitmap \\\n> > +\t\t\t--preferred-pack=\"pack-$p2.pack\" &&\n> > +\t\ttest_path_is_file $midx &&\n> > +\t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap &&\n> > +\n> > +\t\ttest_must_fail git clone --no-local . clone2\n>\n> This section is demonstrating the bug. Perhaps we should have\n> comments indicating that this is not desired behavior, but is\n> being demonstrated so the bug can be fixed by a later change?\n\nYeah, good call. Thanks again!\n\nThanks,\nTaylor\n"},{"id":"461782","messageId":"YwPDu4GRp38ZgXXM@nand.local","threadId":"58331","inReplyTo":"CAPOJW5zSGOY4zryCiMc02HW73nyzzKVopuHgPevgSjeKkt+6zg@mail.gmail.com","subject":"Re: [PATCH 2/6] t/lib-bitmap.sh: avoid silencing stderr","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T17:58:19Z","receivedAt":"2022-08-22T17:58:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Aug 20, 2022 at 10:14:03PM +0530, Abhradeep Chakraborty wrote:\n> On Sat, Aug 20, 2022 at 3:00 AM Taylor Blau wrote:\n> > diff --git a/t/lib-bitmap.sh b/t/lib-bitmap.sh\n> > index a95537e759..f595937094 100644\n> > --- a/t/lib-bitmap.sh\n> > +++ b/t/lib-bitmap.sh\n> > @@ -440,7 +440,7 @@ midx_bitmap_partial_tests () {\n> > test_commit packed &&\n> > git repack &&\n> > test_commit loose &&\n> > - git multi-pack-index write --bitmap 2>err &&\n> > + git multi-pack-index write --bitmap &&\n> > test_path_is_file $midx &&\n> > test_path_is_file $midx-$(midx_checksum $objdir).bitmap\n> > '\n>\n> Thanks Taylor! I would say this is a very good change. It might have\n> been there for some reason when it was written, but that was resisting\n> us to debug what was going on ;-)\n\n:-). I was probably doing something with \"err\" when I originally wrote\nthis test. But I likely dropped whatever assertion I had written and\nforgot to stop redirecting stderr.\n\nBetter late than never ;-).\n\nThanks,\nTaylor\n"},{"id":"461783","messageId":"YwPGEvf210HyLnLy@nand.local","threadId":"58331","inReplyTo":"CAPOJW5zmbQ966KXjaEvxk-oHu01BsxwszUTu3et4SYGFCAegCA@mail.gmail.com","subject":"Re: [PATCH 6/6] midx.c: include preferred pack correctly with existing MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T18:08:18Z","receivedAt":"2022-08-22T18:08:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Aug 21, 2022 at 12:10:42AM +0530, Abhradeep Chakraborty wrote:\n> On Sat, Aug 20, 2022 at 3:00 AM Taylor Blau wrote:\n> >\n> > + if (-1 < preferred_pack && preferred_pack < start_pack)\n> > + midx_fanout_add_pack_fanout(&fanout, info,\n> > + preferred_pack, 1,\n> > + cur_fanout);\n> > +\n>\n> All the other changes make sense to me but I have a question about\n> this particular change. Instead of adding all the preferred objects\n> again (but in this case these are being added from preferred pack) in\n> `fanout->entries`, will it be better if we call\n> `midx_fanout_add_pack_fanout()` function from\n> `midx_fanout_add_midx_fanout()` when above conditions are met?\n> Something like this -\n>\n> static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n> struct multi_pack_index *m,\n> struct pack_info *info,\n> uint32_t cur_pack,\n> int preferred,\n> uint32_t cur_fanout)\n> {\n> ...\n> if (cur_fanout)\n> start = ntohl(m->chunk_oid_fanout[cur_fanout - 1]);\n> end = ntohl(m->chunk_oid_fanout[cur_fanout]);\n> if (preferred) {\n> midx_fanout_add_pack_fanout(&fanout, info, cur_pack,\n>\n> preferred, cur_fanout);\n> return;\n> }\n>\n> for (.....) {\n> ........\n> }\n\nA slightly simpler approach might be to see that the pack_midx_entry\nstructure we get back from calling nth_midxed_pack_midx_entry() contains\nthe pack from which each object is represented. So if we see one that\ncollides with the preferred pack, then we can just skip it, since we\nknow they'll be handled separately down below.\n\nI'll add another patch on top which makes that optimization.\n\nThanks,\nTaylor\n"},{"id":"461784","messageId":"YwPHh/EDIS0S4uoj@nand.local","threadId":"58331","inReplyTo":"3cecd058-aec2-d5f9-ef79-58cc10ce14fb@github.com","subject":"Re: [PATCH 6/6] midx.c: include preferred pack correctly with existing MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T18:14:31Z","receivedAt":"2022-08-22T18:14:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 22, 2022 at 01:03:11PM -0400, Derrick Stolee wrote:\n> On 8/19/2022 5:30 PM, Taylor Blau wrote:\n>\n> > Resolve this by adding all objects from the preferred pack separately\n> > when it appears in the existing MIDX (if one was present). This will\n> > duplicate objects from that pack that *did* appear in the MIDX, but this\n> > is fine, since get_sorted_entries() already handles duplicates. (A\n> > future optimization in this area could avoid adding copies of objects\n> > that we know already existing in the MIDX.)\n>\n> ...\n>\n> > This resolves the bug described in the first patch of this series.\n>\n> Thinking ahead to when this is a commit, perhaps this could instead\n> refer to the 'preferred pack change with existing MIDX bitmap' test\n> case?\n\nGood idea, thanks.\n\n> > @@ -610,10 +609,7 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n> > \t\tnth_midxed_pack_midx_entry(m,\n> > \t\t\t\t\t &fanout->entries[fanout->nr],\n> > \t\t\t\t\t cur_object);\n> > -\t\tif (nth_midxed_pack_int_id(m, cur_object) == preferred_pack)\n> > -\t\t\tfanout->entries[fanout->nr].preferred = 1;\n> > -\t\telse\n> > -\t\t\tfanout->entries[fanout->nr].preferred = 0;\n> > +\t\tfanout->entries[fanout->nr].preferred = 0;\n> > \t\tfanout->nr++;\n>\n> Here, we have lost the ability to set the 'preferred' bit from the\n> previous MIDX. Good.\n\nYep, we don't want to propagate any of these bits forward when reusing\nan existing MIDX. Thinking on it more, I think this is the only\nlegitimate use of MIDX reuse in the \"I'm about to write bitmaps\"\ncontext.\n\nI mentioned before the idea that we could use `--stdin-packs` now when\nwriting a MIDX bitmap where before it wasn't implemented (likely due to\nproblems caused by this bug). But the whole premise doesn't make a ton\nof sense:\n\n - Every pack that's in the include_packs list would need to be handled\n separately.\n\n - And every pack that *isn't* in that list would be skipped.\n\nWhich means that it wouldn't help at all to reuse an existing MIDX. The\nreason that we'd need to handle all included packs separately is subtle\nand a little different from what's going on here, though. The problem\nthere is that if you have two packs, say P1 and P2, and P1 is in the\ninclude list but P2 is not, then any objects duplicated between the two\nand selected from P2 will disappear when writing the new MIDX.\n\nSince the set of packs that are going into the new MIDX are precisely\nequal to the set of packs that we'd need to handle separately, it\nprobably makes sense to continue to avoid using the existing MIDX when\nwriting a bitmap with the `--stdin-packs` option.\n\n> > @@ -694,6 +689,11 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n> > \t\t\t\t\t\t preferred, cur_fanout);\n> > \t\t}\n> >\n> > +\t\tif (-1 < preferred_pack && preferred_pack < start_pack)\n> > +\t\t\tmidx_fanout_add_pack_fanout(&fanout, info,\n> > +\t\t\t\t\t\t preferred_pack, 1,\n> > +\t\t\t\t\t\t cur_fanout);\n> > +\n>\n> And here, when there is a preferred pack _in the previous MIDX_,\n> we add its objects a second time, but now with the preferred bit\n> on. If the preferred pack is _not_ in the previous MIDX, then the\n> 'preferred_pack < start_pack' condition will fail and the bits\n> would have been set within the for loop.\n\nExactly!\n\n> > @@ -346,7 +346,7 @@ test_expect_success 'preferred pack change with existing MIDX bitmap' '\n> > \t\ttest_path_is_file $midx &&\n> > \t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap &&\n> >\n> > -\t\ttest_must_fail git clone --no-local . clone2\n> > +\t\tgit clone --no-local . clone2\n>\n> I mentioned in patch 1 that this test could use some comments about\n> what is unexpected and what _is_ expected. I think this comment needs\n> an update in this patch:\n>\n> \t# Generate a new MIDX which changes the preferred pack to a pack\n> \t# contained in the existing MIDX, such that not all objects from\n> \t# p2 that appear in the MIDX had their copy selected from p2.\n\nGood eyes, thanks for spotting. I updated the comment below, too (which\ndoesn't exist in this version of the patch, but you suggested adding to\nthe first patch in this series).\n\nThanks,\nTaylor\n"},{"id":"461795","messageId":"xmqqbksccbxb.fsf@gitster.g","threadId":"58331","inReplyTo":"YwPDkW8KemC5Hs/C@nand.local","subject":"Re: [PATCH 1/6] t5326: demonstrate potential bitmap corruption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-22T19:31:44Z","receivedAt":"2022-08-22T19:32:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> > +\tgit init repo &&\n>> > +\ttest_when_finished \"rm -fr repo\" &&\n>>\n>> nit: test_when_finished should be the first line of the test.\n>\n> The \"rm-then-init-then-test_when_finished\" is an (unfortunate) pattern\n> extended throughout t5326, mostly that some tests don't clean up \"repo\"\n> after deleting and recreating it.\n\nI do not think it is so bad to be defensive to \"prepare it cleanly\nenough so that I would not be affected\". So \"rm -fr repo && git\ninit repo\" I would fully support. \"init && test_when_finished\" is\ntotally indefensible. It should be the other way around.\n\n> But it's easy enough to just use a separate repository, and avoid\n> removing it altogether. Thanks for the suggestion!\n\nThose who run tests in a batch without \"-i\" would have more material\nto study and find breakages if you did so. I agree that is probably\nsomething worth doing (unless in narrow corner cases where each test\nrepository consumes unusual amount of storage or somethinglike that).\n\n"},{"id":"461796","messageId":"YwPb7qamZGpdRpai@nand.local","threadId":"58331","inReplyTo":"xmqqbksccbxb.fsf@gitster.g","subject":"Re: [PATCH 1/6] t5326: demonstrate potential bitmap corruption","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T19:41:34Z","receivedAt":"2022-08-22T19:41:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 22, 2022 at 12:31:44PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> >> > +\tgit init repo &&\n> >> > +\ttest_when_finished \"rm -fr repo\" &&\n> >>\n> >> nit: test_when_finished should be the first line of the test.\n> >\n> > The \"rm-then-init-then-test_when_finished\" is an (unfortunate) pattern\n> > extended throughout t5326, mostly that some tests don't clean up \"repo\"\n> > after deleting and recreating it.\n>\n> I do not think it is so bad to be defensive to \"prepare it cleanly\n> enough so that I would not be affected\". So \"rm -fr repo && git\n> init repo\" I would fully support. \"init && test_when_finished\" is\n> totally indefensible. It should be the other way around.\n\nAgreed. We should fix those up, probably once Abhradeep's topic has\nlanded, since doing so now would create an avoidable amount of merge\nconflicts.\n\n> > But it's easy enough to just use a separate repository, and avoid\n> > removing it altogether. Thanks for the suggestion!\n>\n> Those who run tests in a batch without \"-i\" would have more material\n> to study and find breakages if you did so. I agree that is probably\n> something worth doing (unless in narrow corner cases where each test\n> repository consumes unusual amount of storage or somethinglike that).\n\nAgreed here, too. It's much handier to get a broken state immediately,\ninstead of realizing that a test was broken, commenting out the\n\"test_when_finished\" bits, and rerunning it again (not to mention hoping\nthat the failure is deterministic).\n\nLuckily these repositories are small in size, so it doesn't hurt to keep\nthem around.\n\nThanks,\nTaylor\n"},{"id":"461798","messageId":"YwPcgIStTEelWJfK@nand.local","threadId":"58331","inReplyTo":"6c146fa9-48da-5f74-c91a-29c54e1da6ce@github.com","subject":"Re: [PATCH 0/6] midx: permit changing the preferred pack when reusing the MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T19:44:00Z","receivedAt":"2022-08-22T19:44:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 22, 2022 at 01:04:29PM -0400, Derrick Stolee wrote:\n> Thanks for putting this together. Definitely not an easy bug to find\n> and fix.\n>\n> I mostly have nitpicks, but the overall structure is sound.\n\nThanks very much for reviewing (both to you and Abhradeep). I have a new\nversion that I'll send now, which incorporates your suggestions.\n\nIt also adds a new patch on top that takes a slight optimization, by\navoiding adding objects from the MIDX that show up in the (new)\npreferred pack, since we know we'll discard them anyway.\n\nThanks,\nTaylor\n"},{"id":"461799","messageId":"cover.1661197803.git.me@ttaylorr.com","threadId":"58331","inReplyTo":"cover.1660944574.git.me@ttaylorr.com","subject":"[PATCH v2 0/7] midx: permit changing the preferred pack when reusing the MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T19:50:24Z","receivedAt":"2022-08-22T19:50:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a small reroll of my series that resolves a bug that was\nreported[1] by Johannes, and investigated by him, Abhradeep, and Stolee\nin that same sub-thread.\n\nAs before: the crux of the issue is that a MIDX bitmap can enter a\ncorrupt state when changing the preferred pack from its value in an\nexisting MIDX in certain circumstances as described in the first and\nfinal patches.\n\nThis version incorporates some cosmetic changes suggested by Stolee, and\nadds a new patch on top which avoids adding objects from the MIDX that\nwere represented by the (new) preferred pack, since we know we'll end up\ndiscarding those objects anyways. For convenience, a range-diff against\nv1 is included below.\n\nThanks again for your review!\n\n[1]: https://lore.kernel.org/git/p3r70610-8n52-s8q0-n641-onp4ps01330n@tzk.qr/\n\nTaylor Blau (7):\n t5326: demonstrate potential bitmap corruption\n t/lib-bitmap.sh: avoid silencing stderr\n midx.c: extract `struct midx_fanout`\n midx.c: extract `midx_fanout_add_midx_fanout()`\n midx.c: extract `midx_fanout_add_pack_fanout()`\n midx.c: include preferred pack correctly with existing MIDX\n midx.c: avoid adding preferred objects twice\n\n midx.c | 139 +++++++++++++++++++++++-----------\n t/lib-bitmap.sh | 2 +-\n t/t5326-multi-pack-bitmaps.sh | 44 +++++++++++\n 3 files changed, 139 insertions(+), 46 deletions(-)\n\nRange-diff against v1:\n1: 3e30ab1a19 ! 1: 6b38bfcd2c t5326: demonstrate potential bitmap corruption\n @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'graceful fallback when missi\n '\n \n +test_expect_success 'preferred pack change with existing MIDX bitmap' '\n -+\trm -fr repo &&\n -+\tgit init repo &&\n -+\ttest_when_finished \"rm -fr repo\" &&\n ++\tgit init preferred-pack-with-existing &&\n +\t(\n -+\t\tcd repo &&\n ++\t\tcd preferred-pack-with-existing &&\n +\n +\t\ttest_commit base &&\n +\t\ttest_commit other &&\n @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'graceful fallback when missi\n +\t\tp2=\"$(git pack-objects \"$objdir/pack/pack\" \\\n +\t\t\t--delta-base-offset \n \n @@ midx.c: static struct pack_midx_entry *get_sorted_entries(struct multi_pack_inde\n \n ## t/t5326-multi-pack-bitmaps.sh ##\n @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'preferred pack change with existing MIDX bitmap' '\n + \t\tgit pack-objects --all --unpacked $objdir/pack/pack0 &&\n + \n + \t\t# Generate a new MIDX which changes the preferred pack\n +-\t\t# to a pack contained in the existing MIDX, such that\n +-\t\t# not all objects from p2 that appear in the MIDX had\n +-\t\t# their copy selected from p2.\n ++\t\t# to a pack contained in the existing MIDX.\n + \t\tgit multi-pack-index write --bitmap \\\n + \t\t\t--preferred-pack=\"pack-$p2.pack\" &&\n \t\ttest_path_is_file $midx &&\n \t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap &&\n \n +-\t\t# When the above circumstances are met, an existing bug\n +-\t\t# in the MIDX machinery will cause the reverse index to\n +-\t\t# be read incorrectly, resulting in failed clones (among\n +-\t\t# other things).\n -\t\ttest_must_fail git clone --no-local . clone2\n ++\t\t# When the above circumstances are met, the preferred\n ++\t\t# pack should change appropriately and clones should\n ++\t\t# (still) succeed.\n +\t\tgit clone --no-local . clone2\n \t)\n '\n-: ---------- > 7: 887ab9485f midx.c: avoid adding preferred objects twice\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"461800","messageId":"6b38bfcd2c2bced350cea198f7d576ffb81f3481.1661197803.git.me@ttaylorr.com","threadId":"58331","inReplyTo":"cover.1661197803.git.me@ttaylorr.com","subject":"[PATCH v2 1/7] t5326: demonstrate potential bitmap corruption","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T19:50:32Z","receivedAt":"2022-08-22T19:50:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"It is possible to generate a corrupt MIDX bitmap when certain conditions\nare met. This happens when the preferred pack \"P\" changes to one (say,\n\"Q\") that:\n\n - \"Q\" has objects included in an existing MIDX,\n - but \"Q\" is different than \"P\",\n - and \"Q\" and \"P\" have some objects in common\n\nWhen this is the case, not all objects from \"Q\" will be selected from\n\"Q\" (ie., the generated MIDX will represent them as coming from a\ndifferent pack), despite \"Q\" being preferred.\n\nThis is an invariant violation, since all objects contained in the\nMIDX's preferred pack are supposed to originate from the preferred pack.\nIn other words, all duplicate objects are resolved in favor of the copy\nthat comes from the MIDX's preferred pack, if any.\n\nThis violation results in a corrupt object order, which cannot be\ninterpreted by the pack-bitmap code, leading to broken clones and other\ndefects.\n\nThis test demonstrates the above problem by constructing a minimal\nreproduction, and showing that the final `git clone` invocation fails.\n\nThe reproduction is mostly straightforward, except that the new pack\ngenerated between MIDX writes (which is necessary in order to prevent\nthat operation from being a noop) must sort ahead of all existing packs\nin order to prevent a different pack (neither \"P\" nor \"Q\") from\nappearing as preferred (meaning all its objects appear in order at the\nbeginning of the pseudo-pack order).\n\nSubsequent commits will first refactor the midx.c::get_sorted_entries()\nfunction, and then fix this bug.\n\nReported-by: Abhradeep Chakraborty \nSigned-off-by: Taylor Blau \n---\n t/t5326-multi-pack-bitmaps.sh | 47 +++++++++++++++++++++++++++++++++++\n 1 file changed, 47 insertions(+)\n\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 4fe57414c1..c364677ae8 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -307,4 +307,51 @@ test_expect_success 'graceful fallback when missing reverse index' '\n \t)\n '\n \n+test_expect_success 'preferred pack change with existing MIDX bitmap' '\n+\tgit init preferred-pack-with-existing &&\n+\t(\n+\t\tcd preferred-pack-with-existing &&\n+\n+\t\ttest_commit base &&\n+\t\ttest_commit other &&\n+\n+\t\tgit rev-list --objects --no-object-names base >p1.objects &&\n+\t\tgit rev-list --objects --no-object-names other >p2.objects &&\n+\n+\t\tp1=\"$(git pack-objects \"$objdir/pack/pack\" \\\n+\t\t\t--delta-base-offset \n---\n t/lib-bitmap.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-bitmap.sh b/t/lib-bitmap.sh\nindex a95537e759..f595937094 100644\n--- a/t/lib-bitmap.sh\n+++ b/t/lib-bitmap.sh\n@@ -440,7 +440,7 @@ midx_bitmap_partial_tests () {\n \t\ttest_commit packed &&\n \t\tgit repack &&\n \t\ttest_commit loose &&\n-\t\tgit multi-pack-index write --bitmap 2>err &&\n+\t\tgit multi-pack-index write --bitmap &&\n \t\ttest_path_is_file $midx &&\n \t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap\n \t'\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"461802","messageId":"ae2077acb795311c87ce3bbcef60bffb66a6aa79.1661197803.git.me@ttaylorr.com","threadId":"58331","inReplyTo":"cover.1661197803.git.me@ttaylorr.com","subject":"[PATCH v2 3/7] midx.c: extract `struct midx_fanout`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T19:50:38Z","receivedAt":"2022-08-22T19:50:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To build up a list of objects (along with their packs, and the offsets\nwithin those packs that each object appears at), the MIDX code\nimplements `get_sorted_entries()` which builds up a list of candidates,\nsorts them, and then removes duplicate entries.\n\nTo do this, it keeps an array of `pack_midx_entry` structures that it\nbuilds up once for each fanout level (ie., for all possible values of\nthe first byte of each object's ID).\n\nThis array is a function-local variable of `get_sorted_entries()`. Since\nit uses the ALLOC_GROW() macro, having the `alloc_fanout` variable also\nbe local to that function, and only modified within that function is\nconvenient.\n\nHowever, subsequent changes will extract the two ways this array is\nfilled (from a pack at some fanout value, and from an existing MIDX at\nsome fanout value) into separate functions. Instead of passing around\npointers to the entries array, along with `nr_fanout` and\n`alloc_fanout`, encapsulate these three into a structure instead. Then\npass around a pointer to this structure instead.\n\nThis patch does not yet extract the above two functions, but sets us up\nto begin doing so in the following commit. For now, the implementation\nof get_sorted_entries() is only modified to replace `entries_by_fanout`\nwith `fanout.entries`, `nr_fanout` with `fanout.nr`, and so on.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 54 +++++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 35 insertions(+), 19 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 4e956cacb7..cdb6c481c7 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -577,6 +577,22 @@ static void fill_pack_entry(uint32_t pack_int_id,\n \tentry->preferred = !!preferred;\n }\n \n+struct midx_fanout {\n+\tstruct pack_midx_entry *entries;\n+\tuint32_t nr;\n+\tuint32_t alloc;\n+};\n+\n+static void midx_fanout_grow(struct midx_fanout *fanout, uint32_t nr)\n+{\n+\tALLOC_GROW(fanout->entries, nr, fanout->alloc);\n+}\n+\n+static void midx_fanout_sort(struct midx_fanout *fanout)\n+{\n+\tQSORT(fanout->entries, fanout->nr, midx_oid_compare);\n+}\n+\n /*\n * It is possible to artificially get into a state where there are many\n * duplicate copies of objects. That can create high memory pressure if\n@@ -595,8 +611,8 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\t\t\t\t int preferred_pack)\n {\n \tuint32_t cur_fanout, cur_pack, cur_object;\n-\tuint32_t alloc_fanout, alloc_objects, total_objects = 0;\n-\tstruct pack_midx_entry *entries_by_fanout = NULL;\n+\tuint32_t alloc_objects, total_objects = 0;\n+\tstruct midx_fanout fanout = { 0 };\n \tstruct pack_midx_entry *deduplicated_entries = NULL;\n \tuint32_t start_pack = m ? m->num_packs : 0;\n \n@@ -608,14 +624,14 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t * slices to be evenly distributed, with some noise. Hence,\n \t * allocate slightly more than one 256th.\n \t */\n-\talloc_objects = alloc_fanout = total_objects > 3200 ? total_objects / 200 : 16;\n+\talloc_objects = fanout.alloc = total_objects > 3200 ? total_objects / 200 : 16;\n \n-\tALLOC_ARRAY(entries_by_fanout, alloc_fanout);\n+\tALLOC_ARRAY(fanout.entries, fanout.alloc);\n \tALLOC_ARRAY(deduplicated_entries, alloc_objects);\n \t*nr_objects = 0;\n \n \tfor (cur_fanout = 0; cur_fanout < 256; cur_fanout++) {\n-\t\tuint32_t nr_fanout = 0;\n+\t\tfanout.nr = 0;\n \n \t\tif (m) {\n \t\t\tuint32_t start = 0, end;\n@@ -625,15 +641,15 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\tend = ntohl(m->chunk_oid_fanout[cur_fanout]);\n \n \t\t\tfor (cur_object = start; cur_object < end; cur_object++) {\n-\t\t\t\tALLOC_GROW(entries_by_fanout, nr_fanout + 1, alloc_fanout);\n+\t\t\t\tmidx_fanout_grow(&fanout, fanout.nr + 1);\n \t\t\t\tnth_midxed_pack_midx_entry(m,\n-\t\t\t\t\t\t\t &entries_by_fanout[nr_fanout],\n+\t\t\t\t\t\t\t &fanout.entries[fanout.nr],\n \t\t\t\t\t\t\t cur_object);\n \t\t\t\tif (nth_midxed_pack_int_id(m, cur_object) == preferred_pack)\n-\t\t\t\t\tentries_by_fanout[nr_fanout].preferred = 1;\n+\t\t\t\t\tfanout.entries[fanout.nr].preferred = 1;\n \t\t\t\telse\n-\t\t\t\t\tentries_by_fanout[nr_fanout].preferred = 0;\n-\t\t\t\tnr_fanout++;\n+\t\t\t\t\tfanout.entries[fanout.nr].preferred = 0;\n+\t\t\t\tfanout.nr++;\n \t\t\t}\n \t\t}\n \n@@ -646,36 +662,36 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\tend = get_pack_fanout(info[cur_pack].p, cur_fanout);\n \n \t\t\tfor (cur_object = start; cur_object < end; cur_object++) {\n-\t\t\t\tALLOC_GROW(entries_by_fanout, nr_fanout + 1, alloc_fanout);\n+\t\t\t\tmidx_fanout_grow(&fanout, fanout.nr + 1);\n \t\t\t\tfill_pack_entry(cur_pack,\n \t\t\t\t\t\tinfo[cur_pack].p,\n \t\t\t\t\t\tcur_object,\n-\t\t\t\t\t\t&entries_by_fanout[nr_fanout],\n+\t\t\t\t\t\t&fanout.entries[fanout.nr],\n \t\t\t\t\t\tpreferred);\n-\t\t\t\tnr_fanout++;\n+\t\t\t\tfanout.nr++;\n \t\t\t}\n \t\t}\n \n-\t\tQSORT(entries_by_fanout, nr_fanout, midx_oid_compare);\n+\t\tmidx_fanout_sort(&fanout);\n \n \t\t/*\n \t\t * The batch is now sorted by OID and then mtime (descending).\n \t\t * Take only the first duplicate.\n \t\t */\n-\t\tfor (cur_object = 0; cur_object < nr_fanout; cur_object++) {\n-\t\t\tif (cur_object && oideq(&entries_by_fanout[cur_object - 1].oid,\n-\t\t\t\t\t\t&entries_by_fanout[cur_object].oid))\n+\t\tfor (cur_object = 0; cur_object < fanout.nr; cur_object++) {\n+\t\t\tif (cur_object && oideq(&fanout.entries[cur_object - 1].oid,\n+\t\t\t\t\t\t&fanout.entries[cur_object].oid))\n \t\t\t\tcontinue;\n \n \t\t\tALLOC_GROW(deduplicated_entries, *nr_objects + 1, alloc_objects);\n \t\t\tmemcpy(&deduplicated_entries[*nr_objects],\n-\t\t\t &entries_by_fanout[cur_object],\n+\t\t\t &fanout.entries[cur_object],\n \t\t\t sizeof(struct pack_midx_entry));\n \t\t\t(*nr_objects)++;\n \t\t}\n \t}\n \n-\tfree(entries_by_fanout);\n+\tfree(fanout.entries);\n \treturn deduplicated_entries;\n }\n \n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"461803","messageId":"2351a9fc27140b99d9306eebcdb306df711f3c83.1661197803.git.me@ttaylorr.com","threadId":"58331","inReplyTo":"cover.1661197803.git.me@ttaylorr.com","subject":"[PATCH v2 4/7] midx.c: extract `midx_fanout_add_midx_fanout()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T19:50:41Z","receivedAt":"2022-08-22T19:50:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Extract a routine to add all objects whose object ID's first byte is\n`cur_fanout` from an existing MIDX.\n\nThis function will only be called once, so extracting it is purely\ncosmetic to improve the readability of `get_sorted_entries()` (its sole\ncaller) below.\n\nThe functionality is unchanged in this commit.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 47 ++++++++++++++++++++++++++++-------------------\n 1 file changed, 28 insertions(+), 19 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex cdb6c481c7..0d40089c4d 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -593,6 +593,31 @@ static void midx_fanout_sort(struct midx_fanout *fanout)\n \tQSORT(fanout->entries, fanout->nr, midx_oid_compare);\n }\n \n+static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n+\t\t\t\t\tstruct multi_pack_index *m,\n+\t\t\t\t\tint preferred_pack,\n+\t\t\t\t\tuint32_t cur_fanout)\n+{\n+\tuint32_t start = 0, end;\n+\tuint32_t cur_object;\n+\n+\tif (cur_fanout)\n+\t\tstart = ntohl(m->chunk_oid_fanout[cur_fanout - 1]);\n+\tend = ntohl(m->chunk_oid_fanout[cur_fanout]);\n+\n+\tfor (cur_object = start; cur_object < end; cur_object++) {\n+\t\tmidx_fanout_grow(fanout, fanout->nr + 1);\n+\t\tnth_midxed_pack_midx_entry(m,\n+\t\t\t\t\t &fanout->entries[fanout->nr],\n+\t\t\t\t\t cur_object);\n+\t\tif (nth_midxed_pack_int_id(m, cur_object) == preferred_pack)\n+\t\t\tfanout->entries[fanout->nr].preferred = 1;\n+\t\telse\n+\t\t\tfanout->entries[fanout->nr].preferred = 0;\n+\t\tfanout->nr++;\n+\t}\n+}\n+\n /*\n * It is possible to artificially get into a state where there are many\n * duplicate copies of objects. That can create high memory pressure if\n@@ -633,25 +658,9 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \tfor (cur_fanout = 0; cur_fanout < 256; cur_fanout++) {\n \t\tfanout.nr = 0;\n \n-\t\tif (m) {\n-\t\t\tuint32_t start = 0, end;\n-\n-\t\t\tif (cur_fanout)\n-\t\t\t\tstart = ntohl(m->chunk_oid_fanout[cur_fanout - 1]);\n-\t\t\tend = ntohl(m->chunk_oid_fanout[cur_fanout]);\n-\n-\t\t\tfor (cur_object = start; cur_object < end; cur_object++) {\n-\t\t\t\tmidx_fanout_grow(&fanout, fanout.nr + 1);\n-\t\t\t\tnth_midxed_pack_midx_entry(m,\n-\t\t\t\t\t\t\t &fanout.entries[fanout.nr],\n-\t\t\t\t\t\t\t cur_object);\n-\t\t\t\tif (nth_midxed_pack_int_id(m, cur_object) == preferred_pack)\n-\t\t\t\t\tfanout.entries[fanout.nr].preferred = 1;\n-\t\t\t\telse\n-\t\t\t\t\tfanout.entries[fanout.nr].preferred = 0;\n-\t\t\t\tfanout.nr++;\n-\t\t\t}\n-\t\t}\n+\t\tif (m)\n+\t\t\tmidx_fanout_add_midx_fanout(&fanout, m, preferred_pack,\n+\t\t\t\t\t\t cur_fanout);\n \n \t\tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++) {\n \t\t\tuint32_t start = 0, end;\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"461804","messageId":"845e1484b49f88bdd64faec2b7e61eddf2e3045c.1661197803.git.me@ttaylorr.com","threadId":"58331","inReplyTo":"cover.1661197803.git.me@ttaylorr.com","subject":"[PATCH v2 5/7] midx.c: extract `midx_fanout_add_pack_fanout()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T19:50:43Z","receivedAt":"2022-08-22T19:51:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Extract a routine to add all objects whose object ID's first byte is\n`cur_fanout` from a given pack (identified by its index into the `struct\npack_info` array maintained by the MIDX writing routine).\n\nUnlike the previous extraction (for `midx_fanout_add_midx_fanout()`),\nthis function will be called twice, once for all new packs, and again\nfor the preferred pack (if it appears in an existing MIDX). The latter\nchange is to resolve the bug described a few patches ago, and will be\nmade in the subsequent commit.\n\nSimilar to the previous refactoring, this function also enhances the\nreadability of its caller in `get_sorted_entries()`.\n\nIts functionality is unchanged in this commit.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 43 ++++++++++++++++++++++++++++---------------\n 1 file changed, 28 insertions(+), 15 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 0d40089c4d..be8186eec2 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -618,6 +618,31 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \t}\n }\n \n+static void midx_fanout_add_pack_fanout(struct midx_fanout *fanout,\n+\t\t\t\t\tstruct pack_info *info,\n+\t\t\t\t\tuint32_t cur_pack,\n+\t\t\t\t\tint preferred,\n+\t\t\t\t\tuint32_t cur_fanout)\n+{\n+\tstruct packed_git *pack = info[cur_pack].p;\n+\tuint32_t start = 0, end;\n+\tuint32_t cur_object;\n+\n+\tif (cur_fanout)\n+\t\tstart = get_pack_fanout(pack, cur_fanout - 1);\n+\tend = get_pack_fanout(pack, cur_fanout);\n+\n+\tfor (cur_object = start; cur_object < end; cur_object++) {\n+\t\tmidx_fanout_grow(fanout, fanout->nr + 1);\n+\t\tfill_pack_entry(cur_pack,\n+\t\t\t\tinfo[cur_pack].p,\n+\t\t\t\tcur_object,\n+\t\t\t\t&fanout->entries[fanout->nr],\n+\t\t\t\tpreferred);\n+\t\tfanout->nr++;\n+\t}\n+}\n+\n /*\n * It is possible to artificially get into a state where there are many\n * duplicate copies of objects. That can create high memory pressure if\n@@ -663,22 +688,10 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\t\t\t\t cur_fanout);\n \n \t\tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++) {\n-\t\t\tuint32_t start = 0, end;\n \t\t\tint preferred = cur_pack == preferred_pack;\n-\n-\t\t\tif (cur_fanout)\n-\t\t\t\tstart = get_pack_fanout(info[cur_pack].p, cur_fanout - 1);\n-\t\t\tend = get_pack_fanout(info[cur_pack].p, cur_fanout);\n-\n-\t\t\tfor (cur_object = start; cur_object < end; cur_object++) {\n-\t\t\t\tmidx_fanout_grow(&fanout, fanout.nr + 1);\n-\t\t\t\tfill_pack_entry(cur_pack,\n-\t\t\t\t\t\tinfo[cur_pack].p,\n-\t\t\t\t\t\tcur_object,\n-\t\t\t\t\t\t&fanout.entries[fanout.nr],\n-\t\t\t\t\t\tpreferred);\n-\t\t\t\tfanout.nr++;\n-\t\t\t}\n+\t\t\tmidx_fanout_add_pack_fanout(&fanout,\n+\t\t\t\t\t\t info, cur_pack,\n+\t\t\t\t\t\t preferred, cur_fanout);\n \t\t}\n \n \t\tmidx_fanout_sort(&fanout);\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"461805","messageId":"887ab9485faa21f5a5cd889d97895ed41013803d.1661197803.git.me@ttaylorr.com","threadId":"58331","inReplyTo":"cover.1661197803.git.me@ttaylorr.com","subject":"[PATCH v2 7/7] midx.c: avoid adding preferred objects twice","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T19:50:49Z","receivedAt":"2022-08-22T19:51:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The last commit changes the behavior of midx.c's `get_sorted_objects()`\nfunction to handle the case of writing a MIDX bitmap while reusing an\nexisting MIDX and changing the identity of the preferred pack\nseparately.\n\nAs part of this change, all objects from the (new) preferred pack are\nadded to the fanout table in a separate pass. Since these copies of the\nobjects all have their preferred bits set, any duplicates will be\nresolved in their favor.\n\nImportantly, this includes any copies of those same objects that come\nfrom the existing MIDX. We know at the time of adding them that they'll\nbe redundant if their source pack is the (new) preferred one, so we can\navoid adding them to the list in this case.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex bd1d27090e..148ecc2f14 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -595,7 +595,8 @@ static void midx_fanout_sort(struct midx_fanout *fanout)\n \n static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \t\t\t\t\tstruct multi_pack_index *m,\n-\t\t\t\t\tuint32_t cur_fanout)\n+\t\t\t\t\tuint32_t cur_fanout,\n+\t\t\t\t\tint preferred_pack)\n {\n \tuint32_t start = 0, end;\n \tuint32_t cur_object;\n@@ -605,6 +606,15 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \tend = ntohl(m->chunk_oid_fanout[cur_fanout]);\n \n \tfor (cur_object = start; cur_object < end; cur_object++) {\n+\t\tif ((preferred_pack > -1) &&\n+\t\t (preferred_pack == nth_midxed_pack_int_id(m, cur_object))) {\n+\t\t\t/*\n+\t\t\t * Objects from preferred packs are added\n+\t\t\t * separately.\n+\t\t\t */\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tmidx_fanout_grow(fanout, fanout->nr + 1);\n \t\tnth_midxed_pack_midx_entry(m,\n \t\t\t\t\t &fanout->entries[fanout->nr],\n@@ -680,7 +690,8 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\tfanout.nr = 0;\n \n \t\tif (m)\n-\t\t\tmidx_fanout_add_midx_fanout(&fanout, m, cur_fanout);\n+\t\t\tmidx_fanout_add_midx_fanout(&fanout, m, cur_fanout,\n+\t\t\t\t\t\t preferred_pack);\n \n \t\tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++) {\n \t\t\tint preferred = cur_pack == preferred_pack;\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"461806","messageId":"d301c4d87f8a490c90268aa251a94253f1e2b730.1661197803.git.me@ttaylorr.com","threadId":"58331","inReplyTo":"cover.1661197803.git.me@ttaylorr.com","subject":"[PATCH v2 6/7] midx.c: include preferred pack correctly with existing MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-08-22T19:50:46Z","receivedAt":"2022-08-22T19:51:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This patch resolves an issue where the object order used to generate a\nMIDX bitmap would violate an invariant that all of the preferred pack's\nobjects are represented by that pack in the MIDX.\n\nThe problem arises when reusing an existing MIDX while generating a new\none, and occurs specifically when the identity of the preferred pack\nchanges from one MIDX to another, along with a few other conditions:\n\n - the new preferred pack must also be present in the existing MIDX\n\n - the new preferred pack must *not* have been the preferred pack in\n the existing MIDX\n\n - most importantly, there must be at least one object present in the\n physical preferred pack (ie., it shows up in that pack's index)\n but was selected from a *different* pack when the previous MIDX\n was generated\n\nWhen the above conditions are all met, we end up (incorrectly)\ndiscarding copies of some objects in the pack selected as the preferred\npack. This is because `get_sorted_entries()` adds objects to its list\nby doing the following at each fanout level:\n\n - first, adding all objects from that fanout level from an existing\n MIDX\n\n - then, adding all objects from that fanout level in each pack *not*\n included in the existing MIDX\n\nSo if some object was not selected from the to-be-preferred pack when\nwriting the previous MIDX, then we will never consider it as a candidate\nwhen generating the new MIDX. This means that it's possible for the\npreferred pack to not include all of its objects in the MIDX's\npseudo-pack object order, which is an invariant violation of that order.\n\nResolve this by adding all objects from the preferred pack separately\nwhen it appears in the existing MIDX (if one was present). This will\nduplicate objects from that pack that *did* appear in the MIDX, but this\nis fine, since get_sorted_entries() already handles duplicates. (A\nfuture optimization in this area could avoid adding copies of objects\nthat we know already existing in the MIDX.)\n\nNote that we no longer need to compute the preferred-ness of objects\nadded from the MIDX, since we only want to select the preferred objects\nfrom a single source. (We could still mark these preferred bits, but\ndoing so is redundant and unnecessary).\n\nThis resolves the bug demonstrated by t5326.174 (\"preferred pack change\nwith existing MIDX bitmap\").\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 14 +++++++-------\n t/t5326-multi-pack-bitmaps.sh | 13 +++++--------\n 2 files changed, 12 insertions(+), 15 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex be8186eec2..bd1d27090e 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -595,7 +595,6 @@ static void midx_fanout_sort(struct midx_fanout *fanout)\n \n static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \t\t\t\t\tstruct multi_pack_index *m,\n-\t\t\t\t\tint preferred_pack,\n \t\t\t\t\tuint32_t cur_fanout)\n {\n \tuint32_t start = 0, end;\n@@ -610,10 +609,7 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \t\tnth_midxed_pack_midx_entry(m,\n \t\t\t\t\t &fanout->entries[fanout->nr],\n \t\t\t\t\t cur_object);\n-\t\tif (nth_midxed_pack_int_id(m, cur_object) == preferred_pack)\n-\t\t\tfanout->entries[fanout->nr].preferred = 1;\n-\t\telse\n-\t\t\tfanout->entries[fanout->nr].preferred = 0;\n+\t\tfanout->entries[fanout->nr].preferred = 0;\n \t\tfanout->nr++;\n \t}\n }\n@@ -684,8 +680,7 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\tfanout.nr = 0;\n \n \t\tif (m)\n-\t\t\tmidx_fanout_add_midx_fanout(&fanout, m, preferred_pack,\n-\t\t\t\t\t\t cur_fanout);\n+\t\t\tmidx_fanout_add_midx_fanout(&fanout, m, cur_fanout);\n \n \t\tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++) {\n \t\t\tint preferred = cur_pack == preferred_pack;\n@@ -694,6 +689,11 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\t\t\t\t preferred, cur_fanout);\n \t\t}\n \n+\t\tif (-1 < preferred_pack && preferred_pack < start_pack)\n+\t\t\tmidx_fanout_add_pack_fanout(&fanout, info,\n+\t\t\t\t\t\t preferred_pack, 1,\n+\t\t\t\t\t\t cur_fanout);\n+\n \t\tmidx_fanout_sort(&fanout);\n \n \t\t/*\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex c364677ae8..89ecd1062c 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -338,19 +338,16 @@ test_expect_success 'preferred pack change with existing MIDX bitmap' '\n \t\tgit pack-objects --all --unpacked $objdir/pack/pack0 &&\n \n \t\t# Generate a new MIDX which changes the preferred pack\n-\t\t# to a pack contained in the existing MIDX, such that\n-\t\t# not all objects from p2 that appear in the MIDX had\n-\t\t# their copy selected from p2.\n+\t\t# to a pack contained in the existing MIDX.\n \t\tgit multi-pack-index write --bitmap \\\n \t\t\t--preferred-pack=\"pack-$p2.pack\" &&\n \t\ttest_path_is_file $midx &&\n \t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap &&\n \n-\t\t# When the above circumstances are met, an existing bug\n-\t\t# in the MIDX machinery will cause the reverse index to\n-\t\t# be read incorrectly, resulting in failed clones (among\n-\t\t# other things).\n-\t\ttest_must_fail git clone --no-local . clone2\n+\t\t# When the above circumstances are met, the preferred\n+\t\t# pack should change appropriately and clones should\n+\t\t# (still) succeed.\n+\t\tgit clone --no-local . clone2\n \t)\n '\n \n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"461859","messageId":"98b4d348-e162-29f0-85e2-e36a9a500792@github.com","threadId":"58331","inReplyTo":"887ab9485faa21f5a5cd889d97895ed41013803d.1661197803.git.me@ttaylorr.com","subject":"Re: [PATCH v2 7/7] midx.c: avoid adding preferred objects twice","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-23T16:22:51Z","receivedAt":"2022-08-23T18:11:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/22/2022 3:50 PM, Taylor Blau wrote:\n> The last commit changes the behavior of midx.c's `get_sorted_objects()`\n> function to handle the case of writing a MIDX bitmap while reusing an\n> existing MIDX and changing the identity of the preferred pack\n> separately.\n> \n> As part of this change, all objects from the (new) preferred pack are\n> added to the fanout table in a separate pass. Since these copies of the\n> objects all have their preferred bits set, any duplicates will be\n> resolved in their favor.\n> \n> Importantly, this includes any copies of those same objects that come\n> from the existing MIDX. We know at the time of adding them that they'll\n> be redundant if their source pack is the (new) preferred one, so we can\n> avoid adding them to the list in this case.\n\nGood call to reduce memory requirements.\n\n> @@ -605,6 +606,15 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n> \tend = ntohl(m->chunk_oid_fanout[cur_fanout]);\n> \n> \tfor (cur_object = start; cur_object < end; cur_object++) {\n> +\t\tif ((preferred_pack > -1) &&\n> +\t\t (preferred_pack == nth_midxed_pack_int_id(m, cur_object))) {\n\nnit: you don't need the extra parentheses here.\n\nThanks,\n-Stolee\n"},{"id":"461860","messageId":"be9c7c72-ba24-3e1a-8428-58a2e2afa09a@github.com","threadId":"58331","inReplyTo":"cover.1661197803.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/7] midx: permit changing the preferred pack when reusing the MIDX","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-23T16:23:52Z","receivedAt":"2022-08-23T18:12:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/22/2022 3:50 PM, Taylor Blau wrote:\n> Here is a small reroll of my series that resolves a bug that was\n> reported[1] by Johannes, and investigated by him, Abhradeep, and Stolee\n> in that same sub-thread.\n> \n> As before: the crux of the issue is that a MIDX bitmap can enter a\n> corrupt state when changing the preferred pack from its value in an\n> existing MIDX in certain circumstances as described in the first and\n> final patches.\n> \n> This version incorporates some cosmetic changes suggested by Stolee, and\n> adds a new patch on top which avoids adding objects from the MIDX that\n> were represented by the (new) preferred pack, since we know we'll end up\n> discarding those objects anyways. For convenience, a range-diff against\n> v1 is included below.\n\nYou resolved the comments from the previous version well. I'm happy\nwith the changes.\n\nI have one style nit in the new patch, but it doesn't merit a re-roll\non its own.\n\nThanks,\n-Stolee\n"}]}