[BUG] Some subcommands ignore color.diff and color.ui in --patch mode

[BUG] Some subcommands ignore color.diff and color.ui in --patch mode

[Isaac Oscar Gariano]

Bassically the colouring behaviour of the interactive --patch option to the various commands differ.
I'll call "commit, add, and stash the "good commands" (as they behave as I expect), and stash push, stash save, checkout, reset, and restore the "bad commands" (which are bugged).

I assume you have git v2.50.1, a dirty working tree, and no colour related settings in any of the config files, and where $CMD is the name of any "bad command".

The following all print in colour (I expect no colour):
    git -c color.diff=never        $CMD --patch .
    git -c color.ui=never          $CMD --patch .

The folowing do not print anything in colour (I expect it to work the same as without the cat):
    git -c color.diff=always        $CMD --patch . | cat
    git -c color.ui=always          $CMD --patch . | cat

Now the documenation for color.interactive says:
    When set to always, always use colors for interactive prompts and displays (such as those used by "git-add --interactive" and
    "git-clean --interactive"). When false (or never), never. When set to true or auto, use colors only when the output is to the
    terminal. If unset, then the value of color.ui is used (auto by default).

Now the bad commands are respecting the setting of color.interactive corroectly (and the same as the good commands).
For example, this always prints a coloured prompt (but not a coloured diff)
    git -c color.interactive=always          $CMD --patch . | cat

But as mentioned above, "color.ui=always" will NOT print the prompt in color.

As for why I care, I was trying to pipe git restore through diff-highlight (this functionality should really be inbuilt into git diff)

A related issue, that is probably not a 'bug': all the --patch options ignore the diff config options (e.g. diff.wordRegex).

— Isaac Oscar Gariano​

Re: [BUG] Some subcommands ignore color.diff and color.ui in --patch mode

[Jeff King]

On Wed, Aug 20, 2025 at 11:05:53AM +0000, Isaac Oscar Gariano wrote:

> Bassically the colouring behaviour of the interactive --patch option
> to the various commands differ.
> I'll call "commit, add, and stash the "good commands" (as they behave
> as I expect), and stash push, stash save, checkout, reset, and restore
> the "bad commands" (which are bugged).

I think this is a regression in the conversion of the interactive-patch
code from a perl script to a C builtin. Bisecting points to 0527ccb1b5
(add -i: default to the built-in implementation, 2021-11-30).

Without digging too deeply, I'd guess the issue is that the original
perl script loaded all config itself. But now that the code runs
in-process, it is depending on the outer command to have loaded the
color.ui setting. And indeed, the code here:

  $ git grep -A3 color.interactive add-interactive.c
  add-interactive.c:      if (repo_config_get_value(r, "color.interactive", &value))
  add-interactive.c-              s->use_color = -1;
  add-interactive.c-      else
  add-interactive.c-              s->use_color =
  add-interactive.c:                      git_config_colorbool("color.interactive", value);
  add-interactive.c-      s->use_color = want_color(s->use_color);

shows that we consult color.interactive correctly, but then depend on
want_color() to do any fallback to color.ui. And that is just looking at
a pre-set variable:

  
  int want_color_fd(int fd, int var)
  {
  [...]
          if (var < 0)
                  var = git_use_color_default;
  [...]
  }

which is expected to be set by the outer command loading the color
config via git_color_config(). And that's why it works for "git add",
but not "git checkout".  Either "checkout" (and other commands) should
learn to call git_color_config(), or the interactive code should itself
learn to handle the fallback.

I'd expect something like this:

diff --git a/add-interactive.c b/add-interactive.c
index 3e692b47ec..ad8b4907e1 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -50,6 +50,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
 	else
 		s->use_color =
 			git_config_colorbool("color.interactive", value);
+	if (s->use_color < 0 && !repo_config_get_value(r, "color.ui", &value))
+		s->use_color = git_config_colorbool("color.ui", value);
 	s->use_color = want_color(s->use_color);
 
 	init_color(r, s, "interactive.header", s->header_color, GIT_COLOR_BOLD);

to work, but it doesn't seem to. Maybe the diff code is independently
looking at git_use_color_default, and we really do need to set the
variable?

At any rate, I think there may be a simpler workaround for you...

> As for why I care, I was trying to pipe git restore through
> diff-highlight (this functionality should really be inbuilt into git
> diff)

Have you tried setting interactive.diffFilter to "diff-highlight"?
That's what it was designed for.

> A related issue, that is probably not a 'bug': all the --patch options
> ignore the diff config options (e.g. diff.wordRegex).

The interactive patch options use the diff plumbing under the hood,
because they have certain requirements from the output. For example, you
can't apply a word-diff (or a colorized one for that matter; the color
is handled specially by generating the diff twice, once with color and
once without, and assuming that the lines correspond between them).

So the interactive code has to manually interpret any diff options that
it thinks are OK and pass them along to the underlying diff command.
There are undoubtedly some that would make sense for it to handle, but
nobody has cared enough yet to teach it (I think it just learned about
diff.context in the latest release).

I'm not sure if diff.wordRegex is such a case, though. You can't apply a
word diff (and it does not have line-to-line correspondence with a
non-word diff, so you can't show one and apply the other). But there may
be spots where we'd use it for generating a normal unified diff.

-Peff

Re: [BUG] Some subcommands ignore color.diff and color.ui in --patch mode

[Isaac Oscar Gariano]

> Have you tried setting interactive.diffFilter to "diff-highlight"?
> That's what it was designed for.
Wow thanks! that worked perfectly. You really should put that in the Readme (it only tells you to set pager.<cmd>).

Re: [BUG] Some subcommands ignore color.diff and color.ui in --patch mode

[Jeff King]

On Wed, Aug 20, 2025 at 11:48:36PM +0000, Isaac Oscar Gariano wrote:

> > Have you tried setting interactive.diffFilter to "diff-highlight"?
> > That's what it was designed for.
> Wow thanks! that worked perfectly. You really should put that in the
> Readme (it only tells you to set pager.<cmd>).

Yes, I suspect that README hasn't been touched since well before the
config option was introduced. ;)

I'm preparing a few patches and will include that.

-Peff

[PATCH 0/4] oddities around add-interactive and color

[Jeff King]

On Wed, Aug 20, 2025 at 06:04:40PM -0400, Jeff King wrote:

> I'd expect something like this:
> 
> diff --git a/add-interactive.c b/add-interactive.c
> index 3e692b47ec..ad8b4907e1 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -50,6 +50,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
>  	else
>  		s->use_color =
>  			git_config_colorbool("color.interactive", value);
> +	if (s->use_color < 0 && !repo_config_get_value(r, "color.ui", &value))
> +		s->use_color = git_config_colorbool("color.ui", value);
>  	s->use_color = want_color(s->use_color);
>  
>  	init_color(r, s, "interactive.header", s->header_color, GIT_COLOR_BOLD);
> 
> to work, but it doesn't seem to. Maybe the diff code is independently
> looking at git_use_color_default, and we really do need to set the
> variable?

Ah, indeed. There's yet another bug here. And while adding a test for
that, I found a third bug. Yikes.

So here's a series which I think addresses everything I found. These
bugs have been lurking for a while, but I guess not many people tend to
set color variables to anything exotic.

  [1/4]: stash: pass --no-color to diff-tree child processes
  [2/4]: add-interactive: respect color.diff for diff coloring
  [3/4]: add-interactive: manually fall back color config to color.ui
  [4/4]: contrib/diff-highlight: mention interactive.diffFilter

 add-interactive.c             | 88 ++++++++++++++++++++++-------------
 add-interactive.h             |  7 ++-
 add-patch.c                   | 12 ++---
 builtin/stash.c               |  4 +-
 contrib/diff-highlight/README |  8 ++++
 t/t3701-add-interactive.sh    | 51 ++++++++++++++++++++
 t/t3904-stash-patch.sh        | 10 ++++
 7 files changed, 138 insertions(+), 42 deletions(-)

-Peff

[PATCH 1/4] stash: pass --no-color to diff-tree child processes

[Jeff King]

After a partial stash, we may clear out the working tree by capturing
the output of diff-tree and piping it into git-apply. So we most
definitely do not want color diff output from that diff-tree process.
And it normally would not produce any, since its stdout is not going to
a tty, and the default value of color.ui is "auto".

However, if GIT_PAGER_IN_USE is set in the environment, that overrides
the tty check, and we'll produce a colorized diff that chokes git-apply:

  $ echo y | GIT_PAGER_IN_USE=1 git stash -p
  [...]
  Saved working directory and index state WIP on main: 4f2e2bb foo
  error: No valid patches in input (allow with "--allow-empty")
  Cannot remove worktree changes

Setting this variable is a relatively silly thing to do, and not
something most users would run into. But we sometimes do it in our tests
to stimulate color. And it is a user-visible bug, so let's fix it rather
than work around it in the tests.

The root issue here is that diff-tree (and other diff plumbing) should
probably not ever produce color by default. It does so not by parsing
color.ui, but because of the baked-in "auto" default from 4c7f1819b3
(make color.ui default to 'auto', 2013-06-10). But changing that is
risky; we've had discussions back and forth on the topic over the years.
E.g.:

  https://lore.kernel.org/git/86D0A377-8AFD-460D-A90E-6327C6934DFC@gmail.com/.

So let's accept that as the status quo for now and protect ourselves by
passing --no-color to the child processes. This is the same thing we did
for add-interactive itself in 1c6ffb546b (add--interactive.perl: specify
--no-color explicitly, 2020-09-07).

Signed-off-by: Jeff King <peff@peff.net>
---
I ran into this while writing tests for the subsequent patches.

Reading that referenced thread again, Junio was in favor of reverting
4c7f1819b3 and replacing it with something that didn't kick in for
plumbing (thus fixing the root issue). I argued against it somewhat
there, but now I think I was foolish and agree with 2017-Junio. ;) I do
think that fixing it now carries some risk of people complaining,
though. So I'd rather do this immediate fix and worry about the larger
problem separately.

I also had another patch long ago that would have helped here:

  https://lore.kernel.org/git/20150810052353.GB15441@sigill.intra.peff.net/

The general idea is for GIT_PAGER_IN_USE to actually identify the pipe
to the pager, so that sub-processes that are not going directly to the
pager know to ignore it. I think I didn't pursue it because I never
worked out the portability issues for Windows.

 builtin/stash.c        |  4 +++-
 t/t3904-stash-patch.sh | 10 ++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 1977e50df2..c55628aafc 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -377,7 +377,7 @@ static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
 	 * however it should be done together with apply_cached.
 	 */
 	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "diff-tree", "--binary", NULL);
+	strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL);
 	strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex);
 
 	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
@@ -1283,6 +1283,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,
 
 	cp_diff_tree.git_cmd = 1;
 	strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "--binary",
+		     "--no-color",
 		     "-U1", "HEAD", oid_to_hex(&info->w_tree), "--", NULL);
 	if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
 		ret = -1;
@@ -1345,6 +1346,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 
 	cp_diff_tree.git_cmd = 1;
 	strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "-U1", "HEAD",
+		     "--no-color",
 		     oid_to_hex(&info->w_tree), "--", NULL);
 	if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
 		ret = -1;
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index ae313e3c70..0bddbce504 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -107,4 +107,14 @@ test_expect_success 'stash -p with split hunk' '
 	! grep "added line 2" test
 '
 
+test_expect_success 'stash -p not confused by GIT_PAGER_IN_USE' '
+	echo to-stash >test &&
+	# Set both GIT_PAGER_IN_USE and TERM. Our goal is entice any
+	# diff subprocesses into thinking that they could output
+	# color, even though their stdout is not going into a tty.
+	echo y |
+	GIT_PAGER_IN_USE=1 TERM=vt100 git stash -p &&
+	git diff --exit-code
+'
+
 test_done
-- 
2.51.0.356.g99d8374de0

Re: [PATCH 1/4] stash: pass --no-color to diff-tree child processes

[Patrick Steinhardt]

On Thu, Aug 21, 2025 at 03:15:17AM -0400, Jeff King wrote:
[snip]
> Reading that referenced thread again, Junio was in favor of reverting
> 4c7f1819b3 and replacing it with something that didn't kick in for
> plumbing (thus fixing the root issue). I argued against it somewhat
> there, but now I think I was foolish and agree with 2017-Junio. ;) I do
> think that fixing it now carries some risk of people complaining,
> though. So I'd rather do this immediate fix and worry about the larger
> problem separately.

Fair. I'm also in the camp of that git-diff-tree(1) shouldn't ever
produce color unless explicitly asked. After all it's part of our
plumbing layer, so it's basically expected to be used mostly for scripts
and not for users.

> diff --git a/builtin/stash.c b/builtin/stash.c
> index 1977e50df2..c55628aafc 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -377,7 +377,7 @@ static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
>  	 * however it should be done together with apply_cached.
>  	 */
>  	cp.git_cmd = 1;
> -	strvec_pushl(&cp.args, "diff-tree", "--binary", NULL);
> +	strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL);
>  	strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex);
>  
>  	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
> @@ -1283,6 +1283,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,
>  
>  	cp_diff_tree.git_cmd = 1;
>  	strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "--binary",
> +		     "--no-color",
>  		     "-U1", "HEAD", oid_to_hex(&info->w_tree), "--", NULL);
>  	if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
>  		ret = -1;

The line-wrapping is a bit funny, but I don't mind that too much.

> @@ -1345,6 +1346,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
>  
>  	cp_diff_tree.git_cmd = 1;
>  	strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "-U1", "HEAD",
> +		     "--no-color",
>  		     oid_to_hex(&info->w_tree), "--", NULL);
>  	if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
>  		ret = -1;

All of these make sense. It feels a bit like whack-a-mole, and fixing
the root cause would address that. But I also understand that you shy
away from addressing it due to the high chance for regressions.

There's also a call to "diff-index" in the same file. Do we also need to
adjust that instance?

> diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
> index ae313e3c70..0bddbce504 100755
> --- a/t/t3904-stash-patch.sh
> +++ b/t/t3904-stash-patch.sh
> @@ -107,4 +107,14 @@ test_expect_success 'stash -p with split hunk' '
>  	! grep "added line 2" test
>  '
>  
> +test_expect_success 'stash -p not confused by GIT_PAGER_IN_USE' '
> +	echo to-stash >test &&
> +	# Set both GIT_PAGER_IN_USE and TERM. Our goal is entice any

s/is/& to/

> +	# diff subprocesses into thinking that they could output
> +	# color, even though their stdout is not going into a tty.
> +	echo y |
> +	GIT_PAGER_IN_USE=1 TERM=vt100 git stash -p &&
> +	git diff --exit-code
> +'
> +
>  test_done

Patrick

Re: [PATCH 1/4] stash: pass --no-color to diff-tree child processes

[Jeff King]

On Wed, Sep 03, 2025 at 09:23:16AM +0200, Patrick Steinhardt wrote:

> > @@ -1345,6 +1346,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
> >  
> >  	cp_diff_tree.git_cmd = 1;
> >  	strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "-U1", "HEAD",
> > +		     "--no-color",
> >  		     oid_to_hex(&info->w_tree), "--", NULL);
> >  	if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
> >  		ret = -1;
> 
> All of these make sense. It feels a bit like whack-a-mole, and fixing
> the root cause would address that. But I also understand that you shy
> away from addressing it due to the high chance for regressions.
> 
> There's also a call to "diff-index" in the same file. Do we also need to
> adjust that instance?

I was focused on the "-p" code path, but yes, I think it fails when
we stash an index change with GIT_PAGER_IN_USE=1. I've fixed it and
added a new test in my re-roll.

-Peff

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

[Jeff King]

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 <peff@peff.net>
---
 add-interactive.c          | 79 ++++++++++++++++++++++----------------
 add-interactive.h          |  7 +++-
 add-patch.c                | 12 +++---
 t/t3701-add-interactive.sh | 36 +++++++++++++++++
 4 files changed, 93 insertions(+), 41 deletions(-)

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))
@@ -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;
diff --git a/add-interactive.h b/add-interactive.h
index 4213dcd67b..ceadfa6bb6 100644
--- a/add-interactive.h
+++ b/add-interactive.h
@@ -12,16 +12,19 @@ struct add_p_opt {
 
 struct add_i_state {
 	struct repository *r;
-	int use_color;
+	int use_color_interactive;
+	int use_color_diff;
 	char header_color[COLOR_MAXLEN];
 	char help_color[COLOR_MAXLEN];
 	char prompt_color[COLOR_MAXLEN];
 	char error_color[COLOR_MAXLEN];
-	char reset_color[COLOR_MAXLEN];
+	char reset_color_interactive[COLOR_MAXLEN];
+
 	char fraginfo_color[COLOR_MAXLEN];
 	char context_color[COLOR_MAXLEN];
 	char file_old_color[COLOR_MAXLEN];
 	char file_new_color[COLOR_MAXLEN];
+	char reset_color_diff[COLOR_MAXLEN];
 
 	int use_single_key;
 	char *interactive_diff_filter, *interactive_diff_algorithm;
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);
 }
 
@@ -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;
 
@@ -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;
@@ -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;
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
+'
+
 test_expect_success 'diffFilter filters diff' '
 	git reset --hard &&
 
-- 
2.51.0.356.g99d8374de0

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

[Patrick Steinhardt]

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

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

[Jeff King]

On Wed, Sep 03, 2025 at 09:23:27AM +0200, Patrick Steinhardt wrote:

> > +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?

My initial reaction was: yeah, we could probably fix this up in a
preparatory patch. But the problem is much deeper than the
add-interactive code. Nobody uses GIT_COLOR_UNKNOWN at all! Even
git_config_colorbool() just returns -1.

Moreover, it does not even use the ALWAYS/NEVER defines, but just 1 and
0. Making things even more complicated, we sometimes want to consider
"do we want color" as this always/never/auto/unknown set, and then
sometimes we collapse that (using the same variable!) into a single
true/false value.

So using that consistently and possibly switching to an enum is a much
bigger topic. It may be worth cleaning up, but I don't think it's worth
derailing this regression fix. In the meantime, I'd rather keep this
code matching the rest of the color code (it's not even really adding
new instances of "-1", but just shuffling them around).

> > -	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.

Yes. I would have liked to split it up more to make this hunk stand out,
but there's some chicken-and-egg dependencies.

> > +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?

Seems reasonable. Knowing that the patch is splitting the diff coloring
off of the interactive, it would be pretty hard to introduce such a bug.
But from a black box perspective, that is probably a good thing to test.

Ultimately the best test would be for every item that _could_ be colored
by each type to be individually checked in each scenario. But I didn't
want the test to depend on enumerating those very specific details of
the code.

-Peff

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

[Patrick Steinhardt]

On Mon, Sep 08, 2025 at 12:16:48PM -0400, Jeff King wrote:
> On Wed, Sep 03, 2025 at 09:23:27AM +0200, Patrick Steinhardt wrote:
> 
> > > +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?
> 
> My initial reaction was: yeah, we could probably fix this up in a
> preparatory patch. But the problem is much deeper than the
> add-interactive code. Nobody uses GIT_COLOR_UNKNOWN at all! Even
> git_config_colorbool() just returns -1.
> 
> Moreover, it does not even use the ALWAYS/NEVER defines, but just 1 and
> 0. Making things even more complicated, we sometimes want to consider
> "do we want color" as this always/never/auto/unknown set, and then
> sometimes we collapse that (using the same variable!) into a single
> true/false value.
> 
> So using that consistently and possibly switching to an enum is a much
> bigger topic. It may be worth cleaning up, but I don't think it's worth
> derailing this regression fix. In the meantime, I'd rather keep this
> code matching the rest of the color code (it's not even really adding
> new instances of "-1", but just shuffling them around).

Makes sense.

Patrick

[PATCH 3/4] add-interactive: manually fall back color config to color.ui

[Jeff King]

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 <isaacoscar@live.com.au>
Signed-off-by: Jeff King <peff@peff.net>
---
 add-interactive.c          |  9 +++++++++
 t/t3701-add-interactive.sh | 15 +++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/add-interactive.c b/add-interactive.c
index 95ab251963..db7e6a81a8 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 3f9cb9453f..0024991257 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1319,6 +1319,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" '
@@ -1335,6 +1341,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.raw >output &&
+		test_cmp output.raw output
+	'
 done
 
 test_done
-- 
2.51.0.356.g99d8374de0

Re: [PATCH 3/4] add-interactive: manually fall back color config to color.ui

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

A very good design decision I can agree with.
Nice.

Re: [PATCH 3/4] add-interactive: manually fall back color config to color.ui

[Patrick Steinhardt]

On Thu, Aug 21, 2025 at 03:22:24AM -0400, Jeff King wrote:
> 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

Is the "color." intended to refer to config keys starting with that
string? If so it would help to quote it and maybe say "color.*".

> diff --git a/add-interactive.c b/add-interactive.c
> index 95ab251963..db7e6a81a8 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);

With the previous comments where I say that we should use
`GIT_COLOR_UNKNOWN` we should probably convert the `ret < 0` to `ret ==
GIT_COLOR_UNKNOWN`, as well.

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 3f9cb9453f..0024991257 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -1335,6 +1341,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.raw >output &&
> +		test_cmp output.raw output
> +	'

Nice and straight-forward.

Patrick

Re: [PATCH 3/4] add-interactive: manually fall back color config to color.ui

[Jeff King]

On Wed, Sep 03, 2025 at 09:23:33AM +0200, Patrick Steinhardt wrote:

> > 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
> 
> Is the "color." intended to refer to config keys starting with that
> string? If so it would help to quote it and maybe say "color.*".

I'm not sure if I meant "config like color" in the general sense, or
typo'd "color.*". Either way, what is there is indeed confusing. ;) I'll
fix it.

-Peff

[PATCH 4/4] contrib/diff-highlight: mention interactive.diffFilter

[Jeff King]

When the README for diff-highlight was written, there was no way to
trigger it for the `add -p` interactive patch mode. We've since grown a
feature to support that, but it was documented only on the Git side.
Let's also let people coming the other direction, from diff-highlight,
know that it's an option.

Suggested-by: Isaac Oscar Gariano <IsaacOscar@live.com.au>
Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/README | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README
index d4c2343175..1db4440e68 100644
--- a/contrib/diff-highlight/README
+++ b/contrib/diff-highlight/README
@@ -58,6 +58,14 @@ following in your git configuration:
 	diff = diff-highlight | less
 ---------------------------------------------
 
+If you use the interactive patch mode of `git add -p`, `git checkout
+-p`, etc, you may also want to configure it to be used there:
+
+---------------------------------------------
+[interactive]
+        diffFilter = diff-highlight
+---------------------------------------------
+
 
 Color Config
 ------------
-- 
2.51.0.356.g99d8374de0

[PATCH v2 0/4] oddities around add-interactive and color

[Jeff King]

On Thu, Aug 21, 2025 at 03:07:40AM -0400, Jeff King wrote:

> So here's a series which I think addresses everything I found. These
> bugs have been lurking for a while, but I guess not many people tend to
> set color variables to anything exotic.

And here's a v2 based on Patrick's review. I also touched up a few lines
whose indentation did not pass clang-format (not new, but ones I was
touching or moving around). The only thing I punted on was refactoring
the GIT_COLOR_* defines, as I think it extends well beyond the code I'm
touching here (see the reply I left in the thread).

-Peff

  [1/4]: stash: pass --no-color to diff plumbing child processes
  [2/4]: add-interactive: respect color.diff for diff coloring
  [3/4]: add-interactive: manually fall back color config to color.ui
  [4/4]: contrib/diff-highlight: mention interactive.diffFilter

 add-interactive.c             | 88 ++++++++++++++++++++++-------------
 add-interactive.h             |  7 ++-
 add-patch.c                   | 12 ++---
 builtin/stash.c               |  5 +-
 contrib/diff-highlight/README |  8 ++++
 t/t3701-add-interactive.sh    | 53 +++++++++++++++++++++
 t/t3904-stash-patch.sh        | 19 ++++++++
 7 files changed, 150 insertions(+), 42 deletions(-)

1:  d1d3c0e7f4 ! 1:  d02117a0d6 stash: pass --no-color to diff-tree child processes
    @@ Metadata
     Author: Jeff King <peff@peff.net>
     
      ## Commit message ##
    -    stash: pass --no-color to diff-tree child processes
    +    stash: pass --no-color to diff plumbing child processes
     
         After a partial stash, we may clear out the working tree by capturing
    -    the output of diff-tree and piping it into git-apply. So we most
    -    definitely do not want color diff output from that diff-tree process.
    -    And it normally would not produce any, since its stdout is not going to
    -    a tty, and the default value of color.ui is "auto".
    +    the output of diff-tree and piping it into git-apply (and likewise we
    +    may use diff-index to restore the index). So we most definitely do not
    +    want color diff output from that diff-tree process.  And it normally
    +    would not produce any, since its stdout is not going to a tty, and the
    +    default value of color.ui is "auto".
     
         However, if GIT_PAGER_IN_USE is set in the environment, that overrides
         the tty check, and we'll produce a colorized diff that chokes git-apply:
    @@ builtin/stash.c: static int stash_patch(struct stash_info *info, const struct pa
      		     oid_to_hex(&info->w_tree), "--", NULL);
      	if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
      		ret = -1;
    +@@ builtin/stash.c: static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
    + 
    + 			cp_diff.git_cmd = 1;
    + 			strvec_pushl(&cp_diff.args, "diff-index", "-p",
    ++				     "--no-color",
    + 				     "--cached", "--binary", "HEAD", "--",
    + 				     NULL);
    + 			add_pathspecs(&cp_diff.args, ps);
     
      ## t/t3904-stash-patch.sh ##
     @@ t/t3904-stash-patch.sh: test_expect_success 'stash -p with split hunk' '
    @@ t/t3904-stash-patch.sh: test_expect_success 'stash -p with split hunk' '
      
     +test_expect_success 'stash -p not confused by GIT_PAGER_IN_USE' '
     +	echo to-stash >test &&
    -+	# Set both GIT_PAGER_IN_USE and TERM. Our goal is entice any
    ++	# Set both GIT_PAGER_IN_USE and TERM. Our goal is to entice any
     +	# diff subprocesses into thinking that they could output
     +	# color, even though their stdout is not going into a tty.
     +	echo y |
     +	GIT_PAGER_IN_USE=1 TERM=vt100 git stash -p &&
     +	git diff --exit-code
     +'
    ++
    ++test_expect_success 'index push not confused by GIT_PAGER_IN_USE' '
    ++	echo index >test &&
    ++	git add test &&
    ++	echo working-tree >test &&
    ++	# As above, we try to entice the child diff into using color.
    ++	GIT_PAGER_IN_USE=1 TERM=vt100 git stash push test &&
    ++	git diff --exit-code
    ++'
     +
      test_done
2:  5d40a0ed74 ! 2:  f2600751b9 add-interactive: respect color.diff for diff coloring
    @@ add-interactive.c: static void init_color(struct repository *r, struct add_i_sta
     +			   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));
    ++		   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));
    ++		   diff_get_color(s->use_color_diff, DIFF_FILE_NEW));
     +	strlcpy(s->reset_color_diff,
     +		s->use_color_diff ? GIT_COLOR_RESET : "", COLOR_MAXLEN);
      
    @@ t/t3701-add-interactive.sh: test_expect_success 'colorized diffs respect diff.ws
     +	test_write_lines s n n |
     +	force_color git \
     +		-c color.interactive=false \
    ++		-c color.interactive.prompt=blue \
     +		-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
    ++	test_grep "<BOLD;MAGENTA>@@" output &&
    ++	test_grep ! "BLUE" output
     +'
     +
      test_expect_success 'diffFilter filters diff' '
3:  44cb772e07 ! 3:  8979bff0c5 add-interactive: manually fall back color config to color.ui
    @@ Commit message
         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
    +    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.
     
4:  31c0a6f81e = 4:  a2b328389a contrib/diff-highlight: mention interactive.diffFilter

[PATCH v2 1/4] stash: pass --no-color to diff plumbing child processes

[Jeff King]

After a partial stash, we may clear out the working tree by capturing
the output of diff-tree and piping it into git-apply (and likewise we
may use diff-index to restore the index). So we most definitely do not
want color diff output from that diff-tree process.  And it normally
would not produce any, since its stdout is not going to a tty, and the
default value of color.ui is "auto".

However, if GIT_PAGER_IN_USE is set in the environment, that overrides
the tty check, and we'll produce a colorized diff that chokes git-apply:

  $ echo y | GIT_PAGER_IN_USE=1 git stash -p
  [...]
  Saved working directory and index state WIP on main: 4f2e2bb foo
  error: No valid patches in input (allow with "--allow-empty")
  Cannot remove worktree changes

Setting this variable is a relatively silly thing to do, and not
something most users would run into. But we sometimes do it in our tests
to stimulate color. And it is a user-visible bug, so let's fix it rather
than work around it in the tests.

The root issue here is that diff-tree (and other diff plumbing) should
probably not ever produce color by default. It does so not by parsing
color.ui, but because of the baked-in "auto" default from 4c7f1819b3
(make color.ui default to 'auto', 2013-06-10). But changing that is
risky; we've had discussions back and forth on the topic over the years.
E.g.:

  https://lore.kernel.org/git/86D0A377-8AFD-460D-A90E-6327C6934DFC@gmail.com/.

So let's accept that as the status quo for now and protect ourselves by
passing --no-color to the child processes. This is the same thing we did
for add-interactive itself in 1c6ffb546b (add--interactive.perl: specify
--no-color explicitly, 2020-09-07).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/stash.c        |  5 ++++-
 t/t3904-stash-patch.sh | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index f5ddee5c7f..67b291f3fd 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -377,7 +377,7 @@ static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
 	 * however it should be done together with apply_cached.
 	 */
 	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "diff-tree", "--binary", NULL);
+	strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL);
 	strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex);
 
 	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
@@ -1284,6 +1284,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,
 
 	cp_diff_tree.git_cmd = 1;
 	strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "--binary",
+		     "--no-color",
 		     "-U1", "HEAD", oid_to_hex(&info->w_tree), "--", NULL);
 	if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
 		ret = -1;
@@ -1346,6 +1347,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 
 	cp_diff_tree.git_cmd = 1;
 	strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "-U1", "HEAD",
+		     "--no-color",
 		     oid_to_hex(&info->w_tree), "--", NULL);
 	if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
 		ret = -1;
@@ -1720,6 +1722,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 
 			cp_diff.git_cmd = 1;
 			strvec_pushl(&cp_diff.args, "diff-index", "-p",
+				     "--no-color",
 				     "--cached", "--binary", "HEAD", "--",
 				     NULL);
 			add_pathspecs(&cp_diff.args, ps);
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index ae313e3c70..90a4ff2c10 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -107,4 +107,23 @@ test_expect_success 'stash -p with split hunk' '
 	! grep "added line 2" test
 '
 
+test_expect_success 'stash -p not confused by GIT_PAGER_IN_USE' '
+	echo to-stash >test &&
+	# Set both GIT_PAGER_IN_USE and TERM. Our goal is to entice any
+	# diff subprocesses into thinking that they could output
+	# color, even though their stdout is not going into a tty.
+	echo y |
+	GIT_PAGER_IN_USE=1 TERM=vt100 git stash -p &&
+	git diff --exit-code
+'
+
+test_expect_success 'index push not confused by GIT_PAGER_IN_USE' '
+	echo index >test &&
+	git add test &&
+	echo working-tree >test &&
+	# As above, we try to entice the child diff into using color.
+	GIT_PAGER_IN_USE=1 TERM=vt100 git stash push test &&
+	git diff --exit-code
+'
+
 test_done
-- 
2.51.0.462.g0a0e5b9b75

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

[Jeff King]

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 <peff@peff.net>
---
 add-interactive.c          | 79 ++++++++++++++++++++++----------------
 add-interactive.h          |  7 +++-
 add-patch.c                | 12 +++---
 t/t3701-add-interactive.sh | 38 ++++++++++++++++++
 4 files changed, 95 insertions(+), 41 deletions(-)

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;
diff --git a/add-interactive.h b/add-interactive.h
index 4213dcd67b..ceadfa6bb6 100644
--- a/add-interactive.h
+++ b/add-interactive.h
@@ -12,16 +12,19 @@ struct add_p_opt {
 
 struct add_i_state {
 	struct repository *r;
-	int use_color;
+	int use_color_interactive;
+	int use_color_diff;
 	char header_color[COLOR_MAXLEN];
 	char help_color[COLOR_MAXLEN];
 	char prompt_color[COLOR_MAXLEN];
 	char error_color[COLOR_MAXLEN];
-	char reset_color[COLOR_MAXLEN];
+	char reset_color_interactive[COLOR_MAXLEN];
+
 	char fraginfo_color[COLOR_MAXLEN];
 	char context_color[COLOR_MAXLEN];
 	char file_old_color[COLOR_MAXLEN];
 	char file_new_color[COLOR_MAXLEN];
+	char reset_color_diff[COLOR_MAXLEN];
 
 	int use_single_key;
 	char *interactive_diff_filter, *interactive_diff_algorithm;
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);
 }
 
@@ -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;
 
@@ -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;
@@ -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;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 04d2a19835..6b400ad9a3 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -866,6 +866,44 @@ 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.interactive.prompt=blue \
+		-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 &&
+	test_grep ! "BLUE" output
+'
+
 test_expect_success 'diffFilter filters diff' '
 	git reset --hard &&
 
-- 
2.51.0.462.g0a0e5b9b75

[PATCH v2 3/4] add-interactive: manually fall back color config to color.ui

[Jeff King]

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 <isaacoscar@live.com.au>
Signed-off-by: Jeff King <peff@peff.net>
---
 add-interactive.c          |  9 +++++++++
 t/t3701-add-interactive.sh | 15 +++++++++++++++
 2 files changed, 24 insertions(+)

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.raw >output &&
+		test_cmp output.raw output
+	'
 done
 
 test_done
-- 
2.51.0.462.g0a0e5b9b75

[PATCH v2 4/4] contrib/diff-highlight: mention interactive.diffFilter

[Jeff King]

When the README for diff-highlight was written, there was no way to
trigger it for the `add -p` interactive patch mode. We've since grown a
feature to support that, but it was documented only on the Git side.
Let's also let people coming the other direction, from diff-highlight,
know that it's an option.

Suggested-by: Isaac Oscar Gariano <IsaacOscar@live.com.au>
Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/diff-highlight/README | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README
index d4c2343175..1db4440e68 100644
--- a/contrib/diff-highlight/README
+++ b/contrib/diff-highlight/README
@@ -58,6 +58,14 @@ following in your git configuration:
 	diff = diff-highlight | less
 ---------------------------------------------
 
+If you use the interactive patch mode of `git add -p`, `git checkout
+-p`, etc, you may also want to configure it to be used there:
+
+---------------------------------------------
+[interactive]
+        diffFilter = diff-highlight
+---------------------------------------------
+
 
 Color Config
 ------------
-- 
2.51.0.462.g0a0e5b9b75

Re: [PATCH v2 0/4] oddities around add-interactive and color

[Patrick Steinhardt]

On Mon, Sep 08, 2025 at 12:41:57PM -0400, Jeff King wrote:
> On Thu, Aug 21, 2025 at 03:07:40AM -0400, Jeff King wrote:
> 
> > So here's a series which I think addresses everything I found. These
> > bugs have been lurking for a while, but I guess not many people tend to
> > set color variables to anything exotic.
> 
> And here's a v2 based on Patrick's review. I also touched up a few lines
> whose indentation did not pass clang-format (not new, but ones I was
> touching or moving around). The only thing I punted on was refactoring
> the GIT_COLOR_* defines, as I think it extends well beyond the code I'm
> touching here (see the reply I left in the thread).

Thanks, this addresses all of my feedback from v1. Well, except the
GIT_COLOR_* defines, but I agree that it doesn't make sense to do that
as part of this series.

Patrick