[PATCH 0/2] midx-write.c: do not optimize out writes with corrupt MIDXs

[PATCH 0/2] midx-write.c: do not optimize out writes with corrupt MIDXs

[Taylor Blau]

These two patches came from my work on implementing incremental MIDX
layer compaction.

When rebasing on top of current 'master' (at the
time of writing, 8745eae506f (The 17th batch, 2026-01-11)), I noticed
an early 'test_done' in t5319 added by 6ce9d558ced (midx-write: skip
rewriting MIDX with `--stdin-packs` unless needed, 2025-12-10). The
series is structured as follows:

 - The first patch removes the extraneous 'test_done', which exposes a
   failing test which is marked as such.

 - The second patch explains and fixes the bug, un-marking the test
   as test_expect_failure.

I was originally planning on adding these onto my series under
tb/incremental-midx-part-3.2. But I opted to split these patches out
into their own topic to ensure they are picked up before v2.53.0 is
tagged, should the larger series not be ready by then.

Thanks in advance for your review!

Taylor Blau (2):
  t/t5319-multi-pack-index.sh: drop early 'test_done'
  midx-write.c: assume checksum-invalid MIDXs require an update

 midx-write.c                | 14 ++++++++++++++
 t/t5319-multi-pack-index.sh |  2 --
 2 files changed, 14 insertions(+), 2 deletions(-)


base-commit: 8745eae506f700657882b9e32b2aa00f234a6fb6
-- 
2.52.0.437.gcc6f76a88cd

[PATCH 1/2] t/t5319-multi-pack-index.sh: drop early 'test_done'

[Taylor Blau]

In 6ce9d558ced (midx-write: skip rewriting MIDX with `--stdin-packs`
unless needed, 2025-12-10), an extra 'test_done' was added, causing the
test script to finish before having run all of its tests.

Dropping this extraneous 'test_done' exposes a bug from commit
6ce9d558ced that causes a subsequent test to fail. Mark that test with a
'test_expect_failure' for now, and the subsequent commit will explain
and fix the bug.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5319-multi-pack-index.sh | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 794f8b5ab4e..b6622849db7 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -415,8 +415,6 @@ test_expect_success 'up-to-date multi-pack-index is retained' '
 	)
 '
 
-test_done
-
 test_expect_success 'verify multi-pack-index success' '
 	git multi-pack-index verify --object-dir=$objdir
 '
@@ -565,7 +563,7 @@ test_expect_success 'git fsck suppresses MIDX output with --no-progress' '
 	! grep "Verifying object offsets" err
 '
 
-test_expect_success 'corrupt MIDX is not reused' '
+test_expect_failure 'corrupt MIDX is not reused' '
 	corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
 		"incorrect object offset" &&
 	git multi-pack-index write 2>err &&
-- 
2.52.0.437.gcc6f76a88cd

[PATCH 2/2] midx-write.c: assume checksum-invalid MIDXs require an update

[Taylor Blau]

In 6ce9d558ced (midx-write: skip rewriting MIDX with `--stdin-packs`
unless needed, 2025-12-10), the MIDX machinery learned how to optimize
out unnecessary writes with "--stdin-packs".

In order to do this, it compares the contents of the in-progress write
against a MIDX loaded directly from the object store. We load a separate
MIDX (as opposed to checking our update relative to "ctx.m") because the
MIDX code does not reuse an existing MIDX with --stdin-packs, and always
leaves "ctx.m" as NULL. See commit 0c5a62f14bc (midx-write.c: do not
read existing MIDX with `packs_to_include`, 2024-06-11) for details on
why.

If "ctx.m" is non-NULL, however, it is guaranteed to be checksum-valid,
since we only assign "ctx.m" when "midx_checksum_valid()" returns true.
Since the same guard does not exist for the MIDX we pass to
"midx_needs_update()", we may ignore on-disk corruption when determining
whether or not we can optimize out the write.

Add a similar guard within "midx_needs_update()" to prevent such an
issue.

A more robust fix would involve revising 0c5a62f14bc and teaching the
MIDX generation code how to reuse an existing MIDX even when invoked
with "--stdin-packs", such that we could avoid side-loading the MIDX
directly from the object store in order to call "midx_needs_update()".
For now, pursue the minimal fix.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx-write.c                | 14 ++++++++++++++
 t/t5319-multi-pack-index.sh |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/midx-write.c b/midx-write.c
index 87b97c70872..6485cb67068 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -1011,6 +1011,20 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c
 	struct strbuf buf = STRBUF_INIT;
 	bool needed = true;
 
+	/*
+	 * Ensure that we have a valid checksum before consulting the
+	 * exisiting MIDX in order to determine if we can avoid an
+	 * update.
+	 *
+	 * This is necessary because the given MIDX is loaded directly
+	 * from the object store (because we still compare our proposed
+	 * update to any on-disk MIDX regardless of whether or not we
+	 * have assigned "ctx.m") and is thus not guaranteed to have a
+	 * valid checksum.
+	 */
+	if (!midx_checksum_valid(midx))
+		goto out;
+
 	/*
 	 * Ignore incremental updates for now. The assumption is that any
 	 * incremental update would be either empty (in which case we will bail
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index b6622849db7..faae98c7e76 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -563,7 +563,7 @@ test_expect_success 'git fsck suppresses MIDX output with --no-progress' '
 	! grep "Verifying object offsets" err
 '
 
-test_expect_failure 'corrupt MIDX is not reused' '
+test_expect_success 'corrupt MIDX is not reused' '
 	corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
 		"incorrect object offset" &&
 	git multi-pack-index write 2>err &&
-- 
2.52.0.437.gcc6f76a88cd

Re: [PATCH 1/2] t/t5319-multi-pack-index.sh: drop early 'test_done'

[Patrick Steinhardt]

On Mon, Jan 12, 2026 at 06:45:03PM -0500, Taylor Blau wrote:
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 794f8b5ab4e..b6622849db7 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -415,8 +415,6 @@ test_expect_success 'up-to-date multi-pack-index is retained' '
>  	)
>  '
>  
> -test_done
> -
>  test_expect_success 'verify multi-pack-index success' '
>  	git multi-pack-index verify --object-dir=$objdir
>  '

Oh dear, this is embarassing.

Patrick

Re: [PATCH 2/2] midx-write.c: assume checksum-invalid MIDXs require an update

[Patrick Steinhardt]

On Mon, Jan 12, 2026 at 06:45:06PM -0500, Taylor Blau wrote:
> diff --git a/midx-write.c b/midx-write.c
> index 87b97c70872..6485cb67068 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1011,6 +1011,20 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c
>  	struct strbuf buf = STRBUF_INIT;
>  	bool needed = true;
>  
> +	/*
> +	 * Ensure that we have a valid checksum before consulting the
> +	 * exisiting MIDX in order to determine if we can avoid an
> +	 * update.
> +	 *
> +	 * This is necessary because the given MIDX is loaded directly
> +	 * from the object store (because we still compare our proposed
> +	 * update to any on-disk MIDX regardless of whether or not we
> +	 * have assigned "ctx.m") and is thus not guaranteed to have a
> +	 * valid checksum.
> +	 */
> +	if (!midx_checksum_valid(midx))
> +		goto out;
> +
>  	/*
>  	 * Ignore incremental updates for now. The assumption is that any
>  	 * incremental update would be either empty (in which case we will bail

This looks sensible to me, thanks for the fix!

Patrick