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

Use eval to evaluate $post variable as command. #4023

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

kugel-
Copy link
Contributor

@kugel- kugel- commented Oct 1, 2024

With the current code, I get this
command cat: command not found

when using any keybinding that does _fzf_complete. Seen on Ubuntu 24.04.1 (GNU bash, version 5.2.21(1)-release (x86_64-pc-linux-gnu))

Fixes: 561e0b0 ("[bash] Use command to “protect” further commands (#3462)")

With the current code, I get this
  command cat: command not found

when using any keybinding that does _fzf_complete.
Seen on Ubuntu 24.04.1 (GNU bash, version 5.2.21(1)-release (x86_64-pc-linux-gnu))

Fixes: 561e0b0 ("[bash] Use `command` to “protect” further commands (junegunn#3462)")
Signed-off-by: Thomas Martitz <[email protected]>
@junegunn
Copy link
Owner

junegunn commented Oct 1, 2024

I can't reproduce the problem with GNU bash, version 5.2.26(1)-release.

@kugel-
Copy link
Contributor Author

kugel- commented Oct 1, 2024

Very strange. I don't know what's the difference then.

On my system, the issue is typically triggered when I hit p <TAB>. I have implemented an fzf-based completion for my dirstack.

Here's the code, maybe it helps you to reproduce?

function complete_pushd
{
    local FZF_COMPLETION_TRIGGER=
    local FZF_DEFAULT_OPTS="$FZF_DEFAULT_OPTS -e +m"
    local cur
    local _ifs="$IFS"

    COMPREPLY=()
    cur=${COMP_WORDS[COMP_CWORD]}

    IFS=$'\n'
    if [ -z "$cur" ]; then
        _fzf_complete "" "$@" < <(dirs -v)
        :
    elif [[ "$cur" =~ ^[0-9]+$ ]]; then
        _fzf_complete "" "$@" < <(dirs -v)
        :
    else
        _cd
    fi

    IFS=$_ifs
    return 0
}

function pushd_n ()
{
    local n=$1 dir=$1
    if [[ "$n" =~ ^[0-9]+$ ]]; then
        dir=$(builtin dirs -l "+$n")
        builtin popd -n "+$n" >/dev/null && builtin pushd $dir >/dev/null
    else
        builtin pushd $dir >/dev/null
    fi
}

alias p=pushd_n

complete -F complete_pushd pushd_n
complete -F complete_pushd p

@LangLangBart
Copy link
Contributor

LangLangBart commented Oct 3, 2024

It seems that assigning the newline character to IFS in your custom function disables the desired Word Splitting1.

post='command -p cat'
for i in $post; do echo "Test 1: '$i'"; done
Test 1: 'command'
Test 1: '-p'
Test 1: 'cat'


IFS=$'\n'
for i in $post; do echo "Test 2: '$i'"; done
Test 2: 'command -p cat'

Maybe like this ?2

--- a/shell/completion.bash
+++ b/shell/completion.bash
@@ -372,9 +372,9 @@ _fzf_complete() {
   cmd="${COMP_WORDS[0]}"
   cur="${COMP_WORDS[COMP_CWORD]}"
   if [[ "$cur" == *"$trigger" ]] && [[ $cur != *'$('* ]] && [[ $cur != *':='* ]] && [[ $cur != *'`'* ]]; then
     cur=${cur:0:${#cur}-${#trigger}}
-
+    local IFS=$' \t\n'    # normalize IFS
     selected=$(
       FZF_DEFAULT_OPTS=$(__fzf_defaults "--reverse" "${FZF_COMPLETION_OPTS-} $str_arg") \
       FZF_DEFAULT_OPTS_FILE='' \
         __fzf_comprun "${rest[0]}" "${args[@]}" -q "$cur" | $post | command tr '\n' ' ')

Footnotes

  1. Bash Reference Manual - Word Splitting

  2. Bash Reference Manual - A Programmable Completion Example

@kugel-
Copy link
Contributor Author

kugel- commented Oct 5, 2024

Seems to work as well. Not sure what's the better approach. While my proposed patch only affects the evaluation of $post, your proposal affects the rest of the function (which can be desired or not). @junegunn what's your take on this?

@kugel-
Copy link
Contributor Author

kugel- commented Oct 10, 2024

In the meantime, I tried to get rid of the IFS override.

I tried with dirs -v | xargs -d'\n' printf "%s\0" | _fzf_complete --read0 "$@", but I found that fzf behavies differently (selection is not printed to the terminal, so no completion).

I could verify with this simpler example: dirs -p | _fzf_complete "" "$@" behaves differently from _fzf_complete "" "$@" < <(dirs -p). Only with the latter completion actually works. Does anyone have an idea?

@LangLangBart
Copy link
Contributor

behavies differently (selection is not printed to the terminal, so no completion)

I could verify with this simpler example: dirs -p | _fzf_complete "" "$@" behaves differently from _fzf_complete "" "$@" < <(dirs -p)

Mini Example:

_complete() {
  COMPREPLY=("$(cat)")
}

push() {
  printf 1 | _complete
  # _complete < <(printf 1)
}

complete -F push p

When running it with printf 1 | _complete, each command is executed in its own subshell1, which is a separate process. This is problematic for COMPREPLY because the assignment in a subshell means the result will not persist in our current shell.

Note

complete 2
-F function
The shell function function is executed in the current shell environment.
[…] possible completions are retrieved from the value of the COMPREPLY array variable.

_complete() {
  echo "PID of the current shell $$" >/tmp/log
  echo "PID of the current bash process $BASHPID" >>/tmp/log
  COMPREPLY=("$(cat)")
}

push() {
  printf 1 | _complete
}

complete -F push p

_complete is being run in a subshell, as its PID differs from that of the current shell process.

cat /tmp/log
# PID of the current shell 93282
# PID of the current bash process 93294

With _complete < <(printf 1), the process substitution345 <(...) creates either a FIFO or a named file descriptor, which is then redirected < into _complete. The intended effect is to prevent the assignment of COMPREPLY from running in a subshell, as it did with the earlier pipeline command, so the value persists.

_complete() {
  echo "PID of the current shell $$" >/tmp/log
  echo "PID of the current bash process $BASHPID" >>/tmp/log
  COMPREPLY=("$(cat)")
}

push() {
  _complete < <(printf 1)
}

complete -F push p
cat /tmp/log
# PID of the current shell 93441
# PID of the current bash process 93441

Summary: As long as COMPREPLY is assigned in a subshell6 (), it won't complete the command in our current shell.

push() {
  (COMPREPLY=("$(printf 1)"))
}

complete -F push p

I tried with dirs -v | xargs -d'\n' printf "%s\0" | _fzf_complete --read0 "$@"
[…] Does anyone have an idea?

_fzf_complete --read0 "$@" < <(dirs -v | tr '\n' '\0')

@Kugel - I wonder why you originally set IFS=$'\n' in your custom function when fzf already splits input by newline? (That this causes a bug in _fzf_complete is still a valid issue.)

Footnotes

  1. Bash Reference Manual - Pipelines

  2. Bash Reference Manual - Programmable Completion Builtins: complete

  3. Bash Reference Manual - ProcessSubstitution

  4. Greg's Wiki- ProcessSubstitution

  5. tldp.org - Process Substitution

  6. ShellCheck: SC2031 – var was modified in a subshell. That change might be lost.

@kugel-
Copy link
Contributor Author

kugel- commented Oct 14, 2024

I wonder why you originally set IFS=$'\n' in your custom function when fzf already splits input by newline?

I don't remember actually. I'm using this for several years now. Maybe fzf behaved differently in past versions?

_fzf_complete "" "$@" < <(dirs -v) seems to do what I want, as of today. Thanks

@junegunn
Copy link
Owner

Thanks @LangLangBart for taking time to look into this.

_fzf_complete function was not meant to be used directly outside a completion function (e.g. _fzf_complete_foo), so there's no guarantee that you can use it that way, but the patch look trivial and harmless, we can consider merging it.

shell/completion.bash Outdated Show resolved Hide resolved
@junegunn junegunn merged commit 97f1dae into junegunn:master Oct 15, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants