On Fri, Nov 18, 2016 at 3:35 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Junio C Hamano <gitster@xxxxxxxxx> 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;
> }
>
Perfect. I get what you're saying, I'll add this in :)
--
Regards,
Karthik Nayak