Skip to content

Commit 38b72e5

Browse files
ttaylorrgitster
authored andcommitted
midx-write.c: assume checksum-invalid MIDXs require an update
In 6ce9d55 (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 0c5a62f (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 0c5a62f 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>
1 parent e16ac6c commit 38b72e5

2 files changed

Lines changed: 15 additions & 1 deletion

File tree

midx-write.c

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1021,6 +1021,20 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c
10211021
struct strbuf buf = STRBUF_INIT;
10221022
bool needed = true;
10231023

1024+
/*
1025+
* Ensure that we have a valid checksum before consulting the
1026+
* exisiting MIDX in order to determine if we can avoid an
1027+
* update.
1028+
*
1029+
* This is necessary because the given MIDX is loaded directly
1030+
* from the object store (because we still compare our proposed
1031+
* update to any on-disk MIDX regardless of whether or not we
1032+
* have assigned "ctx.m") and is thus not guaranteed to have a
1033+
* valid checksum.
1034+
*/
1035+
if (!midx_checksum_valid(midx))
1036+
goto out;
1037+
10241038
/*
10251039
* Ignore incremental updates for now. The assumption is that any
10261040
* incremental update would be either empty (in which case we will bail

t/t5319-multi-pack-index.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -563,7 +563,7 @@ test_expect_success 'git fsck suppresses MIDX output with --no-progress' '
563563
! grep "Verifying object offsets" err
564564
'
565565

566-
test_expect_failure 'corrupt MIDX is not reused' '
566+
test_expect_success 'corrupt MIDX is not reused' '
567567
corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
568568
"incorrect object offset" &&
569569
git multi-pack-index write 2>err &&

0 commit comments

Comments
 (0)