Implement GNU W (write pattern first line) command - #531
Conversation
While at it, also fix the w command on patterns missing a newline. Co-authored-by: Mukunda Katta <mukunda.vjcs6@gmail.com>
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds GNU sed-compatible W (write first line of pattern space) support and fixes w so it doesn’t always force a trailing newline when the input line/pattern isn’t newline-terminated.
Changes:
- Implement non-POSIX
Wcommand in the compiler and processor. - Adjust
w/substitute write behavior to respect newline termination. - Expand test coverage and update README to mention
W.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/by-util/test_sed.rs | Adds tests for W, sandbox rejection, and w behavior when input lacks a newline |
| src/sed/processor.rs | Implements runtime behavior for W and updates w/substitute writing to be newline-aware |
| src/sed/named_writer.rs | Extends writer API to optionally append a newline; adds unit test for no-newline writes |
| src/sed/compiler.rs | Enables W command only when not in --posix mode |
| README.md | Documents the new W command |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * An `F` command outputs the name of the file currently being processed. | ||
| * A `Q` command (optionally followed by an exit code) quits immediately. | ||
| * The `q` command can be optionally followed by an exit code. | ||
| * A `W` command writes to a file the pattern's first line. |
| #[test] | ||
| fn write_first_line_with_w_command_is_non_posix() { | ||
| new_ucmd!() | ||
| .args(&["--posix", "W /tmp/out"]) |
| /// Write String to the file, possibly with a newline, returning errors. | ||
| pub fn write_line(&mut self, line: &str, newline: bool) -> UResult<()> { | ||
| self.write_line_bytes(line.as_bytes(), newline) | ||
| } | ||
|
|
||
| /// Write bytes to the file with a newline, returning descriptive errors. | ||
| pub fn write_line_bytes(&mut self, line: &[u8]) -> UResult<()> { | ||
| /// Write bytes to the file, possibly with a newline, returning errors. | ||
| pub fn write_line_bytes(&mut self, line: &[u8], newline: bool) -> UResult<()> { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/sed/named_writer.rs:64
- The
newline: boolparameter is ambiguous (it reads like the input contains a newline, but it actually controls whether to append one). To avoid misuse at call sites, consider renaming the parameter to something explicit likeappend_newline(or similar), and update the doc comments accordingly (also: “Write String” → “Write a string”).
/// Write String to the file, possibly with a newline, returning errors.
pub fn write_line(&mut self, line: &str, newline: bool) -> UResult<()> {
self.write_line_bytes(line.as_bytes(), newline)
}
/// Write bytes to the file, possibly with a newline, returning errors.
pub fn write_line_bytes(&mut self, line: &[u8], newline: bool) -> UResult<()> {
src/sed/processor.rs:883
- The newline decision
!found_newline && pattern.is_newline_terminated()is correct but hard to read becausefound_newlineactually means “first line slice already contains a\\nbyte”, not “pattern is newline-terminated”. Consider introducing a clearly named local (e.g.,append_newline) derived from these conditions and passing that along; it will make future changes toWsemantics less error-prone.
'W' => {
// Append only the first line of the pattern space.
let writer = extract_variant!(command, NamedWriter);
let pattern_bytes = pattern.as_bytes();
let (first_line, found_newline) =
match pattern_bytes.iter().position(|&b| b == b'\n') {
// A slice including the newline
Some(pos) => (&pattern_bytes[..=pos], true),
None => (pattern_bytes, false),
};
writer.borrow_mut().write_line_bytes(
first_line,
!found_newline && pattern.is_newline_terminated(),
)?;
}
README.md:107
- This PR implements GNU
Wand the tests enforce it as non-POSIX (--posixrejects it). The README entry should mention thatWis a GNU/non-POSIX extension (and ideally clarify it writes the first line of the pattern space).
* A `W` command writes to a file the pattern's first line.
Merging this PR will degrade performance by 4.78%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | number_fix |
1.5 s | 1.7 s | -11.95% |
| ⚡ | access_log_translit |
1 s | 1 s | +2.98% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing W-command (714ec8a) with main (79e5df0)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #531 +/- ##
==========================================
- Coverage 83.18% 83.01% -0.17%
==========================================
Files 13 13
Lines 7001 7037 +36
Branches 398 401 +3
==========================================
+ Hits 5824 5842 +18
- Misses 1174 1192 +18
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:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
tests/by-util/test_sed.rs:1816
- Test name mentions the
wcommand, but the test exercises the non-POSIXWcommand. Renaming improves clarity when reading failures.
fn write_first_line_with_w_command_is_non_posix() {
src/sed/named_writer.rs:58
- Doc comment grammar: “Write String” reads like a type name; this is a docstring, so it should say “Write a string…”.
/// Write String to the file, possibly with a newline, returning errors.
README.md:107
- Wording is a bit ambiguous:
Woperates on the pattern space (not a generic “pattern”). Consider clarifying that it writes the first line of the pattern space.
* A `W` command writes to a file the pattern's first line.
src/sed/named_writer.rs:63
- Doc comment reads a bit awkwardly; consider using “optionally” instead of “possibly” for clarity/grammar.
/// Write bytes to the file, possibly with a newline, returning errors.
While at it, also fix the w command on patterns missing a newline.