Fix: Windows preview window logic, including git diff preview #1452

psyngw · 2023-01-06T19:16:09Z

Fix preview window problems for Windows user.

I generally use Arch for development, but recently I also started using WSL, sometimes I don't want to open WSL, so I just open nvim in terminal and use it, I'm kind of a heavy user of fzf.vim, this plugin is really good. However, I found that the preview window reported various errors when using it directly in Windows (e.g. nvim-qt);

I looked through the documentation and some other PRs, but I found that some of the solutions could cause another problem while solving one, so I tried to fix some of the problems and added an option to use other bash directly ( git bash, for example).

I've tried to minimize the impact on users using other environments, and I use WSL and Linux myself, so I understand the importance of stability.

Here's what I've tested, and even if it doesn't merge successfully, I hope this will help people who are having the same problem as me.

All with or without delta, ag.
All neovim use packer.
All vim use plug.

Windows:

neovim and vim x64 in cmd and powershell
nvim-qt and gVim x64
neovim and vim in WSL - ubuntu and Arch
Git bash with vim

gVim x32 can not work properly because it can't access the bash under C:\system32, some people say 32bit can only access the files under C:\Windows\Sysnative but I don't have the environment to test it for now, replace it in the configuration file to other bash (such as git bash: let g:fzf_preview_win_bash='C:\path\to\Git\bin\bash.exe') can work properly.

Linux:

Arch

Mac:

Maybe will test it next week . Tested: macOS Catalina 10.15.7 with neovim v0.8.2.

junegunn · 2023-01-09T03:01:04Z

autoload/fzf/vim.vim

+let s:is_defined_win_bash = get(g:, 'fzf_preview_win_bash', '') != ''
+if s:is_defined_win_bash
+  let s:defined_win_bash = split(system('for %A in ("'.g:fzf_preview_win_bash.'") do @echo %~sA'), "\n")[0]
+endif


If I'm not mistaken g:fzf_preview_win_bash is only for Windows, so should we move this code block to line 45?

I think we should use something like s:bash or s:bash() that stores or returns the path to bash to use regardless of the platform and refer to it throughout the code instead of putting conditionals here and there.

function! s:bash() if exists('s:bash') return s:bash endif " Non-windows if !s:is_win let s:bash = 'bash' elseif exists('g:fzf_preview_win_bash') " ... else " ... endif return s:bash endfunction

In fact, you can see in line 44 is to determine whether it is running under windows (although only to determine the path of bash, but this path in other environments will not happen, so it should be safe), I changed line 45 to this because in the case of the windows environment has been determined, if you call c:\windows\ system32\bash.exe to use, in the original if only has ('nvim') will lead to path errors (because c:\windows\system32\bash.exe actually requires the path parameters need to be wsl in the format, that is, else contained in the windows format will be amended to /mnt/ blabla... This format, so I made this if, if s:is_defined_win_bash judgment holds and is not custom c:\windows\system32\bash.exe case, we will continue to use the path under the windows rule. (This is also a defensive if on my part, as I ask in the do that if you want to customize bash, make sure the custom bash supports the path to the windows rule as an argument)
It's worth mentioning again that the operations here are based on the windows environment if on line 44, so there should be no conflict (at least in my tests so far)

and for the s:bash:
You're right, and I know I should, but as I mentioned above, I made a lot of poor if else because I wanted to make sure everywhere I could that it wouldn't interfere with running on other platforms besides windows.
Of course, if make a method is necessary, then I cound find some time to make it right ~ : )
And also, obviously my English is very poor, I hope you can understand what I mean

junegunn · 2023-01-09T07:12:55Z

I'm not a Windows user, and it's really hard for me to understand all the different problems from different setups. So let me clarify a few things.

No problem when you're running fzf.vim on WSL bash?
No problem when you're running fzf.vim on cmd or powershell?
Then this patch is supposed to fix issues only on nvim-qt?
- Does gVim x64 also have the problem?
Can you show the error messages?
What are the values of s:is_win and s:is_wsl on nvim-qt?

psyngw · 2023-01-09T08:29:05Z

@junegunn
Okay, I understand your question, later when I finish my daily work, if I have time I will answer your questions one by one and try to optimize the code structure.
I've been a little busy lately, with work, covid-19 and some other stuff, so please give me some time.

junegunn · 2023-01-09T10:34:54Z

No hurry, take your time.

…w option for Windows user(not wsl)

psyngw · 2023-01-09T18:14:23Z

@junegunn
For your question.

No problem when you're running fzf.vim on WSL bash?
Yeah, no problem if in the virtual WSL, obvious it's in a Linux system if in the WSL. All error occur outside the WSL, In the Windows env.
No problem when you're running fzf.vim on cmd or powershell?
No, there's same problems like on nvim-qt(some differences from Gvim, which will be mentioned below.)
Then this patch is supposed to fix issues only on nvim-qt?
No, for cmd, powershell, nvim-qt and gvim, all from Windows env.
Does gVim x64 also have the problem?
Yeah, x64 also have the problem, but less than nvim and nvim-qt. Also blow.
Can you show the error messages?
Of course, This is almost always because of the different path rules used by bash on the two systems.
If errors occurred, it would be fatal:{your exec file path}, No such file or diretory
What are the values of s:is_win and s:is_wsl on nvim-qt?
Of course is 1, which means true. Just like the env nvim and vim on cmd and powershell.

And here's something new.

Let's say u use nvim on cmd or powershell, or nvim-qt directly. And u installed WSL,the env's default bash will be the linux-like bash, Ok the usual preview(e.g. :Files, :Ag) and diff preview both occur error because of the path rules.

If u use vim on cmd or powershell, or gvim directly. And u installed WSL, then the usual preview will working but the diff preview won't, here's less problem than nvim because of the if on line 41 of the original code.

For all the situations, if u didn't install WSL, That will always occur error, of course, that is because the env can not find bash, the solution is also very simple, download the required bash and the accompanying tools, add the env variables. Here only add the fzf_preview_win_bash I added to fix is impossible, because obviously we have to use a series of other tools to preview.

Then let's take a look at the new code. I've separated out a few variables, but I've also tried to make sure that it only affects the logic of the Windows environment. To make it easier to add different regex paths in the future I've extracted the linux-like bash and added another global variable so that users(who knows exactly what they're doing) could directly define whether the bash they are using is linux-like or not, but of course, the new variables will not affect the non-Windows env, no matter how the user defines them.

Here's the new code's test:
Default bash: C:\Windows\System32\bash.exe.
Other bash: Git Bash.
All with delta and ag.
All nvim use packer. All vim use plug.
Each test:

default bash without define any variable
set g:preview_win_bash
different g:preview_win_bash_linux_like with different situations
Focus on the commands that could preview like: GFiles, GFiles?, Buffers, Files, Ag, Lines.....

Windows:

win11 and win10
neovim and vim x64 on cmd and powershell
nvim-qt and gVim x64
neovim and vim in WSL - ubuntu and Arch
cmder and git bash use vim and nvim from outside, not their own vim

Linux:

Arch

Mac:

maybe tomorrow, again :> Done: macOS Catalina 10.15.7 with neovim and vim

For Windows:
there's still some problems in Cygwin, I find out they are win32unix, But for the time being, I think it's better to just use other solutions, such as using an vim and nvim from outside, and use winpty vim in git bash.

Sorry I force-pushed, I feel 1 commit is better to read.

junegunn · 2023-01-11T00:55:59Z

autoload/fzf/vim.vim

-  if has('nvim')
+  if !s:is_wsl_bash


So we no longer need to differentiate between neovim and vim?

Yes through my tests, we just need to handle the path correctly.

junegunn · 2023-01-11T01:00:43Z

autoload/fzf/vim.vim

@@ -28,22  28,54 @@ set cpo&vim
 " Common
 " ------------------------------------------------------------------

+function! s:get_if_linux_like_bash()


Can there be a better way to determine the property of the given bash? For example, I wonder if there's a command that we can run to see the behavior of the bash.

Something like bash -c 'ls /mnt/*' maybe? (and see if it returns one of /mnt/[A-Z])

Something like bash -c 'ls /mnt/*' maybe? (and see if it returns one of /mnt/[A-Z])

This is a good idea, it seems to avoid the maintenance of new and different types of bash in the future.

psyngw · 2023-01-11T09:03:38Z

@junegunn
For the latest code:
I didn't use ls to verify, because I think it seems to work in most cases but I still want to be more stable, so I added a parameter to preview.sh to test it, doesn't affect anything, I don't know what you think about this.

This time I didn't force push because I need to do more test after daily work, and also, your opinion.

Windows:

win10 passed all of the following items, wait for win11 test ... Passed
neovim and vim x64 on cmd and powershell
nvim-qt and gVim x64
neovim and vim in WSL - ubuntu and Arch
cmder and git bash(use winpty vim

Linux:

Arch

Mac:

macOS Catalina 10.15.7 with neovim and vim

junegunn · 2023-01-13T13:17:05Z

autoload/fzf/vim.vim

@@ -633,6  649,7 @@ function! fzf#vim#gitfiles(args, ...)
    return s:warn('Not in git repo')
  endif
  let prefix = 'git -C ' . fzf#shellescape(root) . ' '
+  let diff_prefix = s:is_linux_like_bash ? 'git -C ' . substitute(root, '^\([A-Z]\):', '/mnt/\L\1', '') . ' ' : prefix


Shouldn't we still shellescape (or fzf#shellescape) the /mnt/... path in case it contains whitespaces?

Hmm, shellescape function of Vim is probably not the right choice because this is a Windows vim.

Shouldn't we still shellescape (or fzf#shellescape) the /mnt/... path in case it contains whitespaces?

Understand your point, I'll find a way to deal with it.

I did some test with create some whitespaces git repo.
escape(root, ' ') or substitute(root, '\s', '\\ ', 'g') both worked.

root path with whitespace could be solved, but if there's some whitespaces in filename, I haven't found a solution in fzf.vim yet, because I remember this seems need to be solved from the fzf side, which I met it before when I was using it in Arch, I don't think we should use whitespace in the filename so I didn't thought it's a issus : )

if there's some whitespaces in filename, I haven't found a solution in fzf.vim yet

I'm not sure what you mean by that. {} evaluates to an escaped string, so this works as expected.

echo 'hello' > 'foo bar' echo 'foo bar' | fzf --preview 'cat {}'

I don't think we should use whitespace in the filename

I agree, but file paths with whitespaces do exist (Program Files for example) and we can't just ignore that. Anyway, I believe escape(root, ' ') should be good enough for 99% of the cases.

Let me take this over and finish it up hopefully with some improvements.

Yeah I did know there's some paths like Program Files, so I said escape(root, ' ') worked.
What I mean for the whitespaces in the filename is this, for example on Arch:

If there's whitespaces in the filename, the preview command occur something, but I think we shouldn't use those filenames, so I don't think that's an issus.

And do I need to modify this line to
let diff_prefix = s:is_linux_like_bash ? 'git -C ' . escape(substitute(root, '^\([A-Z]\):', '/mnt/\L\1', ''), ' ') . ' ' : prefix
then rebase to one commit and force push?
And I want to add this to the doc for the fzf_preview_win_bash:
We do not recommend including special characters such as whitespaces in the path if it's not the windows system's native path

#1200 should have fixed it.

Ah, that doesn't address the issue in :GF? variant.

Yeah the :GitFiles? needs this too, I'll see what I could do or maybe I could just look forward to your update : )
Then still the question I mentioned above, do I need force push to one commit for updating diff_prefix line and doc, or u take all over~

Yeah the :GitFiles? needs this too

We'll address this in a separate issue/PR.

* Look for the usual location of Git bash by default. No need to manually set a variable. * Rename `g:fzf_preview_win_bash` to `g:fzf_preview_bash` and use it also on non-Windows environment * Replace \ to / even on Git bash, because cmd.exe was not properly passing backslashes in the argument * This fixes `GF?` not running `preview.sh` > /usr/bin/bash: C:UsersADMINI~1VIM~1pluggedfzf.vimbinpreview.sh: command not found * Remove --bash-test flag from preview.sh * `ls /mnt/[A-Z]` should suffice and is less likely to have issues with cmd.exe argument passing mechanism C:\Users\Administrator>cd "c:\Program Files\Git\bin" c:\Program Files\Git\bin>bash -c "ls C:\\Users" ls: cannot access 'C:Users': No such file or directory c:\Program Files\Git\bin>bash -c "ls C:/Users" Administrator 'All Users' Default 'Default User' Public desktop.ini

junegunn · 2023-01-15T13:13:48Z

I have updated your branch, please see the commit message of 8c29937 for details, and let me know if it doesn't work as expected. One thing I wasn't so sure about was the way you tested "linux-like"-ness of the bash using bash PREVIEW_SCRIPT --bash-test command. Shouldn't we be running bash -c "PREVIEW_SCRIPT --bash-test" so that the path is handled by bash itself not by cmd.exe? Anyway, the result of the check affects the path manipulation afterwards, so I felt it wasn't a good idea to use a path that needs manipulation at this stage, so I changed it to ls /mnt/[A-Z] which does not need any escaping or manipulation.

psyngw · 2023-01-16T02:23:19Z

Shouldn't we be running bash -c "PREVIEW_SCRIPT --bash-test"

I think we are exec the bash script directly so we don't need bash -c to run a command on windows, so cmd.exe is ok to do this.And for why I don't use ls /mnt/, I have mentioned that.

I didn't use ls to verify, because I think it seems to work in most cases but I still want to be more stable, so I added a parameter to preview.sh to test it, doesn't affect anything, I don't know what you think about this.

It seems that you still prefer ls, and I think it's fine, it should be good enough for 99.99999% of the cases : )

At last I push a new commit to fix two lines, I found no problems elsewhere and I have tested the items I tested before and they all worked ~ sorry for forced pushing because the test env is too much to see the wrong file : (

autoload/fzf/vim.vim

junegunn · 2023-01-16T02:32:12Z

I think we are exec the bash script directly

This I'm not so sure about. According to :help system(), it's a function that "Get the output of the shell command {expr} as a String". On my MacBook, echo system('/bin/echo foo') works as expected, but it fails once I unset shellcmdflag which is by default -c, and it seems to tell it's not directly "exec"ing /bin/echo.

sorry for forced pushing

No problem, will merge this, thanks!

autoload/fzf/vim.vim

psyngw · 2023-01-16T02:46:13Z

but it fails once I unset shellcmdflag

Ah, that's where I couldn't see and consider, I'll check it later.

Anyway, thank you very much for some advice and suggestions, I have learned a lot : )

junegunn · 2023-01-16T04:41:23Z

Merged, thanks for the work!

* upstr-master: Add :Jumps (junegunn#710) Require the latest fzf [fzf#vim#grep[2]] Deprecate has_column argument Add :RG command and fzf#vim#grep2 function Update documentation and comments [[B]Commits] Support arguments for git-log (junegunn#1455) Fix: Windows preview window logic, including git diff preview (junegunn#1452) Update RG example fix: GFiles is ignoring args (junegunn#1447) [GFiles] Use ls-files --deduplicate only on git 2.31 or above Add support for nushell (junegunn#1300) Make :GFiles work with "unusual" filenames (junegunn#1200) Fix ctags language force for C (junegunn#1425) Update g:fzf_preview_window example Revert "Add Sponsor Labels action" Add Sponsor Labels action

junegunn reviewed Jan 9, 2023

View reviewed changes

Fix: Windows preview window logic, including git diff preview, add ne…

13a04c5

…w option for Windows user(not wsl)

junegunn added the windows label Jan 10, 2023

junegunn reviewed Jan 11, 2023

View reviewed changes

Fix: update some logic for the prev commit.

f322d7a

junegunn reviewed Jan 13, 2023

View reviewed changes

junegunn added 2 commits January 15, 2023 19:35

Merge branch 'master' into master

7b3c932

Fix: logic for s:escape_for_bash.

253a7c5

junegunn reviewed Jan 16, 2023

View reviewed changes

autoload/fzf/vim.vim Show resolved Hide resolved

psyngw commented Jan 16, 2023

View reviewed changes

autoload/fzf/vim.vim Outdated Show resolved Hide resolved

junegunn reviewed Jan 16, 2023

View reviewed changes

autoload/fzf/vim.vim Outdated Show resolved Hide resolved

Update autoload/fzf/vim.vim

e386712

Move s:warned

9109fb3

junegunn merged commit bdf48c2 into junegunn:master Jan 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Windows preview window logic, including git diff preview #1452

Fix: Windows preview window logic, including git diff preview #1452

psyngw commented Jan 6, 2023 •

edited

Loading

junegunn Jan 9, 2023

junegunn Jan 9, 2023 •

edited

Loading

junegunn Jan 9, 2023

psyngw Jan 9, 2023

junegunn commented Jan 9, 2023

psyngw commented Jan 9, 2023

junegunn commented Jan 9, 2023 •

edited

Loading

psyngw commented Jan 9, 2023 •

edited

Loading

junegunn Jan 11, 2023

psyngw Jan 11, 2023

junegunn Jan 11, 2023

junegunn Jan 11, 2023 •

edited

Loading

psyngw Jan 11, 2023

psyngw commented Jan 11, 2023 •

edited

Loading

junegunn Jan 13, 2023

junegunn Jan 13, 2023

psyngw Jan 13, 2023

psyngw Jan 13, 2023

junegunn Jan 14, 2023

psyngw Jan 14, 2023 •

edited

Loading

junegunn Jan 14, 2023

junegunn Jan 14, 2023

psyngw Jan 14, 2023

junegunn Jan 15, 2023

junegunn commented Jan 15, 2023

psyngw commented Jan 16, 2023

junegunn commented Jan 16, 2023

psyngw commented Jan 16, 2023

junegunn commented Jan 16, 2023

Fix: Windows preview window logic, including git diff preview #1452

Fix: Windows preview window logic, including git diff preview #1452

Conversation

psyngw commented Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

junegunn Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junegunn commented Jan 9, 2023

psyngw commented Jan 9, 2023

junegunn commented Jan 9, 2023 • edited Loading

psyngw commented Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junegunn Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psyngw commented Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psyngw Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junegunn commented Jan 15, 2023

psyngw commented Jan 16, 2023

junegunn commented Jan 16, 2023

psyngw commented Jan 16, 2023

junegunn commented Jan 16, 2023

psyngw commented Jan 6, 2023 •

edited

Loading

junegunn Jan 9, 2023 •

edited

Loading

junegunn commented Jan 9, 2023 •

edited

Loading

psyngw commented Jan 9, 2023 •

edited

Loading

junegunn Jan 11, 2023 •

edited

Loading

psyngw commented Jan 11, 2023 •

edited

Loading

psyngw Jan 14, 2023 •

edited

Loading