midx-write.c: assume checksum-invalid MIDXs require an update

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/midx-write.c b/midx-write.c
index 40abe3868c4d6f357568af6b99159f16818cfdd4..51e13901640445f76cf726c96ed2ea50876a114f 100644 (file)
--- a/midx-write.c
+++ b/midx-write.c
@@ -1021,6 +1021,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 b6622849db7149d7d4b95cf18522f51711b9d94e..faae98c7e76a204d281397282ae75a97bc441c31 100755 (executable)
--- 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 &&