[Fix] nvm --help: render the set-colors legend in color - #3897
Conversation
ljharb
left a comment
There was a problem hiding this comment.
Thanks - I confirmed #3896 on a real pty and verified this branch fixes it (bash/dash/zsh): the legend renders colored on a tty, and stays plain when piped, with --no-colors, and on <8-color terminals. The approach matches the existing NVM_HAS_COLORS idiom in nvm_print_formatted_alias, and shellcheck/eclint are clean.
The main ask is on the tests: the new help test currently passes on unfixed master, so the user-facing fix has no regression coverage - details inline, with a verified alternative stub. A couple of smaller behavioral notes inline as well.
(The failing WSL Alpine check is an unrelated timeout flake.)
| : nvm.sh | ||
| \. ../../../nvm.sh | ||
|
|
||
| nvm_has_colors() { return 0; } |
There was a problem hiding this comment.
This stub defeats the same mechanism this PR fixes: function definitions are inherited by the $(...) subshells, so on master, nvm_wrap_with_color_code's direct nvm_has_colors call succeeds via the stub too, and the legend colors anyway. This test passes verbatim against master (checked in bash, dash, and zsh) - and reverting just the help-branch hunk in nvm() keeps the whole suite green while the legend regresses to plain on a real tty.
A stub whose success can be consumed exactly once distinguishes fixed from unfixed:
NVM_HAS_COLORS_MARKER="$(mktemp)"
nvm_has_colors() {
if [ -e "${NVM_HAS_COLORS_MARKER}" ]; then
rm -f "${NVM_HAS_COLORS_MARKER}"
return 0
fi
return 1
}On this branch, the single top-of-help computation consumes it, NVM_HAS_COLORS=1 short-circuits every wrap, and the legend is fully colored. On master, each wrap re-checks inside $(), finds it consumed, and the assertions fail. Verified both ways in bash/dash/zsh.
There was a problem hiding this comment.
Confirmed, the old stub passed on master in bash, dash, and zsh.
d0fab8b takes your one-shot marker, with one addition: the stub also returns 1 when NVM_NO_COLORS is --no-colors, so the file can end with a second run asserting that nvm --help --no-colors stays plain. Both legs fail on master, and the first one fails if only the nvm() hunk is reverted.
| fi | ||
| done | ||
|
|
||
| local NVM_HAS_COLORS |
There was a problem hiding this comment.
Note: an exported NVM_NO_COLORS='--no-colors' doesn't suppress this - the help branch resets NVM_NO_COLORS="" a few lines up before this computation runs, and that clobber used to be invisible because the legend was always plain. The --no-colors flag form works correctly. Initializing it as NVM_NO_COLORS="${NVM_NO_COLORS-}" up there would make the env var behave like it does for nvm set-colors. Possibly out of scope - nvm ls shadows the env var the same way today.
There was a problem hiding this comment.
I tried NVM_NO_COLORS="${NVM_NO_COLORS-}" there and backed it out.
The assignment in the help branch is not local, so nvm --help --no-colors already leaves --no-colors set in the caller's shell. Today the next nvm --help clears it again. Preserve the value instead and it sticks, so every later nvm --help and nvm set-colors in that session stays plain. I ran into exactly that on a pty while checking this.
Doing it properly means giving the help branch a real local, and then nvm ls deserves the same, so I left it out here. The help test does cover the flag form staying plain.
02a76b2 to
d0fab8b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
| OUTPUT="$(nvm set-colors bygre)" | ||
| EXPECTED_OUTPUT="$(command printf %b 'Setting colors to: \033[0;34mb\033[0m\033[0;33my\033[0m\033[0;32mg\033[0m\033[0;31mr\033[0m\033[0;37me\033[0m')" | ||
|
|
||
| [ "_${OUTPUT}" = "_${EXPECTED_OUTPUT}" ] || die "expected >${EXPECTED_OUTPUT}<; got >${OUTPUT}<" |
There was a problem hiding this comment.
One hunk is still unpinned: deleting local NVM_HAS_COLORS / NVM_HAS_COLORS=0 in nvm_set_colors (nvm.sh:1133-1134) leaves all four tests green - and the leak that local guards is real. Without it, line 1145's NVM_HAS_COLORS=1 persists in the sourcing shell after a colored nvm set-colors, and a later nvm alias foo 18 | cat emits escape codes into piped output (the alias-creation path doesn't shadow the variable, and nvm_print_formatted_alias reads the ambient value). This test can't see any of that because it only runs set-colors inside $(...), where the subshell swallows the global.
Pinning it needs a fresh marker plus a direct, uncaptured call - with the marker already consumed, the warning branch runs, line 1145 never executes, and the mutant passes:
rm -f "${NVM_HAS_COLORS_MARKER}"
NVM_HAS_COLORS_MARKER="$(mktemp)"
nvm set-colors bygre >/dev/null
[ -z "${NVM_HAS_COLORS-}" ] || die "nvm set-colors leaked NVM_HAS_COLORS=${NVM_HAS_COLORS} into the shell"There was a problem hiding this comment.
9d79391 adds this. Verified by mutation: deleting local NVM_HAS_COLORS at nvm.sh:1133 makes the test fail with nvm set-colors leaked NVM_HAS_COLORS=1 into the shell, restoring the line passes it again. Checked in bash, dash, zsh, sh.
| rm -f "${NVM_HAS_COLORS_MARKER}" | ||
| } | ||
|
|
||
| HELP="$(nvm --help)" |
There was a problem hiding this comment.
The help branch's local NVM_HAS_COLORS (nvm.sh:3421) has the same blind spot: every observation in this file goes through $(...), whose subshell would swallow a leak into the calling shell. One direct call at the end pins it - this branch assigns unconditionally, so no marker gymnastics are needed:
nvm --help >/dev/null
[ -z "${NVM_HAS_COLORS-}" ] || die "nvm --help leaked NVM_HAS_COLORS=${NVM_HAS_COLORS} into the shell"There was a problem hiding this comment.
9d79391 adds this. Verified by mutation: deleting local NVM_HAS_COLORS at nvm.sh:3421 fails the same way (nvm --help leaked NVM_HAS_COLORS=1 into the shell), restoring passes. Checked in bash, dash, zsh, sh.
|
|
||
| OUTPUT="$(nvm_nvmrc_invalid_msg 'lts/*' 2>&1)" | ||
| ESC="$(command printf %b '\033[')" | ||
| case "${OUTPUT}" in |
There was a problem hiding this comment.
This assertion is negative-only, so it also passes on empty output - and this file is the only test in the repo that touches nvm_nvmrc_invalid_msg at all. One positive assertion makes it prove the message survives, not just that ESC is absent:
case "${OUTPUT}" in
*'invalid .nvmrc!'*) ;;
*) die "expected the invalid-.nvmrc message; got >${OUTPUT}<" ;;
esacThere was a problem hiding this comment.
9d79391 adds this. Verified by mutation: blanking the invalid .nvmrc! string in error_text fails the assertion as expected, restoring it passes. Checked in bash, dash, zsh, sh.
…id-.nvmrc message Address ljharb's review: assert `nvm --help` and `nvm set-colors` don't leak the `local NVM_HAS_COLORS` guard into the calling shell, and assert `nvm_nvmrc_invalid_msg` still emits its actual message text, not just that it's uncolored.
The
set-colorslegend innvm --helpbuilds its samples inside command substitution, sonvm_wrap_with_color_codesees a pipe on stdout,nvm_has_colorsreturns false, and every token falls back to plain text. That section can never show color, on any terminal.nvm set-colorsprints its confirmation line the same way and loses its colors too, even though it already checked that colors are supported.nvm_wrap_with_color_codenow accepts a pre-computedNVM_HAS_COLORS, the waynvm_print_alias_pathalready does, and both call sites compute it before wrapping. Output stays plain with--no-colors, when stdout is not a terminal, or when the terminal has fewer than 8 colors.Fixes #3896