Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

On Wed, Oct 26, 2022 at 05:42:51PM -0400, Taylor Blau wrote:

A blast from the past. You didn't include the Date field, but this was from ~2017.

It feels like you might be a special case, though. Most people are not hacking on the bitmap code. :)

I wonder if you'd be better served by having good tracing for the bitmap code. Then it could tell you lots of things, like which bitmap it actually opened, which ones it ignored, etc. Of course you'd have to remember to enable that tracing when you're working in the area.

The other thing that has always bugged me about this warning (and would be true of tracing code, too) is that it inherently requires looking at all of the packfiles. I.e., open_pack_bitmap() doesn't return when it finds something; it keeps going just in case it finds another bitmap so it can say "hey, there's an extra one!".

This complicates the code a little bit. But it's also inefficient. If we have N packs, we'll do N open() calls, most of which will fail. Weirder, though, is that we open the pack .idx first. So we're literally opening, mapping, and checking the idx for every pack for no reason!

Now this might not be as bad as it seems:

  - in the long run, we might open those idx files anyway, if we have to
    access those packs. So it's really just overriding the lazy-open
    behavior.

  - in the worst case, the one bitmap file is at the end of the list, so
    you hit all N anyway. And this is actually pretty common, since we
    sort in reverse-chronological order, and the bitmap is usually on
    the oldest and biggest pack.

  - in general, having a lot of packs has bad performance for other
    reasons. So you'd generally want to keep N small-ish in the first
    place.

So it may not be worth worrying about. It does seem like it would be easy to reorder open_pack_bitmap_1() to look for a bitmap file first and only open the idx if it finds something.

It's possible we have a weird race-dependency on opening those idx files early (certainly there's some subtlety with pack-objects keeping the .pack files themselves open which prevents problems with a simultaneous repack). But I think it would be OK; we don't commit to using a particular .idx file in the general case, and one that goes away would just cause us to reprepare_packed_git() and continue. For a particular .bitmap, we do commit to having the matching .idx file, but obviously we'd want to open and check that one before considering the bitmap to be successfully opened.

Yeah. If we are going to keep the warning, this makes much better sense. Possibly we could even just skip_prefix() on the object directory, though I think in practice it amounts to the same thing.

-Peff