Jeff King <peff@xxxxxxxx> writes: > On Wed, May 07, 2025 at 03:51:02PM -0700, Junio C Hamano wrote: > >> shejialuo <shejialuo@xxxxxxxxx> writes: >> >> > Hi All: >> > >> > As discussed in [1], we need to use mmap mechanism to open large >> > "packed_refs" file to save the memory usage. This patch mainly does the >> > following things: >> > >> > 1: Fix an issue that we would report an error when the "packed-refs" >> > file is empty, which does not align with the runtime behavior. >> > 2-4: Extract some logic from the existing code and then use these >> > created helper functions to let fsck code to use mmap necessarily >> > >> > [1] https://lore.kernel.org/git/20250503133158.GA4450@xxxxxxxxxxxxxxxxxxxxxxx >> > >> > Really thank Peff and Patrick to suggest me to do above change. >> >> This round looks good to me. Others? > > 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. 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". THanks.
