Karthik Nayak <[email protected]> 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.
