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@xxxxxxxxxxxxxxxxxxxxxxxx/
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@xxxxxxxx>
---
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é