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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 9, 2024, 21:40 UTC
Message-ID: <CAPig+cR=RgMeaAy1PRGgHu6_Ak+7=_-5tGvBZRekKRxi7GtdHw@mail.gmail.com>
In-Reply-To: <oan7pcamqpi2z4jzfshff2sfziqgisd3jcbpzfoiuyks5g57mk@7isgeztkcgmy>

On Tue, Jul 9, 2024 at 5:30 PM Justin Tobler <jltobler@gmail.com> wrote:

Show 17 quoted lines

> 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.