Re: [PATCH 2/3] midx: pass custom '--base' through incremental writes

On Fri, Jun 12, 2026 at 04:07:11PM -0400, Taylor Blau wrote:
> 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).

I'm a bit confused. Is the "normal" write path the one that generates a
completely new, full MIDX? I assume not, and that you use "normal" to
discern between whether or not we pass "--stdin-packs"? I think that
could be made a bit more explicit.

*goes looking into the code* Yeah, seems like the distinction indeed is
whether "--stdin-packs" was passed in the first place.

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

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

Okay.

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

Previously we only passed the base to `write_midx_file_only()`, which is
what we use with "--stdin-packs". Here we now update the normal write
path to use the incremental base, too.

> 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,
>  	};

I was wondering whether there needs to be error checking somewhere so
that we only accept an incremental base in case MIDX_WRITE_INCREMENTAL
is set. I couldn't find any.

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

Okay. If I understand correctly, the expectation here would be that we
generate a complete MIDX as we don't select any base at all. But we
don't, and instead we base our incremental MIDX on top of the newest
layer by accident.

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

Patrick