On Fri, Dec 9, 2016 at 7:33 PM, Jeff King <peff@xxxxxxxx> 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