- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2k
Don't use chcp. #913
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
Don't use chcp. #913
Conversation
| call writefile(s:wrap_cmds(a:cmd), batchfile) | ||
| let cmd = plug#shellescape(batchfile, {'shell': &shell, 'script': 1}) | ||
| if &shell =~# 'powershell\.exe$' | ||
| let cmd = plug#shellescape(batchfile, {'shell': &shell, 'script': 0}) | 
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 should have been 0 to begin with because the batchfile will run on the shell, not inside another batchfile.
| if shell =~# 'cmd\.exe' | ||
| return s:shellesc_cmd(a:arg, script) | ||
| elseif shell =~# 'powershell\.exe$' || shell =~# 'pwsh$' | ||
| elseif shell =~# 'powershell\.exe' || shell =~# 'pwsh$' | 
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.
$  was a premature optimization because 8.0.1455 and relatex 8.1.x patches were not considered.
        
          
                plug.vim
              
                Outdated
          
        
      | if !has('nvim') | ||
| \ && (has('win32') || has('win32unix')) | ||
| \ && (!has('multi-byte') || &encoding !=# 'utf-8') | ||
| return s:err('Only Vim with +multi_byte feature is supported. Add `set encoding=utf-8` in your vimrc before running `plug#begin()`.') | 
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.
@mattn Is this a reasonable restriction to guarantee that the batchfile has utf-8 encoding?
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.
Why do you check support of encoding=utf-8 ?
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 want to guarantee that writefile works with unicode and batchfile has unicode encoding. I don't know enough about the Vim internals to know if this is needed or is insufficent so I'm asking. Maybe writefile with b flag is enough?
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.
The batch file must be written in code page NOT unicode. So you can use iconv().
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.
let cp = libcallnr('kernel32.dll', 'GetACP', 0)
let text = iconv(text, &encoding, printf('cp%d', cp))
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.
iconv  replaces unicode characters with ?. I'd rather use powershell if the batchfile cannot be executed with unicode characters and non-unicode codepage.
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.
Why you have to handle unicode characters?
| This is my suggestion. https://gist.github.com/mattn/9182880f9080d728338bfea9ef1edccd | 
| Batchfile is required on Vim to support multiple shells while using cmd.exe syntax and not changing the user's shell settings, which depend on the editor and OS. I will reconsider if #594 is finished before removing the batchfile code because Vim's  On Neovim, only  | 
aec464e    to
    c86659e      
    Compare
  
    73407c6    to
    2de5b4a      
    Compare
  
            
          
                plug.vim
              
                Outdated
          
        
      | endif | ||
| if !has('nvim') | ||
| \ && (has('win32') || has('win32unix')) | ||
| \ && (!has('multi-byte') || !has('iconv')) | 
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.
s/multi-byte/multi_byte/
| LGTM | 
b4187cd    to
    ef84ae5      
    Compare
  
    Changing chcp breaks cmd.exe if switching from multi-byte to 65001. cmd.exe depends on codepage to parse batchfile so batchfile cannot have unicode characters. Target powershell if unicode support is required. User should fix their terminal font to display unicode characters. Terminal programs can only use Wide String APIs. For Vim, this requires +multi_byte feature and `set encoding=utf-8` in the user's vimrc. Neovim always defaults to `set encoding=utf-8`. https://dev.to/mattn/please-stop-hack-chcp-65001-27db
ef84ae5    to
    4f18713      
    Compare
  
    | Works with my config on a Windows VM. Tested with cmd.exe, powershell.exe, and bash.exe. Tested only with codepage 1252 (English). | 
| @mattn Thanks for the help. | 
| @junegunn junegunn/fzf@f68017d, port of this PR, breaks filtering for  | 
De-quote only if first and last character of &shell are double-quotes. Avoid string escape issues when passing escaped '&shell' values.
User should fix their terminal font to display unicode characters.
Terminal programs can only use Wide String APIs.
For Vim, this requires +multi_byte feature
andset encoding=utf-8in the user's vimrc.
iconv()andlibcallnrto get the current codepage (can this be cached?).Neovim always defaults to
set encoding=utf-8.vim-plug runs commands in a batchfile with utf-8 characters.On terminal Vim, run the batchfile in a separate minimized console
to not break the terminal console.
https://dev.to/mattn/please-stop-hack-chcp-65001-27db
cc @mattn @junegunn
Close #908