From 456e9db38192ba8be16931e076b1ce88a7138d47 Mon Sep 17 00:00:00 2001 From: pocopepe Date: Thu, 23 Apr 2026 11:23:49 +0530 Subject: [PATCH] fix: correct newline handling for p and x commands --- src/sed/fast_io.rs | 128 ++++++++++++++++++++++++++++++++++---- src/sed/mod.rs | 5 +- src/sed/processor.rs | 10 +-- tests/by-util/test_sed.rs | 64 ++++++++++++++++++- 4 files changed, 184 insertions(+), 23 deletions(-) diff --git a/src/sed/fast_io.rs b/src/sed/fast_io.rs index 2e2403a7..fa912ef7 100644 --- a/src/sed/fast_io.rs +++ b/src/sed/fast_io.rs @@ -559,8 +559,11 @@ pub struct OutputBuffer { max_pending_write: usize, // Max bytes to keep before flushing #[cfg(unix)] mmap_chunk: Option, // Chunk to write + // True when the last write didn't end with \n; the \n is deferred so + // that commands like `p` don't emit a spurious newline under -n. + pending_newline: bool, #[cfg(test)] - low_level_flushes: usize, // Number of system call flushes + low_level_flushes: usize, // Number of system call flushes } /// Threshold to use buffered writes for output @@ -591,6 +594,7 @@ impl OutputBuffer { pub fn new(w: Box) -> Self { Self { out: BufWriter::new(w), + pending_newline: false, #[cfg(test)] low_level_flushes: 0, } @@ -609,16 +613,22 @@ impl OutputBuffer { fast_copy, max_pending_write, mmap_chunk: None, + pending_newline: false, #[cfg(test)] low_level_flushes: 0, } } - /// Schedule the specified String or &strfor eventual output + /// Schedule the specified String or &str for eventual output pub fn write_str>(&mut self, s: S) -> io::Result<()> { + let mut s = s.into(); + let has_newline = s.ends_with('\n'); + if has_newline { + s.truncate(s.len() - 1); + } self.write_chunk(&IOChunk::from_content(IOChunkContent::new_owned( - s.into(), - false, + s, + has_newline, ))) } @@ -667,6 +677,16 @@ enum WriteRange { impl OutputBuffer { /// Schedule the specified output chunk for eventual output pub fn write_chunk(&mut self, new_chunk: &IOChunk) -> io::Result<()> { + if new_chunk.is_empty() && !new_chunk.is_newline_terminated() { + return Ok(()); + } + + if self.pending_newline { + self.flush_mmap(WriteRange::Complete)?; + self.out.write_all(b"\n")?; + self.pending_newline = false; + } + match &new_chunk.content { IOChunkContent::MmapInput { full_span, @@ -712,6 +732,7 @@ impl OutputBuffer { len: new_len, }); } + self.pending_newline = !new_chunk.is_newline_terminated(); } IOChunkContent::Owned { @@ -724,6 +745,7 @@ impl OutputBuffer { if *has_newline { self.out.write_all(b"\n")?; } + self.pending_newline = !has_newline; } } Ok(()) @@ -772,6 +794,16 @@ impl OutputBuffer { Ok(()) } + /// Write a deferred newline if the last output didn't end with one. + pub fn flush_pending_newline(&mut self) -> io::Result<()> { + if self.pending_newline { + self.flush_mmap(WriteRange::Complete)?; + self.out.write_all(b"\n")?; + self.pending_newline = false; + } + Ok(()) + } + /// Flush everything: pending mmap and buffered data. pub fn flush(&mut self) -> io::Result<()> { self.flush_mmap(WriteRange::Complete)?; // flush mmap if any @@ -783,6 +815,15 @@ impl OutputBuffer { impl OutputBuffer { /// Schedule the specified output chunk for eventual output pub fn write_chunk(&mut self, chunk: &IOChunk) -> io::Result<()> { + if chunk.is_empty() && !chunk.is_newline_terminated() { + return Ok(()); + } + + if self.pending_newline { + self.out.write_all(b"\n")?; + self.pending_newline = false; + } + match &chunk.content { IOChunkContent::Owned { content, @@ -793,11 +834,21 @@ impl OutputBuffer { if *has_newline { self.out.write_all(b"\n")?; } + self.pending_newline = !has_newline; Ok(()) } } } + /// Write a deferred newline if the last output didn't end with one. + pub fn flush_pending_newline(&mut self) -> io::Result<()> { + if self.pending_newline { + self.out.write_all(b"\n")?; + self.pending_newline = false; + } + Ok(()) + } + /// Flush everything: pending mmap and buffered data. pub fn flush(&mut self) -> io::Result<()> { self.out.flush() // then flush buffered data @@ -1796,6 +1847,7 @@ mod tests { max_pending_write: 8, #[cfg(unix)] mmap_chunk: None, + pending_newline: false, low_level_flushes: 0, }; (buf, file) @@ -1847,9 +1899,9 @@ mod tests { fn mmap_new_chunk_and_coalesce() { let (mut outbuf, _file) = new_for_test(); // OutputBuffer - let backing = b"abcdefgh"; // contiguous buffer - let c1 = make_mmap_chunk(&backing[0..4]); // "abcd" - let c2 = make_mmap_chunk(&backing[4..8]); // "efgh" + let backing = b"abc\nefg\n"; // contiguous buffer, newline-terminated lines + let c1 = make_mmap_chunk(&backing[0..4]); // "abc\n" + let c2 = make_mmap_chunk(&backing[4..8]); // "efg\n" outbuf.write_chunk(&c1).unwrap(); outbuf.write_chunk(&c2).unwrap(); @@ -1879,13 +1931,13 @@ mod tests { fn mmap_coalesce_and_flush_blocks() { let (mut buf, _file) = new_for_test(); buf.max_pending_write = 4; - let backing = b"abcdefgh"; // contiguous buffer - let c1 = make_mmap_chunk(&backing[0..5]); // "abcde" - let c2 = make_mmap_chunk(&backing[5..8]); // "fgh" + let backing = b"abcde\nfgh\n"; // contiguous newline-terminated lines + let c1 = make_mmap_chunk(&backing[0..6]); // "abcde\n" + let c2 = make_mmap_chunk(&backing[6..10]); // "fgh\n" buf.write_chunk(&c1).unwrap(); buf.write_chunk(&c2).unwrap(); - // After a flush + // After a flush triggered by exceeding max_pending_write assert_eq!(buf.mmap_chunk.as_ref().unwrap().len, 0); } @@ -1916,4 +1968,58 @@ mod tests { assert_eq!(out, "world\n"); } + + // pending_newline is injected between two no-newline chunks + #[test] + fn pending_newline_injected_between_chunks() { + let (mut buf, mut file) = new_for_test(); + buf.write_chunk(&make_owned_chunk("first", false)).unwrap(); + buf.write_chunk(&make_owned_chunk("second", true)).unwrap(); + buf.out.flush().unwrap(); + file.seek(SeekFrom::Start(0)).unwrap(); + let mut out = String::new(); + file.read_to_string(&mut out).unwrap(); + assert_eq!(out, "first\nsecond\n"); + } + + // flush_pending_newline emits the deferred newline + #[test] + fn flush_pending_newline_emits_newline() { + let (mut buf, mut file) = new_for_test(); + buf.write_chunk(&make_owned_chunk("foo", false)).unwrap(); + assert!(buf.pending_newline); + buf.flush_pending_newline().unwrap(); + assert!(!buf.pending_newline); + buf.out.flush().unwrap(); + file.seek(SeekFrom::Start(0)).unwrap(); + let mut out = String::new(); + file.read_to_string(&mut out).unwrap(); + assert_eq!(out, "foo\n"); + } + + // write_str strips trailing newline and sets pending_newline correctly + #[test] + fn write_str_with_trailing_newline() { + let (mut buf, mut file) = new_for_test(); + buf.write_str("bar\n").unwrap(); + assert!(!buf.pending_newline); + buf.out.flush().unwrap(); + file.seek(SeekFrom::Start(0)).unwrap(); + let mut out = String::new(); + file.read_to_string(&mut out).unwrap(); + assert_eq!(out, "bar\n"); + } + + #[test] + fn write_str_without_trailing_newline() { + let (mut buf, mut file) = new_for_test(); + buf.write_str("baz").unwrap(); + assert!(buf.pending_newline); + buf.flush_pending_newline().unwrap(); + buf.out.flush().unwrap(); + file.seek(SeekFrom::Start(0)).unwrap(); + let mut out = String::new(); + file.read_to_string(&mut out).unwrap(); + assert_eq!(out, "baz\n"); + } } diff --git a/src/sed/mod.rs b/src/sed/mod.rs index 8caef1d5..1206140c 100644 --- a/src/sed/mod.rs +++ b/src/sed/mod.rs @@ -211,7 +211,10 @@ fn build_context(matches: &ArgMatches) -> ProcessingContext { stop_processing: false, saved_regex: None, input_action: None, - hold: StringSpace::default(), + hold: StringSpace { + content: String::new(), + has_newline: true, + }, parsed_block_nesting: 0, label_to_command_map: HashMap::new(), range_commands: Vec::new(), diff --git a/src/sed/processor.rs b/src/sed/processor.rs index 3b233941..9dde89fc 100644 --- a/src/sed/processor.rs +++ b/src/sed/processor.rs @@ -557,21 +557,14 @@ fn process_file( continue 'lines; } 'p' => { - // Write the pattern space to standard output. write_chunk(output, context, &pattern)?; - if !pattern.is_newline_terminated() { - // !chomped equivalent - output.write_str("\n")?; // explicit \n - } } 'P' => { - // Output pattern space, up to the first \n. let line = pattern.as_str()?; if let Some(pos) = line.find('\n') { output.write_str(&line[..=pos])?; } else { - output.write_str(line)?; - output.write_str("\n")?; + write_chunk(output, context, &pattern)?; } } 'q' => { @@ -652,6 +645,7 @@ fn process_file( flush_appends(output, context)?; if context.stop_processing { + output.flush_pending_newline()?; break; } } diff --git a/tests/by-util/test_sed.rs b/tests/by-util/test_sed.rs index b0baf479..6c73def9 100644 --- a/tests/by-util/test_sed.rs +++ b/tests/by-util/test_sed.rs @@ -1212,7 +1212,7 @@ fn test_missing_address_re() { } //////////////////////////////////////////////////////////// -// Test for issue #143: Missing newline in output with `-e p` +// issue #143: p with no trailing newline #[test] fn test_print_command_adds_newline() { new_ucmd!() @@ -1222,8 +1222,36 @@ fn test_print_command_adds_newline() { .stdout_is("foo\nfoo"); } -//////////////////////////////////////////////////////////// -// Test for issue #254: Missing newline in exchanged output with `2x` +#[test] +fn test_print_command_multiline_no_newline() { + new_ucmd!() + .args(&["-e", "p"]) + .pipe_in("a\nfoo") + .succeeds() + .stdout_is("a\na\nfoo\nfoo"); +} + +// -n p must not add a trailing newline when input has none +#[test] +fn test_print_command_silent_no_newline() { + new_ucmd!() + .args(&["-n", "p"]) + .pipe_in("foo") + .succeeds() + .stdout_is("foo"); +} + +// sanity: normal newline-terminated input is unaffected +#[test] +fn test_print_command_with_newline() { + new_ucmd!() + .args(&["-e", "p"]) + .pipe_in("foo\n") + .succeeds() + .stdout_is("foo\nfoo\n"); +} + +// issue #254: 2x missing newline #[test] fn test_exchange_command_adds_newline() { new_ucmd!() @@ -1232,3 +1260,33 @@ fn test_exchange_command_adds_newline() { .succeeds() .stdout_is("a\n\nc\n"); } + +// issue #306: 1x with no-newline input should output an empty line +#[test] +fn test_exchange_no_newline_outputs_empty_line() { + new_ucmd!() + .args(&["1x"]) + .pipe_in("abc") + .succeeds() + .stdout_is("\n"); +} + +// q with no newline input must not drop the trailing newline +#[test] +fn test_quit_no_newline() { + new_ucmd!() + .args(&["q"]) + .pipe_in("foo") + .succeeds() + .stdout_is("foo\n"); +} + +// P with single line no newline input +#[test] +fn test_print_first_line_no_newline() { + new_ucmd!() + .args(&["-n", "P"]) + .pipe_in("foo") + .succeeds() + .stdout_is("foo"); +}