From e1e8e534144e11bfc36227d37777f7283396f8de Mon Sep 17 00:00:00 2001 From: Sichen Date: Sun, 6 Sep 2026 14:14:52 +0000 Subject: [PATCH] l command: honor the -l wrap width The l command ignored -l N and instead took its wrap width from terminal_size(). As a result, output depended on the terminal size and differed between a pipe and a tty. Use ProcessingContext::length, which already defaults to 70. GNU sed calls the option -l N, --line-length=N, while we exposed it as --length. Add --line-length and retain --length as a hidden alias for compatibility. A width of 0 means never wrap. In that case, ListLine now periodically flushes its buffer instead of accumulating the entire rendered line. Add OutputBuffer::write_partial_str for these partial writes, since write_str may defer a trailing newline. This removes the crate's last direct use of terminal_size, so drop the direct dependency from [dependencies] and [workspace.dependencies]. The package remains in the dependency tree through clap_builder and textwrap. --- Cargo.lock | 1 - Cargo.toml | 2 - src/sed/compiler.rs | 16 +--- src/sed/fast_io.rs | 69 ++++++++++++++ src/sed/mod.rs | 8 +- src/sed/processor.rs | 98 ++++++++++++++++++-- tests/by-util/test_sed.rs | 184 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 353 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a2640426..6edfcbb8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1174,7 +1174,6 @@ dependencies = [ "rustix", "sha2", "tempfile", - "terminal_size", "textwrap", "uucore", "uutests", diff --git a/Cargo.toml b/Cargo.toml index f5cbc41e..8924c576 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,7 +60,6 @@ rand = { version = "0.10.0" } regex = "1.10.4" sha2 = { version = "0.11.0", default-features = false, features = ["alloc"] } tempfile = "3.10.1" -terminal_size = "0.4.2" textwrap = { version = "0.16.1", features = ["terminal_size"] } uucore = { version = "0.11.0", features = ["libc"] } rustix = "1.1.4" @@ -78,7 +77,6 @@ phf = { workspace = true } predicates = { workspace = true } regex = { workspace = true } tempfile = { workspace = true } -terminal_size = { workspace = true } textwrap = { workspace = true } uucore = { workspace = true } rustix = { workspace = true } diff --git a/src/sed/compiler.rs b/src/sed/compiler.rs index 1ea7a8e1..0d3ae41f 100644 --- a/src/sed/compiler.rs +++ b/src/sed/compiler.rs @@ -27,11 +27,8 @@ use std::mem; use std::path::PathBuf; use std::rc::Rc; -use terminal_size::{Width, terminal_size}; use uucore::error::{UResult, USimpleError}; -const DEFAULT_OUTPUT_WIDTH: usize = 60; - const ERR_ADDRESS_0_USAGE: &str = "address 0 can only be used with ~step, a second regular expression, or a read command"; const ERR_SANDBOX: &str = "command not allowed with --sandbox"; @@ -1188,22 +1185,13 @@ fn compile_label_command( Ok(CommandHandling::Continue) } -/// Return the width of the command's terminal or a default. -fn output_width() -> usize { - if let Some((Width(w), _)) = terminal_size() { - w as usize - } else { - DEFAULT_OUTPUT_WIDTH - } -} - /// Compile commands that take a number as an argument. // Handles l q Q fn compile_number_command( lines: &mut ScriptLineProvider, line: &mut ScriptCharProvider, cmd: &mut Command, - _context: &mut ProcessingContext, + context: &mut ProcessingContext, ) -> UResult { line.advance(); // Skip the command character line.eat_spaces(); // Skip any leading whitespace @@ -1217,7 +1205,7 @@ fn compile_number_command( cmd.data = CommandData::Number(0); } 'l' => { - cmd.data = CommandData::Number(output_width()); + cmd.data = CommandData::Number(context.length); } _ => panic!("invalid number-expecting command"), }, diff --git a/src/sed/fast_io.rs b/src/sed/fast_io.rs index 5ea8bf3d..68c33c85 100644 --- a/src/sed/fast_io.rs +++ b/src/sed/fast_io.rs @@ -665,6 +665,29 @@ impl OutputBuffer { ))) } + /// Schedule the specified string for output as part of an output line. + /// Unlike `write_str`, the string is not treated as a complete line, so + /// no newline is deferred after it and output that follows continues on + /// the same line. + pub fn write_partial_str(&mut self, s: &str) -> io::Result<()> { + if s.is_empty() { + return Ok(()); + } + + // End a preceding complete line whose newline was deferred. + self.flush_pending_newline()?; + + // Mmapped output can be pending without a deferred newline, e.g. a + // newline-terminated input line printed from the mmapped input, so + // keep it ahead of this write. (After a deferred newline was + // written it has already been flushed and this does nothing.) + #[cfg(unix)] + { + self.flush_mmap(WriteRange::Complete)?; + } + self.out.write_all(s.as_bytes()) + } + /// Schedule the specified bytes for eventual output. pub fn write_bytes(&mut self, bytes: &[u8]) -> io::Result<()> { let (content, has_newline) = if bytes.ends_with(b"\n") { @@ -2151,4 +2174,50 @@ mod tests { file.read_to_string(&mut out).unwrap(); assert_eq!(out, "baz\n"); } + + // write_partial_str emits a deferred newline before its string + #[test] + fn write_partial_str_after_pending_newline() { + let (mut buf, mut file) = new_for_test(); + buf.write_str("foo").unwrap(); + assert!(buf.pending_newline); + buf.write_partial_str("bar").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, "foo\nbar"); + } + + // write_partial_str leaves no newline pending, so the output that + // follows continues its line + #[test] + fn write_partial_str_defers_no_newline() { + let (mut buf, mut file) = new_for_test(); + buf.write_str("foo").unwrap(); + buf.write_partial_str("bar").unwrap(); + assert!(!buf.pending_newline); + buf.write_str("baz\n").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, "foo\nbarbaz\n"); + } + + // write_partial_str writes out pending mmapped output before its string + #[cfg(unix)] + #[test] + fn write_partial_str_after_mmap_chunk() { + let (mut buf, mut file) = new_for_test(); + buf.write_chunk(&make_mmap_chunk(b"abc\n")).unwrap(); + assert_eq!(buf.mmap_chunk.as_ref().unwrap().len, 4); + buf.write_partial_str("xyz").unwrap(); + assert_eq!(buf.mmap_chunk.as_ref().unwrap().len, 0); + buf.flush().unwrap(); + file.seek(SeekFrom::Start(0)).unwrap(); + let mut out = String::new(); + file.read_to_string(&mut out).unwrap(); + assert_eq!(out, "abc\nxyz"); + } } diff --git a/src/sed/mod.rs b/src/sed/mod.rs index 1ed79a56..989bfc6b 100644 --- a/src/sed/mod.rs +++ b/src/sed/mod.rs @@ -107,7 +107,9 @@ pub fn uu_app() -> Command { .num_args(0..=1) .default_missing_value(""), // Access with .get_one::("line-length") - arg!(-l --length "Specify the 'l' command line-wrap length.") + arg!(-l --"line-length" "Specify the 'l' command line-wrap length.") + // The long name used before GNU sed's --line-length was accepted. + .alias("length") .value_parser(clap::value_parser!(u32)), arg!(-n --quiet "Suppress automatic printing of pattern space.").aliases(["silent"]), arg!(--posix "Disable non-POSIX extensions."), @@ -241,7 +243,9 @@ fn build_context(matches: &ArgMatches) -> UResult { in_place_suffix: matches .get_one::("in-place") .and_then(|s| if s.is_empty() { None } else { Some(s.clone()) }), - length: matches.get_one::("length").map_or(70, |v| *v as usize), + length: matches + .get_one::("line-length") + .map_or(70, |v| *v as usize), quiet: matches.get_flag("quiet"), posix: matches.get_flag("posix"), separate: matches.get_flag("separate"), diff --git a/src/sed/processor.rs b/src/sed/processor.rs index b703d77c..cdadd0a7 100644 --- a/src/sed/processor.rs +++ b/src/sed/processor.rs @@ -506,29 +506,66 @@ fn readable_char(ch: char) -> Cow<'static, str> { } } +/// Bound on the rendered list output buffered when never wrapping. +// A wrap width of zero never folds, so without periodic draining the whole +// rendered line would be held in memory. A few KB amortize the write calls +// while keeping the buffer bounded. +const LIST_FLUSH_THRESHOLD: usize = 4 * 1024; + /// Buffered state for rendering one list command output line. struct ListLine { buffer: String, + // Bytes of rendered output buffered since the line was last folded or + // drained; when wrapping, also the current column. width: usize, - max_width: usize, + // The wrap width. An item that would make width reach it folds the + // line first, so with its trailing backslash a line stays within + // fold_at columns unless a single item is wider than that. When + // never wrapping, LIST_FLUSH_THRESHOLD: the same test drains the + // buffer first instead, so it never holds that many bytes. + fold_at: usize, + // True for a wrap width of zero: drain rather than fold. + never_wrap: bool, } impl ListLine { /// Create an empty list output line with the specified maximum width. fn new(max_width: usize) -> Self { + // A width of zero means never wrap long lines, per the GNU sed + // manual. Such a line is drained to the output every few KB rather + // than held in memory whole. Deciding that here lets a single width + // test in write_item() cover both folding and draining. + let never_wrap = max_width == 0; Self { buffer: String::new(), width: 0, - max_width, + fold_at: if never_wrap { + LIST_FLUSH_THRESHOLD + } else { + max_width + }, + never_wrap, } } - /// Write a rendered list item, folding before the item if needed. + /// Write a rendered list item. Fold the line before the item if the + /// item does not fit in the wrap width; when never wrapping, instead + /// drain the accumulated output before an item that would make it + /// reach the flush threshold. fn write_item(&mut self, output: &mut OutputBuffer, out_str: &str) -> UResult<()> { let out_len = out_str.len(); - if self.width + out_len + 1 > self.max_width { - self.buffer.push_str("\\\n"); - output.write_str(std::mem::take(&mut self.buffer))?; + if self.width + out_len + 1 > self.fold_at { + if self.never_wrap { + // Drain before appending out_str rather than after it. + // finish() writes the "$" terminator only for a non-empty + // buffer, so a drain that emptied the buffer after the + // line's last item would lose that line's terminator. + output.write_partial_str(&self.buffer)?; + self.buffer.clear(); + } else { + self.buffer.push_str("\\\n"); + output.write_str(std::mem::take(&mut self.buffer))?; + } self.width = 0; } self.buffer.push_str(out_str); @@ -1082,6 +1119,55 @@ mod tests { assert_eq!(written, "abcd\\\n"); } + #[test] + fn test_write_list_item_zero_width_never_folds() { + let mut file = tempfile().unwrap(); + let mut output = OutputBuffer::new(Box::new(file.try_clone().unwrap())); + let mut line = ListLine::new(0); + line.write_item(&mut output, "abcd").unwrap(); + + line.write_item(&mut output, "e").unwrap(); + output.flush().unwrap(); + + assert_eq!(line.buffer, "abcde"); + assert_eq!(line.width, 5); + file.seek(SeekFrom::Start(0)).unwrap(); + let mut written = String::new(); + file.read_to_string(&mut written).unwrap(); + assert_eq!(written, ""); + } + + #[test] + fn test_write_list_item_zero_width_drains_buffer() { + let mut file = tempfile().unwrap(); + let mut output = OutputBuffer::new(Box::new(file.try_clone().unwrap())); + let mut line = ListLine::new(0); + + // Exactly as many items as the drain threshold. + let items = LIST_FLUSH_THRESHOLD; + for _ in 0..items { + line.write_item(&mut output, "a").unwrap(); + } + + // The drain took place before the item that reached the threshold + // was appended, so that item is still buffered. + assert_eq!(line.buffer, "a"); + assert_eq!(line.width, 1); + output.flush().unwrap(); + file.seek(SeekFrom::Start(0)).unwrap(); + let mut drained = String::new(); + file.read_to_string(&mut drained).unwrap(); + assert_eq!(drained, "a".repeat(items - 1)); + + // The complete output is still the unfolded line and its terminator. + line.finish(&mut output).unwrap(); + output.flush().unwrap(); + file.seek(SeekFrom::Start(0)).unwrap(); + let mut written = String::new(); + file.read_to_string(&mut written).unwrap(); + assert_eq!(written, format!("{}$\n", "a".repeat(items))); + } + #[test] fn test_list_line_finish_writes_terminator() { let mut file = tempfile().unwrap(); diff --git a/tests/by-util/test_sed.rs b/tests/by-util/test_sed.rs index 4c5eebaa..b6afda50 100644 --- a/tests/by-util/test_sed.rs +++ b/tests/by-util/test_sed.rs @@ -1930,6 +1930,190 @@ fn list_invalid_utf8_byte_locale() { .stdout_is_bytes(b"\\351$\n"); } +/// Without -l the wrap length defaults to 70, so lines fold after 69 +/// characters followed by a backslash. +#[test] +fn list_default_wrap_length() { + new_ucmd!() + .args(&["-n", "l"]) + .pipe_in("a".repeat(75)) + .succeeds() + .stdout_is_bytes(format!("{}\\\n{}$\n", "a".repeat(69), "a".repeat(6))); +} + +/// The -l option sets the wrap length used by an l command without an argument. +#[test] +fn list_length_option() { + new_ucmd!() + .args(&["-n", "-l", "5", "l"]) + .pipe_in(b"abcdefghij\n".to_vec()) + .succeeds() + .stdout_is_bytes(b"abcd\\\nefgh\\\nij$\n"); +} + +/// GNU sed documents the long form of -l as --line-length. +#[test] +fn list_line_length_option() { + new_ucmd!() + .args(&["-n", "--line-length=5", "l"]) + .pipe_in(b"abcdefghij\n".to_vec()) + .succeeds() + .stdout_is_bytes(b"abcd\\\nefgh\\\nij$\n"); +} + +/// The earlier long option name --length remains accepted as an alias. +#[test] +fn list_length_option_alias() { + new_ucmd!() + .args(&["-n", "--length=5", "l"]) + .pipe_in(b"abcdefghij\n".to_vec()) + .succeeds() + .stdout_is_bytes(b"abcd\\\nefgh\\\nij$\n"); +} + +/// A -l length of zero means never wrap long lines. +#[test] +fn list_length_option_zero_never_wraps() { + new_ucmd!() + .args(&["-n", "-l", "0", "l"]) + .pipe_in("a".repeat(80)) + .succeeds() + .stdout_is_bytes(format!("{}$\n", "a".repeat(80))); +} + +/// An l command argument of zero means never wrap long lines. +#[test] +fn list_command_length_zero_never_wraps() { + new_ucmd!() + .args(&["-n", "l 0"]) + .pipe_in("a".repeat(80)) + .succeeds() + .stdout_is_bytes(format!("{}$\n", "a".repeat(80))); +} + +/// A wrap length of one folds before every character, so the first fold +/// comes before any output has been emitted for the line. +#[test] +fn list_length_option_one() { + new_ucmd!() + .args(&["-n", "-l", "1", "l"]) + .pipe_in(b"abc\n".to_vec()) + .succeeds() + .stdout_is_bytes(b"\\\na\\\nb\\\nc$\n"); +} + +/// An l command argument of one folds before every character, like -l 1. +#[test] +fn list_command_length_one() { + new_ucmd!() + .args(&["-n", "l 1"]) + .pipe_in(b"abc\n".to_vec()) + .succeeds() + .stdout_is_bytes(b"\\\na\\\nb\\\nc$\n"); +} + +/// The length argument of an l command takes precedence over -l. Both +/// lengths fold this input, at different columns, so the expected bytes +/// match only the l argument and not the -l option. +#[test] +fn list_command_length_overrides_option() { + new_ucmd!() + .args(&["-n", "-l", "9", "l 4"]) + .pipe_in(b"abcdefghijklmnopqrst\n".to_vec()) + .succeeds() + .stdout_is_bytes(b"abc\\\ndef\\\nghi\\\njkl\\\nmno\\\npqr\\\nst$\n"); +} + +/// With a wrap length of zero the rendered line is drained to the output +/// every few KB instead of being held whole. The terminator must still +/// follow the complete line whether its length falls just below, at, or +/// just above a multiple of the drain threshold. +#[test] +fn list_length_zero_drains_long_line() { + // LIST_FLUSH_THRESHOLD in src/sed/processor.rs. + const THRESHOLD: usize = 4096; + for length in [ + THRESHOLD - 1, + THRESHOLD, + THRESHOLD + 1, + 2 * THRESHOLD - 1, + 2 * THRESHOLD, + 2 * THRESHOLD + 1, + ] { + new_ucmd!() + .args(&["-n", "-l", "0", "l"]) + .pipe_in("a".repeat(length)) + .succeeds() + .stdout_is_bytes(format!("{}$\n", "a".repeat(length))); + } +} + +/// A drained list line starts after the newline deferred by a preceding p +/// command rather than being spliced into that command's line. +#[test] +fn list_length_zero_after_print() { + let line = "a".repeat(5000); + new_ucmd!() + .args(&["-n", "-l", "0", "p;l"]) + .pipe_in(line.clone()) + .succeeds() + .stdout_is_bytes(format!("{line}\n{line}$\n")); +} + +/// An input line printed from a memory-mapped file is written out before a +/// list line drained after it, keeping the two in order. +#[test] +fn list_length_zero_after_mmap_output() { + let dots = ".".repeat(4096); + new_ucmd!() + .args(&["-l", "0", "l", "input/dots-8k.txt"]) + .succeeds() + .stdout_is_bytes(format!("{dots}$\n{dots}\n{dots}$\n{dots}\n")); +} + +/// An unterminated input line printed from a memory-mapped file leaves its +/// mmapped output pending together with a deferred newline. A list line +/// drained after it must follow the newline, which must follow the mmapped +/// output. +#[test] +fn list_length_zero_after_mmap_print_unterminated() -> std::io::Result<()> { + let dir = tempfile::tempdir()?; + let path = dir.path().join("input"); + let line = "k".repeat(5000); + std::fs::write(&path, &line)?; + + new_ucmd!() + .args(&["-n", "-l", "0", "p;l", path.to_str().unwrap()]) + .succeeds() + .stdout_is_bytes(format!("{line}\n{line}$\n")); + Ok(()) +} + +/// Mmapped output of at least 4 KB going to a regular file, as when editing +/// in place, is copied block-aligned and its remainder after the last full +/// block written separately. A list line drained after it must still +/// follow that remainder. +#[test] +fn list_length_zero_after_mmap_print_to_file() -> std::io::Result<()> { + let dir = tempfile::tempdir()?; + let path = dir.path().join("input"); + // The shortest line whose list output is drained; with its newline the + // mmapped output overruns a 4 KB block by one byte. + let line = "k".repeat(4096); + std::fs::write(&path, format!("{line}\n"))?; + + new_ucmd!() + .args(&["-n", "-l", "0", "-i", "-e", "p;l", path.to_str().unwrap()]) + .succeeds() + .no_stdout(); + + assert_eq!( + std::fs::read_to_string(&path)?, + format!("{line}\n{line}$\n") + ); + Ok(()) +} + //////////////////////////////////////////////////////////// // In-place editing #[test]