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". 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@xxxxxxxxx/. 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@xxxxxxxx> --- I ran into this while writing tests for the subsequent patches. Reading that referenced thread again, Junio was in favor of reverting 4c7f1819b3 and replacing it with something that didn't kick in for plumbing (thus fixing the root issue). I argued against it somewhat there, but now I think I was foolish and agree with 2017-Junio. ;) I do think that fixing it now carries some risk of people complaining, though. So I'd rather do this immediate fix and worry about the larger problem separately. I also had another patch long ago that would have helped here: https://lore.kernel.org/git/20150810052353.GB15441@xxxxxxxxxxxxxxxxxxxxx/ The general idea is for GIT_PAGER_IN_USE to actually identify the pipe to the pager, so that sub-processes that are not going directly to the pager know to ignore it. I think I didn't pursue it because I never worked out the portability issues for Windows. builtin/stash.c | 4 +++- t/t3904-stash-patch.sh | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/builtin/stash.c b/builtin/stash.c index 1977e50df2..c55628aafc 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); @@ -1283,6 +1283,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; @@ -1345,6 +1346,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; diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh index ae313e3c70..0bddbce504 100755 --- a/t/t3904-stash-patch.sh +++ b/t/t3904-stash-patch.sh @@ -107,4 +107,14 @@ 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 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_done -- 2.51.0.356.g99d8374de0
