On Thu, May 08, 2025 at 01:20:54PM -0700, Junio C Hamano wrote: > > 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. > 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
