Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install script should use \. in case of overridden . #1278

Closed
ranqiangjun opened this issue Nov 1, 2016 · 9 comments
Closed

Install script should use \. in case of overridden . #1278

ranqiangjun opened this issue Nov 1, 2016 · 9 comments
Labels
installing nvm Problems installing nvm itself pull request wanted This is a great way to contribute! Help us out :-D shell alias clobbering Anything dealing with users shadowing builtins with aliases or functions.

Comments

@ranqiangjun
Copy link
Contributor

Since #576 was closed. I create a new one here.

I got the same error after installed nvm followed the README on macOS Sierra 10.12.1.

-bash: nvm: command not found

According to @ljharb 's comment: #576 (comment)

. something and source something are the same, except that the . is more portable, so you should definitely remove that redundant source line.

It is NOT TRUE that . something and source something are the same.

In my MBP, . is an alias of cd .. defined in the .bash_profile file. I am not sure it was provided by default or not. But after I replaced . with source explicitly, the nvm works.

Before:

export NVM_DIR="$HOME/.nvm"
[ -s "$NVM_DIR/nvm.sh" ] && .  "$NVM_DIR/nvm.sh" # This loads nvm

After:

export NVM_DIR="$HOME/.nvm"
[ -s "$NVM_DIR/nvm.sh" ] && source "$NVM_DIR/nvm.sh" # This loads nvm

I suggest that whether the . alias was provided by default or not, we should modify our README and INSTALL SCRIPT to use source explicitly, not the ..

@ljharb
Copy link
Member

ljharb commented Nov 1, 2016

@ranqiangjun wow, i've never heard of anyone aliasing over . before - that's hugely problematic and might break a lot of tools. It's definitely not a default.

source does not work on all of the shells that nvm supports. . is the proper thing to use.

@ljharb ljharb added the shell alias clobbering Anything dealing with users shadowing builtins with aliases or functions. label Nov 1, 2016
@ljharb
Copy link
Member

ljharb commented Nov 1, 2016

It might be possible to use \. to source it while bypassing the alias. Does this work for you?

@ranqiangjun
Copy link
Contributor Author

\. Works for me. Thanks for your quick response!

@ljharb
Copy link
Member

ljharb commented Nov 1, 2016

Great, if that change also works on other shells, I'll add that to the install script.

That said, you absolutely should remove that alias immediately - . is a very important shell builtin.

@ljharb ljharb added the installing nvm Problems installing nvm itself label Nov 1, 2016
@ranqiangjun
Copy link
Contributor Author

I run this kind of script before https://github.com/darol100/lazydubuntu/blob/master/bin/lazyaliases.sh. It added aliases for me automatically. I'll remove that. Thanks again. @ljharb

@ljharb
Copy link
Member

ljharb commented Nov 1, 2016

Nothing in that script aliases over one dot, only 2, 3, and 4 dots.

@ranqiangjun
Copy link
Contributor Author

Yes, that script is fine. It is only an example. I just don't know which script added that.

In my .bash_profile

I found:

## a quick way to get out of current directory ##
alias .='cd ..'
alias ..='cd ../..'
alias ...='cd ../../../'
alias ....='cd ../../../../'
alias .....='cd ../../../.././'

It is for sure that this is not added by me manually. Because English is not my first language, that comment ## a quick way to get out of current directory ## is perfect and I can't not write like that.

@ljharb
Copy link
Member

ljharb commented Nov 1, 2016

I'd definitely recommend removing all of those aliases and just using cd when you want to change directories :-p

Thanks for the report!

@ranqiangjun
Copy link
Contributor Author

All removed. I appreciate your time.

@ljharb ljharb changed the title -bash: nvm: command not found Install script should use \. in case of overridden . Nov 1, 2016
@ljharb ljharb added the pull request wanted This is a great way to contribute! Help us out :-D label Nov 1, 2016
ranqiangjun added a commit to ranqiangjun/nvm that referenced this issue Nov 1, 2016
@ljharb ljharb closed this as completed in 2a2b8bd Nov 4, 2016
edwmurph pushed a commit to edwmurph/nvm that referenced this issue Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installing nvm Problems installing nvm itself pull request wanted This is a great way to contribute! Help us out :-D shell alias clobbering Anything dealing with users shadowing builtins with aliases or functions.
Projects
None yet
Development

No branches or pull requests

2 participants