Support cygwin vim via cygpath and Windows binary #933

janlazo · 2017-05-31T03:05:49Z

Since cygwin sh/bash runs cmd.exe in the back (sh/bash by itself is run in a cmd.exe terminal) and use TERM=cygwin, we can run cygwin vim and the fzf (Windows) binary in cmd.exe/powershell. Benefit of including cygwin support to the vim plugin is running the tests as is in Windows. I'm currently using this for the Vader tests.

By unsetting TERM, fzf runs as if vim (Windows) is used. cygpath (via s:cygpath) is used for path conversion between Windows and cygwin filepaths. I updated s:escape to escape () for Windows filepaths.

Currently, this PR does not support mintty (TERM=xterm) because fzf is run inline (no shell script or batchfile) and I can't get the start command to work with shell redirection yet. Neovim TUI in cygwin sh/bash is not supported in the vim plugin even though it runs fine (without mintty) in sh/bash. I'll work on that along with :terminal when 0.2.1 is released.

I'm using the cygwin environment from Git For Windows and tested this PR in sh.exe with TERM=cygwin.

junegunn · 2017-05-31T04:07:48Z

Thanks, before we proceed, I spotted a couple of places where s:escape is used when shellescape is more appropriate.

diff --git a/plugin/fzf.vim b/plugin/fzf.vim
index 8c8ff85..e262317 100644
--- a/plugin/fzf.vim
+++ b/plugin/fzf.vim
@@ -313,7 +313,7 @@ function! fzf#wrap(...)
     if !isdirectory(dir)
       call mkdir(dir, 'p')
     endif
-    let history = s:is_win ? fzf#shellescape(dir.'\'.name) : s:escape(dir.'/'.name)
+    let history = fzf#shellescape(dir.'/'.name)
     let opts.options = join(['--history', history, opts.options])
   endif
 
@@ -553,7 +553,7 @@ function! s:execute_tmux(dict, command, temps) abort
   let command = a:command
   if s:pushd(a:dict)
     " -c '#{pane_current_path}' is only available on tmux 1.9 or above
-    let command = 'cd '.s:escape(a:dict.dir).' && '.command
+    let command = 'cd '.fzf#shellescape(a:dict.dir).' && '.command
   endif
 
   call system(command)

(fzf#shellescape on windows will correctly convert / to \, right?)

janlazo · 2017-05-31T04:58:59Z

You can use shellescape for the shell. Using shellescape for cd didn't work for me for filepaths that have characters handled in s:escape. We can add test cases in Vader to verify this.

(fzf#shellescape on windows will correctly convert / to , right?)

No.
That's what expand, glob, etc. are for and shellslash is strictly for native Windows, not cygwin.
Also, we're not using shellescape for cmd.exe, default shell in Windows Vim/Neovim. I didn't implement slash conversion in s:shellesc_cmd because backslash can escape characters, including itself, but forward slash can't.

I have shell=/usr/bin/bash in cygwin Vim so fzf#shellescape uses the built-in shellescape which does not do the slash conversion. It only escapes for the shell and Windows filepaths are valid in cygwin. The user can mix & match Windows/cygwin filepaths in sh/bash because cmd.exe will resolve the finalized command anyway.

Only /usr/bin/cygpath handles path conversion for any Windows/cygwin filepath in a cygwin environment.

junegunn · 2017-05-31T05:44:25Z

Sorry for not being clear, but I wasn't asking about cygwin environment, but native Windows.

Using shellescape for cd didn't work for me for filepaths that have characters handled in s:escape

You mean :cd? The command is fed into system() so I believe shellescape is the right choice here.

janlazo · 2017-05-31T12:09:54Z

You mean :cd? The command is fed into system() so I believe shellescape is the right choice here.

You're right. I missed s:execute_tmux. There should be no issues committing this change now.

junegunn · 2017-05-31T17:12:18Z

plugin/fzf.vim

@@ -152,7 +158,7 @@ function! s:escape(path)
  let escaped_chars = '$%#''"'

  if has('unix')
-    let escaped_chars .= ' \'
+    let escaped_chars .= ' \()'


() should be added only on cygwin environment, not on all unix.

junegunn · 2017-05-31T17:13:09Z

plugin/fzf.vim

@@ -401,7 +410,6 @@ try
    let optstr .= ' --no-height'
  endif
  let command = prefix.(use_tmux ? s:fzf_tmux(dict) : fzf_exec).' '.optstr.' > '.temps.result
-


Unnecessary.

junegunn · 2017-05-31T17:14:08Z

plugin/fzf.vim

@@ -455,7 +463,8 @@ function! s:pushd(dict)
      return 1
    endif
    let a:dict.prev_dir = cwd
-    execute 'lcd' s:escape(a:dict.dir)
+    let dir = s:escape(a:dict.dir)
+    execute 'lcd' has('win32unix') ? s:escape(s:cygpath(dir)) : dir


Seems to be escaping twice (s:escape(s:cygpath(s:escape(a:dict.dir)))), intentional?

fzf#shellescape isn't used in s:cygwin even though it uses system. To be consistent with recent changes, I'll use fzf#shellescape in s:cygwin.

It will be a swap from s:escape to shellescape in cygwin

Edit: no issues so far so I'm committing the change.

janlazo · 2017-06-02T02:25:13Z

I can't get tmux to work without mintty.

What about using batchfile to run an external cmd.exe terminal for fzf to bypass mintty and other terminals?
We already have the required code for Windows.
In case the command fails, any side-effects caused by unsetting TERM is limited to the external terminal.

janlazo · 2017-06-02T03:04:56Z

If we opt for this, it is tricky to escape because cygwin paths are invalid in cmd.exe and fzf#shellescape(str, 'cmd.exe' must be used in fzf#wrap and fzf#run for cygwin Vim.

If only there's way to do equivalent of the s:xterm_launcher in cygwin ...

junegunn · 2017-06-02T15:21:21Z

So I installed cygwin on my Windows machine, but I was unable to start fzf. Exit with an error message if I just run fzf, terminal hangs if I run TERM= fzf, any tips? Honestly I'm not familiar with all these cygwin, mintty, etc, so I'm having a little hard time following you, so bear with me.

janlazo · 2017-06-03T01:05:20Z

Cygwin's default terminal is mintty but mintty is not required to use cygwin environment.
You can add Cygwin's /bin and /usr/bin to the Windows PATH to use sh, bash, diff, less, man, vim (cygwin), git, ssh, etc. in cmd.exe or powershell.exe.

sh.exe runs on top of cmd.exe so anything passed to sh.exe will be piped to cmd.exe.
This includes batchfiles and Windows programs so vim (Windows) and Neovim 0.2 can run in sh.exe
fzf.exe errors out if TERM is cygwin, xterm (mintty), screen (tmux).
cmd.exe and powershell.exe don't set TERM so I unset TERM in s:execute for vim (cygwin) so sh.exe or bash.exe will pipe it to cmd.exe and run fzf.

It hangs in mintty so I put in the title of PR that mintty is not supported.

junegunn · 2017-06-03T09:47:23Z

Thanks for the explanation, I could confirm that the windows binary runs on cygwin running on conemu if I unset TERM.

How about if I update the Windows binary to unset TERM if it's running on Cygwin ($TERM == cygwin)?Would it make things simpler?

janlazo · 2017-06-03T09:55:21Z

Yes.

Can you try the recent commit on mintty?

junegunn · 2017-06-03T10:13:30Z

I didn't test fzf on Vim, but just on the shell. Now that I try it, It's not working for me on Vim on ConEmu, with fzf executable not found message (there's fzf.exe in bin directory). I'll look into it more.

- Update install script to download Windows binary if $TERM == cygwin - Unset TERM if $TERM == cygwin (#933) - Always use cmd.exe instead of $SHELL when running commands

junegunn · 2017-06-03T10:50:07Z

I updated the Go code to unset TERM if $TERM == cygwin. You can download the binary with the patch here: https://github.com/junegunn/fzf-bin/releases/download/alpha/fzf-0.16.8-alpha-windows_amd64.zip

janlazo · 2017-06-03T11:13:48Z

It works if not running in mintty.

junegunn · 2017-06-03T13:11:32Z

Works fine both on ConEmu and mintty, great job. Let me know if it's ready for merge. I'll merge it to devel branch first.

janlazo · 2017-06-03T15:19:41Z

I don't have X11 for Cygwin so I can't test gvim and xterm.
For now, the gui launcher in cygwin is %s, same as Windows.

janlazo · 2017-06-04T02:19:36Z

  let escaped = (a:use_height || s:is_win) ? a:command : escape(substitute(a:command, '\n', '\\n', 'g'), '%#!')
  if has('gui_running')
    let Launcher = get(a:dict, 'launcher', get(g:, 'Fzf_launcher', get(g:, 'fzf_launcher', s:launcher)))
    let fmt = type(Launcher) == 2 ? call(Launcher, []) : Launcher
    if has('unix')
      let escaped = "'".substitute(escaped, "'", "'\"'\"'", 'g')."'"
    endif
    let command = printf(fmt, escaped)
  else
    let command = escaped
  endif

What's the substitute on single quotes for? I thought built-in shellescape handles this.

junegunn · 2017-06-04T04:05:25Z

git blame on the line points to 7ae877b

I'm not sure if it was the best way to handle such directory arguments, but without it, :FZF command fails in the following case.

Try this:

cd /tmp
mkdir "a'b'c"
mkdir 'x"y"z'
touch "a'b'c"/{foo,bar}
touch 'x"y"z'/{foo,bar}

Then:

" Use tab completion to complete the directory name
:FZF /tmp/a<tab>
:FZF /tmp/x<tab>

junegunn · 2017-06-04T06:06:35Z

plugin/fzf.vim

@@ -537,6 +549,15 @@ function! s:execute(dict, command, use_height, temps) abort
      call jobstart(cmd, fzf)
      return []
    endif
+  elseif has('win32unix')
+    if $TERM ==# 'cygwin'


I think this branch can be removed now as the new binary will unset TERM if TERM == cygwin, so line 552 becomes elseif has('win32unix') && $TERM != 'cygwin'.

Sure.
I intended the vim plugin to be compatible with the current binaries for Windows but cygwin is not officially supported yet.

I'm going to merge this to devel branch first and then merge to master when I release 0.16.8, so let me know if there are other things that can be done on the binary side that can help simplyfing the Vim plugin.

junegunn · 2017-06-04T07:27:27Z

@janlazo Please see bf0cb4b

If TERM is set to cygwin, fzf will use find command as the default command instead of dir.

https://github.com/junegunn/fzf-bin/releases/download/alpha/fzf-0.16.8-alpha-windows_amd64.zip

Do you think we still need s:cygpath after this change?

- remove if branch because fzf supports '$TERM == cygwin' - unset TERM in cmd.exe so sh.exe can set it to 'cygwin'

janlazo · 2017-06-04T09:04:17Z

s:cygpath is required in :FZF for normalized absolute paths and tempname in s:execute to pass an absolute path to sh.exe.
s:cygpath is not required for getcwd, expand with :p.
s:pushd and s:common_sink may not need s:cygpath.

janlazo · 2017-06-04T09:13:43Z

Assuming there's only 1 cygwin installation, fnamemodify with :p is an alternative outside s:cmd

junegunn · 2017-06-04T11:49:24Z

I was able to run :FZF without cygpath on ConEmu and mintty with the following diff.

diff --git a/plugin/fzf.vim b/plugin/fzf.vim
index 8bdeb3b..78e8128 100644
--- a/plugin/fzf.vim
+++ b/plugin/fzf.vim
@@ -386,10 +386,10 @@ try
 
   let prefer_tmux = get(g:, 'fzf_prefer_tmux', 0)
   let use_height = has_key(dict, 'down') &&
-        \ !(has('nvim') || s:is_win || s:present(dict, 'up', 'left', 'right')) &&
+        \ !(has('nvim') || s:is_win || has('win32unix') || s:present(dict, 'up', 'left', 'right')) &&
         \ executable('tput') && filereadable('/dev/tty')
   let use_term = has('nvim') && !s:is_win
-  let use_tmux = (!use_height && !use_term || prefer_tmux) && s:tmux_enabled() && s:splittable(dict)
+  let use_tmux = (!use_height && !use_term || prefer_tmux) && !has('win32unix') && s:tmux_enabled() && s:splittable(dict)
   if prefer_tmux && use_tmux
     let use_height = 0
     let use_term = 0
@@ -485,7 +485,7 @@ function! s:xterm_launcher()
     \ &columns, &lines/2, getwinposx(), getwinposy())
 endfunction
 unlet! s:launcher
-if s:is_win
+if s:is_win || has('win32unix')
   let s:launcher = '%s'
 else
   let s:launcher = function('s:xterm_launcher')
@@ -537,6 +537,11 @@ function! s:execute(dict, command, use_height, temps) abort
       call jobstart(cmd, fzf)
       return []
     endif
+  elseif has('win32unix') && $TERM !=# 'cygwin'
+    let shellscript = s:fzf_tempname()
+    call writefile([command], shellscript)
+    let command = 'cmd.exe /C ''set "TERM=" & start /WAIT sh -c '.fzf#shellescape(shellscript).''''
+    let a:temps.shellscript = shellscript
   endif
   if a:use_height
     let stdin = has_key(a:dict, 'source') ? '' : '< /dev/tty'

junegunn · 2017-06-04T14:44:40Z

Can you test my version with the latest alpha binary? It uses cygwin find command instead of dir so there's no need for path conversion. Also I'm not sure why we need cygpath or fnamemodify on dir when it should be given in /unix/path/name. Am I missing something?

janlazo · 2017-06-04T14:51:34Z

plugin/fzf.vim

@@ -778,7 +772,7 @@ function! s:cmd(bang, ...) abort
    if s:is_win && !&shellslash
      let opts.dir = substitute(opts.dir, '/', '\\', 'g')
    elseif has('win32unix')
-      let opts.dir = s:cygpath(opts.dir)
+      let opts.dir = fnamemodify(opts.dir, ':p')


Required for Windows-style filepaths with the network drive

janlazo · 2017-06-04T14:55:12Z

plugin/fzf.vim

@@ -227,7 +221,7 @@ function! s:common_sink(action, lines) abort
    let autochdir = &autochdir
    set noautochdir
    if has('win32unix')
-      call map(a:lines, 's:cygpath(v:val)')
+      call map(a:lines, 's:fzf_fnamemodify(v:val, ":p")')


Not sure if we need this branch anymore to guarantee absolute paths.

janlazo · 2017-06-04T14:57:52Z

I tried it and find works.

janlazo · 2017-06-04T19:14:50Z

:edit C:\\Windows works but is not supported by tab completion for directories.
:FZF C:\\\\Windows works for isdirectory

Do you want to support it? fnamemodify is required for path conversion.

junegunn · 2017-06-05T01:06:51Z

Do you want to support it? fnamemodify is required for path conversion.

Do you think it would be useful? What is your opinion as a Windows/Cygwin user? Though I don't like the fact that we have to change the code for :FZF (which means that we also have to change :Files in fzf.vim and maybe some others), but if it's useful for the users, well, then it's just two lines of code, so.

By the way, can you give me an example where the extra escaping () is required? I tried something like :FZF /tmp/a\ (B) and it worked.

janlazo · 2017-06-05T02:21:45Z

What is your opinion as a Windows/Cygwin user?

It's useful for me because Git For Windows ships cygwin vim as its default editor. I don't have to install the win32/64 version of vim. I use powershell 5 for my shell so I use Windows-style paths all the time.

By the way, can you give me an example where the extra escaping () is required?

dir returns a Windows-style path with backslashes so, at the time, I had to escape the brackets.

junegunn · 2017-06-05T03:35:16Z

Fair enough, let's keep it.

at the time, I had to escape the brackets.

So can we remove it now?

janlazo · 2017-06-05T04:47:36Z

Done.

junegunn · 2017-06-05T04:54:19Z

Thanks a lot. I'm planning to release 0.16.8 after merging devel branch tomorrow, please let me know if there's any other issues you want to address before the release.

janlazo · 2017-06-05T06:10:56Z

Did you test symlinks via ln in Cygwin and use a relative path to open a symlinked file or directory?
Some files in /bin and /usr/bin are symlinked and this is inconsistent across Cygwin environments.

I think it's outside the scope of 0.16.8 but users may complain that they open a blank file instead of the symlinked file/directory.

junegunn · 2017-06-05T14:13:02Z

Thanks, but I'm not quite sure if I understood you correctly. I just opened a few symlinks with :FZF on cygwin, and I had no issue opening them.

janlazo · 2017-06-06T00:55:24Z

Try making a symlink directory with ln and delete one file in it. Compare with the original directory.
In Windows, ln.exe makes hard links only. The only way to make symlinks in Windows 7/8 is with mklink (cmd.exe) which requires admin privileges.

Compare the files in /bin with /NetworkDrive/path/to/CygwinRoot/bin and do the same for /usr/bin.

See #933 (comment) Close junegunn/fzf.vim#386

[vim] support cygwin

2e8e8c4

junegunn reviewed May 31, 2017

View reviewed changes

janlazo added 2 commits May 31, 2017 20:35

[vim] Resolve unnecessary changes

78c1390

[vim] use fzf#shellescape in s:cygpath

1cfdd49

[vim] start external cmd.exe in mintty

8619ca4

janlazo changed the title ~~Support cygwin vim via cygpath and Windows binary (mintty not supported)~~ Support cygwin vim via cygpath and Windows binary Jun 3, 2017

[vim] use fzf directly only if $TERM is "cygwin"

f80293d

junegunn added a commit that referenced this pull request Jun 3, 2017

Fixes for Cygwin

ca0b3b6

- Update install script to download Windows binary if $TERM == cygwin - Unset TERM if $TERM == cygwin (#933) - Always use cmd.exe instead of $SHELL when running commands

[vim] unset use_tmux in cygwin

0acfaa5

junegunn added the windows label Jun 3, 2017

[vim] do not use xterm in gvim (cygwin)

c25f692

junegunn changed the base branch from master to devel June 3, 2017 17:01

junegunn reviewed Jun 4, 2017

View reviewed changes

[vim] ensure '$TERM == cygwin' before running fzf

98c5d1d

- remove if branch because fzf supports '$TERM == cygwin' - unset TERM in cmd.exe so sh.exe can set it to 'cygwin'

[vim] replace s:cygpath with fnamemodify

a152b03

janlazo commented Jun 4, 2017

View reviewed changes

[vim] remove fnamemodify in common sink

0a686b5

[vim] remove () in s:escape

3529faf

junegunn merged commit 7e483b0 into junegunn:devel Jun 5, 2017

junegunn added a commit that referenced this pull request Jun 9, 2017

[vim] Expand 'dir' on Cygwin to handle Windows-style paths

5efdecc

See #933 (comment) Close junegunn/fzf.vim#386

janlazo deleted the vim/shellesc_cygwin branch November 21, 2017 17:46

janlazo mentioned this pull request Dec 25, 2017

Reduce complexity of the plugin API neovim/node-client#43

Closed

junegunn mentioned this pull request Feb 13, 2021

Vim plugin fix for Cygwin 3.1.7 and above #2349

Closed

This was referenced May 18, 2024

simplify cygwin handling in Git Bash #3804

Closed

On fzf.vim opening in new cmd Window in Git Bash #3809

Closed

Support cygwin vim via cygpath and Windows binary #933

Support cygwin vim via cygpath and Windows binary #933

Conversation

janlazo commented May 31, 2017 • edited Loading

junegunn commented May 31, 2017

janlazo commented May 31, 2017 • edited Loading

junegunn commented May 31, 2017

janlazo commented May 31, 2017

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

janlazo Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

janlazo commented Jun 2, 2017 • edited Loading

janlazo commented Jun 2, 2017

junegunn commented Jun 2, 2017 • edited Loading

janlazo commented Jun 3, 2017 • edited Loading

junegunn commented Jun 3, 2017

janlazo commented Jun 3, 2017

junegunn commented Jun 3, 2017

junegunn commented Jun 3, 2017

janlazo commented Jun 3, 2017

junegunn commented Jun 3, 2017

janlazo commented Jun 3, 2017

janlazo commented Jun 4, 2017

junegunn commented Jun 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junegunn commented Jun 4, 2017

janlazo commented Jun 4, 2017 • edited Loading

janlazo commented Jun 4, 2017

junegunn commented Jun 4, 2017

junegunn commented Jun 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janlazo commented Jun 4, 2017

janlazo commented Jun 4, 2017 • edited Loading

junegunn commented Jun 5, 2017

janlazo commented Jun 5, 2017 • edited Loading

junegunn commented Jun 5, 2017

janlazo commented Jun 5, 2017

junegunn commented Jun 5, 2017

janlazo commented Jun 5, 2017

junegunn commented Jun 5, 2017

janlazo commented Jun 6, 2017

janlazo commented May 31, 2017 •

edited

Loading

janlazo commented May 31, 2017 •

edited

Loading

janlazo Jun 1, 2017 •

edited

Loading

janlazo commented Jun 2, 2017 •

edited

Loading

junegunn commented Jun 2, 2017 •

edited

Loading

janlazo commented Jun 3, 2017 •

edited

Loading

janlazo commented Jun 4, 2017 •

edited

Loading

janlazo commented Jun 4, 2017 •

edited

Loading

janlazo commented Jun 5, 2017 •

edited

Loading