threads / discuss / 22646
Subject: 'git svn log' no longer uses the pager
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
Jeff KingRe: 'git svn log' no longer uses the pager
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
Jonathan NiederRe: 'git svn log' no longer uses the pager
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
Jonathan Nieder[PATCH 1/6] Fix 'git var' usage synopsis
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
Junio C HamanoRe: [PATCH 1/6] Fix 'git var' usage synopsis
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.
Jonathan Nieder[PATCH v2 1/6] Fix 'git var' usage synopsis
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
Jeff KingRe: [PATCH v2 1/6] Fix 'git var' usage synopsis
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
Jonathan Nieder[PATCH 2/6] Make 'git var GIT_PAGER' always print the configured pager
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
Jeff KingRe: [PATCH 2/6] Make 'git var GIT_PAGER' always print the configured pager
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
Jonathan NiederRe: [PATCH 2/6] Make 'git var GIT_PAGER' always print the configured pager
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
Jeff KingRe: [PATCH 2/6] Make 'git var GIT_PAGER' always print the configured pager
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
Jonathan Nieder[PATCH 3/6] git.1: Clarify the behavior of the --paginate option
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
Jeff KingRe: [PATCH 3/6] git.1: Clarify the behavior of the --paginate option
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
Jonathan Nieder[PATCH 4/6] git svn: Fix launching of pager
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
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
Jonathan Nieder[PATCH 5/6] am: Fix launching of pager
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
Junio C HamanoRe: [PATCH 5/6] am: Fix launching of pager
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>
Jonathan Nieder[PATCH v2 5/6] am: Fix launching of pager
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
Jonathan Nieder[PATCH v3 5/6] am: Fix launching of pager
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
Jeff KingRe: [PATCH v3 5/6] am: Fix launching of pager
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/nullWe 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
Jonathan Nieder[PATCH v4 5/6] am: Fix launching of pager
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
Jeff KingRe: [PATCH v4 5/6] am: Fix launching of pager
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
Johannes SixtRe: [PATCH v4 5/6] am: Fix launching of pager
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
Jonathan Nieder[PATCH 6/6] tests: Add tests for automatic use of pager
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
Jeff KingRe: [PATCH 6/6] tests: Add tests for automatic use of pager
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
Jonathan Nieder[PATCH v2 6/6] tests: Add tests for automatic use of pager
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
Jeff KingRe: [PATCH v2 6/6] tests: Add tests for automatic use of pager
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