i/prompting: implement path pattern matching and validation #13866
Conversation
olivercalder
added
the
Needs Samuele review
| break | ||
| } | ||
| switch r { | ||
| case '{': |
There was a problem hiding this comment.
I think I'm confused even more now. The spec does not mention {} for path patterns, so why validate this? Also, there's inconsistency between ValidatePathPattern and MatchPathPatter, where the latter is oblivious to {}, which means that a definition of a valid pattern is different for each of those calls.
There was a problem hiding this comment.
MatchPathPattern directly calls doublestar.Match to do matching (discussed some here: https://docs.google.com/document/d/1tBnefdukP69EUJOlH8bgD2hrvZCYoE8-1ZlqRRYlOqc/edit?pli=1#heading=h.a0e0vfqho1ip). This should be called on expanded patterns, but this isn't technically necessary, since doublestar.Match happily matches patterns with '{' '}' groups in them (see: https://pkg.go.dev/github.com/bmatcuk/doublestar#Match). Path pattern validation should occur on patterns before they are expanded, so they may contain '{' '}'-defined groups.
| } | ||
| case '\\': | ||
| // Skip next rune | ||
| _, _, err = reader.ReadRune() |
There was a problem hiding this comment.
isn't the escape sequence valid only for some of the characters that follow? eg what would a pattern like \c mean?
There was a problem hiding this comment.
According to doublestar.Match (https://pkg.go.dev/github.com/bmatcuk/doublestar#Match), \c matches c for any character c. I'm not adjusting that behavior at all, at the moment.
| matched, err := doublestar.Match(pattern, path) | ||
| if err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
wouldn't the pattern be validated prior to being used? Perhaps ValidatePathPatter should call doublestar.Match with say "/" as path, discard the result and only check the error?
There was a problem hiding this comment.
doublestar.ValidatePattern is called as part of ValidatePathPattern, so the pattern should be valid. This error should thus never occur when the PathPatternMatch is called in practice, but it's a check in case PathPatternMatch is called in isolation without being validated. So I'm inclined to leave the error return in place rather than discarding the error, for completeness.
Path pattern matching is implemented via the doublestar package, which
emulates bash's globstar matching. Patterns may include '*' wildcard
characters (which match any number of non-separator characters), '**'
doublestars (which match zero or more subdirectories), '?' wildcard
characters (which match exactly one non-separator character), and nested
groups delimited by '{' and '}'. Notably, path patterns are *not* allowed
to have character classes delimited by '[' and ']', nor inverted
classes of the form "[^abc]".
There is a limit on the number of groups allowed in path patterns, but
up to that limit, groups may be arbitrarily nested or sequential.
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
i/prompting: fix typo and add notes to remove test boilerplate
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
i/prompting: use separate test suite for patterns
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
i/prompting: improve unit test coverage
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
olivercalder
force-pushed
the
prompting-path-pattern-matching-validation
branch
from
1cab837
to
6e34c06
Compare
Path pattern matching is implemented via the doublestar package, which emulates bash's globstar matching. Patterns may include '*' wildcard characters (which match any number of non-separator characters), '**' doublestars (which match zero or more subdirectories), '?' wildcard characters (which match exactly one non-separator character), and nested groups delimited by '{' and '}'. Notably, path patterns are not allowed to have character classes delimited by '[' and ']', nor inverted classes of the form "[^abc]".
There is a limit on the number of groups allowed in path patterns, but up to that limit, groups may be arbitrarily nested or sequential.
This PR is split from #13730.