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

From: shejialuo <shejialuo@gmail.com>
Date: May 9, 2025, 15:26 UTC
Message-ID: <aB4ewBsxnq0Yv3Fd@ArchLinux>
In-Reply-To: <20250508203350.GG18229@coredump.intra.peff.net>

On Thu, May 08, 2025 at 04:33:50PM -0400, Jeff King wrote:

Show 14 quoted lines

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

Show 22 quoted lines

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