[PATCH/RFC] Fix for default pager

[PATCH/RFC] Fix for default pager

[Dario Rodriguez]

All commands using pager:

Default pager was 'less' even when some systems such AIX and other basic
or old systems do NOT have 'less' installed. In such case, git just
does not display anything in pager-enabled functionalities such as 'git log'
or 'git show', exiting with status 0.

With this patch, git will not use DEFAULT_PAGER macro anymore, instead,
git will look for 'less' and 'more' in the most common paths.
If there is no pager, returns NULL as if it's 'cat'.

Obviously, the commit message needs to be rewritten :p

Signed-off-by: Dario Rodriguez <soft.d4rio@gmail.com>
---
 pager.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 73 insertions(+), 7 deletions(-)

diff --git a/pager.c b/pager.c
index dac358f..3cfbd73 100644
--- a/pager.c
+++ b/pager.c
@@ -2,10 +2,19 @@
 #include "run-command.h"
 #include "sigchain.h"
 
-#ifndef DEFAULT_PAGER
-#define DEFAULT_PAGER "less"
+#ifndef GIT_PAGER_ENVIRONMENT
+#define GIT_PAGER_ENVIRONMENT "GIT_PAGER"
 #endif
 
+#ifndef GIT_PGR_LOOKUP_DBG_ENVIRONMENT
+#define GIT_PGR_LOOKUP_DBG_ENVIRONMENT "GIT_PAGER_LOOKUP_DEBUG"
+#endif
+
+#ifndef PAGER_ENVIRONMENT
+#define PAGER_ENVIRONMENT "PAGER"
+#endif
+
+
 /*
  * This is split up from the rest of git so that we can do
  * something different on Windows.
@@ -48,23 +57,80 @@ static void wait_for_pager_signal(int signo)
 	raise(signo);
 }
 
-const char *git_pager(int stdout_is_tty)
+static int is_executable(const char *name)
+{
+	struct stat st;
+
+	if (stat(name, &st) ||
+	    !S_ISREG(st.st_mode))
+		return 0;
+
+#ifdef WIN32
+{	/* cannot trust the executable bit, peek into the file instead */
+	char buf[3] = { 0 };
+	int n;
+	int fd = open(name, O_RDONLY);
+	st.st_mode &= ~S_IXUSR;
+	if (fd >= 0) {
+		n = read(fd, buf, 2);
+		if (n == 2)
+			/* DOS executables start with "MZ" */
+			if (!strcmp(buf, "#!") || !strcmp(buf, "MZ"))
+				st.st_mode |= S_IXUSR;
+		close(fd);
+	}
+}
+#endif
+	return st.st_mode & S_IXUSR;
+}
+
+const char *git_pager(int stdout_is_tty) 
 {
+	static const char *pager_bins[] =
+		{ "less", "more", NULL };
+	static const char *common_binary_paths[] =
+		{ "/bin/","/usr/bin/","/usr/local/bin/",NULL };
+
 	const char *pager;
+	char **p1,**p2,*pager_heap;
+	char exe_name[255];
+	int pager_lkp_dbg=0;
 
 	if (!stdout_is_tty)
 		return NULL;
 
-	pager = getenv("GIT_PAGER");
+	if(getenv(GIT_PGR_LOOKUP_DBG_ENVIRONMENT))
+		pager_lkp_dbg=1;
+
+	memset( exe_name,0,255 );
+	pager = getenv(GIT_PAGER_ENVIRONMENT);
 	if (!pager) {
 		if (!pager_program)
 			git_config(git_default_config, NULL);
 		pager = pager_program;
 	}
 	if (!pager)
-		pager = getenv("PAGER");
-	if (!pager)
-		pager = DEFAULT_PAGER;
+		pager = getenv(PAGER_ENVIRONMENT);
+	if (!pager) {
+
+		for (p1=(char**)pager_bins; (*p1)&&(!pager)
+			;p1++)
+			for (p2=(char**)common_binary_paths; (*p2)&&(!pager)
+				;p2++) {
+				sprintf( exe_name,"%s%s",
+					 *p2,*p1 );
+				if (is_executable(exe_name)) {
+					pager_heap = (char*) malloc(sizeof(exe_name));
+					strcpy(pager_heap,exe_name);
+					pager = pager_heap;
+				}
+			}
+
+		if (pager_lkp_dbg)
+			fprintf(stderr, "Debug: Lookup for existent pagers, got [%s]\n",
+				(pager==NULL)?"(null)":pager);
+
+	}
 	else if (!*pager || !strcmp(pager, "cat"))
 		pager = NULL;
 
-- 
1.7.1.245.g7c42e.dirty

Re: [PATCH/RFC] Fix for default pager

[Ben Walton]

Excerpts from Dario Rodriguez's message of Mon Jun 07 19:58:08 -0400 2010:

> Default pager was 'less' even when some systems such AIX and other
> basic or old systems do NOT have 'less' installed. In such case, git
> just does not display anything in pager-enabled functionalities such
> as 'git log' or 'git show', exiting with status 0.

Why not set a sensible DEFAULT_PAGER value for the system in your
config.mak file instead?

Just curious.

Thanks
-Ben
-- 
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH/RFC] Fix for default pager

[Dario Rodriguez]

On Mon, Jun 7, 2010 at 9:04 PM, Ben Walton <bwalton@artsci.utoronto.ca> wrote:
>
> Why not set a sensible DEFAULT_PAGER value for the system in your
> config.mak file instead?
>
> Just curious.
>

I was thinking about it before coding the patch, and found some
consequences. First of all, the most important thing I should
understand is that most users will install git from binary,
precompiled packages instead of the good download and compile. So this
is actually a good reason to not do it that way (config.mak)... Some
users may download the compiled binary while it's actually calling
it's default pager.

But this is not the only reason. Let me give you an example: We
develop (I'm actually working at Accenture) using several machines.
When we need some tool, we compile it in our first machine, and
install it for an specific user. In some other environments we cannot
compile things (testing environments) so we transfer (FTP) those
binary files. It's just another case, and the default pager could
cause problems here (in fact, i experienced such problems).

Another case (or something as the first one): If you installed less
(via your package system), then git, and then you happen to uninstall
less... should GIT be uninstalled in absence of it's default pager?

I think GIT must auto-detect it's pager based in what is on the system
at runtime, don't you think so?

Re: [PATCH/RFC] Fix for default pager

[Jeff King]

On Mon, Jun 07, 2010 at 11:14:58PM -0300, Dario Rodriguez wrote:

> On Mon, Jun 7, 2010 at 9:04 PM, Ben Walton <bwalton@artsci.utoronto.ca> wrote:
> >
> > Why not set a sensible DEFAULT_PAGER value for the system in your
> > config.mak file instead?
> >
> > Just curious.
> >
> 
> I was thinking about it before coding the patch, and found some
> consequences. First of all, the most important thing I should
> understand is that most users will install git from binary,
> precompiled packages instead of the good download and compile. So this
> is actually a good reason to not do it that way (config.mak)... Some
> users may download the compiled binary while it's actually calling
> it's default pager.

If you are downloading a binary, the package compiler should do one of
two things:

  1. indicate a package dependency on 'less'

  2. set DEFAULT_PAGER to 'more' (or whatever is appropriate for your
     system)

Yes, auto-detection means we can more flexibly "upgrade" to less when
the package suddenly appears. But if you really care about your pager,
why not just set $PAGER?

The most important thing is that users who _don't_ care don't see
something broken, but the rules above already cover that with current
git.

> But this is not the only reason. Let me give you an example: We
> develop (I'm actually working at Accenture) using several machines.
> When we need some tool, we compile it in our first machine, and
> install it for an specific user. In some other environments we cannot
> compile things (testing environments) so we transfer (FTP) those
> binary files. It's just another case, and the default pager could
> cause problems here (in fact, i experienced such problems).

Then set DEFAULT_PAGER to 'more' (or 'cat' for that matter), and use
$PAGER on machines that are more capable.

-Peff

Re: [PATCH/RFC] Fix for default pager

[Dario Rodriguez]

On Tue, Jun 8, 2010 at 2:35 AM, Jeff King <peff@peff.net> wrote:
> If you are downloading a binary, the package compiler should do one of
> two things:
>
>  1. indicate a package dependency on 'less'
>

but... git will be uninstalled if you happen to uninstall less.

>  2. set DEFAULT_PAGER to 'more' (or whatever is appropriate for your
>     system)
>

and I know it's a very nice fallback default, but you still depending
on the pager, and will display nothing and return 0 if 'more' fails to
execute.

Something needs to be changed... if the sane way is to keep default
pager (almost setting 'more') instead of auto-detection... then we
must say something to the user when the pager fails to execute. (*)
Actually 'git' display nothing... and returns 0.

> Yes, auto-detection means we can more flexibly "upgrade" to less when
> the package suddenly appears. But if you really care about your pager,
> why not just set $PAGER?
>

good point, I agree. But again, back to (*), we must correct
something... the other way is that if pager fails to execute, we
cannot simply return 0.

I could submit a patch for this, may be is a little bit better (or simpler).

Re: [PATCH/RFC] Fix for default pager

[Johannes Sixt]

Am 6/8/2010 15:49, schrieb Dario Rodriguez:
> we must correct
> something... the other way is that if pager fails to execute, we
> cannot simply return 0.

But we do not return 0:

 $ GIT_PAGER=/is/not/there git log
 $ echo $?
 141

That's SIGPIPE, just as I would expect.

And with this change

diff --git a/pager.c b/pager.c
index dac358f..86519cc 100644
--- a/pager.c
+++ b/pager.c
@@ -89,9 +89,6 @@ void setup_pager(void)
 		static const char *env[] = { "LESS=FRSX", NULL };
 		pager_process.env = env;
 	}
-#ifndef WIN32
-	pager_process.preexec_cb = pager_preexec;
-#endif
 	if (start_command(&pager_process))
 	return;


I get:

 $ GIT_PAGER=/is/not/there ./git log -1 --oneline
 error: cannot run /is/not/there: No such file or directory
 1a16cee merge-recursive: demonstrate an incorrect conflict with submodule

-- Hannes

Re: [PATCH/RFC] Fix for default pager

[Dario Rodriguez]

On Tue, Jun 8, 2010 at 11:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 6/8/2010 15:49, schrieb Dario Rodriguez:
>> we must correct
>> something... the other way is that if pager fails to execute, we
>> cannot simply return 0.
>
> But we do not return 0:
>
>  $ GIT_PAGER=/is/not/there git log
>  $ echo $?
>  141
>
> That's SIGPIPE, just as I would expect.
>

As I said in the original thread...

$ PAGER=/nothing/here ../git log
$ echo $?
0

$ GIT_PAGER=/nothing/here ../git log
$ echo $?
0

That's on AIX 5.2


> And with this change
>
> diff --git a/pager.c b/pager.c
> index dac358f..86519cc 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -89,9 +89,6 @@ void setup_pager(void)
>                static const char *env[] = { "LESS=FRSX", NULL };
>                pager_process.env = env;
>        }
> -#ifndef WIN32
> -       pager_process.preexec_cb = pager_preexec;
> -#endif
>        if (start_command(&pager_process))
>        return;
>
>
> I get:
>
>  $ GIT_PAGER=/is/not/there ./git log -1 --oneline
>  error: cannot run /is/not/there: No such file or directory
>  1a16cee merge-recursive: demonstrate an incorrect conflict with submodule
>

Curious... I patched it on AIX and I get:

$ GIT_PAGER=/nothing/here ../git log
error: cannot run /nothing/here: No such file or directory
commit 3274a12f940680612e3bfd3d022a0eab460c0f1f
Author: usuario ####### <#######@Maquina01.(none)>
Date:   Thu Jun 3 20:02:23 2010 +0200

    OtherCom

commit acf110f7c878a37e4a5af8499134df28da0e8ab3
Author: usuario ####### <#######@Maquina01.(none)>
Date:   Thu Jun 3 20:01:37 2010 +0200

    inicial



However, the patch must delete the pager_preexec definition too... but
I wonder, do somebody still need it?

btw: I still think 'more' is much more sane fallback default than
'less'... look (with your patch applied):

$ ../git log
error: cannot run less: No such file or directory
commit 3274a12f940680612e3bfd3d022a0eab460c0f1f
Author: ####### <#######@Maquina01.(none)>
Date:   Thu Jun 3 20:02:23 2010 +0200

    OtherCom

commit acf110f7c878a37e4a5af8499134df28da0e8ab3
Author: ####### <#######@Maquina01.(none)>
Date:   Thu Jun 3 20:01:37 2010 +0200

    inicial

Re: [PATCH/RFC] Fix for default pager

[Johannes Sixt]

Am 08.06.2010 16:39, schrieb Dario Rodriguez:
> On Tue, Jun 8, 2010 at 11:17 AM, Johannes Sixt<j.sixt@viscovery.net>  wrote:
>>   $ GIT_PAGER=/is/not/there git log
>>   $ echo $?
>>   141
>>
>> That's SIGPIPE, just as I would expect.
>
> As I said in the original thread...
>
> $ PAGER=/nothing/here ../git log
> $ echo $?
> 0

That's no surprise with your toy repository: git-log has run to completion 
(without overrunning the pipe buffer) before the pager process that it 
forked can even execute its first instruction.

> btw: I still think 'more' is much more sane fallback default than
> 'less'... look (with your patch applied):
>
> $ ../git log
> error: cannot run less: No such file or directory
> commit 3274a12f940680612e3bfd3d022a0eab460c0f1f
> Author: #######<#######@Maquina01.(none)>
> Date:   Thu Jun 3 20:02:23 2010 +0200
>
>      OtherCom
>
> commit acf110f7c878a37e4a5af8499134df28da0e8ab3
> Author: #######<#######@Maquina01.(none)>
> Date:   Thu Jun 3 20:01:37 2010 +0200
>
>      inicial
>

How is this an argument for 'more'? (Just asking; I don't see your point.)

-- Hannes

Re: [PATCH/RFC] Fix for default pager

[Dario Rodriguez]

On Tue, Jun 8, 2010 at 12:56 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 08.06.2010 16:39, schrieb Dario Rodriguez:
>>
>> On Tue, Jun 8, 2010 at 11:17 AM, Johannes Sixt<j.sixt@viscovery.net>
>>  wrote:
>>>
>>>  $ GIT_PAGER=/is/not/there git log
>>>  $ echo $?
>>>  141
>>>
>>> That's SIGPIPE, just as I would expect.
>>
>> As I said in the original thread...
>>
>> $ PAGER=/nothing/here ../git log
>> $ echo $?
>> 0
>
> That's no surprise with your toy repository: git-log has run to completion
> (without overrunning the pipe buffer) before the pager process that it
> forked can even execute its first instruction.
>

I cannot understand what's the point... I'm running git without
installing it, but why do you say "toy repository"?...

>> btw: I still think 'more' is much more sane fallback default than
>> 'less'... look (with your patch applied):
>>
>> $ ../git log
>> error: cannot run less: No such file or directory
>> commit 3274a12f940680612e3bfd3d022a0eab460c0f1f
>> Author: #######<#######@Maquina01.(none)>
>> Date:   Thu Jun 3 20:02:23 2010 +0200
>>
>>     OtherCom
>>
>> commit acf110f7c878a37e4a5af8499134df28da0e8ab3
>> Author: #######<#######@Maquina01.(none)>
>> Date:   Thu Jun 3 20:01:37 2010 +0200
>>
>>     inicial
>>
>
> How is this an argument for 'more'? (Just asking; I don't see your point.)
>

No problem, let me explain: 'more' is older and standard while
(correct me if not true) 'less' is not in POSIX:2008. I work in a lot
of Unix-like systems and I found 'more' as a standard, while less is
just sometimes installed... my error running 'less' is such an
argument for 'more' because:

$ PAGER=more ../git log
commit fccd640197e34dbff72954924ed5c76c42a4aac7
Author: ###### <######@Maquina01.(none)>
Date:   Tue Jun 8 19:23:23 2010 +0200

    commit
stdin: END

'more' is not a problem... and as a POSIX standard it's almost never a problem.

Re: [PATCH/RFC] Fix for default pager

[Johannes Sixt]

Am 08.06.2010 19:28, schrieb Dario Rodriguez:
> On Tue, Jun 8, 2010 at 12:56 PM, Johannes Sixt<j.sixt@viscovery.net>  wrote:
>> Am 08.06.2010 16:39, schrieb Dario Rodriguez:
>>>
>>> On Tue, Jun 8, 2010 at 11:17 AM, Johannes Sixt<j.sixt@viscovery.net>
>>>   wrote:
>>>>
>>>>   $ GIT_PAGER=/is/not/there git log
>>>>   $ echo $?
>>>>   141
>>>>
>>>> That's SIGPIPE, just as I would expect.
>>>
>>> As I said in the original thread...
>>>
>>> $ PAGER=/nothing/here ../git log
>>> $ echo $?
>>> 0
>>
>> That's no surprise with your toy repository: git-log has run to completion
>> (without overrunning the pipe buffer) before the pager process that it
>> forked can even execute its first instruction.
>>
>
> I cannot understand what's the point... I'm running git without
> installing it, but why do you say "toy repository"?...

Your repository has only 2 commits and its git log output is less than 
1kB, i.e., sufficiently small to fit in a pipe's buffer.

git log calls start_command to fork() the pager. The OS's scheduler does 
not run the newly forked process immediately; rather, git log goes on with 
its own business, writing output to the pipe that connects to the pager. 
Because your repository is so small, git log never has to wait that the 
pager drains the pipe. git log finally reaches exit(0). At this time, an 
atexit() handler (wait_for_pager()) finally calls finish_command() to wait 
for the pager.

This is the first time that the forked child process can run. Only now it 
turns out that the pager cannot be run. The child process closes the pipe 
and exits with an error, but it is too late: wait_for_pager() drops the 
error return code of finish_command() to the floor. The parent process 
(git log) can complete with the exit code that it was given earlier, 0.

Repeat your experiment with ./git log in git.git itself to see the difference.

-- Hannes

Re: [PATCH/RFC] Fix for default pager

[Dario Rodriguez]

On Tue, Jun 8, 2010 at 3:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Your repository has only 2 commits and its git log output is less than 1kB,
> i.e., sufficiently small to fit in a pipe's buffer.
>
> git log calls start_command to fork() the pager. The OS's scheduler does not
> run the newly forked process immediately; rather, git log goes on with its
> own business, writing output to the pipe that connects to the pager. Because
> your repository is so small, git log never has to wait that the pager drains
> the pipe. git log finally reaches exit(0). At this time, an atexit() handler
> (wait_for_pager()) finally calls finish_command() to wait for the pager.
>
> This is the first time that the forked child process can run. Only now it
> turns out that the pager cannot be run. The child process closes the pipe
> and exits with an error, but it is too late: wait_for_pager() drops the
> error return code of finish_command() to the floor. The parent process (git
> log) can complete with the exit code that it was given earlier, 0.
>
> Repeat your experiment with ./git log in git.git itself to see the
> difference.
>
> -- Hannes
>

Capisco & touché, with much more than 1k of info, git show ends with a
"Broken Pipe"... seems hard to detect for little, recently started
projects since I added more than 60k of scripts and I need to do 'git
show' to understand that the problem is a broken pipe.

Now, let me think about it... do we need the pager_preexec function? I
mean... it works fine without it, and the function is there because of
a faulty 'less'.

My problem is obvioulsly solved by adding PAGER=more in my default
environment, but I think this could be a litle bit embarrassing for a
new user, mostly in environments such this AIX :P

Cheers,
Dario

Re: [PATCH/RFC] Fix for default pager

[Andreas Ericsson]

On 06/08/2010 10:44 PM, Dario Rodriguez wrote:
> On Tue, Jun 8, 2010 at 3:59 PM, Johannes Sixt<j.sixt@viscovery.net>  wrote:
>> Your repository has only 2 commits and its git log output is less than 1kB,
>> i.e., sufficiently small to fit in a pipe's buffer.
>>
>> git log calls start_command to fork() the pager. The OS's scheduler does not
>> run the newly forked process immediately; rather, git log goes on with its
>> own business, writing output to the pipe that connects to the pager. Because
>> your repository is so small, git log never has to wait that the pager drains
>> the pipe. git log finally reaches exit(0). At this time, an atexit() handler
>> (wait_for_pager()) finally calls finish_command() to wait for the pager.
>>
>> This is the first time that the forked child process can run. Only now it
>> turns out that the pager cannot be run. The child process closes the pipe
>> and exits with an error, but it is too late: wait_for_pager() drops the
>> error return code of finish_command() to the floor. The parent process (git
>> log) can complete with the exit code that it was given earlier, 0.
>>
>> Repeat your experiment with ./git log in git.git itself to see the
>> difference.
>>
>> -- Hannes
>>
> 
> Capisco&  touché, with much more than 1k of info, git show ends with a
> "Broken Pipe"... seems hard to detect for little, recently started
> projects since I added more than 60k of scripts and I need to do 'git
> show' to understand that the problem is a broken pipe.
> 
> Now, let me think about it... do we need the pager_preexec function? I
> mean... it works fine without it, and the function is there because of
> a faulty 'less'.
> 
> My problem is obvioulsly solved by adding PAGER=more in my default
> environment, but I think this could be a litle bit embarrassing for a
> new user, mostly in environments such this AIX :P
> 

Catering to AIX by default seems stupid beyond belief. AIX users today
are, without fail, accustomed to having to tweak more or less everything
to make the system run smoothly with modern applications (where "modern"
is a generous term, including everything that's been written in the last
10 or so years).

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Re: [PATCH/RFC] Fix for default pager

[Tor Arntsen]

On Tue, Jun 8, 2010 at 23:33, Andreas Ericsson <ae@op5.se> wrote:

> Catering to AIX by default seems stupid beyond belief. AIX users today
> are, without fail, accustomed to having to tweak more or less everything
> to make the system run smoothly with modern applications (where "modern"
> is a generous term, including everything that's been written in the last
> 10 or so years).

AIX doesn't come with 'less', because it doesn't really need one.
'more' on AIX can page backwards/forwards in piped data (unlike 'more'
on Linux etc.), thus negating the most common need for installing
'less' elsewhere. (The other big feature of less, that it by default
clears the screen just as you had found what you wanted to read and
were about to apply it to the command line before 'poof' - gone) is so
annoying to me that I routinely sets PAGER=more before using Git.

If it's true that Git should demand 'less' to be installed before
being usable out of the box.. well, that's just plain silly.

-Tor

Re: [PATCH/RFC] Fix for default pager

[Miles Bader]

Tor Arntsen <tor@spacetec.no> writes:
> AIX doesn't come with 'less', because it doesn't really need one.
> 'more' on AIX can page backwards/forwards in piped data (unlike 'more'
> on Linux etc.), thus negating the most common need for installing
> 'less' elsewhere.

Less has about a zillion features that more [traditionally] doesn't
have, and even some of the more esoteric ones can be quite useful for
git.  For instance you can determine in a great deal of detail exactly
how/when/if it clears the screen/shows a prompt/waits for user input
before exiting, etc.

-Miles

-- 
(\(\
(^.^)
(")")
*This is the cute bunny virus, please copy this into your sig so it can spread.

Re: [PATCH/RFC] Fix for default pager

[Jeff King]

On Wed, Jun 09, 2010 at 11:08:49AM +0200, Tor Arntsen wrote:

> If it's true that Git should demand 'less' to be installed before
> being usable out of the box.. well, that's just plain silly.

It depends on how you define "out of the box". The person compiling it
just needs to set DEFAULT_PAGER appropriately for their system. "less"
is a sane choice for most modern systems. But we can make it even easier
on AIX people with:

diff --git a/Makefile b/Makefile
index 34b7dd5..6ad0aca 100644
--- a/Makefile
+++ b/Makefile
@@ -930,6 +930,7 @@ ifeq ($(uname_S),NetBSD)
 	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),AIX)
+	DEFAULT_PAGER = more
 	NO_STRCASESTR=YesPlease
 	NO_MEMMEM = YesPlease
 	NO_MKDTEMP = YesPlease

That won't do automagic run-time detection if you have "less" installed,
but given your claim that AIX's "more" actually doesn't suck, it's
probably a good default. People who care can set their PAGER environment
variable.

-Peff

Re: [PATCH/RFC] Fix for default pager

[Tor Arntsen]

On Thu, Jun 10, 2010 at 10:29, Jeff King <peff@peff.net> wrote:
> On Wed, Jun 09, 2010 at 11:08:49AM +0200, Tor Arntsen wrote:
>
>> If it's true that Git should demand 'less' to be installed before
>> being usable out of the box.. well, that's just plain silly.
>
> It depends on how you define "out of the box". The person compiling it
> just needs to set DEFAULT_PAGER appropriately for their system. "less"
> is a sane choice for most modern systems. But we can make it even easier
> on AIX people with:
>
> diff --git a/Makefile b/Makefile
> index 34b7dd5..6ad0aca 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -930,6 +930,7 @@ ifeq ($(uname_S),NetBSD)
>        HAVE_PATHS_H = YesPlease
>  endif
>  ifeq ($(uname_S),AIX)
> +       DEFAULT_PAGER = more
>        NO_STRCASESTR=YesPlease
>        NO_MEMMEM = YesPlease
>        NO_MKDTEMP = YesPlease

That looks good to me.

> That won't do automagic run-time detection if you have "less" installed,
> but given your claim that AIX's "more" actually doesn't suck, it's
> probably a good default.

It does the paging just as 'less', yes. I can page 'git log' or any
other output. backwards/forwards as I wish. I'm not aware of any
'more' other than the AIX one that does this. Even the archaic
(pre-AIX5) versions did. The AIX 'more' also have several more
in-buffer options compared to e.g. Linux 'more'.  It sits somewhere
between traditional 'more' and 'less', feature-wise. (The AIX 6
version even does the, to me, annoying less-style erase-screen, unless
turned off by an option or environment variable.)

> People who care can set their PAGER environment
> variable.

-Tor

Re: [PATCH/RFC] Fix for default pager

[Jeff King]

On Thu, Jun 10, 2010 at 10:48:31AM +0200, Tor Arntsen wrote:

> That looks good to me.

OK, here it is with a commit message. Other systems might want the same,
I guess (Solaris, IRIX?). I'm cc'ing Brandon, who might have some input.

Note that this is completely untested by me, as all of my AIX boxen have
gone away in the past few months (yay!).

-- >8 --
Subject: [PATCH] Makefile: default pager on AIX to "more"

AIX doesn't ship with "less" by default, and their "more" is
more featureful than average, so the latter is a more
sensible choice.  People who really want less can set the
compile-time option themselves, or users can set $PAGER.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 34b7dd5..6ad0aca 100644
--- a/Makefile
+++ b/Makefile
@@ -930,6 +930,7 @@ ifeq ($(uname_S),NetBSD)
 	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),AIX)
+	DEFAULT_PAGER = more
 	NO_STRCASESTR=YesPlease
 	NO_MEMMEM = YesPlease
 	NO_MKDTEMP = YesPlease
-- 
1.7.1.514.g71ed8

Re: [PATCH/RFC] Fix for default pager

[Tor Arntsen]

On 06/10/10 10:59, Jeff King wrote:
> On Thu, Jun 10, 2010 at 10:48:31AM +0200, Tor Arntsen wrote:
> 
>> That looks good to me.
> 
> OK, here it is with a commit message. Other systems might want the same,
> I guess (Solaris, IRIX?). I'm cc'ing Brandon, who might have some input.
> 
> Note that this is completely untested by me, as all of my AIX boxen have
> gone away in the past few months (yay!).

I did a re-build and test. Everything works fine.

Tested-by: Tor Arntsen <tor@spacetec.no>

-Tor

-- >8 --
Subject: [PATCH] Makefile: default pager on AIX to "more"

AIX doesn't ship with "less" by default, and their "more" is
more featureful than average, so the latter is a more
sensible choice.  People who really want less can set the
compile-time option themselves, or users can set $PAGER.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 34b7dd5..6ad0aca 100644
--- a/Makefile
+++ b/Makefile
@@ -930,6 +930,7 @@ ifeq ($(uname_S),NetBSD)
 	HAVE_PATHS_H = YesPlease
 endif
 ifeq ($(uname_S),AIX)
+	DEFAULT_PAGER = more
 	NO_STRCASESTR=YesPlease
 	NO_MEMMEM = YesPlease
 	NO_MKDTEMP = YesPlease
-- 1.7.1.514.g71ed8 

Re: [PATCH/RFC] Fix for default pager

[Dario Rodriguez]

On Thu, Jun 10, 2010 at 6:24 AM, Tor Arntsen <tor@spacetec.no> wrote:
> On 06/10/10 10:59, Jeff King wrote:
>> On Thu, Jun 10, 2010 at 10:48:31AM +0200, Tor Arntsen wrote:
>>
>>> That looks good to me.
>>
>> OK, here it is with a commit message. Other systems might want the same,
>> I guess (Solaris, IRIX?). I'm cc'ing Brandon, who might have some input.
>>
>> Note that this is completely untested by me, as all of my AIX boxen have
>> gone away in the past few months (yay!).
>
> I did a re-build and test. Everything works fine.
>
> Tested-by: Tor Arntsen <tor@spacetec.no>
>
> -Tor
>
> -- >8 --
> Subject: [PATCH] Makefile: default pager on AIX to "more"
>
> AIX doesn't ship with "less" by default, and their "more" is
> more featureful than average, so the latter is a more
> sensible choice.  People who really want less can set the
> compile-time option themselves, or users can set $PAGER.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Makefile |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 34b7dd5..6ad0aca 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -930,6 +930,7 @@ ifeq ($(uname_S),NetBSD)
>        HAVE_PATHS_H = YesPlease
>  endif
>  ifeq ($(uname_S),AIX)
> +       DEFAULT_PAGER = more
>        NO_STRCASESTR=YesPlease
>        NO_MEMMEM = YesPlease
>        NO_MKDTEMP = YesPlease
> -- 1.7.1.514.g71ed8
>

It's ok for me, I will test it on AIX today, if I've some time.

Cheers,
Dario

Re: [PATCH/RFC] Fix for default pager

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Subject: [PATCH] Makefile: default pager on AIX to "more"
>
> AIX doesn't ship with "less" by default, and their "more" is
> more featureful than average, so the latter is a more
> sensible choice.  People who really want less can set the
> compile-time option themselves, or users can set $PAGER.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Makefile |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 34b7dd5..6ad0aca 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -930,6 +930,7 @@ ifeq ($(uname_S),NetBSD)
>  	HAVE_PATHS_H = YesPlease
>  endif
>  ifeq ($(uname_S),AIX)
> +	DEFAULT_PAGER = more
>  	NO_STRCASESTR=YesPlease
>  	NO_MEMMEM = YesPlease
>  	NO_MKDTEMP = YesPlease

A very sensible approach.  Thanks.

Re: [PATCH/RFC] Fix for default pager

[Brandon Casey]

On 06/10/2010 03:59 AM, Jeff King wrote:
> On Thu, Jun 10, 2010 at 10:48:31AM +0200, Tor Arntsen wrote:
> 
>> That looks good to me.
> 
> OK, here it is with a commit message. Other systems might want the same,
> I guess (Solaris, IRIX?). I'm cc'ing Brandon, who might have some input.

Yes, I currently set DEFAULT_PAGER to 'more' in my config.mak file
on both of these platforms.  The 'more' on IRIX is decent (it can go
backwards), but the 'more' on Solaris sucks.  I've seen 'less' on some
newer versions of Solaris.  Is it a standard component yet?

So, I think it's appropriate to set DEFAULT_PAGER on IRIX.  There can't
be many users anyway.  It's probably appropriate to set it on Solaris
too, if 'less' is not a commonly installed component on modern systems.
I wonder how surprised existing git users will be, for those on Solaris
platforms that have 'less' installed, when Solaris's crappy 'more'
becomes their pager.

Actually, there is a 'more' in /usr/xpg4/bin that is much better, but
it is not being used when DEFAULT_PAGER is set to 'more'.  Junio
created the SANE_TOOL_PATH hack to add this additional path to the
search path, but it is only implemented in git-sh-setup, so it only
has effect for git scripts.  Maybe it should be added to setup_path().

But, I also think it would be nice if git fell back to the 'cat'
behavior when it fails to spawn the pager, because the following error
is not very informative:

   casey@<a_solaris_box> # git log
   sh: less: not found
   Broken Pipe

-brandon


> Note that this is completely untested by me, as all of my AIX boxen have
> gone away in the past few months (yay!).
> 
> -- >8 --
> Subject: [PATCH] Makefile: default pager on AIX to "more"
> 
> AIX doesn't ship with "less" by default, and their "more" is
> more featureful than average, so the latter is a more
> sensible choice.  People who really want less can set the
> compile-time option themselves, or users can set $PAGER.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Makefile |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 34b7dd5..6ad0aca 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -930,6 +930,7 @@ ifeq ($(uname_S),NetBSD)
>  	HAVE_PATHS_H = YesPlease
>  endif
>  ifeq ($(uname_S),AIX)
> +	DEFAULT_PAGER = more
>  	NO_STRCASESTR=YesPlease
>  	NO_MEMMEM = YesPlease
>  	NO_MKDTEMP = YesPlease

Re: [PATCH/RFC] Fix for default pager

[Tor Arntsen]

On 06/15/10 18:11, Brandon Casey wrote:

> Yes, I currently set DEFAULT_PAGER to 'more' in my config.mak file
> on both of these platforms.  The 'more' on IRIX is decent (it can go
> backwards), but the 'more' on Solaris sucks.  I've seen 'less' on some
> newer versions of Solaris.  Is it a standard component yet?

'less' appears to be standard on Solaris 10 (5.10). My older 5.8 box
is offline so I can't check that one. AFAIK the difference between
Solaris 8 and Solaris 10 is pretty big so I suspect there's no 'less' 
there but I can't say for certain. I could try to fire up that old 
box and check though.

-Tor

Re: [PATCH/RFC] Fix for default pager

[Nazri Ramliy]

On Wed, Jun 16, 2010 at 12:11 AM, Brandon Casey
<brandon.casey.ctr@nrlssc.navy.mil> wrote:
> But, I also think it would be nice if git fell back to the 'cat'
> behavior when it fails to spawn the pager, because the following error
> is not very informative:

Falling back to 'cat' is nice except when you are on a remote machine
with very bad latency.

nazri

Re: [PATCH/RFC] Fix for default pager

[Jeff King]

On Tue, Jun 15, 2010 at 11:11:35AM -0500, Brandon Casey wrote:

> So, I think it's appropriate to set DEFAULT_PAGER on IRIX.  There can't
> be many users anyway.  It's probably appropriate to set it on Solaris
> too, if 'less' is not a commonly installed component on modern systems.
> I wonder how surprised existing git users will be, for those on Solaris
> platforms that have 'less' installed, when Solaris's crappy 'more'
> becomes their pager.

I'm a little worried about that, too. On the other hand, wouldn't people
who actually care about less have set PAGER already, to use it for
things like "man"?

> But, I also think it would be nice if git fell back to the 'cat'
> behavior when it fails to spawn the pager, because the following error
> is not very informative:
> 
>    casey@<a_solaris_box> # git log
>    sh: less: not found
>    Broken Pipe

The pager command is executed by the shell these days. Perhaps we should
simply set DEFAULT_PAGER on these platforms to "less || more || cat",
which seems to work from my simple tests. If that is too hack-ish (e.g.,
we really care about "does less exist", not "did it fail"), we can do a
more invasive patch (or even provide a "git-pager" shell script helper
to do a more thorough job).

-Peff

Re: [PATCH/RFC] Fix for default pager

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 8, 2010 at 21:33, Andreas Ericsson <ae@op5.se> wrote:
> Catering to AIX by default seems stupid beyond belief. AIX users today
> are, without fail, accustomed to having to tweak more or less everything
> to make the system run smoothly with modern applications (where "modern"
> is a generous term, including everything that's been written in the last
> 10 or so years).

BSD users are also accustomed to that to a smaller degree. But as long
as they keep submitting portability patches to keep their ports up to
date, I don't see why those patches shouldn't be accepted. If they're
otherwise fit for inclusion that is.

Re: [PATCH/RFC] Fix for default pager

[Jeff King]

On Mon, Jun 07, 2010 at 08:58:08PM -0300, Dario Rodriguez wrote:

> Default pager was 'less' even when some systems such AIX and other basic
> or old systems do NOT have 'less' installed. In such case, git just
> does not display anything in pager-enabled functionalities such as 'git log'
> or 'git show', exiting with status 0.
> 
> With this patch, git will not use DEFAULT_PAGER macro anymore, instead,
> git will look for 'less' and 'more' in the most common paths.
> If there is no pager, returns NULL as if it's 'cat'.

Run-time pager detection seems like a reasonable goal, I guess, but...

> -const char *git_pager(int stdout_is_tty)
> +static int is_executable(const char *name)
> +{
> +	struct stat st;
> +
> +	if (stat(name, &st) ||
> +	    !S_ISREG(st.st_mode))
> +		return 0;
> +
> +#ifdef WIN32
> +{	/* cannot trust the executable bit, peek into the file instead */
> +	char buf[3] = { 0 };
> +	int n;
> +	int fd = open(name, O_RDONLY);
> +	st.st_mode &= ~S_IXUSR;
> +	if (fd >= 0) {
> +		n = read(fd, buf, 2);
> +		if (n == 2)
> +			/* DOS executables start with "MZ" */
> +			if (!strcmp(buf, "#!") || !strcmp(buf, "MZ"))
> +				st.st_mode |= S_IXUSR;
> +		close(fd);
> +	}
> +}
> +#endif
> +	return st.st_mode & S_IXUSR;
> +}
> +
> +const char *git_pager(int stdout_is_tty) 
>  {
> +	static const char *pager_bins[] =
> +		{ "less", "more", NULL };
> +	static const char *common_binary_paths[] =
> +		{ "/bin/","/usr/bin/","/usr/local/bin/",NULL };

...must we really add code with such ugliness as magic PATHs and DOS
magic numbers?

Right now we fall back to just exec-ing "less". Could we instead just
try to exec "less", if that fails then "more", and then finally "cat"?

That would have almost the same effect and would be much simpler,
wouldn't it? The exceptions I can think of are:

  - we would actually run "cat" in the final case, instead of optimizing
    it out.

  - "git var GIT_PAGER" wouldn't handle this automatically

-Peff

Re: [PATCH/RFC] Fix for default pager

[Johannes Sixt]

Am 6/8/2010 7:29, schrieb Jeff King:
> On Mon, Jun 07, 2010 at 08:58:08PM -0300, Dario Rodriguez wrote:
>> +#ifdef WIN32
>> +{	/* cannot trust the executable bit, peek into the file instead */
>> +	char buf[3] = { 0 };
>> +	int n;
>> +	int fd = open(name, O_RDONLY);
>> +	st.st_mode &= ~S_IXUSR;
>> +	if (fd >= 0) {
>> +		n = read(fd, buf, 2);
>> +		if (n == 2)
>> +			/* DOS executables start with "MZ" */
>> +			if (!strcmp(buf, "#!") || !strcmp(buf, "MZ"))
>> +				st.st_mode |= S_IXUSR;
>> +		close(fd);
>> +	}
>> +}
>> +#endif
>> +	return st.st_mode & S_IXUSR;
>> +}
>> +
>> +const char *git_pager(int stdout_is_tty) 
>>  {
>> +	static const char *pager_bins[] =
>> +		{ "less", "more", NULL };
>> +	static const char *common_binary_paths[] =
>> +		{ "/bin/","/usr/bin/","/usr/local/bin/",NULL };
> 
> ...must we really add code with such ugliness as magic PATHs and DOS
> magic numbers?

Yes and no.

Coding DOS magic numbers is bad because you could wrapped a real pager in
a shell script (even on Windows).

These days, start_command() is able to report ENOENT if it cannot run a
program because it does not exist. However, right in the case of
git_pager() this capability is disabled because it sets this magic
preexec_cb that waits for data in the child process before it execs the
real pager.

If we could get rid of preexec_cb, we could make this work much more
pleasantly. pager_preexec waits for data on stdin; Linus added it to work
around a faulty 'less'. Do we still need it?

-- Hannes

Re: [PATCH/RFC] Fix for default pager

[Dario Rodriguez]

On Tue, Jun 8, 2010 at 2:29 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Jun 07, 2010 at 08:58:08PM -0300, Dario Rodriguez wrote:
>
>> Default pager was 'less' even when some systems such AIX and other basic
>> or old systems do NOT have 'less' installed. In such case, git just
>> does not display anything in pager-enabled functionalities such as 'git log'
>> or 'git show', exiting with status 0.
>>
>> With this patch, git will not use DEFAULT_PAGER macro anymore, instead,
>> git will look for 'less' and 'more' in the most common paths.
>> If there is no pager, returns NULL as if it's 'cat'.
>
> Run-time pager detection seems like a reasonable goal, I guess, but...
>
>> -const char *git_pager(int stdout_is_tty)
>> +static int is_executable(const char *name)
>> +{
>> +     struct stat st;
>> +
>> +     if (stat(name, &st) ||
>> +         !S_ISREG(st.st_mode))
>> +             return 0;
>> +
>> +#ifdef WIN32
>> +{    /* cannot trust the executable bit, peek into the file instead */
>> +     char buf[3] = { 0 };
>> +     int n;
>> +     int fd = open(name, O_RDONLY);
>> +     st.st_mode &= ~S_IXUSR;
>> +     if (fd >= 0) {
>> +             n = read(fd, buf, 2);
>> +             if (n == 2)
>> +                     /* DOS executables start with "MZ" */
>> +                     if (!strcmp(buf, "#!") || !strcmp(buf, "MZ"))
>> +                             st.st_mode |= S_IXUSR;
>> +             close(fd);
>> +     }
>> +}
>> +#endif
>> +     return st.st_mode & S_IXUSR;
>> +}
>> +
>> +const char *git_pager(int stdout_is_tty)
>>  {
>> +     static const char *pager_bins[] =
>> +             { "less", "more", NULL };
>> +     static const char *common_binary_paths[] =
>> +             { "/bin/","/usr/bin/","/usr/local/bin/",NULL };
>
> ...must we really add code with such ugliness as magic PATHs and DOS
> magic numbers?
>

I copied the function 'is_executable' from 'help.c' so we already have
such code... :p

> Right now we fall back to just exec-ing "less". Could we instead just
> try to exec "less", if that fails then "more", and then finally "cat"?
>

is such a good idea but right now, 'git_pager' is not exec-ing, it's
just setting up a pager. If you set-up the pager based on wich one
fails in it's execution, you must avoid usage of this function, since
it will always return 'less' (or 'more...). What I posted is
transparent to any other function; 'git_pager' will be called
returning an existent, working pager, so the flow is the same, however
I like your proposal too, and should be considered.

> That would have almost the same effect and would be much simpler,
> wouldn't it? The exceptions I can think of are:
>
>  - we would actually run "cat" in the final case, instead of optimizing
>    it out.
>

Actually pager is being set to NULL if it's 'cat'... what's git doing
with a NULL pager?

>  - "git var GIT_PAGER" wouldn't handle this automatically
>
> -Peff
>

Blessing,
Dario

Re: [PATCH/RFC] Fix for default pager

[Erik Faye-Lund]

On Tue, Jun 8, 2010 at 2:24 PM, Dario Rodriguez <soft.d4rio@gmail.com> wrote:
> On Tue, Jun 8, 2010 at 2:29 AM, Jeff King <peff@peff.net> wrote:
>> On Mon, Jun 07, 2010 at 08:58:08PM -0300, Dario Rodriguez wrote:
>>
>>> Default pager was 'less' even when some systems such AIX and other basic
>>> or old systems do NOT have 'less' installed. In such case, git just
>>> does not display anything in pager-enabled functionalities such as 'git log'
>>> or 'git show', exiting with status 0.
>>>
>>> With this patch, git will not use DEFAULT_PAGER macro anymore, instead,
>>> git will look for 'less' and 'more' in the most common paths.
>>> If there is no pager, returns NULL as if it's 'cat'.
>>
>> Run-time pager detection seems like a reasonable goal, I guess, but...
>>
>>> -const char *git_pager(int stdout_is_tty)
>>> +static int is_executable(const char *name)
>>> +{
>>> +     struct stat st;
>>> +
>>> +     if (stat(name, &st) ||
>>> +         !S_ISREG(st.st_mode))
>>> +             return 0;
>>> +
>>> +#ifdef WIN32
>>> +{    /* cannot trust the executable bit, peek into the file instead */
>>> +     char buf[3] = { 0 };
>>> +     int n;
>>> +     int fd = open(name, O_RDONLY);
>>> +     st.st_mode &= ~S_IXUSR;
>>> +     if (fd >= 0) {
>>> +             n = read(fd, buf, 2);
>>> +             if (n == 2)
>>> +                     /* DOS executables start with "MZ" */
>>> +                     if (!strcmp(buf, "#!") || !strcmp(buf, "MZ"))
>>> +                             st.st_mode |= S_IXUSR;
>>> +             close(fd);
>>> +     }
>>> +}
>>> +#endif
>>> +     return st.st_mode & S_IXUSR;
>>> +}
>>> +
>>> +const char *git_pager(int stdout_is_tty)
>>>  {
>>> +     static const char *pager_bins[] =
>>> +             { "less", "more", NULL };
>>> +     static const char *common_binary_paths[] =
>>> +             { "/bin/","/usr/bin/","/usr/local/bin/",NULL };
>>
>> ...must we really add code with such ugliness as magic PATHs and DOS
>> magic numbers?
>>
>
> I copied the function 'is_executable' from 'help.c' so we already have
> such code... :p
>

How about just un-staticifying the version in help.c instead of
duplicating it, then? That way we'd have half the ugliness for this
purpose...

-- 
Erik "kusma" Faye-Lund