color: delay auto-color decision until point of use
When we read a color value either from a config file or from the command line, we use git_config_colorbool to convert it from the tristate always/never/auto into a single yes/no boolean value. This has some timing implications with respect to starting a pager. If we start (or decide not to start) the pager before checking the colorbool, everything is fine. Either isatty(1) will give us the right information, or we will properly check for pager_in_use(). However, if we decide to start a pager after we have checked the colorbool, things are not so simple. If stdout is a tty, then we will have already decided to use color. However, the user may also have configured color.pager not to use color with the pager. In this case, we need to actually turn off color. Unfortunately, the pager code has no idea which color variables were turned on (and there are many of them throughout the code, and they may even have been manipulated after the colorbool selection by something like "--color" on the command line). This bug can be seen any time a pager is started after config and command line options are checked. This has affected "git diff" since 89d07f7 (diff: don't run pager if user asked for a diff style exit code, 2007-08-12). It has also affect the log family since 1fda91b (Fix 'git log' early pager startup error case, 2010-08-24). This patch splits the notion of parsing a colorbool and actually checking the configuration. The "use_color" variables now have an additional possible value, GIT_COLOR_AUTO. Users of the variable should use the new "want_color()" wrapper, which will lazily determine and cache the auto-color decision. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King committed
Aug 17, 2011 at 22:04 UTC
daa0c3d9717624a62ce669252be832db12658ec0
11 files changed
+59
-19
builtin/branch.c
+1
-1
@@ -88,7 +88,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)
88
89
static const char *branch_get_color(enum color_branch ix)
90
{
91
- if (branch_use_color > 0)
91
+ if (want_color(branch_use_color))
92
return branch_colors[ix];
93
return "";
94
}
builtin/config.c
+2
@@ -330,6 +330,8 @@ static int get_colorbool(int print)
330
get_colorbool_found = git_use_color_default;
331
}
332
333
+ get_colorbool_found = want_color(get_colorbool_found);
334
+
335
if (print) {
336
printf("%s\n", get_colorbool_found ? "true" : "false");
337
return 0;
builtin/show-branch.c
+2
-2
@@ -26,14 +26,14 @@ static const char **default_arg;
26
27
static const char *get_color_code(int idx)
28
{
29
- if (showbranch_use_color)
29
+ if (want_color(showbranch_use_color))
30
return column_colors_ansi[idx % column_colors_ansi_max];
31
return "";
32
}
33
34
static const char *get_color_reset_code(void)
35
{
36
- if (showbranch_use_color)
36
+ if (want_color(showbranch_use_color))
37
return GIT_COLOR_RESET;
38
return "";
39
}
color.c
+18
-2
@@ -166,7 +166,7 @@ int git_config_colorbool(const char *var, const char *value)
166
if (!strcasecmp(value, "always"))
167
return 1;
168
if (!strcasecmp(value, "auto"))
169
- goto auto_color;
169
+ return GIT_COLOR_AUTO;
170
}
171
172
if (!var)
@@ -177,7 +177,11 @@ int git_config_colorbool(const char *var, const char *value)
177
return 0;
178
179
/* any normal truth value defaults to 'auto' */
180
- auto_color:
180
+ return GIT_COLOR_AUTO;
181
+}
182
+
183
+static int check_auto_color(void)
184
+{
185
if (color_stdout_is_tty < 0)
186
color_stdout_is_tty = isatty(1);
187
if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
@@ -188,6 +192,18 @@ int git_config_colorbool(const char *var, const char *value)
192
return 0;
193
}
194
195
+int want_color(int var)
196
+{
197
+ static int want_auto = -1;
198
+
199
+ if (var == GIT_COLOR_AUTO) {
200
+ if (want_auto < 0)
201
+ want_auto = check_auto_color();
202
+ return want_auto;
203
+ }
204
+ return var > 0;
205
+}
206
+
207
int git_color_default_config(const char *var, const char *value, void *cb)
208
{
209
if (!strcmp(var, "color.ui")) {
color.h
+11
@@ -48,6 +48,16 @@ struct strbuf;
48
/* A special value meaning "no color selected" */
49
#define GIT_COLOR_NIL "NIL"
50
51
+/*
52
+ * The first three are chosen to match common usage in the code, and what is
53
+ * returned from git_config_colorbool. The "auto" value can be returned from
54
+ * config_colorbool, and will be converted by want_color() into either 0 or 1.
55
+ */
56
+#define GIT_COLOR_UNKNOWN -1
57
+#define GIT_COLOR_NEVER 0
58
+#define GIT_COLOR_ALWAYS 1
59
+#define GIT_COLOR_AUTO 2
60
+
61
/*
62
* This variable stores the value of color.ui
63
*/
@@ -69,6 +79,7 @@ extern int color_stdout_is_tty;
79
int git_color_default_config(const char *var, const char *value, void *cb);
80
81
int git_config_colorbool(const char *var, const char *value);
82
+int want_color(int var);
83
void color_parse(const char *value, const char *var, char *dst);
84
void color_parse_mem(const char *value, int len, const char *var, char *dst);
85
__attribute__((format (printf, 3, 4)))
diff.c
+7
-10
@@ -622,7 +622,7 @@ static void emit_rewrite_diff(const char *name_a,
622
size_two = fill_textconv(textconv_two, two, &data_two);
623
624
memset(&ecbdata, 0, sizeof(ecbdata));
625
- ecbdata.color_diff = o->use_color > 0;
625
+ ecbdata.color_diff = want_color(o->use_color);
626
ecbdata.found_changesp = &o->found_changes;
627
ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
628
ecbdata.opt = o;
@@ -1003,7 +1003,7 @@ static void free_diff_words_data(struct emit_callback *ecbdata)
1003
1004
const char *diff_get_color(int diff_use_color, enum color_diff ix)
1005
{
1006
- if (diff_use_color > 0)
1006
+ if (want_color(diff_use_color))
1007
return diff_colors[ix];
1008
return "";
1009
}
@@ -2133,7 +2133,7 @@ static void builtin_diff(const char *name_a,
2133
memset(&xecfg, 0, sizeof(xecfg));
2134
memset(&ecbdata, 0, sizeof(ecbdata));
2135
ecbdata.label_path = lbl;
2136
- ecbdata.color_diff = o->use_color > 0;
2136
+ ecbdata.color_diff = want_color(o->use_color);
2137
ecbdata.found_changesp = &o->found_changes;
2138
ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
2139
if (ecbdata.ws_rule & WS_BLANK_AT_EOF)
@@ -2181,7 +2181,7 @@ static void builtin_diff(const char *name_a,
2181
break;
2182
}
2183
}
2184
- if (o->use_color > 0) {
2184
+ if (want_color(o->use_color)) {
2185
struct diff_words_style *st = ecbdata.diff_words->style;
2186
st->old.color = diff_get_color_opt(o, DIFF_FILE_OLD);
2187
st->new.color = diff_get_color_opt(o, DIFF_FILE_NEW);
@@ -2831,7 +2831,7 @@ static void run_diff_cmd(const char *pgm,
2831
*/
2832
fill_metainfo(msg, name, other, one, two, o, p,
2833
&must_show_header,
2834
- o->use_color > 0 && !pgm);
2834
+ want_color(o->use_color) && !pgm);
2835
xfrm_msg = msg->len ? msg->buf : NULL;
2836
}
2837
@@ -3374,12 +3374,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
3374
options->use_color = 1;
3375
else if (!prefixcmp(arg, "--color=")) {
3376
int value = git_config_colorbool(NULL, arg+8);
3377
- if (value == 0)
3378
- options->use_color = 0;
3379
- else if (value > 0)
3380
- options->use_color = 1;
3381
- else
3377
+ if (value < 0)
3378
return error("option `color' expects \"always\", \"auto\", or \"never\"");
3379
+ options->use_color = value;
3380
}
3381
else if (!strcmp(arg, "--no-color"))
3382
options->use_color = 0;
graph.c
+1
-1
@@ -347,7 +347,7 @@ static struct commit_list *first_interesting_parent(struct git_graph *graph)
347
348
static unsigned short graph_get_current_column_color(const struct git_graph *graph)
349
{
350
- if (graph->revs->diffopt.use_color <= 0)
350
+ if (!want_color(graph->revs->diffopt.use_color))
351
return column_colors_max;
352
return graph->default_column_color;
353
}
grep.c
+1
-1
@@ -430,7 +430,7 @@ static int word_char(char ch)
430
static void output_color(struct grep_opt *opt, const void *data, size_t size,
431
const char *color)
432
{
433
- if (opt->color && color && color[0]) {
433
+ if (want_color(opt->color) && color && color[0]) {
434
opt->output(opt, color, strlen(color));
435
opt->output(opt, data, size);
436
opt->output(opt, GIT_COLOR_RESET, strlen(GIT_COLOR_RESET));
log-tree.c
+1
-1
@@ -31,7 +31,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {
31
32
static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix)
33
{
34
- if (decorate_use_color > 0)
34
+ if (want_color(decorate_use_color))
35
return decoration_colors[ix];
36
return "";
37
}
t/t7006-pager.sh
+12
@@ -167,6 +167,18 @@ test_expect_success TTY 'color when writing to a pager' '
167
colorful paginated.out
168
'
169
170
+test_expect_success TTY 'colors are suppressed by color.pager' '
171
+ rm -f paginated.out &&
172
+ test_config color.ui auto &&
173
+ test_config color.pager false &&
174
+ (
175
+ TERM=vt100 &&
176
+ export TERM &&
177
+ test_terminal git log
178
+ ) &&
179
+ ! colorful paginated.out
180
+'
181
+
182
test_expect_success 'color when writing to a file intended for a pager' '
183
rm -f colorful.log &&
184
test_config color.ui auto ||
wt-status.c
+3
-1
@@ -26,7 +26,9 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
26
27
static const char *color(int slot, struct wt_status *s)
28
{
29
- const char *c = s->use_color > 0 ? s->color_palette[slot] : "";
29
+ const char *c = "";
30
+ if (want_color(s->use_color))
31
+ c = s->color_palette[slot];
32
if (slot == WT_STATUS_ONBRANCH && color_is_nil(c))
33
c = s->color_palette[WT_STATUS_HEADER];
34
return c;