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

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

Karthik Nayak <karthik.188@gmail.com> writes:

> +static char *build_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
> +{

I understand that the return value of this function is used as if
the value given via --format=... option to for-each-ref.

> +	struct strbuf fmt = STRBUF_INIT;
> +	struct strbuf local = STRBUF_INIT;
> +	struct strbuf remote = STRBUF_INIT;
> +
> +	strbuf_addf(&fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)", branch_get_color(BRANCH_COLOR_CURRENT));

This switches between "* " and "  " prefixed for each line of output
in "git branch --list" output, where an asterisk is used to mark the
branch that is currently checked out.  OK.

> +	if (filter->verbose) {
> +		strbuf_addf(&local, "%%(align:%d,left)%%(refname:strip=2)%%(end)", maxwidth);
> +		strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
> +		strbuf_addf(&local, " %%(objectname:short=7) ");
> +
> +		if (filter->verbose > 1)
> +			strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
> +				    "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
> +				    branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
> +		else
> +			strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)");
> +
> +		strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:strip=2)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
> +			    "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
> +			    branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,
> +			    remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
> +	} else {
> +		strbuf_addf(&local, "%%(refname:strip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> +			    branch_get_color(BRANCH_COLOR_RESET));
> +		strbuf_addf(&remote, "%s%s%%(refname:strip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> +			    branch_get_color(BRANCH_COLOR_REMOTE), remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
> +	}

This block prepares "local" and "remote", two formats that are used
for local and remote branches.

> +	strbuf_addf(&fmt, "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)", local.buf, remote.buf);

And this uses the %(if)...%(then)...%(else)...%(end) construct to
switch between these formats.

Sounds good.  

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.