repack: various corner cases for cruft-less MIDXs

This patch series fixes a few bugs I spotted while investigating the cruft-less MIDX feature.

The bugs addressed are found in various corner cases, and, when triggered, may result in a MIDX being written whose objects are not closed under reachability. When this happens while the caller is trying to write reachability bitmaps, bitmap generation may fail if one or more selected commits are descendants of the open portion of the MIDX.

The series is structured as follows:

* The first patch is a preparatory refactoring to add a context struct within pack-objects' handling of '--stdin-packs' to minimize the diff in the subsequent patch.

* The second patch fixes a case where once-cruft tree and annotated tag objects may prevent reachability closure when objects reachable from them are not present in the input pack(s).

* The third patch fixes a case where incremental repack operations may introduce the same bug when the pack generated by an incremental repack does not pack an additional copy of once-cruft object(s).

* The fourth and final patch addresses a similar case involving .keep packs.

Thanks in advance for reviewing!

Taylor Blau (4):
pack-objects: introduce stdin_packs_context struct
pack-objects: ensure tree/tag closure with '--stdin-packs=follow'
repack: retain cruft packs in MIDXs after incremental repacks
repack: retain cruft packs in MIDXs containing kept packs

 Documentation/git-pack-objects.adoc |  2 +
 builtin/pack-objects.c              | 73 ++++++++++++++++++++------
 builtin/repack.c                    |  6 +++
 repack-midx.c                       |  5 ++
 t/t5331-pack-objects-stdin.sh       | 81 +++++++++++++++++++++++++++++
 t/t7704-repack-cruft.sh             | 51 ++++++++++++++++++
 6 files changed, 202 insertions(+), 16 deletions(-)

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4

2.56.0.4.gbee41d2fc68

stdin_packs_read_input() currently receives a pointer to the rev_info struct and '--stdin-packs' mode separately as arguments, but the object enumeration callbacks only receive a pointer to the rev_info struct.

Wrap the pair in a new stdin_packs_context struct so that a future change may reference the '--stdin-packs' mode within the various object enumeration callbacks.

---
 builtin/pack-objects.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

index af9390a46b9..01adf80a2bc 100644
@@ -3804,11 +3804,17 @@ static int git_pack_config(const char *k, const char *v,
 static int stdin_packs_found_nr;
 static int stdin_packs_hints_nr;
 
+struct stdin_packs_context {
+	struct rev_info *revs;
+	enum stdin_packs_mode mode;
+};
+
 static int add_object_entry_from_pack(const struct object_id *oid,
 				      struct packed_git *p,
 				      uint32_t pos,
 				      void *_data)
 {
+	struct stdin_packs_context *ctx = _data;
 	off_t ofs;
 	struct object_info oi = OBJECT_INFO_INIT;
 	enum object_type type = OBJ_NONE;

@@ -3827,7 +3833,6 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 		die(_("could not get type of object %s in pack %s"),
 		    oid_to_hex(oid), p->pack_name);
 	} else if (type == OBJ_COMMIT) {
-		struct rev_info *revs = _data;
 		/*
 		 * commits in included packs are used as starting points
 		 * for the subsequent revision walk

@@ -3840,7 +3845,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 		 * However, we'll only add those objects to the packing
 		 * list after checking `want_object_in_pack()` below.
 		 */
-		add_pending_oid(revs, NULL, oid, 0);
+		add_pending_oid(ctx->revs, NULL, oid, 0);
 	}
 
 	if (!want_object_in_pack(oid, 0, &p, &ofs))

@@ -3954,8 +3959,9 @@ static int stdin_packs_include_check(struct commit *commit, void *data)
 }
 
 static void stdin_packs_add_pack_entries(struct strmap *packs,
-					 struct rev_info *revs)
+					 struct stdin_packs_context *ctx)
 {
+	struct rev_info *revs = ctx->revs;
 	struct string_list keys = STRING_LIST_INIT_NODUP;
 	struct string_list_item *item;
 	struct hashmap_iter iter;

@@ -3994,15 +4000,14 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,
 		    (info->kind & STDIN_PACK_EXCLUDE_OPEN))
 			for_each_object_in_pack(info->p,
 						add_object_entry_from_pack,
-						revs,
+						ctx,
 						ODB_FOR_EACH_OBJECT_PACK_ORDER);
 	}
 
 	string_list_clear(&keys, 0);
 }
 
-static void stdin_packs_read_input(struct rev_info *revs,
-				   enum stdin_packs_mode mode)
+static void stdin_packs_read_input(struct stdin_packs_context *ctx)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct strmap packs = STRMAP_INIT;

@@ -4017,7 +4022,7 @@ static void stdin_packs_read_input(struct rev_info *revs,
 			continue;
 		else if (*key == '^')
 			kind = STDIN_PACK_EXCLUDE_CLOSED;
-		else if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW)
+		else if (*key == '!' && ctx->mode == STDIN_PACKS_MODE_FOLLOW)
 			kind = STDIN_PACK_EXCLUDE_OPEN;
 
 		if (kind != STDIN_PACK_INCLUDE)

@@ -4082,19 +4087,23 @@ static void stdin_packs_read_input(struct rev_info *revs,
 		info->p = p;
 	}
 
-	stdin_packs_add_pack_entries(&packs, revs);
+	stdin_packs_add_pack_entries(&packs, ctx);
 
 	strbuf_release(&buf);
 	strmap_clear(&packs, 1);
 }
 
-static void add_unreachable_loose_objects(struct rev_info *revs);
+static void add_unreachable_loose_objects(struct stdin_packs_context *ctx);
 
 static void read_stdin_packs(struct repository *repo,
 			     enum stdin_packs_mode mode, int rev_list_unpacked)
 {
 	int prev_fetch_if_missing = repo->fetch_if_missing;
 	struct rev_info revs;
+	struct stdin_packs_context ctx = {
+		.revs = &revs,
+		.mode = mode,
+	};
 
 	/*
 	 * The revision walk may hit objects that are promised, only. As the

@@ -4131,9 +4140,9 @@ static void read_stdin_packs(struct repository *repo,
 		 */
 		ignore_packed_keep_in_core_open = 1;
 	}
-	stdin_packs_read_input(&revs, mode);
+	stdin_packs_read_input(&ctx);
 	if (rev_list_unpacked)
-		add_unreachable_loose_objects(&revs);
+		add_unreachable_loose_objects(&ctx);
 
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));

@@ -4541,7 +4550,7 @@ static void add_objects_in_unpacked_packs(void)
 static int add_loose_object(const struct object_id *oid, const char *path,
 			    void *data)
 {
-	struct rev_info *revs = data;
+	struct stdin_packs_context *ctx = data;
 	enum object_type type = odb_read_object_info(the_repository->objects, oid, NULL);
 
 	if (type < 0) {

@@ -4563,8 +4572,8 @@ static int add_loose_object(const struct object_id *oid, const char *path,
 		add_object_entry(oid, type, "", 0);
 	}
 
-	if (revs && type == OBJ_COMMIT)
-		add_pending_oid(revs, NULL, oid, 0);
+	if (ctx && type == OBJ_COMMIT)
+		add_pending_oid(ctx->revs, NULL, oid, 0);
 
 	return 0;
 }

@@ -4574,10 +4583,10 @@ static int add_loose_object(const struct object_id *oid, const char *path,
  * add_object_entry will weed out duplicates, so we just add every
  * loose object we find.
  */
-static void add_unreachable_loose_objects(struct rev_info *revs)
+static void add_unreachable_loose_objects(struct stdin_packs_context *ctx)
 {
 	for_each_loose_file_in_source(the_repository->objects->sources,
-				      add_loose_object, NULL, NULL, revs);
+				      add_loose_object, NULL, NULL, ctx);
 }
 
 static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)

2.56.0.4.gbee41d2fc68

;-)

That's fair. For the following round, I added a small note next to the 'revs' member in the struct's definition to indicate that it must be non-NULL.

Thanks,
Taylor

Since 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where possible, 2025-06-23), when the 'repack.midxMustContainCruft' configuration is set to "false", geometric repacks use '--stdin-packs=follow' to copy needed objects out of cruft packs so the MIDX can omit those packs.

In cd846bacc7d (pack-objects: introduce '--stdin-packs=follow', 2025-06-23), this behavior changed such that whenever excluded-open ('!') packs are present, the walk stops at objects in excluded-closed ('^') packs. Geometric repacks use '^' for retained packs already in the MIDX, relying on the indexed object set being closed under reachability.

However, the walk introduced in cd846bacc7d starts only from commit objects. A geometric repack can therefore produce a MIDX that does not maintain reachability closure for lone trees (that are not reachable from any commit otherwise in the closure).

A later walk with '!' packs can stop at that tree in a retained '^' pack even if a new commit reaches it. If the cruft pack remains excluded, and the bitmap selection picks one or more commits which reach that tree, the MIDX cannot generate a bitmap for that commit.

Add trees and tags from included and '!' packs (and loose ones with '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues their descendants even when no input commit reaches them. Walk these roots after the existing traversal, preserving the SEEN bit to avoid redundant traversals. Ensure that the walk takes place *after* the existing traversal so that we don't lose the path prefix used for trees and blobs wherever possible.

Objects in '^' packs remain cutoffs to avoid rewalking packs that are known to be closed under reachability.

---
 Documentation/git-pack-objects.adoc |  2 +
 builtin/pack-objects.c              | 32 ++++++++++++
 t/t5331-pack-objects-stdin.sh       | 81 +++++++++++++++++++++++++++++
 t/t7704-repack-cruft.sh             | 20 +++++++
 4 files changed, 135 insertions(+)

index 65cd00c152f..1564d44f49d 100644
@@ -112,6 +112,8 @@ pack may include additional objects based on the following:
 This mode is useful, for example, to resurrect once-unreachable
 objects found in cruft packs to generate packs which are closed under
 reachability up to the boundary set by the excluded packs.
+Trees and tags in included or `!` packs are followed even when no
+commit reaches them, as are loose trees and tags with `--unpacked`.
 +
 Incompatible with `--revs`, or options that imply `--revs` (such as
 `--all`), with the exception of `--unpacked`, which is compatible.

index 01adf80a2bc..05a94305265 100644
@@ -3807,6 +3807,7 @@ static int stdin_packs_hints_nr;
 struct stdin_packs_context {
 	struct rev_info *revs;
 	enum stdin_packs_mode mode;
+	struct oid_array extra_roots;
 };
 
 static int add_object_entry_from_pack(const struct object_id *oid,

@@ -3846,6 +3847,9 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 		 * list after checking `want_object_in_pack()` below.
 		 */
 		add_pending_oid(ctx->revs, NULL, oid, 0);
+	} else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW &&
+		   (type == OBJ_TREE || type == OBJ_TAG)) {
+		oid_array_append(&ctx->extra_roots, oid);
 	}
 
 	if (!want_object_in_pack(oid, 0, &p, &ofs))

@@ -4103,6 +4107,7 @@ static void read_stdin_packs(struct repository *repo,
 	struct stdin_packs_context ctx = {
 		.revs = &revs,
 		.mode = mode,
+		.extra_roots = OID_ARRAY_INIT,
 	};
 
 	/*

@@ -4151,6 +4156,30 @@ static void read_stdin_packs(struct repository *repo,
 			     show_object_pack_hint,
 			     &mode);
 
+	/*
+	 * Trees and tags need closure even when no commit reaches them.
+	 * Defer adding these roots to revs.pending until the commit walk
+	 * finishes. Otherwise a subtree may be visited and marked SEEN
+	 * before its commit's root tree, using "a" instead of "sub/a" for
+	 * a blob's namehash and delta attributes.
+	 */
+	for (size_t i = 0; i < ctx.extra_roots.nr; i++) {
+		const struct object_id *oid = &ctx.extra_roots.oid[i];
+		struct object *obj = lookup_object(repo, oid);
+
+		if (!obj || !(obj->flags & SEEN))
+			add_pending_oid(&revs, NULL, oid, 0);
+	}
+	if (revs.pending.nr) {
+		if (prepare_revision_walk(&revs))
+			die(_("revision walk setup failed"));
+		traverse_commit_list(&revs,
+				     show_commit_pack_hint,
+				     show_object_pack_hint,
+				     &mode);
+	}
+	oid_array_clear(&ctx.extra_roots);
+
 	release_revisions(&revs);
 
 	trace2_data_intmax("pack-objects", the_repository, "stdin_packs_found",

@@ -4574,6 +4603,9 @@ static int add_loose_object(const struct object_id *oid, const char *path,
 
 	if (ctx && type == OBJ_COMMIT)
 		add_pending_oid(ctx->revs, NULL, oid, 0);
+	else if (ctx && ctx->mode == STDIN_PACKS_MODE_FOLLOW &&
+		 (type == OBJ_TREE || type == OBJ_TAG))
+		oid_array_append(&ctx->extra_roots, oid);
 
 	return 0;
 }

index b342e82447d..b49f22878f7 100755
@@ -767,6 +767,26 @@ test_expect_success 'repack --write-midx excludes cruft where possible' '
 	)
 '
 
+test_expect_success 'geometric repack rescues descendants of loose trees' '
+	git init loose-tree-cruft &&
+	(
+		cd loose-tree-cruft &&
+		git config repack.midxMustContainCruft false &&
+		test_commit base &&
+		blob=$(echo cruft | git hash-object -w --stdin) &&
+		GIT_TEST_MULTI_PACK_INDEX=0 git repack --cruft -d &&
+
+		printf "100644 blob %s\tfile\n" "$blob" | git mktree &&
+		GIT_TEST_MULTI_PACK_INDEX=0 git repack -d --geometric=2 \
+			--write-midx --write-bitmap-index &&
+
+		test-tool read-midx --show-objects $objdir >midx &&
+		cruft=$(ls $packdir/*.mtimes) &&
+		test_grep ! "$(basename "$cruft" .mtimes).idx" midx &&
+		test_grep "^$blob " midx
+	)
+'
+
 test_expect_success 'repack --write-midx includes cruft when instructed' '
 	setup_cruft_exclude_tests exclude-cruft-when-instructed &&
 	(

2.56.0.4.gbee41d2fc68

Great suggestion! I adjusted it in the following round.

Thanks,
Taylor

Is that true? After iterating with copilot for a while, it says:

The walk supplies paths to pack-objects, and those paths affect more than reachability. In particular, they affect both namehash-based delta selection and path-based attributes.

First, it is possible to demonstrate a pack-quality regression with a vanilla repository configuration:

test_expect_success '--stdin-packs=follow preserves namehash ordering' '

    test_when_finished "rm -rf repo" &&
    git init repo &&
    (
        cd repo &&

        mkdir sub &&
        test-tool genrandom similar 8192 >sub/a &&
        cp sub/a sub/c &&
        printf x >>sub/c &&

        for name in \
            b yzz9 zzz9 aazz9 abzz9 aczz9 \
            adyz9 adzz9 aeyz9 aezz9 afyz9
        do
            test-tool genrandom "unrelated-9-$name" 8192 >"$name" ||
            return 1
        done &&
        git add . &&
        git commit -m base &&

        git rev-parse HEAD^{tree} HEAD:sub >in &&
        P=$(git pack-objects --window=0 $packdir/pack <in) &&
        echo "pack-$P.pack" >in &&

        git pack-objects --stdin-packs=follow \
            --no-reuse-delta $packdir/pack <in &&
        rm "$packdir/pack-$P.pack" "$packdir/pack-$P.idx" &&
        git prune-packed &&

        printf "%s\n" HEAD:sub/a HEAD:sub/c |
            git cat-file --batch-check="%(deltabase)" >actual &&
        printf "%s\n" "$(git rev-parse HEAD:sub/c)" \
            "$ZERO_OID" >expect &&
        test_cmp expect actual
    )

'
The funny-looking names make the test deterministic: their namehashes fall between the hashes for a and c, but not between those for sub/a and sub/c. There are enough of them to fill the normal default delta window. --window=0 on the input pack and --no-reuse-delta on the output merely ensure that the test observes the new delta search; the output pack uses the normal default window.

With the submitted oid_array, the parent-first input-pack order is preserved. sub/a and sub/c remain adjacent in the namehash sort, and sub/a is written as a six-byte delta against sub/c.

With the straightforward oidset conversion, hash iteration visits the subtree first. The blobs are named a and c, the unrelated blobs separate them in the namehash sort, and both are written in full. Both packs are valid, so this is a pack-quality regression rather than repository corruption.

There is also a shorter, but admittedly more contrived, example using path-based attributes:

test_expect_success '--stdin-packs=follow preserves paths for attributes' '

    test_when_finished "rm -rf repo" &&
    git init repo &&
    (
        cd repo &&

        echo "sub/* -delta" >.gitattributes &&
        mkdir sub &&
        test-tool genrandom seed-2 8192 >sub/a &&
        cp sub/a sub/b &&
        echo modified >>sub/b &&
        git add . &&
        git commit -m base &&

        git rev-parse HEAD^{tree} HEAD:sub >in &&
        P=$(git pack-objects $packdir/pack <in) &&
        echo "pack-$P.pack" >in &&

        git pack-objects --stdin-packs=follow $packdir/pack <in &&
        git prune-packed &&

        printf "%s\n" HEAD:sub/a HEAD:sub/b |
            git cat-file --batch-check="%(deltabase)" >actual &&
        printf "%s\n" "$ZERO_OID" "$ZERO_OID" >expect &&
        test_cmp expect actual
    )

'
With the oid_array and parent-first pack order, the blobs are visited as sub/a and sub/b, so sub/* -delta applies. With the oidset, the subtree is visited first and the blobs are seen as a and b, so one is delta-compressed. When the root is processed later, the subtree is already marked SEEN and is not revisited with the sub/ prefix.

The oid_array does not manufacture parent-before-child ordering if the input pack itself has the subtree first; this path information is explicitly best-effort. But it preserves a useful order when one exists, whereas an oidset discards it.

Sure, though as Peff and I discussed elsewhere in the thread, there are also situations where you can produce a sub-optimal pack even with oid_array. That's because the namehash you get for a given tree object depends on the path you took to get there.

So you can certainly come up with examples where the ordering of tree objects in an array of extra roots produces a lesser-quality delta selection than the same objects permuted into some different order.

The other thing to keep in mind is that, while there are clearly trade-offs as we have discussed here, the oidset ensures that we don't allocate memory wastefully when the same object is listed multiple times as an extra root.

The other other thing to keep in mind is that the size of this set is almost always going to be puny compared to the size of the overall pack. These objects are merely meant to pull in the (likely) few objects that need refreshed out of the cruft pack in order to ensure reachability closure.

So I think it's clear that this is a trade-off, and neither decision (oidset vs oid_array) is absolutely perfect for all cases. But on balance I think that the trade-offs push us towards oidset much more than they do towards oid_array.

Thanks,
Taylor

Hmm. Yeah, it is not a 100% solved issue, for sure, but I think Elijah has a point. Even though yes, we may see trees in weird orders between packs, or when visited separate from another commit, the ordering in a single pack _is_ useful, because it puts root trees before subtrees.

So even though these are a few objects we're rescuing out of a cruft pack, we'd expect them to be correlated. E.g., an update to "a/b/c/file" is going to have four trees: the root, a, a/b, and a/b/c. And we'd like to visit them in that order. Which is the order in which we'd typically write them in a pack.

One thing I'm not 100% on is if that "typically" qualifier applies to cruft packs. We might be throwing objects in there with a little less thought, because the point is that they're _not_ reachable, and we didn't get there from a traversal. So I dunno.

Yeah, that was my thinking when endorsing the oidset earlier; it is better bounded. It can have worse memory use in practice, though, because it's a hash table rather than a vanilla array. So if we don't expect a lot of duplicates, then the simpler array may be more efficient.

-Peff

OK. It took me a minute to grok this, and what I got hung up on is "a later walk". I thought you meant a later walk within the same process, but you mean "a subsequent repack / midx generation".

So we fail to walk in an earlier repack, but we might not fail there because no bitmapped commit happens to require that closure. But we've set up a timebomb for that later repack, because our pack which is _supposed_ to be closed (and thus gets marked with "^") is broken.

So this fixes the initial generation of that timebomb. It doesn't help us deal with existing bombs, but presumably the solution there is a full repack (and we would not want to deal with existing bombs, because the point of "^" is that we can trust it and avoid lots of extra traversal).

Not really asking for a change to the commit message, but just documenting my understanding (which hopefully matches yours ;) ).

OK, that makes sense, as we should treat them the same as commits.

Yup, exactly. Hopefully s/walk/repack/ clarifies things for the following round, but in the meantime your understanding matches my own.

When this feature was originally introduced, the idea was "anything packed must also pack its reachability closure, less any objects in excluded packs". That was true for commit objects, but not so for trees and annotated tags, which is what this patch corrects.

Exactly.

Yeah, that's what I was worried about when I wrote this patch, but that's a good point. Really there is no "absolute" correct path for a given tree or tree entry, since it depends on your perspective.

;-).

I tightened up the comment a bit, but I agree that rethinking the traversal machinery is best left for another day.

Thanks,
Taylor

An incremental repack can write a commit and tree into a new pack while leaving objects they reach in an existing cruft pack. For example, a commit can make a previously unreachable blob reachable again. Since 'repack' will invoke 'pack-objects' with '--incremental', it will not copy the blob out of its cruft pack.

When the 'repack.midxMustContainCruft' configuration is set to "false", writing the first MIDX after such a repack may omit that cruft pack. The new pack bypasses the !names.nr fallback, and there are no previous MIDX packs for midx_has_unknown_packs() to check. Selecting the new commit for bitmap coverage then fails because its reachable objects are not all in the MIDX.

The omission dates all the way back to 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where possible, 2025-06-23). It relies on geometric repacking to copy once-cruft objects with '--stdin-packs=follow'. However, an ordinary incremental repack makes no such guarantee. Require the MIDX to include cruft packs in that case, even when a new pack was written.

Exercise this with the existing fixture that makes a cruft commit reachable again and adds a new (unpacked) commit on top, and ensure that the incremental repack is able to successfully write a reachability bitmap.

---
 builtin/repack.c        |  6 ++++++
 t/t7704-repack-cruft.sh | 11 +++++++++++
 2 files changed, 17 insertions(+)

index c4360382c1f..b7596d488da 100644
@@ -539,6 +539,12 @@ int cmd_repack(int argc,
 			strvec_push(&cmd.args, "--stdin-packs=follow");
 		strvec_push(&cmd.args, "--unpacked");
 	} else {
+		/*
+		 * Incremental repacks do not copy already-packed objects,
+		 * so cruft packs may be required to form a reachability
+		 * closure for the MIDX.
+		 */
+		midx_must_contain_cruft = 1;
 		strvec_push(&cmd.args, "--unpacked");
 		strvec_push(&cmd.args, "--incremental");
 	}

index b49f22878f7..f7f83e70ffe 100755
@@ -787,6 +787,17 @@ test_expect_success 'geometric repack rescues descendants of loose trees' '
 	)
 '
 
+test_expect_success 'incremental repack includes cruft for MIDX bitmaps' '
+	setup_cruft_exclude_tests incremental-cruft &&
+	(
+		cd incremental-cruft &&
+
+		GIT_TEST_MULTI_PACK_INDEX=0 \
+		git repack -d --write-midx --write-bitmap-index &&
+		git rev-list --test-bitmap HEAD
+	)
+'
+
 test_expect_success 'repack --write-midx includes cruft when instructed' '
 	setup_cruft_exclude_tests exclude-cruft-when-instructed &&
 	(

2.56.0.4.gbee41d2fc68

OK. So this is a problem with just incremental repacks, but _not_ geometric repacks? And only when those incremental repacks write a midx?

If so, that makes sense to me (and the fix seems reasonable).

BTW, write_midx_incremental() does not check midx_must_contain_cruft. So I think you'd have the same problem with --write-midx=incremental. Adding that to the tests causes them to fail. I thought it might also fail with GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=1, but doesn't seem to.

That's not a new problem, but just a spot where the fix doesn't extend. Not sure how important it is to do now, or if it can wait for future work.

-Peff

Yeah, good point. I have a fix for it in the subsequent round, though it did make the overall series longer to accommodate predatory patches that make the substantive ones easier to grok.

I think that the result is sound, hence its inclusion in v2. I briefly considered dropping that part of this series altogether to deal with it another day. But doing so felt irresponsible as the earlier round pointed out the existence of a bug, the subsequent round should not ignore it.

On the other hand, as you note, it's not a new problem relative to this series, and so perhaps leaving it out wouldn't have been so bad. But I think all thing equal, the patches exist, and I think that they are sound, so I figured that I'd send them in the following round for completeness.

The maintainer should however, feel free to avoid queueing that part of the series if we want to punt on it for now.

Thanks,
Taylor

When performing a geometric repack with 'repack.midxMustContainCruft' set to "false", Git uses '--stdin-packs=follow' to copy (once-cruft) objects needed for reachability closure out of cruft packs. .keep packs do not need to participate in that walk, though they *are* included in the resulting MIDX.

A .keep pack can contain a commit that reaches an object whose only copy is in a cruft pack. When there is no previous MIDX and the repack writes a new pack, neither midx_has_unknown_packs() nor the !names.nr fallback require that cruft pack to be included. If the kept commit (or a descendant of it) is selected for bitmap coverage, the bitmap writer fails because the MIDX does not contain all of its reachable objects.

Include cruft packs whenever the MIDX contains kept packs. This also retains cruft when the kept packs happen to have full closure, or when '--pack-kept-objects' lets the repack walk them. It avoids having to establish their closure before deciding which packs the MIDX needs.

Add a test that packs the tip commit and its tree into a kept pack, leaving its parent in the cruft pack. The new commit's blob remains loose, making the geometric repack write a new pack and bypass the no-new-packs fallback. Verify that the repack succeeds and that we are able to successfully write a bitmap.

---
 repack-midx.c           |  5 +++++
 t/t7704-repack-cruft.sh | 20 ++++++++++++++++++++
 2 files changed, 25 insertions(+)

index 64c7f8d0f42..622c3c9d236 100644
@@ -197,6 +197,7 @@ static void midx_included_packs(struct string_list *include,
 	}
 
 	if (opts->midx_must_contain_cruft ||
+	    existing->kept_packs.nr ||
 	    midx_has_unknown_packs(include, geometry, existing)) {
 		/*
 		 * If there are one or more unknown pack(s) present (see

@@ -209,6 +210,10 @@ static void midx_included_packs(struct string_list *include,
 		 * reachability closure if the MIDX is bitmapped and one
 		 * or more of the bitmap's selected commits reaches a
 		 * once-cruft object that was later made reachable.
+		 *
+		 * Kept packs may also depend on cruft objects, since
+		 * they are included above without necessarily being
+		 * traversed by the repack.
 		 */
 		for_each_string_list_item(item, &existing->cruft_packs) {
 			/*

index f7f83e70ffe..02db2a06d9e 100755
@@ -798,6 +798,26 @@ test_expect_success 'incremental repack includes cruft for MIDX bitmaps' '
 	)
 '
 
+test_expect_success 'geometric repack includes cruft for kept packs' '
+	setup_cruft_exclude_tests kept-cruft &&
+	(
+		cd kept-cruft &&
+
+		# Keep HEAD and its tree outside the geometric repack. Its
+		# parent is reachable again, but still in the cruft pack.
+		git rev-parse HEAD HEAD^{tree} >objects &&
+		pack=$(git pack-objects $packdir/pack <objects) &&
+		touch $packdir/pack-$pack.keep &&
+		git prune-packed &&
+
+		# The new blob is still loose, so this writes a pack instead
+		# of taking the no-new-packs fallback.
+		GIT_TEST_MULTI_PACK_INDEX=0 \
+		git repack -d --geometric=2 --write-midx --write-bitmap-index &&
+		git rev-list --test-bitmap HEAD
+	)
+'
+
 test_expect_success 'repack --write-midx includes cruft when instructed' '
 	setup_cruft_exclude_tests exclude-cruft-when-instructed &&
 	(

2.56.0.4.gbee41d2fc68

OK. This makes sense to me, but two questions:

1. Is this going to kick in racily because of the .keep that we temporarily install during pushes? That could cause unexpected performance changes in a big repo when the midx sometimes has to randomly include cruft packs.

2. I'd have thought that the solution would be to treat .keep packs like other included follow-packs: traverse them in the usual way. But maybe there are good reasons we didn't do that in the first place.

-Peff

Excellent point. If we happen to race between repacking and receiving an incoming push, it is certainly not the case that we should have to then thrust the cruft pack onto the MIDX, and defeat the whole point of this series ;-).

Yeah, I think that's right. The following round passes excluded kept packs into the geometric repack's traversal using '!' for packs outside the existing MIDX and '^' for those already covered by it. That lets us copy their ancestors out of cruft without copying the kept objects themselves. Non-geometric repacks still conservatively retain cruft, since kept objects may remain untraversed.

There is, however, a pre-existing corner case with --pack-kept-objects and a cruft pack that is also kept. Such a pack can enter the MIDX without being traversed. This change doesn't address that case. Handling it cleanly looks more involved, so I'd leave it for a separate follow-up.

Thanks,
Taylor

I think all of these are making things strictly better, but I did find a few spots where the fixes might be incomplete. I'm not sure if that argues for a re-roll or for punting those to future work. ;)

I agree with Stolee that an oidset is perhaps a better data structure for storing the extra roots (which are in a kind-of random order anyway, since we're pulling them in pack order from various packs). But it also probably doesn't make that big a difference in practice (we'll skip duplicates during the traversal, and you probably don't have that many duplicate objects in a repo in the first place).

-Peff

Thanks for the review. Like I wrote in my response to your review, leaving it half-fixed felt dishonest, so fixes are included in the subsequent round, though they do make the series a little longer.

There is a separate, pre-existing bug that I would like to fix outside of this series, but only because it (a) is independent of this series, and (b) I estimate that the fix is considerably more complex.

Yup.

Thanks,
Taylor

This is a reroll of [1]. Thanks to Stolee, Junio, and Peff for reviewing.

With 'repack.midxMustContainCruft=false', geometric repacks copy needed objects out of cruft packs before omitting those packs from the MIDX. The follow walk can stop at objects in retained MIDX packs, relying on the indexed set being closed under reachability. That invariant must hold for directly enumerated trees and tags, too. A subsequent repack may introduce a commit that reaches a tree already in a retained pack.

The series adds those tree and tag roots, retains cruft for ordinary incremental repacks, and includes the required packs when writing either an ordinary or incremental MIDX. These omissions can otherwise make bitmap generation fail even though all reachable objects remain available in the repository.

Changes since v1:

- Use an oidset for the additional tree and tag roots, as Stolee suggested, avoiding duplicate entries when an object appears in several input packs.

- Address Junio's comments by documenting that a non-NULL stdin_packs_context requires a non-NULL revs pointer, and by returning early from add_loose_object() after common handling when there is no context.

- Following Peff's review, clarify that the tree-closure failure occurs in a subsequent repack. Describe the two walks precisely:
directly enumerated commits go first, but deferred tags can introduce commits in the second walk, so path context remains best-effort. Existing MIDXs lacking closure still require a full repack.

- Follow Peff's suggestion to traverse kept packs during geometric repacks, instead of always retaining cruft whenever kept packs exist. A temporary .keep installed by a push should not by itself require retaining cruft. The tests distinguish '--pack-kept-objects' from an explicit '--keep-pack'.

- Fix the separate '--write-midx=incremental' issue Peff identified. Both cases include required packs outside the retained chain, including packs from a replaced tip. Existing append coverage now writes and verifies bitmaps; separate regressions cover repacking without producing a new pack and replacing a tip containing cruft.

Thanks in advance for your review!

Thanks,
Taylor

[1] https://lore.kernel.org/git/[email protected]/

Taylor Blau (8):
pack-objects: introduce stdin_packs_context struct
pack-objects: ensure tree/tag closure with '--stdin-packs=follow'
repack: retain cruft packs in MIDXs after incremental repacks
repack: use a sorted list for explicitly kept packs
repack: follow kept packs when omitting cruft from the MIDX
repack: track the preferred pack explicitly in MIDX write steps
repack: defer allocating the append plan's write step
repack: include required packs in incremental MIDX writes

 Documentation/git-pack-objects.adoc |   2 +
 Documentation/git-repack.adoc       |   5 +-
 builtin/pack-objects.c              |  82 +++++++++++++++----
 builtin/repack.c                    |  40 ++++++++-
 repack-midx.c                       | 123 +++++++++++++++++++---------
 repack.c                            |   8 +-
 repack.h                            |   4 +
 t/t5331-pack-objects-stdin.sh       |  81 ++++++++++++++++++
 t/t7704-repack-cruft.sh             |  74 +++++++++++++++++
 t/t7705-repack-incremental-midx.sh  |  63 +++++++++++---
 10 files changed, 404 insertions(+), 78 deletions(-)

Range-diff against v1:
1: fcc07ede9a0 ! 1: 354c29cae73 pack-objects: introduce stdin_packs_context struct
@@ Commit message
object enumeration callbacks.

- Signed-off-by: Taylor Blau <[email protected]>

## builtin/pack-objects.c ##
@@ builtin/pack-objects.c: static int git_pack_config(const char *k, const char *v,
@@ builtin/pack-objects.c: static int git_pack_config(const char *k, const char *v,
static int stdin_packs_hints_nr;

+struct stdin_packs_context {
-+ struct rev_info *revs;
++ struct rev_info *revs; /* must be non-NULL */
+ enum stdin_packs_mode mode;
+};
+
2: 41448dca614 ! 2: 940953e5c40 pack-objects: ensure tree/tag closure with '--stdin-packs=follow'
@@ Commit message
maintain reachability closure for lone trees (that are not reachable
from any commit otherwise in the closure).

- A later walk with '!' packs can stop at that tree in a retained '^'
- pack even if a new commit reaches it. If the cruft pack remains
+ A subsequent repack with '!' packs can stop at that tree in a retained
+ '^' pack even if a new commit reaches it. If the cruft pack remains
excluded, and the bitmap selection picks one or more commits which reach that tree, the MIDX cannot generate a bitmap for that commit.

@@ Commit message
'--unpacked') as roots in '--stdin-packs=follow' mode. This rescues their descendants even when no input commit reaches them. Walk these roots after the existing traversal, preserving the SEEN bit to avoid
- redundant traversals. Ensure that the walk takes place *after* the
- existing traversal so that we don't lose the path prefix used for trees
- and blobs wherever possible.
+ redundant traversals. This gives directly enumerated commits priority
+ for the path prefixes used by name hashes and delta attributes. Tags can
+ introduce commits in the second walk, so path selection remains
+ best-effort.
+
+ Collect the extra roots in an oidset to avoid queuing duplicates. This
+ uses memory for each distinct root and walks its unvisited descendants.

Objects in '^' packs remain cutoffs to avoid rewalking packs that are
- known to be closed under reachability.
+ known to be closed under reachability, provided '!' packs are present.
+ This does not repair existing MIDXs lacking closure; those need a full
+ repack.

- Signed-off-by: Taylor Blau <[email protected]>

## Documentation/git-pack-objects.adoc ##
@@ Documentation/git-pack-objects.adoc: pack may include additional objects based on the following:
@@ Documentation/git-pack-objects.adoc: pack may include additional objects based o
## builtin/pack-objects.c ##
@@ builtin/pack-objects.c: static int stdin_packs_hints_nr;
struct stdin_packs_context {
- struct rev_info *revs;
+ struct rev_info *revs; /* must be non-NULL */
enum stdin_packs_mode mode;
-+ struct oid_array extra_roots;
++ struct oidset extra_roots;
};

static int add_object_entry_from_pack(const struct object_id *oid,
@@ builtin/pack-objects.c: static int add_object_entry_from_pack(const struct objec
add_pending_oid(ctx->revs, NULL, oid, 0);
+ } else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW &&
+ (type == OBJ_TREE || type == OBJ_TAG)) {
-+ oid_array_append(&ctx->extra_roots, oid);
++ oidset_insert(&ctx->extra_roots, oid);
}

if (!want_object_in_pack(oid, 0, &p, &ofs))
@@ builtin/pack-objects.c: static void read_stdin_packs(struct repository *repo,
struct stdin_packs_context ctx = {
.revs = &revs,
.mode = mode,
-+ .extra_roots = OID_ARRAY_INIT,
++ .extra_roots = OIDSET_INIT,
};
++ struct oidset_iter iter;
++ const struct object_id *oid;

/*
+ * The revision walk may hit objects that are promised, only. As the
@@ builtin/pack-objects.c: static void read_stdin_packs(struct repository *repo,
show_object_pack_hint,
&mode);

+ /*
+ * Trees and tags need closure even when no commit reaches them.
-+ * Defer adding these roots to revs.pending until the commit walk
++ * Defer adding these roots to revs.pending until the first walk
+ * finishes. Otherwise a subtree may be visited and marked SEEN
-+ * before its commit's root tree, using "a" instead of "sub/a" for
-+ * a blob's namehash and delta attributes.
++ * before its commit's root tree, using "a" instead of "sub/a"
++ * for a blob's namehash and delta attributes.
++ *
++ * Tags may introduce more commits in the second walk, so this
++ * does not *always* guarantee that trees are always visited
++ * with their full paths.
+ */
-+ for (size_t i = 0; i < ctx.extra_roots.nr; i++) {
-+ const struct object_id *oid = &ctx.extra_roots.oid[i];
++ oidset_iter_init(&ctx.extra_roots, &iter);
++ while ((oid = oidset_iter_next(&iter))) {
+ struct object *obj = lookup_object(repo, oid);
+
+ if (!obj || !(obj->flags & SEEN))
@@ builtin/pack-objects.c: static void read_stdin_packs(struct repository *repo,
+ show_object_pack_hint,
+ &mode);
+ }
-+ oid_array_clear(&ctx.extra_roots);
++ oidset_clear(&ctx.extra_roots);
+
release_revisions(&revs);

trace2_data_intmax("pack-objects", the_repository, "stdin_packs_found",
@@ builtin/pack-objects.c: static int add_loose_object(const struct object_id *oid, const char *path,
+ add_object_entry(oid, type, "", 0);
+ }

- if (ctx && type == OBJ_COMMIT)
+- if (ctx && type == OBJ_COMMIT)
++ if (!ctx)
++ return 0;
++
++ if (type == OBJ_COMMIT)
add_pending_oid(ctx->revs, NULL, oid, 0);
-+ else if (ctx && ctx->mode == STDIN_PACKS_MODE_FOLLOW &&
++ else if (ctx->mode == STDIN_PACKS_MODE_FOLLOW &&
+ (type == OBJ_TREE || type == OBJ_TAG))
-+ oid_array_append(&ctx->extra_roots, oid);
++ oidset_insert(&ctx->extra_roots, oid);

return 0;
}
3: 72f49ff37bb ! 3: a244b26030c repack: retain cruft packs in MIDXs after incremental repacks
@@ Commit message
such guarantee. Require the MIDX to include cruft packs in that case, even when a new pack was written.

+ This fixes ordinary '--write-midx'. The separate
+ '--write-midx=incremental' writer does not consult this flag and needs
+ its own handling.
+
Exercise this with the existing fixture that makes a cruft commit reachable again and adds a new (unpacked) commit on top, and ensure that the incremental repack is able to successfully write a reachability bitmap.

- Signed-off-by: Taylor Blau <[email protected]>

## builtin/repack.c ##
@@ builtin/repack.c: int cmd_repack(int argc,

-:  ----------- > 4:  c1ff18bf913 repack: use a sorted list for explicitly kept packs
-:  ----------- > 5:  51e20444dac repack: follow kept packs when omitting cruft from the MIDX
-:  ----------- > 6:  a85dbcd04c7 repack: track the preferred pack explicitly in MIDX write steps
-:  ----------- > 7:  4a6504629a2 repack: defer allocating the append plan's write step
4:  a10e3aa86c1 ! 8:  a42f775cbe2 repack: retain cruft packs in MIDXs containing kept packs

@@ Metadata
Author: Taylor Blau <[email protected]>

## Commit message ##
- repack: retain cruft packs in MIDXs containing kept packs
+ repack: include required packs in incremental MIDX writes

- When performing a geometric repack with 'repack.midxMustContainCruft'
- set to "false", Git uses '--stdin-packs=follow' to copy (once-cruft)
- objects needed for reachability closure out of cruft packs. .keep packs
- do not need to participate in that walk, though they *are* included in
- the resulting MIDX.
+ The append plan introduced in 06733a50eee (repack: allow
+ --write-midx=incremental without --geometric, 2026-05-19) adds only
+ newly written packs to the existing MIDX chain. The bitmap writer can
+ use objects from the new layer and all retained base layers, but the
+ plan omits preexisting packs outside the chain. Bitmap generation fails
+ if a selected commit reaches an object absent from the resulting chain.

- A .keep pack can contain a commit that reaches an object whose only copy
- is in a cruft pack. When there is no previous MIDX and the repack writes
- a new pack, neither midx_has_unknown_packs() nor the !names.nr
- fallback require that cruft pack to be included. If the kept commit (or
- a descendant of it) is selected for bitmap coverage, the bitmap writer
- fails because the MIDX does not contain all of its reachable objects.
+ The geometric plan from 1da62fb5c86 (repack: implement incremental MIDX
+ repacking, 2026-05-19) can omit kept and cruft packs, since neither
+ necessarily participates in the geometric repack. Such packs can also be
+ lost when replacing a tip layer that contains them. Neither plan
+ consults midx_included_packs(), so the rules for retaining cruft in
+ ordinary MIDX writes do not protect incremental writes.

- Include cruft packs whenever the MIDX contains kept packs. This also
- retains cruft when the kept packs happen to have full closure, or when
- '--pack-kept-objects' lets the repack walk them. It avoids having to
- establish their closure before deciding which packs the MIDX needs.
+ Use that selection logic to add missing packs to each plan's write step.
+ Skip packs in retained base layers, but include required packs from a
+ replaced tip. Count added objects when choosing which layers to compact,
+ without changing the preferred pack.

- Add a test that packs the tip commit and its tree into a kept pack,
- leaving its parent in the cruft pack. The new commit's blob remains
- loose, making the geometric repack write a new pack and bypass the
- no-new-packs fallback. Verify that the repack succeeds and that we are
- able to successfully write a bitmap.
+ Write and verify bitmaps in the existing append test: its existing
+ checks do not detect the omitted pack containing the first commit. Cover
+ the no-new-pack case separately with a reachable blob in a cruft pack.

- Signed-off-by: Taylor Blau <[email protected]>
+
+ ## Documentation/git-repack.adoc ##
+@@ Documentation/git-repack.adoc: linkgit:git-multi-pack-index[1]).
+ flat MIDX.
+ +
+ Without --geometric, a new MIDX layer is appended to the existing +-chain (or a new chain is started) containing whatever packs were written +-by the repack. Existing layers are preserved as-is.
++chain (or a new chain is started) containing newly written packs and any ++other required packs not already in the chain. Existing layers are ++preserved as-is.
+ +
+ When combined with --geometric, the incremental mode maintains a chain
+ of MIDX layers that is compacted over time using a geometric merging

## repack-midx.c ##
+@@
+ #include "odb.h"
+ #include "oidset.h"
+ #include "pack-bitmap.h"
++#include "packfile.h"
+ #include "path.h"
+ #include "refs.h"
+ #include "run-command.h"
+@@ repack-midx.c: void midx_snapshot_refs(struct repository *repo, struct tempfile *f) +
+ static int midx_has_unknown_packs(struct string_list *include,
+ struct pack_geometry *geometry,
+- struct existing_packs *existing)
++ struct existing_packs *existing,
++ struct multi_pack_index *base)
+ {
+ struct string_list_item *item;
+
+@@ repack-midx.c: static int midx_has_unknown_packs(struct string_list *include,
+ * MIDX. Note this function is called before the include
+ * list is populated with any cruft pack(s).
+ *
++ * - In a MIDX layer retained as part of the new chain's base.
++ *
+ * - Below the geometric split line (if using pack geometry),
+ * indicating that the pack won't be included in the new
+ * MIDX, but its contents were rolled up as part of the
+@@ repack-midx.c: static int midx_has_unknown_packs(struct string_list *include,
+ * - In the existing non-kept packs list (if not using pack
+ * geometry), and marked as non-deleted.
+ */
+- if (string_list_has_string(include, pack_name)) {
++ if (string_list_has_string(include, pack_name) ||
++ midx_contains_pack(base, pack_name)) {
+ continue;
+ } else if (geometry) {
+ struct strbuf buf = STRBUF_INIT;
+@@ repack-midx.c: static int midx_has_unknown_packs(struct string_list *include,
+ }
+
+ static void midx_included_packs(struct string_list *include,
+- struct repack_write_midx_opts *opts)
++ struct repack_write_midx_opts *opts,
++ struct multi_pack_index *base)
+ {
+ struct existing_packs *existing = opts->existing;
+ struct pack_geometry *geometry = opts->geometry;
@@ repack-midx.c: static void midx_included_packs(struct string_list *include,
- }

if (opts->midx_must_contain_cruft ||
-+ existing->kept_packs.nr ||
- midx_has_unknown_packs(include, geometry, existing)) {
+ (!geometry->split_factor && existing->kept_packs.nr) ||
+- midx_has_unknown_packs(include, geometry, existing)) {
++ midx_has_unknown_packs(include, geometry, existing, base)) {
/*
* If there are one or more unknown pack(s) present (see
-@@ repack-midx.c: static void midx_included_packs(struct string_list *include,
- * reachability closure if the MIDX is bitmapped and one
- * or more of the bitmap's selected commits reaches a
- * once-cruft object that was later made reachable.
-+ *
-+ * Kept packs may also depend on cruft objects, since
-+ * they are included above without necessarily being
-+ * traversed by the repack.
- */
- for_each_string_list_item(item, &existing->cruft_packs) {
- /*
+ * midx_has_unknown_packs() for what makes a pack
+@@ repack-midx.c: static int write_midx_included_packs(struct repack_write_midx_opts *opts)
+ struct packed_git *preferred = pack_geometry_preferred_pack(opts->geometry);
+ int ret = 0;
+
+- midx_included_packs(&include, opts);
++ midx_included_packs(&include, opts, NULL);
+ if (!include.nr)
+ goto done;
+
+@@ repack-midx.c: static void midx_compaction_step_release(struct midx_compaction_step *step)
+ free(step->csum);
+ }
+
++static int midx_compaction_step_include_packs(struct midx_compaction_step *step,
++ struct repack_write_midx_opts *opts,
++ struct multi_pack_index *base)
++{
++ struct odb_source_files *files = odb_source_files_downcast(opts->existing->source);
++ struct string_list include = STRING_LIST_INIT_DUP;
++ struct string_list_item *item;
++ struct strbuf path = STRBUF_INIT;
++ int ret = 0;
++
++ midx_included_packs(&include, opts, base);
++ string_list_sort(&step->u.write);
++
++ for_each_string_list_item(item, &include) {
++ struct packed_git *p;
++
++ if (string_list_has_string(&step->u.write, item->string) ||
++ midx_contains_pack(base, item->string))
++ continue;
++
++ strbuf_reset(&path);
++ strbuf_addf(&path, "%s/%s", opts->packdir, item->string);
++ p = packfile_store_load_pack(files->packed, path.buf, 1);
++ if (!p || open_pack_index(p)) {
++ ret = error(_("cannot open index for %s"), path.buf);
++ goto out;
++ }
++ if (unsigned_add_overflows(step->objects_nr, p->num_objects)) {
++ ret = error(_("too many objects in MIDX compaction step"));
++ goto out;
++ }
++ step->objects_nr += p->num_objects;
++ string_list_insert(&step->u.write, item->string);
++ }
++
++out:
++ strbuf_release(&path);
++ string_list_clear(&include, 0);
++ return ret;
++}
++
+ /*
+- * Build an append-only MIDX plan: a single WRITE step for the freshly
+- * written packs, plus COPY steps for every existing layer. No
++ * Build an append-only MIDX plan: a single WRITE step for packs not
++ * already in the chain, plus COPY steps for every existing layer. No
+ * compaction or merging is performed.
+ */
+ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts, +@@ repack-midx.c: static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,
+ size_t *steps_nr_p)
+ {
+ struct odb_source_files *files = odb_source_files_downcast(opts->existing->source);
++ struct string_list include = STRING_LIST_INIT_DUP;
++ struct string_list_item *item;
+ struct multi_pack_index *m;
+ struct midx_compaction_step *steps = NULL;
+ struct midx_compaction_step *step = NULL;
+- struct strbuf buf = STRBUF_INIT;
+ size_t steps_nr = 0, steps_alloc = 0;
+- uint32_t i;
+
+ odb_reprepare(opts->existing->repo->objects);
+ m = get_multi_pack_index(files->packed);
+
+- for (i = 0; i < opts->names->nr; i++) {
++ midx_included_packs(&include, opts, m);
++ for_each_string_list_item(item, &include) {
++ if (midx_contains_pack(m, item->string))
++ continue;
+ if (!step) {
+ ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);
+ step = &steps[steps_nr++];
+@@ repack-midx.c: static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,
+ step->type = MIDX_COMPACTION_STEP_WRITE;
+ string_list_init_dup(&step->u.write);
+ }
+- strbuf_reset(&buf);
+- strbuf_addf(&buf, "pack-%s.idx",
+- opts->names->items[i].string);
+- string_list_append(&step->u.write, buf.buf);
++ string_list_append(&step->u.write, item->string);
+ }
+- strbuf_release(&buf);
++ string_list_clear(&include, 0);
+
+ for (; m; m = m->base_midx) {
+ ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);
+@@ repack-midx.c: static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts,
+ if (opts->geometry->midx_tip_rewritten)
+ m = m->base_midx;
+
++ if (midx_compaction_step_include_packs(&step, opts, m) < 0) {
++ midx_compaction_step_release(&step);
++ ret = -1;
++ goto out;
++ }
++
+ trace2_data_string("repack", opts->existing->repo, "midx:rewrote-tip",
+ opts->geometry->midx_tip_rewritten ? "true" : "false");
+

- ## t/t7704-repack-cruft.sh ##
-@@ t/t7704-repack-cruft.sh: test_expect_success 'incremental repack includes cruft for MIDX bitmaps' '
+ ## t/t7705-repack-incremental-midx.sh ##
+@@ t/t7705-repack-incremental-midx.sh: test_expect_success '--write-midx=incremental without --geometric' '
+ git repack -d &&
+
+ test_commit second &&
+- git repack --write-midx=incremental &&
++ git repack --write-midx=incremental --write-bitmap-index &&
+
+ git multi-pack-index verify &&
+ test_line_count = 1 $midx_chain &&
+@@ t/t7705-repack-incremental-midx.sh: test_expect_success '--write-midx=incremental without --geometric' '
+ # A second repack appends a new layer without
+ # disturbing the existing one.
+ test_commit third &&
+- git repack --write-midx=incremental &&
++ git repack --write-midx=incremental --write-bitmap-index &&
+
+ git multi-pack-index verify &&
+ test_line_count = 2 $midx_chain &&
+@@ t/t7705-repack-incremental-midx.sh: test_expect_success '--write-midx=incremental without --geometric' '
+ head -n 1 $midx_chain >actual &&
+ test_cmp expect actual &&
+
++ git rev-list --test-bitmap HEAD &&
+ git fsck
)
'

-+test_expect_success 'geometric repack includes cruft for kept packs' '
-+ setup_cruft_exclude_tests kept-cruft &&
++test_expect_success 'incremental MIDX includes cruft without a new pack' '
++ git init incremental-cruft &&
++ (
++ cd incremental-cruft &&
++ git config repack.midxMustContainCruft false &&
++
++ test_commit base &&
++ echo cruft | git hash-object -w --stdin &&
++ git repack --cruft -d &&
++ test_commit cruft &&
++ git repack -d &&
++
++ # All objects are packed, but the new MIDX still needs cruft.
++ git repack --write-midx=incremental --write-bitmap-index &&
++ git rev-list --test-bitmap HEAD
++ )
++'
++
++test_expect_success 'geometric incremental MIDX retains cruft when replacing its tip' '
++ git init geometric-incremental-cruft &&
+ (
-+ cd kept-cruft &&
++ cd geometric-incremental-cruft &&
++ git config repack.midxNewLayerThreshold 1 &&
+
-+ # Keep HEAD and its tree outside the geometric repack. Its
-+ # parent is reachable again, but still in the cruft pack.
-+ git rev-parse HEAD HEAD^{tree} >objects &&
-+ pack=$(git pack-objects $packdir/pack <objects) &&
-+ touch $packdir/pack-$pack.keep &&
-+ git prune-packed &&
++ test_commit base &&
++ echo cruft | git hash-object -w --stdin &&
++ git repack --cruft -d &&
++ git multi-pack-index write --incremental --bitmap &&
++ test_commit cruft &&
+
-+ # The new blob is still loose, so this writes a pack instead
-+ # of taking the no-new-packs fallback.
-+ GIT_TEST_MULTI_PACK_INDEX=0 \
-+ git repack -d --geometric=2 --write-midx --write-bitmap-index &&
++ # Pack the new commit and tree, leaving the blob in cruft.
++ git repack -d &&
++ git repack --geometric=2 --write-midx=incremental \
++ --write-bitmap-index &&
++ test_line_count = 1 $midx_chain &&
+ git rev-list --test-bitmap HEAD
+ )
+'
+
- test_expect_success 'repack --write-midx includes cruft when instructed' '
- setup_cruft_exclude_tests exclude-cruft-when-instructed &&
+ test_expect_success 'below layer threshold, tip packs excluded' '
+ git init below-layer-threshold-tip-packs-excluded &&
+ (
+@@ t/t7705-repack-incremental-midx.sh: test_expect_success 'geometric rollup with surviving tip packs' '
+ )
+ '
+
+-test_expect_success 'kept packs are excluded from repack' '
++test_expect_success 'kept packs are excluded from repack but included in MIDX' '
+ git init kept-packs-excluded-from-repack &&
(
+ cd kept-packs-excluded-from-repack &&
+@@ t/t7705-repack-incremental-midx.sh: test_expect_success 'kept packs are excluded from repack' '
+ test_commit "$i" && git repack -d || return 1
+ done &&
+
+- keep=$(ls $packdir/pack-*.idx | head -n 1) &&
+- touch "${keep%.idx}.keep" &&
++ keep=$(test-tool find-pack A) &&
++ touch "${keep%.pack}.keep" &&
+
+- # The kept pack is excluded as a repacking candidate
+- # entirely, so no rollup occurs as there is only one
+- # non-kept pack. A new MIDX layer is written containing
+- # that pack.
+- git repack --geometric=2 -d --write-midx=incremental &&
++ # Neither pack is repacked, but both are needed for the
++ # bitmap of B, which reaches objects in the kept pack.
++ git repack --geometric=2 -d --write-midx=incremental \
++ --write-bitmap-index &&
+
+ test-tool read-midx $objdir >actual &&
+ grep "^pack-.*\.idx$" actual >actual.packs &&
+- test_line_count = 1 actual.packs &&
+- test_grep ! "$keep" actual.packs &&
++ test_line_count = 2 actual.packs &&
+
+ git multi-pack-index verify &&
++ git rev-list --test-bitmap HEAD &&
+
+ # All objects (from both kept and non-kept packs)
+ # must still be accessible.

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4

2.56.0.8.ga42f775cbe2

stdin_packs_read_input() currently receives a pointer to the rev_info struct and '--stdin-packs' mode separately as arguments, but the object enumeration callbacks only receive a pointer to the rev_info struct.

Wrap the pair in a new stdin_packs_context struct so that a future change may reference the '--stdin-packs' mode within the various object enumeration callbacks.

---
 builtin/pack-objects.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

index af9390a46b9..a553064fcce 100644
@@ -3804,11 +3804,17 @@ static int git_pack_config(const char *k, const char *v,
 static int stdin_packs_found_nr;
 static int stdin_packs_hints_nr;
 
+struct stdin_packs_context {
+	struct rev_info *revs; /* must be non-NULL */
+	enum stdin_packs_mode mode;
+};
+
 static int add_object_entry_from_pack(const struct object_id *oid,
 				      struct packed_git *p,
 				      uint32_t pos,
 				      void *_data)
 {
+	struct stdin_packs_context *ctx = _data;
 	off_t ofs;
 	struct object_info oi = OBJECT_INFO_INIT;
 	enum object_type type = OBJ_NONE;

@@ -3827,7 +3833,6 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 		die(_("could not get type of object %s in pack %s"),
 		    oid_to_hex(oid), p->pack_name);
 	} else if (type == OBJ_COMMIT) {
-		struct rev_info *revs = _data;
 		/*
 		 * commits in included packs are used as starting points
 		 * for the subsequent revision walk

@@ -3840,7 +3845,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 		 * However, we'll only add those objects to the packing
 		 * list after checking `want_object_in_pack()` below.
 		 */
-		add_pending_oid(revs, NULL, oid, 0);
+		add_pending_oid(ctx->revs, NULL, oid, 0);
 	}
 
 	if (!want_object_in_pack(oid, 0, &p, &ofs))

@@ -3954,8 +3959,9 @@ static int stdin_packs_include_check(struct commit *commit, void *data)
 }
 
 static void stdin_packs_add_pack_entries(struct strmap *packs,
-					 struct rev_info *revs)
+					 struct stdin_packs_context *ctx)
 {
+	struct rev_info *revs = ctx->revs;
 	struct string_list keys = STRING_LIST_INIT_NODUP;
 	struct string_list_item *item;
 	struct hashmap_iter iter;

@@ -3994,15 +4000,14 @@ static void stdin_packs_add_pack_entries(struct strmap *packs,
 		    (info->kind & STDIN_PACK_EXCLUDE_OPEN))
 			for_each_object_in_pack(info->p,
 						add_object_entry_from_pack,
-						revs,
+						ctx,
 						ODB_FOR_EACH_OBJECT_PACK_ORDER);
 	}
 
 	string_list_clear(&keys, 0);
 }
 
-static void stdin_packs_read_input(struct rev_info *revs,
-				   enum stdin_packs_mode mode)
+static void stdin_packs_read_input(struct stdin_packs_context *ctx)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct strmap packs = STRMAP_INIT;

@@ -4017,7 +4022,7 @@ static void stdin_packs_read_input(struct rev_info *revs,
 			continue;
 		else if (*key == '^')
 			kind = STDIN_PACK_EXCLUDE_CLOSED;
-		else if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW)
+		else if (*key == '!' && ctx->mode == STDIN_PACKS_MODE_FOLLOW)
 			kind = STDIN_PACK_EXCLUDE_OPEN;
 
 		if (kind != STDIN_PACK_INCLUDE)

@@ -4082,19 +4087,23 @@ static void stdin_packs_read_input(struct rev_info *revs,
 		info->p = p;
 	}
 
-	stdin_packs_add_pack_entries(&packs, revs);
+	stdin_packs_add_pack_entries(&packs, ctx);
 
 	strbuf_release(&buf);
 	strmap_clear(&packs, 1);
 }
 
-static void add_unreachable_loose_objects(struct rev_info *revs);
+static void add_unreachable_loose_objects(struct stdin_packs_context *ctx);
 
 static void read_stdin_packs(struct repository *repo,
 			     enum stdin_packs_mode mode, int rev_list_unpacked)
 {
 	int prev_fetch_if_missing = repo->fetch_if_missing;
 	struct rev_info revs;
+	struct stdin_packs_context ctx = {
+		.revs = &revs,
+		.mode = mode,
+	};
 
 	/*
 	 * The revision walk may hit objects that are promised, only. As the

@@ -4131,9 +4140,9 @@ static void read_stdin_packs(struct repository *repo,
 		 */
 		ignore_packed_keep_in_core_open = 1;
 	}
-	stdin_packs_read_input(&revs, mode);
+	stdin_packs_read_input(&ctx);
 	if (rev_list_unpacked)
-		add_unreachable_loose_objects(&revs);
+		add_unreachable_loose_objects(&ctx);
 
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));

@@ -4541,7 +4550,7 @@ static void add_objects_in_unpacked_packs(void)
 static int add_loose_object(const struct object_id *oid, const char *path,
 			    void *data)
 {
-	struct rev_info *revs = data;
+	struct stdin_packs_context *ctx = data;
 	enum object_type type = odb_read_object_info(the_repository->objects, oid, NULL);
 
 	if (type < 0) {

@@ -4563,8 +4572,8 @@ static int add_loose_object(const struct object_id *oid, const char *path,
 		add_object_entry(oid, type, "", 0);
 	}
 
-	if (revs && type == OBJ_COMMIT)
-		add_pending_oid(revs, NULL, oid, 0);
+	if (ctx && type == OBJ_COMMIT)
+		add_pending_oid(ctx->revs, NULL, oid, 0);
 
 	return 0;
 }

@@ -4574,10 +4583,10 @@ static int add_loose_object(const struct object_id *oid, const char *path,
  * add_object_entry will weed out duplicates, so we just add every
  * loose object we find.
  */
-static void add_unreachable_loose_objects(struct rev_info *revs)
+static void add_unreachable_loose_objects(struct stdin_packs_context *ctx)
 {
 	for_each_loose_file_in_source(the_repository->objects->sources,
-				      add_loose_object, NULL, NULL, revs);
+				      add_loose_object, NULL, NULL, ctx);
 }
 
 static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)

2.56.0.8.ga42f775cbe2

Since 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where possible, 2025-06-23), when the 'repack.midxMustContainCruft' configuration is set to "false", geometric repacks use '--stdin-packs=follow' to copy needed objects out of cruft packs so the MIDX can omit those packs.

In cd846bacc7d (pack-objects: introduce '--stdin-packs=follow', 2025-06-23), this behavior changed such that whenever excluded-open ('!') packs are present, the walk stops at objects in excluded-closed ('^') packs. Geometric repacks use '^' for retained packs already in the MIDX, relying on the indexed object set being closed under reachability.

However, the walk introduced in cd846bacc7d starts only from commit objects. A geometric repack can therefore produce a MIDX that does not maintain reachability closure for lone trees (that are not reachable from any commit otherwise in the closure).

A subsequent repack with '!' packs can stop at that tree in a retained '^' pack even if a new commit reaches it. If the cruft pack remains excluded, and the bitmap selection picks one or more commits which reach that tree, the MIDX cannot generate a bitmap for that commit.

Add trees and tags from included and '!' packs (and loose ones with '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues their descendants even when no input commit reaches them. Walk these roots after the existing traversal, preserving the SEEN bit to avoid redundant traversals. This gives directly enumerated commits priority for the path prefixes used by name hashes and delta attributes. Tags can introduce commits in the second walk, so path selection remains best-effort.

Collect the extra roots in an oidset to avoid queuing duplicates. This uses memory for each distinct root and walks its unvisited descendants.

Objects in '^' packs remain cutoffs to avoid rewalking packs that are known to be closed under reachability, provided '!' packs are present. This does not repair existing MIDXs lacking closure; those need a full repack.

---
 Documentation/git-pack-objects.adoc |  2 +
 builtin/pack-objects.c              | 43 ++++++++++++++-
 t/t5331-pack-objects-stdin.sh       | 81 +++++++++++++++++++++++++++++
 t/t7704-repack-cruft.sh             | 20 +++++++
 4 files changed, 145 insertions(+), 1 deletion(-)

index 65cd00c152f..1564d44f49d 100644
@@ -112,6 +112,8 @@ pack may include additional objects based on the following:
 This mode is useful, for example, to resurrect once-unreachable
 objects found in cruft packs to generate packs which are closed under
 reachability up to the boundary set by the excluded packs.
+Trees and tags in included or `!` packs are followed even when no
+commit reaches them, as are loose trees and tags with `--unpacked`.
 +
 Incompatible with `--revs`, or options that imply `--revs` (such as
 `--all`), with the exception of `--unpacked`, which is compatible.

index b342e82447d..b49f22878f7 100755
@@ -767,6 +767,26 @@ test_expect_success 'repack --write-midx excludes cruft where possible' '
 	)
 '
 
+test_expect_success 'geometric repack rescues descendants of loose trees' '
+	git init loose-tree-cruft &&
+	(
+		cd loose-tree-cruft &&
+		git config repack.midxMustContainCruft false &&
+		test_commit base &&
+		blob=$(echo cruft | git hash-object -w --stdin) &&
+		GIT_TEST_MULTI_PACK_INDEX=0 git repack --cruft -d &&
+
+		printf "100644 blob %s\tfile\n" "$blob" | git mktree &&
+		GIT_TEST_MULTI_PACK_INDEX=0 git repack -d --geometric=2 \
+			--write-midx --write-bitmap-index &&
+
+		test-tool read-midx --show-objects $objdir >midx &&
+		cruft=$(ls $packdir/*.mtimes) &&
+		test_grep ! "$(basename "$cruft" .mtimes).idx" midx &&
+		test_grep "^$blob " midx
+	)
+'
+
 test_expect_success 'repack --write-midx includes cruft when instructed' '
 	setup_cruft_exclude_tests exclude-cruft-when-instructed &&
 	(

2.56.0.8.ga42f775cbe2

Yeah, me either. I'm happy to change things around if you feel strongly.

It seems to work in practice. From reading through and thinking about it I couldn't find any obvious issues.

Just as well, there are a couple of spots that I was able to find that already call prepare_revision_walk() more than once:

* In builtin/pack-objects.c::get_object_list() (with the exception of '--path-walk') we call prepare_revision_walk() twice when exploding unreachable objects as loose.

* In reachable.c::mark_reachable_objects(), we also call the prepare_revision_walk() function twice when given a timestamp via mark_recent.

This all works since revs.pending is emptied by the first revwalk. But it is under-documented, so callers relying on this behavior may be surprised if/when it changes. Probably good #leftoverbits.

Thanks,
Taylor

I don't. If there were an easy solution I probably would. ;)

OK. That makes me feel like we're in good company, at least. If some combination turns out to be a problem, we can deal with it later.

I have a feeling that least one of those is my fault, too. ;)

-Peff

An incremental repack can write a commit and tree into a new pack while leaving objects they reach in an existing cruft pack. For example, a commit can make a previously unreachable blob reachable again. Since 'repack' will invoke 'pack-objects' with '--incremental', it will not copy the blob out of its cruft pack.

When the 'repack.midxMustContainCruft' configuration is set to "false", writing the first MIDX after such a repack may omit that cruft pack. The new pack bypasses the !names.nr fallback, and there are no previous MIDX packs for midx_has_unknown_packs() to check. Selecting the new commit for bitmap coverage then fails because its reachable objects are not all in the MIDX.

The omission dates all the way back to 5ee86c273bf (repack: exclude cruft pack(s) from the MIDX where possible, 2025-06-23). It relies on geometric repacking to copy once-cruft objects with '--stdin-packs=follow'. However, an ordinary incremental repack makes no such guarantee. Require the MIDX to include cruft packs in that case, even when a new pack was written.

This fixes ordinary '--write-midx'. The separate '--write-midx=incremental' writer does not consult this flag and needs its own handling.

Exercise this with the existing fixture that makes a cruft commit reachable again and adds a new (unpacked) commit on top, and ensure that the incremental repack is able to successfully write a reachability bitmap.

---
 builtin/repack.c        |  6 ++++++
 t/t7704-repack-cruft.sh | 11 +++++++++++
 2 files changed, 17 insertions(+)

index c4360382c1f..b7596d488da 100644
@@ -539,6 +539,12 @@ int cmd_repack(int argc,
 			strvec_push(&cmd.args, "--stdin-packs=follow");
 		strvec_push(&cmd.args, "--unpacked");
 	} else {
+		/*
+		 * Incremental repacks do not copy already-packed objects,
+		 * so cruft packs may be required to form a reachability
+		 * closure for the MIDX.
+		 */
+		midx_must_contain_cruft = 1;
 		strvec_push(&cmd.args, "--unpacked");
 		strvec_push(&cmd.args, "--incremental");
 	}

index b49f22878f7..f7f83e70ffe 100755
@@ -787,6 +787,17 @@ test_expect_success 'geometric repack rescues descendants of loose trees' '
 	)
 '
 
+test_expect_success 'incremental repack includes cruft for MIDX bitmaps' '
+	setup_cruft_exclude_tests incremental-cruft &&
+	(
+		cd incremental-cruft &&
+
+		GIT_TEST_MULTI_PACK_INDEX=0 \
+		git repack -d --write-midx --write-bitmap-index &&
+		git rev-list --test-bitmap HEAD
+	)
+'
+
 test_expect_success 'repack --write-midx includes cruft when instructed' '
 	setup_cruft_exclude_tests exclude-cruft-when-instructed &&
 	(

2.56.0.8.ga42f775cbe2

existing_packs_collect() performs a linear search through the '--keep-pack' arguments for each local pack. Typically the number of such arguments is small enough that the difference between a linear and binary search is just noise (especially compared with the amount of work that 'repack' is about to perform).

However, an additional caller will wish to search through the same list. To prevent that caller from having to duplicate the clunky for-loop in existing_packs_collect(), sort the list using fspathcmp() and replace the existing caller's loop with string_list_has_string().

This does not change the overall behavior of '--keep-pack' arguments.

---
 builtin/repack.c | 3 +++
 repack.c         | 8 +-------
 repack.h         | 4 ++++
 3 files changed, 8 insertions(+), 7 deletions(-)

index b7596d488da..88b05e96b5b 100644
@@ -2,6 +2,7 @@
 
 #include "builtin.h"
 #include "config.h"
+#include "dir.h"
 #include "environment.h"
 #include "parse-options.h"
 #include "path.h"

@@ -455,6 +456,8 @@ int cmd_repack(int argc,
 	packtmp = mkpathdup("%s/%s", packdir, packtmp_name);
 
 	existing.repo = repo;
+	keep_pack_list.cmp = fspathcmp;
+	string_list_sort(&keep_pack_list);
 	existing_packs_collect(&existing, &keep_pack_list);
 
 	if (geometry.split_factor) {

index d2aa58e1348..fa748ce46ce 100644
@@ -1,5 +1,4 @@
 #include "git-compat-util.h"
-#include "dir.h"
 #include "midx.h"
 #include "odb.h"
 #include "packfile.h"

@@ -131,7 +130,6 @@ void existing_packs_collect(struct existing_packs *existing,
 	struct strbuf buf = STRBUF_INIT;
 
 	repo_for_each_pack(existing->repo, p) {
-		size_t i;
 		const char *base;
 
 		if (p->multi_pack_index)

@@ -142,15 +140,11 @@ void existing_packs_collect(struct existing_packs *existing,
 
 		base = pack_basename(p);
 
-		for (i = 0; i < extra_keep->nr; i++)
-			if (!fspathcmp(base, extra_keep->items[i].string))
-				break;
-
 		strbuf_reset(&buf);
 		strbuf_addstr(&buf, base);
 		strbuf_strip_suffix(&buf, ".pack");
 
-		if ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)
+		if (p->pack_keep || string_list_has_string(extra_keep, base))
 			string_list_append(&existing->kept_packs, buf.buf);
 		else if (p->is_cruft)
 			string_list_append(&existing->cruft_packs, buf.buf);

index 61e554e4ed3..9f95e3a26f4 100644
@@ -76,6 +76,10 @@ struct existing_packs {
  * or packs->kept based on whether each pack has a corresponding
  * .keep file or not.  Packs without a .keep file are not to be kept
  * if we are going to pack everything into one file.
+ *
+ * A non-empty extra_keep must be sorted and use fspathcmp() as its
+ * comparator. Its entries are pack basenames, including the ".pack"
+ * suffix.
  */
 void existing_packs_collect(struct existing_packs *existing,
 			    const struct string_list *extra_keep);

2.56.0.8.ga42f775cbe2

OK, makes sense, and the patch looks correct.

-Peff

When performing a geometric repack with 'repack.midxMustContainCruft' set to "false", Git uses '--stdin-packs=follow' to copy (once-cruft) objects needed for reachability closure out of cruft packs. .keep packs do not normally participate in that walk, though they *are* included in the resulting MIDX.

A .keep pack can contain a commit that reaches an object whose only copy is in a cruft pack. When there is no previous MIDX and the repack writes a new pack, neither midx_has_unknown_packs() nor the !names.nr fallback require that cruft pack to be included. If the kept commit (or a descendant of it) is selected for bitmap coverage, the bitmap writer fails because the MIDX does not contain all of its reachable objects.

Pass excluded kept packs to the follow walk, using '!' for packs outside the existing MIDX and '^' for packs already covered by it. This copies needed objects out of cruft without copying objects in the kept packs. It also avoids including all cruft merely because a concurrent push has installed a temporary '.keep' file.

With '--pack-kept-objects', packs with '.keep' files participate in the geometric repack, and thus may have their objects copied. Packs specified as kept via '--keep-pack' still exclude their objects, even when their packs fall below the geometric split.

---
 builtin/repack.c        | 31 ++++++++++++++++++++++++++---
 repack-midx.c           |  5 +++++
 t/t7704-repack-cruft.sh | 43 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 3 deletions(-)

index 88b05e96b5b..27d6668a4ab 100644
@@ -476,9 +476,11 @@ int cmd_repack(int argc,
 	show_progress = !po_args.quiet && isatty(2);
 
 	strvec_push(&cmd.args, "--keep-true-parents");
-	for (i = 0; i < keep_pack_list.nr; i++)
-		strvec_pushf(&cmd.args, "--keep-pack=%s",
-			     keep_pack_list.items[i].string);
+	/* Geometric follow walks exclude these packs through stdin instead. */
+	if (!(geometry.split_factor && !midx_must_contain_cruft))
+		for (i = 0; i < keep_pack_list.nr; i++)
+			strvec_pushf(&cmd.args, "--keep-pack=%s",
+				     keep_pack_list.items[i].string);
 	strvec_push(&cmd.args, "--non-empty");
 	if (!geometry.split_factor) {
 		/*

@@ -593,6 +595,29 @@ int cmd_repack(int argc,
 
 			fprintf(in, "%c%s\n", marker, basename);
 		}
+		if (!midx_must_contain_cruft) {
+			struct strbuf buf = STRBUF_INIT;
+
+			for_each_string_list_item(item, &existing.kept_packs) {
+				char marker = '^';
+
+				strbuf_reset(&buf);
+				strbuf_addf(&buf, "%s.pack", item->string);
+
+				if (po_args.pack_kept_objects &&
+				    !string_list_has_string(&keep_pack_list,
+							    buf.buf))
+					continue;
+
+				/* Exclusions override any inclusion above. */
+				if (!string_list_has_string(&existing.midx_packs,
+							    buf.buf))
+					marker = '!';
+
+				fprintf(in, "%c%s\n", marker, buf.buf);
+			}
+			strbuf_release(&buf);
+		}
 		fclose(in);
 	}
 

index 64c7f8d0f42..9f7786aaac5 100644
@@ -197,6 +197,7 @@ static void midx_included_packs(struct string_list *include,
 	}
 
 	if (opts->midx_must_contain_cruft ||
+	    (!geometry->split_factor && existing->kept_packs.nr) ||
 	    midx_has_unknown_packs(include, geometry, existing)) {
 		/*
 		 * If there are one or more unknown pack(s) present (see

@@ -209,6 +210,10 @@ static void midx_included_packs(struct string_list *include,
 		 * reachability closure if the MIDX is bitmapped and one
 		 * or more of the bitmap's selected commits reaches a
 		 * once-cruft object that was later made reachable.
+		 *
+		 * Kept packs may also depend on cruft objects, since
+		 * they are included above without necessarily being
+		 * traversed by a non-geometric repack.
 		 */
 		for_each_string_list_item(item, &existing->cruft_packs) {
 			/*

2.56.0.8.ga42f775cbe2

Yeah, I struggled a bit when writing it TBH and flip-flopped between the two. I read the conditional (as proposed in my patch) as:

"If we aren't doing a geometric repack where the MIDX is allowed to omit cruft objects".

But I think the original sin here is midx_must_contain_cruft, which probably should have been midx_may_exclude_cruft, which defaults to false as opposed to the former which defaults to true.

It's not quite a double negation, but I agree that it's a little awkward. TBH I find the rewritten version just as confusing if not more so.

Thanks,
Taylor

A MIDX write step marks preferred packs in its string-list entries and chooses the last marked entry when executing the step. That makes the choice depend on list order, preventing the list from being sorted for membership checks.

Record the last candidate directly in the step, borrowing its name from the write list. This preserves preferred-pack selection while allowing the list to be sorted without changing that choice.

---
 repack-midx.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

index 9f7786aaac5..61832114671 100644
@@ -404,6 +404,7 @@ struct midx_compaction_step {
 
 	uint32_t objects_nr;
 	char *csum;
+	const char *preferred_pack; /* points into u.write */
 
 	enum {
 		MIDX_COMPACTION_STEP_UNKNOWN,

@@ -441,8 +442,6 @@ static int midx_compaction_step_exec_write(struct midx_compaction_step *step,
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct string_list hash = STRING_LIST_INIT_DUP;
-	struct string_list_item *item;
-	const char *preferred_pack = NULL;
 	int ret = 0;
 
 	if (!step->u.write.nr) {

@@ -450,19 +449,14 @@ static int midx_compaction_step_exec_write(struct midx_compaction_step *step,
 		goto out;
 	}
 
-	for_each_string_list_item(item, &step->u.write) {
-		if (item->util)
-			preferred_pack = item->string;
-	}
-
 	repack_prepare_midx_command(&cmd, opts, "write");
 	strvec_pushl(&cmd.args, "--incremental", "--no-write-chain-file", NULL);
 	strvec_pushf(&cmd.args, "--base=%s", base ? base : "none");
 
-	if (preferred_pack) {
+	if (step->preferred_pack) {
 		struct strbuf buf = STRBUF_INIT;
 
-		strbuf_addstr(&buf, preferred_pack);
+		strbuf_addstr(&buf, step->preferred_pack);
 		strbuf_strip_suffix(&buf, ".idx");
 		strbuf_addstr(&buf, ".pack");
 

@@ -719,7 +713,7 @@ static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts,
 
 		item = string_list_append(&step.u.write, buf.buf);
 		if (p->multi_pack_index || i == opts->geometry->pack_nr - 1)
-			item->util = (void *)1; /* mark as preferred */
+			step.preferred_pack = item->string;
 
 		if (unsigned_add_overflows(step.objects_nr, p->num_objects)) {
 			ret = error(_("too many objects in MIDX compaction step"));

@@ -797,7 +791,7 @@ static int repack_make_midx_compaction_plan(struct repack_write_midx_opts *opts,
 
 			item = string_list_append(&step.u.write, buf.buf);
 			if (pack_int_id == preferred_pack_idx)
-				item->util = (void *)1; /* mark as preferred */
+				step.preferred_pack = item->string;
 		}
 
 		if (unsigned_add_overflows(step.objects_nr, m->num_objects)) {

2.56.0.8.ga42f775cbe2

This is certainly cleaner, though it looks like the existing code works by marking item->util and then doing a linear search for it. So wouldn't that work even after sorting?

It would if only one entry were marked, but we can mark several.

For example, when repack_make_midx_compaction_plan() folds multiple MIDX layers into one via a WRITE step, it marks each layer's preferred pack without clearing the earlier marks. The scan doesn't stop at the first such mark, and the last marked entry wins.

So sorting would of course preserve the marks, but may change which one comes last.

Me too ;-).

Thanks,
Taylor

OK, that does make more sense. Re-reading your commit message again, I see it even says that, but somehow it didn't quite sink in the first time for me.

-Peff

The append plan creates a write step before iterating over newly written packs. The next change will consider additional packs and skip those already covered by the MIDX chain, so a non-empty candidate list will no longer imply that a write step is needed.

Allocate the step when adding its first pack instead. Continue to consider only newly written packs here, preserving the existing plan.

---
 repack-midx.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

index 61832114671..06eadb9df82 100644
@@ -559,33 +559,28 @@ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,
 	struct odb_source_files *files = odb_source_files_downcast(opts->existing->source);
 	struct multi_pack_index *m;
 	struct midx_compaction_step *steps = NULL;
-	struct midx_compaction_step *step;
+	struct midx_compaction_step *step = NULL;
+	struct strbuf buf = STRBUF_INIT;
 	size_t steps_nr = 0, steps_alloc = 0;
+	uint32_t i;
 
 	odb_reprepare(opts->existing->repo->objects);
 	m = get_multi_pack_index(files->packed);
 
-	if (opts->names->nr) {
-		struct strbuf buf = STRBUF_INIT;
-		uint32_t i;
-
-		ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);
-
-		step = &steps[steps_nr++];
-		memset(step, 0, sizeof(*step));
-
-		step->type = MIDX_COMPACTION_STEP_WRITE;
-		string_list_init_dup(&step->u.write);
-
-		for (i = 0; i < opts->names->nr; i++) {
-			strbuf_reset(&buf);
-			strbuf_addf(&buf, "pack-%s.idx",
-				    opts->names->items[i].string);
-			string_list_append(&step->u.write, buf.buf);
+	for (i = 0; i < opts->names->nr; i++) {
+		if (!step) {
+			ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);
+			step = &steps[steps_nr++];
+			memset(step, 0, sizeof(*step));
+			step->type = MIDX_COMPACTION_STEP_WRITE;
+			string_list_init_dup(&step->u.write);
 		}
-
-		strbuf_release(&buf);
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "pack-%s.idx",
+			    opts->names->items[i].string);
+		string_list_append(&step->u.write, buf.buf);
 	}
+	strbuf_release(&buf);
 
 	for (; m; m = m->base_midx) {
 		ALLOC_GROW(steps, st_add(steps_nr, 1), steps_alloc);

2.56.0.8.ga42f775cbe2

The append plan introduced in 06733a50eee (repack: allow --write-midx=incremental without --geometric, 2026-05-19) adds only newly written packs to the existing MIDX chain. The bitmap writer can use objects from the new layer and all retained base layers, but the plan omits preexisting packs outside the chain. Bitmap generation fails if a selected commit reaches an object absent from the resulting chain.

The geometric plan from 1da62fb5c86 (repack: implement incremental MIDX repacking, 2026-05-19) can omit kept and cruft packs, since neither necessarily participates in the geometric repack. Such packs can also be lost when replacing a tip layer that contains them. Neither plan consults midx_included_packs(), so the rules for retaining cruft in ordinary MIDX writes do not protect incremental writes.

Use that selection logic to add missing packs to each plan's write step. Skip packs in retained base layers, but include required packs from a replaced tip. Count added objects when choosing which layers to compact, without changing the preferred pack.

Write and verify bitmaps in the existing append test: its existing checks do not detect the omitted pack containing the first commit. Cover the no-new-pack case separately with a reachable blob in a cruft pack.

---
 Documentation/git-repack.adoc      |  5 +-
 repack-midx.c                      | 83 ++++++++++++++++++++++++------
 t/t7705-repack-incremental-midx.sh | 63 ++++++++++++++++++-----
 3 files changed, 122 insertions(+), 29 deletions(-)

index a1f9e64f668..ee59b76580a 100644
@@ -303,8 +303,9 @@ linkgit:git-multi-pack-index[1]).
 		flat MIDX.
 +
 Without `--geometric`, a new MIDX layer is appended to the existing
-chain (or a new chain is started) containing whatever packs were written
-by the repack. Existing layers are preserved as-is.
+chain (or a new chain is started) containing newly written packs and any
+other required packs not already in the chain. Existing layers are
+preserved as-is.
 +
 When combined with `--geometric`, the incremental mode maintains a chain
 of MIDX layers that is compacted over time using a geometric merging

2.56.0.8.ga42f775cbe2

I admit I had a hard time following this patch. I think the point is that we're going to include some packs in the midx that were not covered previously. But it was hard to see where that happens. I think the magic bit is this:

Yeah, that's right. I wrote this code in the first place, and it wasn't even *that* long ago and I had to spend a not-insignificant amount of time (re)acquainting myself with this area before writing this patch.

Thanks,
Taylor

I am trying to eject stale topics out of my tree, and the discussion on this topic is about to pass the 1-month mark. It seems that comments on half of them are "looks good" and the rest are "just as confusing as the original without improvement". How viable is this topic? Are we still improving on it, or has it long passed the point of diminishing returns that nobody is interested in polishing it any longer?

Thanks.

This one should be ready to go as-is!

Thanks,
Taylor

Hmph, the question was not meant to be answered by the author, though ;-)

OK, so I bit the bullet and read the patches and their existing review discussions again myself. I too were confused by pretty much the same things Peff commented on being confusing X-<, but I do not have concrete improvement ideas to offer right now.