* Updated to v2.14.2 on macOS; git add --patch broken @ 2017-09-27 13:23 Toni Uebernickel 2017-09-27 17:07 ` Jeff King 2017-10-02 23:00 ` Jonathan Nieder 0 siblings, 2 replies; 42+ messages in thread From: Toni Uebernickel @ 2017-09-27 13:23 UTC (permalink / raw) To: git [-- 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 --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-09-27 13:23 Updated to v2.14.2 on macOS; git add --patch broken Toni Uebernickel @ 2017-09-27 17:07 ` Jeff King 2017-09-27 17:28 ` Toni Uebernickel 2017-10-02 23:00 ` Jonathan Nieder 1 sibling, 1 reply; 42+ messages in thread From: Jeff King @ 2017-09-27 17:07 UTC (permalink / raw) To: Toni Uebernickel; +Cc: git 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-09-27 17:07 ` Jeff King @ 2017-09-27 17:28 ` Toni Uebernickel 2017-09-27 18:01 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: Toni Uebernickel @ 2017-09-27 17:28 UTC (permalink / raw) To: Jeff King; +Cc: git [-- 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 --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-09-27 17:28 ` Toni Uebernickel @ 2017-09-27 18:01 ` Jeff King 2017-09-27 19:51 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2017-09-27 18:01 UTC (permalink / raw) To: Toni Uebernickel; +Cc: git 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-09-27 18:01 ` Jeff King @ 2017-09-27 19:51 ` Jonathan Nieder 2017-09-27 19:53 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2017-09-27 19:51 UTC (permalink / raw) To: Jeff King; +Cc: Toni Uebernickel, git 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-09-27 19:51 ` Jonathan Nieder @ 2017-09-27 19:53 ` Jonathan Nieder 2017-09-28 5:03 ` Toni Uebernickel 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2017-09-27 19:53 UTC (permalink / raw) To: Jeff King; +Cc: Toni Uebernickel, git 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-09-27 19:53 ` Jonathan Nieder @ 2017-09-28 5:03 ` Toni Uebernickel 2017-09-28 5:20 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: Toni Uebernickel @ 2017-09-28 5:03 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Jeff King, git [-- 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 --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-09-28 5:03 ` Toni Uebernickel @ 2017-09-28 5:20 ` Jeff King 2017-09-28 5:31 ` Toni Uebernickel 0 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2017-09-28 5:20 UTC (permalink / raw) To: Toni Uebernickel; +Cc: Jonathan Nieder, git 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-09-28 5:20 ` Jeff King @ 2017-09-28 5:31 ` Toni Uebernickel 0 siblings, 0 replies; 42+ messages in thread From: Toni Uebernickel @ 2017-09-28 5:31 UTC (permalink / raw) To: Jeff King; +Cc: Jonathan Nieder, git [-- 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 --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-09-27 13:23 Updated to v2.14.2 on macOS; git add --patch broken Toni Uebernickel 2017-09-27 17:07 ` Jeff King @ 2017-10-02 23:00 ` Jonathan Nieder 2017-10-03 1:18 ` Junio C Hamano 2017-10-03 4:07 ` [PATCH 0/2] fixing "add -p" regression Junio C Hamano 1 sibling, 2 replies; 42+ messages in thread From: Jonathan Nieder @ 2017-10-02 23:00 UTC (permalink / raw) To: Toni Uebernickel; +Cc: git, Tsvi Mostovicz, Jeff King 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, ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-02 23:00 ` Jonathan Nieder @ 2017-10-03 1:18 ` Junio C Hamano 2017-10-03 2:25 ` Junio C Hamano 2017-10-03 4:07 ` [PATCH 0/2] fixing "add -p" regression Junio C Hamano 1 sibling, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-03 1:18 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Toni Uebernickel, git, Tsvi Mostovicz, Jeff King 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. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 1:18 ` Junio C Hamano @ 2017-10-03 2:25 ` Junio C Hamano 2017-10-03 3:14 ` Junio C Hamano 2017-10-03 6:15 ` Jeff King 0 siblings, 2 replies; 42+ messages in thread From: Junio C Hamano @ 2017-10-03 2:25 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Toni Uebernickel, git, Tsvi Mostovicz, Jeff King 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? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 2:25 ` Junio C Hamano @ 2017-10-03 3:14 ` Junio C Hamano 2017-10-03 6:15 ` Jeff King 1 sibling, 0 replies; 42+ messages in thread From: Junio C Hamano @ 2017-10-03 3:14 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Toni Uebernickel, git, Tsvi Mostovicz, Jeff King 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) ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 2:25 ` Junio C Hamano 2017-10-03 3:14 ` Junio C Hamano @ 2017-10-03 6:15 ` Jeff King 2017-10-03 7:10 ` Junio C Hamano 2017-10-03 9:31 ` Jeff King 1 sibling, 2 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 6:15 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 6:15 ` Jeff King @ 2017-10-03 7:10 ` Junio C Hamano 2017-10-03 7:16 ` Jeff King 2017-10-03 9:31 ` Jeff King 1 sibling, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-03 7:10 UTC (permalink / raw) To: Jeff King; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 7:10 ` Junio C Hamano @ 2017-10-03 7:16 ` Jeff King 2017-10-03 8:34 ` Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2017-10-03 7:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 7:16 ` Jeff King @ 2017-10-03 8:34 ` Junio C Hamano 2017-10-03 8:45 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-03 8:34 UTC (permalink / raw) To: Jeff King; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 8:34 ` Junio C Hamano @ 2017-10-03 8:45 ` Jeff King 2017-10-03 8:56 ` Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2017-10-03 8:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 8:45 ` Jeff King @ 2017-10-03 8:56 ` Junio C Hamano 2017-10-03 9:10 ` Jeff King 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-03 8:56 UTC (permalink / raw) To: Jeff King; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 8:56 ` Junio C Hamano @ 2017-10-03 9:10 ` Jeff King 2017-10-03 9:18 ` Tsvi Mostovicz ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 9:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 9:10 ` Jeff King @ 2017-10-03 9:18 ` Tsvi Mostovicz 2017-10-03 10:38 ` Junio C Hamano 2017-10-20 13:31 ` Jan Prachař 2 siblings, 0 replies; 42+ messages in thread From: Tsvi Mostovicz @ 2017-10-03 9:18 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Jonathan Nieder, Toni Uebernickel, git 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 9:10 ` Jeff King 2017-10-03 9:18 ` Tsvi Mostovicz @ 2017-10-03 10:38 ` Junio C Hamano 2017-10-03 13:37 ` Jeff King 2017-10-20 13:31 ` Jan Prachař 2 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-03 10:38 UTC (permalink / raw) To: Jeff King; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 10:38 ` Junio C Hamano @ 2017-10-03 13:37 ` Jeff King 2017-10-03 13:39 ` [PATCH 01/12] test-terminal: set TERM=vt100 Jeff King ` (12 more replies) 0 siblings, 13 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/12] test-terminal: set TERM=vt100 2017-10-03 13:37 ` Jeff King @ 2017-10-03 13:39 ` Jeff King 2017-10-03 13:40 ` [PATCH 02/12] t4015: prefer --color to -c color.diff=always Jeff King ` (11 subsequent siblings) 12 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 02/12] t4015: prefer --color to -c color.diff=always 2017-10-03 13:37 ` Jeff King 2017-10-03 13:39 ` [PATCH 01/12] test-terminal: set TERM=vt100 Jeff King @ 2017-10-03 13:40 ` Jeff King 2017-10-03 13:41 ` [PATCH 03/12] t4015: use --color with --color-moved Jeff King ` (10 subsequent siblings) 12 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 03/12] t4015: use --color with --color-moved 2017-10-03 13:37 ` Jeff King 2017-10-03 13:39 ` [PATCH 01/12] test-terminal: set TERM=vt100 Jeff King 2017-10-03 13:40 ` [PATCH 02/12] t4015: prefer --color to -c color.diff=always Jeff King @ 2017-10-03 13:41 ` Jeff King 2017-10-03 13:42 ` [PATCH 04/12] t3701: use test-terminal to collect color output Jeff King ` (9 subsequent siblings) 12 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 04/12] t3701: use test-terminal to collect color output 2017-10-03 13:37 ` Jeff King ` (2 preceding siblings ...) 2017-10-03 13:41 ` [PATCH 03/12] t4015: use --color with --color-moved Jeff King @ 2017-10-03 13:42 ` Jeff King 2017-10-03 13:43 ` [PATCH 05/12] t7508: use test_terminal for " Jeff King ` (8 subsequent siblings) 12 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:42 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 05/12] t7508: use test_terminal for color output 2017-10-03 13:37 ` Jeff King ` (3 preceding siblings ...) 2017-10-03 13:42 ` [PATCH 04/12] t3701: use test-terminal to collect color output Jeff King @ 2017-10-03 13:43 ` Jeff King 2017-10-03 13:43 ` [PATCH 06/12] t7502: use diff.noprefix for --verbose test Jeff King ` (7 subsequent siblings) 12 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:43 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 06/12] t7502: use diff.noprefix for --verbose test 2017-10-03 13:37 ` Jeff King ` (4 preceding siblings ...) 2017-10-03 13:43 ` [PATCH 05/12] t7508: use test_terminal for " Jeff King @ 2017-10-03 13:43 ` Jeff King 2017-10-03 13:44 ` [PATCH 07/12] t6006: drop "always" color config tests Jeff King ` (6 subsequent siblings) 12 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:43 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 07/12] t6006: drop "always" color config tests 2017-10-03 13:37 ` Jeff King ` (5 preceding siblings ...) 2017-10-03 13:43 ` [PATCH 06/12] t7502: use diff.noprefix for --verbose test Jeff King @ 2017-10-03 13:44 ` Jeff King 2017-10-03 13:44 ` [PATCH 08/12] t3203: drop "always" color test Jeff King ` (5 subsequent siblings) 12 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:44 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 08/12] t3203: drop "always" color test 2017-10-03 13:37 ` Jeff King ` (6 preceding siblings ...) 2017-10-03 13:44 ` [PATCH 07/12] t6006: drop "always" color config tests Jeff King @ 2017-10-03 13:44 ` Jeff King 2017-10-03 13:44 ` [PATCH 09/12] t7301: use test_terminal to check color Jeff King ` (4 subsequent siblings) 12 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:44 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 09/12] t7301: use test_terminal to check color 2017-10-03 13:37 ` Jeff King ` (7 preceding siblings ...) 2017-10-03 13:44 ` [PATCH 08/12] t3203: drop "always" color test Jeff King @ 2017-10-03 13:44 ` Jeff King 2017-10-03 13:45 ` [PATCH 10/12] t3205: use --color instead of color.branch=always Jeff King ` (3 subsequent siblings) 12 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:44 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 10/12] t3205: use --color instead of color.branch=always 2017-10-03 13:37 ` Jeff King ` (8 preceding siblings ...) 2017-10-03 13:44 ` [PATCH 09/12] t7301: use test_terminal to check color Jeff King @ 2017-10-03 13:45 ` Jeff King 2017-10-03 13:45 ` [PATCH 11/12] provide --color option for all ref-filter users Jeff King ` (2 subsequent siblings) 12 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 11/12] provide --color option for all ref-filter users 2017-10-03 13:37 ` Jeff King ` (9 preceding siblings ...) 2017-10-03 13:45 ` [PATCH 10/12] t3205: use --color instead of color.branch=always Jeff King @ 2017-10-03 13:45 ` Jeff King 2017-10-03 13:46 ` [PATCH 12/12] color: make "always" the same as "auto" in config Jeff King 2017-10-04 2:59 ` Updated to v2.14.2 on macOS; git add --patch broken Junio C Hamano 12 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 12/12] color: make "always" the same as "auto" in config 2017-10-03 13:37 ` Jeff King ` (10 preceding siblings ...) 2017-10-03 13:45 ` [PATCH 11/12] provide --color option for all ref-filter users Jeff King @ 2017-10-03 13:46 ` Jeff King 2017-10-04 2:59 ` Updated to v2.14.2 on macOS; git add --patch broken Junio C Hamano 12 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 13:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 13:37 ` Jeff King ` (11 preceding siblings ...) 2017-10-03 13:46 ` [PATCH 12/12] color: make "always" the same as "auto" in config Jeff King @ 2017-10-04 2:59 ` Junio C Hamano 2017-10-05 10:06 ` Jeff King 12 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2017-10-04 2:59 UTC (permalink / raw) To: Jeff King; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-04 2:59 ` Updated to v2.14.2 on macOS; git add --patch broken Junio C Hamano @ 2017-10-05 10:06 ` Jeff King 0 siblings, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-05 10:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 9:10 ` Jeff King 2017-10-03 9:18 ` Tsvi Mostovicz 2017-10-03 10:38 ` Junio C Hamano @ 2017-10-20 13:31 ` Jan Prachař 2 siblings, 0 replies; 42+ messages in thread From: Jan Prachař @ 2017-10-20 13:31 UTC (permalink / raw) To: Jeff King, Junio C Hamano Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Updated to v2.14.2 on macOS; git add --patch broken 2017-10-03 6:15 ` Jeff King 2017-10-03 7:10 ` Junio C Hamano @ 2017-10-03 9:31 ` Jeff King 1 sibling, 0 replies; 42+ messages in thread From: Jeff King @ 2017-10-03 9:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Toni Uebernickel, git, Tsvi Mostovicz 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 0/2] fixing "add -p" regression 2017-10-02 23:00 ` Jonathan Nieder 2017-10-03 1:18 ` Junio C Hamano @ 2017-10-03 4:07 ` Junio C Hamano 2017-10-03 4:07 ` [PATCH 1/2] Revert "color: check color.ui in git_default_config()" Junio C Hamano 2017-10-03 4:07 ` [PATCH 2/2] colors: git_default_config() does not read color.ui Junio C Hamano 1 sibling, 2 replies; 42+ messages in thread From: Junio C Hamano @ 2017-10-03 4:07 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, Jeff King 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/2] Revert "color: check color.ui in git_default_config()" 2017-10-03 4:07 ` [PATCH 0/2] fixing "add -p" regression Junio C Hamano @ 2017-10-03 4:07 ` Junio C Hamano 2017-10-03 4:07 ` [PATCH 2/2] colors: git_default_config() does not read color.ui Junio C Hamano 1 sibling, 0 replies; 42+ messages in thread From: Junio C Hamano @ 2017-10-03 4:07 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, Jeff King 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 2/2] colors: git_default_config() does not read color.ui 2017-10-03 4:07 ` [PATCH 0/2] fixing "add -p" regression Junio C Hamano 2017-10-03 4:07 ` [PATCH 1/2] Revert "color: check color.ui in git_default_config()" Junio C Hamano @ 2017-10-03 4:07 ` Junio C Hamano 1 sibling, 0 replies; 42+ messages in thread From: Junio C Hamano @ 2017-10-03 4:07 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, Jeff King 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
end of thread, other threads:[~2017-10-20 13:31 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-27 13:23 Updated to v2.14.2 on macOS; git add --patch broken Toni Uebernickel 2017-09-27 17:07 ` Jeff King 2017-09-27 17:28 ` Toni Uebernickel 2017-09-27 18:01 ` Jeff King 2017-09-27 19:51 ` Jonathan Nieder 2017-09-27 19:53 ` Jonathan Nieder 2017-09-28 5:03 ` Toni Uebernickel 2017-09-28 5:20 ` Jeff King 2017-09-28 5:31 ` Toni Uebernickel 2017-10-02 23:00 ` Jonathan Nieder 2017-10-03 1:18 ` Junio C Hamano 2017-10-03 2:25 ` Junio C Hamano 2017-10-03 3:14 ` Junio C Hamano 2017-10-03 6:15 ` Jeff King 2017-10-03 7:10 ` Junio C Hamano 2017-10-03 7:16 ` Jeff King 2017-10-03 8:34 ` Junio C Hamano 2017-10-03 8:45 ` Jeff King 2017-10-03 8:56 ` Junio C Hamano 2017-10-03 9:10 ` Jeff King 2017-10-03 9:18 ` Tsvi Mostovicz 2017-10-03 10:38 ` Junio C Hamano 2017-10-03 13:37 ` Jeff King 2017-10-03 13:39 ` [PATCH 01/12] test-terminal: set TERM=vt100 Jeff King 2017-10-03 13:40 ` [PATCH 02/12] t4015: prefer --color to -c color.diff=always Jeff King 2017-10-03 13:41 ` [PATCH 03/12] t4015: use --color with --color-moved Jeff King 2017-10-03 13:42 ` [PATCH 04/12] t3701: use test-terminal to collect color output Jeff King 2017-10-03 13:43 ` [PATCH 05/12] t7508: use test_terminal for " Jeff King 2017-10-03 13:43 ` [PATCH 06/12] t7502: use diff.noprefix for --verbose test Jeff King 2017-10-03 13:44 ` [PATCH 07/12] t6006: drop "always" color config tests Jeff King 2017-10-03 13:44 ` [PATCH 08/12] t3203: drop "always" color test Jeff King 2017-10-03 13:44 ` [PATCH 09/12] t7301: use test_terminal to check color Jeff King 2017-10-03 13:45 ` [PATCH 10/12] t3205: use --color instead of color.branch=always Jeff King 2017-10-03 13:45 ` [PATCH 11/12] provide --color option for all ref-filter users Jeff King 2017-10-03 13:46 ` [PATCH 12/12] color: make "always" the same as "auto" in config Jeff King 2017-10-04 2:59 ` Updated to v2.14.2 on macOS; git add --patch broken Junio C Hamano 2017-10-05 10:06 ` Jeff King 2017-10-20 13:31 ` Jan Prachař 2017-10-03 9:31 ` Jeff King 2017-10-03 4:07 ` [PATCH 0/2] fixing "add -p" regression Junio C Hamano 2017-10-03 4:07 ` [PATCH 1/2] Revert "color: check color.ui in git_default_config()" Junio C Hamano 2017-10-03 4:07 ` [PATCH 2/2] colors: git_default_config() does not read color.ui Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).