From 3c3e9b830383364316ba07730aecbc47a680b513 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 16:13:28 -0400 Subject: [PATCH 01/12] color: use GIT_COLOR_* instead of numeric constants Long ago Git's decision to show color for a subsytem was stored in a tri-state variable: it could be true (1), false (0), or unknown (-1). But since daa0c3d971 (color: delay auto-color decision until point of use, 2011-08-17) we want to carry around a new state, "auto", which bases the decision on the tty-ness of stdout (rather than collapsing that "auto" state to a true/false immediately). That commit introduced a set of GIT_COLOR_* defines to represent each state: UNKNOWN, ALWAYS, NEVER, and AUTO. But it only used the AUTO value, and left alone code using bare 0/1/-1 values. And of course since then we've grown many new spots that use those bare values. Let's switch all of these to use the named constants. That should make the code a bit easier to read, as it is more obvious that we're representing a color decision. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- add-interactive.c | 9 +++++---- advice.c | 2 +- builtin/add.c | 2 +- builtin/am.c | 4 ++-- builtin/branch.c | 2 +- builtin/clean.c | 2 +- builtin/commit.c | 2 +- builtin/config.c | 12 ++++++------ builtin/grep.c | 2 +- builtin/push.c | 2 +- builtin/range-diff.c | 3 ++- builtin/show-branch.c | 2 +- color.c | 12 ++++++------ diff.c | 6 +++--- grep.h | 2 +- parse-options-cb.c | 2 +- pretty.c | 2 +- ref-filter.h | 2 +- sideband.c | 4 ++-- transport.c | 2 +- wt-status.c | 6 +++--- 21 files changed, 42 insertions(+), 40 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 4604c69140..34c020673e 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -42,7 +42,7 @@ static int check_color_config(struct repository *r, const char *var) int ret; if (repo_config_get_value(r, var, &value)) - ret = -1; + ret = GIT_COLOR_UNKNOWN; else ret = git_config_colorbool(var, value); @@ -51,7 +51,8 @@ static int check_color_config(struct repository *r, const char *var) * the value parsed by git_color_config(), which may not have been * called by the main command. */ - if (ret < 0 && !repo_config_get_value(r, "color.ui", &value)) + if (ret == GIT_COLOR_UNKNOWN && + !repo_config_get_value(r, "color.ui", &value)) ret = git_config_colorbool("color.ui", value); return want_color(ret); @@ -130,8 +131,8 @@ void clear_add_i_state(struct add_i_state *s) FREE_AND_NULL(s->interactive_diff_filter); FREE_AND_NULL(s->interactive_diff_algorithm); memset(s, 0, sizeof(*s)); - s->use_color_interactive = -1; - s->use_color_diff = -1; + s->use_color_interactive = GIT_COLOR_UNKNOWN; + s->use_color_diff = GIT_COLOR_UNKNOWN; } /* diff --git a/advice.c b/advice.c index e5f0ff8449..a00aaad9de 100644 --- a/advice.c +++ b/advice.c @@ -7,7 +7,7 @@ #include "help.h" #include "string-list.h" -static int advice_use_color = -1; +static int advice_use_color = GIT_COLOR_UNKNOWN; static char advice_colors[][COLOR_MAXLEN] = { GIT_COLOR_RESET, GIT_COLOR_YELLOW, /* HINT */ diff --git a/builtin/add.c b/builtin/add.c index 0235854f80..36475ac39e 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -200,7 +200,7 @@ static int edit_patch(struct repository *repo, argc = setup_revisions(argc, argv, &rev, NULL); rev.diffopt.output_format = DIFF_FORMAT_PATCH; - rev.diffopt.use_color = 0; + rev.diffopt.use_color = GIT_COLOR_NEVER; rev.diffopt.flags.ignore_dirty_submodules = 1; out = xopen(file, O_CREAT | O_WRONLY | O_TRUNC, 0666); rev.diffopt.file = xfdopen(out, "w"); diff --git a/builtin/am.c b/builtin/am.c index 6073d64ae9..277c2e7937 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1408,7 +1408,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm rev_info.no_commit_id = 1; rev_info.diffopt.flags.binary = 1; rev_info.diffopt.flags.full_index = 1; - rev_info.diffopt.use_color = 0; + rev_info.diffopt.use_color = GIT_COLOR_NEVER; rev_info.diffopt.file = fp; rev_info.diffopt.close_file = 1; add_pending_object(&rev_info, &commit->object, ""); @@ -1441,7 +1441,7 @@ static void write_index_patch(const struct am_state *state) rev_info.disable_stdin = 1; rev_info.no_commit_id = 1; rev_info.diffopt.output_format = DIFF_FORMAT_PATCH; - rev_info.diffopt.use_color = 0; + rev_info.diffopt.use_color = GIT_COLOR_NEVER; rev_info.diffopt.file = fp; rev_info.diffopt.close_file = 1; add_pending_object(&rev_info, &tree->object, ""); diff --git a/builtin/branch.c b/builtin/branch.c index fa5ced452e..029223df7b 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -46,7 +46,7 @@ static struct object_id head_oid; static int recurse_submodules = 0; static int submodule_propagate_branches = 0; -static int branch_use_color = -1; +static int branch_use_color = GIT_COLOR_UNKNOWN; static char branch_colors[][COLOR_MAXLEN] = { GIT_COLOR_RESET, GIT_COLOR_NORMAL, /* PLAIN */ diff --git a/builtin/clean.c b/builtin/clean.c index a1977b92dc..8e3598d030 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -64,7 +64,7 @@ static const char *color_interactive_slots[] = { [CLEAN_COLOR_RESET] = "reset", }; -static int clean_use_color = -1; +static int clean_use_color = GIT_COLOR_UNKNOWN; static char clean_colors[][COLOR_MAXLEN] = { [CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED, [CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD, diff --git a/builtin/commit.c b/builtin/commit.c index b5b9608813..6c5784646a 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1016,7 +1016,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); /* Add new line for clarity */ saved_color_setting = s->use_color; - s->use_color = 0; + s->use_color = GIT_COLOR_NEVER; committable = run_status(s->fp, index_file, prefix, 1, s); s->use_color = saved_color_setting; string_list_clear_func(&s->change, change_data_free); diff --git a/builtin/config.c b/builtin/config.c index 59fb113b07..c3da3ae210 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -594,23 +594,23 @@ static int get_colorbool(const struct config_location_options *opts, { struct get_colorbool_config_data data = { .get_colorbool_slot = var, - .get_colorbool_found = -1, - .get_diff_color_found = -1, - .get_color_ui_found = -1, + .get_colorbool_found = GIT_COLOR_UNKNOWN, + .get_diff_color_found = GIT_COLOR_UNKNOWN, + .get_color_ui_found = GIT_COLOR_UNKNOWN, }; config_with_options(git_get_colorbool_config, &data, &opts->source, the_repository, &opts->options); - if (data.get_colorbool_found < 0) { + if (data.get_colorbool_found == GIT_COLOR_UNKNOWN) { if (!strcmp(data.get_colorbool_slot, "color.diff")) data.get_colorbool_found = data.get_diff_color_found; - if (data.get_colorbool_found < 0) + if (data.get_colorbool_found == GIT_COLOR_UNKNOWN) data.get_colorbool_found = data.get_color_ui_found; } - if (data.get_colorbool_found < 0) + if (data.get_colorbool_found == GIT_COLOR_UNKNOWN) /* default value if none found in config */ data.get_colorbool_found = GIT_COLOR_AUTO; diff --git a/builtin/grep.c b/builtin/grep.c index 5df6537333..1d97eb2a2a 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -1091,7 +1091,7 @@ int cmd_grep(int argc, if (show_in_pager == default_pager) show_in_pager = git_pager(the_repository, 1); if (show_in_pager) { - opt.color = 0; + opt.color = GIT_COLOR_NEVER; opt.name_only = 1; opt.null_following_name = 1; opt.output_priv = &path_list; diff --git a/builtin/push.c b/builtin/push.c index d0794b7b30..0962b122c7 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -27,7 +27,7 @@ static const char * const push_usage[] = { NULL, }; -static int push_use_color = -1; +static int push_use_color = GIT_COLOR_UNKNOWN; static char push_colors[][COLOR_MAXLEN] = { GIT_COLOR_RESET, GIT_COLOR_RED, /* ERROR */ diff --git a/builtin/range-diff.c b/builtin/range-diff.c index a563abff5f..0d51ddd623 100644 --- a/builtin/range-diff.c +++ b/builtin/range-diff.c @@ -6,6 +6,7 @@ #include "parse-options.h" #include "range-diff.h" #include "config.h" +#include "color.h" static const char * const builtin_range_diff_usage[] = { @@ -66,7 +67,7 @@ int cmd_range_diff(int argc, /* force color when --dual-color was used */ if (!simple_color) - diffopt.use_color = 1; + diffopt.use_color = GIT_COLOR_ALWAYS; /* If `--diff-merges` was specified, imply `--merges` */ if (diff_merges_arg.nr) { diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 1ab7db9d2c..970e78bc2d 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -29,7 +29,7 @@ static const char*const show_branch_usage[] = { NULL }; -static int showbranch_use_color = -1; +static int showbranch_use_color = GIT_COLOR_UNKNOWN; static struct strvec default_args = STRVEC_INIT; diff --git a/color.c b/color.c index 7df8862c71..22aa453fef 100644 --- a/color.c +++ b/color.c @@ -373,19 +373,19 @@ int git_config_colorbool(const char *var, const char *value) { if (value) { if (!strcasecmp(value, "never")) - return 0; + return GIT_COLOR_NEVER; if (!strcasecmp(value, "always")) - return 1; + return GIT_COLOR_ALWAYS; if (!strcasecmp(value, "auto")) return GIT_COLOR_AUTO; } if (!var) - return -1; + return GIT_COLOR_UNKNOWN; /* Missing or explicit false to turn off colorization */ if (!git_config_bool(var, value)) - return 0; + return GIT_COLOR_NEVER; /* any normal truth value defaults to 'auto' */ return GIT_COLOR_AUTO; @@ -418,7 +418,7 @@ int want_color_fd(int fd, int var) if (fd < 1 || fd >= ARRAY_SIZE(want_auto)) BUG("file descriptor out of range: %d", fd); - if (var < 0) + if (var == GIT_COLOR_UNKNOWN) var = git_use_color_default; if (var == GIT_COLOR_AUTO) { @@ -426,7 +426,7 @@ int want_color_fd(int fd, int var) want_auto[fd] = check_auto_color(fd); return want_auto[fd]; } - return var; + return var == GIT_COLOR_ALWAYS; } int git_color_config(const char *var, const char *value, void *cb UNUSED) diff --git a/diff.c b/diff.c index dca87e164f..d9bed49f61 100644 --- a/diff.c +++ b/diff.c @@ -57,7 +57,7 @@ static int diff_detect_rename_default; static int diff_indent_heuristic = 1; static int diff_rename_limit_default = 1000; static int diff_suppress_blank_empty; -static int diff_use_color_default = -1; +static int diff_use_color_default = GIT_COLOR_UNKNOWN; static int diff_color_moved_default; static int diff_color_moved_ws_default; static int diff_context_default = 3; @@ -5259,7 +5259,7 @@ static int diff_opt_color_words(const struct option *opt, struct diff_options *options = opt->value; BUG_ON_OPT_NEG(unset); - options->use_color = 1; + options->use_color = GIT_COLOR_ALWAYS; options->word_diff = DIFF_WORDS_COLOR; options->word_regex = arg; return 0; @@ -5581,7 +5581,7 @@ static int diff_opt_word_diff(const struct option *opt, if (!strcmp(arg, "plain")) options->word_diff = DIFF_WORDS_PLAIN; else if (!strcmp(arg, "color")) { - options->use_color = 1; + options->use_color = GIT_COLOR_ALWAYS; options->word_diff = DIFF_WORDS_COLOR; } else if (!strcmp(arg, "porcelain")) diff --git a/grep.h b/grep.h index 926c0875c4..43195baab3 100644 --- a/grep.h +++ b/grep.h @@ -198,7 +198,7 @@ struct grep_opt { [GREP_COLOR_SEP] = GIT_COLOR_CYAN, \ }, \ .only_matching = 0, \ - .color = -1, \ + .color = GIT_COLOR_UNKNOWN, \ .output = std_output, \ } diff --git a/parse-options-cb.c b/parse-options-cb.c index 50c8afe412..e13e0a9e33 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -55,7 +55,7 @@ int parse_opt_color_flag_cb(const struct option *opt, const char *arg, if (!arg) arg = unset ? "never" : (const char *)opt->defval; value = git_config_colorbool(NULL, arg); - if (value < 0) + if (value == GIT_COLOR_UNKNOWN) return error(_("option `%s' expects \"always\", \"auto\", or \"never\""), opt->long_name); *(int *)opt->value = value; diff --git a/pretty.c b/pretty.c index cee96b9d94..0521deadc0 100644 --- a/pretty.c +++ b/pretty.c @@ -1462,7 +1462,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ } else { int ret = parse_color(sb, placeholder, c); if (ret) - c->auto_color = 0; + c->auto_color = GIT_COLOR_NEVER; /* * Otherwise, we decided to treat %C * as a literal string, and the previous diff --git a/ref-filter.h b/ref-filter.h index f22ca94b49..644f5c567c 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -111,7 +111,7 @@ struct ref_format { .exclude = STRVEC_INIT, \ } #define REF_FORMAT_INIT { \ - .use_color = -1, \ + .use_color = GIT_COLOR_UNKNOWN, \ } /* Macros for checking --merged and --no-merged options */ diff --git a/sideband.c b/sideband.c index 8f15b98a65..3ac87148b9 100644 --- a/sideband.c +++ b/sideband.c @@ -29,14 +29,14 @@ static struct keyword_entry keywords[] = { /* Returns a color setting (GIT_COLOR_NEVER, etc). */ static int use_sideband_colors(void) { - static int use_sideband_colors_cached = -1; + static int use_sideband_colors_cached = GIT_COLOR_UNKNOWN; const char *key = "color.remote"; struct strbuf sb = STRBUF_INIT; const char *value; int i; - if (use_sideband_colors_cached >= 0) + if (use_sideband_colors_cached != GIT_COLOR_UNKNOWN) return use_sideband_colors_cached; if (!repo_config_get_string_tmp(the_repository, key, &value)) diff --git a/transport.c b/transport.c index e305d6bd22..4f54ef1b12 100644 --- a/transport.c +++ b/transport.c @@ -30,7 +30,7 @@ #include "color.h" #include "bundle-uri.h" -static int transport_use_color = -1; +static int transport_use_color = GIT_COLOR_UNKNOWN; static char transport_colors[][COLOR_MAXLEN] = { GIT_COLOR_RESET, GIT_COLOR_RED /* REJECTED */ diff --git a/wt-status.c b/wt-status.c index 454601afa1..ca14cafea3 100644 --- a/wt-status.c +++ b/wt-status.c @@ -148,7 +148,7 @@ void wt_status_prepare(struct repository *r, struct wt_status *s) memcpy(s->color_palette, default_wt_status_colors, sizeof(default_wt_status_colors)); s->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES; - s->use_color = -1; + s->use_color = GIT_COLOR_UNKNOWN; s->relative_paths = 1; s->branch = refs_resolve_refdup(get_main_ref_store(the_repository), "HEAD", 0, NULL, NULL); @@ -1164,7 +1164,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s) * before. */ if (s->fp != stdout) { - rev.diffopt.use_color = 0; + rev.diffopt.use_color = GIT_COLOR_NEVER; wt_status_add_cut_line(s); } if (s->verbose > 1 && s->committable) { @@ -2164,7 +2164,7 @@ static void wt_shortstatus_print(struct wt_status *s) static void wt_porcelain_print(struct wt_status *s) { - s->use_color = 0; + s->use_color = GIT_COLOR_NEVER; s->relative_paths = 0; s->prefix = NULL; s->no_gettext = 1; From 53e8a435ba94cc222f74efe49efc9f386ad2f490 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 16:14:07 -0400 Subject: [PATCH 02/12] color: return enum from git_config_colorbool() The git_config_colorbool() function returns an integer which is always one of the GIT_COLOR_* constants UNKNOWN, NEVER, ALWAYS, or AUTO. We define these constants with macros, but let's switch to using an enum. Even though the compiler does not strictly enforce enum/int conversions, this should make the intent clearer to human readers. And as a bonus, enum names are typically available to debuggers, making it more pleasant to step through the code there. This patch updates the return type of git_config_colorbool(), but holds off on updating all of the callers. There's some trickiness to some of them, and in the meantime it's perfectly fine to assign an enum into an int. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- color.c | 2 +- color.h | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/color.c b/color.c index 22aa453fef..f3adce0141 100644 --- a/color.c +++ b/color.c @@ -369,7 +369,7 @@ bad: #undef OUT } -int git_config_colorbool(const char *var, const char *value) +enum git_colorbool git_config_colorbool(const char *var, const char *value) { if (value) { if (!strcasecmp(value, "never")) diff --git a/color.h b/color.h index 7ed259a35b..303e2c9a6d 100644 --- a/color.h +++ b/color.h @@ -73,10 +73,12 @@ struct strbuf; * returned from git_config_colorbool. The "auto" value can be returned from * config_colorbool, and will be converted by want_color() into either 0 or 1. */ -#define GIT_COLOR_UNKNOWN -1 -#define GIT_COLOR_NEVER 0 -#define GIT_COLOR_ALWAYS 1 -#define GIT_COLOR_AUTO 2 +enum git_colorbool { + GIT_COLOR_UNKNOWN = -1, + GIT_COLOR_NEVER = 0, + GIT_COLOR_ALWAYS = 1, + GIT_COLOR_AUTO = 2, +}; /* A default list of colors to use for commit graphs and show-branch output */ extern const char *column_colors_ansi[]; @@ -98,7 +100,7 @@ int git_color_config(const char *var, const char *value, void *cb); * GIT_COLOR_ALWAYS for "always" or a positive boolean, * and GIT_COLOR_AUTO for "auto". */ -int git_config_colorbool(const char *var, const char *value); +enum git_colorbool git_config_colorbool(const char *var, const char *value); /* * Return a boolean whether to use color, where the argument 'var' is From 8ee247671ddbe5b1128d774b0950627bb4afc4a6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 16:16:00 -0400 Subject: [PATCH 03/12] grep: don't treat grep_opt.color as a strict bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In show_line(), we check to see if colors are desired with just: if (opt->color) ...we want colors... But this is incorrect. The color field here is really a git_colorbool, so it may be "true" for GIT_COLOR_UNKNOWN or GIT_COLOR_AUTO. Either of those _might_ end up true eventually (once we apply default fallbacks and check stdout's tty), but they may not. E.g.: git grep foo | cat will enter the conditional even though we're not going to show colors. We should collapse it into a true boolean by calling want_color(). It turns out that this does not produce a user-visible bug. We do some extra processing to isolate the matched portion of the line in order to colorize it, but ultimately we pass it to our output_color() helper, which does correctly check want_color(). So we end up with no colors. But dropping the extra processing saves a measurable amount of time. For example, running under hyperfine (which redirects to /dev/null, and thus does not colorize): Benchmark 1: ./git.old grep a Time (mean ± σ): 58.7 ms ± 3.5 ms [User: 580.6 ms, System: 74.3 ms] Range (min … max): 53.5 ms … 67.1 ms 48 runs Benchmark 2: ./git.new grep a Time (mean ± σ): 35.5 ms ± 0.9 ms [User: 276.8 ms, System: 73.8 ms] Range (min … max): 34.3 ms … 39.3 ms 79 runs Summary ./git.new grep a ran 1.65 ± 0.11 times faster than ./git.old grep a That's a fairly extreme benchmark, just because it will come up with a ton of small matches, but it shows that this really does matter. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- grep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/grep.c b/grep.c index 932647e4a6..c7e1dc1e0e 100644 --- a/grep.c +++ b/grep.c @@ -1263,12 +1263,12 @@ static void show_line(struct grep_opt *opt, */ show_line_header(opt, name, lno, cno, sign); } - if (opt->color || opt->only_matching) { + if (want_color(opt->color) || opt->only_matching) { regmatch_t match; enum grep_context ctx = GREP_CONTEXT_BODY; int eflags = 0; - if (opt->color) { + if (want_color(opt->color)) { if (sign == ':') match_color = opt->colors[GREP_COLOR_MATCH_SELECTED]; else From 8efe643e0e0c70b1eff9e96276afecf05684d133 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 16:17:19 -0400 Subject: [PATCH 04/12] diff: simplify color_moved check when flushing In diff_flush_patch_all_file_pairs(), we set o->emitted_symbols if and only if o->color_moved is true. That causes the lower-level routines to fill up o->emitted_symbols, which we then analyze in order to do the actual colorizing. But in that final step, we do: if (o->emitted_symbols) { if (o->color_moved) { ...actual coloring... } ...clean up of emitted_symbols... } The inner "if" will always trigger, since we set emitted_symbols only when doing color_moved (it is a little confusing that it is set inside the diff_options struct, but that is for convenience of passing it to the lower-level routines; we always clear it at the end of flushing, since 48edf3a02a (diff: clear emitted_symbols flag after use, 2019-01-24)). Let's simplify the code a bit by just dropping the inner "if" and running its block unconditionally. In theory the current code might be useful if another feature besides color_moved setup and used emitted_symbols, but it would be easy to refactor later to handle that. And in the meantime, this makes further work in this area easier. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/diff.c b/diff.c index d9bed49f61..c178aba75b 100644 --- a/diff.c +++ b/diff.c @@ -6690,20 +6690,17 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o) } if (o->emitted_symbols) { - if (o->color_moved) { - struct mem_pool entry_pool; - struct moved_entry_list *entry_list; + struct mem_pool entry_pool; + struct moved_entry_list *entry_list; - mem_pool_init(&entry_pool, 1024 * 1024); - entry_list = add_lines_to_move_detection(o, - &entry_pool); - mark_color_as_moved(o, entry_list); - if (o->color_moved == COLOR_MOVED_ZEBRA_DIM) - dim_moved_lines(o); + mem_pool_init(&entry_pool, 1024 * 1024); + entry_list = add_lines_to_move_detection(o, &entry_pool); + mark_color_as_moved(o, entry_list); + if (o->color_moved == COLOR_MOVED_ZEBRA_DIM) + dim_moved_lines(o); - mem_pool_discard(&entry_pool, 0); - free(entry_list); - } + mem_pool_discard(&entry_pool, 0); + free(entry_list); for (i = 0; i < esm.nr; i++) emit_diff_symbol_from_struct(o, &esm.buf[i]); From 4cfc971a2b73dba822a37a2996ff0246155b7fca Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 16:19:33 -0400 Subject: [PATCH 05/12] diff: don't use diff_options.use_color as a strict bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We disable --color-moved if color is not in use at all. This happens in diff_setup_done(), where we set options->color_moved to 0 if options->use_color is not true. But a strict boolean check here is not correct; use_color could be GIT_COLOR_UNKNOWN or GIT_COLOR_AUTO, both of which evaluate to true, even though we may later decide not to show colors. We should be using want_color() to convert that git_colorbool into a true boolean. As it turns out, this does not produce wrong output. Even though we go to the trouble to detect the moved lines, ultimately we get the color values from diff_get_color(), which does check want_color(). And so it returns the empty string for each color, and we "color" the result with nothing. So the output is correct, but there is a small but measurable performance cost to doing the line detection. E.g., in git.git before and after this patch (there are no colors shown because hyperfine redirects output to /dev/null): Benchmark 1: ./git.old log --no-merges -p --color-moved -1000 Time (mean ± σ): 1.019 s ± 0.013 s [User: 0.955 s, System: 0.064 s] Range (min … max): 1.005 s … 1.045 s 10 runs Benchmark 2: ./git.new log --no-merges -p --color-moved -1000 Time (mean ± σ): 982.9 ms ± 14.5 ms [User: 925.8 ms, System: 57.1 ms] Range (min … max): 965.1 ms … 1003.2 ms 10 runs Summary ./git.new log --no-merges -p --color-moved -1000 ran 1.04 ± 0.02 times faster than ./git.old log --no-merges -p --color-moved -1000 Note that the fix is not quite as simple as just calling want_color() from diff_setup_done(). There's a subtle timing issue that goes back to daa0c3d971 (color: delay auto-color decision until point of use, 2011-08-17), the commit that adds want_color() in the first place. As discussed there, we must delay evaluating the colorbool value until all pager setup is complete. So instead, we'll leave the "color_moved" field intact in diff_setup_done(), and modify the point where it is evaluated. Fortunately there is only one such spot that controls whether we run any of the color-moved code at all. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/diff.c b/diff.c index c178aba75b..bce26af0e3 100644 --- a/diff.c +++ b/diff.c @@ -4979,8 +4979,7 @@ void diff_setup_done(struct diff_options *options) if (options->flags.follow_renames) diff_check_follow_pathspec(&options->pathspec, 1); - if (!options->use_color || - (options->flags.allow_external && external_diff())) + if (options->flags.allow_external && external_diff()) options->color_moved = 0; if (options->filter_not) { @@ -6677,7 +6676,7 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o) if (WSEH_NEW & WS_RULE_MASK) BUG("WS rules bit mask overlaps with diff symbol flags"); - if (o->color_moved) + if (o->color_moved && want_color(o->use_color)) o->emitted_symbols = &esm; if (o->additional_path_headers) From 12df3c2e99f0692155c8ad083c6dba8c8ee30033 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 16:20:26 -0400 Subject: [PATCH 06/12] diff: pass o->use_color directly to fill_metainfo() We pass the use_color parameter of fill_metainfo() as a strict boolean, using: want_color(o->use_color) && !pgm to derive its value. But then inside the function, we pass it to diff_get_color(), which expects one of the git_colorbool enum values, and so feeds it to want_color() again. Even though want_color() produces a strict 0/1 boolean, this doesn't produce wrong results because want_color() is idempotent. Since GIT_COLOR_ALWAYS and NEVER are defined as 1 and 0, and because want_color() passes through those values, evaluating "want_color(foo)" and "want_color(want_color(foo))" will return the same result. But as part of a longer strategy to align the types we use for storing these values, let's pass through the colorbool directly. To handle the "&&" case here, we'll convert the presence of "pgm" into "NEVER", which arguably makes the intent of the code more clear anyway. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/diff.c b/diff.c index bce26af0e3..505819c6c6 100644 --- a/diff.c +++ b/diff.c @@ -4580,7 +4580,7 @@ static void run_diff_cmd(const struct external_diff *pgm, */ fill_metainfo(msg, name, other, one, two, o, p, &must_show_header, - want_color(o->use_color) && !pgm); + pgm ? GIT_COLOR_NEVER : o->use_color); xfrm_msg = msg->len ? msg->buf : NULL; } From 955000d91718eee5abd005dd43ed035a5115d870 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 16:21:20 -0400 Subject: [PATCH 07/12] diff: stop passing ecbdata->use_color as boolean In emit_hunk_header(), we evaluate ecbdata->color_diff both as a git_colorbool, passing it to diff_get_color(): const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET); and as a strict boolean: const char *reverse = ecbdata->color_diff ? GIT_COLOR_REVERSE : ""; At first glance this seems wrong. Usually we store the color decision as a git_colorbool, so the second line would get confused by GIT_COLOR_AUTO (which is boolean true, but may still mean we do not produce color). However, the second line is correct because our caller sets color_diff using want_color(), which collapses the colorbool to a strict true/false boolean. The first line is _also_ correct because of the idempotence of want_color(). Even though diff_get_color() will pass our true/false value through want_color() again, the result will be left untouched. But let's pass through the colorbool itself, which makes it more consistent with the rest of the diff code. We'll need to then call want_color() whenever we treat it as a boolean, but there is only such spot (the one quoted above). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/diff.c b/diff.c index 505819c6c6..3544be2318 100644 --- a/diff.c +++ b/diff.c @@ -1678,7 +1678,7 @@ static void emit_hunk_header(struct emit_callback *ecbdata, const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO); const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO); const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET); - const char *reverse = ecbdata->color_diff ? GIT_COLOR_REVERSE : ""; + const char *reverse = want_color(ecbdata->color_diff) ? GIT_COLOR_REVERSE : ""; static const char atat[2] = { '@', '@' }; const char *cp, *ep; struct strbuf msgbuf = STRBUF_INIT; @@ -1832,7 +1832,7 @@ static void emit_rewrite_diff(const char *name_a, size_two = fill_textconv(o->repo, textconv_two, two, &data_two); memset(&ecbdata, 0, sizeof(ecbdata)); - ecbdata.color_diff = want_color(o->use_color); + ecbdata.color_diff = o->use_color; ecbdata.ws_rule = whitespace_rule(o->repo->index, name_b); ecbdata.opt = o; if (ecbdata.ws_rule & WS_BLANK_AT_EOF) { @@ -3729,7 +3729,7 @@ static void builtin_diff(const char *name_a, if (o->flags.suppress_diff_headers) lbl[0] = NULL; ecbdata.label_path = lbl; - ecbdata.color_diff = want_color(o->use_color); + ecbdata.color_diff = o->use_color; ecbdata.ws_rule = whitespace_rule(o->repo->index, name_b); if (ecbdata.ws_rule & WS_BLANK_AT_EOF) check_blank_at_eof(&mf1, &mf2, &ecbdata); From 5e9ddd3c0652ad4e16cc33525d611e23f61dc6a5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 16:22:26 -0400 Subject: [PATCH 08/12] pretty: use format_commit_context.auto_color as colorbool When we see "%C(auto)" as a format placeholder, we evaluate the "color" field of our pretty_print_context to decide whether we want color. The auto_color field of format_commit_context then stores the boolean result of want_color(), telling us the yes/no of whether we want color. But the resulting field is passed to various functions which expect a git_colorbool, like diff_get_color(), that will then pass it to want_color() again. It's not wrong to do so, since want_color() is idempotent. But it makes it harder to reason about the types, since we sometimes confuse colorbools and strict booleans. Let's instead store auto_color as the original colorbool itself. We'll have to make sure it is passed through want_color() when it is evaluated, but there is only one such spot (right next to where we assign it!). Every other caller just ends up passing it to get diff_get_color() either directly or through another helper. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- pretty.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pretty.c b/pretty.c index 0521deadc0..86d69bf877 100644 --- a/pretty.c +++ b/pretty.c @@ -1455,8 +1455,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ switch (placeholder[0]) { case 'C': if (starts_with(placeholder + 1, "(auto)")) { - c->auto_color = want_color(c->pretty_ctx->color); - if (c->auto_color && sb->len) + c->auto_color = c->pretty_ctx->color; + if (want_color(c->auto_color) && sb->len) strbuf_addstr(sb, GIT_COLOR_RESET); return 7; /* consumed 7 bytes, "C(auto)" */ } else { From e9330ae4b820147c98e723399e9438c8bee60a80 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 19:13:59 -0400 Subject: [PATCH 09/12] color: use git_colorbool enum type to store colorbools We traditionally used "int" to store and pass around the values defined by "enum git_colorbool" (which were originally just #define macros). Using an int doesn't produce incorrect results, but using the actual enum makes the intent of the code more clear. It would be nice if the compiler could catch cases where we used the enum and an int interchangeably, since it's very easy to accidentally check the boolean true/false of a colorbool like: if (branch_use_color) This is wrong because GIT_COLOR_UNKNOWN and GIT_COLOR_AUTO evaluate to true in C, even though we may ultimately decide not to use color. But C is pretty happy to convert between ints and enums (even with various -Wenum-* warnings). So this sadly doesn't protect us from such mistakes, but it hopefully does make the code easier to read. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- add-interactive.c | 2 +- advice.c | 2 +- builtin/branch.c | 2 +- builtin/clean.c | 2 +- builtin/commit.c | 2 +- builtin/config.c | 6 +++--- builtin/push.c | 2 +- builtin/show-branch.c | 2 +- color.c | 4 ++-- color.h | 2 +- combine-diff.c | 2 +- diff.c | 6 +++--- diff.h | 5 +++-- grep.h | 2 +- log-tree.c | 4 ++-- log-tree.h | 4 +++- parse-options-cb.c | 2 +- pretty.c | 6 +++--- pretty.h | 3 ++- ref-filter.h | 2 +- sideband.c | 4 ++-- transport.c | 2 +- wt-status.h | 2 +- 23 files changed, 37 insertions(+), 33 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 34c020673e..000315971e 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -39,7 +39,7 @@ static void init_color(struct repository *r, int use_color, static int check_color_config(struct repository *r, const char *var) { const char *value; - int ret; + enum git_colorbool ret; if (repo_config_get_value(r, var, &value)) ret = GIT_COLOR_UNKNOWN; diff --git a/advice.c b/advice.c index a00aaad9de..0018501b7b 100644 --- a/advice.c +++ b/advice.c @@ -7,7 +7,7 @@ #include "help.h" #include "string-list.h" -static int advice_use_color = GIT_COLOR_UNKNOWN; +static enum git_colorbool advice_use_color = GIT_COLOR_UNKNOWN; static char advice_colors[][COLOR_MAXLEN] = { GIT_COLOR_RESET, GIT_COLOR_YELLOW, /* HINT */ diff --git a/builtin/branch.c b/builtin/branch.c index 029223df7b..9fcf04bebb 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -46,7 +46,7 @@ static struct object_id head_oid; static int recurse_submodules = 0; static int submodule_propagate_branches = 0; -static int branch_use_color = GIT_COLOR_UNKNOWN; +static enum git_colorbool branch_use_color = GIT_COLOR_UNKNOWN; static char branch_colors[][COLOR_MAXLEN] = { GIT_COLOR_RESET, GIT_COLOR_NORMAL, /* PLAIN */ diff --git a/builtin/clean.c b/builtin/clean.c index 8e3598d030..f10d984f60 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -64,7 +64,7 @@ static const char *color_interactive_slots[] = { [CLEAN_COLOR_RESET] = "reset", }; -static int clean_use_color = GIT_COLOR_UNKNOWN; +static enum git_colorbool clean_use_color = GIT_COLOR_UNKNOWN; static char clean_colors[][COLOR_MAXLEN] = { [CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED, [CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD, diff --git a/builtin/commit.c b/builtin/commit.c index 6c5784646a..8d40bf8619 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -936,7 +936,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, strbuf_addstr(&committer_ident, git_committer_info(IDENT_STRICT)); if (use_editor && include_status) { int ident_shown = 0; - int saved_color_setting; + enum git_colorbool saved_color_setting; struct ident_split ci, ai; const char *hint_cleanup_all = allow_empty_message ? _("Please enter the commit message for your changes." diff --git a/builtin/config.c b/builtin/config.c index c3da3ae210..9e4e4eb2f1 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -568,9 +568,9 @@ static void get_color(const struct config_location_options *opts, } struct get_colorbool_config_data { - int get_colorbool_found; - int get_diff_color_found; - int get_color_ui_found; + enum git_colorbool get_colorbool_found; + enum git_colorbool get_diff_color_found; + enum git_colorbool get_color_ui_found; const char *get_colorbool_slot; }; diff --git a/builtin/push.c b/builtin/push.c index 0962b122c7..5b6cebbb85 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -27,7 +27,7 @@ static const char * const push_usage[] = { NULL, }; -static int push_use_color = GIT_COLOR_UNKNOWN; +static enum git_colorbool push_use_color = GIT_COLOR_UNKNOWN; static char push_colors[][COLOR_MAXLEN] = { GIT_COLOR_RESET, GIT_COLOR_RED, /* ERROR */ diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 970e78bc2d..441babf2e3 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -29,7 +29,7 @@ static const char*const show_branch_usage[] = { NULL }; -static int showbranch_use_color = GIT_COLOR_UNKNOWN; +static enum git_colorbool showbranch_use_color = GIT_COLOR_UNKNOWN; static struct strvec default_args = STRVEC_INIT; diff --git a/color.c b/color.c index f3adce0141..3348ead534 100644 --- a/color.c +++ b/color.c @@ -9,7 +9,7 @@ #include "pager.h" #include "strbuf.h" -static int git_use_color_default = GIT_COLOR_AUTO; +static enum git_colorbool git_use_color_default = GIT_COLOR_AUTO; int color_stdout_is_tty = -1; /* @@ -404,7 +404,7 @@ static int check_auto_color(int fd) return 0; } -int want_color_fd(int fd, int var) +int want_color_fd(int fd, enum git_colorbool var) { /* * NEEDSWORK: This function is sometimes used from multiple threads, and diff --git a/color.h b/color.h index 303e2c9a6d..fcb38c5562 100644 --- a/color.h +++ b/color.h @@ -106,7 +106,7 @@ enum git_colorbool git_config_colorbool(const char *var, const char *value); * Return a boolean whether to use color, where the argument 'var' is * one of GIT_COLOR_UNKNOWN, GIT_COLOR_NEVER, GIT_COLOR_ALWAYS, GIT_COLOR_AUTO. */ -int want_color_fd(int fd, int var); +int want_color_fd(int fd, enum git_colorbool var); #define want_color(colorbool) want_color_fd(1, (colorbool)) #define want_color_stderr(colorbool) want_color_fd(2, (colorbool)) diff --git a/combine-diff.c b/combine-diff.c index 4ea2dc93c4..9b4deeebeb 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -749,7 +749,7 @@ static void show_line_to_eol(const char *line, int len, const char *reset) static void dump_sline(struct sline *sline, const char *line_prefix, unsigned long cnt, int num_parent, - int use_color, int result_deleted) + enum git_colorbool use_color, int result_deleted) { unsigned long mark = (1UL<use_color, ix) diff --git a/grep.h b/grep.h index 43195baab3..13e26a9318 100644 --- a/grep.h +++ b/grep.h @@ -159,7 +159,7 @@ struct grep_opt { int pathname; int null_following_name; int only_matching; - int color; + enum git_colorbool color; int max_depth; int funcname; int funcbody; diff --git a/log-tree.c b/log-tree.c index 233bf9f227..a2cd5c587b 100644 --- a/log-tree.c +++ b/log-tree.c @@ -57,7 +57,7 @@ static const char *color_decorate_slots[] = { [DECORATION_GRAFTED] = "grafted", }; -static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix) +static const char *decorate_get_color(enum git_colorbool decorate_use_color, enum decoration_type ix) { if (want_color(decorate_use_color)) return decoration_colors[ix]; @@ -341,7 +341,7 @@ static void show_name(struct strbuf *sb, const struct name_decoration *decoratio */ void format_decorations(struct strbuf *sb, const struct commit *commit, - int use_color, + enum git_colorbool use_color, const struct decoration_options *opts) { const struct name_decoration *decoration; diff --git a/log-tree.h b/log-tree.h index ebe491c543..07924be8bc 100644 --- a/log-tree.h +++ b/log-tree.h @@ -1,6 +1,8 @@ #ifndef LOG_TREE_H #define LOG_TREE_H +#include "color.h" + struct rev_info; struct log_info { @@ -26,7 +28,7 @@ int log_tree_diff_flush(struct rev_info *); int log_tree_commit(struct rev_info *, struct commit *); void show_log(struct rev_info *opt); void format_decorations(struct strbuf *sb, const struct commit *commit, - int use_color, const struct decoration_options *opts); + enum git_colorbool use_color, const struct decoration_options *opts); void show_decorations(struct rev_info *opt, struct commit *commit); void log_write_email_headers(struct rev_info *opt, struct commit *commit, char **extra_headers_p, diff --git a/parse-options-cb.c b/parse-options-cb.c index e13e0a9e33..976cc86385 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -50,7 +50,7 @@ int parse_opt_expiry_date_cb(const struct option *opt, const char *arg, int parse_opt_color_flag_cb(const struct option *opt, const char *arg, int unset) { - int value; + enum git_colorbool value; if (!arg) arg = unset ? "never" : (const char *)opt->defval; diff --git a/pretty.c b/pretty.c index 86d69bf877..e0646bbc5d 100644 --- a/pretty.c +++ b/pretty.c @@ -470,7 +470,7 @@ static inline void strbuf_add_with_color(struct strbuf *sb, const char *color, static void append_line_with_color(struct strbuf *sb, struct grep_opt *opt, const char *line, size_t linelen, - int color, enum grep_context ctx, + enum git_colorbool color, enum grep_context ctx, enum grep_header_field field) { const char *buf, *eol, *line_color, *match_color; @@ -899,7 +899,7 @@ struct format_commit_context { const char *message; char *commit_encoding; size_t width, indent1, indent2; - int auto_color; + enum git_colorbool auto_color; int padding; /* These offsets are relative to the start of the commit message. */ @@ -2167,7 +2167,7 @@ static int pp_utf8_width(const char *start, const char *end) } static void strbuf_add_tabexpand(struct strbuf *sb, struct grep_opt *opt, - int color, int tabwidth, const char *line, + enum git_colorbool color, int tabwidth, const char *line, int linelen) { const char *tab; diff --git a/pretty.h b/pretty.h index df267afe4a..fac699033e 100644 --- a/pretty.h +++ b/pretty.h @@ -3,6 +3,7 @@ #include "date.h" #include "string-list.h" +#include "color.h" struct commit; struct repository; @@ -46,7 +47,7 @@ struct pretty_print_context { struct rev_info *rev; const char *output_encoding; struct string_list *mailmap; - int color; + enum git_colorbool color; struct ident_split *from_ident; unsigned encode_email_headers:1; struct pretty_print_describe_status *describe_status; diff --git a/ref-filter.h b/ref-filter.h index 644f5c567c..81f2c229a9 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -95,7 +95,7 @@ struct ref_format { const char *format; const char *rest; int quote_style; - int use_color; + enum git_colorbool use_color; /* Internal state to ref-filter */ int need_color_reset_at_eol; diff --git a/sideband.c b/sideband.c index 3ac87148b9..ea7c25211e 100644 --- a/sideband.c +++ b/sideband.c @@ -27,9 +27,9 @@ static struct keyword_entry keywords[] = { }; /* Returns a color setting (GIT_COLOR_NEVER, etc). */ -static int use_sideband_colors(void) +static enum git_colorbool use_sideband_colors(void) { - static int use_sideband_colors_cached = GIT_COLOR_UNKNOWN; + static enum git_colorbool use_sideband_colors_cached = GIT_COLOR_UNKNOWN; const char *key = "color.remote"; struct strbuf sb = STRBUF_INIT; diff --git a/transport.c b/transport.c index 4f54ef1b12..961f26a9a6 100644 --- a/transport.c +++ b/transport.c @@ -30,7 +30,7 @@ #include "color.h" #include "bundle-uri.h" -static int transport_use_color = GIT_COLOR_UNKNOWN; +static enum git_colorbool transport_use_color = GIT_COLOR_UNKNOWN; static char transport_colors[][COLOR_MAXLEN] = { GIT_COLOR_RESET, GIT_COLOR_RED /* REJECTED */ diff --git a/wt-status.h b/wt-status.h index 4e377ce62b..e40a27214a 100644 --- a/wt-status.h +++ b/wt-status.h @@ -111,7 +111,7 @@ struct wt_status { int amend; enum commit_whence whence; int nowarn; - int use_color; + enum git_colorbool use_color; int no_gettext; int display_comment_prefix; int relative_paths; From b978f7803400b9f0c54f5874b085064c44a6c372 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 16:25:26 -0400 Subject: [PATCH 10/12] color: return bool from want_color() The point of want_color() is to take in a git_colorbool enum value and collapse it down to a single true/false boolean, letting UNKNOWN fall back to the color.ui default and checking isatty() for AUTO. Let's make that more clear in the type system by returning a bool rather than an integer. This sadly still does not help us much with compiler warnings for using the two types interchangeably. But it helps make the intent more clear to a human reader. We still retain the idempotency of want_color(), because in C a bool true/false converts to 1/0 when converted to an integer, which corresponds to GIT_COLOR_ALWAYS and GIT_COLOR_NEVER. So you can store the bool in a git_colorbool and get the right result (something a few pieces of code still do, but which we'll clean up in further patches). Note that we rely on this same bool/int conversion for check_auto_color(). We cache its results in a tristate int with "-1" as "not yet set", but we can assign to it (and return it) with implicit conversions to/from bool. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- color.c | 8 ++++---- color.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/color.c b/color.c index 3348ead534..07ac8c9d40 100644 --- a/color.c +++ b/color.c @@ -391,7 +391,7 @@ enum git_colorbool git_config_colorbool(const char *var, const char *value) return GIT_COLOR_AUTO; } -static int check_auto_color(int fd) +static bool check_auto_color(int fd) { static int color_stderr_is_tty = -1; int *is_tty_p = fd == 1 ? &color_stdout_is_tty : &color_stderr_is_tty; @@ -399,12 +399,12 @@ static int check_auto_color(int fd) *is_tty_p = isatty(fd); if (*is_tty_p || (fd == 1 && pager_in_use() && pager_use_color)) { if (!is_terminal_dumb()) - return 1; + return true; } - return 0; + return false; } -int want_color_fd(int fd, enum git_colorbool var) +bool want_color_fd(int fd, enum git_colorbool var) { /* * NEEDSWORK: This function is sometimes used from multiple threads, and diff --git a/color.h b/color.h index fcb38c5562..43e6c9ad09 100644 --- a/color.h +++ b/color.h @@ -106,7 +106,7 @@ enum git_colorbool git_config_colorbool(const char *var, const char *value); * Return a boolean whether to use color, where the argument 'var' is * one of GIT_COLOR_UNKNOWN, GIT_COLOR_NEVER, GIT_COLOR_ALWAYS, GIT_COLOR_AUTO. */ -int want_color_fd(int fd, enum git_colorbool var); +bool want_color_fd(int fd, enum git_colorbool var); #define want_color(colorbool) want_color_fd(1, (colorbool)) #define want_color_stderr(colorbool) want_color_fd(2, (colorbool)) From 9d241b01132c17a44adda2d762b37adf3625bdd7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 16:26:24 -0400 Subject: [PATCH 11/12] add-interactive: retain colorbool values longer Most of the diff code stores the decision about whether to show color as a git_colorbool, and evaluates it at point-of-use with want_color(). This timing is important for reasons explained in daa0c3d971 (color: delay auto-color decision until point of use, 2011-08-17). The add-interactive code instead converts immediately to strict boolean values using want_color(), and then evaluates those. This isn't wrong. Even though we pass the bool values to diff_use_color(), which expects a colorbool, the values are compatible. But it is unlike the rest of the color code, and is questionable from a type-system perspective (but C's typing between enums, ints, and bools is weak enough that the compiler does not complain). Let's switch it to the more usual way of calling want_color() at the point of use. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- add-interactive.c | 14 +++++++------- add-interactive.h | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 000315971e..6ffe64c38d 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -20,14 +20,14 @@ #include "prompt.h" #include "tree.h" -static void init_color(struct repository *r, int use_color, +static void init_color(struct repository *r, enum git_colorbool use_color, const char *section_and_slot, char *dst, const char *default_color) { char *key = xstrfmt("color.%s", section_and_slot); const char *value; - if (!use_color) + if (!want_color(use_color)) dst[0] = '\0'; else if (repo_config_get_value(r, key, &value) || color_parse(value, dst)) @@ -36,7 +36,7 @@ static void init_color(struct repository *r, int use_color, free(key); } -static int check_color_config(struct repository *r, const char *var) +static enum git_colorbool check_color_config(struct repository *r, const char *var) { const char *value; enum git_colorbool ret; @@ -55,7 +55,7 @@ static int check_color_config(struct repository *r, const char *var) !repo_config_get_value(r, "color.ui", &value)) ret = git_config_colorbool("color.ui", value); - return want_color(ret); + return ret; } void init_add_i_state(struct add_i_state *s, struct repository *r, @@ -76,7 +76,7 @@ void init_add_i_state(struct add_i_state *s, struct repository *r, init_color(r, s->use_color_interactive, "interactive.error", s->error_color, GIT_COLOR_BOLD_RED); strlcpy(s->reset_color_interactive, - s->use_color_interactive ? GIT_COLOR_RESET : "", COLOR_MAXLEN); + want_color(s->use_color_interactive) ? GIT_COLOR_RESET : "", COLOR_MAXLEN); s->use_color_diff = check_color_config(r, "color.diff"); @@ -93,7 +93,7 @@ void init_add_i_state(struct add_i_state *s, struct repository *r, init_color(r, s->use_color_diff, "diff.new", s->file_new_color, diff_get_color(s->use_color_diff, DIFF_FILE_NEW)); strlcpy(s->reset_color_diff, - s->use_color_diff ? GIT_COLOR_RESET : "", COLOR_MAXLEN); + want_color(s->use_color_diff) ? GIT_COLOR_RESET : "", COLOR_MAXLEN); FREE_AND_NULL(s->interactive_diff_filter); repo_config_get_string(r, "interactive.difffilter", @@ -1211,7 +1211,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps, * When color was asked for, use the prompt color for * highlighting, otherwise use square brackets. */ - if (s.use_color_interactive) { + if (want_color(s.use_color_interactive)) { data.color = s.prompt_color; data.reset = s.reset_color_interactive; } diff --git a/add-interactive.h b/add-interactive.h index ceadfa6bb6..da49502b76 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -12,8 +12,8 @@ struct add_p_opt { struct add_i_state { struct repository *r; - int use_color_interactive; - int use_color_diff; + enum git_colorbool use_color_interactive; + enum git_colorbool use_color_diff; char header_color[COLOR_MAXLEN]; char help_color[COLOR_MAXLEN]; char prompt_color[COLOR_MAXLEN]; From 69a7e8d32f37ca9cefc6b82fe848415d1d4200d9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Sep 2025 16:26:37 -0400 Subject: [PATCH 12/12] config: store want_color() result in a separate bool The "git config --get-colorbool foo.bar" command not only digs in the config to find the value of foo.bar, it evaluates the result using want_color() to check the tty-ness of stdout. But it stores the bool result of want_color() in the same git_colorbool that we found in the config. This works in practice because the git_colorbool enum is a superset of the bool values. But it is an oddity from a type system perspective. Let's instead store the result in a separate bool and use that. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/config.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 9e4e4eb2f1..2348a99dd4 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -598,6 +598,7 @@ static int get_colorbool(const struct config_location_options *opts, .get_diff_color_found = GIT_COLOR_UNKNOWN, .get_color_ui_found = GIT_COLOR_UNKNOWN, }; + bool result; config_with_options(git_get_colorbool_config, &data, &opts->source, the_repository, @@ -614,13 +615,13 @@ static int get_colorbool(const struct config_location_options *opts, /* default value if none found in config */ data.get_colorbool_found = GIT_COLOR_AUTO; - data.get_colorbool_found = want_color(data.get_colorbool_found); + result = want_color(data.get_colorbool_found); if (print) { - printf("%s\n", data.get_colorbool_found ? "true" : "false"); + printf("%s\n", result ? "true" : "false"); return 0; } else - return data.get_colorbool_found ? 0 : 1; + return result ? 0 : 1; } static void check_write(const struct git_config_source *source)