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

From: Junio C Hamano <gitster@pobox.com>
Date: May 8, 2025, 20:20 UTC
Message-ID: <xmqqzffm4zx5.fsf@gitster.g>
In-Reply-To: <20250508200802.GC18229@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 23 quoted lines

> 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.