git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Tom Hale <tomnott@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: [PATCH 2/2] add--interactive.perl: specify --no-color explicitly
Date: Mon, 7 Sep 2020 04:17:39 -0400	[thread overview]
Message-ID: <20200907081739.GB1263923@coredump.intra.peff.net> (raw)
In-Reply-To: <20200907080632.GA1261825@coredump.intra.peff.net>

Our color tests of "git add -p" do something a bit different from how a
normal user would behave: we pretend there's a pager in use, so that Git
thinks it's OK to write color to a non-tty stdout.  This comes from
8539b46534 (t3701: avoid depending on the TTY prerequisite, 2019-12-06),
which allows us to avoid a lot of complicated mock-tty code.

However, those environment variables also make their way down to
sub-processes of add--interactive, including the "diff-files" we run to
generate the patches. As a result, it thinks it should output color,
too. So in t3701.50, for example, the machine-readable version of the
diff we get unexpectedly has color in it. We fail to parse it as a diff
and think there are zero hunks.

The test does still pass, though, because even with zero hunks we'll
dump the diff header (and we consider those unparseable bits to be part
of the header!), and so the output still has the expected color codes in
it. We don't notice that the command was totally broken and failed to
apply anything.

And in fact we're not really testing what we think we are about the
color, either. While add--interactive does correctly show the version we
got from running "diff-files --color", we'd also pass the test if we had
accidentally shown the machine-readable version, too, since it
(erroneously) has color codes in it.

One could argue that the test isn't very realistic; it's setting up this
"pretend there's a pager" situation to get around the tty restrictions
of the test environment. So one option would be to move back towards
using a real tty. But the behavior of add--interactive really is
user-visible here. If a user, for whatever reason, did run "git
--paginate add --patch" (perhaps because their pager is really a filter
or something), the command would totally fail to do anything useful.

Since we know that we don't want color in this output, let's just make
add--interactive more defensive, and say "--no-color" explicitly. It
doesn't hurt anything in the common case, but it fixes this odd case and
lets our test function properly again.

Note that the C builtin run_add_p() already passes --no-color, so it
doesn't need a similar fix. That will eventually replace this perl code
anyway, but the test change here will be valuable for ensuring that.

Signed-off-by: Jeff King <peff@peff.net>
---
Curiously, run_add_p() claims that --no-color is important to override
"diff.color = always", but in a quick test that doesn't seem to make any
difference. I didn't dig into whether it's possible to trigger or not
(either way, defensively using --no-color is a reasonable precaution).

 git-add--interactive.perl  | 2 +-
 t/t3701-add-interactive.sh | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index f36c0078ac..b6cdcfef61 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -714,7 +714,7 @@ sub parse_diff {
 	if (defined $patch_mode_revision) {
 		push @diff_cmd, get_diff_reference($patch_mode_revision);
 	}
-	my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
+	my @diff = run_cmd_pipe("git", @diff_cmd, qw(--no-color --), $path);
 	my @colored = ();
 	if ($diff_use_color) {
 		my @display_cmd = ("git", @diff_cmd, qw(--color --), $path);
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 67d1c36cef..1590cf6b98 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -582,6 +582,7 @@ test_expect_success 'diffs can be colorized' '
 	echo content >test &&
 	printf y >y &&
 	force_color git add -p >output 2>&1 <y &&
+	git diff-files --exit-code &&
 
 	# We do not want to depend on the exact coloring scheme
 	# git uses for diffs, so just check that we saw some kind of color.
-- 
2.28.0.889.g64d3ad7607

  parent reply	other threads:[~2020-09-07  8:17 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-07  8:06 [PATCH 0/2] some minor add--interactive fixes Jeff King
2020-09-07  8:08 ` [PATCH 1/2] add-patch: fix inverted return code of repo_read_index() Jeff King
2020-09-07  8:17 ` Jeff King [this message]
2020-09-07  8:25   ` [PATCH 2/2] add--interactive.perl: specify --no-color explicitly Jeff King
2020-09-08 16:25     ` Junio C Hamano
2020-09-08 18:51   ` Johannes Schindelin
2020-09-09  9:15     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200907081739.GB1263923@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=tomnott@gmail.com \
    --subject='Re: [PATCH 2/2] add--interactive.perl: specify --no-color explicitly' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).