[2/7] t3701: avoid depending on the TTY prerequisite

Commit Message

John Passaro via GitGitGadget Dec. 6, 2019, 1:08 p.m. UTC

From: Johannes Schindelin <[e-mail]>

The TTY prerequisite is a rather heavy one: it not only requires Perl to
work, but also the IO/Pty.pm module (with native support, and it
requires pseudo terminals, too).

In particular, test cases marked with the TTY prerequisite would be
skipped in Git for Windows' SDK.

In the case of `git add -p`, we do not actually need that big a hammer,
as we do not want to test any functionality that requires a pseudo
terminal; all we want is for the interactive add command to use color,
even when being called from within the test suite.

And we found exactly such a trick earlier already: when we added a test
case to verify that the main loop of `git add -i` is colored
appropriately. Let's use that trick instead of the TTY prerequisite.

While at it, we avoid the pipes, as we do not want a SIGPIPE to break
the regression test cases (which will be much more likely when we do not
run everything through Perl because that is inherently slower).

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 t/t3701-add-interactive.sh | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

Comments

SZEDER Gábor Dec. 16, 2019, 12:18 p.m. UTC | #1

On Fri, Dec 06, 2019 at 01:08:20PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> The TTY prerequisite is a rather heavy one: it not only requires Perl to
> work, but also the IO/Pty.pm module (with native support, and it
> requires pseudo terminals, too).
> 
> In particular, test cases marked with the TTY prerequisite would be
> skipped in Git for Windows' SDK.
> 
> In the case of `git add -p`, we do not actually need that big a hammer,
> as we do not want to test any functionality that requires a pseudo
> terminal; all we want is for the interactive add command to use color,
> even when being called from within the test suite.
> 
> And we found exactly such a trick earlier already: when we added a test
> case to verify that the main loop of `git add -i` is colored
> appropriately. Let's use that trick instead of the TTY prerequisite.

It's much more interesting _what_ that trick is than when it was
found.  Is it setting TERM=vt100, or is it setting both TERM=vt100 and
GIT_PAGER_IN_USE=true?  I'm inclined to think the latter, but I'm not
sure I interpreted the comment below right.

> +# This function uses a trick to manipulate the interactive add to use color:
> +# the `want_color()` function special-cases the situation where a pager was
> +# spawned and Git now wants to output colored text: to detect that situation,
> +# the environment variable `GIT_PAGER_IN_USE` is set. However, color is

Perhaps a s/is set/has to be set/ would have helped my interpreter,
dunno.

> +# suppressed despite that environment variable if the `TERM` variable
> +# indicates a dumb terminal, so we set that variable, too.
> +
> +force_color () {
> +	env GIT_PAGER_IN_USE=true TERM=vt100 "$@"
> +}

In any case, there are a couple of tests in other test scripts that
test color relying on the TTY prereq.  So maybe it would be worth to
make this into a "global" helper function by adding it to
'test-lib-functions.sh', so we can drop a few more prereqs.

OTOH, some of those other tests have descriptions like:

  t3203-branch-output.sh:test_expect_success TTY '%(color) present with tty'
  t7004-tag.sh:test_expect_success TTY '%(color) present with tty'

i.e. their description is specific about checking the behaviour with a
tty, so I'm not entirely sure.

Jeff King Dec. 17, 2019, 5:53 a.m. UTC | #2

On Mon, Dec 16, 2019 at 01:18:59PM +0100, SZEDER Gábor wrote:

> > And we found exactly such a trick earlier already: when we added a test
> > case to verify that the main loop of `git add -i` is colored
> > appropriately. Let's use that trick instead of the TTY prerequisite.
> 
> It's much more interesting _what_ that trick is than when it was
> found.  Is it setting TERM=vt100, or is it setting both TERM=vt100 and
> GIT_PAGER_IN_USE=true?  I'm inclined to think the latter, but I'm not
> sure I interpreted the comment below right.

It's both. GIT_PAGER_IN_USE tells Git not to bother checking isatty(),
and then TERM=vt100 is necessary to override test-lib's TERM=dumb
specifically for the color code.

> In any case, there are a couple of tests in other test scripts that
> test color relying on the TTY prereq.  So maybe it would be worth to
> make this into a "global" helper function by adding it to
> 'test-lib-functions.sh', so we can drop a few more prereqs.
> 
> OTOH, some of those other tests have descriptions like:
> 
>   t3203-branch-output.sh:test_expect_success TTY '%(color) present with tty'
>   t7004-tag.sh:test_expect_success TTY '%(color) present with tty'
> 
> i.e. their description is specific about checking the behaviour with a
> tty, so I'm not entirely sure.

Hmm. I have mixed feelings on this. I do like the simplicity of avoiding
test_terminal (which is unportable and has also contributed to some
confusing behavior in the past[1]). But it also takes us further away from
a real-world setup.

That might be OK for the tests you quoted above, if we're OK with
assuming the equivalence of isatty() and GIT_PAGER_IN_USE for the color
code (though we'd probably want to make sure that's tested _somewhere_).

But it obviously would not work for test-terminal callers that are
checking the pager behavior. And I suspect it may create other oddities;
e.g., a script which calls a sub-command that looks at GIT_PAGER_IN_USE,
even though the sub-command's output is going to a pipe. Though one
could argue that's a bug[2] (that could be triggered by _actually_
sending the script's output to pager).

If we're going to get rid of test-terminal.pl (and I would be happy
enough to see it go), I'd rather we mock things up at the isatty()
level, something like what I showed in:

  https://lore.kernel.org/git/[e-mail]/

That gives us a more realistic setup, and we could reliably use it
everywhere that test-terminal is used.

-Peff

[1] I had issues a while back with test-terminal's stdin feature being
    racy:

      https://lore.kernel.org/git/[e-mail]/

[2] Long ago I had a patch to make PAGER_IN_USE more careful by making
    sure that our pipe is the same as the pager pipe. It did (and does)
    work, but it would need some portability adjustments. I never
    bothered to follow up because it really does seem to be a pretty
    unlikely setup in practice. But if you're curious:

      https://lore.kernel.org/git/[e-mail]/

-Peff

Patch

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 4da99e27af..793ce28297 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -23,6 +23,17 @@  diff_cmp () {
 	test_cmp "$1.filtered" "$2.filtered"
 }
 
+# This function uses a trick to manipulate the interactive add to use color:
+# the `want_color()` function special-cases the situation where a pager was
+# spawned and Git now wants to output colored text: to detect that situation,
+# the environment variable `GIT_PAGER_IN_USE` is set. However, color is
+# suppressed despite that environment variable if the `TERM` variable
+# indicates a dumb terminal, so we set that variable, too.
+
+force_color () {
+	env GIT_PAGER_IN_USE=true TERM=vt100 "$@"
+}
+
 test_expect_success 'setup (initial)' '
 	echo content >file &&
 	git add file &&
@@ -451,35 +462,38 @@  test_expect_success 'patch mode ignores unmerged entries' '
 	diff_cmp expected diff
 '
 
-test_expect_success TTY 'diffs can be colorized' '
+test_expect_success 'diffs can be colorized' '
 	git reset --hard &&
 
 	echo content >test &&
-	printf y | test_terminal git add -p >output 2>&1 &&
+	printf y >y &&
+	force_color git add -p >output 2>&1 <y &&
 
 	# 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.
 	grep "$(printf "\\033")" output
 '
 
-test_expect_success TTY 'diffFilter filters diff' '
+test_expect_success 'diffFilter filters diff' '
 	git reset --hard &&
 
 	echo content >test &&
 	test_config interactive.diffFilter "sed s/^/foo:/" &&
-	printf y | test_terminal git add -p >output 2>&1 &&
+	printf y >y &&
+	force_color git add -p >output 2>&1 <y &&
 
 	# avoid depending on the exact coloring or content of the prompts,
 	# and just make sure we saw our diff prefixed
 	grep foo:.*content output
 '
 
-test_expect_success TTY 'detect bogus diffFilter output' '
+test_expect_success 'detect bogus diffFilter output' '
 	git reset --hard &&
 
 	echo content >test &&
 	test_config interactive.diffFilter "echo too-short" &&
-	printf y | test_must_fail test_terminal git add -p
+	printf y >y &&
+	test_must_fail force_color git add -p <y
 '
 
 test_expect_success 'patch-mode via -i prompts for files' '
@@ -689,7 +703,7 @@  test_expect_success 'show help from add--helper' '
 	<BOLD;BLUE>What now<RESET>>$SP
 	Bye.
 	EOF
-	test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored &&
+	test_write_lines h | force_color git add -i >actual.colored &&
 	test_decode_color <actual.colored >actual &&
 	test_i18ncmp expect actual
 '