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
cmd/prof: raise error when cmd is bash file #10280
cmd/prof: raise error when cmd is bash file #10280
Conversation
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 @hyuraku! some comments.
Library/Homebrew/commands.rb
Outdated
@@ -197,4 +197,8 @@ def rebuild_commands_completion_list | |||
file = HOMEBREW_CACHE/"all_commands_list.txt" | |||
file.atomic_write("#{commands(aliases: true).sort.join("\n")}\n") | |||
end | |||
|
|||
def only_bash_command_list | |||
internal_commands.reject { |cmd| valid_internal_cmd?(cmd) } |
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.
I think this function makes sense but not the implementation. This should check for a file extension instead.
Library/Homebrew/brew.rb
Outdated
@@ -131,6 +131,8 @@ class MissingEnvironmentVariables < RuntimeError; end | |||
end | |||
exec "brew-#{cmd}", *ARGV | |||
else | |||
raise "command made by bash not ruby: #{cmd}" if Commands.only_bash_command_list.include?(cmd) |
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 seem like this should be in brew.rb
but prof.rb
instead.
Library/Homebrew/dev-cmd/prof.rb
Outdated
@@ -26,6 +26,8 @@ def prof | |||
|
|||
brew_rb = (HOMEBREW_LIBRARY_PATH/"brew.rb").resolved_path | |||
FileUtils.mkdir_p "prof" | |||
cmd = args.named.first | |||
raise "#{cmd} is made by bash not ruby" if Commands.path(cmd).extname == ".sh" |
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.
raise "#{cmd} is made by bash not ruby" if Commands.path(cmd).extname == ".sh" | |
raise UsageError, "#{cmd} is a Bash command!" if Commands.path(cmd).extname == ".sh" |
76f1a7c
to
c56fff3
Compare
@MikeMcQuaid |
Thanks again @hyuraku! |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?brew man
locally and committed any changes?brew prof update
caused an error becauseupdate
is made by bash, not ruby. However, the error messageError: Unknown command: update
was not correct, so I added the correct message.