tb/midx-bitmap-corruption-fix (was: Re: What's cooking in git.git (Jan 2022, #03; Thu, 13))

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 14, 2022, 19:39 UTC
Message-ID: <YeHRYkl2RaQbrtmx@nand.local>
In-Reply-To: <xmqq35lrf8g4.fsf@gitster.g>

On Thu, Jan 13, 2022 at 04:48:59PM -0800, Junio C Hamano wrote:

Show 17 quoted lines

> * tb/midx-bitmap-corruption-fix (2022-01-04) 9 commits
>  - pack-bitmap.c: gracefully fallback after opening pack/MIDX
>  - midx: read `RIDX` chunk when present
>  - t/lib-bitmap.sh: parameterize tests over reverse index source
>  - t5326: move tests to t/lib-bitmap.sh
>  - t5326: extract `test_rev_exists`
>  - t5326: drop unnecessary setup
>  - pack-revindex.c: instrument loading on-disk reverse index
>  - midx.c: make changing the preferred pack safe
>  - t5326: demonstrate bitmap corruption after permutation
>
>  A bug that made multi-pack bitmap and the object order out-of-sync
>  (hence the .midx data gets corrupted) has been fixed.
>
>  Waiting for a hopefully final review.
>  cf. <Ydceeo33Yt4N%2FbrN@nand.local>
>  source: <cover.1641320129.git.me@ttaylorr.com>

I would really like to get this into 2.35 since it's fixing an important source of repository corruption, but I think it should have a careful round of review before merging. And it is pretty late into the cycle anyway, so we may be too late to merge the whole thing.

But the first two patches:

  - midx.c: make changing the preferred pack safe
  - t5326: demonstrate bitmap corruption after permutation

could be applied as-is and the rest of the series left for later, which should be a safer approach (and would be sufficient to resolve the bug at the expense of some redundant bytes on disk[1]).

I think Stolee is probably the most familiar with this topic, but he is off currently and I'm not sure whether or not he'll be back with enough time to get this merged before 2.35.

On the other hand, the bug is pretty difficult to trigger, is affecting a very new feature, shouldn't ever cause permanent damage, and can be recovered from fairly easily (by dropping existing bitmaps). So perhaps it's OK to let it sit out for another release like this...

Thanks, Taylor

[1]: More or less storing the contents of the multi-pack-index-$HASH.rev
     file twice: once in the .rev file itself, and again as an optional
     write-only chunk in the MIDX.