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

[Colin Watson]

Sorry for the delay; I had a few too many things going on in the run-up
to Christmas.

This is an improvement, thanks; but I think it needs one more round of
tightening up.  Detailed comments follow.

On Sun, Dec 24, 2017 at 04:09:02AM -0800, address@hidden wrote:
> The same thing, but without using sh.
> 
> And also a documentation update.
> 
> In version 4 of the patch a memory leak is plugged.
> 
> diff --git a/man/man1/man.man1 b/man/man1/man.man1
> index 4df2823f..23a059e0 100644
> --- a/man/man1/man.man1
> +++ b/man/man1/man.man1
> @@ -874,7 +874,12 @@ Specify which output pager to use.
>  By default,
>  .B %man%
>  uses
> -.BR "%pager%" .
> +.BR "%pager%" ,
> +further falling back to

Drop the word "further", I think.

> +.BR "%cat%"

This should be just:

  .B %cat%

... since there's only one argument to this macro.  (Same with %pager%
two lines below.)

%cat% isn't currently substituted in our generated manual pages.  You'll
need to edit man/replace.sin.in to add this substitution.

> +in case

Just "if".

> +.BR "%pager%"
> +is not found or is not executable.
>  This option overrides the
>  .RB $ MANPAGER
>  environment variable, which in turn overrides the
> @@ -1240,7 +1245,11 @@ is used in preference), its value is used as the name 
> of the program used to
>  display the manual page.
>  By default,
>  .B %pager%
> -is used.
> +is used, with a further fallback to
> +.B %cat%
> +if
> +.B %pager%
> +were not to be found.

Not sure about the subjunctive there.  I'd just use the same phrasing as
above, which also makes things a bit easier for translators:

  By default,
  .B %pager%
  is used, falling back to
  .B %cat%
  if
  .B %pager%
  is not found or is not executable.

> +static void append_char(char *a, int *i, char c)
> +{
> +     a[*i] = c;
> +     (*i)++;
> +}
[...]
> +                             append_char (r, &o, cmd[i]);

This is unnecessary indirection.  Could you please just inline this as
something of the form:

  r[o++] = cmd[i];

Using the post-increment operator is a pretty typical C idiom, so I
think the function wrapper actually decreases clarity here.

> +// Returns the first token of a libpipeline/sh-style command. See SUSv4TC2:

Please use /* ... */ style comments throughout, as found elsewhere in
the codebase; man-db doesn't yet assume C99 (at least not
intentionally).

> +static char *sh_lang_first_word(const char *cmd)

Space before parenthesis, please.  (I'd normally just adjust this before
committing, but since I'm requesting more substantive changes anyway, I
might as well mention it ...)

> +     int i = 0, o = 0;
> +     char *r = xmalloc (strlen (cmd) + 1);

I'd use "ret"; of course you don't need to go overboard with variable
name length, but there are slightly too many single-character variable
names here for my taste.  The indexes are OK though.

> +     while (i < strlen (cmd)) {

strlen has to iterate over the whole string, and you're calling it a lot
so this function is unnecessarily quadratic-time (OK, it's only called
once, but still).  Cheaper:

  int i;

  for (i = 0; cmd[i]; ++i) {
          ...
  }

... and drop the "i++;" at the end of the loop.

(Similar for all the other tests against the result of strlen in this
function; in general testing cmd[i] is a cheap test for
not-at-end-of-string.)

> +             if (cmd[i] == '\\') {

I think this whole thing would be a bit clearer as a switch statement.

> +                     // Escape Character (Backslash)
> +                     i++;
> +                     if (i == strlen (cmd)) {
> +                             break;
> +                     }
> +                     if (cmd[i] != '\n') {
> +                             append_char (r, &o, cmd[i]);
> +                     }

The other branches of the outermost conditional here always leave i at
the last character processed, and you can simplify this if you adopt
that policy here too.  Then you don't need the "break", and you can
write this as:

  if (cmd[i + 1] && cmd[i + 1] != '\n')
          ret[o++] = cmd[++i];

(You may need to adjust the other branches of the conditional to match.)

Note that in general I prefer omitting the braces in single-statement if
blocks.  I'm aware that this makes certain kinds of mistakes a bit
easier, but modern versions of GCC are pretty good at spotting those,
and I think it's justified by making the code more vertically-compact
and so being able to see more useful meaning at once.

> +             } else if (cmd[i] == '"') {
> +                     // Double-Quotes
> +                     i++;
> +                     while (i < strlen (cmd)) {

This would match the structure of the single-quote case better, and
would save you a test-and-break below:

  while (cmd[i] && cmd[i] != '"')

> +                             if (cmd[i] == '\\') {
> +                                     if (cmd[i+1] == '"' ||
> +                                         cmd[i+1] == '\\') {

Spaces around operators, please.

I'd recommend interpreting \$ and \` as well, even though you
(reasonably) aren't interpreting $ and ` on their own.

It's probably worth turning this into a switch statement too.

> +                                             i++;
> +                                             append_char (r, &o, cmd[i]);
> +                                     } else if (cmd[i+1] == '\n') {

Why test for '\n' here?  It's not justified by shell quoting rules.

> +                                             i++;
> +                                     } else {
> +                                             append_char (r, &o, cmd[i]);
> +                                     }
> +                             } else if (cmd[i] == '"') {
> +                                     break;
> +                             } else {
> +                                     append_char (r, &o, cmd[i]);
> +                             }
> +
> +                             i++;

If you put all of the above comments together, I think you can refactor
this block to follow this pseudocode structure, which would be much more
readable:

  if backslash:
    if followed by one of $`"\:
      advance over backslash
  copy character to output, advancing over it

> +     if (pager == NULL) {
> +             char *t = sh_lang_first_word (PAGER);

"pager_program" or something rather than just "t".

Thanks,

-- 
Colin Watson                                       address@hidden