fix: replace Pattern.Includes with Contains and Overlaps - #644
Open
joe0BAB wants to merge 2 commits into
Open
Conversation
joe0BAB
force-pushed
the
fix/include-check
branch
from
September 10, 2026 08:16
0f929cc to
a51f26d
Compare
joe0BAB
marked this pull request as ready for review
September 10, 2026 12:14
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Benehiko
reviewed
Sep 10, 2026
joe0BAB
force-pushed
the
fix/include-check
branch
from
September 10, 2026 14:21
a51f26d to
79cab6a
Compare
Benehiko
reviewed
Sep 11, 2026
Benehiko
left a comment
Benehiko
left a comment
Member
There was a problem hiding this comment.
mostly just godocs
I think we need to make sure it's super clear to a user using this what it means to use p.Overlaps or p.Contains
Comment on lines
+98
to
+99
| // identically (the old Includes was spelling-sensitive and | ||
| // answered true for a/a/** but false for a/a/**/**). |
Comment on lines
+216
to
+217
| // dp[j] == "p[i:] and q[j:] match a common suffix"; row i needs only row | ||
| // i+1 (prev). |
Member
There was a problem hiding this comment.
this comment made no sense to me.
Comment on lines
+229
to
+230
| // The gap either ends, or absorbs the other side's next | ||
| // component. |
| {"bar/**", "foo/**", false}, // first components conflict | ||
| {"foo/*/baz", "foo/bar/**", true}, | ||
| // Overlap without containment in either direction. | ||
| {"docker/*/mcp/*", "docker/proj1/**", true}, // both match docker/proj1/mcp/x |
| {"foo/*/baz", "foo/bar/**", true}, | ||
| // Overlap without containment in either direction. | ||
| {"docker/*/mcp/*", "docker/proj1/**", true}, // both match docker/proj1/mcp/x | ||
| {"foo/foo/foo/**", "foo/foo/**/foo", true}, // both match foo/foo/foo |
Member
There was a problem hiding this comment.
same here - the test shows they both match
Comment on lines
+91
to
+99
| // Overlaps reports whether the pattern and [other] match at least one | ||
| // ID in common: | ||
| // | ||
| // p.Overlaps(other) ⇔ matches(p) ∩ matches(other) ≠ ∅ | ||
| // | ||
| // Symmetric; implied by containment, not the converse: docker/*/mcp/* | ||
| // and docker/proj1/** overlap yet neither contains the other. O(n*m) | ||
| // time, O(m) space, for component counts n and m. | ||
| Overlaps(other Pattern) bool |
Member
There was a problem hiding this comment.
I think Overlaps needs a bit more explaining or better godocs. Difficult to understand the concept of
docker/*/mcp/* and docker/proj1/** overlap yet neither contains the other.
| // or '**' in q. | ||
| func covers(p, q []token) bool { | ||
| np, nq := len(p), len(q) | ||
| // dp[j] == "p[i:] includes q[j:]"; row i needs only row i+1 (prev). |
Member
There was a problem hiding this comment.
don't know what this comment is about
Includes reused the ID matcher and treated a wildcard in the candidate pattern like a component that matches anything, so it implemented neither containment nor overlap: overlapping-but-not-contained patterns were reported as included (`docker/proj1/**` claimed to include `docker/*/mcp/*`), the relation was asymmetric, and spellings with equal match sets (`a/a/**` vs `a/a/**/**`) got different answers. Split it into the two relations callers actually need, each decided by a two-row DP over canonicalized wildcard tokens (runs of `*`/`**` collapse, a pure `**` becomes `*/**` since identifiers are never empty): - `p.Contains(other)`: p matches every ID that other matches — the containment Includes documented all along, with the same operand order. - `p.Overlaps(other)`: p and other match at least one ID in common; symmetric, and spelling-insensitive by construction. Both run in `O(n*m)` time and were verified against a brute-force set oracle on all 115,600 pattern pairs of up to 4 components. The ID matcher drops the candidate-side `*` special case it only carried for Includes, and Filter keeps its one-contains-the-other behavior with corrected docs. Signed-off-by: Johannes Großmann <grossmann.johannes@t-online.de>
joe0BAB
force-pushed
the
fix/include-check
branch
from
September 11, 2026 14:38
79cab6a to
cc41faa
Compare
Filter reduced two patterns to the contained one and returned false for overlapping-but-not-contained pairs, silently dropping providers whose namespace merely overlaps a query. No signature-compatible definition is exact, because the intersection of two patterns is not always expressible as a single pattern. Remove it until a caller needs a well-defined replacement. Signed-off-by: Johannes Großmann <grossmann.johannes@t-online.de>
joe0BAB
force-pushed
the
fix/include-check
branch
from
September 11, 2026 14:55
cc41faa to
a61cee4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pattern.Includesreused the ID matcher and treated a wildcard in the candidate pattern as matching anything, so it implemented neither containment nor overlap: it reported overlapping-but-not-contained patterns as included (docker/proj1/**claimed to includedocker/*/mcp/*), was asymmetric, and gave different answers for spellings with equal match sets (a/a/**vsa/a/**/**).Two changes:
Replace
IncludeswithContainsandOverlaps. Each relation is decided by a two-row DP over canonicalized wildcard tokens (runs of*/**collapse; a lone**becomes*/**since IDs are never empty):p.Contains(other)— p matches every ID that other matches: the containment Includes documented all along, with the same operand order.p.Overlaps(other)— p and other match at least one ID in common: symmetric, and spelling-insensitive by construction.Both run in
O(n*m)time and were verified against a brute-force set oracle on all 115,600 pattern pairs of up to 4 components. The ID matcher drops the candidate-side*special case it only carried for Includes.Remove
Filter. It reduced two patterns to the contained one and returned false for overlapping-but-not-contained pairs, silently dropping providers whose namespace merely overlaps a query. No signature-compatible definition is exact, because the intersection of two patterns is not always expressible as a single pattern; it is removed until a caller needs a well-defined replacement.