Conversation
When a regex string like `/jq/` is provided to `brew search`, it can sometimes produce an error like "Error: /opt/homebrew/opt//jq/ is not a valid keg". So far I've only seen this with a very small number of installed formulae (I only stumbled upon this by chance). I'm not sure why some regexes trigger this and others don't but it traces back to the `Formulary.path` call in `MissingFormula.deleted_reason`. Either way, `MissingFormula.reason` shouldn't be called when the `query` value is a regex string, so this adds a guard to prevent it.
There was a problem hiding this comment.
Pull request overview
Fixes an error path in brew search when the user provides a regex query (e.g. /jq/), by preventing missing-formula lookup logic from running on regex-literal queries.
Changes:
- Skip
MissingFormula.reasonwhen the search query is a/.../regex literal. - Avoid triggering
Formulary.path/keg-path resolution for regex queries when printing “missing formula” help.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sig { params(query: String, found_matches: T::Boolean).void } | ||
| def print_missing_formula_help(query, found_matches) | ||
| return unless $stdout.tty? | ||
| return if query.start_with?("/") && query.end_with?("/") |
There was a problem hiding this comment.
This regex-query guard duplicates the detection logic in Search.query_regexp (which currently uses %r{^/(.*)/$}). To avoid future divergence (e.g., if regex query syntax changes), consider using the same pattern here or extracting a shared helper/predicate for “query is a regex literal” and calling that from both places.
| return if query.start_with?("/") && query.end_with?("/") | |
| return if Search.query_regexp(query) |
There was a problem hiding this comment.
Agreed this is a good idea.
There was a problem hiding this comment.
Would a regex constant [used in both places] be sufficient or is a method the preferred approach?
| sig { params(query: String, found_matches: T::Boolean).void } | ||
| def print_missing_formula_help(query, found_matches) | ||
| return unless $stdout.tty? | ||
| return if query.start_with?("/") && query.end_with?("/") | ||
|
|
||
| reason = MissingFormula.reason(query, silent: true) | ||
| return if reason.nil? |
There was a problem hiding this comment.
This change fixes a user-visible crash path for regex searches (e.g. brew search /jq/) by skipping MissingFormula.reason, but there’s no test asserting this behavior. Please add a spec (likely in Library/Homebrew/test/cmd/search_spec.rb) that stubs $stdout.tty? to true and verifies MissingFormula.reason is not invoked for /.../ queries, so the regression can’t reappear silently.
There was a problem hiding this comment.
A non-integration spec for this seems nice.
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks! Good once comments addressed.
| sig { params(query: String, found_matches: T::Boolean).void } | ||
| def print_missing_formula_help(query, found_matches) | ||
| return unless $stdout.tty? | ||
| return if query.start_with?("/") && query.end_with?("/") |
There was a problem hiding this comment.
Agreed this is a good idea.
| sig { params(query: String, found_matches: T::Boolean).void } | ||
| def print_missing_formula_help(query, found_matches) | ||
| return unless $stdout.tty? | ||
| return if query.start_with?("/") && query.end_with?("/") | ||
|
|
||
| reason = MissingFormula.reason(query, silent: true) | ||
| return if reason.nil? |
There was a problem hiding this comment.
A non-integration spec for this seems nice.
brew lgtm(style, typechecking and tests) with your changes locally?When a regex string like
/jq/is provided tobrew search, it can sometimes produce an error like "Error: /opt/homebrew/opt//jq/ is not a valid keg". So far I've only seen this with a very small number of installed formulae (I only stumbled upon this by chance). I'm not sure why some regexes trigger this and others don't but it traces back to theFormulary.pathcall inMissingFormula.deleted_reason. Either way,MissingFormula.reasonshouldn't be called when thequeryvalue is a regex string, so this adds a guard to prevent it.