Re: [PATCH v7 16/17] branch: use ref-filter printing APIs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 17, 2016, 19:50 UTC
- Message-ID
- <xmqqinrlopge.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20161108201211.25213-17-Karthik.188@gmail.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.
Show 5 quoted lines
> + 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.
Show 22 quoted lines
> + 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.