fix: make_filter_cmd for powershell as shell #16271

3N4N · 2021-11-09T14:30:08Z

Closes #15913 which prevented nvim from creating out temp file for :!cmd commands over a range of lines. The reason is because PowerShell doesn't use < as the stdin redirection. Instead, it uses a parameter -RedirectStandardInput. The rest is explained in the bug report.

justinmk · 2022-04-24T04:05:31Z

@3N4N thanks for looking into this! To exercise this, it should be fairly straightforward to write a test that checks Nvim's :verbose log. So the test can check the actual shell command that is produced.

Example:

neovim/test/functional/vimscript/system_spec.lua

Line 220 in f876a0e

screen:expect{any=[[Executing command: "'fake_shell' 'cmdflag' '"echo hi"'"]]}

3N4N · 2022-04-25T15:56:40Z

@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'"]]}

justinmk · 2022-05-03T13:32:00Z

I plan to write a test for this unless someone else gets to it first. Blocked until then.

3N4N · 2022-06-12T21:19:33Z

@justinmk I wrote a test. Please see if it'll do. And I'm the one whom you answered about testing with screen:expect in Matrix chat.

test/functional/vimscript/system_spec.lua

3N4N · 2022-06-13T15:50:18Z

@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;
#endif

Imitating the code-block for is_fish_shell (pre-existing code, not mine), if in a Unix machine, I blindly assumed the shell isn't powershell. But powershell, in the form of pwsh, is available for Unix machines, too. I should remove that if-unix block, right?

justinmk · 2022-06-16T01:59:34Z

This is looking great, thank you! Just a few minor things then we can merge it.

But powershell, in the form of pwsh, is available for Unix machines, too. I should remove that if-unix block, right?

yes

test/functional/vimscript/system_spec.lua

3N4N · 2022-06-20T04:54:41Z

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?

clason · 2022-06-20T06:20:52Z

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 release-0.7, commits are cherry-picked into this branch after merging to master if they are applicable to a point release.)

3N4N · 2022-06-20T16:25:52Z

Turns out the shellpipe needs to use Out-File whereas shellredir needs to use -RedirectStandardOutput. I updated the tests. The errors should be fixed now.

I also updated the make_filter_cmd function to accommodate pwsh in Linux. Working on both my personal Windows and Ubuntu installations. Let's see how the tests turn out.

3N4N · 2022-06-21T02:06:09Z

@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

    
           it('completions can be listed via getcompletion()', function() 
        
             clear() 
        
             eq('nvim', getcompletion('nvim', 'checkhealth')[1]) 
        
             eq('provider', getcompletion('prov', 'checkhealth')[1]) 
        
             eq('vim.lsp', getcompletion('vim.ls', 'checkhealth')[1]) 
        
             neq('vim', getcompletion('^vim', 'checkhealth')[1])  -- should not complete vim.health 
        
           end)

src/nvim/ex_cmds.c

3N4N · 2022-06-21T09:58:20Z

Okay, all tests green. That should be it.

3N4N · 2022-06-22T03:10:45Z

Update?

test/functional/vimscript/system_spec.lua

3N4N · 2022-06-22T11:03:55Z

macos-10.15 ran okay this time. And by the way, on my fork, CI errored out (timeout error) for macos-10.0. Don't know what was going on.

clason · 2022-06-22T11:04:55Z

Usual flake on those Github runners. Just ignore it and re-run if needed.

test/functional/ex_cmds/shell_filter_spec.lua

justinmk · 2022-06-25T13:33:58Z

pushed an update. got a bit carried away with cleaning up some old debt in the tests, but the relevant change is:

the test now runs on all systems by using a fake pwsh-test binary. Might be useful in other tests too, but I'll save that for later

I just think this is unrelated to system()

@zeertzjq system() and :! largely share the same codepath, especially describe('executes shell function') tests in system_spec are testing largely the same codepaths as one would test for :!. I've added a describe('shell :!',) section in system_spec.

Should we rename system_spec to system_shell_spec ?

justinmk · 2022-06-25T13:35:44Z

src/nvim/ex_cmds.c

@@ -1590,7 +1594,10 @@ char *make_filter_cmd(char *cmd, char *itmp, char *otmp)
 #if defined(UNIX)


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

justinmk · 2022-06-25T14:56:49Z

runtime/doc/options.txt

 		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'


If > doesn't work with powershell then why is it used for 'shellpipe'?

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.

justinmk · 2022-06-25T14:57:41Z

runtime/doc/options.txt

 		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'


Since exit $LastExitCode was removed, will Nvim correctly get the exit code after this change?

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 0

So, ; 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.

github-actions · 2022-06-25T15:53:16Z

Backport failed for release-0.7, because it was unable to cherry-pick the commit(s).

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

3N4N · 2022-06-25T15:56:54Z

Holy. Thanks for the merge. And thanks for reviewing my commit, @justinmk @zeertzjq

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 `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.

zeertzjq added environment user system environment (terminal, shell, tmux) platform:windows labels Nov 9, 2021

3N4N changed the title ~~Fix #15913: do_filter for powershell as shell~~ Fix #15913: make_filter_cmd for powershell as shell Nov 11, 2021

3N4N mentioned this pull request Apr 22, 2022

:! (bang command) problems using powershell on Windows #18014

Closed

justinmk reviewed Jun 13, 2022

View reviewed changes

test/functional/vimscript/system_spec.lua Outdated Show resolved Hide resolved

justinmk reviewed Jun 13, 2022

View reviewed changes

test/functional/vimscript/system_spec.lua Outdated Show resolved Hide resolved

justinmk reviewed Jun 13, 2022

View reviewed changes

test/functional/vimscript/system_spec.lua Outdated Show resolved Hide resolved

3N4N requested a review from justinmk June 15, 2022 19:05

justinmk reviewed Jun 16, 2022

View reviewed changes

test/functional/vimscript/system_spec.lua Outdated Show resolved Hide resolved

3N4N requested a review from justinmk June 17, 2022 03:45

zeertzjq reviewed Jun 21, 2022

View reviewed changes

src/nvim/ex_cmds.c Outdated Show resolved Hide resolved

zeertzjq added backport release-0.7 and removed backport release-0.7 labels Jun 21, 2022

3N4N requested a review from zeertzjq June 21, 2022 14:32

zeertzjq reviewed Jun 22, 2022

View reviewed changes

test/functional/vimscript/system_spec.lua Outdated Show resolved Hide resolved

3N4N requested a review from zeertzjq June 22, 2022 12:24

justinmk reviewed Jun 22, 2022

View reviewed changes

test/functional/ex_cmds/shell_filter_spec.lua Outdated Show resolved Hide resolved

clason added this to the 0.7.1 milestone Jun 25, 2022

clason added the backport release-0.7 label Jun 25, 2022

justinmk reviewed Jun 25, 2022

View reviewed changes

justinmk changed the title ~~Fix #15913: make_filter_cmd for powershell as shell~~ fix: make_filter_cmd for powershell as shell Jun 25, 2022

refactor(tests): introduce testprg()

f977f94

Also: - Add a describe('shell :!') section to system_spec. - Make the test for #16271 work on systems without powershell.

clason merged commit 9f59278 into neovim:master Jun 25, 2022

github-actions bot removed the request for review from zeertzjq June 25, 2022 15:53

clason pushed a commit that referenced this pull request Jun 25, 2022

refactor(tests): introduce testprg()

563d223

Also: - Add a describe('shell :!') section to system_spec. - Make the test for #16271 work on systems without powershell.

clason pushed a commit that referenced this pull request Jun 25, 2022

refactor(tests): introduce testprg()

71e2c49

Also: - Add a describe('shell :!') section to system_spec. - Make the test for #16271 work on systems without powershell.

clason added a commit that referenced this pull request Jun 25, 2022

Merge pull request #19088 from neovim/fix-powershell

b477afa

[Backport release-0.7] fix: make_filter_cmd for powershell as shell (#16271)

3N4N deleted the fix-15913 branch June 26, 2022 07:00

kraftwerk28 pushed a commit to kraftwerk28/neovim that referenced this pull request Jul 6, 2022

refactor(tests): introduce testprg()

d6e691e

Also: - Add a describe('shell :!') section to system_spec. - Make the test for neovim#16271 work on systems without powershell.

3N4N mentioned this pull request Jul 11, 2022

fix (#19250): make_filter_cmd for powershell commands with arguments #19268

Merged

3N4N mentioned this pull request Jul 21, 2022

fix: powershell redirection for filtered bang commands #19438

Merged

3 tasks

smjonas pushed a commit to smjonas/neovim that referenced this pull request Dec 31, 2022

refactor(tests): introduce testprg()

8c6b4ea

Also: - Add a describe('shell :!') section to system_spec. - Make the test for neovim#16271 work on systems without powershell.

justinmk mentioned this pull request Jan 9, 2023

grep/make: remove 'shellpipe', always read from pipe for 'makeef' #20890

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make_filter_cmd for powershell as shell #16271

fix: make_filter_cmd for powershell as shell #16271

3N4N commented Nov 9, 2021 •

edited

Loading

justinmk commented Apr 24, 2022

3N4N commented Apr 25, 2022 •

edited

Loading

justinmk commented May 3, 2022

3N4N commented Jun 12, 2022

3N4N commented Jun 13, 2022 •

edited

Loading

justinmk commented Jun 16, 2022

3N4N commented Jun 20, 2022

clason commented Jun 20, 2022

3N4N commented Jun 20, 2022 •

edited

Loading

3N4N commented Jun 21, 2022 •

edited

Loading

3N4N commented Jun 21, 2022

3N4N commented Jun 22, 2022

3N4N commented Jun 22, 2022

clason commented Jun 22, 2022

justinmk commented Jun 25, 2022 •

edited

Loading

justinmk Jun 25, 2022

justinmk Jun 25, 2022

3N4N Jun 25, 2022

justinmk Jun 25, 2022

3N4N Jun 25, 2022

github-actions bot commented Jun 25, 2022

3N4N commented Jun 25, 2022

		@@ -1590,7 +1594,10 @@ char make_filter_cmd(char cmd, char itmp, char otmp)
		#if defined(UNIX)

fix: make_filter_cmd for powershell as shell #16271

fix: make_filter_cmd for powershell as shell #16271

Conversation

3N4N commented Nov 9, 2021 • edited Loading

justinmk commented Apr 24, 2022

3N4N commented Apr 25, 2022 • edited Loading

justinmk commented May 3, 2022

3N4N commented Jun 12, 2022

3N4N commented Jun 13, 2022 • edited Loading

justinmk commented Jun 16, 2022

3N4N commented Jun 20, 2022

clason commented Jun 20, 2022

3N4N commented Jun 20, 2022 • edited Loading

3N4N commented Jun 21, 2022 • edited Loading

3N4N commented Jun 21, 2022

3N4N commented Jun 22, 2022

3N4N commented Jun 22, 2022

clason commented Jun 22, 2022

justinmk commented Jun 25, 2022 • edited Loading

justinmk Jun 25, 2022

Choose a reason for hiding this comment

justinmk Jun 25, 2022

Choose a reason for hiding this comment

3N4N Jun 25, 2022

Choose a reason for hiding this comment

justinmk Jun 25, 2022

Choose a reason for hiding this comment

3N4N Jun 25, 2022

Choose a reason for hiding this comment

github-actions bot commented Jun 25, 2022

3N4N commented Jun 25, 2022

3N4N commented Nov 9, 2021 •

edited

Loading

3N4N commented Apr 25, 2022 •

edited

Loading

3N4N commented Jun 13, 2022 •

edited

Loading

3N4N commented Jun 20, 2022 •

edited

Loading

3N4N commented Jun 21, 2022 •

edited

Loading

justinmk commented Jun 25, 2022 •

edited

Loading