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

From: Jeff King <peff@peff.net>
Date: Aug 20, 2025, 22:04 UTC
Message-ID: <20250820220439.GA1668511@coredump.intra.peff.net>
In-Reply-To: <SYBP282MB296329544B33E3C16DD99FD28C33A@SYBP282MB2963.AUSP282.PROD.OUTLOOK.COM>

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

Show 5 quoted lines

> 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