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

return to the previous directory after fetching/updating nvm #1144

Merged
merged 1 commit into from
Jul 5, 2016

Conversation

0xmohit
Copy link
Contributor

@0xmohit 0xmohit commented Jul 5, 2016

Fixes #1136

@@ -69,7 +69,7 @@ install_nvm_from_git() {
if [ -d "$INSTALL_DIR/.git" ]; then
echo "=> nvm is already installed in $INSTALL_DIR, trying to update using git"
printf "\r=> "
cd "$INSTALL_DIR" && (command git fetch 2> /dev/null || {
pushd "$INSTALL_DIR" && (command git fetch 2> /dev/null || {
echo >&2 "Failed to update nvm, run 'git fetch' in $INSTALL_DIR yourself." && exit 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this condition triggers, won't the directory still be changed?

It seems like if we're going to popd when things work, we should do it in all the failure cases too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm beginning to think that we could get rid of cd and friends altogether. Instead use the --git-dir and --work-tree options for the git commands.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like that better, provided that those commands were supported in an old enough version of git that it won't suddenly raise the threshold for required git support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git-1.8.2.3.tar.bz2 (09-May-2013) as available on https://www.kernel.org/pub/software/scm/git/ supports those options. Does that appear old enough or do we want to dig further?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appears that the options existed much prior to that. See https://git-scm.com/blog/2010/04/11/environment.html

@ljharb
Copy link
Member

ljharb commented Jul 5, 2016

This LGTM, pending a rebase down to 1 commit, assuming that we can't get away with omitting the "git dir" arg entirely

@ljharb ljharb added the installing node Issues with installing node/io.js versions. label Jul 5, 2016
@ljharb ljharb merged commit 322d81d into nvm-sh:master Jul 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installing node Issues with installing node/io.js versions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants