On Thu, Apr 05, 2018 at 11:36:45AM -0700, Elijah Newren wrote:
> > Do we care about matching the name "foo" against the patchspec_item "foo/"?
> >
> > That matches now, but wouldn't after your patch.
>
> Technically, the tests pass anyway due to the fallback behavior
> mentioned in the commit message, but this is a really good point. It
> looks like the call to submodule_path_match() from builtin/grep.c is
> going to be passing name without the trailing '/', which is contrary
> to how read_directory_recursive() in dir.c builds up paths (namely
> with the trailing '/'). If we tried to force consistency (either
> always omit the trailing slash or always include it), then we'd
> probably want to do so for match_pathspec() calls as well, and there
> are lots of those throughout the code and auditing it all looks
> painful.
>
> So I should probably make the check handle both cases:
>
> @@ -383,8 +383,9 @@ static int match_pathspec_item(const struct
> pathspec_item *item, int prefix,
> /* Perform checks to see if "name" is a super set of the pathspec */
> if (flags & DO_MATCH_LEADING_PATHSPEC) {
> /* name is a literal prefix of the pathspec */
> + int offset = name[namelen-1] == '/' ? 1 : 0;
> if ((namelen < matchlen) &&
> - (match[namelen] == '/') &&
> + (match[namelen-offset] == '/') &&
> !ps_strncmp(item, match, name, namelen))
> return MATCHED_RECURSIVELY_LEADING_PATHSPEC;
That seems reasonable to me, and your "offset" trick here should prevent
us from getting confused. Can namelen ever be zero here? I guess
probably not (I could see an empty pathspec, but an empty path does not
make sense).
There are other similar trailing-slash matches in that function, but I'm
not sure of all the cases in which they're used. I don't know if any of
those would need similar treatment (sorry for being vague; I expect I'd
need a few hours to dig into how the pathspec code actually works, and I
don't have that today).
-Peff
