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.

Sign up for GitHub

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

Jump to bottom

Fix for $path used by zsh #1669

Merged
merged 3 commits into from
Nov 22, 2017
Merged

Fix for $path used by zsh #1669

merged 3 commits into from
Nov 22, 2017

Conversation

pjw91
Copy link
Contributor

@pjw91 pjw91 commented Nov 20, 2017 •
edited
Loading

The $path is used by zsh itself.

Rename path to path_.

Ref:
"The lower-case version of PATH is an array parameter
bound to the scalar upper-case parameter."
-- http://www.zsh.org/mla/users/2015/msg00178.html

@pjw91
Fix for $path used by zsh
91a29c0 
"The lower-case version of PATH is an array parameter
bound to the scalar upper-case parameter."
-- http://www.zsh.org/mla/users/2015/msg00178.html
@ljharb
Copy link
Member

ljharb commented Nov 21, 2017

Thanks; can you add a test that would have failed with this change?

@ljharb ljharb added bugs Oh no, something's broken :-( shell: zsh needs followup We need some info or action from whoever filed this issue/PR. labels Nov 21, 2017
pjw91 added 2 commits November 22, 2017 00:57
@pjw91
Remove unnecessary, incorrect workaround.
16d3e6d 
The "\printf" calls zsh shell builtin instead of `command printf` in scripting.

The workaround is no longer needed given 91a29c0.
@pjw91
Add symbolic link for printf in case of command-not-found
9352f69
@ljharb ljharb mentioned this pull request Nov 22, 2017
Closed
2 tasks
ljharb
ljharb reviewed Nov 22, 2017
View reviewed changes
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.

Any possibility of an explicit test for this behavior?

nvm.sh
command printf %s\\n "$*" 2>/dev/null || {
nvm_echo() {
# shellcheck disable=SC1001
\printf %s\\n "$*" # on zsh, `command printf` sometimes fails
Copy link
Member

Choose a reason for hiding this comment

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

wow, is the implication here that this failure is solely based on using a var named "path" in nvm_find_up??

If so, then I think that shellcheck should warn for variables named path when set to zsh mode - that would have caught this issue much sooner. I'll file one :-) Update: I filed koalaman/shellcheck#1058, but turns out shellcheck doesn't care about zsh.

Copy link
Contributor Author

@pjw91 pjw91 Nov 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure. Just a guess, and it seems OK after removing the code. (Tests are passed.)

The backslash inactivates alias and has no effect in scripting.
Thus this snippet works as "use shell builtin when command fails".

Note: I just noticed that:
command printf in bash: is shell builtin in bash
command printf in zsh: is command /usr/bin/printf

@pjw91
Copy link
Contributor Author

pjw91 commented Nov 22, 2017

This bug should have been found by test/sourcing/Sourcing nvm.sh with --install and .nvmrc should install it, but it was worked around by 4842641.
(That test case calls nvm_rc_version->nvm_find_nvmrc->nvm_find_up.)

@ljharb ljharb removed the needs followup We need some info or action from whoever filed this issue/PR. label Nov 22, 2017
ljharb
ljharb approved these changes Nov 22, 2017
View reviewed changes
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.

Thanks!

@ljharb ljharb merged commit 7bfd1e0 into nvm-sh:master Nov 22, 2017
ljharb added a commit to ljharb/nvm that referenced this pull request Dec 9, 2017
@ljharb
v0.33.7
65f0572 
Fixes
 - fix unassigned variable (nvm-sh#1665, nvm-sh#1664)
 - Fix for $path used by zsh (nvm-sh#1669)
 - `set -u`: ensure `NVM_USE_OUTPUT` is always set (nvm-sh#1671)
 - `install.sh`: Fix a bug that block that installation of node in install.sh (nvm-sh#1676)
 - `nvm install-latest-npm`: fix node 4-4.6

Documentation
 - Make `nvm cache clear` message less ambiguous (nvm-sh#1644)
 - Added missing piece (nvm-sh#1658)
edwmurph pushed a commit to edwmurph/nvm that referenced this pull request Apr 9, 2018
@ljharb
Merge pull request nvm-sh#1669 from mail6543210/master
41e44ab 
Fix for $path used by zsh
edwmurph pushed a commit to edwmurph/nvm that referenced this pull request Apr 9, 2018
@ljharb
v0.33.7
941a96b 
Fixes
 - fix unassigned variable (nvm-sh#1665, nvm-sh#1664)
 - Fix for $path used by zsh (nvm-sh#1669)
 - `set -u`: ensure `NVM_USE_OUTPUT` is always set (nvm-sh#1671)
 - `install.sh`: Fix a bug that block that installation of node in install.sh (nvm-sh#1676)
 - `nvm install-latest-npm`: fix node 4-4.6

Documentation
 - Make `nvm cache clear` message less ambiguous (nvm-sh#1644)
 - Added missing piece (nvm-sh#1658)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
bugs Oh no, something's broken :-( shell: zsh
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pjw91 @ljharb