On Thu, Aug 21, 2025 at 03:15:17AM -0400, Jeff King wrote:
[snip]
> 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.
Fair. I'm also in the camp of that git-diff-tree(1) shouldn't ever
produce color unless explicitly asked. After all it's part of our
plumbing layer, so it's basically expected to be used mostly for scripts
and not for users.
> 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;
The line-wrapping is a bit funny, but I don't mind that too much.
> @@ -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;
All of these make sense. It feels a bit like whack-a-mole, and fixing
the root cause would address that. But I also understand that you shy
away from addressing it due to the high chance for regressions.
There's also a call to "diff-index" in the same file. Do we also need to
adjust that instance?
> 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
s/is/& to/
> + # 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
Patrick
