Implement e (execute) command#481
Conversation
Implement the 'e' command. No argument implies execution of pattern space and any respective argument is to run with the output written to stream. Rejected under --posix and --sandbox. Also stop silently swallowing shell stderr for e and s///e. Signed-off-by: PranavRJoshi <pranavrjoshi1@gmail.com>
Includes 17 new tests. Cover both forms of the new 'e' command: with-argument execution (including shell side effects, addressing, ';' not separating the argument, escape decoding, and backslash continuation) and the no-argument pattern-space form. Also cover --posix/--sandbox rejection and stderr surfacing on command failure. Signed-off-by: PranavRJoshi <pranavrjoshi1@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #481 +/- ##
==========================================
- Coverage 83.05% 81.79% -1.26%
==========================================
Files 13 13
Lines 6120 6214 +94
Branches 358 367 +9
==========================================
Hits 5083 5083
- Misses 1034 1128 +94
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Seems like CRLF issue on Windows. Will make those tests conditionally compile on UNIX only. |
dspinellis
left a comment
dspinellis
left a comment
There was a problem hiding this comment.
Thank you for the quick turnaround and thorough testing! I added a few improvement suggestions.
| let mut child = shell_command(cmd) | ||
| .stdout(std::process::Stdio::piped()) | ||
| .spawn()?; | ||
| let mut stdout = child.stdout.take().expect("stdout was piped"); |
There was a problem hiding this comment.
Better: "stdout should be piped"
| .unwrap_err() | ||
| })?; | ||
| let mut shell_out = String::from_utf8_lossy(&stdout_bytes).into_owned(); | ||
| if shell_out.ends_with("\r\n") { |
There was a problem hiding this comment.
Make this active only for #[cfg(windows)]. I wanted to comment on the previous PR, but let it pass to move ahead.
| if shell_out.ends_with("\r\n") { | ||
| // On windows, both return carriage and newline characters are used | ||
| shell_out.truncate(shell_out.len() - 2); | ||
| } else if shell_out.ends_with('\n') { |
There was a problem hiding this comment.
But keep this also for Windows, because some Windows tools use \n.
| ) | ||
| .unwrap_err() | ||
| })?; | ||
| let mut shell_out = String::from_utf8_lossy(&stdout_bytes).into_owned(); |
There was a problem hiding this comment.
As sed is a data processing tool rather than a UI front-end, we should avoid from_utf8_lossy, which can lead to silent data corruption. I suggest using (through a suitable method) IOChunk::from_content.
| shell_out.pop(); | ||
| } | ||
| pattern.set_to_string(shell_out, pattern.is_newline_terminated()); | ||
| execute_pattern_as_shell(pattern, command, context)?; |
|
|
||
| /// Execute the pattern space as a shell command, replacing its contents | ||
| /// with the command's standard output, minus one trailing newline. | ||
| fn execute_pattern_as_shell( |
There was a problem hiding this comment.
Better names execute_pattern_through_shell or execute_pattern_as_shell_command .
| ) | ||
| .unwrap_err() | ||
| })?; | ||
| output.write_str(String::from_utf8_lossy(&stdout_bytes).into_owned())?; |
There was a problem hiding this comment.
Please avoid from_utf8_lossy
|
|
||
| //////////////////////////////////////////////////////////// | ||
| // e command (execute) | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
Add a separate corresponding test for Windows.
|
Appreciate the thorough review. Will make the appropriate changes and commit them later. |
2 participants
Fixes #476.
Implements the standalone
ecommand (a GNU extension). Supports the command with and without arguments.Changes:
ecommand in compiler.rs/processor.rs, both forms (with argument, bare/pattern-space). Argument parsing reuses the same escape decoding and backslash continuation as a/c/i, as GNU sed'seargument follows the same syntax.--posixand--sandboxat compile time.eands///eto silently swallowing the child shell's stderr.Irrelevant: fixed a typo found at
delimited_parser.rs.