fix: match variable-depth ** alongside {placeholder} and excludeFiles globs - #58
fix: match variable-depth ** alongside {placeholder} and excludeFiles globs#58mliem2k wants to merge 2 commits into
Conversation
fe0e55a to
294e853
Compare
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@mliem2k Thank you for the PR! Overall the fix looks solid, but it introduces one regression elsewhere and therefore does not yet fully address the problem. See comment below and the failure for the e2e test I added.
| if (segment === "**") { | ||
| for ( | ||
| let consumedUpTo = pathIndex; | ||
| consumedUpTo <= pathSegments.length; | ||
| consumedUpTo++ | ||
| ) { | ||
| const rest = matchPatternSegments({ | ||
| patternSegments, | ||
| pathSegments, | ||
| patternIndex: patternIndex + 1, | ||
| pathIndex: consumedUpTo, | ||
| }); | ||
| if (rest !== null) { | ||
| return rest; | ||
| } |
There was a problem hiding this comment.
** stops backtracking before placeholder semantics are validated: The matcher returns the first structurally matching ** split. Placeholder consistency is merged later, and constraints are checked only after matching completes. If that first split fails either check, later valid splits are never tried.
In 7ae5e18, I've added an e2e test that confirms this regression. This needs to be addressed before we can merge this PR.
…excludeFiles globs
** previously matched greedily with no backtracking, so it couldn't
coexist with {placeholder} segments elsewhere in the same pattern (vercel-labs#56),
and excludeFiles globs containing ** never matched anything past the
first consumed segment (vercel-labs#57).
matchPatternSegments now backtracks through candidate ** consumption
counts, and validates placeholder consistency and constraints as soon
as a segment is bound rather than after the full match completes, so a
conflict on one split causes the nearest enclosing ** to try its next
candidate instead of failing the whole match outright.
7ae5e18 to
2aa6e0c
Compare
|
Thanks for catching this, and for the regression test, that pinned down the exact failure mode. Root cause: matchPatternSegments returned the first structurally matching ** split, but placeholder consistency and constraints were only checked after the full match completed. If that first split conflicted with an earlier placeholder value or failed a constraint, later valid splits were never tried. Fix: the accumulated placeholder values are now threaded through the recursion and validated as soon as a segment binds, so a conflict causes the nearest enclosing ** to backtrack to its next candidate instead of failing the whole match. Your e2e test (globstar-placeholder-backtracking) now passes, and I added a matching unit test in path-matcher.test.ts. Full suite (unit + e2e + typecheck) is green. |
Pull Request Description
Fixes #56
Fixes #57
Both issues share the same root cause, per the reproduction steps in each:
tryExtractPlaceholders()insrc/core/path-matcher.tsrequired the pattern and the matched path to have the exact same number of/-separated segments. Since**can legitimately match a variable number of segments, this equal-length check silently rejected any candidate where**didn't happen to consume exactly one segment.**next to a{placeholder}silently matches only one directory level (not all depths) #56:deep/**/{name}.tsagainstdeep/top.ts(depth 0),deep/a/mid.ts(depth 1),deep/a/b/leaf.ts(depth 2) only matched the depth-1 file. The initial glob (viatinyglobby) correctly found all three candidates, but the subsequent placeholder-extraction step dropped the depth-0 and depth-2 ones because their segment counts didn't equal the pattern's.excludeFilespatterns prefixed with**/are silently ignored (docs show**/*.test.tsas valid) #57:excludeFiles: ["**/__test-env-tdd-state.ts"]never excluded anything, because file exclusion falls through to the same kind of segment-count-sensitive matching once glob syntax is involved (bare filenames and full relative paths worked because they only ever went through basename/exact-string comparison, which doesn't care about segment counts).Relevant technical choices
Rather than special-casing or warning about
**next to a placeholder, this implements the "real fix" the reporter suggested: path segment matching (matchPatternSegmentsinpath-matcher.ts) now backtracks through candidate consumption counts when it hits a**pattern segment, trying each possible number of consumed path segments until the rest of the pattern (including any placeholders after the**) also matches. This is a standard backtracking glob-with-captures matcher; it replaces the old index-by-index walk intryExtractPlaceholders.excludeFilesmatching inrunner.ts(isFileExcluded) previously only supported exact-path and basename comparisons — it never actually ran patterns through glob matching, which is why**/-prefixed patterns (and any other wildcard syntax) never worked despitedocs/reference/configuration.mddocumenting"**/*.test.ts"as valid. I added a new exportedmatchesPathPattern()inpath-matcher.ts(reusing the same backtracking segment matcher) and call it fromisFileExcludedas an additional check, soexcludeFilesnow supports the same*,?, and**glob syntax aspaths, while keeping the existing exact-path/basename behavior for plain filenames unchanged.Testing
path-matcher.test.tsreproducing the exact**next to a{placeholder}silently matches only one directory level (not all depths) #56 fixture tree/pattern (all three depths now match with correct placeholder values), a baseline check that plain**without a placeholder still matches every depth, a multi-placeholder-consistency check with**present, and direct tests ofmatchesPathPatterncovering**/-prefixed, trailing-**, and literal patterns.runner.test.tsreproducingexcludeFilespatterns prefixed with**/are silently ignored (docs show**/*.test.tsas valid) #57's exclusion scenario end-to-end throughrun().exclude-filese2e fixture to exclude via a**/-prefixed pattern instead of a full relative path, exercising the realtinyglobby-backed CLI path; confirmed (by temporarily reverting the source fix) that this fixture fails without the fix and passes with it.pnpm test,pnpm typecheck,pnpm check,pnpm test:e2e) passes.Checklist
pnpm changesetin the project root if package files were modified)