[PATCH v2 0/7] Re: 'git svn log' no longer uses the pager

[PATCH v2 0/7] Re: 'git svn log' no longer uses the pager

[Jonathan Nieder]

Here is a replacement for the jn/maint-fix-pager series currently in
pu.  As for the previous version, the point is to fix the breakage
from dec543 (am -i, git-svn: use "git var GIT_PAGER", 2009-10-30) 
reported at http://thread.gmane.org/gmane.comp.version-control.git/139831
I also added some new tests to make sure pagination works in other
ways.

In this version, patches 1-5 are the same fixes as before, with
commit messages tweaked to take into account feedback from the
previous round.  You can see what they fix by using 'git svn log'
and the '(v)iew' option of 'git am --interactive', which both
stopped paginating output in commit dec543.  You can see what they
don’t break by running some other command such as 'git log' in
circumstances where its output is not supposed to be paginated.

The test in patch 6 has some fixes that didn’t make it to pu last
time.  To avoid risk of spewing useless output to /dev/tty, the
tests requiring a terminal only run with the --verbose option.

Patch 7 is the one I am most interested in feedback about.  It allows
automatic runs of tests without a real terminal by creating its own
pty as needed.  But it might be a portability nightmare: is
posix_openpt widely available?  Will open("/dev/ptmx", ...) do just
as well most places?  And how important is it that these tests run
on the most obscure platforms?

Because these questions are still up in the air for me, I do not
think patch 7 is suitable for use outside pu yet.  What I would most
like to hear is that /dev/ptmx works often enough, because that is
a condition that can be tested for at runtime without adding
complication to the Makefile.  But if that is not the case, there
are other options.

Jonathan Nieder (7):
  Fix 'git var' usage synopsis
  Make 'git var GIT_PAGER' always print the configured pager
  git.1: Clarify the behavior of the --paginate option
  git svn: Fix launching of pager
  am: Fix launching of pager
  tests: Add tests for automatic use of pager
  t7006-pager: if stdout is not a terminal, make a new one

 .gitignore                |    1 +
 Documentation/git-var.txt |    2 +-
 Documentation/git.txt     |    8 ++-
 Makefile                  |    6 ++
 builtin-var.c             |    4 +-
 cache.h                   |    2 +-
 git-am.sh                 |    5 +-
 git-sh-setup.sh           |   13 ++++
 git-svn.perl              |    9 ++-
 pager.c                   |    6 +-
 t/t7006-pager.sh          |  171 +++++++++++++++++++++++++++++++++++++++++++++
 test-terminal.c           |   62 ++++++++++++++++
 12 files changed, 274 insertions(+), 15 deletions(-)
 create mode 100644 t/t7006-pager.sh
 create mode 100644 test-terminal.c

[PATCH 1/7] Fix 'git var' usage synopsis

[Jonathan Nieder]

The parameter to 'git var' is not optional.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Same as v1, probably should be ejected from the series.  I am
including it for reference anyway.

 Documentation/git-var.txt |    2 +-
 builtin-var.c             |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index bb98182..458f3e2 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -8,7 +8,7 @@ git-var - Show a git logical variable
 
 SYNOPSIS
 --------
-'git var' [ -l | <variable> ]
+'git var' ( -l | <variable> )
 
 DESCRIPTION
 -----------
diff --git a/builtin-var.c b/builtin-var.c
index 2280518..e6ee7bc 100644
--- a/builtin-var.c
+++ b/builtin-var.c
@@ -6,7 +6,7 @@
 #include "cache.h"
 #include "exec_cmd.h"
 
-static const char var_usage[] = "git var [-l | <variable>]";
+static const char var_usage[] = "git var (-l | <variable>)";
 
 static const char *editor(int flag)
 {
-- 
1.7.0

[PATCH 2/7] Make 'git var GIT_PAGER' always print the configured pager

[Jonathan Nieder]

Scripted commands that want to use git’s configured pager know better
than ‘git var’ does whether stdout is going to be a tty at the
appropriate time.  Checking isatty(1) as git_pager() does now won’t
cut it, since the output of git var itself is almost never a terminal.
The symptom is that when used by humans, ‘git var GIT_PAGER’ behaves
as it should, but when used by scripts, it always returns ‘cat’!

So avoid tricks with isatty() and just always print the configured
pager.

This does not fix callers to check isatty(1) themselves yet.
Nevertheless, this patch alone is enough to fix 'am --interactive',
since as an interactive command its behavior before before dec543
(2009-10-30) was to always paginate.

There are unfortunately no tests for am --interactive or git svn log
in the test suite, so test suite runs would detect none of this.

Thanks to Sebastian Celis for the report and Jeff King for the
analysis.

Reported-by: Sebastian Celis <sebastian@sebastiancelis.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
This commit message should be clearer, I hope.

 builtin-var.c |    2 +-
 cache.h       |    2 +-
 pager.c       |    6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin-var.c b/builtin-var.c
index e6ee7bc..70fdb4d 100644
--- a/builtin-var.c
+++ b/builtin-var.c
@@ -20,7 +20,7 @@ static const char *editor(int flag)
 
 static const char *pager(int flag)
 {
-	const char *pgm = git_pager();
+	const char *pgm = git_pager(1);
 
 	if (!pgm)
 		pgm = "cat";
diff --git a/cache.h b/cache.h
index d478eff..d454b7e 100644
--- a/cache.h
+++ b/cache.h
@@ -775,7 +775,7 @@ extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
 extern const char *git_editor(void);
-extern const char *git_pager(void);
+extern const char *git_pager(int stdout_is_tty);
 
 struct checkout {
 	const char *base_dir;
diff --git a/pager.c b/pager.c
index 2c7e8ec..dac358f 100644
--- a/pager.c
+++ b/pager.c
@@ -48,11 +48,11 @@ static void wait_for_pager_signal(int signo)
 	raise(signo);
 }
 
-const char *git_pager(void)
+const char *git_pager(int stdout_is_tty)
 {
 	const char *pager;
 
-	if (!isatty(1))
+	if (!stdout_is_tty)
 		return NULL;
 
 	pager = getenv("GIT_PAGER");
@@ -73,7 +73,7 @@ const char *git_pager(void)
 
 void setup_pager(void)
 {
-	const char *pager = git_pager();
+	const char *pager = git_pager(isatty(1));
 
 	if (!pager)
 		return;
-- 
1.7.0

[PATCH 3/7] git.1: Clarify the behavior of the --paginate option

[Jonathan Nieder]

The --paginate option is meant to negate the effect of an explicit or
implicit pager.<cmd> = false setting.  Thus it turns the pager on if
output is going to a terminal rather than unconditionally.

Explaining this requires mentioning configuration options very early
in the manual, when some users might not be familiar with them yet.
Provide some pointers to help: to the relevant section of git.1 for
the configuration mechanism in general and to git-config.1 for the
pager.<cmd> options.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Add a second paragraph to explain the less obvious part of the patch.
Jeff reverse-engineered this all correctly in [1], but one shouldn’t
have to think that hard to see where a patch is coming from.

http://thread.gmane.org/gmane.comp.version-control.git/139831/focus=139971

 Documentation/git.txt |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 01c4631..f26641a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -229,7 +229,10 @@ help ...`.
 
 -p::
 --paginate::
-	Pipe all output into 'less' (or if set, $PAGER).
+	Pipe all output into 'less' (or if set, $PAGER) if standard
+	output is a terminal.  This overrides the `pager.<cmd>`
+	configuration options (see the "Configuration Mechanism" section
+	below).
 
 --no-pager::
 	Do not pipe git output into a pager.
@@ -401,7 +404,8 @@ people.  Here is an example:
 ------------
 
 Various commands read from the configuration file and adjust
-their operation accordingly.
+their operation accordingly.  See linkgit:git-config[1] for a
+list.
 
 
 Identifier Terminology
-- 
1.7.0

[PATCH 4/7] git svn: Fix launching of pager

[Jonathan Nieder]

In commit dec543e (am -i, git-svn: use "git var GIT_PAGER"), I tried
to teach git svn to defer to git var on what pager to use.  In the
process, I introduced two bugs:

 - The value set for $pager in config_pager has local scope, so
   run_pager never sees it;

 - git var cannot tell whether git svn’s output is going to a
   terminal, so the value chosen for $pager does not reflect that
   information.

Fix them.

Reported-by: Sebastian Celis <sebastian@sebastiancelis.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Eric Wong <normalperson@yhbt.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Eric Wong wrote:

> Thanks Jonathan, confirmed this works with patch 2 of this series.
>
> Acked-by: Eric Wong <normalperson@yhbt.net>

Thanks.

 git-svn.perl |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 265852f..473a0b9 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -5459,7 +5459,12 @@ sub git_svn_log_cmd {
 
 # adapted from pager.c
 sub config_pager {
-	chomp(my $pager = command_oneline(qw(var GIT_PAGER)));
+	if (! -t *STDOUT) {
+		$ENV{GIT_PAGER_IN_USE} = 'false';
+		$pager = undef;
+		return;
+	}
+	chomp($pager = command_oneline(qw(var GIT_PAGER)));
 	if ($pager eq 'cat') {
 		$pager = undef;
 	}
@@ -5467,7 +5472,7 @@ sub config_pager {
 }
 
 sub run_pager {
-	return unless -t *STDOUT && defined $pager;
+	return unless defined $pager;
 	pipe my ($rfd, $wfd) or return;
 	defined(my $pid = fork) or ::fatal "Can't fork: $!";
 	if (!$pid) {
-- 
1.7.0

[PATCH 5/7] am: Fix launching of pager

[Jonathan Nieder]

The pagination functionality in git am has some problems:

 - It does not check if stdout is a tty, so it always paginates.

 - If $GIT_PAGER uses any environment variables, they are being
   ignored, since it does not run $GIT_PAGER through eval.

 - If $GIT_PAGER is set to the empty string, instead of passing
   output through to stdout, it tries to run $dotest/patch.

Fix them.  While at it, move the definition of git_pager() to
git-sh-setup so authors of other commands are not tempted to
reimplement it with the same mistakes.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Same as before, included for reference.

 git-am.sh       |    5 +----
 git-sh-setup.sh |   13 +++++++++++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 3c08d53..b11af03 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -663,10 +663,7 @@ do
 		[eE]*) git_editor "$dotest/final-commit"
 		       action=again ;;
 		[vV]*) action=again
-		       : ${GIT_PAGER=$(git var GIT_PAGER)}
-		       : ${LESS=-FRSX}
-		       export LESS
-		       $GIT_PAGER "$dotest/patch" ;;
+		       git_pager "$dotest/patch" ;;
 		*)     action=again ;;
 		esac
 	    done
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index d56426d..44fb467 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -107,6 +107,19 @@ git_editor() {
 	eval "$GIT_EDITOR" '"$@"'
 }
 
+git_pager() {
+	if test -t 1
+	then
+		GIT_PAGER=$(git var GIT_PAGER)
+	else
+		GIT_PAGER=cat
+	fi
+	: ${LESS=-FRSX}
+	export LESS
+
+	eval "$GIT_PAGER" '"$@"'
+}
+
 sane_grep () {
 	GREP_OPTIONS= LC_ALL=C grep "$@"
 }
-- 
1.7.0

[PATCH 6/7] tests: Add tests for automatic use of pager

[Jonathan Nieder]

Git’s automatic pagination support has some subtleties.  Add some
tests to make sure we don’t break:

 - when git will use a pager by default;
 - the effect of the --paginate and --no-pager options;
 - the effect of pagination on use of color;
 - how the choice of pager is configured.

This does not yet test:

 - use of pager by scripted commands (git svn and git am);
 - effect of the pager.* configuration variables;
 - setting of the LESS variable.

Some features involve checking whether stdout is a terminal, so many
of these tests are skipped unless output is passed through to the
terminal (i.e., unless $GIT_TEST_OPTS includes --verbose).

The immediate purpose for these tests was to avoid making things worse
after the breakage from my jn/editor-pager series (see commit 376f39,
2009-11-20).  Thanks to Sebastian Celis <sebastian@sebastiancelis.com>
for the report.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7006-pager.sh |  163 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 163 insertions(+), 0 deletions(-)
 create mode 100644 t/t7006-pager.sh

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
new file mode 100644
index 0000000..2e9cb9d
--- /dev/null
+++ b/t/t7006-pager.sh
@@ -0,0 +1,163 @@
+#!/bin/sh
+
+test_description='Test automatic use of a pager.'
+
+. ./test-lib.sh
+
+rm -f stdout_is_tty
+test_expect_success 'is stdout a terminal?' '
+	if test -t 1
+	then
+		: > stdout_is_tty
+	fi
+'
+
+if test -e stdout_is_tty
+then
+	test_set_prereq TTY
+else
+	say stdout is not a terminal, so skipping some tests.
+fi
+
+unset GIT_PAGER GIT_PAGER_IN_USE
+git config --unset core.pager
+PAGER='cat > paginated.out'
+export PAGER
+
+test_expect_success 'setup' '
+	test_commit initial
+'
+
+rm -f paginated.out
+test_expect_success TTY 'some commands use a pager' '
+	git log &&
+	test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success TTY 'some commands do not use a pager' '
+	git rev-list HEAD &&
+	! test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success 'no pager when stdout is a pipe' '
+	git log | cat &&
+	! test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success 'no pager when stdout is a regular file' '
+	git log > file &&
+	! test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success TTY 'git --paginate rev-list uses a pager' '
+	git --paginate rev-list HEAD  &&
+	test -e paginated.out
+'
+
+rm -f file paginated.out
+test_expect_success 'no pager even with --paginate when stdout is a pipe' '
+	git --paginate log | cat &&
+	! test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success TTY 'no pager with --no-pager' '
+	git --no-pager log &&
+	! test -e paginated.out
+'
+
+# A colored commit log will begin with an appropriate ANSI escape
+# for the first color; the text "commit" comes later.
+colorful() {
+	read firstline < $1
+	! expr "$firstline" : "^[a-zA-Z]" >/dev/null
+}
+
+rm -f colorful.log colorless.log
+test_expect_success 'tests can detect color' '
+	git log --no-color > colorless.log &&
+	git log --color > colorful.log &&
+	! colorful colorless.log &&
+	colorful colorful.log
+'
+
+rm -f colorless.log
+git config color.ui auto
+test_expect_success 'no color when stdout is a regular file' '
+	git log > colorless.log &&
+	! colorful colorless.log
+'
+
+rm -f paginated.out
+git config color.ui auto
+test_expect_success TTY 'color when writing to a pager' '
+	TERM=vt100 test_terminal git log &&
+	colorful paginated.out
+'
+
+rm -f colorful.log
+git config color.ui auto
+test_expect_success 'color when writing to a file intended for a pager' '
+	TERM=vt100 GIT_PAGER_IN_USE=true git log > colorful.log &&
+	colorful colorful.log
+'
+
+unset PAGER GIT_PAGER
+git config --unset core.pager
+test_expect_success 'determine default pager' '
+	less=$(git var GIT_PAGER) &&
+	test -n "$less"
+'
+
+if expr "$less" : '^[a-z]*$' > /dev/null && test_have_prereq TTY
+then
+	test_set_prereq SIMPLEPAGER
+fi
+
+unset PAGER GIT_PAGER
+git config --unset core.pager
+rm -f default_pager_used
+test_expect_success SIMPLEPAGER 'default pager is used by default' '
+	cat > $less <<-EOF &&
+	#!$SHELL_PATH
+	: > default_pager_used
+	EOF
+	chmod +x $less &&
+	PATH=.:$PATH git log &&
+	test -e default_pager_used
+'
+
+unset GIT_PAGER
+git config --unset core.pager
+rm -f PAGER_used
+test_expect_success TTY 'PAGER overrides default pager' '
+	PAGER=": > PAGER_used" &&
+	export PAGER &&
+	git log &&
+	test -e PAGER_used
+'
+
+unset GIT_PAGER
+rm -f core.pager_used
+test_expect_success TTY 'core.pager overrides PAGER' '
+	PAGER=: &&
+	export PAGER &&
+	git config core.pager ": > core.pager_used" &&
+	git log &&
+	test -e core.pager_used
+'
+
+rm -f GIT_PAGER_used
+test_expect_success TTY 'GIT_PAGER overrides core.pager' '
+	git config core.pager : &&
+	GIT_PAGER=": > GIT_PAGER_used" &&
+	export GIT_PAGER &&
+	git log &&
+	test -e GIT_PAGER_used
+'
+
+test_done
-- 
1.7.0

[PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Jonathan Nieder]

Test t7006-pager skips most of its tests except when running with
--verbose, for lack of a terminal to use.  Thus many of the tests
would not get much coverage, since whole testsuite runs with --verbose
are rare.  Redirecting output to /dev/tty would be problematic because
(1) there really could be no controlling terminal and (2) test
breakage might manifest itself by the producing extraneous output.
Luckily, there is a way around this: Unix98-style pseudo-terminals
allow us to create a fake terminal on the fly and capture its output.

Do so.  The new test-terminal command to accomplish this uses
posix_openpt (from SuSv3) to create a terminal because that is good
enough on Linux.  I would like some feedback on what platforms are
missing that function and thus what alternate interfaces are worth
supporting.  The perl IO::Tty module could take care of this for us,
but I do not think it is worth the extra dependency.

The test-terminal facility can be disabled by passing
NO_POSIX_OPENPT=YesPlease to the makefile, in which case the test
will revert to its old behavior (skipping most tests unless
GIT_TEST_OPTS includes --verbose).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Requiring users to add NO_POSIX_OPENPT themselves doesn’t seem so
great.  I did it that way just so I could send something and get
some feedback; a more suitable version would need a way to detect
missing openpt on the fly, for example by noticing the compilation
failure and continuing anyway.

 .gitignore       |    1 +
 Makefile         |    6 +++++
 t/t7006-pager.sh |   28 +++++++++++++++--------
 test-terminal.c  |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 87 insertions(+), 10 deletions(-)
 create mode 100644 test-terminal.c

diff --git a/.gitignore b/.gitignore
index 8df8f88..6b405ba 100644
--- a/.gitignore
+++ b/.gitignore
@@ -169,6 +169,7 @@
 /test-run-command
 /test-sha1
 /test-sigchain
+/test-terminal
 /common-cmds.h
 *.tar.gz
 *.dsc
diff --git a/Makefile b/Makefile
index 7bf2fca..9c900a7 100644
--- a/Makefile
+++ b/Makefile
@@ -45,6 +45,8 @@ all::
 #
 # Define NO_MEMMEM if you don't have memmem.
 #
+# Define NO_POSIX_OPENPT if you don't have posix_openpt (for tests).
+#
 # Define NO_STRLCPY if you don't have strlcpy.
 #
 # Define NO_STRTOUMAX if you don't have strtoumax in the C library.
@@ -1727,6 +1729,10 @@ TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
 TEST_PROGRAMS_NEED_X += test-index-version
 
+ifndef NO_POSIX_OPENPT
+TEST_PROGRAMS_NEED_X += test-terminal
+endif
+
 TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
 
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 2e9cb9d..21788e3 100644
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -5,18 +5,26 @@ test_description='Test automatic use of a pager.'
 . ./test-lib.sh
 
 rm -f stdout_is_tty
-test_expect_success 'is stdout a terminal?' '
+test_expect_success 'set up terminal for tests' '
 	if test -t 1
 	then
 		: > stdout_is_tty
+	elif test-terminal sh -c "test -t 1"
+	then
+		: > test_terminal_works
 	fi
 '
 
 if test -e stdout_is_tty
 then
+	test_terminal() { "$@"; }
+	test_set_prereq TTY
+elif test -e test_terminal_works
+then
+	test_terminal() { test-terminal "$@"; }
 	test_set_prereq TTY
 else
-	say stdout is not a terminal, so skipping some tests.
+	say no usable terminal, so skipping some tests
 fi
 
 unset GIT_PAGER GIT_PAGER_IN_USE
@@ -30,13 +38,13 @@ test_expect_success 'setup' '
 
 rm -f paginated.out
 test_expect_success TTY 'some commands use a pager' '
-	git log &&
+	test_terminal git log &&
 	test -e paginated.out
 '
 
 rm -f paginated.out
 test_expect_success TTY 'some commands do not use a pager' '
-	git rev-list HEAD &&
+	test_terminal git rev-list HEAD &&
 	! test -e paginated.out
 '
 
@@ -54,7 +62,7 @@ test_expect_success 'no pager when stdout is a regular file' '
 
 rm -f paginated.out
 test_expect_success TTY 'git --paginate rev-list uses a pager' '
-	git --paginate rev-list HEAD  &&
+	test_terminal git --paginate rev-list HEAD &&
 	test -e paginated.out
 '
 
@@ -66,7 +74,7 @@ test_expect_success 'no pager even with --paginate when stdout is a pipe' '
 
 rm -f paginated.out
 test_expect_success TTY 'no pager with --no-pager' '
-	git --no-pager log &&
+	test_terminal git --no-pager log &&
 	! test -e paginated.out
 '
 
@@ -127,7 +135,7 @@ test_expect_success SIMPLEPAGER 'default pager is used by default' '
 	: > default_pager_used
 	EOF
 	chmod +x $less &&
-	PATH=.:$PATH git log &&
+	PATH=.:$PATH test_terminal git log &&
 	test -e default_pager_used
 '
 
@@ -137,7 +145,7 @@ rm -f PAGER_used
 test_expect_success TTY 'PAGER overrides default pager' '
 	PAGER=": > PAGER_used" &&
 	export PAGER &&
-	git log &&
+	test_terminal git log &&
 	test -e PAGER_used
 '
 
@@ -147,7 +155,7 @@ test_expect_success TTY 'core.pager overrides PAGER' '
 	PAGER=: &&
 	export PAGER &&
 	git config core.pager ": > core.pager_used" &&
-	git log &&
+	test_terminal git log &&
 	test -e core.pager_used
 '
 
@@ -156,7 +164,7 @@ test_expect_success TTY 'GIT_PAGER overrides core.pager' '
 	git config core.pager : &&
 	GIT_PAGER=": > GIT_PAGER_used" &&
 	export GIT_PAGER &&
-	git log &&
+	test_terminal git log &&
 	test -e GIT_PAGER_used
 '
 
diff --git a/test-terminal.c b/test-terminal.c
new file mode 100644
index 0000000..f4d6a71
--- /dev/null
+++ b/test-terminal.c
@@ -0,0 +1,62 @@
+#include "cache.h"
+#include "run-command.h"
+
+static void newtty(int *master, int *slave)
+{
+	int fd;
+	const char *term;
+
+	fd = posix_openpt(O_RDWR|O_NOCTTY);
+	if (fd == -1 || grantpt(fd) || unlockpt(fd) || !(term = ptsname(fd)))
+		die_errno("Could not allocate a new pseudo-terminal");
+	*master = fd;
+	*slave = open(term, O_RDWR|O_NOCTTY);
+	if (*slave == -1)
+		die_errno("Could not open pseudo-terminal: %s", term);
+}
+
+static void setup_child(struct child_process *chld,
+			const char **args, int out)
+{
+	memset(chld, 0, sizeof(*chld));
+	chld->argv = args;
+	chld->out = out;
+}
+
+static void xsendfile(int out_fd, int in_fd)
+{
+	char buf[BUFSIZ];
+
+	/* Note: the real sendfile() cannot read from a terminal. */
+	for (;;) {
+		ssize_t n = xread(in_fd, buf, sizeof(buf));
+
+		/*
+		 * It is unspecified by POSIX whether reads
+		 * from a disconnected terminal will return
+		 * EIO (as in AIX 4.x, IRIX, and Linux) or
+		 * end-of-file.  Either is fine.
+		 */
+		if (n == 0 || (n == -1 && errno == EIO))
+			break;
+		if (n == -1)
+			die_errno("cannot read from child");
+		write_or_die(out_fd, buf, n);
+	}
+}
+
+int main(int argc, const char **argv)
+{
+	int masterfd, slavefd;
+	struct child_process chld;
+
+	if (argc < 2 || (argc == 2 && !strcmp(argv[1], "-h")))
+		usage("test-terminal program args");
+
+	newtty(&masterfd, &slavefd);
+	setup_child(&chld, argv + 1, slavefd);
+	if (start_command(&chld))
+		return error("failed to execute child");
+	xsendfile(1, masterfd);
+	return finish_command(&chld);
+}
-- 
1.7.0

Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Jeff King]

On Fri, Feb 19, 2010 at 01:23:31AM -0600, Jonathan Nieder wrote:

> Test t7006-pager skips most of its tests except when running with
> --verbose, for lack of a terminal to use.  Thus many of the tests
> would not get much coverage, since whole testsuite runs with --verbose
> are rare.  Redirecting output to /dev/tty would be problematic because
> (1) there really could be no controlling terminal and (2) test
> breakage might manifest itself by the producing extraneous output.
> Luckily, there is a way around this: Unix98-style pseudo-terminals
> allow us to create a fake terminal on the fly and capture its output.
> 
> Do so.  The new test-terminal command to accomplish this uses
> posix_openpt (from SuSv3) to create a terminal because that is good
> enough on Linux.  I would like some feedback on what platforms are
> missing that function and thus what alternate interfaces are worth
> supporting.  The perl IO::Tty module could take care of this for us,
> but I do not think it is worth the extra dependency.

Solaris 8 and 9 seem to be lacking it. Solaris 10 does have it. AIX 5.2
and 6.1 both have it.

So it would mean some platforms couldn't run all tests. That is probably
good enough, given that most of our terminal-related bugs have not been
platform-specific problems.

Still, it seems like just wrapping isatty would be simpler. I guess you
are opposed to carrying around test-specific code in the main git
binary?

-Peff

Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Jonathan Nieder]

Jeff King wrote:

> Solaris 8 and 9 seem to be lacking it. Solaris 10 does have it. AIX 5.2
> and 6.1 both have it.
> 
> So it would mean some platforms couldn't run all tests. That is probably
> good enough, given that most of our terminal-related bugs have not been
> platform-specific problems.

Hmm, how about /dev/ptmx?  (One can check by replacing posix_openpt(...)
with open("/dev/ptmx", ...) in the test-terminal.c I sent.)

> Still, it seems like just wrapping isatty would be simpler. I guess you
> are opposed to carrying around test-specific code in the main git
> binary?

No, not opposed.  Just lazy and not so interested in working on it.
I do not want to just take the implementation you provided because I
want to test the scripted git commands, too, though I haven’t gotten
around to that.

Jonathan

Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Jeff King]

On Fri, Feb 19, 2010 at 02:19:47AM -0600, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Solaris 8 and 9 seem to be lacking it. Solaris 10 does have it. AIX 5.2
> > and 6.1 both have it.
> > 
> > So it would mean some platforms couldn't run all tests. That is probably
> > good enough, given that most of our terminal-related bugs have not been
> > platform-specific problems.
> 
> Hmm, how about /dev/ptmx?  (One can check by replacing posix_openpt(...)
> with open("/dev/ptmx", ...) in the test-terminal.c I sent.)

Solaris 8 does have /dev/ptmx. Sorry, I wasn't able to test it, as my
sol8 box is very vanilla right now, and I can't actually compile git on
it (it doesn't even have gmake). :)

Maybe Brandon (cc'd) can try it.

> No, not opposed.  Just lazy and not so interested in working on it.
> I do not want to just take the implementation you provided because I
> want to test the scripted git commands, too, though I haven’t gotten
> around to that.

Ah, right, I forgot about scripts. That makes things a little uglier,
but it should theoretically be possible.

-Peff

Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Brandon Casey]

Jeff King wrote:
> On Fri, Feb 19, 2010 at 02:19:47AM -0600, Jonathan Nieder wrote:
> 
>> Jeff King wrote:
>>
>>> Solaris 8 and 9 seem to be lacking it. Solaris 10 does have it. AIX 5.2
>>> and 6.1 both have it.
>>>
>>> So it would mean some platforms couldn't run all tests. That is probably
>>> good enough, given that most of our terminal-related bugs have not been
>>> platform-specific problems.
>> Hmm, how about /dev/ptmx?  (One can check by replacing posix_openpt(...)
>> with open("/dev/ptmx", ...) in the test-terminal.c I sent.)
> 
> Solaris 8 does have /dev/ptmx. Sorry, I wasn't able to test it, as my
> sol8 box is very vanilla right now, and I can't actually compile git on
> it (it doesn't even have gmake). :)
> 
> Maybe Brandon (cc'd) can try it.

I'm compiling vanilla master now.  If that compiles without error,
I'll apply Jonathan's series and make the changes to test-terminal.c
that he suggested.  I have access to Solaris 7, so you can imagine
how fast the machine is... and how long the compile will take.

-brandon

Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Brandon Casey]

>> On Fri, Feb 19, 2010 at 02:19:47AM -0600, Jonathan Nieder wrote:
>>
>>> Jeff King wrote:
>>>
>>>> Solaris 8 and 9 seem to be lacking it. Solaris 10 does have it. AIX 5.2
>>>> and 6.1 both have it.
>>>>
>>>> So it would mean some platforms couldn't run all tests. That is probably
>>>> good enough, given that most of our terminal-related bugs have not been
>>>> platform-specific problems.
>>> Hmm, how about /dev/ptmx?  (One can check by replacing posix_openpt(...)
>>> with open("/dev/ptmx", ...) in the test-terminal.c I sent.)

Didn't work on Solaris 7.  I applied your series on top of master with the
change you described.  Here's the diff:

diff --git a/test-terminal.c b/test-terminal.c
index f4d6a71..6408a7d 100644
--- a/test-terminal.c
+++ b/test-terminal.c
@@ -6,7 +6,7 @@ static void newtty(int *master, int *slave)
        int fd;
        const char *term;
 
-       fd = posix_openpt(O_RDWR|O_NOCTTY);
+       fd = open("/dev/ptmx", O_RDWR|O_NOCTTY);
        if (fd == -1 || grantpt(fd) || unlockpt(fd) || !(term = ptsname(fd)))
                die_errno("Could not allocate a new pseudo-terminal");
        *master = fd;


Here's the result of the terminal test:

  # ./test-terminal sh -c "test -t 1"
  # echo $?
  1


And here's the output of t7006-pager:
*** t7006-pager.sh ***
*   ok 1: set up terminal for tests
* no usable terminal, so skipping some tests
*   ok 2: setup
* skip 3: some commands use a pager
* skip 4: some commands do not use a pager
*   ok 5: no pager when stdout is a pipe
*   ok 6: no pager when stdout is a regular file
* skip 7: git --paginate rev-list uses a pager
*   ok 8: no pager even with --paginate when stdout is a pipe
* skip 9: no pager with --no-pager
*   ok 10: tests can detect color
*   ok 11: no color when stdout is a regular file
* skip 12: color when writing to a pager
*   ok 13: color when writing to a file intended for a pager
*   ok 14: determine default pager
* skip 15: default pager is used by default
* skip 16: PAGER overrides default pager
* skip 17: core.pager overrides PAGER
* skip 18: GIT_PAGER overrides core.pager
* passed all 18 test(s)

hth,
-brandon

Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Jonathan Nieder]

Brandon Casey wrote:
>>> On Fri, Feb 19, 2010 at 02:19:47AM -0600, Jonathan Nieder wrote:

>>>> Hmm, how about /dev/ptmx?  (One can check by replacing posix_openpt(...)
>>>> with open("/dev/ptmx", ...) in the test-terminal.c I sent.)
> 
> Didn't work on Solaris 7.  I applied your series on top of master with the
> change you described.  Here's the diff:
[...]
> Here's the result of the terminal test:
> 
>   # ./test-terminal sh -c "test -t 1"
>   # echo $?
>   1
[...]
>
> And here's the output of t7006-pager:
> *** t7006-pager.sh ***
> *   ok 1: set up terminal for tests
> * no usable terminal, so skipping some tests

Thanks for trying it out.  That’s an excellent outcome: it means that
test-terminal compiled without trouble with no makefile magic.  It
does seem strange to me that there was no error message.  Is
sh -c "test -t 1" false for an ordinary terminal?

Jonathan

Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Brandon Casey]

On Fri, Feb 19, 2010 at 6:39 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Casey wrote:
>>>> On Fri, Feb 19, 2010 at 02:19:47AM -0600, Jonathan Nieder wrote:
>
>>>>> Hmm, how about /dev/ptmx?  (One can check by replacing posix_openpt(...)
>>>>> with open("/dev/ptmx", ...) in the test-terminal.c I sent.)
>>
>> Didn't work on Solaris 7.  I applied your series on top of master with the
>> change you described.  Here's the diff:
> [...]
>> Here's the result of the terminal test:
>>
>>   # ./test-terminal sh -c "test -t 1"
>>   # echo $?
>>   1
> [...]
>>
>> And here's the output of t7006-pager:
>> *** t7006-pager.sh ***
>> *   ok 1: set up terminal for tests
>> * no usable terminal, so skipping some tests
>
> Thanks for trying it out.  That’s an excellent outcome: it means that
> test-terminal compiled without trouble with no makefile magic.  It
> does seem strange to me that there was no error message.  Is
> sh -c "test -t 1" false for an ordinary terminal?

No, it is true.  I also substituted /usr/xpg4/bin/sh and got the same result.

-brandon

[PATCH v2 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Jonathan Nieder]

Testing pagination requires (fake or real) access to a terminal so we
can see whether the pagination automatically kicks in, which makes it
hard to get good coverage when running tests without --verbose.  There
are a number of ways to work around that:

 - Replace all isatty calls with calls to a custom xisatty wrapper
   that usually checks for a terminal but can be overridden for tests.
   This would be workable, but it would require implementing xisatty
   separately in three languages (C, shell, and perl) and making sure
   that any code that is to be tested always uses the wrapper.

 - Redirect stdout to /dev/tty.  This would be problematic because
   there might be no terminal available, and even if a terminal is
   available, it might not be appropriate to spew output to it.

 - Create a new pseudo-terminal on the fly and capture its output.

This patch implements the third approach.

The new test-terminal command opens /dev/ptmx to create a terminal and
executes the program specified by its arguments with that terminal as
stdout.  All platforms I know of with SuSv3 posix_openpt implement it
as open("/dev/ptmx", ...), so this should be just as portable.

Pre-SuSv3 systems may not have the proper support, but that is okay.
If SVR4 STREAMS are supported, all the functions used by test-terminal
will be available and succeed, though the result will not satisfy
isatty() unless the appropriate ioctl()s are used.  The test suite
checks for this and will not use test-terminal in that case.

On non-Unixlike and pre-SVR4 systems, this functionality should be
disabled by passing NO_GRANTPT=YesPlease to the makefile.  I do not
know whether Cygwin or HP-UX implements the required functions
appropriately, so to be safe this patch includes that option for them.

On platforms where test-terminal does not work, the test script will
maintain its old behavior (skipping most of its tests unless
GIT_TEST_OPTS includes --verbose).

Thanks to Brandon Casey <drafnel@gmail.com> and Jeff King
<peff@peff.net> for porting help.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Brandon Casey wrote:
> On Fri, Feb 19, 2010 at 6:39 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> Thanks for trying it out.  That’s an excellent outcome: it means that
>> test-terminal compiled without trouble with no makefile magic.  It
>> does seem strange to me that there was no error message.  Is
>> sh -c "test -t 1" false for an ordinary terminal?
>
> No, it is true.  I also substituted /usr/xpg4/bin/sh and got the same result.

Ah, okay.  I read up a little more, and it seems that old STREAMS-based
implementations of pseudo-terminals require

	ioctl(*slave, I_PUSH, "ptem");
	ioctl(*slave, I_PUSH, "ldterm");
	ioctl(*slave, I_PUSH, "pckt");

where I_PUSH is ('S' << 8) | 2.  I am terrified of ioctl number conflicts
and do not want to try that, so it is nice to see that this fails so
gracefully.

Thanks again.  Here’s a revised patch.

 .gitignore       |    1 +
 Makefile         |   11 +++++++++
 t/t7006-pager.sh |   28 +++++++++++++++--------
 test-terminal.c  |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 92 insertions(+), 10 deletions(-)
 create mode 100644 test-terminal.c

diff --git a/.gitignore b/.gitignore
index 8df8f88..6b405ba 100644
--- a/.gitignore
+++ b/.gitignore
@@ -169,6 +169,7 @@
 /test-run-command
 /test-sha1
 /test-sigchain
+/test-terminal
 /common-cmds.h
 *.tar.gz
 *.dsc
diff --git a/Makefile b/Makefile
index 7bf2fca..0df3795 100644
--- a/Makefile
+++ b/Makefile
@@ -55,6 +55,9 @@ all::
 #
 # Define NO_UNSETENV if you don't have unsetenv in the C library.
 #
+# Define NO_GRANTPT if you don't have grantpt, unlockpt, and ptsname
+# (for tests).
+#
 # Define NO_MKDTEMP if you don't have mkdtemp in the C library.
 #
 # Define NO_MKSTEMPS if you don't have mkstemps in the C library.
@@ -803,6 +806,7 @@ ifeq ($(uname_O),Cygwin)
 	NO_TRUSTABLE_FILEMODE = UnfortunatelyYes
 	OLD_ICONV = UnfortunatelyYes
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
+	NO_GRANTPT = YesPlease
 	# There are conflicting reports about this.
 	# On some boxes NO_MMAP is needed, and not so elsewhere.
 	# Try commenting this out if you suspect MMAP is more efficient
@@ -910,6 +914,7 @@ ifeq ($(uname_S),HP-UX)
 	NO_UNSETENV = YesPlease
 	NO_HSTRERROR = YesPlease
 	NO_SYS_SELECT_H = YesPlease
+	NO_GRANTPT = YesPlease
 	SNPRINTF_RETURNS_BOGUS = YesPlease
 endif
 ifeq ($(uname_S),Windows)
@@ -937,6 +942,7 @@ ifeq ($(uname_S),Windows)
 	NO_PERL_MAKEMAKER = YesPlease
 	RUNTIME_PREFIX = YesPlease
 	NO_POSIX_ONLY_PROGRAMS = YesPlease
+	NO_GRANTPT = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
@@ -989,6 +995,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_PERL_MAKEMAKER = YesPlease
 	RUNTIME_PREFIX = YesPlease
 	NO_POSIX_ONLY_PROGRAMS = YesPlease
+	NO_GRANTPT = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
@@ -1727,6 +1734,10 @@ TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
 TEST_PROGRAMS_NEED_X += test-index-version
 
+ifndef NO_GRANTPT
+TEST_PROGRAMS_NEED_X += test-terminal
+endif
+
 TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
 
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 2e9cb9d..21788e3 100644
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -5,18 +5,26 @@ test_description='Test automatic use of a pager.'
 . ./test-lib.sh
 
 rm -f stdout_is_tty
-test_expect_success 'is stdout a terminal?' '
+test_expect_success 'set up terminal for tests' '
 	if test -t 1
 	then
 		: > stdout_is_tty
+	elif test-terminal sh -c "test -t 1"
+	then
+		: > test_terminal_works
 	fi
 '
 
 if test -e stdout_is_tty
 then
+	test_terminal() { "$@"; }
+	test_set_prereq TTY
+elif test -e test_terminal_works
+then
+	test_terminal() { test-terminal "$@"; }
 	test_set_prereq TTY
 else
-	say stdout is not a terminal, so skipping some tests.
+	say no usable terminal, so skipping some tests
 fi
 
 unset GIT_PAGER GIT_PAGER_IN_USE
@@ -30,13 +38,13 @@ test_expect_success 'setup' '
 
 rm -f paginated.out
 test_expect_success TTY 'some commands use a pager' '
-	git log &&
+	test_terminal git log &&
 	test -e paginated.out
 '
 
 rm -f paginated.out
 test_expect_success TTY 'some commands do not use a pager' '
-	git rev-list HEAD &&
+	test_terminal git rev-list HEAD &&
 	! test -e paginated.out
 '
 
@@ -54,7 +62,7 @@ test_expect_success 'no pager when stdout is a regular file' '
 
 rm -f paginated.out
 test_expect_success TTY 'git --paginate rev-list uses a pager' '
-	git --paginate rev-list HEAD  &&
+	test_terminal git --paginate rev-list HEAD &&
 	test -e paginated.out
 '
 
@@ -66,7 +74,7 @@ test_expect_success 'no pager even with --paginate when stdout is a pipe' '
 
 rm -f paginated.out
 test_expect_success TTY 'no pager with --no-pager' '
-	git --no-pager log &&
+	test_terminal git --no-pager log &&
 	! test -e paginated.out
 '
 
@@ -127,7 +135,7 @@ test_expect_success SIMPLEPAGER 'default pager is used by default' '
 	: > default_pager_used
 	EOF
 	chmod +x $less &&
-	PATH=.:$PATH git log &&
+	PATH=.:$PATH test_terminal git log &&
 	test -e default_pager_used
 '
 
@@ -137,7 +145,7 @@ rm -f PAGER_used
 test_expect_success TTY 'PAGER overrides default pager' '
 	PAGER=": > PAGER_used" &&
 	export PAGER &&
-	git log &&
+	test_terminal git log &&
 	test -e PAGER_used
 '
 
@@ -147,7 +155,7 @@ test_expect_success TTY 'core.pager overrides PAGER' '
 	PAGER=: &&
 	export PAGER &&
 	git config core.pager ": > core.pager_used" &&
-	git log &&
+	test_terminal git log &&
 	test -e core.pager_used
 '
 
@@ -156,7 +164,7 @@ test_expect_success TTY 'GIT_PAGER overrides core.pager' '
 	git config core.pager : &&
 	GIT_PAGER=": > GIT_PAGER_used" &&
 	export GIT_PAGER &&
-	git log &&
+	test_terminal git log &&
 	test -e GIT_PAGER_used
 '
 
diff --git a/test-terminal.c b/test-terminal.c
new file mode 100644
index 0000000..6408a7d
--- /dev/null
+++ b/test-terminal.c
@@ -0,0 +1,62 @@
+#include "cache.h"
+#include "run-command.h"
+
+static void newtty(int *master, int *slave)
+{
+	int fd;
+	const char *term;
+
+	fd = open("/dev/ptmx", O_RDWR|O_NOCTTY);
+	if (fd == -1 || grantpt(fd) || unlockpt(fd) || !(term = ptsname(fd)))
+		die_errno("Could not allocate a new pseudo-terminal");
+	*master = fd;
+	*slave = open(term, O_RDWR|O_NOCTTY);
+	if (*slave == -1)
+		die_errno("Could not open pseudo-terminal: %s", term);
+}
+
+static void setup_child(struct child_process *chld,
+			const char **args, int out)
+{
+	memset(chld, 0, sizeof(*chld));
+	chld->argv = args;
+	chld->out = out;
+}
+
+static void xsendfile(int out_fd, int in_fd)
+{
+	char buf[BUFSIZ];
+
+	/* Note: the real sendfile() cannot read from a terminal. */
+	for (;;) {
+		ssize_t n = xread(in_fd, buf, sizeof(buf));
+
+		/*
+		 * It is unspecified by POSIX whether reads
+		 * from a disconnected terminal will return
+		 * EIO (as in AIX 4.x, IRIX, and Linux) or
+		 * end-of-file.  Either is fine.
+		 */
+		if (n == 0 || (n == -1 && errno == EIO))
+			break;
+		if (n == -1)
+			die_errno("cannot read from child");
+		write_or_die(out_fd, buf, n);
+	}
+}
+
+int main(int argc, const char **argv)
+{
+	int masterfd, slavefd;
+	struct child_process chld;
+
+	if (argc < 2 || (argc == 2 && !strcmp(argv[1], "-h")))
+		usage("test-terminal program args");
+
+	newtty(&masterfd, &slavefd);
+	setup_child(&chld, argv + 1, slavefd);
+	if (start_command(&chld))
+		return error("failed to execute child");
+	xsendfile(1, masterfd);
+	return finish_command(&chld);
+}
-- 
1.7.0

Re: [PATCH v2 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>  - Create a new pseudo-terminal on the fly and capture its output.
>
> This patch implements the third approach.

Yuck.  You must be having too much fun ;-).

> On non-Unixlike and pre-SVR4 systems, this functionality should be
> disabled by passing NO_GRANTPT=YesPlease to the makefile.  I do not
> know whether Cygwin or HP-UX implements the required functions
> appropriately, so to be safe this patch includes that option for them.
>
> On platforms where test-terminal does not work, the test script will
> maintain its old behavior (skipping most of its tests unless
> GIT_TEST_OPTS includes --verbose).

If we are truly serious about doing this, it might be worthwhile to check
how expect (http://expect.nist.gov) does this portably.

[PATCH v3 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Jonathan Nieder]

Testing pagination requires (fake or real) access to a terminal so we
can see whether the pagination automatically kicks in, which makes it
hard to get good coverage when running tests without --verbose.  There
are a number of ways to work around that:

 - Replace all isatty calls with calls to a custom xisatty wrapper
   that usually checks for a terminal but can be overridden for tests.
   This would be workable, but it would require implementing xisatty
   separately in three languages (C, shell, and perl) and making sure
   that any code that is to be tested always uses the wrapper.

 - Redirect stdout to /dev/tty.  This would be problematic because
   there might be no terminal available, and even if a terminal is
   available, it might not be appropriate to spew output to it.

 - Create a new pseudo-terminal on the fly and capture its output.

This patch implements the third approach.

The new test-terminal.perl helper uses IO::Pty from Expect.pm to create
a terminal and executes the program specified by its arguments with
that terminal as stdout.  If the IO::Pty module is missing or not
working on a system, the test script will maintain its old behavior
(skipping most of its tests unless GIT_TEST_OPTS includes --verbose).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

> > On non-Unixlike and pre-SVR4 systems, this functionality should be
> > disabled by passing NO_GRANTPT=YesPlease to the makefile.  I do not
> > know whether Cygwin or HP-UX implements the required functions
> > appropriately, so to be safe this patch includes that option for them.
> >
> > On platforms where test-terminal does not work, the test script will
> > maintain its old behavior (skipping most of its tests unless
> > GIT_TEST_OPTS includes --verbose).
> 
> If we are truly serious about doing this, it might be worthwhile to check
> how expect (http://expect.nist.gov) does this portably.

I was not interested in making this work perfectly on ancient systems,
because that would involve providing variants for old-style BSD ptys,
STREAMS-based ptys, and the modern simpler ptys.  Looking at the expect
source, it seems there are even more variants to worry about than I had
thought.

On the other hand, I do think it would be better to make this work (or
not work) without makefile changes on all platforms.  So how about this?
It lets the IO::Pty library used to implement Expect.pm do the dirty
work for us.

The main downsides are that this means less coverage (not everyone has
IO::Pty installed) and it is much slower than the C version.

 .gitignore                 |    1 +
 t/t7006-pager.sh           |   33 ++++++++++++++------
 t/t7006/test-terminal.perl |   70 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+), 10 deletions(-)
 create mode 100755 t/t7006/test-terminal.perl

diff --git a/.gitignore b/.gitignore
index 8df8f88..6b405ba 100644
--- a/.gitignore
+++ b/.gitignore
@@ -169,6 +169,7 @@
 /test-run-command
 /test-sha1
 /test-sigchain
+/test-terminal
 /common-cmds.h
 *.tar.gz
 *.dsc
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 2e9cb9d..da0f962 100644
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -5,18 +5,31 @@ test_description='Test automatic use of a pager.'
 . ./test-lib.sh
 
 rm -f stdout_is_tty
-test_expect_success 'is stdout a terminal?' '
+test_expect_success 'set up terminal for tests' '
 	if test -t 1
 	then
 		: > stdout_is_tty
+	elif
+		test_have_prereq PERL &&
+		"$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl \
+			sh -c "test -t 1"
+	then
+		: > test_terminal_works
 	fi
 '
 
 if test -e stdout_is_tty
 then
+	test_terminal() { "$@"; }
+	test_set_prereq TTY
+elif test -e test_terminal_works
+then
+	test_terminal() {
+		"$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl "$@"
+	}
 	test_set_prereq TTY
 else
-	say stdout is not a terminal, so skipping some tests.
+	say no usable terminal, so skipping some tests
 fi
 
 unset GIT_PAGER GIT_PAGER_IN_USE
@@ -30,13 +43,13 @@ test_expect_success 'setup' '
 
 rm -f paginated.out
 test_expect_success TTY 'some commands use a pager' '
-	git log &&
+	test_terminal git log &&
 	test -e paginated.out
 '
 
 rm -f paginated.out
 test_expect_success TTY 'some commands do not use a pager' '
-	git rev-list HEAD &&
+	test_terminal git rev-list HEAD &&
 	! test -e paginated.out
 '
 
@@ -54,7 +67,7 @@ test_expect_success 'no pager when stdout is a regular file' '
 
 rm -f paginated.out
 test_expect_success TTY 'git --paginate rev-list uses a pager' '
-	git --paginate rev-list HEAD  &&
+	test_terminal git --paginate rev-list HEAD &&
 	test -e paginated.out
 '
 
@@ -66,7 +79,7 @@ test_expect_success 'no pager even with --paginate when stdout is a pipe' '
 
 rm -f paginated.out
 test_expect_success TTY 'no pager with --no-pager' '
-	git --no-pager log &&
+	test_terminal git --no-pager log &&
 	! test -e paginated.out
 '
 
@@ -127,7 +140,7 @@ test_expect_success SIMPLEPAGER 'default pager is used by default' '
 	: > default_pager_used
 	EOF
 	chmod +x $less &&
-	PATH=.:$PATH git log &&
+	PATH=.:$PATH test_terminal git log &&
 	test -e default_pager_used
 '
 
@@ -137,7 +150,7 @@ rm -f PAGER_used
 test_expect_success TTY 'PAGER overrides default pager' '
 	PAGER=": > PAGER_used" &&
 	export PAGER &&
-	git log &&
+	test_terminal git log &&
 	test -e PAGER_used
 '
 
@@ -147,7 +160,7 @@ test_expect_success TTY 'core.pager overrides PAGER' '
 	PAGER=: &&
 	export PAGER &&
 	git config core.pager ": > core.pager_used" &&
-	git log &&
+	test_terminal git log &&
 	test -e core.pager_used
 '
 
@@ -156,7 +169,7 @@ test_expect_success TTY 'GIT_PAGER overrides core.pager' '
 	git config core.pager : &&
 	GIT_PAGER=": > GIT_PAGER_used" &&
 	export GIT_PAGER &&
-	git log &&
+	test_terminal git log &&
 	test -e GIT_PAGER_used
 '
 
diff --git a/t/t7006/test-terminal.perl b/t/t7006/test-terminal.perl
new file mode 100755
index 0000000..b51cfc6
--- /dev/null
+++ b/t/t7006/test-terminal.perl
@@ -0,0 +1,70 @@
+#!/usr/bin/perl
+use strict;
+use warnings;
+use IO::Pty;
+
+# Fork and execute @$argv with stdout redirected to $out.
+sub start_child {
+	my ($argv, $out) = @_;
+	my $pid = fork;
+	if (not defined $pid) {
+		die "fork failed: $!"
+	} elsif ($pid == 0) {
+		open STDOUT, ">&", $out;
+		close $out;
+		exec(@$argv) or die "cannot exec '$argv->[0]': $!"
+	}
+	return $pid;
+}
+
+# Wait for $pid to finish.
+sub finish_child {
+	# Simplified from wait_or_whine() in run-command.c.
+	my ($pid) = @_;
+
+	my $waiting = waitpid($pid, 0);
+	if ($waiting < 0) {
+		die "waitpid failed: $!";
+	} elsif ($? & 127) {
+		my $code = $? & 127;
+		warn "died of signal $code";
+		return $code - 128;
+	} else {
+		return $? >> 8;
+	}
+}
+
+sub xsendfile {
+	my ($out, $in) = @_;
+
+	my $buf;
+
+	# Note: the real sendfile() cannot read from a terminal.
+	for (;;) {
+		my $n = sysread $in, $buf, 4096;
+
+		# It is unspecified by POSIX whether reads
+		# from a disconnected terminal will return
+		# EIO (as in AIX 4.x, IRIX, and Linux) or
+		# end-of-file.  Either is fine.
+		if (!defined($n) && $!{EIO}) {
+			return;
+		} elsif (!defined($n)) {
+			die "cannot read from child: $!";
+		} elsif ($n == 0) {
+			return;
+		} else {
+			print $out $buf or die "write error: $!";
+		}
+	}
+}
+
+if ($#ARGV < 1) {
+	die "usage: test-terminal program args";
+}
+my $master = new IO::Pty;
+my $slave = $master->slave;
+my $pid = start_child(\@ARGV, $slave);
+close $slave;
+xsendfile(\*STDOUT, $master);
+exit(finish_child($pid));
-- 
1.7.0

[PATCH squash] Simplify test-terminal.perl

[Jonathan Nieder]

The File::Copy module is included with perl (since version 5.002,
1995), so this simplification comes for free.  It does not make the
test noticeably faster or slower.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jonathan Nieder wrote:
> it is much slower than the C version.

I take that back.  The perl version takes roughly 3.7 s and the
C version 3.4 s here (both with hot cache).  Noticeable alone, but a
tiny blip in the context of a full test run.

Sorry to mislead.

 t/t7006/test-terminal.perl |   26 +++++++-------------------
 1 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/t/t7006/test-terminal.perl b/t/t7006/test-terminal.perl
index b51cfc6..73ff809 100755
--- a/t/t7006/test-terminal.perl
+++ b/t/t7006/test-terminal.perl
@@ -2,8 +2,9 @@
 use strict;
 use warnings;
 use IO::Pty;
+use File::Copy;
 
-# Fork and execute @$argv with stdout redirected to $out.
+# Run @$argv in the background with stdout redirected to $out.
 sub start_child {
 	my ($argv, $out) = @_;
 	my $pid = fork;
@@ -37,26 +38,13 @@ sub finish_child {
 sub xsendfile {
 	my ($out, $in) = @_;
 
-	my $buf;
-
 	# Note: the real sendfile() cannot read from a terminal.
-	for (;;) {
-		my $n = sysread $in, $buf, 4096;
 
-		# It is unspecified by POSIX whether reads
-		# from a disconnected terminal will return
-		# EIO (as in AIX 4.x, IRIX, and Linux) or
-		# end-of-file.  Either is fine.
-		if (!defined($n) && $!{EIO}) {
-			return;
-		} elsif (!defined($n)) {
-			die "cannot read from child: $!";
-		} elsif ($n == 0) {
-			return;
-		} else {
-			print $out $buf or die "write error: $!";
-		}
-	}
+	# It is unspecified by POSIX whether reads
+	# from a disconnected terminal will return
+	# EIO (as in AIX 4.x, IRIX, and Linux) or
+	# end-of-file.  Either is fine.
+	copy($in, $out, 4096) or $!{EIO} or die "cannot copy from child: $!";
 }
 
 if ($#ARGV < 1) {
-- 
1.7.0

Re: [PATCH 6/7] tests: Add tests for automatic use of pager

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> new file mode 100644
> index 0000000..2e9cb9d
> --- /dev/null
> +++ b/t/t7006-pager.sh
> @@ -0,0 +1,163 @@
> ...
> +rm -f paginated.out
> +git config color.ui auto
> +test_expect_success TTY 'color when writing to a pager' '
> +	TERM=vt100 test_terminal git log &&
> +	colorful paginated.out
> +'

I didn't see test_terminal defined up to this point.  Am I missing
something?

[PATCH v2 6/7] tests: Add tests for automatic use of pager

[Jonathan Nieder]

Git’s automatic pagination support has some subtleties.  Add some
tests to make sure we don’t break:

 - when git will use a pager by default;
 - the effect of the --paginate and --no-pager options;
 - the effect of pagination on use of color;
 - how the choice of pager is configured.

This does not yet test:

 - use of the pager by scripted commands (git svn and git am);
 - effect of the pager.* configuration variables;
 - setting of the LESS variable.

Some features involve checking whether stdout is a terminal, so many
of these tests are skipped unless output is passed through to the
terminal (i.e., unless $GIT_TEST_OPTS includes --verbose).

The immediate purpose for these tests was to avoid making things worse
after the breakage from my jn/editor-pager series (see commit 376f39,
2009-11-20).  Thanks to Sebastian Celis <sebastian@sebastiancelis.com>
for the report.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
On Sat, Feb 20, 2010 at 09:33:00AM -0800, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> +	TERM=vt100 test_terminal git log &&
> 
> I didn't see test_terminal defined up to this point.  Am I missing
> something?

Good catch, thanks.

 t/t7006-pager.sh |  163 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 163 insertions(+), 0 deletions(-)
 create mode 100644 t/t7006-pager.sh

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
new file mode 100644
index 0000000..4f52ea5
--- /dev/null
+++ b/t/t7006-pager.sh
@@ -0,0 +1,163 @@
+#!/bin/sh
+
+test_description='Test automatic use of a pager.'
+
+. ./test-lib.sh
+
+rm -f stdout_is_tty
+test_expect_success 'is stdout a terminal?' '
+	if test -t 1
+	then
+		: > stdout_is_tty
+	fi
+'
+
+if test -e stdout_is_tty
+then
+	test_set_prereq TTY
+else
+	say stdout is not a terminal, so skipping some tests.
+fi
+
+unset GIT_PAGER GIT_PAGER_IN_USE
+git config --unset core.pager
+PAGER='cat > paginated.out'
+export PAGER
+
+test_expect_success 'setup' '
+	test_commit initial
+'
+
+rm -f paginated.out
+test_expect_success TTY 'some commands use a pager' '
+	git log &&
+	test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success TTY 'some commands do not use a pager' '
+	git rev-list HEAD &&
+	! test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success 'no pager when stdout is a pipe' '
+	git log | cat &&
+	! test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success 'no pager when stdout is a regular file' '
+	git log > file &&
+	! test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success TTY 'git --paginate rev-list uses a pager' '
+	git --paginate rev-list HEAD  &&
+	test -e paginated.out
+'
+
+rm -f file paginated.out
+test_expect_success 'no pager even with --paginate when stdout is a pipe' '
+	git --paginate log | cat &&
+	! test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success TTY 'no pager with --no-pager' '
+	git --no-pager log &&
+	! test -e paginated.out
+'
+
+# A colored commit log will begin with an appropriate ANSI escape
+# for the first color; the text "commit" comes later.
+colorful() {
+	read firstline < $1
+	! expr "$firstline" : "^[a-zA-Z]" >/dev/null
+}
+
+rm -f colorful.log colorless.log
+test_expect_success 'tests can detect color' '
+	git log --no-color > colorless.log &&
+	git log --color > colorful.log &&
+	! colorful colorless.log &&
+	colorful colorful.log
+'
+
+rm -f colorless.log
+git config color.ui auto
+test_expect_success 'no color when stdout is a regular file' '
+	git log > colorless.log &&
+	! colorful colorless.log
+'
+
+rm -f paginated.out
+git config color.ui auto
+test_expect_success TTY 'color when writing to a pager' '
+	TERM=vt100 git log &&
+	colorful paginated.out
+'
+
+rm -f colorful.log
+git config color.ui auto
+test_expect_success 'color when writing to a file intended for a pager' '
+	TERM=vt100 GIT_PAGER_IN_USE=true git log > colorful.log &&
+	colorful colorful.log
+'
+
+unset PAGER GIT_PAGER
+git config --unset core.pager
+test_expect_success 'determine default pager' '
+	less=$(git var GIT_PAGER) &&
+	test -n "$less"
+'
+
+if expr "$less" : '^[a-z]*$' > /dev/null && test_have_prereq TTY
+then
+	test_set_prereq SIMPLEPAGER
+fi
+
+unset PAGER GIT_PAGER
+git config --unset core.pager
+rm -f default_pager_used
+test_expect_success SIMPLEPAGER 'default pager is used by default' '
+	cat > $less <<-EOF &&
+	#!$SHELL_PATH
+	: > default_pager_used
+	EOF
+	chmod +x $less &&
+	PATH=.:$PATH git log &&
+	test -e default_pager_used
+'
+
+unset GIT_PAGER
+git config --unset core.pager
+rm -f PAGER_used
+test_expect_success TTY 'PAGER overrides default pager' '
+	PAGER=": > PAGER_used" &&
+	export PAGER &&
+	git log &&
+	test -e PAGER_used
+'
+
+unset GIT_PAGER
+rm -f core.pager_used
+test_expect_success TTY 'core.pager overrides PAGER' '
+	PAGER=: &&
+	export PAGER &&
+	git config core.pager ": > core.pager_used" &&
+	git log &&
+	test -e core.pager_used
+'
+
+rm -f GIT_PAGER_used
+test_expect_success TTY 'GIT_PAGER overrides core.pager' '
+	git config core.pager : &&
+	GIT_PAGER=": > GIT_PAGER_used" &&
+	export GIT_PAGER &&
+	git log &&
+	test -e GIT_PAGER_used
+'
+
+test_done
-- 
1.7.0

[PATCH v4 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Jonathan Nieder]

Testing pagination requires (fake or real) access to a terminal so we
can see whether the pagination automatically kicks in, which makes it
hard to get good coverage when running tests without --verbose.  There
are a number of ways to work around that:

 - Replace all isatty calls with calls to a custom xisatty wrapper
   that usually checks for a terminal but can be overridden for tests.
   This would be workable, but it would require implementing xisatty
   separately in three languages (C, shell, and perl) and making sure
   that any code that is to be tested always uses the wrapper.

 - Redirect stdout to /dev/tty.  This would be problematic because
   there might be no terminal available, and even if a terminal is
   available, it might not be appropriate to spew output to it.

 - Create a new pseudo-terminal on the fly and capture its output.

This patch implements the third approach.

The new test-terminal.perl helper uses IPC::Open3 and IO::Pty from
Expect.pm to create a terminal and execute the program specified by
its arguments with that terminal as stdout.  If either module is
missing or not working on a system, the test script will maintain its
old behavior (skipping most of its tests unless GIT_TEST_OPTS includes
--verbose).

It also uses File::Copy, which has been part of core Perl since
version 5.002 (1996).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Here’s test-terminal on top of that.  As long as this using IO::Pty
from CPAN, there’s not much reason not to use the often-used
IPC::Open3 to simplify this.

The nice thing about using a script for this is that it fails
gracefully and doesn’t affect the makefile.

Thanks for bearing with me.

 t/t7006-pager.sh           |   35 ++++++++++++++++++++++++-----------
 t/t7006/test-terminal.perl |   28 ++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 11 deletions(-)
 create mode 100755 t/t7006/test-terminal.perl

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 4f52ea5..da0f962 100644
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -5,18 +5,31 @@ test_description='Test automatic use of a pager.'
 . ./test-lib.sh
 
 rm -f stdout_is_tty
-test_expect_success 'is stdout a terminal?' '
+test_expect_success 'set up terminal for tests' '
 	if test -t 1
 	then
 		: > stdout_is_tty
+	elif
+		test_have_prereq PERL &&
+		"$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl \
+			sh -c "test -t 1"
+	then
+		: > test_terminal_works
 	fi
 '
 
 if test -e stdout_is_tty
 then
+	test_terminal() { "$@"; }
+	test_set_prereq TTY
+elif test -e test_terminal_works
+then
+	test_terminal() {
+		"$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl "$@"
+	}
 	test_set_prereq TTY
 else
-	say stdout is not a terminal, so skipping some tests.
+	say no usable terminal, so skipping some tests
 fi
 
 unset GIT_PAGER GIT_PAGER_IN_USE
@@ -30,13 +43,13 @@ test_expect_success 'setup' '
 
 rm -f paginated.out
 test_expect_success TTY 'some commands use a pager' '
-	git log &&
+	test_terminal git log &&
 	test -e paginated.out
 '
 
 rm -f paginated.out
 test_expect_success TTY 'some commands do not use a pager' '
-	git rev-list HEAD &&
+	test_terminal git rev-list HEAD &&
 	! test -e paginated.out
 '
 
@@ -54,7 +67,7 @@ test_expect_success 'no pager when stdout is a regular file' '
 
 rm -f paginated.out
 test_expect_success TTY 'git --paginate rev-list uses a pager' '
-	git --paginate rev-list HEAD  &&
+	test_terminal git --paginate rev-list HEAD &&
 	test -e paginated.out
 '
 
@@ -66,7 +79,7 @@ test_expect_success 'no pager even with --paginate when stdout is a pipe' '
 
 rm -f paginated.out
 test_expect_success TTY 'no pager with --no-pager' '
-	git --no-pager log &&
+	test_terminal git --no-pager log &&
 	! test -e paginated.out
 '
 
@@ -95,7 +108,7 @@ test_expect_success 'no color when stdout is a regular file' '
 rm -f paginated.out
 git config color.ui auto
 test_expect_success TTY 'color when writing to a pager' '
-	TERM=vt100 git log &&
+	TERM=vt100 test_terminal git log &&
 	colorful paginated.out
 '
 
@@ -127,7 +140,7 @@ test_expect_success SIMPLEPAGER 'default pager is used by default' '
 	: > default_pager_used
 	EOF
 	chmod +x $less &&
-	PATH=.:$PATH git log &&
+	PATH=.:$PATH test_terminal git log &&
 	test -e default_pager_used
 '
 
@@ -137,7 +150,7 @@ rm -f PAGER_used
 test_expect_success TTY 'PAGER overrides default pager' '
 	PAGER=": > PAGER_used" &&
 	export PAGER &&
-	git log &&
+	test_terminal git log &&
 	test -e PAGER_used
 '
 
@@ -147,7 +160,7 @@ test_expect_success TTY 'core.pager overrides PAGER' '
 	PAGER=: &&
 	export PAGER &&
 	git config core.pager ": > core.pager_used" &&
-	git log &&
+	test_terminal git log &&
 	test -e core.pager_used
 '
 
@@ -156,7 +169,7 @@ test_expect_success TTY 'GIT_PAGER overrides core.pager' '
 	git config core.pager : &&
 	GIT_PAGER=": > GIT_PAGER_used" &&
 	export GIT_PAGER &&
-	git log &&
+	test_terminal git log &&
 	test -e GIT_PAGER_used
 '
 
diff --git a/t/t7006/test-terminal.perl b/t/t7006/test-terminal.perl
new file mode 100755
index 0000000..fc3fbb4
--- /dev/null
+++ b/t/t7006/test-terminal.perl
@@ -0,0 +1,28 @@
+#!/usr/bin/perl
+use strict;
+use warnings;
+use IO::Pty;
+use File::Copy;
+use IPC::Open3;
+
+# Wait for $_[0] to finish.
+sub finish_child {
+	# Simplified from wait_or_whine() in run-command.c.
+	waitpid($_[0], 0) == $_[0] or die "waitpid failed: $!";
+	if ($? & 127) {
+		my $code = $? & 127;
+		warn "died of signal $code";
+		return $code - 128;
+	}
+	return $? >> 8;
+}
+
+@ARGV >= 1 or die "usage: test-terminal program args";
+my $master = new IO::Pty;
+my $slave = $master->slave;
+my $pid = open3(\*STDIN, '>&' . fileno($slave), \*STDERR, @ARGV);
+close $slave;
+# Reads from a disconnected terminal may return EIO or end-of-file.
+# Either is fine.
+copy($master, \*STDOUT, 4096) or $!{EIO} or die "cannot copy from child: $!";
+exit(finish_child($pid));
-- 
1.7.0

Re: [PATCH v4 7/7] t7006-pager: if stdout is not a terminal, make a new one

[Jeff King]

On Sat, Feb 20, 2010 at 08:09:22PM -0600, Jonathan Nieder wrote:

> Testing pagination requires (fake or real) access to a terminal so we
> can see whether the pagination automatically kicks in, which makes it
> hard to get good coverage when running tests without --verbose.  There
> are a number of ways to work around that:
> 
>  - Replace all isatty calls with calls to a custom xisatty wrapper
>    that usually checks for a terminal but can be overridden for tests.
>    This would be workable, but it would require implementing xisatty
>    separately in three languages (C, shell, and perl) and making sure
>    that any code that is to be tested always uses the wrapper.
> 
>  - Redirect stdout to /dev/tty.  This would be problematic because
>    there might be no terminal available, and even if a terminal is
>    available, it might not be appropriate to spew output to it.
> 
>  - Create a new pseudo-terminal on the fly and capture its output.
> 
> This patch implements the third approach.

Just to wrap up my end of this patch discussion, I think the approach
you take here is the sanest one. While it would be nice to get test
coverage on every system, I don't think it is worth the effort of
trying to write portable terminal creation code. And this way at least
the code in git is fairly minimal.

So looks good to me.

-Peff

Re: [PATCH v2 6/7] tests: Add tests for automatic use of pager

[Johannes Sixt]

Don't the following pager settings suffer from a race condition?

> +	cat > $less <<-EOF &&
> +	#!$SHELL_PATH
> +	: > default_pager_used
> +	EOF

> +	PAGER=": > PAGER_used" &&

> +	PAGER=: &&

> +	git config core.pager ": > core.pager_used" &&

> +	git config core.pager : &&
> +	GIT_PAGER=": > GIT_PAGER_used" &&

They depend on that the subsequent 'git log' writes to the pipe (and does
not fill it) before the pager can run. If the pager runs first and
completes, and then 'git log' writes to the pipe, it will die from
SIGPIPE, won't it?

I suggest to rewrite all ':' to something that processes stdin, eg. 'wc'
(not 'cat', because this is a magic token).

-- Hannes

[PATCH 8/7] tests: Fix race condition in t7006-pager

[Jonathan Nieder]

Pagers that do not consume their input are dangerous: for example,

 $ GIT_PAGER=: git log
 $ echo $?
 141
 $

The only reason these tests were able to work before was that
'git log' would write to the pipe (and not fill it) before the
pager had time to terminate and close the pipe.

Fix it by using a program that consumes its input, namely wc (as
suggested by Johannes).

Reported-by: Johannes Sixt <j.sixt@viscovery.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Johannes Sixt wrote:

> Don't the following pager settings suffer from a race condition?

Yes.  Thanks for noticing.

 t/t7006-pager.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index da0f962..ec6fd06 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -137,7 +137,7 @@ rm -f default_pager_used
 test_expect_success SIMPLEPAGER 'default pager is used by default' '
 	cat > $less <<-EOF &&
 	#!$SHELL_PATH
-	: > default_pager_used
+	wc > default_pager_used
 	EOF
 	chmod +x $less &&
 	PATH=.:$PATH test_terminal git log &&
@@ -148,7 +148,7 @@ unset GIT_PAGER
 git config --unset core.pager
 rm -f PAGER_used
 test_expect_success TTY 'PAGER overrides default pager' '
-	PAGER=": > PAGER_used" &&
+	PAGER="wc > PAGER_used" &&
 	export PAGER &&
 	test_terminal git log &&
 	test -e PAGER_used
@@ -159,7 +159,7 @@ rm -f core.pager_used
 test_expect_success TTY 'core.pager overrides PAGER' '
 	PAGER=: &&
 	export PAGER &&
-	git config core.pager ": > core.pager_used" &&
+	git config core.pager "wc > core.pager_used" &&
 	test_terminal git log &&
 	test -e core.pager_used
 '
@@ -167,7 +167,7 @@ test_expect_success TTY 'core.pager overrides PAGER' '
 rm -f GIT_PAGER_used
 test_expect_success TTY 'GIT_PAGER overrides core.pager' '
 	git config core.pager : &&
-	GIT_PAGER=": > GIT_PAGER_used" &&
+	GIT_PAGER="wc > GIT_PAGER_used" &&
 	export GIT_PAGER &&
 	test_terminal git log &&
 	test -e GIT_PAGER_used
-- 
1.7.0

Re: [PATCH 8/7] tests: Fix race condition in t7006-pager

[Jonathan Nieder]

Jonathan Nieder wrote:
> Johannes Sixt wrote:
> 
> > Don't the following pager settings suffer from a race condition?
> 
> Yes.  Thanks for noticing.

And to save the reader time wondering: yes, the following examples
suffer from the same race, but if they are used then we _want_ the
test to fail.  It might make sense to squash them in anyway to make
the patch more self-explanatory.

There is also an instance of PAGER='cat > paginated.out'; since the
pager string only starts with but does not equal "cat", this works
fine.  It needed for test ‘12: color when writing to a pager’ to be
able to read the paginated output.

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index ec6fd06..d9202d5 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -157,7 +157,7 @@ test_expect_success TTY 'PAGER overrides default pager' '
 unset GIT_PAGER
 rm -f core.pager_used
 test_expect_success TTY 'core.pager overrides PAGER' '
-	PAGER=: &&
+	PAGER=wc &&
 	export PAGER &&
 	git config core.pager "wc > core.pager_used" &&
 	test_terminal git log &&
@@ -166,7 +166,7 @@ test_expect_success TTY 'core.pager overrides PAGER' '
 
 rm -f GIT_PAGER_used
 test_expect_success TTY 'GIT_PAGER overrides core.pager' '
-	git config core.pager : &&
+	git config core.pager wc &&
 	GIT_PAGER="wc > GIT_PAGER_used" &&
 	export GIT_PAGER &&
 	test_terminal git log &&