'git svn log' no longer uses the pager

'git svn log' no longer uses the pager

[Sebastian Celis]

Hello,

Ever since upgrading to git 1.6.6.1 I have noticed that 'git svn log'
no longer uses the pager. It definitely used to in git 1.6.5.X, but it
no longer does.

Is this a recent bug that was introduced? Or was this changed on
purpose? I definitely preferred the old behavior.

Thank you,

Sebastian

Re: 'git svn log' no longer uses the pager

[Jeff King]

On Sat, Feb 13, 2010 at 03:14:54PM -0600, Sebastian Celis wrote:

> Ever since upgrading to git 1.6.6.1 I have noticed that 'git svn log'
> no longer uses the pager. It definitely used to in git 1.6.5.X, but it
> no longer does.
> 
> Is this a recent bug that was introduced? Or was this changed on
> purpose? I definitely preferred the old behavior.

I think it's a bug. It bisects to Jonathan's dec543e (am -i, git-svn:
use "git var GIT_PAGER", 2009-10-30). But it seems to me that "git var
GIT_PAGER" is fundamentally broken. It calls git_pager, which in turns
checks isatty(1). But of course stdout _isn't_ a tty, because we are
piping it back to the perl process to read the result of "git var".
In other words, I don't see how this ever could have worked.

For git-config's colorbool support we have the caller pass in a
stdout-is-tty flag. I suspect we would need to do the same thing here.

-Peff

Re: 'git svn log' no longer uses the pager

[Jonathan Nieder]

Jeff King wrote:
> On Sat, Feb 13, 2010 at 03:14:54PM -0600, Sebastian Celis wrote:

>> Ever since upgrading to git 1.6.6.1 I have noticed that 'git svn log'
>> no longer uses the pager. It definitely used to in git 1.6.5.X, but it
>> no longer does.
[...]
> I think it's a bug. It bisects to Jonathan's dec543e (am -i, git-svn:
> use "git var GIT_PAGER", 2009-10-30). But it seems to me that "git var
> GIT_PAGER" is fundamentally broken.

Yikes, you’re right.  Sorry about that.

> For git-config's colorbool support we have the caller pass in a
> stdout-is-tty flag. I suspect we would need to do the same thing here.

Thanks for the helpful reference.  I think it is simpler for the caller
to disable the pager himself, i.e., something like patches 2, 4, and 5
below:

Jonathan Nieder (6):
  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

 Documentation/git-var.txt |    2 +-
 Documentation/git.txt     |    8 ++-
 builtin-var.c             |    4 +-
 cache.h                   |    2 +-
 git-am.sh                 |    5 +-
 git-sh-setup.sh           |   16 +++++
 git-svn.perl              |    9 ++-
 pager.c                   |    6 +-
 t/t7006-pager.sh          |  163 +++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 200 insertions(+), 15 deletions(-)
 create mode 100644 t/t7006-pager.sh

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

[Jonathan Nieder]

The parameter to 'git var' is not optional.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Noticed while considering changing the usage.

 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

Re: [PATCH 1/6] Fix 'git var' usage synopsis

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The parameter to 'git var' is not optional.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Noticed while considering changing the usage.

And the patch does not even apply.  Sigh.

[PATCH v2 1/6] Fix 'git var' usage synopsis

[Jonathan Nieder]

Jonathan Nieder wrote:

> --- 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)
>  {

As Junio noticed, this doesn’t apply.  The problem is a missing
blank line between the static const char var_usage and static const
char *editor lines.  Worse still, I have no clue how it happened.

Here’s a revised patch (generated just like the first one...).  It
is really an independent fix from the rest of the series.

-- %< --
Subject: Fix 'git var' usage synopsis

The parameter to 'git var' is not optional.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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

Re: [PATCH v2 1/6] Fix 'git var' usage synopsis

[Jeff King]

On Sun, Feb 14, 2010 at 08:36:36PM -0600, Jonathan Nieder wrote:

> > --- 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)
> >  {
> 
> As Junio noticed, this doesn’t apply.  The problem is a missing
> blank line between the static const char var_usage and static const
> char *editor lines.  Worse still, I have no clue how it happened.

I think it was a stray deletion in your editor. The patch is actually
corrupt (the hunk claims 7 lines, but contains only 6). And you put text
after the "---", so presumably you did so with an editor. So I think it
is probably not a bug, but just a weird once-off user error.

Or maybe cosmic rays ate your line feed.

-Peff

[PATCH 2/6] 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 the callers to check isatty(1) themselves yet.
Nevertheless, this patch alone is enough to fix 'am --interactive'.

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>
---
 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

Re: [PATCH 2/6] Make 'git var GIT_PAGER' always print the configured pager

[Jeff King]

On Sun, Feb 14, 2010 at 05:59:59AM -0600, Jonathan Nieder wrote:

> 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.

I think this is the right thing to do. But isn't "git am" broken in the
meantime, as it now always paginates (whereas before, it would never
paginate)? You fix it later in the series, but is there any test
breakage in the meantime (not rhetorical, I didn't actually check) that
would hurt bisectability?

The same would go for "git svn", except that its pagination was also
broken in other ways. :)

-Peff

Re: [PATCH 2/6] Make 'git var GIT_PAGER' always print the configured pager

[Jonathan Nieder]

On Mon, Feb 15, 2010 at 12:00:07AM -0500, Jeff King wrote:
> On Sun, Feb 14, 2010 at 05:59:59AM -0600, Jonathan Nieder wrote:

>> So avoid tricks with isatty() and just always print the configured
>> pager.
>
> I think this is the right thing to do. But isn't "git am" broken in the
> meantime, as it now always paginates (whereas before, it would never
> paginate)? You fix it later in the series, but is there any test
> breakage in the meantime (not rhetorical, I didn't actually check) that
> would hurt bisectability?

The behavior before dec543 (am -i, git-svn: use "git var GIT_PAGER",
2009-10-30) was to always paginate.  This made some sense, since this
is the (v)iew command in git am --interactive; whether we check or
not, presumably stdout is a terminal.  So for git am, this restores
the older behavior.

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

Thank you for your attention to detail.
Jonathan

Re: [PATCH 2/6] Make 'git var GIT_PAGER' always print the configured pager

[Jeff King]

On Sun, Feb 14, 2010 at 11:44:47PM -0600, Jonathan Nieder wrote:

> > I think this is the right thing to do. But isn't "git am" broken in the
> > meantime, as it now always paginates (whereas before, it would never
> > paginate)? You fix it later in the series, but is there any test
> > breakage in the meantime (not rhetorical, I didn't actually check) that
> > would hurt bisectability?
> 
> The behavior before dec543 (am -i, git-svn: use "git var GIT_PAGER",
> 2009-10-30) was to always paginate.  This made some sense, since this
> is the (v)iew command in git am --interactive; whether we check or
> not, presumably stdout is a terminal.  So for git am, this restores
> the older behavior.
> 
> There are unfortunately no tests for am --interactive in the test
> suite, so test suite runs would detect none of this.
> 
> Thank you for your attention to detail.

Makes sense. Thank _you_ for patches and for checking on my concerns.

-Peff

[PATCH 3/6] 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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Noticed while testing.  (It might be nice to have an --always-paginate
facility to allow debugging by writing output from a pager into
something that is not a terminal.)

 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

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

[Jeff King]

On Sun, Feb 14, 2010 at 06:02:35AM -0600, Jonathan Nieder wrote:

> 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.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Noticed while testing.  (It might be nice to have an --always-paginate
> facility to allow debugging by writing output from a pager into
> something that is not a terminal.)

Hmm. I would have expected --pager to have the "always" effect, though I
will admit I never actually use it (--no-pager, on the other hand...).
But if nobody is complaining, I certainly don't want to change the
behavior haphazardly, and documenting it makes sense. Thanks.

> 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).

The "Configuration Mechanism" section doesn't actually contain anything
related. I guess you were going for "point to that section, and then it
will point to the list of config", which explains this:

> @@ -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.

which seemed at first to be unrelated (but I think is a good change
regardless).

It seems like a long jump for somebody who doesn't know what pager.*
does to follow, but I guess it doesn't hurt to try to provide pointers.
I have long since forgotten what it is like to not know that
"git-config" contains the list of options. :)

-Peff

[PATCH 4/6] 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>
---
Oops.  Eric, this bug is masked by another bug in "git var" whose fix
is patch 2 from this series I sent to the list, but whatever happens,
I think this is an improvement.  Sorry for the breakage.

 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

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

[Eric Wong]

Jonathan Nieder <jrnieder@gmail.com> wrote:
> 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>
> ---
> Oops.  Eric, this bug is masked by another bug in "git var" whose fix
> is patch 2 from this series I sent to the list, but whatever happens,
> I think this is an improvement.  Sorry for the breakage.

Thanks Jonathan, confirmed this works with patch 2 of this series.

Acked-by: Eric Wong <normalperson@yhbt.net>

-- 
Eric Wong

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

[Jonathan Nieder]

The git_pager() function used 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.

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.

If GIT_PAGER is set to the empty string, pagination is disabled.  This
is different from the treatment of GIT_EDITOR by git_editor(), but
consistency with the use of GIT_PAGER by git builtins is more
important.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-am.sh       |    5 +----
 git-sh-setup.sh |   16 ++++++++++++++++
 2 files changed, 17 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..f3b5237 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -107,6 +107,22 @@ git_editor() {
 	eval "$GIT_EDITOR" '"$@"'
 }
 
+git_pager() {
+	if test -z "${GIT_PAGER+set}"
+	then
+		if tty <&1 >/dev/null 2>/dev/null
+		then
+			GIT_PAGER=cat
+		else
+			GIT_PAGER=$(git var GIT_PAGER)
+		fi
+	fi
+	: ${LESS=-FRSX}
+	export LESS
+
+	eval "$GIT_PAGER" '"$@"'
+}
+
 sane_grep () {
 	GREP_OPTIONS= LC_ALL=C grep "$@"
 }
-- 
1.7.0

Re: [PATCH 5/6] am: Fix launching of pager

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index d56426d..f3b5237 100755
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -107,6 +107,22 @@ git_editor() {
>  	eval "$GIT_EDITOR" '"$@"'
>  }
>  
> +git_pager() {
> +	if test -z "${GIT_PAGER+set}"
> +	then
> +		if tty <&1 >/dev/null 2>/dev/null
> +		then
> +			GIT_PAGER=cat
> +		else
> +			GIT_PAGER=$(git var GIT_PAGER)
> +		fi
> +	fi
> +	: ${LESS=-FRSX}
> +	export LESS
> +
> +	eval "$GIT_PAGER" '"$@"'
> +}
> +

I didn't read other patches, but I think this is a good change.

Acked-by: Junio C Hamano <gitster@pobox.com>

[PATCH v2 5/6] am: Fix launching of pager

[Jonathan Nieder]

The git_pager() function used 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.

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.

If GIT_PAGER is set to the empty string, pagination is disabled.  This
is different from the treatment of GIT_EDITOR by git_editor(), but
consistency with the use of GIT_PAGER by git builtins is more
important.

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

> +		if tty <&1 >/dev/null 2>/dev/null
> +		then
> +			GIT_PAGER=cat
> +		else
> +			GIT_PAGER=$(git var GIT_PAGER)
> +		fi

This was backwards.  *sigh*

Here’s a tested version.  Junio, I carried over your Ack, but please
feel free to revoke it.

 git-am.sh       |    5 +----
 git-sh-setup.sh |   16 ++++++++++++++++
 2 files changed, 17 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..4500c12 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -107,6 +107,22 @@ git_editor() {
 	eval "$GIT_EDITOR" '"$@"'
 }
 
+git_pager() {
+	if test -z "${GIT_PAGER+set}"
+	then
+		if tty <&1 >/dev/null 2>/dev/null
+		then
+			GIT_PAGER=$(git var GIT_PAGER)
+		else
+			GIT_PAGER=cat
+		fi
+	fi
+	: ${LESS=-FRSX}
+	export LESS
+
+	eval "$GIT_PAGER" '"$@"'
+}
+
 sane_grep () {
 	GREP_OPTIONS= LC_ALL=C grep "$@"
 }
-- 
1.7.0

[PATCH v3 5/6] am: Fix launching of pager

[Jonathan Nieder]

The (v)iew command in git am -i 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 exec the 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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
The previous version did not handle GIT_PAGER="" properly.  Third
time’s the charm, I hope.

 git-am.sh       |    5 +----
 git-sh-setup.sh |   19 +++++++++++++++++++
 2 files changed, 20 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..18ad1fc 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -107,6 +107,25 @@ git_editor() {
 	eval "$GIT_EDITOR" '"$@"'
 }
 
+git_pager() {
+	if test -z "${GIT_PAGER+set}"
+	then
+		if tty <&1 >/dev/null 2>/dev/null
+		then
+			GIT_PAGER=$(git var GIT_PAGER)
+		else
+			GIT_PAGER=cat
+		fi
+	elif test -z "$GIT_PAGER"
+	then
+		GIT_PAGER=cat
+	fi
+	: ${LESS=-FRSX}
+	export LESS
+
+	eval "$GIT_PAGER" '"$@"'
+}
+
 sane_grep () {
 	GREP_OPTIONS= LC_ALL=C grep "$@"
 }
-- 
1.7.0

Re: [PATCH v3 5/6] am: Fix launching of pager

[Jeff King]

On Sun, Feb 14, 2010 at 09:25:33PM -0600, Jonathan Nieder wrote:

> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -107,6 +107,25 @@ git_editor() {
>  	eval "$GIT_EDITOR" '"$@"'
>  }
>  
> +git_pager() {
> +	if test -z "${GIT_PAGER+set}"
> +	then
> +		if tty <&1 >/dev/null 2>/dev/null

We seem to use "test -t 1" for this elsewhere, and I don't see us using
"tty" anywhere else. It is POSIX, and I tested that even Solaris 8 and
AIX 6.1 have it. So I don't think it is a portability issue, but others
may know more than I.

-Peff

[PATCH v4 5/6] 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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jeff King wrote:
> On Sun, Feb 14, 2010 at 09:25:33PM -0600, Jonathan Nieder wrote:

>> +		if tty <&1 >/dev/null 2>/dev/null
>
> We seem to use "test -t 1" for this elsewhere, and I don't see us using
> "tty" anywhere else. It is POSIX, and I tested that even Solaris 8 and
> AIX 6.1 have it. So I don't think it is a portability issue, but others
> may know more than I.

tty is portable, but "test -t 1" is cleaner.  Thanks for the pointer.

 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

Re: [PATCH v4 5/6] am: Fix launching of pager

[Jeff King]

On Sun, Feb 14, 2010 at 11:04:13PM -0600, Jonathan Nieder wrote:

> Jeff King wrote:
> > On Sun, Feb 14, 2010 at 09:25:33PM -0600, Jonathan Nieder wrote:
> 
> >> +		if tty <&1 >/dev/null 2>/dev/null
> >
> > We seem to use "test -t 1" for this elsewhere, and I don't see us using
> > "tty" anywhere else. It is POSIX, and I tested that even Solaris 8 and
> > AIX 6.1 have it. So I don't think it is a portability issue, but others
> > may know more than I.
> 
> tty is portable, but "test -t 1" is cleaner.  Thanks for the pointer.

There is another one in the test script in patch 6/6, btw.

-Peff

Re: [PATCH v4 5/6] am: Fix launching of pager

[Johannes Sixt]

Jonathan Nieder schrieb:
> tty is portable, but "test -t 1" is cleaner.

tty is not portable enough: we don't have it on Windows. test -t 1 is fine.

Thanks.

-- Hannes

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

[Jonathan Nieder]

Git’s automatic pagination support has some subtleties; add some tests
to make sure they don’t break.

The tests touch on a few different aspects of git’s pagination
behavior:

 - 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.

They 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 require stdout to be a terminal to be testable without
a (not written) shim using pseudo-terminals.  Thus about half of these
tests are skipped unless $GIT_TEST_OPTS includes --verbose.

The immediate purpose for these tests is to document removal of most
breakage from my jn/editor-pager series (see commit 376f39,
2009-11-20).  Thanks to Sebastian Celis <sebastian@sebastiancelis.com>
for the report.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.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..5562b70
--- /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 tty <&1 >/dev/null 2>/dev/null
+	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 '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 '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 '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

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

[Jeff King]

On Sun, Feb 14, 2010 at 06:13:00AM -0600, Jonathan Nieder wrote:

> They 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 require stdout to be a terminal to be testable without
> a (not written) shim using pseudo-terminals.  Thus about half of these
> tests are skipped unless $GIT_TEST_OPTS includes --verbose.

I am a little lukewarm on tests that require --verbose, since they will
not be caught by 99% of the runs of the test suite. Which means they
sneak through things like Junio's automated testing of maint, master,
and next.

Still, perhaps something is better than nothing. I wonder if we can do
better, though, with something like:

  int xisatty(int fd)
  {
    static int fake[3];

    if (fd > ARRAY_SIZE(pretend))
      return isatty(fd);

    if (fake[fd] == -1) {
      const char *x = getenv("GIT_PRETEND_TTY");
      if (x && strchr(x, '0' + fd))
        fake[fd] = 1;
      else
        fake[fd] = isatty(fd);
    }
    return fake[fd];
  }

-Peff

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

[Jonathan Nieder]

Git’s automatic pagination support has some subtleties; add some tests
to make sure they don’t break again.

These test:

 - 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.

They 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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jeff King wrote:

> I am a little lukewarm on tests that require --verbose, since they will
> not be caught by 99% of the runs of the test suite.

Makes sense.

> Still, perhaps something is better than nothing. I wonder if we can do
> better, though, with something like:
[...]
>     if (fake[fd] == -1) {
>       const char *x = getenv("GIT_PRETEND_TTY");
>       if (x && strchr(x, '0' + fd))
>         fake[fd] = 1;
>       else
>         fake[fd] = isatty(fd);

How about this?  It seems a little simpler, though it might be less
reliable.

 t/t7006-pager.sh |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 156 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..458a754
--- /dev/null
+++ b/t/t7006-pager.sh
@@ -0,0 +1,156 @@
+#!/bin/sh
+
+test_description='Test automatic use of a pager.'
+
+. ./test-lib.sh
+
+if eval "exec 8>/dev/tty" 2>/dev/null && test -t 8
+then
+	test_set_prereq TTY
+else
+	say no usable /dev/tty, so skipping some tests.
+	exec 8>&-
+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 -0 >&8 &&
+	test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success TTY 'some commands do not use a pager' '
+	git rev-list -0 HEAD >&8 &&
+	! 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 -0 HEAD >&8 &&
+	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 -0 >&8 &&
+	! 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 >&8 &&
+	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 -0 >&8 &&
+	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 -0 >&8 &&
+	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 -0 >&8 &&
+	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 -0 >&8 &&
+	test -e GIT_PAGER_used
+'
+
+test_done
-- 
1.7.0

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

[Jeff King]

On Sun, Feb 14, 2010 at 11:35:21PM -0600, Jonathan Nieder wrote:

> How about this?  It seems a little simpler, though it might be less
> reliable.
> [...]
> +if eval "exec 8>/dev/tty" 2>/dev/null && test -t 8

Right, it still breaks if we don't have a tty at all. Which is perhaps
not the "99% of runs" I mentioned before, but I think is not all that
uncommon (e.g., cron jobs).

-Peff