Re: [PATCH 2/2] ls-files: add pathspec matching for submodules
On Wed, Sep 21, 2016 at 3:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
>
> 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.
> 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?
Show 48 quoted lines
>
> 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.