Subject: Re: [GSoC][PATCH v9 3/9] fsck: add refs-related options and error report function

From: shejialuo <[email-masked]>
Date: Wed, 10 Jul 2024 20:13:43 +0800

On Tue, Jul 09, 2024 at 05:40:08PM -0400, Eric Sunshine wrote:
> On Tue, Jul 9, 2024 at 5:30 PM Justin Tobler <[email-masked]> wrote:
> > On 24/07/09 08:35PM, shejialuo wrote:
> > > +int fsck_refs_error_function(struct fsck_options *options UNUSED,
> > > +                          const struct object_id *oid,
> > > +                          enum object_type object_type UNUSED,
> > > +                          const char *checked_ref_name,
> > > +                          enum fsck_msg_type msg_type,
> > > +                          enum fsck_msg_id msg_id UNUSED,
> > > +                          const char *message)
> > > +{
> > > +     static struct strbuf sb = STRBUF_INIT;
> > > +
> > > +     strbuf_reset(&sb);
> >
> > Naive question, is there reason to reset `sb` immediately after
> > `STRBUF_INIT`? My understanding is that because we initialize the
> > buffer, the other fields should also be zeroed. If so, resetting the
> > buffer here seems redundant.
> 
> This particular strbuf is static, so it needs to be cleared each time
> the function is called.
> 
> The cover letter provides an argument for making it static: that this
> will be called often, and we don't want to make a lot of repeated
> allocations. Personally, I find that argument rather weak. Why would
> an error function be called frequently? Is this really a hot path that
> needs to worry about a few extra allocations? Also, importantly, every
> static added makes the code harder to "libify", so making it static
> requires a very strong reason, but there doesn't seem to be such a
> reason in this case.

I didn't consider the issue of libify. I just want to reduce some memory
allocations. I will change this in the next version.

Thanks,
Jialuo