Re: [Man-db-devel] [PATCH v5] man(1): Fix override dir handling

[Colin Watson]

On Sat, Jan 07, 2017 at 02:51:27AM +0500, Mihail Konev wrote:
> Previously, override dir was affecting only some cases
> of manpath determination.
> 
> Apply it only when all paths has been gathered instead.
> (Depending on the definition of when the override dir applies,
> this might not be correct).
> 
> Also look for override dir when sorting candidates.
> 
> Fixes src/tests/man-9 failing when --with-override-dir=od
> is passed to ./configure.

Thanks for this, and I'm sorry for taking so long to review it!

Some changes were necessary due to changes since 2017 in how the manpath
list is assembled, but those were fairly obvious.  I did have some other
review comments, but I generally think that if I've taken five years to
review something then it isn't fair to bounce it back to the patch
submitter to make them do it; so I took the liberty of making these
changes myself while applying the patch and just added a note to the
commit message to make it clear what I did.  I also added a NEWS.md
entry.  Details below, and you can see the patch I ended up committing
here:

  https://gitlab.com/cjwatson/man-db/-/commit/a85d7d3284

Let me know if I seem to have made any mistakes here, of course.

> +static int cand1_differs_by_override_dir (const struct candidate *left,
> +                                       const struct candidate *right)

This would be better structured as a strcmp-style function, i.e. one
that returns negative, positive, or zero depending on how the two
candidates compare under the criteria in question; doing so means that
the caller only has to call it once, and I think it also makes the code
structure clearer since it tends to come out looking more symmetrical.

> @@ -1201,18 +1179,30 @@ void create_pathlist (const char *manp, char **mp)
>       const char *p, *end;
>       char **mphead = mp;
>  
> -     /* Expand the manpath into a list for easier handling. */
> +     /* Expand the manpath into a list for easier handling.
> +      * For each entry, add corresponding OVERRIDE_DIR.
> +      * */
>  
>       for (p = manp;; p = end + 1) {
> +             char *element, *element_override;
> +             ssize_t p_len;
> +
>               end = strchr (p, ':');
> -             if (end) {
> -                     char *element = xstrndup (p, end - p);
> -                     mp = add_dir_to_path_list (mphead, mp, element);
> -                     free (element);
> -             } else {
> -                     mp = add_dir_to_path_list (mphead, mp, p);
> -                     break;
> +             p_len = end ? end - p : (ssize_t)strlen(p);
> +
> +             element = xstrndup (p, p_len);

Rather than having to deal with the type awkwardness of strlen returning
size_t (unsigned) while end - p is implicitly signed (although by
construction can never be negative), it's simpler to just write:

  element = end ? xstrndup (p, end - p) : xstrdup (p);

-- 
Colin Watson (he/him)                              [cjwatson@debian.org]