Re: [PATCH v2 0/4] align the behavior when opening "packed-refs"

From: Jeff King <peff@peff.net>
Date: May 8, 2025, 20:33 UTC
Message-ID: <20250508203350.GG18229@coredump.intra.peff.net>
In-Reply-To: <xmqqzffm4zx5.fsf@gitster.g>

On Thu, May 08, 2025 at 01:20:54PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> > I left a few comments that I think bear addressing (or at least some
> > discussion).
> 
> I found both of your points very good ones, especially the "why are
> we making an in-core copy anyway later?"
> 
> Which may probably mean that munmap_temporary_snapshot() is not the
> helper function we want in the code path, so one of the preliminary
> refactoring patches can be removed.

Yep.

Show 5 quoted lines

> Even on mmap-incapable platforms, we have enough emulation in
> git_mmap() and git_munmap(), and this code path that wants to read a
> packed-refs file just mmap(), do its thing, and then munmap(),
> without worrying anything about "ah, temporary, so we need to make
> an in-core copy for ourselves".

Hmm, yeah. I had thought that somebody could explicitly set mmap_strategy themselves via config, in which case we'd want to avoid calling git_mmap() on systems where it actually does mmap. But it doesn't look like we have any mechanism for setting the config. So MMAP_NONE happens only when NO_MMAP is set.

  I briefly wondered if the existing code could be simplified to get rid
  of MMAP_NONE, since it could also rely on git_mmap() to make an
  in-memory copy. But it gets weird with MMAP_PREVENTS_DELETE and
  NO_MMAP combined, since then we make a pointless extra copy (one to
  fake-mmap, and one into the new buffer). OTOH, I'd expect those to be
  mutually exclusive.

  I kind of wonder with MMAP_TEMPORARY why we bother mapping at all and
  not just reading into a buffer. Maybe it's more efficient?

  Probably not worth revisiting all of this old code, though. And
  anyway, because of the SMALL_FILE_SIZE check, we couldn't even
  simplify away the read_in_full() code.

-Peff