Skip to content

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

Merged
ljharb merged 1 commit into
nvm-sh:masterfrom
0xmohit:master
Jul 5, 2016
Merged

return to the previous directory after fetching/updating nvm#1144
ljharb merged 1 commit into
nvm-sh:masterfrom
0xmohit:master

Conversation

@0xmohit

@0xmohit 0xmohit commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

Fixes #1136

Comment thread install.sh
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

ljharb commented Jul 5, 2016

Copy link
Copy Markdown
Member

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