Re: [PATCH v9 1/9] git-clean: refactor git-clean into two phases

From: Jiang Xin <worldhello.net@gmail.com>
Date: May 15, 2013, 15:07 UTC
Message-ID: <CANYiYbF+JLqyB19gHxcC+-ysM=hbMZMve=4JNoiu15B=jaUS9g@mail.gmail.com>
In-Reply-To: <7vbo8ce14u.fsf@alter.siamese.dyndns.org>

2013/5/15 Junio C Hamano <gitster@pobox.com>:

Show 33 quoted lines

> Jiang Xin <worldhello.net@gmail.com> writes:
>
>> 2013/5/15 Junio C Hamano <gitster@pobox.com>:
>>>> @@ -242,11 +287,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>>>>                               continue; /* Yup, this one exists unmerged */
>>>>               }
>>>>
>>>> -             /*
>>>> -              * we might have removed this as part of earlier
>>>> -              * recursive directory removal, so lstat() here could
>>>> -              * fail with ENOENT.
>>>> -              */
>>>>               if (lstat(ent->name, &st))
>>>>                       continue;
>>>
>>> I am guessing that the reason why you removed the comment is because
>>> during this phase there is no way we "might have removed".  But if
>>> that is the case, does it still make sense to run lstat() and ignore
>>> errors from the call?
>>>
>>
>> Run lstat() here is necessary, because we need to check whether
>> ent->name points to a file or a directory. If ent points to a directory,
>> only add to del_list when user provides '-x' option to git-clean.
>
> Sorry, but that was not the question; we can see st is used
> immediately below so somebody needs to fill it.
>
> I was pointing out that the "lstat() is expected to fail with ENOENT
> but it is not an error worth reporting" justification the original
> code had to silently ignore an error, because you no longer remove
> anything immediately in this part of the code.  Is "if () continue"
> still valid thing to do here, not "if () die()"?

I'm clear, it could be:

                if (lstat(ent->name, &st))
                        die_errno("Cannot lstat '%s'", ent->name);

-- 
Jiang Xin