-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Conversation
plugin/fzf.vim
Outdated
endif | ||
return expand(a:fmt) | ||
endfunction | ||
|
There was a problem hiding this comment.
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
" ...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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']]})
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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()
We can move the shellescaping to It's the simplest workaround with minimal changes to |
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
If the prompt is escaped with single quotes, then the space character has to be escaped via We can still use the batch file to avoid anymore complications. I can't use 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. |
There was a problem hiding this 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
@junegunn Can you test if the latest commit requires no changes with FZF.vim? I only tested |
@janlazo Thanks for the update and sorry for the delay. I'll test it this weekend. |
@janlazo Hi, I finally got a chance to test it on Windows box and I made a few changes to your version.
The code is here: Can you review the changes and update your pull request if it makes sense? |
@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. |
I didn't see your implementation in the gist. I also updated FZF command.
It's there. See s:evaluate_opts.
- junegunn
|
How can I know which options to escape if we use a list instead of a dict? |
I'm not sure if I follow you. We allow I suggest that you apply the entire code in the gist to your branch and see what I have changed. |
FWIW since #882 was created, Windows support for |
@junegunn Did 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? |
AppVeyor build should work. That PR doesn't include the TUI though (that is a separate PR not yet merged).
The AppVeyor zip includes a GUI (just double-click |
@janlazo No, like I mentioned above, you can't properly expand |
@janlazo To elaborate, I'd like to prove that we can implement |
- 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
@junegunn I overwrote the vim plugin with your gist as you requested. I added a few changes for |
Hmm, do we need I like what you suggested in here: https://gist.github.com/junegunn/6929954a64bead0e0a2aa1955cf0afd7#gistcomment-2061358 Having two versions of |
@junegunn I thought you wanted to skip a function call since we're defining the functions upfront. I prefer we stick with |
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]) |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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) ? '\' : '')
@justinmk I want to have a TUI to compare with a GUI when running |
I don't understand. |
@janlazo So shellescape fails to handle trailing 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
|
- 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
@janlazo Merged. Thanks for your persistence on this 👍 |
@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 |
@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 I forgot to handle Reference: |
@janlazo Thanks for the heads-up. Hmm, is it trivial to fix? |
Non-trivial if using |
@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 |
PR for the shellescaping issue I described in #882.
I added wrapper functions to handle filepath functions in vim.
set noshellslash
for backslash in filepathsset shellslash
forshellescape()
to single quote first, and then double quote its output (ex. path --> "'path'")C:\Program Files (x86)\
(no quotes) breaks ons:execute()
so it must be escaped with single/double quotes in cmd.exe and powershellI 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
fors:base_dir
.