diff --git a/nvm.sh b/nvm.sh index 9e3f4d60..8032a6e5 100755 --- a/nvm.sh +++ b/nvm.sh @@ -1129,6 +1129,8 @@ nvm_set_colors() { local CURRENT_COLOR local NOT_INSTALLED_COLOR local DEFAULT_COLOR + local NVM_HAS_COLORS + NVM_HAS_COLORS=0 INSTALLED_COLOR="$(echo "$1" | awk '{ print substr($0, 1, 1); }')" LTS_AND_SYSTEM_COLOR="$(echo "$1" | awk '{ print substr($0, 2, 1); }')" @@ -1139,6 +1141,7 @@ nvm_set_colors() { nvm_echo "Setting colors to: ${INSTALLED_COLOR} ${LTS_AND_SYSTEM_COLOR} ${CURRENT_COLOR} ${NOT_INSTALLED_COLOR} ${DEFAULT_COLOR}" nvm_echo "WARNING: Colors may not display because they are not supported in this shell." else + NVM_HAS_COLORS=1 nvm_echo_with_colors "Setting colors to: $(nvm_wrap_with_color_code "${INSTALLED_COLOR}" "${INSTALLED_COLOR}")$(nvm_wrap_with_color_code "${LTS_AND_SYSTEM_COLOR}" "${LTS_AND_SYSTEM_COLOR}")$(nvm_wrap_with_color_code "${CURRENT_COLOR}" "${CURRENT_COLOR}")$(nvm_wrap_with_color_code "${NOT_INSTALLED_COLOR}" "${NOT_INSTALLED_COLOR}")$(nvm_wrap_with_color_code "${DEFAULT_COLOR}" "${DEFAULT_COLOR}")" fi export NVM_COLORS="$1" @@ -1176,7 +1179,10 @@ nvm_wrap_with_color_code() { CODE="$(nvm_print_color_code "${1}" 2>/dev/null ||:)" local TEXT TEXT="${2-}" - if nvm_has_colors && [ -n "${CODE}" ]; then + # `nvm_has_colors` cannot answer for itself inside a command substitution, + # where `[ -t 1 ]` sees the capture pipe: callers that already checked pass + # the answer in, as `nvm_print_alias_path` also accepts it. + if { [ "${NVM_HAS_COLORS-}" = 1 ] || nvm_has_colors; } && [ -n "${CODE}" ]; then nvm_echo_with_colors "\033[${CODE}${TEXT}\033[0m" else nvm_echo "${TEXT}" @@ -1184,9 +1190,14 @@ nvm_wrap_with_color_code() { } nvm_print_color_legend() { - # Every line is built in a command substitution, so this cannot stay - # inline: resolving color support has to happen in one scope that all of - # them share. Pure code motion for now; see the following commit. + # Every line below builds its text in a command substitution, which cannot + # detect color support itself, so resolve it once here. `local` matters: + # leaking the flag would override `--no-colors` for every later call. + local NVM_HAS_COLORS + NVM_HAS_COLORS=0 + if nvm_has_colors; then + NVM_HAS_COLORS=1 + fi nvm_echo ' Initial colors are:' nvm_echo_with_colors " $(nvm_wrap_with_color_code 'b' 'b')$(nvm_wrap_with_color_code 'y' 'y')$(nvm_wrap_with_color_code 'g' 'g')$(nvm_wrap_with_color_code 'r' 'r')$(nvm_wrap_with_color_code 'e' 'e')" nvm_echo ' Color codes:' diff --git a/test/fast/Unit tests/nvm_print_color_legend b/test/fast/Unit tests/nvm_print_color_legend index b82cc8ab..f7279588 100755 --- a/test/fast/Unit tests/nvm_print_color_legend +++ b/test/fast/Unit tests/nvm_print_color_legend @@ -1,11 +1,21 @@ #!/bin/sh -die () { echo "$@" ; exit 1; } +ONESHOT='' +cleanup () { + if [ -n "${ONESHOT}" ]; then + command rm -f "${ONESHOT}" + fi +} +die () { echo "$@" ; cleanup ; exit 1; } \. ../../../nvm.sh set -e +# assert about nvm, not about the caller's environment +NVM_HAS_COLORS_WAS="${NVM_HAS_COLORS-}" +unset NVM_HAS_COLORS + ESC="$(printf '\033')" # The legend's letters are wrapped in color codes when the terminal supports # them, so every text assertion below compares against the output with any @@ -77,6 +87,87 @@ HELP_PLAIN="$(nvm --help | strip_colors)" contains_line ' Color codes:' "${HELP_PLAIN}" \ || die 'nvm --help no longer includes the color legend' +# --- the fix: colors have to survive command substitution ------------------ + +# the bug condition: inside `$( )`, `[ -t 1 ]` sees the capture pipe, so +# `nvm_has_colors` always fails +nvm_has_colors () { return 1; } + +WRAPPED="$(nvm_wrap_with_color_code 'b' 'b')" +case "${WRAPPED}" in + *"${ESC}"*) die "expected plain text when colors are unsupported, got >${WRAPPED}<" ;; +esac + +NVM_HAS_COLORS=1 +export NVM_HAS_COLORS +WRAPPED="$(nvm_wrap_with_color_code 'b' 'b')" +[ "${WRAPPED}" = "${ESC}[0;34mb${ESC}[0m" ] \ + || die "expected a colored 'b' with NVM_HAS_COLORS=1, got >${WRAPPED}<" + +WRAPPED="$(nvm_wrap_with_color_code 'X' 'X')" +[ "${WRAPPED}" = 'X' ] \ + || die "expected plain text for an unknown color code, got >${WRAPPED}<" +unset NVM_HAS_COLORS + +# `0` is what the resolving callers start from, so it must not read as truthy +NVM_HAS_COLORS=0 +export NVM_HAS_COLORS +WRAPPED="$(nvm_wrap_with_color_code 'b' 'b')" +case "${WRAPPED}" in + *"${ESC}"*) die "NVM_HAS_COLORS=0 must not enable colors, got >${WRAPPED}<" ;; +esac +unset NVM_HAS_COLORS + +UNSUPPORTED="$(nvm_print_color_legend)" +case "${UNSUPPORTED}" in + *"${ESC}"*) die "the legend emitted colors while unsupported: ${UNSUPPORTED}" ;; +esac + +# Model the real asymmetry: yes to the first, direct call, no to every later +# one. The state has to live in a file, because each `$( )` is a fresh subshell +# that would take its own copy of an in-shell counter and answer yes every time. +ONESHOT="$(command mktemp)" +oneshot_reset () { nvm_echo 0 > "${ONESHOT}"; } +nvm_has_colors () { + if [ "$(command cat "${ONESHOT}")" = 0 ]; then + nvm_echo 1 > "${ONESHOT}" + return 0 + fi + return 1 +} + +oneshot_reset +COLORED_OUTPUT="$(nvm_print_color_legend)" +# `grep -c` drains its input, so it cannot SIGPIPE its producer +COLORED="$(nvm_echo "${COLORED_OUTPUT}" | nvm_grep -c "${ESC}" || true)" +[ "${COLORED}" = 9 ] \ + || die "expected all 9 legend lines colored, got ${COLORED}: ${COLORED_OUTPUT}" + +# the sample specifically, which is the case #3896 reports +case "${COLORED_OUTPUT}" in + *"${ESC}[0;34mb${ESC}[0m${ESC}[0;33my${ESC}[0m"*) : ;; + *) die "the initial-colors sample was not colored: ${COLORED_OUTPUT}" ;; +esac + +# in a substitution, so `nvm_set_colors`' `export NVM_COLORS` stays contained +oneshot_reset +SET_COLORS_OUTPUT="$(nvm_set_colors 'bygre')" +case "${SET_COLORS_OUTPUT}" in + *"${ESC}"*) : ;; + *) die "nvm_set_colors did not color its confirmation line: ${SET_COLORS_OUTPUT}" ;; +esac + +# coloring must change only the escapes, never the text or the alignment +COLORED_STRIPPED="$(nvm_echo "${COLORED_OUTPUT}" | strip_colors)" +[ "${COLORED_STRIPPED}" = "${PLAIN}" ] \ + || die 'the colored legend does not match the plain legend once escapes are stripped' + +# called directly rather than in a substitution, so a leak is observable at all +oneshot_reset +nvm_print_color_legend >/dev/null +[ -z "${NVM_HAS_COLORS-}" ] \ + || die "nvm_print_color_legend leaked NVM_HAS_COLORS=${NVM_HAS_COLORS}" + # And `nvm unload` has to clean it up, the way it does its sibling helpers. # `set +e` inside the subshell so a non-zero `nvm unload` cannot abort it before # the lookup runs; its status is reported separately, so "unload failed" can @@ -97,4 +188,10 @@ case "${UNLOAD_OUT}" in *) die "nvm unload did not remove nvm_print_color_legend (${UNLOAD_OUT})" ;; esac +if [ -n "${NVM_HAS_COLORS_WAS}" ]; then + NVM_HAS_COLORS="${NVM_HAS_COLORS_WAS}" + export NVM_HAS_COLORS +fi + +cleanup echo "nvm_print_color_legend: passed"