junegunn/fzf

v0.50.0 not showing process data correctly

blaineventurine opened this issue · 10 comments

Checklist

  • I have read through the manual page (man fzf)
  • I have searched through the existing issues
  • For bug reports, I have checked if the bug is reproducible in the latest version of fzf

Output of fzf --version

0.50.0 (brew)

OS

  • Linux
  • macOS
  • Windows
  • Etc.

Shell

  • bash
  • zsh
  • fish

Problem / Steps to reproduce

I have my completion trigger set to ,,. After doing a brew upgrade, this is now the output of kill ,, for me:

image

If I select one (or more), it does not have a process ID, the command will be entered as: kill 2^[[0m

Looks like your ps commmand is producing ANSI color codes when it shouldn't. You see colored output when you do ps -ef | cat? And what is the output of command -v ps?

Looks like your ps commmand is producing ANSI color codes when it shouldn't. You see colored output when you do ps -ef | cat? And what is the output of command -v ps?

No color output, and /bin/ps

@blaineventurine sets an alias for cat1 and has a config file for bat with the --color=always flag. 2

alias cat='bat'

Additionally, there is no --ansi flag in the FZF_DEFAULT_OPTS3.


Minimal setup to reproduce the issue:

export FZF_DEFAULT_OPTS=""
export FZF_COMPLETION_TRIGGER=',,'
alias cat='bat --color=always --number'
source /Users/paria/Developer/fzf/shell/completion.zsh

It seems to have come from commit 152988c.

Prepending command to the cat command in the function below seems to fix the issue. However,
this does not explain why the issue only became apparent after commit 152988c. Anyone understands this?

fzf/shell/completion.zsh

Lines 183 to 187 in f97d275

_fzf_feed_fifo() (
command rm -f "$1"
mkfifo "$1"
cat <&0 > "$1" &
)

Another cat command in that file already has been prefixed with command4, lets just follow suit ?

fzf/shell/completion.zsh

Lines 231 to 236 in f97d275

__fzf_list_hosts() {
setopt localoptions nonomatch
command cat <(command tail -n +1 ~/.ssh/config ~/.ssh/config.d/* /etc/ssh/ssh_config 2> /dev/null | command grep -i '^\s*host\(name\)\? ' | awk '{for (i = 2; i <= NF; i++) print $1 " " $i}' | command grep -v '[*?%]') \
<(command grep -oE '^[[a-z0-9.,:-]+' ~/.ssh/known_hosts 2> /dev/null | tr ',' '\n' | tr -d '[' | awk '{ print $1 " " $1 }') \
<(command grep -v '^\s*\(#\|$\)' /etc/hosts 2> /dev/null | command grep -Fv '0.0.0.0' | command sed 's/#.*//') |
awk '{for (i = 2; i <= NF; i++) print $i}' | sort -u

Footnotes

  1. dotfiles/.config/zsh/aliases.zsh · blaineventurine/dotfiles

  2. dotfiles/.config/bat/config · blaineventurine/dotfiles

  3. dotfiles/.config/zsh/.zshenv · blaineventurine/dotfiles

  4. Pull Request #701 · junegunn/fzf

@blaineventurine sets an alias for cat1

Thank you for doing what I should have done and giving a fully reproducible config.

Prepending command to the cat command in the function below seems to fix the issue.

Confirmed.

But I have to mention that aliasing cat to bat with --color=always is a bad idea and should be discouraged. It will break existing scripts that expect the normal cat behavior. alias cat='bat --number' should be enough. You'll get pretty output when you cat something, and it will not break scripts as bat will not print color codes when the standard output is not a TTY device.

alias cat='bat --number'

# Colors
cat README.md

# No colors
cat README.md | command cat

Commit 152988c wrapped the emulate part in an if statement. With that change, the alias expansion no longer worked as expected.

Moving the interactive check after the emulate command seems to have solved the issue and prevents us from prepending every single command with command, as was described in #1944.

The movement of the interactive check to the top came with #3449, but it was just a simple one-line check, compared with the commit mentioned prior which wrapped everything in an if statement.

Note

I am not confident that the explanation is correct; it was derived only through testing. The documentation about emulate did not provide clear instructions, and searching through the Zsh community posts did not offer much further assistance, other than the frequently mentioned advice of placing emulate at the top of the script.

@LangLangBart Thanks for the investigation. Should we do it like this then?

diff --git a/shell/completion.zsh b/shell/completion.zsh
index 7067b06..8cd9737 100644
--- a/shell/completion.zsh
+++ b/shell/completion.zsh
@@ -9,9 +9,6 @@
 # - $FZF_COMPLETION_TRIGGER (default: '**')
 # - $FZF_COMPLETION_OPTS    (default: empty)
 
-if [[ -o interactive ]]; then
-
-
 # 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
@@ -76,6 +73,8 @@ fi
 # `finally` in lesser languages. We use it to *always* restore user options.
 {
 
+if [[ -o interactive ]]; then
+
 # To use custom commands instead of find, override _fzf_compgen_{path,dir}
 #
 #   _fzf_compgen_path() {
@@ -346,10 +345,10 @@ fzf-completion() {
 zle     -N   fzf-completion
 bindkey '^I' fzf-completion
 
+fi
+
 } always {
   # Restore the original options.
   eval $__fzf_completion_options
   'unset' '__fzf_completion_options'
 }
-
-fi

@LangLangBart Thanks for the investigation. Should we do it like this then?

Yes, key-bindings.zsh may require similar modifications, and maybe in one file a hint ?

# The 'emulate' command should not be placed inside the interactive check,
# as doing so fails to disable alias expansion.
if [[ -o interactive ]]; then

@LangLangBart Sounds good. Can you open a pull request?

But I have to mention that aliasing cat to bat with --color=always is a bad idea and should be discouraged. It will break existing scripts that expect the normal cat behavior. alias cat='bat --number' should be enough. You'll get pretty output when you cat something, and it will not break scripts as bat will not print color codes when the standard output is not a TTY device.

Thanks for this - I believe I copied it out of the bat documentation at some point. Removing --color=always fix the issue I was having.