ls --dired -R: fix the positions (#5341)

* move get_offset_from_previous_line into a specific function

* dired: improve the -R support

* dired: fix the display with subdir

* ls --dired -R: fix the positions

* ls --dired -R: verify also the SUBDIRED coordinate

* ls --dired -R: add a long file name and fix a windows test

* dired: always put dired first in the args + minor fixes

Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>

* ls: add cognitive_complexity to silent a warning

---------

Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>

3 files changed, 312 insertions, 35 deletions

diff --git a/src/uu/ls/src/dired.rs b/src/uu/ls/src/dired.rs
index 8574e750e..74732d37a 100644
--- a/src/uu/ls/src/dired.rs
+++ b/src/uu/ls/src/dired.rs
@@ -9,14 +9,14 @@ use std::fmt;
 use std::io::{BufWriter, Stdout, Write};
 use uucore::error::UResult;
 
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq)]
 pub struct BytePosition {
     pub start: usize,
     pub end: usize,
 }
 
 /// Represents the output structure for DIRED, containing positions for both DIRED and SUBDIRED.
-#[derive(Debug, Clone, Default)]
+#[derive(Debug, Clone, Default, PartialEq)]
 pub struct DiredOutput {
     pub dired_positions: Vec<BytePosition>,
     pub subdired_positions: Vec<BytePosition>,
@@ -32,17 +32,21 @@ 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
+    }
+}
+
 /// Calculates the byte positions for DIRED
 pub fn calculate_dired(
+    dired_positions: &[BytePosition],
     output_display_len: usize,
     dfn_len: usize,
-    dired_positions: &[BytePosition],
 ) -> (usize, usize) {
-    let offset_from_previous_line = if let Some(last_position) = dired_positions.last() {
-        last_position.end + 1
-    } else {
-        0
-    };
+    let offset_from_previous_line = get_offset_from_previous_line(dired_positions);
 
     let start = output_display_len + offset_from_previous_line;
     let end = start + dfn_len;
@@ -55,15 +59,18 @@ pub fn indent(out: &mut BufWriter<Stdout>) -> UResult<()> {
 }
 
 pub fn calculate_subdired(dired: &mut DiredOutput, path_len: usize) {
-    let offset = if dired.subdired_positions.is_empty() {
-        DIRED_TRAILING_OFFSET
+    let offset_from_previous_line = get_offset_from_previous_line(&dired.dired_positions);
+
+    let additional_offset = if dired.subdired_positions.is_empty() {
+        0
     } else {
-        dired.subdired_positions[dired.subdired_positions.len() - 1].start + DIRED_TRAILING_OFFSET
+        // if we have several directories: \n\n
+        2
     };
-    dired.subdired_positions.push(BytePosition {
-        start: offset,
-        end: path_len + offset,
-    });
+
+    let start = offset_from_previous_line + DIRED_TRAILING_OFFSET + additional_offset;
+    let end = start + path_len;
+    dired.subdired_positions.push(BytePosition { start, end });
 }
 
 /// Prints the dired output based on the given configuration and dired structure.
@@ -73,10 +80,11 @@ pub fn print_dired_output(
     out: &mut BufWriter<Stdout>,
 ) -> UResult<()> {
     out.flush()?;
+    if dired.padding == 0 && !dired.dired_positions.is_empty() {
+        print_positions("//DIRED//", &dired.dired_positions);
+    }
     if config.recursive {
         print_positions("//SUBDIRED//", &dired.subdired_positions);
-    } else if dired.padding == 0 {
-        print_positions("//DIRED//", &dired.dired_positions);
     }
     println!("//DIRED-OPTIONS// --quoting-style={}", config.quoting_style);
     Ok(())
@@ -91,17 +99,31 @@ fn print_positions(prefix: &str, positions: &Vec<BytePosition>) {
     println!();
 }
 
-pub fn add_total(total_len: usize, dired: &mut DiredOutput) {
-    // 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 + DIRED_TRAILING_OFFSET;
+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);
+        // 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;
+    } else {
+        // += because if we are in -R, we have "  dir:\n  total X". So, we need to take the
+        // previous padding too.
+        // and we already have the previous position in mind
+        dired.padding += total_len + DIRED_TRAILING_OFFSET;
+    }
+}
+
+// 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;
 }
 
 /// Calculates byte positions and updates the dired structure.
 pub fn calculate_and_update_positions(
+    dired: &mut DiredOutput,
     output_display_len: usize,
     dfn_len: usize,
-    dired: &mut DiredOutput,
 ) {
     let offset = dired
         .dired_positions
@@ -111,14 +133,14 @@ pub fn calculate_and_update_positions(
         });
     let start = output_display_len + offset + DIRED_TRAILING_OFFSET;
     let end = start + dfn_len;
-    update_positions(start, end, dired);
+    update_positions(dired, start, end);
 }
 
 /// 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(start: usize, end: usize, dired: &mut DiredOutput) {
-    // padding can be 0 but as it doesn't matter<
+pub fn update_positions(dired: &mut DiredOutput, start: usize, end: usize) {
+    // padding can be 0 but as it doesn't matter
     dired.dired_positions.push(BytePosition {
         start: start + dired.padding,
         end: end + dired.padding,
@@ -136,13 +158,113 @@ mod tests {
         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(output_display.len(), dfn.len(), &dired_positions);
+        let (start, end) = calculate_dired(&dired_positions, output_display.len(), dfn.len());
 
         assert_eq!(start, 24);
         assert_eq!(end, 35);
     }
 
     #[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);
+    }
+    #[test]
+    fn test_calculate_subdired() {
+        let mut 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,
+        };
+        let path_len = 5;
+        calculate_subdired(&mut dired, path_len);
+        assert_eq!(
+            dired.subdired_positions,
+            vec![BytePosition { start: 14, end: 19 }],
+        );
+    }
+
+    #[test]
+    fn test_add_dir_name() {
+        let mut 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,
+        };
+        let dir_len = 5;
+        add_dir_name(&mut dired, dir_len);
+        assert_eq!(
+            dired,
+            DiredOutput {
+                dired_positions: vec![
+                    BytePosition { start: 0, end: 3 },
+                    BytePosition { start: 4, end: 7 },
+                    BytePosition { start: 8, end: 11 },
+                ],
+                subdired_positions: vec![],
+                // 8 = 1 for the \n + 5 for dir_len + 2 for "  " + 1 for :
+                padding: 8
+            }
+        );
+    }
+
+    #[test]
+    fn test_add_total() {
+        let mut 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,
+        };
+        // 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);
+    }
+
+    #[test]
+    fn test_add_dir_name_and_total() {
+        // test when we have
+        //   dirname:
+        //   total 0
+        //   -rw-r--r-- 1 sylvestre sylvestre 0 Sep 30 09:41 ab
+
+        let mut 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,
+        };
+        let dir_len = 5;
+        add_dir_name(&mut dired, dir_len);
+        // 8 = 2 ("  ") + 1 (\n) + 5 + 1 (: of dirname)
+        assert_eq!(dired.padding, 8);
+
+        let total_len = 8;
+        add_total(&mut dired, total_len);
+        assert_eq!(dired.padding, 18);
+    }
+
+    #[test]
     fn test_dired_update_positions() {
         let mut dired = DiredOutput {
             dired_positions: vec![BytePosition { start: 5, end: 10 }],
@@ -151,15 +273,41 @@ mod tests {
         };
 
         // Test with adjust = true
-        update_positions(15, 20, &mut dired);
+        update_positions(&mut dired, 15, 20);
         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(30, 35, &mut dired);
+        update_positions(&mut dired, 30, 35);
         let last_position = dired.dired_positions.last().unwrap();
         assert_eq!(last_position.start, 30);
         assert_eq!(last_position.end, 35);
     }
+
+    #[test]
+    fn test_calculate_and_update_positions() {
+        let mut dired = DiredOutput {
+            dired_positions: vec![
+                BytePosition { start: 0, end: 3 },
+                BytePosition { start: 4, end: 7 },
+                BytePosition { start: 8, end: 11 },
+            ],
+            subdired_positions: vec![],
+            padding: 5,
+        };
+        let output_display_len = 15;
+        let dfn_len = 5;
+        calculate_and_update_positions(&mut dired, output_display_len, dfn_len);
+        assert_eq!(
+            dired.dired_positions,
+            vec![
+                BytePosition { start: 0, end: 3 },
+                BytePosition { start: 4, end: 7 },
+                BytePosition { start: 8, end: 11 },
+                BytePosition { start: 32, end: 37 },
+            ]
+        );
+        assert_eq!(dired.padding, 0);
+    }
 }
diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs
index 50fbe0c7c..41d2f59b1 100644
--- a/src/uu/ls/src/ls.rs
+++ b/src/uu/ls/src/ls.rs
@@ -1856,6 +1856,10 @@ impl PathData {
     }
 }
 
+fn show_dir_name(dir: &Path, out: &mut BufWriter<Stdout>) {
+    write!(out, "{}:", dir.display()).unwrap();
+}
+
 #[allow(clippy::cognitive_complexity)]
 pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> {
     let mut files = Vec::<PathData>::new();
@@ -1922,10 +1926,17 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> {
                 }
                 writeln!(out, "{}:", path_data.p_buf.display())?;
                 if config.dired {
-                    dired::calculate_subdired(&mut dired, path_data.display_name.len());
+                    // 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);
                 }
             } else {
-                writeln!(out, "\n{}:", path_data.p_buf.display())?;
+                writeln!(out)?;
+                show_dir_name(&path_data.p_buf, &mut out);
+                writeln!(out)?;
             }
         }
         let mut listed_ancestors = HashSet::new();
@@ -2104,7 +2115,7 @@ fn enter_directory(
         let total = return_total(&entries, config, out)?;
         write!(out, "{}", total.as_str())?;
         if config.dired {
-            dired::add_total(total.len(), dired);
+            dired::add_total(dired, total.len());
         }
     }
 
@@ -2132,7 +2143,23 @@ fn enter_directory(
                     if listed_ancestors
                         .insert(FileInformation::from_path(&e.p_buf, e.must_dereference)?)
                     {
-                        writeln!(out, "\n{}:", e.p_buf.display())?;
+                        // when listing several directories in recursive mode, we show
+                        // "dirname:" at the beginning of the file list
+                        writeln!(out)?;
+                        if config.dired {
+                            // We already injected the first dir
+                            // Continue with the others
+                            // 2 = \n + \n
+                            dired.padding = 2;
+                            dired::indent(out)?;
+                            let dir_name_size = e.p_buf.to_string_lossy().len();
+                            dired::calculate_subdired(dired, dir_name_size);
+                            // inject dir name
+                            dired::add_dir_name(dired, dir_name_size);
+                        }
+
+                        show_dir_name(&e.p_buf, out);
+                        writeln!(out)?;
                         enter_directory(e, rd, config, out, listed_ancestors, dired)?;
                         listed_ancestors
                             .remove(&FileInformation::from_path(&e.p_buf, e.must_dereference)?);
@@ -2547,11 +2574,11 @@ fn display_item_long(
         let displayed_file = display_file_name(item, config, None, String::new(), out).contents;
         if config.dired {
             let (start, end) = dired::calculate_dired(
+                &dired.dired_positions,
                 output_display.len(),
                 displayed_file.len(),
-                &dired.dired_positions,
             );
-            dired::update_positions(start, end, dired);
+            dired::update_positions(dired, start, end);
         }
         write!(output_display, "{}{}", displayed_file, config.line_ending).unwrap();
     } else {
@@ -2639,9 +2666,9 @@ fn display_item_long(
 
         if config.dired {
             dired::calculate_and_update_positions(
+                dired,
                 output_display.len(),
                 displayed_file.trim().len(),
-                dired,
             );
         }
         write!(output_display, "{}{}", displayed_file, config.line_ending).unwrap();
diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs
index 8b0032065..15893b0e2 100644
--- a/tests/by-util/test_ls.rs
+++ b/tests/by-util/test_ls.rs
@@ -3581,6 +3581,57 @@ fn test_ls_dired_recursive() {
 }
 
 #[test]
+fn test_ls_dired_recursive_multiple() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.mkdir("d");
+    at.mkdir("d/d1");
+    at.mkdir("d/d2");
+    at.touch("d/d2/a");
+    at.touch("d/d2/c2");
+    at.touch("d/d1/f1");
+    at.touch("d/d1/file-long");
+
+    let mut cmd = scene.ucmd();
+    cmd.arg("--dired").arg("-l").arg("-R").arg("d");
+
+    let result = cmd.succeeds();
+
+    let output = result.stdout_str().to_string();
+    println!("Output:\n{}", output);
+
+    let dired_line = output
+        .lines()
+        .find(|&line| line.starts_with("//DIRED//"))
+        .unwrap();
+    let positions: Vec<usize> = dired_line
+        .split_whitespace()
+        .skip(1)
+        .map(|s| s.parse().unwrap())
+        .collect();
+    println!("Parsed byte positions: {:?}", positions);
+    assert_eq!(positions.len() % 2, 0); // Ensure there's an even number of positions
+
+    let filenames: Vec<String> = positions
+        .chunks(2)
+        .map(|chunk| {
+            let start_pos = chunk[0];
+            let end_pos = chunk[1];
+            let filename = String::from_utf8(output.as_bytes()[start_pos..=end_pos].to_vec())
+                .unwrap()
+                .trim()
+                .to_string();
+            println!("Extracted filename: {}", filename);
+            filename
+        })
+        .collect();
+
+    println!("Extracted filenames: {:?}", filenames);
+    assert_eq!(filenames, vec!["d1", "d2", "f1", "file-long", "a", "c2"]);
+}
+
+#[test]
 fn test_ls_dired_simple() {
     let scene = TestScenario::new(util_name!());
     let at = &scene.fixtures;
@@ -3679,6 +3730,57 @@ fn test_ls_dired_complex() {
     assert_eq!(filenames, vec!["a1", "a22", "a333", "a4444", "d"]);
 }
 
+#[test]
+fn test_ls_subdired_complex() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.mkdir("dir1");
+    at.mkdir("dir1/d");
+    at.mkdir("dir1/c2");
+    at.touch("dir1/a1");
+    at.touch("dir1/a22");
+    at.touch("dir1/a333");
+    at.touch("dir1/c2/a4444");
+
+    let mut cmd = scene.ucmd();
+    cmd.arg("--dired").arg("-l").arg("-R").arg("dir1");
+    let result = cmd.succeeds();
+
+    let output = result.stdout_str().to_string();
+    println!("Output:\n{}", output);
+
+    let dired_line = output
+        .lines()
+        .find(|&line| line.starts_with("//SUBDIRED//"))
+        .unwrap();
+    let positions: Vec<usize> = dired_line
+        .split_whitespace()
+        .skip(1)
+        .map(|s| s.parse().unwrap())
+        .collect();
+    println!("Parsed byte positions: {:?}", positions);
+    assert_eq!(positions.len() % 2, 0); // Ensure there's an even number of positions
+
+    let dirnames: Vec<String> = positions
+        .chunks(2)
+        .map(|chunk| {
+            let start_pos = chunk[0];
+            let end_pos = chunk[1];
+            let dirname =
+                String::from_utf8(output.as_bytes()[start_pos..end_pos].to_vec()).unwrap();
+            println!("Extracted dirname: {}", dirname);
+            dirname
+        })
+        .collect();
+
+    println!("Extracted dirnames: {:?}", dirnames);
+    #[cfg(unix)]
+    assert_eq!(dirnames, vec!["dir1", "dir1/c2", "dir1/d"]);
+    #[cfg(windows)]
+    assert_eq!(dirnames, vec!["dir1", "dir1\\c2", "dir1\\d"]);
+}
+
 #[ignore = "issue #5396"]
 #[test]
 fn test_ls_cf_output_should_be_delimited_by_tab() {