Re: [PATCH v7 16/17] branch: use ref-filter printing APIs

> Junio C Hamano <gitster@pobox.com> writes:
>
>> One worry that I have is if the strings embedded in this function to
>> the final format are safe.  As far as I can tell, the pieces of
>> strings that are literally inserted into the resulting format string
>> by this function are maxwidth, remote_prefix, and return values from
>> branch_get_color() calls.
>>
>> The maxwidth is inserted via "%d" and made into decimal constant,
>> and there is no risk for it being in the resulting format.  Are
>> the return values of branch_get_color() calls safe?  I do not think
>> they can have '%' in them, but if they do, they need to be quoted.
>> The same worry exists for remote_prefix.  Currently it can either be
>> an empty string or "remotes/", and is safe to be embedded in a
>> format string.
>
> In case it was not clear, in short, I do not think there is anything
> broken in the code, but it is a longer-term improvement to introduce
> a helper that takes a string and returns a version of the string
> that is safely quoted to be used in the for-each-ref format string
> use it like so:
>
>     strbuf_addf(&remote,
>                 "%s"
>                 "%%(align:%d,left)%s%%(refname:strip=2)%%(end)"
>                 ...
>                 "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
>                 quote_literal_for_format(branch_get_color(BRANCH_COLOR_REMOTE)),
>                 ...);
>
> and the implementation of the helper may look like:
>
>     const char *quote_literal_for_format(const char *s)
>     {
>         static 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;
>     }
>