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

From: Jiang Xin <worldhello.net@gmail.com>
Date: May 15, 2013, 00:40 UTC
Message-ID: <CANYiYbF3AC=iHaOkj96yhBSj8caC2SdZDygrxV43+mzAd-wGJA@mail.gmail.com>
In-Reply-To: <7vvc6ldtx7.fsf@alter.siamese.dyndns.org>

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

Show 17 quoted lines

>> @@ -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.

                if (S_ISDIR(st.st_mode)) {
                        if (remove_directories || (matches ==
MATCHED_EXACTLY)) {
                                rel = path_relative(ent->name, prefix);
                                string_list_append(&del_list, rel);
                        }
                } ...

When start to do cleaning, there is a duplicate lstat() call, and the removed comments above are moved here.

                /*
                 * we might have removed this as part of earlier
                 * recursive directory removal, so lstat() here could
                 * fail with ENOENT.
                 */
                if (lstat(abs_path.buf, &st))
                        continue;

                if (S_ISDIR(st.st_mode)) {
                        if (remove_dirs(&abs_path, prefix, rm_flags,
dry_run, quiet, &gone))
                                errors++;
                        if (gone && !quiet) {
                                qname =
quote_path_relative(item->string, -1, &buf, NULL);
                                printf(dry_run ? _(msg_would_remove) :
_(msg_remove), qname);
                        }
                }

But I am not clear how "earlier recursive directory removal" could make lstat() failure here. If it is not the case, maybe we can test a directory by ent->name (ends with '/' or '\\' ?).

-- 
Jiang Xin