refactor: read alias files with shell builtins instead of sed/awk/head/tail pipeline#3787
Conversation
however, they could contain anything, and we must be robust against that. (i haven't reviewed yet, to be clear) |
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)?
[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)
2fd18bc to
d62de0e
Compare
…ad/tail pipeline See nvm-sh#3787.
ljharb
left a comment
There was a problem hiding this comment.
Nice direction overall - the nvm_alias rewrite is clean and the edge-case tests are welcome. However there's a critical bug in nvm_resolve_alias: changing SEEN_ALIASES from newline-delimited to space-delimited broke cycle detection (see inline). The PR description says the cycle check was replaced with a case pattern match, but the diff shows it wasn't. I verified by checking out this branch and running nvm_resolve_alias loopback against the existing test/fast/Aliases/circular/ fixtures - it hangs. The claim of "Tested in bash, zsh, dash, ksh, and sh" is inconsistent with this; running urchin test/fast/Aliases/circular/ in any shell would catch it.
Needed before merge: the case-based cycle check the description promised, plus making sure the existing test/fast/Aliases/circular/ fixtures still pass.
A couple smaller notes inline.
Replace the per-character trailing-whitespace loop with a one-pass parameter expansion, per review.
|
Thanks for the catch and the reproduction recipe. Pushed The regression: the earlier commit changed In
Verification:
PR description revised to match the diff. |
|
Re-ran The |
This comment was marked as spam.
This comment was marked as spam.
…nreadable The sed/awk pipeline exits with awk's status, so an existing-but-unreadable alias file produces empty output with status 0 - but only by accident, alongside sed's read error on stderr. Make that contract explicit: a nonzero status here would flip `nvm_ensure_default_set` from "default is already set" to recreating it, silently overwriting a write-only default alias.
[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.
[Fix] `nvm_resolve_alias`: detect cycles via newline-anchored `case`
The original commit referenced above changed SEEN_ALIASES from `\n`-delimited
storage (interpreted by `printf '%b' | nvm_grep -e "^${name}$"`) to space-
delimited but left the line-anchored grep in place — without newlines in
the haystack the anchored pattern can never match, so cycles never break.
Switch to literal-newline storage and a `case` pattern anchored on those
newlines. Newline anchoring also handles alias names containing spaces,
which token-based patterns false-positive on (e.g. lookup of `bar` matches
substring " bar " inside " foo bar midway " when the chain visits the
multi-token alias `foo bar`).
New test file covers self-loop, multi-hop loop, cycle through a
space-bearing alias name, and a non-cycle through a space-bearing
alias name. Existing `test/fast/Aliases/circular/` fixtures continue
to pass.
…res in teardown The cycle scenarios all pass with the old grep-based detection too; the bug the `case`-based detection actually fixes is resolved names interpolated into the grep pattern as regexes, where resolving axb -> a.b falsely reported ∞ because the pattern `a.b` matches the seen name `axb`. Pin both directions: the fixed false positive, and a genuine cycle through a metachar name. Also correct the space-name scenario comment (the old anchored grep passed that scenario; only token-delimited seen-storage would not), and register every new fixture name in the suite teardown, so a mid-test failure can not leak aliases into later tests.
b763492 to
ab54358
Compare
ljharb
left a comment
There was a problem hiding this comment.
did some rebasing and added some commits
ab54358 to
ebada63
Compare
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. Trailing whitespace is then stripped in one pass via parameter expansion.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. Cycle detection uses acasepattern anchored on literal newlines inSEEN_ALIASES, replacing the priorprintf '%b' | nvm_grep -q "^name$"pipeline. Newline anchoring also handles alias names containing spaces; a token-based pattern would false-positive onnvm alias 'foo bar' midwaychained tonvm alias midway bar.All replacements are POSIX (
read -r,case,IFS=, parameter expansion).localdeclarations are separated from assignments for ksh compatibility.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 cover: empty alias file, comment-only file, trailing whitespace, 4-deep chain, nonexistent target. Cycle tests cover: self-loop, multi-hop loop, cycle through a space-bearing alias name, and a non-cycle through a space-bearing alias name. Existing
test/fast/Aliases/circular/fixtures continue to pass.