Re: [PATCH v2] clean: use warning_errno() when appropriate

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclo...@gmail.com> writes:

> All these warning() calls are preceded by a system call. Report the
> actual error to help the user understand why we fail to remove
> something.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
> ---
>  v2 dances with errno

Thanks.

>
>  builtin/clean.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index d6bc3aaae..3569736f6 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char 
> *prefix, int force_flag,
>       struct strbuf quoted = STRBUF_INIT;
>       struct dirent *e;
>       int res = 0, ret = 0, gone = 1, original_len = path->len, len;
> +     int saved_errno;
>       struct string_list dels = STRING_LIST_INIT_DUP;
>  
>       *dir_gone = 1;
> @@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char 
> *prefix, int force_flag,
>       if (!dir) {
>               /* an empty dir could be removed even if it is unreadble */
>               res = dry_run ? 0 : rmdir(path->buf);
> +             saved_errno = errno;
>               if (res) {
>                       quote_path_relative(path->buf, prefix, &quoted);

I think this part should be more like

                res = ... : rmdir(...);
                if (res) {
                        int saved_errno = errno;
                        ... do other things that can touch errno ...
                        errno = saved_errno;
                        ... now we know what the original error was ...

The reason to store the errno in saved_errno here is not because we
want to help code after "if (res) {...}", but the patch sent as-is
gives that impression and is confusing to the readers.  

Perhaps all hunks of this patch share the same issue?  I could
locally amend, of course, but I'd like to double check before doing
so myself---perhaps you did it this way for a good reason that I am
missing?