-
Notifications
You must be signed in to change notification settings - Fork 2.6k
add glob pattern support for known_hosts #15508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Oops, fixing the failing tests... (edit: fixed) |
fb2c996
to
09ae8fe
Compare
src/cargo/sources/git/known_hosts.rs
Outdated
regex::escape(&pattern.replace("*", ",,,,").replace("?", ";;;;")) | ||
.replace(",,,,", ".*") | ||
.replace(";;;;", "."); | ||
if let Ok(regex) = Regex::new(®ex_pattern) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do anything on errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, my thought is that if the pattern string could not be interpreted as glob pattern it's better to just pass glob matching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably emit a tracing::warn!
on failing cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds nice!
09ae8fe
to
786a78a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Just one nit and we are good to go!
@@ -588,7 +589,14 @@ impl KnownHost { | |||
} | |||
for pattern in self.patterns.split(',') { | |||
let pattern = pattern.to_lowercase(); | |||
// FIXME: support * and ? wildcards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was under the impression that this feature is tracked in #11577, but apparently not.
It seems like OpenSSH supports only *
and ?
wildcards. One caveat of using glob
here is we provide more than we should. Anyway, it is good to catch up with the common format.
786a78a
to
087285d
Compare
087285d
to
e68d951
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks for the contribution :)
What does this PR try to resolve?
This PR implements glob pattern match for known_hosts file. Hosts written with
*
and?
now matches correctly.Tests
Tests are added accordingly.
Miscs
This is my first time submitting PR, sorry if there's anything that's off although I've read the contributor guide.