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

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

[Edward Thomson]

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

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

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

[Edward Thomson]

On Wed, May 25, 2016 at 05:39:04PM -0500, Jeff King wrote:
> 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:

Thanks, peff.  Indeed I did squash that into my updated patch.

-ed

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

[Duy Nguyen]

On Thu, May 26, 2016 at 5:39 AM, Jeff King <peff@peff.net> wrote:
> 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.

I think the (old) "auto" here means "automatically select the
color" and what you do would be equivalent to %(auto,auto) where the
first (and new) "auto" is about on/off switch, and the second is about
selecting the actual color.

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

If I could redo, I would go with %C(default) instead of %C(auto) then
we could have %C(auto,default). Perhaps we can make %C(auto) an
equivalent of %C(auto,default) now (i.e. exactly what this patch does)
and at some point in future add %C(default) which is what %C(auto) is
now if people really need to force it on?
-- 
Duy

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

[Jeff King]

On Tue, May 31, 2016 at 07:23:32PM +0700, Duy Nguyen wrote:

> I think the (old) "auto" here means "automatically select the
> color" and what you do would be equivalent to %(auto,auto) where the
> first (and new) "auto" is about on/off switch, and the second is about
> selecting the actual color.

Ah, right. The current behavior does make more sense if you realize we
are talking about two different meaning of "auto" here.

> > 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).
> 
> If I could redo, I would go with %C(default) instead of %C(auto) then
> we could have %C(auto,default). Perhaps we can make %C(auto) an
> equivalent of %C(auto,default) now (i.e. exactly what this patch does)
> and at some point in future add %C(default) which is what %C(auto) is
> now if people really need to force it on?

That makes a lot of sense to me. It does change the current meaning of
"%C(auto)", but the current state is sufficiently confusing that I think
we can call the existing behavior a bug. I'm ambivalent on either
implementing %C(default) now, or waiting until somebody actually wants
it.

Thanks for clarifying the history.

-Peff

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

[Edward Thomson]

git-log(1) documents that when specifying the `%C(auto)` format
placeholder will "turn on auto coloring on the next %placeholders
until the color is switched again."

However, when `%C(auto)` is used, the present implementation will turn
colors on unconditionally (even if the color configuration is turned off
for the current context - for example, `--no-color` was specified or the
color is `auto` and the output is not a tty).

Update `format_commit_one` to examine the current context when a format
string of `%C(auto)` is specified, which ensures that we will not
unconditionally write colors.  This brings that behavior in line with
the behavior of `%C(auto,<colorname>)`, and allows the user the ability
to specify that color should be displayed only when the output is a
tty.

Additionally, add a test for `%C(auto)` and update the existing tests
for `%C(auto,...)` as they were misidentified as being applicable to
`%C(auto)`.

Signed-off-by: Edward Thomson <ethomson@edwardthomson.com>

Tests from Jeff King.

Signed-off-by: Jeff King <peff@peff.net>
---
 pretty.c                   |  2 +-
 t/t6006-rev-list-format.sh | 26 +++++++++++++++++++-------
 2 files changed, 20 insertions(+), 8 deletions(-)

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);
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
 
-- 
2.7.4 (Apple Git-66)

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

[Jeff King]

On Thu, May 26, 2016 at 10:46:10PM -0500, Edward Thomson wrote:

> git-log(1) documents that when specifying the `%C(auto)` format
> placeholder will "turn on auto coloring on the next %placeholders
> until the color is switched again."
> 
> However, when `%C(auto)` is used, the present implementation will turn
> colors on unconditionally (even if the color configuration is turned off
> for the current context - for example, `--no-color` was specified or the
> color is `auto` and the output is not a tty).
> 
> Update `format_commit_one` to examine the current context when a format
> string of `%C(auto)` is specified, which ensures that we will not
> unconditionally write colors.  This brings that behavior in line with
> the behavior of `%C(auto,<colorname>)`, and allows the user the ability
> to specify that color should be displayed only when the output is a
> tty.
> 
> Additionally, add a test for `%C(auto)` and update the existing tests
> for `%C(auto,...)` as they were misidentified as being applicable to
> `%C(auto)`.

Explanation and the patch look good.

> Signed-off-by: Edward Thomson <ethomson@edwardthomson.com>
> 
> Tests from Jeff King.
> 
> Signed-off-by: Jeff King <peff@peff.net>

Trailers should all go at the bottom in a single stanza, and should
generally be in chronological order (so you got the bits from with an
s-o-b, and then you signed off the whole thing). IOW:

> Tests from Jeff King.
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Edward Thomson <ethomson@edwardthomson.com>

I suspect Junio can just tweak that while applying, unless there's
another reason to re-roll.

(Also for anybody watching, Ed did not just make up my signoff; I gave
it to him off-list).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I suspect Junio can just tweak that while applying, unless there's
> another reason to re-roll.
>
> (Also for anybody watching, Ed did not just make up my signoff; I gave
> it to him off-list).

Understood.  Thanks.

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

[Edward Thomson]

Check that we are configured to display colors in the given context when
the user specifies a format string of `%C(auto)`, instead of always
displaying the default color for the given context.  This makes
`%C(auto)` obey the `color=auto` setting and brings its behavior in line
with the behavior of `%C(auto,<colorname>)`.

This allows the user the ability to specify that color should be
displayed for a string only when the output is a tty, and to use the
default color for the given context without having to hardcode a
color value.
---
 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)

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

[Junio C Hamano]

On Tue, May 24, 2016 at 12:48 PM, Edward Thomson
<ethomson@edwardthomson.com> wrote:
> Check that we are configured to display colors in the given context when
> the user specifies a format string of `%C(auto)`, instead of always
> displaying the default color for the given context.  This makes
> `%C(auto)` obey the `color=auto` setting and brings its behavior in line
> with the behavior of `%C(auto,<colorname>)`.
>
> This allows the user the ability to specify that color should be
> displayed for a string only when the output is a tty, and to use the
> default color for the given context without having to hardcode a
> color value.
> ---
>  pretty.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks obviously the right thing to do from a cursory read.
Missing sign-off?

Thanks.

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

[Edward Thomson]

On Tue, May 24, 2016 at 05:55:02PM -0700, Junio C Hamano wrote:
> Looks obviously the right thing to do from a cursory read.
> Missing sign-off?

Yes, apologies for the oversight; will re-send.

Thanks-
-ed