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

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

[Toni Uebernickel]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

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


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jeff King]

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

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

[Toni Uebernickel]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

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

> 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


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jeff King]

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

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

[Jonathan Nieder]

Hi,

Jeff King wrote:
> 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.

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

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

[Jonathan Nieder]

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

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

[Toni Uebernickel]

[-- Attachment #1: Type: text/plain, Size: 12106 bytes --]

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

> 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


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jeff King]

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

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

[Toni Uebernickel]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]

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

> 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


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jonathan Nieder]

Hi,

Toni Uebernickel wrote:

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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?

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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

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

[Jeff King]

On Tue, Oct 03, 2017 at 11:25:44AM +0900, Junio C Hamano wrote:


> 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

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

[Junio C Hamano]

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.

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

[Jeff King]

On Tue, Oct 03, 2017 at 04:10:12PM +0900, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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?

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

[Jeff King]

On Tue, Oct 03, 2017 at 05:34:40PM +0900, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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.

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

[Jeff King]

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

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

[Tsvi Mostovicz]

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

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

[Jeff King]

On Tue, Oct 03, 2017 at 02:15:15AM -0400, Jeff King wrote:

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Jeff King]

On Tue, Oct 03, 2017 at 07:38:24PM +0900, Junio C Hamano wrote:

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

> 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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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.

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

[Jeff King]

On Wed, Oct 04, 2017 at 11:59:57AM +0900, Junio C Hamano wrote:

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

> 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

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

[Jan Prachař]

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