From: Jeff King Date: Mon, 08 Sep 2025 16:41:57 GMT Subject: [PATCH v2 0/4] oddities around add-interactive and color Message-ID: <20250908164157.GA1323487@coredump.intra.peff.net> In-Reply-To: <20250821070740.GA3356411@coredump.intra.peff.net> 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 ## 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 && -+ test_grep "@@" output ++ test_grep "@@" 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