fix: make_filter_cmd for powershell as shell #16271
Conversation
|
@3N4N thanks for looking into this! To exercise this, it should be fairly straightforward to write a test that checks Nvim's Example: |
|
@justinmk I'm sorry. I have no idea how to write tests. Plus, I'm on Windows, and I don't know how to run tests here. I built neovim with Mingw-w64. EDIT: I have figured out, somewhat, how tests work. But then I realized I don't know what you want. Is it something like the following? screen:expect{any=[[Executing command: "'powershell' '-NoLogo' '-NoProfile' '-ExecutionPolicy' 'RemoteSigned' '-Command' '[Console]::InputEncoding=[Console]::OutputEncoding=[System.Text.Encoding]::UTF8;' 'echo hi'"]]} |
|
I plan to write a test for this unless someone else gets to it first. Blocked until then. |
|
@justinmk I wrote a test. Please see if it'll do. And I'm the one whom you answered about testing with |
|
@justinmk I resolved the reviews. I ended up resizing the screen to 500 and checking for the args with pattern matching (".*"). Another thing. Check this code block. char *make_filter_cmd(char *cmd, char *itmp, char *otmp)
{
bool is_fish_shell =
#if defined(UNIX)
STRNCMP(invocation_path_tail(p_sh, NULL), "fish", 4) == 0;
#else
false;
#endif
bool is_pwsh =
#if defined(UNIX)
false;
#else
STRNCMP(invocation_path_tail(p_sh, NULL), "pwsh", 4) == 0
|| STRNCMP(invocation_path_tail(p_sh, NULL), "powershell", 10) == 0;
#endifImitating the code-block for |
|
This is looking great, thank you! Just a few minor things then we can merge it.
yes |
|
Any update on this? Should I look into why these tests are failing? It might be because this branch is built atop an older commit. Should I rebase on master or release-0.7? |
|
Yes, test failures are relevant and need to be looked into. (I don't know anything about Windows, so can't help with the actual changes.) You can rebase on master, if you want, but it should not be necessary. (Don't rebase on |
|
Turns out the I also updated the |
|
@clason would you check this error, please? It's bugging out here, but my commit should have nothing to do with this. Also, it's not Windows-related/specific. neovim/test/functional/plugin/health_spec.lua Lines 35 to 41 in 6d52a29 |
|
Okay, all tests green. That should be it. |
|
Update? |
|
|
|
Usual flake on those Github runners. Just ignore it and re-run if needed. |
|
pushed an update. got a bit carried away with cleaning up some old debt in the tests, but the relevant change is:
@zeertzjq Should we rename |
| @@ -1590,7 +1594,10 @@ char *make_filter_cmd(char *cmd, char *itmp, char *otmp) | |||
| #if defined(UNIX) | |||
There was a problem hiding this comment.
TODO: there's a fair amount of duplicate code in these defines
Problem:
Nvim fails to create tempfile "…/nvim6UJx04/7" when 'shell' is set to
pwsh (PowerShell Core). This breaks filtered shell commands ":{range}!".
With shell set to cmd, it works.
Solution:
PowerShell doesn't use "<" for stdin redirection. Instead, use
"-RedirectStandardInput".
Closes #15913
| let &shellcmdflag = '-NoLogo -NoProfile -ExecutionPolicy RemoteSigned -Command [Console]::InputEncoding=[Console]::OutputEncoding=[System.Text.Encoding]::UTF8;' | ||
| let &shellredir = '2>&1 | Out-File -Encoding UTF8 %s; exit $LastExitCode' | ||
| let &shellredir = '-RedirectStandardOutput %s -NoNewWindow -Wait' | ||
| let &shellpipe = '2>&1 | Out-File -Encoding UTF8 %s; exit $LastExitCode' |
There was a problem hiding this comment.
If > doesn't work with powershell then why is it used for 'shellpipe'?
There was a problem hiding this comment.
There are two cases:
Start-Process sort -RedirectStandardInput filein.txt -RedirectStandardOutput fileout.txt: requires redirection.Start-Process dir: does not require redirection
In the second case, if the command dir does not exist, that is, if the user provides a wrong command, nvim needs to write the error to a file (at least that's my understanding, from tests like this). Now, pwsh doesn't let users direct both std in and err to the same file, that is, it doesn't let us use something like 2>&1. If you give the same file to both -RedirectStandardOutput and RedirectStandardError, pwsh will give error. Stackoverflow suggested simply using Out-File to redirect everything to a file.
But Out-File doesn't work with case 1. My understanding is that, this use case is why vim has two different options for redirection and piping. But I don't have any confirmation except that it seemed to work.
| let &shellcmdflag = '-NoLogo -NoProfile -ExecutionPolicy RemoteSigned -Command [Console]::InputEncoding=[Console]::OutputEncoding=[System.Text.Encoding]::UTF8;' | ||
| let &shellredir = '2>&1 | Out-File -Encoding UTF8 %s; exit $LastExitCode' | ||
| let &shellredir = '-RedirectStandardOutput %s -NoNewWindow -Wait' |
There was a problem hiding this comment.
Since exit $LastExitCode was removed, will Nvim correctly get the exit code after this change?
There was a problem hiding this comment.
I don't know that. But running commands with wrong arguments in cmd directly gives the following results:
C:\Users\ACER> pwsh -c "sldkfj 2>&1 | Out-File -Encoding UTF8 err.txt; exit $LastExitCode"
sldkfj: The term 'sldkfj' is not recognized as a name of a cmdlet, function, script file, or executable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
C:\Users\ACER> echo Exit Code is %errorlevel%
Exit Code is 0
C:\Users\ACER> pwsh -c "sldkfj 2>&1 | Out-File -Encoding UTF8 err.txt"
sldkfj: The term 'sldkfj' is not recognized as a name of a cmdlet, function, script file, or executable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
C:\Users\ACER> echo Exit Code is %errorlevel%
Exit Code is 1
C:\Users\ACER> pwsh -c "Start-Process sort -RedirectStandardInput sldkfj.txt -RedirectStandardOutput out.txt -NoNewWindow -Wait"
Start-Process: This command cannot be run because either the parameter "RedirectStandardInput 'C:\Users\ACER\sldkfj.txt'" has a value that is not valid or cannot be used with this command. Give a valid input and Run your command again.
C:\Users\ACER> echo Exit Code is %errorlevel%
Exit Code is 1
C:\Users\ACER> pwsh -c "Start-Process sort -RedirectStandardInput sldkfj.txt -RedirectStandardOutput out.txt -NoNewWindow -Wait; exit $LastExitCode"
Start-Process: This command cannot be run because either the parameter "RedirectStandardInput 'C:\Users\ACER\sldkfj.txt'" has a value that is not valid or cannot be used with this command. Give a valid input and Run your command again.
C:\Users\ACER> echo Exit Code is %errorlevel%
Exit Code is 0So, ; exit $LastExitCode spends (?) the exit code? I don't know how Nvim handles that. I'm sure you can understand whether ; exit $LastExitCode is needed or not from the above examples.
Also:
- Add a describe('shell :!') section to system_spec.
- Make the test for #16271 work on systems without powershell.
|
Backport failed for Please cherry-pick the changes locally. git fetch origin release-0.7
git worktree add -d .worktree/backport-16271-to-release-0.7 origin/release-0.7
cd .worktree/backport-16271-to-release-0.7
git checkout -b backport-16271-to-release-0.7
ancref=$(git merge-base b7084fef4c850d0352488b14dcff0f36a7e75e1c f977f9445f7689fc32a136108ff92b3c2137968c)
git cherry-pick -x $ancref..f977f9445f7689fc32a136108ff92b3c2137968c |
Also:
- Add a describe('shell :!') section to system_spec.
- Make the test for #16271 work on systems without powershell.
Also:
- Add a describe('shell :!') section to system_spec.
- Make the test for #16271 work on systems without powershell.
[Backport release-0.7] fix: make_filter_cmd for powershell as shell (#16271)
Also:
- Add a describe('shell :!') section to system_spec.
- Make the test for neovim#16271 work on systems without powershell.
Reverts #16271 Fixs #15913 Problem: Since #16271, `make_filter_cmd` uses AB61 `Start-Process` cmdlet to execute the user provided shell command for `:%!`. `Start-Process` requires the command to be split into the shell command and its arguments. This was implemented in #19268 by parsing (splitting the user-provided command at the first space) which didn't handle cases such as -- - commands with escaped space in their filepath - quoted commands with space in their filepath Solution: Use piping. The total shell command formats (excluding noise of unimportant parameters): 1. Before #16271 ```powershell pwsh -C "(shell_cmd) < tmp.in | 2>&1 Out-File -Encoding UTF8 <tmp.out>" # not how powershell commands work ``` 2. Since #16271 ```powershell pwsh -C "Start-Process shell_cmd -RedirectStandardInput <tmp.in> -RedirectStandardOutput <tmp.out>" # doesn't handle executable path with space in it # doesn't write error to <tmp.out> ``` 3. This PR ```powershell pwsh -C "& { Get-Content <tmp.in> | & 'path\with space\to\shell_cmd.exe' arg1 arg2 } 2>&1 | Out-File -Encoding UTF8 <tmp.out>" # also works with forward slash in the filepath # also works with double quotes around shell command ``` After this PR, the user can use the following formats: :%!c:\Program` Files\Git\usr\bin\sort.exe :%!'c:\Program Files\Git\usr\bin\sort.exe' :%!"c:\Program Files\Git\usr\bin\sort.exe" :%!"c:\Program` Files\Git\usr\bin\sort.exe" They can even chain different commands: :%!"c:\Program` Files\Git\usr\bin\sort.exe" | sort.exe -r But if they want to call a stringed executable path, they have to provide the Invoke-Command operator (&). In fact, the first stringed executable path also needs this & operator, but this PR adds that behind the scene. :%!"c:\Program` Files\Git\usr\bin\sort.exe" | sort.exe -r | & 'c:\Program Files\Git\usr\bin\sort.exe' ## What this PR solves - Having to parse the user-provided bang ex-command (for splitting into shell cmd and its args). - Removes a lot of human-unreadable `#ifdef` blocks. - Accepting escaped spaces in executable path. - Accepting quoted string of executable path. - Redirects error and exception to tmp.out (exception for when `wrong_cmd.exe not found`) ## What this PR doesn't solve - Handling wrongly escaped path to executable, which the user may pass because of cmdline tab-completion. #18592 ## Edge cases - (Not handled) If the user themself provides the `&` sign (means `call this.exe` in powershell) - (Not handled) Use `-Encoding utf8` parameter for `Get-Content`? - (Handled) Doesn't write to tmp.out if shell command is not found. - fix: use anonymous function (`{wrong_cmd.exe}`). ## Changes other than `make_filter_cmd()` function - Encoding for piping to external executables. See BOM-less UTF8: PowerShell/PowerShell#4681
Also:
- Add a describe('shell :!') section to system_spec.
- Make the test for neovim#16271 work on systems without powershell.
Closes #15913 which prevented nvim from creating out temp file for
:!cmdcommands over a range of lines. The reason is because PowerShell doesn't use<as the stdin redirecti 8000 on. Instead, it uses a parameter-RedirectStandardInput. The rest is explained in the bug report.