Re: [Man-db-devel] [PATCH 1/2] man(1): add NAME.[N] names

[Colin Watson]

On Mon, Oct 03, 2016 at 08:47:26PM +0000, Mihail Konev wrote:
> `man chmod.` is now the same as 'man -a chmod'
> `man chmod.2` is now the same as 'man 2 chmod'.
> 
> TODO: update manpages
> TODO: check for incompatible arguments
> ---
> A possible convenience enhancement.
> 
> Assumes there are no pages whose name ends with a dot.

For the most part I don't like special-casing names, and the trailing
dot seems a pretty unmemorable way to avoid using an option, so I would
rather not take that part of it.

"man chmod.2" seems like a sensible change, though.  Please could you
add a test for the new behaviour under src/tests/, and document the
change in man/man1/man.man1?

>  src/man.c | 82 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 73 insertions(+), 9 deletions(-)
> 
> diff --git a/src/man.c b/src/man.c
> index ebb97dab4bad..e7b76d66063a 100644
> --- a/src/man.c
> +++ b/src/man.c
> @@ -3810,6 +3810,47 @@ executable_out:
>  }
>  
>  /*
> + * Splits a "name[.section]" into { "name", "section" }.
> + * Section would be NULL if not present.
> + * */
> +static void split_page_name(const char *page_name,
> +             const char **ret_name,
> +             const char **ret_section)

Style for function definitions and function calls in man-db is to put a
space before the opening parenthesis, and to indent continuation lines
so that the parts inside the parentheses line up (so in this case the
"const" bits should be vertically-aligned).  Please follow this
throughout.

> +     static char *buf = NULL;

Static allocation of buffers can make sense in some cases, especially if
it's in a code path that's going to be hit frequently, but in this case
it's too fiddly and error-prone for not enough value.  I'd suggest just
making both *ret_name and *ret_section be char **, allocating them
normally, freeing them in the caller, and forgetting about this buffer.
If you use strdup/strndup as appropriate then you shouldn't need to call
xmalloc directly.

> @@ -3837,20 +3879,42 @@ static int man (const char *name, int *found)
>               return status;
>       }
>  
> -     if (section) {
> -             char **mp;
> +     split_page_name(name, &page_name, &page_section);
>  
> -             for (mp = manpathlist; *mp; mp++)
> -                     *found += locate_page (*mp, section, name, &candidates);
> -     } else {
> -             const char **sp;
> +     if (page_section) {
> +             if (page_section[0]) {
> +                     char **mp;
>  
> -             for (sp = section_list; *sp; sp++) {
> +                     for (mp = manpathlist; *mp; mp++)
> +                             *found += locate_page (*mp, page_section, 
> page_name, &candidates);
> +             } else {
> +                     const char **sp;
> +
> +                     findall = 1;
> +                     for (sp = section_list; *sp; sp++) {
> +                             char **mp;
> +
> +                             for (mp = manpathlist; *mp; mp++)
> +                                     *found += locate_page (*mp, *sp, 
> page_name, &candidates);
> +                     }
> +             }
> +     }

Please follow the prevailing code style and keep line lengths under 80
characters.

Can you do something to avoid having to write out the inner loop even
more times than it was already?  Four versions of almost exactly the
same code is probably an indication that something is wrong.  You could
perhaps have a local variable which is a one-element array containing
page_section if it's set and otherwise is section_list, or something
along those lines.

Thanks,

-- 
Colin Watson                                       address@hidden