Here is a small reroll of my series that resolves a bug that was reported[1] by Johannes, and investigated by him, Abhradeep, and Stolee in that same sub-thread. As before: the crux of the issue is that a MIDX bitmap can enter a corrupt state when changing the preferred pack from its value in an existing MIDX in certain circumstances as described in the first and final patches. This version incorporates some cosmetic changes suggested by Stolee, and adds a new patch on top which avoids adding objects from the MIDX that were represented by the (new) preferred pack, since we know we'll end up discarding those objects anyways. For convenience, a range-diff against v1 is included below. Thanks again for your review! [1]: https://lore.kernel.org/git/p3r70610-8n52-s8q0-n641-onp4ps01330n@xxxxxx/ Taylor Blau (7): t5326: demonstrate potential bitmap corruption t/lib-bitmap.sh: avoid silencing stderr midx.c: extract `struct midx_fanout` midx.c: extract `midx_fanout_add_midx_fanout()` midx.c: extract `midx_fanout_add_pack_fanout()` midx.c: include preferred pack correctly with existing MIDX midx.c: avoid adding preferred objects twice midx.c | 139 +++++++++++++++++++++++----------- t/lib-bitmap.sh | 2 +- t/t5326-multi-pack-bitmaps.sh | 44 +++++++++++ 3 files changed, 139 insertions(+), 46 deletions(-) Range-diff against v1: 1: 3e30ab1a19 ! 1: 6b38bfcd2c t5326: demonstrate potential bitmap corruption @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'graceful fallback when missi ' +test_expect_success 'preferred pack change with existing MIDX bitmap' ' -+ rm -fr repo && -+ git init repo && -+ test_when_finished "rm -fr repo" && ++ git init preferred-pack-with-existing && + ( -+ cd repo && ++ cd preferred-pack-with-existing && + + test_commit base && + test_commit other && @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'graceful fallback when missi + p2="$(git pack-objects "$objdir/pack/pack" \ + --delta-base-offset <p2.objects)" && + -+ # Generate a MIDX containing the first two packs, marking p1 as -+ # preferred, and ensure that it can be successfully cloned. ++ # Generate a MIDX containing the first two packs, ++ # marking p1 as preferred, and ensure that it can be ++ # successfully cloned. + git multi-pack-index write --bitmap \ + --preferred-pack="pack-$p1.pack" && + test_path_is_file $midx && + test_path_is_file $midx-$(midx_checksum $objdir).bitmap && + git clone --no-local . clone1 && + -+ # Then generate a new pack which sorts ahead of any existing -+ # pack (by tweaking the pack prefix). ++ # Then generate a new pack which sorts ahead of any ++ # existing pack (by tweaking the pack prefix). + test_commit foo && + git pack-objects --all --unpacked $objdir/pack/pack0 && + -+ # Generate a new MIDX which changes the preferred pack to a pack -+ # contained in the existing MIDX, such that not all objects from -+ # p2 that appear in the MIDX had their copy selected from p2. ++ # Generate a new MIDX which changes the preferred pack ++ # to a pack contained in the existing MIDX, such that ++ # not all objects from p2 that appear in the MIDX had ++ # their copy selected from p2. + git multi-pack-index write --bitmap \ + --preferred-pack="pack-$p2.pack" && + test_path_is_file $midx && + test_path_is_file $midx-$(midx_checksum $objdir).bitmap && + ++ # When the above circumstances are met, an existing bug ++ # in the MIDX machinery will cause the reverse index to ++ # be read incorrectly, resulting in failed clones (among ++ # other things). + test_must_fail git clone --no-local . clone2 + ) +' 2: 053045db14 = 2: d6648ed88f t/lib-bitmap.sh: avoid silencing stderr 3: 2df8f1e884 = 3: ae2077acb7 midx.c: extract `struct midx_fanout` 4: 92b82c83ea = 4: 2351a9fc27 midx.c: extract `midx_fanout_add_midx_fanout()` 5: db1c6ea8e5 = 5: 845e1484b4 midx.c: extract `midx_fanout_add_pack_fanout()` 6: 4ddddc959b ! 6: d301c4d87f midx.c: include preferred pack correctly with existing MIDX @@ Commit message from a single source. (We could still mark these preferred bits, but doing so is redundant and unnecessary). - This resolves the bug described in the first patch of this series. + This resolves the bug demonstrated by t5326.174 ("preferred pack change + with existing MIDX bitmap"). Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx> @@ midx.c: static struct pack_midx_entry *get_sorted_entries(struct multi_pack_inde ## t/t5326-multi-pack-bitmaps.sh ## @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'preferred pack change with existing MIDX bitmap' ' + git pack-objects --all --unpacked $objdir/pack/pack0 && + + # Generate a new MIDX which changes the preferred pack +- # to a pack contained in the existing MIDX, such that +- # not all objects from p2 that appear in the MIDX had +- # their copy selected from p2. ++ # to a pack contained in the existing MIDX. + git multi-pack-index write --bitmap \ + --preferred-pack="pack-$p2.pack" && test_path_is_file $midx && test_path_is_file $midx-$(midx_checksum $objdir).bitmap && +- # When the above circumstances are met, an existing bug +- # in the MIDX machinery will cause the reverse index to +- # be read incorrectly, resulting in failed clones (among +- # other things). - test_must_fail git clone --no-local . clone2 ++ # When the above circumstances are met, the preferred ++ # pack should change appropriately and clones should ++ # (still) succeed. + git clone --no-local . clone2 ) ' -: ---------- > 7: 887ab9485f midx.c: avoid adding preferred objects twice -- 2.37.0.1.g1379af2e9d
