dirname: better support of the posic spec

sylvestre · sylvestre · commit 439cb177faf0 · 2025-10-16T14:06:40.000+02:00
Closes: uutils#8924
diff --git a/src/uu/dirname/src/dirname.rs b/src/uu/dirname/src/dirname.rs
@@ -5,64 +5,103 @@
 
 use clap::{Arg, ArgAction, Command};
 use std::ffi::OsString;
-use std::path::Path;
 use uucore::display::print_verbatim;
 use uucore::error::{UResult, UUsageError};
 use uucore::format_usage;
 use uucore::line_ending::LineEnding;
 
 use uucore::translate;
 
+#[cfg(not(unix))]
+use std::path::Path;
+
 mod options {
     pub const ZERO: &str = "zero";
     pub const DIR: &str = "dir";
 }
 
-/// Handle the special case where a path ends with "/."
+/// Compute dirname following POSIX/GNU behavior
 ///
-/// This matches GNU/POSIX behavior where `dirname("/home/dos/.")` returns "/home/dos"
-/// rather than "/home" (which would be the result of `Path::parent()` due to normalization).
+/// This implements the POSIX dirname algorithm without path normalization.
 /// Per POSIX.1-2017 dirname specification and GNU coreutils manual:
 /// - POSIX: <https://pubs.opengroup.org/onlinepubs/9699919799/utilities/dirname.html>
 /// - GNU: <https://www.gnu.org/software/coreutils/manual/html_node/dirname-invocation.html>
 ///
-/// dirname should do simple string manipulation without path normalization.
-/// See issue #8910 and similar fix in basename (#8373, commit c5268a897).
+/// The algorithm:
+/// 1. Remove trailing '/' characters
+/// 2. If the path ends with "/.", remove it (handles foo/., foo//., foo///., etc.)
+/// 3. Remove any remaining trailing '/' characters
+/// 4. Apply standard dirname logic (find last '/', return everything before it)
 ///
-/// Returns `Some(())` if the special case was handled (output already printed),
-/// or `None` if normal `Path::parent()` logic should be used.
-fn handle_trailing_dot(path_bytes: &[u8]) -> Option<()> {
-    if !path_bytes.ends_with(b"/.") {
-        return None;
+/// See issues #8910 and #8924, and similar fix in basename (#8373, commit c5268a897).
+fn compute_dirname(path_bytes: &[u8]) -> Vec<u8> {
+    // Handle empty path
+    if path_bytes.is_empty() {
+        return b".".to_vec();
     }
 
-    // Strip the "/." suffix and print the result
-    if path_bytes.len() == 2 {
-        // Special case: "/." -> "/"
-        print!("/");
-        Some(())
-    } else {
-        // General case: "/home/dos/." -> "/home/dos"
-        let stripped = &path_bytes[..path_bytes.len() - 2];
-        #[cfg(unix)]
-        {
-            use std::os::unix::ffi::OsStrExt;
-            let result = std::ffi::OsStr::from_bytes(stripped);
-            print_verbatim(result).unwrap();
-            Some(())
+    // Special case: "//" stays as "/" per POSIX
+    if path_bytes == b"//" {
+        return b"/".to_vec();
+    }
+
+    let mut path = path_bytes.to_vec();
+
+    // If path consists entirely of slashes, return single slash
+    if path.iter().all(|&b| b == b'/') {
+        return b"/".to_vec();
+    }
+
+    // Step 1: Remove trailing slashes (but keep at least one character)
+    while path.len() > 1 && path.last() == Some(&b'/') {
+        path.pop();
+    }
+
+    // Step 2: Check if path ends with "/." and handle specially
+    // This handles foo/., foo//., foo///., and foo/./ (after step 1) etc.
+    if path.len() >= 2 && path[path.len() - 1] == b'.' && path[path.len() - 2] == b'/' {
+        // Remember if the original path was absolute (for handling "/." -> "/")
+        let was_absolute = path[0] == b'/';
+
+        // Remove the "/." suffix
+        path.truncate(path.len() - 2);
+
+        // Remove any additional trailing slashes that might remain (e.g., foo//. -> foo/)
+        while path.len() > 1 && path.last() == Some(&b'/') {
+            path.pop();
         }
-        #[cfg(not(unix))]
-        {
-            // On non-Unix, fall back to lossy conversion
-            if let Ok(s) = std::str::from_utf8(stripped) {
-                print!("{s}");
-                Some(())
+
+        // Handle edge cases: if we're left with nothing or just slashes
+        if path.is_empty() {
+            // If it was an absolute path like "/.", return "/"
+            // Otherwise, return "."
+            return if was_absolute {
+                b"/".to_vec()
             } else {
-                // Can't handle non-UTF-8 on non-Unix, fall through to normal logic
-                None
-            }
+                b".".to_vec()
+            };
+        }
+        if path.iter().all(|&b| b == b'/') {
+            return b"/".to_vec();
+        }
+
+        // What remains IS the dirname for paths ending with "/.".
+        // Example: "foo/bar/." -> "foo/bar", "foo//." -> "foo"
+        return path;
+    }
+
+    // Step 3: Standard dirname logic - find last '/' and return everything before it
+    if let Some(pos) = path.iter().rposition(|&b| b == b'/') {
+        if pos == 0 {
+            // The slash is at the beginning, dirname is "/"
+            return b"/".to_vec();
         }
+        path.truncate(pos);
+        return path;
     }
+
+    // No slash found, dirname is "."
+    b".".to_vec()
 }
 
 #[uucore::main]
@@ -84,26 +123,43 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     for path in &dirnames {
         let path_bytes = uucore::os_str_as_bytes(path.as_os_str()).unwrap_or(&[]);
 
-        if handle_trailing_dot(path_bytes).is_none() {
-            // Normal path handling using Path::parent()
-            let p = Path::new(path);
-            match p.parent() {
-                Some(d) => {
-                    if d.components().next().is_none() {
-                        print!(".");
-                    } else {
-                        print_verbatim(d).unwrap();
+        // Compute dirname using POSIX-compliant algorithm
+        let dirname_bytes = compute_dirname(path_bytes);
+
+        // Print the result
+        #[cfg(unix)]
+        {
+            use std::os::unix::ffi::OsStrExt;
+            let result = std::ffi::OsStr::from_bytes(&dirname_bytes);
+            print_verbatim(result).unwrap();
+        }
+        #[cfg(not(unix))]
+        {
+            // On non-Unix, fall back to lossy conversion
+            if let Ok(s) = std::str::from_utf8(&dirname_bytes) {
+                print!("{s}");
+            } else {
+                // Fallback for non-UTF-8 on non-Unix: use Path::parent() as before
+                let p = Path::new(path);
+                match p.parent() {
+                    Some(d) => {
+                        if d.components().next().is_none() {
+                            print!(".");
+                        } else {
+                            print_verbatim(d).unwrap();
+                        }
                     }
-                }
-                None => {
-                    if p.is_absolute() || path.as_os_str() == "/" {
-                        print!("/");
-                    } else {
-                        print!(".");
+                    None => {
+                        if p.is_absolute() || path.as_os_str() == "/" {
+                            print!("/");
+                        } else {
+                            print!(".");
+                        }
                     }
                 }
             }
         }
+
         print!("{line_ending}");
     }
 
diff --git a/tests/by-util/test_dirname.rs b/tests/by-util/test_dirname.rs
@@ -152,11 +152,11 @@ fn test_trailing_dot_multiple_paths() {
 
 #[test]
 fn test_trailing_dot_edge_cases() {
-    // Double slash before dot (should still work)
+    // Double slash before dot - should strip both the slashes and dot (issue #8924)
     new_ucmd!()
         .arg("/home/dos//.")
         .succeeds()
-        .stdout_is("/home/dos/\n");
+        .stdout_is("/home/dos\n");
 
     // Path with . in middle (should use normal logic)
     new_ucmd!()
@@ -216,3 +216,88 @@ fn test_existing_behavior_preserved() {
         .succeeds()
         .stdout_is("/home/dos\n");
 }
+
+// Comprehensive test for issue #8924
+// Tests GNU coreutils compatibility for complex path patterns with multiple slashes,
+// trailing dots, and various combinations that were not handled correctly
+#[test]
+fn test_issue_8924() {
+    // The five main cases from the issue
+    // 1. dirname foo//. should return "foo" not "foo/"
+    new_ucmd!().arg("foo//.").succeeds().stdout_is("foo\n");
+    // 2. dirname foo///. should return "foo" not "foo//"
+    new_ucmd!().arg("foo///.").succeeds().stdout_is("foo\n");
+    // 3. dirname foo/./ should return "foo" not "."
+    new_ucmd!().arg("foo/./").succeeds().stdout_is("foo\n");
+    // 4. dirname foo/bar/./ should return "foo/bar" not "foo"
+    new_ucmd!()
+        .arg("foo/bar/./")
+        .succeeds()
+        .stdout_is("foo/bar\n");
+    // 5. dirname foo/./bar should return "foo/." not "foo"
+    new_ucmd!().arg("foo/./bar").succeeds().stdout_is("foo/.\n");
+
+    // Test the same patterns with absolute paths
+    new_ucmd!()
+        .arg("/home/foo//.")
+        .succeeds()
+        .stdout_is("/home/foo\n");
+    new_ucmd!()
+        .arg("/home/foo///.")
+        .succeeds()
+        .stdout_is("/home/foo\n");
+    new_ucmd!()
+        .arg("/home/foo/./")
+        .succeeds()
+        .stdout_is("/home/foo\n");
+    new_ucmd!()
+        .arg("/home/foo/bar/./")
+        .succeeds()
+        .stdout_is("/home/foo/bar\n");
+    new_ucmd!()
+        .arg("/home/foo/./bar")
+        .succeeds()
+        .stdout_is("/home/foo/.\n");
+
+    // Test various combinations of multiple slashes
+    new_ucmd!().arg("foo////.").succeeds().stdout_is("foo\n");
+    new_ucmd!().arg("foo/////.").succeeds().stdout_is("foo\n");
+    new_ucmd!().arg("foo/./////").succeeds().stdout_is("foo\n");
+
+    // Edge cases
+    new_ucmd!().arg("//.").succeeds().stdout_is("/\n");
+    new_ucmd!().arg("/./").succeeds().stdout_is("/\n");
+    new_ucmd!().arg(".//.").succeeds().stdout_is(".\n");
+    new_ucmd!().arg("foo/..").succeeds().stdout_is("foo\n");
+
+    // Verify -z flag works correctly with all patterns
+    new_ucmd!()
+        .arg("-z")
+        .arg("foo//.")
+        .succeeds()
+        .stdout_is("foo\u{0}");
+    new_ucmd!()
+        .arg("-z")
+        .arg("foo/./")
+        .succeeds()
+        .stdout_is("foo\u{0}");
+    new_ucmd!()
+        .arg("-z")
+        .arg("foo/./bar")
+        .succeeds()
+        .stdout_is("foo/.\u{0}");
+
+    // More complex real-world-like paths
+    new_ucmd!()
+        .arg("/usr/local/bin//.")
+        .succeeds()
+        .stdout_is("/usr/local/bin\n");
+    new_ucmd!()
+        .arg("/var/lib/./foo")
+        .succeeds()
+        .stdout_is("/var/lib/.\n");
+    new_ucmd!()
+        .arg("src/main/./test.rs")
+        .succeeds()
+        .stdout_is("src/main/.\n");
+}
