Re: [PATCH 1/4] stash: pass --no-color to diff-tree child processes
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 3, 2025, 07:23 UTC
- Message-ID
- <aLfs5EDk-krJHnmQ@pks.im>
- In-Reply-To
- <20250821071517.GA1839835@coredump.intra.peff.net>
On Thu, Aug 21, 2025 at 03:15:17AM -0400, Jeff King wrote: [snip]
Show 7 quoted lines
> 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.
Show 21 quoted lines
> 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.
Show 8 quoted lines
> @@ -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?
Show 11 quoted lines
> 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/
Show 8 quoted lines
> + # 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