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

Embed shell integration scripts in fzf binary (--bash / --zsh / --fish) #3675

Merged
merged 16 commits into from
Mar 13, 2024

Conversation

junegunn
Copy link
Owner

@junegunn junegunn commented Mar 12, 2024

This simplifies the distribution, and the users are less likely to have problems caused by using incompatible scripts and binaries.

  • bash
    # Set up fzf key bindings and fuzzy completion
    eval "$(fzf --bash)"
  • zsh
    # Set up fzf key bindings and fuzzy completion
    eval "$(fzf --zsh)"
  • fish
    # Set up fzf key bindings
    fzf --fish | source

This simplifies the distribution, and the users are less likely to have
problems caused by using incompatible scripts and binaries.

  # Set up bash shell extension
  source <(fzf --bash)

  # Set up zsh shell extension
  source <(fzf --zsh)
@junegunn junegunn self-assigned this Mar 12, 2024
@junegunn junegunn changed the title Embed shell extension in fzf binary (--bash & --zsh) Embed shell integration scripts in fzf binary (--bash & --zsh) Mar 12, 2024
@junegunn junegunn changed the title Embed shell integration scripts in fzf binary (--bash & --zsh) Embed shell integration scripts in fzf binary (--bash / --zsh / --fish) Mar 12, 2024
@akinomyoga
Copy link
Contributor

akinomyoga commented Mar 12, 2024

In Bash 3.2, the source builtin doesn't support reading from a pipe, so source <(...) fails (silently). What is the minimal Bash version that fzf supports? As far as I see key-bindings.bash, fzf intends to cover Bash 3. If that is the case, source "$(fzf --bash)" (edit: sorry, I meant eval "$(fzf --bash)") could be used.

@akinomyoga
Copy link
Contributor

Is there a way to turn on either key-bindings.bash or completion.bash while the other is turned off? I'm not sure about a good interface, but for example,

fzf --bash=[key-bindings | completion | all]

defaulted to all.

@junegunn
Copy link
Owner Author

In Bash 3.2, the source builtin doesn't support reading from a pipe

Ah, that's unfortunate. I wasn't aware of it. I liked source <(...) better than eval "$(...)" because it looked better (no quotes). It's a superficial cosmetic reason that doesn't justify dropping support for bash 3 (which is the built-in version of bash in macOS), so I'm going to suggest eval instead.

Is there a way to turn on either key-bindings.bash or completion.bash while the other is turned off?

I thought about it, but I decided to keep it simple. We could add a magic comment at the top of each section so that advanced users can sed away the unwanted part.

@akinomyoga
Copy link
Contributor

Thanks for your reply!

main.go Outdated Show resolved Hide resolved
@junegunn junegunn changed the base branch from master to rc March 13, 2024 11:51
@junegunn junegunn force-pushed the embed-shell-extension branch 2 times, most recently from 089ca03 to b24eb8b Compare March 13, 2024 13:48
@junegunn junegunn merged commit e74b125 into rc Mar 13, 2024
2 checks passed
@junegunn junegunn deleted the embed-shell-extension branch March 13, 2024 14:59
@akinomyoga
Copy link
Contributor

@junegunn I suggested evaluating the integration shell scripts by eval (for the Bash compatibility), but I noticed there is a problem with the current content of the integration scripts. [[ $- =~ i ]] || return 0 at the beginning of the integration scripts, when executed in non-interactive shells, terminates the execution of .bashrc, .zshrc, etc. in the middle because eval runs the commands in the context where eval is called.

One solution is to avoid using return 0 and enclose the entire content of the integration scripts with a big if statement as if [[ $- =~ i ]]; then <all the script contents>; fi. Another solution is to enclose the script contents within a temporary function when embedding them in stdout. For example, this function

fzf/main.go

Lines 30 to 34 in 4cd37fc

func printScript(label string, content []byte) {
fmt.Println("### " + label + " ###")
fmt.Println(strings.TrimSpace(string(content)))
fmt.Println("### end: " + label + " ###")
}

can be modified to additionally print

  • __fzf_initialize() { and unset -f __fzf_initialize }; __fzf_initialize for Bash,
  • print () { and } for Zsh
  • and something equivalent for Fish.

@junegunn
Copy link
Owner Author

junegunn commented Apr 9, 2024

@akinomyoga Thanks, I'll update the script files. We used to wrap the whole thing before f103aa4.

junegunn added a commit that referenced this pull request Apr 9, 2024
So that there's no error even when the scripts are mistakenly evaluated
in non-interactive sessions.

  bash -c 'eval "$(fzf --bash)"; echo done'
  zsh -c 'eval "$(fzf --zsh)"; echo done'

* #3675 (comment)
* f103aa4
@akinomyoga
Copy link
Contributor

Thank you for fixing it! I think the fish integration script also needs to be adjusted.

status is-interactive; or exit 0

@junegunn
Copy link
Owner Author

We instruct fish users to use source via pipe.

https://github.com/junegunn/fzf?tab=readme-ov-file#setting-up-shell-integration

And as far as I can tell, it doesn't have the same issue.

@akinomyoga
Copy link
Contributor

Ah, OK! I missed it. Sorry for the noise, and thank you for the clarification. I think now everything should be fine.

Thank you for your quick responses and actions.

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

Successfully merging this pull request may close these issues.

2 participants