8000 Fix error with zsh aliases in nvm_print_color_code by hosamaly · Pull Request #2365 · nvm-sh/nvm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix error with zsh aliases in nvm_print_color_code #2365

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
Merged
merged 2 commits into from
Jan 20, 2021
Merged

Fix error with zsh aliases in nvm_print_color_code #2365

merged 2 commits into from
Jan 20, 2021

Conversation

hosamaly
Copy link
Contributor

Hopefully fixes #2362

@hosamaly hosamaly mentioned this pull request Nov 27, 2020
Closed
ljharb
ljharb requested changes Nov 28, 2020
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.

Unfortunately when I run nvm_print_color_code R, with alias -g R='| less -S' set, with this change, I still don't get the expected output of 1;31m.

A regression test might be helpful.

@ljharb
Copy link
Member
ljharb commented Nov 28, 2020

(cc @naomiquinones @gitburd for possibly a bizarre zsh behavior with case statements and global aliases that #2291 may have exposed)

@hosamaly
Copy link
Contributor Author

It works on my machine 🤷
I'm sorry I can't help further. I'm no expert in shell programming (or testing).

@gitburd
Copy link
Contributor
gitburd commented Dec 1, 2020 •
edited
Loading

With alias -g R='| less -S' set running nvm_print_color_code R
echoing $1 inside the function: echo "'${1-}'"
prints ''.
I'm still looking into it, but, it seems based on that altering the case statement to check for a string won't fix the issue.

@hosamaly
Copy link
Contributor Author
hosamaly commented Dec 1, 2020

@ljharb @gitburd Have you tried running nvm_print_color_code 'R' (quoting the argument)?

@ljharb
Copy link
Member
ljharb commented Dec 1, 2020

@hosamaly indeed, that does bypass the issue - even without this PR's change.

However, aliases in everything but zsh can't appear except in command position, and it seems like in zsh, it can appear in any position :-/

We can certainly try modifying nvm to invoke nvm_print_color_code with only quoted arguments, but that means that we'd also have to quote every single hardcoded argument passed to every function, just for zsh :-/

@gitburd
Copy link
Contributor
gitburd commented Dec 1, 2020

Would it makes sense to update just this function to quote or \ the input, since it takes a single character input, which is more likely to be a global alias than a more specific input is? Or should we wait for an alternative solution?

@ljharb
Copy link
Member
ljharb commented Dec 2, 2020

That seems like a reasonable middle ground, for now, but I’d prefer a long-term solution - perhaps like i do for IFS (re-invoke nvm with out any zsh aliases, or something)

@gitburd
Copy link
Contributor
gitburd commented Dec 3, 2020

@ljharb The changes in this PR made it so that with alias -g R='| less -S' set I could source nvm.sh which is the important issue that needs to be fixed, it won't run nvm_print_color_code R correctly but nvm_print_color_code seems to work correctly when called by nvm_ls ect.

@ljharb ljharb mentioned this pull request Dec 3, 2020
Closed
@ljharb
Copy link
Member
ljharb commented Dec 3, 2020

I'm happy to land this PR asap that quotes them; I was able to reproduce this error by setting the alias before sourcing nvm.sh in my ~/.zshrc.

@ljharb ljharb force-pushed the patch-1 branch 4 times, most recently from febe219 to 5ac71ce Compare December 3, 2020 23:00
@ljharb
Copy link
Member
ljharb commented Dec 3, 2020

I'm trying to add a test, but it's not reproducing the failure :-/

@hosamaly
Copy link
Contributor Author

@ljharb I tried to reproduce the error using the aliases defined on my machine, but the tests still pass.

However, I don't think that a fix must wait until a reproducible test is available. I think that the fix is ready to be merged because a) it looks safe (from a code review standpoint), b) it doesn't cause any tests to fail, and c) a few people reported that it fixed the issue.

What do you think?

@ljharb
Copy link
Member
ljharb commented Dec 12, 2020

@hosamaly that's a fair point; and I could certainly land the quotes as-is, but without a regression test, it's highly likely it'll break in the future - and it's also likely i'll forget to add the test later if the fix is already merged :-)

I'll probably land it without a test in the next week or so, but it'd be really nice to have a test first.

@gitburd
Copy link
Contributor
gitburd commented Jan 12, 2021 •
edited
Loading

@ljharb I was able to write a test where sourcing nvm.sh fails by moving it from setup to a new test file.

https://github.com/gitburd/nvm/blob/f3f25ad157a13cdcbb488be762cfc496cc5cb638/test/sourcing/Sourcing%20nvm.sh%20global%20alias%20bug#L1-L21

@gitburd
Copy link
Contributor
gitburd commented Jan 15, 2021

@ljharb This branch adds the test to the fix that @hosamaly made.

@ljharb ljharb force-pushed the patch-1 branch 2 times, most recently from cd4ec40 to 622b53d Compare January 18, 2021 04:17
@ljharb
Copy link
Member
ljharb commented Jan 18, 2021

Thanks! I've pulled that in and confirmed it fails the sourcing tests, only in zsh: https://travis-ci.com/github/nvm-sh/nvm/jobs/472555823

However, I then applied the fix on top, and they're still not passing :-/ so perhaps we still need a different fix?

ljharb
ljharb approved these changes Jan 20, 2021
View reviewed changes
Copy link
Member
@ljharb ljharb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tweaked the test and got it to fail, and then pass, so i think we're good to go!

@ljharb ljharb merged commit 015623e into nvm-sh:master Jan 20, 2021
ljharb added a commit to ljharb/nvm that referenced this pull request Jan 20, 2021
@ljharb
[Fix] zsh: set no_aliases option while sourcing, and in a few fun…
5ef974f 
…ctions

Followup to nvm-sh#2365.
i4u6kk
i4u6kk reviewed Mar 30, 2021
View reviewed changes
Copy link
@i4u6kk i4u6kk left a comment

Choose a reason for hiding this comment

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

Wow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@i4u6kk i4u6kk i4u6kk left review comments

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nvm conflicts with zsh global alias
4 participants
@hosamaly @ljharb @gitburd @i4u6kk
0