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(init): eliminate a potential recursive definition #6398

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NaroZeol
Copy link

@NaroZeol NaroZeol commented Nov 14, 2024

Description

This PR will eliminate a potential recursive definition. It will benefit users who want to use vi mode in zsh.

Motivation and Context

Closes #3418
Closes #5522

Current src/init/starship.zsh has a potential risk to cause recursive definition of starship_zle-keymap-select-wrapped when user set vi mode.

for example:

use this minimal zshrc:

bindkey -v
eval "$(starship init zsh)"

then start a terminal, type source ~/.zshrc twice

~source ~/.zshrc
~source ~/.zshrc

press ESC to trigger vi normal mode, error message show bellow:

~ 
❯ 
starship_zle-keymap-select-wrapped:1: maximum nested function level reached; increase FUNCNEST?
~

check starship_zle-keymap-select-wrapped and __starship_preserved_zle_keymap_select

~
❯ which starship_zle-keymap-select-wrapped
starship_zle-keymap-select-wrapped () {
	$__starship_preserved_zle_keymap_select "$@"
	starship_zle-keymap-select "$@"
}
~echo $__starship_preserved_zle_keymap_select
starship_zle-keymap-select-wrapped

see a recursive definition

How Has This Been Tested?

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

I use this minimal zshrc:

bindkey -v
eval "$(~/github-project/starship/target/debug/starship init zsh)"

~/github-project/starship/target/debug/starship is the binary which I build.

repeat the operations, vi mode works well and no error is reported.

and user defined action works as well:

bindkey -v
function myfoo() {
        echo "\nhello world\n"
}
zle -N zle-keymap-select myfoo; # print "hello world" every time swtich the vi mode
eval "$(~/github-project/starship/target/debug/starship init zsh)"

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@NaroZeol NaroZeol requested a review from davidkna November 23, 2024 05:57
@NaroZeol NaroZeol force-pushed the master branch 2 times, most recently from 1f3d59b to 28f084e Compare November 25, 2024 02:57
@NaroZeol
Copy link
Author

NaroZeol commented Nov 25, 2024

BTW, the correct way to do this after zsh 5.3(include) seems to simply use add-zle-hook-widget.😂

like this:

autoload -Uz add-zle-hook-widget
add-zle-hook-widget zle-keymap-select starship_zle-keymap-select

see these:
zsh-document
zsh-plugin-standard (This doc mentions that add-zle-hook-widget has been introduced in zsh 5.3)
zsh-syntax-highlighting-readme (But I don't know why this doc says the critical point is 5.9)
zsh-syntax-highlighting-code

I have tested it in zsh 5.9 and it works well except cause user's changes disappear in the actions I mentioned before.

So we need to consider whether we should give respect to changes caused by source .zshrc. AFAIC, I am still used to using source .zshrc even though I know this is not most suitable way to enable a new configuration.

You can close this PR if you think current solution is enough.

@NaroZeol
Copy link
Author

Hey @davidkna

It's been a long time, is there anything else I need to change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants