* What happened to "git status --color=(always|auto|never)"? @ 2017-10-09 23:58 Nazri Ramliy 2017-10-10 0:16 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Nazri Ramliy @ 2017-10-09 23:58 UTC (permalink / raw) To: Git Mailing List I used to work before, but now: $ git version git version 2.15.0.rc0.39.g2f0e14e649 $ git status --color=always error: unknown option `color=always' usage: git status [<options>] [--] <pathspec>... Is it no longer supported? nazri ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What happened to "git status --color=(always|auto|never)"? 2017-10-09 23:58 What happened to "git status --color=(always|auto|never)"? Nazri Ramliy @ 2017-10-10 0:16 ` Jonathan Nieder 2017-10-10 0:43 ` Nazri Ramliy 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2017-10-10 0:16 UTC (permalink / raw) To: Nazri Ramliy; +Cc: Git Mailing List Hi, Nazri Ramliy wrote: > I used to work before, but now: > > $ git version > git version 2.15.0.rc0.39.g2f0e14e649 > > $ git status --color=always > error: unknown option `color=always' > usage: git status [<options>] [--] <pathspec>... Which version did it work in? That would allow me to bisect. Thanks, Jonathan ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What happened to "git status --color=(always|auto|never)"? 2017-10-10 0:16 ` Jonathan Nieder @ 2017-10-10 0:43 ` Nazri Ramliy 2017-10-10 0:59 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Nazri Ramliy @ 2017-10-10 0:43 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Git Mailing List On Tue, Oct 10, 2017 at 8:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Hi, > > Nazri Ramliy wrote: > >> I used to work before, but now: >> >> $ git version >> git version 2.15.0.rc0.39.g2f0e14e649 >> >> $ git status --color=always >> error: unknown option `color=always' >> usage: git status [<options>] [--] <pathspec>... > > Which version did it work in? That would allow me to bisect. Sorry. It's my bad. I must have confused this with `git grep`'s --color option. I have a perl script that shells out to `git -c color.status=always status`, and I wrongly documented that the perl script's `--color=(always|never|auto)` is "similar to git-status' option". I verified this down to git-2.1.0, git-1.7.0 and git-1.4.0 confirmed that `git status` do not support --color option then, and mostly likely not in the versions between. nazri ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What happened to "git status --color=(always|auto|never)"? 2017-10-10 0:43 ` Nazri Ramliy @ 2017-10-10 0:59 ` Jonathan Nieder 2017-10-10 4:42 ` Nazri Ramliy 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2017-10-10 0:59 UTC (permalink / raw) To: Nazri Ramliy; +Cc: Git Mailing List Nazri Ramliy wrote: > On Tue, Oct 10, 2017 at 8:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote: >> Nazri Ramliy wrote: >>> I used to work before, but now: >>> >>> $ git version >>> git version 2.15.0.rc0.39.g2f0e14e649 >>> >>> $ git status --color=always >>> error: unknown option `color=always' >>> usage: git status [<options>] [--] <pathspec>... >> >> Which version did it work in? That would allow me to bisect. > > Sorry. It's my bad. I must have confused this with `git grep`'s --color option. No problem. It sounds like a reasonable feature request to me, especially now that we are about to drop support for color.status=always in configuration: commit 6be4595edb8e5b616c6e8b9fbc78b0f831fa2a87 Author: Jeff King <peff@peff.net> Date: Tue Oct 3 09:46:06 2017 -0400 color: make "always" the same as "auto" in config Would you like to take a stab at adding it? builtin/commit.c and Documentation/git-{commit,status}.txt would be my best guesses at where to start. Thanks, Jonathan ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What happened to "git status --color=(always|auto|never)"? 2017-10-10 0:59 ` Jonathan Nieder @ 2017-10-10 4:42 ` Nazri Ramliy 2017-10-10 10:25 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: Nazri Ramliy @ 2017-10-10 4:42 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Git Mailing List, Jeff King > commit 6be4595edb8e5b616c6e8b9fbc78b0f831fa2a87 > Author: Jeff King <peff@peff.net> > Date: Tue Oct 3 09:46:06 2017 -0400 > > color: make "always" the same as "auto" in config > > Would you like to take a stab at adding it? builtin/commit.c and > Documentation/git-{commit,status}.txt would be my best guesses at > where to start. Perhaps, seeing that that commit intentionally "broke" the color output of my tool[1], because it parses the output of `git -c color.status=always status`, which now no longer work the way it used to. I know, I know... shame on me for parsing the output of a porcelain command :) But this also means that I will have to modify [1] to cope with this, given that it may be used with an older version of git (parse git-version and shell out to different git command - either `git -c color.ui=always status`, or the not-yet-exist `git status --color=always`), or make it use the plumbing output of `git status`, but that would just add additional work that I really don't look forward to doing at this moment. nazri [1] https://github.com/holygeek/git-number This tool (naively) parses the porcelain output out `git -c color.status=always status` in order to insert numbers for each filenames that `git status` prints. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What happened to "git status --color=(always|auto|never)"? 2017-10-10 4:42 ` Nazri Ramliy @ 2017-10-10 10:25 ` Jeff King 2017-10-10 12:51 ` Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2017-10-10 10:25 UTC (permalink / raw) To: Nazri Ramliy; +Cc: Jonathan Nieder, Git Mailing List On Tue, Oct 10, 2017 at 12:42:43PM +0800, Nazri Ramliy wrote: > > commit 6be4595edb8e5b616c6e8b9fbc78b0f831fa2a87 > > Author: Jeff King <peff@peff.net> > > Date: Tue Oct 3 09:46:06 2017 -0400 > > > > color: make "always" the same as "auto" in config > > > > Would you like to take a stab at adding it? builtin/commit.c and > > Documentation/git-{commit,status}.txt would be my best guesses at > > where to start. > > Perhaps, seeing that that commit intentionally "broke" the color > output of my tool[1], because it parses the output of `git -c > color.status=always status`, which now no longer work the way it used > to. I know, I know... shame on me for parsing the output of a > porcelain command :) > > But this also means that I will have to modify [1] to cope with this, > given that it may be used with an older version of git (parse > git-version and shell out to different git command - either `git -c > color.ui=always status`, or the not-yet-exist `git status > --color=always`), or make it use the plumbing output of `git status`, > but that would just add additional work that I really don't look > forward to doing at this moment. :( I was worried that this might hit some third-party scripts. One workaround you can do that should work with any version of Git is: GIT_PAGER_IN_USE=1 git status | your-parser That tells Git that even though stdout isn't a tty, you're expecting to present the data to the user and it should be colored appropriately. It has the additional upside that it doesn't override the user's color config. It does have the potential downside that other non-color changes could kick in (e.g., somebody recently proposed that auto-columns kick in with GIT_PAGER_IN_USE). All that said, should we revisit the decision from 6be4595edb? The two code changes we could make are: 1. Adding a "--color" option to "git status". Commit 0c88bf5050 (provide --color option for all ref-filter users, 2017-10-03) from that same series shows some prior art. This is a clean solution, but it does mean that scripts have to adapt (and would potentially need to care about which Git version they're relying on). 2. Re-allow "color.always" config from the command-line. It's actually on-disk config that we want to downgrade, but I wanted to avoid making complicated rules about how the config would behave in different scopes. The patch for this would look something like the one below. 3. Revert the original series, and revisit the original "respect color.ui via porcelain" commit which broke add--interactive in v2.14.2 (136c8c8b8fa). I dunno. I think for your use case, PAGER_IN_USE may actually be the "right" solution, because it most closely expresses what you're doing. We probably ought to have (1) as a general rule for commands which handle color. But (2) and (3) are the only ones that will work seamlessly with existing scripts. I'm not excited about either of them, though. -Peff diff --git a/color.c b/color.c index 9c0dc82370..3870d3e395 100644 --- a/color.c +++ b/color.c @@ -307,8 +307,21 @@ int git_config_colorbool(const char *var, const char *value) if (value) { if (!strcasecmp(value, "never")) return 0; - if (!strcasecmp(value, "always")) - return var ? GIT_COLOR_AUTO : 1; + if (!strcasecmp(value, "always")) { + /* + * Command-line options always respect "always". + * Likewise for "-c" config on the command-line. + */ + if (!var || + current_config_scope() == CONFIG_SCOPE_CMDLINE) + return 1; + + /* + * Otherwise, we're looking at on-disk config; + * downgrade to auto. + */ + return GIT_COLOR_AUTO; + } if (!strcasecmp(value, "auto")) return GIT_COLOR_AUTO; } ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: What happened to "git status --color=(always|auto|never)"? 2017-10-10 10:25 ` Jeff King @ 2017-10-10 12:51 ` Junio C Hamano 2017-10-10 13:06 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-10 12:51 UTC (permalink / raw) To: Jeff King; +Cc: Nazri Ramliy, Jonathan Nieder, Git Mailing List Jeff King <peff@peff.net> writes: > :( I was worried that this might hit some third-party scripts. > ... > All that said, should we revisit the decision from 6be4595edb? The two > code changes we could make are: > > 1. Adding a "--color" option to "git status". Commit 0c88bf5050 > (provide --color option for all ref-filter users, 2017-10-03) from > that same series shows some prior art. > > This is a clean solution, but it does mean that scripts have to > adapt (and would potentially need to care about which Git version > they're relying on). If we view that "always" issue is a regression, then this is not a "solution". It is a part of an ideal world where we never allowed "always" as a value for color.ui, which is not the world we live in. > 2. Re-allow "color.always" config from the command-line. It's actually > on-disk config that we want to downgrade, but I wanted to avoid > making complicated rules about how the config would behave in > different scopes. The patch for this would look something like the > one below. Yuck, ugly. The code is simple (thanks to the "who ordered it?" thing), but the behaviour is rather embarrassing to explain. > 3. Revert the original series, and revisit the original "respect > color.ui via porcelain" commit which broke add--interactive in > v2.14.2 (136c8c8b8fa). Which one do you mean is "the original series"? The one that made plumbing to pay attention to the color config? I think it would be the cleanest "solution" in the world we live in, but the series (and the follow-on changes that started assuming that config_default reads the color config) have a rather large footprint and it will be quite painful to vet the result. I think the right fix to the original problem (you cannot remove auto-color from the plumbing) is to stop paying attention to color configuration from the default config. I wonder if something like this would work? - Initialize color.c::git_use_color_default to GIT_COLOR_UNKNOWN; - When git_color_config() is called, and if git_use_color_default is still GIT_COLOR_UNKNOWN, set it to GIT_COLOR_AUTO (regardless of the variable git_color_config() is called for). - In color.c::want_color(), when git_use_color_default is used, notice if it is GIT_COLOR_UNKNOWN and behave as if it is GIT_COLOR_NEVER. Then we make sure that git_color_config() is never called by any plumbing command. The fact it is (ever) called can be taken as a clue that we are running a Porcelain (hence we transition from UNKNOWN to AUTO), so we'd get the desirable "no default color for plumbing, auto color for Porcelain", I would think. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What happened to "git status --color=(always|auto|never)"? 2017-10-10 12:51 ` Junio C Hamano @ 2017-10-10 13:06 ` Jeff King 2017-10-10 19:03 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2017-10-10 13:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: Nazri Ramliy, Jonathan Nieder, Git Mailing List On Tue, Oct 10, 2017 at 09:51:38PM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > :( I was worried that this might hit some third-party scripts. > > ... > > All that said, should we revisit the decision from 6be4595edb? The two > > code changes we could make are: > > > > 1. Adding a "--color" option to "git status". Commit 0c88bf5050 > > (provide --color option for all ref-filter users, 2017-10-03) from > > that same series shows some prior art. > > > > This is a clean solution, but it does mean that scripts have to > > adapt (and would potentially need to care about which Git version > > they're relying on). > > If we view that "always" issue is a regression, then this is not a > "solution". It is a part of an ideal world where we never allowed > "always" as a value for color.ui, which is not the world we live in. Right, this doesn't solve any regression. It solves the "there's no way to do this thing I might want to" that exists in either world (one where "always" never existed, or one where "always" does not do that anymore but we accept it as either not a regression or an acceptable regression). > > 2. Re-allow "color.always" config from the command-line. It's actually > > on-disk config that we want to downgrade, but I wanted to avoid > > making complicated rules about how the config would behave in > > different scopes. The patch for this would look something like the > > one below. > > Yuck, ugly. The code is simple (thanks to the "who ordered it?" > thing), but the behaviour is rather embarrassing to explain. Yes, it's definitely the most ugly of all the options. The reason I mention it is that it's also the only one that solves the "git -c color.ui=always" regression (if we consider it one) without making a huge and risky change. > > 3. Revert the original series, and revisit the original "respect > > color.ui via porcelain" commit which broke add--interactive in > > v2.14.2 (136c8c8b8fa). > > Which one do you mean is "the original series"? The one that made > plumbing to pay attention to the color config? No, I meant reverting jk/ui-color-always-to-auto. But if we revert that, it leaves "add -p" broken when you set color.ui=always. So we must either accept that, or _also_ revert 136c8c8b8fa and come up with a different solution. > I think it would be > the cleanest "solution" in the world we live in, but the series (and > the follow-on changes that started assuming that config_default > reads the color config) have a rather large footprint and it will be > quite painful to vet the result. I agree that is a risk. It might not be _too_ bad, though this is an area that historically has poor test coverage. I know that for-each-ref and tag are two that would need touched (and it was them that led me down to the path to 136c8c8b8fa in the first place). > I think the right fix to the original problem (you cannot remove > auto-color from the plumbing) is to stop paying attention to color > configuration from the default config. I wonder if something like > this would work? > > - Initialize color.c::git_use_color_default to GIT_COLOR_UNKNOWN; > > - When git_color_config() is called, and if git_use_color_default > is still GIT_COLOR_UNKNOWN, set it to GIT_COLOR_AUTO (regardless > of the variable git_color_config() is called for). > > - In color.c::want_color(), when git_use_color_default is used, > notice if it is GIT_COLOR_UNKNOWN and behave as if it is > GIT_COLOR_NEVER. > > Then we make sure that git_color_config() is never called by any > plumbing command. The fact it is (ever) called can be taken as a > clue that we are running a Porcelain (hence we transition from > UNKNOWN to AUTO), so we'd get the desirable "no default color for > plumbing, auto color for Porcelain", I would think. Yes, I think that's the simplest way to implement the "plumbing should never do color without a command-line option" scheme. I do wonder if people would end up seeing some corner cases as regressions, though. Right now "diff-tree" _does_ color the output by default, and it would stop doing so under your scheme. That's the right thing to do by the plumbing/porcelain distinction, but users with scripts that use diff-tree (or other plumbing) to generate user-visible output may unexpectedly lose their color, until the calling script is fixed to add back in a --color option[1]. -Peff [1] Actually, just saying "--color" isn't enough, since you'd want to respect the user's color options. add--interactive does this, but it's a slight pain. It would be nice to have a --color=config variant that just calls git_color_config(). But if we are talking regression-fixes before v2.15, I don't think we need to have such niceties. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What happened to "git status --color=(always|auto|never)"? 2017-10-10 13:06 ` Jeff King @ 2017-10-10 19:03 ` Jonathan Nieder 2017-10-10 19:37 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2017-10-10 19:03 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Nazri Ramliy, Git Mailing List Hi, Jeff King wrote: > On Tue, Oct 10, 2017 at 09:51:38PM +0900, Junio C Hamano wrote: >> I think the right fix to the original problem (you cannot remove >> auto-color from the plumbing) is to stop paying attention to color >> configuration from the default config. I wonder if something like >> this would work? >> >> - Initialize color.c::git_use_color_default to GIT_COLOR_UNKNOWN; >> >> - When git_color_config() is called, and if git_use_color_default >> is still GIT_COLOR_UNKNOWN, set it to GIT_COLOR_AUTO (regardless >> of the variable git_color_config() is called for). >> >> - In color.c::want_color(), when git_use_color_default is used, >> notice if it is GIT_COLOR_UNKNOWN and behave as if it is >> GIT_COLOR_NEVER. >> >> Then we make sure that git_color_config() is never called by any >> plumbing command. The fact it is (ever) called can be taken as a >> clue that we are running a Porcelain (hence we transition from >> UNKNOWN to AUTO), so we'd get the desirable "no default color for >> plumbing, auto color for Porcelain", I would think. > > Yes, I think that's the simplest way to implement the "plumbing should > never do color without a command-line option" scheme. > > I do wonder if people would end up seeing some corner cases as > regressions, though. Right now "diff-tree" _does_ color the output by > default, and it would stop doing so under your scheme. That's the right > thing to do by the plumbing/porcelain distinction, but users with > scripts that use diff-tree (or other plumbing) to generate user-visible > output may unexpectedly lose their color, until the calling script is > fixed to add back in a --color option[1]. I think it's better for the calling script to be fixed to use "git diff", since it is producing output for the sake of the user instead of for machine parsing. That way, the script gets the benefit of other changes like --decorate automatically. So I don't see that as a regression. Where I worry is about commands where the line between porcelain and plumbing blur, like "git log --format=raw". I actually still prefer the approach where "color.ui=always" becomes impossible to express in config and each command takes a --color option. If we want to be extra fancy, we could make git take a --color option instead of requiring each command to do it. To support existing scripts, we could treat "-c color.ui=always" as a historical synonym for --color=always, either temporarily or indefinitely. Making it clear that this is only there for historical reasons would make it less likely that other options make the same mistake in the future. Thanks, Jonathan ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What happened to "git status --color=(always|auto|never)"? 2017-10-10 19:03 ` Jonathan Nieder @ 2017-10-10 19:37 ` Jeff King 2017-10-11 2:05 ` Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2017-10-10 19:37 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Junio C Hamano, Nazri Ramliy, Git Mailing List On Tue, Oct 10, 2017 at 12:03:14PM -0700, Jonathan Nieder wrote: > > I do wonder if people would end up seeing some corner cases as > > regressions, though. Right now "diff-tree" _does_ color the output by > > default, and it would stop doing so under your scheme. That's the right > > thing to do by the plumbing/porcelain distinction, but users with > > scripts that use diff-tree (or other plumbing) to generate user-visible > > output may unexpectedly lose their color, until the calling script is > > fixed to add back in a --color option[1]. > > I think it's better for the calling script to be fixed to use "git > diff", since it is producing output for the sake of the user instead > of for machine parsing. That way, the script gets the benefit of > other changes like --decorate automatically. > > So I don't see that as a regression. I agree that may be the best way for those scripts to do it. But it's still a regression to them, if their script used to do what they wanted and now it doesn't. It may be one we don't want to care about because the script is doing something we don't want to support. But then, think we are still deciding whether "color.always" in your ~/.gitconfig is in the same boat. > Where I worry is about commands where the line between porcelain and > plumbing blur, like "git log --format=raw". I actually still prefer > the approach where "color.ui=always" becomes impossible to express in > config and each command takes a --color option. > > If we want to be extra fancy, we could make git take a --color option > instead of requiring each command to do it. > > To support existing scripts, we could treat "-c color.ui=always" as a > historical synonym for --color=always, either temporarily or > indefinitely. Making it clear that this is only there for historical > reasons would make it less likely that other options make the same > mistake in the future. So that's basically my (2), with the twist that we claim it's only horrible and inconsistent for historical reasons. :) Is that the direction we want to go? -Peff ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: What happened to "git status --color=(always|auto|never)"? 2017-10-10 19:37 ` Jeff King @ 2017-10-11 2:05 ` Junio C Hamano 2017-10-12 2:10 ` [PATCH 0/2] Piling more kludge on top of color.ui Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-11 2:05 UTC (permalink / raw) To: Jeff King; +Cc: Jonathan Nieder, Nazri Ramliy, Git Mailing List Jeff King <peff@peff.net> writes: > On Tue, Oct 10, 2017 at 12:03:14PM -0700, Jonathan Nieder wrote: > >> Where I worry is about commands where the line between porcelain and >> plumbing blur, like "git log --format=raw". I actually still prefer >> the approach where "color.ui=always" becomes impossible to express in >> config and each command takes a --color option. >> >> If we want to be extra fancy, we could make git take a --color option >> instead of requiring each command to do it. >> >> To support existing scripts, we could treat "-c color.ui=always" as a >> historical synonym for --color=always, either temporarily or >> indefinitely. Making it clear that this is only there for historical >> reasons would make it less likely that other options make the same >> mistake in the future. > > So that's basically my (2), with the twist that we claim it's only > horrible and inconsistent for historical reasons. :) > > Is that the direction we want to go? Your (2), and Jonathan's "git --color=..." as an extension to it, is probably the least risky approach forward, as you said earlier. And I think that it would get us closest to the ideal world (in which color=always did not exist in the configuration system), while breaking least number of various "questionable" scripts in the wild. ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 0/2] Piling more kludge on top of color.ui 2017-10-11 2:05 ` Junio C Hamano @ 2017-10-12 2:10 ` Junio C Hamano 2017-10-12 2:10 ` [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration Junio C Hamano 2017-10-12 2:10 ` [PATCH 2/2] color: discourage use of ui.color=always Junio C Hamano 0 siblings, 2 replies; 42+ messages in thread From: Junio C Hamano @ 2017-10-12 2:10 UTC (permalink / raw) To: git Earlier we added a patch to unconditionally downgrade 'always' that is set to the color.ui configuration variable. This was done to correc the unintended regression to "git add -i" that was caused by two earlier mistakes that we no longer can undo. - The "add -i" command wants to parse output from "git diff-index" plumbing. The plumbing commands started paying attention to color configuration variables (which is a mistaken "solution" to cover another mistake), which caused people who have color.ui set to "always" to see breakage, as their "git diff-index" started coloring its output even when "git add -i" wanted to read it and parse it (without expecting to see any colors in its input); - The mistake the mistaken "solution" wanted to cover was that some time ago we started to automatically color the output (i.e. color when the output goes to the terminal) by default, but did so even to the plumbing commands. As many plumbing commands do not even have their own color control, it made it impossible to disable this auto-coloring--a mistaken "solution" was to pay attention to "git -c color.ui=never" from the command line. It turns out that there are many third-party scripts that do want to read colored output from our tools and the way they do so is to run "git -c color.ui=always cmd", which is a way to defeat any end-user settings and force coloured output reliably---at least they thought that they can rely on it working, that is. We saw one report of such a private tool getting broken on list, and I've seen another one inside $work. Let's keep "git -c color.ui=always cmd" form "working", while downgrading the setting made in the configuration files to "auto", to placate both camps. Let's also discourage use of 'always' and leave the door open for us to introduce "git --default-color=<what> cmd" later as a substitute for "git -c color.ui=<what>". Jeff King (1): color: downgrade "always" to "auto" only for on-disk configuration Junio C Hamano (1): color: discourage use of ui.color=always Documentation/config.txt | 2 +- color.c | 24 ++++++++++++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-) -- 2.15.0-rc1-151-g44fe2f342f ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-12 2:10 ` [PATCH 0/2] Piling more kludge on top of color.ui Junio C Hamano @ 2017-10-12 2:10 ` Junio C Hamano 2017-10-12 4:47 ` Jonathan Nieder 2017-10-12 12:31 ` Jeff King 2017-10-12 2:10 ` [PATCH 2/2] color: discourage use of ui.color=always Junio C Hamano 1 sibling, 2 replies; 42+ messages in thread From: Junio C Hamano @ 2017-10-12 2:10 UTC (permalink / raw) To: git; +Cc: Jeff King From: Jeff King <peff@peff.net> An earlier patch downgraded "always" that comes via the ui.color configuration variable to "auto", in order to work around an unfortunate regression to "git add -i". That "fix" however regressed other third-party tools that depend on "git -c color.ui=always cmd" as the way to defeat any end-user configuration and force coloured output from git subcommand, even when the output does not go to a terminal. It is a bit ugly to treat "-c color.ui=always" from the command line differently from a definition that comes from on-disk configuration files, but it is a moral equivalent of "--color=always" option given to the subcommand from the command line, i.e. a signal that tells us that the script writer knows what s/he is doing. So let's take that route to unbreak this case while defeating a (now declared to be) misguided color.ui that is set to always in the configuration file. NEEDS-SIGN-OFF-BY: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- color.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/color.c b/color.c index 17e2713f96..5b06c76bdc 100644 --- a/color.c +++ b/color.c @@ -307,8 +307,21 @@ int git_config_colorbool(const char *var, const char *value) if (value) { if (!strcasecmp(value, "never")) return 0; - if (!strcasecmp(value, "always")) - return var ? GIT_COLOR_AUTO : 1; + if (!strcasecmp(value, "always")) { + /* + * Command-line options always respect "always". + * Likewise for "-c" config on the command-line. + */ + if (!var || + current_config_scope() == CONFIG_SCOPE_CMDLINE) + return 1; + + /* + * Otherwise, we're looking at on-disk config; + * downgrade to auto. + */ + return GIT_COLOR_AUTO; + } if (!strcasecmp(value, "auto")) return GIT_COLOR_AUTO; } -- 2.15.0-rc1-151-g44fe2f342f ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-12 2:10 ` [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration Junio C Hamano @ 2017-10-12 4:47 ` Jonathan Nieder 2017-10-12 5:05 ` Junio C Hamano 2017-10-12 12:31 ` Jeff King 1 sibling, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2017-10-12 4:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King Junio C Hamano wrote: [...] > --- a/color.c > +++ b/color.c > @@ -307,8 +307,21 @@ int git_config_colorbool(const char *var, const char *value) > if (value) { > if (!strcasecmp(value, "never")) > return 0; > - if (!strcasecmp(value, "always")) > - return var ? GIT_COLOR_AUTO : 1; > + if (!strcasecmp(value, "always")) { > + /* > + * Command-line options always respect "always". > + * Likewise for "-c" config on the command-line. > + */ > + if (!var || > + current_config_scope() == CONFIG_SCOPE_CMDLINE) > + return 1; > + > + /* > + * Otherwise, we're looking at on-disk config; > + * downgrade to auto. > + */ > + return GIT_COLOR_AUTO; > + } Yes, this looks good to me. Should we document this special case treatment of color.* in -c somewhere? E.g. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> diff --git i/Documentation/config.txt w/Documentation/config.txt index 13ce76d48b..d7bd6b169c 100644 --- i/Documentation/config.txt +++ w/Documentation/config.txt @@ -1067,11 +1067,15 @@ clean.requireForce:: -i or -n. Defaults to true. color.branch:: - A boolean to enable/disable color in the output of - linkgit:git-branch[1]. May be set to `false` (or `never`) to - disable color entirely, `auto` (or `true` or `always`) in which - case colors are used only when the output is to a terminal. If - unset, then the value of `color.ui` is used (`auto` by default). + When to use color in the output of linkgit:git-branch[1]. + May be set to `never` (or `false`) to disable color entirely, + or `auto` (or `true`) in which case colors are used only when + the output is to a terminal. If unset, then the value of + `color.ui` is used (`auto` by default). ++ +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it is a historical synonym +for `--color=always`. color.branch.<slot>:: Use customized color for branch coloration. `<slot>` is one of @@ -1084,10 +1088,13 @@ color.diff:: Whether to use ANSI escape sequences to add color to patches. If this is set to `true` or `auto`, linkgit:git-diff[1], linkgit:git-log[1], and linkgit:git-show[1] will use color - when output is to the terminal. The value `always` is a - historical synonym for `auto`. If unset, then the value of + when output is to the terminal. If unset, then the value of `color.ui` is used (`auto` by default). + +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it is a historical +synonym for `--color=always`. ++ This does not affect linkgit:git-format-patch[1] or the 'git-diff-{asterisk}' plumbing commands. Can be overridden on the command line with the `--color[=<when>]` option. @@ -1118,10 +1125,14 @@ color.decorate.<slot>:: branches, remote-tracking branches, tags, stash and HEAD, respectively. color.grep:: - When set to `always`, always highlight matches. When `false` (or + When to highlight matches using color. When `false` (or `never`), never. When set to `true` or `auto`, use color only when the output is written to the terminal. If unset, then the value of `color.ui` is used (`auto` by default). ++ +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it is a historical synonym +for `--color=always`. color.grep.<slot>:: Use customized color for grep colorization. `<slot>` specifies which @@ -1153,9 +1164,11 @@ color.interactive:: When set to `true` or `auto`, use colors for interactive prompts and displays (such as those used by "git-add --interactive" and "git-clean --interactive") when the output is to the terminal. - When false (or `never`), never show colors. The value `always` - is a historical synonym for `auto`. If unset, then the value of - `color.ui` is used (`auto` by default). + When false (or `never`), never show colors. ++ +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it means to use color +regardless of whether output is to the terminal. color.interactive.<slot>:: Use customized color for 'git add --interactive' and 'git clean @@ -1168,18 +1181,24 @@ color.pager:: use (default is true). color.showBranch:: - A boolean to enable/disable color in the output of - linkgit:git-show-branch[1]. May be set to `always`, - `false` (or `never`) or `auto` (or `true`), in which case colors are used - only when the output is to a terminal. If unset, then the - value of `color.ui` is used (`auto` by default). + When to use color in the output of linkgit:git-show-branch[1]. + May be set to `never` (or `false`) to disable color or `auto` + (or `true`) to use colors only when the output is to a terminal. + If unset, the value of `color.ui` is used (`auto` by default). ++ +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it is a historical synonym +for `--color=always`. color.status:: - A boolean to enable/disable color in the output of - linkgit:git-status[1]. May be set to `always`, - `false` (or `never`) or `auto` (or `true`), in which case colors are used - only when the output is to a terminal. If unset, then the - value of `color.ui` is used (`auto` by default). + When to use color in the output of linkgit:git-status[1]. + May be set to `never` (or `false`) to disable color or `auto` + (or `true`) to use colors only when the output is to the terminal. + If unset, then the value of `color.ui` is used (`auto` by default). ++ +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it means to use color +regardless of whether output is to the terminal. color.status.<slot>:: Use customized color for status colorization. `<slot>` is @@ -1204,8 +1223,11 @@ color.ui:: color unless enabled explicitly with some other configuration or the `--color` option. Set it to `true` or `auto` to enable color when output is written to the terminal (this is also the - default since Git 1.8.4). The value `always` is a historical - synonym for `auto`. + default since Git 1.8.4). ++ +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it means to use color +regardless of whether output is to the terminal. column.ui:: Specify whether supported commands should output in columns. ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-12 4:47 ` Jonathan Nieder @ 2017-10-12 5:05 ` Junio C Hamano 2017-10-12 5:40 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-12 5:05 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git, Jeff King Jonathan Nieder <jrnieder@gmail.com> writes: > Should we document this special case treatment of color.* in -c > somewhere? E.g. Perhaps, although I'd save that until we actually add the new option to "git" potty, which hasn't happened yet, if I were thinking about adding some text like that. Also I'd call that --default-color=always or something like that, to avoid having to answer: what are the differences between these two --color thing in the following? git --color=foo cmd --color=bar ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-12 5:05 ` Junio C Hamano @ 2017-10-12 5:40 ` Jonathan Nieder 2017-10-12 6:15 ` Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2017-10-12 5:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: >> Should we document this special case treatment of color.* in -c >> somewhere? E.g. > > Perhaps, although I'd save that until we actually add the new option > to "git" potty, which hasn't happened yet, if I were thinking about > adding some text like that. Also I'd call that --default-color=always > or something like that, to avoid having to answer: what are the > differences between these two --color thing in the following? > > git --color=foo cmd --color=bar I agree that the color.status text in the example doc is unfortunate. But the surprising thing I found when writing that doc is that color.status ("git status", "git commit --dry-run") and color.interactive are the only items that needed it (aside from color.ui that needed it for those two). All the other commands that use color already accept git cmd --color=bar color.interactive applies to multiple commands (e.g. "git clean"), so it would take a little more chasing down to make them all use OPT__COLOR. Heading off to sleep, can look more tomorrow. I don't think we can get around documenting this -c special case behavior, though. diff --git i/builtin/commit.c w/builtin/commit.c index d75b3805ea..fc5b7cd538 100644 --- i/builtin/commit.c +++ w/builtin/commit.c @@ -1345,6 +1345,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) struct object_id oid; static struct option builtin_status_options[] = { OPT__VERBOSE(&verbose, N_("be verbose")), + OPT__COLOR(&s.use_color, N_("use color")), OPT_SET_INT('s', "short", &status_format, N_("show status concisely"), STATUS_FORMAT_SHORT), OPT_BOOL('b', "branch", &s.show_branch, @@ -1595,6 +1596,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) static struct option builtin_commit_options[] = { OPT__QUIET(&quiet, N_("suppress summary after successful commit")), OPT__VERBOSE(&verbose, N_("show diff in commit message template")), + OPT__COLOR(&s.use_color, N_("use color")), OPT_GROUP(N_("Commit message options")), OPT_FILENAME('F', "file", &logfile, N_("read message from file")), ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-12 5:40 ` Jonathan Nieder @ 2017-10-12 6:15 ` Junio C Hamano 2017-10-12 6:58 ` Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-12 6:15 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git, Jeff King Jonathan Nieder <jrnieder@gmail.com> writes: > Junio C Hamano wrote: >> Jonathan Nieder <jrnieder@gmail.com> writes: > >>> Should we document this special case treatment of color.* in -c >>> somewhere? E.g. >> >> Perhaps, although I'd save that until we actually add the new option >> to "git" potty, which hasn't happened yet, if I were thinking about >> adding some text like that. Also I'd call that --default-color=always >> or something like that, to avoid having to answer: what are the >> differences between these two --color thing in the following? >> >> git --color=foo cmd --color=bar > > I agree that the color.status text in the example doc is unfortunate. > But the surprising thing I found when writing that doc is that > color.status ("git status", "git commit --dry-run") and > color.interactive are the only items that needed it (aside from > color.ui that needed it for those two). All the other commands that > use color already accept > > git cmd --color=bar Ahh, I take it that you mean by "it" (in "needed it") the "git potty" option, not a "--color=<what>" option individual "git cmd" takes? If so, then it makes sense to say "that's another way to spell --color=always from the command line". We need to be able to answer "why does '-c color.ui=always' work only from the command line?", but I doubt we want to actively encourage the use of it, though, so I dunno. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-12 6:15 ` Junio C Hamano @ 2017-10-12 6:58 ` Junio C Hamano 2017-10-12 13:06 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-12 6:58 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git, Jeff King Junio C Hamano <gitster@pobox.com> writes: > We need to be able to answer "why does '-c color.ui=always' work > only from the command line?", but I doubt we want to actively > encourage the use of it, though, so I dunno. For today's pushout, I've queued this as [PATCH 3/2] Thanks.. -- >8 -- From: Jonathan Nieder <jrnieder@gmail.com> Subject: color: document that "git -c color.*=always" is a bit special Date: Wed, 11 Oct 2017 21:47:24 -0700 When used from the command line as an option to "git" potty, 'always' does not get demoted to 'auto', to help third-party scripts that (ab)used it to override the settings the end-user has. Document it. While at it, clarify description for per-command configuration variables (color.branch, color.grep, color.interactive, color.showBranch and color.status) so that they can more easily share the new text to talk about this special-casing. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/config.txt | 68 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 23 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index ba01b8d3df..f79e82b79a 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1051,11 +1051,15 @@ clean.requireForce:: -i or -n. Defaults to true. color.branch:: - A boolean to enable/disable color in the output of - linkgit:git-branch[1]. May be set to `false` (or `never`) to - disable color entirely, `auto` (or `true` or `always`) in which - case colors are used only when the output is to a terminal. If - unset, then the value of `color.ui` is used (`auto` by default). + When to use color in the output of linkgit:git-branch[1]. + May be set to `never` (or `false`) to disable color entirely, + or `auto` (or `true`) in which case colors are used only when + the output is to a terminal. If unset, then the value of + `color.ui` is used (`auto` by default). ++ +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it is a historical synonym +for `--color=always`. color.branch.<slot>:: Use customized color for branch coloration. `<slot>` is one of @@ -1068,10 +1072,13 @@ color.diff:: Whether to use ANSI escape sequences to add color to patches. If this is set to `true` or `auto`, linkgit:git-diff[1], linkgit:git-log[1], and linkgit:git-show[1] will use color - when output is to the terminal. The value `always` is a - historical synonym for `auto`. If unset, then the value of + when output is to the terminal. If unset, then the value of `color.ui` is used (`auto` by default). + +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it is a historical +synonym for `--color=always`. ++ This does not affect linkgit:git-format-patch[1] or the 'git-diff-{asterisk}' plumbing commands. Can be overridden on the command line with the `--color[=<when>]` option. @@ -1091,10 +1098,14 @@ color.decorate.<slot>:: branches, remote-tracking branches, tags, stash and HEAD, respectively. color.grep:: - When set to `always`, always highlight matches. When `false` (or + When to highlight matches using color. When `false` (or `never`), never. When set to `true` or `auto`, use color only when the output is written to the terminal. If unset, then the value of `color.ui` is used (`auto` by default). ++ +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it is a historical synonym +for `--color=always`. color.grep.<slot>:: Use customized color for grep colorization. `<slot>` specifies which @@ -1126,9 +1137,11 @@ color.interactive:: When set to `true` or `auto`, use colors for interactive prompts and displays (such as those used by "git-add --interactive" and "git-clean --interactive") when the output is to the terminal. - When false (or `never`), never show colors. The value `always` - is a historical synonym for `auto`. If unset, then the value of - `color.ui` is used (`auto` by default). + When false (or `never`), never show colors. ++ +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it means to use color +regardless of whether output is to the terminal. color.interactive.<slot>:: Use customized color for 'git add --interactive' and 'git clean @@ -1141,18 +1154,24 @@ color.pager:: use (default is true). color.showBranch:: - A boolean to enable/disable color in the output of - linkgit:git-show-branch[1]. May be set to `always`, - `false` (or `never`) or `auto` (or `true`), in which case colors are used - only when the output is to a terminal. If unset, then the - value of `color.ui` is used (`auto` by default). + When to use color in the output of linkgit:git-show-branch[1]. + May be set to `never` (or `false`) to disable color or `auto` + (or `true`) to use colors only when the output is to a terminal. + If unset, the value of `color.ui` is used (`auto` by default). ++ +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it is a historical synonym +for `--color=always`. color.status:: - A boolean to enable/disable color in the output of - linkgit:git-status[1]. May be set to `always`, - `false` (or `never`) or `auto` (or `true`), in which case colors are used - only when the output is to a terminal. If unset, then the - value of `color.ui` is used (`auto` by default). + When to use color in the output of linkgit:git-status[1]. + May be set to `never` (or `false`) to disable color or `auto` + (or `true`) to use colors only when the output is to the terminal. + If unset, then the value of `color.ui` is used (`auto` by default). ++ +The value `always` is a historical synonym for `auto`, except when +passed on the command line using `-c`, where it means to use color +regardless of whether output is to the terminal. color.status.<slot>:: Use customized color for status colorization. `<slot>` is @@ -1177,8 +1196,11 @@ color.ui:: color unless enabled explicitly with some other configuration or the `--color` option. Set it to `true` or `auto` to enable color when output is written to the terminal (this is also the - default since Git 1.8.4). The value `always` is a historical - synonym for `auto` and its use is discouraged. + default since Git 1.8.4). ++ +The value `always` is a historical synonym for `auto` (and its use is +discouraged), except when passed on the command line using `-c`, where +it means to use color regardless of whether output is to the terminal. column.ui:: Specify whether supported commands should output in columns. -- 2.15.0-rc1-154-g0b692121ee ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-12 6:58 ` Junio C Hamano @ 2017-10-12 13:06 ` Jeff King 2017-10-12 15:12 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2017-10-12 13:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git On Thu, Oct 12, 2017 at 03:58:18PM +0900, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > We need to be able to answer "why does '-c color.ui=always' work > > only from the command line?", but I doubt we want to actively > > encourage the use of it, though, so I dunno. > > For today's pushout, I've queued this as [PATCH 3/2] > > Thanks.. > > -- >8 -- > From: Jonathan Nieder <jrnieder@gmail.com> > Subject: color: document that "git -c color.*=always" is a bit special > Date: Wed, 11 Oct 2017 21:47:24 -0700 This looks reasonable to me to ship in v2.15. I assume we're going to leave any "git --default-color=..." options to post-release, since we're already in -rc1. -Peff ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-12 13:06 ` Jeff King @ 2017-10-12 15:12 ` Jeff King 0 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-12 15:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git On Thu, Oct 12, 2017 at 09:06:49AM -0400, Jeff King wrote: > > -- >8 -- > > From: Jonathan Nieder <jrnieder@gmail.com> > > Subject: color: document that "git -c color.*=always" is a bit special > > Date: Wed, 11 Oct 2017 21:47:24 -0700 > > This looks reasonable to me to ship in v2.15. I assume we're going to > leave any "git --default-color=..." options to post-release, since we're > already in -rc1. Ah, I hadn't yet read your cover letter, since I wasn't on the cc for that. So yes, the overall plan seems OK to me. I do have a lingering reservation that the fact that: git -c color.ui=always add -p will break may come back to bite us. In particular, any such: git --default-color=always add -p will run into the same problem if it is respected by plumbing. But in theory we are free to have it not be so. Arguably we could do the same for "-c color.ui", which I guess leaves us an "out" to later fix up that case (my, the kludges are certainly piling up on this one). -Peff ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-12 2:10 ` [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration Junio C Hamano 2017-10-12 4:47 ` Jonathan Nieder @ 2017-10-12 12:31 ` Jeff King 2017-10-13 0:09 ` Junio C Hamano 1 sibling, 1 reply; 42+ messages in thread From: Jeff King @ 2017-10-12 12:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, Oct 12, 2017 at 11:10:06AM +0900, Junio C Hamano wrote: > From: Jeff King <peff@peff.net> > > An earlier patch downgraded "always" that comes via the ui.color > configuration variable to "auto", in order to work around an > unfortunate regression to "git add -i". > > That "fix" however regressed other third-party tools that depend on > "git -c color.ui=always cmd" as the way to defeat any end-user > configuration and force coloured output from git subcommand, even > when the output does not go to a terminal. > > It is a bit ugly to treat "-c color.ui=always" from the command line > differently from a definition that comes from on-disk configuration > files, but it is a moral equivalent of "--color=always" option given > to the subcommand from the command line, i.e. a signal that tells us > that the script writer knows what s/he is doing. So let's take that > route to unbreak this case while defeating a (now declared to be) > misguided color.ui that is set to always in the configuration file. > > NEEDS-SIGN-OFF-BY: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Thanks for picking this up. I meant to get to it yesterday but ran out of time. Your description looks good to me. > color.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) We should probably protect the command-line behavior with a test. Can you squash this in? diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh index 25a9c65dc5..582cab5c8a 100755 --- a/t/t6006-rev-list-format.sh +++ b/t/t6006-rev-list-format.sh @@ -261,6 +261,17 @@ test_expect_success 'rev-list %C(auto,...) respects --color' ' test_cmp expect actual ' +test_expect_success "color.ui=always in config file same as auto" ' + test_config color.ui always && + git log --format=$COLOR -1 >actual && + has_no_color actual +' + +test_expect_success "color.ui=always on command-line is always" ' + git -c color.ui=always log --format=$COLOR -1 >actual && + has_color actual +' + iconv -f utf-8 -t $test_encoding > commit-msg <<EOF Test printing of complex bodies Technically the first test is already covered by the "add -p" we added elsewhere, but I think the sequence make sit easier to understand. Also as an aside, I think this patch means that: git -c color.ui=always add -p is broken (as would a hypothetical "git --default-color=always add -p"). That's sufficiently insane that I'm not sure we should care about it. -Peff ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-12 12:31 ` Jeff King @ 2017-10-13 0:09 ` Junio C Hamano 2017-10-13 1:47 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-13 0:09 UTC (permalink / raw) To: Jeff King; +Cc: git Jeff King <peff@peff.net> writes: > ... Also > as an aside, I think this patch means that: > > git -c color.ui=always add -p > > is broken (as would a hypothetical "git --default-color=always add -p"). > That's sufficiently insane that I'm not sure we should care about it. Do you mean that "'-c color.ui=always' from the command line is passed down to the invocations of 'git' the 'add' command makes, and would break output from 'diff-index' that 'add -i' wants to parse"? With the breakage that motivated "downgrade only for on-disk" change in mind, I do think that is the right behaviour. Those third-party scripts we broke knew how '-c color.ui=always' works and depended on it, and I consider that the command line configuration getting passed around as an integral part of 'how it works'. "Fixing" it will break them again. Let's take it as a signal that tells us that the script writers know what they are doing and leave it as a longish rope they can play with. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-13 0:09 ` Junio C Hamano @ 2017-10-13 1:47 ` Jeff King 2017-10-13 3:37 ` Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2017-10-13 1:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Fri, Oct 13, 2017 at 09:09:09AM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > ... Also > > as an aside, I think this patch means that: > > > > git -c color.ui=always add -p > > > > is broken (as would a hypothetical "git --default-color=always add -p"). > > That's sufficiently insane that I'm not sure we should care about it. > > Do you mean that "'-c color.ui=always' from the command line is > passed down to the invocations of 'git' the 'add' command makes, and > would break output from 'diff-index' that 'add -i' wants to parse"? Yes, exactly. > With the breakage that motivated "downgrade only for on-disk" change > in mind, I do think that is the right behaviour. Those third-party > scripts we broke knew how '-c color.ui=always' works and depended on > it, and I consider that the command line configuration getting > passed around as an integral part of 'how it works'. "Fixing" it > will break them again. Yeah, agreed. We cannot know what the script is expecting, so without that we cannot win, short of turning off color.ui entirely for plumbing. > Let's take it as a signal that tells us that the script writers know > what they are doing and leave it as a longish rope they can play with. OK. For the record, I'm not against scrapping this whole thing and trying to rollback to your "plumbing never looks at color.ui" proposal. It's quite late in the -rc cycle to do that, but there's nothing that says we can't bump the release date if that's what we need to do to get it right. If we ship v2.15 with the "color.ui=always really means auto", I don't think we'd want to undo that. So if we ship with what's in -rc1 (plus this new hack on top) I think that would be fairly final. -Peff ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-13 1:47 ` Jeff King @ 2017-10-13 3:37 ` Junio C Hamano 2017-10-13 13:06 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-13 3:37 UTC (permalink / raw) To: Jeff King; +Cc: git Jeff King <peff@peff.net> writes: > OK. For the record, I'm not against scrapping this whole thing and > trying to rollback to your "plumbing never looks at color.ui" proposal. > It's quite late in the -rc cycle to do that, but there's nothing that > says we can't bump the release date if that's what we need to do to get > it right. I think that it is too late, regardless of our release cycle. "Plumbing never looks at color.ui" implies that "plumbing must not get color.ui=auto from 4c7f1819", but given that 4c7f1819 is from 2013, I'd be surprised if we stopped coloring output from plumbing without getting any complaints from third-party script writers. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-13 3:37 ` Junio C Hamano @ 2017-10-13 13:06 ` Jeff King 2017-10-13 17:20 ` [PATCH 0/4] peeling back color.ui=always hacks Jeff King 2017-10-14 3:01 ` [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration Junio C Hamano 0 siblings, 2 replies; 42+ messages in thread From: Jeff King @ 2017-10-13 13:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Fri, Oct 13, 2017 at 12:37:57PM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > OK. For the record, I'm not against scrapping this whole thing and > > trying to rollback to your "plumbing never looks at color.ui" proposal. > > It's quite late in the -rc cycle to do that, but there's nothing that > > says we can't bump the release date if that's what we need to do to get > > it right. > > I think that it is too late, regardless of our release cycle. > > "Plumbing never looks at color.ui" implies that "plumbing must not > get color.ui=auto from 4c7f1819", but given that 4c7f1819 is from > 2013, I'd be surprised if we stopped coloring output from plumbing > without getting any complaints from third-party script writers. I agree that 4c7f1819 is the root of things. But there also weren't a lot of people complaining about it. I only noticed it as part of other work I was doing, and (perhaps foolishly) tried to clean it up. All of the regressions people have actually _noticed_ stem from my 136c8c8b8f in v2.14.2. So I think it is a viable option to try to go back to the pre-v2.14.2 state. I.e.: 1. Revert my 20120618c1 (color: downgrade "always" to "auto" only for on-disk configuration, 2017-10-10) and the test changes that came with it. 2. Teach for-each-ref and tag to use git_color_default_config(). These are the two I _know_ need this treatment as part of the series that contained 136c8c8b8f. As you've noted there may be others, but I'd be surprised if there are many. There hasn't been a lot of color work in the last few months besides what I've done. 3. Revert 136c8c8b8f. That takes us back to the pre-regression state. The ancient bug from 4c7f1819 still exists, but that would be OK for v2.15. We'd probably want to bump the -rc cycle a bit to give more confidence that (2) caught everything. Post-release, we would either: a. Do nothing. As far as we know, nobody has cared deeply about 4c7f1819 for the past 4 years. b. Teach git_default_config() to respect "never" but not "always", so that you can disable the auto-color in the plumbing (but not shoot yourself in the foot). c. Go all-out and remove the "auto" behavior from plumbing. This is much more likely to have fallouts since we've had 4 years of the wrong behavior. But we'd have a whole cycle to identify regressions. I'd probably vote for (b), followed by (a). Option (c) seems like a lot of risk for little benefit. But we could punt on that part until after the release. The only thing we'd need to decide on now is that first set of reversions. What I really _don't_ want to do is ship v2.15 with "always works like auto" and then flip that back in v2.16. I know you're probably infuriated with me because I'm essentially arguing the opposite of what I did earlier in the cycle. And I really am OK with going either way (shipping what's in -rc1 plus the "let 'always' work from the command line", or doing something like what I outlined above). But you've convinced me that the road I was going down really is piling up the hacks, and I want to make it clear that I'm not married to following the path I outlined earlier, and that I think there _is_ a viable alternative. -Peff ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 0/4] peeling back color.ui=always hacks 2017-10-13 13:06 ` Jeff King @ 2017-10-13 17:20 ` Jeff King 2017-10-13 17:23 ` [PATCH 1/4] Revert "color: make "always" the same as "auto" in config" Jeff King ` (3 more replies) 2017-10-14 3:01 ` [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration Junio C Hamano 1 sibling, 4 replies; 42+ messages in thread From: Jeff King @ 2017-10-13 17:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git On Fri, Oct 13, 2017 at 09:06:38AM -0400, Jeff King wrote: > > I think that it is too late, regardless of our release cycle. > > > > "Plumbing never looks at color.ui" implies that "plumbing must not > > get color.ui=auto from 4c7f1819", but given that 4c7f1819 is from > > 2013, I'd be surprised if we stopped coloring output from plumbing > > without getting any complaints from third-party script writers. > > I agree that 4c7f1819 is the root of things. But there also weren't a > lot of people complaining about it. I only noticed it as part of other > work I was doing, and (perhaps foolishly) tried to clean it up. > > All of the regressions people have actually _noticed_ stem from my > 136c8c8b8f in v2.14.2. So I think it is a viable option to try to go > back to the pre-v2.14.2 state. I.e.: > > 1. Revert my 20120618c1 (color: downgrade "always" to "auto" only for > on-disk configuration, 2017-10-10) and the test changes that came > with it. > > 2. Teach for-each-ref and tag to use git_color_default_config(). These > are the two I _know_ need this treatment as part of the series that > contained 136c8c8b8f. As you've noted there may be others, but I'd > be surprised if there are many. There hasn't been a lot of color > work in the last few months besides what I've done. > > 3. Revert 136c8c8b8f. > > That takes us back to the pre-regression state. The ancient bug from > 4c7f1819 still exists, but that would be OK for v2.15. We'd probably > want to bump the -rc cycle a bit to give more confidence that (2) caught > everything. Here's a series which does that. I'm torn on the correct direction, but I took the time to prepare these patches because I wanted to have two concrete alternatives to examine, not hand-waving about what one of the solutions would look like. I'm adding Jonathan back to the cc as somebody who was interested in the whole sequence (I think these patches should stand alone as explaining their reasoning, but you may want to look back in the thread a little for context). [1/4]: Revert "color: make "always" the same as "auto" in config" [2/4]: Revert "t6006: drop "always" color config tests" [3/4]: Revert "color: check color.ui in git_default_config()" [4/4]: tag: respect color.ui config Documentation/config.txt | 35 ++++++++++++++++++----------------- builtin/branch.c | 2 +- builtin/clean.c | 3 ++- builtin/grep.c | 2 +- builtin/show-branch.c | 2 +- builtin/tag.c | 2 +- color.c | 10 +++++++++- config.c | 4 ---- diff.c | 3 +++ t/t6006-rev-list-format.sh | 20 +++++++++++++++----- t/t6300-for-each-ref.sh | 5 +++++ t/t7004-tag.sh | 6 ++++++ 12 files changed, 62 insertions(+), 32 deletions(-) -Peff ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/4] Revert "color: make "always" the same as "auto" in config" 2017-10-13 17:20 ` [PATCH 0/4] peeling back color.ui=always hacks Jeff King @ 2017-10-13 17:23 ` Jeff King 2017-10-13 17:23 ` [PATCH 2/4] Revert "t6006: drop "always" color config tests" Jeff King ` (2 subsequent siblings) 3 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-13 17:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git This reverts commit 6be4595edb8e5b616c6e8b9fbc78b0f831fa2a87. That commit weakened the "always" setting of color config so that it acted as "auto". This was meant to solve regressions in v2.14.2 in which setting "color.ui=always" in the on-disk config broke scripts like add--interactive, because the plumbing diff commands began to generate color output. This was due to 136c8c8b8f (color: check color.ui in git_default_config(), 2017-07-13), which was in turn trying to fix issues caused by 4c7f1819b3 (make color.ui default to 'auto', 2013-06-10). But in weakening "always", we created even more problems, as people expect to be able to use "git -c color.ui=always" to force color (especially because some commands don't have their own --color flag). We can fix that by special-casing the command-line "-c", but now things are getting pretty confusing. Instead of piling hacks upon hacks, let's start peeling off the hacks. The first step is dropping the weakening of "always", which this revert does. Note that we could actually revert the whole series merged in by da15b78e52642bd45fd5513ab0000fdf2e58a6f4. Most of that series consists of preparations to the tests to handle the weakening of "-c color.ui=always". But it's worth keeping for a few reasons: - there are some other preparatory cleanups, like e433749d86 (test-terminal: set TERM=vt100, 2017-10-03) - it adds "--color" options more consistently in 0c88bf5050 (provide --color option for all ref-filter users, 2017-10-03) - some of the cases dropping "-c" end up being more robust and realistic tests, as in 01c94e9001 (t7508: use test_terminal for color output, 2017-10-03) - the preferred tool for overriding config is "--color", and we should be modeling that consistently We can individually revert the few commits necessary to restore some useful tests (which will be done on top of this patch). Note that this isn't a pure revert; we'll keep the test added in t3701, but mark it as failure for now. Signed-off-by: Jeff King <peff@peff.net> --- Documentation/config.txt | 35 ++++++++++++++++++----------------- color.c | 2 +- t/t3701-add-interactive.sh | 2 +- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index b53c994d0a..1ac0ae6adb 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1058,10 +1058,10 @@ clean.requireForce:: color.branch:: A boolean to enable/disable color in the output of - linkgit:git-branch[1]. May be set to `false` (or `never`) to - disable color entirely, `auto` (or `true` or `always`) in which - case colors are used only when the output is to a terminal. If - unset, then the value of `color.ui` is used (`auto` by default). + linkgit:git-branch[1]. May be set to `always`, + `false` (or `never`) or `auto` (or `true`), in which case colors are used + only when the output is to a terminal. If unset, then the + value of `color.ui` is used (`auto` by default). color.branch.<slot>:: Use customized color for branch coloration. `<slot>` is one of @@ -1072,11 +1072,12 @@ color.branch.<slot>:: color.diff:: Whether to use ANSI escape sequences to add color to patches. - If this is set to `true` or `auto`, linkgit:git-diff[1], + If this is set to `always`, linkgit:git-diff[1], linkgit:git-log[1], and linkgit:git-show[1] will use color - when output is to the terminal. The value `always` is a - historical synonym for `auto`. If unset, then the value of - `color.ui` is used (`auto` by default). + for all patches. If it is set to `true` or `auto`, those + commands will only use color when output is to the terminal. + If unset, then the value of `color.ui` is used (`auto` by + default). + This does not affect linkgit:git-format-patch[1] or the 'git-diff-{asterisk}' plumbing commands. Can be overridden on the @@ -1140,12 +1141,12 @@ color.grep.<slot>:: -- color.interactive:: - When set to `true` or `auto`, use colors for interactive prompts + When set to `always`, always use colors for interactive prompts and displays (such as those used by "git-add --interactive" and - "git-clean --interactive") when the output is to the terminal. - When false (or `never`), never show colors. The value `always` - is a historical synonym for `auto`. If unset, then the value of - `color.ui` is used (`auto` by default). + "git-clean --interactive"). When false (or `never`), never. + When set to `true` or `auto`, use colors only when the output is + to the terminal. If unset, then the value of `color.ui` is + used (`auto` by default). color.interactive.<slot>:: Use customized color for 'git add --interactive' and 'git clean @@ -1192,10 +1193,10 @@ color.ui:: configuration to set a default for the `--color` option. Set it to `false` or `never` if you prefer Git commands not to use color unless enabled explicitly with some other configuration - or the `--color` option. Set it to `true` or `auto` to enable - color when output is written to the terminal (this is also the - default since Git 1.8.4). The value `always` is a historical - synonym for `auto`. + or the `--color` option. Set it to `always` if you want all + output not intended for machine consumption to use color, to + `true` or `auto` (this is the default since Git 1.8.4) if you + want such output to use color when written to the terminal. column.ui:: Specify whether supported commands should output in columns. diff --git a/color.c b/color.c index 9c0dc82370..9ccd954d6b 100644 --- a/color.c +++ b/color.c @@ -308,7 +308,7 @@ int git_config_colorbool(const char *var, const char *value) if (!strcasecmp(value, "never")) return 0; if (!strcasecmp(value, "always")) - return var ? GIT_COLOR_AUTO : 1; + return 1; if (!strcasecmp(value, "auto")) return GIT_COLOR_AUTO; } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index a49c12c79b..87ffd4d43c 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -483,7 +483,7 @@ test_expect_success 'hunk-editing handles custom comment char' ' git diff --exit-code ' -test_expect_success 'add -p works even with color.ui=always' ' +test_expect_failure 'add -p works even with color.ui=always' ' git reset --hard && echo change >>file && test_config color.ui always && -- 2.15.0.rc1.395.ga4290b5804 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 2/4] Revert "t6006: drop "always" color config tests" 2017-10-13 17:20 ` [PATCH 0/4] peeling back color.ui=always hacks Jeff King 2017-10-13 17:23 ` [PATCH 1/4] Revert "color: make "always" the same as "auto" in config" Jeff King @ 2017-10-13 17:23 ` Jeff King 2017-10-13 17:24 ` [PATCH 3/4] Revert "color: check color.ui in git_default_config()" Jeff King 2017-10-13 17:26 ` [PATCH 4/4] tag: respect color.ui config Jeff King 3 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-13 17:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git This reverts commit c5bdfe677cfab5b2e87771c35565d44d3198efda. That commit was done primarily to prepare for the weakening of "always" in 6be4595edb (color: make "always" the same as "auto" in config, 2017-10-03). But since we've now reverted 6be4595edb, there's no need for us to remove "-c color.ui=always" from the tests. And in fact it's a good idea to restore these tests, to make sure that "always" continues to work. Signed-off-by: Jeff King <peff@peff.net> --- t/t6006-rev-list-format.sh | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh index 25a9c65dc5..98be78b4a2 100755 --- a/t/t6006-rev-list-format.sh +++ b/t/t6006-rev-list-format.sh @@ -208,11 +208,26 @@ do has_no_color actual ' + test_expect_success "$desc enables colors for color.diff" ' + git -c color.diff=always log --format=$color -1 >actual && + has_color actual + ' + + test_expect_success "$desc enables colors for color.ui" ' + git -c color.ui=always log --format=$color -1 >actual && + has_color actual + ' + test_expect_success "$desc respects --color" ' git log --format=$color -1 --color >actual && has_color actual ' + test_expect_success "$desc respects --no-color" ' + git -c color.ui=always log --format=$color -1 --no-color >actual && + has_no_color actual + ' + test_expect_success TTY "$desc respects --color=auto (stdout is tty)" ' test_terminal git log --format=$color -1 --color=auto >actual && has_color actual @@ -225,11 +240,6 @@ do has_no_color actual ) ' - - test_expect_success TTY "$desc respects --no-color" ' - test_terminal git log --format=$color -1 --no-color >actual && - has_no_color actual - ' done test_expect_success '%C(always,...) enables color even without tty' ' -- 2.15.0.rc1.395.ga4290b5804 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 3/4] Revert "color: check color.ui in git_default_config()" 2017-10-13 17:20 ` [PATCH 0/4] peeling back color.ui=always hacks Jeff King 2017-10-13 17:23 ` [PATCH 1/4] Revert "color: make "always" the same as "auto" in config" Jeff King 2017-10-13 17:23 ` [PATCH 2/4] Revert "t6006: drop "always" color config tests" Jeff King @ 2017-10-13 17:24 ` Jeff King 2017-10-13 17:26 ` [PATCH 4/4] tag: respect color.ui config Jeff King 3 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-13 17:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git This reverts commit 136c8c8b8fa39f1315713248473dececf20f8fe7. That commit was trying to address a bug caused by 4c7f1819b3 (make color.ui default to 'auto', 2013-06-10), in which plumbing like diff-tree defaulted to "auto" color, but did not respect a "color.ui" directive to disable it. But it also meant that we started respecting "color.ui" set to "always". This was a known problem, but 4c7f1819b3 argued that nobody ought to be doing that. However, that turned out to be wrong, and we got a number of bug reports related to "add -p" regressing in v2.14.2. Let's revert 136c8c8b8, fixing the regression to "add -p". This leaves the problem from 4c7f1819b3 unfixed, but: 1. It's a pretty obscure problem in the first place. I only noticed it while working on the color code, and we haven't got a single bug report or complaint about it. 2. We can make a more moderate fix on top by respecting "never" but not "always" for plumbing commands. This is just the minimal fix to go back to the working state we had before v2.14.2. Note that this isn't a pure revert. We now have a test in t3701 which shows off the "add -p" regression. This can be flipped to success. Signed-off-by: Jeff King <peff@peff.net> --- builtin/branch.c | 2 +- builtin/clean.c | 3 ++- builtin/grep.c | 2 +- builtin/show-branch.c | 2 +- color.c | 8 ++++++++ config.c | 4 ---- diff.c | 3 +++ t/t3701-add-interactive.sh | 2 +- 8 files changed, 17 insertions(+), 9 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index b67593288c..79dc9181fd 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -93,7 +93,7 @@ static int git_branch_config(const char *var, const char *value, void *cb) return config_error_nonbool(var); return color_parse(value, branch_colors[slot]); } - return git_default_config(var, value, cb); + return git_color_default_config(var, value, cb); } static const char *branch_get_color(enum color_branch ix) diff --git a/builtin/clean.c b/builtin/clean.c index 733b6d3745..189e20628c 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -126,7 +126,8 @@ static int git_clean_config(const char *var, const char *value, void *cb) return 0; } - return git_default_config(var, value, cb); + /* inspect the color.ui config variable and others */ + return git_color_default_config(var, value, cb); } static const char *clean_get_color(enum color_clean ix) diff --git a/builtin/grep.c b/builtin/grep.c index 19e23946ac..2d65f27d01 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -275,7 +275,7 @@ static int wait_all(void) static int grep_cmd_config(const char *var, const char *value, void *cb) { int st = grep_config(var, value, cb); - if (git_default_config(var, value, cb) < 0) + if (git_color_default_config(var, value, cb) < 0) st = -1; if (!strcmp(var, "grep.threads")) { diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 84547d6fba..6fa1f62a88 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -554,7 +554,7 @@ static int git_show_branch_config(const char *var, const char *value, void *cb) return 0; } - return git_default_config(var, value, cb); + return git_color_default_config(var, value, cb); } static int omit_in_dense(struct commit *commit, struct commit **rev, int n) diff --git a/color.c b/color.c index 9ccd954d6b..9a9261ac16 100644 --- a/color.c +++ b/color.c @@ -368,6 +368,14 @@ int git_color_config(const char *var, const char *value, void *cb) return 0; } +int git_color_default_config(const char *var, const char *value, void *cb) +{ + if (git_color_config(var, value, cb) < 0) + return -1; + + return git_default_config(var, value, cb); +} + void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb) { if (*color) diff --git a/config.c b/config.c index 4831c12735..adb7d7a3e5 100644 --- a/config.c +++ b/config.c @@ -16,7 +16,6 @@ #include "string-list.h" #include "utf8.h" #include "dir.h" -#include "color.h" struct config_source { struct config_source *prev; @@ -1351,9 +1350,6 @@ int git_default_config(const char *var, const char *value, void *dummy) if (starts_with(var, "advice.")) return git_default_advice_config(var, value); - if (git_color_config(var, value, dummy) < 0) - return -1; - if (!strcmp(var, "pager.color") || !strcmp(var, "color.pager")) { pager_use_color = git_config_bool(var,value); return 0; diff --git a/diff.c b/diff.c index 69f03570ad..30b2e271b0 100644 --- a/diff.c +++ b/diff.c @@ -358,6 +358,9 @@ int git_diff_ui_config(const char *var, const char *value, void *cb) return 0; } + if (git_color_config(var, value, cb) < 0) + return -1; + return git_diff_basic_config(var, value, cb); } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 87ffd4d43c..a49c12c79b 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -483,7 +483,7 @@ test_expect_success 'hunk-editing handles custom comment char' ' git diff --exit-code ' -test_expect_failure 'add -p works even with color.ui=always' ' +test_expect_success 'add -p works even with color.ui=always' ' git reset --hard && echo change >>file && test_config color.ui always && -- 2.15.0.rc1.395.ga4290b5804 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 4/4] tag: respect color.ui config 2017-10-13 17:20 ` [PATCH 0/4] peeling back color.ui=always hacks Jeff King ` (2 preceding siblings ...) 2017-10-13 17:24 ` [PATCH 3/4] Revert "color: check color.ui in git_default_config()" Jeff King @ 2017-10-13 17:26 ` Jeff King 3 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-13 17:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git Since 11b087adfd (ref-filter: consult want_color() before emitting colors, 2017-07-13), we expect that setting "color.ui" to "always" will enable color tag formats even without a tty. As that commit was built on top of 136c8c8b8f (color: check color.ui in git_default_config(), 2017-07-13) from the same series, we didn't need to touch tag's config parsing at all. However, since we reverted 136c8c8b8f, we now need to explicitly call git_color_default_config() to make this work. Let's do so, and also restore the test dropped in 0c88bf5050 (provide --color option for all ref-filter users, 2017-10-03). That commit swapped out our "color.ui=always" test for "--color" in preparation for "always" going away. But since it is here to stay, we should test both cases. Note that for-each-ref also lost its color.ui support as part of reverting 136c8c8b8f. But as a plumbing command, it should _not_ respect the color.ui config. Since it also gained a --color option in 0c88bf5050, that's the correct way to ask it for color. We'll continue to test that, and confirm that "color.ui" is not respected. Signed-off-by: Jeff King <peff@peff.net> --- builtin/tag.c | 2 +- t/t6300-for-each-ref.sh | 5 +++++ t/t7004-tag.sh | 6 ++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/builtin/tag.c b/builtin/tag.c index 695cb0778e..b38329b593 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -158,7 +158,7 @@ static int git_tag_config(const char *var, const char *value, void *cb) if (starts_with(var, "column.")) return git_column_config(var, value, "tag", &colopts); - return git_default_config(var, value, cb); + return git_color_default_config(var, value, cb); } static void write_tag_body(int fd, const struct object_id *oid) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 416ff7d0b8..3aa534933e 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -442,6 +442,11 @@ test_expect_success '--color can override tty check' ' test_cmp expected.color actual ' +test_expect_success 'color.ui=always does not override tty check' ' + git -c color.ui=always for-each-ref --format="$color_format" >actual && + test_cmp expected.bare actual +' + cat >expected <<\EOF heads/master tags/master diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 4e62c505fc..a9af2de996 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1918,6 +1918,12 @@ test_expect_success '--color overrides auto-color' ' test_cmp expect.color actual ' +test_expect_success 'color.ui=always overrides auto-color' ' + git -c color.ui=always tag $color_args >actual.raw && + test_decode_color <actual.raw >actual && + test_cmp expect.color actual +' + test_expect_success 'setup --merged test tags' ' git tag mergetest-1 HEAD~2 && git tag mergetest-2 HEAD~1 && -- 2.15.0.rc1.395.ga4290b5804 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-13 13:06 ` Jeff King 2017-10-13 17:20 ` [PATCH 0/4] peeling back color.ui=always hacks Jeff King @ 2017-10-14 3:01 ` Junio C Hamano 2017-10-16 21:53 ` Jeff King 1 sibling, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-14 3:01 UTC (permalink / raw) To: Jeff King; +Cc: git Jeff King <peff@peff.net> writes: > All of the regressions people have actually _noticed_ stem from my > 136c8c8b8f in v2.14.2. So I think it is a viable option to try to go > back to the pre-v2.14.2 state. I.e.: > ... > That takes us back to the pre-regression state. The ancient bug from > 4c7f1819 still exists, but that would be OK for v2.15. We'd probably > want to bump the -rc cycle a bit to give more confidence that (2) caught > everything. Yes, I think that is the approach I was pushing initially with the jc/ref-filter-colors-fix topic that was later retracted; the result of your 4-patch series more or less matches that one, modulo that I didn't treat for-each-ref as a plumbing. I do share the worry that it is hard to make sure that these post-revert adjustment caught everything; after all, that was a major part of the reason why my earlier attempt was retracted. I still think this is the _right_ direction to go in, even though it is harder to get right. > Post-release, we would either: > ... > But we could punt on that part until after the release. The only thing > we'd need to decide on now is that first set of reversions. What I > really _don't_ want to do is ship v2.15 with "always works like auto" > and then flip that back in v2.16. True. Let's see what others think. I know Jonathan is running the fork at $work with "downgrade always to auto" patches, and while I think both approaches would probably work well in practice, I have preference for this "harder but right" approach, so I'd want to see different views discussed on the list before we decide. Thanks. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-14 3:01 ` [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration Junio C Hamano @ 2017-10-16 21:53 ` Jeff King 2017-10-17 1:06 ` Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2017-10-16 21:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Sat, Oct 14, 2017 at 12:01:46PM +0900, Junio C Hamano wrote: > > That takes us back to the pre-regression state. The ancient bug from > > 4c7f1819 still exists, but that would be OK for v2.15. We'd probably > > want to bump the -rc cycle a bit to give more confidence that (2) caught > > everything. > > Yes, I think that is the approach I was pushing initially with the > jc/ref-filter-colors-fix topic that was later retracted; the result > of your 4-patch series more or less matches that one, modulo that I > didn't treat for-each-ref as a plumbing. Ah, right, I forgot about that one while I was putting it together. So many alternatives floating around. > I do share the worry that > it is hard to make sure that these post-revert adjustment caught > everything; after all, that was a major part of the reason why my > earlier attempt was retracted. I still think this is the _right_ > direction to go in, even though it is harder to get right. To be honest, I'm not actually very worried. I think missing a post-revert adjustment is the main risk, but my general sense is that there hasn't been a lot going on with color fixes outside of my recent work. Famous last words and all that, I'm sure. :) > True. Let's see what others think. I know Jonathan is running > the fork at $work with "downgrade always to auto" patches, and while > I think both approaches would probably work well in practice, I have > preference for this "harder but right" approach, so I'd want to see > different views discussed on the list before we decide. After pondering over it, I have a slight preference for that, too. But I'm also happy to hear more input. -Peff ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-16 21:53 ` Jeff King @ 2017-10-17 1:06 ` Junio C Hamano 2017-10-17 6:26 ` Junio C Hamano 2017-10-17 6:51 ` Jonathan Nieder 0 siblings, 2 replies; 42+ messages in thread From: Junio C Hamano @ 2017-10-17 1:06 UTC (permalink / raw) To: Jeff King; +Cc: git, Jonathan Nieder Jeff King <peff@peff.net> writes: > On Sat, Oct 14, 2017 at 12:01:46PM +0900, Junio C Hamano wrote: > >> > That takes us back to the pre-regression state. The ancient bug from >> > 4c7f1819 still exists, but that would be OK for v2.15. We'd probably >> > want to bump the -rc cycle a bit to give more confidence that (2) caught >> > everything. >> >> Yes, I think that is the approach I was pushing initially with the >> jc/ref-filter-colors-fix topic that was later retracted; the result >> of your 4-patch series more or less matches that one, modulo that I >> didn't treat for-each-ref as a plumbing. > > Ah, right, I forgot about that one while I was putting it together. So > many alternatives floating around. > >> I do share the worry that >> it is hard to make sure that these post-revert adjustment caught >> everything; after all, that was a major part of the reason why my >> earlier attempt was retracted. I still think this is the _right_ >> direction to go in, even though it is harder to get right. > > To be honest, I'm not actually very worried. I think missing a > post-revert adjustment is the main risk, but my general sense is that > there hasn't been a lot going on with color fixes outside of my recent > work. Famous last words and all that, I'm sure. :) > >> True. Let's see what others think. I know Jonathan is running >> the fork at $work with "downgrade always to auto" patches, and while >> I think both approaches would probably work well in practice, I have >> preference for this "harder but right" approach, so I'd want to see >> different views discussed on the list before we decide. > > After pondering over it, I have a slight preference for that, too. But > I'm also happy to hear more input. OK, so it seems we both have slight preference for the "peel back" approach. Adding Jonathan to Cc: ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-17 1:06 ` Junio C Hamano @ 2017-10-17 6:26 ` Junio C Hamano 2017-10-18 5:28 ` Jeff King 2017-10-17 6:51 ` Jonathan Nieder 1 sibling, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-17 6:26 UTC (permalink / raw) To: Jeff King; +Cc: git, Jonathan Nieder Junio C Hamano <gitster@pobox.com> writes: > Jeff King <peff@peff.net> writes: > >> After pondering over it, I have a slight preference for that, too. But >> I'm also happy to hear more input. > > OK, so it seems we both have slight preference for the "peel back" > approach. Adding Jonathan to Cc: It was a bit more painful than necessary to make sure I have something that can be merged for 2.14.x maintenance track, but I think the topic is now in a reasonable shape, and I've merged it to 'next'. On the first-parent chain from 'master' to 'pu', the merge of this topic is the very first one, and after reading it over once again, I think this is OK. Thanks. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-17 6:26 ` Junio C Hamano @ 2017-10-18 5:28 ` Jeff King 2017-10-18 5:57 ` Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2017-10-18 5:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder On Tue, Oct 17, 2017 at 03:26:25PM +0900, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Jeff King <peff@peff.net> writes: > > > >> After pondering over it, I have a slight preference for that, too. But > >> I'm also happy to hear more input. > > > > OK, so it seems we both have slight preference for the "peel back" > > approach. Adding Jonathan to Cc: > > It was a bit more painful than necessary to make sure I have > something that can be merged for 2.14.x maintenance track, but I > think the topic is now in a reasonable shape, and I've merged it to > 'next'. On the first-parent chain from 'master' to 'pu', the merge > of this topic is the very first one, and after reading it over once > again, I think this is OK. Hmm. I think you would just want the top two commits for maint-2.14 (reverting 136c8c8b8f and fixing up git-tag to check color config). But of course you can't do a partial merge because they come on top of the other dead-end/revert pair. You'd have to cherry-pick (and even then fix up a few bits, like adding in the "add -p" test). Though if we take all of jk/ui-color-always-to-auto-maint, and then do the whole reversion on top of that, I think that should work. It just doesn't look like that topic ever made it to "maint" (I see mention of a jk/ref-filter-colors-fix-maint in the log for master, but there's no such branch). I started to prepare a patch directly on v2.14.2 just to see what it would look like. The first one (the revert) is fine, but we then have to fixup tag and for-each-ref. And since they don't have --color added by the dead-end fixups, the tests get harder... -Peff ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-18 5:28 ` Jeff King @ 2017-10-18 5:57 ` Junio C Hamano 0 siblings, 0 replies; 42+ messages in thread From: Junio C Hamano @ 2017-10-18 5:57 UTC (permalink / raw) To: Jeff King; +Cc: git, Jonathan Nieder Jeff King <peff@peff.net> writes: >> It was a bit more painful than necessary to make sure I have >> something that can be merged for 2.14.x maintenance track, but I >> think the topic is now in a reasonable shape, and I've merged it to >> 'next'. On the first-parent chain from 'master' to 'pu', the merge >> of this topic is the very first one, and after reading it over once >> again, I think this is OK. > > Hmm. I think you would just want the top two commits for maint-2.14 > (reverting 136c8c8b8f and fixing up git-tag to check color config). But > of course you can't do a partial merge because they come on top of the > other dead-end/revert pair. You'd have to cherry-pick (and even then fix > up a few bits, like adding in the "add -p" test). > > Though if we take all of jk/ui-color-always-to-auto-maint, and then do > the whole reversion on top of that, I think that should work. It just > doesn't look like that topic ever made it to "maint" (I see mention of a > jk/ref-filter-colors-fix-maint in the log for master, but there's no > such branch). Yeah, that is what ended up to be jk/ref-filter-colors-fix; the branch merges cleanly to 'master', but also to 'maint' without dragging the rest of the recent development along with it---I did a rebase before sending out the message you are responding to. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-17 1:06 ` Junio C Hamano 2017-10-17 6:26 ` Junio C Hamano @ 2017-10-17 6:51 ` Jonathan Nieder 2017-10-18 5:34 ` Jeff King 1 sibling, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2017-10-17 6:51 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git Hi, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: >> On Sat, Oct 14, 2017 at 12:01:46PM +0900, Junio C Hamano wrote: >>> True. Let's see what others think. I know Jonathan is running >>> the fork at $work with "downgrade always to auto" patches, and while >>> I think both approaches would probably work well in practice, I have >>> preference for this "harder but right" approach, so I'd want to see >>> different views discussed on the list before we decide. >> >> After pondering over it, I have a slight preference for that, too. But >> I'm also happy to hear more input. > > OK, so it seems we both have slight preference for the "peel back" > approach. Adding Jonathan to Cc: Which approach is "harder but right" / "peel back"? I agree with the goal of making color.ui=always a synonym for auto in file-based config. Peff found some problems with the warning patch (scripted commands produce too many warnings), which are not an issue for $dayjob but may be for upstream, so I see the value of holding off on the warning for now. I'm also fine with "revert the proximate cause of the latest complaints" as a stepping stone toward making color.ui=always a synonym for auto in file-based config in a later release. Another issue is diff-files paying attention to this configuration. If I'm reading Documentation/config.txt correctly, that was simply a bug. diff-files and diff-index are never supposed to use color, regardless of configuration. I'm fine with "revert the proximate cause of the latest complaints" as a stepping stone toward fixing that, too. :) Sorry I don't have more detailed advice. I was planning to look more closely at how these features evolved over time and haven't had enough time for it yet. Jonathan ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration 2017-10-17 6:51 ` Jonathan Nieder @ 2017-10-18 5:34 ` Jeff King 0 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-18 5:34 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Junio C Hamano, git On Mon, Oct 16, 2017 at 11:51:01PM -0700, Jonathan Nieder wrote: > > OK, so it seems we both have slight preference for the "peel back" > > approach. Adding Jonathan to Cc: > > Which approach is "harder but right" / "peel back"? "peel back" is reverting back to the pre-v2.14.2 state (Junio has the patches queued in jk/ref-filter-colors-fix). > I agree with the goal of making color.ui=always a synonym for auto in > file-based config. Peff found some problems with the warning patch > (scripted commands produce too many warnings), which are not an issue > for $dayjob but may be for upstream, so I see the value of holding off > on the warning for now. > > I'm also fine with "revert the proximate cause of the latest > complaints" as a stepping stone toward making color.ui=always a > synonym for auto in file-based config in a later release. I do think "color.ui=always" is a foot-gun, but I wasn't happy with the number of weird hacks we ended up with in trying to fix that (like "it means one thing in the on-disk config and another thing with "git -c"). We can take it up as a topic post-release. At the very least, I think we will want to change the documentation to make it more clear that you almost certainly _don't_ want to use "always". -Peff ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 2/2] color: discourage use of ui.color=always 2017-10-12 2:10 ` [PATCH 0/2] Piling more kludge on top of color.ui Junio C Hamano 2017-10-12 2:10 ` [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration Junio C Hamano @ 2017-10-12 2:10 ` Junio C Hamano 2017-10-12 4:48 ` Jonathan Nieder 2017-10-12 15:08 ` Jeff King 1 sibling, 2 replies; 42+ messages in thread From: Junio C Hamano @ 2017-10-12 2:10 UTC (permalink / raw) To: git Warn when we read such a configuration from a file, and nudge the users to spell them 'auto' instead. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/config.txt | 2 +- color.c | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index cb0f951ddc..ba01b8d3df 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1178,7 +1178,7 @@ color.ui:: or the `--color` option. Set it to `true` or `auto` to enable color when output is written to the terminal (this is also the default since Git 1.8.4). The value `always` is a historical - synonym for `auto`. + synonym for `auto` and its use is discouraged. column.ui:: Specify whether supported commands should output in columns. diff --git a/color.c b/color.c index 5b06c76bdc..5119f9bca0 100644 --- a/color.c +++ b/color.c @@ -308,6 +308,8 @@ int git_config_colorbool(const char *var, const char *value) if (!strcasecmp(value, "never")) return 0; if (!strcasecmp(value, "always")) { + static int warn_once; + /* * Command-line options always respect "always". * Likewise for "-c" config on the command-line. @@ -320,6 +322,11 @@ int git_config_colorbool(const char *var, const char *value) * Otherwise, we're looking at on-disk config; * downgrade to auto. */ + if (!warn_once) { + warn_once++; + warning("setting '%s' to '%s' is no longer valid; " + "set it to 'auto' instead", var, value); + } return GIT_COLOR_AUTO; } if (!strcasecmp(value, "auto")) -- 2.15.0-rc1-151-g44fe2f342f ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] color: discourage use of ui.color=always 2017-10-12 2:10 ` [PATCH 2/2] color: discourage use of ui.color=always Junio C Hamano @ 2017-10-12 4:48 ` Jonathan Nieder 2017-10-12 15:08 ` Jeff King 1 sibling, 0 replies; 42+ messages in thread From: Jonathan Nieder @ 2017-10-12 4:48 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano wrote: > Warn when we read such a configuration from a file, and nudge the > users to spell them 'auto' instead. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > Documentation/config.txt | 2 +- > color.c | 7 +++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) This warning only kicks in when `always` is being silently downgraded to `auto`. It makes sense to me. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] color: discourage use of ui.color=always 2017-10-12 2:10 ` [PATCH 2/2] color: discourage use of ui.color=always Junio C Hamano 2017-10-12 4:48 ` Jonathan Nieder @ 2017-10-12 15:08 ` Jeff King 2017-10-13 0:02 ` Junio C Hamano 1 sibling, 1 reply; 42+ messages in thread From: Jeff King @ 2017-10-12 15:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, Oct 12, 2017 at 11:10:07AM +0900, Junio C Hamano wrote: > Warn when we read such a configuration from a file, and nudge the > users to spell them 'auto' instead. Hmm. On the one hand, it is nice to make people aware that their config isn't doing what they might think. On the other hand, if "always" is no longer a problem for anybody, do we need to force users to take the step to eradicate it? I dunno. Were we planning to eventually remove it? > @@ -320,6 +322,11 @@ int git_config_colorbool(const char *var, const char *value) > * Otherwise, we're looking at on-disk config; > * downgrade to auto. > */ > + if (!warn_once) { > + warn_once++; > + warning("setting '%s' to '%s' is no longer valid; " > + "set it to 'auto' instead", var, value); > + } This warn_once is sadly not enough to give non-annoying output to scripts that call many git commands. E.g.: $ git config color.ui always $ git add -p warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead diff --git a/file b/file [...] -Peff ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] color: discourage use of ui.color=always 2017-10-12 15:08 ` Jeff King @ 2017-10-13 0:02 ` Junio C Hamano 0 siblings, 0 replies; 42+ messages in thread From: Junio C Hamano @ 2017-10-13 0:02 UTC (permalink / raw) To: Jeff King; +Cc: git Jeff King <peff@peff.net> writes: > On Thu, Oct 12, 2017 at 11:10:07AM +0900, Junio C Hamano wrote: > >> Warn when we read such a configuration from a file, and nudge the >> users to spell them 'auto' instead. > > Hmm. On the one hand, it is nice to make people aware that their config > isn't doing what they might think. > > On the other hand, if "always" is no longer a problem for anybody, do we > need to force users to take the step to eradicate it? I dunno. Were we > planning to eventually remove it? > >> @@ -320,6 +322,11 @@ int git_config_colorbool(const char *var, const char *value) >> * Otherwise, we're looking at on-disk config; >> * downgrade to auto. >> */ >> + if (!warn_once) { >> + warn_once++; >> + warning("setting '%s' to '%s' is no longer valid; " >> + "set it to 'auto' instead", var, value); >> + } > > This warn_once is sadly not enough to give non-annoying output to > scripts that call many git commands. E.g.: > > $ git config color.ui always > $ git add -p > warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead > warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead > warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead > warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead > warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead > warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead > warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead > warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead > warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead > warning: setting 'color.ui' to 'always' is no longer valid; set it to 'auto' instead > diff --git a/file b/file > [...] I am ambivalent. We (especially you) have kept saying that "always" is a mistake that makes little sense, and wanted to force users to "fix" their configuration. But as you said, we made it not a mistake, so it is OK to leave it as they are, I guess. Let's drop the warning part of the change, at least. ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2017-10-18 5:57 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-09 23:58 What happened to "git status --color=(always|auto|never)"? Nazri Ramliy 2017-10-10 0:16 ` Jonathan Nieder 2017-10-10 0:43 ` Nazri Ramliy 2017-10-10 0:59 ` Jonathan Nieder 2017-10-10 4:42 ` Nazri Ramliy 2017-10-10 10:25 ` Jeff King 2017-10-10 12:51 ` Junio C Hamano 2017-10-10 13:06 ` Jeff King 2017-10-10 19:03 ` Jonathan Nieder 2017-10-10 19:37 ` Jeff King 2017-10-11 2:05 ` Junio C Hamano 2017-10-12 2:10 ` [PATCH 0/2] Piling more kludge on top of color.ui Junio C Hamano 2017-10-12 2:10 ` [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration Junio C Hamano 2017-10-12 4:47 ` Jonathan Nieder 2017-10-12 5:05 ` Junio C Hamano 2017-10-12 5:40 ` Jonathan Nieder 2017-10-12 6:15 ` Junio C Hamano 2017-10-12 6:58 ` Junio C Hamano 2017-10-12 13:06 ` Jeff King 2017-10-12 15:12 ` Jeff King 2017-10-12 12:31 ` Jeff King 2017-10-13 0:09 ` Junio C Hamano 2017-10-13 1:47 ` Jeff King 2017-10-13 3:37 ` Junio C Hamano 2017-10-13 13:06 ` Jeff King 2017-10-13 17:20 ` [PATCH 0/4] peeling back color.ui=always hacks Jeff King 2017-10-13 17:23 ` [PATCH 1/4] Revert "color: make "always" the same as "auto" in config" Jeff King 2017-10-13 17:23 ` [PATCH 2/4] Revert "t6006: drop "always" color config tests" Jeff King 2017-10-13 17:24 ` [PATCH 3/4] Revert "color: check color.ui in git_default_config()" Jeff King 2017-10-13 17:26 ` [PATCH 4/4] tag: respect color.ui config Jeff King 2017-10-14 3:01 ` [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration Junio C Hamano 2017-10-16 21:53 ` Jeff King 2017-10-17 1:06 ` Junio C Hamano 2017-10-17 6:26 ` Junio C Hamano 2017-10-18 5:28 ` Jeff King 2017-10-18 5:57 ` Junio C Hamano 2017-10-17 6:51 ` Jonathan Nieder 2017-10-18 5:34 ` Jeff King 2017-10-12 2:10 ` [PATCH 2/2] color: discourage use of ui.color=always Junio C Hamano 2017-10-12 4:48 ` Jonathan Nieder 2017-10-12 15:08 ` Jeff King 2017-10-13 0:02 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).