{"thread":{"id":"46942","subject":"What happened to \"git status --color=(always|auto|never)\"?","startedAt":"2017-10-09T23:59:12Z","lastAt":"2017-10-18T05:57:45Z","messageCount":42,"participants":["Nazri Ramliy","Jonathan Nieder","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"330067","messageId":"CAEY4ZpO2G-kTmuReE5gwKpftFqLfAqdpQwCK4R+qYbogCgGtUA@mail.gmail.com","threadId":"46942","inReplyTo":null,"subject":"What happened to \"git status --color=(always|auto|never)\"?","fromName":"Nazri Ramliy","fromEmail":"ayiehere@gmail.com","sentAt":"2017-10-09T23:58:46Z","receivedAt":"2017-10-09T23:59:12Z","isPatch":false,"sender":{"key":"ayiehere@gmail.com","avatar":"https://avatars.githubusercontent.com/u/164756?v=4"},"body":"I used to work before, but now:\n\n$ git version\ngit version 2.15.0.rc0.39.g2f0e14e649\n\n$ git status --color=always\nerror: unknown option `color=always'\nusage: git status [] [--] ...\n\nIs it no longer supported?\n\nnazri\n"},{"id":"330072","messageId":"20171010001619.GL19555@aiede.mtv.corp.google.com","threadId":"46942","inReplyTo":"CAEY4ZpO2G-kTmuReE5gwKpftFqLfAqdpQwCK4R+qYbogCgGtUA@mail.gmail.com","subject":"Re: What happened to \"git status --color=(always|auto|never)\"?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-10T00:16:19Z","receivedAt":"2017-10-10T00:16:26Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nNazri Ramliy wrote:\n\n> I used to work before, but now:\n>\n> $ git version\n> git version 2.15.0.rc0.39.g2f0e14e649\n>\n> $ git status --color=always\n> error: unknown option `color=always'\n> usage: git status [] [--] ...\n\nWhich version did it work in? That would allow me to bisect.\n\nThanks,\nJonathan\n"},{"id":"330075","messageId":"CAEY4ZpPj3=+gL_wBW548qzAuS=aC=qswuPx-4H9DS=X10iJWVw@mail.gmail.com","threadId":"46942","inReplyTo":"20171010001619.GL19555@aiede.mtv.corp.google.com","subject":"Re: What happened to \"git status --color=(always|auto|never)\"?","fromName":"Nazri Ramliy","fromEmail":"ayiehere@gmail.com","sentAt":"2017-10-10T00:43:40Z","receivedAt":"2017-10-10T00:44:07Z","isPatch":false,"sender":{"key":"ayiehere@gmail.com","avatar":"https://avatars.githubusercontent.com/u/164756?v=4"},"body":"On Tue, Oct 10, 2017 at 8:16 AM, Jonathan Nieder wrote:\n> Hi,\n>\n> Nazri Ramliy wrote:\n>\n>> I used to work before, but now:\n>>\n>> $ git version\n>> git version 2.15.0.rc0.39.g2f0e14e649\n>>\n>> $ git status --color=always\n>> error: unknown option `color=always'\n>> usage: git status [] [--] ...\n>\n> Which version did it work in? That would allow me to bisect.\n\nSorry. It's my bad. I must have confused this with `git grep`'s --color option.\n\nI have a perl script that shells out to `git -c color.status=always\nstatus`, and I wrongly documented that the perl script's\n`--color=(always|never|auto)` is \"similar to git-status' option\".\n\nI verified this down to git-2.1.0, git-1.7.0 and git-1.4.0 confirmed\nthat `git status` do not support --color option then, and mostly\nlikely not in the versions between.\n\nnazri\n"},{"id":"330077","messageId":"20171010005942.GO19555@aiede.mtv.corp.google.com","threadId":"46942","inReplyTo":"CAEY4ZpPj3=+gL_wBW548qzAuS=aC=qswuPx-4H9DS=X10iJWVw@mail.gmail.com","subject":"Re: What happened to \"git status --color=(always|auto|never)\"?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-10T00:59:42Z","receivedAt":"2017-10-10T00:59:50Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nazri Ramliy wrote:\n> On Tue, Oct 10, 2017 at 8:16 AM, Jonathan Nieder wrote:\n>> Nazri Ramliy wrote:\n\n>>> I used to work before, but now:\n>>>\n>>> $ git version\n>>> git version 2.15.0.rc0.39.g2f0e14e649\n>>>\n>>> $ git status --color=always\n>>> error: unknown option `color=always'\n>>> usage: git status [] [--] ...\n>>\n>> Which version did it work in? That would allow me to bisect.\n>\n> Sorry. It's my bad. I must have confused this with `git grep`'s --color option.\n\nNo problem. It sounds like a reasonable feature request to me,\nespecially now that we are about to drop support for\ncolor.status=always in configuration:\n\n\tcommit 6be4595edb8e5b616c6e8b9fbc78b0f831fa2a87\n\tAuthor: Jeff King \n\tDate: Tue Oct 3 09:46:06 2017 -0400\n\n\t color: make \"always\" the same as \"auto\" in config\n\nWould you like to take a stab at adding it? builtin/commit.c and\nDocumentation/git-{commit,status}.txt would be my best guesses at\nwhere to start.\n\nThanks,\nJonathan\n"},{"id":"330093","messageId":"CAEY4ZpMKE6yf2baaJt+x6c_esorFnyWvLZ=_KS1iRs6XbL42hw@mail.gmail.com","threadId":"46942","inReplyTo":"20171010005942.GO19555@aiede.mtv.corp.google.com","subject":"Re: What happened to \"git status --color=(always|auto|never)\"?","fromName":"Nazri Ramliy","fromEmail":"ayiehere@gmail.com","sentAt":"2017-10-10T04:42:43Z","receivedAt":"2017-10-10T04:43:09Z","isPatch":false,"sender":{"key":"ayiehere@gmail.com","avatar":"https://avatars.githubusercontent.com/u/164756?v=4"},"body":"> commit 6be4595edb8e5b616c6e8b9fbc78b0f831fa2a87\n> Author: Jeff King \n> Date: Tue Oct 3 09:46:06 2017 -0400\n>\n> color: make \"always\" the same as \"auto\" in config\n>\n> Would you like to take a stab at adding it? builtin/commit.c and\n> Documentation/git-{commit,status}.txt would be my best guesses at\n> where to start.\n\nPerhaps, seeing that that commit intentionally \"broke\" the color\noutput of my tool[1], because it parses the output of `git -c\ncolor.status=always status`, which now no longer work the way it used\nto. I know, I know... shame on me for parsing the output of a\nporcelain command :)\n\nBut this also means that I will have to modify [1] to cope with this,\ngiven that it may be used with an older version of git (parse\ngit-version and shell out to different git command - either `git -c\ncolor.ui=always status`, or the not-yet-exist `git status\n--color=always`), or make it use the plumbing output of `git status`,\nbut that would just add additional work that I really don't look\nforward to doing at this moment.\n\nnazri\n\n[1] https://github.com/holygeek/git-number\nThis tool (naively) parses the porcelain output out `git -c\ncolor.status=always status` in order to insert numbers for each\nfilenames that `git status` prints.\n"},{"id":"330103","messageId":"20171010102509.e7ucbyon6ka6722l@sigill.intra.peff.net","threadId":"46942","inReplyTo":"CAEY4ZpMKE6yf2baaJt+x6c_esorFnyWvLZ=_KS1iRs6XbL42hw@mail.gmail.com","subject":"Re: What happened to \"git status --color=(always|auto|never)\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-10T10:25:10Z","receivedAt":"2017-10-10T10:25:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 10, 2017 at 12:42:43PM +0800, Nazri Ramliy wrote:\n\n> > commit 6be4595edb8e5b616c6e8b9fbc78b0f831fa2a87\n> > Author: Jeff King \n> > Date: Tue Oct 3 09:46:06 2017 -0400\n> >\n> > color: make \"always\" the same as \"auto\" in config\n> >\n> > Would you like to take a stab at adding it? builtin/commit.c and\n> > Documentation/git-{commit,status}.txt would be my best guesses at\n> > where to start.\n> \n> Perhaps, seeing that that commit intentionally \"broke\" the color\n> output of my tool[1], because it parses the output of `git -c\n> color.status=always status`, which now no longer work the way it used\n> to. I know, I know... shame on me for parsing the output of a\n> porcelain command :)\n> \n> But this also means that I will have to modify [1] to cope with this,\n> given that it may be used with an older version of git (parse\n> git-version and shell out to different git command - either `git -c\n> color.ui=always status`, or the not-yet-exist `git status\n> --color=always`), or make it use the plumbing output of `git status`,\n> but that would just add additional work that I really don't look\n> forward to doing at this moment.\n\n:( I was worried that this might hit some third-party scripts.\n\nOne workaround you can do that should work with any version of Git is:\n\n GIT_PAGER_IN_USE=1 git status | your-parser\n\nThat tells Git that even though stdout isn't a tty, you're expecting to\npresent the data to the user and it should be colored appropriately. It\nhas the additional upside that it doesn't override the user's color\nconfig.\n\nIt does have the potential downside that other non-color changes could\nkick in (e.g., somebody recently proposed that auto-columns kick in with\nGIT_PAGER_IN_USE).\n\nAll that said, should we revisit the decision from 6be4595edb? The two\ncode changes we could make are:\n\n 1. Adding a \"--color\" option to \"git status\". Commit 0c88bf5050\n (provide --color option for all ref-filter users, 2017-10-03) from\n that same series shows some prior art.\n\n This is a clean solution, but it does mean that scripts have to\n adapt (and would potentially need to care about which Git version\n they're relying on).\n\n 2. Re-allow \"color.always\" config from the command-line. It's actually\n on-disk config that we want to downgrade, but I wanted to avoid\n making complicated rules about how the config would behave in\n different scopes. The patch for this would look something like the\n one below.\n\n 3. Revert the original series, and revisit the original \"respect\n color.ui via porcelain\" commit which broke add--interactive in\n v2.14.2 (136c8c8b8fa).\n\nI dunno. I think for your use case, PAGER_IN_USE may actually be the\n\"right\" solution, because it most closely expresses what you're doing.\nWe probably ought to have (1) as a general rule for commands which\nhandle color.\n\nBut (2) and (3) are the only ones that will work seamlessly with\nexisting scripts. I'm not excited about either of them, though.\n\n-Peff\n\ndiff --git a/color.c b/color.c\nindex 9c0dc82370..3870d3e395 100644\n--- a/color.c\n+++ b/color.c\n@@ -307,8 +307,21 @@ int git_config_colorbool(const char *var, const char *value)\n \tif (value) {\n \t\tif (!strcasecmp(value, \"never\"))\n \t\t\treturn 0;\n-\t\tif (!strcasecmp(value, \"always\"))\n-\t\t\treturn var ? GIT_COLOR_AUTO : 1;\n+\t\tif (!strcasecmp(value, \"always\")) {\n+\t\t\t/*\n+\t\t\t * Command-line options always respect \"always\".\n+\t\t\t * Likewise for \"-c\" config on the command-line.\n+\t\t\t */\n+\t\t\tif (!var ||\n+\t\t\t current_config_scope() == CONFIG_SCOPE_CMDLINE)\n+\t\t\t\treturn 1;\n+\n+\t\t\t/*\n+\t\t\t * Otherwise, we're looking at on-disk config;\n+\t\t\t * downgrade to auto.\n+\t\t\t */\n+\t\t\treturn GIT_COLOR_AUTO;\n+\t\t}\n \t\tif (!strcasecmp(value, \"auto\"))\n \t\t\treturn GIT_COLOR_AUTO;\n \t}\n"},{"id":"330109","messageId":"xmqqfuarp3mt.fsf@gitster.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171010102509.e7ucbyon6ka6722l@sigill.intra.peff.net","subject":"Re: What happened to \"git status --color=(always|auto|never)\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T12:51:38Z","receivedAt":"2017-10-10T12:51:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> :( I was worried that this might hit some third-party scripts.\n> ...\n> All that said, should we revisit the decision from 6be4595edb? The two\n> code changes we could make are:\n>\n> 1. Adding a \"--color\" option to \"git status\". Commit 0c88bf5050\n> (provide --color option for all ref-filter users, 2017-10-03) from\n> that same series shows some prior art.\n>\n> This is a clean solution, but it does mean that scripts have to\n> adapt (and would potentially need to care about which Git version\n> they're relying on).\n\nIf we view that \"always\" issue is a regression, then this is not a\n\"solution\". It is a part of an ideal world where we never allowed\n\"always\" as a value for color.ui, which is not the world we live in.\n\n> 2. Re-allow \"color.always\" config from the command-line. It's actually\n> on-disk config that we want to downgrade, but I wanted to avoid\n> making complicated rules about how the config would behave in\n> different scopes. The patch for this would look something like the\n> one below.\n\nYuck, ugly. The code is simple (thanks to the \"who ordered it?\"\nthing), but the behaviour is rather embarrassing to explain.\n\n> 3. Revert the original series, and revisit the original \"respect\n> color.ui via porcelain\" commit which broke add--interactive in\n> v2.14.2 (136c8c8b8fa).\n\nWhich one do you mean is \"the original series\"? The one that made\nplumbing to pay attention to the color config? I think it would be\nthe cleanest \"solution\" in the world we live in, but the series (and\nthe follow-on changes that started assuming that config_default\nreads the color config) have a rather large footprint and it will be\nquite painful to vet the result.\n\nI think the right fix to the original problem (you cannot remove\nauto-color from the plumbing) is to stop paying attention to color\nconfiguration from the default config. I wonder if something like\nthis would work?\n\n - Initialize color.c::git_use_color_default to GIT_COLOR_UNKNOWN;\n\n - When git_color_config() is called, and if git_use_color_default\n is still GIT_COLOR_UNKNOWN, set it to GIT_COLOR_AUTO (regardless\n of the variable git_color_config() is called for).\n\n - In color.c::want_color(), when git_use_color_default is used,\n notice if it is GIT_COLOR_UNKNOWN and behave as if it is\n GIT_COLOR_NEVER.\n\nThen we make sure that git_color_config() is never called by any\nplumbing command. The fact it is (ever) called can be taken as a\nclue that we are running a Porcelain (hence we transition from\nUNKNOWN to AUTO), so we'd get the desirable \"no default color for\nplumbing, auto color for Porcelain\", I would think.\n\n"},{"id":"330112","messageId":"20171010130602.ivhsbu2ymnzt7gko@sigill.intra.peff.net","threadId":"46942","inReplyTo":"xmqqfuarp3mt.fsf@gitster.mtv.corp.google.com","subject":"Re: What happened to \"git status --color=(always|auto|never)\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-10T13:06:02Z","receivedAt":"2017-10-10T13:06:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 10, 2017 at 09:51:38PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > :( I was worried that this might hit some third-party scripts.\n> > ...\n> > All that said, should we revisit the decision from 6be4595edb? The two\n> > code changes we could make are:\n> >\n> > 1. Adding a \"--color\" option to \"git status\". Commit 0c88bf5050\n> > (provide --color option for all ref-filter users, 2017-10-03) from\n> > that same series shows some prior art.\n> >\n> > This is a clean solution, but it does mean that scripts have to\n> > adapt (and would potentially need to care about which Git version\n> > they're relying on).\n> \n> If we view that \"always\" issue is a regression, then this is not a\n> \"solution\". It is a part of an ideal world where we never allowed\n> \"always\" as a value for color.ui, which is not the world we live in.\n\nRight, this doesn't solve any regression. It solves the \"there's no way\nto do this thing I might want to\" that exists in either world (one where\n\"always\" never existed, or one where \"always\" does not do that anymore\nbut we accept it as either not a regression or an acceptable\nregression).\n\n> > 2. Re-allow \"color.always\" config from the command-line. It's actually\n> > on-disk config that we want to downgrade, but I wanted to avoid\n> > making complicated rules about how the config would behave in\n> > different scopes. The patch for this would look something like the\n> > one below.\n> \n> Yuck, ugly. The code is simple (thanks to the \"who ordered it?\"\n> thing), but the behaviour is rather embarrassing to explain.\n\nYes, it's definitely the most ugly of all the options. The reason I\nmention it is that it's also the only one that solves the \"git -c\ncolor.ui=always\" regression (if we consider it one) without making a\nhuge and risky change.\n\n> > 3. Revert the original series, and revisit the original \"respect\n> > color.ui via porcelain\" commit which broke add--interactive in\n> > v2.14.2 (136c8c8b8fa).\n> \n> Which one do you mean is \"the original series\"? The one that made\n> plumbing to pay attention to the color config?\n\nNo, I meant reverting jk/ui-color-always-to-auto. But if we revert that,\nit leaves \"add -p\" broken when you set color.ui=always. So we must\neither accept that, or _also_ revert 136c8c8b8fa and come up with a\ndifferent solution.\n\n> I think it would be\n> the cleanest \"solution\" in the world we live in, but the series (and\n> the follow-on changes that started assuming that config_default\n> reads the color config) have a rather large footprint and it will be\n> quite painful to vet the result.\n\nI agree that is a risk. It might not be _too_ bad, though this is an\narea that historically has poor test coverage. I know that for-each-ref\nand tag are two that would need touched (and it was them that led me\ndown to the path to 136c8c8b8fa in the first place).\n\n> I think the right fix to the original problem (you cannot remove\n> auto-color from the plumbing) is to stop paying attention to color\n> configuration from the default config. I wonder if something like\n> this would work?\n> \n> - Initialize color.c::git_use_color_default to GIT_COLOR_UNKNOWN;\n> \n> - When git_color_config() is called, and if git_use_color_default\n> is still GIT_COLOR_UNKNOWN, set it to GIT_COLOR_AUTO (regardless\n> of the variable git_color_config() is called for).\n> \n> - In color.c::want_color(), when git_use_color_default is used,\n> notice if it is GIT_COLOR_UNKNOWN and behave as if it is\n> GIT_COLOR_NEVER.\n>\n> Then we make sure that git_color_config() is never called by any\n> plumbing command. The fact it is (ever) called can be taken as a\n> clue that we are running a Porcelain (hence we transition from\n> UNKNOWN to AUTO), so we'd get the desirable \"no default color for\n> plumbing, auto color for Porcelain\", I would think.\n\nYes, I think that's the simplest way to implement the \"plumbing should\nnever do color without a command-line option\" scheme.\n\nI do wonder if people would end up seeing some corner cases as\nregressions, though. Right now \"diff-tree\" _does_ color the output by\ndefault, and it would stop doing so under your scheme. That's the right\nthing to do by the plumbing/porcelain distinction, but users with\nscripts that use diff-tree (or other plumbing) to generate user-visible\noutput may unexpectedly lose their color, until the calling script is\nfixed to add back in a --color option[1].\n\n-Peff\n\n[1] Actually, just saying \"--color\" isn't enough, since you'd want to\n respect the user's color options. add--interactive does this, but\n it's a slight pain. It would be nice to have a --color=config\n variant that just calls git_color_config(). But if we are talking\n regression-fixes before v2.15, I don't think we need to have such\n niceties.\n"},{"id":"330132","messageId":"20171010190314.GW19555@aiede.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171010130602.ivhsbu2ymnzt7gko@sigill.intra.peff.net","subject":"Re: What happened to \"git status --color=(always|auto|never)\"?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-10T19:03:14Z","receivedAt":"2017-10-10T19:03:23Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Tue, Oct 10, 2017 at 09:51:38PM +0900, Junio C Hamano wrote:\n\n>> I think the right fix to the original problem (you cannot remove\n>> auto-color from the plumbing) is to stop paying attention to color\n>> configuration from the default config. I wonder if something like\n>> this would work?\n>>\n>> - Initialize color.c::git_use_color_default to GIT_COLOR_UNKNOWN;\n>>\n>> - When git_color_config() is called, and if git_use_color_default\n>> is still GIT_COLOR_UNKNOWN, set it to GIT_COLOR_AUTO (regardless\n>> of the variable git_color_config() is called for).\n>>\n>> - In color.c::want_color(), when git_use_color_default is used,\n>> notice if it is GIT_COLOR_UNKNOWN and behave as if it is\n>> GIT_COLOR_NEVER.\n>>\n>> Then we make sure that git_color_config() is never called by any\n>> plumbing command. The fact it is (ever) called can be taken as a\n>> clue that we are running a Porcelain (hence we transition from\n>> UNKNOWN to AUTO), so we'd get the desirable \"no default color for\n>> plumbing, auto color for Porcelain\", I would think.\n>\n> Yes, I think that's the simplest way to implement the \"plumbing should\n> never do color without a command-line option\" scheme.\n>\n> I do wonder if people would end up seeing some corner cases as\n> regressions, though. Right now \"diff-tree\" _does_ color the output by\n> default, and it would stop doing so under your scheme. That's the right\n> thing to do by the plumbing/porcelain distinction, but users with\n> scripts that use diff-tree (or other plumbing) to generate user-visible\n> output may unexpectedly lose their color, until the calling script is\n> fixed to add back in a --color option[1].\n\nI think it's better for the calling script to be fixed to use \"git\ndiff\", since it is producing output for the sake of the user instead\nof for machine parsing. That way, the script gets the benefit of\nother changes like --decorate automatically.\n\nSo I don't see that as a regression.\n\nWhere I worry is about commands where the line between porcelain and\nplumbing blur, like \"git log --format=raw\". I actually still prefer\nthe approach where \"color.ui=always\" becomes impossible to express in\nconfig and each command takes a --color option.\n\nIf we want to be extra fancy, we could make git take a --color option\ninstead of requiring each command to do it.\n\nTo support existing scripts, we could treat \"-c color.ui=always\" as a\nhistorical synonym for --color=always, either temporarily or\nindefinitely. Making it clear that this is only there for historical\nreasons would make it less likely that other options make the same\nmistake in the future.\n\nThanks,\nJonathan\n"},{"id":"330133","messageId":"20171010193729.nrx7cgifsmpd4c2e@sigill.intra.peff.net","threadId":"46942","inReplyTo":"20171010190314.GW19555@aiede.mtv.corp.google.com","subject":"Re: What happened to \"git status --color=(always|auto|never)\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-10T19:37:30Z","receivedAt":"2017-10-10T19:37:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 10, 2017 at 12:03:14PM -0700, Jonathan Nieder wrote:\n\n> > I do wonder if people would end up seeing some corner cases as\n> > regressions, though. Right now \"diff-tree\" _does_ color the output by\n> > default, and it would stop doing so under your scheme. That's the right\n> > thing to do by the plumbing/porcelain distinction, but users with\n> > scripts that use diff-tree (or other plumbing) to generate user-visible\n> > output may unexpectedly lose their color, until the calling script is\n> > fixed to add back in a --color option[1].\n> \n> I think it's better for the calling script to be fixed to use \"git\n> diff\", since it is producing output for the sake of the user instead\n> of for machine parsing. That way, the script gets the benefit of\n> other changes like --decorate automatically.\n> \n> So I don't see that as a regression.\n\nI agree that may be the best way for those scripts to do it. But it's\nstill a regression to them, if their script used to do what they wanted\nand now it doesn't.\n\nIt may be one we don't want to care about because the script is doing\nsomething we don't want to support. But then, think we are still\ndeciding whether \"color.always\" in your ~/.gitconfig is in the same\nboat.\n\n> Where I worry is about commands where the line between porcelain and\n> plumbing blur, like \"git log --format=raw\". I actually still prefer\n> the approach where \"color.ui=always\" becomes impossible to express in\n> config and each command takes a --color option.\n> \n> If we want to be extra fancy, we could make git take a --color option\n> instead of requiring each command to do it.\n> \n> To support existing scripts, we could treat \"-c color.ui=always\" as a\n> historical synonym for --color=always, either temporarily or\n> indefinitely. Making it clear that this is only there for historical\n> reasons would make it less likely that other options make the same\n> mistake in the future.\n\nSo that's basically my (2), with the twist that we claim it's only\nhorrible and inconsistent for historical reasons. :)\n\nIs that the direction we want to go?\n\n-Peff\n"},{"id":"330144","messageId":"xmqqr2uao2vy.fsf@gitster.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171010193729.nrx7cgifsmpd4c2e@sigill.intra.peff.net","subject":"Re: What happened to \"git status --color=(always|auto|never)\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-11T02:05:21Z","receivedAt":"2017-10-11T02:05:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Oct 10, 2017 at 12:03:14PM -0700, Jonathan Nieder wrote:\n>\n>> Where I worry is about commands where the line between porcelain and\n>> plumbing blur, like \"git log --format=raw\". I actually still prefer\n>> the approach where \"color.ui=always\" becomes impossible to express in\n>> config and each command takes a --color option.\n>> \n>> If we want to be extra fancy, we could make git take a --color option\n>> instead of requiring each command to do it.\n>> \n>> To support existing scripts, we could treat \"-c color.ui=always\" as a\n>> historical synonym for --color=always, either temporarily or\n>> indefinitely. Making it clear that this is only there for historical\n>> reasons would make it less likely that other options make the same\n>> mistake in the future.\n>\n> So that's basically my (2), with the twist that we claim it's only\n> horrible and inconsistent for historical reasons. :)\n>\n> Is that the direction we want to go?\n\nYour (2), and Jonathan's \"git --color=...\" as an extension to it,\nis probably the least risky approach forward, as you said earlier.\nAnd I think that it would get us closest to the ideal world (in\nwhich color=always did not exist in the configuration system), while\nbreaking least number of various \"questionable\" scripts in the wild.\n\n\n"},{"id":"330223","messageId":"20171012021007.7441-1-gitster@pobox.com","threadId":"46942","inReplyTo":"xmqqr2uao2vy.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 0/2] Piling more kludge on top of color.ui","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-12T02:10:05Z","receivedAt":"2017-10-12T02:10:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Earlier we added a patch to unconditionally downgrade 'always' that\nis set to the color.ui configuration variable. This was done to\ncorrec the unintended regression to \"git add -i\" that was caused by\ntwo earlier mistakes that we no longer can undo.\n\n - The \"add -i\" command wants to parse output from \"git diff-index\"\n plumbing. The plumbing commands started paying attention to\n color configuration variables (which is a mistaken \"solution\" to\n cover another mistake), which caused people who have color.ui set\n to \"always\" to see breakage, as their \"git diff-index\" started\n coloring its output even when \"git add -i\" wanted to read it and\n parse it (without expecting to see any colors in its input);\n\n - The mistake the mistaken \"solution\" wanted to cover was that some\n time ago we started to automatically color the output (i.e. color\n when the output goes to the terminal) by default, but did so even\n to the plumbing commands. As many plumbing commands do not even\n have their own color control, it made it impossible to disable\n this auto-coloring--a mistaken \"solution\" was to pay attention to\n \"git -c color.ui=never\" from the command line.\n\nIt turns out that there are many third-party scripts that do want to\nread colored output from our tools and the way they do so is to run\n\"git -c color.ui=always cmd\", which is a way to defeat any end-user\nsettings and force coloured output reliably---at least they thought\nthat they can rely on it working, that is. We saw one report of\nsuch a private tool getting broken on list, and I've seen another\none inside $work.\n\nLet's keep \"git -c color.ui=always cmd\" form \"working\", while\ndowngrading the setting made in the configuration files to \"auto\",\nto placate both camps. Let's also discourage use of 'always' and\nleave the door open for us to introduce \"git --default-color=\ncmd\" later as a substitute for \"git -c color.ui=\".\n\nJeff King (1):\n color: downgrade \"always\" to \"auto\" only for on-disk configuration\n\nJunio C Hamano (1):\n color: discourage use of ui.color=always\n\n Documentation/config.txt | 2 +-\n color.c | 24 ++++++++++++++++++++++--\n 2 files changed, 23 insertions(+), 3 deletions(-)\n\n-- \n2.15.0-rc1-151-g44fe2f342f\n\n"},{"id":"330224","messageId":"20171012021007.7441-3-gitster@pobox.com","threadId":"46942","inReplyTo":"20171012021007.7441-1-gitster@pobox.com","subject":"[PATCH 2/2] color: discourage use of ui.color=always","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-12T02:10:07Z","receivedAt":"2017-10-12T02:10:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Warn when we read such a configuration from a file, and nudge the\nusers to spell them 'auto' instead.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 2 +-\n color.c | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex cb0f951ddc..ba01b8d3df 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1178,7 +1178,7 @@ color.ui::\n \tor the `--color` option. Set it to `true` or `auto` to enable\n \tcolor when output is written to the terminal (this is also the\n \tdefault since Git 1.8.4). The value `always` is a historical\n-\tsynonym for `auto`.\n+\tsynonym for `auto` and its use is discouraged.\n \n column.ui::\n \tSpecify whether supported commands should output in columns.\ndiff --git a/color.c b/color.c\nindex 5b06c76bdc..5119f9bca0 100644\n--- a/color.c\n+++ b/color.c\n@@ -308,6 +308,8 @@ int git_config_colorbool(const char *var, const char *value)\n \t\tif (!strcasecmp(value, \"never\"))\n \t\t\treturn 0;\n \t\tif (!strcasecmp(value, \"always\")) {\n+\t\t\tstatic int warn_once;\n+\n \t\t\t/*\n \t\t\t * Command-line options always respect \"always\".\n \t\t\t * Likewise for \"-c\" config on the command-line.\n@@ -320,6 +322,11 @@ int git_config_colorbool(const char *var, const char *value)\n \t\t\t * Otherwise, we're looking at on-disk config;\n \t\t\t * downgrade to auto.\n \t\t\t */\n+\t\t\tif (!warn_once) {\n+\t\t\t\twarn_once++;\n+\t\t\t\twarning(\"setting '%s' to '%s' is no longer valid; \"\n+\t\t\t\t\t\"set it to 'auto' instead\", var, value);\n+\t\t\t}\n \t\t\treturn GIT_COLOR_AUTO;\n \t\t}\n \t\tif (!strcasecmp(value, \"auto\"))\n-- \n2.15.0-rc1-151-g44fe2f342f\n\n"},{"id":"330225","messageId":"20171012021007.7441-2-gitster@pobox.com","threadId":"46942","inReplyTo":"20171012021007.7441-1-gitster@pobox.com","subject":"[PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-12T02:10:06Z","receivedAt":"2017-10-12T02:10:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King \n\nAn earlier patch downgraded \"always\" that comes via the ui.color\nconfiguration variable to \"auto\", in order to work around an\nunfortunate regression to \"git add -i\".\n\nThat \"fix\" however regressed other third-party tools that depend on\n\"git -c color.ui=always cmd\" as the way to defeat any end-user\nconfiguration and force coloured output from git subcommand, even\nwhen the output does not go to a terminal.\n\nIt is a bit ugly to treat \"-c color.ui=always\" from the command line\ndifferently from a definition that comes from on-disk configuration\nfiles, but it is a moral equivalent of \"--color=always\" option given\nto the subcommand from the command line, i.e. a signal that tells us\nthat the script writer knows what s/he is doing. So let's take that\nroute to unbreak this case while defeating a (now declared to be)\nmisguided color.ui that is set to always in the configuration file.\n\nNEEDS-SIGN-OFF-BY: Jeff King \nSigned-off-by: Junio C Hamano \n---\n color.c | 17 +++++++++++++++--\n 1 file changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/color.c b/color.c\nindex 17e2713f96..5b06c76bdc 100644\n--- a/color.c\n+++ b/color.c\n@@ -307,8 +307,21 @@ int git_config_colorbool(const char *var, const char *value)\n \tif (value) {\n \t\tif (!strcasecmp(value, \"never\"))\n \t\t\treturn 0;\n-\t\tif (!strcasecmp(value, \"always\"))\n-\t\t\treturn var ? GIT_COLOR_AUTO : 1;\n+\t\tif (!strcasecmp(value, \"always\")) {\n+\t\t\t/*\n+\t\t\t * Command-line options always respect \"always\".\n+\t\t\t * Likewise for \"-c\" config on the command-line.\n+\t\t\t */\n+\t\t\tif (!var ||\n+\t\t\t current_config_scope() == CONFIG_SCOPE_CMDLINE)\n+\t\t\t\treturn 1;\n+\n+\t\t\t/*\n+\t\t\t * Otherwise, we're looking at on-disk config;\n+\t\t\t * downgrade to auto.\n+\t\t\t */\n+\t\t\treturn GIT_COLOR_AUTO;\n+\t\t}\n \t\tif (!strcasecmp(value, \"auto\"))\n \t\t\treturn GIT_COLOR_AUTO;\n \t}\n-- \n2.15.0-rc1-151-g44fe2f342f\n\n"},{"id":"330238","messageId":"20171012044724.GD155740@aiede.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171012021007.7441-2-gitster@pobox.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-12T04:47:24Z","receivedAt":"2017-10-12T04:47:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n[...]\n> --- a/color.c\n> +++ b/color.c\n> @@ -307,8 +307,21 @@ int git_config_colorbool(const char *var, const char *value)\n> \tif (value) {\n> \t\tif (!strcasecmp(value, \"never\"))\n> \t\t\treturn 0;\n> -\t\tif (!strcasecmp(value, \"always\"))\n> -\t\t\treturn var ? GIT_COLOR_AUTO : 1;\n> +\t\tif (!strcasecmp(value, \"always\")) {\n> +\t\t\t/*\n> +\t\t\t * Command-line options always respect \"always\".\n> +\t\t\t * Likewise for \"-c\" config on the command-line.\n> +\t\t\t */\n> +\t\t\tif (!var ||\n> +\t\t\t current_config_scope() == CONFIG_SCOPE_CMDLINE)\n> +\t\t\t\treturn 1;\n> +\n> +\t\t\t/*\n> +\t\t\t * Otherwise, we're looking at on-disk config;\n> +\t\t\t * downgrade to auto.\n> +\t\t\t */\n> +\t\t\treturn GIT_COLOR_AUTO;\n> +\t\t}\n\nYes, this looks good to me.\n\nShould we document this special case treatment of color.* in -c\nsomewhere? E.g.\n\nSigned-off-by: Jonathan Nieder \n\ndiff --git i/Documentation/config.txt w/Documentation/config.txt\nindex 13ce76d48b..d7bd6b169c 100644\n--- i/Documentation/config.txt\n+++ w/Documentation/config.txt\n@@ -1067,11 +1067,15 @@ clean.requireForce::\n \t-i or -n. Defaults to true.\n \n color.branch::\n-\tA boolean to enable/disable color in the output of\n-\tlinkgit:git-branch[1]. May be set to `false` (or `never`) to\n-\tdisable color entirely, `auto` (or `true` or `always`) in which\n-\tcase colors are used only when the output is to a terminal. If\n-\tunset, then the value of `color.ui` is used (`auto` by default).\n+\tWhen to use color in the output of linkgit:git-branch[1].\n+\tMay be set to `never` (or `false`) to disable color entirely,\n+\tor `auto` (or `true`) in which case colors are used only when\n+\tthe output is to a terminal. If unset, then the value of\n+\t`color.ui` is used (`auto` by default).\n++\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it is a historical synonym\n+for `--color=always`.\n \n color.branch.::\n \tUse customized color for branch coloration. `` is one of\n@@ -1084,10 +1088,13 @@ color.diff::\n \tWhether to use ANSI escape sequences to add color to patches.\n \tIf this is set to `true` or `auto`, linkgit:git-diff[1],\n \tlinkgit:git-log[1], and linkgit:git-show[1] will use color\n-\twhen output is to the terminal. The value `always` is a\n-\thistorical synonym for `auto`. If unset, then the value of\n+\twhen output is to the terminal. If unset, then the value of\n \t`color.ui` is used (`auto` by default).\n +\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it is a historical\n+synonym for `--color=always`.\n++\n This does not affect linkgit:git-format-patch[1] or the\n 'git-diff-{asterisk}' plumbing commands. Can be overridden on the\n command line with the `--color[=]` option.\n@@ -1118,10 +1125,14 @@ color.decorate.::\n \tbranches, remote-tracking branches, tags, stash and HEAD, respectively.\n \n color.grep::\n-\tWhen set to `always`, always highlight matches. When `false` (or\n+\tWhen to highlight matches using color. When `false` (or\n \t`never`), never. When set to `true` or `auto`, use color only\n \twhen the output is written to the terminal. If unset, then the\n \tvalue of `color.ui` is used (`auto` by default).\n++\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it is a historical synonym\n+for `--color=always`.\n \n color.grep.::\n \tUse customized color for grep colorization. `` specifies which\n@@ -1153,9 +1164,11 @@ color.interactive::\n \tWhen set to `true` or `auto`, use colors for interactive prompts\n \tand displays (such as those used by \"git-add --interactive\" and\n \t\"git-clean --interactive\") when the output is to the terminal.\n-\tWhen false (or `never`), never show colors. The value `always`\n-\tis a historical synonym for `auto`. If unset, then the value of\n-\t`color.ui` is used (`auto` by default).\n+\tWhen false (or `never`), never show colors.\n++\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it means to use color\n+regardless of whether output is to the terminal.\n \n color.interactive.::\n \tUse customized color for 'git add --interactive' and 'git clean\n@@ -1168,18 +1181,24 @@ color.pager::\n \tuse (default is true).\n \n color.showBranch::\n-\tA boolean to enable/disable color in the output of\n-\tlinkgit:git-show-branch[1]. May be set to `always`,\n-\t`false` (or `never`) or `auto` (or `true`), in which case colors are used\n-\tonly when the output is to a terminal. If unset, then the\n-\tvalue of `color.ui` is used (`auto` by default).\n+\tWhen to use color in the output of linkgit:git-show-branch[1].\n+\tMay be set to `never` (or `false`) to disable color or `auto`\n+\t(or `true`) to use colors only when the output is to a terminal.\n+\tIf unset, the value of `color.ui` is used (`auto` by default).\n++\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it is a historical synonym\n+for `--color=always`.\n \n color.status::\n-\tA boolean to enable/disable color in the output of\n-\tlinkgit:git-status[1]. May be set to `always`,\n-\t`false` (or `never`) or `auto` (or `true`), in which case colors are used\n-\tonly when the output is to a terminal. If unset, then the\n-\tvalue of `color.ui` is used (`auto` by default).\n+\tWhen to use color in the output of linkgit:git-status[1].\n+\tMay be set to `never` (or `false`) to disable color or `auto`\n+\t(or `true`) to use colors only when the output is to the terminal.\n+\tIf unset, then the value of `color.ui` is used (`auto` by default).\n++\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it means to use color\n+regardless of whether output is to the terminal.\n \n color.status.::\n \tUse customized color for status colorization. `` is\n@@ -1204,8 +1223,11 @@ color.ui::\n \tcolor unless enabled explicitly with some other configuration\n \tor the `--color` option. Set it to `true` or `auto` to enable\n \tcolor when output is written to the terminal (this is also the\n-\tdefault since Git 1.8.4). The value `always` is a historical\n-\tsynonym for `auto`.\n+\tdefault since Git 1.8.4).\n++\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it means to use color\n+regardless of whether output is to the terminal.\n \n column.ui::\n \tSpecify whether supported commands should output in columns.\n"},{"id":"330239","messageId":"20171012044818.GE155740@aiede.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171012021007.7441-3-gitster@pobox.com","subject":"Re: [PATCH 2/2] color: discourage use of ui.color=always","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-12T04:48:18Z","receivedAt":"2017-10-12T04:48:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Warn when we read such a configuration from a file, and nudge the\n> users to spell them 'auto' instead.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/config.txt | 2 +-\n> color.c | 7 +++++++\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n\nThis warning only kicks in when `always` is being silently downgraded\nto `auto`. It makes sense to me.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"330240","messageId":"xmqqa80x0xcw.fsf@gitster.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171012044724.GD155740@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-12T05:05:35Z","receivedAt":"2017-10-12T05:05:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Should we document this special case treatment of color.* in -c\n> somewhere? E.g.\n\nPerhaps, although I'd save that until we actually add the new option\nto \"git\" potty, which hasn't happened yet, if I were thinking about\nadding some text like that. Also I'd call that --default-color=always\nor something like that, to avoid having to answer: what are the\ndifferences between these two --color thing in the following?\n\n git --color=foo cmd --color=bar\n\n"},{"id":"330242","messageId":"20171012054049.GF155740@aiede.mtv.corp.google.com","threadId":"46942","inReplyTo":"xmqqa80x0xcw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-12T05:40:49Z","receivedAt":"2017-10-12T05:40:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> Should we document this special case treatment of color.* in -c\n>> somewhere? E.g.\n>\n> Perhaps, although I'd save that until we actually add the new option\n> to \"git\" potty, which hasn't happened yet, if I were thinking about\n> adding some text like that. Also I'd call that --default-color=always\n> or something like that, to avoid having to answer: what are the\n> differences between these two --color thing in the following?\n>\n> git --color=foo cmd --color=bar\n\nI agree that the color.status text in the example doc is unfortunate.\nBut the surprising thing I found when writing that doc is that\ncolor.status (\"git status\", \"git commit --dry-run\") and\ncolor.interactive are the only items that needed it (aside from\ncolor.ui that needed it for those two). All the other commands that\nuse color already accept\n\n\tgit cmd --color=bar\n\ncolor.interactive applies to multiple commands (e.g. \"git clean\"), so\nit would take a little more chasing down to make them all use\nOPT__COLOR.\n\nHeading off to sleep, can look more tomorrow.\n\nI don't think we can get around documenting this -c special case\nbehavior, though.\n\ndiff --git i/builtin/commit.c w/builtin/commit.c\nindex d75b3805ea..fc5b7cd538 100644\n--- i/builtin/commit.c\n+++ w/builtin/commit.c\n@@ -1345,6 +1345,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \tstruct object_id oid;\n \tstatic struct option builtin_status_options[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n+\t\tOPT__COLOR(&s.use_color, N_(\"use color\")),\n \t\tOPT_SET_INT('s', \"short\", &status_format,\n \t\t\t N_(\"show status concisely\"), STATUS_FORMAT_SHORT),\n \t\tOPT_BOOL('b', \"branch\", &s.show_branch,\n@@ -1595,6 +1596,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tstatic struct option builtin_commit_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress summary after successful commit\")),\n \t\tOPT__VERBOSE(&verbose, N_(\"show diff in commit message template\")),\n+\t\tOPT__COLOR(&s.use_color, N_(\"use color\")),\n \n \t\tOPT_GROUP(N_(\"Commit message options\")),\n \t\tOPT_FILENAME('F', \"file\", &logfile, N_(\"read message from file\")),\n"},{"id":"330244","messageId":"xmqq1sm828pi.fsf@gitster.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171012054049.GF155740@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-12T06:15:05Z","receivedAt":"2017-10-12T06:15:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>> Jonathan Nieder writes:\n>\n>>> Should we document this special case treatment of color.* in -c\n>>> somewhere? E.g.\n>>\n>> Perhaps, although I'd save that until we actually add the new option\n>> to \"git\" potty, which hasn't happened yet, if I were thinking about\n>> adding some text like that. Also I'd call that --default-color=always\n>> or something like that, to avoid having to answer: what are the\n>> differences between these two --color thing in the following?\n>>\n>> git --color=foo cmd --color=bar\n>\n> I agree that the color.status text in the example doc is unfortunate.\n> But the surprising thing I found when writing that doc is that\n> color.status (\"git status\", \"git commit --dry-run\") and\n> color.interactive are the only items that needed it (aside from\n> color.ui that needed it for those two). All the other commands that\n> use color already accept\n>\n> \tgit cmd --color=bar\n\nAhh, I take it that you mean by \"it\" (in \"needed it\") the \"git\npotty\" option, not a \"--color=\" option individual \"git cmd\"\ntakes? If so, then it makes sense to say \"that's another way to\nspell --color=always from the command line\".\n\nWe need to be able to answer \"why does '-c color.ui=always' work\nonly from the command line?\", but I doubt we want to actively\nencourage the use of it, though, so I dunno.\n\n"},{"id":"330246","messageId":"xmqqwp40zwc5.fsf@gitster.mtv.corp.google.com","threadId":"46942","inReplyTo":"xmqq1sm828pi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-12T06:58:18Z","receivedAt":"2017-10-12T06:58:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> We need to be able to answer \"why does '-c color.ui=always' work\n> only from the command line?\", but I doubt we want to actively\n> encourage the use of it, though, so I dunno.\n\nFor today's pushout, I've queued this as [PATCH 3/2]\n\nThanks..\n\n-- >8 --\nFrom: Jonathan Nieder \nSubject: color: document that \"git -c color.*=always\" is a bit special\nDate: Wed, 11 Oct 2017 21:47:24 -0700\n\nWhen used from the command line as an option to \"git\" potty,\n'always' does not get demoted to 'auto', to help third-party scripts\nthat (ab)used it to override the settings the end-user has.\nDocument it.\n\nWhile at it, clarify description for per-command configuration\nvariables (color.branch, color.grep, color.interactive,\ncolor.showBranch and color.status) so that they can more easily\nshare the new text to talk about this special-casing.\n\nSigned-off-by: Jonathan Nieder \nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 68 ++++++++++++++++++++++++++++++++----------------\n 1 file changed, 45 insertions(+), 23 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ba01b8d3df..f79e82b79a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1051,11 +1051,15 @@ clean.requireForce::\n \t-i or -n. Defaults to true.\n \n color.branch::\n-\tA boolean to enable/disable color in the output of\n-\tlinkgit:git-branch[1]. May be set to `false` (or `never`) to\n-\tdisable color entirely, `auto` (or `true` or `always`) in which\n-\tcase colors are used only when the output is to a terminal. If\n-\tunset, then the value of `color.ui` is used (`auto` by default).\n+\tWhen to use color in the output of linkgit:git-branch[1].\n+\tMay be set to `never` (or `false`) to disable color entirely,\n+\tor `auto` (or `true`) in which case colors are used only when\n+\tthe output is to a terminal. If unset, then the value of\n+\t`color.ui` is used (`auto` by default).\n++\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it is a historical synonym\n+for `--color=always`.\n \n color.branch.::\n \tUse customized color for branch coloration. `` is one of\n@@ -1068,10 +1072,13 @@ color.diff::\n \tWhether to use ANSI escape sequences to add color to patches.\n \tIf this is set to `true` or `auto`, linkgit:git-diff[1],\n \tlinkgit:git-log[1], and linkgit:git-show[1] will use color\n-\twhen output is to the terminal. The value `always` is a\n-\thistorical synonym for `auto`. If unset, then the value of\n+\twhen output is to the terminal. If unset, then the value of\n \t`color.ui` is used (`auto` by default).\n +\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it is a historical\n+synonym for `--color=always`.\n++\n This does not affect linkgit:git-format-patch[1] or the\n 'git-diff-{asterisk}' plumbing commands. Can be overridden on the\n command line with the `--color[=]` option.\n@@ -1091,10 +1098,14 @@ color.decorate.::\n \tbranches, remote-tracking branches, tags, stash and HEAD, respectively.\n \n color.grep::\n-\tWhen set to `always`, always highlight matches. When `false` (or\n+\tWhen to highlight matches using color. When `false` (or\n \t`never`), never. When set to `true` or `auto`, use color only\n \twhen the output is written to the terminal. If unset, then the\n \tvalue of `color.ui` is used (`auto` by default).\n++\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it is a historical synonym\n+for `--color=always`.\n \n color.grep.::\n \tUse customized color for grep colorization. `` specifies which\n@@ -1126,9 +1137,11 @@ color.interactive::\n \tWhen set to `true` or `auto`, use colors for interactive prompts\n \tand displays (such as those used by \"git-add --interactive\" and\n \t\"git-clean --interactive\") when the output is to the terminal.\n-\tWhen false (or `never`), never show colors. The value `always`\n-\tis a historical synonym for `auto`. If unset, then the value of\n-\t`color.ui` is used (`auto` by default).\n+\tWhen false (or `never`), never show colors.\n++\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it means to use color\n+regardless of whether output is to the terminal.\n \n color.interactive.::\n \tUse customized color for 'git add --interactive' and 'git clean\n@@ -1141,18 +1154,24 @@ color.pager::\n \tuse (default is true).\n \n color.showBranch::\n-\tA boolean to enable/disable color in the output of\n-\tlinkgit:git-show-branch[1]. May be set to `always`,\n-\t`false` (or `never`) or `auto` (or `true`), in which case colors are used\n-\tonly when the output is to a terminal. If unset, then the\n-\tvalue of `color.ui` is used (`auto` by default).\n+\tWhen to use color in the output of linkgit:git-show-branch[1].\n+\tMay be set to `never` (or `false`) to disable color or `auto`\n+\t(or `true`) to use colors only when the output is to a terminal.\n+\tIf unset, the value of `color.ui` is used (`auto` by default).\n++\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it is a historical synonym\n+for `--color=always`.\n \n color.status::\n-\tA boolean to enable/disable color in the output of\n-\tlinkgit:git-status[1]. May be set to `always`,\n-\t`false` (or `never`) or `auto` (or `true`), in which case colors are used\n-\tonly when the output is to a terminal. If unset, then the\n-\tvalue of `color.ui` is used (`auto` by default).\n+\tWhen to use color in the output of linkgit:git-status[1].\n+\tMay be set to `never` (or `false`) to disable color or `auto`\n+\t(or `true`) to use colors only when the output is to the terminal.\n+\tIf unset, then the value of `color.ui` is used (`auto` by default).\n++\n+The value `always` is a historical synonym for `auto`, except when\n+passed on the command line using `-c`, where it means to use color\n+regardless of whether output is to the terminal.\n \n color.status.::\n \tUse customized color for status colorization. `` is\n@@ -1177,8 +1196,11 @@ color.ui::\n \tcolor unless enabled explicitly with some other configuration\n \tor the `--color` option. Set it to `true` or `auto` to enable\n \tcolor when output is written to the terminal (this is also the\n-\tdefault since Git 1.8.4). The value `always` is a historical\n-\tsynonym for `auto` and its use is discouraged.\n+\tdefault since Git 1.8.4).\n++\n+The value `always` is a historical synonym for `auto` (and its use is\n+discouraged), except when passed on the command line using `-c`, where\n+it means to use color regardless of whether output is to the terminal.\n \n column.ui::\n \tSpecify whether supported commands should output in columns.\n-- \n2.15.0-rc1-154-g0b692121ee\n\n"},{"id":"330278","messageId":"20171012123153.i265nun6pklw7kjg@sigill.intra.peff.net","threadId":"46942","inReplyTo":"20171012021007.7441-2-gitster@pobox.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-12T12:31:53Z","receivedAt":"2017-10-12T12:32:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 12, 2017 at 11:10:06AM +0900, Junio C Hamano wrote:\n\n> From: Jeff King \n> \n> An earlier patch downgraded \"always\" that comes via the ui.color\n> configuration variable to \"auto\", in order to work around an\n> unfortunate regression to \"git add -i\".\n> \n> That \"fix\" however regressed other third-party tools that depend on\n> \"git -c color.ui=always cmd\" as the way to defeat any end-user\n> configuration and force coloured output from git subcommand, even\n> when the output does not go to a terminal.\n> \n> It is a bit ugly to treat \"-c color.ui=always\" from the command line\n> differently from a definition that comes from on-disk configuration\n> files, but it is a moral equivalent of \"--color=always\" option given\n> to the subcommand from the command line, i.e. a signal that tells us\n> that the script writer knows what s/he is doing. So let's take that\n> route to unbreak this case while defeating a (now declared to be)\n> misguided color.ui that is set to always in the configuration file.\n> \n> NEEDS-SIGN-OFF-BY: Jeff King \n\nSigned-off-by: Jeff King \n\nThanks for picking this up. I meant to get to it yesterday but ran out\nof time. Your description looks good to me.\n\n> color.c | 17 +++++++++++++++--\n> 1 file changed, 15 insertions(+), 2 deletions(-)\n\nWe should probably protect the command-line behavior with a test. Can\nyou squash this in?\n\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex 25a9c65dc5..582cab5c8a 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -261,6 +261,17 @@ test_expect_success 'rev-list %C(auto,...) respects --color' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success \"color.ui=always in config file same as auto\" '\n+\ttest_config color.ui always &&\n+\tgit log --format=$COLOR -1 >actual &&\n+\thas_no_color actual\n+'\n+\n+test_expect_success \"color.ui=always on command-line is always\" '\n+\tgit -c color.ui=always log --format=$COLOR -1 >actual &&\n+\thas_color actual\n+'\n+\n iconv -f utf-8 -t $test_encoding > commit-msg < Junio C Hamano writes:\n> \n> > We need to be able to answer \"why does '-c color.ui=always' work\n> > only from the command line?\", but I doubt we want to actively\n> > encourage the use of it, though, so I dunno.\n> \n> For today's pushout, I've queued this as [PATCH 3/2]\n> \n> Thanks..\n> \n> -- >8 --\n> From: Jonathan Nieder \n> Subject: color: document that \"git -c color.*=always\" is a bit special\n> Date: Wed, 11 Oct 2017 21:47:24 -0700\n\nThis looks reasonable to me to ship in v2.15. I assume we're going to\nleave any \"git --default-color=...\" options to post-release, since we're\nalready in -rc1.\n\n-Peff\n"},{"id":"330287","messageId":"20171012150844.jhdbnckabkbdzi4d@sigill.intra.peff.net","threadId":"46942","inReplyTo":"20171012021007.7441-3-gitster@pobox.com","subject":"Re: [PATCH 2/2] color: discourage use of ui.color=always","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-12T15:08:45Z","receivedAt":"2017-10-12T15:08:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 12, 2017 at 11:10:07AM +0900, Junio C Hamano wrote:\n\n> Warn when we read such a configuration from a file, and nudge the\n> users to spell them 'auto' instead.\n\nHmm. On the one hand, it is nice to make people aware that their config\nisn't doing what they might think.\n\nOn the other hand, if \"always\" is no longer a problem for anybody, do we\nneed to force users to take the step to eradicate it? I dunno. Were we\nplanning to eventually remove it?\n\n> @@ -320,6 +322,11 @@ int git_config_colorbool(const char *var, const char *value)\n> \t\t\t * Otherwise, we're looking at on-disk config;\n> \t\t\t * downgrade to auto.\n> \t\t\t */\n> +\t\t\tif (!warn_once) {\n> +\t\t\t\twarn_once++;\n> +\t\t\t\twarning(\"setting '%s' to '%s' is no longer valid; \"\n> +\t\t\t\t\t\"set it to 'auto' instead\", var, value);\n> +\t\t\t}\n\nThis warn_once is sadly not enough to give non-annoying output to\nscripts that call many git commands. E.g.:\n\n $ git config color.ui always\n $ git add -p\n warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n diff --git a/file b/file\n [...]\n\n-Peff\n"},{"id":"330288","messageId":"20171012151207.liri6qnqasihjg3l@sigill.intra.peff.net","threadId":"46942","inReplyTo":"20171012130649.b4tmofbvdgkevmnn@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-12T15:12:07Z","receivedAt":"2017-10-12T15:12:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 12, 2017 at 09:06:49AM -0400, Jeff King wrote:\n\n> > -- >8 --\n> > From: Jonathan Nieder \n> > Subject: color: document that \"git -c color.*=always\" is a bit special\n> > Date: Wed, 11 Oct 2017 21:47:24 -0700\n> \n> This looks reasonable to me to ship in v2.15. I assume we're going to\n> leave any \"git --default-color=...\" options to post-release, since we're\n> already in -rc1.\n\nAh, I hadn't yet read your cover letter, since I wasn't on the cc for\nthat.\n\nSo yes, the overall plan seems OK to me. I do have a lingering\nreservation that the fact that:\n\n git -c color.ui=always add -p\n\nwill break may come back to bite us. In particular, any such:\n\n git --default-color=always add -p\n\nwill run into the same problem if it is respected by plumbing. But in\ntheory we are free to have it not be so. Arguably we could do the same\nfor \"-c color.ui\", which I guess leaves us an \"out\" to later fix up that\ncase (my, the kludges are certainly piling up on this one).\n\n-Peff\n"},{"id":"330318","messageId":"xmqqmv4vykx0.fsf@gitster.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171012150844.jhdbnckabkbdzi4d@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] color: discourage use of ui.color=always","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-13T00:02:35Z","receivedAt":"2017-10-13T00:02:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Oct 12, 2017 at 11:10:07AM +0900, Junio C Hamano wrote:\n>\n>> Warn when we read such a configuration from a file, and nudge the\n>> users to spell them 'auto' instead.\n>\n> Hmm. On the one hand, it is nice to make people aware that their config\n> isn't doing what they might think.\n>\n> On the other hand, if \"always\" is no longer a problem for anybody, do we\n> need to force users to take the step to eradicate it? I dunno. Were we\n> planning to eventually remove it?\n>\n>> @@ -320,6 +322,11 @@ int git_config_colorbool(const char *var, const char *value)\n>> \t\t\t * Otherwise, we're looking at on-disk config;\n>> \t\t\t * downgrade to auto.\n>> \t\t\t */\n>> +\t\t\tif (!warn_once) {\n>> +\t\t\t\twarn_once++;\n>> +\t\t\t\twarning(\"setting '%s' to '%s' is no longer valid; \"\n>> +\t\t\t\t\t\"set it to 'auto' instead\", var, value);\n>> +\t\t\t}\n>\n> This warn_once is sadly not enough to give non-annoying output to\n> scripts that call many git commands. E.g.:\n>\n> $ git config color.ui always\n> $ git add -p\n> warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n> warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n> warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n> warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n> warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n> warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n> warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n> warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n> warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n> warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead\n> diff --git a/file b/file\n> [...]\n\nI am ambivalent. \n\nWe (especially you) have kept saying that \"always\" is a mistake that\nmakes little sense, and wanted to force users to \"fix\" their\nconfiguration. But as you said, we made it not a mistake, so it is\nOK to leave it as they are, I guess.\n\nLet's drop the warning part of the change, at least.\n\n\n\n"},{"id":"330319","messageId":"xmqqinfjykm2.fsf@gitster.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171012123153.i265nun6pklw7kjg@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-13T00:09:09Z","receivedAt":"2017-10-13T00:09:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... Also\n> as an aside, I think this patch means that:\n>\n> git -c color.ui=always add -p\n>\n> is broken (as would a hypothetical \"git --default-color=always add -p\").\n> That's sufficiently insane that I'm not sure we should care about it.\n\nDo you mean that \"'-c color.ui=always' from the command line is\npassed down to the invocations of 'git' the 'add' command makes, and\nwould break output from 'diff-index' that 'add -i' wants to parse\"?\n\nWith the breakage that motivated \"downgrade only for on-disk\" change\nin mind, I do think that is the right behaviour. Those third-party\nscripts we broke knew how '-c color.ui=always' works and depended on\nit, and I consider that the command line configuration getting\npassed around as an integral part of 'how it works'. \"Fixing\" it\nwill break them again.\n\nLet's take it as a signal that tells us that the script writers know\nwhat they are doing and leave it as a longish rope they can play with.\n\n"},{"id":"330329","messageId":"20171013014721.d4vesqv4v5j7tmk2@sigill.intra.peff.net","threadId":"46942","inReplyTo":"xmqqinfjykm2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-13T01:47:21Z","receivedAt":"2017-10-13T01:47:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 13, 2017 at 09:09:09AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... Also\n> > as an aside, I think this patch means that:\n> >\n> > git -c color.ui=always add -p\n> >\n> > is broken (as would a hypothetical \"git --default-color=always add -p\").\n> > That's sufficiently insane that I'm not sure we should care about it.\n> \n> Do you mean that \"'-c color.ui=always' from the command line is\n> passed down to the invocations of 'git' the 'add' command makes, and\n> would break output from 'diff-index' that 'add -i' wants to parse\"?\n\nYes, exactly.\n\n> With the breakage that motivated \"downgrade only for on-disk\" change\n> in mind, I do think that is the right behaviour. Those third-party\n> scripts we broke knew how '-c color.ui=always' works and depended on\n> it, and I consider that the command line configuration getting\n> passed around as an integral part of 'how it works'. \"Fixing\" it\n> will break them again.\n\nYeah, agreed. We cannot know what the script is expecting, so without\nthat we cannot win, short of turning off color.ui entirely for plumbing.\n\n> Let's take it as a signal that tells us that the script writers know\n> what they are doing and leave it as a longish rope they can play with.\n\nOK. For the record, I'm not against scrapping this whole thing and\ntrying to rollback to your \"plumbing never looks at color.ui\" proposal.\nIt's quite late in the -rc cycle to do that, but there's nothing that\nsays we can't bump the release date if that's what we need to do to get\nit right.\n\nIf we ship v2.15 with the \"color.ui=always really means auto\", I don't\nthink we'd want to undo that. So if we ship with what's in -rc1 (plus\nthis new hack on top) I think that would be fairly final.\n\n-Peff\n"},{"id":"330332","messageId":"xmqqzi8vvht6.fsf@gitster.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171013014721.d4vesqv4v5j7tmk2@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-13T03:37:57Z","receivedAt":"2017-10-13T03:38:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> OK. For the record, I'm not against scrapping this whole thing and\n> trying to rollback to your \"plumbing never looks at color.ui\" proposal.\n> It's quite late in the -rc cycle to do that, but there's nothing that\n> says we can't bump the release date if that's what we need to do to get\n> it right.\n\nI think that it is too late, regardless of our release cycle.\n\n\"Plumbing never looks at color.ui\" implies that \"plumbing must not\nget color.ui=auto from 4c7f1819\", but given that 4c7f1819 is from\n2013, I'd be surprised if we stopped coloring output from plumbing\nwithout getting any complaints from third-party script writers.\n"},{"id":"330353","messageId":"20171013130638.dgc6kawy5mvrbasz@sigill.intra.peff.net","threadId":"46942","inReplyTo":"xmqqzi8vvht6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-13T13:06:38Z","receivedAt":"2017-10-13T13:06:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 13, 2017 at 12:37:57PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > OK. For the record, I'm not against scrapping this whole thing and\n> > trying to rollback to your \"plumbing never looks at color.ui\" proposal.\n> > It's quite late in the -rc cycle to do that, but there's nothing that\n> > says we can't bump the release date if that's what we need to do to get\n> > it right.\n> \n> I think that it is too late, regardless of our release cycle.\n> \n> \"Plumbing never looks at color.ui\" implies that \"plumbing must not\n> get color.ui=auto from 4c7f1819\", but given that 4c7f1819 is from\n> 2013, I'd be surprised if we stopped coloring output from plumbing\n> without getting any complaints from third-party script writers.\n\nI agree that 4c7f1819 is the root of things. But there also weren't a\nlot of people complaining about it. I only noticed it as part of other\nwork I was doing, and (perhaps foolishly) tried to clean it up.\n\nAll of the regressions people have actually _noticed_ stem from my\n136c8c8b8f in v2.14.2. So I think it is a viable option to try to go\nback to the pre-v2.14.2 state. I.e.:\n\n 1. Revert my 20120618c1 (color: downgrade \"always\" to \"auto\" only for\n on-disk configuration, 2017-10-10) and the test changes that came\n with it.\n\n 2. Teach for-each-ref and tag to use git_color_default_config(). These\n are the two I _know_ need this treatment as part of the series that\n contained 136c8c8b8f. As you've noted there may be others, but I'd\n be surprised if there are many. There hasn't been a lot of color\n work in the last few months besides what I've done.\n\n 3. Revert 136c8c8b8f.\n\nThat takes us back to the pre-regression state. The ancient bug from\n4c7f1819 still exists, but that would be OK for v2.15. We'd probably\nwant to bump the -rc cycle a bit to give more confidence that (2) caught\neverything.\n\nPost-release, we would either:\n\n a. Do nothing. As far as we know, nobody has cared deeply about\n 4c7f1819 for the past 4 years.\n\n b. Teach git_default_config() to respect \"never\" but not \"always\", so\n that you can disable the auto-color in the plumbing (but not shoot\n yourself in the foot).\n\n c. Go all-out and remove the \"auto\" behavior from plumbing. This is\n much more likely to have fallouts since we've had 4 years of the\n wrong behavior. But we'd have a whole cycle to identify\n regressions.\n\nI'd probably vote for (b), followed by (a). Option (c) seems like a lot\nof risk for little benefit.\n\nBut we could punt on that part until after the release. The only thing\nwe'd need to decide on now is that first set of reversions. What I\nreally _don't_ want to do is ship v2.15 with \"always works like auto\"\nand then flip that back in v2.16.\n\nI know you're probably infuriated with me because I'm essentially\narguing the opposite of what I did earlier in the cycle. And I really am\nOK with going either way (shipping what's in -rc1 plus the \"let 'always'\nwork from the command line\", or doing something like what I outlined\nabove). But you've convinced me that the road I was going down really is\npiling up the hacks, and I want to make it clear that I'm not married to\nfollowing the path I outlined earlier, and that I think there _is_ a\nviable alternative.\n\n-Peff\n"},{"id":"330370","messageId":"20171013172020.adc2fkddgp3g2ses@sigill.intra.peff.net","threadId":"46942","inReplyTo":"20171013130638.dgc6kawy5mvrbasz@sigill.intra.peff.net","subject":"[PATCH 0/4] peeling back color.ui=always hacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-13T17:20:20Z","receivedAt":"2017-10-13T17:20:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 13, 2017 at 09:06:38AM -0400, Jeff King wrote:\n\n> > I think that it is too late, regardless of our release cycle.\n> > \n> > \"Plumbing never looks at color.ui\" implies that \"plumbing must not\n> > get color.ui=auto from 4c7f1819\", but given that 4c7f1819 is from\n> > 2013, I'd be surprised if we stopped coloring output from plumbing\n> > without getting any complaints from third-party script writers.\n> \n> I agree that 4c7f1819 is the root of things. But there also weren't a\n> lot of people complaining about it. I only noticed it as part of other\n> work I was doing, and (perhaps foolishly) tried to clean it up.\n> \n> All of the regressions people have actually _noticed_ stem from my\n> 136c8c8b8f in v2.14.2. So I think it is a viable option to try to go\n> back to the pre-v2.14.2 state. I.e.:\n> \n> 1. Revert my 20120618c1 (color: downgrade \"always\" to \"auto\" only for\n> on-disk configuration, 2017-10-10) and the test changes that came\n> with it.\n> \n> 2. Teach for-each-ref and tag to use git_color_default_config(). These\n> are the two I _know_ need this treatment as part of the series that\n> contained 136c8c8b8f. As you've noted there may be others, but I'd\n> be surprised if there are many. There hasn't been a lot of color\n> work in the last few months besides what I've done.\n> \n> 3. Revert 136c8c8b8f.\n> \n> That takes us back to the pre-regression state. The ancient bug from\n> 4c7f1819 still exists, but that would be OK for v2.15. We'd probably\n> want to bump the -rc cycle a bit to give more confidence that (2) caught\n> everything.\n\nHere's a series which does that. I'm torn on the correct direction, but\nI took the time to prepare these patches because I wanted to have two\nconcrete alternatives to examine, not hand-waving about what one of the\nsolutions would look like.\n\nI'm adding Jonathan back to the cc as somebody who was interested in the\nwhole sequence (I think these patches should stand alone as explaining\ntheir reasoning, but you may want to look back in the thread a little\nfor context).\n\n [1/4]: Revert \"color: make \"always\" the same as \"auto\" in config\"\n [2/4]: Revert \"t6006: drop \"always\" color config tests\"\n [3/4]: Revert \"color: check color.ui in git_default_config()\"\n [4/4]: tag: respect color.ui config\n\n Documentation/config.txt | 35 ++++++++++++++++++-----------------\n builtin/branch.c | 2 +-\n builtin/clean.c | 3 ++-\n builtin/grep.c | 2 +-\n builtin/show-branch.c | 2 +-\n builtin/tag.c | 2 +-\n color.c | 10 +++++++++-\n config.c | 4 ----\n diff.c | 3 +++\n t/t6006-rev-list-format.sh | 20 +++++++++++++++-----\n t/t6300-for-each-ref.sh | 5 +++++\n t/t7004-tag.sh | 6 ++++++\n 12 files changed, 62 insertions(+), 32 deletions(-)\n\n-Peff\n"},{"id":"330371","messageId":"20171013172323.txugkww6pylwhcnr@sigill.intra.peff.net","threadId":"46942","inReplyTo":"20171013172020.adc2fkddgp3g2ses@sigill.intra.peff.net","subject":"[PATCH 1/4] Revert \"color: make \"always\" the same as \"auto\" in config\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-13T17:23:24Z","receivedAt":"2017-10-13T17:23:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This reverts commit 6be4595edb8e5b616c6e8b9fbc78b0f831fa2a87.\n\nThat commit weakened the \"always\" setting of color config so\nthat it acted as \"auto\". This was meant to solve regressions\nin v2.14.2 in which setting \"color.ui=always\" in the on-disk\nconfig broke scripts like add--interactive, because the\nplumbing diff commands began to generate color output.\n\nThis was due to 136c8c8b8f (color: check color.ui in\ngit_default_config(), 2017-07-13), which was in turn trying\nto fix issues caused by 4c7f1819b3 (make color.ui default to\n'auto', 2013-06-10). But in weakening \"always\", we created\neven more problems, as people expect to be able to use \"git\n-c color.ui=always\" to force color (especially because some\ncommands don't have their own --color flag). We can fix that\nby special-casing the command-line \"-c\", but now things are\ngetting pretty confusing.\n\nInstead of piling hacks upon hacks, let's start peeling off\nthe hacks. The first step is dropping the weakening of\n\"always\", which this revert does.\n\nNote that we could actually revert the whole series merged\nin by da15b78e52642bd45fd5513ab0000fdf2e58a6f4. Most of that\nseries consists of preparations to the tests to handle the\nweakening of \"-c color.ui=always\". But it's worth keeping\nfor a few reasons:\n\n - there are some other preparatory cleanups, like\n e433749d86 (test-terminal: set TERM=vt100, 2017-10-03)\n\n - it adds \"--color\" options more consistently in\n 0c88bf5050 (provide --color option for all ref-filter\n users, 2017-10-03)\n\n - some of the cases dropping \"-c\" end up being more robust\n and realistic tests, as in 01c94e9001 (t7508: use\n test_terminal for color output, 2017-10-03)\n\n - the preferred tool for overriding config is \"--color\",\n and we should be modeling that consistently\n\nWe can individually revert the few commits necessary to\nrestore some useful tests (which will be done on top of this\npatch).\n\nNote that this isn't a pure revert; we'll keep the test\nadded in t3701, but mark it as failure for now.\n\nSigned-off-by: Jeff King \n---\n Documentation/config.txt | 35 ++++++++++++++++++-----------------\n color.c | 2 +-\n t/t3701-add-interactive.sh | 2 +-\n 3 files changed, 20 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex b53c994d0a..1ac0ae6adb 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1058,10 +1058,10 @@ clean.requireForce::\n \n color.branch::\n \tA boolean to enable/disable color in the output of\n-\tlinkgit:git-branch[1]. May be set to `false` (or `never`) to\n-\tdisable color entirely, `auto` (or `true` or `always`) in which\n-\tcase colors are used only when the output is to a terminal. If\n-\tunset, then the value of `color.ui` is used (`auto` by default).\n+\tlinkgit:git-branch[1]. May be set to `always`,\n+\t`false` (or `never`) or `auto` (or `true`), in which case colors are used\n+\tonly when the output is to a terminal. If unset, then the\n+\tvalue of `color.ui` is used (`auto` by default).\n \n color.branch.::\n \tUse customized color for branch coloration. `` is one of\n@@ -1072,11 +1072,12 @@ color.branch.::\n \n color.diff::\n \tWhether to use ANSI escape sequences to add color to patches.\n-\tIf this is set to `true` or `auto`, linkgit:git-diff[1],\n+\tIf this is set to `always`, linkgit:git-diff[1],\n \tlinkgit:git-log[1], and linkgit:git-show[1] will use color\n-\twhen output is to the terminal. The value `always` is a\n-\thistorical synonym for `auto`. If unset, then the value of\n-\t`color.ui` is used (`auto` by default).\n+\tfor all patches. If it is set to `true` or `auto`, those\n+\tcommands will only use color when output is to the terminal.\n+\tIf unset, then the value of `color.ui` is used (`auto` by\n+\tdefault).\n +\n This does not affect linkgit:git-format-patch[1] or the\n 'git-diff-{asterisk}' plumbing commands. Can be overridden on the\n@@ -1140,12 +1141,12 @@ color.grep.::\n --\n \n color.interactive::\n-\tWhen set to `true` or `auto`, use colors for interactive prompts\n+\tWhen set to `always`, always use colors for interactive prompts\n \tand displays (such as those used by \"git-add --interactive\" and\n-\t\"git-clean --interactive\") when the output is to the terminal.\n-\tWhen false (or `never`), never show colors. The value `always`\n-\tis a historical synonym for `auto`. If unset, then the value of\n-\t`color.ui` is used (`auto` by default).\n+\t\"git-clean --interactive\"). When false (or `never`), never.\n+\tWhen set to `true` or `auto`, use colors only when the output is\n+\tto the terminal. If unset, then the value of `color.ui` is\n+\tused (`auto` by default).\n \n color.interactive.::\n \tUse customized color for 'git add --interactive' and 'git clean\n@@ -1192,10 +1193,10 @@ color.ui::\n \tconfiguration to set a default for the `--color` option. Set it\n \tto `false` or `never` if you prefer Git commands not to use\n \tcolor unless enabled explicitly with some other configuration\n-\tor the `--color` option. Set it to `true` or `auto` to enable\n-\tcolor when output is written to the terminal (this is also the\n-\tdefault since Git 1.8.4). The value `always` is a historical\n-\tsynonym for `auto`.\n+\tor the `--color` option. Set it to `always` if you want all\n+\toutput not intended for machine consumption to use color, to\n+\t`true` or `auto` (this is the default since Git 1.8.4) if you\n+\twant such output to use color when written to the terminal.\n \n column.ui::\n \tSpecify whether supported commands should output in columns.\ndiff --git a/color.c b/color.c\nindex 9c0dc82370..9ccd954d6b 100644\n--- a/color.c\n+++ b/color.c\n@@ -308,7 +308,7 @@ int git_config_colorbool(const char *var, const char *value)\n \t\tif (!strcasecmp(value, \"never\"))\n \t\t\treturn 0;\n \t\tif (!strcasecmp(value, \"always\"))\n-\t\t\treturn var ? GIT_COLOR_AUTO : 1;\n+\t\t\treturn 1;\n \t\tif (!strcasecmp(value, \"auto\"))\n \t\t\treturn GIT_COLOR_AUTO;\n \t}\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex a49c12c79b..87ffd4d43c 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -483,7 +483,7 @@ test_expect_success 'hunk-editing handles custom comment char' '\n \tgit diff --exit-code\n '\n \n-test_expect_success 'add -p works even with color.ui=always' '\n+test_expect_failure 'add -p works even with color.ui=always' '\n \tgit reset --hard &&\n \techo change >>file &&\n \ttest_config color.ui always &&\n-- \n2.15.0.rc1.395.ga4290b5804\n\n"},{"id":"330372","messageId":"20171013172341.kdxckcl4mum5wsoq@sigill.intra.peff.net","threadId":"46942","inReplyTo":"20171013172020.adc2fkddgp3g2ses@sigill.intra.peff.net","subject":"[PATCH 2/4] Revert \"t6006: drop \"always\" color config tests\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-13T17:23:41Z","receivedAt":"2017-10-13T17:23:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This reverts commit c5bdfe677cfab5b2e87771c35565d44d3198efda.\n\nThat commit was done primarily to prepare for the weakening\nof \"always\" in 6be4595edb (color: make \"always\" the same as\n\"auto\" in config, 2017-10-03). But since we've now reverted\n6be4595edb, there's no need for us to remove \"-c\ncolor.ui=always\" from the tests. And in fact it's a good\nidea to restore these tests, to make sure that \"always\"\ncontinues to work.\n\nSigned-off-by: Jeff King \n---\n t/t6006-rev-list-format.sh | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex 25a9c65dc5..98be78b4a2 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -208,11 +208,26 @@ do\n \t\thas_no_color actual\n \t'\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+\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 \ttest_expect_success \"$desc respects --color\" '\n \t\tgit log --format=$color -1 --color >actual &&\n \t\thas_color actual\n \t'\n \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+\n \ttest_expect_success TTY \"$desc respects --color=auto (stdout is tty)\" '\n \t\ttest_terminal git log --format=$color -1 --color=auto >actual &&\n \t\thas_color actual\n@@ -225,11 +240,6 @@ do\n \t\t\thas_no_color actual\n \t\t)\n \t'\n-\n-\ttest_expect_success TTY \"$desc respects --no-color\" '\n-\t\ttest_terminal git log --format=$color -1 --no-color >actual &&\n-\t\thas_no_color actual\n-\t'\n done\n \n test_expect_success '%C(always,...) enables color even without tty' '\n-- \n2.15.0.rc1.395.ga4290b5804\n\n"},{"id":"330373","messageId":"20171013172431.7v6voqt2qd6suhno@sigill.intra.peff.net","threadId":"46942","inReplyTo":"20171013172020.adc2fkddgp3g2ses@sigill.intra.peff.net","subject":"[PATCH 3/4] Revert \"color: check color.ui in git_default_config()\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-13T17:24:31Z","receivedAt":"2017-10-13T17:24:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This reverts commit 136c8c8b8fa39f1315713248473dececf20f8fe7.\n\nThat commit was trying to address a bug caused by 4c7f1819b3\n(make color.ui default to 'auto', 2013-06-10), in which\nplumbing like diff-tree defaulted to \"auto\" color, but did\nnot respect a \"color.ui\" directive to disable it.\n\nBut it also meant that we started respecting \"color.ui\" set\nto \"always\". This was a known problem, but 4c7f1819b3 argued\nthat nobody ought to be doing that. However, that turned out\nto be wrong, and we got a number of bug reports related to\n\"add -p\" regressing in v2.14.2.\n\nLet's revert 136c8c8b8, fixing the regression to \"add -p\".\nThis leaves the problem from 4c7f1819b3 unfixed, but:\n\n 1. It's a pretty obscure problem in the first place. I\n only noticed it while working on the color code, and we\n haven't got a single bug report or complaint about it.\n\n 2. We can make a more moderate fix on top by respecting\n \"never\" but not \"always\" for plumbing commands. This\n is just the minimal fix to go back to the working state\n we had before v2.14.2.\n\nNote that this isn't a pure revert. We now have a test in\nt3701 which shows off the \"add -p\" regression. This can be\nflipped to success.\n\nSigned-off-by: Jeff King \n---\n builtin/branch.c | 2 +-\n builtin/clean.c | 3 ++-\n builtin/grep.c | 2 +-\n builtin/show-branch.c | 2 +-\n color.c | 8 ++++++++\n config.c | 4 ----\n diff.c | 3 +++\n t/t3701-add-interactive.sh | 2 +-\n 8 files changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex b67593288c..79dc9181fd 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -93,7 +93,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)\n \t\t\treturn config_error_nonbool(var);\n \t\treturn color_parse(value, branch_colors[slot]);\n \t}\n-\treturn git_default_config(var, value, cb);\n+\treturn git_color_default_config(var, value, cb);\n }\n \n static const char *branch_get_color(enum color_branch ix)\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 733b6d3745..189e20628c 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -126,7 +126,8 @@ static int git_clean_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\treturn git_default_config(var, value, cb);\n+\t/* inspect the color.ui config variable and others */\n+\treturn git_color_default_config(var, value, cb);\n }\n \n static const char *clean_get_color(enum color_clean ix)\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 19e23946ac..2d65f27d01 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -275,7 +275,7 @@ static int wait_all(void)\n static int grep_cmd_config(const char *var, const char *value, void *cb)\n {\n \tint st = grep_config(var, value, cb);\n-\tif (git_default_config(var, value, cb) < 0)\n+\tif (git_color_default_config(var, value, cb) < 0)\n \t\tst = -1;\n \n \tif (!strcmp(var, \"grep.threads\")) {\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 84547d6fba..6fa1f62a88 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -554,7 +554,7 @@ static int git_show_branch_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\treturn git_default_config(var, value, cb);\n+\treturn git_color_default_config(var, value, cb);\n }\n \n static int omit_in_dense(struct commit *commit, struct commit **rev, int n)\ndiff --git a/color.c b/color.c\nindex 9ccd954d6b..9a9261ac16 100644\n--- a/color.c\n+++ b/color.c\n@@ -368,6 +368,14 @@ int git_color_config(const char *var, const char *value, void *cb)\n \treturn 0;\n }\n \n+int git_color_default_config(const char *var, const char *value, void *cb)\n+{\n+\tif (git_color_config(var, value, cb) < 0)\n+\t\treturn -1;\n+\n+\treturn git_default_config(var, value, cb);\n+}\n+\n void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb)\n {\n \tif (*color)\ndiff --git a/config.c b/config.c\nindex 4831c12735..adb7d7a3e5 100644\n--- a/config.c\n+++ b/config.c\n@@ -16,7 +16,6 @@\n #include \"string-list.h\"\n #include \"utf8.h\"\n #include \"dir.h\"\n-#include \"color.h\"\n \n struct config_source {\n \tstruct config_source *prev;\n@@ -1351,9 +1350,6 @@ int git_default_config(const char *var, const char *value, void *dummy)\n \tif (starts_with(var, \"advice.\"))\n \t\treturn git_default_advice_config(var, value);\n \n-\tif (git_color_config(var, value, dummy) < 0)\n-\t\treturn -1;\n-\n \tif (!strcmp(var, \"pager.color\") || !strcmp(var, \"color.pager\")) {\n \t\tpager_use_color = git_config_bool(var,value);\n \t\treturn 0;\ndiff --git a/diff.c b/diff.c\nindex 69f03570ad..30b2e271b0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -358,6 +358,9 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (git_color_config(var, value, cb) < 0)\n+\t\treturn -1;\n+\n \treturn git_diff_basic_config(var, value, cb);\n }\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 87ffd4d43c..a49c12c79b 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -483,7 +483,7 @@ test_expect_success 'hunk-editing handles custom comment char' '\n \tgit diff --exit-code\n '\n \n-test_expect_failure 'add -p works even with color.ui=always' '\n+test_expect_success 'add -p works even with color.ui=always' '\n \tgit reset --hard &&\n \techo change >>file &&\n \ttest_config color.ui always &&\n-- \n2.15.0.rc1.395.ga4290b5804\n\n"},{"id":"330374","messageId":"20171013172601.spqiyuutxs4fmlgz@sigill.intra.peff.net","threadId":"46942","inReplyTo":"20171013172020.adc2fkddgp3g2ses@sigill.intra.peff.net","subject":"[PATCH 4/4] tag: respect color.ui config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-13T17:26:02Z","receivedAt":"2017-10-13T17:27:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 11b087adfd (ref-filter: consult want_color() before\nemitting colors, 2017-07-13), we expect that setting\n\"color.ui\" to \"always\" will enable color tag formats even\nwithout a tty. As that commit was built on top of\n136c8c8b8f (color: check color.ui in git_default_config(),\n2017-07-13) from the same series, we didn't need to touch\ntag's config parsing at all.\n\nHowever, since we reverted 136c8c8b8f, we now need to\nexplicitly call git_color_default_config() to make this\nwork.\n\nLet's do so, and also restore the test dropped in 0c88bf5050\n(provide --color option for all ref-filter users,\n2017-10-03). That commit swapped out our \"color.ui=always\"\ntest for \"--color\" in preparation for \"always\" going away.\nBut since it is here to stay, we should test both cases.\n\nNote that for-each-ref also lost its color.ui support as\npart of reverting 136c8c8b8f. But as a plumbing command, it\nshould _not_ respect the color.ui config. Since it also\ngained a --color option in 0c88bf5050, that's the correct\nway to ask it for color. We'll continue to test that, and\nconfirm that \"color.ui\" is not respected.\n\nSigned-off-by: Jeff King \n---\n builtin/tag.c | 2 +-\n t/t6300-for-each-ref.sh | 5 +++++\n t/t7004-tag.sh | 6 ++++++\n 3 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 695cb0778e..b38329b593 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -158,7 +158,7 @@ static int git_tag_config(const char *var, const char *value, void *cb)\n \n \tif (starts_with(var, \"column.\"))\n \t\treturn git_column_config(var, value, \"tag\", &colopts);\n-\treturn git_default_config(var, value, cb);\n+\treturn git_color_default_config(var, value, cb);\n }\n \n static void write_tag_body(int fd, const struct object_id *oid)\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 416ff7d0b8..3aa534933e 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -442,6 +442,11 @@ test_expect_success '--color can override tty check' '\n \ttest_cmp expected.color actual\n '\n \n+test_expect_success 'color.ui=always does not override tty check' '\n+\tgit -c color.ui=always for-each-ref --format=\"$color_format\" >actual &&\n+\ttest_cmp expected.bare actual\n+'\n+\n cat >expected <<\\EOF\n heads/master\n tags/master\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 4e62c505fc..a9af2de996 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1918,6 +1918,12 @@ test_expect_success '--color overrides auto-color' '\n \ttest_cmp expect.color actual\n '\n \n+test_expect_success 'color.ui=always overrides auto-color' '\n+\tgit -c color.ui=always tag $color_args >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect.color actual\n+'\n+\n test_expect_success 'setup --merged test tags' '\n \tgit tag mergetest-1 HEAD~2 &&\n \tgit tag mergetest-2 HEAD~1 &&\n-- \n2.15.0.rc1.395.ga4290b5804\n"},{"id":"330391","messageId":"xmqqshemtoth.fsf@gitster.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171013130638.dgc6kawy5mvrbasz@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-14T03:01:46Z","receivedAt":"2017-10-14T03:02:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> All of the regressions people have actually _noticed_ stem from my\n> 136c8c8b8f in v2.14.2. So I think it is a viable option to try to go\n> back to the pre-v2.14.2 state. I.e.:\n> ...\n> That takes us back to the pre-regression state. The ancient bug from\n> 4c7f1819 still exists, but that would be OK for v2.15. We'd probably\n> want to bump the -rc cycle a bit to give more confidence that (2) caught\n> everything.\n\nYes, I think that is the approach I was pushing initially with the\njc/ref-filter-colors-fix topic that was later retracted; the result\nof your 4-patch series more or less matches that one, modulo that I\ndidn't treat for-each-ref as a plumbing. I do share the worry that\nit is hard to make sure that these post-revert adjustment caught\neverything; after all, that was a major part of the reason why my\nearlier attempt was retracted. I still think this is the _right_\ndirection to go in, even though it is harder to get right.\n\n> Post-release, we would either:\n> ...\n> But we could punt on that part until after the release. The only thing\n> we'd need to decide on now is that first set of reversions. What I\n> really _don't_ want to do is ship v2.15 with \"always works like auto\"\n> and then flip that back in v2.16.\n\nTrue. Let's see what others think. I know Jonathan is running\nthe fork at $work with \"downgrade always to auto\" patches, and while\nI think both approaches would probably work well in practice, I have\npreference for this \"harder but right\" approach, so I'd want to see\ndifferent views discussed on the list before we decide.\n\nThanks.\n"},{"id":"330483","messageId":"20171016215311.m72jarmqhjagy6o6@sigill.intra.peff.net","threadId":"46942","inReplyTo":"xmqqshemtoth.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-16T21:53:11Z","receivedAt":"2017-10-16T21:53:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 14, 2017 at 12:01:46PM +0900, Junio C Hamano wrote:\n\n> > That takes us back to the pre-regression state. The ancient bug from\n> > 4c7f1819 still exists, but that would be OK for v2.15. We'd probably\n> > want to bump the -rc cycle a bit to give more confidence that (2) caught\n> > everything.\n> \n> Yes, I think that is the approach I was pushing initially with the\n> jc/ref-filter-colors-fix topic that was later retracted; the result\n> of your 4-patch series more or less matches that one, modulo that I\n> didn't treat for-each-ref as a plumbing.\n\nAh, right, I forgot about that one while I was putting it together. So\nmany alternatives floating around.\n\n> I do share the worry that\n> it is hard to make sure that these post-revert adjustment caught\n> everything; after all, that was a major part of the reason why my\n> earlier attempt was retracted. I still think this is the _right_\n> direction to go in, even though it is harder to get right.\n\nTo be honest, I'm not actually very worried. I think missing a\npost-revert adjustment is the main risk, but my general sense is that\nthere hasn't been a lot going on with color fixes outside of my recent\nwork. Famous last words and all that, I'm sure. :)\n\n> True. Let's see what others think. I know Jonathan is running\n> the fork at $work with \"downgrade always to auto\" patches, and while\n> I think both approaches would probably work well in practice, I have\n> preference for this \"harder but right\" approach, so I'd want to see\n> different views discussed on the list before we decide.\n\nAfter pondering over it, I have a slight preference for that, too. But\nI'm also happy to hear more input.\n\n-Peff\n"},{"id":"330496","messageId":"xmqqo9p6r3ai.fsf@gitster.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171016215311.m72jarmqhjagy6o6@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-17T01:06:29Z","receivedAt":"2017-10-17T01:06:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Oct 14, 2017 at 12:01:46PM +0900, Junio C Hamano wrote:\n>\n>> > That takes us back to the pre-regression state. The ancient bug from\n>> > 4c7f1819 still exists, but that would be OK for v2.15. We'd probably\n>> > want to bump the -rc cycle a bit to give more confidence that (2) caught\n>> > everything.\n>> \n>> Yes, I think that is the approach I was pushing initially with the\n>> jc/ref-filter-colors-fix topic that was later retracted; the result\n>> of your 4-patch series more or less matches that one, modulo that I\n>> didn't treat for-each-ref as a plumbing.\n>\n> Ah, right, I forgot about that one while I was putting it together. So\n> many alternatives floating around.\n>\n>> I do share the worry that\n>> it is hard to make sure that these post-revert adjustment caught\n>> everything; after all, that was a major part of the reason why my\n>> earlier attempt was retracted. I still think this is the _right_\n>> direction to go in, even though it is harder to get right.\n>\n> To be honest, I'm not actually very worried. I think missing a\n> post-revert adjustment is the main risk, but my general sense is that\n> there hasn't been a lot going on with color fixes outside of my recent\n> work. Famous last words and all that, I'm sure. :)\n>\n>> True. Let's see what others think. I know Jonathan is running\n>> the fork at $work with \"downgrade always to auto\" patches, and while\n>> I think both approaches would probably work well in practice, I have\n>> preference for this \"harder but right\" approach, so I'd want to see\n>> different views discussed on the list before we decide.\n>\n> After pondering over it, I have a slight preference for that, too. But\n> I'm also happy to hear more input.\n\nOK, so it seems we both have slight preference for the \"peel back\"\napproach. Adding Jonathan to Cc:\n"},{"id":"330522","messageId":"xmqqvajenvce.fsf@gitster.mtv.corp.google.com","threadId":"46942","inReplyTo":"xmqqo9p6r3ai.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-17T06:26:25Z","receivedAt":"2017-10-17T06:26:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> After pondering over it, I have a slight preference for that, too. But\n>> I'm also happy to hear more input.\n>\n> OK, so it seems we both have slight preference for the \"peel back\"\n> approach. Adding Jonathan to Cc:\n\nIt was a bit more painful than necessary to make sure I have\nsomething that can be merged for 2.14.x maintenance track, but I\nthink the topic is now in a reasonable shape, and I've merged it to\n'next'. On the first-parent chain from 'master' to 'pu', the merge\nof this topic is the very first one, and after reading it over once\nagain, I think this is OK.\n\nThanks.\n"},{"id":"330524","messageId":"20171017065101.ismnplaynumt5bdh@aiede.mtv.corp.google.com","threadId":"46942","inReplyTo":"xmqqo9p6r3ai.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-17T06:51:01Z","receivedAt":"2017-10-17T06:51:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n> Jeff King writes:\n>> On Sat, Oct 14, 2017 at 12:01:46PM +0900, Junio C Hamano wrote:\n\n>>> True. Let's see what others think. I know Jonathan is running\n>>> the fork at $work with \"downgrade always to auto\" patches, and while\n>>> I think both approaches would probably work well in practice, I have\n>>> preference for this \"harder but right\" approach, so I'd want to see\n>>> different views discussed on the list before we decide.\n>>\n>> After pondering over it, I have a slight preference for that, too. But\n>> I'm also happy to hear more input.\n>\n> OK, so it seems we both have slight preference for the \"peel back\"\n> approach. Adding Jonathan to Cc:\n\nWhich approach is \"harder but right\" / \"peel back\"?\n\nI agree with the goal of making color.ui=always a synonym for auto in\nfile-based config. Peff found some problems with the warning patch\n(scripted commands produce too many warnings), which are not an issue\nfor $dayjob but may be for upstream, so I see the value of holding off\non the warning for now.\n\nI'm also fine with \"revert the proximate cause of the latest\ncomplaints\" as a stepping stone toward making color.ui=always a\nsynonym for auto in file-based config in a later release.\n\nAnother issue is diff-files paying attention to this configuration.\nIf I'm reading Documentation/config.txt correctly, that was simply a\nbug. diff-files and diff-index are never supposed to use color,\nregardless of configuration.\n\nI'm fine with \"revert the proximate cause of the latest complaints\" as\na stepping stone toward fixing that, too. :)\n\nSorry I don't have more detailed advice. I was planning to look more\nclosely at how these features evolved over time and haven't had enough\ntime for it yet.\n\nJonathan\n"},{"id":"330594","messageId":"20171018052845.y54l6cd3dz64l5i4@sigill.intra.peff.net","threadId":"46942","inReplyTo":"xmqqvajenvce.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-18T05:28:45Z","receivedAt":"2017-10-18T05:28:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 17, 2017 at 03:26:25PM +0900, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Jeff King writes:\n> >\n> >> After pondering over it, I have a slight preference for that, too. But\n> >> I'm also happy to hear more input.\n> >\n> > OK, so it seems we both have slight preference for the \"peel back\"\n> > approach. Adding Jonathan to Cc:\n> \n> It was a bit more painful than necessary to make sure I have\n> something that can be merged for 2.14.x maintenance track, but I\n> think the topic is now in a reasonable shape, and I've merged it to\n> 'next'. On the first-parent chain from 'master' to 'pu', the merge\n> of this topic is the very first one, and after reading it over once\n> again, I think this is OK.\n\nHmm. I think you would just want the top two commits for maint-2.14\n(reverting 136c8c8b8f and fixing up git-tag to check color config). But\nof course you can't do a partial merge because they come on top of the\nother dead-end/revert pair. You'd have to cherry-pick (and even then fix\nup a few bits, like adding in the \"add -p\" test).\n\nThough if we take all of jk/ui-color-always-to-auto-maint, and then do\nthe whole reversion on top of that, I think that should work. It just\ndoesn't look like that topic ever made it to \"maint\" (I see mention of a\njk/ref-filter-colors-fix-maint in the log for master, but there's no\nsuch branch).\n\nI started to prepare a patch directly on v2.14.2 just to see what it\nwould look like. The first one (the revert) is fine, but we then have to\nfixup tag and for-each-ref. And since they don't have --color added by\nthe dead-end fixups, the tests get harder...\n\n-Peff\n"},{"id":"330595","messageId":"20171018053413.j2h63tq2qpkfev3x@sigill.intra.peff.net","threadId":"46942","inReplyTo":"20171017065101.ismnplaynumt5bdh@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-18T05:34:13Z","receivedAt":"2017-10-18T05:34:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 16, 2017 at 11:51:01PM -0700, Jonathan Nieder wrote:\n\n> > OK, so it seems we both have slight preference for the \"peel back\"\n> > approach. Adding Jonathan to Cc:\n> \n> Which approach is \"harder but right\" / \"peel back\"?\n\n\"peel back\" is reverting back to the pre-v2.14.2 state (Junio has the\npatches queued in jk/ref-filter-colors-fix).\n\n> I agree with the goal of making color.ui=always a synonym for auto in\n> file-based config. Peff found some problems with the warning patch\n> (scripted commands produce too many warnings), which are not an issue\n> for $dayjob but may be for upstream, so I see the value of holding off\n> on the warning for now.\n> \n> I'm also fine with \"revert the proximate cause of the latest\n> complaints\" as a stepping stone toward making color.ui=always a\n> synonym for auto in file-based config in a later release.\n\nI do think \"color.ui=always\" is a foot-gun, but I wasn't happy with the\nnumber of weird hacks we ended up with in trying to fix that (like \"it\nmeans one thing in the on-disk config and another thing with \"git -c\").\n\nWe can take it up as a topic post-release. At the very least, I think we\nwill want to change the documentation to make it more clear that you\nalmost certainly _don't_ want to use \"always\".\n\n-Peff\n"},{"id":"330596","messageId":"xmqq4lqxknfx.fsf@gitster.mtv.corp.google.com","threadId":"46942","inReplyTo":"20171018052845.y54l6cd3dz64l5i4@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] color: downgrade \"always\" to \"auto\" only for on-disk configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-18T05:57:38Z","receivedAt":"2017-10-18T05:57:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> It was a bit more painful than necessary to make sure I have\n>> something that can be merged for 2.14.x maintenance track, but I\n>> think the topic is now in a reasonable shape, and I've merged it to\n>> 'next'. On the first-parent chain from 'master' to 'pu', the merge\n>> of this topic is the very first one, and after reading it over once\n>> again, I think this is OK.\n>\n> Hmm. I think you would just want the top two commits for maint-2.14\n> (reverting 136c8c8b8f and fixing up git-tag to check color config). But\n> of course you can't do a partial merge because they come on top of the\n> other dead-end/revert pair. You'd have to cherry-pick (and even then fix\n> up a few bits, like adding in the \"add -p\" test).\n>\n> Though if we take all of jk/ui-color-always-to-auto-maint, and then do\n> the whole reversion on top of that, I think that should work. It just\n> doesn't look like that topic ever made it to \"maint\" (I see mention of a\n> jk/ref-filter-colors-fix-maint in the log for master, but there's no\n> such branch).\n\nYeah, that is what ended up to be jk/ref-filter-colors-fix; the\nbranch merges cleanly to 'master', but also to 'maint' without\ndragging the rest of the recent development along with it---I did a\nrebase before sending out the message you are responding to.\n"}]}