Re: [PATCH/RFC] Fix for default pager
- From
Dario Rodriguez <soft.d4rio@gmail.com>
- Date
- Jun 8, 2010, 12:24 UTC
- Message-ID
- <AANLkTinxcrIV2TM966EkOC_crR0bHdNllEIdibz4gGjd@mail.gmail.com>
- In-Reply-To
- <20100608052929.GA15156@coredump.intra.peff.net>
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