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