From: René Scharfe Date: Mon, 10 Oct 2016 17:09:12 GMT Subject: Re: [PATCH] pretty: respect color settings for %C placeholders Message-ID: <42b3be14-fadb-4801-f377-59fb42b7ccdf@web.de> In-Reply-To: <20161010151517.6wszhuyp57yfncaj@sigill.intra.peff.net> 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 > --- > 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é