junegunn / fzf

:cherry_blossom: A command-line fuzzy finder
https://junegunn.github.io/fzf/
MIT License
65.81k stars 2.41k forks source link

fix(zsh): handle backtick trigger edge case #4090

Closed LangLangBart closed 1 week ago

LangLangBart commented 2 weeks ago

Description

If a single backtick is used as a trigger, the words array fails to populate with the command elements.

~~The idea here is to temporarily alter the LBUFFER before calling __fzf_extract_command by escaping the backtick.~~

The idea is to move the CURSOR before the trigger.

This approach seems a bit hacky, which is why this PR is marked as a draft.

Alternative A

Create a list of invalid triggers, since other characters have also been found to cause the same issue:

Testing covered only ASCII values 32-126; others were excluded for simplicity.

Symbol Common Name ASCII Occurrences in Public Git Repos
& Ampersand 38 1
; Semicolon 59 ~25
[ Left Bracket 91 1
` Back-tick 96 ~20
\| Pipe 124 0

If the user attempts to use one, display a message using zle -M [string] to indicate the error. This places the string below the command line, prompting the user to choose a different trigger character.

Alternative B

Fall back to the old extraction method if the words array is empty and the LBUFFER is not empty, with the downside that it would only work for the first command.

--- a/shell/completion.zsh
+++ b/shell/completion.zsh
@@ -123,9 +123,20 @@ __fzf_comprun() {
 # Extract the name of the command. e.g. ls; foo=1 ssh **<tab>
 __fzf_extract_command() {
+  local token tokens
   setopt localoptions noksh_arrays
-  # Control completion with the "compstate" parameter, insert and list noting
+  # Control completion with the "compstate" parameter, insert and list nothing
   compstate[insert]=
   compstate[list]=
   cmd_word="${words[1]}"
+  [[ -n $cmd_word ]] && return
+  tokens=(${(z)LBUFFER})
+  for token in $tokens; do
+    token=${(Q)token}
+    if [[ "$token" =~ [[:alnum:]] && ! "$token" =~ "=" ]]; then
+      cmd_word="$token"
+      return
+    fi
+  done
+  cmd_word="${tokens[1]}"
 }

@@ -341,5 +352,4 @@ fzf-completion() {
     # Make the 'cmd_word' global
     zle __fzf_extract_command || :
-    [[ -z "$cmd_word" ]] && return

     [ -z "$trigger"      ] && prefix=${tokens[-1]} || prefix=${tokens[-1]:0:-${#trigger}}

Alternative B might be the best option. If a user encounters an issue like the one described in #1992, simply suggest using a different trigger?

jefferickson commented 2 weeks ago

I'd advocate for Alternative B as I've used the backtick as my trigger for 6+ years. The key is right next to the on an American layout and quite convenient.

Thank you!

LangLangBart commented 1 week ago

For the current version, the CURSOR is moved before the trigger, as the words array likely interprets any of the listed characters in the table above as the start of a new command.

The approach would still extract the correct command.

More testing of these changes is needed.

LangLangBart commented 1 week ago

I'd advocate for Alternative B

That was also my intention, but temporarily moving the cursor before calling the __fzf_extract_command completion widget seems to work well and ensures that the user still gets the correct list of matches displayed in commands like below:

ls ; kill -9 `[TAB]

Please test the latest patch. The zsh version affects many users, and it's important to avoid releasing with errors. Feedback will help ensure the fix works well. Thanks!

source <(curl -fsSL https://raw.githubusercontent.com/LangLangBart/fzf/refs/heads/zsh_backtick/shell/completion.zsh)
jefferickson commented 1 week ago

Please test the latest patch.

The patch works for me and my backtick trigger! Thank you!

junegunn commented 1 week ago

I'm testing the latest revision, and I'm seeing this warning message.

image

Turns out I have setopt WARN_CREATE_GLOBAL in my .zshrc. Can we fix it? Or am I supposed to remove the setting from my config? From a confused bash user.

Having said that, the fix works as expected.

LangLangBart commented 1 week ago

Turns out I have setopt WARN_CREATE_GLOBAL in my .zshrc. Can we fix it? Or am I supposed to remove the setting from my config? From a confused bash user.

Thanks for testing. I will add typeset -g cmd_word.

man zshoptions | less --pattern 'WARN_CREATE_GLOBAL'
WARN_CREATE_GLOBAL
  Print a warning message when a global parameter is created in  a
  function  by an assignment or in math context.  This often indi-
  cates that a parameter has  not  been  declared  local  when  it
  should  have  been.   Parameters explicitly declared global from
  within a function using typeset -g do not cause a warning.  Note
  that  there  is no warning when a local parameter is assigned to
  in a nested function, which may also indicate an error.
# see 'zshbuiltins'  for 'typeset'
man zshbuiltins
junegunn commented 1 week ago

Thanks, it all looks good to me, is there anything else you want to address?

LangLangBart commented 1 week ago

Thanks, it all looks good to me, is there anything else you want to address?

I am finished. During testing, I found no other issues. I hope fzf users won't find any either.

The bugs (missing enhancement) reported from my zsh patches weren't created intentionally, and I am sorry you had to deal with so many releases.

junegunn commented 1 week ago

No problem at all, I really appreciate your help.

junegunn commented 1 week ago

Merged thanks!

jefferickson commented 1 week ago

Thank you @LangLangBart ! Really appreciate the fast turn around and response to my report.