From f1c9626105d5e4962a5ccaa4620114d03f32ad02 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 17 Aug 2011 22:03:12 -0700 Subject: diff: refactor COLOR_DIFF from a flag into an int This lets us store more than just a bit flag for whether we want color; we can also store whether we want automatic colors. This can be useful for making the automatic-color decision closer to the point of use. This mostly just involves replacing DIFF_OPT_* calls with manipulations of the flag. The biggest exception is that calls to DIFF_OPT_TST must check for "o->use_color > 0", which lets an "unknown" value (i.e., the default) stay at "no color". In the previous code, a value of "-1" was not propagated at all. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/merge.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'builtin') diff --git a/builtin/merge.c b/builtin/merge.c index 325891edb6..7209edf76a 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -390,8 +390,6 @@ static void finish(const unsigned char *new_head, const char *msg) opts.output_format |= DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT; opts.detect_rename = DIFF_DETECT_RENAME; - if (diff_use_color_default > 0) - DIFF_OPT_SET(&opts, COLOR_DIFF); if (diff_setup_done(&opts) < 0) die(_("diff_setup_done failed")); diff_tree_sha1(head, new_head, "", &opts); -- cgit v1.2.1 From e269eb7946d0a4ba6a4e175133b5479446ac04a5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 17 Aug 2011 22:03:48 -0700 Subject: git_config_colorbool: refactor stdout_is_tty handling Usually this function figures out for itself whether stdout is a tty. However, it has an extra parameter just to allow git-config to override the auto-detection for its --get-colorbool option. Instead of an extra parameter, let's just use a global variable. This makes calling easier in the common case, and will make refactoring the colorbool code much simpler. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/branch.c | 2 +- builtin/commit.c | 2 +- builtin/config.c | 23 +++++++---------------- builtin/grep.c | 2 +- builtin/show-branch.c | 2 +- 5 files changed, 11 insertions(+), 20 deletions(-) (limited to 'builtin') diff --git a/builtin/branch.c b/builtin/branch.c index 3142daa57a..b15fee5e31 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -71,7 +71,7 @@ static int parse_branch_color_slot(const char *var, int ofs) static int git_branch_config(const char *var, const char *value, void *cb) { if (!strcmp(var, "color.branch")) { - branch_use_color = git_config_colorbool(var, value, -1); + branch_use_color = git_config_colorbool(var, value); return 0; } if (!prefixcmp(var, "color.branch.")) { diff --git a/builtin/commit.c b/builtin/commit.c index e1af9b19f0..295803a265 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1144,7 +1144,7 @@ static int git_status_config(const char *k, const char *v, void *cb) return 0; } if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) { - s->use_color = git_config_colorbool(k, v, -1); + s->use_color = git_config_colorbool(k, v); return 0; } if (!prefixcmp(k, "status.color.") || !prefixcmp(k, "color.status.")) { diff --git a/builtin/config.c b/builtin/config.c index 211e118d57..5505ced8c1 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -303,24 +303,17 @@ static void get_color(const char *def_color) fputs(parsed_color, stdout); } -static int stdout_is_tty; static int get_colorbool_found; static int get_diff_color_found; static int git_get_colorbool_config(const char *var, const char *value, void *cb) { - if (!strcmp(var, get_colorbool_slot)) { - get_colorbool_found = - git_config_colorbool(var, value, stdout_is_tty); - } - if (!strcmp(var, "diff.color")) { - get_diff_color_found = - git_config_colorbool(var, value, stdout_is_tty); - } - if (!strcmp(var, "color.ui")) { - git_use_color_default = git_config_colorbool(var, value, stdout_is_tty); - return 0; - } + if (!strcmp(var, get_colorbool_slot)) + get_colorbool_found = git_config_colorbool(var, value); + else if (!strcmp(var, "diff.color")) + get_diff_color_found = git_config_colorbool(var, value); + else if (!strcmp(var, "color.ui")) + git_use_color_default = git_config_colorbool(var, value); return 0; } @@ -510,9 +503,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) } else if (actions == ACTION_GET_COLORBOOL) { if (argc == 1) - stdout_is_tty = git_config_bool("command line", argv[0]); - else if (argc == 0) - stdout_is_tty = isatty(1); + color_stdout_is_tty = git_config_bool("command line", argv[0]); return get_colorbool(argc != 0); } diff --git a/builtin/grep.c b/builtin/grep.c index 871afaa3c7..c17d7de562 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -316,7 +316,7 @@ static int grep_config(const char *var, const char *value, void *cb) } if (!strcmp(var, "color.grep")) - opt->color = git_config_colorbool(var, value, -1); + opt->color = git_config_colorbool(var, value); else if (!strcmp(var, "color.grep.context")) color = opt->color_context; else if (!strcmp(var, "color.grep.filename")) diff --git a/builtin/show-branch.c b/builtin/show-branch.c index facc63a79e..e6650b4837 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -573,7 +573,7 @@ static int git_show_branch_config(const char *var, const char *value, void *cb) } if (!strcmp(var, "color.showbranch")) { - showbranch_use_color = git_config_colorbool(var, value, -1); + showbranch_use_color = git_config_colorbool(var, value); return 0; } -- cgit v1.2.1 From daa0c3d9717624a62ce669252be832db12658ec0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 17 Aug 2011 22:04:23 -0700 Subject: 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 Signed-off-by: Junio C Hamano --- builtin/branch.c | 2 +- builtin/config.c | 2 ++ builtin/show-branch.c | 4 ++-- 3 files changed, 5 insertions(+), 3 deletions(-) (limited to 'builtin') diff --git a/builtin/branch.c b/builtin/branch.c index b15fee5e31..d6d3c7d85b 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -88,7 +88,7 @@ static int git_branch_config(const char *var, const char *value, void *cb) static const char *branch_get_color(enum color_branch ix) { - if (branch_use_color > 0) + if (want_color(branch_use_color)) return branch_colors[ix]; return ""; } diff --git a/builtin/config.c b/builtin/config.c index 5505ced8c1..3a092966d2 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -330,6 +330,8 @@ static int get_colorbool(int print) get_colorbool_found = git_use_color_default; } + get_colorbool_found = want_color(get_colorbool_found); + if (print) { printf("%s\n", get_colorbool_found ? "true" : "false"); return 0; diff --git a/builtin/show-branch.c b/builtin/show-branch.c index e6650b4837..4b726fabde 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -26,14 +26,14 @@ static const char **default_arg; static const char *get_color_code(int idx) { - if (showbranch_use_color) + if (want_color(showbranch_use_color)) return column_colors_ansi[idx % column_colors_ansi_max]; return ""; } static const char *get_color_reset_code(void) { - if (showbranch_use_color) + if (want_color(showbranch_use_color)) return GIT_COLOR_RESET; return ""; } -- cgit v1.2.1 From c659f55b31bb928688afd4ddfd94f8bea978ff1a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 17 Aug 2011 22:04:56 -0700 Subject: config: refactor get_colorbool function For "git config --get-colorbool color.foo", we use a custom callback that looks not only for the key that the user gave us, but also for "diff.color" (for backwards compatibility) and "color.ui" (as a fallback). For the former, we use a custom variable to store the diff.color value. For the latter, though, we store it in the main "git_use_color_default" variable, turning on color.ui for any other parts of git that respect this value. In practice, this doesn't cause any bugs, because git-config runs without caring about git_use_color_default, and then exits. But it crosses module boundaries in an unusual and confusing way, and it makes refactoring color handling harder than it needs to be. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/config.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'builtin') diff --git a/builtin/config.c b/builtin/config.c index 3a092966d2..0b4ecac855 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -305,6 +305,7 @@ static void get_color(const char *def_color) static int get_colorbool_found; static int get_diff_color_found; +static int get_color_ui_found; static int git_get_colorbool_config(const char *var, const char *value, void *cb) { @@ -313,7 +314,7 @@ static int git_get_colorbool_config(const char *var, const char *value, else if (!strcmp(var, "diff.color")) get_diff_color_found = git_config_colorbool(var, value); else if (!strcmp(var, "color.ui")) - git_use_color_default = git_config_colorbool(var, value); + get_color_ui_found = git_config_colorbool(var, value); return 0; } @@ -327,7 +328,7 @@ static int get_colorbool(int print) if (!strcmp(get_colorbool_slot, "color.diff")) get_colorbool_found = get_diff_color_found; if (get_colorbool_found < 0) - get_colorbool_found = git_use_color_default; + get_colorbool_found = get_color_ui_found; } get_colorbool_found = want_color(get_colorbool_found); -- cgit v1.2.1 From c9bfb953489e559d513c1627150aa16f8d42d6c5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 17 Aug 2011 22:05:35 -0700 Subject: want_color: automatically fallback to color.ui All of the "do we want color" flags default to -1 to indicate that we don't have any color configured. This value is handled in one of two ways: 1. In porcelain, we check early on whether the value is still -1 after reading the config, and set it to the value of color.ui (which defaults to 0). 2. In plumbing, it stays untouched as -1, and want_color defaults it to off. This works fine, but means that every porcelain has to check and reassign its color flag. Now that want_color gives us a place to put this check in a single spot, we can do that, simplifying the calling code. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/branch.c | 3 --- builtin/commit.c | 11 +---------- builtin/diff.c | 3 --- builtin/grep.c | 2 -- builtin/log.c | 12 ------------ builtin/merge.c | 4 ---- builtin/show-branch.c | 3 --- 7 files changed, 1 insertion(+), 37 deletions(-) (limited to 'builtin') diff --git a/builtin/branch.c b/builtin/branch.c index d6d3c7d85b..73d41700d1 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -673,9 +673,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix) git_config(git_branch_config, NULL); - if (branch_use_color == -1) - branch_use_color = git_use_color_default; - track = git_branch_track; head = resolve_ref("HEAD", head_sha1, 0, NULL); diff --git a/builtin/commit.c b/builtin/commit.c index 295803a265..9763146b6f 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1237,10 +1237,6 @@ int cmd_status(int argc, const char **argv, const char *prefix) if (s.relative_paths) s.prefix = prefix; - if (s.use_color == -1) - s.use_color = git_use_color_default; - if (diff_use_color_default == -1) - diff_use_color_default = git_use_color_default; switch (status_format) { case STATUS_FORMAT_SHORT: @@ -1394,15 +1390,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix) git_config(git_commit_config, &s); determine_whence(&s); - if (s.use_color == -1) - s.use_color = git_use_color_default; argc = parse_and_validate_options(argc, argv, builtin_commit_usage, prefix, &s); - if (dry_run) { - if (diff_use_color_default == -1) - diff_use_color_default = git_use_color_default; + if (dry_run) return dry_run_commit(argc, argv, prefix, &s); - } index_file = prepare_index(argc, argv, prefix, 0); /* Set up everything for writing the commit object. This includes diff --git a/builtin/diff.c b/builtin/diff.c index 69cd5eed78..1118689fb2 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -277,9 +277,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix) gitmodules_config(); git_config(git_diff_ui_config, NULL); - if (diff_use_color_default == -1) - diff_use_color_default = git_use_color_default; - init_revisions(&rev, prefix); /* If this is a no-index diff, just run it and exit there. */ diff --git a/builtin/grep.c b/builtin/grep.c index c17d7de562..18522cab75 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -883,8 +883,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix) strcpy(opt.color_sep, GIT_COLOR_CYAN); opt.color = -1; git_config(grep_config, &opt); - if (opt.color == -1) - opt.color = git_use_color_default; /* * If there is no -- then the paths must exist in the working diff --git a/builtin/log.c b/builtin/log.c index 5c2af59004..d760ee0885 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -359,9 +359,6 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix) git_config(git_log_config, NULL); - if (diff_use_color_default == -1) - diff_use_color_default = git_use_color_default; - init_revisions(&rev, prefix); rev.diff = 1; rev.simplify_history = 0; @@ -446,9 +443,6 @@ int cmd_show(int argc, const char **argv, const char *prefix) git_config(git_log_config, NULL); - if (diff_use_color_default == -1) - diff_use_color_default = git_use_color_default; - init_pathspec(&match_all, NULL); init_revisions(&rev, prefix); rev.diff = 1; @@ -524,9 +518,6 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix) git_config(git_log_config, NULL); - if (diff_use_color_default == -1) - diff_use_color_default = git_use_color_default; - init_revisions(&rev, prefix); init_reflog_walk(&rev.reflog_info); rev.verbose_header = 1; @@ -549,9 +540,6 @@ int cmd_log(int argc, const char **argv, const char *prefix) git_config(git_log_config, NULL); - if (diff_use_color_default == -1) - diff_use_color_default = git_use_color_default; - init_revisions(&rev, prefix); rev.always_show_header = 1; memset(&opt, 0, sizeof(opt)); diff --git a/builtin/merge.c b/builtin/merge.c index 7209edf76a..b75ae0193c 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1031,10 +1031,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix) git_config(git_merge_config, NULL); - /* for color.ui */ - if (diff_use_color_default == -1) - diff_use_color_default = git_use_color_default; - if (branch_mergeoptions) parse_branch_merge_options(branch_mergeoptions); argc = parse_options(argc, argv, prefix, builtin_merge_options, diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 4b726fabde..4b480d7c7c 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -685,9 +685,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) git_config(git_show_branch_config, NULL); - if (showbranch_use_color == -1) - showbranch_use_color = git_use_color_default; - /* If nothing is specified, try the default first */ if (ac == 1 && default_num) { ac = default_num; -- cgit v1.2.1