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(core): Use full path to env in functions.zsh #11709

Closed
wants to merge 1 commit into from

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented May 23, 2023

Standards checklist:

  • The PR title is descriptive.
  • The PR doesn't replicate another PR which is already open.
  • I have read the contribution guide and followed all the instructions.
  • The code follows the code style guide detailed in the wiki.
  • The code is mine or it's from somewhere with an MIT-compatible license.
  • The code is efficient, to the best of my ability, and does not waste computer resources.
  • The code is stable and I have tested it myself, to the best of my abilities.
  • If the code introduces new aliases, I provide a valid use case for all plugin users down below.

Changes:

  • Replace bare 'env' with '/usr/bin/env', in uninstall_oh_my_zsh()

Other comments:

I have a shell alias for env, in my interactive shell environment. So, with just a bare env called in the uninstall_oh_my_zsh() function, re-executions of my .zshrc would report a parse error:

$ . ~/.zshrc                       
/.../lib/functions.zsh:8: parse error near `ZSH="$ZSH"'

It's safer to use /usr/bin/env there, same as in the various shebang lines for the executable scripts.

@ohmyzsh ohmyzsh bot added the Area: core Issue or PR related to core parts of the project label May 23, 2023
@ferdnyc
Copy link
Contributor Author

ferdnyc commented May 23, 2023

Although it wasn't my intention in making this change, this PR may also help to address the issues encountered in the discussion at #11602.

However, to completely address that issue, it may also be advisable to unset the PATH envvar in the env command (/usr/bin/env -u PATH ZSH="$ZSH"), so that it will reset to the system default and allow sh "$ZSH/tools/uninstall.sh" to run successfully even if the $PATH was previously broken.

Thoughts on adding a -u PATH to the command?

@carlosala carlosala closed this in 902b79e May 23, 2023
@carlosala
Copy link
Member

Thanks for your contribution!

@ferdnyc ferdnyc deleted the patch-1 branch May 25, 2023 22:34
@ferdnyc ferdnyc restored the patch-1 branch May 25, 2023 22:34
@ferdnyc ferdnyc deleted the patch-1 branch May 25, 2023 22:34
@ferdnyc ferdnyc restored the patch-1 branch May 25, 2023 22:37
@ferdnyc
Copy link
Contributor Author

ferdnyc commented May 25, 2023

@carlosala

command env is a better solution for my case of having env aliased, agreed. But IIUC, it still won't help at all with situations like #11602 where (seemingly) the $PATH has been utterly corrupted, correct?

I wonder if there's still more that can be done to make the uninstall bulletproof no matter what state the system is in? (Just to give people a parachute for when they've broken something and can't figure out what / how to fix it.)

nbaronov pushed a commit to nbaronov/oh-my-zsh that referenced this pull request Aug 3, 2023
cschuyle pushed a commit to cschuyle/ohmyzsh that referenced this pull request Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Issue or PR related to core parts of the project
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants