[Fix] add missing "command" prefix call for the commands #1296

PeterDaveHello · 2016-11-11T11:38:34Z

No description provided.

ljharb · 2016-11-12T19:53:41Z

A number of these are intentional as it caused problems in some shells. I'd prefer to wait until there are specific bug reports, as aliasing over builtin commands is a very bad practice, and as such, isn't super common.

PeterDaveHello · 2016-11-12T19:56:08Z

@ljharb how do we determinate if it's intentional or not? I believe at least many of these should be fixed as there are not only one usage but both with and without command prefix for those commands.

ljharb · 2016-11-12T19:57:29Z

Fair question.

Certainly if I've prefixed a foo with command somewhere, it should be done everywhere. I just don't want to do it blindly, without extensive manual testing on all supported shells first.

PeterDaveHello · 2016-11-12T20:04:21Z

Any idea about how we could do on this PR? Just prefix the commands have been prefixed before and don't touch the others until bug report? Actually, I only prefix the non-builtin commands, is this good enough?

ljharb

I've commented on the things that should be reverted; the rest look great and are likely just omissions on my part.

ljharb · 2016-11-12T20:19:26Z

install.sh

@@ -49,7 +49,7 @@ nvm_node_version() {

 nvm_download() {
  if nvm_has "curl"; then
-    curl -q "$@"
+    command curl -q "$@"


this one i think should be reverted; sometimes people alias curl intentionally

ljharb · 2016-11-12T20:20:10Z

install.sh

@@ -231,7 +231,7 @@ nvm_check_global_modules() {

  local NPM_GLOBAL_MODULES
  NPM_GLOBAL_MODULES="$(
-    npm list -g --depth=0 |
+    command npm list -g --depth=0 |


let's revert this since it hasn't been a problem before

ljharb · 2016-11-12T20:20:33Z

install.sh

@@ -240,7 +240,7 @@ nvm_check_global_modules() {
  MODULE_COUNT="$(
    command printf %s\\n "$NPM_GLOBAL_MODULES" |
    command sed -ne '1!p' |                     # Remove the first line
-    wc -l | tr -d ' '                           # Count entries
+    command wc -l | tr -d ' '                           # Count entries


let's revert this since it hasn't been a problem before (note that tr isn't prefixed either)

ljharb · 2016-11-12T20:20:50Z

nvm-exec

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash

-DIR="$(command cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+DIR="$(command cd "$( command dirname "${BASH_SOURCE[0]}" )" && pwd )"


let's revert this since it hasn't been a problem before

ljharb · 2016-11-12T20:21:03Z

nvm.sh

@@ -603,7 +603,7 @@ nvm_make_alias() {
    nvm_err "an alias target version is required"
    return 2
  fi
-  nvm_echo "${VERSION}" | tee "$(nvm_alias_path)/${ALIAS}" >/dev/null
+  nvm_echo "${VERSION}" | command tee "$(nvm_alias_path)/${ALIAS}" >/dev/null


let's revert this since it hasn't been a problem before

ljharb · 2016-11-12T20:23:55Z

nvm.sh

@@ -1364,7 +1364,7 @@ nvm_print_implicit_alias() {
      NVM_IOJS_VERSION="$($NVM_COMMAND)" &&:
      EXIT_CODE="$?"
      if [ "_$EXIT_CODE" = "_0" ]; then
-        NVM_IOJS_VERSION="$(nvm_echo "$NVM_IOJS_VERSION" | command sed "s/^$NVM_IMPLICIT-//" | nvm_grep -e '^v' | command cut -c2- | command cut -d . -f 1,2 | uniq | command tail -1)"
+        NVM_IOJS_VERSION="$(nvm_echo "$NVM_IOJS_VERSION" | command sed "s/^$NVM_IMPLICIT-//" | nvm_grep -e '^v' | command cut -c2- | command cut -d . -f 1,2 | command uniq | command tail -1)"


let's revert this since it hasn't been a problem before

ljharb · 2016-11-12T20:24:00Z

nvm.sh

@@ -1394,7 +1394,7 @@ nvm_print_implicit_alias() {
        setopt shwordsplit
      fi

-      LAST_TWO=$($NVM_COMMAND | nvm_grep -e '^v' | command cut -c2- | command cut -d . -f 1,2 | uniq)
+      LAST_TWO=$($NVM_COMMAND | nvm_grep -e '^v' | command cut -c2- | command cut -d . -f 1,2 | command uniq)


let's revert this since it hasn't been a problem before

ljharb · 2016-11-12T20:24:08Z

nvm.sh

@@ -1775,9 +1775,9 @@ nvm_get_make_jobs() {
  elif [ "_$NVM_OS" = "_freebsd" ] || [ "_$NVM_OS" = "_darwin" ]; then
    NVM_CPU_THREADS="$(sysctl -n hw.ncpu)"
  elif [ "_$NVM_OS" = "_sunos" ]; then
-    NVM_CPU_THREADS="$(psrinfo | wc -l)"
+    NVM_CPU_THREADS="$(psrinfo | command wc -l)"


let's revert this since it hasn't been a problem before

ljharb · 2016-11-12T20:24:27Z

nvm.sh

@@ -1886,10 +1886,10 @@ nvm_install_source() {
        if nvm_version_greater 0.2.3 "$VERSION"; then
          nvm_err 'npm requires node v0.2.3 or higher'
        else
-          nvm_download -L https://npmjs.org/install.sh -o - | clean=yes npm_install=0.2.19 sh
+          nvm_download -L https://npmjs.org/install.sh -o - | clean=yes npm_install=0.2.19 command sh


let's revert this since it hasn't been a problem before

ljharb · 2016-11-12T20:24:30Z

nvm.sh

        fi
      else
-        nvm_download -L https://npmjs.org/install.sh -o - | clean=yes sh
+        nvm_download -L https://npmjs.org/install.sh -o - | clean=yes command sh


let's revert this since it hasn't been a problem before

PeterDaveHello · 2016-11-12T20:37:16Z

@ljharb sorry but I just didn't understand, do we really need to wait for the problem occurs even we can prevent it? We never know if there would be a guy that write his/her own rev function or alias wc with --max-line-length/--words by his/her own favor, command is how we prevent the problem, so why don't we prepare for the familiar cases may got problem in future, even nobody complains about the problem yet? I didn't see any possibility of the bad side of using command prefix in our cases yet, and as we have that many test cases, these changes still passed the tests without any problem, I wonder if we could just apply the command prefix for the non-builtin commands, IMO, it's not blindly at all(just my thought).

ljharb · 2016-11-12T20:49:14Z

It's a reasonable thing to do - and if command always worked, I'd have done that before. However, in zsh specifically, some things work unpredictably when prefixed with command (see the line in nvm_echo about command printf) and I've been burned too many times to trust it.

PeterDaveHello · 2016-11-12T20:54:05Z

Updated!

PeterDaveHello force-pushed the fix-command-prefix branch 2 times, most recently from 6f9dea9 to 3c61756 Compare November 11, 2016 11:42

ljharb requested changes Nov 12, 2016

View reviewed changes

[Fix] add missing "command" prefix call for some commands

eab41ed

PeterDaveHello force-pushed the fix-command-prefix branch from 3c61756 to eab41ed Compare November 12, 2016 20:53

ljharb approved these changes Nov 12, 2016

View reviewed changes

ljharb added the shell alias clobbering Anything dealing with users shadowing builtins with aliases or functions. label Nov 12, 2016

ljharb merged commit eab41ed into nvm-sh:master Nov 13, 2016

PeterDaveHello deleted the fix-command-prefix branch November 13, 2016 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] add missing "command" prefix call for the commands #1296

[Fix] add missing "command" prefix call for the commands #1296

PeterDaveHello commented Nov 11, 2016

ljharb commented Nov 12, 2016

PeterDaveHello commented Nov 12, 2016

ljharb commented Nov 12, 2016

PeterDaveHello commented Nov 12, 2016

ljharb left a comment

ljharb Nov 12, 2016

ljharb Nov 12, 2016

ljharb Nov 12, 2016

ljharb Nov 12, 2016

ljharb Nov 12, 2016

ljharb Nov 12, 2016

ljharb Nov 12, 2016

ljharb Nov 12, 2016

ljharb Nov 12, 2016

ljharb Nov 12, 2016

PeterDaveHello commented Nov 12, 2016

ljharb commented Nov 12, 2016

PeterDaveHello commented Nov 12, 2016

[Fix] add missing "command" prefix call for the commands #1296

[Fix] add missing "command" prefix call for the commands #1296

Conversation

PeterDaveHello commented Nov 11, 2016

ljharb commented Nov 12, 2016

PeterDaveHello commented Nov 12, 2016

ljharb commented Nov 12, 2016

PeterDaveHello commented Nov 12, 2016

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterDaveHello commented Nov 12, 2016

ljharb commented Nov 12, 2016

PeterDaveHello commented Nov 12, 2016