Re: [PATCH v2 4/4] packed-backend: mmap large "packed-refs" file during fsck

[shejialuo <shejialuo@gmail.com>]

On Fri, May 09, 2025 at 11:59:34AM -0400, Jeff King wrote:
> On Fri, May 09, 2025 at 11:21:34PM +0800, shejialuo wrote:
> 
> > > > -	struct strbuf packed_ref_content = STRBUF_INIT;
> > > > +	struct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));
> > > 
> > > Minor, but is there any reason to allocate this here and not just:
> > > 
> > >   struct snapshot snapshot = { 0 };
> > > 
> > > ?
> > 
> > I simply copy the code from the existing code... I will change.
> 
> Ah, I see. The existing code must allocate on the heap because it is
> returning the snapshot to its caller. But here the variable is
> completely local to the function.
> 
> However, if we stop using mmap_strategy altogether and just use xmmap()
> directly, I don't think you'd even need a snapshot variable.
> 

Yes, that's right. Maybe the simplest way is to use `xmmap()`. But I
don't want to introduce repetition. In the current codebase, we already
have the logic to load the "packed-refs". Let's just reuse it.

Out of topic, I have more to express.

Actually, my eventual goal is to unify the fsck and other parts. For
example, "create_snapshot" would also do some basic sanity checks. And
of course, there is some overlap between fsck and this function. And
also for "next_record", it would also check something.

During my implementation, I find it hard to unify and it would require a
lot of effort. So, I simply introduce some redundant logic. But this is
not perfect. Because I still parse the "packed-refs" file just like
"next_record" and "create_snapshot" do. And the most disappointed thing
is that I cannot reuse them at all. But the things I want to do is very
similar to these functions.

So, I think I would eventually to find out a way to do above in the
future.

Thanks,
Jialuo