Repository: junegunn/fzf (language: go)

Issue:
shell script interactiveness checks
- [X] I have read through the manual page (`man fzf`)
- [X] I have the latest version of fzf
- [X] I have searched through the existing issues

## Info

- OS
    - [X] Linux
    - [ ] Mac OS X
    - [ ] Windows
    - [ ] Etc.
- Shell
    - [X] bash
    - [ ] zsh
    - [ ] fish

## Problem / Steps to reproduce

I've looked at the code of `completion.bash` and `key-bindings.bash`. Both (but AFAICS also the ones for the other shells) contain checks for whether the shell is interactive or not.

- Are these checks even necessary at all?<br>Completion should anyway only happen when the shell is interactive, so when a package installs `completion.bash` e.g. into `/usr/share/bash-completion/completions/fzf`, it should never get sourced anyway, when not interactive.<br>The same IMO for `key-bindings.bash`, which is probably sourced by `.bashrc`, which in turn is only executed for interactive non-login shells.
- One could argue that people might source it from `.profile`, which is also souced for non-interactive login shells. But then,... why does e.g. `key-bindings.bash` define `__fzf_select__()` before the check for interactiveness? Shouldn't that also only be defined if the shell is interactive?

I could try to provide a patch, if you tell me what you think should be done.

Cheers,
Chris.


Discussion / hints:
> Are these checks even necessary at all?

Probably not in most setups. But I don't think having another safety net is a problem. fzf is an extremely popular program that is used by many users with different skill levels with vastly different setups, so yeah, I think it helps.

> why does e.g. key-bindings.bash define __fzf_select__() before the check for interactiveness?

We needed to define it even in non-interactive shells so that the early tmux integration would work without having to load the scripts for interactivity. (`-ci` -> `-c`)

https://github.com/junegunn/fzf/commit/77d45cb17372045fb52e9d32562349d3fa083117

However, that is no longer the case because we use fzf-tmux script instead.
> However, that is no longer the case because we use fzf-tmux script instead.

So… shouldn't we then move everything under the safety net?
And as said… I think at least for the bash completion scripts it's likely unnecessary in practise... I mean it makes only sense to protect people from shooting into their own foot up to some certain level ;-)
> So… shouldn't we then move everything under the safety net?

Yes. I just found it too. I had to dig through the git history to find out why it's bypassing the check. Would you mind opening a PR?

> I mean it makes only sense to protect people from shooting into their own foot up to some certain level ;-)

Fair enough. Please note that fzf can be installed by manually cloning the repo, so the files can be placed anywhere the user wants and they can make any mistake they want :)

Fix patch:
diff --git a/shell/completion.bash b/shell/completion.bash
index fb0b6c83b49..463d7764066 100644
--- a/shell/completion.bash
+++ b/shell/completion.bash
@@ -9,7 +9,8 @@
 # - $FZF_COMPLETION_TRIGGER (default: '**')
 # - $FZF_COMPLETION_OPTS    (default: empty)
 
-if [[ $- =~ i ]]; then
+[[ $- =~ i ]] || return 0
+
 
 # To use custom commands instead of find, override _fzf_compgen_{path,dir}
 if ! declare -F _fzf_compgen_path > /dev/null; then
@@ -401,5 +402,3 @@ _fzf_setup_completion 'var'   export unset printenv
 _fzf_setup_completion 'alias' unalias
 _fzf_setup_completion 'host'  telnet
 _fzf_setup_completion 'proc'  kill
-
-fi
diff --git a/shell/completion.zsh b/shell/completion.zsh
index 1e7d6f10e3b..a0406cb428c 100644
--- a/shell/completion.zsh
+++ b/shell/completion.zsh
@@ -9,6 +9,9 @@
 # - $FZF_COMPLETION_TRIGGER (default: '**')
 # - $FZF_COMPLETION_OPTS    (default: empty)
 
+[[ -o interactive ]] || return 0
+
+
 # Both branches of the following `if` do the same thing -- define
 # __fzf_completion_options such that `eval $__fzf_completion_options` sets
 # all options to the same values they currently have. We'll do just that at
@@ -73,9 +76,6 @@ fi
 # `finally` in lesser languages. We use it to *always* restore user options.
 {
 
-# Bail out if not interactive shell.
-[[ -o interactive ]] || return 0
-
 # To use custom commands instead of find, override _fzf_compgen_{path,dir}
 if ! declare -f _fzf_compgen_path > /dev/null; then
   _fzf_compgen_path() {
diff --git a/shell/key-bindings.bash b/shell/key-bindings.bash
index 398f66e49c4..e57f2af8107 100644
--- a/shell/key-bindings.bash
+++ b/shell/key-bindings.bash
@@ -11,6 +11,9 @@
 # - $FZF_ALT_C_COMMAND
 # - $FZF_ALT_C_OPTS
 
+[[ $- =~ i ]] || return 0
+
+
 # Key bindings
 # ------------
 __fzf_select__() {
@@ -27,8 +30,6 @@ __fzf_select__() {
     done
 }
 
-if [[ $- =~ i ]]; then
-
 __fzfcmd() {
   [[ -n "${TMUX_PANE-}" ]] && { [[ "${FZF_TMUX:-0}" != 0 ]] || [[ -n "${FZF_TMUX_OPTS-}" ]]; } &&
     echo "fzf-tmux ${FZF_TMUX_OPTS:--d${FZF_TMUX_HEIGHT:-40%}} -- " || echo "fzf"
@@ -130,5 +131,3 @@ fi
 bind -m emacs-standard '"\ec": " \C-b\C-k \C-u`__fzf_cd__`\e\C-e\er\C-m\C-y\C-h\e \C-y\ey\C-x\C-x\C-d"'
 bind -m vi-command '"\ec": "\C-z\ec\C-z"'
 bind -m vi-insert '"\ec": "\C-z\ec\C-z"'
-
-fi
diff --git a/shell/key-bindings.zsh b/shell/key-bindings.zsh
index c04a00201fe..4c4f0a0864a 100644
--- a/shell/key-bindings.zsh
+++ b/shell/key-bindings.zsh
@@ -11,6 +11,9 @@
 # - $FZF_ALT_C_COMMAND
 # - $FZF_ALT_C_OPTS
 
+[[ -o interactive ]] || return 0
+
+
 # Key bindings
 # ------------
 
@@ -36,8 +39,6 @@ fi
 
 {
 
-[[ -o interactive ]] || return 0
-
 # CTRL-T - Paste the selected file path(s) into the command line
 __fsel() {
   local cmd="${FZF_CTRL_T_COMMAND:-"command find -L . -mindepth 1 \\( -path '*/.*' -o -fstype 'sysfs' -o -fstype 'devfs' -o -fstype 'devtmpfs' -o -fstype 'proc' \\) -prune \


Test patch: