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.
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.
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.
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 latestin 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 latestasdfexecutable on the user's$PATHbin/asdf, parses the arguments, determines what asdfcommand_script to run and then sources it.latest_commandlatest_commandis a function that is invoked in the current processlatest_commandis 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.