What happened to "git status --color=(always|auto|never)"?

What happened to "git status --color=(always|auto|never)"?

[Nazri Ramliy]

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

Re: What happened to "git status --color=(always|auto|never)"?

[Jonathan Nieder]

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

Re: What happened to "git status --color=(always|auto|never)"?

[Nazri Ramliy]

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

Re: What happened to "git status --color=(always|auto|never)"?

[Jonathan Nieder]

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

Re: What happened to "git status --color=(always|auto|never)"?

[Nazri Ramliy]

>         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.

Re: What happened to "git status --color=(always|auto|never)"?

[Jeff King]

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;
 	}

Re: What happened to "git status --color=(always|auto|never)"?

[Junio C Hamano]

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.

Re: What happened to "git status --color=(always|auto|never)"?

[Jeff King]

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.

Re: What happened to "git status --color=(always|auto|never)"?

[Jonathan Nieder]

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

Re: What happened to "git status --color=(always|auto|never)"?

[Jeff King]

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

Re: What happened to "git status --color=(always|auto|never)"?

[Junio C Hamano]

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.

[PATCH 0/2] Piling more kludge on top of color.ui

[Junio C Hamano]

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

[PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

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

[PATCH 2/2] color: discourage use of ui.color=always

[Junio C Hamano]

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

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jonathan Nieder]

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.

Re: [PATCH 2/2] color: discourage use of ui.color=always

[Jonathan Nieder]

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>

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

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

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jonathan Nieder]

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")),

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

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.

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

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

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jeff King]

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

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jeff King]

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

Re: [PATCH 2/2] color: discourage use of ui.color=always

[Jeff King]

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

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jeff King]

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

Re: [PATCH 2/2] color: discourage use of ui.color=always

[Junio C Hamano]

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.

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

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.

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jeff King]

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

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

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.

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jeff King]

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

[PATCH 0/4] peeling back color.ui=always hacks

[Jeff King]

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

[PATCH 1/4] Revert "color: make "always" the same as "auto" in config"

[Jeff King]

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

[PATCH 2/4] Revert "t6006: drop "always" color config tests"

[Jeff King]

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

[PATCH 3/4] Revert "color: check color.ui in git_default_config()"

[Jeff King]

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

[PATCH 4/4] tag: respect color.ui config

[Jeff King]

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

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

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.

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jeff King]

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

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

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:

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

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.

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jonathan Nieder]

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

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jeff King]

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

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Jeff King]

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

Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration

[Junio C Hamano]

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.