threads / rfc / 24037

RFC patchFix for default pager

Subject: [PATCH/RFC] Fix for default pager

## tl;dr

29 messages between Jun 7, 2010 and Jun 16, 2010. Diffs are folded; open one to read it.

replies: 28people: 12as markdown or json

Dario Rodriguez· Jun 7, 2010, 23:58 UTC · lore
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(-)
Show changes to pager.c +73 −7
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
Ben Walton· Jun 8, 2010, 00:04 UTC · re: Dario Rodriguez · lore

Re: [PATCH/RFC] Fix for default pager

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
Dario Rodriguez· Jun 8, 2010, 02:14 UTC · re: Ben Walton · lore

Re: [PATCH/RFC] Fix for default pager

On Mon, Jun 7, 2010 at 9:04 PM, Ben Walton <bwalton@artsci.utoronto.ca> wrote:
Show 6 quoted lines
>
> 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?

Jeff King· Jun 8, 2010, 05:35 UTC · re: Dario Rodriguez · lore

Re: [PATCH/RFC] Fix for default pager

On Mon, Jun 07, 2010 at 11:14:58PM -0300, Dario Rodriguez wrote:
Show 15 quoted lines
> 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.

Show 7 quoted lines
> 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
Dario Rodriguez· Jun 8, 2010, 13:49 UTC · re: Jeff King · lore

Re: [PATCH/RFC] Fix for default pager

On Tue, Jun 8, 2010 at 2:35 AM, Jeff King <peff@peff.net> wrote:
Show 5 quoted lines
> 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).
Johannes Sixt· Jun 8, 2010, 14:17 UTC · re: Dario Rodriguez · lore

Re: [PATCH/RFC] Fix for default pager

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
Show changes to pager.c +0 −4
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
Dario Rodriguez· Jun 8, 2010, 14:39 UTC · re: Johannes Sixt · lore

Re: [PATCH/RFC] Fix for default pager

On Tue, Jun 8, 2010 at 11:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 13 quoted lines
> 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
Show 23 quoted lines
> 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
Johannes Sixt· Jun 8, 2010, 15:56 UTC · re: Dario Rodriguez · lore

Re: [PATCH/RFC] Fix for default pager

Am 08.06.2010 16:39, schrieb Dario Rodriguez:
Show 12 quoted lines
> 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.

Show 17 quoted lines
> 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
Dario Rodriguez· Jun 8, 2010, 17:28 UTC · re: Johannes Sixt · lore

Re: [PATCH/RFC] Fix for default pager

On Tue, Jun 8, 2010 at 12:56 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 21 quoted lines
> 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"?...

Show 20 quoted lines
>> 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.
Johannes Sixt· Jun 8, 2010, 18:59 UTC · re: Dario Rodriguez · lore

Re: [PATCH/RFC] Fix for default pager

Am 08.06.2010 19:28, schrieb Dario Rodriguez:
Show 25 quoted lines
> 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
Dario Rodriguez· Jun 8, 2010, 20:44 UTC · re: Johannes Sixt · lore

Re: [PATCH/RFC] Fix for default pager

On Tue, Jun 8, 2010 at 3:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 21 quoted lines
> 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

Andreas Ericsson· Jun 8, 2010, 21:33 UTC · re: Dario Rodriguez · lore

Re: [PATCH/RFC] Fix for default pager

On 06/08/2010 10:44 PM, Dario Rodriguez wrote:
Show 36 quoted lines
> 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.
Tor Arntsen· Jun 9, 2010, 09:08 UTC · re: Andreas Ericsson · lore

Re: [PATCH/RFC] Fix for default pager

On Tue, Jun 8, 2010 at 23:33, Andreas Ericsson <ae@op5.se> wrote:
Show 5 quoted lines
> 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
Miles Bader· Jun 9, 2010, 09:29 UTC · re: Tor Arntsen · lore

Re: [PATCH/RFC] Fix for default pager

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.
Jeff King· Jun 10, 2010, 08:29 UTC · re: Tor Arntsen · lore

Re: [PATCH/RFC] Fix for default pager

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:

Show changes to Makefile +1 −1
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
Tor Arntsen· Jun 10, 2010, 08:48 UTC · re: Jeff King · lore

Re: [PATCH/RFC] Fix for default pager

On Thu, Jun 10, 2010 at 10:29, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> 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
Jeff King· Jun 10, 2010, 08:59 UTC · re: Tor Arntsen · lore

Re: [PATCH/RFC] Fix for default pager

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(-)
Show changes to Makefile +1 −0
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
Tor Arntsen· Jun 10, 2010, 09:24 UTC · re: Jeff King · lore

Re: [PATCH/RFC] Fix for default pager

On 06/10/10 10:59, Jeff King wrote:
Show 9 quoted lines
> 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(-)
Show changes to Makefile +1 −1
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 
Dario Rodriguez· Jun 10, 2010, 11:31 UTC · re: Tor Arntsen · lore

Re: [PATCH/RFC] Fix for default pager

On Thu, Jun 10, 2010 at 6:24 AM, Tor Arntsen <tor@spacetec.no> wrote:
Show 44 quoted lines
> 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

Junio C Hamano· Jun 10, 2010, 14:55 UTC · re: Jeff King · lore

Re: [PATCH/RFC] Fix for default pager

Jeff King <peff@peff.net> writes:
Show 24 quoted lines
> 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.
Brandon Casey· Jun 15, 2010, 16:11 UTC · re: Jeff King · lore

Re: [PATCH/RFC] Fix for default pager

On 06/10/2010 03:59 AM, Jeff King wrote:
Show 6 quoted lines
> 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
Show 28 quoted lines
> 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
Tor Arntsen· Jun 15, 2010, 16:32 UTC · re: Brandon Casey · lore

Re: [PATCH/RFC] Fix for default pager

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
Nazri Ramliy· Jun 16, 2010, 01:34 UTC · re: Brandon Casey · lore

Re: [PATCH/RFC] Fix for default pager

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
Jeff King· Jun 16, 2010, 06:28 UTC · re: Brandon Casey · lore

Re: [PATCH/RFC] Fix for default pager

On Tue, Jun 15, 2010 at 11:11:35AM -0500, Brandon Casey wrote:
Show 6 quoted lines
> 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"?

Show 7 quoted lines
> 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
Ævar Arnfjörð Bjarmason· Jun 9, 2010, 18:57 UTC · re: Andreas Ericsson · lore

Re: [PATCH/RFC] Fix for default pager

On Tue, Jun 8, 2010 at 21:33, Andreas Ericsson <ae@op5.se> wrote:
Show 5 quoted lines
> 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.

Jeff King· Jun 8, 2010, 05:29 UTC · re: Dario Rodriguez · lore

Re: [PATCH/RFC] Fix for default pager

On Mon, Jun 07, 2010 at 08:58:08PM -0300, Dario Rodriguez wrote:
Show 8 quoted lines
> 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...
Show 34 quoted lines
> -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
Johannes Sixt· Jun 8, 2010, 06:12 UTC · re: Jeff King · lore

Re: [PATCH/RFC] Fix for default pager

Am 6/8/2010 7:29, schrieb Jeff King:
Show 29 quoted lines
> 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
Dario Rodriguez· Jun 8, 2010, 12:24 UTC · re: Jeff King · lore

Re: [PATCH/RFC] Fix for default pager

On Tue, Jun 8, 2010 at 2:29 AM, Jeff King <peff@peff.net> wrote:
Show 51 quoted lines
> 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.

Show 6 quoted lines
> 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

Erik Faye-Lund· Jun 8, 2010, 14:04 UTC · re: Dario Rodriguez · lore

Re: [PATCH/RFC] Fix for default pager

On Tue, Jun 8, 2010 at 2:24 PM, Dario Rodriguez <soft.d4rio@gmail.com> wrote:
Show 56 quoted lines
> 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

← back to recent threads