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

`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.

Signed-off-by: Taylor Blau <ttaylorr@openai.com>
---
 builtin/pack-objects.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

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.

Signed-off-by: Taylor Blau <ttaylorr@openai.com>
---
 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(+)

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.

Signed-off-by: Taylor Blau <ttaylorr@openai.com>
---
 builtin/repack.c        |  6 ++++++
 t/t7704-repack-cruft.sh | 11 +++++++++++
 2 files changed, 17 insertions(+)

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.

Signed-off-by: Taylor Blau <ttaylorr@openai.com>
---
 repack-midx.c           |  5 +++++
 t/t7704-repack-cruft.sh | 20 ++++++++++++++++++++
 2 files changed, 25 insertions(+)

Taylor Blau <ttaylorr@openai.com> writes:

We used to take _data that is rev_info, but no longer. We lost decl for "struct rev_info *revs" and rewrote its only use to directly reference ctx->revs. As long as the result compiles, we know there is no stray reference to "revs" left in this function, so the rewrite is complete. It is rare but I love this kind of patch whose correctness can be seen without reading beyond the context ;-)

Ditto.

We used to take two separately, but now we can take them in one package.

The same argument tells us that this is the right refactoring as long as the result compiles.

Ditto.

This one, ...

... together with the change to add_unreachable_loose_objects() here, it is not immediately obvious if we do not have to worry about the case where (ctx && !ctx->revs).

Given that 'struct stdin_packs_context' is a new structure, the fact that the instantiation on the stack in read_stdin_packs() is the only one that can give us a non-NULL 'ctx' pointer we can see in this patch means that a non-NULL 'ctx' cannot have a NULL '.revs' pointer in it. Again, as long as this patch alone compiles, we know this refactoring is correct.

It is not clear to me what the implication of assuming a non-NULL 'ctx' always means a non-NULL 'ctx->revs' is for the code health in the longer term, though.

Thanks.

Taylor Blau <ttaylorr@openai.com> writes:

Makes me wonder if this function will always end with "if ctx is not NULL, then depending on these conditions do one more thing" like this, or if it would change later. If the former,

	if (!ctx)
		return 0;

	if (type == OBJ_COMMIT)
		do the commit thing;
	else if (ctx->mode == follow && type in (tree, tag))
		do the tag or tree thing;

	return 0;

might be easier to follow, perhaps?

On 9/29/2026 9:28 PM, Taylor Blau wrote:

I believe this should be an oidset to avoid adding duplicate objects that appear multiple times. The order of these extra roots doesn't matter (such as in a --topo-order walk). We only care about the binary "reachable or not?" question.

Thanks, -Stolee

On Tue, Sep 29, 2026 at 08:28:49PM -0500, Taylor Blau wrote:

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.

And this is the interesting part. What about blobs? I guess we don't care about them because they are either there or not. There is no need to walk them independently because they can't reference anything.

Why do we need a separate extra_roots here, rather than just using add_pending_oid()? I'd have thought we'd add it all to the same ("--objects") walk.

I guess that is explained here:

but I'm not sure I buy it. Don't we always visit the commits first in a walk? So a single walk with all of the proposed objects would be fine?

If I understand this subtree claim, you are worried about the (single-traversal) case that we manually queue tree A, and then later visit commit C, which eventually has A as a sub-tree. So we queue A again _after_ its original, but that second visit (that we skip) would have had more interesting information (like path context).

But I don't think a second walk clears you of that possibility. You are queuing tags, too, which might in turn point to commits. So you might get the same commit traversal within that second walk.

I think you could fix it by putting tags into the first walk. But it will always exist to some degree (you could have a tag that points to a tree and queue that tree, but also a commit that points to it).

It's not clear to me how big a problem this is in practice. We know that the "path" of a tree or blob in a traversal is subject to context. There might be multiple commits that point to it at different levels. I guess it might be more common if we are adding random trees from a pack without context.

I think the more complete solution there is not two walks, but that the traversal machinery should queue context-ful trees ahead of low-context ones. I don't think we want to make the queue a stack (that would change the output considerably), so you'd probably need to keep a separate queue of low-context objects, and drain it only after the high-context ones we get from traversing the commits.

I certainly think this patch is a strict improvement, and should fix the main bug. It can't make anything worse for these extra trees and tags, because we weren't even including them before. ;) But I think the subtle side-bug here is not a complete fix (though I do think it is strictly better than doing nothing).

So I dunno. I'd probably be OK proceeding with this as-is, because I fear that dual-queue thing I mentioned above might turn into a rabbit hole that would derail the much more important fix.

-Peff

On Tue, Sep 29, 2026 at 08:28:53PM -0500, Taylor Blau wrote:

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

On Tue, Sep 29, 2026 at 08:28:58PM -0500, Taylor Blau wrote:

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

On Tue, Sep 29, 2026 at 08:28:34PM -0500, Taylor Blau wrote:

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

On Wed, Sep 30, 2026 at 10:42:10AM -0700, Junio C Hamano wrote:

;-)

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

On Wed, Sep 30, 2026 at 02:16:53PM -0400, Derrick Stolee wrote:

Great suggestion! I adjusted it in the following round.

Thanks, Taylor

On Wed, Sep 30, 2026 at 04:31:08PM -0400, Jeff King wrote:

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

On Wed, Sep 30, 2026 at 04:45:29PM -0400, Jeff King wrote:

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

On Wed, Sep 30, 2026 at 04:53:11PM -0400, Jeff King wrote:

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

On Wed, Sep 30, 2026 at 04:55:35PM -0400, Jeff King wrote:

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/cover.1790731662.git.me@ttaylorr.com/

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 <ttaylorr@openai.com>
    -    Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## 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 <ttaylorr@openai.com>
    -    Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## 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 <ttaylorr@openai.com>
    -    Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## 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 <me@ttaylorr.com>
     
      ## 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 <ttaylorr@openai.com>
    -    Signed-off-by: Taylor Blau <me@ttaylorr.com>
    +
    + ## 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

`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.

Signed-off-by: Taylor Blau <ttaylorr@openai.com>
---
 builtin/pack-objects.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

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.

Signed-off-by: Taylor Blau <ttaylorr@openai.com>
---
 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(-)

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.

Signed-off-by: Taylor Blau <ttaylorr@openai.com>
---
 builtin/repack.c        |  6 ++++++
 t/t7704-repack-cruft.sh | 11 +++++++++++
 2 files changed, 17 insertions(+)

`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.

Signed-off-by: Taylor Blau <ttaylorr@openai.com>
---
 builtin/repack.c | 3 +++
 repack.c         | 8 +-------
 repack.h         | 4 ++++
 3 files changed, 8 insertions(+), 7 deletions(-)

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.

Signed-off-by: Taylor Blau <ttaylorr@openai.com>
---
 builtin/repack.c        | 31 ++++++++++++++++++++++++++---
 repack-midx.c           |  5 +++++
 t/t7704-repack-cruft.sh | 43 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 3 deletions(-)

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(-)

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.

Signed-off-by: Taylor Blau <ttaylorr@openai.com>
---
 repack-midx.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

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.

Signed-off-by: Taylor Blau <ttaylorr@openai.com>
---
 Documentation/git-repack.adoc      |  5 +-
 repack-midx.c                      | 83 ++++++++++++++++++++++++------
 t/t7705-repack-incremental-midx.sh | 63 ++++++++++++++++++-----
 3 files changed, 122 insertions(+), 29 deletions(-)

On Wed, Sep 30, 2026 at 3:23 PM Derrick Stolee <stolee@gmail.com> wrote:

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.

On Thu, Oct 01, 2026 at 04:22:11PM -0700, Elijah Newren wrote:

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

On Thu, Oct 01, 2026 at 07:51:48PM -0500, Taylor Blau wrote:

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

On Wed, Sep 30, 2026 at 11:11:39PM -0500, Taylor Blau wrote:

I think one of the things that confused me reading the original patch (but is still present here) is that this comment is in read_stdin_packs, when we're actually doing the walks. So "defer adding these roots" feels quite late. We already did that deferring long before in add_object_entry_from_pack() and add_loose_object(), when we called oidset_insert() instead of add_pending().

So it would have made more sense to me to comment it there. Of course that is hard when there are two such places.

I dunno.

BTW, is it safe to prepare_revision_walk() twice on the same rev_info? I could believe it works, but I could also believe that there are hidden corner cases, as I don't think it was ever really intended to work this way.

Maybe OK for the vanilla set of options we are using here (as opposed to taking arbitrary options from the user). The rev_info is created locally in this function, though, so I guess if we wanted to be double-plus sure we could release and reinit the struct.

-Peff

On Wed, Sep 30, 2026 at 11:11:47PM -0500, Taylor Blau wrote:

OK, makes sense, and the patch looks correct.

-Peff

On Wed, Sep 30, 2026 at 11:11:51PM -0500, Taylor Blau wrote:

This conditional makes my head hurt because of the double-negation. By De Morgan's it is just:

  if (!geometry.split_factor || midx_must_contain_cruft)

which at least untangles it. The comment makes sense to say "we do not need to do this in geometric" mode, which matches the first half. But why does midx_must_contain_cruft trigger it? I guess it is "we do not need to bother doing the "^"-exclusion later in that mode", but I wonder if there is any advantage to suppressing it. I don't remember enough of the details here about why we were treating keep packs specially in the first place.

OK, and this is the flip side of the earlier conditional. We are in geometric mode if we get here, and we kick in only in non-midx-cruft mode.

IMHO the De Morgan untangling above makes it more clear, but you could probably even further with:

  /* explanatory comment here */
  int handle_keep_packs_via_follow = geometry.split_factor && !midx_must_contain_cruft;

And then use that in both spots. That might be overkill, though (and the name I proposed certainly sucks).

-Peff

On Wed, Sep 30, 2026 at 11:11:58PM -0500, Taylor Blau wrote:

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?

I am certainly happy to see these gross casts go away, though.

-Peff

On Wed, Sep 30, 2026 at 11:12:05PM -0500, Taylor Blau wrote:

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:

where we rely on midx_included_packs() to do that selection.

So I _think_ this is doing the right thing, but my confidence in my review is kind of low. To some degree I'd just rely on the functional tests here.

-Peff

On Fri, Oct 02, 2026 at 07:25:29PM -0400, Jeff King wrote:

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

On Fri, Oct 02, 2026 at 07:13:36PM -0400, Jeff King wrote:

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

On Fri, Oct 02, 2026 at 07:28:34PM -0400, Jeff King wrote:

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

On Fri, Oct 02, 2026 at 07:41:57PM -0400, Jeff King wrote:

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

On Fri, Oct 02, 2026 at 07:55:32PM -0500, Taylor Blau wrote:

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

On Fri, Oct 02, 2026 at 08:00:52PM -0500, Taylor Blau wrote:

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