git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Patrick Steinhardt <ps@pks.im>
Date
Sep 3, 2025, 07:23 UTC
Message-ID
<aLfs7wuFpMhg8fK_@pks.im>
In-Reply-To
<20250821071918.GB1839835@coredump.intra.peff.net>
On Thu, Aug 21, 2025 at 03:19:18AM -0400, Jeff King wrote:
Show 21 quoted lines
> 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`.

Show 13 quoted lines
> @@ -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?
Show 10 quoted lines
> @@ -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.

Show 12 quoted lines
> @@ -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.

Show 12 quoted lines
> 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.
Show 9 quoted lines
> @@ -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.

Show 18 quoted lines
> @@ -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.
Show 11 quoted lines
> @@ -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.

Show 44 quoted lines
> 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
Previous: Jeff KingNext: Jeff King
Message 10 of 23 in “[BUG] Some subcommands ignore color.diff and color.ui in --patch mode”
  1. Isaac Oscar GarianoAug 20, 2025
  2. Jeff KingAug 20, 2025
  3. Isaac Oscar GarianoAug 20, 2025
  4. Jeff KingAug 21, 2025
  5. 0/4 oddities around add-interactive and colorJeff King, Aug 21, 2025
  6. 1/4 stash: pass --no-color to diff-tree child processesJeff King, Aug 21, 2025
  7. Patrick SteinhardtSep 3, 2025
  8. Jeff KingSep 8, 2025
  9. 2/4 add-interactive: respect color.diff for diff coloringJeff King, Aug 21, 2025
  10. Patrick SteinhardtSep 3, 2025
  11. Jeff KingSep 8, 2025
  12. Patrick SteinhardtSep 9, 2025
  13. 3/4 add-interactive: manually fall back color config to color.uiJeff King, Aug 21, 2025
  14. Junio C HamanoAug 21, 2025
  15. Patrick SteinhardtSep 3, 2025
  16. Jeff KingSep 8, 2025
  17. 4/4 contrib/diff-highlight: mention interactive.diffFilterJeff King, Aug 21, 2025
  18. 0/4 oddities around add-interactive and colorJeff King, Sep 8, 2025
  19. 1/4 stash: pass --no-color to diff plumbing child processesJeff King, Sep 8, 2025
  20. 2/4 add-interactive: respect color.diff for diff coloringJeff King, Sep 8, 2025
  21. 3/4 add-interactive: manually fall back color config to color.uiJeff King, Sep 8, 2025
  22. 4/4 contrib/diff-highlight: mention interactive.diffFilterJeff King, Sep 8, 2025
  23. Patrick SteinhardtSep 9, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.