On Tue, Nov 22, 2016 at 2:41 AM, Duy Nguyen <pclouds@xxxxxxxxx> wrote:
> On Fri, Nov 11, 2016 at 3:34 AM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
>> @@ -139,7 +140,8 @@ static size_t common_prefix_len(const struct pathspec *pathspec)
>> PATHSPEC_LITERAL |
>> PATHSPEC_GLOB |
>> PATHSPEC_ICASE |
>> - PATHSPEC_EXCLUDE);
>> + PATHSPEC_EXCLUDE |
>> + PATHSPEC_ATTR);
>
> Hmm.. common_prefix_len() has always been a bit relaxing and can cover
> more than needed. It's for early pruning. Exact pathspec matching
> _will_ be done later anyway.
>
> Is that obvious?
Yes it is.
Not sure what your concern is, though.
Given the pathspec ":(attr:plumbing)Documentation/", the common_prefix_len
is still able to figure out that any match has a prefix of
strlen("Documentation/"),
no matter what attr stuff is involved, because the attr stuff is also
just reducing the
matching set.
Now if we have such a pathspec it is easier to claim common_prefix_len supports
attr as it is a correct thing to ignore the attrs completely, than to
rewrite the call to
common_prefix_len to be without attrs involved.
You *may* be able to improve it with knowledge of the attrs:
":(attr:internal-technical-api)Documentation/" may restrict the results to match
only "Documentation/technical/", but as you said, we don't have to be
exact here.
> I'm wondering if we need to add a line or two in the
> big comment code before this statement. I'm thinking it is and we
> probably don't need more comments...
Do I misunderstand the code completely here?
Thanks,
Stefan
> --
> Duy