threads / discuss / 46845

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

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

## tl;dr

42 messages between Sep 27, 2017 and Oct 20, 2017.

replies: 41people: 6as markdown or json

Toni Uebernickel· Sep 27, 2017, 13:23 UTC · lore
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

tuebernickel@gmail.com - https://keybase.io/havvg https://github.com/havvg - https://www.xing.com/profile/Toni_Uebernickel

Jeff King· Sep 27, 2017, 17:07 UTC · re: Toni Uebernickel · lore

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

On Wed, Sep 27, 2017 at 03:23:21PM +0200, Toni Uebernickel wrote:
Show 8 quoted lines
> 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.

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
Toni Uebernickel· Sep 27, 2017, 17:28 UTC · re: Jeff King · lore

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

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

tuebernickel@gmail.com - https://keybase.io/havvg https://github.com/havvg - https://www.xing.com/profile/Toni_Uebernickel

Show 22 quoted lines
> On 27. Sep 2017, at 19:07, Jeff King <peff@peff.net> wrote:
> 
> On Wed, Sep 27, 2017 at 03:23:21PM +0200, Toni Uebernickel wrote:
> 
>> 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.
> 
> 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
Jeff King· Sep 27, 2017, 18:01 UTC · re: Toni Uebernickel · lore

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

On Wed, Sep 27, 2017 at 07:28:49PM +0200, Toni Uebernickel wrote:
> The previous version was v2.13.2.
> I switched back to this version, and it works perfectly fine; without any changes to my system.
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
Jonathan Nieder· Sep 27, 2017, 19:51 UTC · re: Jeff King · lore

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

Hi,
Jeff King wrote:
> On Wed, Sep 27, 2017 at 07:28:49PM +0200, Toni Uebernickel wrote:
Show 12 quoted lines
>> The previous version was v2.13.2.
>> I switched back to this version, and it works perfectly fine; without any changes to my system.
>
> 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.
How about this change?
 commit 136c8c8b8fa39f1315713248473dececf20f8fe7
 Author: Jeff King <peff@peff.net>
 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

Jonathan Nieder· Sep 27, 2017, 19:53 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
> Jeff King wrote:
Show 15 quoted lines
>> 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.
>
> How about this change?
>
>  commit 136c8c8b8fa39f1315713248473dececf20f8fe7
>  Author: Jeff King <peff@peff.net>
>  Date:   Thu Jul 13 11:07:03 2017 -0400
>
>      color: check color.ui in git_default_config()

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

> Toni, what is the output of "git config -l"?
I'm still curious about this.

Thanks, Jonathan

Toni Uebernickel· Sep 28, 2017, 05:03 UTC · re: Jonathan Nieder · lore

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

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 user.email=tuebernickel@gmail.com user.signingkey=F6035C7A core.editor=mate -w -l 1 commit.gpgsign=true github.user=havvg github.token=PRIVATE_CREDENTIALS_REMOVED travis.user=tuebernickel@gmail.com 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 remote.origin.url=git@gitlab.tarifhaus.ag: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 user.email=toni.uebernickel@tarifhaus.ag 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

tuebernickel@gmail.com - https://keybase.io/havvg https://github.com/havvg - https://www.xing.com/profile/Toni_Uebernickel

Show 30 quoted lines
> On 27. Sep 2017, at 21:53, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 
> Jonathan Nieder wrote:
>> Jeff King wrote:
> 
>>> 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.
>> 
>> How about this change?
>> 
>> commit 136c8c8b8fa39f1315713248473dececf20f8fe7
>> Author: Jeff King <peff@peff.net>
>> Date:   Thu Jul 13 11:07:03 2017 -0400
>> 
>>     color: check color.ui in git_default_config()
> 
> Uh, I think I was thinking of another thread when I wrote this.  Sorry
> for the nonsense.
> 
>> Toni, what is the output of "git config -l"?
> 
> I'm still curious about this.
> 
> Thanks,
> Jonathan
Jeff King· Sep 28, 2017, 05:20 UTC · re: Toni Uebernickel · lore

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

On Thu, Sep 28, 2017 at 07:03:55AM +0200, Toni Uebernickel wrote:
> color.ui=always

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
Toni Uebernickel· Sep 28, 2017, 05:31 UTC · re: Jeff King · lore

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

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

tuebernickel@gmail.com - https://keybase.io/havvg https://github.com/havvg - https://www.xing.com/profile/Toni_Uebernickel

Show 17 quoted lines
> On 28. Sep 2017, at 07:20, Jeff King <peff@peff.net> wrote:
> 
> On Thu, Sep 28, 2017 at 07:03:55AM +0200, Toni Uebernickel wrote:
> 
>> color.ui=always
> 
> 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
Jonathan Nieder· Oct 2, 2017, 23:00 UTC · re: Toni Uebernickel · lore

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

Hi,
Toni Uebernickel wrote:
Show 6 quoted lines
> 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.
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

diff --git i/git-add--interactive.perl w/git-add--interactive.perl
index 28b325d754..4ea69538c7 100755
--- i/git-add--interactive.perl
+++ w/git-add--interactive.perl
@@ -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,
Junio C Hamano· Oct 3, 2017, 01:18 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
> 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.

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

Junio C Hamano· Oct 3, 2017, 02:25 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> 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.
>
> 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?
Junio C Hamano· Oct 3, 2017, 03:14 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> 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(-)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 5d7c921a77..238eb00e09 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -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) {
diff --git a/builtin/tag.c b/builtin/tag.c
index c627794181..f8bc1393ed 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)
Jeff King· Oct 3, 2017, 06:15 UTC · re: Junio C Hamano · lore

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

On Tue, Oct 03, 2017 at 11:25:44AM +0900, Junio C Hamano wrote:
Show 24 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Jonathan Nieder <jrnieder@gmail.com> writes:
> >
> >> 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.
> >
> > 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?

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
Junio C Hamano· Oct 3, 2017, 07:10 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> 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.
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.

Jeff King· Oct 3, 2017, 07:16 UTC · re: Junio C Hamano · lore

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

On Tue, Oct 03, 2017 at 04:10:12PM +0900, Junio C Hamano wrote:
Show 18 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> 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
Junio C Hamano· Oct 3, 2017, 08:34 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
> 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).

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?

Jeff King· Oct 3, 2017, 08:45 UTC · re: Junio C Hamano · lore

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

On Tue, Oct 03, 2017 at 05:34:40PM +0900, Junio C Hamano wrote:
Show 14 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > 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).
> 
> 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?

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.

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

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
Junio C Hamano· Oct 3, 2017, 08:56 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
> 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.
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.

Jeff King· Oct 3, 2017, 09:10 UTC · re: Junio C Hamano · lore

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

On Tue, Oct 03, 2017 at 05:56:53PM +0900, Junio C Hamano wrote:
Show 13 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> 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
Tsvi Mostovicz· Oct 3, 2017, 09:18 UTC · re: Jeff King · lore

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

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 ttmost@gmail.com www.linkedin.com/in/tsvim

On Tue, Oct 3, 2017 at 12:10 PM, Jeff King <peff@peff.net> wrote:
Show 33 quoted lines
> On Tue, Oct 03, 2017 at 05:56:53PM +0900, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > 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.
>>
>> 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
Junio C Hamano· Oct 3, 2017, 10:38 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 21 quoted lines
> On Tue, Oct 03, 2017 at 05:56:53PM +0900, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > 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.
>> 
>> 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).

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.

Jeff King· Oct 3, 2017, 13:37 UTC · re: Junio C Hamano · lore

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

On Tue, Oct 03, 2017 at 07:38:24PM +0900, Junio C Hamano wrote:
Show 11 quoted lines
> 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.

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.
Show 12 quoted lines
> 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.

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
Jeff King· Oct 3, 2017, 13:39 UTC · re: Jeff King · lore

[PATCH 01/12] test-terminal: set TERM=vt100

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.

Signed-off-by: Jeff King <peff@peff.net>
---
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(-)
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index d2aec0f38b..86286f263d 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -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
 '
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 36d120c969..8f155da7a5 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -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
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index b326d550f3..98be78b4a2 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -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
 	'
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2274a4b733..d6bffe6273 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -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
 '
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b545c33f83..0a86f8ea39 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -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
 '
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 9128ec5acd..f0f1abd1c2 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -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
 '
 
diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 96b6a03e1c..46bf618479 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -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
Jeff King· Oct 3, 2017, 13:40 UTC · re: Jeff King · lore

[PATCH 02/12] t4015: prefer --color to -c color.diff=always

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t4015-diff-whitespace.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 12d182dc1b..b9df886e37 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -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
Jeff King· Oct 3, 2017, 13:41 UTC · re: Jeff King · lore

[PATCH 03/12] t4015: use --color with --color-moved

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.

Signed-off-by: Jeff King <peff@peff.net>
---
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(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index b9df886e37..3bca958863 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -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
Jeff King· Oct 3, 2017, 13:42 UTC · re: Jeff King · lore

[PATCH 04/12] t3701: use test-terminal to collect color output

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3701-add-interactive.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 2f3e7cea64..39d0130f88 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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
Jeff King· Oct 3, 2017, 13:43 UTC · re: Jeff King · lore

[PATCH 05/12] t7508: use test_terminal for color output

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.

Signed-off-by: Jeff King <peff@peff.net>
---
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(-)
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 43d19a9b22..a3d760e63a 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -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
Jeff King· Oct 3, 2017, 13:43 UTC · re: Jeff King · lore

[PATCH 06/12] t7502: use diff.noprefix for --verbose test

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7502-commit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 725687d5d5..d33a3cb331 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -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
Jeff King· Oct 3, 2017, 13:44 UTC · re: Jeff King · lore

[PATCH 07/12] t6006: drop "always" color config tests

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t6006-rev-list-format.sh | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 98be78b4a2..25a9c65dc5 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -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
Jeff King· Oct 3, 2017, 13:44 UTC · re: Jeff King · lore

[PATCH 08/12] t3203: drop "always" color test

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3203-branch-output.sh | 6 ------
 1 file changed, 6 deletions(-)
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 86286f263d..ee6787614c 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -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
Jeff King· Oct 3, 2017, 13:44 UTC · re: Jeff King · lore

[PATCH 09/12] t7301: use test_terminal to check color

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7301-clean-interactive.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/t7301-clean-interactive.sh b/t/t7301-clean-interactive.sh
index 556e1850e2..1bf9789c8a 100755
--- a/t/t7301-clean-interactive.sh
+++ b/t/t7301-clean-interactive.sh
@@ -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
Jeff King· Oct 3, 2017, 13:45 UTC · re: Jeff King · lore

[PATCH 10/12] t3205: use --color instead of color.branch=always

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3205-branch-color.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/t/t3205-branch-color.sh b/t/t3205-branch-color.sh
index 9343550f50..4f1e16bb44 100755
--- a/t/t3205-branch-color.sh
+++ b/t/t3205-branch-color.sh
@@ -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
Jeff King· Oct 3, 2017, 13:45 UTC · re: Jeff King · lore

[PATCH 11/12] provide --color option for all ref-filter users

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

Signed-off-by: Jeff King <peff@peff.net>
---
 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(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 66b4e0a405..cbd0a6212a 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -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::
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 95e9f391d8..956fc019f9 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -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.
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 5d7c921a77..e931be9ce4 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -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,
diff --git a/builtin/tag.c b/builtin/tag.c
index c627794181..12dbbc56d9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -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()
 	};
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index d6bffe6273..6358134805 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -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
 '
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 0a86f8ea39..4e62c505fc 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -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
Jeff King· Oct 3, 2017, 13:46 UTC · re: Jeff King · lore

[PATCH 12/12] color: make "always" the same as "auto" in config

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config.txt   | 35 +++++++++++++++++------------------
 color.c                    |  2 +-
 t/t3701-add-interactive.sh | 10 ++++++++++
 3 files changed, 28 insertions(+), 19 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6adb..b53c994d0a 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 `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.
diff --git a/color.c b/color.c
index 9ccd954d6b..9c0dc82370 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 1;
+			return var ? GIT_COLOR_AUTO : 1;
 		if (!strcasecmp(value, "auto"))
 			return GIT_COLOR_AUTO;
 	}
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 39d0130f88..a49c12c79b 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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
Junio C Hamano· Oct 4, 2017, 02:59 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 17 quoted lines
> 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

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.
Jeff King· Oct 5, 2017, 10:06 UTC · re: Junio C Hamano · lore

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

On Wed, Oct 04, 2017 at 11:59:57AM +0900, Junio C Hamano wrote:
Show 23 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > 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
> 
> 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.

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.

Show 7 quoted lines
> 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.

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
Jan Prachař· Oct 20, 2017, 13:31 UTC · re: Jeff King · lore

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

On Tue, 2017-10-03 at 05:10 -0400, Jeff King wrote:
Show 7 quoted lines
> 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.
> 

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.

Jeff King· Oct 3, 2017, 09:31 UTC · re: Jeff King · lore

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

On Tue, Oct 03, 2017 at 02:15:15AM -0400, Jeff King wrote:
Show 28 quoted lines
> 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.

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
Junio C Hamano· Oct 3, 2017, 04:07 UTC · re: Jonathan Nieder · lore

[PATCH 0/2] fixing "add -p" regression

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
Junio C Hamano· Oct 3, 2017, 04:07 UTC · re: Junio C Hamano · lore

[PATCH 1/2] Revert "color: check color.ui in git_default_config()"

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 16d391b407..1969c7116c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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)
diff --git a/builtin/clean.c b/builtin/clean.c
index c1bafda5b6..057fc97fe4 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -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)
diff --git a/builtin/grep.c b/builtin/grep.c
index a7157f5632..0d6e669732 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -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")) {
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 28f245c8cc..7073a3eb97 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 7aa8b076f0..31b6207a00 100644
--- a/color.c
+++ b/color.c
@@ -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)
diff --git a/config.c b/config.c
index bc290e7563..a9356c1383 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 9c38258030..85e714f6c6 100644
--- a/diff.c
+++ b/diff.c
@@ -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
Junio C Hamano· Oct 3, 2017, 04:07 UTC · re: Junio C Hamano · lore

[PATCH 2/2] colors: git_default_config() does not read color.ui

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/for-each-ref.c | 3 ++-
 builtin/tag.c          | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 5d7c921a77..238eb00e09 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -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) {
diff --git a/builtin/tag.c b/builtin/tag.c
index 66e35b823b..46c3e78b55 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)
-- 
2.14.2-882-gfe33df219d

← back to recent threads