[vim/vim] Input redirection doesn't work with powershell (PR #11257)
Yegappan Lakshmanan
Patch contributed by Enan Ajmain.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/11257
Commit Summary
- a7a5653 Input redirection doesn't work with powershell
- c379d15 Merge branch 'vim:master' into cmdexpand
File Changes
(2 files)
- M src/ex_cmds.c (102)
- M src/testdir/test_shell.vim (31)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
codecov[bot]
Codecov Report
Merging #11257 (6b39125) into master (b850c39) will increase coverage by
0.61%.
The diff coverage is81.81%.
@@ Coverage Diff @@ ## master #11257 +/- ## ========================================== + Coverage 81.82% 82.44% +0.61% ========================================== Files 162 152 -10 Lines 188943 178951 -9992 Branches 42960 40612 -2348 ========================================== - Hits 154607 147537 -7070 + Misses 21790 19150 -2640 + Partials 12546 12264 -282
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | 82.72% <81.81%> (-0.01%) |
⬇️ |
| huge-gcc-none | ? |
|
| huge-gcc-testgui | 53.10% <0.00%> (+<0.01%) |
⬆️ |
| huge-gcc-unittests | 0.29% <0.00%> (-0.01%) |
⬇️ |
| linux | 82.44% <81.81%> (-0.06%) |
⬇️ |
| mingw-x64-HUGE | ? |
|
| mingw-x86-HUGE | ? |
|
| windows | ? |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/ex_cmds.c | 86.13% <81.81%> (+0.45%) |
⬆️ |
| src/regexp_bt.c | 78.48% <0.00%> (-7.48%) |
⬇️ |
| src/json.c | 77.84% <0.00%> (-5.47%) |
⬇️ |
| src/regexp_nfa.c | 84.89% <0.00%> (-4.65%) |
⬇️ |
| src/libvterm/src/parser.c | 55.18% <0.00%> (-4.15%) |
⬇️ |
| src/edit.c | 82.86% <0.00%> (-3.49%) |
⬇️ |
| src/gui.c | 69.78% <0.00%> (-3.20%) |
⬇️ |
| src/normal.c | 87.93% <0.00%> (-3.00%) |
⬇️ |
| src/clipboard.c | 71.68% <0.00%> (-2.91%) |
⬇️ |
| src/libvterm/src/screen.c | 50.18% <0.00%> (-2.85%) |
⬇️ |
| ... and 122 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan pushed 1 commit.
- 8de2356 Input redirection doesn't work with powershell
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan pushed 3 commits.
- a7a5653 Input redirection doesn't work with powershell
- 67c9e18 Merge branch 'cmdexpand' of https://github.com/yegappan/vim into cmdexpand
- bb37fd7 Optimize the check for the power shell name
You are receiving this because you are subscribed to this thread.
Enan Ajmain
@3N4N commented on this pull request.
In src/ex_cmds.c:
> + if (is_powershell) + len = (long_u)STRLEN(cmd) + 3; + else + len = (long_u)STRLEN(cmd) + 3; // "()" + NUL
Check for powershell looks redundant.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Enan Ajmain
> if (is_fish_shell)
len = (long_u)STRLEN(cmd) + 13; // "begin; " + "; end" + NUL
else
#endif
- len = (long_u)STRLEN(cmd) + 3; // "()" + NUL
+ {
+ is_powershell = (shell_name[0] == 'p')
PowerShell is nowadays available for Linux, too, in the form of PowerShell Core (v7). (Ignore if we don't care about that too much; who would use powershell in Linux anyway?)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Enan Ajmain
> + // "& { Get-Content" + " | & " + " }"
+ len += (long_u)STRLEN(itmp) + 23;
There is a space after Get-Content (see L1603). len should be 24, I think.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Enan Ajmain
> if (is_fish_shell)
len = (long_u)STRLEN(cmd) + 13; // "begin; " + "; end" + NUL
else
#endif
- len = (long_u)STRLEN(cmd) + 3; // "()" + NUL
+ {
+ is_powershell = (shell_name[0] == 'p')
Never mind. I misread #endif as #else.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Enan Ajmain
> #else
- // For shells that don't understand braces around commands, at least allow
- // the use of commands in a pipe.
- if (*p_sxe != NUL && *p_sxq == '(')
+ if (is_powershell)
Okay, here is where I wanted to mention powershell being available for Linux, too (mistakenly did so on a previous line). Taking the if is_powershell block above #ifdef UNIX should be enough. (Unless we don't care for people using PowerShell on Linux, assuming anyone does.)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
Yegappan Lakshmanan
@yegappan commented on this pull request.
In src/ex_cmds.c:
> + if (is_powershell) + len = (long_u)STRLEN(cmd) + 3; + else + len = (long_u)STRLEN(cmd) + 3; // "()" + NUL
Yes. In the original patch, the length was set to zero. But that broke some other functionality. When I reverted that change, I forgot to remove this.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
> + // "& { Get-Content" + " | & " + " }"
+ len += (long_u)STRLEN(itmp) + 23;
Changed the value to 24.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
> #else
- // For shells that don't understand braces around commands, at least allow
- // the use of commands in a pipe.
- if (*p_sxe != NUL && *p_sxq == '(')
+ if (is_powershell)
Moved the powershell handling code to outside of the '#ifdef/#endif'.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Enan Ajmain
The patch now looks like it should be fine. I can't tell why the tests are failing (haven't looked at the test files yet).
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
The current patch checks for powershell on any system.
It is available on Linux, but does it use exactly the same syntax then?
Is it even practical to set 'shell' to powershell on any other system
than MS-Windows?
--
hundred-and-one symptoms of being an internet addict:
256. You are able to write down over 250 symptoms of being an internet
addict, even though they only asked for 101.
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Enan Ajmain
It is available on Linux, but does it use exactly the same syntax then?
There may be differences in some cases, but as far as I have used it I haven't found any.
Is it even practical to set 'shell' to powershell on any other system
than MS-Windows?
I don't think so. First, practically no one will use it. Second, there are several bugs/oddities in pwsh in Linux: e.g., using sort with Start-Process sort -RedirectStandardInput in.txt works fine, but if we redirect it to a file (-RedirectStandardOutput out.txt) then it eats all the empty lines.
But I want to leave it to the maintainer (you, Bram) to decide. We can always take the other choice later, if required; it's not a huge change.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
The current patch checks for powershell on any system.
It is available on Linux, but does it use exactly the same syntax then?
Is it even practical to set 'shell' to powershell on any other system
than MS-Windows?
vim-dev ML
On Mon, Oct 3, 2022 at 5:06 AM Bram Moolenaar ***@***.***>
wrote:
>
> The current patch checks for powershell on any system.
> It is available on Linux, but does it use exactly the same syntax then?
> Is it even practical to set 'shell' to powershell on any other system
> than MS-Windows?
>
>
>
Setting 'shell' to 'pwsh' or 'powershell' (Powershell) was supported on
Linux before
this patch. But "%!sort" was failing on Linux (just like on MS-Windows)
before
this patch. This patch makes it work on both Linux and MS-Windows.
- Yegappan
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
