{"thread":{"id":"22646","subject":"'git svn log' no longer uses the pager","startedAt":"2010-02-13T21:14:54Z","lastAt":"2010-02-15T20:26:43Z","messageCount":27,"participants":["Sebastian Celis","Jeff King","Jonathan Nieder","Junio C Hamano","Eric Wong","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"134464","messageId":"462027ff1002131314k62069160h63760fc8316aa43b@mail.gmail.com","threadId":"22646","inReplyTo":null,"subject":"'git svn log' no longer uses the pager","fromName":"Sebastian Celis","fromEmail":"sebastian@sebastiancelis.com","sentAt":"2010-02-13T21:14:54Z","receivedAt":"2010-02-13T21:14:54Z","isPatch":false,"sender":{"key":"sebastian@sebastiancelis.com","avatar":null},"body":"Hello,\n\nEver since upgrading to git 1.6.6.1 I have noticed that 'git svn log'\nno longer uses the pager. It definitely used to in git 1.6.5.X, but it\nno longer does.\n\nIs this a recent bug that was introduced? Or was this changed on\npurpose? I definitely preferred the old behavior.\n\nThank you,\n\nSebastian\n"},{"id":"134502","messageId":"20100213235156.GA9054@coredump.intra.peff.net","threadId":"22646","inReplyTo":"462027ff1002131314k62069160h63760fc8316aa43b@mail.gmail.com","subject":"Re: 'git svn log' no longer uses the pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-13T23:51:56Z","receivedAt":"2010-02-13T23:51:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 13, 2010 at 03:14:54PM -0600, Sebastian Celis wrote:\n\n> Ever since upgrading to git 1.6.6.1 I have noticed that 'git svn log'\n> no longer uses the pager. It definitely used to in git 1.6.5.X, but it\n> no longer does.\n> \n> Is this a recent bug that was introduced? Or was this changed on\n> purpose? I definitely preferred the old behavior.\n\nI think it's a bug. It bisects to Jonathan's dec543e (am -i, git-svn:\nuse \"git var GIT_PAGER\", 2009-10-30). But it seems to me that \"git var\nGIT_PAGER\" is fundamentally broken. It calls git_pager, which in turns\nchecks isatty(1). But of course stdout _isn't_ a tty, because we are\npiping it back to the perl process to read the result of \"git var\".\nIn other words, I don't see how this ever could have worked.\n\nFor git-config's colorbool support we have the caller pass in a\nstdout-is-tty flag. I suspect we would need to do the same thing here.\n\n-Peff\n"},{"id":"134531","messageId":"20100214115430.GA1849@progeny.tock","threadId":"22646","inReplyTo":"20100213235156.GA9054@coredump.intra.peff.net","subject":"Re: 'git svn log' no longer uses the pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-14T11:54:30Z","receivedAt":"2010-02-14T11:54:30Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Sat, Feb 13, 2010 at 03:14:54PM -0600, Sebastian Celis wrote:\n\n>> Ever since upgrading to git 1.6.6.1 I have noticed that 'git svn log'\n>> no longer uses the pager. It definitely used to in git 1.6.5.X, but it\n>> no longer does.\n[...]\n> I think it's a bug. It bisects to Jonathan's dec543e (am -i, git-svn:\n> use \"git var GIT_PAGER\", 2009-10-30). But it seems to me that \"git var\n> GIT_PAGER\" is fundamentally broken.\n\nYikes, you’re right.  Sorry about that.\n\n> For git-config's colorbool support we have the caller pass in a\n> stdout-is-tty flag. I suspect we would need to do the same thing here.\n\nThanks for the helpful reference.  I think it is simpler for the caller\nto disable the pager himself, i.e., something like patches 2, 4, and 5\nbelow:\n\nJonathan Nieder (6):\n  Fix 'git var' usage synopsis\n  Make 'git var GIT_PAGER' always print the configured pager\n  git.1: Clarify the behavior of the --paginate option\n  git svn: Fix launching of pager\n  am: Fix launching of pager\n  tests: Add tests for automatic use of pager\n\n Documentation/git-var.txt |    2 +-\n Documentation/git.txt     |    8 ++-\n builtin-var.c             |    4 +-\n cache.h                   |    2 +-\n git-am.sh                 |    5 +-\n git-sh-setup.sh           |   16 +++++\n git-svn.perl              |    9 ++-\n pager.c                   |    6 +-\n t/t7006-pager.sh          |  163 +++++++++++++++++++++++++++++++++++++++++++++\n 9 files changed, 200 insertions(+), 15 deletions(-)\n create mode 100644 t/t7006-pager.sh\n"},{"id":"134533","messageId":"20100214115533.GA3499@progeny.tock","threadId":"22646","inReplyTo":"20100214115430.GA1849@progeny.tock","subject":"[PATCH 1/6] Fix 'git var' usage synopsis","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-14T11:55:53Z","receivedAt":"2010-02-14T11:55:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The parameter to 'git var' is not optional.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nNoticed while considering changing the usage.\n\n Documentation/git-var.txt |    2 +-\n builtin-var.c             |    2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-var.txt b/Documentation/git-var.txt\nindex bb98182..458f3e2 100644\n--- a/Documentation/git-var.txt\n+++ b/Documentation/git-var.txt\n@@ -8,7 +8,7 @@ git-var - Show a git logical variable\n \n SYNOPSIS\n --------\n-'git var' [ -l | <variable> ]\n+'git var' ( -l | <variable> )\n \n DESCRIPTION\n -----------\ndiff --git a/builtin-var.c b/builtin-var.c\nindex 2280518..e6ee7bc 100644\n--- a/builtin-var.c\n+++ b/builtin-var.c\n@@ -6,7 +6,7 @@\n #include \"cache.h\"\n #include \"exec_cmd.h\"\n \n-static const char var_usage[] = \"git var [-l | <variable>]\";\n+static const char var_usage[] = \"git var (-l | <variable>)\";\n static const char *editor(int flag)\n {\n-- \n1.7.0\n"},{"id":"134534","messageId":"20100214115959.GB3499@progeny.tock","threadId":"22646","inReplyTo":"20100214115430.GA1849@progeny.tock","subject":"[PATCH 2/6] Make 'git var GIT_PAGER' always print the configured pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-14T11:59:59Z","receivedAt":"2010-02-14T11:59:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Scripted commands that want to use git’s configured pager know better\nthan ‘git var’ does whether stdout is going to be a tty at the\nappropriate time.  Checking isatty(1) as git_pager() does now won’t\ncut it, since the output of git var itself is almost never a terminal.\nThe symptom is that when used by humans, ‘git var GIT_PAGER’ behaves\nas it should, but when used by scripts, it always returns ‘cat’!\n\nSo avoid tricks with isatty() and just always print the configured\npager.\n\nThis does not fix the callers to check isatty(1) themselves yet.\nNevertheless, this patch alone is enough to fix 'am --interactive'.\n\nThanks to Sebastian Celis for the report and Jeff King for the\nanalysis.\n\nReported-by: Sebastian Celis <sebastian@sebastiancelis.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n builtin-var.c |    2 +-\n cache.h       |    2 +-\n pager.c       |    6 +++---\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-var.c b/builtin-var.c\nindex e6ee7bc..70fdb4d 100644\n--- a/builtin-var.c\n+++ b/builtin-var.c\n@@ -20,7 +20,7 @@ static const char *editor(int flag)\n \n static const char *pager(int flag)\n {\n-\tconst char *pgm = git_pager();\n+\tconst char *pgm = git_pager(1);\n \n \tif (!pgm)\n \t\tpgm = \"cat\";\ndiff --git a/cache.h b/cache.h\nindex d478eff..d454b7e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -775,7 +775,7 @@ extern const char *git_committer_info(int);\n extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);\n extern const char *fmt_name(const char *name, const char *email);\n extern const char *git_editor(void);\n-extern const char *git_pager(void);\n+extern const char *git_pager(int stdout_is_tty);\n \n struct checkout {\n \tconst char *base_dir;\ndiff --git a/pager.c b/pager.c\nindex 2c7e8ec..dac358f 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -48,11 +48,11 @@ static void wait_for_pager_signal(int signo)\n \traise(signo);\n }\n \n-const char *git_pager(void)\n+const char *git_pager(int stdout_is_tty)\n {\n \tconst char *pager;\n \n-\tif (!isatty(1))\n+\tif (!stdout_is_tty)\n \t\treturn NULL;\n \n \tpager = getenv(\"GIT_PAGER\");\n@@ -73,7 +73,7 @@ const char *git_pager(void)\n \n void setup_pager(void)\n {\n-\tconst char *pager = git_pager();\n+\tconst char *pager = git_pager(isatty(1));\n \n \tif (!pager)\n \t\treturn;\n-- \n1.7.0\n"},{"id":"134535","messageId":"20100214120235.GC3499@progeny.tock","threadId":"22646","inReplyTo":"20100214115430.GA1849@progeny.tock","subject":"[PATCH 3/6] git.1: Clarify the behavior of the --paginate option","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-14T12:02:35Z","receivedAt":"2010-02-14T12:02:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The --paginate option is meant to negate the effect of an explicit or\nimplicit pager.<cmd> = false setting.  Thus it turns the pager on if\noutput is going to a terminal rather than unconditionally.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nNoticed while testing.  (It might be nice to have an --always-paginate\nfacility to allow debugging by writing output from a pager into\nsomething that is not a terminal.)\n\n Documentation/git.txt |    8 ++++++--\n 1 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 01c4631..f26641a 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -229,7 +229,10 @@ help ...`.\n \n -p::\n --paginate::\n-\tPipe all output into 'less' (or if set, $PAGER).\n+\tPipe all output into 'less' (or if set, $PAGER) if standard\n+\toutput is a terminal.  This overrides the `pager.<cmd>`\n+\tconfiguration options (see the \"Configuration Mechanism\" section\n+\tbelow).\n \n --no-pager::\n \tDo not pipe git output into a pager.\n@@ -401,7 +404,8 @@ people.  Here is an example:\n ------------\n \n Various commands read from the configuration file and adjust\n-their operation accordingly.\n+their operation accordingly.  See linkgit:git-config[1] for a\n+list.\n \n \n Identifier Terminology\n-- \n1.7.0\n"},{"id":"134536","messageId":"20100214120610.GD3499@progeny.tock","threadId":"22646","inReplyTo":"20100214115430.GA1849@progeny.tock","subject":"[PATCH 4/6] git svn: Fix launching of pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-14T12:06:10Z","receivedAt":"2010-02-14T12:06:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"In commit dec543e (am -i, git-svn: use \"git var GIT_PAGER\"), I tried\nto teach git svn to defer to git var on what pager to use. In the\nprocess, I introduced two bugs:\n\n - The value set for $pager in config_pager has local scope, so\n   run_pager never sees it;\n\n - git var cannot tell whether git svn’s output is going to a\n   terminal, so the value chosen for $pager does not reflect that\n   information.\n\nFix them.\n\nReported-by: Sebastian Celis <sebastian@sebastiancelis.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nOops.  Eric, this bug is masked by another bug in \"git var\" whose fix\nis patch 2 from this series I sent to the list, but whatever happens,\nI think this is an improvement.  Sorry for the breakage.\n\n git-svn.perl |    9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 265852f..473a0b9 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -5459,7 +5459,12 @@ sub git_svn_log_cmd {\n \n # adapted from pager.c\n sub config_pager {\n-\tchomp(my $pager = command_oneline(qw(var GIT_PAGER)));\n+\tif (! -t *STDOUT) {\n+\t\t$ENV{GIT_PAGER_IN_USE} = 'false';\n+\t\t$pager = undef;\n+\t\treturn;\n+\t}\n+\tchomp($pager = command_oneline(qw(var GIT_PAGER)));\n \tif ($pager eq 'cat') {\n \t\t$pager = undef;\n \t}\n@@ -5467,7 +5472,7 @@ sub config_pager {\n }\n \n sub run_pager {\n-\treturn unless -t *STDOUT && defined $pager;\n+\treturn unless defined $pager;\n \tpipe my ($rfd, $wfd) or return;\n \tdefined(my $pid = fork) or ::fatal \"Can't fork: $!\";\n \tif (!$pid) {\n-- \n1.7.0\n"},{"id":"134537","messageId":"20100214120731.GE3499@progeny.tock","threadId":"22646","inReplyTo":"20100214115430.GA1849@progeny.tock","subject":"[PATCH 5/6] am: Fix launching of pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-14T12:07:31Z","receivedAt":"2010-02-14T12:07:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The git_pager() function used in git am has some problems:\n\n - It does not check if stdout is a tty, so it always paginates.\n\n - If $GIT_PAGER uses any environment variables, they are being\n   ignored, since it does not run $GIT_PAGER through eval.\n\nFix them.  While at it, move the definition of git_pager() to\ngit-sh-setup so authors of other commands are not tempted to\nreimplement it with the same mistakes.\n\nIf GIT_PAGER is set to the empty string, pagination is disabled.  This\nis different from the treatment of GIT_EDITOR by git_editor(), but\nconsistency with the use of GIT_PAGER by git builtins is more\nimportant.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n git-am.sh       |    5 +----\n git-sh-setup.sh |   16 ++++++++++++++++\n 2 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 3c08d53..b11af03 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -663,10 +663,7 @@ do\n \t\t[eE]*) git_editor \"$dotest/final-commit\"\n \t\t       action=again ;;\n \t\t[vV]*) action=again\n-\t\t       : ${GIT_PAGER=$(git var GIT_PAGER)}\n-\t\t       : ${LESS=-FRSX}\n-\t\t       export LESS\n-\t\t       $GIT_PAGER \"$dotest/patch\" ;;\n+\t\t       git_pager \"$dotest/patch\" ;;\n \t\t*)     action=again ;;\n \t\tesac\n \t    done\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex d56426d..f3b5237 100755\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -107,6 +107,22 @@ git_editor() {\n \teval \"$GIT_EDITOR\" '\"$@\"'\n }\n \n+git_pager() {\n+\tif test -z \"${GIT_PAGER+set}\"\n+\tthen\n+\t\tif tty <&1 >/dev/null 2>/dev/null\n+\t\tthen\n+\t\t\tGIT_PAGER=cat\n+\t\telse\n+\t\t\tGIT_PAGER=$(git var GIT_PAGER)\n+\t\tfi\n+\tfi\n+\t: ${LESS=-FRSX}\n+\texport LESS\n+\n+\teval \"$GIT_PAGER\" '\"$@\"'\n+}\n+\n sane_grep () {\n \tGREP_OPTIONS= LC_ALL=C grep \"$@\"\n }\n-- \n1.7.0\n"},{"id":"134538","messageId":"20100214121300.GF3499@progeny.tock","threadId":"22646","inReplyTo":"20100214115430.GA1849@progeny.tock","subject":"[PATCH 6/6] tests: Add tests for automatic use of pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-14T12:13:00Z","receivedAt":"2010-02-14T12:13:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Git’s automatic pagination support has some subtleties; add some tests\nto make sure they don’t break.\n\nThe tests touch on a few different aspects of git’s pagination\nbehavior:\n\n - when git will use a pager by default;\n - the effect of the --paginate and --no-pager options;\n - the effect of pagination on use of color;\n - how the choice of pager is configured.\n\nThey does not yet test:\n\n - use of pager by scripted commands (git svn and git am);\n - effect of the pager.* configuration variables;\n - setting of the LESS variable.\n\nSome features require stdout to be a terminal to be testable without\na (not written) shim using pseudo-terminals.  Thus about half of these\ntests are skipped unless $GIT_TEST_OPTS includes --verbose.\n\nThe immediate purpose for these tests is to document removal of most\nbreakage from my jn/editor-pager series (see commit 376f39,\n2009-11-20).  Thanks to Sebastian Celis <sebastian@sebastiancelis.com>\nfor the report.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n t/t7006-pager.sh |  163 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 163 insertions(+), 0 deletions(-)\n create mode 100644 t/t7006-pager.sh\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nnew file mode 100644\nindex 0000000..5562b70\n--- /dev/null\n+++ b/t/t7006-pager.sh\n@@ -0,0 +1,163 @@\n+#!/bin/sh\n+\n+test_description='Test automatic use of a pager.'\n+\n+. ./test-lib.sh\n+\n+rm -f stdout_is_tty\n+test_expect_success 'is stdout a terminal?' '\n+\tif tty <&1 >/dev/null 2>/dev/null\n+\tthen\n+\t\t: > stdout_is_tty\n+\tfi\n+'\n+\n+if test -e stdout_is_tty\n+then\n+\ttest_set_prereq TTY\n+else\n+\tsay stdout is not a terminal, so skipping some tests.\n+fi\n+\n+unset GIT_PAGER GIT_PAGER_IN_USE\n+git config --unset core.pager\n+PAGER='cat > paginated.out'\n+export PAGER\n+\n+test_expect_success 'setup' '\n+\ttest_commit initial\n+'\n+\n+rm -f paginated.out\n+test_expect_success TTY 'some commands use a pager' '\n+\tgit log &&\n+\ttest -e paginated.out\n+'\n+\n+rm -f paginated.out\n+test_expect_success 'some commands do not use a pager' '\n+\tgit rev-list HEAD &&\n+\t! test -e paginated.out\n+'\n+\n+rm -f paginated.out\n+test_expect_success 'no pager when stdout is a pipe' '\n+\tgit log | cat &&\n+\t! test -e paginated.out\n+'\n+\n+rm -f paginated.out\n+test_expect_success 'no pager when stdout is a regular file' '\n+\tgit log > file &&\n+\t! test -e paginated.out\n+'\n+\n+rm -f paginated.out\n+test_expect_success TTY 'git --paginate rev-list uses a pager' '\n+\tgit --paginate rev-list HEAD  &&\n+\ttest -e paginated.out\n+'\n+\n+rm -f file paginated.out\n+test_expect_success 'no pager even with --paginate when stdout is a pipe' '\n+\tgit --paginate log | cat &&\n+\t! test -e paginated.out\n+'\n+\n+rm -f paginated.out\n+test_expect_success 'no pager with --no-pager' '\n+\tgit --no-pager log &&\n+\t! test -e paginated.out\n+'\n+\n+# A colored commit log will begin with an appropriate ANSI escape\n+# for the first color; the text \"commit\" comes later.\n+colorful() {\n+\tread firstline < $1\n+\t! expr \"$firstline\" : \"^[a-zA-Z]\" >/dev/null\n+}\n+\n+rm -f colorful.log colorless.log\n+test_expect_success 'tests can detect color' '\n+\tgit log --no-color > colorless.log &&\n+\tgit log --color > colorful.log &&\n+\t! colorful colorless.log &&\n+\tcolorful colorful.log\n+'\n+\n+rm -f colorless.log\n+git config color.ui auto\n+test_expect_success 'no color when stdout is a regular file' '\n+\tgit log > colorless.log &&\n+\t! colorful colorless.log\n+'\n+\n+rm -f paginated.out\n+git config color.ui auto\n+test_expect_success TTY 'color when writing to a pager' '\n+\tTERM=vt100 git log &&\n+\tcolorful paginated.out\n+'\n+\n+rm -f colorful.log\n+git config color.ui auto\n+test_expect 'color when writing to a file intended for a pager' '\n+\tTERM=vt100 GIT_PAGER_IN_USE=true git log > colorful.log &&\n+\tcolorful colorful.log\n+'\n+\n+unset PAGER GIT_PAGER\n+git config --unset core.pager\n+test_expect_success 'determine default pager' '\n+\tless=$(git var GIT_PAGER) &&\n+\ttest -n \"$less\"\n+'\n+\n+if expr \"$less\" : '^[a-z]*$' > /dev/null && test_have_prereq TTY\n+then\n+\ttest_set_prereq SIMPLEPAGER\n+fi\n+\n+unset PAGER GIT_PAGER\n+git config --unset core.pager\n+rm -f default_pager_used\n+test_expect_success SIMPLEPAGER 'default pager is used by default' '\n+\tcat > $less <<-EOF &&\n+\t#!$SHELL_PATH\n+\t: > default_pager_used\n+\tEOF\n+\tchmod +x $less &&\n+\tPATH=.:$PATH git log &&\n+\ttest -e default_pager_used\n+'\n+\n+unset GIT_PAGER\n+git config --unset core.pager\n+rm -f PAGER_used\n+test_expect_success TTY 'PAGER overrides default pager' '\n+\tPAGER=\": > PAGER_used\" &&\n+\texport PAGER &&\n+\tgit log &&\n+\ttest -e PAGER_used\n+'\n+\n+unset GIT_PAGER\n+rm -f core.pager_used\n+test_expect_success TTY 'core.pager overrides PAGER' '\n+\tPAGER=: &&\n+\texport PAGER &&\n+\tgit config core.pager \": > core.pager_used\" &&\n+\tgit log &&\n+\ttest -e core.pager_used\n+'\n+\n+rm -f GIT_PAGER_used\n+test_expect_success TTY 'GIT_PAGER overrides core.pager' '\n+\tgit config core.pager : &&\n+\tGIT_PAGER=\": > GIT_PAGER_used\" &&\n+\texport GIT_PAGER &&\n+\tgit log &&\n+\ttest -e GIT_PAGER_used\n+'\n+\n+test_done\n-- \n1.7.0\n"},{"id":"134590","messageId":"7vfx53xmo8.fsf@alter.siamese.dyndns.org","threadId":"22646","inReplyTo":"20100214115533.GA3499@progeny.tock","subject":"Re: [PATCH 1/6] Fix 'git var' usage synopsis","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-15T02:22:47Z","receivedAt":"2010-02-15T02:22:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> The parameter to 'git var' is not optional.\n>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> Noticed while considering changing the usage.\n\nAnd the patch does not even apply.  Sigh.\n"},{"id":"134591","messageId":"7viq9zw7qa.fsf@alter.siamese.dyndns.org","threadId":"22646","inReplyTo":"20100214120731.GE3499@progeny.tock","subject":"Re: [PATCH 5/6] am: Fix launching of pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-15T02:30:53Z","receivedAt":"2010-02-15T02:30:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index d56426d..f3b5237 100755\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -107,6 +107,22 @@ git_editor() {\n>  \teval \"$GIT_EDITOR\" '\"$@\"'\n>  }\n>  \n> +git_pager() {\n> +\tif test -z \"${GIT_PAGER+set}\"\n> +\tthen\n> +\t\tif tty <&1 >/dev/null 2>/dev/null\n> +\t\tthen\n> +\t\t\tGIT_PAGER=cat\n> +\t\telse\n> +\t\t\tGIT_PAGER=$(git var GIT_PAGER)\n> +\t\tfi\n> +\tfi\n> +\t: ${LESS=-FRSX}\n> +\texport LESS\n> +\n> +\teval \"$GIT_PAGER\" '\"$@\"'\n> +}\n> +\n\nI didn't read other patches, but I think this is a good change.\n\nAcked-by: Junio C Hamano <gitster@pobox.com>\n"},{"id":"134592","messageId":"20100215023636.GA17444@progeny.tock","threadId":"22646","inReplyTo":"20100214115533.GA3499@progeny.tock","subject":"[PATCH v2 1/6] Fix 'git var' usage synopsis","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-15T02:36:36Z","receivedAt":"2010-02-15T02:36:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> --- a/builtin-var.c\n> +++ b/builtin-var.c\n> @@ -6,7 +6,7 @@\n>  #include \"cache.h\"\n>  #include \"exec_cmd.h\"\n>  \n> -static const char var_usage[] = \"git var [-l | <variable>]\";\n> +static const char var_usage[] = \"git var (-l | <variable>)\";\n>  static const char *editor(int flag)\n>  {\n\nAs Junio noticed, this doesn’t apply.  The problem is a missing\nblank line between the static const char var_usage and static const\nchar *editor lines.  Worse still, I have no clue how it happened.\n\nHere’s a revised patch (generated just like the first one...).  It\nis really an independent fix from the rest of the series.\n\n-- %< --\nSubject: Fix 'git var' usage synopsis\n\nThe parameter to 'git var' is not optional.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n Documentation/git-var.txt |    2 +-\n builtin-var.c             |    2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-var.txt b/Documentation/git-var.txt\nindex bb98182..458f3e2 100644\n--- a/Documentation/git-var.txt\n+++ b/Documentation/git-var.txt\n@@ -8,7 +8,7 @@ git-var - Show a git logical variable\n \n SYNOPSIS\n --------\n-'git var' [ -l | <variable> ]\n+'git var' ( -l | <variable> )\n \n DESCRIPTION\n -----------\ndiff --git a/builtin-var.c b/builtin-var.c\nindex 2280518..e6ee7bc 100644\n--- a/builtin-var.c\n+++ b/builtin-var.c\n@@ -6,7 +6,7 @@\n #include \"cache.h\"\n #include \"exec_cmd.h\"\n \n-static const char var_usage[] = \"git var [-l | <variable>]\";\n+static const char var_usage[] = \"git var (-l | <variable>)\";\n \n static const char *editor(int flag)\n {\n-- \n1.7.0\n"},{"id":"134593","messageId":"20100215025958.GB17444@progeny.tock","threadId":"22646","inReplyTo":"20100214120731.GE3499@progeny.tock","subject":"[PATCH v2 5/6] am: Fix launching of pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-15T02:59:58Z","receivedAt":"2010-02-15T02:59:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The git_pager() function used in git am has some problems:\n\n - It does not check if stdout is a tty, so it always paginates.\n\n - If $GIT_PAGER uses any environment variables, they are being\n   ignored, since it does not run $GIT_PAGER through eval.\n\nFix them.  While at it, move the definition of git_pager() to\ngit-sh-setup so authors of other commands are not tempted to\nreimplement it with the same mistakes.\n\nIf GIT_PAGER is set to the empty string, pagination is disabled.  This\nis different from the treatment of GIT_EDITOR by git_editor(), but\nconsistency with the use of GIT_PAGER by git builtins is more\nimportant.\n\nAcked-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nJonathan Nieder wrote:\n\n> +\t\tif tty <&1 >/dev/null 2>/dev/null\n> +\t\tthen\n> +\t\t\tGIT_PAGER=cat\n> +\t\telse\n> +\t\t\tGIT_PAGER=$(git var GIT_PAGER)\n> +\t\tfi\n\nThis was backwards.  *sigh*\n\nHere’s a tested version.  Junio, I carried over your Ack, but please\nfeel free to revoke it.\n\n git-am.sh       |    5 +----\n git-sh-setup.sh |   16 ++++++++++++++++\n 2 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 3c08d53..b11af03 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -663,10 +663,7 @@ do\n \t\t[eE]*) git_editor \"$dotest/final-commit\"\n \t\t       action=again ;;\n \t\t[vV]*) action=again\n-\t\t       : ${GIT_PAGER=$(git var GIT_PAGER)}\n-\t\t       : ${LESS=-FRSX}\n-\t\t       export LESS\n-\t\t       $GIT_PAGER \"$dotest/patch\" ;;\n+\t\t       git_pager \"$dotest/patch\" ;;\n \t\t*)     action=again ;;\n \t\tesac\n \t    done\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex d56426d..4500c12 100755\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -107,6 +107,22 @@ git_editor() {\n \teval \"$GIT_EDITOR\" '\"$@\"'\n }\n \n+git_pager() {\n+\tif test -z \"${GIT_PAGER+set}\"\n+\tthen\n+\t\tif tty <&1 >/dev/null 2>/dev/null\n+\t\tthen\n+\t\t\tGIT_PAGER=$(git var GIT_PAGER)\n+\t\telse\n+\t\t\tGIT_PAGER=cat\n+\t\tfi\n+\tfi\n+\t: ${LESS=-FRSX}\n+\texport LESS\n+\n+\teval \"$GIT_PAGER\" '\"$@\"'\n+}\n+\n sane_grep () {\n \tGREP_OPTIONS= LC_ALL=C grep \"$@\"\n }\n-- \n1.7.0\n"},{"id":"134594","messageId":"20100215032533.GA19230@progeny.tock","threadId":"22646","inReplyTo":"20100215025958.GB17444@progeny.tock","subject":"[PATCH v3 5/6] am: Fix launching of pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-15T03:25:33Z","receivedAt":"2010-02-15T03:25:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The (v)iew command in git am -i has some problems:\n\n - It does not check if stdout is a tty, so it always paginates.\n\n - If $GIT_PAGER uses any environment variables, they are being\n   ignored, since it does not run $GIT_PAGER through eval.\n\n - If $GIT_PAGER is set to the empty string, instead of passing\n   output through to stdout, it tries to exec the patch.\n\nFix them.  While at it, move the definition of git_pager() to\ngit-sh-setup so authors of other commands are not tempted to\nreimplement it with the same mistakes.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nThe previous version did not handle GIT_PAGER=\"\" properly.  Third\ntime’s the charm, I hope.\n\n git-am.sh       |    5 +----\n git-sh-setup.sh |   19 +++++++++++++++++++\n 2 files changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 3c08d53..b11af03 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -663,10 +663,7 @@ do\n \t\t[eE]*) git_editor \"$dotest/final-commit\"\n \t\t       action=again ;;\n \t\t[vV]*) action=again\n-\t\t       : ${GIT_PAGER=$(git var GIT_PAGER)}\n-\t\t       : ${LESS=-FRSX}\n-\t\t       export LESS\n-\t\t       $GIT_PAGER \"$dotest/patch\" ;;\n+\t\t       git_pager \"$dotest/patch\" ;;\n \t\t*)     action=again ;;\n \t\tesac\n \t    done\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex d56426d..18ad1fc 100755\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -107,6 +107,25 @@ git_editor() {\n \teval \"$GIT_EDITOR\" '\"$@\"'\n }\n \n+git_pager() {\n+\tif test -z \"${GIT_PAGER+set}\"\n+\tthen\n+\t\tif tty <&1 >/dev/null 2>/dev/null\n+\t\tthen\n+\t\t\tGIT_PAGER=$(git var GIT_PAGER)\n+\t\telse\n+\t\t\tGIT_PAGER=cat\n+\t\tfi\n+\telif test -z \"$GIT_PAGER\"\n+\tthen\n+\t\tGIT_PAGER=cat\n+\tfi\n+\t: ${LESS=-FRSX}\n+\texport LESS\n+\n+\teval \"$GIT_PAGER\" '\"$@\"'\n+}\n+\n sane_grep () {\n \tGREP_OPTIONS= LC_ALL=C grep \"$@\"\n }\n-- \n1.7.0\n"},{"id":"134600","messageId":"20100215044223.GA3336@coredump.intra.peff.net","threadId":"22646","inReplyTo":"20100215032533.GA19230@progeny.tock","subject":"Re: [PATCH v3 5/6] am: Fix launching of pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-15T04:42:24Z","receivedAt":"2010-02-15T04:42:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2010 at 09:25:33PM -0600, Jonathan Nieder wrote:\n\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -107,6 +107,25 @@ git_editor() {\n>  \teval \"$GIT_EDITOR\" '\"$@\"'\n>  }\n>  \n> +git_pager() {\n> +\tif test -z \"${GIT_PAGER+set}\"\n> +\tthen\n> +\t\tif tty <&1 >/dev/null 2>/dev/null\n\nWe seem to use \"test -t 1\" for this elsewhere, and I don't see us using\n\"tty\" anywhere else. It is POSIX, and I tested that even Solaris 8 and\nAIX 6.1 have it. So I don't think it is a portability issue, but others\nmay know more than I.\n\n-Peff\n"},{"id":"134602","messageId":"20100215044722.GC3336@coredump.intra.peff.net","threadId":"22646","inReplyTo":"20100215023636.GA17444@progeny.tock","subject":"Re: [PATCH v2 1/6] Fix 'git var' usage synopsis","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-15T04:47:22Z","receivedAt":"2010-02-15T04:47:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2010 at 08:36:36PM -0600, Jonathan Nieder wrote:\n\n> > --- a/builtin-var.c\n> > +++ b/builtin-var.c\n> > @@ -6,7 +6,7 @@\n> >  #include \"cache.h\"\n> >  #include \"exec_cmd.h\"\n> >  \n> > -static const char var_usage[] = \"git var [-l | <variable>]\";\n> > +static const char var_usage[] = \"git var (-l | <variable>)\";\n> >  static const char *editor(int flag)\n> >  {\n> \n> As Junio noticed, this doesn’t apply.  The problem is a missing\n> blank line between the static const char var_usage and static const\n> char *editor lines.  Worse still, I have no clue how it happened.\n\nI think it was a stray deletion in your editor. The patch is actually\ncorrupt (the hunk claims 7 lines, but contains only 6). And you put text\nafter the \"---\", so presumably you did so with an editor. So I think it\nis probably not a bug, but just a weird once-off user error.\n\nOr maybe cosmic rays ate your line feed.\n\n-Peff\n"},{"id":"134603","messageId":"20100215045535.GD3336@coredump.intra.peff.net","threadId":"22646","inReplyTo":"20100214120235.GC3499@progeny.tock","subject":"Re: [PATCH 3/6] git.1: Clarify the behavior of the --paginate option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-15T04:55:35Z","receivedAt":"2010-02-15T04:55:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2010 at 06:02:35AM -0600, Jonathan Nieder wrote:\n\n> The --paginate option is meant to negate the effect of an explicit or\n> implicit pager.<cmd> = false setting.  Thus it turns the pager on if\n> output is going to a terminal rather than unconditionally.\n> \n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> Noticed while testing.  (It might be nice to have an --always-paginate\n> facility to allow debugging by writing output from a pager into\n> something that is not a terminal.)\n\nHmm. I would have expected --pager to have the \"always\" effect, though I\nwill admit I never actually use it (--no-pager, on the other hand...).\nBut if nobody is complaining, I certainly don't want to change the\nbehavior haphazardly, and documenting it makes sense. Thanks.\n\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 01c4631..f26641a 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -229,7 +229,10 @@ help ...`.\n>  \n>  -p::\n>  --paginate::\n> -\tPipe all output into 'less' (or if set, $PAGER).\n> +\tPipe all output into 'less' (or if set, $PAGER) if standard\n> +\toutput is a terminal.  This overrides the `pager.<cmd>`\n> +\tconfiguration options (see the \"Configuration Mechanism\" section\n> +\tbelow).\n\nThe \"Configuration Mechanism\" section doesn't actually contain anything\nrelated. I guess you were going for \"point to that section, and then it\nwill point to the list of config\", which explains this:\n\n> @@ -401,7 +404,8 @@ people.  Here is an example:\n>  ------------\n>  \n>  Various commands read from the configuration file and adjust\n> -their operation accordingly.\n> +their operation accordingly.  See linkgit:git-config[1] for a\n> +list.\n\nwhich seemed at first to be unrelated (but I think is a good change\nregardless).\n\nIt seems like a long jump for somebody who doesn't know what pager.*\ndoes to follow, but I guess it doesn't hurt to try to provide pointers.\nI have long since forgotten what it is like to not know that\n\"git-config\" contains the list of options. :)\n\n-Peff\n"},{"id":"134604","messageId":"20100215050007.GE3336@coredump.intra.peff.net","threadId":"22646","inReplyTo":"20100214115959.GB3499@progeny.tock","subject":"Re: [PATCH 2/6] Make 'git var GIT_PAGER' always print the configured pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-15T05:00:07Z","receivedAt":"2010-02-15T05:00:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2010 at 05:59:59AM -0600, Jonathan Nieder wrote:\n\n> Scripted commands that want to use git’s configured pager know better\n> than ‘git var’ does whether stdout is going to be a tty at the\n> appropriate time.  Checking isatty(1) as git_pager() does now won’t\n> cut it, since the output of git var itself is almost never a terminal.\n> The symptom is that when used by humans, ‘git var GIT_PAGER’ behaves\n> as it should, but when used by scripts, it always returns ‘cat’!\n> \n> So avoid tricks with isatty() and just always print the configured\n> pager.\n\nI think this is the right thing to do. But isn't \"git am\" broken in the\nmeantime, as it now always paginates (whereas before, it would never\npaginate)? You fix it later in the series, but is there any test\nbreakage in the meantime (not rhetorical, I didn't actually check) that\nwould hurt bisectability?\n\nThe same would go for \"git svn\", except that its pagination was also\nbroken in other ways. :)\n\n-Peff\n"},{"id":"134606","messageId":"20100215050413.GA19586@progeny.tock","threadId":"22646","inReplyTo":"20100215044223.GA3336@coredump.intra.peff.net","subject":"[PATCH v4 5/6] am: Fix launching of pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-15T05:04:13Z","receivedAt":"2010-02-15T05:04:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The pagination functionality in git am has some problems:\n\n - It does not check if stdout is a tty, so it always paginates.\n\n - If $GIT_PAGER uses any environment variables, they are being\n   ignored, since it does not run $GIT_PAGER through eval.\n\n - If $GIT_PAGER is set to the empty string, instead of passing\n   output through to stdout, it tries to run $dotest/patch.\n\nFix them.  While at it, move the definition of git_pager() to\ngit-sh-setup so authors of other commands are not tempted to\nreimplement it with the same mistakes.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nJeff King wrote:\n> On Sun, Feb 14, 2010 at 09:25:33PM -0600, Jonathan Nieder wrote:\n\n>> +\t\tif tty <&1 >/dev/null 2>/dev/null\n>\n> We seem to use \"test -t 1\" for this elsewhere, and I don't see us using\n> \"tty\" anywhere else. It is POSIX, and I tested that even Solaris 8 and\n> AIX 6.1 have it. So I don't think it is a portability issue, but others\n> may know more than I.\n\ntty is portable, but \"test -t 1\" is cleaner.  Thanks for the pointer.\n\n git-am.sh       |    5 +----\n git-sh-setup.sh |   13 +++++++++++++\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 3c08d53..b11af03 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -663,10 +663,7 @@ do\n \t\t[eE]*) git_editor \"$dotest/final-commit\"\n \t\t       action=again ;;\n \t\t[vV]*) action=again\n-\t\t       : ${GIT_PAGER=$(git var GIT_PAGER)}\n-\t\t       : ${LESS=-FRSX}\n-\t\t       export LESS\n-\t\t       $GIT_PAGER \"$dotest/patch\" ;;\n+\t\t       git_pager \"$dotest/patch\" ;;\n \t\t*)     action=again ;;\n \t\tesac\n \t    done\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex d56426d..44fb467 100755\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -107,6 +107,19 @@ git_editor() {\n \teval \"$GIT_EDITOR\" '\"$@\"'\n }\n \n+git_pager() {\n+\tif test -t 1\n+\tthen\n+\t\tGIT_PAGER=$(git var GIT_PAGER)\n+\telse\n+\t\tGIT_PAGER=cat\n+\tfi\n+\t: ${LESS=-FRSX}\n+\texport LESS\n+\n+\teval \"$GIT_PAGER\" '\"$@\"'\n+}\n+\n sane_grep () {\n \tGREP_OPTIONS= LC_ALL=C grep \"$@\"\n }\n-- \n1.7.0\n"},{"id":"134608","messageId":"20100215051016.GF3336@coredump.intra.peff.net","threadId":"22646","inReplyTo":"20100214121300.GF3499@progeny.tock","subject":"Re: [PATCH 6/6] tests: Add tests for automatic use of pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-15T05:10:16Z","receivedAt":"2010-02-15T05:10:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2010 at 06:13:00AM -0600, Jonathan Nieder wrote:\n\n> They does not yet test:\n> \n>  - use of pager by scripted commands (git svn and git am);\n>  - effect of the pager.* configuration variables;\n>  - setting of the LESS variable.\n> \n> Some features require stdout to be a terminal to be testable without\n> a (not written) shim using pseudo-terminals.  Thus about half of these\n> tests are skipped unless $GIT_TEST_OPTS includes --verbose.\n\nI am a little lukewarm on tests that require --verbose, since they will\nnot be caught by 99% of the runs of the test suite. Which means they\nsneak through things like Junio's automated testing of maint, master,\nand next.\n\nStill, perhaps something is better than nothing. I wonder if we can do\nbetter, though, with something like:\n\n  int xisatty(int fd)\n  {\n    static int fake[3];\n\n    if (fd > ARRAY_SIZE(pretend))\n      return isatty(fd);\n\n    if (fake[fd] == -1) {\n      const char *x = getenv(\"GIT_PRETEND_TTY\");\n      if (x && strchr(x, '0' + fd))\n        fake[fd] = 1;\n      else\n        fake[fd] = isatty(fd);\n    }\n    return fake[fd];\n  }\n\n-Peff\n"},{"id":"134609","messageId":"20100215051248.GG3336@coredump.intra.peff.net","threadId":"22646","inReplyTo":"20100215050413.GA19586@progeny.tock","subject":"Re: [PATCH v4 5/6] am: Fix launching of pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-15T05:12:48Z","receivedAt":"2010-02-15T05:12:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2010 at 11:04:13PM -0600, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> > On Sun, Feb 14, 2010 at 09:25:33PM -0600, Jonathan Nieder wrote:\n> \n> >> +\t\tif tty <&1 >/dev/null 2>/dev/null\n> >\n> > We seem to use \"test -t 1\" for this elsewhere, and I don't see us using\n> > \"tty\" anywhere else. It is POSIX, and I tested that even Solaris 8 and\n> > AIX 6.1 have it. So I don't think it is a portability issue, but others\n> > may know more than I.\n> \n> tty is portable, but \"test -t 1\" is cleaner.  Thanks for the pointer.\n\nThere is another one in the test script in patch 6/6, btw.\n\n-Peff\n"},{"id":"134614","messageId":"20100215053521.GA22121@progeny.tock","threadId":"22646","inReplyTo":"20100215051016.GF3336@coredump.intra.peff.net","subject":"[PATCH v2 6/6] tests: Add tests for automatic use of pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-15T05:35:21Z","receivedAt":"2010-02-15T05:35:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Git’s automatic pagination support has some subtleties; add some tests\nto make sure they don’t break again.\n\nThese test:\n\n - when git will use a pager by default;\n - the effect of the --paginate and --no-pager options;\n - the effect of pagination on use of color;\n - how the choice of pager is configured.\n\nThey does not yet test:\n\n - use of pager by scripted commands (git svn and git am);\n - effect of the pager.* configuration variables;\n - setting of the LESS variable.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nJeff King wrote:\n\n> I am a little lukewarm on tests that require --verbose, since they will\n> not be caught by 99% of the runs of the test suite.\n\nMakes sense.\n\n> Still, perhaps something is better than nothing. I wonder if we can do\n> better, though, with something like:\n[...]\n>     if (fake[fd] == -1) {\n>       const char *x = getenv(\"GIT_PRETEND_TTY\");\n>       if (x && strchr(x, '0' + fd))\n>         fake[fd] = 1;\n>       else\n>         fake[fd] = isatty(fd);\n\nHow about this?  It seems a little simpler, though it might be less\nreliable.\n\n t/t7006-pager.sh |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 156 insertions(+), 0 deletions(-)\n create mode 100644 t/t7006-pager.sh\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nnew file mode 100644\nindex 0000000..458a754\n--- /dev/null\n+++ b/t/t7006-pager.sh\n@@ -0,0 +1,156 @@\n+#!/bin/sh\n+\n+test_description='Test automatic use of a pager.'\n+\n+. ./test-lib.sh\n+\n+if eval \"exec 8>/dev/tty\" 2>/dev/null && test -t 8\n+then\n+\ttest_set_prereq TTY\n+else\n+\tsay no usable /dev/tty, so skipping some tests.\n+\texec 8>&-\n+fi\n+\n+unset GIT_PAGER GIT_PAGER_IN_USE\n+git config --unset core.pager\n+PAGER='cat > paginated.out'\n+export PAGER\n+\n+test_expect_success 'setup' '\n+\ttest_commit initial\n+'\n+\n+rm -f paginated.out\n+test_expect_success TTY 'some commands use a pager' '\n+\tgit log -0 >&8 &&\n+\ttest -e paginated.out\n+'\n+\n+rm -f paginated.out\n+test_expect_success TTY 'some commands do not use a pager' '\n+\tgit rev-list -0 HEAD >&8 &&\n+\t! test -e paginated.out\n+'\n+\n+rm -f paginated.out\n+test_expect_success 'no pager when stdout is a pipe' '\n+\tgit log | cat &&\n+\t! test -e paginated.out\n+'\n+\n+rm -f paginated.out\n+test_expect_success 'no pager when stdout is a regular file' '\n+\tgit log > file &&\n+\t! test -e paginated.out\n+'\n+\n+rm -f paginated.out\n+test_expect_success TTY 'git --paginate rev-list uses a pager' '\n+\tgit --paginate rev-list -0 HEAD >&8 &&\n+\ttest -e paginated.out\n+'\n+\n+rm -f file paginated.out\n+test_expect_success 'no pager even with --paginate when stdout is a pipe' '\n+\tgit --paginate log | cat &&\n+\t! test -e paginated.out\n+'\n+\n+rm -f paginated.out\n+test_expect_success TTY 'no pager with --no-pager' '\n+\tgit --no-pager log -0 >&8 &&\n+\t! test -e paginated.out\n+'\n+\n+# A colored commit log will begin with an appropriate ANSI escape\n+# for the first color; the text \"commit\" comes later.\n+colorful() {\n+\tread firstline < $1\n+\t! expr \"$firstline\" : \"^[a-zA-Z]\" >/dev/null\n+}\n+\n+rm -f colorful.log colorless.log\n+test_expect_success 'tests can detect color' '\n+\tgit log --no-color > colorless.log &&\n+\tgit log --color > colorful.log &&\n+\t! colorful colorless.log &&\n+\tcolorful colorful.log\n+'\n+\n+rm -f colorless.log\n+git config color.ui auto\n+test_expect_success 'no color when stdout is a regular file' '\n+\tgit log > colorless.log &&\n+\t! colorful colorless.log\n+'\n+\n+rm -f paginated.out\n+git config color.ui auto\n+test_expect_success TTY 'color when writing to a pager' '\n+\tTERM=vt100 git log >&8 &&\n+\tcolorful paginated.out\n+'\n+\n+rm -f colorful.log\n+git config color.ui auto\n+test_expect_success 'color when writing to a file intended for a pager' '\n+\tTERM=vt100 GIT_PAGER_IN_USE=true git log > colorful.log &&\n+\tcolorful colorful.log\n+'\n+\n+unset PAGER GIT_PAGER\n+git config --unset core.pager\n+test_expect_success 'determine default pager' '\n+\tless=$(git var GIT_PAGER) &&\n+\ttest -n \"$less\"\n+'\n+\n+if expr \"$less\" : '^[a-z]*$' > /dev/null && test_have_prereq TTY\n+then\n+\ttest_set_prereq SIMPLEPAGER\n+fi\n+\n+unset PAGER GIT_PAGER\n+git config --unset core.pager\n+rm -f default_pager_used\n+test_expect_success SIMPLEPAGER 'default pager is used by default' '\n+\tcat > $less <<-EOF &&\n+\t#!$SHELL_PATH\n+\t: > default_pager_used\n+\tEOF\n+\tchmod +x $less &&\n+\tPATH=.:$PATH git log -0 >&8 &&\n+\ttest -e default_pager_used\n+'\n+\n+unset GIT_PAGER\n+git config --unset core.pager\n+rm -f PAGER_used\n+test_expect_success TTY 'PAGER overrides default pager' '\n+\tPAGER=\": > PAGER_used\" &&\n+\texport PAGER &&\n+\tgit log -0 >&8 &&\n+\ttest -e PAGER_used\n+'\n+\n+unset GIT_PAGER\n+rm -f core.pager_used\n+test_expect_success TTY 'core.pager overrides PAGER' '\n+\tPAGER=: &&\n+\texport PAGER &&\n+\tgit config core.pager \": > core.pager_used\" &&\n+\tgit log -0 >&8 &&\n+\ttest -e core.pager_used\n+'\n+\n+rm -f GIT_PAGER_used\n+test_expect_success TTY 'GIT_PAGER overrides core.pager' '\n+\tgit config core.pager : &&\n+\tGIT_PAGER=\": > GIT_PAGER_used\" &&\n+\texport GIT_PAGER &&\n+\tgit log -0 >&8 &&\n+\ttest -e GIT_PAGER_used\n+'\n+\n+test_done\n-- \n1.7.0\n"},{"id":"134615","messageId":"20100215053747.GA4897@coredump.intra.peff.net","threadId":"22646","inReplyTo":"20100215053521.GA22121@progeny.tock","subject":"Re: [PATCH v2 6/6] tests: Add tests for automatic use of pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-15T05:37:47Z","receivedAt":"2010-02-15T05:37:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2010 at 11:35:21PM -0600, Jonathan Nieder wrote:\n\n> How about this?  It seems a little simpler, though it might be less\n> reliable.\n> [...]\n> +if eval \"exec 8>/dev/tty\" 2>/dev/null && test -t 8\n\nRight, it still breaks if we don't have a tty at all. Which is perhaps\nnot the \"99% of runs\" I mentioned before, but I think is not all that\nuncommon (e.g., cron jobs).\n\n-Peff\n"},{"id":"134616","messageId":"20100215054447.GB22121@progeny.tock","threadId":"22646","inReplyTo":"20100215050007.GE3336@coredump.intra.peff.net","subject":"Re: [PATCH 2/6] Make 'git var GIT_PAGER' always print the configured pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-15T05:44:47Z","receivedAt":"2010-02-15T05:44:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"On Mon, Feb 15, 2010 at 12:00:07AM -0500, Jeff King wrote:\n> On Sun, Feb 14, 2010 at 05:59:59AM -0600, Jonathan Nieder wrote:\n\n>> So avoid tricks with isatty() and just always print the configured\n>> pager.\n>\n> I think this is the right thing to do. But isn't \"git am\" broken in the\n> meantime, as it now always paginates (whereas before, it would never\n> paginate)? You fix it later in the series, but is there any test\n> breakage in the meantime (not rhetorical, I didn't actually check) that\n> would hurt bisectability?\n\nThe behavior before dec543 (am -i, git-svn: use \"git var GIT_PAGER\",\n2009-10-30) was to always paginate.  This made some sense, since this\nis the (v)iew command in git am --interactive; whether we check or\nnot, presumably stdout is a terminal.  So for git am, this restores\nthe older behavior.\n\nThere are unfortunately no tests for am --interactive in the test\nsuite, so test suite runs would detect none of this.\n\nThank you for your attention to detail.\nJonathan\n"},{"id":"134627","messageId":"20100215075752.GC5347@coredump.intra.peff.net","threadId":"22646","inReplyTo":"20100215054447.GB22121@progeny.tock","subject":"Re: [PATCH 2/6] Make 'git var GIT_PAGER' always print the configured pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-15T07:57:52Z","receivedAt":"2010-02-15T07:57:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2010 at 11:44:47PM -0600, Jonathan Nieder wrote:\n\n> > I think this is the right thing to do. But isn't \"git am\" broken in the\n> > meantime, as it now always paginates (whereas before, it would never\n> > paginate)? You fix it later in the series, but is there any test\n> > breakage in the meantime (not rhetorical, I didn't actually check) that\n> > would hurt bisectability?\n> \n> The behavior before dec543 (am -i, git-svn: use \"git var GIT_PAGER\",\n> 2009-10-30) was to always paginate.  This made some sense, since this\n> is the (v)iew command in git am --interactive; whether we check or\n> not, presumably stdout is a terminal.  So for git am, this restores\n> the older behavior.\n> \n> There are unfortunately no tests for am --interactive in the test\n> suite, so test suite runs would detect none of this.\n> \n> Thank you for your attention to detail.\n\nMakes sense. Thank _you_ for patches and for checking on my concerns.\n\n-Peff\n"},{"id":"134628","messageId":"20100215075824.GA9997@dcvr.yhbt.net","threadId":"22646","inReplyTo":"20100214120610.GD3499@progeny.tock","subject":"Re: [PATCH 4/6] git svn: Fix launching of pager","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2010-02-15T07:58:24Z","receivedAt":"2010-02-15T07:58:24Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jonathan Nieder <jrnieder@gmail.com> wrote:\n> In commit dec543e (am -i, git-svn: use \"git var GIT_PAGER\"), I tried\n> to teach git svn to defer to git var on what pager to use. In the\n> process, I introduced two bugs:\n> \n>  - The value set for $pager in config_pager has local scope, so\n>    run_pager never sees it;\n> \n>  - git var cannot tell whether git svn’s output is going to a\n>    terminal, so the value chosen for $pager does not reflect that\n>    information.\n> \n> Fix them.\n> \n> Reported-by: Sebastian Celis <sebastian@sebastiancelis.com>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> Oops.  Eric, this bug is masked by another bug in \"git var\" whose fix\n> is patch 2 from this series I sent to the list, but whatever happens,\n> I think this is an improvement.  Sorry for the breakage.\n\nThanks Jonathan, confirmed this works with patch 2 of this series.\n\nAcked-by: Eric Wong <normalperson@yhbt.net>\n\n-- \nEric Wong\n"},{"id":"134658","messageId":"4B79AE03.6050108@kdbg.org","threadId":"22646","inReplyTo":"20100215050413.GA19586@progeny.tock","subject":"Re: [PATCH v4 5/6] am: Fix launching of pager","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-02-15T20:26:43Z","receivedAt":"2010-02-15T20:26:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jonathan Nieder schrieb:\n> tty is portable, but \"test -t 1\" is cleaner.\n\ntty is not portable enough: we don't have it on Windows. test -t 1 is fine.\n\nThanks.\n\n-- Hannes\n"}]}