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

[vim] Use backslash for Windows filepaths #896

Merged
merged 12 commits into from
Apr 22, 2017

Conversation

janlazo
Copy link
Contributor

@janlazo janlazo commented Apr 2, 2017

  • 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.

plugin/fzf.vim Outdated
endif
return expand(a:fmt)
endfunction

Copy link
Owner

Choose a reason for hiding this comment

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

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

" ...

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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']]})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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.

plugin/fzf.vim Outdated
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')
Copy link
Owner

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

janlazo commented Apr 5, 2017

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
Copy link
Owner

junegunn commented Apr 5, 2017

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
Copy link
Contributor Author

janlazo commented Apr 7, 2017

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.

Copy link
Contributor Author

@janlazo janlazo left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

janlazo commented Apr 14, 2017

@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
Copy link
Owner

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

@junegunn
Copy link
Owner

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

  1. fzf_expand doesn't work with <sfile> (see :help <sfile> for details), so I had to call expand at the top-level
  2. Remove runtime check of s:is_win in s:fzf_* functions by defining different versions of functions depending on s:is_win up-front.
  3. :FZF works regardless of shellslash, but :Files breaks with shellslash since it does not use fzf#shellescape.
  4. 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
Copy link
Contributor Author

janlazo commented Apr 16, 2017

@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
Copy link
Owner

junegunn commented Apr 16, 2017 via email

@janlazo
Copy link
Contributor Author

janlazo commented Apr 16, 2017

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
Copy link
Owner

junegunn commented Apr 16, 2017

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
Copy link
Contributor

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

@janlazo
Copy link
Contributor Author

janlazo commented Apr 16, 2017

@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
Copy link
Contributor

justinmk commented Apr 16, 2017

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
Copy link
Owner

@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
Copy link
Owner

@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.

janlazo added 2 commits April 16, 2017 23:03
- 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
Copy link
Contributor Author

janlazo commented Apr 17, 2017

@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
Copy link
Owner

junegunn commented Apr 17, 2017

@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
Copy link
Contributor Author

janlazo commented Apr 17, 2017

@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
Copy link
Contributor Author

janlazo commented Apr 17, 2017

Testing it in janlazo@9d8ef7d

plugin/fzf.vim Outdated
endif
let opts.options .= ' '.join(args)
call extend(opts.options, ['--prompt', (s:is_win && !&shellslash) ? escape(prompt, '\') : prompt])
Copy link
Owner

@junegunn junegunn Apr 19, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@janlazo janlazo Apr 19, 2017

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

janlazo commented Apr 19, 2017

@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
Copy link
Contributor

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

@junegunn
Copy link
Owner

@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\

janlazo referenced this pull request in janlazo/fzf Apr 22, 2017
- 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
- 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
Copy link
Owner

@janlazo Merged. Thanks for your persistence on this 👍

@janlazo
Copy link
Contributor Author

janlazo commented Apr 22, 2017

@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
Copy link
Owner

@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.

junegunn added a commit that referenced this pull request Apr 27, 2017
@janlazo
Copy link
Contributor Author

janlazo commented Apr 29, 2017

@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
Copy link
Owner

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

@janlazo
Copy link
Contributor Author

janlazo commented Apr 30, 2017

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
Copy link
Contributor Author

janlazo commented May 7, 2017

@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?

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.

3 participants