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

janlazo · 2017-05-07T05:05:51Z

Continuation of #896.

Previous PR cannot escape double quotes properly in s:evaluate_opts because of escaping assumptions made by cmd.exe. Instead on relying shellescape which relies on shellslash, use s:shellesc to escape with \ in Unix and \^ in Windows. For now, only " is escaped this way. The last backslash is escaped the same way as in s:fzf_shellescape.

The outer double quotes are escaped via ^ so cmd.exe knows where the escaping starts and ends. Without it, shell redirection with > fails even if the double quote character is escaped.

> fzf --prompt "C:/Program \" Files (x86)/" "--multi" > tmp.txt
unknown option: >

janlazo · 2017-05-12T04:13:52Z

~~I can't think of better way to escape the backslash for \" so the double backslashes are back.~~
Resolved as of 98c569c

janlazo · 2017-05-14T23:09:17Z

@junegunn Any issues with using s:shellesc instead of shellescape? I can move on to the non-Windows version of s:shellesc if there are no issues.

junegunn · 2017-05-17T15:45:45Z

Yeah, looks like we don't need both s:fzf_shellescape and s:shellesc. But I'm not familiar with Windows and I can't verify the new escaping code for windows, can you show me some examples?

janlazo · 2017-05-18T03:40:25Z

The following examples mainly focus on the escape characters: \, ^, "

^ is to tell cmd.exe to interpret the character literally and not make any assumption yet.
\ is for escaping escape characters, primarily \ and ^
" is for wrapping the argument as a whole string

Metacharacters (:h sxe) run as expected in cmd.exe inside ^"^" so they have to be escaped with ^.
Since ^ is already used for the outer double quotes, inner double quotes need their ^ to be escaped and ^^" doesn't work which leaves \^.
Same as s:fzf_shellescape, the final backslash has to be escaped because \^" is valid and we need the closing ^" to not be escaped with \.

I haven't escaped variables (wrapped in %% such as %PATH%) yet but we can escape that as well if you want.

:: Following comments before each command are the resulting prompt
:: If fzf doesn't run, output the error below the command

:: hello>
> fzf "--prompt" "hello>" "--multi" "--query" "world" > foobar.txt

:: he\^l^^l\\o>
> fzf "--prompt" "he\^l^^l\\o>" "--multi" "--query" "world" > foobar.txt

> fzf "--prompt" "he"llo>" "--multi" "--query" "world" > foobar.txt
The filename, directory name, or volume label syntax is incorrect

:: he"llo> --multi --query world
> fzf "--prompt" "he""llo>" "--multi" "--query" "world" > foobar.txt

> fzf "--prompt" "he\"llo>" "--multi" "--query" "world" > foobar.txt
The filename, directory name, or volume label syntax is incorrect

> fzf "--prompt" "he^"llo>" "--multi" "--query" "world" > foobar.txt
The filename, directory name, or volume label syntax is incorrect

> fzf "--prompt" "he\^"llo>" "--multi" "--query" "world" > foobar.txt
The filename, directory name, or volume label syntax is incorrect

> fzf "--prompt" ^"hello>" "--multi" "--query" "world" > foobar.txt
The filename, directory name, or volume label syntax is incorrect

:: hello>^
> fzf "--prompt" "hello>^" "--multi" "--query" "world" > foobar.txt

:: hello
> fzf "--prompt" ^"hello>^" "--multi" "--query" "world" > foobar.txt

:: hello>
> fzf "--prompt" ^"hello^>^" "--multi" "--query" "world" > foobar.txt

:: hello^>^ --multi --query world
> fzf "--prompt" ^"hel"lo^>^" "--multi" "--query" "world" > foobar.txt

:: hello> --multi --query world
> fzf "--prompt" ^"hel^"lo^>^" "--multi" "--query" "world" > foobar.txt

:: hel"lo>
> fzf "--prompt" ^"hel\^"lo^>^" "--multi" "--query" "world" > foobar.txt

:: hel"lo>" --multi --query world
> fzf "--prompt" ^"hel\^"lo^>\^" "--multi" "--query" "world" > foobar.txt

:: hel"lo>\
> fzf "--prompt" ^"hel\^"lo^>\\^" "--multi" "--query" "world" > foobar.txt

:: dir
> fzf "--prompt" ^"dir && ag fzf %USERPROFILE%\vimfiles ^" "--multi" "--query" "world" > foobar.txt
ERR: Error opening directory --multi : No such file or directory

:: dir && ag fzf C:\Users\janlazo\vimfiles
> fzf "--prompt" ^"dir ^&^& ag fzf %USERPROFILE%\vimfiles ^" "--multi" "--query" "world" > foobar.txt

junegunn · 2017-05-18T11:43:06Z

Thank you very much for the detailed explanation, do you think this is ready for merge?

janlazo · 2017-05-18T12:10:51Z

I'm testing this PR with junegunn/fzf.vim#372 so I want to make sure that both versions of s:shellesc are resolved and any changes to fzf.vim are minimized if using list type for options.
For the Windows version of s:shellesc, it cannot handle something like \\\\\^" which should be escaped and simplified as \^\\^" or \\\^" and finalized in cmd.exe as \". Because anything can happen with ^ and \, and each can break each other or affect the outer ^", it needs more testing.
For the default version, it has to escape the same set of characters to not lose features. The user shouldn't have to use tempname unless necessary (Helptags in Windows) to run the command.

I recommend that we keep both PRs open until you want to make a new release to minimize upgrade issues for the users.

Sample prompt escaping via s:shellesc: \\\\^\^\\"\ becomes ^"\\\^^\^^\\\^"\\^"

janlazo · 2017-05-24T05:06:17Z

Because of s:evaluate_opts, we can test s:shellesc or s:fzf_shellescape for the prompt with vader but I don't see any test cases for the prompt in https://github.com/junegunn/fzf/blob/master/test/fzf.vader. We don't have appveyor setup so we can't automate testing for the Windows version of s:shellesc for FZF and fzf.vim.

Do you have time to setup appveyor so we can convert the test cases in #916 (comment) to vader tests?

junegunn · 2017-05-24T05:21:20Z

Well I have no prior experience with appveyor and honestly I'm not sure if I can find time (and motivation) to look into that any time soon.

junegunn · 2017-05-24T05:31:41Z

Anyway, I like the idea of setting up Vader test cases for the function. And I can live with having the function visible for testing, i.e. fzf#_shellesc, or maybe both fzf#_shellesc_unix and fzf#_shellesc_win)

janlazo · 2017-05-25T01:27:52Z

How about using shell instead?

function! s:shellesc_sh(arg)
  return '"'.substitute(a:arg, '"', '\\"', 'g').'"'
endfunction

function! s:shellesc_cmd(arg)
  let escaped = substitute(a:arg, '[&|<>()@^]', '^&', 'g')
  let escaped = substitute(escaped, '"', '\\^&', 'g')
  let escaped = substitute(escaped, '\\\+\(\\^\)', '\\\\\1', 'g')
  return '^"'.substitute(escaped, '[^\\]\zs\\$', '\\\\', '').'^"'
endfunction

function! fzf#shellesc(arg, ...)
  let shell = exists('a:1') ? a:1 : &shell
  if shell =~# 'cmd.exe$'
    return s:shellesc_cmd(a:arg)
  endif
  return s:shellesc_sh(a:arg)
endfunction

junegunn · 2017-05-25T01:31:08Z

I like that. get(a:000, 0, &shell) should work too, right?

fzf#shellesc(string : {arg}, [string : {shell}]) Escape {arg} based on the shell. If {shell} is omitted, &shell is used. If {shell} contains cmd.exe, then escape {arg} for Window's cmd.exe Else, escape {arg} for sh.

junegunn · 2017-05-25T17:09:32Z

LGTM, ready for merge?

janlazo · 2017-05-26T00:00:13Z

test/fzf.vader

+  AssertEqual '""', fzf#shellesc('', 'sh')
+  AssertEqual '"\"\""', fzf#shellesc('""', 'sh')
+  AssertEqual '"foobar>"', fzf#shellesc('foobar>', 'sh')
+  AssertEqual '"\\""', fzf#shellesc('\"', 'sh')


The inner double quote is unescaped but this case is handled in s:shellesc_cmd.
Need to handle backslashes preceding the escaped double quote.

I don't remember why I added s:shellesc when there's builtin shellescape. What do you think about removing s:shellesc_sh and just using shellescape instead?

EDIT: Maybe I was trying to allow $ENV vars.

Do you remember the issue with shellescape for $ENV vars and tmux integration?

No I was not referring to tmux integration as it's no longer the default (thanks to --height).

Maybe I was confused, I thought something like :call fzf#run({'source': 'echo $PATH'}) wouldn't work if we use builtin shellescape, but it is unaffected.

shellescape in cygwin vim uses single quotes so it doesn't expand env vars.

Say that the user wants to use $VIMRUNTIME as their prompt. What should appear, $VIMRUNTIME or the resolved path after variable expansion?

Yeah, actually this was not properly discussed when we added support for list typed options. Currently call fzf#run({'options': ['--prompt', '$USER']}) gives literal $USER due to the use of builtin shellescape, and I think it makes sense as one can manually expand the var when needed: call fzf#run({'options': ['--prompt', expand('$USER')]}).

I briefly looked at the other places where s:shellesc is used and it appeared that not expanding env vars anymore will not break anything.

This PR becomes Use fzf#shellescape for escaping shell arguments to deprecate s:shellesc and the Window's version of s:fzf_shellescape. To be consistent with the built-in shellescape, I assume Windows-style env vars should be escaped.

junegunn · 2017-05-26T05:47:37Z

plugin/fzf.vim

 endif

+function! s:shellesc_sh(arg)
+  return '"'.substitute(a:arg, '"', '\\"', 'g').'"'


return '"'.escape(a:arg, '\"').'"'

What do you think?

I can't test in a linux box atm but I had no issues with this change in cygwin (using ConEmu, not mintty). It didn't collide with s:escape. Tested with this branch, https://github.com/janlazo/fzf/tree/vim/shellesc_cygwin_sh

If there's no issues on your end (including fzf.vim), I'll commit your suggestion (not the cygwin changes) and update the test cases.

It works well. Please update as above. Thanks.

No, wait, I guess we should probably just use builtin shellescape if it's okay.

janlazo · 2017-05-27T23:02:14Z

The first escape in s:execute to handle %#! breaks FZF because fzf is run in the batchfile.
% are escaped as %% in batchfiles and ^% in the interactive shell.

junegunn · 2017-05-28T04:51:09Z

It's a different issue, right? Escaping of %#! is only needed when we execute the command using :!, so I think the following fix should do.

let escaped = (a:use_height || s:is_win) ? a:command : escape(substitute(a:command, '\n', '\\n', 'g'), '%#!')

- renamed from fzf#shellesc - replaces s:fzf_shellescape and s:shellesc - if shell argument is omitted, use &shell - if &shell is cmd.exe, use s:shellesc_cmd for batchfile escaping Else, use built-in shellescape. - returns a string so &shell treats it as literal string

janlazo · 2017-05-28T21:13:08Z

@junegunn It's done with the test cases updated.

junegunn · 2017-05-29T01:06:14Z

Thank you 👍

janlazo added 2 commits May 7, 2017 00:55

[vim] Replace s:fzf_shellescape with s:shellesc

aa2dcaa

[vim] escape backslash to avoid \"

c9c0317

[vim] escape >

05d72cc

janlazo mentioned this pull request May 13, 2017

Support Windows (requires 0.16.10) junegunn/fzf.vim#372

Closed

45 tasks

janlazo force-pushed the vim/shellesc branch from 70bb59c to 8ec4fcc Compare May 13, 2017 07:59

[vim] handle '>' and " separately

7d2375a

janlazo force-pushed the vim/shellesc branch from 8ec4fcc to 7d2375a Compare May 13, 2017 12:10

[vim] resolve shellxescape in s:shellesc

98c569c

junegunn added the windows label May 18, 2017

[vim] Reduce 1+ backlashes before \^ to \\

3e73d5a

Sample prompt escaping via s:shellesc: \\\\^\^\\"\ becomes ^"\\\^^\^^\\\^"\\^"

[vim] Replace s:shellesc with fzf#shellesc

cc1273b

fzf#shellesc(string : {arg}, [string : {shell}]) Escape {arg} based on the shell. If {shell} is omitted, &shell is used. If {shell} contains cmd.exe, then escape {arg} for Window's cmd.exe Else, escape {arg} for sh.

janlazo changed the title ~~[vim] Replace s:fzf_shellescape with s:shellesc~~ [vim] Replace s:fzf_shellescape with fzf#shellesc May 25, 2017

[vim] Add Vader tests for fzf#shellesc

6f7bf22

janlazo commented May 26, 2017

View reviewed changes

junegunn reviewed May 26, 2017

View reviewed changes

janlazo changed the title ~~[vim] Replace s:fzf_shellescape with fzf#shellesc~~ [vim] Replace s:fzf_shellescape with fzf#shellescape May 28, 2017

janlazo changed the title ~~[vim] Replace s:fzf_shellescape with fzf#shellescape~~ [vim] Replace s:fzf_shellescape and s:shellesc with fzf#shellescape May 28, 2017

[vim] use s:fzf_call for shellescape

7ae359b

junegunn merged commit 8aab0fc into junegunn:master May 29, 2017

janlazo mentioned this pull request Jun 8, 2017

[vader] uncomment test case to escape % in cmd.exe #941

Merged

janlazo mentioned this pull request Aug 14, 2017

[Files] runs in Windows junegunn/fzf.vim#418

Merged

This was referenced Oct 4, 2017

[RFC] win: defaults: 'shellcmdflag', 'shellxquote' neovim/neovim#7343

Closed

Fix ExecCommandWith for cmd.exe in Windows #1072

Closed

janlazo deleted the vim/shellesc branch April 12, 2018 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

janlazo commented May 7, 2017 •

edited

Loading

janlazo commented May 12, 2017 •

edited

Loading

janlazo commented May 14, 2017

junegunn commented May 17, 2017

janlazo commented May 18, 2017 •

edited

Loading

junegunn commented May 18, 2017

janlazo commented May 18, 2017 •

edited

Loading

janlazo commented May 24, 2017

junegunn commented May 24, 2017

junegunn commented May 24, 2017

janlazo commented May 25, 2017

junegunn commented May 25, 2017 •

edited

Loading

junegunn commented May 25, 2017

janlazo May 26, 2017

junegunn May 26, 2017 •

edited

Loading

janlazo May 27, 2017

junegunn May 27, 2017

janlazo May 27, 2017

janlazo May 27, 2017 •

edited

Loading

junegunn May 27, 2017

janlazo May 27, 2017

junegunn May 26, 2017

janlazo May 27, 2017

junegunn May 27, 2017

junegunn May 27, 2017

janlazo commented May 27, 2017

junegunn commented May 28, 2017

janlazo commented May 28, 2017

junegunn commented May 29, 2017

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

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

Conversation

janlazo commented May 7, 2017 • edited Loading

janlazo commented May 12, 2017 • edited Loading

janlazo commented May 14, 2017

junegunn commented May 17, 2017

janlazo commented May 18, 2017 • edited Loading

junegunn commented May 18, 2017

janlazo commented May 18, 2017 • edited Loading

janlazo commented May 24, 2017

junegunn commented May 24, 2017

junegunn commented May 24, 2017

janlazo commented May 25, 2017

junegunn commented May 25, 2017 • edited Loading

junegunn commented May 25, 2017

Choose a reason for hiding this comment

junegunn May 26, 2017 • 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

janlazo May 27, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janlazo commented May 27, 2017

junegunn commented May 28, 2017

janlazo commented May 28, 2017

junegunn commented May 29, 2017

janlazo commented May 7, 2017 •

edited

Loading

janlazo commented May 12, 2017 •

edited

Loading

janlazo commented May 18, 2017 •

edited

Loading

janlazo commented May 18, 2017 •

edited

Loading

junegunn commented May 25, 2017 •

edited

Loading

junegunn May 26, 2017 •

edited

Loading

janlazo May 27, 2017 •

edited

Loading