From 58f500d81bbc4f11043b384121079f1ef651c6c3 Mon Sep 17 00:00:00 2001 From: naoNao89 <90588855+naoNao89@users.noreply.github.com> Date: Thu, 16 Oct 2025 12:04:50 +0700 Subject: [PATCH 1/4] test(dirname): add coverage for missing operand and multiple paths Add test_missing_operand to cover error path when dirname called with no arguments (lines 80-82). Add test_multiple_paths_comprehensive to cover loop iteration with mixed edge cases including trailing dot paths, empty strings, and various path types (line 84). --- tests/by-util/test_dirname.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/by-util/test_dirname.rs b/tests/by-util/test_dirname.rs index c7cdf3a4633..31b96da241c 100644 --- a/tests/by-util/test_dirname.rs +++ b/tests/by-util/test_dirname.rs @@ -9,6 +9,13 @@ fn test_invalid_arg() { new_ucmd!().arg("--definitely-invalid").fails_with_code(1); } +#[test] +fn test_missing_operand() { + // Test calling dirname with no arguments - should fail + // This covers the error path at line 80-82 in dirname.rs + new_ucmd!().fails_with_code(1); +} + #[test] fn test_path_with_trailing_slashes() { new_ucmd!() @@ -216,3 +223,21 @@ fn test_existing_behavior_preserved() { .succeeds() .stdout_is("/home/dos\n"); } + +#[test] +fn test_multiple_paths_comprehensive() { + // Comprehensive test for multiple paths in single invocation + // Tests the loop at line 84 with various edge cases mixed + new_ucmd!() + .args(&[ + "/home/dos/.", // trailing dot case + "/var/log", // normal path + ".", // current directory + "/tmp/.", // another trailing dot + "", // empty string + "/", // root + "relative/path", // relative path + ]) + .succeeds() + .stdout_is("/home/dos\n/var\n.\n/tmp\n.\n/\nrelative\n"); +} From 676fc0346a9f4901d4520b32f529e6839213ff21 Mon Sep 17 00:00:00 2001 From: naoNao89 <90588855+naoNao89@users.noreply.github.com> Date: Fri, 17 Oct 2025 08:41:19 +0700 Subject: [PATCH 2/4] refactor(dirname): implement pure string manipulation per POSIX Replace Path::parent() approach with byte-level string operations to avoid path normalization. This ensures dirname behaves as a pure string manipulation tool per POSIX/GNU specifications, correctly handling patterns like foo//., foo/./bar, and preserving /. components in middle of paths. Add comprehensive test coverage for edge cases including multiple slash variations, dot-slash patterns, and component preservation. Addresses issue #8910 with complete POSIX-compliant implementation. --- src/uu/dirname/src/dirname.rs | 136 +++++++++++++++++++++------------- tests/by-util/test_dirname.rs | 63 ++++++++++++++-- 2 files changed, 138 insertions(+), 61 deletions(-) diff --git a/src/uu/dirname/src/dirname.rs b/src/uu/dirname/src/dirname.rs index 3399b4a031c..b949dee0cd5 100644 --- a/src/uu/dirname/src/dirname.rs +++ b/src/uu/dirname/src/dirname.rs @@ -5,7 +5,6 @@ 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; @@ -18,51 +17,84 @@ mod options { pub const DIR: &str = "dir"; } -/// Handle the special case where a path ends with "/." +/// Perform dirname as pure string manipulation per POSIX/GNU behavior. +/// +/// dirname should NOT normalize paths. It does simple string manipulation: +/// 1. Strip trailing slashes (unless path is all slashes) +/// 2. If ends with `/.` (possibly `//.` or `///.`), strip the `/+.` pattern +/// 3. Otherwise, remove everything after the last `/` +/// 4. If no `/` found, return `.` +/// 5. Strip trailing slashes from result (unless result would be empty) +/// +/// Examples: +/// - `foo/.` → `foo` +/// - `foo/./bar` → `foo/.` +/// - `foo/bar` → `foo` +/// - `a/b/c` → `a/b` /// -/// 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). /// Per POSIX.1-2017 dirname specification and GNU coreutils manual: /// - POSIX: /// - GNU: /// -/// dirname should do simple string manipulation without path normalization. /// See issue #8910 and similar fix in basename (#8373, commit c5268a897). -/// -/// 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; +fn dirname_string_manipulation(path_bytes: &[u8]) -> Vec { + 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(()) - } - #[cfg(not(unix))] - { - // On non-Unix, fall back to lossy conversion - if let Ok(s) = std::str::from_utf8(stripped) { - print!("{s}"); - Some(()) - } else { - // Can't handle non-UTF-8 on non-Unix, fall through to normal logic - None + let mut bytes = path_bytes; + + // Step 1: Strip trailing slashes (but not if the entire path is slashes) + let all_slashes = bytes.iter().all(|&b| b == b'/'); + if all_slashes { + return b"/".to_vec(); + } + + while bytes.len() > 1 && bytes.ends_with(b"/") { + bytes = &bytes[..bytes.len() - 1]; + } + + // Step 2: Check if it ends with `/.` and strip the `/+.` pattern + if bytes.ends_with(b".") && bytes.len() >= 2 { + let dot_pos = bytes.len() - 1; + if bytes[dot_pos - 1] == b'/' { + // Find where the slashes before the dot start + let mut slash_start = dot_pos - 1; + while slash_start > 0 && bytes[slash_start - 1] == b'/' { + slash_start -= 1; + } + // Return the stripped result + if slash_start == 0 { + // Result would be empty + return if path_bytes.starts_with(b"/") { + b"/".to_vec() + } else { + b".".to_vec() + }; } + return bytes[..slash_start].to_vec(); } } + + // Step 3: Normal dirname - find last / and remove everything after it + if let Some(last_slash_pos) = bytes.iter().rposition(|&b| b == b'/') { + // Found a slash, remove everything after it + let mut result = &bytes[..last_slash_pos]; + + // Strip trailing slashes from result (but keep at least one if at the start) + while result.len() > 1 && result.ends_with(b"/") { + result = &result[..result.len() - 1]; + } + + if result.is_empty() { + return b"/".to_vec(); + } + + return result.to_vec(); + } + + // No slash found, return "." + b".".to_vec() } #[uucore::main] @@ -83,27 +115,25 @@ 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(&[]); + let result = dirname_string_manipulation(path_bytes); - 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(); - } - } - None => { - if p.is_absolute() || path.as_os_str() == "/" { - print!("/"); - } else { - print!("."); - } - } + #[cfg(unix)] + { + use std::os::unix::ffi::OsStrExt; + let result_os = std::ffi::OsStr::from_bytes(&result); + print_verbatim(result_os).unwrap(); + } + #[cfg(not(unix))] + { + // On non-Unix, fall back to lossy conversion + if let Ok(s) = std::str::from_utf8(&result) { + print!("{s}"); + } else { + // Fallback for non-UTF-8 paths on non-Unix systems + print!("."); } } + print!("{line_ending}"); } diff --git a/tests/by-util/test_dirname.rs b/tests/by-util/test_dirname.rs index 31b96da241c..95723dcf9df 100644 --- a/tests/by-util/test_dirname.rs +++ b/tests/by-util/test_dirname.rs @@ -163,7 +163,7 @@ fn test_trailing_dot_edge_cases() { 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!() @@ -230,14 +230,61 @@ fn test_multiple_paths_comprehensive() { // Tests the loop at line 84 with various edge cases mixed new_ucmd!() .args(&[ - "/home/dos/.", // trailing dot case - "/var/log", // normal path - ".", // current directory - "/tmp/.", // another trailing dot - "", // empty string - "/", // root - "relative/path", // relative path + "/home/dos/.", // trailing dot case + "/var/log", // normal path + ".", // current directory + "/tmp/.", // another trailing dot + "", // empty string + "/", // root + "relative/path", // relative path ]) .succeeds() .stdout_is("/home/dos\n/var\n.\n/tmp\n.\n/\nrelative\n"); } + +#[test] +fn test_all_dot_slash_variations() { + // Tests for all the cases mentioned in issue #8910 comment + // https://github.com/uutils/coreutils/issues/8910#issuecomment-3408735720 + + // foo//. -> foo + new_ucmd!().arg("foo//.").succeeds().stdout_is("foo\n"); + + // foo///. -> foo + new_ucmd!().arg("foo///.").succeeds().stdout_is("foo\n"); + + // foo/./ -> foo + new_ucmd!().arg("foo/./").succeeds().stdout_is("foo\n"); + + // foo/bar/./ -> foo/bar + new_ucmd!() + .arg("foo/bar/./") + .succeeds() + .stdout_is("foo/bar\n"); + + // foo/./bar -> foo/. + new_ucmd!().arg("foo/./bar").succeeds().stdout_is("foo/.\n"); +} + +#[test] +fn test_dot_slash_component_preservation() { + // Ensure that /. components in the middle are preserved + // These should NOT be normalized away + + new_ucmd!().arg("a/./b").succeeds().stdout_is("a/.\n"); + + new_ucmd!() + .arg("a/./b/./c") + .succeeds() + .stdout_is("a/./b/.\n"); + + new_ucmd!() + .arg("foo/./bar/baz") + .succeeds() + .stdout_is("foo/./bar\n"); + + new_ucmd!() + .arg("/path/./to/file") + .succeeds() + .stdout_is("/path/./to\n"); +} From 2c72ad31fd78b6c7133575924843f5f8e27d8f32 Mon Sep 17 00:00:00 2001 From: naoNao89 <90588855+naoNao89@users.noreply.github.com> Date: Mon, 20 Oct 2025 14:58:02 +0700 Subject: [PATCH 3/4] test(dirname): remove redundant inline comments per review --- .../cspell.dictionaries/jargon.wordlist.txt | 1 + src/uu/dirname/src/dirname.rs | 1 + tests/by-util/test_dirname.rs | 20 +++++++------------ 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/.vscode/cspell.dictionaries/jargon.wordlist.txt b/.vscode/cspell.dictionaries/jargon.wordlist.txt index ef8a2026fc1..1f737df2696 100644 --- a/.vscode/cspell.dictionaries/jargon.wordlist.txt +++ b/.vscode/cspell.dictionaries/jargon.wordlist.txt @@ -176,3 +176,4 @@ nofield # * clippy uninlined nonminimal +rposition diff --git a/src/uu/dirname/src/dirname.rs b/src/uu/dirname/src/dirname.rs index b949dee0cd5..5a0fe59b9d4 100644 --- a/src/uu/dirname/src/dirname.rs +++ b/src/uu/dirname/src/dirname.rs @@ -5,6 +5,7 @@ use clap::{Arg, ArgAction, Command}; use std::ffi::OsString; +#[cfg(unix)] use uucore::display::print_verbatim; use uucore::error::{UResult, UUsageError}; use uucore::format_usage; diff --git a/tests/by-util/test_dirname.rs b/tests/by-util/test_dirname.rs index 95723dcf9df..0af740c4a64 100644 --- a/tests/by-util/test_dirname.rs +++ b/tests/by-util/test_dirname.rs @@ -227,16 +227,15 @@ fn test_existing_behavior_preserved() { #[test] fn test_multiple_paths_comprehensive() { // Comprehensive test for multiple paths in single invocation - // Tests the loop at line 84 with various edge cases mixed new_ucmd!() .args(&[ - "/home/dos/.", // trailing dot case - "/var/log", // normal path - ".", // current directory - "/tmp/.", // another trailing dot - "", // empty string - "/", // root - "relative/path", // relative path + "/home/dos/.", + "/var/log", + ".", + "/tmp/.", + "", + "/", + "relative/path", ]) .succeeds() .stdout_is("/home/dos\n/var\n.\n/tmp\n.\n/\nrelative\n"); @@ -247,22 +246,17 @@ fn test_all_dot_slash_variations() { // Tests for all the cases mentioned in issue #8910 comment // https://github.com/uutils/coreutils/issues/8910#issuecomment-3408735720 - // foo//. -> foo new_ucmd!().arg("foo//.").succeeds().stdout_is("foo\n"); - // foo///. -> foo new_ucmd!().arg("foo///.").succeeds().stdout_is("foo\n"); - // foo/./ -> foo new_ucmd!().arg("foo/./").succeeds().stdout_is("foo\n"); - // foo/bar/./ -> foo/bar new_ucmd!() .arg("foo/bar/./") .succeeds() .stdout_is("foo/bar\n"); - // foo/./bar -> foo/. new_ucmd!().arg("foo/./bar").succeeds().stdout_is("foo/.\n"); } From d8287a6df96f45950ddb1fff88acbbd384531b05 Mon Sep 17 00:00:00 2001 From: naoNao89 <90588855+naoNao89@users.noreply.github.com> Date: Fri, 2 Jan 2026 21:59:15 +0000 Subject: [PATCH 4/4] dirname: implement PR #8936 feedback - Remove unnecessary comments from test_dirname.rs - Change dirname_string_manipulation() return type to Cow<'_, [u8]> - Add explicit lifetime annotation to resolve compiler warning - Optimize memory allocation with copy-on-write pattern --- src/uu/dirname/src/dirname.rs | 19 ++++++++++--------- tests/by-util/test_dirname.rs | 6 ------ 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/uu/dirname/src/dirname.rs b/src/uu/dirname/src/dirname.rs index 5a0fe59b9d4..8659465cdb0 100644 --- a/src/uu/dirname/src/dirname.rs +++ b/src/uu/dirname/src/dirname.rs @@ -4,6 +4,7 @@ // file that was distributed with this source code. use clap::{Arg, ArgAction, Command}; +use std::borrow::Cow; use std::ffi::OsString; #[cfg(unix)] use uucore::display::print_verbatim; @@ -38,9 +39,9 @@ mod options { /// - GNU: /// /// See issue #8910 and similar fix in basename (#8373, commit c5268a897). -fn dirname_string_manipulation(path_bytes: &[u8]) -> Vec { +fn dirname_string_manipulation(path_bytes: &[u8]) -> Cow<'_, [u8]> { if path_bytes.is_empty() { - return b".".to_vec(); + return Cow::Borrowed(b"."); } let mut bytes = path_bytes; @@ -48,7 +49,7 @@ fn dirname_string_manipulation(path_bytes: &[u8]) -> Vec { // Step 1: Strip trailing slashes (but not if the entire path is slashes) let all_slashes = bytes.iter().all(|&b| b == b'/'); if all_slashes { - return b"/".to_vec(); + return Cow::Borrowed(b"/"); } while bytes.len() > 1 && bytes.ends_with(b"/") { @@ -68,12 +69,12 @@ fn dirname_string_manipulation(path_bytes: &[u8]) -> Vec { if slash_start == 0 { // Result would be empty return if path_bytes.starts_with(b"/") { - b"/".to_vec() + Cow::Borrowed(b"/") } else { - b".".to_vec() + Cow::Borrowed(b".") }; } - return bytes[..slash_start].to_vec(); + return Cow::Owned(bytes[..slash_start].to_vec()); } } @@ -88,14 +89,14 @@ fn dirname_string_manipulation(path_bytes: &[u8]) -> Vec { } if result.is_empty() { - return b"/".to_vec(); + return Cow::Borrowed(b"/"); } - return result.to_vec(); + return Cow::Owned(result.to_vec()); } // No slash found, return "." - b".".to_vec() + Cow::Borrowed(b".") } #[uucore::main] diff --git a/tests/by-util/test_dirname.rs b/tests/by-util/test_dirname.rs index 0af740c4a64..bd6994107ec 100644 --- a/tests/by-util/test_dirname.rs +++ b/tests/by-util/test_dirname.rs @@ -11,8 +11,6 @@ fn test_invalid_arg() { #[test] fn test_missing_operand() { - // Test calling dirname with no arguments - should fail - // This covers the error path at line 80-82 in dirname.rs new_ucmd!().fails_with_code(1); } @@ -78,15 +76,11 @@ fn test_dirname_non_utf8_paths() { use std::ffi::OsStr; use std::os::unix::ffi::OsStrExt; - // Create a test file with non-UTF-8 bytes in the name let non_utf8_bytes = b"test_\xFF\xFE/file.txt"; let non_utf8_name = OsStr::from_bytes(non_utf8_bytes); - // Test that dirname handles non-UTF-8 paths without crashing let result = new_ucmd!().arg(non_utf8_name).succeeds(); - // Just verify it didn't crash and produced some output - // The exact output format may vary due to lossy conversion let output = result.stdout_str_lossy(); assert!(!output.is_empty()); assert!(output.contains("test_"));