Repository navigation
[Perf]: Optimize nvm_tree_contains_path by eliminating subshell loop - #3912
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new case-pattern containment check can misbehave for directory names containing glob metacharacters (e.g., *, ?, []), causing incorrect containment results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR optimizes nvm_tree_contains_path in nvm.sh, aiming to remove repeated dirname subprocess calls and speed up path containment checks that are used in common nvm operations.
Changes:
- Replaced the prior
dirname-driven parent-walk with an in-memory string/pattern-based containment check. - Added unit tests for tree paths with trailing slashes and for
/containing an absolute descendant path.
File summaries
| File | Description |
|---|---|
| nvm.sh | Refactors nvm_tree_contains_path to avoid per-iteration subprocesses. |
| test/fast/Unit tests/nvm_tree_contains_path | Extends unit coverage for trailing-slash trees and the root tree case. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ebed861 to
f4dbc6a
Compare
|
Updated the branch addressing the review feedback:
|
This comment was marked as spam.
This comment was marked as spam.
2d5f3d7 to
13eaa28
Compare
|
Updated
|
Replace looping subshell process forks (dirname) in nvm_tree_contains_path with case-guarded in-memory POSIX parameter expansion () and exact string equality comparisons. Ensures literal string matching for glob metacharacters (*, ?, []) and full zsh compatibility across all platforms.
eefc4a2 to
c56e953
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The updated containment logic removes subprocess overhead while preserving expected behavior, and the added unit tests cover the key new edge cases.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
|
@schnare22 That was the review for the initial draft. Copilot AI has already re-reviewed the updated commit ( |
c56e953 to
f7f4263
Compare
|
Freshly rebased the PR branch on top of the latest |
f7f4263 to
cd84623
Compare
|
Thank you @ljharb for the review and for merging this. Glad to contribute to nvm. |
Bug Fixes - `nvm_alias`, `nvm_version_path`: reject `..` path components - `nvm_get_make_jobs`: count cores on platforms the `case` does not name - `nvm-exec`: improve the no-version failure message - `nvm use`: do not clobber `man`'s default search path (#3890) - `nvm_err`, `nvm_err_with_colors`: do not fail when stderr is closed (#3907) - `nvm_hash_reset`: only run `hash -r` where the shell supports it - `nvm_alias`: unset zsh's `extendedglob` while reading an alias file (#3891) - zsh: restore `nomatch` and `markdirs` rather than clobbering them - `nvm --help`: render the set-colors legend in color (#3915) - `nvm unload`: drop the undefined `nvm_format_help_message_colors` - `nvm_get_mirror`: allow ports, userinfo, IPv6 hosts, and percent-encoding - `nvm_sanitize_auth_header`: allow `~`, completing RFC 7235 `token68` (#3751) Refactors - `nvm --help`: extract the set-colors legend into `nvm_print_color_legend` Performance - Optimize nvm_tree_contains_path with in-memory POSIX parent-walk (#3912) Docs - [readme] document platforms with no official node binaries - `install.sh`: Fix missing words in bash/zsh completion message (#3887) - Remove stray closing HTML tag (#3919) Misc - [meta] remove CLAUDE.md symlink in favor of AGENTS.md (#3928) - [meta] add `release-notes.sh`, the release notes generator Tests - homebrew/actions changed their default branch - [actions] WSL: pin every distribution, and test the newest of each line - `nvm_print_color_legend`: look up the function with `PATH` emptied - `nvm_print_color_legend`: check for the function, not a same-named file - `nvm_print_color_legend`: fix the `fast` suite on GNU userlands - [actions] `release.yml`: provide `GH_TOKEN` so release-notes PR lookup can run
What usually happens
The function
nvm_tree_contains_pathchecks if a target file path resides within a parent directory tree. To do this, it runs awhileloop that callsdirnamerepeatedly to move up directory levels one component at a time until it matches the root tree path or reaches the root directory.Why is that a bottleneck
Calling
dirnameinside a subshell$(dirname ...)spawns a new subshell process on every loop iteration. For paths that are 7 to 10 directory levels deep, a single call tonvm_tree_contains_pathcreates 7 to 10 process forks. Running this function across multiple version checks accumulates process creation overhead during shell initialization and commands likenvm current,nvm use, andnvm ls.How we are solving it
We replace the
dirnamesubshell loop with pure POSIX in-memory parameter expansion and pattern matching.Why does it work
Checking whether a path is inside a directory tree is equivalent to checking if the path starts with that directory path followed by a slash boundary. Removing trailing slashes and performing string pattern matching produces the exact same containment result as stepping up directory levels with
dirname.How does it work
clean_tree="${clean_tree%/}".casestatement: exact match"${clean_tree}"or child match"${clean_tree}"/*."${clean_tree}"inside thecasepattern ensures characters like*or?in directory names are matched literally and not treated as wildcards./, stripping the slash reducesclean_treeto empty string"", which matches/*for any absolute path.What is the result
Subshell process creation drops from N subshells to 0. In benchmark testing of 50 calls:
This speedup for this function, making shell initialization and version checks faster and lag-free.
How to reproduce and verify
Run the unit test suite:
sh "test/fast/Unit tests/nvm_tree_contains_path"