Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> On Mon, Apr 5, 2021 at 12:58 PM Tom Saeger <tom.saeger@xxxxxxxxxx> wrote:
>> On Mon, Apr 05, 2021 at 01:04:13PM +0000, Derrick Stolee via GitGitGadget wrote:
>> > +const char *refspec_item_format(const struct refspec_item *rsi)
>> > +{
>> > + static struct strbuf buf = STRBUF_INIT;
>> > +
>> > + strbuf_reset(&buf);
>>
>> is this even needed?
>
> This is needed due to the `static` strbuf declaration (which is easy
> to overlook).
>
>> > + if (rsi->matching)
>> > + return ":";
>> > +
>> > + if (rsi->negative)
>> > + strbuf_addch(&buf, '^');
>> > + else if (rsi->force)
>> > + strbuf_addch(&buf, '+');
>> > +
>> > + if (rsi->src)
>> > + strbuf_addstr(&buf, rsi->src);
>> > +
>> > + if (rsi->dst) {
>> > + strbuf_addch(&buf, ':');
>> > + strbuf_addstr(&buf, rsi->dst);
>> > + }
>> > +
>> > + return buf.buf;
>>
>> should this be strbuf_detach?
>
> In normal circumstances, yes, however, with the `static` strbuf, this
> is correct.
>
> However, a more significant question, perhaps, is why this is using a
> `static` strbuf in the first place? Does this need to be optimized
> because it is on a hot path? If not, then the only obvious reason why
> `static` was chosen was that sometimes the function returns a string
> literal and sometimes a constructed string. However, that's minor, and
> it would feel cleaner to avoid the `static` strbuf altogether by using
> strbuf_detach() for the constructed case and xstrdup() for the string
> literal case, and making it the caller's responsibility to free the
> result. (The comment in the header file would need to be updated to
> say as much.)
Very good suggestion. That would also make this codepath
thread-safe (I do not offhand know how important that is, though).
Thanks.
