Implement GNU R (read next line) command - #533
Merged
Merged
Conversation
Merging this PR will degrade performance by 3.98%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | access_log_subst |
2.4 s | 3 s | -19.05% |
| ❌ | no_op_short |
2.4 s | 2.7 s | -10.49% |
| ❌ | access_log_no_op |
1.8 s | 1.9 s | -7.44% |
| ❌ | access_log_append |
1.5 s | 1.6 s | -4.97% |
| ❌ | access_log_translit |
1 s | 1.1 s | -4.54% |
| ❌ | access_log_no_subst |
1.1 s | 1.2 s | -2.93% |
| ❌ | remove_cr |
521.8 ms | 537.6 ms | -2.93% |
| ❌ | access_log_no_del |
1.1 s | 1.2 s | -2.42% |
| ⚡ | number_fix |
1.8 s | 1.6 s | +16.29% |
| ⚡ | genome_subst |
461.2 ms | 450.3 ms | +2.42% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing dspinellis:R-command (35fce5a) with multiple-writers (c8b5dc5)
|
GNU sed testsuite comparison: |
There was a problem hiding this comment.
Pull request overview
This PR adds GNU sed’s R (read next line) extension and refactors output handling to better match GNU sed’s “missing newline” behavior by tracking record separation explicitly across outputs.
Changes:
- Implement GNU
Rcommand using shared per-path readers (NamedReader) and add fixtures/tests forR. - Rework “missing newline” handling by introducing
RecordSeparatorStateand removingOutputBuffer’s deferred-newline mechanism. - Unify named file I/O concerns by replacing
named_writerwithnamed_io(shared writers + new readers) and expand tests/fixtures accordingly.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures/sed/output/read_one_twice | New expected output fixture for repeated R reads. |
| tests/fixtures/sed/output/read_one_no_newline | New expected output fixture for R when the read file lacks a trailing newline. |
| tests/fixtures/sed/output/read_one_missing | New expected output fixture for R on missing file. |
| tests/fixtures/sed/output/read_one_many | New expected output fixture for R reading many times. |
| tests/fixtures/sed/output/read_one_empty | New expected output fixture for R with empty file. |
| tests/fixtures/sed/output/read_one | New expected output fixture for basic R. |
| tests/fixtures/sed/output/read_no_newline | New expected output fixture for r with missing newline behavior interactions. |
| tests/fixtures/sed/output/multiple_input_files | Updates expected output to reflect revised missing-newline handling. |
| tests/by-util/test_sed.rs | Adds/updates tests for R and missing-newline behavior; includes new write unification test. |
| src/sed/processor.rs | Implements RecordSeparatorState, integrates it into output paths, and adds runtime support for R. |
| src/sed/named_writer.rs | Removed in favor of consolidated named I/O module. |
| src/sed/named_io.rs | Adds shared NamedReader (for R) and shared NamedWriter (for w/W unification). |
| src/sed/mod.rs | Wires new named_io module and initializes record-separator state in context. |
| src/sed/fast_io.rs | Removes deferred newline state from OutputBuffer now handled at the processor level. |
| src/sed/compiler.rs | Adds compilation support for R (GNU extension) and updates writer imports. |
| src/sed/command.rs | Extends command data/types for R and record-separator state tracking. |
| README.md | Documents new GNU extensions/behaviors (R, missing-newline behavior for p). |
Suppressed comments (1)
tests/by-util/test_sed.rs:1683
- Spelling/grammar: duplicated "to" and awkward phrasing in this comment block.
// r respects a lacking newline and adds it and also resets the state to
// to no newline needed.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+101
to
+107
| pub fn new(path: PathBuf, location: ScriptLocation) -> UResult<Rc<RefCell<Self>>> { | ||
| let canonical_path = canonicalize_output_path(&path, &location)?; | ||
|
|
||
| if let Some(writer) = WRITERS.with(|writers| writers.borrow().get(&canonical_path).cloned()) | ||
| { | ||
| return Ok(writer); | ||
| } |
| // r, R, w, W commands | ||
| check_output!(read_ok, [format!("4r {LINES2}"), LINES1.to_string()]); | ||
| check_output!(read_no_newline, ["4r input/no-new-line.txt", LINES1]); | ||
| // r Doesn't doesn't record lacking newline |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## multiple-writers #533 +/- ##
====================================================
- Coverage 83.12% 82.92% -0.21%
====================================================
Files 13 13
Lines 7083 7193 +110
Branches 401 406 +5
====================================================
+ Hits 5888 5965 +77
- Misses 1192 1226 +34
+ Partials 3 2 -1
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:
|
|
GNU sed testsuite comparison: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (7)
src/sed/processor.rs:704
- In -u/--unbuffered mode, address-0
0routput is written via copy_file but never flushed, so early pre-scan output may be delayed until later buffered flushes.
let path = extract_variant!(cmd, Path);
context.rss.new_record(output)?;
output.copy_file(path)?;
// GNU sed doesn't care about read file \n ending.
context.rss.has_newline(true);
src/sed/processor.rs:856
- In -u/--unbuffered mode, the
lcommand writes output but does not flush the OutputBuffer, which makes-ubehavior inconsistent with other output paths that call write_*_record.
'l' => {
let width = *extract_variant!(command, Number);
context.rss.new_record(output)?;
list(output, &pattern, width, &command.location, context)?;
context.rss.has_newline(true);
src/sed/named_io.rs:59
- NamedReader::new canonicalizes the path before storing it, so original_path() can return a canonicalized path rather than the original input path. The doc comment should reflect that to avoid misleading callers.
tests/by-util/test_sed.rs:1671 - Comment has a duplicated/incorrect capitalization (“Doesn't doesn't”).
// r Doesn't doesn't record lacking newline
src/sed/processor.rs:532
- In -u/--unbuffered mode, this function writes appended output (from a/r/R) but never flushes the OutputBuffer, so output can remain buffered contrary to the flag’s documented behavior and inconsistent with write_chunk_record/write_buffer_record.
This issue also appears in the following locations of the same file:
- line 700
- line 852
fn flush_appends(output: &mut OutputBuffer, context: &mut ProcessingContext) -> UResult<()> {
for elem in &context.append_elements {
match elem {
AppendElement::Text(text) => {
context.rss.new_record(output)?;
tests/by-util/test_sed.rs:1683
- Comment has a duplicated word (“to”).
// to no newline needed.
README.md:107
- This bullet makes the “missing newline” behavior sound specific to the
pcommand, but the implementation (RecordSeparatorState) applies the separator before any subsequent output record to the same stream. Consider rewording to describe the general output-stream behavior.
* The `p` command adds a missing newline.
(POSIX is silent; FreeBSD sed ignores the newline; original sed ignores
the line).
|
GNU sed testsuite comparison: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/sed/processor.rs:856
- The
lcommand path writes tooutputbut does not flush whencontext.unbufferedis set, sosedcan bufferloutput even though other output paths flush in unbuffered mode.
let width = *extract_variant!(command, Number);
context.rss.new_record(output)?;
list(output, &pattern, width, &command.location, context)?;
context.rss.has_newline(true);
src/sed/processor.rs:704
- Address-0
routput (handled inprocess_address_0) is written without honoringcontext.unbuffered, so when stdout is a terminal the pre-scan output may be buffered until later writes.
context.rss.new_record(output)?;
output.copy_file(path)?;
// GNU sed doesn't care about read file \n ending.
context.rss.has_newline(true);
tests/by-util/test_sed.rs:1671
- Spelling/grammar: duplicated “Doesn't doesn't” in this comment.
// r Doesn't doesn't record lacking newline
src/sed/processor.rs:551
- In unbuffered mode (
context.unbuffered), output written byflush_appendsis never flushed, soa/r/Rappended data can be delayed until a later write/flush. This is inconsistent withwrite_chunk_record/write_buffer_recordwhich flush immediately when unbuffered.
This issue also appears in the following locations of the same file:
- line 701
- line 853
}
context.append_elements.clear();
Ok(())
src/sed/named_io.rs:62
NamedReader::original_path()returns the canonicalized path stored in the reader, not the original path passed by the script. The name is misleading (and the unit tests assert canonicalization). Consider renaming topath()/canonical_path()or storing both original and canonical paths.
tests/by-util/test_sed.rs:1683- Spelling/grammar: duplicated “to” across these two lines.
// r respects a lacking newline and adds it and also resets the state to
// to no newline needed.
Co-authored-by: Mukunda Katta <mukunda.vjcs6@gmail.com>
It violated layering by having it track higher-level newline behavior and caused R of non-newline terminated files to fail; (see read_one_no_newline test).
|
GNU sed testsuite comparison: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/sed/named_io.rs:85
NamedReader::read_linemarks the reader as done at EOF / error but keeps theBufReader<File>inself.reader, which keeps the file descriptor open for the rest of the process. This can leak FDs when many distinctRpaths are used.
tests/by-util/test_sed.rs:1671- Comment typo: duplicate “Doesn't doesn't” and inconsistent capitalization.
// r Doesn't doesn't record lacking newline
tests/by-util/test_sed.rs:1683
- Comment typo: duplicated “to” and phrasing is a bit unclear.
// r respects a lacking newline and adds it and also resets the state to
// to no newline needed.
README.md:107
- The README entry attributes “adds a missing newline” specifically to the
pcommand, but the implementation tracks missing-newline separation as a general output-stream rule (it affects default printing and other output records too). This bullet is misleading as written.
* The `p` command adds a missing newline.
(POSIX is silent; FreeBSD sed ignores the newline; original sed ignores
the line).
|
GNU sed testsuite comparison: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/by-util/test_sed.rs:1671
- Typo/duplication in comment: "Doesn't doesn't".
// r Doesn't doesn't record lacking newline
README.md:107
- The README bullet says only the
pcommand adds a missing newline, but the implementation introduces record-separator handling for standard output more generally (newline is emitted before the next output record after any unterminated output). Reword this bullet to describe the general GNU sed behavior rather than attributing it specifically top.
* The `p` command adds a missing newline.
(POSIX is silent; FreeBSD sed ignores the newline; original sed ignores
the line).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This also requires rewriting and fixing the handling of missing newlines.