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
