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

[Junio C Hamano <gitster@pobox.com>]

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;
    }