Re: [PATCH 2/4] add-interactive: respect color.diff for diff coloring

[Patrick Steinhardt <ps@pks.im>]

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