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.
