ls: fix --zero line endings for column and comma formats - #14789
An1MuS1111 wants to merge 1 commit into
Conversation
When --zero precedes column formatting (-C, -x), every row should end with NUL instead of newline. Previously, the grid display unconditionally emitted \n as the row delimiter, and writeln! in display_items/display_grid caused double newlines on wrapped output. Stream grid rows with the configured line ending directly to avoid converting to String or clobbering embedded newlines in filenames, and use write! instead of writeln! for line ending outputs. Closes uutils#14774
There was a problem hiding this comment.
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.
Copilot review overview
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes ls --zero behavior for column (-C, -x) and comma (-m) formats so that wrapped rows terminate with NUL, and avoids emitting double newlines.
Changes:
- Pass
line_endingthrough grid/column display paths and usewrite!instead ofwriteln!where applicable. - Stream grid output rows directly with the configured line ending (NUL vs newline) to avoid corrupting embedded newlines in filenames.
- Add regression tests covering option ordering, wrapping, width edge cases, and embedded newlines.
| File | Description |
|---|---|
| tests/by-util/test_ls.rs | Adds regression coverage for --zero with column and comma formats, including wrapping and embedded newlines. |
| src/uu/ls/src/display.rs | Implements NUL-terminated grid row streaming and fixes comma wrapping to respect configured line endings. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| cursor += total_spaces; | ||
| } | ||
| } | ||
| out.write_all(&[line_ending as u8])?; |
| { | ||
| current_col = name_width + 2; | ||
| writeln!(state.out, ",")?; | ||
| // Using write! instead of writeln! because config.line_ending already formats |
There was a problem hiding this comment.
please make it shorter, one line is enough
| } | ||
|
|
||
| /// Writes `count` spaces in fixed-size stack buffer chunks to avoid heap allocations. | ||
| fn write_spaces(out: &mut impl Write, count: usize) -> std::io::Result<()> { |
There was a problem hiding this comment.
do we really need this helper? the grid crate already keeps a padding buffer for that
|
|
||
| /// Formats and streams grid rows directly to `out`, terminating each row with `line_ending`. | ||
| /// | ||
| /// We avoid formatting the Grid to an intermediate String and replacing '\n' with '\0', |
There was a problem hiding this comment.
nobody will read such a long comment :) two lines max please
| /// because filenames under literal quoting can legitimately contain embedded newline bytes. | ||
| /// A blind string replacement would corrupt those filenames. Streaming each cell preserves | ||
| /// its exact byte contents while properly delimiting each row with `line_ending`. | ||
| fn write_grid( |
There was a problem hiding this comment.
this is a copy of the Display impl of uutils_term_grid, cursor/closest_tab/rest_spaces and all.
we maintain that crate, so please add a line separator option there instead - otherwise the two will drift and only the nul path will be wrong.
There was a problem hiding this comment.
I'll be sending a pr in that crate soon.
There was a problem hiding this comment.
@sylvestre I've opened an upsteam pr in uutils/uutils-term-grid (uutils/uutils-term-grid#68). Once that is merged and published to crates.io, I will bump the version here and finalize the PR.
| .collect() | ||
| }; | ||
|
|
||
| if names.is_empty() { |
There was a problem hiding this comment.
is this needed? Grid already returns early when cells is empty
|
|
||
| let grid = Grid::new( | ||
| names, | ||
| names.iter().map(String::as_str).collect(), |
There was a problem hiding this comment.
this collects a second vec on every call, including the plain newline case. why not keep Grid and only build the &str view when needed?
| LineEnding::Newline => { | ||
| write!(out, "{grid}")?; | ||
| } | ||
| LineEnding::Nul => { |
There was a problem hiding this comment.
two code paths for the same grid means only one of them gets tested in practice, could we have a single one?
| /// multiline row wrapping, comma format line wrapping (`-m`), | ||
| /// and filenames with embedded newlines. | ||
| #[test] | ||
| fn test_ls_zero_column_formats() { |
There was a problem hiding this comment.
one test with 10 unrelated cases is hard to debug when it fails, please split it (grid, -m, embedded newline) and drop the 6 lines header comment
When --zero precedes column formatting (-C, -x), every row should end with NUL instead of newline. Previously, the grid display unconditionally emitted \n as the row delimiter, and writeln! in display_items/display_grid caused double newlines on wrapped output.
Stream grid rows with the configured line ending directly to avoid converting to String or clobbering embedded newlines in filenames, and use write! instead of writeln! for line ending outputs.
Closes #14774