From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Tue, 14 Dec 2010 14:44:48 GMT
Subject: Re: [PATCH 11/19] tree_entry_interesting: support depth limit
Message-ID: <AANLkTikkU1h3vYDGpf4h8-9-sSWeYqtnZh1ZfB3NU3Db@mail.gmail.com>
In-Reply-To: <7vwrnd1r2s.fsf@alter.siamese.dyndns.org>

2010/12/14 Junio C Hamano <gitster@pobox.com>:
>> +int within_depth(const char *name, int namelen,
>> +                     int depth, int max_depth)
>> +{
>> +     const char *cp = name, *cpe = name + namelen;
>> +
>> +     while (cp < cpe) {
>> +             if (*cp++ != '/')
>> +                     continue;
>> +             depth++;
>> +             if (depth > max_depth)
>> +                     return 0;
>> +     }
>> +     return 1;
>> +}
>
> Makes me almost suspect that it may make more sense to keep track of the
> "depth" in a similar way as "base" and "baselen" as "traversal state" on
> the side of the caller so that you do not have to scan the string for
> slashes over and over again.  But given the codeflow, doing so might make
> the result look too ugly, so I won't recommend that without thinking,
> though.

Moreover, this function is also used by match_pathspec_depth().

>> -     if (!ps || !ps->nr)
>> +     if (!ps)
>>               return 1;
>>
>> +     if (!ps->nr) {
>> +             if (!ps->recursive || ps->max_depth == -1)
>> +                     return 1;
>> +             return !!within_depth(base, baselen,
>> +                                   !!S_ISDIR(entry->mode),
>> +                                   ps->max_depth);
>> +     }
>
> This gives different behaviour to between callers that give you NULL as
> pathspec and callers that give you a pathspec with zero elements.  Is this
> intended?  What is the use case (iow, what does an end user give from the
> command line to experience this difference)?

Old pathspec type's legacy. When pathspec is of "const char **", it
can be NULL. When struct pathspec is used, I don't think we need to
support NULL pathspec. Will remove that "if (!ps)" clause.

>> @@ -571,7 +580,13 @@ int tree_entry_interesting(const struct name_entry *entry,
>>                       if (!match_dir_prefix(base, baselen, match, matchlen))
>>                               /* Just a random prefix match */
>>                               continue;
>> -                     return 2;
>> +
>> +                     if (!ps->recursive || ps->max_depth == -1)
>> +                             return 2;
>> +
>> +                     return !!within_depth(base+matchlen+1, baselen-matchlen-1,
>> +                                           !!S_ISDIR(entry->mode),
>> +                                           ps->max_depth);
>>               }
>
> If two pathspecs that overlap with each other (e.g. "Documentation/" and
> "Documentation/technical") are given, and if the shorter one comes before
> the longer one in ps[], wouldn't this give you an unexpected result?  When
> inspecting "Documentation/technical/api/foo.txt" with depth limit of 2, if
> you didn't have "Documentation/" pathspec, you count "api/foo.txt"
> relative to "Documentation/technical", declare that the path is within
> limit, and show it.  But if you have "Documentation/" in ps[], you look at
> it, decide "technical/api/foo.txt" is too deep and return false without
> even looking at "Documentation/technical" that may appear later in ps[],
> no?

Right. Hmm.. grep's pathspec_matches() probably has this problem too.
-- 
Duy