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

Brandon Williams [off-list ref] writes:

quoted

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()"?

quoted

I am starting to have a feeling that the best we can do in this
function safely is to see if prefix (i.e. the constant part of the
pathspec before the first wildcard) is long enough to cover the
"name" and if "name" part [matches or does not match] ...
If these two checks cannot decide, we may have to be pessimistic and
say "it may match; we don't know until we descend into it".
...
So I would think we'd be in the business of counting slashes in the
name (called "string" in this function) and the pathspec, while
noticing '*' and '**' in the latter, and we may be able to be more
precise, but I am not sure how complex the end result would become.

I agree, I'm not too sure how much more complex the logic would need
to be to handle
all matters of wildcard characters.  We could initially be more
lenient on what qualifies as
a match and then later (or in the near future) revisit the wildmatch
function (which is complex)
and see if we can add better matching capabilities more suited for
submodules while at the
same time fixing that bug discussed above.

I think it is reasonable to start a function that is meant to never
have false negatives pessimistic and return "might match" from it
when in doubt.

Thanks.

On Mon, Sep 19, 2016 at 11:04 AM, Junio C Hamano [off-list ref] wrote:

Brandon Williams [off-list ref] writes:

quoted

quoted

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).

quoted

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.