{"thread":{"id":"42444","subject":"[PATCH] format_commit_message: honor `color=auto` for `%C(auto)`","startedAt":"2016-05-25T01:56:49Z","lastAt":"2016-05-31T22:18:05Z","messageCount":5,"participants":["Edward Thomson","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"287472","messageId":"20160525015649.GA13258@zoidberg","threadId":"42444","inReplyTo":null,"subject":"[PATCH] format_commit_message: honor `color=auto` for `%C(auto)`","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2016-05-25T01:56:49Z","receivedAt":"2016-05-25T01:56:49Z","isPatch":true,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"Check that we are configured to display colors in the given context when\nthe user specifies a format string of `%C(auto)`. This brings that\nbehavior in line with the behavior of `%C(auto,)`, which will\ndisplay the given color only when the configuration specifies to do so.\n\nThis allows the user the ability to specify that color should be\ndisplayed only when the output is a tty, and to use the default color\nfor the given context (instead of a hardcoded color value).\n\nSigned-off-by: Edward Thomson \n---\n pretty.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 87c4497..c3ec430 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1063,7 +1063,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tswitch (placeholder[0]) {\n \tcase 'C':\n \t\tif (starts_with(placeholder + 1, \"(auto)\")) {\n-\t\t\tc->auto_color = 1;\n+\t\t\tc->auto_color = want_color(c->pretty_ctx->color);\n \t\t\treturn 7; /* consumed 7 bytes, \"C(auto)\" */\n \t\t} else {\n \t\t\tint ret = parse_color(sb, placeholder, c);\n-- \n2.6.4 (Apple Git-63)\n"},{"id":"287522","messageId":"20160525223904.GD13776@sigill.intra.peff.net","threadId":"42444","inReplyTo":"20160525015649.GA13258@zoidberg","subject":"Re: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-25T22:39:04Z","receivedAt":"2016-05-25T22:39:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 24, 2016 at 08:56:49PM -0500, Edward Thomson wrote:\n\n> Check that we are configured to display colors in the given context when\n> the user specifies a format string of `%C(auto)`. This brings that\n> behavior in line with the behavior of `%C(auto,)`, which will\n> display the given color only when the configuration specifies to do so.\n> \n> This allows the user the ability to specify that color should be\n> displayed only when the output is a tty, and to use the default color\n> for the given context (instead of a hardcoded color value).\n> \n> Signed-off-by: Edward Thomson \n\nI somehow had trouble figuring out the problem from this description and\nthe patch. It seems to be about much more than just color=auto or a\ngiven context, and more like:\n\n When %C(auto) is used, we unconditionally turn on color for any\n subsequent placeholders, even if the user said \"--no-color\", or color\n config is turned off, or it is set to \"auto\" and we are not going to a\n tty.\n\nIt's possible somebody is relying on the ability to unconditionally turn\non color for \"auto-colored\" placeholders like \"%H\" or \"%d\", but I'm\ninclined to call this a strict bug-fix, for two reasons:\n\n 1. It says \"%C(auto)\", not \"%C(on)\".\n\n 2. This is documented as behaving like \"%C(auto,...)\", which as you\n note works in a more sane way.\n\nI think it's worth mentioning this explicitly in the commit message. We\ncould also add \"%C(on)\", I guess, but it's unclear to me whether anybody\nwould want it (they would probably just use \"--color\" in that case,\nunless they really want unconditional coloring for just _some_\nelements).\n\nI'm adding Duy to the cc as the original author of %C(auto), in case\nthere is something subtle I'm missing.\n\n> ---\n> pretty.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nLooks like we didn't have any tests at all for %C(auto). And the tests\nfor %C(auto,...) were labeled as %C(auto), making it all the more\nconfusing. Perhaps it is worth squashing this in:\n\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex b77d4c9..a1dcdb8 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -184,38 +184,38 @@ commit $head1\n \u001b[1;31;43mfoo\u001b[m\n EOF\n \n-test_expect_success '%C(auto) does not enable color by default' '\n+test_expect_success '%C(auto,...) does not enable color by default' '\n \tgit log --format=$AUTO_COLOR -1 >actual &&\n \thas_no_color actual\n '\n \n-test_expect_success '%C(auto) enables colors for color.diff' '\n+test_expect_success '%C(auto,...) enables colors for color.diff' '\n \tgit -c color.diff=always log --format=$AUTO_COLOR -1 >actual &&\n \thas_color actual\n '\n \n-test_expect_success '%C(auto) enables colors for color.ui' '\n+test_expect_success '%C(auto,...) enables colors for color.ui' '\n \tgit -c color.ui=always log --format=$AUTO_COLOR -1 >actual &&\n \thas_color actual\n '\n \n-test_expect_success '%C(auto) respects --color' '\n+test_expect_success '%C(auto,...) respects --color' '\n \tgit log --format=$AUTO_COLOR -1 --color >actual &&\n \thas_color actual\n '\n \n-test_expect_success '%C(auto) respects --no-color' '\n+test_expect_success '%C(auto,...) respects --no-color' '\n \tgit -c color.ui=always log --format=$AUTO_COLOR -1 --no-color >actual &&\n \thas_no_color actual\n '\n \n-test_expect_success TTY '%C(auto) respects --color=auto (stdout is tty)' '\n+test_expect_success TTY '%C(auto,...) respects --color=auto (stdout is tty)' '\n \ttest_terminal env TERM=vt100 \\\n \t\tgit log --format=$AUTO_COLOR -1 --color=auto >actual &&\n \thas_color actual\n '\n \n-test_expect_success '%C(auto) respects --color=auto (stdout not tty)' '\n+test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '\n \t(\n \t\tTERM=vt100 && export TERM &&\n \t\tgit log --format=$AUTO_COLOR -1 --color=auto >actual &&\n@@ -223,6 +223,18 @@ test_expect_success '%C(auto) respects --color=auto (stdout not tty)' '\n \t)\n '\n \n+test_expect_success '%C(auto) respects --color' '\n+\tgit log --color --format=\"%C(auto)%H\" -1 >actual &&\n+\tprintf \"\\\\033[33m%s\\\\033[m\\\\n\" $(git rev-parse HEAD) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%C(auto) respects --no-color' '\n+\tgit log --no-color --format=\"%C(auto)%H\" -1 >actual &&\n+\tgit rev-parse HEAD >expect &&\n+\ttest_cmp expect actual\n+'\n+\n iconv -f utf-8 -t $test_encoding > commit-msg < Looks like we didn't have any tests at all for %C(auto). And the tests\n> for %C(auto,...) were labeled as %C(auto), making it all the more\n> confusing. Perhaps it is worth squashing this in:\n\nThanks, peff. Indeed I did squash that into my updated patch.\n\n-ed\n"},{"id":"287893","messageId":"CACsJy8BF6woZy8WUsJzVFqaMDCOMEYK-3xFNNeOQ6B+OMyqJLw@mail.gmail.com","threadId":"42444","inReplyTo":"20160525223904.GD13776@sigill.intra.peff.net","subject":"Re: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)`","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-31T12:23:32Z","receivedAt":"2016-05-31T12:23:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, May 26, 2016 at 5:39 AM, Jeff King wrote:\n> On Tue, May 24, 2016 at 08:56:49PM -0500, Edward Thomson wrote:\n>\n>> Check that we are configured to display colors in the given context when\n>> the user specifies a format string of `%C(auto)`. This brings that\n>> behavior in line with the behavior of `%C(auto,)`, which will\n>> display the given color only when the configuration specifies to do so.\n>>\n>> This allows the user the ability to specify that color should be\n>> displayed only when the output is a tty, and to use the default color\n>> for the given context (instead of a hardcoded color value).\n>>\n>> Signed-off-by: Edward Thomson \n>\n> I somehow had trouble figuring out the problem from this description and\n> the patch. It seems to be about much more than just color=auto or a\n> given context, and more like:\n>\n> When %C(auto) is used, we unconditionally turn on color for any\n> subsequent placeholders, even if the user said \"--no-color\", or color\n> config is turned off, or it is set to \"auto\" and we are not going to a\n> tty.\n\nI think the (old) \"auto\" here means \"automatically select the\ncolor\" and what you do would be equivalent to %(auto,auto) where the\nfirst (and new) \"auto\" is about on/off switch, and the second is about\nselecting the actual color.\n\n> It's possible somebody is relying on the ability to unconditionally turn\n> on color for \"auto-colored\" placeholders like \"%H\" or \"%d\", but I'm\n> inclined to call this a strict bug-fix, for two reasons:\n>\n> 1. It says \"%C(auto)\", not \"%C(on)\".\n>\n> 2. This is documented as behaving like \"%C(auto,...)\", which as you\n> note works in a more sane way.\n>\n> I think it's worth mentioning this explicitly in the commit message. We\n> could also add \"%C(on)\", I guess, but it's unclear to me whether anybody\n> would want it (they would probably just use \"--color\" in that case,\n> unless they really want unconditional coloring for just _some_\n> elements).\n\nIf I could redo, I would go with %C(default) instead of %C(auto) then\nwe could have %C(auto,default). Perhaps we can make %C(auto) an\nequivalent of %C(auto,default) now (i.e. exactly what this patch does)\nand at some point in future add %C(default) which is what %C(auto) is\nnow if people really need to force it on?\n-- \nDuy\n"},{"id":"287943","messageId":"20160531221805.GB3824@sigill.intra.peff.net","threadId":"42444","inReplyTo":"CACsJy8BF6woZy8WUsJzVFqaMDCOMEYK-3xFNNeOQ6B+OMyqJLw@mail.gmail.com","subject":"Re: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-31T22:18:05Z","receivedAt":"2016-05-31T22:18:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 31, 2016 at 07:23:32PM +0700, Duy Nguyen wrote:\n\n> I think the (old) \"auto\" here means \"automatically select the\n> color\" and what you do would be equivalent to %(auto,auto) where the\n> first (and new) \"auto\" is about on/off switch, and the second is about\n> selecting the actual color.\n\nAh, right. The current behavior does make more sense if you realize we\nare talking about two different meaning of \"auto\" here.\n\n> > I think it's worth mentioning this explicitly in the commit message. We\n> > could also add \"%C(on)\", I guess, but it's unclear to me whether anybody\n> > would want it (they would probably just use \"--color\" in that case,\n> > unless they really want unconditional coloring for just _some_\n> > elements).\n> \n> If I could redo, I would go with %C(default) instead of %C(auto) then\n> we could have %C(auto,default). Perhaps we can make %C(auto) an\n> equivalent of %C(auto,default) now (i.e. exactly what this patch does)\n> and at some point in future add %C(default) which is what %C(auto) is\n> now if people really need to force it on?\n\nThat makes a lot of sense to me. It does change the current meaning of\n\"%C(auto)\", but the current state is sufficiently confusing that I think\nwe can call the existing behavior a bug. I'm ambivalent on either\nimplementing %C(default) now, or waiting until somebody actually wants\nit.\n\nThanks for clarifying the history.\n\n-Peff\n"}]}