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

From: shejialuo <shejialuo@gmail.com>
Date: May 9, 2025, 16:40 UTC
Message-ID: <aB4v6kBdNHmFlPlR@ArchLinux>
In-Reply-To: <20250509155934.GA25686@coredump.intra.peff.net>

On Fri, May 09, 2025 at 11:59:34AM -0400, Jeff King wrote:

Show 20 quoted lines

> 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