On Thu, Aug 21, 2025 at 03:19:18AM -0400, Jeff King wrote:
> diff --git a/add-interactive.c b/add-interactive.c
> index 3e692b47ec..95ab251963 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))
Okay. This needs to change so that we can pass in either
`use_color_diff` or `use_color_interactive`.
> @@ -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;
Not an old issue, but should we use `GIT_COLOR_UNKNOWN` here?
> @@ -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;
> }
>
> /*
Same here, should we use `GIT_COLOR_UNKNOWN` to initialize these fields?
It would be even better if the `GIT_COLOR` values were a proper enum so
that we can use the type in both `want_color_fd()` and for these struct
members.
> @@ -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;
Makes sense. We don't want to show the diff here, but render the prompt,
which should of course honor the interactive colors.
> diff --git a/add-patch.c b/add-patch.c
> index 302e6ba7d9..b0389c5d5b 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -300,7 +300,7 @@ static void err(struct add_p_state *s, const char *fmt, ...)
> va_start(args, fmt);
> fputs(s->s.error_color, stdout);
> vprintf(fmt, args);
> - puts(s->s.reset_color);
> + puts(s->s.reset_color_interactive);
> va_end(args);
> }
Yup, printing an error message should respect interactive colors.
> @@ -457,7 +457,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
> }
> strbuf_complete_line(plain);
>
> - if (want_color_fd(1, -1)) {
> + if (want_color_fd(1, s->s.use_color_diff)) {
> struct child_process colored_cp = CHILD_PROCESS_INIT;
> const char *diff_filter = s->s.interactive_diff_filter;
>
We're printing the diff here, and this change is the whole point of this
commit as far as I understand as we now properly respect configured diff
colors.
> @@ -714,7 +714,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
> if (len)
> strbuf_add(out, p, len);
> else if (colored)
> - strbuf_addf(out, "%s\n", s->s.reset_color);
> + strbuf_addf(out, "%s\n", s->s.reset_color_diff);
> else
> strbuf_addch(out, '\n');
> }
> @@ -1107,7 +1107,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)
> s->s.file_new_color :
> s->s.context_color);
> strbuf_add(&s->colored, plain + current, eol - current);
> - strbuf_addstr(&s->colored, s->s.reset_color);
> + strbuf_addstr(&s->colored, s->s.reset_color_diff);
> if (next > eol)
> strbuf_add(&s->colored, plain + eol, next - eol);
> current = next;
Both of these print diff hunks, which should use diff colors.
> @@ -1528,8 +1528,8 @@ static int patch_update_file(struct add_p_state *s,
> : 1));
> printf(_(s->mode->prompt_mode[prompt_mode_type]),
> s->buf.buf);
> - if (*s->s.reset_color)
> - fputs(s->s.reset_color, stdout);
> + if (*s->s.reset_color_interactive)
> + fputs(s->s.reset_color_interactive, stdout);
> fflush(stdout);
> if (read_single_character(s) == EOF)
> break;
And here we reset colors after the prompt. So all of these conversions
look good.
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 04d2a19835..3f9cb9453f 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -866,6 +866,42 @@ test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
> test_grep "old<" output
> '
>
> +test_expect_success 'diff color respects color.diff' '
> + git reset --hard &&
> +
> + echo old >test &&
> + git add test &&
> + echo new >test &&
> +
> + printf n >n &&
> + force_color git \
> + -c color.interactive=auto \
> + -c color.interactive.prompt=blue \
> + -c color.diff=false \
> + -c color.diff.old=red \
> + add -p >output.raw 2>&1 <n &&
> + test_decode_color <output.raw >output &&
> + test_grep "BLUE.*Stage this hunk" output &&
> + test_grep ! "RED" output
> +'
> +
> +test_expect_success 're-coloring diff without color.interactive' '
> + git reset --hard &&
> +
> + test_write_lines 1 2 3 >test &&
> + git add test &&
> + test_write_lines one 2 three >test &&
> +
> + test_write_lines s n n |
> + force_color git \
> + -c color.interactive=false \
> + -c color.diff=true \
> + -c color.diff.frag="bold magenta" \
> + add -p >output.raw 2>&1 &&
> + test_decode_color <output.raw >output &&
> + test_grep "<BOLD;MAGENTA>@@" output
> +'
> +
Should we also verify that the interactive prompts aren't colored here?
Patrick
