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

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Edward Thomson-2
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 <[hidden email]>
---
 pretty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index 87c4497..c3ec430 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1063,7 +1063,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
  switch (placeholder[0]) {
  case 'C':
  if (starts_with(placeholder + 1, "(auto)")) {
- c->auto_color = 1;
+ c->auto_color = want_color(c->pretty_ctx->color);
  return 7; /* consumed 7 bytes, "C(auto)" */
  } else {
  int ret = parse_color(sb, placeholder, c);
--
2.6.4 (Apple Git-63)

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Jeff King
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 <[hidden email]>

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
 foo
 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
 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html