Updated to v2.14.2 on macOS; git add --patch broken

Hi there,

I updated to git version v2.14.2 on macOS using homebrew.

Since then git add --patch and git stash save --patch are not working anymore. It's just printing the complete diff without ever stopping to ask for actions. This results in an unusable state, as the whole command option is rendered useless.

For illustration see: https://asciinema.org/a/fw6cy9Ds6CeNOt4RYknLYgOD3

Kind Regards,
Toni

--
Toni Uebernickel

[email protected] - https://keybase.io/havvg
https://github.com/havvg - https://www.xing.com/profile/Toni_Uebernickel

What was the previous version that was working? It is possible to bisect to find the culprit commit?

There weren't any updates to git-add--interactive.perl (the program that underlies the --patch implementation in both cases) in v2.14.2, and very few even in v2.14.0. Of course the problem may be in one of the sub-programs it calls.

-Peff

The previous version was v2.13.2.
I switched back to this version, and it works perfectly fine; without any changes to my system.

--
Toni Uebernickel

[email protected] - https://keybase.io/havvg
https://github.com/havvg - https://www.xing.com/profile/Toni_Uebernickel

Thanks for confirming.

There aren't a lot of changes to the script between v2.13.2 and v2.14.2. The most plausible culprit is d5addcf522 (add--interactive: handle EOF in prompt_yesno, 2017-06-21), but I'm scratching my head over how that could cause what you're seeing.

Are you able to build Git from source and bisect the problem? It would help to know which commit introduced the problem.

-Peff

Hi,

How about this change?

commit 136c8c8b8fa39f1315713248473dececf20f8fe7
Author: Jeff King <[email protected]>
Date: Thu Jul 13 11:07:03 2017 -0400

color: check color.ui in git_default_config()

Toni, what is the output of "git config -l"?

Thanks,
Jonathan

Uh, I think I was thinking of another thread when I wrote this. Sorry for the nonsense.

I'm still curious about this.

Thanks,
Jonathan

Hi Jonathan,

my configuration reads as follows, I only removed private tokens content. I will try to get more details on which version exactly breaks the command.

Kind Regards,
Toni

core.excludesfile=/usr/local/etc/gitignore core.whitespace=trailing-space,space-before-tab,-indent-with-non-tab,tab-in-indent,tabwidth=2
core.autocrlf=input
core.ignorecase=true
gpg.program=gpg2
credential.helper=osxkeychain
diff.patience=true
diff.algorithm=patience
diff.compactionheuristic=true
notes.displayref=refs/notes/*
advice.detachedhead=false
pull.rebase=preserve
push.default=upstream
merge.ff=false
merge.summary=true
merge.log=false
rerere.enabled=true
rebase.autosquash=true
rebase.autostash=true
stash.showpatch=true
branch.autosetuprebase=always
color.ui=always
color.branch.current=yellow reverse
color.branch.local=yellow
color.branch.remote=green
color.diff.meta=yellow
color.diff.func=cyan bold
color.diff.frag=magenta
color.diff.old=red
color.diff.new=green
color.diff.whitespace=red reverse
color.status.added=green
color.status.changed=yellow
color.status.untracked=cyan
color.status.nobranch=red
pretty.changelog=format:* %h %s
pretty.public-changelog=format:* %s
pretty.pretty-history=format:%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr) %C(bold blue)<%an>%Creset gitflow.multi-hotfix=true
gitflow.prefix.feature=feature/
gitflow.prefix.release=release/
gitflow.prefix.bugfix=bugfix/
gitflow.prefix.hotfix=hotfix/
gitflow.prefix.support=support/
gitflow.prefix.versiontag=v
gitflow.feature.finish.rebase=true
gitflow.feature.finish.no-ff=true
gitflow.feature.finish.keepremote=true
gitflow.feature.finish.keeplocal=false
gitflow.release.finish.keepremote=true
gitflow.release.finish.keeplocal=false
gitflow.hotfix.finish.keepremote=true
gitflow.hotfix.finish.keeplocal=false
alias.st=status
alias.co=checkout
alias.di=diff
alias.ci=commit
alias.ci-rm=!git commit && git notes --ref=redmine add
alias.ci-gh=!git commit && git notes --ref=github.issues add
alias.ci-n=!git commit && git notes add
alias.amend=commit --amend
alias.update=add -u
alias.up-head=!f() { git diff-tree --no-commit-id --name-only --diff-filter=ACMR -r HEAD; }; git add -p f alias.staged=diff --cached
alias.ls-staged=diff-index --cached --name-only HEAD
alias.di-staged=!git diff-index --cached --name-only --diff-filter=ACMR HEAD | xargs git di
alias.co-staged=!git diff-index --cached --name-only --diff-filter=ACMR HEAD | xargs git co
alias.up-staged=!f() { git ls-staged; }; git add -p f
alias.coding-standards=!php-cs-fixer fix
alias.cs=!f() { git diff-tree --no-commit-id --name-only --diff-filter=ACMR -r "$1" | xargs -n1 git coding-standards; }; f alias.cs-staged=!git diff-index --cached --name-only --diff-filter=ACMR HEAD | xargs -n1 git coding-standards alias.cs-head=!git diff-tree --no-commit-id --name-only --diff-filter=ACMR -r HEAD | xargs -n1 git coding-standards alias.cs-tree=!f() { git ls-tree -r --name-only HEAD "$1" | xargs -n1 git coding-standards; }; f
alias.ph=log --graph --pretty=pretty-history --abbrev-commit --date=relative
alias.cl=log --pretty=changelog --no-merges --cherry --abbrev-commit --date-order
alias.sl=log --oneline --reverse --no-merges --stat
alias.rv=remote -v
alias.fa=fetch --all -v
alias.ff=pull --ff-only
alias.wu=branch -a --no-merged
alias.latest-version=!git tag --sort=v:refname | tail -n1
alias.pt=push --tags origin master:master develop:develop
git-up.fetch.prune=true
git-up.fetch.all=true
filter.lfs.clean=git-lfs clean %f
filter.lfs.smudge=git-lfs smudge %f
filter.lfs.required=true
user.name=Toni Uebernickel
[email protected]
user.signingkey=F6035C7A
core.editor=mate -w -l 1
commit.gpgsign=true
github.user=havvg
github.token=PRIVATE_CREDENTIALS_REMOVED
[email protected]
travis.token=PRIVATE_CREDENTIALS_REMOVED
alias.review=!f() { git ph --author="$1" ; }; f
alias.rf=!f() { git flow feature track "$1" && git co feature/"$1" && git ff && git flow finish; }; f
alias.upstream=!git merge --ff-only upstream/`git branch --list --no-color | grep -e '^\*' | cut -d" " -f2 core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
core.ignorecase=true
core.precomposeunicode=true
[email protected]:tarifhaus/thengine.git
remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
branch.develop.remote=origin
branch.develop.merge=refs/heads/develop
branch.develop.rebase=true
commit.gpgsign=false
branch.master.remote=origin
branch.master.merge=refs/heads/master
branch.master.rebase=true
gitflow.branch.master=master
gitflow.branch.develop=develop gitflow.path.hooks=/Users/toni.uebernickel/Development/Tarifhaus/repositories/thengine/.git/hooks [email protected]
gitflow.branch.feature/ITTPL-36-coupon-token-email-limit.base=develop gitflow.branch.feature/ITTPL-49-command-locking.base=develop
gitflow.branch.release/2.5.0.base=develop
gitflow.branch.feature/ITTHE-243-landingpage-content-tags.base=develop
gitflow.branch.release/2.6.0.base=develop
gitflow.branch.feature/ITTHE-246-json-web-token.base=develop
gitflow.branch.release/2.7.0.base=develop
gitflow.branch.feature/ITTHE-249-post-request-daten.base=develop
gitflow.branch.feature/ITTHE-248-users-api.base=develop
gitflow.branch.release/2.8.0.base=develop
gitflow.branch.feature/ITTPL-92-url-shortener.base=develop
gitflow.branch.release/2.9.0.base=develop
gitflow.branch.feature/ITTHE-255-basic-orders-api.base=develop gitflow.branch.feature/ITTHE-260-migrate-databases-helper.base=develop
gitflow.branch.feature/ITTHE-257-signup.base=develop
gitflow.branch.feature/ITTHE-256-api-profile.base=develop
gitflow.branch.feature/ITTHE-267-convert-to-testdox.base=develop
gitflow.branch.release/2.10.0.base=develop
gitflow.branch.feature/ITTPL-137-besser-mobile-portal.base=develop gitflow.branch.feature/ITTHE-251-HTTP-500-api-problem.base=develop gitflow.branch.feature/ITTHE-272-message-queue.base=develop gitflow.branch.feature/ITTBO-7-einfache-faq-verwaltung.base=hotfix/2.9.2 gitflow.branch.feature/Tarifporfolio-V3.base=master
gitflow.branch.release/2.11.0.base=develop
gitflow.branch.release/2.12.0.base=develop
branch.support/ITTHE-262.remote=origin
branch.support/ITTHE-262.merge=refs/heads/support/ITTHE-262
branch.support/ITTHE-262.rebase=true
gitflow.branch.feature/ITTHE-277-change-password.base=develop
gitflow.branch.feature/ITTHE-280-TEF-MyPay.base=develop gitflow.branch.feature/ITTHE-292-msisdn-on-contract-activation.base=release/2.14.0 gitflow.branch.feature/ITTHE-280-mypay-vouchercode-transactions.base=release/2.14.0 branch.feature/ITTHE-280-mypay-vouchercode-transactions.remote=origin branch.feature/ITTHE-280-mypay-vouchercode-transactions.merge=refs/heads/feature/ITTHE-280-mypay-vouchercode-transactions branch.feature/ITTHE-280-mypay-vouchercode-transactions.rebase=true gitflow.branch.feature/ITTHE-293-translate-security-error-messages.base=develop gitflow.branch.feature/ITTHE-295-combined-contract-endpoint.base=release/2.14.0 gitflow.branch.feature/ITTHE-294-simcard-activation.base=release/2.14.0
branch.support/json-rpc-form-type.remote=origin branch.support/json-rpc-form-type.merge=refs/heads/support/json-rpc-form-type
branch.support/json-rpc-form-type.rebase=true
gitflow.branch.feature/ITTHE-310-user-profile-for-existing-users.base=develop gitflow.branch.feature/ITTHE-312-refactor-obtaining-vouchers.base=develop gitflow.branch.feature/ITTHE-298-vouchers-for-user-subset.base=develop gitflow.branch.feature/ITTHE-313-send-signup-message.base=develop
gitflow.branch.release/2.16.0.base=develop
gitflow.branch.release/2.17.0.base=develop
gitflow.branch.feature/ITTHE-315-explicit-persist-attribute-value.base=develop
gitflow.branch.release/2.18.0.base=develop
gitflow.branch.feature/ITTPL-350-bcc-club-email-to-lbe.base=develop gitflow.branch.feature/ITTHE-321-promotion-voucher-on-contract-basis.base=develop gitflow.branch.feature/ITTHE-325-lock-voucher-when-obtaining.base=develop
gitflow.branch.release/2.19.0.base=develop
gitflow.branch.hotfix/2.19.1.base=master
gitflow.branch.feature/ITTHE-339-api-access-control.base=develop
gitflow.branch.release/2.20.0.base=develop
gitflow.branch.feature/ITTHE-334-promotion-for-all-customers.base=develop gitflow.branch.feature/ITTHE-331-behat-context-api-problem.base=develop gitflow.branch.feature/ITTHE-336-canonical-user-email-address.base=develop gitflow.branch.feature/ITTHE-138-address-validation-setup-wrapper.base=develop lfs.https://gitlab.tarifhaus.ag/tarifhaus/thengine.git/info/lfs.locksverify=false gitflow.branch.feature/ITTHE-351-behat-assert-response-content.base=develop
gitflow.branch.release/2.21.0.base=develop
gitflow.branch.feature/ITTHE-355-refactor-chat-api.base=develop gitflow.branch.feature/ITADM-48-feature-deployments.base=develop
gitflow.branch.release/2.22.0.base=develop
gitflow.branch.feature/ITTHE-360-media-service.base=develop
branch.feature/ITTHE-360-media-service.remote=origin branch.feature/ITTHE-360-media-service.merge=refs/heads/feature/ITTHE-360-media-service branch.feature/ITTHE-360-media-service.rebase=true
gitflow.branch.feature/tarifhaus-club-support.base=develop gitflow.branch.feature/ITTHE-370-open-promotions-not-listed.base=develop gitflow.branch.feature/ITTHE-371-support-registration-vouchers.base=develop
gitflow.branch.release/2.23.0.base=develop
gitflow.branch.feature/ITTHE-375-support-promotion-participants.base=develop gitflow.branch.feature/ITTHE-288-aboalarm-bank-account-check.base=develop
gitflow.branch.release/2.25.0.base=develop branch.feature/ITTHE-384-products-and-attributes-new-portal-strategy.remote=origin branch.feature/ITTHE-384-products-and-attributes-new-portal-strategy.merge=refs/heads/feature/ITTHE-384-products-and-attributes-new-portal-strategy branch.feature/ITTHE-384-products-and-attributes-new-portal-strategy.rebase=true gitflow.branch.feature/ITTHE-388-json-rpc.base=develop gitflow.branch.feature/ITTHE-382-anonymise-order-notification.base=develop
branch.feature/ITTHE-388-json-rpc.remote=origin branch.feature/ITTHE-388-json-rpc.merge=refs/heads/feature/ITTHE-388-json-rpc
branch.feature/ITTHE-388-json-rpc.rebase=true
gitflow.branch.feature/ITTHE-372-vouchers-on-monthly-basis.base=develop
branch.feature/ITTHE-385-timeline.remote=origin branch.feature/ITTHE-385-timeline.merge=refs/heads/feature/ITTHE-385-timeline
branch.feature/ITTHE-385-timeline.rebase=true
gitflow.branch.release/2.26.0.base=develop
branch.release/2.26.0.remote=origin
branch.release/2.26.0.merge=refs/heads/release/2.26.0
branch.release/2.26.0.rebase=true
branch.feature/ITTHE-386-member-connections.remote=origin branch.feature/ITTHE-386-member-connections.merge=refs/heads/feature/ITTHE-386-member-connections branch.feature/ITTHE-386-member-connections.rebase=true

--
Toni Uebernickel

[email protected] - https://keybase.io/havvg
https://github.com/havvg - https://www.xing.com/profile/Toni_Uebernickel

This is the problem, and Jonathan's guess was correct that 136c8c8b8f (color: check color.ui in git_default_config(), 2017-07-13) is related.

Re-reading that commit message, I'm inclined to say that the commit isn't wrong, and that setting color.ui to "always" has always been a bad idea. Of course, I also wrote that commit message, so I may be biased. :)

What led you to setting color.ui to "always" in your config? The more usual value is "auto" (which is also the default these days).

-Peff

That's a very very old setting :)
Honestly, I don't know why it's "always". I have set up this setting years ago and never thought about it again, as it worked out.

I changed it to "auto" and the --patch options are working again on v2.14.2!

Thank you very much for your time & efforts.
If I can be of any further help on this matter, just let me know.

Kind Regards,
Toni

--
Toni Uebernickel

[email protected] - https://keybase.io/havvg
https://github.com/havvg - https://www.xing.com/profile/Toni_Uebernickel

Hi,

Would a patch like the following help?

I am worried that other scripts using diff-files would need the same kind of patch. So it seems worthwhile to look for alternatives.

An alternative would be to partially roll back v2.14.2~61^2~4 (color:
check color.ui in git_default_config, 2017-07-13) by making it not take effect for plumbing commands (i.e., by adding a boolean to "struct startup_info" to indicate whether a command is low-level plumbing). That would make the behavior of Git harder to explain so I don't particularly like it. Plus it defeats the point of the patch.

Yet another alternative would be to treat color.ui=always as a deprecated synonym for color.ui=auto. I think that's my preferred fix.

What do you think?

Thanks again for reporting,
Jonathan

index 28b325d754..4ea69538c7 100755
@@ -101,49 +101,49 @@ sub apply_patch_for_stash;
 
 my %patch_modes = (
 	'stage' => {
-		DIFF => 'diff-files -p',
+		DIFF => 'diff-files --no-color -p',
 		APPLY => sub { apply_patch 'apply --cached', @_; },
 		APPLY_CHECK => 'apply --cached',
 		FILTER => 'file-only',
 		IS_REVERSE => 0,
 	},
 	'stash' => {
-		DIFF => 'diff-index -p HEAD',
+		DIFF => 'diff-index --no-color -p HEAD',
 		APPLY => sub { apply_patch 'apply --cached', @_; },
 		APPLY_CHECK => 'apply --cached',
 		FILTER => undef,
 		IS_REVERSE => 0,
 	},
 	'reset_head' => {
-		DIFF => 'diff-index -p --cached',
+		DIFF => 'diff-index --no-color -p --cached',
 		APPLY => sub { apply_patch 'apply -R --cached', @_; },
 		APPLY_CHECK => 'apply -R --cached',
 		FILTER => 'index-only',
 		IS_REVERSE => 1,
 	},
 	'reset_nothead' => {
-		DIFF => 'diff-index -R -p --cached',
+		DIFF => 'diff-index --no-color -R -p --cached',
 		APPLY => sub { apply_patch 'apply --cached', @_; },
 		APPLY_CHECK => 'apply --cached',
 		FILTER => 'index-only',
 		IS_REVERSE => 0,
 	},
 	'checkout_index' => {
-		DIFF => 'diff-files -p',
+		DIFF => 'diff-files --no-color -p',
 		APPLY => sub { apply_patch 'apply -R', @_; },
 		APPLY_CHECK => 'apply -R',
 		FILTER => 'file-only',
 		IS_REVERSE => 1,
 	},
 	'checkout_head' => {
-		DIFF => 'diff-index -p',
+		DIFF => 'diff-index --no-color -p',
 		APPLY => sub { apply_patch_for_checkout_commit '-R', @_ },
 		APPLY_CHECK => 'apply -R',
 		FILTER => undef,
 		IS_REVERSE => 1,
 	},
 	'checkout_nothead' => {
-		DIFF => 'diff-index -R -p',
+		DIFF => 'diff-index --no-color -R -p',
 		APPLY => sub { apply_patch_for_checkout_commit '', @_ },
 		APPLY_CHECK => 'apply',
 		FILTER => undef,

It is mine, too. And I do not think color.ui=absolutely-always for those who want to keep the current behaviour is unneeded.

Having said that, I do not mind solving what 136c8c8b ("color: check color.ui in git_default_config()", 2017-07-13) tried to do in a different way. If 4c7f1819b that made even plumbing to default to auto was wrong (because plumbing did not pay attention to color.ui so people could not override that 'auto' default), we can partially revert 4c7f1819 ("make color.ui default to 'auto'", 2013-06-10) to make the default 'auto' not apply to plumbing.

In any case, I think the first step may be to revert 136c8c8b from both 'master' and 2.14.x. These alternative solutions can come on top.

Thoughts?

With the attached patch, after reverting 136c8c8b ("color: check color.ui in git_default_config()", 2017-07-13) from the tip of 'master', all tests seem to pass. More importantly,

git -c color.ui=always diff-files -p >not-a-tty

will not get colors on its output file, because it does not pay attention to color.ui configuration.

Note that I haven't _decided_ that reverting is the best way to move forward (yet). I am just giving a datapoint for people to use when they assess how painful each possible avenues proposed are.

 builtin/for-each-ref.c | 3 ++-
 builtin/tag.c          | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

index 5d7c921a77..238eb00e09 100644
@@ -5,6 +5,7 @@
 #include "object.h"
 #include "parse-options.h"
 #include "ref-filter.h"
+#include "color.h"
 
 static char const * const for_each_ref_usage[] = {
 	N_("git for-each-ref [<options>] [<pattern>]"),

@@ -54,7 +55,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 
 	format.format = "%(objectname) %(objecttype)\t%(refname)";
 
-	git_config(git_default_config, NULL);
+	git_config(git_color_default_config, NULL);
 
 	parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);
 	if (maxcount < 0) {

index c627794181..f8bc1393ed 100644
@@ -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)

I'd prefer not to revert. I think setting any of the color config to "always" in an on-disk file is basically a broken config. It was exacerbated by 4c7f1819b, but it was already broken for scripts that call "git log" or "git diff", or even just something as simple as piping those programs on the command line.

The two reasonable paths forward to me are:

1. Do nothing. Putting "color.ui = always" in your on-disk config is a bad idea, and the right fix is to stop doing it.

2. Make "always" a synonym for "auto". This has the advantage over (1) that you can't shoot yourself in the foot with it (so the expanded foot-shooting capabilities of 4c7f1819b don't matter). The disadvantage is that you can no longer do:

git -c color.ui=always foo >not-a-tty

to ask for color in all sub-programs of "foo". I have no idea if anybody cares. I came up with that example in 4c7f1819b as the most plausible reason somebody might actually care about "always", but I've never used it myself.

And there _is_ a way to get the same thing, which is:

GIT_PAGER_IN_USE=1 git foo >not-a-tty

I.e., stay in "auto" but make the claim that "auto" really ought to be showing color because the output is going to be consumed eventually by a human. While it looks a bit funny in my made-up example above (because the variables says "pager" but we're not actually piping to a pager directly), I think this is the most plausible reason that an actual program might want to override the auto-color decision.

-Peff

I actually disagree with that reasoning.

We've promised that plumbing commands were safe to use in scripts, and 4c7f1819 ("make color.ui default to 'auto'", 2013-06-10) got it closer to breaking it (but not quite), and 136c8c8b finally broke it. Setting ui.color=always and shooting themselves in the foot by seeing ANSI escapes in "git log >file" output is totally user's choice. Breaking scripts that carefully chose to use plumbing, believing our earlier promise, and blaming user's ui.color=always does not sound quite like the same thing, at least to me.

I agree it's not quite the same thing, and I agree the problem was made much worse by 4c7f1819b. But I still think color.ui=always is inherently a foot-gun, and in either case it is the user that sets it that is harmed (and they are the ones who have the power to fix it).

At any rate, I think you only need to agree with that reason to buy into my "do nothing" option.

Option 2 would make everybody happy, I think (I'm working up a patch, though note that it isn't quite trivial because of some abuses of the color system in the test scripts).

-Peff

Yeah, but it is inherently a foot-gun only for those who write scripts around porcelain commands, which are expected to honor color.ui=always. If you write a script using the plumbing commands because you did not want to get broken by color.ui=always, and then your script gets broken because plumbing commands you relied on suddenly start paying attention to color.ui---is that the user's fault who used color.ui?

The end-users have the power to work the breakage around by not using "always". The script writers have the power to work the breakage around with "--no-color". But these are workarounds that shouldn't have been needed in the first place, no?

Note that I'm arguing that it's a foot-gun even without scripts in the picture at all. Forget about plumbing versus porcelain. If you set color.ui to "always", you're going to get unexpected and confusing results from time to time.

To be clear, I don't think script writers should work around it at all. It is either the fault of the user for having a broken config, or Git for allowing such a broken config (and after having studied the documentation, I think Git really is not helping here; nobody should ever use "always", but the documentation introduces it as the most obvious option to choose).

-Peff

Really?

I would think you would consistently get ANSI colored output in any medium, even in files that you would later "cat" or "less -R" to view. Is that unexpected? Those who set "always" (I am not among them, of course) would expect that, I would think.

Those cases might be expected. But color when piping to grep or sed are not. I guess you can lump those under "well, they should be using plumbing, of course" but I don't think that's very realistic. People do ad-hoc pipes in their shells all the time (well, I assume so; I certainly do).

I don't argue that people don't have a need to write colors to a non-terminal. Certainly they do. I argue that setting it in your on-disk config is likely to have it trigger at other times when it's unexpected and inconvenient.

I dunno. Maybe I am wrong, because certainly I never set it. We've had two reports on the list since v2.14.2. The motivation for the first was "I have no idea why I set that, and I'll switch to auto". This is the second.

-Peff

I also don't remember why I set it, and as such I removed it.

A helpful hint as to a bad config option would've been great. Something along the lines of "The use of color.ui = always is not recommended", with a flag allowing you to disable said warning.

Thanks,

Tsvi
Tsvi Mostovicz
[email protected]
www.linkedin.com/in/tsvim

That's an argument to say color.ui=always is not a useful thing to use and Git is wrong to offer such a nonsense option. I agree with both of them.

But it is a different matter that plumbing commands are now doing the "color" thing without being asked. Reading the configuration that is meant for human interaction adds insult to injury. I think the earlier "everybody is colored by default" that forgot to make sure the change does not affect plumbing was the main culprit, and we were merely lucky that thanks to the auto-detection of "auto" we did not break scripts.

Having said all that, unless we revert the entire series (and possibly other things that happened after the series was merged on 'master' that assumes that now default_config would read the color.ui thing), we won't be able to get back to the state before the "add -p" regression, it seems. As -rc freeze period for the next cycle is approaching fast, I wanted to make sure that we have a plan to address the regression (which does not have to solve what the reverted commit tried to solve). If you think we can get a workable code in 2.14.x and 'master' that essentially castrates "always" and makes it the same as "auto" in several days tops, then I'd prefer such a solution, as honoring "color.ui=always" does not feel all that important.

Yes, I agree that the "everybody is colored by default" is the root of the problem. And in that sense all of this discussion is band-aid fixes on that.

At the same time, I have a suspicion that "even plumbing is color=auto by default" may actually be _helping_ in many scripts. Because they to use the plumbing to show output to the user, respecting the user's normal color preferences. That's just a hunch, though.

I also think trying to revert that "default" patch (4c7f1819) may be a pretty big pain at this point.

Side note: Sorry, I noticed while writing this that I got my commit references mixed up earlier between 4c7f1819 (which turned the default to auto even for plumbing) and 136c8c8b, my recent change to parse color.ui in more places. When I said the problem was exacerbated/made worse by 4c7f1819, I really meant 136c8c8b. Hopefully that didn't confuse the discussion too much.

Here's what I came up with on the "color.ui=always is nonsense that we should not offer" front. The number of patches may be a little daunting, but most of them are just removing cases of "git -c color.ui=always" from the tests.

[01/12]: test-terminal: set TERM=vt100
[02/12]: t4015: prefer --color to -c color.diff=always [03/12]: t4015: use --color with --color-moved [04/12]: t3701: use test-terminal to collect color output [05/12]: t7508: use test_terminal for color output [06/12]: t7502: use diff.noprefix for --verbose test [07/12]: t6006: drop "always" color config tests [08/12]: t3203: drop "always" color test [09/12]: t7301: use test_terminal to check color [10/12]: t3205: use --color instead of color.branch=always [11/12]: provide --color option for all ref-filter users [12/12]: color: make "always" the same as "auto" in config

 Documentation/config.txt           | 35 ++++++++++++-------------
 Documentation/git-for-each-ref.txt |  5 ++++
 Documentation/git-tag.txt          |  5 ++++
 builtin/for-each-ref.c             |  1 +
 builtin/tag.c                      |  1 +
 color.c                            |  2 +-
 t/t3203-branch-output.sh           |  8 +-----
 t/t3205-branch-color.sh            |  5 ++--
 t/t3701-add-interactive.sh         | 18 +++++++++----
 t/t4015-diff-whitespace.sh         | 53 +++++++++++++++++++-------------------
 t/t4202-log.sh                     |  2 +-
 t/t6006-rev-list-format.sh         | 23 +++++------------
 t/t6300-for-each-ref.sh            |  7 +++--
 t/t7004-tag.sh                     |  6 ++---
 t/t7006-pager.sh                   |  6 ++---
 t/t7301-clean-interactive.sh       |  5 ++--
 t/t7502-commit.sh                  |  4 +--
 t/t7508-status.sh                  | 41 +++++++++++++++--------------
 t/test-terminal.perl               |  1 +
 19 files changed, 115 insertions(+), 113 deletions(-)

-Peff

The point of the test-terminal script is to simulate in the test scripts an environment where output is going to a real terminal.

But since test-lib.sh also sets TERM=dumb, the simulation isn't very realistic. The color code will skip auto-coloring for TERM=dumb, leading to us liberally sprinkling

test_terminal env TERM=vt100 git ...

through the test suite to convince the tests to actually generate colors. Let's set TERM for programs run under test_terminal, which is one less thing for test-writers to remember.

In most cases the callers can be simplified, but note there is one interesting case in t4202. It uses test_terminal to check the auto-enabling of --decorate, but the expected output _doesn't_ contain colors (because TERM=dumb suppresses them). Using TERM=vt100 is closer to what the real world looks like; adjust the expected output to match.

---
Not strictly necessary for this series, but after banging my shins on this nuisance for the umpteenth time, I decided to finally do something about it.

 t/t3203-branch-output.sh   | 2 +-
 t/t4202-log.sh             | 2 +-
 t/t6006-rev-list-format.sh | 3 +--
 t/t6300-for-each-ref.sh    | 3 +--
 t/t7004-tag.sh             | 2 +-
 t/t7006-pager.sh           | 6 +++---
 t/test-terminal.perl       | 1 +
 7 files changed, 9 insertions(+), 10 deletions(-)

index d2aec0f38b..86286f263d 100755
@@ -253,7 +253,7 @@ test_expect_success '%(color) omitted without tty' '
 '
 
 test_expect_success TTY '%(color) present with tty' '
-	test_terminal env TERM=vt100 git branch $color_args >actual.raw &&
+	test_terminal git branch $color_args >actual.raw &&
 	test_decode_color <actual.raw >actual &&
 	test_cmp expect.color actual
 '

index 36d120c969..8f155da7a5 100755
@@ -750,7 +750,7 @@ test_expect_success 'log.decorate config parsing' '
 '
 
 test_expect_success TTY 'log output on a TTY' '
-	git log --oneline --decorate >expect.short &&
+	git log --color --oneline --decorate >expect.short &&
 
 	test_terminal git log --oneline >actual &&
 	test_cmp expect.short actual

index b326d550f3..98be78b4a2 100755
@@ -229,8 +229,7 @@ do
 	'
 
 	test_expect_success TTY "$desc respects --color=auto (stdout is tty)" '
-		test_terminal env TERM=vt100 \
-			git log --format=$color -1 --color=auto >actual &&
+		test_terminal git log --format=$color -1 --color=auto >actual &&
 		has_color actual
 	'
 

index 2274a4b733..d6bffe6273 100755
@@ -425,8 +425,7 @@ test_expect_success 'set up color tests' '
 '
 
 test_expect_success TTY '%(color) shows color with a tty' '
-	test_terminal env TERM=vt100 \
-		git for-each-ref --format="$color_format" >actual.raw &&
+	test_terminal git for-each-ref --format="$color_format" >actual.raw &&
 	test_decode_color <actual.raw >actual &&
 	test_cmp expected.color actual
 '

index b545c33f83..0a86f8ea39 100755
@@ -1907,7 +1907,7 @@ test_expect_success '%(color) omitted without tty' '
 '
 
 test_expect_success TTY '%(color) present with tty' '
-	test_terminal env TERM=vt100 git tag $color_args >actual.raw &&
+	test_terminal git tag $color_args >actual.raw &&
 	test_decode_color <actual.raw >actual &&
 	test_cmp expect.color actual
 '

index 9128ec5acd..f0f1abd1c2 100755
@@ -239,7 +239,7 @@ test_expect_success 'no color when stdout is a regular file' '
 test_expect_success TTY 'color when writing to a pager' '
 	rm -f paginated.out &&
 	test_config color.ui auto &&
-	test_terminal env TERM=vt100 git log &&
+	test_terminal git log &&
 	colorful paginated.out
 '
 

@@ -247,7 +247,7 @@ test_expect_success TTY 'colors are suppressed by color.pager' '
 	rm -f paginated.out &&
 	test_config color.ui auto &&
 	test_config color.pager false &&
-	test_terminal env TERM=vt100 git log &&
+	test_terminal git log &&
 	! colorful paginated.out
 '
 

@@ -266,7 +266,7 @@ test_expect_success 'color when writing to a file intended for a pager' '
 test_expect_success TTY 'colors are sent to pager for external commands' '
 	test_config alias.externallog "!git log" &&
 	test_config color.ui auto &&
-	test_terminal env TERM=vt100 git -p externallog &&
+	test_terminal git -p externallog &&
 	colorful paginated.out
 '
 

index 96b6a03e1c..46bf618479 100755
@@ -80,6 +80,7 @@ sub copy_stdio {
 if ($#ARGV < 1) {
 	die "usage: test-terminal program args";
 }
+$ENV{TERM} = 'vt100';
 my $master_in = new IO::Pty;
 my $master_out = new IO::Pty;
 my $master_err = new IO::Pty;

2.14.2.1079.gce6b466188

t4015 contains many color-related tests which need to override the "is stdout a tty" check. They do so by setting the color.diff config, but we can accomplish the same with the --color option. Besides being shorter to type, switching will prepare us for upcoming changes to "always" when see it in config.

---
 t/t4015-diff-whitespace.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

index 12d182dc1b..b9df886e37 100755
@@ -821,7 +821,7 @@ test_expect_success 'diff that introduces a line with only tabs' '
 	echo "test" >x &&
 	git commit -m "initial" x &&
 	echo "{NTN}" | tr "NT" "\n\t" >>x &&
-	git -c color.diff=always diff | test_decode_color >current &&
+	git diff --color | test_decode_color >current &&
 
 	cat >expected <<-\EOF &&
 	<BOLD>diff --git a/x b/x<RESET>

@@ -851,7 +851,7 @@ test_expect_success 'diff that introduces and removes ws breakages' '
 		echo "2. and a new line "
 	} >x &&
 
-	git -c color.diff=always diff |
+	git diff --color |
 	test_decode_color >current &&
 
 	cat >expected <<-\EOF &&

@@ -923,15 +923,15 @@ test_expect_success 'ws-error-highlight test setup' '
 
 test_expect_success 'test --ws-error-highlight option' '
 
-	git -c color.diff=always diff --ws-error-highlight=default,old |
+	git diff --color --ws-error-highlight=default,old |
 	test_decode_color >current &&
 	test_cmp expect.default-old current &&
 
-	git -c color.diff=always diff --ws-error-highlight=all |
+	git diff --color --ws-error-highlight=all |
 	test_decode_color >current &&
 	test_cmp expect.all current &&
 
-	git -c color.diff=always diff --ws-error-highlight=none |
+	git diff --color --ws-error-highlight=none |
 	test_decode_color >current &&
 	test_cmp expect.none current
 

@@ -939,15 +939,15 @@ test_expect_success 'test --ws-error-highlight option' '
 
 test_expect_success 'test diff.wsErrorHighlight config' '
 
-	git -c color.diff=always -c diff.wsErrorHighlight=default,old diff |
+	git -c diff.wsErrorHighlight=default,old diff --color |
 	test_decode_color >current &&
 	test_cmp expect.default-old current &&
 
-	git -c color.diff=always -c diff.wsErrorHighlight=all diff |
+	git -c diff.wsErrorHighlight=all diff --color |
 	test_decode_color >current &&
 	test_cmp expect.all current &&
 
-	git -c color.diff=always -c diff.wsErrorHighlight=none diff |
+	git -c diff.wsErrorHighlight=none diff --color |
 	test_decode_color >current &&
 	test_cmp expect.none current
 

@@ -955,18 +955,18 @@ test_expect_success 'test diff.wsErrorHighlight config' '
 
 test_expect_success 'option overrides diff.wsErrorHighlight' '
 
-	git -c color.diff=always -c diff.wsErrorHighlight=none \
-		diff --ws-error-highlight=default,old |
+	git -c diff.wsErrorHighlight=none \
+		diff --color --ws-error-highlight=default,old |
 	test_decode_color >current &&
 	test_cmp expect.default-old current &&
 
-	git -c color.diff=always -c diff.wsErrorHighlight=default \
-		diff --ws-error-highlight=all |
+	git -c diff.wsErrorHighlight=default \
+		diff --color --ws-error-highlight=all |
 	test_decode_color >current &&
 	test_cmp expect.all current &&
 
-	git -c color.diff=always -c diff.wsErrorHighlight=all \
-		diff --ws-error-highlight=none |
+	git -c diff.wsErrorHighlight=all \
+		diff --color --ws-error-highlight=none |
 	test_decode_color >current &&
 	test_cmp expect.none current
 

2.14.2.1079.gce6b466188

The tests for --color-moved write their output to a file, but doing so suppresses color output under "auto". Right now this is solved by running the whole script under "color.diff=always". In preparation for the behavior of "always" changing, let's explicitly enable color.

---
I kind of think --color-moved should imply --color, as --color-words seems to. But that seemed like a potential rabbit-hole, and this series is already contentious enough.

 t/t4015-diff-whitespace.sh | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

index b9df886e37..3bca958863 100755
@@ -802,7 +802,6 @@ test_expect_success 'combined diff with autocrlf conversion' '
 # Start testing the colored format for whitespace checks
 
 test_expect_success 'setup diff colors' '
-	git config color.diff always &&
 	git config color.diff.plain normal &&
 	git config color.diff.meta bold &&
 	git config color.diff.frag cyan &&

@@ -986,7 +985,7 @@ test_expect_success 'detect moved code, complete file' '
 	git mv test.c main.c &&
 	test_config color.diff.oldMoved "normal red" &&
 	test_config color.diff.newMoved "normal green" &&
-	git diff HEAD --color-moved=zebra --no-renames | test_decode_color >actual &&
+	git diff HEAD --color-moved=zebra --color --no-renames | test_decode_color >actual &&
 	cat >expected <<-\EOF &&
 	<BOLD>diff --git a/main.c b/main.c<RESET>
 	<BOLD>new file mode 100644<RESET>

@@ -1087,7 +1086,7 @@ test_expect_success 'detect malicious moved code, inside file' '
 			bar();
 		}
 	EOF
-	git diff HEAD --no-renames --color-moved=zebra| test_decode_color >actual &&
+	git diff HEAD --no-renames --color-moved=zebra --color | test_decode_color >actual &&
 	cat <<-\EOF >expected &&
 	<BOLD>diff --git a/main.c b/main.c<RESET>
 	<BOLD>index 27a619c..7cf9336 100644<RESET>

@@ -1136,7 +1135,7 @@ test_expect_success 'plain moved code, inside file' '
 	test_config color.diff.oldMovedAlternative "blue" &&
 	test_config color.diff.newMovedAlternative "yellow" &&
 	# needs previous test as setup
-	git diff HEAD --no-renames --color-moved=plain| test_decode_color >actual &&
+	git diff HEAD --no-renames --color-moved=plain --color | test_decode_color >actual &&
 	cat <<-\EOF >expected &&
 	<BOLD>diff --git a/main.c b/main.c<RESET>
 	<BOLD>index 27a619c..7cf9336 100644<RESET>

@@ -1227,7 +1226,7 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
 	test_config color.diff.newMovedDimmed "normal cyan" &&
 	test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
 	test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
-	git diff HEAD --no-renames --color-moved=dimmed_zebra |
+	git diff HEAD --no-renames --color-moved=dimmed_zebra --color |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&

@@ -1271,7 +1270,7 @@ test_expect_success 'cmd option assumes configured colored-moved' '
 	test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
 	test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
 	test_config diff.colorMoved zebra &&
-	git diff HEAD --no-renames --color-moved |
+	git diff HEAD --no-renames --color-moved --color |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&

@@ -1343,7 +1342,7 @@ line 4
 EOF
 	test_config color.diff.oldMoved "magenta" &&
 	test_config color.diff.newMoved "cyan" &&
-	git diff HEAD --no-renames --color-moved |
+	git diff HEAD --no-renames --color-moved --color |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&

@@ -1364,7 +1363,7 @@ EOF
 	EOF
 	test_cmp expected actual &&
 
-	git diff HEAD --no-renames -w --color-moved |
+	git diff HEAD --no-renames -w --color-moved --color |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&

@@ -1403,7 +1402,7 @@ test_expect_success '--color-moved block at end of diff output respects MIN_ALNU
 	irrelevant_line
 	EOF
 
-	git diff HEAD --color-moved=zebra --no-renames |
+	git diff HEAD --color-moved=zebra --color --no-renames |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat >expected <<-\EOF &&

@@ -1442,7 +1441,7 @@ test_expect_success '--color-moved respects MIN_ALNUM_COUNT' '
 	nineteen chars 456789
 	EOF
 
-	git diff HEAD --color-moved=zebra --no-renames |
+	git diff HEAD --color-moved=zebra --color --no-renames |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat >expected <<-\EOF &&

@@ -1485,7 +1484,7 @@ test_expect_success '--color-moved treats adjacent blocks as separate for MIN_AL
 	7charsA
 	EOF
 
-	git diff HEAD --color-moved=zebra --no-renames | grep -v "index" | test_decode_color >actual &&
+	git diff HEAD --color-moved=zebra --color --no-renames | grep -v "index" | test_decode_color >actual &&
 	cat >expected <<-\EOF &&
 	<BOLD>diff --git a/bar b/bar<RESET>
 	<BOLD>--- a/bar<RESET>

@@ -1519,7 +1518,7 @@ test_expect_success 'move detection with submodules' '
 	echo foul >bananas/recipe &&
 	echo ripe >fruit.t &&
 
-	git diff --submodule=diff --color-moved >actual &&
+	git diff --submodule=diff --color-moved --color >actual &&
 
 	# no move detection as the moved line is across repository boundaries.
 	test_decode_color <actual >decoded_actual &&

@@ -1527,7 +1526,7 @@ test_expect_success 'move detection with submodules' '
 	! grep BRED decoded_actual &&
 
 	# nor did we mess with it another way
-	git diff --submodule=diff | test_decode_color >expect &&
+	git diff --submodule=diff --color | test_decode_color >expect &&
 	test_cmp expect decoded_actual
 '
 

2.14.2.1079.gce6b466188

When testing whether "add -p" can generate colors, we set color.ui to "always". This isn't a very good test, as in the real-world a user typically has "auto" coupled with stdout going to a terminal (and it's plausible that this could mask a real bug in add--interactive if we depend on plumbing's isatty check).

Let's switch to test_terminal, which gives us a more realistic environment. This also prepare us for future changes to the "always" color option.

---
 t/t3701-add-interactive.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

index 2f3e7cea64..39d0130f88 100755
@@ -2,6 +2,7 @@
 
 test_description='add -i basic tests'
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 if ! test_have_prereq PERL
 then

@@ -380,14 +381,11 @@ test_expect_success 'patch mode ignores unmerged entries' '
 	test_cmp expected diff
 '
 
-test_expect_success 'diffs can be colorized' '
+test_expect_success TTY 'diffs can be colorized' '
 	git reset --hard &&
 
-	# force color even though the test script has no terminal
-	test_config color.ui always &&
-
 	echo content >test &&
-	printf y | git add -p >output 2>&1 &&
+	printf y | test_terminal git add -p >output 2>&1 &&
 
 	# We do not want to depend on the exact coloring scheme
 	# git uses for diffs, so just check that we saw some kind of color.

2.14.2.1079.gce6b466188

This script tests the output of status with various formats when color is enabled. It uses the "always" setting so that the output is valid even though we capture it in a file. Using test_terminal gives us a more realistic environment, and prepares us for the behavior of "always" changing.

Arguably we are testing less than before, since "auto" is already the default, and we can no longer tell if the config is actually doing anything.

---
I noticed that "status" does not have a "--color" option. I think it might be worth adding one for completeness, though I still prefer the test_terminal solution here.

 t/t7508-status.sh | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

index 43d19a9b22..a3d760e63a 100755
@@ -6,6 +6,7 @@
 test_description='git status'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success 'status -h in broken repository' '
 	git config --global advice.statusuoption false &&

@@ -667,7 +668,7 @@ test_expect_success 'setup unique colors' '
 
 '
 
-test_expect_success 'status with color.ui' '
+test_expect_success TTY 'status with color.ui' '
 	cat >expect <<\EOF &&
 On branch <GREEN>master<RESET>
 Your branch and '\''upstream'\'' have diverged,

@@ -694,14 +695,14 @@ Untracked files:
 	<BLUE>untracked<RESET>
 
 EOF
-	test_config color.ui always &&
-	git status | test_decode_color >output &&
+	test_config color.ui auto &&
+	test_terminal git status | test_decode_color >output &&
 	test_i18ncmp expect output
 '
 
-test_expect_success 'status with color.status' '
-	test_config color.status always &&
-	git status | test_decode_color >output &&
+test_expect_success TTY 'status with color.status' '
+	test_config color.status auto &&
+	test_terminal git status | test_decode_color >output &&
 	test_i18ncmp expect output
 '
 

@@ -714,19 +715,19 @@ cat >expect <<\EOF
 <BLUE>??<RESET> untracked
 EOF
 
-test_expect_success 'status -s with color.ui' '
+test_expect_success TTY 'status -s with color.ui' '
 
-	git config color.ui always &&
-	git status -s | test_decode_color >output &&
+	git config color.ui auto &&
+	test_terminal git status -s | test_decode_color >output &&
 	test_cmp expect output
 
 '
 
-test_expect_success 'status -s with color.status' '
+test_expect_success TTY 'status -s with color.status' '
 
 	git config --unset color.ui &&
-	git config color.status always &&
-	git status -s | test_decode_color >output &&
+	git config color.status auto &&
+	test_terminal git status -s | test_decode_color >output &&
 	test_cmp expect output
 
 '

@@ -741,9 +742,9 @@ cat >expect <<\EOF
 <BLUE>??<RESET> untracked
 EOF
 
-test_expect_success 'status -s -b with color.status' '
+test_expect_success TTY 'status -s -b with color.status' '
 
-	git status -s -b | test_decode_color >output &&
+	test_terminal git status -s -b | test_decode_color >output &&
 	test_i18ncmp expect output
 
 '

@@ -757,20 +758,20 @@ A  dir2/added
 ?? untracked
 EOF
 
-test_expect_success 'status --porcelain ignores color.ui' '
+test_expect_success TTY 'status --porcelain ignores color.ui' '
 
 	git config --unset color.status &&
-	git config color.ui always &&
-	git status --porcelain | test_decode_color >output &&
+	git config color.ui auto &&
+	test_terminal git status --porcelain | test_decode_color >output &&
 	test_cmp expect output
 
 '
 
-test_expect_success 'status --porcelain ignores color.status' '
+test_expect_success TTY 'status --porcelain ignores color.status' '
 
 	git config --unset color.ui &&
-	git config color.status always &&
-	git status --porcelain | test_decode_color >output &&
+	git config color.status auto &&
+	test_terminal git status --porcelain | test_decode_color >output &&
 	test_cmp expect output
 
 '

2.14.2.1079.gce6b466188

To check that "status -v" respects diff config, we set "color.diff" and look at the output of "status". We could equally well use any diff config. Since color output depends on a lot of other factors (like whether stdout is a tty, and how we interpret "always"), let's use a more mundane option.

---
 t/t7502-commit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 725687d5d5..d33a3cb331 100755
@@ -171,9 +171,9 @@ test_expect_success 'verbose' '
 
 test_expect_success 'verbose respects diff config' '
 
-	test_config color.diff always &&
+	test_config diff.noprefix true &&
 	git status -v >actual &&
-	grep "\[1mdiff --git" actual
+	grep "diff --git negative negative" actual
 '
 
 mesg_with_comment_and_newlines='

2.14.2.1079.gce6b466188

We test the %C() format placeholders with a variety of color-inducing options, including "--color" and "-c color.ui=always". In preparation for the behavior of "always" changing, we need to do something with those "always" tests.

We can drop ones that expect "always" to turn on color even to a file, as that will become a synonym for "auto", which is already tested.

For the "--no-color" test, we need to make sure that color would otherwise be shown. To do this, we can use test_terminal, which enables colors in the default setup.

---
 t/t6006-rev-list-format.sh | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

index 98be78b4a2..25a9c65dc5 100755
@@ -208,26 +208,11 @@ 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

@@ -240,6 +225,11 @@ 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.14.2.1079.gce6b466188

In preparation for the behavior of "always" changing to match "auto", we can simply drop this test. We already check other forms (like "--color") independently.

---
 t/t3203-branch-output.sh | 6 ------
 1 file changed, 6 deletions(-)

index 86286f263d..ee6787614c 100755
@@ -258,12 +258,6 @@ test_expect_success TTY '%(color) present with tty' '
 	test_cmp expect.color actual
 '
 
-test_expect_success 'color.branch=always overrides auto-color' '
-	git -c color.branch=always branch $color_args >actual.raw &&
-	test_decode_color <actual.raw >actual &&
-	test_cmp expect.color actual
-'
-
 test_expect_success '--color overrides auto-color' '
 	git branch --color $color_args >actual.raw &&
 	test_decode_color <actual.raw >actual &&

2.14.2.1079.gce6b466188

This test wants to confirm that "clean -i" shows color output. Using test_terminal gives us a more realistic environment than "color.ui=always", and prepares us for the behavior of "always" changing in a future patch.

---
 t/t7301-clean-interactive.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 556e1850e2..1bf9789c8a 100755
@@ -3,6 +3,7 @@
 test_description='git clean -i basic tests'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success 'setup' '
 

@@ -472,10 +473,10 @@ test_expect_success 'git clean -id with prefix and path (ask)' '
 
 '
 
-test_expect_success 'git clean -i paints the header in HEADER color' '
+test_expect_success TTY 'git clean -i paints the header in HEADER color' '
 	>a.out &&
 	echo q |
-	git -c color.ui=always clean -i |
+	test_terminal git clean -i |
 	test_decode_color |
 	head -n 1 >header &&
 	# not i18ngrep

2.14.2.1079.gce6b466188

To test the color output, we must convince "git branch" to write colors to a non-terminal. We do that now by setting the color config to "always". In preparation for the behavior of "always" changing, let's switch to using the "--color" command-line option, which is more direct.

---
 t/t3205-branch-color.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index 9343550f50..4f1e16bb44 100755
@@ -12,7 +12,6 @@ test_expect_success 'set up some sample branches' '
 # choose non-default colors to make sure config
 # is taking effect
 test_expect_success 'set up some color config' '
-	git config color.branch always &&
 	git config color.branch.local blue &&
 	git config color.branch.remote yellow &&
 	git config color.branch.current cyan

@@ -24,7 +23,7 @@ test_expect_success 'regular output shows colors' '
 	  <BLUE>other<RESET>
 	  <YELLOW>remotes/origin/master<RESET>
 	EOF
-	git branch -a >actual.raw &&
+	git branch --color -a >actual.raw &&
 	test_decode_color <actual.raw >actual &&
 	test_cmp expect actual
 '

@@ -36,7 +35,7 @@ test_expect_success 'verbose output shows colors' '
 	  <BLUE>other                <RESET> $oid foo
 	  <YELLOW>remotes/origin/master<RESET> $oid foo
 	EOF
-	git branch -v -a >actual.raw &&
+	git branch --color -v -a >actual.raw &&
 	test_decode_color <actual.raw >actual &&
 	test_cmp expect actual
 '

2.14.2.1079.gce6b466188

When ref-filter learned about want_color() in 11b087adfd (ref-filter: consult want_color() before emitting colors, 2017-07-13), it became useful to be able to turn colors off and on for specific commands. For git-branch, you can do so with --color/--no-color.

But for git-for-each-ref and git-tag, the other users of ref-filter, you have no option except to tweak the "color.ui" config setting. Let's give both of these commands the usual color command-line options.

This is a bit more obvious as a method for overriding the config. And it also prepares us for the behavior of "always" changing (so that we are still left with a way of forcing color when our output goes to a non-terminal).

---
 Documentation/git-for-each-ref.txt | 5 +++++
 Documentation/git-tag.txt          | 5 +++++
 builtin/for-each-ref.c             | 1 +
 builtin/tag.c                      | 1 +
 t/t6300-for-each-ref.sh            | 4 ++--
 t/t7004-tag.sh                     | 4 ++--
 6 files changed, 16 insertions(+), 4 deletions(-)

index 66b4e0a405..cbd0a6212a 100644
@@ -57,6 +57,11 @@ OPTIONS
 	`xx`; for example `%00` interpolates to `\0` (NUL),
 	`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
 
+--color[=<when>]:
+	Respect any colors specified in the `--format` option. The
+	`<when>` field must be one of `always`, `never`, or `auto` (if
+	`<when>` is absent, behave as if `always` was given).
+
 --shell::
 --perl::
 --python::

index 95e9f391d8..956fc019f9 100644
@@ -115,6 +115,11 @@ options for details.
 	variable if it exists, or lexicographic order otherwise. See
 	linkgit:git-config[1].
 
+--color[=<when>]:
+	Respect any colors specified in the `--format` option. The
+	`<when>` field must be one of `always`, `never`, or `auto` (if
+	`<when>` is absent, behave as if `always` was given).
+
 -i::
 --ignore-case::
 	Sorting and filtering tags are case insensitive.

index 5d7c921a77..e931be9ce4 100644
@@ -36,6 +36,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(""),
 		OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
 		OPT_STRING(  0 , "format", &format.format, N_("format"), N_("format to use for the output")),
+		OPT__COLOR(&format.use_color, N_("respect format colors")),
 		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
 			    N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_CALLBACK(0, "points-at", &filter.points_at,

index c627794181..12dbbc56d9 100644
@@ -411,6 +411,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		},
 		OPT_STRING(  0 , "format", &format.format, N_("format"),
 			   N_("format to use for the output")),
+		OPT__COLOR(&format.use_color, N_("respect format colors")),
 		OPT_BOOL('i', "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
 		OPT_END()
 	};

index d6bffe6273..6358134805 100755
@@ -435,8 +435,8 @@ test_expect_success '%(color) does not show color without tty' '
 	test_cmp expected.bare actual
 '
 
-test_expect_success 'color.ui=always can override tty check' '
-	git -c color.ui=always for-each-ref --format="$color_format" >actual.raw &&
+test_expect_success '--color can override tty check' '
+	git for-each-ref --color --format="$color_format" >actual.raw &&
 	test_decode_color <actual.raw >actual &&
 	test_cmp expected.color actual
 '

index 0a86f8ea39..4e62c505fc 100755
@@ -1912,8 +1912,8 @@ test_expect_success TTY '%(color) present with tty' '
 	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_expect_success '--color overrides auto-color' '
+	git tag --color $color_args >actual.raw &&
 	test_decode_color <actual.raw >actual &&
 	test_cmp expect.color actual
 '

2.14.2.1079.gce6b466188

It can be handy to use --color=always (or it's synonym --color) on the command-line to convince a command to produce color even if it's stdout isn't going to the terminal or a pager.

What's less clear is whether it makes sense to set config variables like color.ui to always. For a one-shot like:

git -c color.ui=always ...

it's potentially useful (especially if the command doesn't directly support the --color option). But setting always in your on-disk config is much muddier, as you may be surprised when piped commands generate colors (and send them to whatever is consuming the pipe downstream).

Some people have done this anyway, because:

1. The documentation for color.ui makes it sound like using always is a good idea, when you almost certainly want auto.

2. Traditionally not every command (and especially not plumbing) respected color.ui in the first place. So the confusion came up less frequently than it might have.

The situation changed in 136c8c8b8f (color: check color.ui in git_default_config(), 2017-07-13), which negated point (2): now scripts using only plumbing commands (like add-interactive) are broken by this setting.

That commit was fixing real issues (e.g., by making color.ui=never work, since auto is the default), so we don't want to just revert it. We could turn always into a noop in plumbing commands, but that creates a hard-to-explain inconsistency between the plumbing and other commands.

Instead, let's just turn always into auto for all config. This does break the "one-shot" config shown above, but again, we're probably better to have simple and consistent rules than to try to special-case command-line config.

There is one place where always should retain its meaning:
on the command line, --color=always should continue to be the same as --color, overriding any isatty checks. Since the command-line parser also depends on git_config_colorbool(), we can use the existence of the "var" string to deterine whether we are serving the command-line or the config.

---
 Documentation/config.txt   | 35 +++++++++++++++++------------------
 color.c                    |  2 +-
 t/t3701-add-interactive.sh | 10 ++++++++++
 3 files changed, 28 insertions(+), 19 deletions(-)

index 1ac0ae6adb..b53c994d0a 100644
@@ -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 `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).
+	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).
 
 color.branch.<slot>::
 	Use customized color for branch coloration. `<slot>` is one of

@@ -1072,12 +1072,11 @@ color.branch.<slot>::
 
 color.diff::
 	Whether to use ANSI escape sequences to add color to patches.
-	If this is set to `always`, linkgit:git-diff[1],
+	If this is set to `true` or `auto`, linkgit:git-diff[1],
 	linkgit:git-log[1], and linkgit:git-show[1] will use color
-	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).
+	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).
 +
 This does not affect linkgit:git-format-patch[1] or the
 'git-diff-{asterisk}' plumbing commands.  Can be overridden on the

@@ -1141,12 +1140,12 @@ color.grep.<slot>::
 --
 
 color.interactive::
-	When set to `always`, always use colors for interactive prompts
+	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 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).
+	"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).
 
 color.interactive.<slot>::
 	Use customized color for 'git add --interactive' and 'git clean

@@ -1193,10 +1192,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 `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.
+	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`.
 
 column.ui::
 	Specify whether supported commands should output in columns.

index 9ccd954d6b..9c0dc82370 100644
@@ -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 1;
+			return var ? GIT_COLOR_AUTO : 1;
 		if (!strcasecmp(value, "auto"))
 			return GIT_COLOR_AUTO;
 	}

index 39d0130f88..a49c12c79b 100755
@@ -483,4 +483,14 @@ test_expect_success 'hunk-editing handles custom comment char' '
 	git diff --exit-code
 '
 
+test_expect_success 'add -p works even with color.ui=always' '
+	git reset --hard &&
+	echo change >>file &&
+	test_config color.ui always &&
+	echo y | git add -p &&
+	echo file >expect &&
+	git diff --cached --name-only >actual &&
+	test_cmp expect actual
+'
+
 test_done

2.14.2.1079.gce6b466188

I'm shuffling these so that everything except 03/12 and 09/12 goes on top of jk/ref-filter-colors (to be merged later for v2.14.x) to create jk/ui-color-always-to-auto-maint branch.

Another branch jk/ui-color-always-to-auto would fork from 'master' and have 03/12 and 09/12 (with a tweak to use vt100 explicitly, as we lack 01/12 at that point) applied, and then merge the above one. And then queuing another one that drops "env TERM=vt100" tweak added to 09/12 would bring us to the same state as applying your 12 patches directly on top. That will cook in 'next' down to 'master' to make sure we do not regress in 2.15.

Thanks.

Thanks, I was so busy slogging through the set of broken tests that I didn't even think that some of the cases would not be available on maint. Sorry to create work for you in untangling it.

I eyeballed what you pushed out, and it all looks good to me (looks like you did that final tweak as part of the merge of the two branches).

-Peff

I also came across this git add -p regression. The reason I added color.ui =
always was that git log --pretty="tformat:%Cgreen%h%Creset" | less stopped
having colored output with color.ui = auto. And git config man page suggeseted
color.ui = always on the first place.

I've got a series mostly done for option 2. It actually touches quite a few of the test scripts, since we use "-c color.ui=always" to test color output. Normally having to touch the test suite a lot gives me pause that we're breaking something for real users, but I think in this case the test suite is doing something that normal users simply don't.

A variant of 2 (let's call it 2a) is to convert "always" to "auto" _only_ for on-disk config, allowing "-c" to still work. This dull the test-suite pain, but there are still several test scripts which use test_config and need updating. And it introduces a weird inconsistency that I'd just as soon skip.

If we buy into the idea that "color.ui=always" is actually a useful construct to put in your on-disk config (and I'm not sure that I do), then our options are basically:

3. Go back to the earlier behavior, which i what Junio's 2-patch revert series does. That has two issues though:

- after patch 2, for-each-ref still respects color.ui=always, even though it's plumbing. This is maybe-OK because we don't generate colors unless somebody puts %C() codes into their format.

- plumbing like diff-tree still doesn't respect color.ui=never

4. Read color.ui everywhere, but downgrade "always" to "auto" in plumbing commands (which would have to mark themselves as such by setting a global). This again falls into the "inconsistent and hard to explain" category, though I think it at least behaves as people would generally want. Except for people who really do want:

git -c color.always=true foo >file

to produce color when "foo" is a script that uses diff plumbing under the hood. I don't think that _can_ be solved, since from the plumbing perspective this is the same as the buggy add--interactive case.

I still lean towards option 2 (kill off "always"), but I'm trying to consider the full range of options.

-Peff

We were a bit too agressive in pushing color.ui configuration to plumbing commands. A real fix must be found to override the default "auto" use of colors for any color-capable plumbing commands, but let's leave that to a later round and concentrate on fixing the regression first.

Junio C Hamano (2):
Revert "color: check color.ui in git_default_config()"
colors: git_default_config() does not read color.ui

 builtin/branch.c       | 2 +-
 builtin/clean.c        | 3 ++-
 builtin/for-each-ref.c | 3 ++-
 builtin/grep.c         | 2 +-
 builtin/show-branch.c  | 2 +-
 builtin/tag.c          | 2 +-
 color.c                | 8 ++++++++
 config.c               | 4 ----
 diff.c                 | 3 +++
 9 files changed, 19 insertions(+), 10 deletions(-)

2.14.2-882-gfe33df219d

This reverts commit 136c8c8b8fa39f1315713248473dececf20f8fe7.

Even though we do want to fix the fallout from 4c7f1819 ("make color.ui default to 'auto'", 2013-06-10), which made it impossible to override it with "git -c color.ui={never,always} $plumbing", allowing the plumbing commands to pay attention to color.ui configuration variable turned out to be an unsatisfactory fix.

People who had color.ui=always, thinking that it should be safe to do, because it won't apply to plumbing commands, got burned by it.

A bit of fix-up patches are needed, as the series that included the patch being reverted, and changes after the series landed, have and/or added code that assumes git_default_config() would read the color.ui, and they need to be adjusted.

---
 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 +++
 7 files changed, 16 insertions(+), 8 deletions(-)

index 16d391b407..1969c7116c 100644
@@ -92,7 +92,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)

index c1bafda5b6..057fc97fe4 100644
@@ -125,7 +125,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)

index a7157f5632..0d6e669732 100644
@@ -284,7 +284,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")) {

index 28f245c8cc..7073a3eb97 100644
@@ -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)

index 7aa8b076f0..31b6207a00 100644
@@ -361,6 +361,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)

index bc290e7563..a9356c1383 100644
@@ -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;

index 9c38258030..85e714f6c6 100644
@@ -299,6 +299,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);
 }
 

2.14.2-882-gfe33df219d

As we reverted 136c8c8b ("color: check color.ui in git_default_config()", 2017-07-13), these need to be added back to the codebase so that "git tag --list" and "git for-each-ref" would still pay attention to color.ui setting.

A real fix to the problem introduced by 4c7f1819 ("make color.ui default to 'auto'", 2013-06-10) must be found, to allow users to override the default "auto" use of colors for any color-capable plumbing commands, but let's leave that to a later round.

---
 builtin/for-each-ref.c | 3 ++-
 builtin/tag.c          | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

index 5d7c921a77..238eb00e09 100644
@@ -5,6 +5,7 @@
 #include "object.h"
 #include "parse-options.h"
 #include "ref-filter.h"
+#include "color.h"
 
 static char const * const for_each_ref_usage[] = {
 	N_("git for-each-ref [<options>] [<pattern>]"),

@@ -54,7 +55,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 
 	format.format = "%(objectname) %(objecttype)\t%(refname)";
 
-	git_config(git_default_config, NULL);
+	git_config(git_color_default_config, NULL);
 
 	parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);
 	if (maxcount < 0) {

index 66e35b823b..46c3e78b55 100644
@@ -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)

2.14.2-882-gfe33df219d