Re: [PATCH v2 0/8] midx: prevent bitmap corruption when permuting pack order

[Derrick Stolee <stolee@gmail.com>]

On 12/15/2021 2:46 PM, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> 
>> Here is a reroll of my series which fixes a serious problem with MIDX bitmaps by
>> which they can become corrupt when permuting their pack order.
...
>> So I'm definitely open to suggestions there, but otherwise this series should go
>> a long ways towards fixing my design mistake of having the MIDX .rev file be
>> separate from the MIDX itself.
> 
> Yeah, a single file with different chunks is a good way to ensure
> atomicity of update.

I just reviewed this series for the first time. Sorry for being so
late getting to it.

I had a few minor recommendations but they are mostly nitpicks and
don't deserve holding up the series if there are no other major
comments.

> A note to reviewers.
> 
> We need to make sure that not just we can still read .rev in
> existing repositories (and convert it to the new chunk) correctly,
> but also decide what to do to older versions of Git once the
> repository is touched by this new version.  Would they be upset to
> see no .rev files or is it just the performance thing (and it is
> more correct to recompute the reverse index on the fly)?  Should the
> new chunk be made mandatory to cause them notice that they should
> not muck with the repository, or is it optional?  Things like that.

1. Can we still read a .rev?

The new test script specifically verifies that existing repositories
will continue to read their .rev upon upgrade. Their .rev files will
be replaced with the chunk during the next write.

2. What if they downgrade after the RIDX chunk is in place?

The .rev file is missing and the repo has a performance issue because
they can't use bitmaps. Correctness is not a problem. Anyone using
.rev files for server use (where bitmaps are most useful) is hopefully
already careful about downgrading Git versions.

3. Should the chunk be made mandatory?

Unfortunately, the chunk format did not follow the index format's
example of making lowercase chunk IDs required. Instead, the chunks
that are necessary for v1 are necessary forever and all other chunks
are deemed optional. Changing this would require something more
drastic like updating the version number or giving some grace period
where released versions start treating lowercase chunk IDs as required
before creating a new "required" chunk.

This does mean that if there is a version incompatibility, the RIDX
chunk will just be ignored by the older version of Git.

In terms of making this a safe format upgrade, I think Taylor has
achieved that.

The only thing I can think is that server operators might want to
deploy this version with GIT_TEST_MIDX_WRITE_REV=1 for a while, so
any need to downgrade would not suffer a performance penalty for a
missing .rev file. If that is a planned way to safely deploy this
change, then it might be worth adding a test that we safely delete
a .rev file after writing both a .rev file and a RIDX chunk. (The
RIDX chunk will be preferred, so maybe the previous .rev file hits
some logic that would skip its deletion?)

Thanks,
-Stolee