* [PATCH v2 0/7] Re: 'git svn log' no longer uses the pager @ 2010-02-19 6:50 Jonathan Nieder 2010-02-19 6:51 ` [PATCH 1/7] Fix 'git var' usage synopsis Jonathan Nieder ` (6 more replies) 0 siblings, 7 replies; 26+ messages in thread From: Jonathan Nieder @ 2010-02-19 6:50 UTC (permalink / raw) To: git; +Cc: Sebastian Celis, Jeff King, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/7] Fix 'git var' usage synopsis 2010-02-19 6:50 [PATCH v2 0/7] Re: 'git svn log' no longer uses the pager Jonathan Nieder @ 2010-02-19 6:51 ` Jonathan Nieder 2010-02-19 7:00 ` [PATCH 2/7] Make 'git var GIT_PAGER' always print the configured pager Jonathan Nieder ` (5 subsequent siblings) 6 siblings, 0 replies; 26+ messages in thread From: Jonathan Nieder @ 2010-02-19 6:51 UTC (permalink / raw) To: git; +Cc: Sebastian Celis, Jeff King, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/7] Make 'git var GIT_PAGER' always print the configured pager 2010-02-19 6:50 [PATCH v2 0/7] Re: 'git svn log' no longer uses the pager Jonathan Nieder 2010-02-19 6:51 ` [PATCH 1/7] Fix 'git var' usage synopsis Jonathan Nieder @ 2010-02-19 7:00 ` Jonathan Nieder 2010-02-19 7:06 ` [PATCH 3/7] git.1: Clarify the behavior of the --paginate option Jonathan Nieder ` (4 subsequent siblings) 6 siblings, 0 replies; 26+ messages in thread From: Jonathan Nieder @ 2010-02-19 7:00 UTC (permalink / raw) To: git; +Cc: Sebastian Celis, Jeff King, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/7] git.1: Clarify the behavior of the --paginate option 2010-02-19 6:50 [PATCH v2 0/7] Re: 'git svn log' no longer uses the pager Jonathan Nieder 2010-02-19 6:51 ` [PATCH 1/7] Fix 'git var' usage synopsis Jonathan Nieder 2010-02-19 7:00 ` [PATCH 2/7] Make 'git var GIT_PAGER' always print the configured pager Jonathan Nieder @ 2010-02-19 7:06 ` Jonathan Nieder 2010-02-19 7:09 ` [PATCH 4/7] git svn: Fix launching of pager Jonathan Nieder ` (3 subsequent siblings) 6 siblings, 0 replies; 26+ messages in thread From: Jonathan Nieder @ 2010-02-19 7:06 UTC (permalink / raw) To: git; +Cc: Sebastian Celis, Jeff King, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/7] git svn: Fix launching of pager 2010-02-19 6:50 [PATCH v2 0/7] Re: 'git svn log' no longer uses the pager Jonathan Nieder ` (2 preceding siblings ...) 2010-02-19 7:06 ` [PATCH 3/7] git.1: Clarify the behavior of the --paginate option Jonathan Nieder @ 2010-02-19 7:09 ` Jonathan Nieder 2010-02-19 7:12 ` [PATCH 5/7] am: " Jonathan Nieder ` (2 subsequent siblings) 6 siblings, 0 replies; 26+ messages in thread From: Jonathan Nieder @ 2010-02-19 7:09 UTC (permalink / raw) To: git; +Cc: Sebastian Celis, Jeff King, Junio C Hamano, Johannes Sixt, Eric Wong 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 5/7] am: Fix launching of pager 2010-02-19 6:50 [PATCH v2 0/7] Re: 'git svn log' no longer uses the pager Jonathan Nieder ` (3 preceding siblings ...) 2010-02-19 7:09 ` [PATCH 4/7] git svn: Fix launching of pager Jonathan Nieder @ 2010-02-19 7:12 ` Jonathan Nieder 2010-02-19 7:18 ` [PATCH 6/7] tests: Add tests for automatic use " Jonathan Nieder 2010-02-19 7:23 ` [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one Jonathan Nieder 6 siblings, 0 replies; 26+ messages in thread From: Jonathan Nieder @ 2010-02-19 7:12 UTC (permalink / raw) To: git; +Cc: Sebastian Celis, Jeff King, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 6/7] tests: Add tests for automatic use of pager 2010-02-19 6:50 [PATCH v2 0/7] Re: 'git svn log' no longer uses the pager Jonathan Nieder ` (4 preceding siblings ...) 2010-02-19 7:12 ` [PATCH 5/7] am: " Jonathan Nieder @ 2010-02-19 7:18 ` Jonathan Nieder 2010-02-20 17:33 ` Junio C Hamano 2010-02-19 7:23 ` [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one Jonathan Nieder 6 siblings, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2010-02-19 7:18 UTC (permalink / raw) To: git; +Cc: Sebastian Celis, Jeff King, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 6/7] tests: Add tests for automatic use of pager 2010-02-19 7:18 ` [PATCH 6/7] tests: Add tests for automatic use " Jonathan Nieder @ 2010-02-20 17:33 ` Junio C Hamano 2010-02-21 2:03 ` [PATCH v2 " Jonathan Nieder 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2010-02-20 17:33 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git, Sebastian Celis, Jeff King, Johannes Sixt 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? ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 6/7] tests: Add tests for automatic use of pager 2010-02-20 17:33 ` Junio C Hamano @ 2010-02-21 2:03 ` Jonathan Nieder 2010-02-21 2:09 ` [PATCH v4 7/7] t7006-pager: if stdout is not a terminal, make a new one Jonathan Nieder 2010-02-22 8:19 ` [PATCH v2 6/7] tests: Add tests for automatic use of pager Johannes Sixt 0 siblings, 2 replies; 26+ messages in thread From: Jonathan Nieder @ 2010-02-21 2:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Sebastian Celis, Jeff King, Johannes Sixt 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-21 2:03 ` [PATCH v2 " Jonathan Nieder @ 2010-02-21 2:09 ` Jonathan Nieder 2010-02-21 7:30 ` Jeff King 2010-02-22 8:19 ` [PATCH v2 6/7] tests: Add tests for automatic use of pager Johannes Sixt 1 sibling, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2010-02-21 2:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Sebastian Celis, Jeff King, Johannes Sixt 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-21 2:09 ` [PATCH v4 7/7] t7006-pager: if stdout is not a terminal, make a new one Jonathan Nieder @ 2010-02-21 7:30 ` Jeff King 0 siblings, 0 replies; 26+ messages in thread From: Jeff King @ 2010-02-21 7:30 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Junio C Hamano, git, Sebastian Celis, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 6/7] tests: Add tests for automatic use of pager 2010-02-21 2:03 ` [PATCH v2 " Jonathan Nieder 2010-02-21 2:09 ` [PATCH v4 7/7] t7006-pager: if stdout is not a terminal, make a new one Jonathan Nieder @ 2010-02-22 8:19 ` Johannes Sixt 2010-02-22 8:46 ` [PATCH 8/7] tests: Fix race condition in t7006-pager Jonathan Nieder 1 sibling, 1 reply; 26+ messages in thread From: Johannes Sixt @ 2010-02-22 8:19 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Junio C Hamano, git, Sebastian Celis, Jeff King 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 8/7] tests: Fix race condition in t7006-pager 2010-02-22 8:19 ` [PATCH v2 6/7] tests: Add tests for automatic use of pager Johannes Sixt @ 2010-02-22 8:46 ` Jonathan Nieder 2010-02-22 9:12 ` Jonathan Nieder 0 siblings, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2010-02-22 8:46 UTC (permalink / raw) To: Johannes Sixt; +Cc: Junio C Hamano, git, Sebastian Celis, Jeff King 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 8/7] tests: Fix race condition in t7006-pager 2010-02-22 8:46 ` [PATCH 8/7] tests: Fix race condition in t7006-pager Jonathan Nieder @ 2010-02-22 9:12 ` Jonathan Nieder 0 siblings, 0 replies; 26+ messages in thread From: Jonathan Nieder @ 2010-02-22 9:12 UTC (permalink / raw) To: Johannes Sixt; +Cc: Junio C Hamano, git, Sebastian Celis, Jeff King 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 && ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-19 6:50 [PATCH v2 0/7] Re: 'git svn log' no longer uses the pager Jonathan Nieder ` (5 preceding siblings ...) 2010-02-19 7:18 ` [PATCH 6/7] tests: Add tests for automatic use " Jonathan Nieder @ 2010-02-19 7:23 ` Jonathan Nieder 2010-02-19 8:08 ` Jeff King 6 siblings, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2010-02-19 7:23 UTC (permalink / raw) To: git; +Cc: Sebastian Celis, Jeff King, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-19 7:23 ` [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one Jonathan Nieder @ 2010-02-19 8:08 ` Jeff King 2010-02-19 8:19 ` Jonathan Nieder 0 siblings, 1 reply; 26+ messages in thread From: Jeff King @ 2010-02-19 8:08 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git, Sebastian Celis, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-19 8:08 ` Jeff King @ 2010-02-19 8:19 ` Jonathan Nieder 2010-02-19 8:34 ` Jeff King 0 siblings, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2010-02-19 8:19 UTC (permalink / raw) To: Jeff King; +Cc: git, Sebastian Celis, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-19 8:19 ` Jonathan Nieder @ 2010-02-19 8:34 ` Jeff King 2010-02-19 16:25 ` Brandon Casey 0 siblings, 1 reply; 26+ messages in thread From: Jeff King @ 2010-02-19 8:34 UTC (permalink / raw) To: Jonathan Nieder Cc: Brandon Casey, git, Sebastian Celis, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-19 8:34 ` Jeff King @ 2010-02-19 16:25 ` Brandon Casey 2010-02-20 0:29 ` Brandon Casey 0 siblings, 1 reply; 26+ messages in thread From: Brandon Casey @ 2010-02-19 16:25 UTC (permalink / raw) To: Jeff King Cc: Jonathan Nieder, Brandon Casey, git, Sebastian Celis, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-19 16:25 ` Brandon Casey @ 2010-02-20 0:29 ` Brandon Casey 2010-02-20 0:39 ` Jonathan Nieder 0 siblings, 1 reply; 26+ messages in thread From: Brandon Casey @ 2010-02-20 0:29 UTC (permalink / raw) To: Jeff King Cc: Jonathan Nieder, Brandon Casey, git, Sebastian Celis, Junio C Hamano, Johannes Sixt >> 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-20 0:29 ` Brandon Casey @ 2010-02-20 0:39 ` Jonathan Nieder 2010-02-20 3:42 ` Brandon Casey 0 siblings, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2010-02-20 0:39 UTC (permalink / raw) To: Brandon Casey Cc: Jeff King, git, Sebastian Celis, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-20 0:39 ` Jonathan Nieder @ 2010-02-20 3:42 ` Brandon Casey 2010-02-20 5:25 ` [PATCH v2 " Jonathan Nieder 0 siblings, 1 reply; 26+ messages in thread From: Brandon Casey @ 2010-02-20 3:42 UTC (permalink / raw) To: Jonathan Nieder Cc: Jeff King, git, Sebastian Celis, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-20 3:42 ` Brandon Casey @ 2010-02-20 5:25 ` Jonathan Nieder 2010-02-20 6:53 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2010-02-20 5:25 UTC (permalink / raw) To: Brandon Casey Cc: Jeff King, git, Sebastian Celis, Junio C Hamano, Johannes Sixt 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-20 5:25 ` [PATCH v2 " Jonathan Nieder @ 2010-02-20 6:53 ` Junio C Hamano 2010-02-20 8:50 ` [PATCH v3 " Jonathan Nieder 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2010-02-20 6:53 UTC (permalink / raw) To: Jonathan Nieder Cc: Brandon Casey, Jeff King, git, Sebastian Celis, Junio C Hamano, Johannes Sixt 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 7/7] t7006-pager: if stdout is not a terminal, make a new one 2010-02-20 6:53 ` Junio C Hamano @ 2010-02-20 8:50 ` Jonathan Nieder 2010-02-20 9:48 ` [PATCH squash] Simplify test-terminal.perl Jonathan Nieder 0 siblings, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2010-02-20 8:50 UTC (permalink / raw) To: Junio C Hamano Cc: Brandon Casey, Jeff King, git, Sebastian Celis, Johannes Sixt 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH squash] Simplify test-terminal.perl 2010-02-20 8:50 ` [PATCH v3 " Jonathan Nieder @ 2010-02-20 9:48 ` Jonathan Nieder 0 siblings, 0 replies; 26+ messages in thread From: Jonathan Nieder @ 2010-02-20 9:48 UTC (permalink / raw) To: Junio C Hamano Cc: Brandon Casey, Jeff King, git, Sebastian Celis, Johannes Sixt 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
end of thread, other threads:[~2010-02-22 9:12 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-02-19 6:50 [PATCH v2 0/7] Re: 'git svn log' no longer uses the pager Jonathan Nieder 2010-02-19 6:51 ` [PATCH 1/7] Fix 'git var' usage synopsis Jonathan Nieder 2010-02-19 7:00 ` [PATCH 2/7] Make 'git var GIT_PAGER' always print the configured pager Jonathan Nieder 2010-02-19 7:06 ` [PATCH 3/7] git.1: Clarify the behavior of the --paginate option Jonathan Nieder 2010-02-19 7:09 ` [PATCH 4/7] git svn: Fix launching of pager Jonathan Nieder 2010-02-19 7:12 ` [PATCH 5/7] am: " Jonathan Nieder 2010-02-19 7:18 ` [PATCH 6/7] tests: Add tests for automatic use " Jonathan Nieder 2010-02-20 17:33 ` Junio C Hamano 2010-02-21 2:03 ` [PATCH v2 " Jonathan Nieder 2010-02-21 2:09 ` [PATCH v4 7/7] t7006-pager: if stdout is not a terminal, make a new one Jonathan Nieder 2010-02-21 7:30 ` Jeff King 2010-02-22 8:19 ` [PATCH v2 6/7] tests: Add tests for automatic use of pager Johannes Sixt 2010-02-22 8:46 ` [PATCH 8/7] tests: Fix race condition in t7006-pager Jonathan Nieder 2010-02-22 9:12 ` Jonathan Nieder 2010-02-19 7:23 ` [PATCH/RFC 7/7] t7006-pager: if stdout is not a terminal, make a new one Jonathan Nieder 2010-02-19 8:08 ` Jeff King 2010-02-19 8:19 ` Jonathan Nieder 2010-02-19 8:34 ` Jeff King 2010-02-19 16:25 ` Brandon Casey 2010-02-20 0:29 ` Brandon Casey 2010-02-20 0:39 ` Jonathan Nieder 2010-02-20 3:42 ` Brandon Casey 2010-02-20 5:25 ` [PATCH v2 " Jonathan Nieder 2010-02-20 6:53 ` Junio C Hamano 2010-02-20 8:50 ` [PATCH v3 " Jonathan Nieder 2010-02-20 9:48 ` [PATCH squash] Simplify test-terminal.perl Jonathan Nieder
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 NNTP newsgroup(s).