Re: [PATCH 3/5] refspec: output a refspec item

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Apr 5, 2021 at 12:58 PM Tom Saeger <tom.saeger@oracle.com> 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.