Re: [PATCH/RFC] Fix for default pager

[Dario Rodriguez <soft.d4rio@gmail.com>]

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