Re: [PATCH 12/41] builtin/update-index.c: use error_errno()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 1, 2016, 18:40 UTC
Message-ID: <CAPig+cQmjXcsDsCO0A7dgeDg2ho_eFFf9wCouz528kBk_W=b9w@mail.gmail.com>
In-Reply-To: <1462101297-8610-13-git-send-email-pclouds@gmail.com>

On Sun, May 1, 2016 at 7:14 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

> "err" is deleted because it just causes confusion when "errno" is also
> used directly in process_lstat_error().

Despite the function name which may imply that it is consulting errno, this change makes me feel slightly uncomfortable since it increases coupling. Whereas consulting of errno had been explicit, with this change, it becomes implicit, and someone changing the code needs to be extra careful about ensuring that errno does not get clobbered.

Not a big objection, but a nagging doubt...

Show 24 quoted lines

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> @@ -251,11 +251,11 @@ static int remove_one_path(const char *path)
> -static int process_lstat_error(const char *path, int err)
> +static int process_lstat_error(const char *path)
>  {
> -       if (err == ENOENT || err == ENOTDIR)
> +       if (errno == ENOENT || errno == ENOTDIR)
>                 return remove_one_path(path);
> -       return error("lstat(\"%s\"): %s", path, strerror(errno));
> +       return error_errno("lstat(\"%s\")", path);
>  }
>
>  static int add_one_path(const struct cache_entry *old, const char *path, int len, struct stat *st)
> @@ -382,7 +382,7 @@ static int process_path(const char *path)
>          * what to do about the pathname!
>          */
>         if (lstat(path, &st) < 0)
> -               return process_lstat_error(path, errno);
> +               return process_lstat_error(path);
>
>         if (S_ISDIR(st.st_mode))
>                 return process_directory(path, len, &st);