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

Don't give detached HEAD advice when installing #1704

Merged
merged 3 commits into from
Jan 13, 2018
Merged

Don't give detached HEAD advice when installing #1704

merged 3 commits into from
Jan 13, 2018

Conversation

pnorman
Copy link
Contributor

@pnorman pnorman commented Jan 10, 2018

I didn't see relevant tests to if git produces output or not, so haven't added any. I did test reinstalling with this commit, and it does do as it describes, removing the detached head warning.

I didn't add --quiet to the git clone, but it's worth considering, or if it should be kept for a download progress indicator.

@@ -110,13 +110,13 @@ install_nvm_from_git() {
exit 2
}
else
command git clone "$(nvm_source)" -b "$(nvm_latest_version)" --depth=1 "${INSTALL_DIR}" || {
command git -c advice.detachedHead=false clone "$(nvm_source)" -b "$(nvm_latest_version)" --depth=1 "${INSTALL_DIR}" || {
Copy link
Member

Choose a reason for hiding this comment

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

what happens in git version v1.7?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both the current git command and the one in this PR fail for other reasons. -c advice.detachedHead=false cause no additional failures.

The failure is

warning: Remote branch v4.6.0 not found in upstream origin, using HEAD instead

In some version between current and 1.7 the ability to use a commitish instead of only a branch for the -b option was added, with the man page text changing from

       --branch <name>, -b <name>
           Instead of pointing the newly created HEAD to the branch pointed to by the cloned repository’s HEAD, point to <name> branch instead.
           In a non-bare repository, this is the branch that will be checked out.

to

       --branch <name>, -b <name>
           Instead of pointing the newly created HEAD to the branch pointed to by the cloned repository’s HEAD, point to <name> branch instead.
           In a non-bare repository, this is the branch that will be checked out.  --branch can also take tags and detaches the HEAD at that
           commit in the resulting repository.

This was tested with git version 1.7.9.5, provided by 1:1.7.9.5-1ubuntu0.2

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, interesting - that suggests that we need to ratchet git support up. Do you know when that ability was added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometime before 1.9, which is what Ubuntu 14.04 has. I'm not sure it's a serious issue, a setup that old is likely to have other backwards compatibility problems. If it is a concern, I guess another issue could be opened. I don't want to tackle that here, because this is just a simple backwards compatible change.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like v1.17.10 - git/git@5a7d5b6 - so pretty close.

I'll update your branch so the readme reflects that, and then this can go in. Thanks!

@ljharb ljharb added the installing nvm Problems installing nvm itself label Jan 13, 2018
@ljharb ljharb merged commit d34bb32 into nvm-sh:master Jan 13, 2018
edwmurph pushed a commit to edwmurph/nvm that referenced this pull request 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants