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
