color: check color.ui in git_default_config()

peff · gitster · commit 136c8c8b8fa3 · 2017-07-13T12:42:51.000-07:00
Back in prehistoric times, our decision on whether or not to show color by default relied on using a config callback that either did or didn't load color config like color.diff. When we introduced color.ui, we put it in the same boat: commands had to manually respect it by using git_color_config() or its git_color_default_config() convenience wrapper. But in 4c7f181 (make color.ui default to 'auto', 2013-06-10), that changed. Since then, we default color.ui to auto in all programs, meaning that even plumbing commands like "git diff-tree --pretty" might colorize the output. Nobody seems to have complained in the intervening years, presumably because the "is stdout a tty" check does a good job of catching the right cases. But that leaves an interesting curiosity: color.ui defaults to auto even in plumbing, but you can't actually _disable_ the color via config. So if you really hate color and set "color.ui" to false, diff-tree will still show color (but porcelain like git-diff won't). Nobody noticed that either, probably because very few people disable color. One could argue that the plumbing should _always_ disable color unless an explicit --color option is given on the command line. But in practice, this creates a lot of complications for scripts which do want plumbing to show user-visible output. They can't just pass "--color" blindly; they need to check the user's config and decide what to send. Given that nobody has complained about the current behavior, let's assume it's a good path, and follow it to its conclusion: supporting color.ui everywhere. Note that you can create havoc by setting color.ui=always in your config, but that's more or less already the case. We could disallow it entirely, but it is handy for one-offs like: git -c color.ui=always foo >not-a-tty when "foo" does not take a --color option itself. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
diff --git 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_color_default_config(var, value, cb);
+	return git_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
@@ -125,8 +125,7 @@ static int git_clean_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	/* inspect the color.ui config variable and others */
-	return git_color_default_config(var, value, cb);
+	return git_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
@@ -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_color_default_config(var, value, cb) < 0)
+	if (git_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
@@ -554,7 +554,7 @@ static int git_show_branch_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	return git_color_default_config(var, value, cb);
+	return git_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
@@ -361,14 +361,6 @@ 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
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -1350,6 +1351,9 @@ 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
@@ -299,9 +299,6 @@ 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);
 }
 

Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,7 @@ static int git_branch_config(const char var, const char value, void *cb)`
`92`	`92`	`return config_error_nonbool(var);`
`93`	`93`	`return color_parse(value, branch_colors[slot]);`
`94`	`94`	`}`
`95`		`- return git_color_default_config(var, value, cb);`
	`95`	`+ return git_default_config(var, value, cb);`
`96`	`96`	`}`
`97`	`97`
`98`	`98`	`static const char *branch_get_color(enum color_branch ix)`
Original file line number	Diff line number	Diff line change
`@@ -125,8 +125,7 @@ static int git_clean_config(const char var, const char value, void *cb)`
`125`	`125`	`return 0;`
`126`	`126`	`}`
`127`	`127`
`128`		`- /* inspect the color.ui config variable and others */`
`129`		`- return git_color_default_config(var, value, cb);`
	`128`	`+ return git_default_config(var, value, cb);`
`130`	`129`	`}`
`131`	`130`
`132`	`131`	`static const char *clean_get_color(enum color_clean ix)`
Original file line number	Diff line number	Diff line change
`@@ -284,7 +284,7 @@ static int wait_all(void)`
`284`	`284`	`static int grep_cmd_config(const char var, const char value, void *cb)`
`285`	`285`	`{`
`286`	`286`	`int st = grep_config(var, value, cb);`
`287`		`- if (git_color_default_config(var, value, cb) < 0)`
	`287`	`+ if (git_default_config(var, value, cb) < 0)`
`288`	`288`	`st = -1;`
`289`	`289`
`290`	`290`	`if (!strcmp(var, "grep.threads")) {`
Original file line number	Diff line number	Diff line change
`@@ -554,7 +554,7 @@ static int git_show_branch_config(const char var, const char value, void *cb)`
`554`	`554`	`return 0;`
`555`	`555`	`}`
`556`	`556`
`557`		`- return git_color_default_config(var, value, cb);`
	`557`	`+ return git_default_config(var, value, cb);`
`558`	`558`	`}`
`559`	`559`
`560`	`560`	`static int omit_in_dense(struct commit commit, struct commit *rev, int n)`
Original file line number	Diff line number	Diff line change
`@@ -361,14 +361,6 @@ int git_color_config(const char var, const char value, void *cb)`
`361`	`361`	`return 0;`
`362`	`362`	`}`
`363`	`363`
`364`		`-int git_color_default_config(const char var, const char value, void *cb)`
`365`		`-{`
`366`		`- if (git_color_config(var, value, cb) < 0)`
`367`		`- return -1;`
`368`		`-`
`369`		`- return git_default_config(var, value, cb);`
`370`		`-}`
`371`		`-`
`372`	`364`	`void color_print_strbuf(FILE fp, const char color, const struct strbuf *sb)`
`373`	`365`	`{`
`374`	`366`	`if (*color)`
Original file line number	Diff line number	Diff line change
`@@ -299,9 +299,6 @@ int git_diff_ui_config(const char var, const char value, void *cb)`
`299`	`299`	`return 0;`
`300`	`300`	`}`
`301`	`301`
`302`		`- if (git_color_config(var, value, cb) < 0)`
`303`		`- return -1;`
`304`		`-`
`305`	`302`	`return git_diff_basic_config(var, value, cb);`
`306`	`303`	`}`
`307`	`304`