Re: [PATCH] midx.c: clear auxiliary MIDX files first

[Jeff King <peff@peff.net>]

On Wed, Oct 26, 2022 at 09:31:28AM -0400, Derrick Stolee wrote:

> This was always possible before, too: the midx could be read by a
> reader process before the repack process deletes that file. However,
> if the reader does not also gain a handle on the corresponding
> .bitmap before the repack process deletes that file, too, then the
> reader is also left thinking that no .bitmap exists.

Good point. Neither the writer _nor_ the reader is atomic. :)

> I think the old code is more correct, here. The window is slightly
> smaller, and the new code creates a window where the filesystem
> doesn't need to change for readers to get an imperfect view of
> things.

Yeah, I agree that the old code is nicer in that the window is a little
smaller.

Do we want to do something about the warning, though? Falling back to a
slow path sucks, of course, but racily generating user-visible warnings
for something that is not actually a problem is also not great.

> Aside: in these cases where a .bitmap file is not found for a midx,
> do we fall back to trying to find a .bitmap file for a pack-file?
> That would assuage most of the concerns here about what happens in
> this window where the repack has a new .pack/.bitmap pair but the
> old midx still exists (without a .bitmap, depending on timing).

Yes, we should. Code paths generally go through open_bitmap(), which
tries the midx first, then looks for pack bitmaps.

And in that sense, the race after this patch is fairly harmless. We fail
to see the midx bitmap, but we'll see the pack one (which must have been
created before we deleted the midx one, assuming this is a "git repack
-adb" or equivalent).

Is that also true of the race before this patch? I'm not sure which
warning is being generated, but if it's in open_midx_bitmap_1(), then
the same logic would apply.

-Peff