Merge pull request #8375 from drinkcat/basename-osstr

basename: Handle non-unicode parameters

Author: cakebaker

src/uu/basename/src/basename.rs

@@ -5,8 +5,11 @@
 
 // spell-checker:ignore (ToDO) fullname
 
+use clap::builder::ValueParser;
 use clap::{Arg, ArgAction, Command};
 use std::collections::HashMap;
+use std::ffi::OsString;
+use std::io::{Write, stdout};
 use std::path::PathBuf;
 use uucore::display::Quotable;
 use uucore::error::{UResult, UUsageError};
@@ -24,8 +27,6 @@ pub mod options {
 
 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
-    let args = args.collect_lossy();
-
     //
     // Argument parsing
     //
@@ -34,7 +35,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let line_ending = LineEnding::from_zero_flag(matches.get_flag(options::ZERO));
 
     let mut name_args = matches
-        .get_many::<String>(options::NAME)
+        .get_many::<OsString>(options::NAME)
         .unwrap_or_default()
         .collect::<Vec<_>>();
     if name_args.is_empty() {
@@ -43,18 +44,18 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
             get_message("basename-error-missing-operand"),
         ));
     }
-    let multiple_paths =
-        matches.get_one::<String>(options::SUFFIX).is_some() || matches.get_flag(options::MULTIPLE);
+    let multiple_paths = matches.get_one::<OsString>(options::SUFFIX).is_some()
+        || matches.get_flag(options::MULTIPLE);
     let suffix = if multiple_paths {
         matches
-            .get_one::<String>(options::SUFFIX)
+            .get_one::<OsString>(options::SUFFIX)
             .cloned()
             .unwrap_or_default()
     } else {
         // "simple format"
         match name_args.len() {
             0 => panic!("already checked"),
-            1 => String::default(),
+            1 => OsString::default(),
             2 => name_args.pop().unwrap().clone(),
             _ => {
                 return Err(UUsageError::new(
@@ -73,7 +74,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     //
 
     for path in name_args {
-        print!("{}{line_ending}", basename(path, &suffix));
+        stdout().write_all(&basename(path, &suffix)?)?;
+        print!("{line_ending}");
     }
 
     Ok(())
@@ -96,6 +98,7 @@ pub fn uu_app() -> Command {
         .arg(
             Arg::new(options::NAME)
                 .action(ArgAction::Append)
+                .value_parser(ValueParser::os_string())
                 .value_hint(clap::ValueHint::AnyPath)
                 .hide(true)
                 .trailing_var_arg(true),
@@ -105,6 +108,7 @@ pub fn uu_app() -> Command {
                 .short('s')
                 .long(options::SUFFIX)
                 .value_name("SUFFIX")
+                .value_parser(ValueParser::os_string())
                 .help(get_message("basename-help-suffix"))
                 .overrides_with(options::SUFFIX),
         )
@@ -118,21 +122,30 @@ pub fn uu_app() -> Command {
         )
 }
 
-fn basename(fullname: &str, suffix: &str) -> String {
+// We return a Vec<u8>. Returning a seemingly more proper `OsString` would
+// require back and forth conversions as we need a &[u8] for printing anyway.
+fn basename(fullname: &OsString, suffix: &OsString) -> UResult<Vec<u8>> {
+    let fullname_bytes = uucore::os_str_as_bytes(fullname)?;
+
     // Handle special case where path ends with /.
-    if fullname.ends_with("/.") {
-        return ".".to_string();
+    if fullname_bytes.ends_with(b"/.") {
+        return Ok(b".".into());
     }
 
     // Convert to path buffer and get last path component
     let pb = PathBuf::from(fullname);
 
-    pb.components().next_back().map_or_else(String::new, |c| {
-        let name = c.as_os_str().to_str().unwrap();
+    pb.components().next_back().map_or(Ok([].into()), |c| {
+        let name = c.as_os_str();
+        let name_bytes = uucore::os_str_as_bytes(name)?;
         if name == suffix {
-            name.to_string()
+            Ok(name_bytes.into())
         } else {
-            name.strip_suffix(suffix).unwrap_or(name).to_string()
+            let suffix_bytes = uucore::os_str_as_bytes(suffix)?;
+            Ok(name_bytes
+                .strip_suffix(suffix_bytes)
+                .unwrap_or(name_bytes)
+                .into())
         }
     })
 }

tests/by-util/test_basename.rs

@@ -4,8 +4,6 @@
 // file that was distributed with this source code.
 // spell-checker:ignore (words) reallylongexecutable nbaz
 
-#[cfg(any(unix, target_os = "redox"))]
-use std::ffi::OsStr;
 use uutests::new_ucmd;
 
 #[test]
@@ -138,20 +136,25 @@ fn test_too_many_args_output() {
         .usage_error("extra operand 'c'");
 }
 
-#[cfg(any(unix, target_os = "redox"))]
-fn test_invalid_utf8_args(os_str: &OsStr) {
-    let test_vec = vec![os_str.to_os_string()];
-    new_ucmd!().args(&test_vec).succeeds().stdout_is("fo�o\n");
-}
-
 #[cfg(any(unix, target_os = "redox"))]
 #[test]
-fn invalid_utf8_args_unix() {
-    use std::os::unix::ffi::OsStrExt;
+fn test_invalid_utf8_args() {
+    let param = uucore::os_str_from_bytes(b"/tmp/some-\xc0-file.k\xf3")
+        .expect("Only unix platforms can test non-unicode names");
+
+    new_ucmd!()
+        .arg(&param)
+        .succeeds()
+        .stdout_is_bytes(b"some-\xc0-file.k\xf3\n");
+
+    let suffix = uucore::os_str_from_bytes(b".k\xf3")
+        .expect("Only unix platforms can test non-unicode names");
 
-    let source = [0x66, 0x6f, 0x80, 0x6f];
-    let os_str = OsStr::from_bytes(&source[..]);
-    test_invalid_utf8_args(os_str);
+    new_ucmd!()
+        .arg(&param)
+        .arg(&suffix)
+        .succeeds()
+        .stdout_is_bytes(b"some-\xc0-file\n");
 }
 
 #[test]