[PATCH v2 1/4] stash: pass --no-color to diff plumbing child processes
- From
Jeff King <peff@peff.net>
- Date
- Sep 8, 2025, 16:42 UTC
- Message-ID
- <20250908164232.GA1323964@coredump.intra.peff.net>
- In-Reply-To
- <20250908164157.GA1323487@coredump.intra.peff.net>
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