Re: [PATCH 05/10] copy.c: convert copy_file() to copy_dir_recursively()

[Eric Sunshine <sunshine@sunshineco.com>]

Sorry for the (more than a year) late review. I realize that a
subsequent version of this patch series renders a few of the review
comments meaningless, but I'm including them in case the code is
revived later.

On Sat, Jun 25, 2016 at 3:54 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> This finally enables busybox's copy_file() code under a new name
> (because "copy_file" is already taken in Git code base). Because this
> comes from busybox, POSIXy (or even Linuxy) behavior is expected. More
> changes may be needed for Windows support.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/copy.c b/copy.c
> +/*
> + * Find out if the last character of a string matches the one given.
> + * Don't underrun the buffer if the string length is 0.
>   */
> +static inline char *last_char_is(const char *s, int c)

Is this function ever used in anything other than a boolean capacity?
If not, then perhaps a simpler signature would work?

    static inline int ends_with(const char *s, char c)

(and you could drop the ugly casting in the function body)

> +{
> +       if (s && *s) {
> +               size_t sz = strlen(s) - 1;
> +               s += sz;
> +               if ( (unsigned char)*s == c)

Style: if ((...

> +                       return (char*)s;
> +       }
> +       return NULL;
> +}
> +
> +static int do_unlink(const char *dest)
> +{
> +       int e = errno;
> +
> +       if (unlink(dest) < 0) {
> +               errno = e; /* do not use errno from unlink */

This comment is repeating the code itself but does not explain _why_.
More helpful might be a function-level comment explaining that
do_unlink() does not clobber errno.

> +               return error_errno(_("can't create '%s'"), dest);

However, it's not really clear what the intention is here. This is
emitting an error message only if unlink() failed, but the actual
error message is from some entirely unrelated operation. Confusing.

Also, is the idea that 'errno' should be restored no matter what happens?

> +       }
> +       return 0;
> +}