Re: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)`
On Tue, May 24, 2016 at 08:56:49PM -0500, Edward Thomson wrote:
Show 10 quoted lines
> 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:
Show changes to t/t6006-rev-list-format.sh +19 −7
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
Edward Thomson
