On Thu, May 08, 2025 at 04:33:50PM -0400, Jeff King wrote: > 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. > Right, I will remove this patch in the next version. > > 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? > I am also confused about this. If we eventually would allocate a buffer, it is wired that we map in the first place. > 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. > Right, I will try to clean the code later. At now, I think we need to concentrate on using `mmap`. I will add this into my TODOs. Thanks for the suggestion. > -Peff Thanks, Jialuo
