-
Notifications
You must be signed in to change notification settings - Fork 94
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
Avoid checking PATH for functions #161
Conversation
I force-pushed this a couple times just to clean it up. |
Have you made a benchmark? The command substitution $ time for i in {000..999}; do [[ $(PATH= type -t _nonexistent) == function ]]; done
real 0m2.894s
user 0m1.010s
sys 0m1.898s
$ time for i in {000..999}; do [[ $(PATH= type -t _comp_initialize) == function ]]; done
real 0m2.954s
user 0m1.063s
sys 0m1.954s
$ time for i in {000..999}; do type -t _nonexistent >/dev/null; done
real 0m0.034s
user 0m0.019s
sys 0m0.014s
$ time for i in {000..999}; do type -t _comp_initialize >/dev/null; done
real 0m0.015s
user 0m0.012s
sys 0m0.004s
Despite the name "precmd_function", an entry in the array could be an executable file. Users have been able to specify executable command names, and they have worked all the time until now. This PR breaks the compatibility. In addition, for the users who specify only function names in |
Hey there, thanks for looking at this. I did not benchmark. The motivator for this PR is a badly-behaved system where FWIW, command substitutions typically don't fork in bash unless they call an external command. Bash has some internal optimizations to avoid forking for builtins, for example. You can see this with:
That's a great point about searching functions before PATH, though! I agree I opened this without sufficient research and experimentation! |
No, you misunderstand
You should check the results of the following commands: $ echo "$$:$BASHPID"
1997853:1997853
$ echo $(echo "$$:$BASHPID")
1997853:1998890 |
Oh, thanks! |
Searching PATH for functions is slow and unnecessary...