From 8c78b5c8bc42728dcc8a527401955b7d1089e667 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 8 Sep 2025 12:42:36 -0400 Subject: add-interactive: respect color.diff for diff coloring The old perl git-add--interactive.perl script used the color.diff config option to decide whether to color diffs (and if not set, it fell back to the value of color.ui via git-config's --get-colorbool option). When we switched to the builtin version, this was lost: we respect only color.ui. So for example: git -c color.diff=false add -p would color the diff, even when it should not. The culprit is this line in add-interactive.c's parse_diff(): if (want_color_fd(1, -1)) That "-1" means "no config has been set", which causes it to fall back to the color.ui setting. We should instead be passing the value of color.diff. But the problem is that we never even parse that config option! Instead the builtin interactive code parses only the value of color.interactive, which is used for prompts and other messages. One could perhaps argue that this should cover interactive diff coloring, too, but historically it did not. The perl script treated color.interactive and color.diff separately. So we should grab the values for both, keeping separate fields in our add_i_state variable, rather than a single use_color field. We also load individual color slots (e.g., color.interactive.prompt), leaving them as the empty string when color is disabled. This happens via the init_color() helper in add-interactive, which checks that use_color field. Now that there are two such fields, we need to pass the appropriate one for each color. The colors are mostly easy to divide up; color.interactive.* follows color.interactive, and color.diff.* follows color.diff. But the "reset" color is tricky. It is used for both types of coloring, but the two can be configured independently. So we introduce two separate reset colors, and use each in the appropriate spot. There are two new tests. The first enables interactive prompt colors but disables color.diff. We should see a colored prompt but not a colored diff, showing that we are now respecting color.diff (and not color.interactive or color.ui). The second does the opposite. We disable color.interactive but turn on color.diff with a custom fragment color. When we split a hunk, the interactive code has to re-color the hunk header, which lets us check that we correctly loaded the color.diff.frag config based on color.diff, not color.interactive. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- add-interactive.c | 79 ++++++++++++++++++++++++++++++++----------------------- 1 file changed, 46 insertions(+), 33 deletions(-) (limited to 'add-interactive.c') diff --git a/add-interactive.c b/add-interactive.c index 3e692b47ec..877160d298 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, struct add_i_state *s, +static void init_color(struct repository *r, int 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 (!s->use_color) + if (!use_color) dst[0] = '\0'; else if (repo_config_get_value(r, key, &value) || color_parse(value, dst)) @@ -36,42 +36,54 @@ static void init_color(struct repository *r, struct add_i_state *s, free(key); } -void init_add_i_state(struct add_i_state *s, struct repository *r, - struct add_p_opt *add_p_opt) +static int check_color_config(struct repository *r, const char *var) { const char *value; + int ret; + + if (repo_config_get_value(r, var, &value)) + ret = -1; + else + ret = git_config_colorbool(var, value); + return want_color(ret); +} +void init_add_i_state(struct add_i_state *s, struct repository *r, + struct add_p_opt *add_p_opt) +{ s->r = r; s->context = -1; s->interhunkcontext = -1; - if (repo_config_get_value(r, "color.interactive", &value)) - s->use_color = -1; - else - s->use_color = - git_config_colorbool("color.interactive", value); - s->use_color = want_color(s->use_color); - - init_color(r, s, "interactive.header", s->header_color, GIT_COLOR_BOLD); - init_color(r, s, "interactive.help", s->help_color, GIT_COLOR_BOLD_RED); - init_color(r, s, "interactive.prompt", s->prompt_color, - GIT_COLOR_BOLD_BLUE); - init_color(r, s, "interactive.error", s->error_color, - GIT_COLOR_BOLD_RED); - - init_color(r, s, "diff.frag", s->fraginfo_color, - diff_get_color(s->use_color, DIFF_FRAGINFO)); - init_color(r, s, "diff.context", s->context_color, "fall back"); + s->use_color_interactive = check_color_config(r, "color.interactive"); + + init_color(r, s->use_color_interactive, "interactive.header", + s->header_color, GIT_COLOR_BOLD); + init_color(r, s->use_color_interactive, "interactive.help", + s->help_color, GIT_COLOR_BOLD_RED); + init_color(r, s->use_color_interactive, "interactive.prompt", + s->prompt_color, GIT_COLOR_BOLD_BLUE); + 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); + + s->use_color_diff = check_color_config(r, "color.diff"); + + init_color(r, s->use_color_diff, "diff.frag", s->fraginfo_color, + diff_get_color(s->use_color_diff, DIFF_FRAGINFO)); + init_color(r, s->use_color_diff, "diff.context", s->context_color, + "fall back"); if (!strcmp(s->context_color, "fall back")) - init_color(r, s, "diff.plain", s->context_color, - diff_get_color(s->use_color, DIFF_CONTEXT)); - init_color(r, s, "diff.old", s->file_old_color, - diff_get_color(s->use_color, DIFF_FILE_OLD)); - init_color(r, s, "diff.new", s->file_new_color, - diff_get_color(s->use_color, DIFF_FILE_NEW)); - - strlcpy(s->reset_color, - s->use_color ? GIT_COLOR_RESET : "", COLOR_MAXLEN); + init_color(r, s->use_color_diff, "diff.plain", + s->context_color, + diff_get_color(s->use_color_diff, DIFF_CONTEXT)); + init_color(r, s->use_color_diff, "diff.old", s->file_old_color, + diff_get_color(s->use_color_diff, DIFF_FILE_OLD)); + 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); FREE_AND_NULL(s->interactive_diff_filter); repo_config_get_string(r, "interactive.difffilter", @@ -109,7 +121,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 = -1; + s->use_color_interactive = -1; + s->use_color_diff = -1; } /* @@ -1188,9 +1201,9 @@ 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) { + if (s.use_color_interactive) { data.color = s.prompt_color; - data.reset = s.reset_color; + data.reset = s.reset_color_interactive; } print_file_item_data.color = data.color; print_file_item_data.reset = data.reset; -- cgit v1.2.3 From 776d6fbd45cfcb0a1287dc2a03c391164fbf6453 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 8 Sep 2025 12:42:39 -0400 Subject: add-interactive: manually fall back color config to color.ui Color options like color.interactive and color.diff should fall back to the value of color.ui if they aren't set. In add-interactive, we check the specific options (e.g., color.diff) via repo_config_get_value(), which does not depend on the main command having loaded any color config via the git_config() callback mechanism. But then we call want_color() on the result; if our specific config is unset then that function uses the value of git_use_color_default. That variable is typically set from color.ui by the git_color_config() callback, which is called by the main command in its own git_config() callback function. This works fine for "add -p", whose add_config() callback calls into git_color_config(). But it doesn't work for other commands like "checkout -p", which is otherwise unaware of color at all. People tend not to notice because the default is "auto", and that's what they'd set color.ui to as well. But something like: git -c color.ui=false checkout -p should disable color, and it doesn't. This regression goes back to 0527ccb1b5 (add -i: default to the built-in implementation, 2021-11-30). In the perl version we got the color config from "git config --get-colorbool", which did the full lookup for us. The obvious fix is for git-checkout to add a call to git_color_config() to its own config callback. But we'd have to do so for every command with this problem, which is error-prone. Let's see if we can fix it more centrally. It is tempting to teach want_color() to look up the value of repo_config_get_value("color.ui") itself. But I think that would have disastrous consequences. Plumbing commands, especially older ones, avoid porcelain config like "color.*" by simply not parsing it in their config callbacks. Looking up the value of color.ui under the hood would undermine that. Instead, let's do that lookup in the add-interactive setup code. We're already demand-loading other color config there, which is probably fine (even in a plumbing command like "git reset", the interactive mode is inherently porcelain-ish). That catches all commands that use the interactive code, whether they were calling git_color_config() themselves or not. Reported-by: Isaac Oscar Gariano Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- add-interactive.c | 9 +++++++++ t/t3701-add-interactive.sh | 15 +++++++++++++++ 2 files changed, 24 insertions(+) (limited to 'add-interactive.c') diff --git a/add-interactive.c b/add-interactive.c index 877160d298..4604c69140 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -45,6 +45,15 @@ static int check_color_config(struct repository *r, const char *var) ret = -1; else ret = git_config_colorbool(var, value); + + /* + * Do not rely on want_color() to fall back to color.ui for us. It uses + * 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)) + ret = git_config_colorbool("color.ui", value); + return want_color(ret); } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 6b400ad9a3..d9fe289a7a 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -1321,6 +1321,12 @@ test_expect_success 'stash accepts -U and --inter-hunk-context' ' test_grep "@@ -2,20 +2,20 @@" actual ' +test_expect_success 'set up base for -p color tests' ' + echo commit >file && + git commit -am "commit state" && + git tag patch-base +' + for cmd in add checkout commit reset restore "stash save" "stash push" do test_expect_success "$cmd rejects invalid context options" ' @@ -1337,6 +1343,15 @@ do test_must_fail git $cmd --inter-hunk-context 2 2>actual && test_grep -E ".--inter-hunk-context. requires .(--interactive/)?--patch." actual ' + + test_expect_success "$cmd falls back to color.ui" ' + git reset --hard patch-base && + echo working-tree >file && + test_write_lines y | + force_color git -c color.ui=false $cmd -p >output.raw 2>&1 && + test_decode_color output && + test_cmp output.raw output + ' done test_done -- cgit v1.2.3