%C(auto) not working as expected

%C(auto) not working as expected

[Tom Hale]

$ ~/repo/git/git --version
git version 2.10.0.GIT

The `git-log` man page says:

`auto` alone (i.e.  %C(auto)) will turn on auto coloring on the next 
placeholders until the color is switched again.

In this example:

http://i.imgur.com/y3yLxk7.png

I turn on auto colouring for green, but it seems that this is not being 
respected when piped through `cat`.

Am I misunderstanding the manual?

-- 
Tom Hale

Re: %C(auto) not working as expected

[René Scharfe]

Am 09.10.2016 um 07:43 schrieb Tom Hale:
> $ ~/repo/git/git --version
> git version 2.10.0.GIT
>
> The `git-log` man page says:
>
> `auto` alone (i.e.  %C(auto)) will turn on auto coloring on the next
> placeholders until the color is switched again.
>
> In this example:
>
> http://i.imgur.com/y3yLxk7.png
>
> I turn on auto colouring for green, but it seems that this is not being
> respected when piped through `cat`.
>
> Am I misunderstanding the manual?

So this colors the ref name decoration and the short hash:

	$ git log -n1 --format="%C(auto)%d %C(auto)%Cgreen%h"

And this only colors the short hash:

	$ git log -n1 --format="%C(auto)%d %C(auto)%Cgreen%h" | cat

%C(auto) respects the color-related configuration settings; that's 
mentioned on the man page for git log in the section on %C(...).  You 
probably have color.ui=auto or color.diff=auto in your config, which 
means that output to terminals is to be colored, but output to files and 
pipes isn't.  You could override that setting e.g. by adding the command 
line option --color=always.

%Cgreen emits color codes unconditionally.  %C(auto,green) would respect 
the config settings.

Your second %C(auto) has no effect because it is overridden by the 
immediately following %Cgreen.

You may want to add a %Creset at the end to avoid colors bleeding out 
over the end of the line.  You can see that happening e.g. with:

	$ git log -n3 --format="normal %C(green)green" | cat

Without cat bash seems to add a reset automatically.

René

Re: %C(auto) not working as expected

[Tom Hale]

On 2016-10-09 13:47, René Scharfe wrote:

> %Cgreen emits color codes unconditionally.  %C(auto,green) would respect
> the config settings.

Thanks, I've never seen the (,) syntax documented before!

What's strange is that this works:
   %C(auto,green bold)
but
   %C(auto,green,bold)
does not.

Also:
Given it's very rare to want only part of a string to emit colour codes, 
if something like "bold" carries through until a "no-bold", why doesn't 
"auto" do the same thing?

-- 
Tom Hale

Re: %C(auto) not working as expected

[René Scharfe]

Am 09.10.2016 um 12:04 schrieb Tom Hale:
> On 2016-10-09 13:47, René Scharfe wrote:
> 
>> %Cgreen emits color codes unconditionally.  %C(auto,green) would respect
>> the config settings.
> 
> Thanks, I've never seen the (,) syntax documented before!

Both the prefix "auto," for terminal-detection and "%C(auto)" for
choosing colors automatically are mentioned in the manpage for git log
(from Documentation/pretty-formats.txt):

- '%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.

> What's strange is that this works:
>   %C(auto,green bold)
> but
>   %C(auto,green,bold)
> does not.

Looking at the code that's not surprising; colors and attributes are
interpreted as a space-separated list.  The prefix "auto," is handled
specially.  For a user it may look strange, admittedly.

Supporting "auto " as well would be easy.  Supporting it in such a way
that it can be mixed freely with colors and attributes as in
%C(bold auto green) would be a bit harder.  Could this lead to confusion
between the auto for terminal-detection and the one for automatic color
selection?

The documentation cited above says the color specification was explained
together with the color.branch.* config option, but that part only says
(from Documentation/config.txt):

color.branch.<slot>::
        Use customized color for branch coloration. `<slot>` is one of
        `current` (the current branch), `local` (a local branch),
        `remote` (a remote-tracking branch in refs/remotes/),
        `upstream` (upstream tracking branch), `plain` (other
        refs).

It really is described earlier in the same file, in the Values section
(a fitting place, I think).  Here's just the first sentence:

color::
       The value for a variable that takes a color is a list of
       colors (at most two, one for foreground and one for background)
       and attributes (as many as you want), separated by spaces.

Patch below.  Does it help a little?

> Also:
> Given it's very rare to want only part of a string to emit colour codes,
> if something like "bold" carries through until a "no-bold", why doesn't
> "auto" do the same thing?

No state is kept for "auto,".  Attributes and colors are switched
separately by terminals, that's why e.g. bold stays in effect through
a color change -- unless you specify an attribute change as well.

Offering a way to enable terminal-detection for all color codes of a
format would be useful, but using the existing "auto," prefix for that
would be a behaviour change that could surprise users.

René


-- >8 --
Subject: [PATCH] pretty: fix document reference for color specification

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 Documentation/pretty-formats.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a942d57..89e3bc6 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -166,7 +166,8 @@ endif::git-rev-list[]
 - '%Cgreen': switch color to green
 - '%Cblue': switch color to blue
 - '%Creset': reset color
-- '%C(...)': color specification, as described in color.branch.* config option;
+- '%C(...)': color specification, as described under Values, color in the
+  "CONFIGURATION FILE" section of linkgit:git-config[1];
   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
-- 
2.10.1

Re: %C(auto) not working as expected

[Jeff King]

On Sun, Oct 09, 2016 at 03:24:17PM +0200, René Scharfe wrote:

> Offering a way to enable terminal-detection for all color codes of a
> format would be useful, but using the existing "auto," prefix for that
> would be a behaviour change that could surprise users.

Yeah. In retrospect, it probably would have been saner to make %C(red) a
noop when --color is not in effect (either because of --no-color, or
more likely when --color=auto is in effect and stdout is not a
terminal). But that ship has long since sailed, I think.

If we do a revamp of the pretty-formats to bring them more in line with
ref-filter (e.g., something like "%(color:red)") maybe that would be an
opportunity to make minor adjustments. Though, hmm, it looks like
for-each-ref already knows "%(color:red)", and it's unconditional.
<sigh> So perhaps we would need to go through some deprecation period or
other transition.

> ---
>  Documentation/pretty-formats.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index a942d57..89e3bc6 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -166,7 +166,8 @@ endif::git-rev-list[]
>  - '%Cgreen': switch color to green
>  - '%Cblue': switch color to blue
>  - '%Creset': reset color
> -- '%C(...)': color specification, as described in color.branch.* config option;
> +- '%C(...)': color specification, as described under Values, color in the
> +  "CONFIGURATION FILE" section of linkgit:git-config[1];

I like the intent here, though I found "Values, color" hard to parse (it
was not immediately clear that you mean "the color paragraph of the
Values section", as commas are already being used in that sentence for
the parenthetical phrase).

I'm not sure how to say that succinctly, as we are four levels deep
(git-config -> configuration file -> values -> color). Too bad there is
no good link syntax for it. Maybe:

  ...color specification, as described in linkgit:git-config[1] (see the
  paragraph on colors in the "Values" section, under "CONFIGURATION
  FILE")

or something.

-Peff

Re: %C(auto) not working as expected

[Duy Nguyen]

On Mon, Oct 10, 2016 at 6:46 AM, Jeff King <peff@peff.net> wrote:
> On Sun, Oct 09, 2016 at 03:24:17PM +0200, René Scharfe wrote:
>
>> Offering a way to enable terminal-detection for all color codes of a
>> format would be useful, but using the existing "auto," prefix for that
>> would be a behaviour change that could surprise users.

I wonder if we made a mistake associating terminal-detection with
%C(auto,...). The more likely use case is enable or disable all
colors, not "the next tag".

> Yeah. In retrospect, it probably would have been saner to make %C(red) a
> noop when --color is not in effect (either because of --no-color, or
> more likely when --color=auto is in effect and stdout is not a
> terminal). But that ship has long since sailed, I think.
>
> If we do a revamp of the pretty-formats to bring them more in line with
> ref-filter (e.g., something like "%(color:red)") maybe that would be an
> opportunity to make minor adjustments. Though, hmm, it looks like
> for-each-ref already knows "%(color:red)", and it's unconditional.
> <sigh> So perhaps we would need to go through some deprecation period or
> other transition.

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?
-- 
Duy

Re: %C(auto) not working as expected

[Jeff King]

On Mon, Oct 10, 2016 at 04:26:18PM +0700, Duy Nguyen wrote:

> > If we do a revamp of the pretty-formats to bring them more in line with
> > ref-filter (e.g., something like "%(color:red)") maybe that would be an
> > opportunity to make minor adjustments. Though, hmm, it looks like
> > for-each-ref already knows "%(color:red)", and it's unconditional.
> > <sigh> So perhaps we would need to go through some deprecation period or
> > other transition.
> 
> 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).

-Peff

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

[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.

-- >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;
 		}
+
+		/* 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;
 }
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index a1dcdb8..29b1a1a 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -59,7 +59,10 @@ test_format () {
 }
 
 # Feed to --format to provide predictable colored sequences.
+BASIC_COLOR='%Credfoo%Creset'
+COLOR='%C(red)foo%C(reset)'
 AUTO_COLOR='%C(auto,red)foo%C(auto,reset)'
+ALWAYS_COLOR='%C(always,red)foo%C(always,reset)'
 has_color () {
 	printf '\033[31mfoo\033[m\n' >expect &&
 	test_cmp expect "$1"
@@ -170,57 +173,56 @@ $added
 
 EOF
 
-test_format colors %Credfoo%Cgreenbar%Cbluebaz%Cresetxyzzy <<EOF
-commit $head2
-^[[31mfoo^[[32mbar^[[34mbaz^[[mxyzzy
-commit $head1
-^[[31mfoo^[[32mbar^[[34mbaz^[[mxyzzy
-EOF
-
-test_format advanced-colors '%C(red yellow bold)foo%C(reset)' <<EOF
-commit $head2
-^[[1;31;43mfoo^[[m
-commit $head1
-^[[1;31;43mfoo^[[m
-EOF
-
-test_expect_success '%C(auto,...) does not enable color by default' '
-	git log --format=$AUTO_COLOR -1 >actual &&
-	has_no_color actual
-'
-
-test_expect_success '%C(auto,...) enables colors for color.diff' '
-	git -c color.diff=always log --format=$AUTO_COLOR -1 >actual &&
-	has_color actual
-'
+for spec in \
+	"%Cred:$BASIC_COLOR" \
+	"%C(...):$COLOR" \
+	"%C(auto,...):$AUTO_COLOR"
+do
+	desc=${spec%%:*}
+	color=${spec#*:}
+	test_expect_success "$desc does not enable color by default" '
+		git log --format=$color -1 >actual &&
+		has_no_color actual
+	'
 
-test_expect_success '%C(auto,...) enables colors for color.ui' '
-	git -c color.ui=always log --format=$AUTO_COLOR -1 >actual &&
-	has_color actual
-'
+	test_expect_success "$desc enables colors for color.diff" '
+		git -c color.diff=always log --format=$color -1 >actual &&
+		has_color actual
+	'
 
-test_expect_success '%C(auto,...) respects --color' '
-	git log --format=$AUTO_COLOR -1 --color >actual &&
-	has_color actual
-'
+	test_expect_success "$desc enables colors for color.ui" '
+		git -c color.ui=always log --format=$color -1 >actual &&
+		has_color actual
+	'
 
-test_expect_success '%C(auto,...) respects --no-color' '
-	git -c color.ui=always log --format=$AUTO_COLOR -1 --no-color >actual &&
-	has_no_color actual
-'
+	test_expect_success "$desc respects --color" '
+		git log --format=$color -1 --color >actual &&
+		has_color actual
+	'
 
-test_expect_success TTY '%C(auto,...) respects --color=auto (stdout is tty)' '
-	test_terminal env TERM=vt100 \
-		git log --format=$AUTO_COLOR -1 --color=auto >actual &&
-	has_color actual
-'
-
-test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '
-	(
-		TERM=vt100 && export TERM &&
-		git log --format=$AUTO_COLOR -1 --color=auto >actual &&
+	test_expect_success "$desc respects --no-color" '
+		git -c color.ui=always log --format=$color -1 --no-color >actual &&
 		has_no_color actual
-	)
+	'
+
+	test_expect_success TTY "$desc respects --color=auto (stdout is tty)" '
+		test_terminal env TERM=vt100 \
+			git log --format=$color -1 --color=auto >actual &&
+		has_color actual
+	'
+
+	test_expect_success "$desc respects --color=auto (stdout not tty)" '
+		(
+			TERM=vt100 && export TERM &&
+			git log --format=$color -1 --color=auto >actual &&
+			has_no_color actual
+		)
+	'
+done
+
+test_expect_success '%C(always,...) enables color even without tty' '
+	git log --format=$ALWAYS_COLOR -1 >actual &&
+	has_color actual
 '
 
 test_expect_success '%C(auto) respects --color' '
-- 
2.10.1.527.g93d4615

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

[René Scharfe]

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é

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

[Jeff King]

On Mon, Oct 10, 2016 at 07:09:12PM +0200, René Scharfe wrote:

> > 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?

I'm not sure what you mean; can you write it out?

> > -	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?

I actually wrote it that way first (I called it "maybe_add_color()"),
but it felt a little funny to have a separate function that people might
be tempted to reuse (the right solution is generally to check
want_color() early as above, but we can't do that here because we have
to find the end of each placeholder).

What I have here is a little funny, too, though, as it keeps trying
other color names if it finds "red" but want_color() returns 0.

-Peff

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

[René Scharfe]

Am 10.10.2016 um 19:42 schrieb Jeff King:
> On Mon, Oct 10, 2016 at 07:09:12PM +0200, René Scharfe wrote:
>
>>> 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?
>
> I'm not sure what you mean; can you write it out?

 > -		if (skip_prefix(begin, "auto,", &begin)) {
 > +
 > +		if (!skip_prefix(begin, "always,", &begin)) {
 >  			if (!want_color(c->pretty_ctx->color))
 >  				return end - placeholder + 1;
 >  		}

		else {	/* here */

 > +		/* this is a historical noop */
 > +		skip_prefix(begin, "auto,", &begin);

		}

Otherwise "always,auto," would be allowed and mean the same as 
"always,", which just seems wrong.  Not a biggie.

>>> -	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?
>
> I actually wrote it that way first (I called it "maybe_add_color()"),
> but it felt a little funny to have a separate function that people might
> be tempted to reuse (the right solution is generally to check
> want_color() early as above, but we can't do that here because we have
> to find the end of each placeholder).

OK.  A variable then?  Lazy pseudo-code:

	if (RED)
		color = red;
	else if (GREEN)
		...

	if (want_color())
		strbuf_addstr(sb, color);

> What I have here is a little funny, too, though, as it keeps trying
> other color names if it finds "red" but want_color() returns 0.

Oh, missed that somehow.. O_o

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

[Jeff King]

On Mon, Oct 10, 2016 at 09:59:17PM +0200, René Scharfe wrote:

> > > Shouldn't we have an "else" here?
> > 
> > I'm not sure what you mean; can you write it out?
> 
> > -		if (skip_prefix(begin, "auto,", &begin)) {
> > +
> > +		if (!skip_prefix(begin, "always,", &begin)) {
> >  			if (!want_color(c->pretty_ctx->color))
> >  				return end - placeholder + 1;
> >  		}
> 
> 		else {	/* here */
> 
> > +		/* this is a historical noop */
> > +		skip_prefix(begin, "auto,", &begin);
> 
> 		}
> 
> Otherwise "always,auto," would be allowed and mean the same as "always,",
> which just seems wrong.  Not a biggie.

I don't think that will parse "%C(auto,foo)", as we hit the
!skip_prefix() of the conditional, and do not look for "auto," at all.

I think you'd have to move the check for "auto," inside the if block.

I'm leaning towards just writing it out the long way, though, as I did
in my reply to Junio.

> > > Perhaps it's a funtion like add_color(sb, ctx, color) or similar would be
> > > nice?
> > 
> > I actually wrote it that way first (I called it "maybe_add_color()"),
> > but it felt a little funny to have a separate function that people might
> > be tempted to reuse (the right solution is generally to check
> > want_color() early as above, but we can't do that here because we have
> > to find the end of each placeholder).
> 
> OK.  A variable then?  Lazy pseudo-code:
> 
> 	if (RED)
> 		color = red;
> 	else if (GREEN)
> 		...
> 
> 	if (want_color())
> 		strbuf_addstr(sb, color);

Yeah, that is a bit more clear (the final conditional just needs to
check that we actually found a color).

-Peff

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

[Junio C Hamano]

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.

> +  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...

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

[Jeff King]

On Mon, Oct 10, 2016 at 11:52:14AM -0700, Junio C Hamano wrote:

> > +  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=...".

True. I consider that a feature and more likely to be pointing the user
in the right direction, but it should probably be s/format/output/ in
the last sentence.

> > 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?

I'll see what I can do. The most readable one is probably:

  if (skip_prefix(begin, "auto,", &begin)) {
	if (!want_color(c->pretty_ctx->color))
		return end - placeholder + 1;
  } else if (skip_prefix(begin, "always,", &begin)) {
	/* nothing to do; we do not respect want_color at all */
  } else {
	/* the default is now the same as "auto" */
	if (!want_color(c->pretty_ctx->color))
		return end - placeholder + 1;
  }

I avoided that because of the repetition, but it probably is not too
bad.

> > -	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...

No, we don't come here from %C() at all. This is for bare "%Cred", which
cannot have "always" (or "auto"), as there is no syntactic spot for it.
It is mostly historical (it _only_ supports red, green, and blue). We
could actually leave this as-is to show the colors unconditionally. I
changed it to keep the new behavior consistent, but I doubt anybody
cares much either way.

-Peff

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

[Jeff King]

On Mon, Oct 10, 2016 at 02:59:35PM -0400, Jeff King wrote:

> > I must be reading the patch incorrectly, but I cannot quite tell
> > where I want astray...
> 
> No, we don't come here from %C() at all. This is for bare "%Cred", which
> cannot have "always" (or "auto"), as there is no syntactic spot for it.
> It is mostly historical (it _only_ supports red, green, and blue). We
> could actually leave this as-is to show the colors unconditionally. I
> changed it to keep the new behavior consistent, but I doubt anybody
> cares much either way.

Speaking of consistent behavior, if we do this, I think we should give
"%(color:red)" in for-each-ref and tag the same treatment. That requires
some infrastructure refactoring to get the value down to the
ref-formatting code. I'm working on it, but might not have it out today.

-Peff

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

[Duy Nguyen]

On Mon, Oct 10, 2016 at 10:15 PM, Jeff King <peff@peff.net> wrote:
> 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.

I think you covered all bases with %C(always,..) and updating
for-each-ref code. And changing behavior of visual features like this
sounds more like "evolving" than "breaking backward compatibility" to
me. So +1.
-- 
Duy

Re: %C(auto) not working as expected

[Duy Nguyen]

On Mon, Oct 10, 2016 at 9:28 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Oct 10, 2016 at 04:26:18PM +0700, Duy Nguyen wrote:
>
>> > If we do a revamp of the pretty-formats to bring them more in line with
>> > ref-filter (e.g., something like "%(color:red)") maybe that would be an
>> > opportunity to make minor adjustments. Though, hmm, it looks like
>> > for-each-ref already knows "%(color:red)", and it's unconditional.
>> > <sigh> So perhaps we would need to go through some deprecation period or
>> > other transition.
>>
>> 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.

Silence in two weeks. I vote (*) making %(<color-name>) honor --color
and turning the %(auto, no-op, for both log family and for-each-ref.
We could keep old behavior behind some environment variable if it's
not much work so it keeps working while people come here and tell us
about their use cases.

(*) I know.. voting is not how things work around here, unless you
vote with patches, but I can't take on another topic.

> 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).
>
> -Peff
-- 
Duy

Re: %C(auto) not working as expected

[Jeff King]

On Tue, Oct 25, 2016 at 07:52:21PM +0700, Duy Nguyen wrote:

> > 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.
> 
> Silence in two weeks. I vote (*) making %(<color-name>) honor --color
> and turning the %(auto, no-op, for both log family and for-each-ref.
> We could keep old behavior behind some environment variable if it's
> not much work so it keeps working while people come here and tell us
> about their use cases.

Yeah, sorry. I was blocked on making %(color:) in ref-filter work, which
required a bunch of refactoring in ref-filter, which conflicted heavily
with the kn/ref-filter-branch-list (which wants to do a lot of the same
things), and then I got blocked on reviewing that series (which overall
looks pretty sane, but I wanted to really dig in because I think it
hasn't gotten very careful review, or at least not recently).

So I'm still hoping to shave that yak at some point. Maybe this week.

-Peff

Re: %C(auto) not working as expected

[Duy Nguyen]

On Tue, Oct 25, 2016 at 7:58 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 25, 2016 at 07:52:21PM +0700, Duy Nguyen wrote:
>
>> > 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.
>>
>> Silence in two weeks. I vote (*) making %(<color-name>) honor --color
>> and turning the %(auto, no-op, for both log family and for-each-ref.
>> We could keep old behavior behind some environment variable if it's
>> not much work so it keeps working while people come here and tell us
>> about their use cases.
>
> Yeah, sorry.

Just to be clear (after re-reading my mail), the unwritten part after
"Silence in two weeks" was "so nobody disagreed with you", not "how is
your progress?". It's great that you keep working on this regardless
:D
-- 
Duy

Re: %C(auto) not working as expected

[René Scharfe]

Am 10.10.2016 um 01:46 schrieb Jeff King:
>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
>> index a942d57..89e3bc6 100644
>> --- a/Documentation/pretty-formats.txt
>> +++ b/Documentation/pretty-formats.txt
>> @@ -166,7 +166,8 @@ endif::git-rev-list[]
>>  - '%Cgreen': switch color to green
>>  - '%Cblue': switch color to blue
>>  - '%Creset': reset color
>> -- '%C(...)': color specification, as described in color.branch.* config option;
>> +- '%C(...)': color specification, as described under Values, color in the
>> +  "CONFIGURATION FILE" section of linkgit:git-config[1];
> 
> I like the intent here, though I found "Values, color" hard to parse (it
> was not immediately clear that you mean "the color paragraph of the
> Values section", as commas are already being used in that sentence for
> the parenthetical phrase).
> 
> I'm not sure how to say that succinctly, as we are four levels deep
> (git-config -> configuration file -> values -> color). Too bad there is
> no good link syntax for it. Maybe:
> 
>   ...color specification, as described in linkgit:git-config[1] (see the
>   paragraph on colors in the "Values" section, under "CONFIGURATION
>   FILE")
> 
> or something.

That's better.  Or we could remove the "color" part and trust readers to
find the right paragraph in the Values sub-section?

-- '%C(...)': color specification, as described in color.branch.* config option;
+- '%C(...)': color specification, as described under Values in the
+  "CONFIGURATION FILE" section of linkgit:git-config[1];

Re: %C(auto) not working as expected

[Jeff King]

On Mon, Oct 10, 2016 at 10:52:47PM +0200, René Scharfe wrote:

> > I like the intent here, though I found "Values, color" hard to parse (it
> > was not immediately clear that you mean "the color paragraph of the
> > Values section", as commas are already being used in that sentence for
> > the parenthetical phrase).
> > 
> > I'm not sure how to say that succinctly, as we are four levels deep
> > (git-config -> configuration file -> values -> color). Too bad there is
> > no good link syntax for it. Maybe:
> > 
> >   ...color specification, as described in linkgit:git-config[1] (see the
> >   paragraph on colors in the "Values" section, under "CONFIGURATION
> >   FILE")
> > 
> > or something.
> 
> That's better.  Or we could remove the "color" part and trust readers to
> find the right paragraph in the Values sub-section?
> 
> -- '%C(...)': color specification, as described in color.branch.* config option;
> +- '%C(...)': color specification, as described under Values in the
> +  "CONFIGURATION FILE" section of linkgit:git-config[1];

Yeah, I think that is plenty of roadmap to give the user.

-Peff

[PATCH v2] pretty: fix document link for color specification

[René Scharfe]

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Removed confusing and unnecessary reference to the "colors" paragraph.

 Documentation/pretty-formats.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a942d57..89e3bc6 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -166,7 +166,8 @@ endif::git-rev-list[]
 - '%Cgreen': switch color to green
 - '%Cblue': switch color to blue
 - '%Creset': reset color
-- '%C(...)': color specification, as described in color.branch.* config option;
+- '%C(...)': color specification, as described under Values in the
+  "CONFIGURATION FILE" section of linkgit:git-config[1];
   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
-- 
2.10.1

Re: [PATCH v2] pretty: fix document link for color specification

[Jeff King]

On Tue, Oct 11, 2016 at 05:41:14AM +0200, René Scharfe wrote:

> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> Removed confusing and unnecessary reference to the "colors" paragraph.

This looks good to me.

-Peff