[Fix] install.sh: Force remote name of cloned repo to be 'origin'

[0cjs]

See the commit message(s) for details. (I avoid duplicating them here because the commit messages are being updated as we review.)

Feel free to change the commit message (or anything else) as necessary. The [Fix] prefix is not mentioned in the "rest of the conventions" for commit messages, but seems to be what is used for similar things in previous commits.

This has been tested manually on my personal machine, where I use clone.defaultRemoteName. The automated tests against the current head of master (make TEST_SUITE=fast test-bash) had some issues for me, including what looked like spurious failures; I assume that this is just my configuration.

The CI tests below also appear to have unrelated issues, giving e.g. sed: /etc/apt/sources.list: No such file or directory on WSL.

[ljharb]

This is a duplicate of #3341, and can't land as-is for the same reason.

[ljharb]

It'd be great to add tests, perhaps inside https://github.com/nvm-sh/nvm/blob/2b5e53fcd57409e018b468fd05eebad06fbff3fa/test/install_script/install_nvm_from_git

[0cjs]

It'd be great to add tests....

It would. I've just updated the commit message to bring in the version information from #3341 so that doesn't get lost in the depths of GitHub tickets. I'll should be able to look at the testing situation later today, or over the weekend at the latest. Depending on the difficulty, I may suggest it's better just to get the change in without waiting for proper automated tests if those will be difficult, since this change on the face of it can't break anything that's not already broken unless someone's using a truly ancient version of Git, but it's worth delaying slightly until we actually know what's involved with such a test.

And there's also the issue of all the failing PR checks at least some of which seem clearly unrelated to this change; I have no idea what to do about that. But I think we at least want some idea of why they're happening before we e.g. decide to ignore them or whatever.

[ljharb]

The WSL changes are known; and the v0.40.0 failures are expected, so indeed it’s nothing you need to worry about.

[0cjs]

Ok, I've started looking at adding a test for this, and I've run into a few issues. Some of these are no doubt due to me being unfamiliar with the code, but I'm documenting them here for posterity in case you want to consider mitigating any of them.

1. I noticed you make a Docker image available for development, so I tried to use that so I could avoid any local configuration issues. Unfortunately it didn't build. I fixed this, fixed a couple of minor errors in the docs, and did a bit of documentation cleanup, and filed this as PR Docker Fixes #3657. All further notes below are from running things inside that container.

2. From README §Running Tests, npm install gives a vulnerabilities warning: "17 vulnerabilities (9 moderate, 7 high, 1 critical)". (I just ignored this.)

3. From the same section, npm run test/fast does not work:

    nvm@nvm-dev:~/.nvm$ \
        npm run test/fast
   
    > [email protected] test/fast
    > shell=$(basename -- $(ps -o comm= $(ps -o ppid= -p $PPID)) | sed 's/^-//'); env -i TERM="$TERM" bash -lc "make TEST_SUITE=fast test-$shell"
   
    /usr/bin/env: 'node': No such file or directory
    Makefile:15: *** Did you forget to run `npm install` after cloning the repo? At least one of the required supporting utilities not found: urchin replace semver.  Stop.
    nvm@nvm-dev:~/.nvm$
   

   Node does seem to be available; node --version and /usr/bin/env node --version both give me "v24.8.0".

4. Moving on to just trying to run the install_nvm_from_git test script directly, running it from the root of the repo doesn't work:

    nvm@nvm-dev:~/.nvm$ \
        ./test/install_script/install_nvm_from_git
    ./test/install_script/install_nvm_from_git: 16: .: cannot open ../../install.sh: No such file
   

5. Running install_nvm_from_git from its own directory doesn't work:

    nvm@nvm-dev:~/.nvm/test/install_script$ \
        ./install_nvm_from_git
    Error: the install instructions explicitly say to pipe the install script to `bash`; please follow them
   

6. Oddly, running bash on that script does seem to work:

    nvm@nvm-dev:~/.nvm/test/install_script$ \
        bash ./install_nvm_from_git >../../z.log 2>&1 ; echo exitcode=$?
    exitcode=0
    # (Completes after about 45 seconds.)
    nvm@nvm-dev:~/.nvm/test/install_script$ \
        wc -l ../../z.log
    1717 ../../z.log
   

   So after about 45 seconds it completes without failure, and with a couple thousand lines of output, most of which looks like traces from the shell -x option.

   Note that running bash test/install_script/install_nvm_from_git from the root directory doesn't work; it also fails with ../../install.sh: No such file or directory.

So am I correct that this script is intended to be run as follows?

cd test/install_script/
bash install_nvm_from_git >../../z.log 2>&1; echo exitcode=$?

I'll look through the script and give you further notes in the next comment.

[0cjs]

Here are a few thoughts on the bash test/install_script/install_nvm_from_git script. (Note that I'm still not clear if this is supposed to be run stand-alone.) I'm running it with the following, so I can confirm if it passed or not and keep a copy of the logs. (I would like to use tee here, but I believe that will consume the error code.)

cd test/install_script/
bash install_nvm_from_git >../../z.log 2>&1; echo ━━━━━ EXITCODE=$?; tail ../../z.log

My notes:

1. Don't depend on the current working directory; use e.g. PROJDIR=$(cd $(dirname "$0"/../..); pwd -P to get the project directory and reference files relative to that.

2. The issue with it not working when run directly seems to be that the shebang is #!/bin/sh making it use Bourne shell (dash on Debian/Ubuntu); the same error arises if I run it with sh install_nvm_from_git. It's not clear to me whether the script is actually intended to be run with sh and is broken, or if the script is intended to be run with bash (and perhaps is, elsewhere) and an incorrect shebang just hasn't been noticed since some switch. (The #/bin/sh shebang has been there unchanged since the file was introduced in 2021-01-07 commit 502089a. There are no comments about anything in this script in that commit; it has just a summary saying "[New] install script: Allow installation from other repository also for git method". )

3. I'm not clear on the purpose of cleanup(), SAVE_NVM_DIR and the like. This is merely restoring environment variables and removing shell functions that will all disappear when the process exits, anyway. Or is this file, as well as being run directly also sourceed by something, or expected sometimes to be sourced by users?

4. To test with a Git configuration that has clone.defaultRemoteName, we can probably create a file containing that setting and export GIT_CONFIG_GLOBAL pointing to it before running it. There's the question of where to put that, though.

5. Another option is to start testing with new values of HOME, which would:

   • Give us a cleaner configuration against which we're testing, reducing the chance that user configuration could break a test.
   • Allow us to put a .gitconfig in that dir, obviating the need to use GIT_CONFIG_GLOBAL.
   • Let us test that nvm sets proper defaults when NVM_DIR is not set.

6. It would be useful to have some documentation in comments at the top of this file, or at least a pointer to the documentation for it. (But I don't think that there is any; I didn't find any.) Keeping the documentation in this file, with a pointer to it in the testing section of the README would be my preference, since documentation is more likely to be updated to match code changes when it's near the code.

Overall there's a fair amount of work to be done here, and it probably won't be quick work even for those who know what they're doing given that the script takes 45 seconds for every run. I don't feel particularly comfortable hacking on this by myself since even after all I've learned in the last few hours of work I still keep discovering things I don't know.

My suggestion is that we simply merge this fix, and if we want to improve the testing of this, make it a separate project. I've got tons of experience doing testing like this and would be happy to remote pair-program with someone (using voice chat and tmate) if someone who knows more about the overall testing system wants to put in the time and effort.

[0cjs]

So what are we currently waiting on in order to get this fix merged?