8000 fix: powershell redirection for filtered bang commands by 3N4N · Pull Request #19438 · neovim/neovim · GitHub
[go: up one dir, main page]
Skip to content

fix: powershell redirection for filtered bang commands #19438

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 1, 2022
Merged

fix: powershell redirection for filtered bang commands #19438

merged 2 commits into from
Oct 1, 2022

Conversation

3N4N
Copy link
Contributor
@3N4N 3N4N commented Jul 19, 2022 •
edited
Loading

Revert #16271 and fix #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 fix: make_filter_cmd for powershell as shell #16271 
    pwsh -C "(shell_cmd) < tmp.in | 2>&1 Out-File -Encoding UTF8 <tmp.out>"
# not how powershell commands work
  2. Since fix: make_filter_cmd for powershell as shell #16271 
    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 
    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

Edge cases

  • If the user themself provides the & sign (means call this.exe in
    powershell) (I don't plan to handle it)
  • Use -Encoding utf8 parameter for Get-Content? (need to know about
    encoding, which I don't)
  • Doesn't write to tmp.out if shell command is not found (I have a fix:
    using anonymous function ({wrong_cmd.exe}))

Changes other than make_filter_cmd() function

  • Encoding (see BOM-less UTF8) for piping to external executables

@3N4N 3N4N marked this pull request as ready for review July 19, 2022 20:36
@3N4N 3N4N marked this pull request as draft July 20, 2022 15:13
@3N4N 3N4N changed the title fix: make_filter_cmd for :! powershell fix: pwsh redirection for filtered bang commands Jul 20, 2022
@3N4N 3N4N changed the title fix: pwsh redirection for filtered bang commands fix: powershell redirection for filtered bang commands Jul 20, 2022
@3N4N 3N4N marked this pull request as ready for review July 20, 2022 15:54
@3N4N 3N4N marked this pull request as draft July 20, 2022 16:09
@3N4N 3N4N marked this pull request as ready for review July 20, 2022 16:31
3N4N
3N4N commented Jul 24, 2022
View reviewed changes
@zeertzjq

This comment was marked as outdated.

Sign in to view
3N4N added 2 commits September 30, 2022 21:07
@3N4N
fix: make_filter_cmd for :! powershell
b4d42bb 
Problem:
`Start-Process` requires the command to be split into the shell
command and its arguments. Previously it was done by parsing, which
didn't handle cases such as
  - commands with escaped space in their filepath
  - quoted commands with space in their filepath

Solution:
Use
  - `pwsh -Command` instead of `Start-Process`
  - `Get-Content` instead of `-RedirectStandardInput`
  - `Out-File` instead of `-RedirectStandardOutput`
@3N4N
fix: :! pwsh redirection for command not found
f2482b3 
Problem:
If the shell command passed to the filtered bang command isn't found,
the error isn't redirected to the temp.out file when shell is set to
powershell.

Solution: Use anonymous function with Invoke-Command operator (&).
@justinmk justinmk merged commit 01721aa into neovim:master Oct 1, 2022
@justinmk
Copy link
Member
justinmk commented Oct 1, 2022

@3N4N thank you for your tenacity here. Do we need a doc update that mentions these points:

The total shell command formats (excluding noise of unimportant parameters):...

After this PR, the user can use the following formats:...

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.

?

@3N4N
Copy link
Contributor Author
3N4N commented Oct 2, 2022 •
edited
Loading

@justinmk Other than the requirement of & before stringed executable path, users don't need to know about the others: it should just work any and every way they try.

We can document & like this below. But the example command is way too long. Can't think of a shorter command.

diff --git a/runtime/doc/various.txt b/runtime/doc/various.txt
index cae9c760308a..c2df8c914b25 100644
--- a/runtime/doc/various.txt
+++ b/runtime/doc/various.txt
@@ -259,6 +259,10 @@ g8			Print the hex values of the bytes used in the
 			or stderr, the streams are closed immediately. |E5677|
 			Use |jobstart()| instead. >
 				:call jobstart('foo', {'detach':1})
+
+			For powershell
8000
, chaining a stringed executable path
+			requires using the call operator (&).
+				:!Write-Output "1`n2" | & "C:\Windows\System32\sort.exe" /r
 <
 							*E34*
 			Any "!" in {cmd} is replaced with the previous

@justinmk
Copy link
Member
justinmk commented Oct 2, 2022

Let's start with that. We want to write these things down, else they get lost in time.

@3N4N
Copy link
Contributor Author
3N4N commented Oct 2, 2022

Let's start with that.

Do you mean push a new commit here, or a new PR?

@justinmk
Copy link
Member
justinmk commented Oct 2, 2022

new PR

@3N4N 3N4N mentioned this pull request Oct 3, 2022
Merged
justinmk pushed a commit that referenced 8000 this pull request Oct 3, 2022
@3N4N
docs(shell): mention "&" for piping with powershell #20459
51d379d 
New behaviour since PR #19438
@justinmk justinmk mentioned this pull request Oct 3, 2022
Open
@github-actions github-actions bot mentioned this pull request Oct 3, 2022
Merged
@github-actions
Copy link
Contributor
github-actions bot commented Oct 3, 2022

Successfully created backport PR #20469 for release-0.8.

@3N4N 3N4N mentioned this pull request Oct 7, 2022
Merged
@sebshader
Copy link
sebshader commented Mar 21, 2024 •
edited
Loading

one slight incompatibility I found with this is that on unix-like I seem to remember :.! not needing any input; here I get an error if the command (like Write-Output/echo) isn't able to take piped input. (I sometimes used to use uuidgen with this feature)

a workaround that does work with this pr is just to use ? .; first like :.! ? .; echo "no piped input" instead, just thought I'd mention it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot create temp file for executing external commands over a range when shell is set to powershell
4 participants
@3N4N @zeertzjq @justinmk @sebshader
0