Skip to content

Conversation

@Adnn
Copy link

@Adnn Adnn commented Oct 12, 2022

This is the other part of the issue 1432 opened in the fzf.vim repository.

It fixes the feature of :Buffers in Neovim running on Windows' git-bash, while making sure not to break when running in Windows' cmd.

plugin/fzf.vim Outdated
let temps.input = s:fzf_tempname()
call s:writefile(source, temps.input)
let source_command = (s:is_win ? 'type ' : 'cat ').fzf#shellescape(temps.input)
let source_command = (s:is_win && !s:is_gitbash ? 'type ' : 'cat ')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken, environ() is a recent addition to Vim (after Vim 8), so it's probably safer to use exists('$VARNAME') not to break this on older Vim versions.

Suggested change
let source_command = (s:is_win && !s:is_gitbash ? 'type ' : 'cat ')
let source_command = (s:is_win && !exists('$SHELL') ? 'type ' : 'cat ')
@junegunn
Copy link
Owner

I wonder if it's possible to treat git-bash environment like a Unix shell environment. Does this work? Just a wild guess.

diff --git a/plugin/fzf.vim b/plugin/fzf.vim
index 40f01a0..2190209 100644
--- a/plugin/fzf.vim
+++ b/plugin/fzf.vim
@@ -26,7 +26,7 @@ if exists('g:loaded_fzf')
 endif
 let g:loaded_fzf = 1
 
-let s:is_win = has('win32') || has('win64')
+let s:is_win = (has('win32') || has('win64')) && !exists('$SHELL')
 if s:is_win && &shellslash
   set noshellslash
   let s:base_dir = expand('<sfile>:h:h')
@Adnn
Copy link
Author

Adnn commented Oct 19, 2022

@junegunn Thank you for your feedback.

I tried what you proposed, globally treating git-bash like a Unix shell env by replacing:
let s:is_win = (has('win32') || has('win64')) && !exists('$SHELL')

Sadly it broke in other places.

I also tried to scope the change more thightly, by defining:
let s:is_win_cmd = s:is_win && !exists('$SHELL')
And using it in both shellescape and let source_command = (s:is_win_cmd ? 'type ' : 'cat ').

Sadly this broke the :Files: command. I suspect this is because :Files: uses shellescape, and it needs the cmd.exe like type of escaping somewhere.

All in all, I would say the change needs to be very precisely scoped, otherwise other parts are breaking.

Would you be okay that I revert to the initial proposed change? (but using exists('$SHELL'), as you advised).

@Adnn
Copy link
Author

Adnn commented Oct 19, 2022

I reverted while using exists('$SHELL'), so all commands work on my test environment.

let source_command = (s:is_win ? 'type ' : 'cat ').fzf#shellescape(temps.input)
" Disable shell escape for git bash, as it breaks the command here
let source_command = (s:is_win_cmd ? 'type ' : 'cat ')
\.(!s:is_win || !exists('$SHELL') ? fzf#shellescape(temps.input) : substitute(temps.input, '\', '/', 'g'))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we still shellescape (not fzf#shellescape) temps.input for safety?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expression is getting a bit too complicated. How about splitting it into if-else statements?

if !s:is_win
  let source_command = ...
elseif exists('$SHELL')
  " Git bash
  let source_command = ...
else
  " cmd.exe
  let source_command = ...
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

2 participants