[vim] Use backslash for Windows filepaths #896

janlazo · 2017-04-02T20:27:26Z

Category
- fzf binary
- fzf-tmux script
- Key bindings
- Completion
- Vim
- Neovim (starting 3adea89)
- Etc.
OS
- Linux
- Mac OS X
- Windows
- Windows Subsystem for Linux
- Etc.
Shell
- bash
- zsh
- fish

PR for the shellescaping issue I described in #882.

I added wrapper functions to handle filepath functions in vim.

set noshellslash for backslash in filepaths
set shellslash for shellescape() to single quote first, and then double quote its output (ex. path --> "'path'")
- Vim runs cmd.exe twice, one is a wrapper proxy, and the other is the actual command
- cmd.exe strips the double quotes of each argument
- C:\Program Files (x86)\ (no quotes) breaks on s:execute() so it must be escaped with single/double quotes in cmd.exe and powershell

I wasn't sure where to put the new functions so I placed them just after if guard at the top of the file because I used fzf#expand for s:base_dir.

junegunn · 2017-04-03T02:21:41Z

plugin/fzf.vim

+  endif
+  return expand(a:fmt)
+endfunction
+


Three functions are almost identical. We can refactor the code using call().

function! s:without_shellescape(fn, ...) if s:is_win let shellslash = &shellslash try set noshellslash return call(a:fn, a:000) finally let &shellslash = shellslash endtry endif return call(a:fn, a:000) endfunction function! fzf#expand(fmt) return s:without_shellescape('expand', a:fmt) endfunction " ...

By the way, I'm not sure if we should expose these functions to the users by using fzf# prefix? Should they be script-local? What do you think?

I used fzf# prefix because I thought that fzf.vim can use it to handle shellslash. However, these new functions are not suppose to change and are integral to fzf's Vim/Neovim plugin. Outside of fzf and fzf.vim, users shouldn't rely on these new functions. Do you prefer to minimize Vimscript dependencies between fzf and fzf.vim?

Yes. Ideally, users of the core fzf plugin, such as fzf.vim, should only rely on fzf#run and fzf#wrap but nothing else, and fzf#run should handle all the escaping issues automatically. So I'm not happy that we have to change the implementation of :FZF command.

Can we do better? If it's not feasible to properly escape a raw options string for different platforms, we can consider extending fzf#run so that it can process array arguments like so:

call fzf#run({'options': [['--prompt', getcwd()], '--reverse', ['--expect', 'ctrl-a']]})

What about fzf#shellescape()? Make it a script-local function as well?
For :FZF, what should I do to not change the implementation? Move it to fzf#wrap or fzf#run?

I'll think about it and let you know. I need to find time to test the plugin on Windows before making the decision. Thanks.

junegunn · 2017-04-03T02:22:17Z

plugin/fzf.vim

    let opts.dir = substitute(substitute(remove(args, -1), '\\\(["'']\)', '\1', 'g'), '[/\\]*$', '/', '')
-    let opts.options .= ' --prompt '.fzf#shellescape(opts.dir)
+    if s:is_win
+        let opts.dir = substitute(opts.dir, '/', '\\', 'g')


A minor nit. Indentation should be 2 spaces.

- fzf# functions using "noshellslash" are script-local proxies - all of them call s:without_shellescape to use noshellslash - prefix changed from fzf# to s:fzf_ - move fzf#shellescape() to its original spot - it's not script local before this PR - users could be using this function for the fzf prompt - reindent to 2 spaces in s:cmd()

janlazo · 2017-04-05T01:26:04Z

We can move the shellescaping to s:execute by putting the command in a temporary shell script (or batch file) to get around cmd.exe. We can refactor fzf#shellescape to use s:without_shellescape and continue using strings for the fzf options because the prompt won't lose its quoting. Side benefit is that we bypass shellescaping issues for Neovim in Windows and have more flexibility with the launcher format (e.g. Vim allows :!start cmd /c but not :!start /WAIT cmd /c).

It's the simplest workaround with minimal changes to :FZF

junegunn · 2017-04-05T01:31:11Z

If it means that it fixes the issue on Windows without requiring user scripts to change to deal with escaping issues, then I'm all for it.

- rename s:without_shellescape() to s:fzf_call() - fzf#shellescape() acts as proxy to call s:fzf_call() for shellescape() - double the backslash for the prompt so it doesn't escape double quotes

janlazo · 2017-04-07T01:19:47Z

If the prompt is escaped with single quotes, then the space character has to be escaped via escape().
If the prompt is escaped with double quotes, then the double quote character has to be escaped via escape() to avoid escaping it with last backslash meant for the filepath.
escape(path, ' \') as suggested in :help escape() catches both cases but we may not be able to handle it outside of s:cmd so, as you suggested, modifying fzf#run to allow list/object arguments is a better long-term solution.

We can still use the batch file to avoid anymore complications. I can't use :!start /WAIT directly. It has to be !cmd /c start /WAIT . Seems to be an edge case in Vim especially on gVim.

Do you want a separate PR for the common launcher? Since what we have already works in Windows, I think that only Neovim will benefit from this.

janlazo

g:fzf_history_dir is a filepath so shouldn't fzf#shellescape() be used here like in s:cmd() for the prompt?

Also, should we allow :FZF to break if g:fzf_history_dir points to a restricted filepath (e.g. C:\Program Files (x86)\?

- resolves shellescaping issues for filepaths - forward slash or backslash - directory/file with spaces - compatible with :Files - use jobstart for neovim in s:execute (Windows) - only supports builds after PR #6497

janlazo · 2017-04-14T09:59:44Z

@junegunn Can you test if the latest commit requires no changes with FZF.vim? I only tested :FZF, :Files, and :Colors on both Vim and Neovim (I mentioned that it only supports builds starting neovim/neovim#6497 and I added the bare minimum code to launch it).

junegunn · 2017-04-14T13:30:50Z

@janlazo Thanks for the update and sorry for the delay. I'll test it this weekend.

junegunn · 2017-04-16T04:53:16Z

@janlazo Hi, I finally got a chance to test it on Windows box and I made a few changes to your version.

fzf_expand doesn't work with <sfile> (see :help <sfile> for details), so I had to call expand at the top-level
Remove runtime check of s:is_win in s:fzf_* functions by defining different versions of functions depending on s:is_win up-front.
:FZF works regardless of shellslash, but :Files breaks with shellslash since it does not use fzf#shellescape.
But instead of exposing/using fzf#shellescape, I decided that it's better to allow fzf functions to take options in list type. :Files will still have to be updated, but it doesn't have to know about fzf#shellescape.

The code is here:
https://gist.github.com/junegunn/6929954a64bead0e0a2aa1955cf0afd7

Can you review the changes and update your pull request if it makes sense?

janlazo · 2017-04-16T06:48:05Z

@junegunn Thanks for the feedback. I'll review your gist and update the PR during the week. I'll implement the list type for options if you don't mind. I didn't see your implementation in the gist.

junegunn · 2017-04-16T08:22:07Z

I didn't see your implementation in the gist. I also updated FZF command.

It's there. See s:evaluate_opts. - junegunn

janlazo · 2017-04-16T14:10:53Z

How can I know which options to escape if we use a list instead of a dict?
s:evaluate_opts is fine if we're sticking with double quotes for shell-quoting and the shell quoting for list type only happens once. But, if we need any more flexibility, s:evaluate_opts is not enough.

junegunn · 2017-04-16T14:30:32Z

I'm not sure if I follow you. We allow options entry of the dictionary to be a list instead of a string. And the user code simply does not escape the arguments in the list, fzf#run or fzf#wrap does.
So one can write, call fzf#run({'options': ['--prompt', 'Prompt "string">']}) without worrying about with escaping issues. i.e. call fzf#run({'options': '--prompt "Prompt \"string\">"'}).

I suggest that you apply the entire code in the gist to your branch and see what I have changed.

justinmk · 2017-04-16T14:45:58Z

FWIW since #882 was created, Windows support for :terminal materialized in neovim/neovim#6383. I haven't tried it with fzf yet.

janlazo · 2017-04-16T21:16:55Z

@junegunn Did fzf_expand break because of fzf_call? fzf_expand can work if it uses expand directly (I assume this is why your gist had it as well).
Testing it with janlazo@4dad69b

let s:is_win = has('win32') || has('win64')
if s:is_win
  function! s:fzf_expand(fmt)
    let shellslash = &shellslash
    try
      set noshellslash
      return expand(a:fmt)
    finally
      let &shellslash = shellslash
    endtry
  endfunction
else
  function! s:fzf_expand(fmt)
    return expand(a:fmt)
  endfunction
endif
let s:base_dir = s:fzf_expand('<sfile>:h:h')     " it worked with :p as well

@justinmk As long as it works in cmd.exe (colorless or not) along with the terminal UI, I can test it. Do I have to compile it manually or can I get the appveyor build from neovim/neovim#6383?

justinmk · 2017-04-16T23:26:21Z

AppVeyor build should work. That PR doesn't include the TUI though (that is a separate PR not yet merged).

As long as it works in cmd.exe (colorless or not) along with the terminal UI,

The AppVeyor zip includes a GUI (just double-click nvim-qt.exe), so why is a TUI required?

junegunn · 2017-04-17T01:03:42Z

@janlazo No, like I mentioned above, you can't properly expand <sfile> in a function call, see :help <sfile>. If you remove fzf executable from your %PATH% and put it under bin directory of fzf repo, you'll notice that the code will fail. And your branch still misses a few important points of my change (expanding s:fzf_go, not using s:fzf_expand in s:cmd). Why don't you just overwrite the code entirely with my gist and start from there? Or do you want me to commit my change at this point? Agree with your comment here though.

junegunn · 2017-04-17T01:38:52Z

@janlazo To elaborate, I'd like to prove that we can implement :FZF without using any of s:fzf_* functions as it's a reference implementation of FZF-based commands. s:shortpath still contains calls to those functions, but I believe we can remove them as well, as it only affects the prompt string, and I think it's okay to have /s in it if the user set shellslash.

Gist Link: https://gist.github.com/junegunn/6929954a64bead0e0a2aa1955cf0afd7

- remove runtime check in s:fzf_call - add fallback implementation of s:fzf_call if not running in Windows - remove s:fzf_ functions in s:shortpath - match the last slash of the prompt based on &shellslash

janlazo · 2017-04-17T03:41:39Z

@junegunn I overwrote the vim plugin with your gist as you requested. I added a few changes for s:shortpath and added a fallback for s:fzf_call. Is this enough or should we go further?

junegunn · 2017-04-17T04:05:07Z

@janlazo

added a fallback for s:fzf_call

Hmm, do we need s:fzf_call when s:is_win is false? Am I missing something?

I like what you suggested in here: https://gist.github.com/junegunn/6929954a64bead0e0a2aa1955cf0afd7#gistcomment-2061358

Having two versions of s:fzf_call only.

janlazo · 2017-04-17T04:28:03Z

@junegunn I thought you wanted to skip a function call since we're defining the functions upfront. I prefer we stick with s:fzf_ functions where we can for consistency so I added fzf_call. If something similar to expand('<sfile>') happens, we'll handle it upfront similar to s:base_dir.

janlazo · 2017-04-17T04:58:43Z

Testing it in janlazo@9d8ef7d

junegunn · 2017-04-19T01:32:27Z

plugin/fzf.vim

  endif
-  let opts.options .= ' '.join(args)
+  call extend(opts.options, ['--prompt', (s:is_win && !&shellslash) ? escape(prompt, '\') : prompt])


One question: do we need escape call here? I thought s:evaluate_opts will take care of it automatically using shellescape.

It's a hack.
'\\' is a literal backslash and \" is a literal double quote in cmd.
We need an additional backslash to escape the last backslash of the prompt such that the double quotes can be removed by cmd to process the prompt.
Instead of just appending a backslash because it's a directory filepath, I escaped all the backslash of the filepath. You can test by replacing the ternary expression for escape with

prompt.((s:is_win && !&shellslash) ? '\' : '')

janlazo · 2017-04-19T03:12:16Z

@justinmk I want to have a TUI to compare with a GUI when running :terminal. TUI is not required but it gives me a visual assurance of the :terminal support in Windows even though Neovim abstracts the terminal issues for any UI anyway.

justinmk · 2017-04-19T04:49:15Z

I don't understand. :terminal (programs running inside nvim) is orthogonal to TUI (nvim running in console.exe).

junegunn · 2017-04-21T14:30:56Z

@janlazo So shellescape fails to handle trailing \, hmm. How about if we directly tackle the problem in fzf_shellescape?

diff --git a/plugin/fzf.vim b/plugin/fzf.vim
index 72be9f2..e4b388d 100644
--- a/plugin/fzf.vim
+++ b/plugin/fzf.vim
@@ -44,10 +44,18 @@ if s:is_win
       let &shellslash = shellslash
     endtry
   endfunction
+
+  function! s:fzf_shellescape(path)
+    return substitute(s:fzf_call('shellescape', a:path), '[^\\]\zs\\"$', '\\\\"', '')
+  endfunction
 else
   function! s:fzf_call(fn, ...)
     return call(a:fn, a:000)
   endfunction
+
+  function! s:fzf_shellescape(path)
+    return shellescape(a:path)
+  endfunction
 endif
 
 function! s:fzf_getcwd()
@@ -66,10 +74,6 @@ function! s:fzf_tempname()
   return s:fzf_call('tempname')
 endfunction
 
-function! s:fzf_shellescape(path)
-  return s:fzf_call('shellescape', a:path)
-endfunction
-
 let s:default_layout = { 'down': '~40%' }
 let s:layout_keys = ['window', 'up', 'down', 'left', 'right']
 let s:fzf_go = s:base_dir.'/bin/fzf'
@@ -743,7 +747,7 @@ function! s:cmd(bang, ...) abort
   else
     let prompt = s:shortpath()
   endif
-  call extend(opts.options, ['--prompt', (s:is_win && !&shellslash) ? escape(prompt, '\') : prompt])
+  call extend(opts.options, ['--prompt', prompt])
   call extend(opts.options, args)
   call fzf#run(fzf#wrap('FZF', opts, a:bang))
 endfunction

Seems to work in all cases, while keeping :FZF code simple.

noshellslash
- :FZF
- :FZF C:\Program\ Files\
- :FZF C:/Program\ Files/
shellslash
- :FZF
- :FZF C:/Program\ Files/
- :FZF C:\Program\ Files\

@echo

- Code Reference: https://github.com/junegunn/fzf/pull/896#issuecomment-29620672://github.com/junegunn/fzf/pull/896#issuecomment-296206724 - make 2 s:fzf_shellescape functions - (Windows) substitute \" with \\" to escape the last backslash - (default) regular shellescape - s:evaluate_opts can handle filepaths with backslash as last character - simplify prompt option of :FZF because of s:fzf_shellescape (Windows) - Miscellaneous: - add "@echo off" to the batchfile used to execute fzf

@echo

- Code Reference: junegunn#896 (comment) - make 2 s:fzf_shellescape functions - (Windows) substitute \" with \\" to escape the last backslash - (default) regular shellescape - s:evaluate_opts can handle filepaths with backslash as last character - simplify prompt option of :FZF because of s:fzf_shellescape (Windows) - Miscellaneous: - add "@echo off" to the batchfile used to execute fzf

junegunn · 2017-04-22T02:31:24Z

@janlazo Merged. Thanks for your persistence on this 👍

janlazo · 2017-04-22T04:45:01Z

@junegunn Thank you for your assistance throughout this PR. We can move forward to the docs (fzf.txt) and fzf.vim with minimal changes now because of s:evaluate_opts and list type for options.

junegunn · 2017-04-22T05:58:10Z

@janlazo No problem. Let me update vim doc and README-VIM.md. I think we should update fzf.vim a little later, maybe after a new release of fzf, for the users who don't use the master branch of fzf.

janlazo · 2017-04-29T03:28:38Z

@junegunn I forgot to handle sxe for s:fzf_shellescape. It includes " and '\' so C:\Program "Files\ has to be escaped with ^, which has to escaped with \ as well. s:fzf_shellescape should output ^"C:\Program \^"Files\^\^". shellescape by itself doesn't handle this.

Reference:
(Check the "A better method of quoting" section for escaping the double quotes with ^)
https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/

junegunn · 2017-04-30T02:47:09Z

@janlazo Thanks for the heads-up. Hmm, is it trivial to fix?

janlazo · 2017-04-30T12:09:48Z

Non-trivial if using ^ to escape " because shellescape and escape are out.
Trivial if literal quotes are allowed via \" for the prompt. We can use substitute or escape to replace shellescape in s:fzf_shellescape.

janlazo · 2017-05-07T19:20:04Z

@justinmk Windows TUI for Neovim for 0.2.x works with the vim plugin for fzf (via jobstart) but the scroll up bug is still unresolved. Can neovim/neovim#6383 for :terminal be merged for 0.2.1 so TUI can be tested with :terminal? If not, can the Appveyor build be restarted?

[vim] Use backslash for Windows filepaths

d0c5508

junegunn reviewed Apr 3, 2017

View reviewed changes

[vim] Escape backslash for prompt

22c07f1

- rename s:without_shellescape() to s:fzf_call() - fzf#shellescape() acts as proxy to call s:fzf_call() for shellescape() - double the backslash for the prompt so it doesn't escape double quotes

[vim] use backslash for tempname() in Windows

f65e26e

janlazo commented Apr 10, 2017

View reviewed changes

[vim] shellescape g:fzf_history_dir in Windows

24a7030

janlazo mentioned this pull request Apr 12, 2017

[vim] Start new terminal for nvim in Windows #882

Closed

[vim/neovim] Run fzf command in batchfile (Windows)

889fa9c

- resolves shellescaping issues for filepaths - forward slash or backslash - directory/file with spaces - compatible with :Files - use jobstart for neovim in s:execute (Windows) - only supports builds after PR #6497

[nvim] Remove redundant batchfile

3adea89

janlazo added 2 commits April 16, 2017 23:03

[vim] Overwrite vim plugin with junegunn's gist

05157ed

Gist Link: https://gist.github.com/junegunn/6929954a64bead0e0a2aa1955cf0afd7

[vim] Refactor s:fzf_call and s:shortpath

bca4084

- remove runtime check in s:fzf_call - add fallback implementation of s:fzf_call if not running in Windows - remove s:fzf_ functions in s:shortpath - match the last slash of the prompt based on &shellslash

janlazo force-pushed the vim/launcher branch from a90cf73 to bca4084 Compare April 17, 2017 03:26

[vim] Allow forward slash in prompt of :FZF (Windows)

3254fe9

[vim] Remove s:fzf_ functions not using s:fzf_call

9d8ef7d

junegunn reviewed Apr 19, 2017

View reviewed changes

janlazo force-pushed the vim/launcher branch from 90633f6 to 0ae8efb Compare April 22, 2017 00:56

junegunn added bug neovim windows labels Apr 22, 2017

junegunn merged commit 7a11a06 into junegunn:master Apr 22, 2017

junegunn added a commit that referenced this pull request Apr 27, 2017

Update README-VIM: options as list (#896)

d5e72bf

janlazo mentioned this pull request May 7, 2017

[vim] Replace s:fzf_shellescape and s:shellesc with fzf#shellescape #916

Merged

janlazo mentioned this pull request Jul 9, 2017

[neovim] Fix lcd when fzf job exits on Windows #970

Merged

[vim] Use backslash for Windows filepaths #896

[vim] Use backslash for Windows filepaths #896

Conversation

janlazo commented Apr 2, 2017 • edited Loading

junegunn Apr 3, 2017

Choose a reason for hiding this comment

junegunn Apr 3, 2017

Choose a reason for hiding this comment

janlazo Apr 3, 2017

Choose a reason for hiding this comment

junegunn Apr 3, 2017

Choose a reason for hiding this comment

janlazo Apr 4, 2017

Choose a reason for hiding this comment

junegunn Apr 4, 2017

Choose a reason for hiding this comment

junegunn Apr 3, 2017

Choose a reason for hiding this comment

janlazo commented Apr 5, 2017

junegunn commented Apr 5, 2017

janlazo commented Apr 7, 2017 • edited Loading

janlazo left a comment • edited Loading

Choose a reason for hiding this comment

janlazo commented Apr 14, 2017

junegunn commented Apr 14, 2017

junegunn commented Apr 16, 2017

janlazo commented Apr 16, 2017 • edited Loading

junegunn commented Apr 16, 2017 via email • edited Loading

janlazo commented Apr 16, 2017 • edited Loading

junegunn commented Apr 16, 2017 • edited Loading

justinmk commented Apr 16, 2017

janlazo commented Apr 16, 2017 • edited Loading

justinmk commented Apr 16, 2017 • edited Loading

junegunn commented Apr 17, 2017

junegunn commented Apr 17, 2017

janlazo commented Apr 17, 2017

junegunn commented Apr 17, 2017 • edited Loading

janlazo commented Apr 17, 2017

janlazo commented Apr 17, 2017

junegunn Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

janlazo Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

janlazo commented Apr 19, 2017

justinmk commented Apr 19, 2017

junegunn commented Apr 21, 2017

junegunn commented Apr 22, 2017

janlazo commented Apr 22, 2017

junegunn commented Apr 22, 2017

janlazo commented Apr 29, 2017 • edited Loading

junegunn commented Apr 30, 2017

janlazo commented Apr 30, 2017

janlazo commented May 7, 2017 • edited Loading

janlazo commented Apr 2, 2017 •

edited

Loading

janlazo commented Apr 7, 2017 •

edited

Loading

janlazo left a comment •

edited

Loading

janlazo commented Apr 16, 2017 •

edited

Loading

junegunn commented Apr 16, 2017 via email •

edited

Loading

janlazo commented Apr 16, 2017 •

edited

Loading

junegunn commented Apr 16, 2017 •

edited

Loading

janlazo commented Apr 16, 2017 •

edited

Loading

justinmk commented Apr 16, 2017 •

edited

Loading

junegunn commented Apr 17, 2017 •

edited

Loading

junegunn Apr 19, 2017 •

edited

Loading

janlazo Apr 19, 2017 •

edited

Loading

janlazo commented Apr 29, 2017 •

edited

Loading

janlazo commented May 7, 2017 •

edited

Loading