Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary

From: Jeff King <peff@peff.net>
Date: Aug 25, 2020, 16:47 UTC
Message-ID: <20200825164721.GA1414394@coredump.intra.peff.net>
In-Reply-To: <20200825144515.GB7671@syl.lan>

On Tue, Aug 25, 2020 at 10:45:15AM -0400, Taylor Blau wrote:

Show 11 quoted lines

> > I wonder if its worth to trigger write_midx_file() to update the midx instead of
> > just removing MIDX?
> 
> There's no reason that we couldn't do this, but I don't think that it's
> a very good idea, especially if the new 'git maintenance' command will
> be able to do something like this by itself.
> 
> I'm hesitant to add yet another option to 'git repack', which I have
> always thought as a plumbing tool. That's important because callers
> (like 'git maintenance' or user scripts) can 'git multi-pack-index write
> ...' after their 'git repack' to generate a new MIDX if they want one.

It may be worth thinking a bit about atomicity here, though. Rather than separate delete and write steps, would somebody want a sequence like:

  - we have a midx with packs A, B, C

  - we find out that pack C is redundant and want to drop it

  - we create a new midx with A and B in a tempfile

  - we atomically rename the new midx over the old

  - we delete pack C

A simultaneous reader always sees a consistent midx. Whereas in a delete-then-rewrite scenario there is a moment where there is no midx, and they'd fall back to reading the individual idx files.

It may not matter all that much, though, for two reasons:

  - reading individual idx files should still give a correct answer.
    It just may be a bit slower.

  - even with an atomic replacement, I think caching on the reader side
    may cause interesting effects. For instance, if a reader process
    opens the old midx, it will generally cache that knowledge in memory
    (including mmap the content). So even after the replacement and
    deletion of C, it may still try to use the old midx that references
    C.

If we do care, though, that implies some cooperation between the deletion process and the midx code. Perhaps it even argues that repack should refuse to delete such a single pack at all, since it _isn't_ redundant. It's part of a midx, and the caller should rewrite the midx first itself, and _then_ look for redundant packs.

-Peff