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>

diff --git a/builtin/stash.c b/builtin/stash.c
index 1977e50df27fc57b01dbc0a2d4485d0aadf8aaaf..063208b9b4834e2c57f3454a2bc07dada33a90f6 100644 (file)
--- 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;
@@ -1719,6 +1721,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 ae313e3c705e460f2f905e4cbdd25b694f0417a0..90a4ff2c102cefb86d62446685f0c8ab292683a3 100755 (executable)
--- 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