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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Wed, May 07, 2025 at 03:51:02PM -0700, Junio C Hamano wrote:
>
>> shejialuo <shejialuo@gmail.com> 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@coredump.intra.peff.net
>> >
>> > 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.