[Fix] Add missing "command" prefixes for few commands #1736
Conversation
PeterDaveHello
force-pushed
the
fix-command-prefixes
branch
from
2ee381a
to
42b3c93
Compare
nvm.sh
Outdated
| @@ -104,7 +104,7 @@ nvm_download() { | |||
| if nvm_curl_use_compression; then | |||
| CURL_COMPRESSED_FLAG="--compressed" | |||
| fi | |||
| curl --fail ${CURL_COMPRESSED_FLAG:-} -q "$@" | |||
| command curl --fail ${CURL_COMPRESSED_FLAG:-} -q "$@" | |||
There was a problem hiding this comment.
i feel like i left this one as-is for some reason; can you try git blame-ing this line?
(i think there's probably people that alias or shadow curl, and this would break them)
There was a problem hiding this comment.
I traced it, at least can trace back to #452, didn't see that reason yet, what do you think?
There was a problem hiding this comment.
Looks like it'll break our tests, as they are shadow curl in fact ... maybe we should use a stronger mock method like to temporarily override the curl binary or something like that? It'll increase the complexity, but I'm not sure if it'll be good to not use the command prefix just because we want simpler tests.
There was a problem hiding this comment.
Let's skip the prefix on curl for now; we can address that in a future PR.
There was a problem hiding this comment.
Actually, we have command prefix in nvm debug, as it's not tested, maybe we can still keep the one on the curl not tested?
There was a problem hiding this comment.
Since some tests require curl be un-prefixed, i think all curls should be unprefixed, including in nvm debug.
There was a problem hiding this comment.
Okay, I think we should separate that change another place, right?
There was a problem hiding this comment.
Reverted that prefix added here for curl.
PeterDaveHello
force-pushed
the
fix-command-prefixes
branch
2 times, most recently
from
43722f1
to
94d2920
Compare
|
Hope we're good to go now ：） |
ljharb
added
the
shell alias clobbering
ljharb
force-pushed
the
fix-command-prefixes
branch
from
94d2920
to
f3076d1
Compare
Just take a deeper look to find out those commands suppose to have
commandprefix but was missing, so fix now.