refactor: read alias files with shell builtins instead of sed/awk/head/tail pipeline#3787
refactor: read alias files with shell builtins instead of sed/awk/head/tail pipeline#3787jakelodwick wants to merge 2 commits intonvm-sh:masterfrom
Conversation
[Tests] `nvm_alias`, `nvm_resolve_alias`: add edge-case tests nvm_alias() used a sed/awk pipeline to strip comments and blank lines from alias files that almost always contain a single word. A while-read loop with parameter expansion does the same filtering more directly. nvm_resolve_alias() piped nvm_alias through head and tail to extract one line, and used printf/grep for cycle detection. Parameter expansion and a case statement replace both without the extra plumbing. All replacements are POSIX (read -r, case, IFS=, parameter expansion). As a side effect, this also removes 4 external process invocations during shell init.
however, they could contain anything, and we must be robust against that. (i haven't reviewed yet, to be clear) |
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
It would be good to add more test cases for edge cases, like binary data, very long lines, embedded NUL characters, no trailing newline, other whitespace characters (carriage return \r, form feed, etc.) that [[:space:]] would catch etc, an alias that has spaces (nvm alias 'foo bar' node, eg)?
nvm.sh
Outdated
| while IFS= read -r NVM_ALIAS_LINE || [ -n "${NVM_ALIAS_LINE}" ]; do | ||
| NVM_ALIAS_LINE="${NVM_ALIAS_LINE%%#*}" | ||
| case "${NVM_ALIAS_LINE}" in | ||
| *[!\ \ ]*) ;; |
There was a problem hiding this comment.
is there another way to represent the literal tab here, that's less likely to be accidentally converted to spaces by some random tool?
There was a problem hiding this comment.
Yes — [[:space:]] POSIX class. Can't be mangled by editors or formatters, and it's already used in nvm.sh (lines 540, 569, 570). The trailing-strip uses %[[:space:]] so the removal is explicit too.
[Tests] `nvm_alias`: add edge-case tests for hostile file content
|
Added seven test files: binary data after version, CRLF + bare CR, embedded NUL, form feed + vertical tab, no trailing newline, 10k-char long line, alias name with spaces. All passing in sh/bash/zsh/dash. |
|
Agreed. The new edge-case tests exercise exactly that. |
…d/tail pipeline (fixes nvm-sh#3787)
2 participants
Responding to @ljharb's note in #1261 — "If you can think of ways to speed up alias resolution, I'm all for it" — this simplifies how
nvm_alias()andnvm_resolve_alias()read alias files.nvm_alias()currently pipes throughsed | awkto strip comments and blank lines. Alias files almost always contain a single line with one version string, so the pipeline is more machinery than the job needs. Awhile IFS= read -rloop with inline filtering does the same work more directly.nvm_resolve_alias()wraps that inhead -n N | tail -n 1to extract a single line. Since we only ever need the first non-empty line, parameter expansion (${var%%newline*}) does the same thing without the pipeline. The cycle-detection check (printf | nvm_grep -q) is similarly replaced with acasepattern match — same logic, reads more clearly.All replacements are POSIX (
read -r,case,IFS=, parameter expansion).localdeclarations are separated from assignments for ksh compatibility. Tested in bash, zsh, dash, ksh, and sh.As a side effect, this removes several subprocess forks per alias lookup — 5 for a single-hop alias, up to 12 for a two-hop with cycle check.
Edge-case tests added: empty alias file, comment-only file, trailing whitespace, 4-deep chain, nonexistent target.