-
Notifications
You must be signed in to change notification settings - Fork 777
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: don't invoke asdf inside asdf commands #1208
Conversation
Recursive calls have a number of disadvantages: * Poorer performance since each invocation spawns and new process and re-executes all the code in bin/asdf * Makes debugging more difficult * More likely to introduce subtle bugs and the possibility for infinite loops
@@ -48,7 +50,8 @@ current_command() { | |||
|
|||
# printf "$terminal_format" "PLUGIN" "VERSION" "SET BY CONFIG" # disbale this until we release headings across the board | |||
if [ $# -eq 0 ]; then | |||
for plugin in $(asdf plugin list); do | |||
# shellcheck disable=SC2119 | |||
for plugin in $(plugin_list_command); do |
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.
asdf
is a binary on the user's path, whereas plugin_list_command
is a Bash function already in memory. While the code looks similar invoking asdf
results in a new process that re-executes all the code in bin/asdf
.
@@ -1,4 +1,6 @@ | |||
# -*- sh -*- | |||
# shellcheck source=lib/functions/versions.bash | |||
. "$(dirname "$(dirname "$0")")/lib/functions/versions.bash" |
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.
Since we are now invoking the functions instead of bin/asdf
I moved functions that need to be invoked from multiple commands into a couple "function" files that store related functions. In this case, functions related to tool versions.
# shellcheck source=lib/commands/reshim.bash | ||
. "$(dirname "$ASDF_CMD_FILE")/reshim.bash" | ||
# shellcheck source=lib/functions/installs.bash | ||
. "$(dirname "$(dirname "$0")")/lib/functions/installs.bash" |
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.
Code was re-arranged and calls to asdf <cmd>
were replaced with the corresponding function calls, but no other code was changed. The diff is pretty big but most of it is moving code into the new function files.
Ping. |
Merging now as the only code changes here are to replace internal |
Sorry I didn't get to reviewing this before merge. Really liking the |
Yes absolutely! |
Recursive calls have a number of disadvantages:
Fixes: No known bugs this fixes, but hopefully these changes have flushed out a few obscure bugs.
Reviewing this code you'll see commands like
asdf latest
in the code have been replaced by function calls likelatest_command
. I'll break down how each of these work to show how important this fix is.asdf latest
asdf
executable on the user's$PATH
bin/asdf
, parses the arguments, determines what asdfcommand_
script to run and then sources it.latest_command
latest_command
is a function that is invoked in the current processlatest_command
is executed, but everything it does is needed by the calling script, so there is no wasteAs you can see there is a lot of unnecessary overhead when executing
asdf ...
commands from inside other asdf command scripts. I did some rough benchmarking locally and saw a speedup of about 10% with this fix.