Re: [RFC PATCH 2/7] dir.c: fix off-by-one error in match_pathspec_item
- Date: Fri, 6 Apr 2018 13:53:49 -0400
- From: Jeff King <peff@xxxxxxxx>
- Subject: Re: [RFC PATCH 2/7] dir.c: fix off-by-one error in match_pathspec_item
On Thu, Apr 05, 2018 at 01:06:30PM -0700, Elijah Newren wrote:
> > 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).
> If it'd only take you a few hours, then you're a lot faster than me.
> It took me a while to start wrapping my head around it.
OK, I was being overly optimistic. :)
> The other trailing-slash matches in the function are all correct,
> according to the testsuite. (I'm not sure I like the
> DO_MATCH_DIRECTORY stuff, but it is encoded in tests and backward
> compatibility is important.) In particular, changing the earlier code
> to have the same offset trick would make it claim that e.g. either
> "a/b" or "a/b/" as names match unconditionally against "a/b/c" as a
> pathspec. We need it to be conditional: we only want that to be
> considered a match when checking whether we want to recurse into the
> directory for other matches, not when checking whether the directory
> itself matches the pathspec. Thus, it should be behind a separate
> flag, in a subsequent check, which is what this series does (namely
> with DO_MATCH_LEADING_PATHSPEC).
OK, that makes some sense to me.
> To be more precise, here is how a matrix of pathnames and pathspecs
> would be treated by match_pathspec_item(), where I am abbreviating
> names like MATCH_RECURSIVELY_LEADING_PATHSPEC to LEADING):
> | a/b | a/b/ | a/b/c
> a/b | EXACT | RECURSIVE | LEADING
> Names a/b/ | EXACT | EXACT | LEADING
> a/b/c | RECURSIVE | RECURSIVE | EXACT
>  Only if DO_MATCH_DIRECTORY is passed. Otherwise,
> this is NOT a match at all.
>  Only if DO_MATCH_LEADING_PATHSPEC is passed,
> after applying this series. Otherwise, not a match
> at all.
>  Without the fix in this thread that you highlighted,
> and assuming we apply patch 7, this would actually
> mistakenly return RECURSIVE.
> Now for a separate question: How much of the above would you like
> added to the commit message...or even as a comment in the code to make
> it clearer to other folks trying to make sense of it?
That table seems quite illuminating to me. It's hard to pick out all the
special-cases from the code, or what they're _supposed_ to be doing. I
think it makes sense as a code comment.
PS I'm going to be on a 3-week vacation starting tomorrow, so apologies
in advance for ignoring any follow-ups.