Re: [Man-db-devel] [PATCH] Add fallback pager if the compile time default is not executable

[Colin Watson]

On Fri, Dec 15, 2017 at 11:06:07PM -0800, address@hidden wrote:
> diff --git a/src/man.c b/src/man.c
> index 6bc9642c..a3b72b96 100644
> --- a/src/man.c
> +++ b/src/man.c
> @@ -4020,6 +4020,22 @@ static const char **get_section_list (void)
>       }
>  }
> 
> +static int configuredPagerIsExecutable(void)

We name functions_like_this rather than functionsLikeThis in man-db.

> +     // The sh script to check PAGER executability.
> +     //
> +     // We are being quite paranoid with the unalias and unset. They guard
> +     // from an sh implementation which allows exporting or initializing
> +     // aliased names or functions, and the unlikely possibility of 'command'
> +     // being spoofed by such an exported aliased name or function.
> +     char command[] =
> +             "\\unset -f unalias command less more cat\n"
> +             "\\unalias -a\n"
> +             "\\command -v " PAGER " 1>/dev/null 2>/dev/null\n";

"command -v" is an extension not historically implemented in all shells,
so I'm not really happy about using it here.  In general I work very
hard in man-db to avoid having to depend on the shell at run-time.

Instead, I suggest picking out the first word of the configured pager
(since this is based on configure parameters rather than environment
variables, we can probably get away without working too hard here), and
using man-db's pathsearch_executable function to see whether it exists
as an executable on $PATH.

> @@ -4119,8 +4135,16 @@ int main (int argc, char *argv[])
>               pager = getenv ("MANPAGER");
>               if (pager == NULL) {
>                       pager = getenv ("PAGER");
> -                     if (pager == NULL)
> -                             pager = get_def_user ("pager", PAGER);
> +                     if (pager == NULL) {
> +                             pager = get_def_user ("pager", NULL);
> +                             if (pager == NULL) {

The indentation level is getting rather deep here; it might be worth
reorganising this whole if block to be more like this:

  if (pager == NULL)
          pager = getenv ("MANPAGER");
  if (pager == NULL)
          pager = getenv ("PAGER");
  if (pager == NULL)
          ...

> +                                     if (configuredPagerIsExecutable ()) {

I think it'd be somewhat better design to pass the pager as an argument
to this function rather than having the function look at the
configuration itself, i.e. "if (pager_is_executable (PAGER))".

Thanks,

-- 
Colin Watson                                       address@hidden