Re: [PATCH v8 18/19] branch: use ref-filter printing APIs

[Karthik Nayak <karthik.188@gmail.com>]

On Fri, Dec 9, 2016 at 7:33 PM, Jeff King <peff@peff.net> wrote:
> 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).
>
> -Peff

Ah! 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