-
Notifications
You must be signed in to change notification settings - Fork 220
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: Improve script performance and reliability #552
fix: Improve script performance and reliability #552
Conversation
3c1a5d1
to
5e52c52
Compare
Thank you for submitting a PR! I will take a look and come back in a few days. |
Are you sure this will fix #510? In the issue they have |
Oh oops, you're right, it doesn't fix that issue. I was having issues where the existing lefthook binary couldn't be found because I was in a subdirectory of the root git directory - and I did not read that issue carefully enough and thought it was the same problem. I removed the reference to that issue. |
@hyperupcall , I've just reviewed the PR one more time. I can't think of a case when we need to check parent directories to find the lefthook executable in node_modules. What do you think about removing this part? Other optimizations look good to me. |
5e52c52
to
e9ed0a5
Compare
@mrexox Sounds good - I have removed the recursive parent directory search. I have kept the other optimizations. Have gave everything another look and test and it's ready |
try_exec "./node_modules/lefthook/bin/index.js" "$@" | ||
try_exec "./node_modules/@evilmartians/lefthook/bin/lefthook_${osArch}_${cpuArch}/lefthook{{.Extension}}" "$@" | ||
try_exec "./node_modules/@evilmartians/lefthook-installer/bin/lefthook_${osArch}_${cpuArch}/lefthook{{.Extension}}" "$@" |
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.
There was $dir variable assigned before. Since we don't check upper directories, please, add this variable
Because the original changes of this PR did not fully fix the mentioned issue, and the current diff no longer reflecting the original issue, and the length of time that has passed since creating this issue, I'm closing this PR. If needed, someone else can pick up the torch 🔥 - a fresh PR would probably be easier to review too |
Previously, only lefthook stuff under the current$PWD/node_modules
was checked. Now, the current directory up until the Git root is checked (seetry_exec
function.I did chang the formatting since it was really difficult to parse the shell script. If you want to change it back, let me know.
⚡ Summary
Optimizations:
Don't waste time running a subshell withgit rev-parse --show-toplevel
when it is not neededlefthook
to determine iflefthook
can be run (usecommand -v
instead)'exec
to run commandsNote that this slightly changes the resolution algorithm. Before, it would look for./node_modules/lefthook/bin/index.js
, then./node_modules/@evilmartians/lefthook/bin/lefthook_${osArch}_${cpuArch}/lefthook{{.Extension}}
, etc. Now, it looks for./node_modules/lefthook/bin/index.js
, then../node_modules/lefthook/bin/index.js
, etc., all the way up to the git root. Once it reaches git root, then it goes back to the original$PWD
and looks for./node_modules/@evilmartians/lefthook/bin/lefthook_${osArch}_${cpuArch}/lefthook{{.Extension}}
, and climbs up the directory tree for that path.☑️ Checklist