On Wed, May 17, 2017 at 10:34 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Samuel Lijin <sxlijin@xxxxxxxxx> writes:
>
>> @@ -932,7 +935,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>>
>> fill_directory(&dir, &pathspec);
>>
>> - for (i = 0; i < dir.nr; i++) {
>> + for (k = i = 0; i < dir.nr; i++) {
>> struct dir_entry *ent = dir.entries[i];
>> int matches = 0;
>> struct stat st;
>> @@ -954,10 +957,35 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>> matches != MATCHED_EXACTLY)
>> continue;
>>
>> + // skip any dir.entries which contains a dir.ignored
>> + for (; k < dir.ignored_nr; k++) {
>> + if (cmp_dir_entry(&dir.entries[i],
>> + &dir.ignored[k]) < 0)
>> + break;
>
> It is a bit unfortunate that we do not use the short-hand "ent" we
> established for dir.entries[i] at the beginning of this loop here.
>
>> + }
>> + if ((k < dir.ignored_nr) &&
>> + check_dir_entry_contains(dir.entries[i], dir.ignored[k])) {
>> + continue;
>> + }
>
> The above logic is not needed when dir.entries[i] is a directory,
> right?
Au contraire - this logic only matters when dir.entries[i] is a directory.
>> +
>> + // current entry does not contain any ignored files
>
> ... or the entry may not have been a directory in the first place.
If it's not a directory, it can't contain ignored files ;)
>> rel = relative_path(ent->name, prefix, &buf);
>> string_list_append(&del_list, rel);
>> +
>> + // skip untracked contents of an untracked dir
>> + for (j = i + 1;
>> + j < dir.nr &&
>> + check_dir_entry_contains(dir.entries[i], dir.entries[j]);
>> + j++);
>> + i = j - 1;
>> }
>>
>> + for (i = 0; i < dir.nr; i++)
>> + free(dir.entries[i]);
>> +
>> + for (i = 0; i < dir.ignored_nr; i++)
>> + free(dir.ignored[i]);
>> +
>
> The above logic may not be incorrect per-se, but I have this
> suspicion that the resulting code may become cleaner and easier to
> understand if we did it in a new separate loop we call immediately
> after fill_directory() returns. Or it could even be a call to a
> helper that "post processes" the "dir" thing that was polupated by
> fill_directory() that removes elements in the entries[] array that
> contains any element in ignored[] array.
Will try it out.
>> if (interactive && del_list.nr > 0)
>> interactive_main_loop();
>
> Thanks.