From 2c1acdf6c9f6668dc520e75709af8447587eb4d0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Oct 2017 13:23:24 -0400 Subject: Revert "color: make "always" the same as "auto" in config" This reverts commit 6be4595edb8e5b616c6e8b9fbc78b0f831fa2a87. That commit weakened the "always" setting of color config so that it acted as "auto". This was meant to solve regressions in v2.14.2 in which setting "color.ui=always" in the on-disk config broke scripts like add--interactive, because the plumbing diff commands began to generate color output. This was due to 136c8c8b8f (color: check color.ui in git_default_config(), 2017-07-13), which was in turn trying to fix issues caused by 4c7f1819b3 (make color.ui default to 'auto', 2013-06-10). But in weakening "always", we created even more problems, as people expect to be able to use "git -c color.ui=always" to force color (especially because some commands don't have their own --color flag). We can fix that by special-casing the command-line "-c", but now things are getting pretty confusing. Instead of piling hacks upon hacks, let's start peeling off the hacks. The first step is dropping the weakening of "always", which this revert does. Note that we could actually revert the whole series merged in by da15b78e52642bd45fd5513ab0000fdf2e58a6f4. Most of that series consists of preparations to the tests to handle the weakening of "-c color.ui=always". But it's worth keeping for a few reasons: - there are some other preparatory cleanups, like e433749d86 (test-terminal: set TERM=vt100, 2017-10-03) - it adds "--color" options more consistently in 0c88bf5050 (provide --color option for all ref-filter users, 2017-10-03) - some of the cases dropping "-c" end up being more robust and realistic tests, as in 01c94e9001 (t7508: use test_terminal for color output, 2017-10-03) - the preferred tool for overriding config is "--color", and we should be modeling that consistently We can individually revert the few commits necessary to restore some useful tests (which will be done on top of this patch). Note that this isn't a pure revert; we'll keep the test added in t3701, but mark it as failure for now. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t3701-add-interactive.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 't') diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index a49c12c79b..87ffd4d43c 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -483,7 +483,7 @@ test_expect_success 'hunk-editing handles custom comment char' ' git diff --exit-code ' -test_expect_success 'add -p works even with color.ui=always' ' +test_expect_failure 'add -p works even with color.ui=always' ' git reset --hard && echo change >>file && test_config color.ui always && -- cgit v1.2.1 From 1d4b12fe7c44da166070a4b577c62fb83127f10e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Oct 2017 13:23:41 -0400 Subject: Revert "t6006: drop "always" color config tests" This reverts commit c5bdfe677cfab5b2e87771c35565d44d3198efda. That commit was done primarily to prepare for the weakening of "always" in 6be4595edb (color: make "always" the same as "auto" in config, 2017-10-03). But since we've now reverted 6be4595edb, there's no need for us to remove "-c color.ui=always" from the tests. And in fact it's a good idea to restore these tests, to make sure that "always" continues to work. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t6006-rev-list-format.sh | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) (limited to 't') diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh index 25a9c65dc5..98be78b4a2 100755 --- a/t/t6006-rev-list-format.sh +++ b/t/t6006-rev-list-format.sh @@ -208,11 +208,26 @@ 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 @@ -225,11 +240,6 @@ 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' ' -- cgit v1.2.1 From 33c643bb083133376f3fdcb190ebc58f9eef12bb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Oct 2017 13:24:31 -0400 Subject: Revert "color: check color.ui in git_default_config()" This reverts commit 136c8c8b8fa39f1315713248473dececf20f8fe7. That commit was trying to address a bug caused by 4c7f1819b3 (make color.ui default to 'auto', 2013-06-10), in which plumbing like diff-tree defaulted to "auto" color, but did not respect a "color.ui" directive to disable it. But it also meant that we started respecting "color.ui" set to "always". This was a known problem, but 4c7f1819b3 argued that nobody ought to be doing that. However, that turned out to be wrong, and we got a number of bug reports related to "add -p" regressing in v2.14.2. Let's revert 136c8c8b8, fixing the regression to "add -p". This leaves the problem from 4c7f1819b3 unfixed, but: 1. It's a pretty obscure problem in the first place. I only noticed it while working on the color code, and we haven't got a single bug report or complaint about it. 2. We can make a more moderate fix on top by respecting "never" but not "always" for plumbing commands. This is just the minimal fix to go back to the working state we had before v2.14.2. Note that this isn't a pure revert. We now have a test in t3701 which shows off the "add -p" regression. This can be flipped to success. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t3701-add-interactive.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 't') diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 87ffd4d43c..a49c12c79b 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -483,7 +483,7 @@ test_expect_success 'hunk-editing handles custom comment char' ' git diff --exit-code ' -test_expect_failure 'add -p works even with color.ui=always' ' +test_expect_success 'add -p works even with color.ui=always' ' git reset --hard && echo change >>file && test_config color.ui always && -- cgit v1.2.1 From b521fd122865dca88b99d05344ec189d39efcefb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Oct 2017 13:26:02 -0400 Subject: tag: respect color.ui config Since 11b087adfd (ref-filter: consult want_color() before emitting colors, 2017-07-13), we expect that setting "color.ui" to "always" will enable color tag formats even without a tty. As that commit was built on top of 136c8c8b8f (color: check color.ui in git_default_config(), 2017-07-13) from the same series, we didn't need to touch tag's config parsing at all. However, since we reverted 136c8c8b8f, we now need to explicitly call git_color_default_config() to make this work. Let's do so, and also restore the test dropped in 0c88bf5050 (provide --color option for all ref-filter users, 2017-10-03). That commit swapped out our "color.ui=always" test for "--color" in preparation for "always" going away. But since it is here to stay, we should test both cases. Note that for-each-ref also lost its color.ui support as part of reverting 136c8c8b8f. But as a plumbing command, it should _not_ respect the color.ui config. Since it also gained a --color option in 0c88bf5050, that's the correct way to ask it for color. We'll continue to test that, and confirm that "color.ui" is not respected. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t6300-for-each-ref.sh | 5 +++++ t/t7004-tag.sh | 6 ++++++ 2 files changed, 11 insertions(+) (limited to 't') diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 6358134805..09f2b7799d 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -441,6 +441,11 @@ test_expect_success '--color can override tty check' ' test_cmp expected.color actual ' +test_expect_success 'color.ui=always does not override tty check' ' + git -c color.ui=always for-each-ref --format="$color_format" >actual && + test_cmp expected.bare actual +' + cat >expected <<\EOF heads/master tags/master diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index b90db1bf74..62aa322846 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1925,6 +1925,12 @@ test_expect_success '--color overrides auto-color' ' 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_decode_color actual && + test_cmp expect.color actual +' + test_expect_success 'setup --merged test tags' ' git tag mergetest-1 HEAD~2 && git tag mergetest-2 HEAD~1 && -- cgit v1.2.1