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

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

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