Re: [PATCH] pretty: respect color settings for %C placeholders

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 10, 2016, 18:52 UTC
Message-ID: <xmqq1szof34x.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161010151517.6wszhuyp57yfncaj@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> So here's what a patch to do that would look like. I admit that "I can't
> think of a good use" does not mean there _isn't_ one, but perhaps by
> posting this, it might provoke other people to think on it, too. And if
> nobody can come up with, maybe it's a good idea.

I do not have a fundamental opposition to this approach myself, modulo a minor nit.

Show 7 quoted lines

> +  By default, colors are shown only when enabled for log output (by
> +  `color.diff`, `color.ui`, or `--color`, and respecting the `auto`
> +  settings of the former if we are going to a terminal). `%C(auto,...)`
> +  is accepted as a historical synonym for the default. Specifying
> +  `%C(always,...) will show the colors always, even when colors are not
> +  otherwise enabled (to enable this behavior for the whole format, use
> +  `--color=always`).

It is not just "for the whole format", but also affects other parts of the output, no? I am thinking about "git log -p --format=...".

Show 14 quoted lines

> diff --git a/pretty.c b/pretty.c
> index 25efbca..73e58b5 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
>  
>  		if (!end)
>  			return 0;
> -		if (skip_prefix(begin, "auto,", &begin)) {
> +
> +		if (!skip_prefix(begin, "always,", &begin)) {
>  			if (!want_color(c->pretty_ctx->color))
>  				return end - placeholder + 1;
>  		}

As a way to say "when color is not enabled, ignore everything unless it begins with 'always,'", this was a bit hard to read. Perhaps an in-code comment is in order?

Show 13 quoted lines

> +
> +		/* this is a historical noop */
> +		skip_prefix(begin, "auto,", &begin);
> +
>  		if (color_parse_mem(begin, end - begin, color) < 0)
>  			die(_("unable to parse --pretty format"));
>  		strbuf_addstr(sb, color);
>  		return end - placeholder + 1;
>  	}
> -	if (skip_prefix(placeholder + 1, "red", &rest))
> +	if (skip_prefix(placeholder + 1, "red", &rest) &&
> +	    want_color(c->pretty_ctx->color))
>  		strbuf_addstr(sb, GIT_COLOR_RED);

Hmm. If we are in "no I do not want color" mode and "always,red" was given, we countermanded !want_color() check up above and come here. Then we check want_color() again and refuse to paint it red?

I must be reading the patch incorrectly, but I cannot quite tell where I want astray...