Re: [PATCH] ls-files: add pathspec matching for submodules

[Brandon Williams <bmwill@google.com>]

On Mon, Sep 19, 2016 at 11:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>>> Again, what do we have in "name" and "item" at this point?  If we
>>> have a submodule at "sub/" and we are checking a pathspec element
>>> "sub/dir1/*", what is the non-wildcard part of the pathspec and what
>>> is the "string"?  Aren't they "sub/dir1/" and "sub/" respectively,
>>> which would not pass ps_strncmp() and produce a (false) negative?
>>
>> item will be the pathspec_item struct that we are trying to match against.
>
> ... which would mean "sub/dir1/" in the above example (which is
> followed by '*' that is wildcard).
>
>> name will be the file we are trying to match, which should already have the
>> 'prefix' cut off (this is the prefix that is used as an optimization
>> in the common
>> case, which isn't used in the submodule case).
>
> ... which would be "sub/" in the above example, because we disable
> the common-prefix optimization.
>
> So in short, the answer to the last questions in the first quoted
> paragraph are yes, yes, and "no they do not pass ps_strncmp()"?

Yes in that case it wouldn't have passed ps_strncmp()...but we should have never
made it there in the first place due to a piece of logic in match_pathspec_item:

@@ -283,6 +308,24 @@ static int match_pathspec_item(const struct
pathspec_item *item, int prefix,
                         item->nowildcard_len - prefix))
                return MATCHED_FNMATCH;

+       /* Perform checks to see if "name" is a super set of the pathspec */
+       if (flags & DO_MATCH_SUBMODULE) {
+               int matched = 0;
+
+               /* Check if the name is a literal prefix of the pathspec */
+               if ((item->match[namelen] == '/') &&
+                   !ps_strncmp(item, match, name, namelen)) {
+                       matched = MATCHED_RECURSIVELY;
+               /* Check if the name wildmatches to the pathspec */
+               } else if (item->nowildcard_len < item->len &&
+                          !prefix_fnmatch(item, match, name,
+                                          item->nowildcard_len - prefix)) {
+                       matched = MATCHED_FNMATCH;
+               }
+
+               return matched;
+       }

Perhaps the call structure and code organization could be changes a bit to make
a little more sense.