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

From: Taylor Blau <me@ttaylorr.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, peff@peff.net, stolee@gmail.com
Subject: Re: [PATCH v2 0/8] midx: prevent bitmap corruption when permuting pack order
Date: Wed, 15 Dec 2021 16:37:32 -0500	[thread overview]
Message-ID: <YbpgHBcyq/2+nLJB@nand.local> (raw)
In-Reply-To: <xmqqee6d648n.fsf@gitster.g>

On Wed, Dec 15, 2021 at 11:46:16AM -0800, 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.
>
> This seems to depend on tb/cruft-packs that is not yet in 'next', so
> I'll redo this topic branch by forking it at 'master', merging the
> other topic in, and then queuing these 8 patches.

Hmm. They shouldn't depend on that topic. My local copy depends on
abe6bb3905 (The first batch to start the current cycle, 2021-11-29), but
I'm happy to rebase things if basing on abe6bb3905 was a mistake.

> > 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.
>
> 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.

An old client that is looking in a repository where the reverse index is
stored as a chunk inside of the MIDX (and does not appear as a separate
file on disk) will not be able to read the MIDX's bitmap. We could
compute the reverse index on the fly (as we did for a long time with
single packs) but it is slow, and older clients obviously would not know
how to do it.

But the failure is graceful(ish): we'll get a warning that says we
couldn't find a reverse index, and then carry on pretending that the
MIDX did not have a bitmap.

    ~/s/git [nand] (master) $ git.compile multi-pack-index write --bitmap
    Selecting bitmap commits: 73008, done.
    Building bitmaps: 100% (319/319), done.
    ~/s/git [nand] (master) $ rm -f .git/objects/pack/multi-pack-index-*.rev
    ~/s/git [nand] (master) $ git.compile rev-list --count --objects --use-bitmap-index HEAD
    warning: multi-pack bitmap is missing required reverse index
    warning: ignoring extra bitmap file:
    .git/objects/pack/pack-00465554dc3f3d96ac89a0ecc73cb5f5abbb35a5.pack
    warning: multi-pack bitmap is missing required reverse index
    warning: ignoring extra bitmap file:
    .git/objects/pack/pack-00465554dc3f3d96ac89a0ecc73cb5f5abbb35a5.pack
    307913

As far as whether or not the chunk is necessary, it only *needs* to be
present in the MIDX if there is a corresponding MIDX bitmap. We could
generate it always, but this series keeps in the tradition of the .rev
file and only writes it when we are also writing a bitmap.

> Thanks.

Thanks,
Taylor