{"thread":{"id":"64597","subject":"[PATCH 0/2] builtin/repack: avoid rewriting up-to-date MIDX","startedAt":"2025-12-08T18:27:34Z","lastAt":"2025-12-19T06:26:04Z","messageCount":16,"participants":["Patrick Steinhardt","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"531851","messageId":"20251208-pks-skip-noop-rewrite-v1-0-430d52dba9f0@pks.im","threadId":"64597","inReplyTo":null,"subject":"[PATCH 0/2] builtin/repack: avoid rewriting up-to-date MIDX","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-08T18:27:13Z","receivedAt":"2025-12-08T18:27:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series introduces logic to avoid rewriting the\nmulti-pack index in case it's up-to-date already. This is especially\nrelevant in the context of geometric repacking, where we may decide to\nnot write any new packfiles, but we'd still rewrite the multi-pack\nindex.\n\nThis is a follow-up for the discussion that happened at [1].\n\nThanks!\n\nPatrick\n\n[1]: <20251025191550.GA279793@coredump.intra.peff.net>\n\n---\nPatrick Steinhardt (2):\n      midx: fix `BUG()` when getting preferred pack without a reverse index\n      builtin/repack: don't regenerate MIDX unless needed\n\n midx.c                      |  2 +-\n pack-revindex.h             |  3 +-\n repack-midx.c               | 90 +++++++++++++++++++++++++++++++++++++++++++++\n t/t5319-multi-pack-index.sh | 55 +++++++++++++++++++++++++++\n t/t7703-repack-geometric.sh | 80 ++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 228 insertions(+), 2 deletions(-)\n\n\n---\nbase-commit: bdc5341ff65278a3cc80b2e8a02a2f02aa1fac06\nchange-id: 20251208-pks-skip-noop-rewrite-38d7f01c79c5\n\n"},{"id":"531852","messageId":"20251208-pks-skip-noop-rewrite-v1-1-430d52dba9f0@pks.im","threadId":"64597","inReplyTo":"20251208-pks-skip-noop-rewrite-v1-0-430d52dba9f0@pks.im","subject":"[PATCH 1/2] midx: fix `BUG()` when getting preferred pack without a reverse index","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-08T18:27:14Z","receivedAt":"2025-12-08T18:27:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `midx_preferred_pack()` returns the preferred pack for a\ngiven multi-pack index. To compute the preferred pack we:\n\n  1. Look up the position of the first object indexed by the multi-pack\n     index.\n\n  2. Convert this position from pseudo-pack order into MIDX order.\n\n  3. We then look up pack that corresponds to this MIDX index.\n\nThis reliably returns the preferred pack given that all of its contained\nobjects will be up front in pseudo-pack order.\n\nThe second step that turns the pseudo-pack order into MIDX order\nrequires the reverse index though, which may not exist for example when\nthe MIDX does not have a bitmap. And in that case one may easily hit a\nbug:\n\n    BUG: ../pack-revindex.c:491: pack_pos_to_midx: reverse index not yet loaded\n\nIn theory, `midx_preferred_pack()` already knows to handle the case\nwhere no reverse index exists, as it calls `load_midx_revindex()` before\ncalling into `midx_preferred_pack()`. But we only check for negative\nreturn values there, even though the function returns a positive error\ncode in case the reverse index does not exist.\n\nFix the issue by testing for a non-zero return value instead, same as\nall the other callers of this function already do. While at it, document\nthe return value of `load_midx_revindex()`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n midx.c                      |  2 +-\n pack-revindex.h             |  3 ++-\n t/t5319-multi-pack-index.sh | 13 +++++++++++++\n 3 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 24e1e72175..b681b18fc1 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -686,7 +686,7 @@ int midx_preferred_pack(struct multi_pack_index *m, uint32_t *pack_int_id)\n {\n \tif (m->preferred_pack_idx == -1) {\n \t\tuint32_t midx_pos;\n-\t\tif (load_midx_revindex(m) < 0) {\n+\t\tif (load_midx_revindex(m)) {\n \t\t\tm->preferred_pack_idx = -2;\n \t\t\treturn -1;\n \t\t}\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex 422c2487ae..0042892091 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -72,7 +72,8 @@ int verify_pack_revindex(struct packed_git *p);\n  * multi-pack index by mmap-ing it and assigning pointers in the\n  * multi_pack_index to point at it.\n  *\n- * A negative number is returned on error.\n+ * A negative number is returned on error. A positive number is returned in\n+ * case the multi-pack-index does not have a reverse index.\n  */\n int load_midx_revindex(struct multi_pack_index *m);\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 93f319a4b2..9492a9737b 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -350,7 +350,20 @@ test_expect_success 'preferred pack from existing MIDX without bitmaps' '\n \t\t# the new MIDX\n \t\tgit multi-pack-index write --preferred-pack=pack-$pack.pack\n \t)\n+'\n \n+test_expect_success 'preferred pack cannot be determined without bitmap' '\n+\ttest_when_finished \"rm -fr preferred-can-be-queried\" &&\n+\tgit init preferred-can-be-queried &&\n+\t(\n+\t\tcd preferred-can-be-queried &&\n+\t\ttest_commit initial &&\n+\t\tgit repack -Adl --write-midx --no-write-bitmap-index &&\n+\t\ttest_must_fail test-tool read-midx --preferred-pack .git/objects 2>err &&\n+\t\ttest_grep \"could not determine MIDX preferred pack\" err &&\n+\t\tgit repack -Adl --write-midx --write-bitmap-index &&\n+\t\ttest-tool read-midx --preferred-pack .git/objects\n+\t)\n '\n \n test_expect_success 'verify multi-pack-index success' '\n\n-- \n2.52.0.270.g3f4935d65f.dirty\n\n"},{"id":"531853","messageId":"20251208-pks-skip-noop-rewrite-v1-2-430d52dba9f0@pks.im","threadId":"64597","inReplyTo":"20251208-pks-skip-noop-rewrite-v1-0-430d52dba9f0@pks.im","subject":"[PATCH 2/2] builtin/repack: don't regenerate MIDX unless needed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-08T18:27:15Z","receivedAt":"2025-12-08T18:27:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When calling `git repack --write-midx` we will unconditionally rewrite\nthe multi-pack index. This is of course expected in the case where the\nlayout of packfiles has changed. But when the layout of packfiles did\nnot change it's of course a potentially-large waste of time.\n\nWith our default maintenance strategy this isn't really that much of a\nproblem, as git-repack(1) would only be called by git-gc(1) in case we\nknow that the layout _will_ change. But that is changing with geometric\nrepacking, where it is the responsibility of git-repack(1) itself to\ndetermine whether or not any packs need to be merged together. So while\ngit-repack happily declares that there is \"Nothing new to pack.\", we\nend up rewriting the MIDX.\n\nThis issue can be demonstrated trivially with a benchmark in the Git\nrepository: executing `git repack --geometric=2 --write-midx -d` in the\nGit repository takes more than 3 seconds only to end up with the same\nmulti-pack index as we already had before.\n\nAddress this issue by introducing a new function that determines whether\na rewrite of the MIDX would cause any user-visible changes. This covers\nthe following cases:\n\n  - No multi-pack index exists at all.\n\n  - The user asked us to write a bitmap, and we don't have any.\n\n  - The request preferred pack is different than the one that we have.\n\n  - The packfiles covered by the MIDX are changing.\n\nOnly if any of these conditions trigger we decide to write a new MIDX.\nThis allows us to significantly reduce the time for repacks that end up\ndoing nothing:\n\n    Benchmark 1: git repack --geometric=2 --write-midx -d\n      Time (mean ± σ):      3.183 s ±  0.078 s    [User: 2.924 s, System: 0.219 s]\n      Range (min … max):    2.985 s …  3.260 s    10 runs\n\n    Benchmark 2: ./git repack --geometric=2 --write-midx -d\n      Time (mean ± σ):     102.5 ms ±   1.0 ms    [User: 89.3 ms, System: 12.7 ms]\n      Range (min … max):   101.3 ms … 105.3 ms    28 runs\n\n    Summary\n      ./git repack --geometric=2 --write-midx -d ran\n       31.06 ± 0.82 times faster than git repack --geometric=2 --write-midx -d\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n repack-midx.c               | 90 +++++++++++++++++++++++++++++++++++++++++++++\n t/t5319-multi-pack-index.sh | 42 +++++++++++++++++++++\n t/t7703-repack-geometric.sh | 80 ++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 212 insertions(+)\n\ndiff --git a/repack-midx.c b/repack-midx.c\nindex 74bdfa3a6e..4e47f7c713 100644\n--- a/repack-midx.c\n+++ b/repack-midx.c\n@@ -2,6 +2,7 @@\n #include \"repack.h\"\n #include \"hash.h\"\n #include \"hex.h\"\n+#include \"midx.h\"\n #include \"odb.h\"\n #include \"oidset.h\"\n #include \"pack-bitmap.h\"\n@@ -283,6 +284,92 @@ static void remove_redundant_bitmaps(struct string_list *include,\n \tstrbuf_release(&path);\n }\n \n+static bool midx_needs_update(struct repack_write_midx_opts *opts,\n+\t\t\t      struct string_list *include,\n+\t\t\t      struct packed_git *new_preferred)\n+{\n+\tstruct multi_pack_index *midx;\n+\tuint32_t preferred_pack_id;\n+\tbool needed = true;\n+\n+\t/*\n+\t * If there is no multi-pack index we obviously need to generate a new\n+\t * one.  Note that we cannot use `get_multi_pack_index()` here, as we\n+\t * might be operating with a closed object database.\n+\t */\n+\tmidx = load_multi_pack_index(opts->existing->repo->objects->sources);\n+\tif (!midx)\n+\t\tgoto out;\n+\n+\t/*\n+\t * If we're instructed to write a bitmap we need to verify whether we\n+\t * already got one.\n+\t */\n+\tif (opts->write_bitmaps) {\n+\t\tstruct bitmap_index *bitmap = prepare_midx_bitmap_git(midx);\n+\t\tbool bitmap_exists = bitmap && bitmap_is_midx(bitmap);\n+\t\tfree_bitmap_index(bitmap);\n+\t\tif (!bitmap_exists)\n+\t\t\tgoto out;\n+\t}\n+\n+\t/*\n+\t * If we're asked to generate the MIDX with a preferred pack we need to\n+\t * verify that the current prepared pack matches the desired one. We\n+\t * can only determine this in the case where we write bitmaps though,\n+\t * so we ignore this setting otherwise.\n+\t */\n+\tif (new_preferred && opts->write_bitmaps) {\n+\t\tstruct packed_git *old_preferred = NULL;\n+\n+\t\tif (!midx_preferred_pack(midx, &preferred_pack_id))\n+\t\t\told_preferred = nth_midxed_pack(midx, preferred_pack_id);\n+\n+\t\tif (!old_preferred)\n+\t\t\tgoto out;\n+\n+\t\tif (strcmp(pack_basename(new_preferred),\n+\t\t\t   pack_basename(old_preferred)))\n+\t\t\tgoto out;\n+\t }\n+\n+\t/*\n+\t * Otherwise, we need to verify that the packs that are about to be\n+\t * included in the MIDX matches the currently covered packs.\n+\t */\n+\tif (include->nr != opts->existing->midx_packs.nr)\n+\t\tgoto out;\n+\n+\tstring_list_sort(include);\n+\tstring_list_sort(&opts->existing->midx_packs);\n+\tfor (size_t i = 0; i < include->nr; i++) {\n+\t\tconst char *include_name = include->items[i].string;\n+\t\tconst char *existing_name = opts->existing->midx_packs.items[i].string;\n+\t\tsize_t include_len = strlen(include_name);\n+\t\tsize_t existing_len = strlen(existing_name);\n+\n+\t\t/*\n+\t\t * We track pack indices for the include list, but the\n+\t\t * packfiles themselves in the existing list. We thus need to\n+\t\t * strip these suffixes.\n+\t\t */\n+\t\tif (ends_with(include_name, \".idx\"))\n+\t\t\tinclude_len -= strlen(\".idx\");\n+\t\tif (ends_with(existing_name, \".pack\"))\n+\t\t\texisting_len -= strlen(\".pack\");\n+\t\tif (include_len != existing_len)\n+\t\t\tgoto out;\n+\t\tif (memcmp(include_name, existing_name, include_len))\n+\t\t\tgoto out;\n+\t}\n+\n+\tneeded = false;\n+\n+out:\n+\tclose_midx(midx);\n+\treturn needed;\n+}\n+\n int write_midx_included_packs(struct repack_write_midx_opts *opts)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n@@ -296,6 +383,9 @@ int write_midx_included_packs(struct repack_write_midx_opts *opts)\n \tif (!include.nr)\n \t\tgoto done;\n \n+\tif (!midx_needs_update(opts, &include, preferred))\n+\t\tgoto done;\n+\n \tcmd.in = -1;\n \tcmd.git_cmd = 1;\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 9492a9737b..4adf67385a 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -366,6 +366,48 @@ test_expect_success 'preferred pack cannot be determined without bitmap' '\n \t)\n '\n \n+test_midx_is_retained () {\n+\ttest-tool chmtime =0 .git/objects/pack/multi-pack-index &&\n+\tls -l .git/objects/pack/multi-pack-index >expect &&\n+\tgit repack \"$@\" &&\n+\tls -l .git/objects/pack/multi-pack-index >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_midx_is_rewritten () {\n+\ttest-tool chmtime =0 .git/objects/pack/multi-pack-index &&\n+\tls -l .git/objects/pack/multi-pack-index >expect &&\n+\tgit repack --write-midx \"$@\" &&\n+\tls -l .git/objects/pack/multi-pack-index >actual &&\n+\t! test_cmp expect actual\n+}\n+\n+test_expect_success 'up-to-date multi-pack-index is retained' '\n+\ttest_when_finished \"rm -fr midx-up-to-date\" &&\n+\tgit init midx-up-to-date &&\n+\t(\n+\t\tcd midx-up-to-date &&\n+\n+\t\t# Write the initial pack that contains the most objects. This\n+\t\t# will be the preferred pack.\n+\t\ttest_commit first &&\n+\t\ttest_commit second &&\n+\t\tgit repack -Ad --write-midx &&\n+\t\ttest_midx_is_retained -Ad &&\n+\n+\t\t# Writing a new bitmap index should cause us to regenerate the MIDX.\n+\t\ttest_midx_is_rewritten -Ad --write-bitmap-index &&\n+\t\ttest_midx_is_retained -Ad --write-bitmap-index &&\n+\n+\t\t# Ensure that writing a new packfile causes us to rewrite the index.\n+\t\ttest_commit incremental &&\n+\t\ttest_midx_is_rewritten -d &&\n+\t\ttest_midx_is_retained -d\n+\t)\n+'\n+\n+test_done\n+\n test_expect_success 'verify multi-pack-index success' '\n \tgit multi-pack-index verify --object-dir=$objdir\n '\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 9fc1626fbf..980599961c 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -287,6 +287,86 @@ test_expect_success '--geometric with pack.packSizeLimit' '\n \t)\n '\n \n+test_expect_success '--geometric --write-midx retains up-to-date MIDX without bitmap index' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit initial &&\n+\n+\t\ttest_path_is_missing .git/objects/pack/multi-pack-index &&\n+\t\tgit repack --geometric=2 --write-midx --no-write-bitmap-index &&\n+\t\ttest_path_is_file .git/objects/pack/multi-pack-index &&\n+\t\ttest-tool chmtime =0 .git/objects/pack/multi-pack-index &&\n+\n+\t\tls -l .git/objects/pack/ >expect &&\n+\t\tgit repack --geometric=2 --write-midx --no-write-bitmap-index &&\n+\t\tls -l .git/objects/pack/ >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--geometric --write-midx regenerates MIDX when preferred pack changes' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit first &&\n+\t\ttest_commit second &&\n+\t\ttest_commit third &&\n+\t\tgit repack --geometric=2 --write-midx --write-bitmap-index &&\n+\t\ttest_commit fourth &&\n+\t\tgit repack --geometric=2 --write-midx --write-bitmap-index &&\n+\n+\t\tls .git/objects/pack/*.pack >packs &&\n+\t\ttest_line_count = 2 packs &&\n+\t\tpreferred_pack=$(test-tool read-midx --preferred-pack .git/objects) &&\n+\t\tother_pack=$(ls .git/objects/pack/*.idx | grep -v \"$preferred_pack\") &&\n+\t\techo \"$preferred_pack\" &&\n+\t\techo \"$other_pack\" &&\n+\n+\t\t# Rewrite the multi-pack index with the current preferred pack.\n+\t\t# git-repack(1) should decide to _not_ repack the MIDX in that\n+\t\t# case. This is mostly a sanity check to verify that the reason\n+\t\t# for the repack really only is the changed preferred pack.\n+\t\trm -f .git/objects/pack/multi-pack-index* &&\n+\t\tgit multi-pack-index write --bitmap --preferred-pack=\"$preferred_pack\" &&\n+\t\ttest-tool chmtime =0 .git/objects/pack/multi-pack-index &&\n+\t\tls -l .git/objects/pack/ >expect &&\n+\t\tgit repack --geometric=2 --write-midx --write-bitmap-index &&\n+\t\tls -l .git/objects/pack/ >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# Rewrite the multi-pack index with a different preferred pack.\n+\t\t# This time around, git-repack(1) should decide to repack the\n+\t\t# MIDX to rectify the preferred pack.\n+\t\trm -f .git/objects/pack/multi-pack-index* &&\n+\t\tgit multi-pack-index write --bitmap --preferred-pack=\"$(basename \"$other_pack\")\" &&\n+\t\ttest-tool chmtime =0 .git/objects/pack/multi-pack-index &&\n+\t\tls -l .git/objects/pack/ >expect &&\n+\t\tgit repack --geometric=2 --write-midx --write-bitmap-index &&\n+\t\tls -l .git/objects/pack/ >actual &&\n+\t\t! test_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--geometric --write-midx retains up-to-date MIDX with bitmap index' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\ttest_commit -C repo initial &&\n+\n+\ttest_path_is_missing repo/.git/objects/pack/multi-pack-index &&\n+\tgit -C repo repack --geometric=2 --write-midx --write-bitmap-index &&\n+\ttest_path_is_file repo/.git/objects/pack/multi-pack-index &&\n+\ttest-tool chmtime =0 repo/.git/objects/pack/multi-pack-index &&\n+\n+\tls -l repo/.git/objects/pack/ >expect &&\n+\tgit -C repo repack --geometric=2 --write-midx --write-bitmap-index &&\n+\tls -l repo/.git/objects/pack/ >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--geometric --write-midx with packfiles in main and alternate ODB' '\n \ttest_when_finished \"rm -fr shared member\" &&\n \n\n-- \n2.52.0.270.g3f4935d65f.dirty\n\n"},{"id":"531938","messageId":"aTi9M/F0sZzK/usA@nand.local","threadId":"64597","inReplyTo":"20251208-pks-skip-noop-rewrite-v1-1-430d52dba9f0@pks.im","subject":"Re: [PATCH 1/2] midx: fix `BUG()` when getting preferred pack without a reverse index","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-12-10T00:22:11Z","receivedAt":"2025-12-10T00:22:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Dec 08, 2025 at 07:27:14PM +0100, Patrick Steinhardt wrote:\n> The function `midx_preferred_pack()` returns the preferred pack for a\n> given multi-pack index. To compute the preferred pack we:\n>\n>   1. Look up the position of the first object indexed by the multi-pack\n>      index.\n>\n>   2. Convert this position from pseudo-pack order into MIDX order.\n>\n>   3. We then look up pack that corresponds to this MIDX index.\n\nI think the implementation of midx_preferred_pack() works a little bit\ndifferently than is described here. I often get confused when working in\nthis area juggling between the various object/pack orderings in my head.\n\nmidx_preferred_pack() cares about converting from the first position in\npseudo-pack order back into MIDX object order. To do that, we convert\nthe pseudo-pack position into a MIDX one, and then lookup the pack that\nrepresents that object.\n\nEffectively we're doing something like:\n\n    uint32_t pseudo_pack_pos = m->num_objects_in_base;\n    uint32_t midx_pos = pack_pos_to_midx(m, pseudo_pack_pos);\n    uint32_t pack_int_id = nth_midxed_pack_int_id(m, midx_pos);\n\n> This reliably returns the preferred pack given that all of its contained\n> objects will be up front in pseudo-pack order.\n>\n> The second step that turns the pseudo-pack order into MIDX order\n> requires the reverse index though, which may not exist for example when\n> the MIDX does not have a bitmap. And in that case one may easily hit a\n> bug:\n>\n>     BUG: ../pack-revindex.c:491: pack_pos_to_midx: reverse index not yet loaded\n\nThat makes sense, we can't convert a pseudo-pack position into a\nMIDX-relative position without a reverse index to tell us where that bit\ngoes.\n\n> In theory, `midx_preferred_pack()` already knows to handle the case\n> where no reverse index exists, as it calls `load_midx_revindex()` before\n> calling into `midx_preferred_pack()`. [...]\n\nRight, it handles this case by returning -1 to indicate that it didn't\nhave enough information to determine the preferred pack.\n\n> [...] But we only check for negative\n> return values there, even though the function returns a positive error\n> code in case the reverse index does not exist.\n\nAh. It looks like that was changed in 5a6072f631d (fsck: validate .rev\nfile header, 2023-04-17), but it looks like the caller here did not\nlearn about that change. It may be worth mentioning that commit in your\npatch message.\n\nWhile reviewing, I wanted to make sure that there weren't any other\ncallers of load_midx_revindex() that were also missing this check. The\nreturn value of that function is propagated through the two expected\nfunctions:\n\n    $ git grep -p load_revindex_from_disk\n    pack-revindex.c=struct revindex_header {\n    pack-revindex.c:static int load_revindex_from_disk(const struct git_hash_algo *algo,\n    pack-revindex.c=int load_pack_revindex_from_disk(struct packed_git *p)\n    pack-revindex.c:        ret = load_revindex_from_disk(p->repo->hash_algo,\n    pack-revindex.c=int load_midx_revindex(struct multi_pack_index *m)\n    pack-revindex.c:        ret = load_revindex_from_disk(m->source->odb->repo->hash_algo,\n\n, and checking through the callers of those two functions, all are\nprepared to handle a >0 return value.\n\n> diff --git a/midx.c b/midx.c\n> index 24e1e72175..b681b18fc1 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -686,7 +686,7 @@ int midx_preferred_pack(struct multi_pack_index *m, uint32_t *pack_int_id)\n>  {\n>  \tif (m->preferred_pack_idx == -1) {\n>  \t\tuint32_t midx_pos;\n> -\t\tif (load_midx_revindex(m) < 0) {\n> +\t\tif (load_midx_revindex(m)) {\n>  \t\t\tm->preferred_pack_idx = -2;\n>  \t\t\treturn -1;\n>  \t\t}\n> diff --git a/pack-revindex.h b/pack-revindex.h\n> index 422c2487ae..0042892091 100644\n> --- a/pack-revindex.h\n> +++ b/pack-revindex.h\n> @@ -72,7 +72,8 @@ int verify_pack_revindex(struct packed_git *p);\n>   * multi-pack index by mmap-ing it and assigning pointers in the\n>   * multi_pack_index to point at it.\n>   *\n> - * A negative number is returned on error.\n> + * A negative number is returned on error. A positive number is returned in\n> + * case the multi-pack-index does not have a reverse index.\n\nMakes sense.\n\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 93f319a4b2..9492a9737b 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -350,7 +350,20 @@ test_expect_success 'preferred pack from existing MIDX without bitmaps' '\n>  \t\t# the new MIDX\n>  \t\tgit multi-pack-index write --preferred-pack=pack-$pack.pack\n>  \t)\n> +'\n>\n> +test_expect_success 'preferred pack cannot be determined without bitmap' '\n> +\ttest_when_finished \"rm -fr preferred-can-be-queried\" &&\n> +\tgit init preferred-can-be-queried &&\n> +\t(\n> +\t\tcd preferred-can-be-queried &&\n> +\t\ttest_commit initial &&\n> +\t\tgit repack -Adl --write-midx --no-write-bitmap-index &&\n> +\t\ttest_must_fail test-tool read-midx --preferred-pack .git/objects 2>err &&\n> +\t\ttest_grep \"could not determine MIDX preferred pack\" err &&\n\nLooks good. I think that it's fine to end the test here, since we have\nextensive coverage that we can determine the preferred pack when there\nis a MIDX and matching bitmap, but I definitely don't feel strongly\nabout it.\n\nThanks,\nTaylor\n"},{"id":"531941","messageId":"aTjfj45uFl/f3b4K@nand.local","threadId":"64597","inReplyTo":"20251208-pks-skip-noop-rewrite-v1-2-430d52dba9f0@pks.im","subject":"Re: [PATCH 2/2] builtin/repack: don't regenerate MIDX unless needed","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-12-10T02:48:47Z","receivedAt":"2025-12-10T02:48:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Dec 08, 2025 at 07:27:15PM +0100, Patrick Steinhardt wrote:\n> Address this issue by introducing a new function that determines whether\n> a rewrite of the MIDX would cause any user-visible changes. This covers\n> the following cases:\n>\n>   - No multi-pack index exists at all.\n>\n>   - The user asked us to write a bitmap, and we don't have any.\n>\n>   - The request preferred pack is different than the one that we have.\n>\n>   - The packfiles covered by the MIDX are changing.\n\nI can't think of any cases beyond the ones you listed here that would\nrequire us to regenerate the MIDX. One kind-of-exception here would be:\n\n    $ git repack [...] --write-midx --write-bitmap-index\n    $ git repack [...] --write-midx\n\nwhere the second repack would generate an identical MIDX, but does not\nwant to retain a bitmap. That case is already handled in the MIDX\nwriting code if you search for \"want_bitmap\".\n\nThat makes me wonder whether the repack layer is the most appropriate\none to handle this logic. It seems like write_midx_internal() would\nreasonably be able to detect whether or not the MIDX we have already is\nup-to-date with respect to the given input.\n\nI think that makes some things about your patch easier and other things\na little harder ;-).\n\n - On the \"easier\" front: while both the MIDX code and the portion of\n   the repack code that drives it receive the same set of packs to\n   include, the MIDX code already has the packs it would compare\n   in a standard format. That would avoid you having to handle ends_with\n   ends_with(include_name, \".idx\") and ends_with(existing_name, \".pack\")\n   as special cases, which would be nice.\n\n - On the \"harder\" front: when driving MIDX generation with the\n   '--stdin-packs' option, we *don't* load an existing MIDX ever since\n   0c5a62f14bc (midx-write.c: do not read existing MIDX with\n   `packs_to_include`, 2024-06-11).\n\nI don't think that \"harder\" one is a show-stopper, though. Commit\nt0c5a62f14bc has enough gory details around how we generate pack IDs and\nvarious subtle assumptions about how and when we load packs that I am\nvery hesitant to recommend changing it given its fragility (though we\nshould examine and harden any fragilities within midx-write.c, maybe\njust separately ;-)).\n\nSo I don't think that we should make that change ahead of this patch.\nWhile you can't rely on being able to read 'ctx.m', I think you could\nload the MIDX belonging to \"source\" ad-hoc after we have computed the\npacks to fill from the MIDX's perspective, which is right around where\nthat want_bitmap code lives.\n\nI suspect that that may simplify some of your patch, though please let\nme know if that turns out not to be the case.\n\n> Only if any of these conditions trigger we decide to write a new MIDX.\n> This allows us to significantly reduce the time for repacks that end up\n> doing nothing:\n>\n>     Benchmark 1: git repack --geometric=2 --write-midx -d\n>       Time (mean ± σ):      3.183 s ±  0.078 s    [User: 2.924 s, System: 0.219 s]\n>       Range (min … max):    2.985 s …  3.260 s    10 runs\n>\n>     Benchmark 2: ./git repack --geometric=2 --write-midx -d\n>       Time (mean ± σ):     102.5 ms ±   1.0 ms    [User: 89.3 ms, System: 12.7 ms]\n>       Range (min … max):   101.3 ms … 105.3 ms    28 runs\n>\n>     Summary\n>       ./git repack --geometric=2 --write-midx -d ran\n>        31.06 ± 0.82 times faster than git repack --geometric=2 --write-midx -d\n\nMakes sense since we're effectively timing just MIDX generation here.\n\n(I'll avoid reading midx_needs_update() too closely here in case you\nmake any changes based on the above. Glancing over it briefly, it all\nseemed reasonable to me.)\n\n> +test_expect_success 'up-to-date multi-pack-index is retained' '\n> +\ttest_when_finished \"rm -fr midx-up-to-date\" &&\n> +\tgit init midx-up-to-date &&\n> +\t(\n> +\t\tcd midx-up-to-date &&\n> +\n> +\t\t# Write the initial pack that contains the most objects. This\n> +\t\t# will be the preferred pack.\n> +\t\ttest_commit first &&\n> +\t\ttest_commit second &&\n> +\t\tgit repack -Ad --write-midx &&\n> +\t\ttest_midx_is_retained -Ad &&\n\nShould the MIDX be retained in this case? I think there is a reasonable\nargument to be made in either direction here.\n\nOn the one hand: we performed the expected repack, and doing so did not\ncause the existing MIDX to be invalid, so we left it alone. On the other\nhand: the caller asked us to repack without writing a MIDX, so could be\nsurprised that one exists.\n\n> diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\n> index 9fc1626fbf..980599961c 100755\n> --- a/t/t7703-repack-geometric.sh\n> +++ b/t/t7703-repack-geometric.sh\n> @@ -287,6 +287,86 @@ test_expect_success '--geometric with pack.packSizeLimit' '\n>  \t)\n>  '\n>\n> +test_expect_success '--geometric --write-midx retains up-to-date MIDX without bitmap index' '\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit initial &&\n> +\n> +\t\ttest_path_is_missing .git/objects/pack/multi-pack-index &&\n> +\t\tgit repack --geometric=2 --write-midx --no-write-bitmap-index &&\n> +\t\ttest_path_is_file .git/objects/pack/multi-pack-index &&\n> +\t\ttest-tool chmtime =0 .git/objects/pack/multi-pack-index &&\n\nThis portion is very similar to the new function added in t5319. I\nwonder if it's worth sticking that in t/lib-midx.sh or similar?\n\nI was wondering what this test was exercising that wasn't covered in the\nearlier script, but since the geometric code path is quite different\nfrom the non-geometric one, I think having coverage there is definitely\nworthwhile.\n\n> +test_expect_success '--geometric --write-midx regenerates MIDX when preferred pack changes' '\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\ttest_commit first &&\n> +\t\ttest_commit second &&\n> +\t\ttest_commit third &&\n> +\t\tgit repack --geometric=2 --write-midx --write-bitmap-index &&\n> +\t\ttest_commit fourth &&\n> +\t\tgit repack --geometric=2 --write-midx --write-bitmap-index &&\n\nThis part is a little subtle since we're relying on the geometric repack\nto pick up any loose objects that don't appear in existing pack(s). I\nmight suggest something like:\n\n    for c in first second third\n    do\n            test_commit $c || return 1\n    done &&\n    git repack -d && # ...\n\n, to make that step explicit, but I don't think it's a huge deal.\n\n> +\n> +\t\tls .git/objects/pack/*.pack >packs &&\n\nHere and below you should be able to use $packdir instead of writing out\n\".git/objects/pack\" explicitly.\n\n> +\t\ttest_line_count = 2 packs &&\n> +\t\tpreferred_pack=$(test-tool read-midx --preferred-pack .git/objects) &&\n> +\t\tother_pack=$(ls .git/objects/pack/*.idx | grep -v \"$preferred_pack\") &&\n> +\t\techo \"$preferred_pack\" &&\n> +\t\techo \"$other_pack\" &&\n\nStray debug output?\n> +\n> +\t\t# Rewrite the multi-pack index with the current preferred pack.\n> +\t\t# git-repack(1) should decide to _not_ repack the MIDX in that\n\ns/repack the MIDX/rewrite the MIDX/\n\n> +\t\t# case. This is mostly a sanity check to verify that the reason\n> +\t\t# for the repack really only is the changed preferred pack.\n> +\t\trm -f .git/objects/pack/multi-pack-index* &&\n\nSame note as above except here you can use $midx. I don't think you ever\nhave a .git/objects/pack/multi-pack-index.d here, so a standard removal\nshould do the trick. If you did, you would need to pass '-r' as well.\n\n> +\t\tgit multi-pack-index write --bitmap --preferred-pack=\"$preferred_pack\" &&\n> +\t\ttest-tool chmtime =0 .git/objects/pack/multi-pack-index &&\n> +\t\tls -l .git/objects/pack/ >expect &&\n> +\t\tgit repack --geometric=2 --write-midx --write-bitmap-index &&\n\nI'm having a little bit of a hard time following this one. Are we\nguaranteed to pick $preferred_pack as our preferred pack here in the\nsecond repack?\n\nThanks,\nTaylor\n"},{"id":"531951","messageId":"aTk_-pvNA31ScZyu@pks.im","threadId":"64597","inReplyTo":"aTi9M/F0sZzK/usA@nand.local","subject":"Re: [PATCH 1/2] midx: fix `BUG()` when getting preferred pack without a reverse index","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-10T09:40:10Z","receivedAt":"2025-12-10T09:40:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Dec 09, 2025 at 07:22:11PM -0500, Taylor Blau wrote:\n> On Mon, Dec 08, 2025 at 07:27:14PM +0100, Patrick Steinhardt wrote:\n> > The function `midx_preferred_pack()` returns the preferred pack for a\n> > given multi-pack index. To compute the preferred pack we:\n> >\n> >   1. Look up the position of the first object indexed by the multi-pack\n> >      index.\n> >\n> >   2. Convert this position from pseudo-pack order into MIDX order.\n> >\n> >   3. We then look up pack that corresponds to this MIDX index.\n> \n> I think the implementation of midx_preferred_pack() works a little bit\n> differently than is described here. I often get confused when working in\n> this area juggling between the various object/pack orderings in my head.\n\nHm, I feel like I am missing something.\n\n> midx_preferred_pack() cares about converting from the first position in\n> pseudo-pack order back into MIDX object order. To do that, we convert\n> the pseudo-pack position into a MIDX one, and then lookup the pack that\n> represents that object.\n\nIsn't that what I say in (2) and (3)? Or is this about (1) being\ninaccurate? Would this sequence be more accurate:\n\n  1. Take the first position indexed by the MIDX in pseudo-pack order.\n\n  2. Convert this pseudo-pack position into the MIDX position.\n\n  3. We then look up the pack that corresponds to this MIDX position.\n\nIn any case, I agree with you that juggling these different positions is\nquite something :)\n\n> > [...] But we only check for negative\n> > return values there, even though the function returns a positive error\n> > code in case the reverse index does not exist.\n> \n> Ah. It looks like that was changed in 5a6072f631d (fsck: validate .rev\n> file header, 2023-04-17), but it looks like the caller here did not\n> learn about that change. It may be worth mentioning that commit in your\n> patch message.\n\nThe caller was introduced at a later point though, via b1e3333068 (midx:\nimplement `midx_preferred_pack()`, 2023-12-14). So there wasn't really\nany overlap here where both topics were cooking at the same point in\ntime, at least not upstream. And the commit that changed the return\nvalue of `load_midx_revindex()` did update all callsites.\n\n> While reviewing, I wanted to make sure that there weren't any other\n> callers of load_midx_revindex() that were also missing this check. The\n> return value of that function is propagated through the two expected\n> functions:\n> \n>     $ git grep -p load_revindex_from_disk\n>     pack-revindex.c=struct revindex_header {\n>     pack-revindex.c:static int load_revindex_from_disk(const struct git_hash_algo *algo,\n>     pack-revindex.c=int load_pack_revindex_from_disk(struct packed_git *p)\n>     pack-revindex.c:        ret = load_revindex_from_disk(p->repo->hash_algo,\n>     pack-revindex.c=int load_midx_revindex(struct multi_pack_index *m)\n>     pack-revindex.c:        ret = load_revindex_from_disk(m->source->odb->repo->hash_algo,\n> \n> , and checking through the callers of those two functions, all are\n> prepared to handle a >0 return value.\n\nYup, thanks for double checking.\n\nPatrick\n"},{"id":"531952","messageId":"aTlABTCWRPNvUEGc@pks.im","threadId":"64597","inReplyTo":"aTjfj45uFl/f3b4K@nand.local","subject":"Re: [PATCH 2/2] builtin/repack: don't regenerate MIDX unless needed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-10T09:40:21Z","receivedAt":"2025-12-10T09:40:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Dec 09, 2025 at 09:48:47PM -0500, Taylor Blau wrote:\n> On Mon, Dec 08, 2025 at 07:27:15PM +0100, Patrick Steinhardt wrote:\n> > Address this issue by introducing a new function that determines whether\n> > a rewrite of the MIDX would cause any user-visible changes. This covers\n> > the following cases:\n> >\n> >   - No multi-pack index exists at all.\n> >\n> >   - The user asked us to write a bitmap, and we don't have any.\n> >\n> >   - The request preferred pack is different than the one that we have.\n> >\n> >   - The packfiles covered by the MIDX are changing.\n> \n> I can't think of any cases beyond the ones you listed here that would\n> require us to regenerate the MIDX. One kind-of-exception here would be:\n> \n>     $ git repack [...] --write-midx --write-bitmap-index\n>     $ git repack [...] --write-midx\n> \n> where the second repack would generate an identical MIDX, but does not\n> want to retain a bitmap. That case is already handled in the MIDX\n> writing code if you search for \"want_bitmap\".\n> \n> That makes me wonder whether the repack layer is the most appropriate\n> one to handle this logic. It seems like write_midx_internal() would\n> reasonably be able to detect whether or not the MIDX we have already is\n> up-to-date with respect to the given input.\n\nOne upside of having it in git-repack(1) is that we need to care about\nless situations in general as we are operating on a higher level. And\nbecause of that we can make more assumptions.\n\nThat being said, putting it into `write_midx_internal()` has the benefit\nthat we're of course covering more potential cases where we can avoid a\nneedless rewrite of the MIDX, and that we have better information to\ndecide whether it would be needed or not.\n\n> I think that makes some things about your patch easier and other things\n> a little harder ;-).\n> \n>  - On the \"easier\" front: while both the MIDX code and the portion of\n>    the repack code that drives it receive the same set of packs to\n>    include, the MIDX code already has the packs it would compare\n>    in a standard format. That would avoid you having to handle ends_with\n>    ends_with(include_name, \".idx\") and ends_with(existing_name, \".pack\")\n>    as special cases, which would be nice.\n> \n>  - On the \"harder\" front: when driving MIDX generation with the\n>    '--stdin-packs' option, we *don't* load an existing MIDX ever since\n>    0c5a62f14bc (midx-write.c: do not read existing MIDX with\n>    `packs_to_include`, 2024-06-11).\n> \n> I don't think that \"harder\" one is a show-stopper, though. Commit\n> t0c5a62f14bc has enough gory details around how we generate pack IDs and\n> various subtle assumptions about how and when we load packs that I am\n> very hesitant to recommend changing it given its fragility (though we\n> should examine and harden any fragilities within midx-write.c, maybe\n> just separately ;-)).\n> \n> So I don't think that we should make that change ahead of this patch.\n> While you can't rely on being able to read 'ctx.m', I think you could\n> load the MIDX belonging to \"source\" ad-hoc after we have computed the\n> packs to fill from the MIDX's perspective, which is right around where\n> that want_bitmap code lives.\n\nYeah, we're already loading the MIDX on-demand because in git-repack(1)\nas we have closed the object database at the point in time where we're\nabout to write the MIDX. So overall the change is rather easy to make.\n\nAlso, now that I see that we already have some short-circuiting\nconditions in git-multi-pack-index(1) I guess it makes sense to extend\nthose checks. They explicitly don't cover `--stdin-packs` rewrites right\nnow, so adding that check is an obvious improvement.\n\nOne thing I'm a bit torn on is whether or not to handle preferred packs\nin the adjusted logic. We don't do so right now either, so I think I'll\ndrop this for now. I can see an argument that us not handling a changed\npreferred pack is a bug though, in which case I'm happy to iterate.\n\nThanks for your feedback!\n\nPatrick\n"},{"id":"531967","messageId":"20251210-pks-skip-noop-rewrite-v2-0-f813a9e44f28@pks.im","threadId":"64597","inReplyTo":"20251208-pks-skip-noop-rewrite-v1-0-430d52dba9f0@pks.im","subject":"[PATCH v2 0/3] builtin/repack: avoid rewriting up-to-date MIDX","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-10T12:52:17Z","receivedAt":"2025-12-10T12:52:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series introduces logic to avoid rewriting the\nmulti-pack index in case it's up-to-date already. This is especially\nrelevant in the context of geometric repacking, where we may decide to\nnot write any new packfiles, but we'd still rewrite the multi-pack\nindex.\n\nThis is a follow-up for the discussion that happened at [1].\n\nChanges in v2:\n  - Move the logic to skip writing updates into `write_midx_internal()`.\n    We already had some logic there to skip no-op rewrites, so we only\n    extend that logic now to handle the \"--stdin-packs\" option. This\n    also has the added benefit that we know to strip bitmaps in case the\n    write is a no-op.\n  - I don't handle the case anymore where the preferred pack is\n    changing. We didn't do so in the preexisting checks either, so I\n    decided to drop this for now. This _can_ be considered as a bug, and\n    if anyone thinks it is then I'll extend these checks.\n  - Adapt the tests to use git-multi-pack-index(1) directly.\n  - Link to v1: https://lore.kernel.org/r/20251208-pks-skip-noop-rewrite-v1-0-430d52dba9f0@pks.im\n\nThanks!\n\nPatrick\n\n[1]: <20251025191550.GA279793@coredump.intra.peff.net>\n\n---\nPatrick Steinhardt (3):\n      midx: fix `BUG()` when getting preferred pack without a reverse index\n      midx-write: extract function to test whether MIDX needs updating\n      midx-write: skip rewriting MIDX with `--stdin-packs` unless needed\n\n midx-write.c                | 113 ++++++++++++++++++++++++++++++++++++--------\n midx.c                      |   2 +-\n pack-revindex.h             |   3 +-\n t/t5319-multi-pack-index.sh |  64 +++++++++++++++++++++++++\n t/t7703-repack-geometric.sh |  35 ++++++++++++++\n 5 files changed, 195 insertions(+), 22 deletions(-)\n\nRange-diff versus v1:\n\n1:  11258a799b ! 1:  88ba93d3a5 midx: fix `BUG()` when getting preferred pack without a reverse index\n    @@ Commit message\n         The function `midx_preferred_pack()` returns the preferred pack for a\n         given multi-pack index. To compute the preferred pack we:\n     \n    -      1. Look up the position of the first object indexed by the multi-pack\n    -         index.\n    +      1. Take the first position indexed by the MIDX in pseudo-pack order.\n     \n    -      2. Convert this position from pseudo-pack order into MIDX order.\n    +      2. Convert this pseudo-pack position into the MIDX position.\n     \n    -      3. We then look up pack that corresponds to this MIDX index.\n    +      3. We then look up the pack that corresponds to this MIDX position.\n     \n         This reliably returns the preferred pack given that all of its contained\n         objects will be up front in pseudo-pack order.\n2:  baf307b521 < -:  ---------- builtin/repack: don't regenerate MIDX unless needed\n-:  ---------- > 2:  9bd320b2cf midx-write: extract function to test whether MIDX needs updating\n-:  ---------- > 3:  f594865c12 midx-write: skip rewriting MIDX with `--stdin-packs` unless needed\n\n---\nbase-commit: bdc5341ff65278a3cc80b2e8a02a2f02aa1fac06\nchange-id: 20251208-pks-skip-noop-rewrite-38d7f01c79c5\n\n"},{"id":"531968","messageId":"20251210-pks-skip-noop-rewrite-v2-1-f813a9e44f28@pks.im","threadId":"64597","inReplyTo":"20251210-pks-skip-noop-rewrite-v2-0-f813a9e44f28@pks.im","subject":"[PATCH v2 1/3] midx: fix `BUG()` when getting preferred pack without a reverse index","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-10T12:52:18Z","receivedAt":"2025-12-10T12:52:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `midx_preferred_pack()` returns the preferred pack for a\ngiven multi-pack index. To compute the preferred pack we:\n\n  1. Take the first position indexed by the MIDX in pseudo-pack order.\n\n  2. Convert this pseudo-pack position into the MIDX position.\n\n  3. We then look up the pack that corresponds to this MIDX position.\n\nThis reliably returns the preferred pack given that all of its contained\nobjects will be up front in pseudo-pack order.\n\nThe second step that turns the pseudo-pack order into MIDX order\nrequires the reverse index though, which may not exist for example when\nthe MIDX does not have a bitmap. And in that case one may easily hit a\nbug:\n\n    BUG: ../pack-revindex.c:491: pack_pos_to_midx: reverse index not yet loaded\n\nIn theory, `midx_preferred_pack()` already knows to handle the case\nwhere no reverse index exists, as it calls `load_midx_revindex()` before\ncalling into `midx_preferred_pack()`. But we only check for negative\nreturn values there, even though the function returns a positive error\ncode in case the reverse index does not exist.\n\nFix the issue by testing for a non-zero return value instead, same as\nall the other callers of this function already do. While at it, document\nthe return value of `load_midx_revindex()`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n midx.c                      |  2 +-\n pack-revindex.h             |  3 ++-\n t/t5319-multi-pack-index.sh | 13 +++++++++++++\n 3 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 24e1e72175..b681b18fc1 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -686,7 +686,7 @@ int midx_preferred_pack(struct multi_pack_index *m, uint32_t *pack_int_id)\n {\n \tif (m->preferred_pack_idx == -1) {\n \t\tuint32_t midx_pos;\n-\t\tif (load_midx_revindex(m) < 0) {\n+\t\tif (load_midx_revindex(m)) {\n \t\t\tm->preferred_pack_idx = -2;\n \t\t\treturn -1;\n \t\t}\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex 422c2487ae..0042892091 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -72,7 +72,8 @@ int verify_pack_revindex(struct packed_git *p);\n  * multi-pack index by mmap-ing it and assigning pointers in the\n  * multi_pack_index to point at it.\n  *\n- * A negative number is returned on error.\n+ * A negative number is returned on error. A positive number is returned in\n+ * case the multi-pack-index does not have a reverse index.\n  */\n int load_midx_revindex(struct multi_pack_index *m);\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 93f319a4b2..9492a9737b 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -350,7 +350,20 @@ test_expect_success 'preferred pack from existing MIDX without bitmaps' '\n \t\t# the new MIDX\n \t\tgit multi-pack-index write --preferred-pack=pack-$pack.pack\n \t)\n+'\n \n+test_expect_success 'preferred pack cannot be determined without bitmap' '\n+\ttest_when_finished \"rm -fr preferred-can-be-queried\" &&\n+\tgit init preferred-can-be-queried &&\n+\t(\n+\t\tcd preferred-can-be-queried &&\n+\t\ttest_commit initial &&\n+\t\tgit repack -Adl --write-midx --no-write-bitmap-index &&\n+\t\ttest_must_fail test-tool read-midx --preferred-pack .git/objects 2>err &&\n+\t\ttest_grep \"could not determine MIDX preferred pack\" err &&\n+\t\tgit repack -Adl --write-midx --write-bitmap-index &&\n+\t\ttest-tool read-midx --preferred-pack .git/objects\n+\t)\n '\n \n test_expect_success 'verify multi-pack-index success' '\n\n-- \n2.52.0.270.g3f4935d65f.dirty\n\n"},{"id":"531969","messageId":"20251210-pks-skip-noop-rewrite-v2-2-f813a9e44f28@pks.im","threadId":"64597","inReplyTo":"20251210-pks-skip-noop-rewrite-v2-0-f813a9e44f28@pks.im","subject":"[PATCH v2 2/3] midx-write: extract function to test whether MIDX needs updating","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-10T12:52:19Z","receivedAt":"2025-12-10T12:52:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `write_midx_internal()` we know to skip writing the new multi-pack\nindex in case it would be the same as the existing one. This logic does\nnot handle the `--stdin-packs` option yet though, so we end up always\nrewriting the MIDX if that option is passed to us.\n\nExtract the logic to decide whether or not to rewrite the MIDX into a\nseparate function. This will allow us to extend that feature in the next\ncommit to address the above issue.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n midx-write.c | 39 ++++++++++++++++++++++++++++++++++++---\n 1 file changed, 36 insertions(+), 3 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex e3e9be6d03..78bc8a65b8 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1014,6 +1014,41 @@ static void clear_midx_files(struct odb_source *source,\n \tstrbuf_release(&buf);\n }\n \n+static bool midx_needs_update(struct write_midx_context *ctx)\n+{\n+\tstruct multi_pack_index *midx = ctx->m;\n+\tbool needed = true;\n+\n+\t/*\n+\t * Ignore incremental updates for now. The assumption is that any\n+\t * incremental update would be either empty (in which case we will bail\n+\t * out later) or it would actually cover at least one new pack.\n+\t */\n+\tif (ctx->incremental)\n+\t\tgoto out;\n+\n+\t/*\n+\t * If there is no MIDX then either it doesn't exist, or we're doing a\n+\t * geometric repack. We cannot (yet) determine whether we need to\n+\t * update the multi-pack index in the second case.\n+\t */\n+\tif (!midx)\n+\t\tgoto out;\n+\n+\t/*\n+\t * Otherwise, we need to verify that the packs covered by the existing\n+\t * MIDX match the packs that we already have. This test is somewhat\n+\t * lenient and will be fixed.\n+\t */\n+\tif (ctx->nr != midx->num_packs + midx->num_packs_in_base)\n+\t\tgoto out;\n+\n+\tneeded = false;\n+\n+out:\n+\treturn needed;\n+}\n+\n static int write_midx_internal(struct odb_source *source,\n \t\t\t       struct string_list *packs_to_include,\n \t\t\t       struct string_list *packs_to_drop,\n@@ -1111,9 +1146,7 @@ static int write_midx_internal(struct odb_source *source,\n \tfor_each_file_in_pack_dir(source->path, add_pack_to_midx, &ctx);\n \tstop_progress(&ctx.progress);\n \n-\tif ((ctx.m && ctx.nr == ctx.m->num_packs + ctx.m->num_packs_in_base) &&\n-\t    !ctx.incremental &&\n-\t    !(packs_to_include || packs_to_drop)) {\n+\tif (!packs_to_include && !packs_to_drop && !midx_needs_update(&ctx)) {\n \t\tstruct bitmap_index *bitmap_git;\n \t\tint bitmap_exists;\n \t\tint want_bitmap = flags & MIDX_WRITE_BITMAP;\n\n-- \n2.52.0.270.g3f4935d65f.dirty\n\n"},{"id":"531970","messageId":"20251210-pks-skip-noop-rewrite-v2-3-f813a9e44f28@pks.im","threadId":"64597","inReplyTo":"20251210-pks-skip-noop-rewrite-v2-0-f813a9e44f28@pks.im","subject":"[PATCH v2 3/3] midx-write: skip rewriting MIDX with `--stdin-packs` unless needed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-10T12:52:20Z","receivedAt":"2025-12-10T12:52:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `write_midx_internal()` we know to skip rewriting the multi-pack\nindex in case the existing one already covers all packs. This logic does\nnot know to handle `git multi-pack-index write --stdin-packs` though, so\nwe end up always rewriting the MIDX in this case even if the MIDX would\nnot change.\n\nWith our default maintenance strategy this isn't really much of a\nproblem, as git-gc(1) does not use the \"--stdin-packs\" option. But that\nis changing with geometric repacking, where \"--stdin-packs\" is used to\nexplicitly select the packfiles part of the geometric sequence.\n\nThis issue can be demonstrated trivially with a benchmark in the Git\nrepository: executing `git repack --geometric=2 --write-midx -d` in the\nGit repository takes more than 3 seconds only to end up with the same\nmulti-pack index as we already had before.\n\nThe logic that decides if we need to rewrite the MIDX only checks\nwhether the number of packfiles covered will change. That check is of\ncourse too lenient for \"--stdin-packs\", as it could happen that we want\nto cover a different-but-same-size set of packfiles. But there is no\ninherent reason why we cannot handle \"--stdin-packs\".\n\nImprove the logic to not only check for the number of packs, but to also\nverify that we are asked to generate a MIDX for the _same_ packs. This\nallows us to also skip no-op rewrites for \"--stdin-packs\".\n\nHelped-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n midx-write.c                | 100 +++++++++++++++++++++++++++++++-------------\n t/t5319-multi-pack-index.sh |  51 ++++++++++++++++++++++\n t/t7703-repack-geometric.sh |  35 ++++++++++++++++\n 3 files changed, 156 insertions(+), 30 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 78bc8a65b8..ce459b02c3 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1014,9 +1014,10 @@ static void clear_midx_files(struct odb_source *source,\n \tstrbuf_release(&buf);\n }\n \n-static bool midx_needs_update(struct write_midx_context *ctx)\n+static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_context *ctx)\n {\n-\tstruct multi_pack_index *midx = ctx->m;\n+\tstruct strset packs = STRSET_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tbool needed = true;\n \n \t/*\n@@ -1027,25 +1028,48 @@ static bool midx_needs_update(struct write_midx_context *ctx)\n \tif (ctx->incremental)\n \t\tgoto out;\n \n-\t/*\n-\t * If there is no MIDX then either it doesn't exist, or we're doing a\n-\t * geometric repack. We cannot (yet) determine whether we need to\n-\t * update the multi-pack index in the second case.\n-\t */\n-\tif (!midx)\n-\t\tgoto out;\n-\n \t/*\n \t * Otherwise, we need to verify that the packs covered by the existing\n-\t * MIDX match the packs that we already have. This test is somewhat\n-\t * lenient and will be fixed.\n+\t * MIDX match the packs that we already have. The logic to do so is way\n+\t * more complicated than it has any right to be. This is because:\n+\t *\n+\t *   - We cannot assume any ordering.\n+\t *\n+\t *   - The MIDX packs may not be loaded at all, and loading them would\n+\t *     be wasteful. So we need to use the pack names tracked by the\n+\t *     MIDX itself.\n+\t *\n+\t *   - The MIDX pack names are tracking the \".idx\" files, whereas the\n+\t *     packs themselves are tracking the \".pack\" files. So we need to\n+\t *     strip suffixes.\n \t */\n \tif (ctx->nr != midx->num_packs + midx->num_packs_in_base)\n \t\tgoto out;\n \n+\tfor (uint32_t i = 0; i < ctx->nr; i++) {\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addstr(&buf, pack_basename(ctx->info[i].p));\n+\t\tstrbuf_strip_suffix(&buf, \".pack\");\n+\n+\t\tif (!strset_add(&packs, buf.buf))\n+\t\t\tBUG(\"same pack added twice?\");\n+\t}\n+\n+\tfor (uint32_t i = 0; i < ctx->nr; i++) {\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addstr(&buf, midx->pack_names[i]);\n+\t\tstrbuf_strip_suffix(&buf, \".idx\");\n+\n+\t\tif (!strset_contains(&packs, buf.buf))\n+\t\t\tgoto out;\n+\t\tstrset_remove(&packs, buf.buf);\n+\t}\n+\n \tneeded = false;\n \n out:\n+\tstrbuf_release(&buf);\n+\tstrset_clear(&packs);\n \treturn needed;\n }\n \n@@ -1066,6 +1090,7 @@ static int write_midx_internal(struct odb_source *source,\n \tstruct write_midx_context ctx = {\n \t\t.preferred_pack_idx = NO_PREFERRED_PACK,\n \t };\n+\tstruct multi_pack_index *midx_to_free = NULL;\n \tint bitmapped_packs_concat_len = 0;\n \tint pack_name_concat_len = 0;\n \tint dropped_packs = 0;\n@@ -1146,25 +1171,39 @@ static int write_midx_internal(struct odb_source *source,\n \tfor_each_file_in_pack_dir(source->path, add_pack_to_midx, &ctx);\n \tstop_progress(&ctx.progress);\n \n-\tif (!packs_to_include && !packs_to_drop && !midx_needs_update(&ctx)) {\n-\t\tstruct bitmap_index *bitmap_git;\n-\t\tint bitmap_exists;\n-\t\tint want_bitmap = flags & MIDX_WRITE_BITMAP;\n-\n-\t\tbitmap_git = prepare_midx_bitmap_git(ctx.m);\n-\t\tbitmap_exists = bitmap_git && bitmap_is_midx(bitmap_git);\n-\t\tfree_bitmap_index(bitmap_git);\n-\n-\t\tif (bitmap_exists || !want_bitmap) {\n-\t\t\t/*\n-\t\t\t * The correct MIDX already exists, and so does a\n-\t\t\t * corresponding bitmap (or one wasn't requested).\n-\t\t\t */\n-\t\t\tif (!want_bitmap)\n-\t\t\t\tclear_midx_files_ext(source, \"bitmap\", NULL);\n-\t\t\tresult = 0;\n-\t\t\tgoto cleanup;\n+\tif (!packs_to_drop) {\n+\t\t/*\n+\t\t * If there is no MIDX then either it doesn't exist, or we're\n+\t\t * doing a geometric repack. Try to load it from the source to\n+\t\t * tell these two cases apart.\n+\t\t */\n+\t\tstruct multi_pack_index *midx = ctx.m;\n+\t\tif (!midx)\n+\t\t\tmidx = midx_to_free = load_multi_pack_index(ctx.source);\n+\n+\t\tif (midx && !midx_needs_update(midx, &ctx)) {\n+\t\t\tstruct bitmap_index *bitmap_git;\n+\t\t\tint bitmap_exists;\n+\t\t\tint want_bitmap = flags & MIDX_WRITE_BITMAP;\n+\n+\t\t\tbitmap_git = prepare_midx_bitmap_git(midx);\n+\t\t\tbitmap_exists = bitmap_git && bitmap_is_midx(bitmap_git);\n+\t\t\tfree_bitmap_index(bitmap_git);\n+\n+\t\t\tif (bitmap_exists || !want_bitmap) {\n+\t\t\t\t/*\n+\t\t\t\t * The correct MIDX already exists, and so does a\n+\t\t\t\t * corresponding bitmap (or one wasn't requested).\n+\t\t\t\t */\n+\t\t\t\tif (!want_bitmap)\n+\t\t\t\t\tclear_midx_files_ext(source, \"bitmap\", NULL);\n+\t\t\t\tresult = 0;\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n \t\t}\n+\n+\t\tclose_midx(midx_to_free);\n+\t\tmidx_to_free = NULL;\n \t}\n \n \tif (ctx.incremental && !ctx.nr) {\n@@ -1520,6 +1559,7 @@ static int write_midx_internal(struct odb_source *source,\n \t\tfree(keep_hashes);\n \t}\n \tstrbuf_release(&midx_name);\n+\tclose_midx(midx_to_free);\n \n \ttrace2_region_leave(\"midx\", \"write_midx_internal\", r);\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 9492a9737b..794f8b5ab4 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -366,6 +366,57 @@ test_expect_success 'preferred pack cannot be determined without bitmap' '\n \t)\n '\n \n+test_midx_is_retained () {\n+\ttest-tool chmtime =0 .git/objects/pack/multi-pack-index &&\n+\tls -l .git/objects/pack/multi-pack-index >expect &&\n+\tgit multi-pack-index write \"$@\" &&\n+\tls -l .git/objects/pack/multi-pack-index >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_midx_is_rewritten () {\n+\ttest-tool chmtime =0 .git/objects/pack/multi-pack-index &&\n+\tls -l .git/objects/pack/multi-pack-index >expect &&\n+\tgit multi-pack-index write \"$@\" &&\n+\tls -l .git/objects/pack/multi-pack-index >actual &&\n+\t! test_cmp expect actual\n+}\n+\n+test_expect_success 'up-to-date multi-pack-index is retained' '\n+\ttest_when_finished \"rm -fr midx-up-to-date\" &&\n+\tgit init midx-up-to-date &&\n+\t(\n+\t\tcd midx-up-to-date &&\n+\n+\t\t# Write the initial pack that contains the most objects.\n+\t\ttest_commit first &&\n+\t\ttest_commit second &&\n+\t\tgit repack -Ad --write-midx &&\n+\t\ttest_midx_is_retained &&\n+\n+\t\t# Writing a new bitmap index should cause us to regenerate the MIDX.\n+\t\ttest_midx_is_rewritten --bitmap &&\n+\t\ttest_midx_is_retained --bitmap &&\n+\n+\t\t# Ensure that writing a new packfile causes us to rewrite the index.\n+\t\ttest_commit incremental &&\n+\t\tgit repack -d &&\n+\t\ttest_midx_is_rewritten &&\n+\t\ttest_midx_is_retained &&\n+\n+\t\tfor pack in .git/objects/pack/*.idx\n+\t\tdo\n+\t\t\tbasename \"$pack\" || exit 1\n+\t\tdone >stdin &&\n+\t\ttest_line_count = 2 stdin &&\n+\t\ttest_midx_is_retained --stdin-packs <stdin &&\n+\t\thead -n1 stdin >stdin.trimmed &&\n+\t\ttest_midx_is_rewritten --stdin-packs <stdin.trimmed\n+\t)\n+'\n+\n+test_done\n+\n test_expect_success 'verify multi-pack-index success' '\n \tgit multi-pack-index verify --object-dir=$objdir\n '\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 9fc1626fbf..98806cdb6f 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -287,6 +287,41 @@ test_expect_success '--geometric with pack.packSizeLimit' '\n \t)\n '\n \n+test_expect_success '--geometric --write-midx retains up-to-date MIDX without bitmap index' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit initial &&\n+\n+\t\ttest_path_is_missing .git/objects/pack/multi-pack-index &&\n+\t\tgit repack --geometric=2 --write-midx --no-write-bitmap-index &&\n+\t\ttest_path_is_file .git/objects/pack/multi-pack-index &&\n+\t\ttest-tool chmtime =0 .git/objects/pack/multi-pack-index &&\n+\n+\t\tls -l .git/objects/pack/ >expect &&\n+\t\tgit repack --geometric=2 --write-midx --no-write-bitmap-index &&\n+\t\tls -l .git/objects/pack/ >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--geometric --write-midx retains up-to-date MIDX with bitmap index' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\ttest_commit -C repo initial &&\n+\n+\ttest_path_is_missing repo/.git/objects/pack/multi-pack-index &&\n+\tgit -C repo repack --geometric=2 --write-midx --write-bitmap-index &&\n+\ttest_path_is_file repo/.git/objects/pack/multi-pack-index &&\n+\ttest-tool chmtime =0 repo/.git/objects/pack/multi-pack-index &&\n+\n+\tls -l repo/.git/objects/pack/ >expect &&\n+\tgit -C repo repack --geometric=2 --write-midx --write-bitmap-index &&\n+\tls -l repo/.git/objects/pack/ >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--geometric --write-midx with packfiles in main and alternate ODB' '\n \ttest_when_finished \"rm -fr shared member\" &&\n \n\n-- \n2.52.0.270.g3f4935d65f.dirty\n\n"},{"id":"532027","messageId":"xmqqsedhe78q.fsf@gitster.g","threadId":"64597","inReplyTo":"20251210-pks-skip-noop-rewrite-v2-0-f813a9e44f28@pks.im","subject":"Re: [PATCH v2 0/3] builtin/repack: avoid rewriting up-to-date MIDX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-12-11T08:46:13Z","receivedAt":"2025-12-11T08:46:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This and Taylor's incremental part 3.2 have a slight conflict in\nthat this topic factors away the logic to compute if we need\nrecomputing MIDX while the other one tweaks with yet another flag.\n\nMy tentative resolution in 'seen' looks like the attached.  Sanity\nchecking is very much appreciated.\n\nThanks.\n\ndiff --cc midx-write.c\nindex ce459b02c3,f2dbacef4c..66c125ccb0\n--- a/midx-write.c\n+++ b/midx-write.c\n@@@ -1014,73 -1131,30 +1131,89 @@@ static void clear_midx_files(struct odb\n  \tstrbuf_release(&buf);\n  }\n  \n +static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_context *ctx)\n +{\n +\tstruct strset packs = STRSET_INIT;\n +\tstruct strbuf buf = STRBUF_INIT;\n +\tbool needed = true;\n +\n +\t/*\n +\t * Ignore incremental updates for now. The assumption is that any\n +\t * incremental update would be either empty (in which case we will bail\n +\t * out later) or it would actually cover at least one new pack.\n +\t */\n- \tif (ctx->incremental)\n++\tif (ctx->incremental || ctx->compact)\n +\t\tgoto out;\n +\n +\t/*\n +\t * Otherwise, we need to verify that the packs covered by the existing\n +\t * MIDX match the packs that we already have. The logic to do so is way\n +\t * more complicated than it has any right to be. This is because:\n +\t *\n +\t *   - We cannot assume any ordering.\n +\t *\n +\t *   - The MIDX packs may not be loaded at all, and loading them would\n +\t *     be wasteful. So we need to use the pack names tracked by the\n +\t *     MIDX itself.\n +\t *\n +\t *   - The MIDX pack names are tracking the \".idx\" files, whereas the\n +\t *     packs themselves are tracking the \".pack\" files. So we need to\n +\t *     strip suffixes.\n +\t */\n +\tif (ctx->nr != midx->num_packs + midx->num_packs_in_base)\n +\t\tgoto out;\n +\n +\tfor (uint32_t i = 0; i < ctx->nr; i++) {\n +\t\tstrbuf_reset(&buf);\n +\t\tstrbuf_addstr(&buf, pack_basename(ctx->info[i].p));\n +\t\tstrbuf_strip_suffix(&buf, \".pack\");\n +\n +\t\tif (!strset_add(&packs, buf.buf))\n +\t\t\tBUG(\"same pack added twice?\");\n +\t}\n +\n +\tfor (uint32_t i = 0; i < ctx->nr; i++) {\n +\t\tstrbuf_reset(&buf);\n +\t\tstrbuf_addstr(&buf, midx->pack_names[i]);\n +\t\tstrbuf_strip_suffix(&buf, \".idx\");\n +\n +\t\tif (!strset_contains(&packs, buf.buf))\n +\t\t\tgoto out;\n +\t\tstrset_remove(&packs, buf.buf);\n +\t}\n +\n +\tneeded = false;\n +\n +out:\n +\tstrbuf_release(&buf);\n +\tstrset_clear(&packs);\n +\treturn needed;\n +}\n +\n- static int write_midx_internal(struct odb_source *source,\n- \t\t\t       struct string_list *packs_to_include,\n- \t\t\t       struct string_list *packs_to_drop,\n- \t\t\t       const char *preferred_pack_name,\n- \t\t\t       const char *refs_snapshot,\n- \t\t\t       unsigned flags)\n+ static int midx_hashcmp(const struct multi_pack_index *a,\n+ \t\t\tconst struct multi_pack_index *b,\n+ \t\t\tconst struct git_hash_algo *algop)\n  {\n- \tstruct repository *r = source->odb->repo;\n+ \treturn hashcmp(get_midx_hash(a), get_midx_hash(b), algop);\n+ }\n+ \n+ struct write_midx_opts {\n+ \tstruct odb_source *source;\n+ \n+ \tstruct string_list *packs_to_include;\n+ \tstruct string_list *packs_to_drop;\n+ \n+ \tstruct multi_pack_index *compact_from;\n+ \tstruct multi_pack_index *compact_to;\n+ \n+ \tconst char *preferred_pack_name;\n+ \tconst char *refs_snapshot;\n+ \tunsigned flags;\n+ };\n+ \n+ static int write_midx_internal(struct write_midx_opts *opts)\n+ {\n+ \tstruct repository *r = opts->source->odb->repo;\n  \tstruct strbuf midx_name = STRBUF_INIT;\n  \tunsigned char midx_hash[GIT_MAX_RAWSZ];\n  \tuint32_t start_pack;\n@@@ -1166,44 -1257,43 +1317,54 @@@\n  \telse\n  \t\tctx.progress = NULL;\n  \n- \tctx.to_include = packs_to_include;\n+ \tif (ctx.compact) {\n+ \t\tint bitmap_order = 0;\n+ \t\tif (opts->preferred_pack_name)\n+ \t\t\tbitmap_order |= 1;\n+ \t\telse if (opts->flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))\n+ \t\t\tbitmap_order |= 1;\n  \n- \tfor_each_file_in_pack_dir(source->path, add_pack_to_midx, &ctx);\n+ \t\tfill_packs_from_midx_range(&ctx, bitmap_order);\n+ \t} else {\n+ \t\tctx.to_include = opts->packs_to_include;\n+ \t\tfor_each_file_in_pack_dir(opts->source->path, add_pack_to_midx, &ctx);\n+ \t}\n  \tstop_progress(&ctx.progress);\n  \n- \tif (!packs_to_drop) {\n -\tif ((ctx.m && ctx.nr == ctx.m->num_packs + ctx.m->num_packs_in_base) &&\n -\t    !ctx.incremental &&\n -\t    !ctx.compact &&\n -\t    !(opts->packs_to_include || opts->packs_to_drop)) {\n -\t\tstruct bitmap_index *bitmap_git;\n -\t\tint bitmap_exists;\n -\t\tint want_bitmap = opts->flags & MIDX_WRITE_BITMAP;\n -\n -\t\tbitmap_git = prepare_midx_bitmap_git(ctx.m);\n -\t\tbitmap_exists = bitmap_git && bitmap_is_midx(bitmap_git);\n -\t\tfree_bitmap_index(bitmap_git);\n -\n -\t\tif (bitmap_exists || !want_bitmap) {\n -\t\t\t/*\n -\t\t\t * The correct MIDX already exists, and so does a\n -\t\t\t * corresponding bitmap (or one wasn't requested).\n -\t\t\t */\n -\t\t\tif (!want_bitmap)\n -\t\t\t\tclear_midx_files_ext(opts->source, \"bitmap\",\n -\t\t\t\t\t\t     NULL);\n -\t\t\tresult = 0;\n -\t\t\tgoto cleanup;\n++\tif (!opts->packs_to_drop) {\n +\t\t/*\n +\t\t * If there is no MIDX then either it doesn't exist, or we're\n +\t\t * doing a geometric repack. Try to load it from the source to\n +\t\t * tell these two cases apart.\n +\t\t */\n +\t\tstruct multi_pack_index *midx = ctx.m;\n +\t\tif (!midx)\n +\t\t\tmidx = midx_to_free = load_multi_pack_index(ctx.source);\n +\n +\t\tif (midx && !midx_needs_update(midx, &ctx)) {\n +\t\t\tstruct bitmap_index *bitmap_git;\n +\t\t\tint bitmap_exists;\n- \t\t\tint want_bitmap = flags & MIDX_WRITE_BITMAP;\n++\t\t\tint want_bitmap = opts->flags & MIDX_WRITE_BITMAP;\n +\n +\t\t\tbitmap_git = prepare_midx_bitmap_git(midx);\n +\t\t\tbitmap_exists = bitmap_git && bitmap_is_midx(bitmap_git);\n +\t\t\tfree_bitmap_index(bitmap_git);\n +\n +\t\t\tif (bitmap_exists || !want_bitmap) {\n +\t\t\t\t/*\n +\t\t\t\t * The correct MIDX already exists, and so does a\n +\t\t\t\t * corresponding bitmap (or one wasn't requested).\n +\t\t\t\t */\n +\t\t\t\tif (!want_bitmap)\n- \t\t\t\t\tclear_midx_files_ext(source, \"bitmap\", NULL);\n++\t\t\t\t\tclear_midx_files_ext(opts->source,\n++\t\t\t\t\t\t\t     \"bitmap\", NULL);\n +\t\t\t\tresult = 0;\n +\t\t\t\tgoto cleanup;\n +\t\t\t}\n  \t\t}\n +\n +\t\tclose_midx(midx_to_free);\n +\t\tmidx_to_free = NULL;\n  \t}\n  \n  \tif (ctx.incremental && !ctx.nr) {\n"},{"id":"532060","messageId":"aTvFOlhtPHgWQC5L@pks.im","threadId":"64597","inReplyTo":"xmqqsedhe78q.fsf@gitster.g","subject":"Re: [PATCH v2 0/3] builtin/repack: avoid rewriting up-to-date MIDX","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-12T07:33:14Z","receivedAt":"2025-12-12T07:33:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 11, 2025 at 05:46:13PM +0900, Junio C Hamano wrote:\n> This and Taylor's incremental part 3.2 have a slight conflict in\n> that this topic factors away the logic to compute if we need\n> recomputing MIDX while the other one tweaks with yet another flag.\n> \n> My tentative resolution in 'seen' looks like the attached.  Sanity\n> checking is very much appreciated.\n> \n> Thanks.\n> \n> diff --cc midx-write.c\n> index ce459b02c3,f2dbacef4c..66c125ccb0\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@@ -1014,73 -1131,30 +1131,89 @@@ static void clear_midx_files(struct odb\n>   \tstrbuf_release(&buf);\n>   }\n>   \n>  +static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_context *ctx)\n>  +{\n>  +\tstruct strset packs = STRSET_INIT;\n>  +\tstruct strbuf buf = STRBUF_INIT;\n>  +\tbool needed = true;\n>  +\n>  +\t/*\n>  +\t * Ignore incremental updates for now. The assumption is that any\n>  +\t * incremental update would be either empty (in which case we will bail\n>  +\t * out later) or it would actually cover at least one new pack.\n>  +\t */\n> - \tif (ctx->incremental)\n> ++\tif (ctx->incremental || ctx->compact)\n>  +\t\tgoto out;\n\nSo this here is essentially the change you had to port over, which looks\nabout right to me. The comment is becoming somewhat stale due to the\nchange, but I don't think that's much of an issue for now.\n\nThanks!\n\nPatrick\n"},{"id":"532500","messageId":"aURufIXsiNfPh02X@nand.local","threadId":"64597","inReplyTo":"aTk_-pvNA31ScZyu@pks.im","subject":"Re: [PATCH 1/2] midx: fix `BUG()` when getting preferred pack without a reverse index","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-12-18T21:13:32Z","receivedAt":"2025-12-18T21:13:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Dec 10, 2025 at 10:40:10AM +0100, Patrick Steinhardt wrote:\n> On Tue, Dec 09, 2025 at 07:22:11PM -0500, Taylor Blau wrote:\n> > On Mon, Dec 08, 2025 at 07:27:14PM +0100, Patrick Steinhardt wrote:\n> > > The function `midx_preferred_pack()` returns the preferred pack for a\n> > > given multi-pack index. To compute the preferred pack we:\n> > >\n> > >   1. Look up the position of the first object indexed by the multi-pack\n> > >      index.\n> > >\n> > >   2. Convert this position from pseudo-pack order into MIDX order.\n> > >\n> > >   3. We then look up pack that corresponds to this MIDX index.\n> >\n> > I think the implementation of midx_preferred_pack() works a little bit\n> > differently than is described here. I often get confused when working in\n> > this area juggling between the various object/pack orderings in my head.\n>\n> Hm, I feel like I am missing something.\n>\n> > midx_preferred_pack() cares about converting from the first position in\n> > pseudo-pack order back into MIDX object order. To do that, we convert\n> > the pseudo-pack position into a MIDX one, and then lookup the pack that\n> > represents that object.\n>\n> Isn't that what I say in (2) and (3)? Or is this about (1) being\n> inaccurate? Would this sequence be more accurate:\n>\n>   1. Take the first position indexed by the MIDX in pseudo-pack order.\n>\n>   2. Convert this pseudo-pack position into the MIDX position.\n>\n>   3. We then look up the pack that corresponds to this MIDX position.\n>\n> In any case, I agree with you that juggling these different positions is\n> quite something :)\n\nI was thinking about (1) being inaccurate, but the rephrasing you\nprovided here and in the new version of the patch look good to me.\n\nBoth (2) and (3) from your original patch are correct. I was commenting\nhere on the phrasing in (1) suggesting we \"look up\" a position from the\nMIDX, which is inaccurate. The position is given as the first position\nin pseudo-pack order, and \"take the first position\" perfectly captures\nthat.\n\nThanks,\nTaylor\n"},{"id":"532501","messageId":"aURvuLcIVpBSIhiE@nand.local","threadId":"64597","inReplyTo":"aTvFOlhtPHgWQC5L@pks.im","subject":"Re: [PATCH v2 0/3] builtin/repack: avoid rewriting up-to-date MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-12-18T21:18:48Z","receivedAt":"2025-12-18T21:18:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Dec 12, 2025 at 08:33:14AM +0100, Patrick Steinhardt wrote:\n> On Thu, Dec 11, 2025 at 05:46:13PM +0900, Junio C Hamano wrote:\n> > This and Taylor's incremental part 3.2 have a slight conflict in\n> > that this topic factors away the logic to compute if we need\n> > recomputing MIDX while the other one tweaks with yet another flag.\n> >\n> > My tentative resolution in 'seen' looks like the attached.  Sanity\n> > checking is very much appreciated.\n> >\n> > Thanks.\n> >\n> > diff --cc midx-write.c\n> > index ce459b02c3,f2dbacef4c..66c125ccb0\n> > --- a/midx-write.c\n> > +++ b/midx-write.c\n> > @@@ -1014,73 -1131,30 +1131,89 @@@ static void clear_midx_files(struct odb\n> >   \tstrbuf_release(&buf);\n> >   }\n> >\n> >  +static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_context *ctx)\n> >  +{\n> >  +\tstruct strset packs = STRSET_INIT;\n> >  +\tstruct strbuf buf = STRBUF_INIT;\n> >  +\tbool needed = true;\n> >  +\n> >  +\t/*\n> >  +\t * Ignore incremental updates for now. The assumption is that any\n> >  +\t * incremental update would be either empty (in which case we will bail\n> >  +\t * out later) or it would actually cover at least one new pack.\n> >  +\t */\n> > - \tif (ctx->incremental)\n> > ++\tif (ctx->incremental || ctx->compact)\n> >  +\t\tgoto out;\n>\n> So this here is essentially the change you had to port over, which looks\n> about right to me. The comment is becoming somewhat stale due to the\n> change, but I don't think that's much of an issue for now.\n>\n> Thanks!\n\nThanks, both. The new version of these patches looks good to me. FYI I\nam going out of office beginning tomorrow through the end of the year.\nIn case it's easier to queue, it's fine to drop my 3.2 patches from\n'seen' and take Patrick's v2 as-is.\n\nI plan on sending a new round of 3.2 in the first week of the new year\nand don't mind it being dropped in the meantime, especially if it makes\nthings easier for the maintainer.\n\nEnjoy the holidays everyone!\n\nThanks,\nTaylor\n"},{"id":"532517","messageId":"aUTv9g9QxJ3aSGuL@pks.im","threadId":"64597","inReplyTo":"aURvuLcIVpBSIhiE@nand.local","subject":"Re: [PATCH v2 0/3] builtin/repack: avoid rewriting up-to-date MIDX","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-19T06:25:58Z","receivedAt":"2025-12-19T06:26:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 18, 2025 at 04:18:48PM -0500, Taylor Blau wrote:\n> Thanks, both. The new version of these patches looks good to me. FYI I\n> am going out of office beginning tomorrow through the end of the year.\n> In case it's easier to queue, it's fine to drop my 3.2 patches from\n> 'seen' and take Patrick's v2 as-is.\n> \n> I plan on sending a new round of 3.2 in the first week of the new year\n> and don't mind it being dropped in the meantime, especially if it makes\n> things easier for the maintainer.\n\nThanks for your review!\n\n> Enjoy the holidays everyone!\n\nLikewise, enjoy your holidays and see you next year!\n\nPatrick\n"}]}