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

Port fzf#shellescape to fix escaping issues in Windows #676

Merged
merged 1 commit into from
Sep 19, 2017

Conversation

janlazo
Copy link
Contributor

@janlazo janlazo commented Sep 14, 2017

Initial work to fix all escaping issues for Vim and Neovim in Windows. tempname is necessary to bypass Vim's regular shellescaping in both s:system and s:bang like in Vim plugin of fzf because regular Vim seems to assume that the shell options are unchanged in Windows even if all but shell are unset. It's possible to not require batchfiles but it means double-escaping (ie. fzf#shellescape(fzf#shellescape(cmd)) ) for Vim and single-escaping for Neovim with the same values for shell options.

@janlazo janlazo changed the title Port fzf#shellescape Port fzf#shellescape to fix escaping issues in Windows Sep 14, 2017
call writefile(['@echo off', cmd], batchfile)
let cmd = batchfile
endif
execute 'silent %!' cmd
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like s:bang now but %#! are not escaped.

@janlazo
Copy link
Contributor Author

janlazo commented Sep 15, 2017

s:spawn needs tempname for the jobs for the same reasons on system and :!, especially in regular Vim 8.

@janlazo
Copy link
Contributor Author

janlazo commented Sep 16, 2017

@junegunn Do I have to update the python and ruby jobs?

@junegunn
Copy link
Owner

junegunn commented Sep 17, 2017

Do I have to update the python and ruby jobs?

No, we don't want to spend more of our time on those legacy installers.

So this fixes #606, right? It's not clear to me though how using a batchfile instead of a command string suppresses "Enter key" prompt. Can you explain? Thanks.

@janlazo
Copy link
Contributor Author

janlazo commented Sep 17, 2017

This PR targets #635, #668, #539 (shell points to $SHELL), Case 1 in https://github.com/junegunn/vim-plug/wiki/faq#windows-system-error-e484.

For 606, appending silent to the bang command in s:bang (see #606 (comment)) is enough. The trick to get around the prompt works but the external shell remains open. I don't know if this is required for MacVim as well.

@junegunn
Copy link
Owner

junegunn commented Sep 17, 2017

This PR targets #635, #668, #539

Thanks for the clarification. I completely lost track of those issues, largely because I'm not a Windows user. Can you update the commit message with Close #xxx lines so that those issues are automatically closed when this is merged?

I just reviewed the code, although it's unfortunate that we have to introduce extra code branches here and there for Windows, the change looks good to me. Is there anything else you want to address?

@janlazo
Copy link
Contributor Author

janlazo commented Sep 17, 2017

Can you update the commit message with Close #xxx lines so that those issues are automatically closed when this is merged?

I'll rebase when I think it's ready.

Is there anything else you want to address?

I prefer vim-plug to use cmd.exe only in Windows so the side effects of setting SHELL before Vim runs should be reverted for system to work properly. This is for #539 and for handling SHELL in cmd.exe or powershell.

@janlazo
Copy link
Contributor Author

janlazo commented Sep 17, 2017

@junegunn do you want the fix for #606 in here or in a separate PR? Also, do you want to keep the old s:shellesc as fallback instead of shellescape?

@janlazo
Copy link
Contributor Author

janlazo commented Sep 17, 2017

@junegunn Unlike fzf's Vim plugin, shell is not set at the start so s:shellesc may not work as expected. To guarantee that it will work in the batchfile, I think it's better to make it non-configurable. It's the same loophole in fzf#wrap because shell is not passed to fzf#shellescape.

if has('win32')
  function s:shellesc(arg)
    return s:shellesc_cmd(a:arg)
  endfunction
else
  function s:shellesc(arg)
    return shellescape(a:arg)
  endfunction
endif

Close junegunn#635
Close junegunn#668
Close junegunn#539

Use a temporary batchfile for :!, system(), and jobs and run it in cmd.exe.
This bypasses Vim/Neovim issues in Windows and reduces the need to set more options.
Also, s:shellesc_cmd works in a batchfile only.

Set shellredir for system() in Windows
$SHELL sets the default value of 'shell' (see :h 'shell').
This affects shellredir but cmd.exe requires '>%s 2>&1'.
@junegunn
Copy link
Owner

do you want the fix for #606 in here or in a separate PR?

As long as it's on a separate commit, I'm fine with either :) By the way, we are going to apply silent only on GVim, right? As you did in #606 (comment). On terminal Vim, it messes up the screen and requires additional :redraw!.

do you want to keep the old s:shellesc as fallback instead of shellescape?

No, you properly handled escaping issues on Windows, so I guess that crude solution is no longer needed.

@junegunn
Copy link
Owner

Reviewed the code and it looks good to me. Thanks, again. I see that you rebased your commits, ready for merge?

@janlazo
Copy link
Contributor Author

janlazo commented Sep 19, 2017

As long as it's on a separate commit, I'm fine with either :) By the way, we are going to apply silent only on GVim, right?

Yes. I'll open a separate PR for #606 after the merge.

ready for merge?

Yes for Windows unless you want to handle msysgit or cygwin as well.

@junegunn junegunn merged commit 05c8983 into junegunn:master Sep 19, 2017
@junegunn
Copy link
Owner

Thanks, I really appreciate your contribution.

@janlazo
Copy link
Contributor Author

janlazo commented Jan 5, 2018

@junegunn Do you know why some s:system calls use s:esc for tags, branches instead of s:shellesc? Backslash escaping doesn't work in cmd.exe so any argument with spaces must be quoted. In an interactive session, cmd.exe autocompletes paths with spaces by wrapping it in double quotes. Powershell does the same but with single quotes.

I traced it to eb47183 from the blame. I don't use tags or branches so I didn't notice this while working on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants