freshlybakedca.ke / git
Git fork
fork atom

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 (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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

authored by

Jeff King and committed by
Junio C Hamano 89b4183e c44beea4

+23 -1
unified split
+4 -1
builtin/stash.c
··· 377 377 * however it should be done together with apply_cached. 378 378 */ 379 379 cp.git_cmd = 1; 380 - strvec_pushl(&cp.args, "diff-tree", "--binary", NULL); 380 + strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL); 381 381 strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex); 382 382 383 383 return pipe_command(&cp, NULL, 0, out, 0, NULL, 0); ··· 1283 1283 1284 1284 cp_diff_tree.git_cmd = 1; 1285 1285 strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "--binary", 1286 + "--no-color", 1286 1287 "-U1", "HEAD", oid_to_hex(&info->w_tree), "--", NULL); 1287 1288 if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) { 1288 1289 ret = -1; ··· 1345 1346 1346 1347 cp_diff_tree.git_cmd = 1; 1347 1348 strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "-U1", "HEAD", 1349 + "--no-color", 1348 1350 oid_to_hex(&info->w_tree), "--", NULL); 1349 1351 if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) { 1350 1352 ret = -1; ··· 1719 1721 1720 1722 cp_diff.git_cmd = 1; 1721 1723 strvec_pushl(&cp_diff.args, "diff-index", "-p", 1724 + "--no-color", 1722 1725 "--cached", "--binary", "HEAD", "--", 1723 1726 NULL); 1724 1727 add_pathspecs(&cp_diff.args, ps);
+19
t/t3904-stash-patch.sh
··· 107 107 ! grep "added line 2" test 108 108 ' 109 109 110 + test_expect_success 'stash -p not confused by GIT_PAGER_IN_USE' ' 111 + echo to-stash >test && 112 + # Set both GIT_PAGER_IN_USE and TERM. Our goal is to entice any 113 + # diff subprocesses into thinking that they could output 114 + # color, even though their stdout is not going into a tty. 115 + echo y | 116 + GIT_PAGER_IN_USE=1 TERM=vt100 git stash -p && 117 + git diff --exit-code 118 + ' 119 + 120 + test_expect_success 'index push not confused by GIT_PAGER_IN_USE' ' 121 + echo index >test && 122 + git add test && 123 + echo working-tree >test && 124 + # As above, we try to entice the child diff into using color. 125 + GIT_PAGER_IN_USE=1 TERM=vt100 git stash push test && 126 + git diff --exit-code 127 + ' 128 + 110 129 test_done