From 2c1acdf6c9f6668dc520e75709af8447587eb4d0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Oct 2017 13:23:24 -0400 Subject: [PATCH 1/4] 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 --- Documentation/config.txt | 35 ++++++++++++++++++----------------- color.c | 2 +- t/t3701-add-interactive.sh | 2 +- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 517ee48ba6..2271809d90 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1052,10 +1052,10 @@ clean.requireForce:: color.branch:: A boolean to enable/disable color in the output of - 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). + 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). color.branch.:: Use customized color for branch coloration. `` is one of @@ -1066,11 +1066,12 @@ color.branch.:: color.diff:: Whether to use ANSI escape sequences to add color to patches. - If this is set to `true` or `auto`, linkgit:git-diff[1], + If this is set to `always`, linkgit:git-diff[1], linkgit:git-log[1], and linkgit:git-show[1] will use color - 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). + 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). + This does not affect linkgit:git-format-patch[1] or the 'git-diff-{asterisk}' plumbing commands. Can be overridden on the @@ -1123,12 +1124,12 @@ color.grep.:: -- color.interactive:: - When set to `true` or `auto`, use colors for interactive prompts + When set to `always`, always use colors for interactive prompts and displays (such as those used by "git-add --interactive" and - "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). + "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). color.interactive.:: Use customized color for 'git add --interactive' and 'git clean @@ -1175,10 +1176,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 `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`. + 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. column.ui:: Specify whether supported commands should output in columns. diff --git a/color.c b/color.c index 17e2713f96..7aa8b076f0 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 var ? GIT_COLOR_AUTO : 1; + return 1; if (!strcasecmp(value, "auto")) return GIT_COLOR_AUTO; } 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 && From 1d4b12fe7c44da166070a4b577c62fb83127f10e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Oct 2017 13:23:41 -0400 Subject: [PATCH 2/4] 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(-) 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' ' From 33c643bb083133376f3fdcb190ebc58f9eef12bb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Oct 2017 13:24:31 -0400 Subject: [PATCH 3/4] 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 --- 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 +++ t/t3701-add-interactive.sh | 2 +- 8 files changed, 17 insertions(+), 9 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 42ff87065a..7e79eb1a75 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 acebc85d81..231f9a750b 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); } 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 && From b521fd122865dca88b99d05344ec189d39efcefb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Oct 2017 13:26:02 -0400 Subject: [PATCH 4/4] 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 --- builtin/tag.c | 2 +- t/t6300-for-each-ref.sh | 5 +++++ t/t7004-tag.sh | 6 ++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/builtin/tag.c b/builtin/tag.c index fe0a8ab944..00382a56f5 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) 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 &&