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

["René Scharfe" <l.s.r@web.de>]

Am 10.10.2016 um 17:15 schrieb Jeff King:
> On Mon, Oct 10, 2016 at 10:28:18AM -0400, Jeff King wrote:
>
>>> We could add some new tag to change the behavior of all following %C
>>> tags. Something like %C(tty) maybe (probably a bad name), then
>>> discourage the use if "%C(auto" for terminal detection?
>>
>> Yeah, adding a "%C(enable-auto-color)" or something would be backwards
>> compatible and less painful than using "%C(auto)" everywhere. I do
>> wonder if anybody actually _wants_ the "always show color, even if
>> --no-color" behavior. I'm having trouble thinking of a good use for it.
>>
>> IOW, I'm wondering if anyone would disagree that the current behavior is
>> simply buggy. Reading the thread at:
>>
>>   http://public-inbox.org/git/7v4njkmq07.fsf@alter.siamese.dyndns.org/
>>
>> I don't really see any compelling reason against it (there was some
>> question of which config to use, but we already answered that with
>> "%C(auto)", and use the value from the pretty_ctx).
>
> 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.

Color tags that respect the config and the --color option make the most 
sense to me as well.

Nevertheless a possible counterpoint: Coloring is used in commands that 
are intended for human consumption.  Most of the time there is no need 
to to conserve the output in a file.  But even then, and of course with 
pipes, once you look at it using less -R or grep you still get nice 
colored lines and not a monochrome wall of text.

> -- >8 --
> Subject: pretty: respect color settings for %C placeholders
>
> The color placeholders have traditionally been
> unconditional, showing colors even when git is not otherwise
> configured to do so. This was not so bad for their original
> use, which was on the command-line (and the user could
> decide at that moment whether to add colors or not). But
> these days we have configured formats via pretty.*, and
> those should operate in multiple contexts.
>
> In 3082517 (log --format: teach %C(auto,black) to respect
> color config, 2012-12-17), we gave an extended placeholder
> that could be used to accomplish this. But it's rather
> clunky to use, because you have to specify it individually
> for each color (and their matching resets) in the format.
> We shied away from just switching the default to auto,
> because it is technically breaking backwards compatibility.
>
> However, there's not really a use case for unconditional
> colors. The most plausible reason you would want them
> unconditional is to redirect "git log" output to a file. But
> there, the right answer is --color=always, as it does the
> right thing both with custom user-format colors and
> git-generated colors.
>
> So let's switch to the more useful default. In the
> off-chance that somebody really does find a use for
> unconditional colors without wanting to enable the rest of
> git's colors, we can provide %C(always,...) to enable the
> old behavior.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The tests unsurprisingly needed updating, as we're breaking the old
> behavior. The diff is easier to read read with "-w".
>
>  Documentation/pretty-formats.txt | 13 +++---
>  pretty.c                         | 19 +++++---
>  t/t6006-rev-list-format.sh       | 94 ++++++++++++++++++++--------------------
>  3 files changed, 70 insertions(+), 56 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index a942d57..7aa1a8b 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -167,11 +167,14 @@ endif::git-rev-list[]
>  - '%Cblue': switch color to blue
>  - '%Creset': reset color
>  - '%C(...)': color specification, as described in color.branch.* config option;
> -  adding `auto,` at the beginning will emit color only when colors are
> -  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). `auto` alone (i.e. `%C(auto)`) will turn on auto coloring
> -  on the next placeholders until the color is switched again.
> +  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`). `auto` alone (i.e. `%C(auto)`) will turn on auto
> +  coloring on the next placeholders until the color is switched again.
>  - '%m': left (`<`), right (`>`) or boundary (`-`) mark
>  - '%n': newline
>  - '%%': a raw '%'
> 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;
>  		}

Shouldn't we have an "else" here?

> +
> +		/* 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);
> -	else if (skip_prefix(placeholder + 1, "green", &rest))
> +	else if (skip_prefix(placeholder + 1, "green", &rest) &&
> +		 want_color(c->pretty_ctx->color))
>  		strbuf_addstr(sb, GIT_COLOR_GREEN);
> -	else if (skip_prefix(placeholder + 1, "blue", &rest))
> +	else if (skip_prefix(placeholder + 1, "blue", &rest) &&
> +		 want_color(c->pretty_ctx->color))
>  		strbuf_addstr(sb, GIT_COLOR_BLUE);
> -	else if (skip_prefix(placeholder + 1, "reset", &rest))
> +	else if (skip_prefix(placeholder + 1, "reset", &rest) &&
> +		 want_color(c->pretty_ctx->color))
>  		strbuf_addstr(sb, GIT_COLOR_RESET);
>  	return rest - placeholder;
>  }

Perhaps it's a funtion like add_color(sb, ctx, color) or similar would 
be nice?

René