> diff --git a/t/lib-midx.sh b/t/lib-midx.sh
> index e38c609604c..b522dbdb0f4 100644
> --- a/t/lib-midx.sh
> +++ b/t/lib-midx.sh
> @@ -34,3 +34,9 @@ compare_results_with_midx () {
>  		midx_git_two_modes "cat-file --batch-all-objects --batch-check --unordered" sorted
>  	'
>  }
> +
> +nth_line() {
> +	local n="$1"
> +	shift
> +	awk "NR==$n" "$@"
> +}

> The 'multi-pack-index' builtin parses '--base' for incremental writes,
> but the normal write path does not pass that value through to
> `write_midx_file()`.
> 
> As a result, something like:
> 
>     $ git multi-pack-index write --incremental --base=<base>
> 
> behaves as if no custom base had been given (unless the caller used the
> '--stdin-packs' path).

> Thread the parsed base through `write_midx_file()`, and update the
> repack caller to pass NULL for the new argument where no custom base
> selection is needed.
> 
> This exposes a pre-existing problem in incremental writes with custom
> bases: the writer skips packs from the full existing MIDX chain, even
> when the caller selected an older base or no base at all.

> The affected t5334 cases fail while trying to write MIDX bitmaps. The
> detached layer omits packs above the selected base, and thus the
> resulting MIDX does not have a reachability closure, making it
> impossible to generate reachability bitmaps.
> 
> Mark those tests as expected failures accordingly. The following commit
> will fix the broken behavior and restore these tests.

> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index 00ffb36394d..949bfa796b2 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -224,7 +224,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,
>  	}
>  
>  	ret = write_midx_file(source, opts.preferred_pack,
> -			      opts.refs_snapshot, opts.flags);
> +			      opts.refs_snapshot, opts.incremental_base,
> +			      opts.flags);
>  
>  	free(opts.refs_snapshot);
>  	return ret;

> diff --git a/midx-write.c b/midx-write.c
> index 561e9eedc0e..aa438775ebd 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1850,12 +1850,14 @@ static int write_midx_internal(struct write_midx_opts *opts)
>  int write_midx_file(struct odb_source *source,
>  		    const char *preferred_pack_name,
>  		    const char *refs_snapshot,
> +		    const char *incremental_base,
>  		    unsigned flags)
>  {
>  	struct write_midx_opts opts = {
>  		.source = source,
>  		.preferred_pack_name = preferred_pack_name,
>  		.refs_snapshot = refs_snapshot,
> +		.incremental_base = incremental_base,
>  		.flags = flags,
>  	};

> diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh
> index 68a103d13d2..69e96bf8d93 100755
> --- a/t/t5334-incremental-multi-pack-index.sh
> +++ b/t/t5334-incremental-multi-pack-index.sh
> @@ -119,7 +119,7 @@ test_expect_success 'write MIDX layer with --base without --no-write-chain-file'
>  	test_grep "cannot use --base without --no-write-chain-file" err
>  '
>  
> -test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '
> +test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file' '
>  	test_commit base-none &&
>  	git repack -d &&
>  

> @@ -128,19 +128,33 @@ test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file
>  		--no-write-chain-file --base=none)" &&
>  
>  	test_cmp "$midx_chain.bak" "$midx_chain" &&
> -	test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
> +	test_path_is_file "$midxdir/multi-pack-index-$layer.midx" &&
> +
> +	echo "$layer" >"$midx_chain" &&
> +	test-tool read-midx --show-objects "$objdir" "$layer" >midx.objects &&
> +	test_grep "^$(git rev-parse 2.2) " midx.objects &&
> +	cp "$midx_chain.bak" "$midx_chain"
>  '

> diff --git a/midx-write.c b/midx-write.c
> index aa438775ebd..c50fdb5c6d1 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -133,8 +133,17 @@ static uint32_t midx_pack_perm(struct write_midx_context *ctx,
>  static int should_include_pack(const struct write_midx_context *ctx,
>  			       const char *file_name)
>  {
> +	struct multi_pack_index *m = ctx->m;
>  	/*
> -	 * Note that at most one of ctx->m and ctx->to_include are set,
> +	 * When writing incrementally, ctx->m may contain layers above
> +	 * the selected base MIDX, which must be included in the new
> +	 * layer.
> +	 */
> +	if (ctx->incremental)
> +		m = ctx->base_midx;
> +	/*
> +	 * Note that at most one of m and ctx->to_include are set,

> @@ -148,10 +157,7 @@ static int should_include_pack(const struct write_midx_context *ctx,
>  	 * should be performed independently (likely checking
>  	 * to_include before the existing MIDX).
>  	 */
> -	if (ctx->m && midx_contains_pack(ctx->m, file_name))
> -		return 0;
> -	else if (ctx->base_midx && midx_contains_pack(ctx->base_midx,
> -						      file_name))
> +	if (m && midx_contains_pack(m, file_name))
>  		return 0;

>  	else if (ctx->to_include &&
>  		 !string_list_has_string(ctx->to_include, file_name))
> diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh
> index 69e96bf8d93..84ff6120978 100755
> --- a/t/t5334-incremental-multi-pack-index.sh
> +++ b/t/t5334-incremental-multi-pack-index.sh
> @@ -119,7 +119,7 @@ test_expect_success 'write MIDX layer with --base without --no-write-chain-file'
>  	test_grep "cannot use --base without --no-write-chain-file" err
>  '
>  
> -test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file' '
> +test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '
>  	test_commit base-none &&
>  	git repack -d &&
>  
> @@ -136,7 +136,7 @@ test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file
>  	cp "$midx_chain.bak" "$midx_chain"
>  '
>  
> -test_expect_failure 'write MIDX layer with --base=<hash> and --no-write-chain-file' '
> +test_expect_success 'write MIDX layer with --base=<hash> and --no-write-chain-file' '
>  	test_commit base-hash &&
>  	git repack -d &&

> On Fri, Jun 12, 2026 at 04:07:08PM -0400, Taylor Blau wrote:
> > diff --git a/t/lib-midx.sh b/t/lib-midx.sh
> > index e38c609604c..b522dbdb0f4 100644
> > --- a/t/lib-midx.sh
> > +++ b/t/lib-midx.sh
> > @@ -34,3 +34,9 @@ compare_results_with_midx () {
> >  		midx_git_two_modes "cat-file --batch-all-objects --batch-check --unordered" sorted
> >  	'
> >  }
> > +
> > +nth_line() {
> > +	local n="$1"
> > +	shift
> > +	awk "NR==$n" "$@"
> > +}
>
> It feels a bit weird to have such a general function in "lib-midx.sh",
> but so be it.

> > Thread the parsed base through `write_midx_file()`, and update the
> > repack caller to pass NULL for the new argument where no custom base
> > selection is needed.
> >
> > This exposes a pre-existing problem in incremental writes with custom
> > bases: the writer skips packs from the full existing MIDX chain, even
> > when the caller selected an older base or no base at all.
>
> So as the "normal" write path didn't honor this option at all, I assume
> this bug here then refers to "--stdin-packs" being broken?

> > @@ -128,19 +128,33 @@ test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file
> >  		--no-write-chain-file --base=none)" &&
> >
> >  	test_cmp "$midx_chain.bak" "$midx_chain" &&
> > -	test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
> > +	test_path_is_file "$midxdir/multi-pack-index-$layer.midx" &&
> > +
> > +	echo "$layer" >"$midx_chain" &&
> > +	test-tool read-midx --show-objects "$objdir" "$layer" >midx.objects &&
> > +	test_grep "^$(git rev-parse 2.2) " midx.objects &&
> > +	cp "$midx_chain.bak" "$midx_chain"
> >  '
>
> Would it make sense to also test for an object from the first MIDX layer
> to be included? Otherwise we don't really assert that all layers are
> included in the new MIDX.

> On Fri, Jun 12, 2026 at 04:07:14PM -0400, Taylor Blau wrote:
> > diff --git a/midx-write.c b/midx-write.c
> > index aa438775ebd..c50fdb5c6d1 100644
> > --- a/midx-write.c
> > +++ b/midx-write.c
> > @@ -133,8 +133,17 @@ static uint32_t midx_pack_perm(struct write_midx_context *ctx,
> >  static int should_include_pack(const struct write_midx_context *ctx,
> >  			       const char *file_name)
> >  {
> > +	struct multi_pack_index *m = ctx->m;
> >  	/*
> > -	 * Note that at most one of ctx->m and ctx->to_include are set,
> > +	 * When writing incrementally, ctx->m may contain layers above
> > +	 * the selected base MIDX, which must be included in the new
> > +	 * layer.
> > +	 */
> > +	if (ctx->incremental)
> > +		m = ctx->base_midx;
> > +	/*
> > +	 * Note that at most one of m and ctx->to_include are set,
>
> Is that true? With "--stdin-packs --incremental --base=<foo>" I'd expect
> that we have both set now.

> Okay, previously we were always checking against `ctx->m`, so we
> would exclude packs that are contained in the current MIDX. And that
> includes the case where parts of the current MIDX are supposed to be
> thrown away because we want to write a new layer that excludes all
> layers starting at the base.

> This is fixed by instead always comparing against the base MIDX in case
> "--incremental" was passed. When the user passes "--base=none" we don't
> have any base, and consequently we'd include all packs. Otherwise, we'll
> exclude all packs that are already covered by our base, but include all
> the other ones.
>
> That feels sensible to me.

> On Thu, Aug 13, 2026 at 10:49:00AM +0200, Patrick Steinhardt wrote:
> > > @@ -128,19 +128,33 @@ test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file
> > >  		--no-write-chain-file --base=none)" &&
> > >
> > >  	test_cmp "$midx_chain.bak" "$midx_chain" &&
> > > -	test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
> > > +	test_path_is_file "$midxdir/multi-pack-index-$layer.midx" &&
> > > +
> > > +	echo "$layer" >"$midx_chain" &&
> > > +	test-tool read-midx --show-objects "$objdir" "$layer" >midx.objects &&
> > > +	test_grep "^$(git rev-parse 2.2) " midx.objects &&
> > > +	cp "$midx_chain.bak" "$midx_chain"
> > >  '
> >
> > Would it make sense to also test for an object from the first MIDX layer
> > to be included? Otherwise we don't really assert that all layers are
> > included in the new MIDX.
> 
> I don't think that is necessary in this case, but let me know if I am
> missing something below.
> 
> The new layer is written with '--bitmap', and '--base=none' means that
> there is no base layer from which the bitmap can inherit objects. Since
> 1.2 is an ancestor of 2.2, writing a bitmap for the new layer already
> requires that it contain 1.2 and the rest of its reachable history.
> Otherwise bitmap generation would fail with the missing-closure error
> before we reached the assertion.
> 
> Checking 2.2 confirms that an object from the old tip was pulled into
> the new layer; the successful bitmap write already establishes that its
> objects from the earlier layer were pulled in, too.

> `ctx->to_include` can indeed be non-NULL.
> ...
> The filtering still does the right thing: packs covered by the selected
> base are excluded, and the remaining packs are checked against the stdin
> list. But the comment is wrong, so I'll fix it.

> Taylor Blau <ttaylorr@openai.com> writes:
>
> > `ctx->to_include` can indeed be non-NULL.
> > ...
> > The filtering still does the right thing: packs covered by the selected
> > base are excluded, and the remaining packs are checked against the stdin
> > list. But the comment is wrong, so I'll fix it.
>
> Has anything happened since we saw this comment on Aug 13th?

> On Wed, Aug 26, 2026 at 02:37:23PM -0700, Junio C Hamano wrote:
> > Taylor Blau <ttaylorr@openai.com> writes:
> >
> > > `ctx->to_include` can indeed be non-NULL.
> > > ...
> > > The filtering still does the right thing: packs covered by the selected
> > > base are excluded, and the remaining packs are checked against the stdin
> > > list. But the comment is wrong, so I'll fix it.
> >
> > Has anything happened since we saw this comment on Aug 13th?
> 
> Not until you sent this message ;-).
> 
> I had a small reroll prepped that I had meant to send a couple of weeks
> ago but never got around to doing so. When I looked at it just now, I
> found that I wasn't quite satisfied with the range-diff in that the
> resulting block comment was somewhat confusing.
> 
> Instead of sending a new round immediately, let me instead share the
> comment that I wrote instead. Patrick (or others): does this comment
> seem clear, or do you think there are ways to tighten it up further?
> 
> --- 8< ---
> diff --git a/midx-write.c b/midx-write.c
> index 66da608370..ff94076104 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -143,15 +143,31 @@ static int should_include_pack(const struct write_midx_context *ctx,
>  		m = ctx->base_midx;
> 
>  	/*
> -	 * Note that m and ctx->to_include may both be set,
> -	 * so we are testing midx_contains_pack() and
> -	 * string_list_has_string() independently (guarded by the
> -	 * appropriate NULL checks).
> -	 *
> -	 * We could support passing to_include while reusing an existing
> -	 * MIDX, but don't currently since the reuse process drags
> -	 * forward all packs from an existing MIDX (without checking
> -	 * whether or not they appear in the to_include list).
> +	 * Note that it is OK for both ctx->base_midx and
> +	 * ctx->to_include may both be non-NULL, but at most one of
> +	 * ctx->m and ctx->to_include may be non-NULL.

> +	 * When ctx->m is NULL we are writing a new MIDX without reusing
> +	 * any packs from the previous layer(s). In that case, we care
> +	 * that both:
> +	 *
> +	 *   - the new layer's base MIDX (ctx->base_midx) does not
> +	 *     already contain the pack we are considering, or the new
> +	 *     layer has no base (i.e., it is a non-incremental MIDX)
> +	 *
> +	 *   - the pack appears in ctx->to_include, or ctx->to_include
> +	 *     is NULL, meaning that we can include any pack provided
> +	 *     the above condition is met.
> +	 *
> +	 * When ctx->m is non-NULL, we are writing a new MIDX that will
> +	 * subsume ctx->m and thus includes its packs. In this case, we
> +	 * could support respecting ctx->to_include, but currently
> +	 * don't.
> +	 *
> +	 * The only caller of this function which permits
> +	 * ctx->to_include being non-NULL restricts setting ctx->m when
> +	 * this is the case. So in this setting it is impossible that
> +	 * both will be non-NULL.