On Tue, Jul 09, 2024 at 05:40:08PM -0400, Eric Sunshine wrote:
> On Tue, Jul 9, 2024 at 5:30 PM Justin Tobler <jltobler@xxxxxxxxx> 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
