-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Fix tap constants. #16728
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
Fix tap constants. #16728
Conversation
b3f4a44
to
c65f640
Compare
c65f640
to
677a61f
Compare
677a61f
to
ed07203
Compare
%r{\A(?<user>[\w-]+)/(?<repo>[\w-]+)/#{HOMEBREW_TAP_FORMULA_NAME_REGEX.source}\Z}, | ||
%r{\A(?<user>[^/]+)/(?<repo>[^/]+)/#{HOMEBREW_TAP_FORMULA_NAME_REGEX.source}\Z}, |
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 for the quick fix but: why do we need to permanently widen the range of acceptable tap names to fix this regression?
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.
@MikeMcQuaid, for example, brew tap <user>/<repo>
doesn't validate the name at all, except for the check for slashes in Tap.fetch
. So this was always broken in these regexes (and in Tap::from_path
which uses them), we just weren't using them consistently for every loader in Formulary
before, and we weren't using Tap::from_path
in Tap::each
before.
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.
@reitermarkus Ok. I'd like in future changes like this that are not strictly a fix but an actual functionality change to have some discussion before a critical
self-merge, thanks.
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.
It doesn't change functionality though, except for making Tap::from_path
consistent with what we actually allow in other places (e.g. Tap::fetch
). Note that e.g. brew install some/tap.with.dots/myformula
always worked, we just didn't use these regexes consistently in Formulary
before.
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Accept any tap name, as long as it is a valid directory name, i.e. doesn't contain slashes in
user
orrepo
.Fixes #16727.