From: Jeff King <peff@peff•net>
To: git@vger•kernel.org
Cc: Isaac Oscar Gariano <isaacoscar@live•com.au>,
Patrick Steinhardt <ps@pks•im>
Subject: [PATCH v2 1/4] stash: pass --no-color to diff plumbing child processes
Date: Mon, 8 Sep 2025 12:42:32 -0400 [thread overview]
Message-ID: <20250908164232.GA1323964@coredump.intra.peff.net> (raw)
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
next prev parent reply other threads:[~2025-09-08 16:42 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-20 11:05 [BUG] Some subcommands ignore color.diff and color.ui in --patch mode Isaac Oscar Gariano
2025-08-20 22:04 ` Jeff King
2025-08-20 23:48 ` Isaac Oscar Gariano
2025-08-21 7:00 ` Jeff King
2025-08-21 7:07 ` [PATCH 0/4] oddities around add-interactive and color Jeff King
2025-08-21 7:15 ` [PATCH 1/4] stash: pass --no-color to diff-tree child processes Jeff King
2025-09-03 7:23 ` Patrick Steinhardt
2025-09-08 16:06 ` Jeff King
2025-08-21 7:19 ` [PATCH 2/4] add-interactive: respect color.diff for diff coloring Jeff King
2025-09-03 7:23 ` Patrick Steinhardt
2025-09-08 16:16 ` Jeff King
2025-09-09 6:06 ` Patrick Steinhardt
2025-08-21 7:22 ` [PATCH 3/4] add-interactive: manually fall back color config to color.ui Jeff King
2025-08-21 15:42 ` Junio C Hamano
2025-09-03 7:23 ` Patrick Steinhardt
2025-09-08 16:17 ` Jeff King
2025-08-21 7:22 ` [PATCH 4/4] contrib/diff-highlight: mention interactive.diffFilter Jeff King
2025-09-08 16:41 ` [PATCH v2 0/4] oddities around add-interactive and color Jeff King
2025-09-08 16:42 ` Jeff King [this message]
2025-09-08 16:42 ` [PATCH v2 2/4] add-interactive: respect color.diff for diff coloring Jeff King
2025-09-08 16:42 ` [PATCH v2 3/4] add-interactive: manually fall back color config to color.ui Jeff King
2025-09-08 16:42 ` [PATCH v2 4/4] contrib/diff-highlight: mention interactive.diffFilter Jeff King
2025-09-09 6:09 ` [PATCH v2 0/4] oddities around add-interactive and color Patrick Steinhardt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250908164232.GA1323964@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=isaacoscar@live$(echo .)com.au \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox