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

[Fix] specify 'origin' remote name with git clone ... installs #3341

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rivy
Copy link

@rivy rivy commented May 1, 2024

I ran into this as an installation failure when installing nvm on a PC using some git configuration customizations.

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.

@ljharb
Copy link
Member

ljharb commented May 1, 2024

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?

@rivy
Copy link
Author

rivy commented May 3, 2024

Hmm.

To my knowledge, there is no real default remote after the repo is created. I don't believe git holds any special regard for 'origin', or any other remote, as a default (other than then as the default initial remote name). There's no configuration info that I've seen that makes any differentiation. If there is more than one remote, git remote just displays all of them in alpha order.

A couple of ways to deal with the issue that come to mind are just using the git ... command as amended in the PR followed by the original version as an alternate. From looking at the docs, I believe that prior to v1.1.0 with the '-o' option [ref: git/git@e6c310f], there was no way to clone a repo to anything other than the 'origin' remote. It looks like the configuration variable 'clone.defaultRemoteName' was added in v2.30.0 [ref: git/git@de9ed3e]. So, if command git clone "$(nvm_source)" --depth=1 --origin=origin "${INSTALL_DIR}" fails then command git clone "$(nvm_source)" --depth=1 "${INSTALL_DIR}" should always install the remote as 'origin'.

Then using command git clone "$(nvm_source)" --depth=1 --origin=origin "${INSTALL_DIR}" || command git clone "$(nvm_source)" --depth=1 "${INSTALL_DIR}" || { ... } should always set up the repo to have the remote 'origin' pointing at the expected URL.

Alternatively, the script could cd "${INSTALL_DIR" ; git remote rename $(git remote) origin immediately after the clone (without a --origin=origin flag). That should also work for any version and set up the remote 'origin' correctly. (git doesn't throw an error if you rename a remote to the same name.)

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.

@rivy rivy force-pushed the fix.install-by-clone branch 2 times, most recently from 96cdb38 to 5d2a10d Compare May 3, 2024 04:57
@rivy
Copy link
Author

rivy commented May 3, 2024

I pushed a modified version.

@ljharb
Copy link
Member

ljharb commented May 3, 2024

git docs say Overrides clone.defaultRemoteName from the config. which suggests that git config clone.defaultRemoteName || echo origin would be the value to use?

@rivy
Copy link
Author

rivy commented May 3, 2024

git docs say Overrides clone.defaultRemoteName from the config. which suggests that git config clone.defaultRemoteName || echo origin would be the value to use?

Yep, but that's only really good at the time of initial cloning.

@ljharb
Copy link
Member

ljharb commented May 3, 2024

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?

@rivy
Copy link
Author

rivy commented May 3, 2024

I added references and corrected the mentioned versions to my second post. And, in looking at them again, it looks like -o was added in v1.1.0 [ref: git/git@e6c310f], though I don't see the where --origin was added.

I could just simply change it to use -o origin and that would, I believe, fit the version restriction.

@rivy
Copy link
Author

rivy commented May 3, 2024

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?

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.
@rivy rivy force-pushed the fix.install-by-clone branch from 5d2a10d to cae2eb7 Compare May 3, 2024 05:38
@ljharb
Copy link
Member

ljharb commented May 3, 2024

alrighty, let's go with -o origin, and just hardcode it, then.

Copy link
Member

@ljharb ljharb left a 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?

@@ -163,7 +163,8 @@ install_nvm_from_git() {
}
else
# 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}" || {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|| command git clone "$(nvm_source)" --depth=1 "${INSTALL_DIR}" || {
|| command git clone "$(nvm_source)" --depth=1 -o origin "${INSTALL_DIR}" || {

Copy link
Author

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.

Copy link
Member

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.

@rivy
Copy link
Author

rivy commented May 3, 2024

Can there be any install tests that cover this?

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.

@ljharb
Copy link
Member

ljharb commented May 3, 2024

No, git version is just an implicit requirement.

@ljharb ljharb force-pushed the master branch 2 times, most recently from c6cfc3a to c20db2a Compare June 10, 2024 18:13
@ljharb ljharb added the pull request wanted This is a great way to contribute! Help us out :-D label Jul 26, 2024
@ljharb ljharb marked this pull request as draft July 26, 2024 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull request wanted This is a great way to contribute! Help us out :-D
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants