# [PATCH] format_commit_message: honor `color=auto` for `%C(auto)` 5 messages from 2016-05-25 to 2016-05-31. Participants: Edward Thomson, Jeff King, Duy Nguyen. Thread: https://gitlist.dev/t/42444 ## Edward Thomson, 2016-05-25 01:56 Subject: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)` Message-ID: <20160525015649.GA13258@zoidberg> URL: https://gitlist.dev/e/20160525015649.GA13258%40zoidberg ``` 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,)`, 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 --- 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) ``` ## Jeff King, 2016-05-25 22:39 Subject: Re: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)` Message-ID: <20160525223904.GD13776@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160525223904.GD13776%40sigill.intra.peff.net In-Reply-To: <20160525015649.GA13258@zoidberg> ``` 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,)`, 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 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 < URL: https://gitlist.dev/e/20160527034748.GB31629%40zoidberg In-Reply-To: <20160525223904.GD13776@sigill.intra.peff.net> ``` 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 ``` ## Duy Nguyen, 2016-05-31 12:23 Subject: Re: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)` Message-ID: URL: https://gitlist.dev/e/CACsJy8BF6woZy8WUsJzVFqaMDCOMEYK-3xFNNeOQ6B%2BOMyqJLw%40mail.gmail.com In-Reply-To: <20160525223904.GD13776@sigill.intra.peff.net> ``` On Thu, May 26, 2016 at 5:39 AM, Jeff King 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,)`, 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 > > 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 ``` ## Jeff King, 2016-05-31 22:18 Subject: Re: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)` Message-ID: <20160531221805.GB3824@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160531221805.GB3824%40sigill.intra.peff.net In-Reply-To: ``` 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 ```