Merge pull request #2621 from blyxxyz/filename-quoting

Implement proper quoting/escaping for filenames

.vscode/cspell.dictionaries/jargon.wordlist.txt

@@ -1,3 +1,4 @@
+AFAICT
 arity
 autogenerate
 autogenerated

.vscode/cspell.dictionaries/shell.wordlist.txt

@@ -8,6 +8,7 @@ csh
 globstar
 inotify
 localtime
+mksh
 mountinfo
 mountpoint
 mtab

src/uu/ls/src/ls.rs

@@ -21,6 +21,7 @@ use lscolors::LsColors;
 use number_prefix::NumberPrefix;
 use once_cell::unsync::OnceCell;
 use quoting_style::{escape_name, QuotingStyle};
+use std::ffi::OsString;
 #[cfg(windows)]
 use std::os::windows::fs::MetadataExt;
 use std::{
@@ -248,7 +249,7 @@ struct LongFormat {
 
 impl Config {
     #[allow(clippy::cognitive_complexity)]
-    fn from(options: clap::ArgMatches) -> UResult<Config> {
+    fn from(options: &clap::ArgMatches) -> UResult<Config> {
         let (mut format, opt) = if let Some(format_) = options.value_of(options::FORMAT) {
             (
                 match format_ {
@@ -428,11 +429,10 @@ impl Config {
         #[allow(clippy::needless_bool)]
         let show_control = if options.is_present(options::HIDE_CONTROL_CHARS) {
             false
-        } else if options.is_present(options::SHOW_CONTROL_CHARS) || atty::is(atty::Stream::Stdout)
-        {
+        } else if options.is_present(options::SHOW_CONTROL_CHARS) {
             true
         } else {
-            false
+            !atty::is(atty::Stream::Stdout)
         };
 
         let quoting_style = if let Some(style) = options.value_of(options::QUOTING_STYLE) {
@@ -599,22 +599,19 @@ impl Config {
 
 #[uucore_procs::gen_uumain]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
-    let args = args
-        .collect_str(InvalidEncodingHandling::Ignore)
-        .accept_any();
-
     let usage = usage();
 
     let app = uu_app().usage(&usage[..]);
 
     let matches = app.get_matches_from(args);
 
+    let config = Config::from(&matches)?;
     let locs = matches
-        .values_of(options::PATHS)
-        .map(|v| v.map(ToString::to_string).collect())
-        .unwrap_or_else(|| vec![String::from(".")]);
+        .values_of_os(options::PATHS)
+        .map(|v| v.map(Path::new).collect())
+        .unwrap_or_else(|| vec![Path::new(".")]);
 
-    list(locs, Config::from(matches)?)
+    list(locs, config)
 }
 
 pub fn uu_app() -> App<'static, 'static> {
@@ -1177,7 +1174,7 @@ struct PathData {
     md: OnceCell<Option<Metadata>>,
     ft: OnceCell<Option<FileType>>,
     // Name of the file - will be empty for . or ..
-    display_name: String,
+    display_name: OsString,
     // PathBuf that all above data corresponds to
     p_buf: PathBuf,
     must_dereference: bool,
@@ -1187,7 +1184,7 @@ impl PathData {
     fn new(
         p_buf: PathBuf,
         file_type: Option<std::io::Result<FileType>>,
-        file_name: Option<String>,
+        file_name: Option<OsString>,
         config: &Config,
         command_line: bool,
     ) -> Self {
@@ -1195,16 +1192,13 @@ impl PathData {
         // For '..', the filename is None
         let display_name = if let Some(name) = file_name {
             name
+        } else if command_line {
+            p_buf.clone().into()
         } else {
-            let display_os_str = if command_line {
-                p_buf.as_os_str()
-            } else {
-                p_buf
-                    .file_name()
-                    .unwrap_or_else(|| p_buf.iter().next_back().unwrap())
-            };
-
-            display_os_str.to_string_lossy().into_owned()
+            p_buf
+                .file_name()
+                .unwrap_or_else(|| p_buf.iter().next_back().unwrap())
+                .to_owned()
         };
         let must_dereference = match &config.dereference {
             Dereference::All => true,
@@ -1249,14 +1243,14 @@ impl PathData {
     }
 }
 
-fn list(locs: Vec<String>, config: Config) -> UResult<()> {
+fn list(locs: Vec<&Path>, config: Config) -> UResult<()> {
     let mut files = Vec::<PathData>::new();
     let mut dirs = Vec::<PathData>::new();
 
     let mut out = BufWriter::new(stdout());
 
     for loc in &locs {
-        let p = PathBuf::from(&loc);
+        let p = PathBuf::from(loc);
         let path_data = PathData::new(p, None, None, &config, true);
 
         if path_data.md().is_none() {
@@ -1286,6 +1280,7 @@ fn list(locs: Vec<String>, config: Config) -> UResult<()> {
     sort_entries(&mut dirs, &config);
     for dir in dirs {
         if locs.len() > 1 || config.recursive {
+            // FIXME: This should use the quoting style and propagate errors
             let _ = writeln!(out, "\n{}:", dir.p_buf.display());
         }
         enter_directory(&dir, &config, &mut out);
@@ -1671,7 +1666,6 @@ fn get_inode(metadata: &Metadata) -> String {
 use std::sync::Mutex;
 #[cfg(unix)]
 use uucore::entries;
-use uucore::InvalidEncodingHandling;
 
 #[cfg(unix)]
 fn cached_uid2usr(uid: u32) -> String {

src/uu/ls/src/quoting_style.rs

@@ -1,4 +1,5 @@
 use std::char::from_digit;
+use std::ffi::OsStr;
 
 // These are characters with special meaning in the shell (e.g. bash).
 // The first const contains characters that only have a special meaning when they appear at the beginning of a name.
@@ -255,19 +256,21 @@ fn shell_with_escape(name: &str, quotes: Quotes) -> (String, bool) {
     (escaped_str, must_quote)
 }
 
-pub(super) fn escape_name(name: &str, style: &QuotingStyle) -> String {
+pub(super) fn escape_name(name: &OsStr, style: &QuotingStyle) -> String {
     match style {
         QuotingStyle::Literal { show_control } => {
             if !show_control {
-                name.chars()
+                name.to_string_lossy()
+                    .chars()
                     .flat_map(|c| EscapedChar::new_literal(c).hide_control())
                     .collect()
             } else {
-                name.into()
+                name.to_string_lossy().into_owned()
             }
         }
         QuotingStyle::C { quotes } => {
             let escaped_str: String = name
+                .to_string_lossy()
                 .chars()
                 .flat_map(|c| EscapedChar::new_c(c, *quotes))
                 .collect();
@@ -283,6 +286,7 @@ pub(super) fn escape_name(name: &str, style: &QuotingStyle) -> String {
             always_quote,
             show_control,
         } => {
+            let name = name.to_string_lossy();
             let (quotes, must_quote) = if name.contains('"') {
                 (Quotes::Single, true)
             } else if name.contains('\'') {
@@ -294,9 +298,9 @@ pub(super) fn escape_name(name: &str, style: &QuotingStyle) -> String {
             };
 
             let (escaped_str, contains_quote_chars) = if *escape {
-                shell_with_escape(name, quotes)
+                shell_with_escape(&name, quotes)
             } else {
-                shell_without_escape(name, quotes, *show_control)
+                shell_without_escape(&name, quotes, *show_control)
             };
 
             match (must_quote | contains_quote_chars, quotes) {
@@ -362,7 +366,7 @@ mod tests {
     fn check_names(name: &str, map: Vec<(&str, &str)>) {
         assert_eq!(
             map.iter()
-                .map(|(_, style)| escape_name(name, &get_style(style)))
+                .map(|(_, style)| escape_name(name.as_ref(), &get_style(style)))
                 .collect::<Vec<String>>(),
             map.iter()
                 .map(|(correct, _)| correct.to_string())

src/uu/pwd/src/pwd.rs

@@ -10,9 +10,10 @@ extern crate uucore;
 
 use clap::{crate_version, App, Arg};
 use std::env;
-use std::io::{self, Write};
-use std::path::{Path, PathBuf};
+use std::io;
+use std::path::PathBuf;
 
+use uucore::display::println_verbatim;
 use uucore::error::{FromIo, UResult};
 
 static ABOUT: &str = "Display the full filename of the current working directory.";
@@ -57,6 +58,7 @@ fn logical_path() -> io::Result<PathBuf> {
     // POSIX: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/pwd.html
     #[cfg(not(windows))]
     {
+        use std::path::Path;
         fn looks_reasonable(path: &Path) -> bool {
             // First, check if it's an absolute path.
             if !path.has_root() {
@@ -148,30 +150,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         .map(Into::into)
         .unwrap_or(cwd);
 
-    print_path(&cwd).map_err_context(|| "failed to print current directory".to_owned())?;
-
-    Ok(())
-}
-
-fn print_path(path: &Path) -> io::Result<()> {
-    let stdout = io::stdout();
-    let mut stdout = stdout.lock();
-
-    // On Unix we print non-lossily.
-    #[cfg(unix)]
-    {
-        use std::os::unix::ffi::OsStrExt;
-        stdout.write_all(path.as_os_str().as_bytes())?;
-        stdout.write_all(b"\n")?;
-    }
-
-    // On other platforms we potentially mangle it.
-    // There might be some clever way to do it correctly on Windows, but
-    // invalid unicode in filenames is rare there.
-    #[cfg(not(unix))]
-    {
-        writeln!(stdout, "{}", path.display())?;
-    }
+    println_verbatim(&cwd).map_err_context(|| "failed to print current directory".to_owned())?;
 
     Ok(())
 }

src/uu/rmdir/src/rmdir.rs

@@ -14,6 +14,7 @@ use clap::{crate_version, App, Arg};
 use std::fs::{read_dir, remove_dir};
 use std::io;
 use std::path::Path;
+use uucore::display::Quotable;
 use uucore::error::{set_exit_code, strip_errno, UResult};
 use uucore::util_name;
 
@@ -77,27 +78,23 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
                     Ok(path.metadata()?.file_type().is_dir())
                 }
 
-                let path = path.as_os_str().as_bytes();
-                if error.raw_os_error() == Some(libc::ENOTDIR) && path.ends_with(b"/") {
+                let bytes = path.as_os_str().as_bytes();
+                if error.raw_os_error() == Some(libc::ENOTDIR) && bytes.ends_with(b"/") {
                     // Strip the trailing slash or .symlink_metadata() will follow the symlink
-                    let path: &Path = OsStr::from_bytes(&path[..path.len() - 1]).as_ref();
-                    if is_symlink(path).unwrap_or(false)
-                        && points_to_directory(path).unwrap_or(true)
+                    let no_slash: &Path = OsStr::from_bytes(&bytes[..bytes.len() - 1]).as_ref();
+                    if is_symlink(no_slash).unwrap_or(false)
+                        && points_to_directory(no_slash).unwrap_or(true)
                     {
                         show_error!(
-                            "failed to remove '{}/': Symbolic link not followed",
-                            path.display()
+                            "failed to remove {}: Symbolic link not followed",
+                            path.quote()
                         );
                         continue;
                     }
                 }
             }
 
-            show_error!(
-                "failed to remove '{}': {}",
-                path.display(),
-                strip_errno(&error)
-            );
+            show_error!("failed to remove {}: {}", path.quote(), strip_errno(&error));
         }
     }
 
@@ -125,7 +122,7 @@ fn remove(mut path: &Path, opts: Opts) -> Result<(), Error<'_>> {
 
 fn remove_single(path: &Path, opts: Opts) -> Result<(), Error<'_>> {
     if opts.verbose {
-        println!("{}: removing directory, '{}'", util_name(), path.display());
+        println!("{}: removing directory, {}", util_name(), path.quote());
     }
     remove_dir(path).map_err(|error| Error { error, path })
 }

src/uu/wc/src/wc.rs

@@ -24,6 +24,8 @@ use std::fs::{self, File};
 use std::io::{self, Write};
 use std::path::{Path, PathBuf};
 
+use uucore::display::{Quotable, Quoted};
+
 /// The minimum character width for formatting counts when reading from stdin.
 const MINIMUM_WIDTH: usize = 7;
 
@@ -122,10 +124,10 @@ impl Input {
         }
     }
 
-    fn path_display(&self) -> std::path::Display<'_> {
+    fn path_display(&self) -> Quoted<'_> {
         match self {
-            Input::Path(path) => path.display(),
-            Input::Stdin(_) => Path::display("'standard input'".as_ref()),
+            Input::Path(path) => path.maybe_quote(),
+            Input::Stdin(_) => "standard input".maybe_quote(),
         }
     }
 }
@@ -448,7 +450,10 @@ fn wc(inputs: Vec<Input>, settings: &Settings) -> Result<(), u32> {
         if let Err(err) = print_stats(settings, &result, max_width) {
             show_warning!(
                 "failed to print result for {}: {}",
-                result.title.unwrap_or_else(|| "<stdin>".as_ref()).display(),
+                result
+                    .title
+                    .unwrap_or_else(|| "<stdin>".as_ref())
+                    .maybe_quote(),
                 err
             );
             failure = true;
@@ -526,7 +531,7 @@ fn print_stats(
     }
 
     if let Some(title) = result.title {
-        writeln!(stdout_lock, " {}", title.display())?;
+        writeln!(stdout_lock, " {}", title.maybe_quote())?;
     } else {
         writeln!(stdout_lock)?;
     }

src/uucore/src/lib/lib.rs

@@ -19,6 +19,7 @@ mod parser; // string parsing modules
 // * cross-platform modules
 pub use crate::mods::backup_control;
 pub use crate::mods::coreopts;
+pub use crate::mods::display;
 pub use crate::mods::error;
 pub use crate::mods::os;
 pub use crate::mods::panic;

src/uucore/src/lib/mods.rs

@@ -2,6 +2,7 @@
 
 pub mod backup_control;
 pub mod coreopts;
+pub mod display;
 pub mod error;
 pub mod os;
 pub mod panic;

tests/by-util/test_ls.rs

@@ -354,6 +354,7 @@ fn test_ls_long_format() {
     at.mkdir(&at.plus_as_string("test-long-dir/test-long-dir"));
 
     for arg in &["-l", "--long", "--format=long", "--format=verbose"] {
+        #[allow(unused_variables)]
         let result = scene.ucmd().arg(arg).arg("test-long-dir").succeeds();
         // Assuming sane username do not have spaces within them.
         // A line of the output should be:
@@ -373,6 +374,7 @@ fn test_ls_long_format() {
         ).unwrap());
     }
 
+    #[allow(unused_variables)]
     let result = scene.ucmd().arg("-lan").arg("test-long-dir").succeeds();
     // This checks for the line with the .. entry. The uname and group should be digits.
     #[cfg(not(windows))]
@@ -1416,6 +1418,7 @@ fn test_ls_quoting_style() {
         // Default is shell-escape
         scene
             .ucmd()
+            .arg("--hide-control-chars")
             .arg("one\ntwo")
             .succeeds()
             .stdout_only("'one'$'\\n''two'\n");
@@ -1437,23 +1440,8 @@ fn test_ls_quoting_style() {
         ] {
             scene
                 .ucmd()
-                .arg(arg)
-                .arg("one\ntwo")
-                .succeeds()
-                .stdout_only(format!("{}\n", correct));
-        }
-
-        for (arg, correct) in &[
-            ("--quoting-style=literal", "one?two"),
-            ("-N", "one?two"),
-            ("--literal", "one?two"),
-            ("--quoting-style=shell", "one?two"),
-            ("--quoting-style=shell-always", "'one?two'"),
-        ] {
-            scene
-                .ucmd()
-                .arg(arg)
                 .arg("--hide-control-chars")
+                .arg(arg)
                 .arg("one\ntwo")
                 .succeeds()
                 .stdout_only(format!("{}\n", correct));
@@ -1463,7 +1451,7 @@ fn test_ls_quoting_style() {
             ("--quoting-style=literal", "one\ntwo"),
             ("-N", "one\ntwo"),
             ("--literal", "one\ntwo"),
-            ("--quoting-style=shell", "one\ntwo"),
+            ("--quoting-style=shell", "one\ntwo"), // FIXME: GNU ls quotes this case
             ("--quoting-style=shell-always", "'one\ntwo'"),
         ] {
             scene
@@ -1490,6 +1478,7 @@ fn test_ls_quoting_style() {
         ] {
             scene
                 .ucmd()
+                .arg("--hide-control-chars")
                 .arg(arg)
                 .arg("one\\two")
                 .succeeds()
@@ -1505,6 +1494,7 @@ fn test_ls_quoting_style() {
         ] {
             scene
                 .ucmd()
+                .arg("--hide-control-chars")
                 .arg(arg)
                 .arg("one\n&two")
                 .succeeds()
@@ -1535,6 +1525,7 @@ fn test_ls_quoting_style() {
     ] {
         scene
             .ucmd()
+            .arg("--hide-control-chars")
             .arg(arg)
             .arg("one two")
             .succeeds()
@@ -1558,6 +1549,7 @@ fn test_ls_quoting_style() {
     ] {
         scene
             .ucmd()
+            .arg("--hide-control-chars")
             .arg(arg)
             .arg("one")
             .succeeds()