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

Issues 1432 #3002

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Issues 1432 #3002

wants to merge 7 commits into from

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
@@ -484,7 +487,8 @@ try
elseif type == 3
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants