Jeff King <p...@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.
> + 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=...".
> 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?
> +
> + /* 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...
