Re: [PATCH 1/4] stash: pass --no-color to diff-tree child processes

[Patrick Steinhardt <ps@pks.im>]

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