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

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 15, 2017, 00:49 UTC
Message-ID: <CACsJy8BXSAUr2knrkOfO0gXYAwQoJpL2hCXy44Q37H4GE_-yVA@mail.gmail.com>
In-Reply-To: <xmqqh93wabev.fsf@gitster.mtv.corp.google.com>

On Wed, Feb 15, 2017 at 1:13 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 53 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@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 <pclouds@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?

One thing I like about putting saved_errno right next to the related syscall is, the syscall is visible from the diff (previously some are out of context). This is really minor though. I briefly thought of introducing rmdir_errno() and friends that return -errno on error, so we could do

res = ... : rmdir_errno(..);
if (res) {
    errno = -res;
    warning_errno(...);
}

But that's more work and the errno = -res is not particularly pleasing to read. I'm fine with moving saved_errno in the error handling blocks.

-- 
Duy