junegunn / fzf

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

Cannot run `fzf --tmux` if bash env has an exported function #4001

Closed przepompownia closed 1 month ago

przepompownia commented 1 month ago

Checklist

Output of fzf --version

0.55 (devel) (from Debian Sid)

OS

Shell

Problem / Steps to reproduce

test@del:~$ foo() { :; }; export -f foo; tmux new fzf --tmux
[exited]

Inside tmux session:

test@del:~$ fzf --tmux
exit status 2

After unset foo the command tmux new fzf --tmux works again.

Tested on clean ~/.bashrc, ~/.tmux.conf and fzf env.

junegunn commented 1 month ago

Though fzf --tmux works for me even with an exported function, I can see an error message flashing before fzf opens. It looks like fzf is trying to export BASH_FUNC_foo%%='() { .... I'll see what I can do.

przepompownia commented 1 month ago

I found some suggestion: https://unix.stackexchange.com/questions/498905/why-is-my-bash-func-foobar-environment-variable-unset-in-shell-subprocesses. For me

 $ stat /usr/bin/sh
  File: /usr/bin/sh -> dash

but I still can't figure out which part (fzf or tmux) is trying to invoke sh as opposed to bash.

przepompownia commented 1 month ago

tmux-server-620294.log:

1726934290.409360 cmd_parse_build_commands: display-popup -BE -d /home/test -h "50%" -w "50%" -x C -y C sh /tmp/fzf-temp-956960853
przepompownia commented 1 month ago

Seems to be here: https://github.com/junegunn/fzf/blob/855f90727af7827d9934b7fa00ea5ed51f5e4e81/src/tmux.go#L52-L56

przepompownia commented 1 month ago

I'm not sure if the change

diff --git a/src/tmux.go b/src/tmux.go
index 246222f..9a5fe2c 100644
--- a/src/tmux.go
+++ b/src/tmux.go
@@ -50,7 +50,7 @@ func runTmux(args []string, opts *Options) (int, error) {
        tmuxArgs = append(tmuxArgs, "-h"+opts.Tmux.height.String())

        return runProxy(argStr, func(temp string) *exec.Cmd {
-               sh, _ := sh()
+               sh := os.Getenv("SHELL")
                tmuxArgs = append(tmuxArgs, sh, temp)
                return exec.Command("tmux", tmuxArgs...)
        }, opts, true)

is correct in general, but works for me ($SHELL is /bin/bash).

image

junegunn commented 1 month ago

I don't think it's the right way to fix it because

  1. $SHELL may not be a Posix-compliant shell. For example, tcsh cannot process export foo='bar' lines we produce in https://github.com/junegunn/fzf/blob/855f90727af7827d9934b7fa00ea5ed51f5e4e81/src/proxy.go#L99
  2. Even when $SHELL is bash, export BASH_FUNC_foo%%='() { ... } is incorrect because BASH_FUNC_foo%% is not a valid identifier in bash.

So what can we do?

  1. Do not try to export variables with strange names. Easy and simple. But this means we no longer can access exported bash functions in fzf on a tmux popup.
  2. If $SHELL is bash`, we try to define such functions for the subshell. It's a little complicated and may have some unwanted side effects.

I have a patch for option 1, but I'm going to experiment with option 2 as well.

przepompownia commented 1 month ago

If we cannot always guess the correct shell binary, maybe we should make it configurable (with sh or the parent shell binary as the default - bash in my case), to avoid using dash to perform bash scripts, like in my example.

In my personal bash-only environment I want to feel free to keep such bashisms as the example function. No need for portability there.

Offtopic: I didn't found where BASH_FUNC_foo%% is created. I also still don't know how to see what /tmp/fzf-temp* contains.

przepompownia commented 1 month ago

Thanks, bash detection seems to work for me.