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

[Brandon Williams <bmwill@google.com>]

On Wed, Sep 21, 2016 at 3:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Sounds sensible.  Just a minor nit in terminology, but I think we
> fairly consistently say "a superproject contains submodules" (run
> "git grep -E 'super *(module|project)'").
>
> I'd suggest s/super module/superproject/ for consistency.

Will do.

> An example of this test would be to match pathspec "sub/file" with
> submodule path "sub"?

Yep,  I believe there's a test for that case

> item->match[namelen] is accessed without checking if item->match[]
> is long enough here; shouldn't item->len be checked before doing
> that?

Oh right!  Good catch.

>
> Hmph, isn't this the one that is allowed produce false positive but
> cannot afford to give any false negative?  It feels a bit strange
> that the code checks two cases where we can positively say that it
> is worth descending into, and falling through would give "no this
> will never match".  That sounds like invitation for false negatives.
>
> IOW, I would have expected
>
>         if (flags & DO_MATCH_SUBMODULE) {
>                 if (may match in this case)
>                         return MATCHED_RECURSIVE;
>                 if (may match in this other case)
>                         return MATCHED_RECURSIVE;
>                 ...
>                 if (obviously cannot match in this case)
>                         return 0;
>                 if (obviously cannot match in this other case)
>                         return 0;
>                 /* otherwise we cannot say */
>                 return MATCHED_RECURSIVELY;
>         }
>
> as the general code structure.
>
> Fully spelled out,
>
>         if (flags & DO_MATCH_SUBMODULE) {
>                 /* Check if the name is a literal prefix of the pathspec */
>                 if (namelen < item->len &&
>                     item->match[namelen] == '/' &&
>                     !ps_strncmp(item, match, name, namelen))
>                         return MATCHED_RECURSIVE;
>
>                 /* Does the literal leading part have chance of matching? */
>                 if (item->nowildcard_len < item->len &&
>                     namelen <= item->nowildcard_len &&
>                     ps_strncmp(item, match, name, namelen))
>                         return 0; /* no way "su?/file" can match "sib" */
>
>                 /* Otherwise we cannot say */
>                 return MATCHED_RECURSIVELY;
>         }
>
> or something like that.  There may be other "obviously cannot match"
> cases we may want to add further.
>
> Thanks.

You're right it should be structured the other way.