Re: [PATCH v8 18/19] branch: use ref-filter printing APIs
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Dec 12, 2016, 11:20 UTC
- Message-ID
- <CAOLa=ZSPDLwziGEvyixebAkS2M1JMYidQNHfDbnmYarFCjn80A@mail.gmail.com>
- In-Reply-To
- <20161209140345.76ybodldmg2lxgbn@sigill.intra.peff.net>
On Fri, Dec 9, 2016 at 7:33 PM, Jeff King <peff@peff.net> wrote:
Show 37 quoted lines
> On Wed, Dec 07, 2016 at 09:06:26PM +0530, Karthik Nayak wrote:
>
>> +const char *quote_literal_for_format(const char *s)
>> {
>> + struct strbuf buf = STRBUF_INIT;
>>
>> + strbuf_reset(&buf);
>> + while (*s) {
>> + const char *ep = strchrnul(s, '%');
>> + if (s < ep)
>> + strbuf_add(&buf, s, ep - s);
>> + if (*ep == '%') {
>> + strbuf_addstr(&buf, "%%");
>> + s = ep + 1;
>> + } else {
>> + s = ep;
>> + }
>> }
>> + return buf.buf;
>> }
>
> You should use strbuf_detach() to return the buffer from a strbuf.
> Otherwise it is undefined whether the pointer is allocated or not (and
> whether it needs to be freed).
>
> In this case, if "s" is empty, buf.buf would point to a string literal,
> but otherwise to allocated memory. strbuf_detach() normalizes that.
>
> But...
>
>> + branch_get_color(BRANCH_COLOR_REMOTE), maxwidth, quote_literal_for_format(remote_prefix),
>
> This caller never stores the return value, and it ends up leaking. So I
> wonder if you wanted "static struct strbuf" in the first place (and that
> would explain the strbuf_reset() in your function).
>
> -PeffAh! Yes this should be 'static struct strbuf' indeed, I blindly copied Junio's suggestion.
strbuf_detach() is also a better way to go.
Thanks.
-- Regards, Karthik Nayak