{"thread":{"id":"44256","subject":"%C(auto) not working as expected","startedAt":"2016-10-09T05:43:57Z","lastAt":"2016-10-25T13:03:23Z","messageCount":23,"participants":["Tom Hale","René Scharfe","Jeff King","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"303653","messageId":"cb81631e-9623-9020-f955-ec215b493a50@hale.ee","threadId":"44256","inReplyTo":null,"subject":"%C(auto) not working as expected","fromName":"Tom Hale","fromEmail":"tom@hale.ee","sentAt":"2016-10-09T05:43:49Z","receivedAt":"2016-10-09T05:43:57Z","isPatch":false,"sender":{"key":"tom@hale.ee","avatar":"https://gravatar.com/avatar/61d620fad9c6227a6f41dee4cdb68be18a7d8bf8c69c3fd70d6b3c65eb65eea8?d=mp&s=160"},"body":"$ ~/repo/git/git --version\ngit version 2.10.0.GIT\n\nThe `git-log` man page says:\n\n`auto` alone (i.e. %C(auto)) will turn on auto coloring on the next \nplaceholders until the color is switched again.\n\nIn this example:\n\nhttp://i.imgur.com/y3yLxk7.png\n\nI turn on auto colouring for green, but it seems that this is not being \nrespected when piped through `cat`.\n\nAm I misunderstanding the manual?\n\n-- \nTom Hale\n"},{"id":"303656","messageId":"f35965e9-2901-b9b5-92e5-9bc7fe673637@web.de","threadId":"44256","inReplyTo":"cb81631e-9623-9020-f955-ec215b493a50@hale.ee","subject":"Re: %C(auto) not working as expected","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-09T06:47:25Z","receivedAt":"2016-10-09T06:48:30Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.10.2016 um 07:43 schrieb Tom Hale:\n> $ ~/repo/git/git --version\n> git version 2.10.0.GIT\n>\n> The `git-log` man page says:\n>\n> `auto` alone (i.e. %C(auto)) will turn on auto coloring on the next\n> placeholders until the color is switched again.\n>\n> In this example:\n>\n> http://i.imgur.com/y3yLxk7.png\n>\n> I turn on auto colouring for green, but it seems that this is not being\n> respected when piped through `cat`.\n>\n> Am I misunderstanding the manual?\n\nSo this colors the ref name decoration and the short hash:\n\n\t$ git log -n1 --format=\"%C(auto)%d %C(auto)%Cgreen%h\"\n\nAnd this only colors the short hash:\n\n\t$ git log -n1 --format=\"%C(auto)%d %C(auto)%Cgreen%h\" | cat\n\n%C(auto) respects the color-related configuration settings; that's \nmentioned on the man page for git log in the section on %C(...). You \nprobably have color.ui=auto or color.diff=auto in your config, which \nmeans that output to terminals is to be colored, but output to files and \npipes isn't. You could override that setting e.g. by adding the command \nline option --color=always.\n\n%Cgreen emits color codes unconditionally. %C(auto,green) would respect \nthe config settings.\n\nYour second %C(auto) has no effect because it is overridden by the \nimmediately following %Cgreen.\n\nYou may want to add a %Creset at the end to avoid colors bleeding out \nover the end of the line. You can see that happening e.g. with:\n\n\t$ git log -n3 --format=\"normal %C(green)green\" | cat\n\nWithout cat bash seems to add a reset automatically.\n\nRené\n\n"},{"id":"303664","messageId":"65d8def3-df62-6c45-7d8f-79b6a8769bf5@hale.ee","threadId":"44256","inReplyTo":"f35965e9-2901-b9b5-92e5-9bc7fe673637@web.de","subject":"Re: %C(auto) not working as expected","fromName":"Tom Hale","fromEmail":"tom@hale.ee","sentAt":"2016-10-09T10:04:22Z","receivedAt":"2016-10-09T10:04:29Z","isPatch":false,"sender":{"key":"tom@hale.ee","avatar":"https://gravatar.com/avatar/61d620fad9c6227a6f41dee4cdb68be18a7d8bf8c69c3fd70d6b3c65eb65eea8?d=mp&s=160"},"body":"On 2016-10-09 13:47, René Scharfe wrote:\n\n> %Cgreen emits color codes unconditionally. %C(auto,green) would respect\n> the config settings.\n\nThanks, I've never seen the (,) syntax documented before!\n\nWhat's strange is that this works:\n %C(auto,green bold)\nbut\n %C(auto,green,bold)\ndoes not.\n\nAlso:\nGiven it's very rare to want only part of a string to emit colour codes, \nif something like \"bold\" carries through until a \"no-bold\", why doesn't \n\"auto\" do the same thing?\n\n-- \nTom Hale\n"},{"id":"303676","messageId":"25c17e16-2456-7da3-ae22-2dc812a3aa0d@web.de","threadId":"44256","inReplyTo":"65d8def3-df62-6c45-7d8f-79b6a8769bf5@hale.ee","subject":"Re: %C(auto) not working as expected","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-09T13:24:17Z","receivedAt":"2016-10-09T13:24:51Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.10.2016 um 12:04 schrieb Tom Hale:\n> On 2016-10-09 13:47, René Scharfe wrote:\n> \n>> %Cgreen emits color codes unconditionally. %C(auto,green) would respect\n>> the config settings.\n> \n> Thanks, I've never seen the (,) syntax documented before!\n\nBoth the prefix \"auto,\" for terminal-detection and \"%C(auto)\" for\nchoosing colors automatically are mentioned in the manpage for git log\n(from Documentation/pretty-formats.txt):\n\n- '%C(...)': color specification, as described in color.branch.* config option;\n adding `auto,` at the beginning will emit color only when colors are\n enabled for log output (by `color.diff`, `color.ui`, or `--color`, and\n respecting the `auto` settings of the former if we are going to a\n terminal). `auto` alone (i.e. `%C(auto)`) will turn on auto coloring\n on the next placeholders until the color is switched again.\n\n> What's strange is that this works:\n> %C(auto,green bold)\n> but\n> %C(auto,green,bold)\n> does not.\n\nLooking at the code that's not surprising; colors and attributes are\ninterpreted as a space-separated list. The prefix \"auto,\" is handled\nspecially. For a user it may look strange, admittedly.\n\nSupporting \"auto \" as well would be easy. Supporting it in such a way\nthat it can be mixed freely with colors and attributes as in\n%C(bold auto green) would be a bit harder. Could this lead to confusion\nbetween the auto for terminal-detection and the one for automatic color\nselection?\n\nThe documentation cited above says the color specification was explained\ntogether with the color.branch.* config option, but that part only says\n(from Documentation/config.txt):\n\ncolor.branch.::\n Use customized color for branch coloration. `` is one of\n `current` (the current branch), `local` (a local branch),\n `remote` (a remote-tracking branch in refs/remotes/),\n `upstream` (upstream tracking branch), `plain` (other\n refs).\n\nIt really is described earlier in the same file, in the Values section\n(a fitting place, I think). Here's just the first sentence:\n\ncolor::\n The value for a variable that takes a color is a list of\n colors (at most two, one for foreground and one for background)\n and attributes (as many as you want), separated by spaces.\n\nPatch below. Does it help a little?\n\n> Also:\n> Given it's very rare to want only part of a string to emit colour codes,\n> if something like \"bold\" carries through until a \"no-bold\", why doesn't\n> \"auto\" do the same thing?\n\nNo state is kept for \"auto,\". Attributes and colors are switched\nseparately by terminals, that's why e.g. bold stays in effect through\na color change -- unless you specify an attribute change as well.\n\nOffering a way to enable terminal-detection for all color codes of a\nformat would be useful, but using the existing \"auto,\" prefix for that\nwould be a behaviour change that could surprise users.\n\nRené\n\n\n-- >8 --\nSubject: [PATCH] pretty: fix document reference for color specification\n\nSigned-off-by: Rene Scharfe \n---\n Documentation/pretty-formats.txt | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex a942d57..89e3bc6 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -166,7 +166,8 @@ endif::git-rev-list[]\n - '%Cgreen': switch color to green\n - '%Cblue': switch color to blue\n - '%Creset': reset color\n-- '%C(...)': color specification, as described in color.branch.* config option;\n+- '%C(...)': color specification, as described under Values, color in the\n+ \"CONFIGURATION FILE\" section of linkgit:git-config[1];\n adding `auto,` at the beginning will emit color only when colors are\n enabled for log output (by `color.diff`, `color.ui`, or `--color`, and\n respecting the `auto` settings of the former if we are going to a\n-- \n2.10.1\n\n"},{"id":"303685","messageId":"20161009234617.y6xfjyv6xjkf2afi@sigill.intra.peff.net","threadId":"44256","inReplyTo":"25c17e16-2456-7da3-ae22-2dc812a3aa0d@web.de","subject":"Re: %C(auto) not working as expected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-09T23:46:18Z","receivedAt":"2016-10-09T23:46:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 09, 2016 at 03:24:17PM +0200, René Scharfe wrote:\n\n> Offering a way to enable terminal-detection for all color codes of a\n> format would be useful, but using the existing \"auto,\" prefix for that\n> would be a behaviour change that could surprise users.\n\nYeah. In retrospect, it probably would have been saner to make %C(red) a\nnoop when --color is not in effect (either because of --no-color, or\nmore likely when --color=auto is in effect and stdout is not a\nterminal). But that ship has long since sailed, I think.\n\nIf we do a revamp of the pretty-formats to bring them more in line with\nref-filter (e.g., something like \"%(color:red)\") maybe that would be an\nopportunity to make minor adjustments. Though, hmm, it looks like\nfor-each-ref already knows \"%(color:red)\", and it's unconditional.\n So perhaps we would need to go through some deprecation period or\nother transition.\n\n> ---\n> Documentation/pretty-formats.txt | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index a942d57..89e3bc6 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -166,7 +166,8 @@ endif::git-rev-list[]\n> - '%Cgreen': switch color to green\n> - '%Cblue': switch color to blue\n> - '%Creset': reset color\n> -- '%C(...)': color specification, as described in color.branch.* config option;\n> +- '%C(...)': color specification, as described under Values, color in the\n> + \"CONFIGURATION FILE\" section of linkgit:git-config[1];\n\nI like the intent here, though I found \"Values, color\" hard to parse (it\nwas not immediately clear that you mean \"the color paragraph of the\nValues section\", as commas are already being used in that sentence for\nthe parenthetical phrase).\n\nI'm not sure how to say that succinctly, as we are four levels deep\n(git-config -> configuration file -> values -> color). Too bad there is\nno good link syntax for it. Maybe:\n\n ...color specification, as described in linkgit:git-config[1] (see the\n paragraph on colors in the \"Values\" section, under \"CONFIGURATION\n FILE\")\n\nor something.\n\n-Peff\n"},{"id":"303699","messageId":"CACsJy8CroyynVMctbPhuVr2VVQB7YyfcxDaMT25BikQ4R4We0Q@mail.gmail.com","threadId":"44256","inReplyTo":"20161009234617.y6xfjyv6xjkf2afi@sigill.intra.peff.net","subject":"Re: %C(auto) not working as expected","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-10T09:26:18Z","receivedAt":"2016-10-10T09:27:34Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 10, 2016 at 6:46 AM, Jeff King wrote:\n> On Sun, Oct 09, 2016 at 03:24:17PM +0200, René Scharfe wrote:\n>\n>> Offering a way to enable terminal-detection for all color codes of a\n>> format would be useful, but using the existing \"auto,\" prefix for that\n>> would be a behaviour change that could surprise users.\n\nI wonder if we made a mistake associating terminal-detection with\n%C(auto,...). The more likely use case is enable or disable all\ncolors, not \"the next tag\".\n\n> Yeah. In retrospect, it probably would have been saner to make %C(red) a\n> noop when --color is not in effect (either because of --no-color, or\n> more likely when --color=auto is in effect and stdout is not a\n> terminal). But that ship has long since sailed, I think.\n>\n> If we do a revamp of the pretty-formats to bring them more in line with\n> ref-filter (e.g., something like \"%(color:red)\") maybe that would be an\n> opportunity to make minor adjustments. Though, hmm, it looks like\n> for-each-ref already knows \"%(color:red)\", and it's unconditional.\n> So perhaps we would need to go through some deprecation period or\n> other transition.\n\nWe could add some new tag to change the behavior of all following %C\ntags. Something like %C(tty) maybe (probably a bad name), then\ndiscourage the use if \"%C(auto\" for terminal detection?\n-- \nDuy\n"},{"id":"303723","messageId":"20161010142818.lglwrxpks6l6aqrm@sigill.intra.peff.net","threadId":"44256","inReplyTo":"CACsJy8CroyynVMctbPhuVr2VVQB7YyfcxDaMT25BikQ4R4We0Q@mail.gmail.com","subject":"Re: %C(auto) not working as expected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-10T14:28:18Z","receivedAt":"2016-10-10T14:28:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2016 at 04:26:18PM +0700, Duy Nguyen wrote:\n\n> > If we do a revamp of the pretty-formats to bring them more in line with\n> > ref-filter (e.g., something like \"%(color:red)\") maybe that would be an\n> > opportunity to make minor adjustments. Though, hmm, it looks like\n> > for-each-ref already knows \"%(color:red)\", and it's unconditional.\n> > So perhaps we would need to go through some deprecation period or\n> > other transition.\n> \n> We could add some new tag to change the behavior of all following %C\n> tags. Something like %C(tty) maybe (probably a bad name), then\n> discourage the use if \"%C(auto\" for terminal detection?\n\nYeah, adding a \"%C(enable-auto-color)\" or something would be backwards\ncompatible and less painful than using \"%C(auto)\" everywhere. I do\nwonder if anybody actually _wants_ the \"always show color, even if\n--no-color\" behavior. I'm having trouble thinking of a good use for it.\n\nIOW, I'm wondering if anyone would disagree that the current behavior is\nsimply buggy. Reading the thread at:\n\n http://public-inbox.org/git/7v4njkmq07.fsf@alter.siamese.dyndns.org/\n\nI don't really see any compelling reason against it (there was some\nquestion of which config to use, but we already answered that with\n\"%C(auto)\", and use the value from the pretty_ctx).\n\n-Peff\n"},{"id":"303725","messageId":"20161010151517.6wszhuyp57yfncaj@sigill.intra.peff.net","threadId":"44256","inReplyTo":"20161010142818.lglwrxpks6l6aqrm@sigill.intra.peff.net","subject":"[PATCH] pretty: respect color settings for %C placeholders","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-10T15:15:17Z","receivedAt":"2016-10-10T15:16:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2016 at 10:28:18AM -0400, Jeff King wrote:\n\n> > We could add some new tag to change the behavior of all following %C\n> > tags. Something like %C(tty) maybe (probably a bad name), then\n> > discourage the use if \"%C(auto\" for terminal detection?\n> \n> Yeah, adding a \"%C(enable-auto-color)\" or something would be backwards\n> compatible and less painful than using \"%C(auto)\" everywhere. I do\n> wonder if anybody actually _wants_ the \"always show color, even if\n> --no-color\" behavior. I'm having trouble thinking of a good use for it.\n> \n> IOW, I'm wondering if anyone would disagree that the current behavior is\n> simply buggy. Reading the thread at:\n> \n> http://public-inbox.org/git/7v4njkmq07.fsf@alter.siamese.dyndns.org/\n> \n> I don't really see any compelling reason against it (there was some\n> question of which config to use, but we already answered that with\n> \"%C(auto)\", and use the value from the pretty_ctx).\n\nSo here's what a patch to do that would look like. I admit that \"I can't\nthink of a good use\" does not mean there _isn't_ one, but perhaps by\nposting this, it might provoke other people to think on it, too. And if\nnobody can come up with, maybe it's a good idea.\n\n-- >8 --\nSubject: pretty: respect color settings for %C placeholders\n\nThe color placeholders have traditionally been\nunconditional, showing colors even when git is not otherwise\nconfigured to do so. This was not so bad for their original\nuse, which was on the command-line (and the user could\ndecide at that moment whether to add colors or not). But\nthese days we have configured formats via pretty.*, and\nthose should operate in multiple contexts.\n\nIn 3082517 (log --format: teach %C(auto,black) to respect\ncolor config, 2012-12-17), we gave an extended placeholder\nthat could be used to accomplish this. But it's rather\nclunky to use, because you have to specify it individually\nfor each color (and their matching resets) in the format.\nWe shied away from just switching the default to auto,\nbecause it is technically breaking backwards compatibility.\n\nHowever, there's not really a use case for unconditional\ncolors. The most plausible reason you would want them\nunconditional is to redirect \"git log\" output to a file. But\nthere, the right answer is --color=always, as it does the\nright thing both with custom user-format colors and\ngit-generated colors.\n\nSo let's switch to the more useful default. In the\noff-chance that somebody really does find a use for\nunconditional colors without wanting to enable the rest of\ngit's colors, we can provide %C(always,...) to enable the\nold behavior.\n\nSigned-off-by: Jeff King \n---\nThe tests unsurprisingly needed updating, as we're breaking the old\nbehavior. The diff is easier to read read with \"-w\".\n\n Documentation/pretty-formats.txt | 13 +++---\n pretty.c | 19 +++++---\n t/t6006-rev-list-format.sh | 94 ++++++++++++++++++++--------------------\n 3 files changed, 70 insertions(+), 56 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex a942d57..7aa1a8b 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -167,11 +167,14 @@ endif::git-rev-list[]\n - '%Cblue': switch color to blue\n - '%Creset': reset color\n - '%C(...)': color specification, as described in color.branch.* config option;\n- adding `auto,` at the beginning will emit color only when colors are\n- enabled for log output (by `color.diff`, `color.ui`, or `--color`, and\n- respecting the `auto` settings of the former if we are going to a\n- terminal). `auto` alone (i.e. `%C(auto)`) will turn on auto coloring\n- on the next placeholders until the color is switched again.\n+ By default, colors are shown only when enabled for log output (by\n+ `color.diff`, `color.ui`, or `--color`, and respecting the `auto`\n+ settings of the former if we are going to a terminal). `%C(auto,...)`\n+ is accepted as a historical synonym for the default. Specifying\n+ `%C(always,...) will show the colors always, even when colors are not\n+ otherwise enabled (to enable this behavior for the whole format, use\n+ `--color=always`). `auto` alone (i.e. `%C(auto)`) will turn on auto\n+ coloring on the next placeholders until the color is switched again.\n - '%m': left (`<`), right (`>`) or boundary (`-`) mark\n - '%n': newline\n - '%%': a raw '%'\ndiff --git a/pretty.c b/pretty.c\nindex 25efbca..73e58b5 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n \n \t\tif (!end)\n \t\t\treturn 0;\n-\t\tif (skip_prefix(begin, \"auto,\", &begin)) {\n+\n+\t\tif (!skip_prefix(begin, \"always,\", &begin)) {\n \t\t\tif (!want_color(c->pretty_ctx->color))\n \t\t\t\treturn end - placeholder + 1;\n \t\t}\n+\n+\t\t/* this is a historical noop */\n+\t\tskip_prefix(begin, \"auto,\", &begin);\n+\n \t\tif (color_parse_mem(begin, end - begin, color) < 0)\n \t\t\tdie(_(\"unable to parse --pretty format\"));\n \t\tstrbuf_addstr(sb, color);\n \t\treturn end - placeholder + 1;\n \t}\n-\tif (skip_prefix(placeholder + 1, \"red\", &rest))\n+\tif (skip_prefix(placeholder + 1, \"red\", &rest) &&\n+\t want_color(c->pretty_ctx->color))\n \t\tstrbuf_addstr(sb, GIT_COLOR_RED);\n-\telse if (skip_prefix(placeholder + 1, \"green\", &rest))\n+\telse if (skip_prefix(placeholder + 1, \"green\", &rest) &&\n+\t\t want_color(c->pretty_ctx->color))\n \t\tstrbuf_addstr(sb, GIT_COLOR_GREEN);\n-\telse if (skip_prefix(placeholder + 1, \"blue\", &rest))\n+\telse if (skip_prefix(placeholder + 1, \"blue\", &rest) &&\n+\t\t want_color(c->pretty_ctx->color))\n \t\tstrbuf_addstr(sb, GIT_COLOR_BLUE);\n-\telse if (skip_prefix(placeholder + 1, \"reset\", &rest))\n+\telse if (skip_prefix(placeholder + 1, \"reset\", &rest) &&\n+\t\t want_color(c->pretty_ctx->color))\n \t\tstrbuf_addstr(sb, GIT_COLOR_RESET);\n \treturn rest - placeholder;\n }\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex a1dcdb8..29b1a1a 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -59,7 +59,10 @@ test_format () {\n }\n \n # Feed to --format to provide predictable colored sequences.\n+BASIC_COLOR='%Credfoo%Creset'\n+COLOR='%C(red)foo%C(reset)'\n AUTO_COLOR='%C(auto,red)foo%C(auto,reset)'\n+ALWAYS_COLOR='%C(always,red)foo%C(always,reset)'\n has_color () {\n \tprintf '\\033[31mfoo\\033[m\\n' >expect &&\n \ttest_cmp expect \"$1\"\n@@ -170,57 +173,56 @@ $added\n \n EOF\n \n-test_format colors %Credfoo%Cgreenbar%Cbluebaz%Cresetxyzzy <actual &&\n-\thas_no_color actual\n-'\n-\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+for spec in \\\n+\t\"%Cred:$BASIC_COLOR\" \\\n+\t\"%C(...):$COLOR\" \\\n+\t\"%C(auto,...):$AUTO_COLOR\"\n+do\n+\tdesc=${spec%%:*}\n+\tcolor=${spec#*:}\n+\ttest_expect_success \"$desc does not enable color by default\" '\n+\t\tgit log --format=$color -1 >actual &&\n+\t\thas_no_color actual\n+\t'\n \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+\ttest_expect_success \"$desc enables colors for color.diff\" '\n+\t\tgit -c color.diff=always log --format=$color -1 >actual &&\n+\t\thas_color actual\n+\t'\n \n-test_expect_success '%C(auto,...) respects --color' '\n-\tgit log --format=$AUTO_COLOR -1 --color >actual &&\n-\thas_color actual\n-'\n+\ttest_expect_success \"$desc enables colors for color.ui\" '\n+\t\tgit -c color.ui=always log --format=$color -1 >actual &&\n+\t\thas_color actual\n+\t'\n \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+\ttest_expect_success \"$desc respects --color\" '\n+\t\tgit log --format=$color -1 --color >actual &&\n+\t\thas_color actual\n+\t'\n \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-\t(\n-\t\tTERM=vt100 && export TERM &&\n-\t\tgit log --format=$AUTO_COLOR -1 --color=auto >actual &&\n+\ttest_expect_success \"$desc respects --no-color\" '\n+\t\tgit -c color.ui=always log --format=$color -1 --no-color >actual &&\n \t\thas_no_color actual\n-\t)\n+\t'\n+\n+\ttest_expect_success TTY \"$desc respects --color=auto (stdout is tty)\" '\n+\t\ttest_terminal env TERM=vt100 \\\n+\t\t\tgit log --format=$color -1 --color=auto >actual &&\n+\t\thas_color actual\n+\t'\n+\n+\ttest_expect_success \"$desc respects --color=auto (stdout not tty)\" '\n+\t\t(\n+\t\t\tTERM=vt100 && export TERM &&\n+\t\t\tgit log --format=$color -1 --color=auto >actual &&\n+\t\t\thas_no_color actual\n+\t\t)\n+\t'\n+done\n+\n+test_expect_success '%C(always,...) enables color even without tty' '\n+\tgit log --format=$ALWAYS_COLOR -1 >actual &&\n+\thas_color actual\n '\n \n test_expect_success '%C(auto) respects --color' '\n-- \n2.10.1.527.g93d4615\n\n"},{"id":"303737","messageId":"42b3be14-fadb-4801-f377-59fb42b7ccdf@web.de","threadId":"44256","inReplyTo":"20161010151517.6wszhuyp57yfncaj@sigill.intra.peff.net","subject":"Re: [PATCH] pretty: respect color settings for %C placeholders","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-10T17:09:12Z","receivedAt":"2016-10-10T17:09:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.10.2016 um 17:15 schrieb Jeff King:\n> On Mon, Oct 10, 2016 at 10:28:18AM -0400, Jeff King wrote:\n>\n>>> We could add some new tag to change the behavior of all following %C\n>>> tags. Something like %C(tty) maybe (probably a bad name), then\n>>> discourage the use if \"%C(auto\" for terminal detection?\n>>\n>> Yeah, adding a \"%C(enable-auto-color)\" or something would be backwards\n>> compatible and less painful than using \"%C(auto)\" everywhere. I do\n>> wonder if anybody actually _wants_ the \"always show color, even if\n>> --no-color\" behavior. I'm having trouble thinking of a good use for it.\n>>\n>> IOW, I'm wondering if anyone would disagree that the current behavior is\n>> simply buggy. Reading the thread at:\n>>\n>> http://public-inbox.org/git/7v4njkmq07.fsf@alter.siamese.dyndns.org/\n>>\n>> I don't really see any compelling reason against it (there was some\n>> question of which config to use, but we already answered that with\n>> \"%C(auto)\", and use the value from the pretty_ctx).\n>\n> So here's what a patch to do that would look like. I admit that \"I can't\n> think of a good use\" does not mean there _isn't_ one, but perhaps by\n> posting this, it might provoke other people to think on it, too. And if\n> nobody can come up with, maybe it's a good idea.\n\nColor tags that respect the config and the --color option make the most \nsense to me as well.\n\nNevertheless a possible counterpoint: Coloring is used in commands that \nare intended for human consumption. Most of the time there is no need \nto to conserve the output in a file. But even then, and of course with \npipes, once you look at it using less -R or grep you still get nice \ncolored lines and not a monochrome wall of text.\n\n> -- >8 --\n> Subject: pretty: respect color settings for %C placeholders\n>\n> The color placeholders have traditionally been\n> unconditional, showing colors even when git is not otherwise\n> configured to do so. This was not so bad for their original\n> use, which was on the command-line (and the user could\n> decide at that moment whether to add colors or not). But\n> these days we have configured formats via pretty.*, and\n> those should operate in multiple contexts.\n>\n> In 3082517 (log --format: teach %C(auto,black) to respect\n> color config, 2012-12-17), we gave an extended placeholder\n> that could be used to accomplish this. But it's rather\n> clunky to use, because you have to specify it individually\n> for each color (and their matching resets) in the format.\n> We shied away from just switching the default to auto,\n> because it is technically breaking backwards compatibility.\n>\n> However, there's not really a use case for unconditional\n> colors. The most plausible reason you would want them\n> unconditional is to redirect \"git log\" output to a file. But\n> there, the right answer is --color=always, as it does the\n> right thing both with custom user-format colors and\n> git-generated colors.\n>\n> So let's switch to the more useful default. In the\n> off-chance that somebody really does find a use for\n> unconditional colors without wanting to enable the rest of\n> git's colors, we can provide %C(always,...) to enable the\n> old behavior.\n>\n> Signed-off-by: Jeff King \n> ---\n> The tests unsurprisingly needed updating, as we're breaking the old\n> behavior. The diff is easier to read read with \"-w\".\n>\n> Documentation/pretty-formats.txt | 13 +++---\n> pretty.c | 19 +++++---\n> t/t6006-rev-list-format.sh | 94 ++++++++++++++++++++--------------------\n> 3 files changed, 70 insertions(+), 56 deletions(-)\n>\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index a942d57..7aa1a8b 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -167,11 +167,14 @@ endif::git-rev-list[]\n> - '%Cblue': switch color to blue\n> - '%Creset': reset color\n> - '%C(...)': color specification, as described in color.branch.* config option;\n> - adding `auto,` at the beginning will emit color only when colors are\n> - enabled for log output (by `color.diff`, `color.ui`, or `--color`, and\n> - respecting the `auto` settings of the former if we are going to a\n> - terminal). `auto` alone (i.e. `%C(auto)`) will turn on auto coloring\n> - on the next placeholders until the color is switched again.\n> + By default, colors are shown only when enabled for log output (by\n> + `color.diff`, `color.ui`, or `--color`, and respecting the `auto`\n> + settings of the former if we are going to a terminal). `%C(auto,...)`\n> + is accepted as a historical synonym for the default. Specifying\n> + `%C(always,...) will show the colors always, even when colors are not\n> + otherwise enabled (to enable this behavior for the whole format, use\n> + `--color=always`). `auto` alone (i.e. `%C(auto)`) will turn on auto\n> + coloring on the next placeholders until the color is switched again.\n> - '%m': left (`<`), right (`>`) or boundary (`-`) mark\n> - '%n': newline\n> - '%%': a raw '%'\n> diff --git a/pretty.c b/pretty.c\n> index 25efbca..73e58b5 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n>\n> \t\tif (!end)\n> \t\t\treturn 0;\n> -\t\tif (skip_prefix(begin, \"auto,\", &begin)) {\n> +\n> +\t\tif (!skip_prefix(begin, \"always,\", &begin)) {\n> \t\t\tif (!want_color(c->pretty_ctx->color))\n> \t\t\t\treturn end - placeholder + 1;\n> \t\t}\n\nShouldn't we have an \"else\" here?\n\n> +\n> +\t\t/* this is a historical noop */\n> +\t\tskip_prefix(begin, \"auto,\", &begin);\n> +\n> \t\tif (color_parse_mem(begin, end - begin, color) < 0)\n> \t\t\tdie(_(\"unable to parse --pretty format\"));\n> \t\tstrbuf_addstr(sb, color);\n> \t\treturn end - placeholder + 1;\n> \t}\n> -\tif (skip_prefix(placeholder + 1, \"red\", &rest))\n> +\tif (skip_prefix(placeholder + 1, \"red\", &rest) &&\n> +\t want_color(c->pretty_ctx->color))\n> \t\tstrbuf_addstr(sb, GIT_COLOR_RED);\n> -\telse if (skip_prefix(placeholder + 1, \"green\", &rest))\n> +\telse if (skip_prefix(placeholder + 1, \"green\", &rest) &&\n> +\t\t want_color(c->pretty_ctx->color))\n> \t\tstrbuf_addstr(sb, GIT_COLOR_GREEN);\n> -\telse if (skip_prefix(placeholder + 1, \"blue\", &rest))\n> +\telse if (skip_prefix(placeholder + 1, \"blue\", &rest) &&\n> +\t\t want_color(c->pretty_ctx->color))\n> \t\tstrbuf_addstr(sb, GIT_COLOR_BLUE);\n> -\telse if (skip_prefix(placeholder + 1, \"reset\", &rest))\n> +\telse if (skip_prefix(placeholder + 1, \"reset\", &rest) &&\n> +\t\t want_color(c->pretty_ctx->color))\n> \t\tstrbuf_addstr(sb, GIT_COLOR_RESET);\n> \treturn rest - placeholder;\n> }\n\nPerhaps it's a funtion like add_color(sb, ctx, color) or similar would \nbe nice?\n\nRené\n"},{"id":"303765","messageId":"20161010174257.b4uxplavefjyr6rl@sigill.intra.peff.net","threadId":"44256","inReplyTo":"42b3be14-fadb-4801-f377-59fb42b7ccdf@web.de","subject":"Re: [PATCH] pretty: respect color settings for %C placeholders","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-10T17:42:57Z","receivedAt":"2016-10-10T17:43:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2016 at 07:09:12PM +0200, René Scharfe wrote:\n\n> > diff --git a/pretty.c b/pretty.c\n> > index 25efbca..73e58b5 100644\n> > --- a/pretty.c\n> > +++ b/pretty.c\n> > @@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n> > \n> > \t\tif (!end)\n> > \t\t\treturn 0;\n> > -\t\tif (skip_prefix(begin, \"auto,\", &begin)) {\n> > +\n> > +\t\tif (!skip_prefix(begin, \"always,\", &begin)) {\n> > \t\t\tif (!want_color(c->pretty_ctx->color))\n> > \t\t\t\treturn end - placeholder + 1;\n> > \t\t}\n> \n> Shouldn't we have an \"else\" here?\n\nI'm not sure what you mean; can you write it out?\n\n> > -\tif (skip_prefix(placeholder + 1, \"red\", &rest))\n> > +\tif (skip_prefix(placeholder + 1, \"red\", &rest) &&\n> > +\t want_color(c->pretty_ctx->color))\n> > \t\tstrbuf_addstr(sb, GIT_COLOR_RED);\n> > -\telse if (skip_prefix(placeholder + 1, \"green\", &rest))\n> > +\telse if (skip_prefix(placeholder + 1, \"green\", &rest) &&\n> > +\t\t want_color(c->pretty_ctx->color))\n> > \t\tstrbuf_addstr(sb, GIT_COLOR_GREEN);\n> > -\telse if (skip_prefix(placeholder + 1, \"blue\", &rest))\n> > +\telse if (skip_prefix(placeholder + 1, \"blue\", &rest) &&\n> > +\t\t want_color(c->pretty_ctx->color))\n> > \t\tstrbuf_addstr(sb, GIT_COLOR_BLUE);\n> > -\telse if (skip_prefix(placeholder + 1, \"reset\", &rest))\n> > +\telse if (skip_prefix(placeholder + 1, \"reset\", &rest) &&\n> > +\t\t want_color(c->pretty_ctx->color))\n> > \t\tstrbuf_addstr(sb, GIT_COLOR_RESET);\n> > \treturn rest - placeholder;\n> > }\n> \n> Perhaps it's a funtion like add_color(sb, ctx, color) or similar would be\n> nice?\n\nI actually wrote it that way first (I called it \"maybe_add_color()\"),\nbut it felt a little funny to have a separate function that people might\nbe tempted to reuse (the right solution is generally to check\nwant_color() early as above, but we can't do that here because we have\nto find the end of each placeholder).\n\nWhat I have here is a little funny, too, though, as it keeps trying\nother color names if it finds \"red\" but want_color() returns 0.\n\n-Peff\n"},{"id":"303784","messageId":"xmqq1szof34x.fsf@gitster.mtv.corp.google.com","threadId":"44256","inReplyTo":"20161010151517.6wszhuyp57yfncaj@sigill.intra.peff.net","subject":"Re: [PATCH] pretty: respect color settings for %C placeholders","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-10T18:52:14Z","receivedAt":"2016-10-10T18:52:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So here's what a patch to do that would look like. I admit that \"I can't\n> think of a good use\" does not mean there _isn't_ one, but perhaps by\n> posting this, it might provoke other people to think on it, too. And if\n> nobody can come up with, maybe it's a good idea.\n\nI do not have a fundamental opposition to this approach myself,\nmodulo a minor nit.\n\n> + By default, colors are shown only when enabled for log output (by\n> + `color.diff`, `color.ui`, or `--color`, and respecting the `auto`\n> + settings of the former if we are going to a terminal). `%C(auto,...)`\n> + is accepted as a historical synonym for the default. Specifying\n> + `%C(always,...) will show the colors always, even when colors are not\n> + otherwise enabled (to enable this behavior for the whole format, use\n> + `--color=always`).\n\nIt is not just \"for the whole format\", but also affects other parts\nof the output, no? I am thinking about \"git log -p --format=...\".\n\n> diff --git a/pretty.c b/pretty.c\n> index 25efbca..73e58b5 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n> \n> \t\tif (!end)\n> \t\t\treturn 0;\n> -\t\tif (skip_prefix(begin, \"auto,\", &begin)) {\n> +\n> +\t\tif (!skip_prefix(begin, \"always,\", &begin)) {\n> \t\t\tif (!want_color(c->pretty_ctx->color))\n> \t\t\t\treturn end - placeholder + 1;\n> \t\t}\n\nAs a way to say \"when color is not enabled, ignore everything unless\nit begins with 'always,'\", this was a bit hard to read. Perhaps an\nin-code comment is in order?\n\n> +\n> +\t\t/* this is a historical noop */\n> +\t\tskip_prefix(begin, \"auto,\", &begin);\n> +\n> \t\tif (color_parse_mem(begin, end - begin, color) < 0)\n> \t\t\tdie(_(\"unable to parse --pretty format\"));\n> \t\tstrbuf_addstr(sb, color);\n> \t\treturn end - placeholder + 1;\n> \t}\n> -\tif (skip_prefix(placeholder + 1, \"red\", &rest))\n> +\tif (skip_prefix(placeholder + 1, \"red\", &rest) &&\n> +\t want_color(c->pretty_ctx->color))\n> \t\tstrbuf_addstr(sb, GIT_COLOR_RED);\n\nHmm. If we are in \"no I do not want color\" mode and \"always,red\"\nwas given, we countermanded !want_color() check up above and come\nhere. Then we check want_color() again and refuse to paint it red?\n\nI must be reading the patch incorrectly, but I cannot quite tell\nwhere I want astray...\n"},{"id":"303786","messageId":"20161010185935.r3wnfglpi7hjjpdk@sigill.intra.peff.net","threadId":"44256","inReplyTo":"xmqq1szof34x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pretty: respect color settings for %C placeholders","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-10T18:59:36Z","receivedAt":"2016-10-10T19:06:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2016 at 11:52:14AM -0700, Junio C Hamano wrote:\n\n> > + By default, colors are shown only when enabled for log output (by\n> > + `color.diff`, `color.ui`, or `--color`, and respecting the `auto`\n> > + settings of the former if we are going to a terminal). `%C(auto,...)`\n> > + is accepted as a historical synonym for the default. Specifying\n> > + `%C(always,...) will show the colors always, even when colors are not\n> > + otherwise enabled (to enable this behavior for the whole format, use\n> > + `--color=always`).\n> \n> It is not just \"for the whole format\", but also affects other parts\n> of the output, no? I am thinking about \"git log -p --format=...\".\n\nTrue. I consider that a feature and more likely to be pointing the user\nin the right direction, but it should probably be s/format/output/ in\nthe last sentence.\n\n> > diff --git a/pretty.c b/pretty.c\n> > index 25efbca..73e58b5 100644\n> > --- a/pretty.c\n> > +++ b/pretty.c\n> > @@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n> > \n> > \t\tif (!end)\n> > \t\t\treturn 0;\n> > -\t\tif (skip_prefix(begin, \"auto,\", &begin)) {\n> > +\n> > +\t\tif (!skip_prefix(begin, \"always,\", &begin)) {\n> > \t\t\tif (!want_color(c->pretty_ctx->color))\n> > \t\t\t\treturn end - placeholder + 1;\n> > \t\t}\n> \n> As a way to say \"when color is not enabled, ignore everything unless\n> it begins with 'always,'\", this was a bit hard to read. Perhaps an\n> in-code comment is in order?\n\nI'll see what I can do. The most readable one is probably:\n\n if (skip_prefix(begin, \"auto,\", &begin)) {\n\tif (!want_color(c->pretty_ctx->color))\n\t\treturn end - placeholder + 1;\n } else if (skip_prefix(begin, \"always,\", &begin)) {\n\t/* nothing to do; we do not respect want_color at all */\n } else {\n\t/* the default is now the same as \"auto\" */\n\tif (!want_color(c->pretty_ctx->color))\n\t\treturn end - placeholder + 1;\n }\n\nI avoided that because of the repetition, but it probably is not too\nbad.\n\n> > -\tif (skip_prefix(placeholder + 1, \"red\", &rest))\n> > +\tif (skip_prefix(placeholder + 1, \"red\", &rest) &&\n> > +\t want_color(c->pretty_ctx->color))\n> > \t\tstrbuf_addstr(sb, GIT_COLOR_RED);\n> \n> Hmm. If we are in \"no I do not want color\" mode and \"always,red\"\n> was given, we countermanded !want_color() check up above and come\n> here. Then we check want_color() again and refuse to paint it red?\n> \n> I must be reading the patch incorrectly, but I cannot quite tell\n> where I want astray...\n\nNo, we don't come here from %C() at all. This is for bare \"%Cred\", which\ncannot have \"always\" (or \"auto\"), as there is no syntactic spot for it.\nIt is mostly historical (it _only_ supports red, green, and blue). We\ncould actually leave this as-is to show the colors unconditionally. I\nchanged it to keep the new behavior consistent, but I doubt anybody\ncares much either way.\n\n-Peff\n"},{"id":"303796","messageId":"19e59db7-f3dd-35ec-8cf1-b070b1c05abe@web.de","threadId":"44256","inReplyTo":"20161010174257.b4uxplavefjyr6rl@sigill.intra.peff.net","subject":"Re: [PATCH] pretty: respect color settings for %C placeholders","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-10T19:59:17Z","receivedAt":"2016-10-10T19:59:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.10.2016 um 19:42 schrieb Jeff King:\n> On Mon, Oct 10, 2016 at 07:09:12PM +0200, René Scharfe wrote:\n>\n>>> diff --git a/pretty.c b/pretty.c\n>>> index 25efbca..73e58b5 100644\n>>> --- a/pretty.c\n>>> +++ b/pretty.c\n>>> @@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n>>>\n>>> \t\tif (!end)\n>>> \t\t\treturn 0;\n>>> -\t\tif (skip_prefix(begin, \"auto,\", &begin)) {\n>>> +\n>>> +\t\tif (!skip_prefix(begin, \"always,\", &begin)) {\n>>> \t\t\tif (!want_color(c->pretty_ctx->color))\n>>> \t\t\t\treturn end - placeholder + 1;\n>>> \t\t}\n>>\n>> Shouldn't we have an \"else\" here?\n>\n> I'm not sure what you mean; can you write it out?\n\n > -\t\tif (skip_prefix(begin, \"auto,\", &begin)) {\n > +\n > +\t\tif (!skip_prefix(begin, \"always,\", &begin)) {\n > \t\t\tif (!want_color(c->pretty_ctx->color))\n > \t\t\t\treturn end - placeholder + 1;\n > \t\t}\n\n\t\telse {\t/* here */\n\n > +\t\t/* this is a historical noop */\n > +\t\tskip_prefix(begin, \"auto,\", &begin);\n\n\t\t}\n\nOtherwise \"always,auto,\" would be allowed and mean the same as \n\"always,\", which just seems wrong. Not a biggie.\n\n>>> -\tif (skip_prefix(placeholder + 1, \"red\", &rest))\n>>> +\tif (skip_prefix(placeholder + 1, \"red\", &rest) &&\n>>> +\t want_color(c->pretty_ctx->color))\n>>> \t\tstrbuf_addstr(sb, GIT_COLOR_RED);\n>>> -\telse if (skip_prefix(placeholder + 1, \"green\", &rest))\n>>> +\telse if (skip_prefix(placeholder + 1, \"green\", &rest) &&\n>>> +\t\t want_color(c->pretty_ctx->color))\n>>> \t\tstrbuf_addstr(sb, GIT_COLOR_GREEN);\n>>> -\telse if (skip_prefix(placeholder + 1, \"blue\", &rest))\n>>> +\telse if (skip_prefix(placeholder + 1, \"blue\", &rest) &&\n>>> +\t\t want_color(c->pretty_ctx->color))\n>>> \t\tstrbuf_addstr(sb, GIT_COLOR_BLUE);\n>>> -\telse if (skip_prefix(placeholder + 1, \"reset\", &rest))\n>>> +\telse if (skip_prefix(placeholder + 1, \"reset\", &rest) &&\n>>> +\t\t want_color(c->pretty_ctx->color))\n>>> \t\tstrbuf_addstr(sb, GIT_COLOR_RESET);\n>>> \treturn rest - placeholder;\n>>> }\n>>\n>> Perhaps it's a funtion like add_color(sb, ctx, color) or similar would be\n>> nice?\n>\n> I actually wrote it that way first (I called it \"maybe_add_color()\"),\n> but it felt a little funny to have a separate function that people might\n> be tempted to reuse (the right solution is generally to check\n> want_color() early as above, but we can't do that here because we have\n> to find the end of each placeholder).\n\nOK. A variable then? Lazy pseudo-code:\n\n\tif (RED)\n\t\tcolor = red;\n\telse if (GREEN)\n\t\t...\n\n\tif (want_color())\n\t\tstrbuf_addstr(sb, color);\n\n> What I have here is a little funny, too, though, as it keeps trying\n> other color names if it finds \"red\" but want_color() returns 0.\n\nOh, missed that somehow.. O_o\n"},{"id":"303797","messageId":"20161010200457.6dzcrkhoi5id3z4x@sigill.intra.peff.net","threadId":"44256","inReplyTo":"19e59db7-f3dd-35ec-8cf1-b070b1c05abe@web.de","subject":"Re: [PATCH] pretty: respect color settings for %C placeholders","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-10T20:04:57Z","receivedAt":"2016-10-10T20:05:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2016 at 09:59:17PM +0200, René Scharfe wrote:\n\n> > > Shouldn't we have an \"else\" here?\n> > \n> > I'm not sure what you mean; can you write it out?\n> \n> > -\t\tif (skip_prefix(begin, \"auto,\", &begin)) {\n> > +\n> > +\t\tif (!skip_prefix(begin, \"always,\", &begin)) {\n> > \t\t\tif (!want_color(c->pretty_ctx->color))\n> > \t\t\t\treturn end - placeholder + 1;\n> > \t\t}\n> \n> \t\telse {\t/* here */\n> \n> > +\t\t/* this is a historical noop */\n> > +\t\tskip_prefix(begin, \"auto,\", &begin);\n> \n> \t\t}\n> \n> Otherwise \"always,auto,\" would be allowed and mean the same as \"always,\",\n> which just seems wrong. Not a biggie.\n\nI don't think that will parse \"%C(auto,foo)\", as we hit the\n!skip_prefix() of the conditional, and do not look for \"auto,\" at all.\n\nI think you'd have to move the check for \"auto,\" inside the if block.\n\nI'm leaning towards just writing it out the long way, though, as I did\nin my reply to Junio.\n\n> > > Perhaps it's a funtion like add_color(sb, ctx, color) or similar would be\n> > > nice?\n> > \n> > I actually wrote it that way first (I called it \"maybe_add_color()\"),\n> > but it felt a little funny to have a separate function that people might\n> > be tempted to reuse (the right solution is generally to check\n> > want_color() early as above, but we can't do that here because we have\n> > to find the end of each placeholder).\n> \n> OK. A variable then? Lazy pseudo-code:\n> \n> \tif (RED)\n> \t\tcolor = red;\n> \telse if (GREEN)\n> \t\t...\n> \n> \tif (want_color())\n> \t\tstrbuf_addstr(sb, color);\n\nYeah, that is a bit more clear (the final conditional just needs to\ncheck that we actually found a color).\n\n-Peff\n"},{"id":"303805","messageId":"6689ae49-6095-7bb4-ea06-1aaded174811@web.de","threadId":"44256","inReplyTo":"20161009234617.y6xfjyv6xjkf2afi@sigill.intra.peff.net","subject":"Re: %C(auto) not working as expected","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-10T20:52:47Z","receivedAt":"2016-10-10T20:53:15Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.10.2016 um 01:46 schrieb Jeff King:\n>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n>> index a942d57..89e3bc6 100644\n>> --- a/Documentation/pretty-formats.txt\n>> +++ b/Documentation/pretty-formats.txt\n>> @@ -166,7 +166,8 @@ endif::git-rev-list[]\n>> - '%Cgreen': switch color to green\n>> - '%Cblue': switch color to blue\n>> - '%Creset': reset color\n>> -- '%C(...)': color specification, as described in color.branch.* config option;\n>> +- '%C(...)': color specification, as described under Values, color in the\n>> + \"CONFIGURATION FILE\" section of linkgit:git-config[1];\n> \n> I like the intent here, though I found \"Values, color\" hard to parse (it\n> was not immediately clear that you mean \"the color paragraph of the\n> Values section\", as commas are already being used in that sentence for\n> the parenthetical phrase).\n> \n> I'm not sure how to say that succinctly, as we are four levels deep\n> (git-config -> configuration file -> values -> color). Too bad there is\n> no good link syntax for it. Maybe:\n> \n> ...color specification, as described in linkgit:git-config[1] (see the\n> paragraph on colors in the \"Values\" section, under \"CONFIGURATION\n> FILE\")\n> \n> or something.\n\nThat's better. Or we could remove the \"color\" part and trust readers to\nfind the right paragraph in the Values sub-section?\n\n-- '%C(...)': color specification, as described in color.branch.* config option;\n+- '%C(...)': color specification, as described under Values in the\n+ \"CONFIGURATION FILE\" section of linkgit:git-config[1];\n"},{"id":"303806","messageId":"20161010205424.p25nlrtwhhwu33ye@sigill.intra.peff.net","threadId":"44256","inReplyTo":"20161010185935.r3wnfglpi7hjjpdk@sigill.intra.peff.net","subject":"Re: [PATCH] pretty: respect color settings for %C placeholders","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-10T20:54:25Z","receivedAt":"2016-10-10T20:54:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2016 at 02:59:35PM -0400, Jeff King wrote:\n\n> > I must be reading the patch incorrectly, but I cannot quite tell\n> > where I want astray...\n> \n> No, we don't come here from %C() at all. This is for bare \"%Cred\", which\n> cannot have \"always\" (or \"auto\"), as there is no syntactic spot for it.\n> It is mostly historical (it _only_ supports red, green, and blue). We\n> could actually leave this as-is to show the colors unconditionally. I\n> changed it to keep the new behavior consistent, but I doubt anybody\n> cares much either way.\n\nSpeaking of consistent behavior, if we do this, I think we should give\n\"%(color:red)\" in for-each-ref and tag the same treatment. That requires\nsome infrastructure refactoring to get the value down to the\nref-formatting code. I'm working on it, but might not have it out today.\n\n-Peff\n"},{"id":"303807","messageId":"20161010205521.vxpwjrlelig3ilpe@sigill.intra.peff.net","threadId":"44256","inReplyTo":"6689ae49-6095-7bb4-ea06-1aaded174811@web.de","subject":"Re: %C(auto) not working as expected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-10T20:55:21Z","receivedAt":"2016-10-10T20:55:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2016 at 10:52:47PM +0200, René Scharfe wrote:\n\n> > I like the intent here, though I found \"Values, color\" hard to parse (it\n> > was not immediately clear that you mean \"the color paragraph of the\n> > Values section\", as commas are already being used in that sentence for\n> > the parenthetical phrase).\n> > \n> > I'm not sure how to say that succinctly, as we are four levels deep\n> > (git-config -> configuration file -> values -> color). Too bad there is\n> > no good link syntax for it. Maybe:\n> > \n> > ...color specification, as described in linkgit:git-config[1] (see the\n> > paragraph on colors in the \"Values\" section, under \"CONFIGURATION\n> > FILE\")\n> > \n> > or something.\n> \n> That's better. Or we could remove the \"color\" part and trust readers to\n> find the right paragraph in the Values sub-section?\n> \n> -- '%C(...)': color specification, as described in color.branch.* config option;\n> +- '%C(...)': color specification, as described under Values in the\n> + \"CONFIGURATION FILE\" section of linkgit:git-config[1];\n\nYeah, I think that is plenty of roadmap to give the user.\n\n-Peff\n"},{"id":"303852","messageId":"e0d64654-1562-2e71-8d42-3fc549a98d12@web.de","threadId":"44256","inReplyTo":"20161010205521.vxpwjrlelig3ilpe@sigill.intra.peff.net","subject":"[PATCH v2] pretty: fix document link for color specification","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-11T03:41:14Z","receivedAt":"2016-10-11T03:41:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Helped-by: Jeff King \nSigned-off-by: Rene Scharfe \n---\nRemoved confusing and unnecessary reference to the \"colors\" paragraph.\n\n Documentation/pretty-formats.txt | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex a942d57..89e3bc6 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -166,7 +166,8 @@ endif::git-rev-list[]\n - '%Cgreen': switch color to green\n - '%Cblue': switch color to blue\n - '%Creset': reset color\n-- '%C(...)': color specification, as described in color.branch.* config option;\n+- '%C(...)': color specification, as described under Values in the\n+ \"CONFIGURATION FILE\" section of linkgit:git-config[1];\n adding `auto,` at the beginning will emit color only when colors are\n enabled for log output (by `color.diff`, `color.ui`, or `--color`, and\n respecting the `auto` settings of the former if we are going to a\n-- \n2.10.1\n\n"},{"id":"303853","messageId":"20161011044518.6s2mtd6pvoiwgz37@sigill.intra.peff.net","threadId":"44256","inReplyTo":"e0d64654-1562-2e71-8d42-3fc549a98d12@web.de","subject":"Re: [PATCH v2] pretty: fix document link for color specification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-11T04:45:18Z","receivedAt":"2016-10-11T04:45:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 11, 2016 at 05:41:14AM +0200, René Scharfe wrote:\n\n> Helped-by: Jeff King \n> Signed-off-by: Rene Scharfe \n> ---\n> Removed confusing and unnecessary reference to the \"colors\" paragraph.\n\nThis looks good to me.\n\n-Peff\n"},{"id":"303865","messageId":"CACsJy8ABn_hp4nC-7ao7UPLc1vrC1B+50YwaRNyK5J-H_7f6JQ@mail.gmail.com","threadId":"44256","inReplyTo":"20161010151517.6wszhuyp57yfncaj@sigill.intra.peff.net","subject":"Re: [PATCH] pretty: respect color settings for %C placeholders","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-11T10:59:47Z","receivedAt":"2016-10-11T11:00:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 10, 2016 at 10:15 PM, Jeff King wrote:\n> On Mon, Oct 10, 2016 at 10:28:18AM -0400, Jeff King wrote:\n>\n>> > We could add some new tag to change the behavior of all following %C\n>> > tags. Something like %C(tty) maybe (probably a bad name), then\n>> > discourage the use if \"%C(auto\" for terminal detection?\n>>\n>> Yeah, adding a \"%C(enable-auto-color)\" or something would be backwards\n>> compatible and less painful than using \"%C(auto)\" everywhere. I do\n>> wonder if anybody actually _wants_ the \"always show color, even if\n>> --no-color\" behavior. I'm having trouble thinking of a good use for it.\n>>\n>> IOW, I'm wondering if anyone would disagree that the current behavior is\n>> simply buggy. Reading the thread at:\n>>\n>> http://public-inbox.org/git/7v4njkmq07.fsf@alter.siamese.dyndns.org/\n>>\n>> I don't really see any compelling reason against it (there was some\n>> question of which config to use, but we already answered that with\n>> \"%C(auto)\", and use the value from the pretty_ctx).\n>\n> So here's what a patch to do that would look like. I admit that \"I can't\n> think of a good use\" does not mean there _isn't_ one, but perhaps by\n> posting this, it might provoke other people to think on it, too. And if\n> nobody can come up with, maybe it's a good idea.\n\nI think you covered all bases with %C(always,..) and updating\nfor-each-ref code. And changing behavior of visual features like this\nsounds more like \"evolving\" than \"breaking backward compatibility\" to\nme. So +1.\n-- \nDuy\n"},{"id":"304840","messageId":"CACsJy8B_AQxm1=vF8i4FPtinq0id1QZPrqp9vvAmAgUns_kgGg@mail.gmail.com","threadId":"44256","inReplyTo":"20161010142818.lglwrxpks6l6aqrm@sigill.intra.peff.net","subject":"Re: %C(auto) not working as expected","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-25T12:52:21Z","receivedAt":"2016-10-25T12:52:57Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 10, 2016 at 9:28 PM, Jeff King wrote:\n> On Mon, Oct 10, 2016 at 04:26:18PM +0700, Duy Nguyen wrote:\n>\n>> > If we do a revamp of the pretty-formats to bring them more in line with\n>> > ref-filter (e.g., something like \"%(color:red)\") maybe that would be an\n>> > opportunity to make minor adjustments. Though, hmm, it looks like\n>> > for-each-ref already knows \"%(color:red)\", and it's unconditional.\n>> > So perhaps we would need to go through some deprecation period or\n>> > other transition.\n>>\n>> We could add some new tag to change the behavior of all following %C\n>> tags. Something like %C(tty) maybe (probably a bad name), then\n>> discourage the use if \"%C(auto\" for terminal detection?\n>\n> Yeah, adding a \"%C(enable-auto-color)\" or something would be backwards\n> compatible and less painful than using \"%C(auto)\" everywhere. I do\n> wonder if anybody actually _wants_ the \"always show color, even if\n> --no-color\" behavior. I'm having trouble thinking of a good use for it.\n>\n> IOW, I'm wondering if anyone would disagree that the current behavior is\n> simply buggy.\n\nSilence in two weeks. I vote (*) making %() honor --color\nand turning the %(auto, no-op, for both log family and for-each-ref.\nWe could keep old behavior behind some environment variable if it's\nnot much work so it keeps working while people come here and tell us\nabout their use cases.\n\n(*) I know.. voting is not how things work around here, unless you\nvote with patches, but I can't take on another topic.\n\n> Reading the thread at:\n>\n> http://public-inbox.org/git/7v4njkmq07.fsf@alter.siamese.dyndns.org/\n>\n> I don't really see any compelling reason against it (there was some\n> question of which config to use, but we already answered that with\n> \"%C(auto)\", and use the value from the pretty_ctx).\n>\n> -Peff\n-- \nDuy\n"},{"id":"304841","messageId":"20161025125856.p2paxt7erl2szptv@sigill.intra.peff.net","threadId":"44256","inReplyTo":"CACsJy8B_AQxm1=vF8i4FPtinq0id1QZPrqp9vvAmAgUns_kgGg@mail.gmail.com","subject":"Re: %C(auto) not working as expected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-25T12:58:56Z","receivedAt":"2016-10-25T13:00:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 25, 2016 at 07:52:21PM +0700, Duy Nguyen wrote:\n\n> > Yeah, adding a \"%C(enable-auto-color)\" or something would be backwards\n> > compatible and less painful than using \"%C(auto)\" everywhere. I do\n> > wonder if anybody actually _wants_ the \"always show color, even if\n> > --no-color\" behavior. I'm having trouble thinking of a good use for it.\n> >\n> > IOW, I'm wondering if anyone would disagree that the current behavior is\n> > simply buggy.\n> \n> Silence in two weeks. I vote (*) making %() honor --color\n> and turning the %(auto, no-op, for both log family and for-each-ref.\n> We could keep old behavior behind some environment variable if it's\n> not much work so it keeps working while people come here and tell us\n> about their use cases.\n\nYeah, sorry. I was blocked on making %(color:) in ref-filter work, which\nrequired a bunch of refactoring in ref-filter, which conflicted heavily\nwith the kn/ref-filter-branch-list (which wants to do a lot of the same\nthings), and then I got blocked on reviewing that series (which overall\nlooks pretty sane, but I wanted to really dig in because I think it\nhasn't gotten very careful review, or at least not recently).\n\nSo I'm still hoping to shave that yak at some point. Maybe this week.\n\n-Peff\n"},{"id":"304842","messageId":"CACsJy8AdwpoTRaaPBtmwipRP84Q=kUAabagYeMkHmT2kE3m+pw@mail.gmail.com","threadId":"44256","inReplyTo":"20161025125856.p2paxt7erl2szptv@sigill.intra.peff.net","subject":"Re: %C(auto) not working as expected","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-25T13:02:45Z","receivedAt":"2016-10-25T13:03:23Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 25, 2016 at 7:58 PM, Jeff King wrote:\n> On Tue, Oct 25, 2016 at 07:52:21PM +0700, Duy Nguyen wrote:\n>\n>> > Yeah, adding a \"%C(enable-auto-color)\" or something would be backwards\n>> > compatible and less painful than using \"%C(auto)\" everywhere. I do\n>> > wonder if anybody actually _wants_ the \"always show color, even if\n>> > --no-color\" behavior. I'm having trouble thinking of a good use for it.\n>> >\n>> > IOW, I'm wondering if anyone would disagree that the current behavior is\n>> > simply buggy.\n>>\n>> Silence in two weeks. I vote (*) making %() honor --color\n>> and turning the %(auto, no-op, for both log family and for-each-ref.\n>> We could keep old behavior behind some environment variable if it's\n>> not much work so it keeps working while people come here and tell us\n>> about their use cases.\n>\n> Yeah, sorry.\n\nJust to be clear (after re-reading my mail), the unwritten part after\n\"Silence in two weeks\" was \"so nobody disagreed with you\", not \"how is\nyour progress?\". It's great that you keep working on this regardless\n:D\n-- \nDuy\n"}]}