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

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2013, 15:03 UTC
Message-ID: <7vbo8ce14u.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CANYiYbF3AC=iHaOkj96yhBSj8caC2SdZDygrxV43+mzAd-wGJA@mail.gmail.com>

Jiang Xin <worldhello.net@gmail.com> writes:

Show 22 quoted lines

> 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()"?