{"thread":{"id":"66424","subject":"[PATCH 0/4] repack: various corner cases for cruft-less MIDXs","startedAt":"2026-09-30T01:28:45Z","lastAt":"2026-10-03T01:07:22Z","messageCount":41,"participants":["Taylor Blau","Junio C Hamano","Derrick Stolee","Jeff King","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"553660","messageId":"cover.1790731662.git.me@ttaylorr.com","threadId":"66424","inReplyTo":null,"subject":"[PATCH 0/4] repack: various corner cases for cruft-less MIDXs","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-09-30T01:28:34Z","receivedAt":"2026-09-30T01:28:45Z","isPatch":true,"body":"This patch series fixes a few bugs I spotted while investigating the\ncruft-less MIDX feature.\n\nThe bugs addressed are found in various corner cases, and, when\ntriggered, may result in a MIDX being written whose objects are not\nclosed under reachability. When this happens while the caller is trying\nto write reachability bitmaps, bitmap generation may fail if one or more\nselected commits are descendants of the open portion of the MIDX.\n\nThe series is structured as follows:\n\n * The first patch is a preparatory refactoring to add a context struct\n within pack-objects' handling of '--stdin-packs' to minimize the diff\n in the subsequent patch.\n\n * The second patch fixes a case where once-cruft tree and annotated tag\n objects may prevent reachability closure when objects reachable from\n them are not present in the input pack(s).\n\n * The third patch fixes a case where incremental repack operations may\n introduce the same bug when the pack generated by an incremental\n repack does not pack an additional copy of once-cruft object(s).\n\n * The fourth and final patch addresses a similar case involving .keep\n packs.\n\nThanks in advance for reviewing!\n\nTaylor Blau (4):\n pack-objects: introduce `stdin_packs_context` struct\n pack-objects: ensure tree/tag closure with '--stdin-packs=follow'\n repack: retain cruft packs in MIDXs after incremental repacks\n repack: retain cruft packs in MIDXs containing kept packs\n\n Documentation/git-pack-objects.adoc | 2 +\n builtin/pack-objects.c | 73 ++++++++++++++++++++------\n builtin/repack.c | 6 +++\n repack-midx.c | 5 ++\n t/t5331-pack-objects-stdin.sh | 81 +++++++++++++++++++++++++++++\n t/t7704-repack-cruft.sh | 51 ++++++++++++++++++\n 6 files changed, 202 insertions(+), 16 deletions(-)\n\n\nbase-commit: a018953688f1b10bddf91bff8747068f5f4746a4\n-- \n2.56.0.4.gbee41d2fc68\n"},{"id":"553661","messageId":"64bb13e2db2e5c22e842c188e08861d63e99dc77.1790731662.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790731662.git.me@ttaylorr.com","subject":"[PATCH 1/4] pack-objects: introduce `stdin_packs_context` struct","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-09-30T01:28:45Z","receivedAt":"2026-09-30T01:28:49Z","isPatch":true,"body":"`stdin_packs_read_input()` currently receives a pointer to the\n`rev_info` struct and '--stdin-packs' mode separately as arguments, but\nthe object enumeration callbacks only receive a pointer to the\n`rev_info` struct.\n\nWrap the pair in a new `stdin_packs_context` struct so that a future\nchange may reference the '--stdin-packs' mode within the various\nobject enumeration callbacks.\n\nSigned-off-by: Taylor Blau \n---\n builtin/pack-objects.c | 41 +++++++++++++++++++++++++----------------\n 1 file changed, 25 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex af9390a46b9..01adf80a2bc 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3804,11 +3804,17 @@ static int git_pack_config(const char *k, const char *v,\n static int stdin_packs_found_nr;\n static int stdin_packs_hints_nr;\n \n+struct stdin_packs_context {\n+\tstruct rev_info *revs;\n+\tenum stdin_packs_mode mode;\n+};\n+\n static int add_object_entry_from_pack(const struct object_id *oid,\n \t\t\t\t struct packed_git *p,\n \t\t\t\t uint32_t pos,\n \t\t\t\t void *_data)\n {\n+\tstruct stdin_packs_context *ctx = _data;\n \toff_t ofs;\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \tenum object_type type = OBJ_NONE;\n@@ -3827,7 +3833,6 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\tdie(_(\"could not get type of object %s in pack %s\"),\n \t\t oid_to_hex(oid), p->pack_name);\n \t} else if (type == OBJ_COMMIT) {\n-\t\tstruct rev_info *revs = _data;\n \t\t/*\n \t\t * commits in included packs are used as starting points\n \t\t * for the subsequent revision walk\n@@ -3840,7 +3845,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\t * However, we'll only add those objects to the packing\n \t\t * list after checking `want_object_in_pack()` below.\n \t\t */\n-\t\tadd_pending_oid(revs, NULL, oid, 0);\n+\t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n \t}\n \n \tif (!want_object_in_pack(oid, 0, &p, &ofs))\n@@ -3954,8 +3959,9 @@ static int stdin_packs_include_check(struct commit *commit, void *data)\n }\n \n static void stdin_packs_add_pack_entries(struct strmap *packs,\n-\t\t\t\t\t struct rev_info *revs)\n+\t\t\t\t\t struct stdin_packs_context *ctx)\n {\n+\tstruct rev_info *revs = ctx->revs;\n \tstruct string_list keys = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n \tstruct hashmap_iter iter;\n@@ -3994,15 +4000,14 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,\n \t\t (info->kind & STDIN_PACK_EXCLUDE_OPEN))\n \t\t\tfor_each_object_in_pack(info->p,\n \t\t\t\t\t\tadd_object_entry_from_pack,\n-\t\t\t\t\t\trevs,\n+\t\t\t\t\t\tctx,\n \t\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n \t}\n \n \tstring_list_clear(&keys, 0);\n }\n \n-static void stdin_packs_read_input(struct rev_info *revs,\n-\t\t\t\t enum stdin_packs_mode mode)\n+static void stdin_packs_read_input(struct stdin_packs_context *ctx)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strmap packs = STRMAP_INIT;\n@@ -4017,7 +4022,7 @@ static void stdin_packs_read_input(struct rev_info *revs,\n \t\t\tcontinue;\n \t\telse if (*key == '^')\n \t\t\tkind = STDIN_PACK_EXCLUDE_CLOSED;\n-\t\telse if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW)\n+\t\telse if (*key == '!' && ctx->mode == STDIN_PACKS_MODE_FOLLOW)\n \t\t\tkind = STDIN_PACK_EXCLUDE_OPEN;\n \n \t\tif (kind != STDIN_PACK_INCLUDE)\n@@ -4082,19 +4087,23 @@ static void stdin_packs_read_input(struct rev_info *revs,\n \t\tinfo->p = p;\n \t}\n \n-\tstdin_packs_add_pack_entries(&packs, revs);\n+\tstdin_packs_add_pack_entries(&packs, ctx);\n \n \tstrbuf_release(&buf);\n \tstrmap_clear(&packs, 1);\n }\n \n-static void add_unreachable_loose_objects(struct rev_info *revs);\n+static void add_unreachable_loose_objects(struct stdin_packs_context *ctx);\n \n static void read_stdin_packs(struct repository *repo,\n \t\t\t enum stdin_packs_mode mode, int rev_list_unpacked)\n {\n \tint prev_fetch_if_missing = repo->fetch_if_missing;\n \tstruct rev_info revs;\n+\tstruct stdin_packs_context ctx = {\n+\t\t.revs = &revs,\n+\t\t.mode = mode,\n+\t};\n \n \t/*\n \t * The revision walk may hit objects that are promised, only. As the\n@@ -4131,9 +4140,9 @@ static void read_stdin_packs(struct repository *repo,\n \t\t */\n \t\tignore_packed_keep_in_core_open = 1;\n \t}\n-\tstdin_packs_read_input(&revs, mode);\n+\tstdin_packs_read_input(&ctx);\n \tif (rev_list_unpacked)\n-\t\tadd_unreachable_loose_objects(&revs);\n+\t\tadd_unreachable_loose_objects(&ctx);\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\n@@ -4541,7 +4550,7 @@ static void add_objects_in_unpacked_packs(void)\n static int add_loose_object(const struct object_id *oid, const char *path,\n \t\t\t void *data)\n {\n-\tstruct rev_info *revs = data;\n+\tstruct stdin_packs_context *ctx = data;\n \tenum object_type type = odb_read_object_info(the_repository->objects, oid, NULL);\n \n \tif (type < 0) {\n@@ -4563,8 +4572,8 @@ static int add_loose_object(const struct object_id *oid, const char *path,\n \t\tadd_object_entry(oid, type, \"\", 0);\n \t}\n \n-\tif (revs && type == OBJ_COMMIT)\n-\t\tadd_pending_oid(revs, NULL, oid, 0);\n+\tif (ctx && type == OBJ_COMMIT)\n+\t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n \n \treturn 0;\n }\n@@ -4574,10 +4583,10 @@ static int add_loose_object(const struct object_id *oid, const char *path,\n * add_object_entry will weed out duplicates, so we just add every\n * loose object we find.\n */\n-static void add_unreachable_loose_objects(struct rev_info *revs)\n+static void add_unreachable_loose_objects(struct stdin_packs_context *ctx)\n {\n \tfor_each_loose_file_in_source(the_repository->objects->sources,\n-\t\t\t\t add_loose_object, NULL, NULL, revs);\n+\t\t\t\t add_loose_object, NULL, NULL, ctx);\n }\n \n static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)\n-- \n2.56.0.4.gbee41d2fc68\n\n"},{"id":"553662","messageId":"6348667e2e3fe63aeb139888e877dd8447570253.1790731662.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790731662.git.me@ttaylorr.com","subject":"[PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-09-30T01:28:49Z","receivedAt":"2026-09-30T01:28:54Z","isPatch":true,"body":"Since 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where\npossible, 2025-06-23), when the 'repack.midxMustContainCruft'\nconfiguration is set to \"false\", geometric repacks use\n'--stdin-packs=follow' to copy needed objects out of cruft packs so the\nMIDX can omit those packs.\n\nIn cd846bacc7d (pack-objects: introduce '--stdin-packs=follow',\n2025-06-23), this behavior changed such that whenever excluded-open\n('!') packs are present, the walk stops at objects in excluded-closed\n('^') packs. Geometric repacks use '^' for retained packs already in the\nMIDX, relying on the indexed object set being closed under reachability.\n\nHowever, the walk introduced in cd846bacc7d starts only from commit\nobjects. A geometric repack can therefore produce a MIDX that does not\nmaintain reachability closure for lone trees (that are not reachable\nfrom any commit otherwise in the closure).\n\nA later walk with '!' packs can stop at that tree in a retained '^'\npack even if a new commit reaches it. If the cruft pack remains\nexcluded, and the bitmap selection picks one or more commits which reach\nthat tree, the MIDX cannot generate a bitmap for that commit.\n\nAdd trees and tags from included and '!' packs (and loose ones with\n'--unpacked') as roots in '--stdin-packs=follow' mode. This rescues\ntheir descendants even when no input commit reaches them. Walk these\nroots after the existing traversal, preserving the `SEEN` bit to avoid\nredundant traversals. Ensure that the walk takes place *after* the\nexisting traversal so that we don't lose the path prefix used for trees\nand blobs wherever possible.\n\nObjects in '^' packs remain cutoffs to avoid rewalking packs that are\nknown to be closed under reachability.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-pack-objects.adoc | 2 +\n builtin/pack-objects.c | 32 ++++++++++++\n t/t5331-pack-objects-stdin.sh | 81 +++++++++++++++++++++++++++++\n t/t7704-repack-cruft.sh | 20 +++++++\n 4 files changed, 135 insertions(+)\n\ndiff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc\nindex 65cd00c152f..1564d44f49d 100644\n--- a/Documentation/git-pack-objects.adoc\n+++ b/Documentation/git-pack-objects.adoc\n@@ -112,6 +112,8 @@ pack may include additional objects based on the following:\n This mode is useful, for example, to resurrect once-unreachable\n objects found in cruft packs to generate packs which are closed under\n reachability up to the boundary set by the excluded packs.\n+Trees and tags in included or `!` packs are followed even when no\n+commit reaches them, as are loose trees and tags with `--unpacked`.\n +\n Incompatible with `--revs`, or options that imply `--revs` (such as\n `--all`), with the exception of `--unpacked`, which is compatible.\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 01adf80a2bc..05a94305265 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3807,6 +3807,7 @@ static int stdin_packs_hints_nr;\n struct stdin_packs_context {\n \tstruct rev_info *revs;\n \tenum stdin_packs_mode mode;\n+\tstruct oid_array extra_roots;\n };\n \n static int add_object_entry_from_pack(const struct object_id *oid,\n@@ -3846,6 +3847,9 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\t * list after checking `want_object_in_pack()` below.\n \t\t */\n \t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n+\t} else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW &&\n+\t\t (type == OBJ_TREE || type == OBJ_TAG)) {\n+\t\toid_array_append(&ctx->extra_roots, oid);\n \t}\n \n \tif (!want_object_in_pack(oid, 0, &p, &ofs))\n@@ -4103,6 +4107,7 @@ static void read_stdin_packs(struct repository *repo,\n \tstruct stdin_packs_context ctx = {\n \t\t.revs = &revs,\n \t\t.mode = mode,\n+\t\t.extra_roots = OID_ARRAY_INIT,\n \t};\n \n \t/*\n@@ -4151,6 +4156,30 @@ static void read_stdin_packs(struct repository *repo,\n \t\t\t show_object_pack_hint,\n \t\t\t &mode);\n \n+\t/*\n+\t * Trees and tags need closure even when no commit reaches them.\n+\t * Defer adding these roots to revs.pending until the commit walk\n+\t * finishes. Otherwise a subtree may be visited and marked SEEN\n+\t * before its commit's root tree, using \"a\" instead of \"sub/a\" for\n+\t * a blob's namehash and delta attributes.\n+\t */\n+\tfor (size_t i = 0; i < ctx.extra_roots.nr; i++) {\n+\t\tconst struct object_id *oid = &ctx.extra_roots.oid[i];\n+\t\tstruct object *obj = lookup_object(repo, oid);\n+\n+\t\tif (!obj || !(obj->flags & SEEN))\n+\t\t\tadd_pending_oid(&revs, NULL, oid, 0);\n+\t}\n+\tif (revs.pending.nr) {\n+\t\tif (prepare_revision_walk(&revs))\n+\t\t\tdie(_(\"revision walk setup failed\"));\n+\t\ttraverse_commit_list(&revs,\n+\t\t\t\t show_commit_pack_hint,\n+\t\t\t\t show_object_pack_hint,\n+\t\t\t\t &mode);\n+\t}\n+\toid_array_clear(&ctx.extra_roots);\n+\n \trelease_revisions(&revs);\n \n \ttrace2_data_intmax(\"pack-objects\", the_repository, \"stdin_packs_found\",\n@@ -4574,6 +4603,9 @@ static int add_loose_object(const struct object_id *oid, const char *path,\n \n \tif (ctx && type == OBJ_COMMIT)\n \t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n+\telse if (ctx && ctx->mode == STDIN_PACKS_MODE_FOLLOW &&\n+\t\t (type == OBJ_TREE || type == OBJ_TAG))\n+\t\toid_array_append(&ctx->extra_roots, oid);\n \n \treturn 0;\n }\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex c74b5861af3..aa79ecdf13c 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -520,4 +520,85 @@ test_expect_success '--stdin-packs with !-delimited pack without follow' '\n \t)\n '\n \n+test_expect_success '--stdin-packs=follow traverses a tree-only input pack' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit base &&\n+\t\ttree=$(git rev-parse HEAD^{tree}) &&\n+\t\tP=$(echo \"$tree\" | git pack-objects $packdir/pack) &&\n+\t\techo \"pack-$P.pack\" >in &&\n+\n+\t\t# Only --stdin-packs=follow should start a walk from the tree.\n+\t\t: >trace.txt &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" git pack-objects \\\n+\t\t\t--stdin-packs --stdout /dev/null &&\n+\n+\t\ttest_trace2_data pack-objects stdin_packs_hints 0 expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\t\tobjects_in_packs $P >actual &&\n+\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs=follow traverses an excluded-open tag' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit --annotate base &&\n+\n+\t\t# Put the commit, tree, and blob in one pack, and the tag in another.\n+\t\t# Give only the second pack as input with a \"!\" prefix. The result\n+\t\t# must contain the commit, tree, and blob, but not the tag.\n+\t\tP=$(echo HEAD | git pack-objects --revs $packdir/pack) &&\n+\t\tobjects_in_packs $P >expect &&\n+\n+\t\tgit rev-parse base >in &&\n+\t\tP=$(git pack-objects $packdir/pack in &&\n+\t\tP=$(git pack-objects --stdin-packs=follow $packdir/pack actual &&\n+\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs=follow respects delta attributes for subtree contents' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\techo \"sub/* -delta\" >.gitattributes &&\n+\t\tmkdir sub &&\n+\t\ttest-tool genrandom seed 8192 >sub/a &&\n+\t\tcp sub/a sub/b &&\n+\t\techo modified >>sub/b &&\n+\t\tgit add sub &&\n+\t\tgit commit -m base &&\n+\n+\t\t# If the subtree is visited first, the blobs are found as a and\n+\t\t# b, so the sub/* attribute does not apply.\n+\t\tgit rev-parse HEAD HEAD:sub >in &&\n+\t\tP=$(git pack-objects $packdir/pack in &&\n+\n+\t\tgit pack-objects --stdin-packs=follow $packdir/pack actual &&\n+\t\tprintf \"%s\\n\" \"$ZERO_OID\" \"$ZERO_OID\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex b342e82447d..b49f22878f7 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -767,6 +767,26 @@ test_expect_success 'repack --write-midx excludes cruft where possible' '\n \t)\n '\n \n+test_expect_success 'geometric repack rescues descendants of loose trees' '\n+\tgit init loose-tree-cruft &&\n+\t(\n+\t\tcd loose-tree-cruft &&\n+\t\tgit config repack.midxMustContainCruft false &&\n+\t\ttest_commit base &&\n+\t\tblob=$(echo cruft | git hash-object -w --stdin) &&\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 git repack --cruft -d &&\n+\n+\t\tprintf \"100644 blob %s\\tfile\\n\" \"$blob\" | git mktree &&\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 git repack -d --geometric=2 \\\n+\t\t\t--write-midx --write-bitmap-index &&\n+\n+\t\ttest-tool read-midx --show-objects $objdir >midx &&\n+\t\tcruft=$(ls $packdir/*.mtimes) &&\n+\t\ttest_grep ! \"$(basename \"$cruft\" .mtimes).idx\" midx &&\n+\t\ttest_grep \"^$blob \" midx\n+\t)\n+'\n+\n test_expect_success 'repack --write-midx includes cruft when instructed' '\n \tsetup_cruft_exclude_tests exclude-cruft-when-instructed &&\n \t(\n-- \n2.56.0.4.gbee41d2fc68\n\n"},{"id":"553663","messageId":"1774fed77be11b37ce9eb4b7806f5f14539503fb.1790731662.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790731662.git.me@ttaylorr.com","subject":"[PATCH 3/4] repack: retain cruft packs in MIDXs after incremental repacks","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-09-30T01:28:53Z","receivedAt":"2026-09-30T01:28:59Z","isPatch":true,"body":"An incremental repack can write a commit and tree into a new pack while\nleaving objects they reach in an existing cruft pack. For example, a\ncommit can make a previously unreachable blob reachable again. Since\n'repack' will invoke 'pack-objects' with '--incremental', it will not\ncopy the blob out of its cruft pack.\n\nWhen the 'repack.midxMustContainCruft' configuration is set to \"false\",\nwriting the first MIDX after such a repack may omit that cruft pack. The\nnew pack bypasses the `!names.nr` fallback, and there are no previous\nMIDX packs for `midx_has_unknown_packs()` to check. Selecting the new\ncommit for bitmap coverage then fails because its reachable objects are\nnot all in the MIDX.\n\nThe omission dates all the way back to 5ee86c273bf (repack: exclude\ncruft pack(s) from the MIDX where possible, 2025-06-23). It relies on\ngeometric repacking to copy once-cruft objects with\n'--stdin-packs=follow'. However, an ordinary incremental repack makes no\nsuch guarantee. Require the MIDX to include cruft packs in that case,\neven when a new pack was written.\n\nExercise this with the existing fixture that makes a cruft commit\nreachable again and adds a new (unpacked) commit on top, and ensure that\nthe incremental repack is able to successfully write a reachability\nbitmap.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 6 ++++++\n t/t7704-repack-cruft.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex c4360382c1f..b7596d488da 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -539,6 +539,12 @@ int cmd_repack(int argc,\n \t\t\tstrvec_push(&cmd.args, \"--stdin-packs=follow\");\n \t\tstrvec_push(&cmd.args, \"--unpacked\");\n \t} else {\n+\t\t/*\n+\t\t * Incremental repacks do not copy already-packed objects,\n+\t\t * so cruft packs may be required to form a reachability\n+\t\t * closure for the MIDX.\n+\t\t */\n+\t\tmidx_must_contain_cruft = 1;\n \t\tstrvec_push(&cmd.args, \"--unpacked\");\n \t\tstrvec_push(&cmd.args, \"--incremental\");\n \t}\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex b49f22878f7..f7f83e70ffe 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -787,6 +787,17 @@ test_expect_success 'geometric repack rescues descendants of loose trees' '\n \t)\n '\n \n+test_expect_success 'incremental repack includes cruft for MIDX bitmaps' '\n+\tsetup_cruft_exclude_tests incremental-cruft &&\n+\t(\n+\t\tcd incremental-cruft &&\n+\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 \\\n+\t\tgit repack -d --write-midx --write-bitmap-index &&\n+\t\tgit rev-list --test-bitmap HEAD\n+\t)\n+'\n+\n test_expect_success 'repack --write-midx includes cruft when instructed' '\n \tsetup_cruft_exclude_tests exclude-cruft-when-instructed &&\n \t(\n-- \n2.56.0.4.gbee41d2fc68\n\n"},{"id":"553664","messageId":"e942c256334e4de31ec0a1cb2d5f8c7465d8696f.1790731662.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790731662.git.me@ttaylorr.com","subject":"[PATCH 4/4] repack: retain cruft packs in MIDXs containing kept packs","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-09-30T01:28:58Z","receivedAt":"2026-09-30T01:29:03Z","isPatch":true,"body":"When performing a geometric repack with 'repack.midxMustContainCruft'\nset to \"false\", Git uses '--stdin-packs=follow' to copy (once-cruft)\nobjects needed for reachability closure out of cruft packs. .keep packs\ndo not need to participate in that walk, though they *are* included in\nthe resulting MIDX.\n\nA .keep pack can contain a commit that reaches an object whose only copy\nis in a cruft pack. When there is no previous MIDX and the repack writes\na new pack, neither `midx_has_unknown_packs()` nor the `!names.nr`\nfallback require that cruft pack to be included. If the kept commit (or\na descendant of it) is selected for bitmap coverage, the bitmap writer\nfails because the MIDX does not contain all of its reachable objects.\n\nInclude cruft packs whenever the MIDX contains kept packs. This also\nretains cruft when the kept packs happen to have full closure, or when\n'--pack-kept-objects' lets the repack walk them. It avoids having to\nestablish their closure before deciding which packs the MIDX needs.\n\nAdd a test that packs the tip commit and its tree into a kept pack,\nleaving its parent in the cruft pack. The new commit's blob remains\nloose, making the geometric repack write a new pack and bypass the\nno-new-packs fallback. Verify that the repack succeeds and that we are\nable to successfully write a bitmap.\n\nSigned-off-by: Taylor Blau \n---\n repack-midx.c | 5 +++++\n t/t7704-repack-cruft.sh | 20 ++++++++++++++++++++\n 2 files changed, 25 insertions(+)\n\ndiff --git a/repack-midx.c b/repack-midx.c\nindex 64c7f8d0f42..622c3c9d236 100644\n--- a/repack-midx.c\n+++ b/repack-midx.c\n@@ -197,6 +197,7 @@ static void midx_included_packs(struct string_list *include,\n \t}\n \n \tif (opts->midx_must_contain_cruft ||\n+\t existing->kept_packs.nr ||\n \t midx_has_unknown_packs(include, geometry, existing)) {\n \t\t/*\n \t\t * If there are one or more unknown pack(s) present (see\n@@ -209,6 +210,10 @@ static void midx_included_packs(struct string_list *include,\n \t\t * reachability closure if the MIDX is bitmapped and one\n \t\t * or more of the bitmap's selected commits reaches a\n \t\t * once-cruft object that was later made reachable.\n+\t\t *\n+\t\t * Kept packs may also depend on cruft objects, since\n+\t\t * they are included above without necessarily being\n+\t\t * traversed by the repack.\n \t\t */\n \t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n \t\t\t/*\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex f7f83e70ffe..02db2a06d9e 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -798,6 +798,26 @@ test_expect_success 'incremental repack includes cruft for MIDX bitmaps' '\n \t)\n '\n \n+test_expect_success 'geometric repack includes cruft for kept packs' '\n+\tsetup_cruft_exclude_tests kept-cruft &&\n+\t(\n+\t\tcd kept-cruft &&\n+\n+\t\t# Keep HEAD and its tree outside the geometric repack. Its\n+\t\t# parent is reachable again, but still in the cruft pack.\n+\t\tgit rev-parse HEAD HEAD^{tree} >objects &&\n+\t\tpack=$(git pack-objects $packdir/pack writes:\n\n> static int add_object_entry_from_pack(const struct object_id *oid,\n> \t\t\t\t struct packed_git *p,\n> \t\t\t\t uint32_t pos,\n> \t\t\t\t void *_data)\n> {\n> +\tstruct stdin_packs_context *ctx = _data;\n> \toff_t ofs;\n> \tstruct object_info oi = OBJECT_INFO_INIT;\n> \tenum object_type type = OBJ_NONE;\n> @@ -3827,7 +3833,6 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n> \t\tdie(_(\"could not get type of object %s in pack %s\"),\n> \t\t oid_to_hex(oid), p->pack_name);\n> \t} else if (type == OBJ_COMMIT) {\n> -\t\tstruct rev_info *revs = _data;\n> \t\t/*\n> \t\t * commits in included packs are used as starting points\n> \t\t * for the subsequent revision walk\n> @@ -3840,7 +3845,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n> \t\t * However, we'll only add those objects to the packing\n> \t\t * list after checking `want_object_in_pack()` below.\n> \t\t */\n> -\t\tadd_pending_oid(revs, NULL, oid, 0);\n> +\t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n> \t}\n> \n> \tif (!want_object_in_pack(oid, 0, &p, &ofs))\n> @@ -3954,8 +3959,9 @@ static int stdin_packs_include_check(struct commit *commit, void *data)\n> }\n\nWe used to take _data that is rev_info, but no longer. We lost decl\nfor \"struct rev_info *revs\" and rewrote its only use to directly\nreference ctx->revs. As long as the result compiles, we know there\nis no stray reference to \"revs\" left in this function, so the\nrewrite is complete. It is rare but I love this kind of patch whose\ncorrectness can be seen without reading beyond the context ;-)\n\n> static void stdin_packs_add_pack_entries(struct strmap *packs,\n> -\t\t\t\t\t struct rev_info *revs)\n> +\t\t\t\t\t struct stdin_packs_context *ctx)\n> {\n> +\tstruct rev_info *revs = ctx->revs;\n> \tstruct string_list keys = STRING_LIST_INIT_NODUP;\n> \tstruct string_list_item *item;\n> \tstruct hashmap_iter iter;\n> @@ -3994,15 +4000,14 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,\n> \t\t (info->kind & STDIN_PACK_EXCLUDE_OPEN))\n> \t\t\tfor_each_object_in_pack(info->p,\n> \t\t\t\t\t\tadd_object_entry_from_pack,\n> -\t\t\t\t\t\trevs,\n> +\t\t\t\t\t\tctx,\n> \t\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n> \t}\n> \n> \tstring_list_clear(&keys, 0);\n> }\n\nDitto.\n\n> -static void stdin_packs_read_input(struct rev_info *revs,\n> -\t\t\t\t enum stdin_packs_mode mode)\n> +static void stdin_packs_read_input(struct stdin_packs_context *ctx)\n\nWe used to take two separately, but now we can take them in one package.\n\n> {\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tstruct strmap packs = STRMAP_INIT;\n> @@ -4017,7 +4022,7 @@ static void stdin_packs_read_input(struct rev_info *revs,\n> \t\t\tcontinue;\n> \t\telse if (*key == '^')\n> \t\t\tkind = STDIN_PACK_EXCLUDE_CLOSED;\n> -\t\telse if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW)\n> +\t\telse if (*key == '!' && ctx->mode == STDIN_PACKS_MODE_FOLLOW)\n> \t\t\tkind = STDIN_PACK_EXCLUDE_OPEN;\n> \n> \t\tif (kind != STDIN_PACK_INCLUDE)\n> @@ -4082,19 +4087,23 @@ static void stdin_packs_read_input(struct rev_info *revs,\n> \t\tinfo->p = p;\n> \t}\n> \n> -\tstdin_packs_add_pack_entries(&packs, revs);\n> +\tstdin_packs_add_pack_entries(&packs, ctx);\n> \n> \tstrbuf_release(&buf);\n> \tstrmap_clear(&packs, 1);\n> }\n\nThe same argument tells us that this is the right refactoring as\nlong as the result compiles.\n\n> -static void add_unreachable_loose_objects(struct rev_info *revs);\n> +static void add_unreachable_loose_objects(struct stdin_packs_context *ctx);\n> \n> static void read_stdin_packs(struct repository *repo,\n> \t\t\t enum stdin_packs_mode mode, int rev_list_unpacked)\n> {\n> \tint prev_fetch_if_missing = repo->fetch_if_missing;\n> \tstruct rev_info revs;\n> +\tstruct stdin_packs_context ctx = {\n> +\t\t.revs = &revs,\n> +\t\t.mode = mode,\n> +\t};\n> \n> \t/*\n> \t * The revision walk may hit objects that are promised, only. As the\n> @@ -4131,9 +4140,9 @@ static void read_stdin_packs(struct repository *repo,\n> \t\t */\n> \t\tignore_packed_keep_in_core_open = 1;\n> \t}\n> -\tstdin_packs_read_input(&revs, mode);\n> +\tstdin_packs_read_input(&ctx);\n> \tif (rev_list_unpacked)\n> -\t\tadd_unreachable_loose_objects(&revs);\n> +\t\tadd_unreachable_loose_objects(&ctx);\n> \n> \tif (prepare_revision_walk(&revs))\n> \t\tdie(_(\"revision walk setup failed\"));\n\nDitto.\n\n> @@ -4541,7 +4550,7 @@ static void add_objects_in_unpacked_packs(void)\n> static int add_loose_object(const struct object_id *oid, const char *path,\n> \t\t\t void *data)\n> {\n> -\tstruct rev_info *revs = data;\n> +\tstruct stdin_packs_context *ctx = data;\n> \tenum object_type type = odb_read_object_info(the_repository->objects, oid, NULL);\n> \n> \tif (type < 0) {\n> @@ -4563,8 +4572,8 @@ static int add_loose_object(const struct object_id *oid, const char *path,\n> \t\tadd_object_entry(oid, type, \"\", 0);\n> \t}\n> \n> -\tif (revs && type == OBJ_COMMIT)\n> -\t\tadd_pending_oid(revs, NULL, oid, 0);\n> +\tif (ctx && type == OBJ_COMMIT)\n> +\t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n> \n> \treturn 0;\n> }\n\nThis one, ...\n\n> @@ -4574,10 +4583,10 @@ static int add_loose_object(const struct object_id *oid, const char *path,\n> * add_object_entry will weed out duplicates, so we just add every\n> * loose object we find.\n> */\n> -static void add_unreachable_loose_objects(struct rev_info *revs)\n> +static void add_unreachable_loose_objects(struct stdin_packs_context *ctx)\n> {\n> \tfor_each_loose_file_in_source(the_repository->objects->sources,\n> -\t\t\t\t add_loose_object, NULL, NULL, revs);\n> +\t\t\t\t add_loose_object, NULL, NULL, ctx);\n> }\n\n... together with the change to add_unreachable_loose_objects()\nhere, it is not immediately obvious if we do not have to worry about\nthe case where (ctx && !ctx->revs).\n\nGiven that 'struct stdin_packs_context' is a new structure, the fact\nthat the instantiation on the stack in read_stdin_packs() is the only\none that can give us a non-NULL 'ctx' pointer we can see in this\npatch means that a non-NULL 'ctx' cannot have a NULL '.revs' pointer\nin it. Again, as long as this patch alone compiles, we know this\nrefactoring is correct.\n\nIt is not clear to me what the implication of assuming a non-NULL\n'ctx' always means a non-NULL 'ctx->revs' is for the code health in\nthe longer term, though.\n\nThanks.\n"},{"id":"553732","messageId":"xmqqcxtubpbj.fsf@gitster.g","threadId":"66424","inReplyTo":"6348667e2e3fe63aeb139888e877dd8447570253.1790731662.git.me@ttaylorr.com","subject":"Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-30T17:51:12Z","receivedAt":"2026-09-30T17:51:15Z","isPatch":true,"body":"Taylor Blau writes:\n\n> @@ -4574,6 +4603,9 @@ static int add_loose_object(const struct object_id *oid, const char *path,\n> \n> \tif (ctx && type == OBJ_COMMIT)\n> \t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n> +\telse if (ctx && ctx->mode == STDIN_PACKS_MODE_FOLLOW &&\n> +\t\t (type == OBJ_TREE || type == OBJ_TAG))\n> +\t\toid_array_append(&ctx->extra_roots, oid);\n> \n> \treturn 0;\n> }\n\nMakes me wonder if this function will always end with \"if ctx is not\nNULL, then depending on these conditions do one more thing\" like\nthis, or if it would change later. If the former,\n\n\n\tif (!ctx)\n\t\treturn 0;\n\n\tif (type == OBJ_COMMIT)\n\t\tdo the commit thing;\n\telse if (ctx->mode == follow && type in (tree, tag))\n\t\tdo the tag or tree thing;\n\n\treturn 0;\n\nmight be easier to follow, perhaps?\n"},{"id":"553737","messageId":"a78a38ca-b08a-4194-b17c-b8802e4d43a7@gmail.com","threadId":"66424","inReplyTo":"6348667e2e3fe63aeb139888e877dd8447570253.1790731662.git.me@ttaylorr.com","subject":"Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-09-30T18:16:53Z","receivedAt":"2026-09-30T18:16:57Z","isPatch":true,"body":"On 9/29/2026 9:28 PM, Taylor Blau wrote:\n\n> Add trees and tags from included and '!' packs (and loose ones with\n> '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues\n> their descendants even when no input commit reaches them. Walk these\n> roots after the existing traversal, preserving the `SEEN` bit to avoid\n> redundant traversals. Ensure that the walk takes place *after* the\n> existing traversal so that we don't lose the path prefix used for trees\n> and blobs wherever possible.\n\n> @@ -3807,6 +3807,7 @@ static int stdin_packs_hints_nr;\n> struct stdin_packs_context {\n> \tstruct rev_info *revs;\n> \tenum stdin_packs_mode mode;\n> +\tstruct oid_array extra_roots;\n\nI believe this should be an oidset to avoid adding duplicate objects\nthat appear multiple times. The order of these extra roots doesn't\nmatter (such as in a --topo-order walk). We only care about the\nbinary \"reachable or not?\" question.\n\nThanks,\n-Stolee\n\n"},{"id":"553754","messageId":"20260930203108.GA747209@coredump.intra.peff.net","threadId":"66424","inReplyTo":"6348667e2e3fe63aeb139888e877dd8447570253.1790731662.git.me@ttaylorr.com","subject":"Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T20:31:08Z","receivedAt":"2026-09-30T20:31:16Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 08:28:49PM -0500, Taylor Blau wrote:\n\n> In cd846bacc7d (pack-objects: introduce '--stdin-packs=follow',\n> 2025-06-23), this behavior changed such that whenever excluded-open\n> ('!') packs are present, the walk stops at objects in excluded-closed\n> ('^') packs. Geometric repacks use '^' for retained packs already in the\n> MIDX, relying on the indexed object set being closed under reachability.\n> \n> However, the walk introduced in cd846bacc7d starts only from commit\n> objects. A geometric repack can therefore produce a MIDX that does not\n> maintain reachability closure for lone trees (that are not reachable\n> from any commit otherwise in the closure).\n> \n> A later walk with '!' packs can stop at that tree in a retained '^'\n> pack even if a new commit reaches it. If the cruft pack remains\n> excluded, and the bitmap selection picks one or more commits which reach\n> that tree, the MIDX cannot generate a bitmap for that commit.\n\nOK. It took me a minute to grok this, and what I got hung up on is \"a\nlater walk\". I thought you meant a later walk within the same process,\nbut you mean \"a subsequent repack / midx generation\".\n\nSo we fail to walk in an earlier repack, but we might not fail there\nbecause no bitmapped commit happens to require that closure. But we've\nset up a timebomb for that later repack, because our pack which is\n_supposed_ to be closed (and thus gets marked with \"^\") is broken.\n\nSo this fixes the initial generation of that timebomb. It doesn't help\nus deal with existing bombs, but presumably the solution there is a full\nrepack (and we would not want to deal with existing bombs, because the\npoint of \"^\" is that we can trust it and avoid lots of extra traversal).\n\nNot really asking for a change to the commit message, but just\ndocumenting my understanding (which hopefully matches yours ;) ).\n\n> Add trees and tags from included and '!' packs (and loose ones with\n> '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues\n> their descendants even when no input commit reaches them. Walk these\n> roots after the existing traversal, preserving the `SEEN` bit to avoid\n> redundant traversals. Ensure that the walk takes place *after* the\n> existing traversal so that we don't lose the path prefix used for trees\n> and blobs wherever possible.\n\nOK, that makes sense, as we should treat them the same as commits.\n\n> @@ -3846,6 +3847,9 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n> \t\t * list after checking `want_object_in_pack()` below.\n> \t\t */\n> \t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n> +\t} else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW &&\n> +\t\t (type == OBJ_TREE || type == OBJ_TAG)) {\n> +\t\toid_array_append(&ctx->extra_roots, oid);\n> \t}\n\nAnd this is the interesting part. What about blobs? I guess we don't\ncare about them because they are either there or not. There is no need\nto walk them independently because they can't reference anything.\n\nWhy do we need a separate extra_roots here, rather than just using\nadd_pending_oid()? I'd have thought we'd add it all to the same\n(\"--objects\") walk.\n\nI guess that is explained here:\n\n> +\t/*\n> +\t * Trees and tags need closure even when no commit reaches them.\n> +\t * Defer adding these roots to revs.pending until the commit walk\n> +\t * finishes. Otherwise a subtree may be visited and marked SEEN\n> +\t * before its commit's root tree, using \"a\" instead of \"sub/a\" for\n> +\t * a blob's namehash and delta attributes.\n> +\t */\n> +\tfor (size_t i = 0; i < ctx.extra_roots.nr; i++) {\n> +\t\tconst struct object_id *oid = &ctx.extra_roots.oid[i];\n> +\t\tstruct object *obj = lookup_object(repo, oid);\n> +\n> +\t\tif (!obj || !(obj->flags & SEEN))\n> +\t\t\tadd_pending_oid(&revs, NULL, oid, 0);\n> +\t}\n\nbut I'm not sure I buy it. Don't we always visit the commits first in a\nwalk? So a single walk with all of the proposed objects would be fine?\n\nIf I understand this subtree claim, you are worried about the\n(single-traversal) case that we manually queue tree A, and then later\nvisit commit C, which eventually has A as a sub-tree. So we queue A\nagain _after_ its original, but that second visit (that we skip) would\nhave had more interesting information (like path context).\n\nBut I don't think a second walk clears you of that possibility. You are\nqueuing tags, too, which might in turn point to commits. So you might\nget the same commit traversal within that second walk.\n\nI think you could fix it by putting tags into the first walk. But it\nwill always exist to some degree (you could have a tag that points to a\ntree and queue that tree, but also a commit that points to it). \n\nIt's not clear to me how big a problem this is in practice. We know that\nthe \"path\" of a tree or blob in a traversal is subject to context. There\nmight be multiple commits that point to it at different levels. I guess\nit might be more common if we are adding random trees from a pack\nwithout context.\n\nI think the more complete solution there is not two walks, but that the\ntraversal machinery should queue context-ful trees ahead of low-context\nones. I don't think we want to make the queue a stack (that would change\nthe output considerably), so you'd probably need to keep a separate\nqueue of low-context objects, and drain it only after the high-context\nones we get from traversing the commits.\n\n\nI certainly think this patch is a strict improvement, and should fix the\nmain bug. It can't make anything worse for these extra trees and tags,\nbecause we weren't even including them before. ;) But I think the subtle\nside-bug here is not a complete fix (though I do think it is strictly\nbetter than doing nothing).\n\nSo I dunno. I'd probably be OK proceeding with this as-is, because I\nfear that dual-queue thing I mentioned above might turn into a rabbit\nhole that would derail the much more important fix.\n\n-Peff\n"},{"id":"553758","messageId":"20260930204529.GB747209@coredump.intra.peff.net","threadId":"66424","inReplyTo":"1774fed77be11b37ce9eb4b7806f5f14539503fb.1790731662.git.me@ttaylorr.com","subject":"Re: [PATCH 3/4] repack: retain cruft packs in MIDXs after incremental repacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T20:45:29Z","receivedAt":"2026-09-30T20:45:31Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 08:28:53PM -0500, Taylor Blau wrote:\n\n> When the 'repack.midxMustContainCruft' configuration is set to \"false\",\n> writing the first MIDX after such a repack may omit that cruft pack. The\n> new pack bypasses the `!names.nr` fallback, and there are no previous\n> MIDX packs for `midx_has_unknown_packs()` to check. Selecting the new\n> commit for bitmap coverage then fails because its reachable objects are\n> not all in the MIDX.\n> \n> The omission dates all the way back to 5ee86c273bf (repack: exclude\n> cruft pack(s) from the MIDX where possible, 2025-06-23). It relies on\n> geometric repacking to copy once-cruft objects with\n> '--stdin-packs=follow'. However, an ordinary incremental repack makes no\n> such guarantee. Require the MIDX to include cruft packs in that case,\n> even when a new pack was written.\n\nOK. So this is a problem with just incremental repacks, but _not_\ngeometric repacks? And only when those incremental repacks write a midx?\n\nIf so, that makes sense to me (and the fix seems reasonable).\n\nBTW, write_midx_incremental() does not check midx_must_contain_cruft. So\nI think you'd have the same problem with --write-midx=incremental.\nAdding that to the tests causes them to fail. I thought it might also\nfail with GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=1, but doesn't\nseem to.\n\nThat's not a new problem, but just a spot where the fix doesn't extend.\nNot sure how important it is to do now, or if it can wait for future\nwork.\n\n-Peff\n"},{"id":"553759","messageId":"20260930205311.GC747209@coredump.intra.peff.net","threadId":"66424","inReplyTo":"e942c256334e4de31ec0a1cb2d5f8c7465d8696f.1790731662.git.me@ttaylorr.com","subject":"Re: [PATCH 4/4] repack: retain cruft packs in MIDXs containing kept packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T20:53:11Z","receivedAt":"2026-09-30T20:53:13Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 08:28:58PM -0500, Taylor Blau wrote:\n\n> When performing a geometric repack with 'repack.midxMustContainCruft'\n> set to \"false\", Git uses '--stdin-packs=follow' to copy (once-cruft)\n> objects needed for reachability closure out of cruft packs. .keep packs\n> do not need to participate in that walk, though they *are* included in\n> the resulting MIDX.\n> \n> A .keep pack can contain a commit that reaches an object whose only copy\n> is in a cruft pack. When there is no previous MIDX and the repack writes\n> a new pack, neither `midx_has_unknown_packs()` nor the `!names.nr`\n> fallback require that cruft pack to be included. If the kept commit (or\n> a descendant of it) is selected for bitmap coverage, the bitmap writer\n> fails because the MIDX does not contain all of its reachable objects.\n> \n> Include cruft packs whenever the MIDX contains kept packs. This also\n> retains cruft when the kept packs happen to have full closure, or when\n> '--pack-kept-objects' lets the repack walk them. It avoids having to\n> establish their closure before deciding which packs the MIDX needs.\n\nOK. This makes sense to me, but two questions:\n\n 1. Is this going to kick in racily because of the .keep that we\n temporarily install during pushes? That could cause unexpected\n performance changes in a big repo when the midx sometimes has to\n randomly include cruft packs.\n\n 2. I'd have thought that the solution would be to treat .keep packs\n like other included follow-packs: traverse them in the usual way.\n But maybe there are good reasons we didn't do that in the first\n place.\n\n-Peff\n"},{"id":"553760","messageId":"20260930205535.GD747209@coredump.intra.peff.net","threadId":"66424","inReplyTo":"cover.1790731662.git.me@ttaylorr.com","subject":"Re: [PATCH 0/4] repack: various corner cases for cruft-less MIDXs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T20:55:35Z","receivedAt":"2026-09-30T20:55:36Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 08:28:34PM -0500, Taylor Blau wrote:\n\n> This patch series fixes a few bugs I spotted while investigating the\n> cruft-less MIDX feature.\n> \n> The bugs addressed are found in various corner cases, and, when\n> triggered, may result in a MIDX being written whose objects are not\n> closed under reachability. When this happens while the caller is trying\n> to write reachability bitmaps, bitmap generation may fail if one or more\n> selected commits are descendants of the open portion of the MIDX.\n\nI think all of these are making things strictly better, but I did find a\nfew spots where the fixes might be incomplete. I'm not sure if that\nargues for a re-roll or for punting those to future work. ;)\n\nI agree with Stolee that an oidset is perhaps a better data structure\nfor storing the extra roots (which are in a kind-of random order anyway,\nsince we're pulling them in pack order from various packs). But it also\nprobably doesn't make that big a difference in practice (we'll skip\nduplicates during the traversal, and you probably don't have that many\nduplicate objects in a repo in the first place).\n\n-Peff\n"},{"id":"553787","messageId":"ar3P9650Hj1uOR3C@com-79390","threadId":"66424","inReplyTo":"xmqqik3mbpql.fsf@gitster.g","subject":"Re: [PATCH 1/4] pack-objects: introduce `stdin_packs_context` struct","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T03:13:59Z","receivedAt":"2026-10-01T03:14:13Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 10:42:10AM -0700, Junio C Hamano wrote:\n> We used to take _data that is rev_info, but no longer. We lost decl\n> for \"struct rev_info *revs\" and rewrote its only use to directly\n> reference ctx->revs. As long as the result compiles, we know there\n> is no stray reference to \"revs\" left in this function, so the\n> rewrite is complete. It is rare but I love this kind of patch whose\n> correctness can be seen without reading beyond the context ;-)\n\n;-)\n\n> It is not clear to me what the implication of assuming a non-NULL\n> 'ctx' always means a non-NULL 'ctx->revs' is for the code health in\n> the longer term, though.\n\nThat's fair. For the following round, I added a small note next to the\n'revs' member in the struct's definition to indicate that it must be\nnon-NULL.\n\nThanks,\nTaylor\n"},{"id":"553788","messageId":"ar3QM2QkmXfq11xc@com-79390","threadId":"66424","inReplyTo":"a78a38ca-b08a-4194-b17c-b8802e4d43a7@gmail.com","subject":"Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T03:14:59Z","receivedAt":"2026-10-01T03:15:03Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 02:16:53PM -0400, Derrick Stolee wrote:\n> > @@ -3807,6 +3807,7 @@ static int stdin_packs_hints_nr;\n> > struct stdin_packs_context {\n> > \tstruct rev_info *revs;\n> > \tenum stdin_packs_mode mode;\n> > +\tstruct oid_array extra_roots;\n>\n> I believe this should be an oidset to avoid adding duplicate objects\n> that appear multiple times. The order of these extra roots doesn't\n> matter (such as in a --topo-order walk). We only care about the\n> binary \"reachable or not?\" question.\n\nGreat suggestion! I adjusted it in the following round.\n\nThanks,\nTaylor\n"},{"id":"553789","messageId":"ar3Q_by48uOxBDB0@com-79390","threadId":"66424","inReplyTo":"20260930203108.GA747209@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T03:18:21Z","receivedAt":"2026-10-01T03:18:25Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 04:31:08PM -0400, Jeff King wrote:\n> On Tue, Sep 29, 2026 at 08:28:49PM -0500, Taylor Blau wrote:\n>\n> > In cd846bacc7d (pack-objects: introduce '--stdin-packs=follow',\n> > 2025-06-23), this behavior changed such that whenever excluded-open\n> > ('!') packs are present, the walk stops at objects in excluded-closed\n> > ('^') packs. Geometric repacks use '^' for retained packs already in the\n> > MIDX, relying on the indexed object set being closed under reachability.\n> >\n> > However, the walk introduced in cd846bacc7d starts only from commit\n> > objects. A geometric repack can therefore produce a MIDX that does not\n> > maintain reachability closure for lone trees (that are not reachable\n> > from any commit otherwise in the closure).\n> >\n> > A later walk with '!' packs can stop at that tree in a retained '^'\n> > pack even if a new commit reaches it. If the cruft pack remains\n> > excluded, and the bitmap selection picks one or more commits which reach\n> > that tree, the MIDX cannot generate a bitmap for that commit.\n>\n> OK. It took me a minute to grok this, and what I got hung up on is \"a\n> later walk\". I thought you meant a later walk within the same process,\n> but you mean \"a subsequent repack / midx generation\".\n>\n> So we fail to walk in an earlier repack, but we might not fail there\n> because no bitmapped commit happens to require that closure. But we've\n> set up a timebomb for that later repack, because our pack which is\n> _supposed_ to be closed (and thus gets marked with \"^\") is broken.\n>\n> So this fixes the initial generation of that timebomb. It doesn't help\n> us deal with existing bombs, but presumably the solution there is a full\n> repack (and we would not want to deal with existing bombs, because the\n> point of \"^\" is that we can trust it and avoid lots of extra traversal).\n>\n> Not really asking for a change to the commit message, but just\n> documenting my understanding (which hopefully matches yours ;) ).\n\nYup, exactly. Hopefully s/walk/repack/ clarifies things for the\nfollowing round, but in the meantime your understanding matches my own.\n\nWhen this feature was originally introduced, the idea was \"anything\npacked must also pack its reachability closure, less any objects in\nexcluded packs\". That was true for commit objects, but not so for trees\nand annotated tags, which is what this patch corrects.\n\n> > @@ -3846,6 +3847,9 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n> > \t\t * list after checking `want_object_in_pack()` below.\n> > \t\t */\n> > \t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n> > +\t} else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW &&\n> > +\t\t (type == OBJ_TREE || type == OBJ_TAG)) {\n> > +\t\toid_array_append(&ctx->extra_roots, oid);\n> > \t}\n>\n> And this is the interesting part. What about blobs? I guess we don't\n> care about them because they are either there or not. There is no need\n> to walk them independently because they can't reference anything.\n\nExactly.\n\n> If I understand this subtree claim, you are worried about the\n> (single-traversal) case that we manually queue tree A, and then later\n> visit commit C, which eventually has A as a sub-tree. So we queue A\n> again _after_ its original, but that second visit (that we skip) would\n> have had more interesting information (like path context).\n>\n> But I don't think a second walk clears you of that possibility. You are\n> queuing tags, too, which might in turn point to commits. So you might\n> get the same commit traversal within that second walk.\n\nYeah, that's what I was worried about when I wrote this patch, but\nthat's a good point. Really there is no \"absolute\" correct path for a\ngiven tree or tree entry, since it depends on your perspective.\n\n> I think you could fix it by putting tags into the first walk. But it\n> will always exist to some degree (you could have a tag that points to a\n> tree and queue that tree, but also a commit that points to it).\n>\n> It's not clear to me how big a problem this is in practice. We know that\n> the \"path\" of a tree or blob in a traversal is subject to context. There\n> might be multiple commits that point to it at different levels. I guess\n> it might be more common if we are adding random trees from a pack\n> without context.\n\n;-).\n\n> So I dunno. I'd probably be OK proceeding with this as-is, because I\n> fear that dual-queue thing I mentioned above might turn into a rabbit\n> hole that would derail the much more important fix.\n\nI tightened up the comment a bit, but I agree that rethinking the\ntraversal machinery is best left for another day.\n\nThanks,\nTaylor\n"},{"id":"553790","messageId":"ar3Rxga-GXTvcvFH@com-79390","threadId":"66424","inReplyTo":"20260930204529.GB747209@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] repack: retain cruft packs in MIDXs after incremental repacks","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T03:21:42Z","receivedAt":"2026-10-01T03:21:47Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 04:45:29PM -0400, Jeff King wrote:\n> That's not a new problem, but just a spot where the fix doesn't extend.\n> Not sure how important it is to do now, or if it can wait for future\n> work.\n\nYeah, good point. I have a fix for it in the subsequent round, though it\ndid make the overall series longer to accommodate predatory patches that\nmake the substantive ones easier to grok.\n\nI think that the result is sound, hence its inclusion in v2. I briefly\nconsidered dropping that part of this series altogether to deal with it\nanother day. But doing so felt irresponsible as the earlier round\npointed out the existence of a bug, the subsequent round should not\nignore it.\n\nOn the other hand, as you note, it's not a new problem relative to this\nseries, and so perhaps leaving it out wouldn't have been so bad. But I\nthink all thing equal, the patches exist, and I think that they are\nsound, so I figured that I'd send them in the following round for\ncompleteness.\n\nThe maintainer should however, feel free to avoid queueing that part of\nthe series if we want to punt on it for now.\n\nThanks,\nTaylor\n"},{"id":"553792","messageId":"ar3VB-bYA2kbVWyR@com-79390","threadId":"66424","inReplyTo":"20260930205311.GC747209@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] repack: retain cruft packs in MIDXs containing kept packs","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T03:35:35Z","receivedAt":"2026-10-01T03:35:41Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 04:53:11PM -0400, Jeff King wrote:\n> OK. This makes sense to me, but two questions:\n>\n> 1. Is this going to kick in racily because of the .keep that we\n> temporarily install during pushes? That could cause unexpected\n> performance changes in a big repo when the midx sometimes has to\n> randomly include cruft packs.\n\nExcellent point. If we happen to race between repacking and receiving an\nincoming push, it is certainly not the case that we should have to then\nthrust the cruft pack onto the MIDX, and defeat the whole point of this\nseries ;-).\n\n> 2. I'd have thought that the solution would be to treat .keep packs\n> like other included follow-packs: traverse them in the usual way.\n> But maybe there are good reasons we didn't do that in the first\n> place.\n\nYeah, I think that's right. The following round passes excluded kept\npacks into the geometric repack's traversal using '!' for packs outside\nthe existing MIDX and '^' for those already covered by it. That lets us\ncopy their ancestors out of cruft without copying the kept objects\nthemselves. Non-geometric repacks still conservatively retain cruft,\nsince kept objects may remain untraversed.\n\nThere is, however, a pre-existing corner case with --pack-kept-objects\nand a cruft pack that is also kept. Such a pack can enter the MIDX\nwithout being traversed. This change doesn't address that case. Handling\nit cleanly looks more involved, so I'd leave it for a separate\nfollow-up.\n\nThanks,\nTaylor\n"},{"id":"553793","messageId":"ar3VXavSoKS3xaiT@com-79390","threadId":"66424","inReplyTo":"20260930205535.GD747209@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] repack: various corner cases for cruft-less MIDXs","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T03:37:01Z","receivedAt":"2026-10-01T03:37:07Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 04:55:35PM -0400, Jeff King wrote:\n> On Tue, Sep 29, 2026 at 08:28:34PM -0500, Taylor Blau wrote:\n>\n> > This patch series fixes a few bugs I spotted while investigating the\n> > cruft-less MIDX feature.\n> >\n> > The bugs addressed are found in various corner cases, and, when\n> > triggered, may result in a MIDX being written whose objects are not\n> > closed under reachability. When this happens while the caller is trying\n> > to write reachability bitmaps, bitmap generation may fail if one or more\n> > selected commits are descendants of the open portion of the MIDX.\n>\n> I think all of these are making things strictly better, but I did find a\n> few spots where the fixes might be incomplete. I'm not sure if that\n> argues for a re-roll or for punting those to future work. ;)\n\nThanks for the review. Like I wrote in my response to your review,\nleaving it half-fixed felt dishonest, so fixes are included in the\nsubsequent round, though they do make the series a little longer.\n\nThere is a separate, pre-existing bug that I would like to fix outside\nof this series, but only because it (a) is independent of this series,\nand (b) I estimate that the fix is considerably more complex.\n\n> I agree with Stolee that an oidset is perhaps a better data structure\n> for storing the extra roots (which are in a kind-of random order anyway,\n> since we're pulling them in pack order from various packs). But it also\n> probably doesn't make that big a difference in practice (we'll skip\n> duplicates during the traversal, and you probably don't have that many\n> duplicate objects in a repo in the first place).\n\nYup.\n\n\nThanks,\nTaylor\n"},{"id":"553794","messageId":"cover.1790827875.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790731662.git.me@ttaylorr.com","subject":"[PATCH v2 0/8] repack: various corner cases for cruft-less MIDXs","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T04:11:29Z","receivedAt":"2026-10-01T04:11:36Z","isPatch":true,"body":"This is a reroll of [1]. Thanks to Stolee, Junio, and Peff for\nreviewing.\n\nWith 'repack.midxMustContainCruft=false', geometric repacks copy needed\nobjects out of cruft packs before omitting those packs from the MIDX.\nThe follow walk can stop at objects in retained MIDX packs, relying on\nthe indexed set being closed under reachability. That invariant must\nhold for directly enumerated trees and tags, too. A subsequent repack\nmay introduce a commit that reaches a tree already in a retained pack.\n\nThe series adds those tree and tag roots, retains cruft for ordinary\nincremental repacks, and includes the required packs when writing either\nan ordinary or incremental MIDX. These omissions can otherwise make\nbitmap generation fail even though all reachable objects remain\navailable in the repository.\n\nChanges since v1:\n\n - Use an oidset for the additional tree and tag roots, as Stolee\n suggested, avoiding duplicate entries when an object appears in\n several input packs.\n\n - Address Junio's comments by documenting that a non-NULL\n stdin_packs_context requires a non-NULL revs pointer, and by\n returning early from add_loose_object() after common handling when\n there is no context.\n\n - Following Peff's review, clarify that the tree-closure failure\n occurs in a subsequent repack. Describe the two walks precisely:\n directly enumerated commits go first, but deferred tags can\n introduce commits in the second walk, so path context remains\n best-effort. Existing MIDXs lacking closure still require a full\n repack.\n\n - Follow Peff's suggestion to traverse kept packs during geometric\n repacks, instead of always retaining cruft whenever kept packs\n exist. A temporary .keep installed by a push should not by itself\n require retaining cruft. The tests distinguish '--pack-kept-objects'\n from an explicit '--keep-pack'.\n\n - Fix the separate '--write-midx=incremental' issue Peff identified.\n Both cases include required packs outside the retained chain,\n including packs from a replaced tip. Existing append coverage now\n writes and verifies bitmaps; separate regressions cover repacking\n without producing a new pack and replacing a tip containing cruft.\n\nThanks in advance for your review!\n\nThanks,\nTaylor\n\n[1] https://lore.kernel.org/git/cover.1790731662.git.me@ttaylorr.com/\n\nTaylor Blau (8):\n pack-objects: introduce `stdin_packs_context` struct\n pack-objects: ensure tree/tag closure with '--stdin-packs=follow'\n repack: retain cruft packs in MIDXs after incremental repacks\n repack: use a sorted list for explicitly kept packs\n repack: follow kept packs when omitting cruft from the MIDX\n repack: track the preferred pack explicitly in MIDX write steps\n repack: defer allocating the append plan's write step\n repack: include required packs in incremental MIDX writes\n\n Documentation/git-pack-objects.adoc | 2 +\n Documentation/git-repack.adoc | 5 +-\n builtin/pack-objects.c | 82 +++++++++++++++----\n builtin/repack.c | 40 ++++++++-\n repack-midx.c | 123 +++++++++++++++++++---------\n repack.c | 8 +-\n repack.h | 4 +\n t/t5331-pack-objects-stdin.sh | 81 ++++++++++++++++++\n t/t7704-repack-cruft.sh | 74 +++++++++++++++++\n t/t7705-repack-incremental-midx.sh | 63 +++++++++++---\n 10 files changed, 404 insertions(+), 78 deletions(-)\n\nRange-diff against v1:\n1: fcc07ede9a0 ! 1: 354c29cae73 pack-objects: introduce `stdin_packs_context` struct\n @@ Commit message\n object enumeration callbacks.\n \n Signed-off-by: Taylor Blau \n - Signed-off-by: Taylor Blau \n \n ## builtin/pack-objects.c ##\n @@ builtin/pack-objects.c: static int git_pack_config(const char *k, const char *v,\n @@ builtin/pack-objects.c: static int git_pack_config(const char *k, const char *v,\n static int stdin_packs_hints_nr;\n \n +struct stdin_packs_context {\n -+\tstruct rev_info *revs;\n ++\tstruct rev_info *revs; /* must be non-NULL */\n +\tenum stdin_packs_mode mode;\n +};\n +\n2: 41448dca614 ! 2: 940953e5c40 pack-objects: ensure tree/tag closure with '--stdin-packs=follow'\n @@ Commit message\n maintain reachability closure for lone trees (that are not reachable\n from any commit otherwise in the closure).\n \n - A later walk with '!' packs can stop at that tree in a retained '^'\n - pack even if a new commit reaches it. If the cruft pack remains\n + A subsequent repack with '!' packs can stop at that tree in a retained\n + '^' pack even if a new commit reaches it. If the cruft pack remains\n excluded, and the bitmap selection picks one or more commits which reach\n that tree, the MIDX cannot generate a bitmap for that commit.\n \n @@ Commit message\n '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues\n their descendants even when no input commit reaches them. Walk these\n roots after the existing traversal, preserving the `SEEN` bit to avoid\n - redundant traversals. Ensure that the walk takes place *after* the\n - existing traversal so that we don't lose the path prefix used for trees\n - and blobs wherever possible.\n + redundant traversals. This gives directly enumerated commits priority\n + for the path prefixes used by name hashes and delta attributes. Tags can\n + introduce commits in the second walk, so path selection remains\n + best-effort.\n +\n + Collect the extra roots in an oidset to avoid queuing duplicates. This\n + uses memory for each distinct root and walks its unvisited descendants.\n \n Objects in '^' packs remain cutoffs to avoid rewalking packs that are\n - known to be closed under reachability.\n + known to be closed under reachability, provided '!' packs are present.\n + This does not repair existing MIDXs lacking closure; those need a full\n + repack.\n \n Signed-off-by: Taylor Blau \n - Signed-off-by: Taylor Blau \n \n ## Documentation/git-pack-objects.adoc ##\n @@ Documentation/git-pack-objects.adoc: pack may include additional objects based on the following:\n @@ Documentation/git-pack-objects.adoc: pack may include additional objects based o\n ## builtin/pack-objects.c ##\n @@ builtin/pack-objects.c: static int stdin_packs_hints_nr;\n struct stdin_packs_context {\n - \tstruct rev_info *revs;\n + \tstruct rev_info *revs; /* must be non-NULL */\n \tenum stdin_packs_mode mode;\n -+\tstruct oid_array extra_roots;\n ++\tstruct oidset extra_roots;\n };\n \n static int add_object_entry_from_pack(const struct object_id *oid,\n @@ builtin/pack-objects.c: static int add_object_entry_from_pack(const struct objec\n \t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n +\t} else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW &&\n +\t\t (type == OBJ_TREE || type == OBJ_TAG)) {\n -+\t\toid_array_append(&ctx->extra_roots, oid);\n ++\t\toidset_insert(&ctx->extra_roots, oid);\n \t}\n \n \tif (!want_object_in_pack(oid, 0, &p, &ofs))\n @@ builtin/pack-objects.c: static void read_stdin_packs(struct repository *repo,\n \tstruct stdin_packs_context ctx = {\n \t\t.revs = &revs,\n \t\t.mode = mode,\n -+\t\t.extra_roots = OID_ARRAY_INIT,\n ++\t\t.extra_roots = OIDSET_INIT,\n \t};\n ++\tstruct oidset_iter iter;\n ++\tconst struct object_id *oid;\n \n \t/*\n + \t * The revision walk may hit objects that are promised, only. As the\n @@ builtin/pack-objects.c: static void read_stdin_packs(struct repository *repo,\n \t\t\t show_object_pack_hint,\n \t\t\t &mode);\n \n +\t/*\n +\t * Trees and tags need closure even when no commit reaches them.\n -+\t * Defer adding these roots to revs.pending until the commit walk\n ++\t * Defer adding these roots to revs.pending until the first walk\n +\t * finishes. Otherwise a subtree may be visited and marked SEEN\n -+\t * before its commit's root tree, using \"a\" instead of \"sub/a\" for\n -+\t * a blob's namehash and delta attributes.\n ++\t * before its commit's root tree, using \"a\" instead of \"sub/a\"\n ++\t * for a blob's namehash and delta attributes.\n ++\t *\n ++\t * Tags may introduce more commits in the second walk, so this\n ++\t * does not *always* guarantee that trees are always visited\n ++\t * with their full paths.\n +\t */\n -+\tfor (size_t i = 0; i < ctx.extra_roots.nr; i++) {\n -+\t\tconst struct object_id *oid = &ctx.extra_roots.oid[i];\n ++\toidset_iter_init(&ctx.extra_roots, &iter);\n ++\twhile ((oid = oidset_iter_next(&iter))) {\n +\t\tstruct object *obj = lookup_object(repo, oid);\n +\n +\t\tif (!obj || !(obj->flags & SEEN))\n @@ builtin/pack-objects.c: static void read_stdin_packs(struct repository *repo,\n +\t\t\t\t show_object_pack_hint,\n +\t\t\t\t &mode);\n +\t}\n -+\toid_array_clear(&ctx.extra_roots);\n ++\toidset_clear(&ctx.extra_roots);\n +\n \trelease_revisions(&revs);\n \n \ttrace2_data_intmax(\"pack-objects\", the_repository, \"stdin_packs_found\",\n @@ builtin/pack-objects.c: static int add_loose_object(const struct object_id *oid, const char *path,\n + \t\tadd_object_entry(oid, type, \"\", 0);\n + \t}\n \n - \tif (ctx && type == OBJ_COMMIT)\n +-\tif (ctx && type == OBJ_COMMIT)\n ++\tif (!ctx)\n ++\t\treturn 0;\n ++\n ++\tif (type == OBJ_COMMIT)\n \t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n -+\telse if (ctx && ctx->mode == STDIN_PACKS_MODE_FOLLOW &&\n ++\telse if (ctx->mode == STDIN_PACKS_MODE_FOLLOW &&\n +\t\t (type == OBJ_TREE || type == OBJ_TAG))\n -+\t\toid_array_append(&ctx->extra_roots, oid);\n ++\t\toidset_insert(&ctx->extra_roots, oid);\n \n \treturn 0;\n }\n3: 72f49ff37bb ! 3: a244b26030c repack: retain cruft packs in MIDXs after incremental repacks\n @@ Commit message\n such guarantee. Require the MIDX to include cruft packs in that case,\n even when a new pack was written.\n \n + This fixes ordinary '--write-midx'. The separate\n + '--write-midx=incremental' writer does not consult this flag and needs\n + its own handling.\n +\n Exercise this with the existing fixture that makes a cruft commit\n reachable again and adds a new (unpacked) commit on top, and ensure that\n the incremental repack is able to successfully write a reachability\n bitmap.\n \n Signed-off-by: Taylor Blau \n - Signed-off-by: Taylor Blau \n \n ## builtin/repack.c ##\n @@ builtin/repack.c: int cmd_repack(int argc,\n-: ----------- > 4: c1ff18bf913 repack: use a sorted list for explicitly kept packs\n-: ----------- > 5: 51e20444dac repack: follow kept packs when omitting cruft from the MIDX\n-: ----------- > 6: a85dbcd04c7 repack: track the preferred pack explicitly in MIDX write steps\n-: ----------- > 7: 4a6504629a2 repack: defer allocating the append plan's write step\n4: a10e3aa86c1 ! 8: a42f775cbe2 repack: retain cruft packs in MIDXs containing kept packs\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - repack: retain cruft packs in MIDXs containing kept packs\n + repack: include required packs in incremental MIDX writes\n \n - When performing a geometric repack with 'repack.midxMustContainCruft'\n - set to \"false\", Git uses '--stdin-packs=follow' to copy (once-cruft)\n - objects needed for reachability closure out of cruft packs. .keep packs\n - do not need to participate in that walk, though they *are* included in\n - the resulting MIDX.\n + The append plan introduced in 06733a50eee (repack: allow\n + `--write-midx=incremental` without `--geometric`, 2026-05-19) adds only\n + newly written packs to the existing MIDX chain. The bitmap writer can\n + use objects from the new layer and all retained base layers, but the\n + plan omits preexisting packs outside the chain. Bitmap generation fails\n + if a selected commit reaches an object absent from the resulting chain.\n \n - A .keep pack can contain a commit that reaches an object whose only copy\n - is in a cruft pack. When there is no previous MIDX and the repack writes\n - a new pack, neither `midx_has_unknown_packs()` nor the `!names.nr`\n - fallback require that cruft pack to be included. If the kept commit (or\n - a descendant of it) is selected for bitmap coverage, the bitmap writer\n - fails because the MIDX does not contain all of its reachable objects.\n + The geometric plan from 1da62fb5c86 (repack: implement incremental MIDX\n + repacking, 2026-05-19) can omit kept and cruft packs, since neither\n + necessarily participates in the geometric repack. Such packs can also be\n + lost when replacing a tip layer that contains them. Neither plan\n + consults `midx_included_packs()`, so the rules for retaining cruft in\n + ordinary MIDX writes do not protect incremental writes.\n \n - Include cruft packs whenever the MIDX contains kept packs. This also\n - retains cruft when the kept packs happen to have full closure, or when\n - '--pack-kept-objects' lets the repack walk them. It avoids having to\n - establish their closure before deciding which packs the MIDX needs.\n + Use that selection logic to add missing packs to each plan's write step.\n + Skip packs in retained base layers, but include required packs from a\n + replaced tip. Count added objects when choosing which layers to compact,\n + without changing the preferred pack.\n \n - Add a test that packs the tip commit and its tree into a kept pack,\n - leaving its parent in the cruft pack. The new commit's blob remains\n - loose, making the geometric repack write a new pack and bypass the\n - no-new-packs fallback. Verify that the repack succeeds and that we are\n - able to successfully write a bitmap.\n + Write and verify bitmaps in the existing append test: its existing\n + checks do not detect the omitted pack containing the first commit. Cover\n + the no-new-pack case separately with a reachable blob in a cruft pack.\n \n Signed-off-by: Taylor Blau \n - Signed-off-by: Taylor Blau \n +\n + ## Documentation/git-repack.adoc ##\n +@@ Documentation/git-repack.adoc: linkgit:git-multi-pack-index[1]).\n + \t\tflat MIDX.\n + +\n + Without `--geometric`, a new MIDX layer is appended to the existing\n +-chain (or a new chain is started) containing whatever packs were written\n +-by the repack. Existing layers are preserved as-is.\n ++chain (or a new chain is started) containing newly written packs and any\n ++other required packs not already in the chain. Existing layers are\n ++preserved as-is.\n + +\n + When combined with `--geometric`, the incremental mode maintains a chain\n + of MIDX layers that is compacted over time using a geometric merging\n \n ## repack-midx.c ##\n +@@\n + #include \"odb.h\"\n + #include \"oidset.h\"\n + #include \"pack-bitmap.h\"\n ++#include \"packfile.h\"\n + #include \"path.h\"\n + #include \"refs.h\"\n + #include \"run-command.h\"\n +@@ repack-midx.c: void midx_snapshot_refs(struct repository *repo, struct tempfile *f)\n + \n + static int midx_has_unknown_packs(struct string_list *include,\n + \t\t\t\t struct pack_geometry *geometry,\n +-\t\t\t\t struct existing_packs *existing)\n ++\t\t\t\t struct existing_packs *existing,\n ++\t\t\t\t struct multi_pack_index *base)\n + {\n + \tstruct string_list_item *item;\n + \n +@@ repack-midx.c: static int midx_has_unknown_packs(struct string_list *include,\n + \t\t * MIDX. Note this function is called before the include\n + \t\t * list is populated with any cruft pack(s).\n + \t\t *\n ++\t\t * - In a MIDX layer retained as part of the new chain's base.\n ++\t\t *\n + \t\t * - Below the geometric split line (if using pack geometry),\n + \t\t * indicating that the pack won't be included in the new\n + \t\t * MIDX, but its contents were rolled up as part of the\n +@@ repack-midx.c: static int midx_has_unknown_packs(struct string_list *include,\n + \t\t * - In the existing non-kept packs list (if not using pack\n + \t\t * geometry), and marked as non-deleted.\n + \t\t */\n +-\t\tif (string_list_has_string(include, pack_name)) {\n ++\t\tif (string_list_has_string(include, pack_name) ||\n ++\t\t midx_contains_pack(base, pack_name)) {\n + \t\t\tcontinue;\n + \t\t} else if (geometry) {\n + \t\t\tstruct strbuf buf = STRBUF_INIT;\n +@@ repack-midx.c: static int midx_has_unknown_packs(struct string_list *include,\n + }\n + \n + static void midx_included_packs(struct string_list *include,\n +-\t\t\t\tstruct repack_write_midx_opts *opts)\n ++\t\t\t\tstruct repack_write_midx_opts *opts,\n ++\t\t\t\tstruct multi_pack_index *base)\n + {\n + \tstruct existing_packs *existing = opts->existing;\n + \tstruct pack_geometry *geometry = opts->geometry;\n @@ repack-midx.c: static void midx_included_packs(struct string_list *include,\n - \t}\n \n \tif (opts->midx_must_contain_cruft ||\n -+\t existing->kept_packs.nr ||\n - \t midx_has_unknown_packs(include, geometry, existing)) {\n + \t (!geometry->split_factor && existing->kept_packs.nr) ||\n +-\t midx_has_unknown_packs(include, geometry, existing)) {\n ++\t midx_has_unknown_packs(include, geometry, existing, base)) {\n \t\t/*\n \t\t * If there are one or more unknown pack(s) present (see\n -@@ repack-midx.c: static void midx_included_packs(struct string_list *include,\n - \t\t * reachability closure if the MIDX is bitmapped and one\n - \t\t * or more of the bitmap's selected commits reaches a\n - \t\t * once-cruft object that was later made reachable.\n -+\t\t *\n -+\t\t * Kept packs may also depend on cruft objects, since\n -+\t\t * they are included above without necessarily being\n -+\t\t * traversed by the repack.\n - \t\t */\n - \t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n - \t\t\t/*\n + \t\t * midx_has_unknown_packs() for what makes a pack\n +@@ repack-midx.c: static int write_midx_included_packs(struct repack_write_midx_opts *opts)\n + \tstruct packed_git *preferred = pack_geometry_preferred_pack(opts->geometry);\n + \tint ret = 0;\n + \n +-\tmidx_included_packs(&include, opts);\n ++\tmidx_included_packs(&include, opts, NULL);\n + \tif (!include.nr)\n + \t\tgoto done;\n + \n +@@ repack-midx.c: static void midx_compaction_step_release(struct midx_compaction_step *step)\n + \tfree(step->csum);\n + }\n + \n ++static int midx_compaction_step_include_packs(struct midx_compaction_step *step,\n ++\t\t\t\t\t struct repack_write_midx_opts *opts,\n ++\t\t\t\t\t struct multi_pack_index *base)\n ++{\n ++\tstruct odb_source_files *files = odb_source_files_downcast(opts->existing->source);\n ++\tstruct string_list include = STRING_LIST_INIT_DUP;\n ++\tstruct string_list_item *item;\n ++\tstruct strbuf path = STRBUF_INIT;\n ++\tint ret = 0;\n ++\n ++\tmidx_included_packs(&include, opts, base);\n ++\tstring_list_sort(&step->u.write);\n ++\n ++\tfor_each_string_list_item(item, &include) {\n ++\t\tstruct packed_git *p;\n ++\n ++\t\tif (string_list_has_string(&step->u.write, item->string) ||\n ++\t\t midx_contains_pack(base, item->string))\n ++\t\t\tcontinue;\n ++\n ++\t\tstrbuf_reset(&path);\n ++\t\tstrbuf_addf(&path, \"%s/%s\", opts->packdir, item->string);\n ++\t\tp = packfile_store_load_pack(files->packed, path.buf, 1);\n ++\t\tif (!p || open_pack_index(p)) {\n ++\t\t\tret = error(_(\"cannot open index for %s\"), path.buf);\n ++\t\t\tgoto out;\n ++\t\t}\n ++\t\tif (unsigned_add_overflows(step->objects_nr, p->num_objects)) {\n ++\t\t\tret = error(_(\"too many objects in MIDX compaction step\"));\n ++\t\t\tgoto out;\n ++\t\t}\n ++\t\tstep->objects_nr += p->num_objects;\n ++\t\tstring_list_insert(&step->u.write, item->string);\n ++\t}\n ++\n ++out:\n ++\tstrbuf_release(&path);\n ++\tstring_list_clear(&include, 0);\n ++\treturn ret;\n ++}\n ++\n + /*\n +- * Build an append-only MIDX plan: a single WRITE step for the freshly\n +- * written packs, plus COPY steps for every existing layer. No\n ++ * Build an append-only MIDX plan: a single WRITE step for packs not\n ++ * already in the chain, plus COPY steps for every existing layer. No\n + * compaction or merging is performed.\n + */\n + static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,\n +@@ repack-midx.c: static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,\n + \t\t\t\t\t size_t *steps_nr_p)\n + {\n + \tstruct odb_source_files *files = odb_source_files_downcast(opts->existing->source);\n ++\tstruct string_list include = STRING_LIST_INIT_DUP;\n ++\tstruct string_list_item *item;\n + \tstruct multi_pack_index *m;\n + \tstruct midx_compaction_step *steps = NULL;\n + \tstruct midx_compaction_step *step = NULL;\n +-\tstruct strbuf buf = STRBUF_INIT;\n + \tsize_t steps_nr = 0, steps_alloc = 0;\n +-\tuint32_t i;\n + \n + \todb_reprepare(opts->existing->repo->objects);\n + \tm = get_multi_pack_index(files->packed);\n + \n +-\tfor (i = 0; i < opts->names->nr; i++) {\n ++\tmidx_included_packs(&include, opts, m);\n ++\tfor_each_string_list_item(item, &include) {\n ++\t\tif (midx_contains_pack(m, item->string))\n ++\t\t\tcontinue;\n + \t\tif (!step) {\n + \t\t\tALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);\n + \t\t\tstep = &steps[steps_nr++];\n +@@ repack-midx.c: static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,\n + \t\t\tstep->type = MIDX_COMPACTION_STEP_WRITE;\n + \t\t\tstring_list_init_dup(&step->u.write);\n + \t\t}\n +-\t\tstrbuf_reset(&buf);\n +-\t\tstrbuf_addf(&buf, \"pack-%s.idx\",\n +-\t\t\t opts->names->items[i].string);\n +-\t\tstring_list_append(&step->u.write, buf.buf);\n ++\t\tstring_list_append(&step->u.write, item->string);\n + \t}\n +-\tstrbuf_release(&buf);\n ++\tstring_list_clear(&include, 0);\n + \n + \tfor (; m; m = m->base_midx) {\n + \t\tALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);\n +@@ repack-midx.c: static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts,\n + \tif (opts->geometry->midx_tip_rewritten)\n + \t\tm = m->base_midx;\n + \n ++\tif (midx_compaction_step_include_packs(&step, opts, m) < 0) {\n ++\t\tmidx_compaction_step_release(&step);\n ++\t\tret = -1;\n ++\t\tgoto out;\n ++\t}\n ++\n + \ttrace2_data_string(\"repack\", opts->existing->repo, \"midx:rewrote-tip\",\n + \t\t\t opts->geometry->midx_tip_rewritten ? \"true\" : \"false\");\n + \n \n - ## t/t7704-repack-cruft.sh ##\n -@@ t/t7704-repack-cruft.sh: test_expect_success 'incremental repack includes cruft for MIDX bitmaps' '\n + ## t/t7705-repack-incremental-midx.sh ##\n +@@ t/t7705-repack-incremental-midx.sh: test_expect_success '--write-midx=incremental without --geometric' '\n + \t\tgit repack -d &&\n + \n + \t\ttest_commit second &&\n +-\t\tgit repack --write-midx=incremental &&\n ++\t\tgit repack --write-midx=incremental --write-bitmap-index &&\n + \n + \t\tgit multi-pack-index verify &&\n + \t\ttest_line_count = 1 $midx_chain &&\n +@@ t/t7705-repack-incremental-midx.sh: test_expect_success '--write-midx=incremental without --geometric' '\n + \t\t# A second repack appends a new layer without\n + \t\t# disturbing the existing one.\n + \t\ttest_commit third &&\n +-\t\tgit repack --write-midx=incremental &&\n ++\t\tgit repack --write-midx=incremental --write-bitmap-index &&\n + \n + \t\tgit multi-pack-index verify &&\n + \t\ttest_line_count = 2 $midx_chain &&\n +@@ t/t7705-repack-incremental-midx.sh: test_expect_success '--write-midx=incremental without --geometric' '\n + \t\thead -n 1 $midx_chain >actual &&\n + \t\ttest_cmp expect actual &&\n + \n ++\t\tgit rev-list --test-bitmap HEAD &&\n + \t\tgit fsck\n \t)\n '\n \n -+test_expect_success 'geometric repack includes cruft for kept packs' '\n -+\tsetup_cruft_exclude_tests kept-cruft &&\n ++test_expect_success 'incremental MIDX includes cruft without a new pack' '\n ++\tgit init incremental-cruft &&\n ++\t(\n ++\t\tcd incremental-cruft &&\n ++\t\tgit config repack.midxMustContainCruft false &&\n ++\n ++\t\ttest_commit base &&\n ++\t\techo cruft | git hash-object -w --stdin &&\n ++\t\tgit repack --cruft -d &&\n ++\t\ttest_commit cruft &&\n ++\t\tgit repack -d &&\n ++\n ++\t\t# All objects are packed, but the new MIDX still needs cruft.\n ++\t\tgit repack --write-midx=incremental --write-bitmap-index &&\n ++\t\tgit rev-list --test-bitmap HEAD\n ++\t)\n ++'\n ++\n ++test_expect_success 'geometric incremental MIDX retains cruft when replacing its tip' '\n ++\tgit init geometric-incremental-cruft &&\n +\t(\n -+\t\tcd kept-cruft &&\n ++\t\tcd geometric-incremental-cruft &&\n ++\t\tgit config repack.midxNewLayerThreshold 1 &&\n +\n -+\t\t# Keep HEAD and its tree outside the geometric repack. Its\n -+\t\t# parent is reachable again, but still in the cruft pack.\n -+\t\tgit rev-parse HEAD HEAD^{tree} >objects &&\n -+\t\tpack=$(git pack-objects $packdir/pack actual &&\n + \t\tgrep \"^pack-.*\\.idx$\" actual >actual.packs &&\n +-\t\ttest_line_count = 1 actual.packs &&\n +-\t\ttest_grep ! \"$keep\" actual.packs &&\n ++\t\ttest_line_count = 2 actual.packs &&\n + \n + \t\tgit multi-pack-index verify &&\n ++\t\tgit rev-list --test-bitmap HEAD &&\n + \n + \t\t# All objects (from both kept and non-kept packs)\n + \t\t# must still be accessible.\n\nbase-commit: a018953688f1b10bddf91bff8747068f5f4746a4\n-- \n2.56.0.8.ga42f775cbe2\n"},{"id":"553795","messageId":"354c29cae732703e75b22fa347cb07d898304f01.1790827875.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790827875.git.me@ttaylorr.com","subject":"[PATCH v2 1/8] pack-objects: introduce `stdin_packs_context` struct","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T04:11:35Z","receivedAt":"2026-10-01T04:11:40Z","isPatch":true,"body":"`stdin_packs_read_input()` currently receives a pointer to the\n`rev_info` struct and '--stdin-packs' mode separately as arguments, but\nthe object enumeration callbacks only receive a pointer to the\n`rev_info` struct.\n\nWrap the pair in a new `stdin_packs_context` struct so that a future\nchange may reference the '--stdin-packs' mode within the various\nobject enumeration callbacks.\n\nSigned-off-by: Taylor Blau \n---\n builtin/pack-objects.c | 41 +++++++++++++++++++++++++----------------\n 1 file changed, 25 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex af9390a46b9..a553064fcce 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3804,11 +3804,17 @@ static int git_pack_config(const char *k, const char *v,\n static int stdin_packs_found_nr;\n static int stdin_packs_hints_nr;\n \n+struct stdin_packs_context {\n+\tstruct rev_info *revs; /* must be non-NULL */\n+\tenum stdin_packs_mode mode;\n+};\n+\n static int add_object_entry_from_pack(const struct object_id *oid,\n \t\t\t\t struct packed_git *p,\n \t\t\t\t uint32_t pos,\n \t\t\t\t void *_data)\n {\n+\tstruct stdin_packs_context *ctx = _data;\n \toff_t ofs;\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \tenum object_type type = OBJ_NONE;\n@@ -3827,7 +3833,6 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\tdie(_(\"could not get type of object %s in pack %s\"),\n \t\t oid_to_hex(oid), p->pack_name);\n \t} else if (type == OBJ_COMMIT) {\n-\t\tstruct rev_info *revs = _data;\n \t\t/*\n \t\t * commits in included packs are used as starting points\n \t\t * for the subsequent revision walk\n@@ -3840,7 +3845,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\t * However, we'll only add those objects to the packing\n \t\t * list after checking `want_object_in_pack()` below.\n \t\t */\n-\t\tadd_pending_oid(revs, NULL, oid, 0);\n+\t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n \t}\n \n \tif (!want_object_in_pack(oid, 0, &p, &ofs))\n@@ -3954,8 +3959,9 @@ static int stdin_packs_include_check(struct commit *commit, void *data)\n }\n \n static void stdin_packs_add_pack_entries(struct strmap *packs,\n-\t\t\t\t\t struct rev_info *revs)\n+\t\t\t\t\t struct stdin_packs_context *ctx)\n {\n+\tstruct rev_info *revs = ctx->revs;\n \tstruct string_list keys = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n \tstruct hashmap_iter iter;\n@@ -3994,15 +4000,14 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,\n \t\t (info->kind & STDIN_PACK_EXCLUDE_OPEN))\n \t\t\tfor_each_object_in_pack(info->p,\n \t\t\t\t\t\tadd_object_entry_from_pack,\n-\t\t\t\t\t\trevs,\n+\t\t\t\t\t\tctx,\n \t\t\t\t\t\tODB_FOR_EACH_OBJECT_PACK_ORDER);\n \t}\n \n \tstring_list_clear(&keys, 0);\n }\n \n-static void stdin_packs_read_input(struct rev_info *revs,\n-\t\t\t\t enum stdin_packs_mode mode)\n+static void stdin_packs_read_input(struct stdin_packs_context *ctx)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strmap packs = STRMAP_INIT;\n@@ -4017,7 +4022,7 @@ static void stdin_packs_read_input(struct rev_info *revs,\n \t\t\tcontinue;\n \t\telse if (*key == '^')\n \t\t\tkind = STDIN_PACK_EXCLUDE_CLOSED;\n-\t\telse if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW)\n+\t\telse if (*key == '!' && ctx->mode == STDIN_PACKS_MODE_FOLLOW)\n \t\t\tkind = STDIN_PACK_EXCLUDE_OPEN;\n \n \t\tif (kind != STDIN_PACK_INCLUDE)\n@@ -4082,19 +4087,23 @@ static void stdin_packs_read_input(struct rev_info *revs,\n \t\tinfo->p = p;\n \t}\n \n-\tstdin_packs_add_pack_entries(&packs, revs);\n+\tstdin_packs_add_pack_entries(&packs, ctx);\n \n \tstrbuf_release(&buf);\n \tstrmap_clear(&packs, 1);\n }\n \n-static void add_unreachable_loose_objects(struct rev_info *revs);\n+static void add_unreachable_loose_objects(struct stdin_packs_context *ctx);\n \n static void read_stdin_packs(struct repository *repo,\n \t\t\t enum stdin_packs_mode mode, int rev_list_unpacked)\n {\n \tint prev_fetch_if_missing = repo->fetch_if_missing;\n \tstruct rev_info revs;\n+\tstruct stdin_packs_context ctx = {\n+\t\t.revs = &revs,\n+\t\t.mode = mode,\n+\t};\n \n \t/*\n \t * The revision walk may hit objects that are promised, only. As the\n@@ -4131,9 +4140,9 @@ static void read_stdin_packs(struct repository *repo,\n \t\t */\n \t\tignore_packed_keep_in_core_open = 1;\n \t}\n-\tstdin_packs_read_input(&revs, mode);\n+\tstdin_packs_read_input(&ctx);\n \tif (rev_list_unpacked)\n-\t\tadd_unreachable_loose_objects(&revs);\n+\t\tadd_unreachable_loose_objects(&ctx);\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\n@@ -4541,7 +4550,7 @@ static void add_objects_in_unpacked_packs(void)\n static int add_loose_object(const struct object_id *oid, const char *path,\n \t\t\t void *data)\n {\n-\tstruct rev_info *revs = data;\n+\tstruct stdin_packs_context *ctx = data;\n \tenum object_type type = odb_read_object_info(the_repository->objects, oid, NULL);\n \n \tif (type < 0) {\n@@ -4563,8 +4572,8 @@ static int add_loose_object(const struct object_id *oid, const char *path,\n \t\tadd_object_entry(oid, type, \"\", 0);\n \t}\n \n-\tif (revs && type == OBJ_COMMIT)\n-\t\tadd_pending_oid(revs, NULL, oid, 0);\n+\tif (ctx && type == OBJ_COMMIT)\n+\t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n \n \treturn 0;\n }\n@@ -4574,10 +4583,10 @@ static int add_loose_object(const struct object_id *oid, const char *path,\n * add_object_entry will weed out duplicates, so we just add every\n * loose object we find.\n */\n-static void add_unreachable_loose_objects(struct rev_info *revs)\n+static void add_unreachable_loose_objects(struct stdin_packs_context *ctx)\n {\n \tfor_each_loose_file_in_source(the_repository->objects->sources,\n-\t\t\t\t add_loose_object, NULL, NULL, revs);\n+\t\t\t\t add_loose_object, NULL, NULL, ctx);\n }\n \n static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)\n-- \n2.56.0.8.ga42f775cbe2\n\n"},{"id":"553796","messageId":"940953e5c407046ac6789367f3341dc8ea73d07a.1790827875.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790827875.git.me@ttaylorr.com","subject":"[PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T04:11:39Z","receivedAt":"2026-10-01T04:11:43Z","isPatch":true,"body":"Since 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where\npossible, 2025-06-23), when the 'repack.midxMustContainCruft'\nconfiguration is set to \"false\", geometric repacks use\n'--stdin-packs=follow' to copy needed objects out of cruft packs so the\nMIDX can omit those packs.\n\nIn cd846bacc7d (pack-objects: introduce '--stdin-packs=follow',\n2025-06-23), this behavior changed such that whenever excluded-open\n('!') packs are present, the walk stops at objects in excluded-closed\n('^') packs. Geometric repacks use '^' for retained packs already in the\nMIDX, relying on the indexed object set being closed under reachability.\n\nHowever, the walk introduced in cd846bacc7d starts only from commit\nobjects. A geometric repack can therefore produce a MIDX that does not\nmaintain reachability closure for lone trees (that are not reachable\nfrom any commit otherwise in the closure).\n\nA subsequent repack with '!' packs can stop at that tree in a retained\n'^' pack even if a new commit reaches it. If the cruft pack remains\nexcluded, and the bitmap selection picks one or more commits which reach\nthat tree, the MIDX cannot generate a bitmap for that commit.\n\nAdd trees and tags from included and '!' packs (and loose ones with\n'--unpacked') as roots in '--stdin-packs=follow' mode. This rescues\ntheir descendants even when no input commit reaches them. Walk these\nroots after the existing traversal, preserving the `SEEN` bit to avoid\nredundant traversals. This gives directly enumerated commits priority\nfor the path prefixes used by name hashes and delta attributes. Tags can\nintroduce commits in the second walk, so path selection remains\nbest-effort.\n\nCollect the extra roots in an oidset to avoid queuing duplicates. This\nuses memory for each distinct root and walks its unvisited descendants.\n\nObjects in '^' packs remain cutoffs to avoid rewalking packs that are\nknown to be closed under reachability, provided '!' packs are present.\nThis does not repair existing MIDXs lacking closure; those need a full\nrepack.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-pack-objects.adoc | 2 +\n builtin/pack-objects.c | 43 ++++++++++++++-\n t/t5331-pack-objects-stdin.sh | 81 +++++++++++++++++++++++++++++\n t/t7704-repack-cruft.sh | 20 +++++++\n 4 files changed, 145 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc\nindex 65cd00c152f..1564d44f49d 100644\n--- a/Documentation/git-pack-objects.adoc\n+++ b/Documentation/git-pack-objects.adoc\n@@ -112,6 +112,8 @@ pack may include additional objects based on the following:\n This mode is useful, for example, to resurrect once-unreachable\n objects found in cruft packs to generate packs which are closed under\n reachability up to the boundary set by the excluded packs.\n+Trees and tags in included or `!` packs are followed even when no\n+commit reaches them, as are loose trees and tags with `--unpacked`.\n +\n Incompatible with `--revs`, or options that imply `--revs` (such as\n `--all`), with the exception of `--unpacked`, which is compatible.\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex a553064fcce..fb603059a92 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3807,6 +3807,7 @@ static int stdin_packs_hints_nr;\n struct stdin_packs_context {\n \tstruct rev_info *revs; /* must be non-NULL */\n \tenum stdin_packs_mode mode;\n+\tstruct oidset extra_roots;\n };\n \n static int add_object_entry_from_pack(const struct object_id *oid,\n@@ -3846,6 +3847,9 @@ static int add_object_entry_from_pack(const struct object_id *oid,\n \t\t * list after checking `want_object_in_pack()` below.\n \t\t */\n \t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n+\t} else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW &&\n+\t\t (type == OBJ_TREE || type == OBJ_TAG)) {\n+\t\toidset_insert(&ctx->extra_roots, oid);\n \t}\n \n \tif (!want_object_in_pack(oid, 0, &p, &ofs))\n@@ -4103,7 +4107,10 @@ static void read_stdin_packs(struct repository *repo,\n \tstruct stdin_packs_context ctx = {\n \t\t.revs = &revs,\n \t\t.mode = mode,\n+\t\t.extra_roots = OIDSET_INIT,\n \t};\n+\tstruct oidset_iter iter;\n+\tconst struct object_id *oid;\n \n \t/*\n \t * The revision walk may hit objects that are promised, only. As the\n@@ -4151,6 +4158,34 @@ static void read_stdin_packs(struct repository *repo,\n \t\t\t show_object_pack_hint,\n \t\t\t &mode);\n \n+\t/*\n+\t * Trees and tags need closure even when no commit reaches them.\n+\t * Defer adding these roots to revs.pending until the first walk\n+\t * finishes. Otherwise a subtree may be visited and marked SEEN\n+\t * before its commit's root tree, using \"a\" instead of \"sub/a\"\n+\t * for a blob's namehash and delta attributes.\n+\t *\n+\t * Tags may introduce more commits in the second walk, so this\n+\t * does not *always* guarantee that trees are always visited\n+\t * with their full paths.\n+\t */\n+\toidset_iter_init(&ctx.extra_roots, &iter);\n+\twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tstruct object *obj = lookup_object(repo, oid);\n+\n+\t\tif (!obj || !(obj->flags & SEEN))\n+\t\t\tadd_pending_oid(&revs, NULL, oid, 0);\n+\t}\n+\tif (revs.pending.nr) {\n+\t\tif (prepare_revision_walk(&revs))\n+\t\t\tdie(_(\"revision walk setup failed\"));\n+\t\ttraverse_commit_list(&revs,\n+\t\t\t\t show_commit_pack_hint,\n+\t\t\t\t show_object_pack_hint,\n+\t\t\t\t &mode);\n+\t}\n+\toidset_clear(&ctx.extra_roots);\n+\n \trelease_revisions(&revs);\n \n \ttrace2_data_intmax(\"pack-objects\", the_repository, \"stdin_packs_found\",\n@@ -4572,8 +4607,14 @@ static int add_loose_object(const struct object_id *oid, const char *path,\n \t\tadd_object_entry(oid, type, \"\", 0);\n \t}\n \n-\tif (ctx && type == OBJ_COMMIT)\n+\tif (!ctx)\n+\t\treturn 0;\n+\n+\tif (type == OBJ_COMMIT)\n \t\tadd_pending_oid(ctx->revs, NULL, oid, 0);\n+\telse if (ctx->mode == STDIN_PACKS_MODE_FOLLOW &&\n+\t\t (type == OBJ_TREE || type == OBJ_TAG))\n+\t\toidset_insert(&ctx->extra_roots, oid);\n \n \treturn 0;\n }\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex c74b5861af3..aa79ecdf13c 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -520,4 +520,85 @@ test_expect_success '--stdin-packs with !-delimited pack without follow' '\n \t)\n '\n \n+test_expect_success '--stdin-packs=follow traverses a tree-only input pack' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit base &&\n+\t\ttree=$(git rev-parse HEAD^{tree}) &&\n+\t\tP=$(echo \"$tree\" | git pack-objects $packdir/pack) &&\n+\t\techo \"pack-$P.pack\" >in &&\n+\n+\t\t# Only --stdin-packs=follow should start a walk from the tree.\n+\t\t: >trace.txt &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" git pack-objects \\\n+\t\t\t--stdin-packs --stdout /dev/null &&\n+\n+\t\ttest_trace2_data pack-objects stdin_packs_hints 0 expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\t\tobjects_in_packs $P >actual &&\n+\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs=follow traverses an excluded-open tag' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit --annotate base &&\n+\n+\t\t# Put the commit, tree, and blob in one pack, and the tag in another.\n+\t\t# Give only the second pack as input with a \"!\" prefix. The result\n+\t\t# must contain the commit, tree, and blob, but not the tag.\n+\t\tP=$(echo HEAD | git pack-objects --revs $packdir/pack) &&\n+\t\tobjects_in_packs $P >expect &&\n+\n+\t\tgit rev-parse base >in &&\n+\t\tP=$(git pack-objects $packdir/pack in &&\n+\t\tP=$(git pack-objects --stdin-packs=follow $packdir/pack actual &&\n+\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs=follow respects delta attributes for subtree contents' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\techo \"sub/* -delta\" >.gitattributes &&\n+\t\tmkdir sub &&\n+\t\ttest-tool genrandom seed 8192 >sub/a &&\n+\t\tcp sub/a sub/b &&\n+\t\techo modified >>sub/b &&\n+\t\tgit add sub &&\n+\t\tgit commit -m base &&\n+\n+\t\t# If the subtree is visited first, the blobs are found as a and\n+\t\t# b, so the sub/* attribute does not apply.\n+\t\tgit rev-parse HEAD HEAD:sub >in &&\n+\t\tP=$(git pack-objects $packdir/pack in &&\n+\n+\t\tgit pack-objects --stdin-packs=follow $packdir/pack actual &&\n+\t\tprintf \"%s\\n\" \"$ZERO_OID\" \"$ZERO_OID\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex b342e82447d..b49f22878f7 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -767,6 +767,26 @@ test_expect_success 'repack --write-midx excludes cruft where possible' '\n \t)\n '\n \n+test_expect_success 'geometric repack rescues descendants of loose trees' '\n+\tgit init loose-tree-cruft &&\n+\t(\n+\t\tcd loose-tree-cruft &&\n+\t\tgit config repack.midxMustContainCruft false &&\n+\t\ttest_commit base &&\n+\t\tblob=$(echo cruft | git hash-object -w --stdin) &&\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 git repack --cruft -d &&\n+\n+\t\tprintf \"100644 blob %s\\tfile\\n\" \"$blob\" | git mktree &&\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 git repack -d --geometric=2 \\\n+\t\t\t--write-midx --write-bitmap-index &&\n+\n+\t\ttest-tool read-midx --show-objects $objdir >midx &&\n+\t\tcruft=$(ls $packdir/*.mtimes) &&\n+\t\ttest_grep ! \"$(basename \"$cruft\" .mtimes).idx\" midx &&\n+\t\ttest_grep \"^$blob \" midx\n+\t)\n+'\n+\n test_expect_success 'repack --write-midx includes cruft when instructed' '\n \tsetup_cruft_exclude_tests exclude-cruft-when-instructed &&\n \t(\n-- \n2.56.0.8.ga42f775cbe2\n\n"},{"id":"553797","messageId":"a244b26030ca2387e6feb768f849626540091624.1790827875.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790827875.git.me@ttaylorr.com","subject":"[PATCH v2 3/8] repack: retain cruft packs in MIDXs after incremental repacks","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T04:11:44Z","receivedAt":"2026-10-01T04:11:47Z","isPatch":true,"body":"An incremental repack can write a commit and tree into a new pack while\nleaving objects they reach in an existing cruft pack. For example, a\ncommit can make a previously unreachable blob reachable again. Since\n'repack' will invoke 'pack-objects' with '--incremental', it will not\ncopy the blob out of its cruft pack.\n\nWhen the 'repack.midxMustContainCruft' configuration is set to \"false\",\nwriting the first MIDX after such a repack may omit that cruft pack. The\nnew pack bypasses the `!names.nr` fallback, and there are no previous\nMIDX packs for `midx_has_unknown_packs()` to check. Selecting the new\ncommit for bitmap coverage then fails because its reachable objects are\nnot all in the MIDX.\n\nThe omission dates all the way back to 5ee86c273bf (repack: exclude\ncruft pack(s) from the MIDX where possible, 2025-06-23). It relies on\ngeometric repacking to copy once-cruft objects with\n'--stdin-packs=follow'. However, an ordinary incremental repack makes no\nsuch guarantee. Require the MIDX to include cruft packs in that case,\neven when a new pack was written.\n\nThis fixes ordinary '--write-midx'. The separate\n'--write-midx=incremental' writer does not consult this flag and needs\nits own handling.\n\nExercise this with the existing fixture that makes a cruft commit\nreachable again and adds a new (unpacked) commit on top, and ensure that\nthe incremental repack is able to successfully write a reachability\nbitmap.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 6 ++++++\n t/t7704-repack-cruft.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex c4360382c1f..b7596d488da 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -539,6 +539,12 @@ int cmd_repack(int argc,\n \t\t\tstrvec_push(&cmd.args, \"--stdin-packs=follow\");\n \t\tstrvec_push(&cmd.args, \"--unpacked\");\n \t} else {\n+\t\t/*\n+\t\t * Incremental repacks do not copy already-packed objects,\n+\t\t * so cruft packs may be required to form a reachability\n+\t\t * closure for the MIDX.\n+\t\t */\n+\t\tmidx_must_contain_cruft = 1;\n \t\tstrvec_push(&cmd.args, \"--unpacked\");\n \t\tstrvec_push(&cmd.args, \"--incremental\");\n \t}\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex b49f22878f7..f7f83e70ffe 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -787,6 +787,17 @@ test_expect_success 'geometric repack rescues descendants of loose trees' '\n \t)\n '\n \n+test_expect_success 'incremental repack includes cruft for MIDX bitmaps' '\n+\tsetup_cruft_exclude_tests incremental-cruft &&\n+\t(\n+\t\tcd incremental-cruft &&\n+\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 \\\n+\t\tgit repack -d --write-midx --write-bitmap-index &&\n+\t\tgit rev-list --test-bitmap HEAD\n+\t)\n+'\n+\n test_expect_success 'repack --write-midx includes cruft when instructed' '\n \tsetup_cruft_exclude_tests exclude-cruft-when-instructed &&\n \t(\n-- \n2.56.0.8.ga42f775cbe2\n\n"},{"id":"553798","messageId":"c1ff18bf91363c638536265c600a7ce5ac4e1218.1790827875.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790827875.git.me@ttaylorr.com","subject":"[PATCH v2 4/8] repack: use a sorted list for explicitly kept packs","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T04:11:47Z","receivedAt":"2026-10-01T04:11:51Z","isPatch":true,"body":"`existing_packs_collect()` performs a linear search through the\n'--keep-pack' arguments for each local pack. Typically the number of\nsuch arguments is small enough that the difference between a linear and\nbinary search is just noise (especially compared with the amount of work\nthat 'repack' is about to perform).\n\nHowever, an additional caller will wish to search through the same list.\nTo prevent that caller from having to duplicate the clunky for-loop in\n`existing_packs_collect()`, sort the list using `fspathcmp()` and\nreplace the existing caller's loop with `string_list_has_string()`.\n\nThis does not change the overall behavior of '--keep-pack' arguments.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 3 +++\n repack.c | 8 +-------\n repack.h | 4 ++++\n 3 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex b7596d488da..88b05e96b5b 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -2,6 +2,7 @@\n \n #include \"builtin.h\"\n #include \"config.h\"\n+#include \"dir.h\"\n #include \"environment.h\"\n #include \"parse-options.h\"\n #include \"path.h\"\n@@ -455,6 +456,8 @@ int cmd_repack(int argc,\n \tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n \n \texisting.repo = repo;\n+\tkeep_pack_list.cmp = fspathcmp;\n+\tstring_list_sort(&keep_pack_list);\n \texisting_packs_collect(&existing, &keep_pack_list);\n \n \tif (geometry.split_factor) {\ndiff --git a/repack.c b/repack.c\nindex d2aa58e1348..fa748ce46ce 100644\n--- a/repack.c\n+++ b/repack.c\n@@ -1,5 +1,4 @@\n #include \"git-compat-util.h\"\n-#include \"dir.h\"\n #include \"midx.h\"\n #include \"odb.h\"\n #include \"packfile.h\"\n@@ -131,7 +130,6 @@ void existing_packs_collect(struct existing_packs *existing,\n \tstruct strbuf buf = STRBUF_INIT;\n \n \trepo_for_each_pack(existing->repo, p) {\n-\t\tsize_t i;\n \t\tconst char *base;\n \n \t\tif (p->multi_pack_index)\n@@ -142,15 +140,11 @@ void existing_packs_collect(struct existing_packs *existing,\n \n \t\tbase = pack_basename(p);\n \n-\t\tfor (i = 0; i < extra_keep->nr; i++)\n-\t\t\tif (!fspathcmp(base, extra_keep->items[i].string))\n-\t\t\t\tbreak;\n-\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addstr(&buf, base);\n \t\tstrbuf_strip_suffix(&buf, \".pack\");\n \n-\t\tif ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)\n+\t\tif (p->pack_keep || string_list_has_string(extra_keep, base))\n \t\t\tstring_list_append(&existing->kept_packs, buf.buf);\n \t\telse if (p->is_cruft)\n \t\t\tstring_list_append(&existing->cruft_packs, buf.buf);\ndiff --git a/repack.h b/repack.h\nindex 61e554e4ed3..9f95e3a26f4 100644\n--- a/repack.h\n+++ b/repack.h\n@@ -76,6 +76,10 @@ struct existing_packs {\n * or packs->kept based on whether each pack has a corresponding\n * .keep file or not. Packs without a .keep file are not to be kept\n * if we are going to pack everything into one file.\n+ *\n+ * A non-empty extra_keep must be sorted and use fspathcmp() as its\n+ * comparator. Its entries are pack basenames, including the \".pack\"\n+ * suffix.\n */\n void existing_packs_collect(struct existing_packs *existing,\n \t\t\t const struct string_list *extra_keep);\n-- \n2.56.0.8.ga42f775cbe2\n\n"},{"id":"553799","messageId":"51e20444dac1223f0e0485dc5799ed6d592f7614.1790827875.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790827875.git.me@ttaylorr.com","subject":"[PATCH v2 5/8] repack: follow kept packs when omitting cruft from the MIDX","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T04:11:51Z","receivedAt":"2026-10-01T04:11:54Z","isPatch":true,"body":"When performing a geometric repack with 'repack.midxMustContainCruft'\nset to \"false\", Git uses '--stdin-packs=follow' to copy (once-cruft)\nobjects needed for reachability closure out of cruft packs. .keep packs\ndo not normally participate in that walk, though they *are* included in\nthe resulting MIDX.\n\nA .keep pack can contain a commit that reaches an object whose only copy\nis in a cruft pack. When there is no previous MIDX and the repack writes\na new pack, neither `midx_has_unknown_packs()` nor the `!names.nr`\nfallback require that cruft pack to be included. If the kept commit (or\na descendant of it) is selected for bitmap coverage, the bitmap writer\nfails because the MIDX does not contain all of its reachable objects.\n\nPass excluded kept packs to the follow walk, using '!' for packs outside\nthe existing MIDX and '^' for packs already covered by it. This copies\nneeded objects out of cruft without copying objects in the kept packs.\nIt also avoids including all cruft merely because a concurrent push has\ninstalled a temporary '.keep' file.\n\nWith '--pack-kept-objects', packs with '.keep' files participate in the\ngeometric repack, and thus may have their objects copied. Packs\nspecified as kept via '--keep-pack' still exclude their objects, even\nwhen their packs fall below the geometric split.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 31 ++++++++++++++++++++++++++---\n repack-midx.c | 5 +++++\n t/t7704-repack-cruft.sh | 43 +++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 76 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 88b05e96b5b..27d6668a4ab 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -476,9 +476,11 @@ int cmd_repack(int argc,\n \tshow_progress = !po_args.quiet && isatty(2);\n \n \tstrvec_push(&cmd.args, \"--keep-true-parents\");\n-\tfor (i = 0; i < keep_pack_list.nr; i++)\n-\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\",\n-\t\t\t keep_pack_list.items[i].string);\n+\t/* Geometric follow walks exclude these packs through stdin instead. */\n+\tif (!(geometry.split_factor && !midx_must_contain_cruft))\n+\t\tfor (i = 0; i < keep_pack_list.nr; i++)\n+\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\",\n+\t\t\t\t keep_pack_list.items[i].string);\n \tstrvec_push(&cmd.args, \"--non-empty\");\n \tif (!geometry.split_factor) {\n \t\t/*\n@@ -593,6 +595,29 @@ int cmd_repack(int argc,\n \n \t\t\tfprintf(in, \"%c%s\\n\", marker, basename);\n \t\t}\n+\t\tif (!midx_must_contain_cruft) {\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\t\tfor_each_string_list_item(item, &existing.kept_packs) {\n+\t\t\t\tchar marker = '^';\n+\n+\t\t\t\tstrbuf_reset(&buf);\n+\t\t\t\tstrbuf_addf(&buf, \"%s.pack\", item->string);\n+\n+\t\t\t\tif (po_args.pack_kept_objects &&\n+\t\t\t\t !string_list_has_string(&keep_pack_list,\n+\t\t\t\t\t\t\t buf.buf))\n+\t\t\t\t\tcontinue;\n+\n+\t\t\t\t/* Exclusions override any inclusion above. */\n+\t\t\t\tif (!string_list_has_string(&existing.midx_packs,\n+\t\t\t\t\t\t\t buf.buf))\n+\t\t\t\t\tmarker = '!';\n+\n+\t\t\t\tfprintf(in, \"%c%s\\n\", marker, buf.buf);\n+\t\t\t}\n+\t\t\tstrbuf_release(&buf);\n+\t\t}\n \t\tfclose(in);\n \t}\n \ndiff --git a/repack-midx.c b/repack-midx.c\nindex 64c7f8d0f42..9f7786aaac5 100644\n--- a/repack-midx.c\n+++ b/repack-midx.c\n@@ -197,6 +197,7 @@ static void midx_included_packs(struct string_list *include,\n \t}\n \n \tif (opts->midx_must_contain_cruft ||\n+\t (!geometry->split_factor && existing->kept_packs.nr) ||\n \t midx_has_unknown_packs(include, geometry, existing)) {\n \t\t/*\n \t\t * If there are one or more unknown pack(s) present (see\n@@ -209,6 +210,10 @@ static void midx_included_packs(struct string_list *include,\n \t\t * reachability closure if the MIDX is bitmapped and one\n \t\t * or more of the bitmap's selected commits reaches a\n \t\t * once-cruft object that was later made reachable.\n+\t\t *\n+\t\t * Kept packs may also depend on cruft objects, since\n+\t\t * they are included above without necessarily being\n+\t\t * traversed by a non-geometric repack.\n \t\t */\n \t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n \t\t\t/*\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex f7f83e70ffe..bc6d6f588fa 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -798,6 +798,49 @@ test_expect_success 'incremental repack includes cruft for MIDX bitmaps' '\n \t)\n '\n \n+test_expect_success 'geometric repack follows kept packs to cruft objects' '\n+\tsetup_cruft_exclude_tests kept-cruft &&\n+\t(\n+\t\tcd kept-cruft &&\n+\n+\t\t# Put HEAD in a kept pack, while its parent is still in\n+\t\t# a cruft pack.\n+\t\tpack=$(echo \"HEAD^..HEAD\" | git pack-objects --revs $packdir/pack) &&\n+\t\tgit prune-packed &&\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 \\\n+\t\tgit repack -d --geometric=2 --write-midx --write-bitmap-index \\\n+\t\t\t--keep-pack=pack-$pack.pack &&\n+\n+\t\ttest-tool find-pack -c 1 HEAD &&\n+\t\ttest-tool read-midx --show-objects $objdir >midx &&\n+\t\tcruft=$(ls $packdir/*.mtimes) &&\n+\t\ttest_grep ! \"$(basename \"$cruft\" .mtimes).idx\" midx\n+\t)\n+'\n+\n+test_expect_success 'full repack retains cruft pack in MIDX for unreachable kept objects' '\n+\tsetup_cruft_exclude_tests unreachable-kept-cruft &&\n+\t(\n+\t\tcd unreachable-kept-cruft &&\n+\n+\t\tpack=$(echo \"HEAD^..HEAD\" | git pack-objects --revs $packdir/pack) &&\n+\t\ttouch $packdir/pack-$pack.keep &&\n+\n+\t\t# Make the kept commit unreachable so that the full\n+\t\t# repack leaves its parent in a cruft pack.\n+\t\tgit reset --hard one &&\n+\t\tgit tag -d four &&\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 \\\n+\t\tgit repack -a --write-midx --write-bitmap-index &&\n+\n+\t\ttest-tool read-midx --show-objects $objdir >midx &&\n+\t\tcruft=$(ls $packdir/*.mtimes) &&\n+\t\ttest_grep \"$(basename \"$cruft\" .mtimes).idx\" midx\n+\t)\n+'\n+\n test_expect_success 'repack --write-midx includes cruft when instructed' '\n \tsetup_cruft_exclude_tests exclude-cruft-when-instructed &&\n \t(\n-- \n2.56.0.8.ga42f775cbe2\n\n"},{"id":"553800","messageId":"a85dbcd04c7957756848e5f3102744d20b509fc4.1790827875.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790827875.git.me@ttaylorr.com","subject":"[PATCH v2 6/8] repack: track the preferred pack explicitly in MIDX write steps","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T04:11:58Z","receivedAt":"2026-10-01T04:12:02Z","isPatch":true,"body":"A MIDX write step marks preferred packs in its string-list entries and\nchooses the last marked entry when executing the step. That makes the\nchoice depend on list order, preventing the list from being sorted for\nmembership checks.\n\nRecord the last candidate directly in the step, borrowing its name from\nthe write list. This preserves preferred-pack selection while allowing\nthe list to be sorted without changing that choice.\n---\n repack-midx.c | 16 +++++-----------\n 1 file changed, 5 insertions(+), 11 deletions(-)\n\ndiff --git a/repack-midx.c b/repack-midx.c\nindex 9f7786aaac5..61832114671 100644\n--- a/repack-midx.c\n+++ b/repack-midx.c\n@@ -404,6 +404,7 @@ struct midx_compaction_step {\n \n \tuint32_t objects_nr;\n \tchar *csum;\n+\tconst char *preferred_pack; /* points into u.write */\n \n \tenum {\n \t\tMIDX_COMPACTION_STEP_UNKNOWN,\n@@ -441,8 +442,6 @@ static int midx_compaction_step_exec_write(struct midx_compaction_step *step,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list hash = STRING_LIST_INIT_DUP;\n-\tstruct string_list_item *item;\n-\tconst char *preferred_pack = NULL;\n \tint ret = 0;\n \n \tif (!step->u.write.nr) {\n@@ -450,19 +449,14 @@ static int midx_compaction_step_exec_write(struct midx_compaction_step *step,\n \t\tgoto out;\n \t}\n \n-\tfor_each_string_list_item(item, &step->u.write) {\n-\t\tif (item->util)\n-\t\t\tpreferred_pack = item->string;\n-\t}\n-\n \trepack_prepare_midx_command(&cmd, opts, \"write\");\n \tstrvec_pushl(&cmd.args, \"--incremental\", \"--no-write-chain-file\", NULL);\n \tstrvec_pushf(&cmd.args, \"--base=%s\", base ? base : \"none\");\n \n-\tif (preferred_pack) {\n+\tif (step->preferred_pack) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tstrbuf_addstr(&buf, preferred_pack);\n+\t\tstrbuf_addstr(&buf, step->preferred_pack);\n \t\tstrbuf_strip_suffix(&buf, \".idx\");\n \t\tstrbuf_addstr(&buf, \".pack\");\n \n@@ -719,7 +713,7 @@ static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts,\n \n \t\titem = string_list_append(&step.u.write, buf.buf);\n \t\tif (p->multi_pack_index || i == opts->geometry->pack_nr - 1)\n-\t\t\titem->util = (void *)1; /* mark as preferred */\n+\t\t\tstep.preferred_pack = item->string;\n \n \t\tif (unsigned_add_overflows(step.objects_nr, p->num_objects)) {\n \t\t\tret = error(_(\"too many objects in MIDX compaction step\"));\n@@ -797,7 +791,7 @@ static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts,\n \n \t\t\titem = string_list_append(&step.u.write, buf.buf);\n \t\t\tif (pack_int_id == preferred_pack_idx)\n-\t\t\t\titem->util = (void *)1; /* mark as preferred */\n+\t\t\t\tstep.preferred_pack = item->string;\n \t\t}\n \n \t\tif (unsigned_add_overflows(step.objects_nr, m->num_objects)) {\n-- \n2.56.0.8.ga42f775cbe2\n\n"},{"id":"553801","messageId":"4a6504629a24cc802462f44b36f02b7118d2a9b7.1790827875.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790827875.git.me@ttaylorr.com","subject":"[PATCH v2 7/8] repack: defer allocating the append plan's write step","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T04:12:02Z","receivedAt":"2026-10-01T04:12:05Z","isPatch":true,"body":"The append plan creates a write step before iterating over newly\nwritten packs. The next change will consider additional packs and skip\nthose already covered by the MIDX chain, so a non-empty candidate list\nwill no longer imply that a write step is needed.\n\nAllocate the step when adding its first pack instead. Continue to\nconsider only newly written packs here, preserving the existing plan.\n\nSigned-off-by: Taylor Blau \n---\n repack-midx.c | 35 +++++++++++++++--------------------\n 1 file changed, 15 insertions(+), 20 deletions(-)\n\ndiff --git a/repack-midx.c b/repack-midx.c\nindex 61832114671..06eadb9df82 100644\n--- a/repack-midx.c\n+++ b/repack-midx.c\n@@ -559,33 +559,28 @@ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,\n \tstruct odb_source_files *files = odb_source_files_downcast(opts->existing->source);\n \tstruct multi_pack_index *m;\n \tstruct midx_compaction_step *steps = NULL;\n-\tstruct midx_compaction_step *step;\n+\tstruct midx_compaction_step *step = NULL;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tsize_t steps_nr = 0, steps_alloc = 0;\n+\tuint32_t i;\n \n \todb_reprepare(opts->existing->repo->objects);\n \tm = get_multi_pack_index(files->packed);\n \n-\tif (opts->names->nr) {\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tuint32_t i;\n-\n-\t\tALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);\n-\n-\t\tstep = &steps[steps_nr++];\n-\t\tmemset(step, 0, sizeof(*step));\n-\n-\t\tstep->type = MIDX_COMPACTION_STEP_WRITE;\n-\t\tstring_list_init_dup(&step->u.write);\n-\n-\t\tfor (i = 0; i < opts->names->nr; i++) {\n-\t\t\tstrbuf_reset(&buf);\n-\t\t\tstrbuf_addf(&buf, \"pack-%s.idx\",\n-\t\t\t\t opts->names->items[i].string);\n-\t\t\tstring_list_append(&step->u.write, buf.buf);\n+\tfor (i = 0; i < opts->names->nr; i++) {\n+\t\tif (!step) {\n+\t\t\tALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);\n+\t\t\tstep = &steps[steps_nr++];\n+\t\t\tmemset(step, 0, sizeof(*step));\n+\t\t\tstep->type = MIDX_COMPACTION_STEP_WRITE;\n+\t\t\tstring_list_init_dup(&step->u.write);\n \t\t}\n-\n-\t\tstrbuf_release(&buf);\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addf(&buf, \"pack-%s.idx\",\n+\t\t\t opts->names->items[i].string);\n+\t\tstring_list_append(&step->u.write, buf.buf);\n \t}\n+\tstrbuf_release(&buf);\n \n \tfor (; m; m = m->base_midx) {\n \t\tALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);\n-- \n2.56.0.8.ga42f775cbe2\n\n"},{"id":"553802","messageId":"a42f775cbe27b385bfc8ff38f33604b3913dc340.1790827875.git.me@ttaylorr.com","threadId":"66424","inReplyTo":"cover.1790827875.git.me@ttaylorr.com","subject":"[PATCH v2 8/8] repack: include required packs in incremental MIDX writes","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-01T04:12:05Z","receivedAt":"2026-10-01T04:12:10Z","isPatch":true,"body":"The append plan introduced in 06733a50eee (repack: allow\n`--write-midx=incremental` without `--geometric`, 2026-05-19) adds only\nnewly written packs to the existing MIDX chain. The bitmap writer can\nuse objects from the new layer and all retained base layers, but the\nplan omits preexisting packs outside the chain. Bitmap generation fails\nif a selected commit reaches an object absent from the resulting chain.\n\nThe geometric plan from 1da62fb5c86 (repack: implement incremental MIDX\nrepacking, 2026-05-19) can omit kept and cruft packs, since neither\nnecessarily participates in the geometric repack. Such packs can also be\nlost when replacing a tip layer that contains them. Neither plan\nconsults `midx_included_packs()`, so the rules for retaining cruft in\nordinary MIDX writes do not protect incremental writes.\n\nUse that selection logic to add missing packs to each plan's write step.\nSkip packs in retained base layers, but include required packs from a\nreplaced tip. Count added objects when choosing which layers to compact,\nwithout changing the preferred pack.\n\nWrite and verify bitmaps in the existing append test: its existing\nchecks do not detect the omitted pack containing the first commit. Cover\nthe no-new-pack case separately with a reachable blob in a cruft pack.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-repack.adoc | 5 +-\n repack-midx.c | 83 ++++++++++++++++++++++++------\n t/t7705-repack-incremental-midx.sh | 63 ++++++++++++++++++-----\n 3 files changed, 122 insertions(+), 29 deletions(-)\n\ndiff --git a/Documentation/git-repack.adoc b/Documentation/git-repack.adoc\nindex a1f9e64f668..ee59b76580a 100644\n--- a/Documentation/git-repack.adoc\n+++ b/Documentation/git-repack.adoc\n@@ -303,8 +303,9 @@ linkgit:git-multi-pack-index[1]).\n \t\tflat MIDX.\n +\n Without `--geometric`, a new MIDX layer is appended to the existing\n-chain (or a new chain is started) containing whatever packs were written\n-by the repack. Existing layers are preserved as-is.\n+chain (or a new chain is started) containing newly written packs and any\n+other required packs not already in the chain. Existing layers are\n+preserved as-is.\n +\n When combined with `--geometric`, the incremental mode maintains a chain\n of MIDX layers that is compacted over time using a geometric merging\ndiff --git a/repack-midx.c b/repack-midx.c\nindex 06eadb9df82..58776c44691 100644\n--- a/repack-midx.c\n+++ b/repack-midx.c\n@@ -7,6 +7,7 @@\n #include \"odb.h\"\n #include \"oidset.h\"\n #include \"pack-bitmap.h\"\n+#include \"packfile.h\"\n #include \"path.h\"\n #include \"refs.h\"\n #include \"run-command.h\"\n@@ -73,7 +74,8 @@ void midx_snapshot_refs(struct repository *repo, struct tempfile *f)\n \n static int midx_has_unknown_packs(struct string_list *include,\n \t\t\t\t struct pack_geometry *geometry,\n-\t\t\t\t struct existing_packs *existing)\n+\t\t\t\t struct existing_packs *existing,\n+\t\t\t\t struct multi_pack_index *base)\n {\n \tstruct string_list_item *item;\n \n@@ -91,6 +93,8 @@ static int midx_has_unknown_packs(struct string_list *include,\n \t\t * MIDX. Note this function is called before the include\n \t\t * list is populated with any cruft pack(s).\n \t\t *\n+\t\t * - In a MIDX layer retained as part of the new chain's base.\n+\t\t *\n \t\t * - Below the geometric split line (if using pack geometry),\n \t\t * indicating that the pack won't be included in the new\n \t\t * MIDX, but its contents were rolled up as part of the\n@@ -99,7 +103,8 @@ static int midx_has_unknown_packs(struct string_list *include,\n \t\t * - In the existing non-kept packs list (if not using pack\n \t\t * geometry), and marked as non-deleted.\n \t\t */\n-\t\tif (string_list_has_string(include, pack_name)) {\n+\t\tif (string_list_has_string(include, pack_name) ||\n+\t\t midx_contains_pack(base, pack_name)) {\n \t\t\tcontinue;\n \t\t} else if (geometry) {\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n@@ -141,7 +146,8 @@ static int midx_has_unknown_packs(struct string_list *include,\n }\n \n static void midx_included_packs(struct string_list *include,\n-\t\t\t\tstruct repack_write_midx_opts *opts)\n+\t\t\t\tstruct repack_write_midx_opts *opts,\n+\t\t\t\tstruct multi_pack_index *base)\n {\n \tstruct existing_packs *existing = opts->existing;\n \tstruct pack_geometry *geometry = opts->geometry;\n@@ -198,7 +204,7 @@ static void midx_included_packs(struct string_list *include,\n \n \tif (opts->midx_must_contain_cruft ||\n \t (!geometry->split_factor && existing->kept_packs.nr) ||\n-\t midx_has_unknown_packs(include, geometry, existing)) {\n+\t midx_has_unknown_packs(include, geometry, existing, base)) {\n \t\t/*\n \t\t * If there are one or more unknown pack(s) present (see\n \t\t * midx_has_unknown_packs() for what makes a pack\n@@ -336,7 +342,7 @@ static int write_midx_included_packs(struct repack_write_midx_opts *opts)\n \tstruct packed_git *preferred = pack_geometry_preferred_pack(opts->geometry);\n \tint ret = 0;\n \n-\tmidx_included_packs(&include, opts);\n+\tmidx_included_packs(&include, opts, NULL);\n \tif (!include.nr)\n \t\tgoto done;\n \n@@ -547,9 +553,50 @@ static void midx_compaction_step_release(struct midx_compaction_step *step)\n \tfree(step->csum);\n }\n \n+static int midx_compaction_step_include_packs(struct midx_compaction_step *step,\n+\t\t\t\t\t struct repack_write_midx_opts *opts,\n+\t\t\t\t\t struct multi_pack_index *base)\n+{\n+\tstruct odb_source_files *files = odb_source_files_downcast(opts->existing->source);\n+\tstruct string_list include = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *item;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tint ret = 0;\n+\n+\tmidx_included_packs(&include, opts, base);\n+\tstring_list_sort(&step->u.write);\n+\n+\tfor_each_string_list_item(item, &include) {\n+\t\tstruct packed_git *p;\n+\n+\t\tif (string_list_has_string(&step->u.write, item->string) ||\n+\t\t midx_contains_pack(base, item->string))\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_reset(&path);\n+\t\tstrbuf_addf(&path, \"%s/%s\", opts->packdir, item->string);\n+\t\tp = packfile_store_load_pack(files->packed, path.buf, 1);\n+\t\tif (!p || open_pack_index(p)) {\n+\t\t\tret = error(_(\"cannot open index for %s\"), path.buf);\n+\t\t\tgoto out;\n+\t\t}\n+\t\tif (unsigned_add_overflows(step->objects_nr, p->num_objects)) {\n+\t\t\tret = error(_(\"too many objects in MIDX compaction step\"));\n+\t\t\tgoto out;\n+\t\t}\n+\t\tstep->objects_nr += p->num_objects;\n+\t\tstring_list_insert(&step->u.write, item->string);\n+\t}\n+\n+out:\n+\tstrbuf_release(&path);\n+\tstring_list_clear(&include, 0);\n+\treturn ret;\n+}\n+\n /*\n- * Build an append-only MIDX plan: a single WRITE step for the freshly\n- * written packs, plus COPY steps for every existing layer. No\n+ * Build an append-only MIDX plan: a single WRITE step for packs not\n+ * already in the chain, plus COPY steps for every existing layer. No\n * compaction or merging is performed.\n */\n static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,\n@@ -557,17 +604,20 @@ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,\n \t\t\t\t\t size_t *steps_nr_p)\n {\n \tstruct odb_source_files *files = odb_source_files_downcast(opts->existing->source);\n+\tstruct string_list include = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *item;\n \tstruct multi_pack_index *m;\n \tstruct midx_compaction_step *steps = NULL;\n \tstruct midx_compaction_step *step = NULL;\n-\tstruct strbuf buf = STRBUF_INIT;\n \tsize_t steps_nr = 0, steps_alloc = 0;\n-\tuint32_t i;\n \n \todb_reprepare(opts->existing->repo->objects);\n \tm = get_multi_pack_index(files->packed);\n \n-\tfor (i = 0; i < opts->names->nr; i++) {\n+\tmidx_included_packs(&include, opts, m);\n+\tfor_each_string_list_item(item, &include) {\n+\t\tif (midx_contains_pack(m, item->string))\n+\t\t\tcontinue;\n \t\tif (!step) {\n \t\t\tALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);\n \t\t\tstep = &steps[steps_nr++];\n@@ -575,12 +625,9 @@ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,\n \t\t\tstep->type = MIDX_COMPACTION_STEP_WRITE;\n \t\t\tstring_list_init_dup(&step->u.write);\n \t\t}\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addf(&buf, \"pack-%s.idx\",\n-\t\t\t opts->names->items[i].string);\n-\t\tstring_list_append(&step->u.write, buf.buf);\n+\t\tstring_list_append(&step->u.write, item->string);\n \t}\n-\tstrbuf_release(&buf);\n+\tstring_list_clear(&include, 0);\n \n \tfor (; m; m = m->base_midx) {\n \t\tALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);\n@@ -729,6 +776,12 @@ static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts,\n \tif (opts->geometry->midx_tip_rewritten)\n \t\tm = m->base_midx;\n \n+\tif (midx_compaction_step_include_packs(&step, opts, m) < 0) {\n+\t\tmidx_compaction_step_release(&step);\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n+\n \ttrace2_data_string(\"repack\", opts->existing->repo, \"midx:rewrote-tip\",\n \t\t\t opts->geometry->midx_tip_rewritten ? \"true\" : \"false\");\n \ndiff --git a/t/t7705-repack-incremental-midx.sh b/t/t7705-repack-incremental-midx.sh\nindex 25a8c40e8ee..4760c920a50 100755\n--- a/t/t7705-repack-incremental-midx.sh\n+++ b/t/t7705-repack-incremental-midx.sh\n@@ -74,7 +74,7 @@ test_expect_success '--write-midx=incremental without --geometric' '\n \t\tgit repack -d &&\n \n \t\ttest_commit second &&\n-\t\tgit repack --write-midx=incremental &&\n+\t\tgit repack --write-midx=incremental --write-bitmap-index &&\n \n \t\tgit multi-pack-index verify &&\n \t\ttest_line_count = 1 $midx_chain &&\n@@ -83,7 +83,7 @@ test_expect_success '--write-midx=incremental without --geometric' '\n \t\t# A second repack appends a new layer without\n \t\t# disturbing the existing one.\n \t\ttest_commit third &&\n-\t\tgit repack --write-midx=incremental &&\n+\t\tgit repack --write-midx=incremental --write-bitmap-index &&\n \n \t\tgit multi-pack-index verify &&\n \t\ttest_line_count = 2 $midx_chain &&\n@@ -91,10 +91,50 @@ test_expect_success '--write-midx=incremental without --geometric' '\n \t\thead -n 1 $midx_chain >actual &&\n \t\ttest_cmp expect actual &&\n \n+\t\tgit rev-list --test-bitmap HEAD &&\n \t\tgit fsck\n \t)\n '\n \n+test_expect_success 'incremental MIDX includes cruft without a new pack' '\n+\tgit init incremental-cruft &&\n+\t(\n+\t\tcd incremental-cruft &&\n+\t\tgit config repack.midxMustContainCruft false &&\n+\n+\t\ttest_commit base &&\n+\t\techo cruft | git hash-object -w --stdin &&\n+\t\tgit repack --cruft -d &&\n+\t\ttest_commit cruft &&\n+\t\tgit repack -d &&\n+\n+\t\t# All objects are packed, but the new MIDX still needs cruft.\n+\t\tgit repack --write-midx=incremental --write-bitmap-index &&\n+\t\tgit rev-list --test-bitmap HEAD\n+\t)\n+'\n+\n+test_expect_success 'geometric incremental MIDX retains cruft when replacing its tip' '\n+\tgit init geometric-incremental-cruft &&\n+\t(\n+\t\tcd geometric-incremental-cruft &&\n+\t\tgit config repack.midxNewLayerThreshold 1 &&\n+\n+\t\ttest_commit base &&\n+\t\techo cruft | git hash-object -w --stdin &&\n+\t\tgit repack --cruft -d &&\n+\t\tgit multi-pack-index write --incremental --bitmap &&\n+\t\ttest_commit cruft &&\n+\n+\t\t# Pack the new commit and tree, leaving the blob in cruft.\n+\t\tgit repack -d &&\n+\t\tgit repack --geometric=2 --write-midx=incremental \\\n+\t\t\t--write-bitmap-index &&\n+\t\ttest_line_count = 1 $midx_chain &&\n+\t\tgit rev-list --test-bitmap HEAD\n+\t)\n+'\n+\n test_expect_success 'below layer threshold, tip packs excluded' '\n \tgit init below-layer-threshold-tip-packs-excluded &&\n \t(\n@@ -338,7 +378,7 @@ test_expect_success 'geometric rollup with surviving tip packs' '\n \t)\n '\n \n-test_expect_success 'kept packs are excluded from repack' '\n+test_expect_success 'kept packs are excluded from repack but included in MIDX' '\n \tgit init kept-packs-excluded-from-repack &&\n \t(\n \t\tcd kept-packs-excluded-from-repack &&\n@@ -353,21 +393,20 @@ test_expect_success 'kept packs are excluded from repack' '\n \t\t\ttest_commit \"$i\" && git repack -d || return 1\n \t\tdone &&\n \n-\t\tkeep=$(ls $packdir/pack-*.idx | head -n 1) &&\n-\t\ttouch \"${keep%.idx}.keep\" &&\n+\t\tkeep=$(test-tool find-pack A) &&\n+\t\ttouch \"${keep%.pack}.keep\" &&\n \n-\t\t# The kept pack is excluded as a repacking candidate\n-\t\t# entirely, so no rollup occurs as there is only one\n-\t\t# non-kept pack. A new MIDX layer is written containing\n-\t\t# that pack.\n-\t\tgit repack --geometric=2 -d --write-midx=incremental &&\n+\t\t# Neither pack is repacked, but both are needed for the\n+\t\t# bitmap of B, which reaches objects in the kept pack.\n+\t\tgit repack --geometric=2 -d --write-midx=incremental \\\n+\t\t\t--write-bitmap-index &&\n \n \t\ttest-tool read-midx $objdir >actual &&\n \t\tgrep \"^pack-.*\\.idx$\" actual >actual.packs &&\n-\t\ttest_line_count = 1 actual.packs &&\n-\t\ttest_grep ! \"$keep\" actual.packs &&\n+\t\ttest_line_count = 2 actual.packs &&\n \n \t\tgit multi-pack-index verify &&\n+\t\tgit rev-list --test-bitmap HEAD &&\n \n \t\t# All objects (from both kept and non-kept packs)\n \t\t# must still be accessible.\n-- \n2.56.0.8.ga42f775cbe2\n"},{"id":"553877","messageId":"CABPp-BHE662t9aaNcZ4DZ+2AU_C7jR7_VyHZwt2Tm8JSPE3JZw@mail.gmail.com","threadId":"66424","inReplyTo":"a78a38ca-b08a-4194-b17c-b8802e4d43a7@gmail.com","subject":"Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-10-01T23:22:11Z","receivedAt":"2026-10-01T23:22:23Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 3:23 PM Derrick Stolee wrote:\n>\n> On 9/29/2026 9:28 PM, Taylor Blau wrote:\n>\n> > Add trees and tags from included and '!' packs (and loose ones with\n> > '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues\n> > their descendants even when no input commit reaches them. Walk these\n> > roots after the existing traversal, preserving the `SEEN` bit to avoid\n> > redundant traversals. Ensure that the walk takes place *after* the\n> > existing traversal so that we don't lose the path prefix used for trees\n> > and blobs wherever possible.\n>\n> > @@ -3807,6 +3807,7 @@ static int stdin_packs_hints_nr;\n> > struct stdin_packs_context {\n> > struct rev_info *revs;\n> > enum stdin_packs_mode mode;\n> > + struct oid_array extra_roots;\n>\n> I believe this should be an oidset to avoid adding duplicate objects\n> that appear multiple times. The order of these extra roots doesn't\n> matter (such as in a --topo-order walk). We only care about the\n> binary \"reachable or not?\" question.\n\nIs that true? After iterating with copilot for a while, it says:\n\nThe walk supplies paths to pack-objects, and those paths affect more\nthan reachability. In particular, they affect both namehash-based\ndelta selection and path-based attributes.\n\nFirst, it is possible to demonstrate a pack-quality regression with a\nvanilla repository configuration:\n\ntest_expect_success '--stdin-packs=follow preserves namehash ordering' '\n test_when_finished \"rm -rf repo\" &&\n git init repo &&\n (\n cd repo &&\n\n mkdir sub &&\n test-tool genrandom similar 8192 >sub/a &&\n cp sub/a sub/c &&\n printf x >>sub/c &&\n\n for name in \\\n b yzz9 zzz9 aazz9 abzz9 aczz9 \\\n adyz9 adzz9 aeyz9 aezz9 afyz9\n do\n test-tool genrandom \"unrelated-9-$name\" 8192 >\"$name\" ||\n return 1\n done &&\n git add . &&\n git commit -m base &&\n\n git rev-parse HEAD^{tree} HEAD:sub >in &&\n P=$(git pack-objects --window=0 $packdir/pack in &&\n\n git pack-objects --stdin-packs=follow \\\n --no-reuse-delta $packdir/pack actual &&\n printf \"%s\\n\" \"$(git rev-parse HEAD:sub/c)\" \\\n \"$ZERO_OID\" >expect &&\n test_cmp expect actual\n )\n'\nThe funny-looking names make the test deterministic: their namehashes\nfall between the hashes for a and c, but not between those for sub/a\nand sub/c. There are enough of them to fill the normal default delta\nwindow. --window=0 on the input pack and --no-reuse-delta on the\noutput merely ensure that the test observes the new delta search; the\noutput pack uses the normal default window.\n\nWith the submitted oid_array, the parent-first input-pack order is\npreserved. sub/a and sub/c remain adjacent in the namehash sort, and\nsub/a is written as a six-byte delta against sub/c.\n\nWith the straightforward oidset conversion, hash iteration visits the\nsubtree first. The blobs are named a and c, the unrelated blobs\nseparate them in the namehash sort, and both are written in full. Both\npacks are valid, so this is a pack-quality regression rather than\nrepository corruption.\n\nThere is also a shorter, but admittedly more contrived, example using\npath-based attributes:\n\ntest_expect_success '--stdin-packs=follow preserves paths for attributes' '\n test_when_finished \"rm -rf repo\" &&\n git init repo &&\n (\n cd repo &&\n\n echo \"sub/* -delta\" >.gitattributes &&\n mkdir sub &&\n test-tool genrandom seed-2 8192 >sub/a &&\n cp sub/a sub/b &&\n echo modified >>sub/b &&\n git add . &&\n git commit -m base &&\n\n git rev-parse HEAD^{tree} HEAD:sub >in &&\n P=$(git pack-objects $packdir/pack in &&\n\n git pack-objects --stdin-packs=follow $packdir/pack actual &&\n printf \"%s\\n\" \"$ZERO_OID\" \"$ZERO_OID\" >expect &&\n test_cmp expect actual\n )\n'\nWith the oid_array and parent-first pack order, the blobs are visited\nas sub/a and sub/b, so sub/* -delta applies. With the oidset, the\nsubtree is visited first and the blobs are seen as a and b, so one is\ndelta-compressed. When the root is processed later, the subtree is\nalready marked SEEN and is not revisited with the sub/ prefix.\n\nThe oid_array does not manufacture parent-before-child ordering if the\ninput pack itself has the subtree first; this path information is\nexplicitly best-effort. But it preserves a useful order when one\nexists, whereas an oidset discards it.\n"},{"id":"553880","messageId":"ar8AJLYZVb6sCIO-@com-79390","threadId":"66424","inReplyTo":"CABPp-BHE662t9aaNcZ4DZ+2AU_C7jR7_VyHZwt2Tm8JSPE3JZw@mail.gmail.com","subject":"Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-02T00:51:48Z","receivedAt":"2026-10-02T00:51:59Z","isPatch":true,"body":"On Thu, Oct 01, 2026 at 04:22:11PM -0700, Elijah Newren wrote:\n> With the oid_array and parent-first pack order, the blobs are visited\n> as sub/a and sub/b, so sub/* -delta applies. With the oidset, the\n> subtree is visited first and the blobs are seen as a and b, so one is\n> delta-compressed. When the root is processed later, the subtree is\n> already marked SEEN and is not revisited with the sub/ prefix.\n>\n> The oid_array does not manufacture parent-before-child ordering if the\n> input pack itself has the subtree first; this path information is\n> explicitly best-effort. But it preserves a useful order when one\n> exists, whereas an oidset discards it.\n\nSure, though as Peff and I discussed elsewhere in the thread, there are\nalso situations where you can produce a sub-optimal pack even with\noid_array. That's because the namehash you get for a given tree object\ndepends on the path you took to get there.\n\nSo you can certainly come up with examples where the ordering of tree\nobjects in an array of extra roots produces a lesser-quality delta\nselection than the same objects permuted into some different order.\n\nThe other thing to keep in mind is that, while there are clearly\ntrade-offs as we have discussed here, the oidset ensures that we don't\nallocate memory wastefully when the same object is listed multiple times\nas an extra root.\n\nThe other other thing to keep in mind is that the size of this set is\nalmost always going to be puny compared to the size of the overall pack.\nThese objects are merely meant to pull in the (likely) few objects that\nneed refreshed out of the cruft pack in order to ensure reachability\nclosure.\n\nSo I think it's clear that this is a trade-off, and neither decision\n(oidset vs oid_array) is absolutely perfect for all cases. But on\nbalance I think that the trade-offs push us towards oidset much more\nthan they do towards oid_array.\n\nThanks,\nTaylor\n"},{"id":"554022","messageId":"20261002230225.GA834759@coredump.intra.peff.net","threadId":"66424","inReplyTo":"ar8AJLYZVb6sCIO-@com-79390","subject":"Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-02T23:02:25Z","receivedAt":"2026-10-02T23:02:26Z","isPatch":true,"body":"On Thu, Oct 01, 2026 at 07:51:48PM -0500, Taylor Blau wrote:\n\n> On Thu, Oct 01, 2026 at 04:22:11PM -0700, Elijah Newren wrote:\n> > With the oid_array and parent-first pack order, the blobs are visited\n> > as sub/a and sub/b, so sub/* -delta applies. With the oidset, the\n> > subtree is visited first and the blobs are seen as a and b, so one is\n> > delta-compressed. When the root is processed later, the subtree is\n> > already marked SEEN and is not revisited with the sub/ prefix.\n> >\n> > The oid_array does not manufacture parent-before-child ordering if the\n> > input pack itself has the subtree first; this path information is\n> > explicitly best-effort. But it preserves a useful order when one\n> > exists, whereas an oidset discards it.\n> \n> Sure, though as Peff and I discussed elsewhere in the thread, there are\n> also situations where you can produce a sub-optimal pack even with\n> oid_array. That's because the namehash you get for a given tree object\n> depends on the path you took to get there.\n> \n> So you can certainly come up with examples where the ordering of tree\n> objects in an array of extra roots produces a lesser-quality delta\n> selection than the same objects permuted into some different order.\n\nHmm. Yeah, it is not a 100% solved issue, for sure, but I think Elijah\nhas a point. Even though yes, we may see trees in weird orders between\npacks, or when visited separate from another commit, the ordering in a\nsingle pack _is_ useful, because it puts root trees before subtrees.\n\nSo even though these are a few objects we're rescuing out of a cruft\npack, we'd expect them to be correlated. E.g., an update to \"a/b/c/file\"\nis going to have four trees: the root, a, a/b, and a/b/c. And we'd like\nto visit them in that order. Which is the order in which we'd typically\nwrite them in a pack.\n\nOne thing I'm not 100% on is if that \"typically\" qualifier applies to\ncruft packs. We might be throwing objects in there with a little less\nthought, because the point is that they're _not_ reachable, and we\ndidn't get there from a traversal. So I dunno.\n\n> The other thing to keep in mind is that, while there are clearly\n> trade-offs as we have discussed here, the oidset ensures that we don't\n> allocate memory wastefully when the same object is listed multiple times\n> as an extra root.\n\nYeah, that was my thinking when endorsing the oidset earlier; it is\nbetter bounded. It can have worse memory use in practice, though,\nbecause it's a hash table rather than a vanilla array. So if we don't\nexpect a lot of duplicates, then the simpler array may be more\nefficient.\n\n-Peff\n"},{"id":"554023","messageId":"20261002231336.GB834759@coredump.intra.peff.net","threadId":"66424","inReplyTo":"940953e5c407046ac6789367f3341dc8ea73d07a.1790827875.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-02T23:13:36Z","receivedAt":"2026-10-02T23:13:38Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 11:11:39PM -0500, Taylor Blau wrote:\n\n> @@ -4151,6 +4158,34 @@ static void read_stdin_packs(struct repository *repo,\n> \t\t\t show_object_pack_hint,\n> \t\t\t &mode);\n> \n> +\t/*\n> +\t * Trees and tags need closure even when no commit reaches them.\n> +\t * Defer adding these roots to revs.pending until the first walk\n> +\t * finishes. Otherwise a subtree may be visited and marked SEEN\n> +\t * before its commit's root tree, using \"a\" instead of \"sub/a\"\n> +\t * for a blob's namehash and delta attributes.\n> +\t *\n> +\t * Tags may introduce more commits in the second walk, so this\n> +\t * does not *always* guarantee that trees are always visited\n> +\t * with their full paths.\n> +\t */\n\nI think one of the things that confused me reading the original patch\n(but is still present here) is that this comment is in read_stdin_packs,\nwhen we're actually doing the walks. So \"defer adding these roots\" feels\nquite late. We already did that deferring long before in\nadd_object_entry_from_pack() and add_loose_object(), when we called\noidset_insert() instead of add_pending().\n\nSo it would have made more sense to me to comment it there. Of course\nthat is hard when there are two such places.\n\nI dunno.\n\n> +\toidset_iter_init(&ctx.extra_roots, &iter);\n> +\twhile ((oid = oidset_iter_next(&iter))) {\n> +\t\tstruct object *obj = lookup_object(repo, oid);\n> +\n> +\t\tif (!obj || !(obj->flags & SEEN))\n> +\t\t\tadd_pending_oid(&revs, NULL, oid, 0);\n> +\t}\n> +\tif (revs.pending.nr) {\n> +\t\tif (prepare_revision_walk(&revs))\n> +\t\t\tdie(_(\"revision walk setup failed\"));\n> +\t\ttraverse_commit_list(&revs,\n> +\t\t\t\t show_commit_pack_hint,\n> +\t\t\t\t show_object_pack_hint,\n> +\t\t\t\t &mode);\n> +\t}\n> +\toidset_clear(&ctx.extra_roots);\n> +\n> \trelease_revisions(&revs);\n\nBTW, is it safe to prepare_revision_walk() twice on the same rev_info? I\ncould believe it works, but I could also believe that there are hidden\ncorner cases, as I don't think it was ever really intended to work this\nway.\n\nMaybe OK for the vanilla set of options we are using here (as opposed to\ntaking arbitrary options from the user). The rev_info is created locally\nin this function, though, so I guess if we wanted to be double-plus sure\nwe could release and reinit the struct.\n\n-Peff\n"},{"id":"554024","messageId":"20261002231601.GC834759@coredump.intra.peff.net","threadId":"66424","inReplyTo":"c1ff18bf91363c638536265c600a7ce5ac4e1218.1790827875.git.me@ttaylorr.com","subject":"Re: [PATCH v2 4/8] repack: use a sorted list for explicitly kept packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-02T23:16:01Z","receivedAt":"2026-10-02T23:16:02Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 11:11:47PM -0500, Taylor Blau wrote:\n\n> `existing_packs_collect()` performs a linear search through the\n> '--keep-pack' arguments for each local pack. Typically the number of\n> such arguments is small enough that the difference between a linear and\n> binary search is just noise (especially compared with the amount of work\n> that 'repack' is about to perform).\n> \n> However, an additional caller will wish to search through the same list.\n> To prevent that caller from having to duplicate the clunky for-loop in\n> `existing_packs_collect()`, sort the list using `fspathcmp()` and\n> replace the existing caller's loop with `string_list_has_string()`.\n> \n> This does not change the overall behavior of '--keep-pack' arguments.\n\nOK, makes sense, and the patch looks correct.\n\n-Peff\n"},{"id":"554025","messageId":"20261002232529.GD834759@coredump.intra.peff.net","threadId":"66424","inReplyTo":"51e20444dac1223f0e0485dc5799ed6d592f7614.1790827875.git.me@ttaylorr.com","subject":"Re: [PATCH v2 5/8] repack: follow kept packs when omitting cruft from the MIDX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-02T23:25:29Z","receivedAt":"2026-10-02T23:25:30Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 11:11:51PM -0500, Taylor Blau wrote:\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 88b05e96b5b..27d6668a4ab 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -476,9 +476,11 @@ int cmd_repack(int argc,\n> \tshow_progress = !po_args.quiet && isatty(2);\n> \n> \tstrvec_push(&cmd.args, \"--keep-true-parents\");\n> -\tfor (i = 0; i < keep_pack_list.nr; i++)\n> -\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\",\n> -\t\t\t keep_pack_list.items[i].string);\n> +\t/* Geometric follow walks exclude these packs through stdin instead. */\n> +\tif (!(geometry.split_factor && !midx_must_contain_cruft))\n> +\t\tfor (i = 0; i < keep_pack_list.nr; i++)\n> +\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\",\n> +\t\t\t\t keep_pack_list.items[i].string);\n\nThis conditional makes my head hurt because of the double-negation. By\nDe Morgan's it is just:\n\n if (!geometry.split_factor || midx_must_contain_cruft)\n\nwhich at least untangles it. The comment makes sense to say \"we do not\nneed to do this in geometric\" mode, which matches the first half. But\nwhy does midx_must_contain_cruft trigger it? I guess it is \"we do not\nneed to bother doing the \"^\"-exclusion later in that mode\", but I wonder\nif there is any advantage to suppressing it. I don't remember enough of\nthe details here about why we were treating keep packs specially in the\nfirst place.\n\n> @@ -593,6 +595,29 @@ int cmd_repack(int argc,\n> \n> \t\t\tfprintf(in, \"%c%s\\n\", marker, basename);\n> \t\t}\n> +\t\tif (!midx_must_contain_cruft) {\n\nOK, and this is the flip side of the earlier conditional. We are in\ngeometric mode if we get here, and we kick in only in non-midx-cruft\nmode.\n\nIMHO the De Morgan untangling above makes it more clear, but you could\nprobably even further with:\n\n /* explanatory comment here */\n int handle_keep_packs_via_follow = geometry.split_factor && !midx_must_contain_cruft;\n\nAnd then use that in both spots. That might be overkill, though (and the\nname I proposed certainly sucks).\n\n-Peff\n"},{"id":"554026","messageId":"20261002232834.GE834759@coredump.intra.peff.net","threadId":"66424","inReplyTo":"a85dbcd04c7957756848e5f3102744d20b509fc4.1790827875.git.me@ttaylorr.com","subject":"Re: [PATCH v2 6/8] repack: track the preferred pack explicitly in MIDX write steps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-02T23:28:34Z","receivedAt":"2026-10-02T23:28:35Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 11:11:58PM -0500, Taylor Blau wrote:\n\n> A MIDX write step marks preferred packs in its string-list entries and\n> chooses the last marked entry when executing the step. That makes the\n> choice depend on list order, preventing the list from being sorted for\n> membership checks.\n> \n> Record the last candidate directly in the step, borrowing its name from\n> the write list. This preserves preferred-pack selection while allowing\n> the list to be sorted without changing that choice.\n\nThis is certainly cleaner, though it looks like the existing code works\nby marking item->util and then doing a linear search for it. So wouldn't\nthat work even after sorting?\n\n> @@ -719,7 +713,7 @@ static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts,\n> \n> \t\titem = string_list_append(&step.u.write, buf.buf);\n> \t\tif (p->multi_pack_index || i == opts->geometry->pack_nr - 1)\n> -\t\t\titem->util = (void *)1; /* mark as preferred */\n> +\t\t\tstep.preferred_pack = item->string;\n\nI am certainly happy to see these gross casts go away, though.\n\n-Peff\n"},{"id":"554027","messageId":"20261002234157.GF834759@coredump.intra.peff.net","threadId":"66424","inReplyTo":"a42f775cbe27b385bfc8ff38f33604b3913dc340.1790827875.git.me@ttaylorr.com","subject":"Re: [PATCH v2 8/8] repack: include required packs in incremental MIDX writes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-02T23:41:57Z","receivedAt":"2026-10-02T23:41:59Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 11:12:05PM -0500, Taylor Blau wrote:\n\n> The geometric plan from 1da62fb5c86 (repack: implement incremental MIDX\n> repacking, 2026-05-19) can omit kept and cruft packs, since neither\n> necessarily participates in the geometric repack. Such packs can also be\n> lost when replacing a tip layer that contains them. Neither plan\n> consults `midx_included_packs()`, so the rules for retaining cruft in\n> ordinary MIDX writes do not protect incremental writes.\n> \n> Use that selection logic to add missing packs to each plan's write step.\n> Skip packs in retained base layers, but include required packs from a\n> replaced tip. Count added objects when choosing which layers to compact,\n> without changing the preferred pack.\n\nI admit I had a hard time following this patch. I think the point is\nthat we're going to include some packs in the midx that were not covered\npreviously. But it was hard to see where that happens. I think the magic\nbit is this:\n\n> @@ -557,17 +604,20 @@ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,\n> \t\t\t\t\t size_t *steps_nr_p)\n> [..]\n> -\tfor (i = 0; i < opts->names->nr; i++) {\n> +\tmidx_included_packs(&include, opts, m);\n\nwhere we rely on midx_included_packs() to do that selection.\n\nSo I _think_ this is doing the right thing, but my confidence in my\nreview is kind of low. To some degree I'd just rely on the functional\ntests here.\n\n-Peff\n"},{"id":"554029","messageId":"asBRayrg2RvzjevI@com-79390","threadId":"66424","inReplyTo":"20261002232529.GD834759@coredump.intra.peff.net","subject":"Re: [PATCH v2 5/8] repack: follow kept packs when omitting cruft from the MIDX","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-03T00:50:51Z","receivedAt":"2026-10-03T00:51:01Z","isPatch":true,"body":"On Fri, Oct 02, 2026 at 07:25:29PM -0400, Jeff King wrote:\n> On Wed, Sep 30, 2026 at 11:11:51PM -0500, Taylor Blau wrote:\n>\n> > diff --git a/builtin/repack.c b/builtin/repack.c\n> > index 88b05e96b5b..27d6668a4ab 100644\n> > --- a/builtin/repack.c\n> > +++ b/builtin/repack.c\n> > @@ -476,9 +476,11 @@ int cmd_repack(int argc,\n> > \tshow_progress = !po_args.quiet && isatty(2);\n> >\n> > \tstrvec_push(&cmd.args, \"--keep-true-parents\");\n> > -\tfor (i = 0; i < keep_pack_list.nr; i++)\n> > -\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\",\n> > -\t\t\t keep_pack_list.items[i].string);\n> > +\t/* Geometric follow walks exclude these packs through stdin instead. */\n> > +\tif (!(geometry.split_factor && !midx_must_contain_cruft))\n> > +\t\tfor (i = 0; i < keep_pack_list.nr; i++)\n> > +\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\",\n> > +\t\t\t\t keep_pack_list.items[i].string);\n>\n> This conditional makes my head hurt because of the double-negation. By\n> De Morgan's it is just:\n>\n> if (!geometry.split_factor || midx_must_contain_cruft)\n\nYeah, I struggled a bit when writing it TBH and flip-flopped between the\ntwo. I read the conditional (as proposed in my patch) as:\n\n \"If we aren't doing a geometric repack where the MIDX is allowed to\n omit cruft objects\".\n\nBut I think the original sin here is midx_must_contain_cruft, which\nprobably should have been midx_may_exclude_cruft, which defaults to\nfalse as opposed to the former which defaults to true.\n\nIt's not quite a double negation, but I agree that it's a little\nawkward. TBH I find the rewritten version just as confusing if not more\nso.\n\nThanks,\nTaylor\n"},{"id":"554030","messageId":"asBShFkQRWJX4RQU@com-79390","threadId":"66424","inReplyTo":"20261002231336.GB834759@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-03T00:55:32Z","receivedAt":"2026-10-03T00:55:40Z","isPatch":true,"body":"On Fri, Oct 02, 2026 at 07:13:36PM -0400, Jeff King wrote:\n> So it would have made more sense to me to comment it there. Of course\n> that is hard when there are two such places.\n>\n> I dunno.\n\nYeah, me either. I'm happy to change things around if you feel strongly.\n\n> > +\toidset_iter_init(&ctx.extra_roots, &iter);\n> > +\twhile ((oid = oidset_iter_next(&iter))) {\n> > +\t\tstruct object *obj = lookup_object(repo, oid);\n> > +\n> > +\t\tif (!obj || !(obj->flags & SEEN))\n> > +\t\t\tadd_pending_oid(&revs, NULL, oid, 0);\n> > +\t}\n> > +\tif (revs.pending.nr) {\n> > +\t\tif (prepare_revision_walk(&revs))\n> > +\t\t\tdie(_(\"revision walk setup failed\"));\n> > +\t\ttraverse_commit_list(&revs,\n> > +\t\t\t\t show_commit_pack_hint,\n> > +\t\t\t\t show_object_pack_hint,\n> > +\t\t\t\t &mode);\n> > +\t}\n> > +\toidset_clear(&ctx.extra_roots);\n> > +\n> > \trelease_revisions(&revs);\n>\n> BTW, is it safe to prepare_revision_walk() twice on the same rev_info? I\n> could believe it works, but I could also believe that there are hidden\n> corner cases, as I don't think it was ever really intended to work this\n> way.\n>\n> Maybe OK for the vanilla set of options we are using here (as opposed to\n> taking arbitrary options from the user). The rev_info is created locally\n> in this function, though, so I guess if we wanted to be double-plus sure\n> we could release and reinit the struct.\n\nIt seems to work in practice. From reading through and thinking about it\nI couldn't find any obvious issues.\n\nJust as well, there are a couple of spots that I was able to find that\nalready call `prepare_revision_walk()` more than once:\n\n * In builtin/pack-objects.c::get_object_list() (with the exception of\n '--path-walk') we call `prepare_revision_walk()` twice when\n exploding unreachable objects as loose.\n\n * In reachable.c::mark_reachable_objects(), we also call the\n `prepare_revision_walk()` function twice when given a timestamp via\n `mark_recent`.\n\nThis all works since `revs.pending` is emptied by the first revwalk. But\nit is under-documented, so callers relying on this behavior may be\nsurprised if/when it changes. Probably good #leftoverbits.\n\nThanks,\nTaylor\n"},{"id":"554031","messageId":"asBTxLW9j2AIVlxZ@com-79390","threadId":"66424","inReplyTo":"20261002232834.GE834759@coredump.intra.peff.net","subject":"Re: [PATCH v2 6/8] repack: track the preferred pack explicitly in MIDX write steps","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-03T01:00:52Z","receivedAt":"2026-10-03T01:00:59Z","isPatch":true,"body":"On Fri, Oct 02, 2026 at 07:28:34PM -0400, Jeff King wrote:\n> On Wed, Sep 30, 2026 at 11:11:58PM -0500, Taylor Blau wrote:\n>\n> > A MIDX write step marks preferred packs in its string-list entries and\n> > chooses the last marked entry when executing the step. That makes the\n> > choice depend on list order, preventing the list from being sorted for\n> > membership checks.\n> >\n> > Record the last candidate directly in the step, borrowing its name from\n> > the write list. This preserves preferred-pack selection while allowing\n> > the list to be sorted without changing that choice.\n>\n> This is certainly cleaner, though it looks like the existing code works\n> by marking item->util and then doing a linear search for it. So wouldn't\n> that work even after sorting?\n\nIt would if only one entry were marked, but we can mark several.\n\nFor example, when `repack_make_midx_compaction_plan()` folds multiple\nMIDX layers into one via a WRITE step, it marks each layer's preferred pack\nwithout clearing the earlier marks. The scan doesn't stop at the first\nsuch mark, and the last marked entry wins.\n\nSo sorting would of course preserve the marks, but may change which one\ncomes last.\n\n> > @@ -719,7 +713,7 @@ static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts,\n> >\n> > \t\titem = string_list_append(&step.u.write, buf.buf);\n> > \t\tif (p->multi_pack_index || i == opts->geometry->pack_nr - 1)\n> > -\t\t\titem->util = (void *)1; /* mark as preferred */\n> > +\t\t\tstep.preferred_pack = item->string;\n>\n> I am certainly happy to see these gross casts go away, though.\n\nMe too ;-).\n\nThanks,\nTaylor\n"},{"id":"554032","messageId":"asBUBwM2N8lQM602@com-79390","threadId":"66424","inReplyTo":"20261002234157.GF834759@coredump.intra.peff.net","subject":"Re: [PATCH v2 8/8] repack: include required packs in incremental MIDX writes","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-10-03T01:01:59Z","receivedAt":"2026-10-03T01:02:04Z","isPatch":true,"body":"On Fri, Oct 02, 2026 at 07:41:57PM -0400, Jeff King wrote:\n> On Wed, Sep 30, 2026 at 11:12:05PM -0500, Taylor Blau wrote:\n>\n> > The geometric plan from 1da62fb5c86 (repack: implement incremental MIDX\n> > repacking, 2026-05-19) can omit kept and cruft packs, since neither\n> > necessarily participates in the geometric repack. Such packs can also be\n> > lost when replacing a tip layer that contains them. Neither plan\n> > consults `midx_included_packs()`, so the rules for retaining cruft in\n> > ordinary MIDX writes do not protect incremental writes.\n> >\n> > Use that selection logic to add missing packs to each plan's write step.\n> > Skip packs in retained base layers, but include required packs from a\n> > replaced tip. Count added objects when choosing which layers to compact,\n> > without changing the preferred pack.\n>\n> I admit I had a hard time following this patch. I think the point is\n> that we're going to include some packs in the midx that were not covered\n> previously. But it was hard to see where that happens. I think the magic\n> bit is this:\n>\n> > @@ -557,17 +604,20 @@ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,\n> > \t\t\t\t\t size_t *steps_nr_p)\n> > [..]\n> > -\tfor (i = 0; i < opts->names->nr; i++) {\n> > +\tmidx_included_packs(&include, opts, m);\n>\n> where we rely on midx_included_packs() to do that selection.\n\nYeah, that's right. I wrote this code in the first place, and it wasn't\neven *that* long ago and I had to spend a not-insignificant amount of\ntime (re)acquainting myself with this area before writing this patch.\n\nThanks,\nTaylor\n"},{"id":"554033","messageId":"20261003010613.GA839051@coredump.intra.peff.net","threadId":"66424","inReplyTo":"asBShFkQRWJX4RQU@com-79390","subject":"Re: [PATCH v2 2/8] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-03T01:06:13Z","receivedAt":"2026-10-03T01:06:14Z","isPatch":true,"body":"On Fri, Oct 02, 2026 at 07:55:32PM -0500, Taylor Blau wrote:\n\n> On Fri, Oct 02, 2026 at 07:13:36PM -0400, Jeff King wrote:\n> > So it would have made more sense to me to comment it there. Of course\n> > that is hard when there are two such places.\n> >\n> > I dunno.\n> \n> Yeah, me either. I'm happy to change things around if you feel strongly.\n\nI don't. If there were an easy solution I probably would. ;)\n\n> > BTW, is it safe to prepare_revision_walk() twice on the same rev_info? I\n> > could believe it works, but I could also believe that there are hidden\n> > corner cases, as I don't think it was ever really intended to work this\n> > way.\n> [...]\n> Just as well, there are a couple of spots that I was able to find that\n> already call `prepare_revision_walk()` more than once:\n\nOK. That makes me feel like we're in good company, at least. If some\ncombination turns out to be a problem, we can deal with it later.\n\n> * In builtin/pack-objects.c::get_object_list() (with the exception of\n> '--path-walk') we call `prepare_revision_walk()` twice when\n> exploding unreachable objects as loose.\n> \n> * In reachable.c::mark_reachable_objects(), we also call the\n> `prepare_revision_walk()` function twice when given a timestamp via\n> `mark_recent`.\n\nI have a feeling that least one of those is my fault, too. ;)\n\n-Peff\n"},{"id":"554034","messageId":"20261003010720.GB839051@coredump.intra.peff.net","threadId":"66424","inReplyTo":"asBTxLW9j2AIVlxZ@com-79390","subject":"Re: [PATCH v2 6/8] repack: track the preferred pack explicitly in MIDX write steps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-03T01:07:20Z","receivedAt":"2026-10-03T01:07:22Z","isPatch":true,"body":"On Fri, Oct 02, 2026 at 08:00:52PM -0500, Taylor Blau wrote:\n\n> On Fri, Oct 02, 2026 at 07:28:34PM -0400, Jeff King wrote:\n> > On Wed, Sep 30, 2026 at 11:11:58PM -0500, Taylor Blau wrote:\n> >\n> > > A MIDX write step marks preferred packs in its string-list entries and\n> > > chooses the last marked entry when executing the step. That makes the\n> > > choice depend on list order, preventing the list from being sorted for\n> > > membership checks.\n> > >\n> > > Record the last candidate directly in the step, borrowing its name from\n> > > the write list. This preserves preferred-pack selection while allowing\n> > > the list to be sorted without changing that choice.\n> >\n> > This is certainly cleaner, though it looks like the existing code works\n> > by marking item->util and then doing a linear search for it. So wouldn't\n> > that work even after sorting?\n> \n> It would if only one entry were marked, but we can mark several.\n> \n> For example, when `repack_make_midx_compaction_plan()` folds multiple\n> MIDX layers into one via a WRITE step, it marks each layer's preferred pack\n> without clearing the earlier marks. The scan doesn't stop at the first\n> such mark, and the last marked entry wins.\n> \n> So sorting would of course preserve the marks, but may change which one\n> comes last.\n\nOK, that does make more sense. Re-reading your commit message again, I\nsee it even says that, but somehow it didn't quite sink in the first\ntime for me.\n\n-Peff\n"}]}