Re: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)`

[Jeff King <peff@peff.net>]

On Tue, May 24, 2016 at 08:56:49PM -0500, Edward Thomson wrote:

> Check that we are configured to display colors in the given context when
> the user specifies a format string of `%C(auto)`.  This brings that
> behavior in line with the behavior of `%C(auto,<colorname>)`, which will
> display the given color only when the configuration specifies to do so.
> 
> This allows the user the ability to specify that color should be
> displayed only when the output is a tty, and to use the default color
> for the given context (instead of a hardcoded color value).
> 
> Signed-off-by: Edward Thomson <ethomson@edwardthomson.com>

I somehow had trouble figuring out the problem from this description and
the patch. It seems to be about much more than just color=auto or a
given context, and more like:

  When %C(auto) is used, we unconditionally turn on color for any
  subsequent placeholders, even if the user said "--no-color", or color
  config is turned off, or it is set to "auto" and we are not going to a
  tty.

It's possible somebody is relying on the ability to unconditionally turn
on color for "auto-colored" placeholders like "%H" or "%d", but I'm
inclined to call this a strict bug-fix, for two reasons:

  1. It says "%C(auto)", not "%C(on)".

  2. This is documented as behaving like "%C(auto,...)", which as you
     note works in a more sane way.

I think it's worth mentioning this explicitly in the commit message. We
could also add "%C(on)", I guess, but it's unclear to me whether anybody
would want it (they would probably just use "--color" in that case,
unless they really want unconditional coloring for just _some_
elements).

I'm adding Duy to the cc as the original author of %C(auto), in case
there is something subtle I'm missing.

> ---
>  pretty.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks like we didn't have any tests at all for %C(auto). And the tests
for %C(auto,...) were labeled as %C(auto), making it all the more
confusing. Perhaps it is worth squashing this in:

diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index b77d4c9..a1dcdb8 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -184,38 +184,38 @@ commit $head1
 ^[[1;31;43mfoo^[[m
 EOF
 
-test_expect_success '%C(auto) does not enable color by default' '
+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' '
+test_expect_success '%C(auto,...) enables colors for color.diff' '
 	git -c color.diff=always log --format=$AUTO_COLOR -1 >actual &&
 	has_color actual
 '
 
-test_expect_success '%C(auto) enables colors for color.ui' '
+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 '%C(auto) respects --color' '
+test_expect_success '%C(auto,...) respects --color' '
 	git log --format=$AUTO_COLOR -1 --color >actual &&
 	has_color actual
 '
 
-test_expect_success '%C(auto) respects --no-color' '
+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 TTY '%C(auto) respects --color=auto (stdout is tty)' '
+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)' '
+test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '
 	(
 		TERM=vt100 && export TERM &&
 		git log --format=$AUTO_COLOR -1 --color=auto >actual &&
@@ -223,6 +223,18 @@ test_expect_success '%C(auto) respects --color=auto (stdout not tty)' '
 	)
 '
 
+test_expect_success '%C(auto) respects --color' '
+	git log --color --format="%C(auto)%H" -1 >actual &&
+	printf "\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%C(auto) respects --no-color' '
+	git log --no-color --format="%C(auto)%H" -1 >actual &&
+	git rev-parse HEAD >expect &&
+	test_cmp expect actual
+'
+
 iconv -f utf-8 -t $test_encoding > commit-msg <<EOF
 Test printing of complex bodies