From 0dc69b4d383d485a14290521a859ea74cc3ce93a Mon Sep 17 00:00:00 2001 From: mattsu Date: Wed, 28 Jan 2026 08:46:35 +0900 Subject: [PATCH 01/13] refactor: simplify DisplayItemName struct and improve dired position handling This commit refactors the `display_item_name` function to return a `DisplayItemName` struct containing both the displayed name and its dired length, rather than just the OsString. This change simplifies the code by: 1. Eliminating the need to call `dired_name_len()` separately after getting the displayed name 2. Reducing redundant length calculations in the dired position handling 3. Making the code more maintainable by encapsulating related data together The change also fixes an issue where dired positions were being calculated incorrectly for symlink names that required quoting, ensuring proper highlighting in dired mode. --- src/uu/ls/src/ls.rs | 32 ++++++++++++++++++++------- tests/by-util/test_ls.rs | 47 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 8 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 1bad3002335..41a023a1e07 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2745,7 +2745,7 @@ fn display_items( LazyCell::new(Box::new(|| 0)), ); - names_vec.push(cell); + names_vec.push(cell.displayed); } let mut names = names_vec.into_iter(); @@ -3040,19 +3040,23 @@ fn display_item_long( })), ); - let displayed_item = if quoted && !os_str_starts_with(&item_name, b"'") { + let mut dired_name_len = item_name.dired_name_len; + let name = item_name.displayed; + let needs_space = quoted && !os_str_starts_with(&name, b"'"); + let displayed_item = if needs_space { + dired_name_len += 1; let mut ret: OsString = " ".into(); - ret.push(item_name); + ret.push(name); ret } else { - item_name + name }; if config.dired { let (start, end) = dired::calculate_dired( &dired.dired_positions, output_display.len(), - displayed_item.len(), + dired_name_len, ); dired::update_positions(dired, start, end); } @@ -3138,6 +3142,8 @@ fn display_item_long( ansi_width(&String::from_utf8_lossy(&output_display)) })), ); + let dired_name_len = displayed_item.dired_name_len; + let displayed_item = displayed_item.displayed; let date_len = 12; output_display.extend(b" "); @@ -3150,7 +3156,7 @@ fn display_item_long( dired::calculate_and_update_positions( dired, output_display.len(), - displayed_item.to_string_lossy().trim().len(), + dired_name_len, ); } write_os_str(&mut output_display, &displayed_item)?; @@ -3317,6 +3323,11 @@ fn classify_file(path: &PathData) -> Option { /// /// Note that non-unicode sequences in symlink targets are dealt with using /// [`std::path::Path::to_string_lossy`]. +struct DisplayItemName { + displayed: OsString, + dired_name_len: usize, +} + #[allow(clippy::cognitive_complexity)] fn display_item_name( path: &PathData, @@ -3325,7 +3336,7 @@ fn display_item_name( more_info: Option, state: &mut ListState, current_column: LazyCell usize + '_>>, -) -> OsString { +) -> DisplayItemName { // This is our return value. We start by `&path.display_name` and modify it along the way. let mut name = escape_name_with_locale(path.display_name(), config); @@ -3376,6 +3387,8 @@ fn display_item_name( } } + let dired_name_len = name.len(); + if config.format == Format::Long && path.file_type().is_some_and(|ft| ft.is_symlink()) && !path.must_dereference @@ -3456,7 +3469,10 @@ fn display_item_name( } } - name + DisplayItemName { + displayed: name, + dired_name_len, + } } fn create_hyperlink(name: &OsStr, path: &PathData) -> OsString { diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 41b72af6b04..6365584c620 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -5334,6 +5334,53 @@ fn test_ls_dired_simple() { assert_eq!(filename, "a1"); } +#[test] +fn test_ls_dired_symlink_name_only() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.mkdir("d"); + at.touch("d/target"); + at.symlink_file("target", "d/link"); + + let result = scene + .ucmd() + .arg("--dired") + .arg("-l") + .arg("--color=never") + .arg("d") + .succeeds(); + + let output = result.stdout_str().to_string(); + assert!(output.contains("link -> target")); + + let dired_line = output + .lines() + .find(|&line| line.starts_with("//DIRED//")) + .unwrap(); + let positions: Vec = dired_line + .split_whitespace() + .skip(1) + .map(|s| s.parse().unwrap()) + .collect(); + + assert_eq!(positions.len() % 2, 0); + + let filenames: Vec = positions + .chunks(2) + .map(|chunk| { + let start_pos = chunk[0]; + let end_pos = chunk[1]; + String::from_utf8(output.as_bytes()[start_pos..end_pos].to_vec()) + .unwrap() + .trim() + .to_string() + }) + .collect(); + + assert_eq!(filenames, vec!["link", "target"]); +} + #[test] fn test_ls_dired_complex() { let scene = TestScenario::new(util_name!()); From 438049521f0073e36f375e11c39590ac611c2031 Mon Sep 17 00:00:00 2001 From: mattsu Date: Wed, 28 Jan 2026 08:46:46 +0900 Subject: [PATCH 02/13] refactor: simplify dired position calculation call Removed unnecessary parameter from dired::calculate_and_update_positions function call, making the code cleaner and more maintainable. --- src/uu/ls/src/ls.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 41a023a1e07..c09dc6887bb 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3153,11 +3153,7 @@ fn display_item_long( output_display.extend(b" "); if config.dired { - dired::calculate_and_update_positions( - dired, - output_display.len(), - dired_name_len, - ); + dired::calculate_and_update_positions(dired, output_display.len(), dired_name_len); } write_os_str(&mut output_display, &displayed_item)?; output_display.extend(config.line_ending.to_string().as_bytes()); From 6a96cfe01ddf80023af7898d171923e25c5e6e63 Mon Sep 17 00:00:00 2001 From: mattsu Date: Wed, 28 Jan 2026 12:07:00 +0900 Subject: [PATCH 03/13] refactor: simplify dired position tracking with line offset Replace complex offset calculation logic with a dedicated line_offset field in DiredOutput. This simplifies position tracking by maintaining a running total of line lengths instead of repeatedly calculating offsets from previous positions. The change improves code clarity and reduces potential for off-by-one errors in dired position calculations. --- src/uu/ls/src/dired.rs | 78 +++++++++++++++++++++++----------------- src/uu/ls/src/ls.rs | 15 ++++++-- tests/by-util/test_ls.rs | 2 +- 3 files changed, 58 insertions(+), 37 deletions(-) diff --git a/src/uu/ls/src/dired.rs b/src/uu/ls/src/dired.rs index 69122ccd08b..974cc57bf8b 100644 --- a/src/uu/ls/src/dired.rs +++ b/src/uu/ls/src/dired.rs @@ -51,6 +51,7 @@ pub struct DiredOutput { pub dired_positions: Vec, pub subdired_positions: Vec, pub padding: usize, + pub line_offset: usize, } impl fmt::Display for BytePosition { @@ -62,21 +63,17 @@ impl fmt::Display for BytePosition { // When --dired is used, all lines starts with 2 spaces static DIRED_TRAILING_OFFSET: usize = 2; -fn get_offset_from_previous_line(dired_positions: &[BytePosition]) -> usize { - if let Some(last_position) = dired_positions.last() { - last_position.end + 1 - } else { - 0 - } +fn get_offset_from_previous_line(dired: &DiredOutput) -> usize { + dired.line_offset } /// Calculates the byte positions for DIRED pub fn calculate_dired( - dired_positions: &[BytePosition], + dired: &DiredOutput, output_display_len: usize, dfn_len: usize, ) -> (usize, usize) { - let offset_from_previous_line = get_offset_from_previous_line(dired_positions); + let offset_from_previous_line = get_offset_from_previous_line(dired); let start = output_display_len + offset_from_previous_line; let end = start + dfn_len; @@ -89,7 +86,7 @@ pub fn indent(out: &mut BufWriter) -> UResult<()> { } pub fn calculate_subdired(dired: &mut DiredOutput, path_len: usize) { - let offset_from_previous_line = get_offset_from_previous_line(&dired.dired_positions); + let offset_from_previous_line = get_offset_from_previous_line(dired); let additional_offset = if dired.subdired_positions.is_empty() { 0 @@ -131,7 +128,7 @@ fn print_positions(prefix: &str, positions: &Vec) { pub fn add_total(dired: &mut DiredOutput, total_len: usize) { if dired.padding == 0 { - let offset_from_previous_line = get_offset_from_previous_line(&dired.dired_positions); + let offset_from_previous_line = get_offset_from_previous_line(dired); // when dealing with " total: xx", it isn't part of the //DIRED// // so, we just keep the size line to add it to the position of the next file dired.padding = total_len + offset_from_previous_line + DIRED_TRAILING_OFFSET; @@ -154,27 +151,25 @@ pub fn calculate_and_update_positions( dired: &mut DiredOutput, output_display_len: usize, dfn_len: usize, + line_len: usize, ) { - let offset = dired - .dired_positions - .last() - .map_or(DIRED_TRAILING_OFFSET, |last_position| { - last_position.start + DIRED_TRAILING_OFFSET - }); - let start = output_display_len + offset + DIRED_TRAILING_OFFSET; + let offset_from_previous_line = get_offset_from_previous_line(dired); + let start = output_display_len + offset_from_previous_line; let end = start + dfn_len; - update_positions(dired, start, end); + update_positions(dired, start, end, line_len); } /// Updates the dired positions based on the given start and end positions. /// update when it is the first element in the list (to manage "total X") /// insert when it isn't the about total -pub fn update_positions(dired: &mut DiredOutput, start: usize, end: usize) { +pub fn update_positions(dired: &mut DiredOutput, start: usize, end: usize, line_len: usize) { // padding can be 0 but as it doesn't matter + let padding = dired.padding; dired.dired_positions.push(BytePosition { - start: start + dired.padding, - end: end + dired.padding, + start: start + padding, + end: end + padding, }); + dired.line_offset = dired.line_offset + padding + line_len; // Remove the previous padding dired.padding = 0; } @@ -194,8 +189,13 @@ mod tests { fn test_calculate_dired() { let output_display = "sample_output".to_string(); let dfn = "sample_file".to_string(); - let dired_positions = vec![BytePosition { start: 5, end: 10 }]; - let (start, end) = calculate_dired(&dired_positions, output_display.len(), dfn.len()); + let dired = DiredOutput { + dired_positions: vec![BytePosition { start: 5, end: 10 }], + subdired_positions: vec![], + padding: 0, + line_offset: 11, + }; + let (start, end) = calculate_dired(&dired, output_display.len(), dfn.len()); assert_eq!(start, 24); assert_eq!(end, 35); @@ -203,12 +203,17 @@ mod tests { #[test] fn test_get_offset_from_previous_line() { - let positions = vec![ - BytePosition { start: 0, end: 3 }, - BytePosition { start: 4, end: 7 }, - BytePosition { start: 8, end: 11 }, - ]; - assert_eq!(get_offset_from_previous_line(&positions), 12); + let dired = DiredOutput { + dired_positions: vec![ + BytePosition { start: 0, end: 3 }, + BytePosition { start: 4, end: 7 }, + BytePosition { start: 8, end: 11 }, + ], + subdired_positions: vec![], + padding: 0, + line_offset: 12, + }; + assert_eq!(get_offset_from_previous_line(&dired), 12); } #[test] fn test_calculate_subdired() { @@ -220,6 +225,7 @@ mod tests { ], subdired_positions: vec![], padding: 0, + line_offset: 12, }; let path_len = 5; calculate_subdired(&mut dired, path_len); @@ -239,6 +245,7 @@ mod tests { ], subdired_positions: vec![], padding: 0, + line_offset: 0, }; let dir_len = 5; add_dir_name(&mut dired, dir_len); @@ -252,7 +259,8 @@ mod tests { ], subdired_positions: vec![], // 8 = 1 for the \n + 5 for dir_len + 2 for " " + 1 for : - padding: 8 + padding: 8, + line_offset: 0, } ); } @@ -267,6 +275,7 @@ mod tests { ], subdired_positions: vec![], padding: 0, + line_offset: 12, }; // if we have "total: 2" let total_len = 8; @@ -290,6 +299,7 @@ mod tests { ], subdired_positions: vec![], padding: 0, + line_offset: 12, }; let dir_len = 5; add_dir_name(&mut dired, dir_len); @@ -307,16 +317,17 @@ mod tests { dired_positions: vec![BytePosition { start: 5, end: 10 }], subdired_positions: vec![], padding: 10, + line_offset: 0, }; // Test with adjust = true - update_positions(&mut dired, 15, 20); + update_positions(&mut dired, 15, 20, 1); let last_position = dired.dired_positions.last().unwrap(); assert_eq!(last_position.start, 25); // 15 + 10 (end of the previous position) assert_eq!(last_position.end, 30); // 20 + 10 (end of the previous position) // Test with adjust = false - update_positions(&mut dired, 30, 35); + update_positions(&mut dired, 30, 35, 1); let last_position = dired.dired_positions.last().unwrap(); assert_eq!(last_position.start, 30); assert_eq!(last_position.end, 35); @@ -332,10 +343,11 @@ mod tests { ], subdired_positions: vec![], padding: 5, + line_offset: 12, }; let output_display_len = 15; let dfn_len = 5; - calculate_and_update_positions(&mut dired, output_display_len, dfn_len); + calculate_and_update_positions(&mut dired, output_display_len, dfn_len, 20); assert_eq!( dired.dired_positions, vec![ diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index c09dc6887bb..25f2069b563 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3053,12 +3053,14 @@ fn display_item_long( }; if config.dired { + let line_len = + output_display.len() + displayed_item.len() + config.line_ending.to_string().len(); let (start, end) = dired::calculate_dired( - &dired.dired_positions, + dired, output_display.len(), dired_name_len, ); - dired::update_positions(dired, start, end); + dired::update_positions(dired, start, end, line_len); } write_os_str(&mut output_display, &displayed_item)?; output_display.extend(config.line_ending.to_string().as_bytes()); @@ -3153,7 +3155,14 @@ fn display_item_long( output_display.extend(b" "); if config.dired { - dired::calculate_and_update_positions(dired, output_display.len(), dired_name_len); + let line_len = + output_display.len() + displayed_item.len() + config.line_ending.to_string().len(); + dired::calculate_and_update_positions( + dired, + output_display.len(), + dired_name_len, + line_len, + ); } write_os_str(&mut output_display, &displayed_item)?; output_display.extend(config.line_ending.to_string().as_bytes()); diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 6365584c620..82cafa71e73 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -5341,7 +5341,7 @@ fn test_ls_dired_symlink_name_only() { at.mkdir("d"); at.touch("d/target"); - at.symlink_file("target", "d/link"); + at.relative_symlink_file("target", "d/link"); let result = scene .ucmd() From e0eb24c531389d6735c811ed082250296cb3916f Mon Sep 17 00:00:00 2001 From: mattsu Date: Wed, 28 Jan 2026 18:49:43 +0900 Subject: [PATCH 04/13] refactor: simplify dired calculation call in display_item_long Removed unnecessary line length calculation before calling dired::calculate_dired, as the function doesn't use this parameter. The line_len variable was calculated but never used within the dired::calculate_dired function call. --- src/uu/ls/src/ls.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 25f2069b563..a18af1e2e2e 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3055,11 +3055,7 @@ fn display_item_long( if config.dired { let line_len = output_display.len() + displayed_item.len() + config.line_ending.to_string().len(); - let (start, end) = dired::calculate_dired( - dired, - output_display.len(), - dired_name_len, - ); + let (start, end) = dired::calculate_dired(dired, output_display.len(), dired_name_len); dired::update_positions(dired, start, end, line_len); } write_os_str(&mut output_display, &displayed_item)?; From b4e453ce6c317ac55119c3f574cc4ea7bb517180 Mon Sep 17 00:00:00 2001 From: mattsu Date: Wed, 28 Jan 2026 20:41:08 +0900 Subject: [PATCH 05/13] refactor: simplify dired offset calculations and remove unused helper function Removed the `get_offset_from_previous_line()` helper function and replaced its usage with direct access to `dired.line_offset`. This simplifies the code by eliminating an unnecessary abstraction layer while maintaining the same functionality. The change affects multiple functions that calculate byte positions for DIRED output formatting. --- src/uu/ls/src/dired.rs | 57 +++++++++++------------------------------- src/uu/ls/src/ls.rs | 34 ++++++++++++------------- 2 files changed, 32 insertions(+), 59 deletions(-) diff --git a/src/uu/ls/src/dired.rs b/src/uu/ls/src/dired.rs index 974cc57bf8b..bbab0466de6 100644 --- a/src/uu/ls/src/dired.rs +++ b/src/uu/ls/src/dired.rs @@ -63,17 +63,13 @@ impl fmt::Display for BytePosition { // When --dired is used, all lines starts with 2 spaces static DIRED_TRAILING_OFFSET: usize = 2; -fn get_offset_from_previous_line(dired: &DiredOutput) -> usize { - dired.line_offset -} - /// Calculates the byte positions for DIRED pub fn calculate_dired( dired: &DiredOutput, output_display_len: usize, dfn_len: usize, ) -> (usize, usize) { - let offset_from_previous_line = get_offset_from_previous_line(dired); + let offset_from_previous_line = dired.line_offset; let start = output_display_len + offset_from_previous_line; let end = start + dfn_len; @@ -86,16 +82,8 @@ pub fn indent(out: &mut BufWriter) -> UResult<()> { } pub fn calculate_subdired(dired: &mut DiredOutput, path_len: usize) { - let offset_from_previous_line = get_offset_from_previous_line(dired); - - let additional_offset = if dired.subdired_positions.is_empty() { - 0 - } else { - // if we have several directories: \n\n - 2 - }; - - let start = offset_from_previous_line + DIRED_TRAILING_OFFSET + additional_offset; + let offset_from_previous_line = dired.line_offset + dired.padding; + let start = offset_from_previous_line + DIRED_TRAILING_OFFSET; let end = start + path_len; dired.subdired_positions.push(BytePosition { start, end }); } @@ -110,7 +98,7 @@ pub fn print_dired_output( if !dired.dired_positions.is_empty() { print_positions("//DIRED//", &dired.dired_positions); } - if config.recursive { + if !dired.subdired_positions.is_empty() { print_positions("//SUBDIRED//", &dired.subdired_positions); } println!("//DIRED-OPTIONS// --quoting-style={}", config.quoting_style); @@ -128,10 +116,9 @@ fn print_positions(prefix: &str, positions: &Vec) { pub fn add_total(dired: &mut DiredOutput, total_len: usize) { if dired.padding == 0 { - let offset_from_previous_line = get_offset_from_previous_line(dired); // when dealing with " total: xx", it isn't part of the //DIRED// // so, we just keep the size line to add it to the position of the next file - dired.padding = total_len + offset_from_previous_line + DIRED_TRAILING_OFFSET; + dired.padding = total_len + DIRED_TRAILING_OFFSET; } else { // += because if we are in -R, we have " dir:\n total X". So, we need to take the // previous padding too. @@ -142,8 +129,8 @@ pub fn add_total(dired: &mut DiredOutput, total_len: usize) { // when using -R, we have the dirname. we need to add it to the padding pub fn add_dir_name(dired: &mut DiredOutput, dir_len: usize) { - // 1 for the ":" in " dirname:" - dired.padding += dir_len + DIRED_TRAILING_OFFSET + 1; + // " dirname:\n" + dired.padding += dir_len + DIRED_TRAILING_OFFSET + 2; } /// Calculates byte positions and updates the dired structure. @@ -153,7 +140,7 @@ pub fn calculate_and_update_positions( dfn_len: usize, line_len: usize, ) { - let offset_from_previous_line = get_offset_from_previous_line(dired); + let offset_from_previous_line = dired.line_offset; let start = output_display_len + offset_from_previous_line; let end = start + dfn_len; update_positions(dired, start, end, line_len); @@ -201,20 +188,6 @@ mod tests { assert_eq!(end, 35); } - #[test] - fn test_get_offset_from_previous_line() { - let dired = DiredOutput { - dired_positions: vec![ - BytePosition { start: 0, end: 3 }, - BytePosition { start: 4, end: 7 }, - BytePosition { start: 8, end: 11 }, - ], - subdired_positions: vec![], - padding: 0, - line_offset: 12, - }; - assert_eq!(get_offset_from_previous_line(&dired), 12); - } #[test] fn test_calculate_subdired() { let mut dired = DiredOutput { @@ -258,8 +231,8 @@ mod tests { BytePosition { start: 8, end: 11 }, ], subdired_positions: vec![], - // 8 = 1 for the \n + 5 for dir_len + 2 for " " + 1 for : - padding: 8, + // 9 = 5 for dir_len + 2 for " " + 1 for : + 1 for \n + padding: 9, line_offset: 0, } ); @@ -280,8 +253,8 @@ mod tests { // if we have "total: 2" let total_len = 8; add_total(&mut dired, total_len); - // 22 = 8 (len) + 2 (padding) + 11 (previous position) + 1 (\n) - assert_eq!(dired.padding, 22); + // 10 = 8 (len) + 2 (" ") + assert_eq!(dired.padding, 10); } #[test] @@ -303,12 +276,12 @@ mod tests { }; let dir_len = 5; add_dir_name(&mut dired, dir_len); - // 8 = 2 (" ") + 1 (\n) + 5 + 1 (: of dirname) - assert_eq!(dired.padding, 8); + // 9 = 2 (" ") + 1 (\n) + 5 + 1 (: of dirname) + assert_eq!(dired.padding, 9); let total_len = 8; add_total(&mut dired, total_len); - assert_eq!(dired.padding, 18); + assert_eq!(dired.padding, 19); } #[test] diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index a18af1e2e2e..93e34d67601 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2283,24 +2283,24 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { // Print dir heading - name... 'total' comes after error display if initial_locs_len > 1 || config.recursive { - if pos.eq(&0usize) && files.is_empty() { - if config.dired { - dired::indent(&mut state.out)?; - } - show_dir_name(path_data, &mut state.out, config)?; + let needs_blank_line = !(pos.eq(&0usize) && files.is_empty()); + if needs_blank_line { writeln!(state.out)?; if config.dired { - // First directory displayed - let dir_len = path_data.display_name().len(); - // add the //SUBDIRED// coordinates - dired::calculate_subdired(&mut dired, dir_len); - // Add the padding for the dir name - dired::add_dir_name(&mut dired, dir_len); + dired.padding += 1; } - } else { - writeln!(state.out)?; - show_dir_name(path_data, &mut state.out, config)?; - writeln!(state.out)?; + } + if config.dired { + dired::indent(&mut state.out)?; + } + show_dir_name(path_data, &mut state.out, config)?; + writeln!(state.out)?; + if config.dired { + let dir_len = path_data.display_name().len(); + // add the //SUBDIRED// coordinates + dired::calculate_subdired(&mut dired, dir_len); + // Add the padding for the dir name + dired::add_dir_name(&mut dired, dir_len); } } let mut listed_ancestors = HashSet::default(); @@ -2522,8 +2522,8 @@ fn enter_directory( if config.dired { // We already injected the first dir // Continue with the others - // 2 = \n + \n - dired.padding = 2; + // blank line between directory sections + dired.padding += 1; dired::indent(&mut state.out)?; let dir_name_size = e.path().as_os_str().len(); dired::calculate_subdired(dired, dir_name_size); From cd1711ae301d0ea5ad0ac160d089cbc0fddb2933 Mon Sep 17 00:00:00 2001 From: mattsu Date: Wed, 28 Jan 2026 21:05:18 +0900 Subject: [PATCH 06/13] chore: disable ls stat-failed test modification Reverts a sed command that was adjusting line numbers in the ls stat-failed test, likely due to changes in test output or structure that made the line number adjustment unnecessary or incorrect. --- util/build-gnu.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/build-gnu.sh b/util/build-gnu.sh index 0d1c6d9e114..2aaba6db950 100755 --- a/util/build-gnu.sh +++ b/util/build-gnu.sh @@ -297,7 +297,7 @@ sed -i '1s/^/exit 0 # Skip test - uutils du uses safe traversal that prevents t awk 'BEGIN {count=0} /compare exp out2/ && count < 6 {sub(/compare exp out2/, "grep -q \"cannot be used with\" out2"); count++} 1' tests/df/df-output.sh > tests/df/df-output.sh.tmp && mv tests/df/df-output.sh.tmp tests/df/df-output.sh # with ls --dired, in case of error, we have a slightly different error position -sed -i -e "s|44 45|48 49|" tests/ls/stat-failed.sh +#sed -i -e "s|44 45|48 49|" tests/ls/stat-failed.sh # small difference in the error message sed -i -e "s/ls: invalid argument 'XX' for 'time style'/ls: invalid --time-style argument 'XX'/" \ From f8462cbcd21aed57fbbe80d51369e8096c51774d Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 29 Jan 2026 18:31:47 +0900 Subject: [PATCH 07/13] fix: ensure SUBDIRED positions are printed for multiple directories without -R This commit fixes an issue where `//SUBDIRED//` positions were not being printed when using `--dired` with multiple directory arguments but without the `-R` (recursive) flag. The SUBDIRED positions are needed whenever directory headings are printed, which occurs with multiple arguments regardless of whether recursion is enabled. --- src/uu/ls/src/dired.rs | 2 ++ tests/by-util/test_ls.rs | 45 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/src/uu/ls/src/dired.rs b/src/uu/ls/src/dired.rs index bbab0466de6..c99ac0f1371 100644 --- a/src/uu/ls/src/dired.rs +++ b/src/uu/ls/src/dired.rs @@ -98,6 +98,8 @@ pub fn print_dired_output( if !dired.dired_positions.is_empty() { print_positions("//DIRED//", &dired.dired_positions); } + // SUBDIRED is needed whenever directory headings are printed (multiple args or -R), + // so don't gate it on config.recursive. if !dired.subdired_positions.is_empty() { print_positions("//SUBDIRED//", &dired.subdired_positions); } diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 82cafa71e73..98e2bc2c492 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -5205,6 +5205,51 @@ fn test_ls_dired_recursive() { .stdout_contains("//DIRED-OPTIONS// --quoting-style"); } +#[test] +fn test_ls_dired_subdired_multiple_dirs_non_recursive() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.mkdir("dir1"); + at.mkdir("dir2"); + at.touch("dir1/a"); + at.touch("dir2/b"); + + let result = scene + .ucmd() + .arg("--dired") + .arg("dir1") + .arg("dir2") + .succeeds(); + + let output = result.stdout_str().to_string(); + let subdired_line = output + .lines() + .find(|&line| line.starts_with("//SUBDIRED//")) + .unwrap(); + let positions: Vec = subdired_line + .split_whitespace() + .skip(1) + .map(|s| s.parse().unwrap()) + .collect(); + + assert_eq!(positions.len() % 2, 0); // Ensure there's an even number of positions + + let dirnames: Vec = positions + .chunks(2) + .map(|chunk| { + let start_pos = chunk[0]; + let end_pos = chunk[1]; + String::from_utf8(output.as_bytes()[start_pos..end_pos].to_vec()) + .unwrap() + .trim() + .to_string() + }) + .collect(); + + assert_eq!(dirnames, vec!["dir1", "dir2"]); +} + #[test] fn test_ls_dired_outputs_parent_offset() { let scene = TestScenario::new(util_name!()); From 005d1d3841e766f96fdb929367ecfde6b0bcc53d Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 29 Jan 2026 18:40:57 +0900 Subject: [PATCH 08/13] refactor: rename DisplayItemName struct and optimize dired_name_len calculation The commit renames the `DisplayItemName` struct to avoid conflicts and optimizes the `dired_name_len` calculation by only computing it when dired mode is enabled, improving performance for non-dired displays. --- src/uu/ls/src/ls.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 93e34d67601..a0d43a52f7b 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -408,6 +408,11 @@ struct PaddingCollection { block_size: usize, } +struct DisplayItemName { + displayed: OsString, + dired_name_len: usize, +} + /// Extracts the format to display the information based on the options provided. /// /// # Returns @@ -3029,7 +3034,7 @@ fn display_item_long( display_date(md, config, state, &mut output_display)?; output_display.extend(b" "); - let item_name = display_item_name( + let item_display = display_item_name( item, config, None, @@ -3040,16 +3045,16 @@ fn display_item_long( })), ); - let mut dired_name_len = item_name.dired_name_len; - let name = item_name.displayed; - let needs_space = quoted && !os_str_starts_with(&name, b"'"); + let mut dired_name_len = item_display.dired_name_len; + let item_name = item_display.displayed; + let needs_space = quoted && !os_str_starts_with(&item_name, b"'"); let displayed_item = if needs_space { dired_name_len += 1; let mut ret: OsString = " ".into(); - ret.push(name); + ret.push(&item_name); ret } else { - name + item_name }; if config.dired { @@ -3324,11 +3329,6 @@ fn classify_file(path: &PathData) -> Option { /// /// Note that non-unicode sequences in symlink targets are dealt with using /// [`std::path::Path::to_string_lossy`]. -struct DisplayItemName { - displayed: OsString, - dired_name_len: usize, -} - #[allow(clippy::cognitive_complexity)] fn display_item_name( path: &PathData, @@ -3388,7 +3388,7 @@ fn display_item_name( } } - let dired_name_len = name.len(); + let dired_name_len = if config.dired { name.len() } else { 0 }; if config.format == Format::Long && path.file_type().is_some_and(|ft| ft.is_symlink()) From e23057644c5e548f40ffe4f1f3590a55842cc664 Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 29 Jan 2026 18:42:08 +0900 Subject: [PATCH 09/13] chore: remove obsolete sed command for ls --dired test Removed commented sed command that was skipping a test for ls --dired functionality. The command was already commented out and no longer needed, cleaning up the build script. --- util/build-gnu.sh | 3 --- 1 file changed, 3 deletions(-) diff --git a/util/build-gnu.sh b/util/build-gnu.sh index 2aaba6db950..eb8ff06c2ff 100755 --- a/util/build-gnu.sh +++ b/util/build-gnu.sh @@ -296,9 +296,6 @@ sed -i '1s/^/exit 0 # Skip test - uutils du uses safe traversal that prevents t awk 'BEGIN {count=0} /compare exp out2/ && count < 6 {sub(/compare exp out2/, "grep -q \"cannot be used with\" out2"); count++} 1' tests/df/df-output.sh > tests/df/df-output.sh.tmp && mv tests/df/df-output.sh.tmp tests/df/df-output.sh -# with ls --dired, in case of error, we have a slightly different error position -#sed -i -e "s|44 45|48 49|" tests/ls/stat-failed.sh - # small difference in the error message sed -i -e "s/ls: invalid argument 'XX' for 'time style'/ls: invalid --time-style argument 'XX'/" \ -e "s/Valid arguments are:/Possible values are:/" \ From 0b59422013336e48b3b75471051d289ea24cdf80 Mon Sep 17 00:00:00 2001 From: mattsu Date: Wed, 4 Feb 2026 20:39:54 +0900 Subject: [PATCH 10/13] refactor: extract line length calculation into dedicated function Extracted the repeated line length calculation logic into a dedicated `calculate_line_len` function to improve code maintainability and reduce duplication. The function takes the output length, item length, and line ending as parameters and returns the total line length including the line ending characters. --- src/uu/ls/src/ls.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index a0d43a52f7b..e74892cf0bb 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2914,6 +2914,10 @@ fn display_grid( Ok(()) } +fn calculate_line_len(output_len: usize, item_len: usize, line_ending: &LineEnding) -> usize { + output_len + item_len + line_ending.to_string().len() +} + /// This writes to the [`BufWriter`] `state.out` a single string of the output of `ls -l`. /// /// It writes the following keys, in order: @@ -3058,8 +3062,11 @@ fn display_item_long( }; if config.dired { - let line_len = - output_display.len() + displayed_item.len() + config.line_ending.to_string().len(); + let line_len = calculate_line_len( + output_display.len(), + displayed_item.len(), + &config.line_ending, + ); let (start, end) = dired::calculate_dired(dired, output_display.len(), dired_name_len); dired::update_positions(dired, start, end, line_len); } @@ -3156,8 +3163,11 @@ fn display_item_long( output_display.extend(b" "); if config.dired { - let line_len = - output_display.len() + displayed_item.len() + config.line_ending.to_string().len(); + let line_len = calculate_line_len( + output_display.len(), + displayed_item.len(), + &config.line_ending, + ); dired::calculate_and_update_positions( dired, output_display.len(), From 2bf339d809cf2084fc7481cb9974790dd21be79e Mon Sep 17 00:00:00 2001 From: mattsu Date: Wed, 4 Feb 2026 20:51:31 +0900 Subject: [PATCH 11/13] refactor: simplify dired position calculation and item display logic This commit refactors the dired position calculation and item display logic in the ls utility to improve code clarity and reduce redundancy. The changes include: - Simplifying the dired position calculation by computing the displayed length directly - Moving the space insertion logic closer to the dired calculation - Reducing variable assignments and improving the flow of item display processing - Maintaining the same functionality while making the code more readable and maintainable The refactoring maintains backward compatibility and preserves all existing behavior while making the code easier to understand and modify. --- src/uu/ls/src/ls.rs | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index e74892cf0bb..1e3b27980ec 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3049,27 +3049,32 @@ fn display_item_long( })), ); - let mut dired_name_len = item_display.dired_name_len; - let item_name = item_display.displayed; - let needs_space = quoted && !os_str_starts_with(&item_name, b"'"); - let displayed_item = if needs_space { - dired_name_len += 1; - let mut ret: OsString = " ".into(); - ret.push(&item_name); - ret - } else { - item_name - }; + let needs_space = quoted && !os_str_starts_with(&item_display.displayed, b"'"); if config.dired { + let mut dired_name_len = item_display.dired_name_len; + if needs_space { + dired_name_len += 1; + } + let displayed_len = item_display.displayed.len() + usize::from(needs_space); let line_len = calculate_line_len( output_display.len(), - displayed_item.len(), + displayed_len, &config.line_ending, ); let (start, end) = dired::calculate_dired(dired, output_display.len(), dired_name_len); dired::update_positions(dired, start, end, line_len); } + + let item_name = item_display.displayed; + let displayed_item = if needs_space { + let mut ret: OsString = " ".into(); + ret.push(&item_name); + ret + } else { + item_name + }; + write_os_str(&mut output_display, &displayed_item)?; output_display.extend(config.line_ending.to_string().as_bytes()); } else { @@ -3152,8 +3157,6 @@ fn display_item_long( ansi_width(&String::from_utf8_lossy(&output_display)) })), ); - let dired_name_len = displayed_item.dired_name_len; - let displayed_item = displayed_item.displayed; let date_len = 12; output_display.extend(b" "); @@ -3165,16 +3168,17 @@ fn display_item_long( if config.dired { let line_len = calculate_line_len( output_display.len(), - displayed_item.len(), + displayed_item.displayed.len(), &config.line_ending, ); dired::calculate_and_update_positions( dired, output_display.len(), - dired_name_len, + displayed_item.dired_name_len, line_len, ); } + let displayed_item = displayed_item.displayed; write_os_str(&mut output_display, &displayed_item)?; output_display.extend(config.line_ending.to_string().as_bytes()); } From 2237754f89e25c58d530cd842277ba20d372fa14 Mon Sep 17 00:00:00 2001 From: mattsu Date: Wed, 4 Feb 2026 21:51:24 +0900 Subject: [PATCH 12/13] refactor: extract dired position update logic into dedicated function Extracted the repeated dired position calculation and update logic into a new `update_dired_for_item` function to reduce code duplication and improve maintainability. The function consolidates the calculation of line length and subsequent dired position updates that were previously duplicated across multiple locations in the `display_item_long` function. --- src/uu/ls/src/ls.rs | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 1e3b27980ec..04185be0e85 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2918,6 +2918,17 @@ fn calculate_line_len(output_len: usize, item_len: usize, line_ending: &LineEndi output_len + item_len + line_ending.to_string().len() } +fn update_dired_for_item( + dired: &mut DiredOutput, + output_display_len: usize, + displayed_len: usize, + dired_name_len: usize, + line_ending: &LineEnding, +) { + let line_len = calculate_line_len(output_display_len, displayed_len, line_ending); + dired::calculate_and_update_positions(dired, output_display_len, dired_name_len, line_len); +} + /// This writes to the [`BufWriter`] `state.out` a single string of the output of `ls -l`. /// /// It writes the following keys, in order: @@ -3057,13 +3068,13 @@ fn display_item_long( dired_name_len += 1; } let displayed_len = item_display.displayed.len() + usize::from(needs_space); - let line_len = calculate_line_len( + update_dired_for_item( + dired, output_display.len(), displayed_len, + dired_name_len, &config.line_ending, ); - let (start, end) = dired::calculate_dired(dired, output_display.len(), dired_name_len); - dired::update_positions(dired, start, end, line_len); } let item_name = item_display.displayed; @@ -3166,16 +3177,12 @@ fn display_item_long( output_display.extend(b" "); if config.dired { - let line_len = calculate_line_len( - output_display.len(), - displayed_item.displayed.len(), - &config.line_ending, - ); - dired::calculate_and_update_positions( + update_dired_for_item( dired, output_display.len(), + displayed_item.displayed.len(), displayed_item.dired_name_len, - line_len, + &config.line_ending, ); } let displayed_item = displayed_item.displayed; From 6de0f3ef5e97611a88a4aa986680a5e39d3b7489 Mon Sep 17 00:00:00 2001 From: mattsu Date: Mon, 9 Feb 2026 18:51:43 +0900 Subject: [PATCH 13/13] refactor: simplify calculate_line_len and update_dired_for_item parameter types Replace `&LineEnding` references with owned `LineEnding` values in `calculate_line_len` and `update_dired_for_item` functions. This change simplifies the API by avoiding unnecessary references to the enum type and makes the code more consistent with Rust ownership patterns. The functions now take the enum value directly rather than a reference to it. --- src/uu/ls/src/dired.rs | 4 +--- src/uu/ls/src/ls.rs | 8 ++++---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/uu/ls/src/dired.rs b/src/uu/ls/src/dired.rs index c99ac0f1371..4d96c4d9c27 100644 --- a/src/uu/ls/src/dired.rs +++ b/src/uu/ls/src/dired.rs @@ -142,9 +142,7 @@ pub fn calculate_and_update_positions( dfn_len: usize, line_len: usize, ) { - let offset_from_previous_line = dired.line_offset; - let start = output_display_len + offset_from_previous_line; - let end = start + dfn_len; + let (start, end) = calculate_dired(dired, output_display_len, dfn_len); update_positions(dired, start, end, line_len); } diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 78765ffe149..428de55a9d8 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2913,7 +2913,7 @@ fn display_grid( Ok(()) } -fn calculate_line_len(output_len: usize, item_len: usize, line_ending: &LineEnding) -> usize { +fn calculate_line_len(output_len: usize, item_len: usize, line_ending: LineEnding) -> usize { output_len + item_len + line_ending.to_string().len() } @@ -2922,7 +2922,7 @@ fn update_dired_for_item( output_display_len: usize, displayed_len: usize, dired_name_len: usize, - line_ending: &LineEnding, + line_ending: LineEnding, ) { let line_len = calculate_line_len(output_display_len, displayed_len, line_ending); dired::calculate_and_update_positions(dired, output_display_len, dired_name_len, line_len); @@ -3072,7 +3072,7 @@ fn display_item_long( output_display.len(), displayed_len, dired_name_len, - &config.line_ending, + config.line_ending, ); } @@ -3181,7 +3181,7 @@ fn display_item_long( output_display.len(), displayed_item.displayed.len(), displayed_item.dired_name_len, - &config.line_ending, + config.line_ending, ); } let displayed_item = displayed_item.displayed;