Re: [PATCH 3/3] midx-write: include packs above custom incremental base

[Patrick Steinhardt <ps@pks.im>]

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.

> @@ -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;

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.

>  	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 &&

And those two tests pass now.

Thanks!

Patrick