Skip to content

Conversation

@aclemons
Copy link
Contributor

@aclemons aclemons commented Apr 8, 2025

When checking the version in the resolved .nvmrc file found, the
change made in d2163a6 tried to read the file a second time, but did not
use the resolved $nvm_path but rather tried to simply read it from the
current directory. This fails if the file was resolved to a parent
directory.

This change updates the code to reuse the version value which was read
from the file two lines prior.

@akinomyoga
Copy link
Contributor

akinomyoga commented Apr 9, 2025

This file is just a copy of the upstream. I checked the upstream change history again. There seem to have been more changes than I checked previously. I guess I missed the changes in git blame because of the timestamp of nvm-sh/nvm#1569 (which was submitted to nvm-sh/nvm on 2017-06-30 but merged on 2023-12-05).

@akinomyoga
Copy link
Contributor

I'll check this PR again later.

@akinomyoga
Copy link
Contributor

I doubt tr ... is needed in the first place. The implementation of find-up should print a valid path, so there is no need to remove anything from the output. Rather, one shouldn't change anything in the path. The original intent of tr could possibly be to remove the newline added by the echo builtin, but in that case, that extra newline is supposed to be removed by the command substitution $(...), so one doesn't need to remove the ending newline anyway.

I'm not sure if nvm_find_up is really ensured to exist in the user's environment, so I won't pick this up for now.

I added changes in 4098fed.

aclemons and others added 4 commits April 9, 2025 21:24
When checking the version in the resolved `.nvmrc` file found, the
change made in d2163a6 tried to read the file a second time, but did not
use the resolved `$nvm_path` but rather tried to simply read it from the
current directory. This fails if the file was resolved to a parent
directory.

This change updates the code to reuse the version value which was read
from the file two lines prior.
@akinomyoga akinomyoga merged commit 1f5112a into ohmybash:master Apr 19, 2025
4 checks passed
@akinomyoga
Copy link
Contributor

Thanks!

@aclemons aclemons deleted the plugins-nvm-fix branch April 22, 2025 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants