On Wed, May 17, 2017 at 2:23 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Samuel Lijin <sxlijin@xxxxxxxxx> writes:
>
>> We consider directories containing only untracked and ignored files to
>> be themselves untracked, which in the usual case means we don't have to
>> search these directories. This is problematic when we want to collect
>> ignored files with DIR_SHOW_IGNORED_TOO, though, so we teach
>> read_directory_recursive() to recurse into untracked directories to find
>> the ignored files they contain when DIR_SHOW_IGNORED_TOO is set. This
>> has the side effect of also collecting all untracked files in untracked
>> directories as well.
>>
>> Signed-off-by: Samuel Lijin <sxlijin@xxxxxxxxx>
>> ---
>> dir.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/dir.c b/dir.c
>> index f451bfa48..6bd0350e9 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1784,7 +1784,12 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,
>> dir_state = state;
>>
>> /* recurse into subdir if instructed by treat_path */
>> - if (state == path_recurse) {
>> + if ((state == path_recurse) ||
>> + ((get_dtype(cdir.de, path.buf, path.len) == DT_DIR) &&
>
> I see a conditional in treat_path() that is prepared to deal with a
> NULL in cdir.de; do we know cdir.de is always non-NULL at this point
> in the code, or is get_dtype() prepared to see NULL as its first
> parameter?
>
> ... goes and looks ...
>
> Yes, it falls back to the usual lstat() dance in such a case, so
> we'd be OK here.
>
>> + (state == path_untracked) &&
>> + (dir->flags & DIR_SHOW_IGNORED_TOO))
>
> If we are told to SHOW_IGNORED_TOO, we'd recurse into an untracked
> thing if it is a directory. No other behaviour change.
>
> Isn't get_dtype() potentially slower than other two checks if it
> triggers? I am wondering if these three conditions in &&-chain
> should be reordered to call get_dtype() the last, i.e.
>
> if ((state == path_recurse) ||
> ((dir->flags & DIR_SHOW_IGNORED_TOO)) &&
> (state == path_untracked) &&
> (get_dtype(cdir.de, path.buf, path.len) == DT_DIR)) {
Ah, I didn't realize that. I figured that get_dtype() was just a
helper to invoke the appropriate macros, but if it's actually mildly
expensive I'll take your reorder.
>> + )
>> + {
>
> It may be just a style, but these new lines are indented overly too
> deep. We don't use a lone "{" on a line to open a block, either.
>
>> struct untracked_cache_dir *ud;
>> ud = lookup_untracked(dir->untracked, untracked,
>> path.buf + baselen,