-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
[Fix] specify 'origin' remote name with git clone ...
installs
#3341
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
Conversation
Thanks, this is a great catch! Unfortunately, i believe git didn't add this option until v2.29, and we currently only require git v1.7.10. Would there be an alternative approach where instead of hardcoding "origin", we derive the default remote name, and just use that throughout the install script? |
Hmm. To my knowledge, there is no real default remote after the repo is created. I don't believe A couple of ways to deal with the issue that come to mind are just using the Then using Alternatively, the script could I think the first option is likely the better choice. It should work for any version, be localized to a single code location, and should always result in the expected 'origin' setup. |
96cdb38
to
5d2a10d
Compare
I pushed a modified version. |
git docs say |
Yep, but that's only really good at the time of initial cloning. |
ah, so you're saying that that would work for the initial clone, but wouldn't help with future git operations? wouldn't we be able to assume that the default, if configured, is unlikely to change? |
I added references and corrected the mentioned versions to my second post. And, in looking at them again, it looks like I could just simply change it to use |
If some user changed it, I think it's actually more likely to change in the future. And the change will be external and unknown as a change to the repo. I think that setting it up with the expected 'origin' name from the start is going to be much more robust. Much less likely that a user would go into the repo and rename it than rethinking the config choice. Personally, I had 'main' for a while, then went with 'local' on my home PC because I have a lot of customized soft-forks. But, I'm likely a 0.1%-er here (or even 0.001%-er 😄). |
`git` may be configured locally to use a non-'origin' default remote name. So, specify 'origin' as the remote name when cloning to get the expected setup.
alrighty, let's go with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can there be any install tests that cover this?
# Cloning repo | ||
command git clone "$(nvm_source)" --depth=1 "${INSTALL_DIR}" || { | ||
command git clone "$(nvm_source)" --depth=1 -o origin "${INSTALL_DIR}" 2> /dev/null \ | ||
|| command git clone "$(nvm_source)" --depth=1 "${INSTALL_DIR}" || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|| command git clone "$(nvm_source)" --depth=1 "${INSTALL_DIR}" || { | |
|| command git clone "$(nvm_source)" --depth=1 -o origin "${INSTALL_DIR}" || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll just shrink it to the single command/line since at least -o origin
looks like it'll work with the version restriction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is that the first one suppresses stderr; it's intentional to have the command twice so that when it fails, it tries again and prints out stderr for debugging purposes.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Sure. I'll take a look at your tests and try to work up a something that fits. I think I should be able to add something to install_nvm_from_git. Are you testing with the minimum git version? I don't see that anywhere. |
No, git version is just an implicit requirement. |
c6cfc3a
to
c20db2a
Compare
@rivy can you please check the "allow edits" checkbox? also, are you interested in completing this PR? |
This is not correct. So I don't see any Git version issues here. So what is this patch waiting on? (I just ran into this, created a PR to fix it, and immediately found out that the bug has been known for more than a year.) |
That's great to hear. Since it seems this PR was made from a fork that's not on the author's username ( |
The script assumes that the name of the remote is `origin`, but this is not the case if the user has set `clone.defaultRemoteName` to another value in the ~/.gitconfig (or elsewhere in the configuration). Adding `-o origin` ensures that the remote will be called `origin` regardless of the `clone.defaultRemoteName` setting. Per PR nvm-sh#3341: - The minimum Git version this should work with is v1.7.10. (This is not documented in the repo itself; it's just an implicit requirement.) - The `--origin` option was added to `git clone` in commit 98a4fef3f2 which was released in v1.2.5. From the diff of that commit, the `-o` option was already available at that time. So this easily satisfies the above. - A comment in nvm-sh#3341 indicates that `-o` was added in v1.1.0. I've not verified this, but we probably don't need to track that down since by the above we're already well within requirements.
Since
git
may be configured locally to use a non-'origin' default remote name, specify 'origin' as the remote name when cloning to get the expected local repo setup.