From 4eda814c1b15e72e7025a1dc18cc59a8ec5394d3 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Tue, 28 Dec 2021 14:13:33 -0600 Subject: [PATCH 01/23] Various improvements to ls --- src/uu/ls/src/ls.rs | 524 ++++++++++++++++++++++++--------------- tests/by-util/test_ls.rs | 91 ++++++- 2 files changed, 419 insertions(+), 196 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 356e4c0f5b5..558b4357488 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -29,7 +29,7 @@ use std::{ error::Error, fmt::Display, fs::{self, DirEntry, FileType, Metadata}, - io::{stdout, BufWriter, Stdout, Write}, + io::{stdout, BufWriter, ErrorKind, Stdout, Write}, path::{Path, PathBuf}, time::{SystemTime, UNIX_EPOCH}, }; @@ -142,14 +142,16 @@ const DEFAULT_TERM_WIDTH: u16 = 80; #[derive(Debug)] enum LsError { InvalidLineWidth(String), - NoMetadata(PathBuf), + IOError(std::io::Error), + IOErrorContext(std::io::Error, OsString), } impl UError for LsError { fn code(&self) -> i32 { match self { LsError::InvalidLineWidth(_) => 2, - LsError::NoMetadata(_) => 1, + LsError::IOError(_) => 1, + LsError::IOErrorContext(_, _) => 1, } } } @@ -160,7 +162,40 @@ impl Display for LsError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { LsError::InvalidLineWidth(s) => write!(f, "invalid line width: {}", s.quote()), - LsError::NoMetadata(p) => write!(f, "could not open file: {}", p.quote()), + LsError::IOError(e) => write!(f, "general io error: {}", e), + LsError::IOErrorContext(e, s) => { + let path = Path::new(s); + let error_kind = e.kind(); + + match error_kind { + ErrorKind::NotFound => write!( + f, + "cannot access '{}': No such file or directory", + s.to_string_lossy() + ), + ErrorKind::PermissionDenied => { + if path.is_dir() { + write!( + f, + "cannot open directory '{}': Permission denied", + s.to_string_lossy() + ) + } else { + write!( + f, + "cannot open file '{}': Permission denied", + s.to_string_lossy() + ) + } + } + _ => write!( + f, + "unknown io error: '{:?}', '{:?}'", + s.to_string_lossy(), + e + ), + } + } } } } @@ -625,6 +660,7 @@ impl Config { } #[uucore_procs::gen_uumain] +#[allow(unused_mut)] pub fn uumain(args: impl uucore::Args) -> UResult<()> { let usage = usage(); @@ -632,13 +668,23 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let matches = app.get_matches_from(args); - let config = Config::from(&matches)?; + let mut config = Config::from(&matches)?; + + #[cfg(unix)] + { + // There may be more formatting issues like this. + // Be on the look out! + if config.inode && config.format == Format::OneLine { + config.format = Format::Columns; + } + } + let locs = matches .values_of_os(options::PATHS) .map(|v| v.map(Path::new).collect()) .unwrap_or_else(|| vec![Path::new(".")]); - list(locs, config) + list(locs, &config) } pub fn uu_app() -> App<'static, 'static> { @@ -1205,6 +1251,7 @@ only ignore '.' and '..'.", /// Represents a Path along with it's associated data /// Any data that will be reused several times makes sense to be added to this structure /// Caching data here helps eliminate redundant syscalls to fetch same information +#[derive(Debug)] struct PathData { // Result got from symlink_metadata() or metadata() based on config md: OnceCell>, @@ -1215,6 +1262,7 @@ struct PathData { p_buf: PathBuf, must_dereference: bool, security_context: String, + store_md: bool, } impl PathData { @@ -1253,17 +1301,23 @@ impl PathData { } Dereference::None => false, }; + let ft = match file_type { Some(ft) => OnceCell::from(ft.ok()), None => OnceCell::new(), }; let security_context = if config.context { - get_security_context(config, &p_buf, must_dereference) + get_security_context(config, &p_buf, &must_dereference) } else { String::new() }; + let store_md: bool = match config.sort { + Sort::None => false, + _ => true, + }; + Self { md: OnceCell::new(), ft, @@ -1271,45 +1325,50 @@ impl PathData { p_buf, must_dereference, security_context, + store_md, } } - fn md(&self) -> Option<&Metadata> { - self.md - .get_or_init(|| get_metadata(&self.p_buf, self.must_dereference).ok()) - .as_ref() + fn md(&self) -> Option { + let res = get_metadata(&self.p_buf, &self.must_dereference).ok(); + if self.store_md { + self.md.get_or_init(|| res.to_owned()); + } + res } fn file_type(&self) -> Option<&FileType> { self.ft - .get_or_init(|| self.md().map(|md| md.file_type())) + .get_or_init(|| { + get_metadata(&self.p_buf, &self.must_dereference) + .ok() + .map(|md| md.file_type()) + }) .as_ref() } } -fn list(locs: Vec<&Path>, config: Config) -> UResult<()> { +fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { let mut files = Vec::::new(); let mut dirs = Vec::::new(); - let mut out = BufWriter::new(stdout()); - - for loc in &locs { - let p = PathBuf::from(loc); - let path_data = PathData::new(p, None, None, &config, true); - - if path_data.md().is_none() { - // FIXME: Would be nice to use the actual error instead of hardcoding it - // Presumably other errors can happen too? - show_error!( - "cannot access {}: No such file or directory", - path_data.p_buf.quote() - ); - set_exit_code(1); - // We found an error, no need to continue the execution + let locs_len = locs.len(); + + for loc in locs { + let pd = PathData::new(PathBuf::from(loc), None, None, &config, true); + + if get_metadata(&pd.p_buf, &pd.must_dereference).ok().is_none() { + let _ = out.flush(); + show!(LsError::IOErrorContext( + std::io::Error::new(ErrorKind::NotFound, "NotFound"), + pd.p_buf.into_os_string() + )); + //show!(err.map_err_context(|| pd.p_buf.to_string_lossy().into_owned())); continue; - } + }; - let show_dir_contents = match path_data.file_type() { + // don't do file_type() to save allocating metadata + let show_dir_contents = match pd.file_type() { Some(ft) => !config.directory && ft.is_dir(), None => { set_exit_code(1); @@ -1318,21 +1377,23 @@ fn list(locs: Vec<&Path>, config: Config) -> UResult<()> { }; if show_dir_contents { - dirs.push(path_data); + dirs.push(pd); } else { - files.push(path_data); + files.push(pd); } } - sort_entries(&mut files, &config); - display_items(&files, &config, &mut out); - sort_entries(&mut dirs, &config); - for dir in dirs { - if locs.len() > 1 || config.recursive { + sort_entries(&mut files, config); + sort_entries(&mut dirs, config); + + display_items(&files, config, &mut out); + + 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); + enter_directory(dir, config, &mut out); } Ok(()) @@ -1343,13 +1404,11 @@ fn sort_entries(entries: &mut Vec, config: &Config) { Sort::Time => entries.sort_by_key(|k| { Reverse( k.md() - .and_then(|md| get_system_time(md, config)) + .and_then(|md| get_system_time(&md, config)) .unwrap_or(UNIX_EPOCH), ) }), - Sort::Size => { - entries.sort_by_key(|k| Reverse(k.md().as_ref().map(|md| md.len()).unwrap_or(0))) - } + Sort::Size => entries.sort_by_key(|k| Reverse(k.md().map(|md| md.len()).unwrap_or(0))), // The default sort in GNU ls is case insensitive Sort::Name => entries.sort_by(|a, b| a.display_name.cmp(&b.display_name)), Sort::Version => entries @@ -1399,43 +1458,72 @@ fn should_display(entry: &DirEntry, config: &Config) -> bool { }; continue; } + // else default to display true } fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) { - let mut entries: Vec<_> = if config.files == Files::All { + // Create vec of entries with initial dot files + let mut entries: Vec = if config.files == Files::All { vec![ - PathData::new( - dir.p_buf.clone(), - Some(Ok(*dir.file_type().unwrap())), - Some(".".into()), - config, - false, - ), + PathData::new(dir.p_buf.clone(), None, Some(".".into()), config, false), PathData::new(dir.p_buf.join(".."), None, Some("..".into()), config, false), ] } else { vec![] }; - let mut temp: Vec<_> = crash_if_err!(1, fs::read_dir(&dir.p_buf)) - .map(|res| crash_if_err!(1, res)) - .filter(|res| should_display(res, config)) - .map(|res| { - PathData::new( - DirEntry::path(&res), - Some(res.file_type()), - None, - config, - false, - ) - }) - .collect(); + // Convert those entries to the PathData struct + let mut vec_path_data = Vec::new(); - sort_entries(&mut temp, config); + // check for errors early, and ignore entries with errors + let read_dir = match fs::read_dir(&dir.p_buf) { + Err(err) => { + // flush buffer because the error may get printed in the wrong order + let _ = out.flush(); + show!(LsError::IOErrorContext(err, dir.display_name.to_owned())); + return; + } + Ok(res) => res, + }; - entries.append(&mut temp); + for entry in read_dir { + let unwrapped = match entry { + Ok(path) => path, + Err(error) => { + let _ = out.flush(); + show!(LsError::IOError(error)); + continue; + } + }; + if should_display(&unwrapped, config) { + // why check the DirEntry file_type()? B/c the call is + // nearly free compared to a metadata() or file_type() call on a dir/file + let pd = match unwrapped.file_type() { + Err(_err) => { + let _ = out.flush(); + show!(LsError::IOErrorContext( + std::io::Error::new(ErrorKind::NotFound, "NotFound"), + unwrapped.file_name() + )); + //show!(err.map_err_context(|| pd.p_buf.to_string_lossy().into_owned())); + continue; + } + Ok(dir_ft) => { + PathData::new(unwrapped.path(), Some(Ok(dir_ft)), None, &config, false) + } + }; + vec_path_data.push(pd); + } + } + + sort_entries(&mut vec_path_data, config); + entries.append(&mut vec_path_data); + + if config.format == Format::Long { + display_total(&entries, config, out); + } display_items(&entries, config, out); @@ -1451,11 +1539,11 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) } } -fn get_metadata(entry: &Path, dereference: bool) -> std::io::Result { - if dereference { - entry.metadata() +fn get_metadata(p_buf: &PathBuf, deref: &bool) -> std::io::Result { + if deref.to_owned() { + p_buf.metadata() } else { - entry.symlink_metadata() + p_buf.symlink_metadata() } } @@ -1463,10 +1551,10 @@ fn display_dir_entry_size(entry: &PathData, config: &Config) -> (usize, usize, u // TODO: Cache/memorize the display_* results so we don't have to recalculate them. if let Some(md) = entry.md() { ( - display_symlink_count(md).len(), - display_uname(md, config).len(), - display_group(md, config).len(), - display_size_or_rdev(md, config).len(), + display_symlink_count(&md).len(), + display_uname(&md, config).len(), + display_group(&md, config).len(), + display_size_or_rdev(&md, config).len(), ) } else { (0, 0, 0, 0) @@ -1481,6 +1569,15 @@ fn pad_right(string: &str, count: usize) -> String { format!("{:) { + let mut total_size = 0; + for item in items { + total_size += get_metadata(&item.p_buf, &item.must_dereference) + .map_or(0, |md| get_block_size(&md, config)); + } + let _ = writeln!(out, "total {}", display_size(total_size, config)); +} + fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter) { // `-Z`, `--context`: // Display the SELinux security context or '?' if none is found. When used with the `-l` @@ -1494,7 +1591,6 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter 0 { - let _ = writeln!(out, "total {}", display_size(total_size, config)); } for item in items { @@ -1540,9 +1631,12 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter = items .iter() - .filter_map(|i| display_file_name(i, config, prefix_context)); + .map(|i| display_file_name(i, config, prefix_context, out)) + .collect::>() + .into_iter(); match config.format { Format::Columns => display_grid(names, config.width, Direction::TopToBottom, out), @@ -1671,85 +1765,142 @@ fn display_grid( /// longest_size_len: usize, /// ``` /// that decide the maximum possible character count of each field. +#[allow(clippy::write_literal)] fn display_item_long( item: &PathData, padding: PaddingCollection, config: &Config, out: &mut BufWriter, ) { - let md = match item.md() { - None => { - show!(LsError::NoMetadata(item.p_buf.clone())); - return; - } - Some(md) => md, - }; - - #[cfg(unix)] - { - if config.inode { - let _ = write!(out, "{} ", get_inode(md)); + if let Some(md) = item.md() { + #[cfg(unix)] + { + if config.inode { + let _ = write!(out, "{} ", get_inode(&md)); + } } - } - - let _ = write!( - out, - "{}{} {}", - display_permissions(md, true), - if item.security_context.len() > 1 { - // GNU `ls` uses a "." character to indicate a file with a security context, - // but not other alternate access method. - "." - } else { - "" - }, - pad_left(&display_symlink_count(md), padding.longest_link_count_len), - ); - if config.long.owner { let _ = write!( out, - " {}", - pad_right(&display_uname(md, config), padding.longest_uname_len) + "{}{} {}", + display_permissions(&md, true), + if item.security_context.len() > 1 { + // GNU `ls` uses a "." character to indicate a file with a security context, + // but not other alternate access method. + "." + } else { + "" + }, + pad_left(&display_symlink_count(&md), padding.longest_link_count_len), ); - } - if config.long.group { - let _ = write!( + if config.long.owner { + let _ = write!( + out, + " {}", + pad_right(&display_uname(&md, config), padding.longest_uname_len), + ); + } + + if config.long.group { + let _ = write!( + out, + " {}", + pad_right(&display_group(&md, config), padding.longest_group_len), + ); + } + + if config.context { + let _ = write!( + out, + " {}", + pad_right(&item.security_context, padding.longest_context_len), + ); + } + + // Author is only different from owner on GNU/Hurd, so we reuse + // the owner, since GNU/Hurd is not currently supported by Rust. + if config.long.author { + let _ = write!( + out, + " {}", + pad_right(&display_uname(&md, config), padding.longest_uname_len), + ); + } + + let dfn = display_file_name(item, config, None, out).contents; + + let _ = writeln!( out, - " {}", - pad_right(&display_group(md, config), padding.longest_group_len) + " {} {} {}", + pad_left(&display_size_or_rdev(&md, config), padding.longest_size_len), + display_date(&md, config), + dfn, ); - } + } else { + // this is expressly for the case of a dangling symlink + let _ = out.flush(); + show!(LsError::IOErrorContext( + std::io::Error::new(ErrorKind::NotFound, "NotFound"), + item.p_buf.as_os_str().to_owned() + )); + #[cfg(unix)] + { + if config.inode { + if config.recursive { + let _ = write!(out, "{} ", "?".to_string()); + } else { + let _ = write!(out, " {} ", "?".to_string()); + } + } + } - if config.context { let _ = write!( out, - " {}", - pad_right(&item.security_context, padding.longest_context_len) + "{}{} {}", + "l?????????".to_string(), + if item.security_context.len() > 1 { + // GNU `ls` uses a "." character to indicate a file with a security context, + // but not other alternate access method. + "." + } else { + "" + }, + pad_left("", padding.longest_link_count_len), ); - } - // Author is only different from owner on GNU/Hurd, so we reuse - // the owner, since GNU/Hurd is not currently supported by Rust. - if config.long.author { - let _ = write!( + if config.long.owner { + let _ = write!(out, " {}", pad_right("?", padding.longest_uname_len)); + } + + if config.long.group { + let _ = write!(out, " {}", pad_right("?", padding.longest_group_len)); + } + + if config.context { + let _ = write!( + out, + " {}", + pad_right(&item.security_context, padding.longest_context_len) + ); + } + + // Author is only different from owner on GNU/Hurd, so we reuse + // the owner, since GNU/Hurd is not currently supported by Rust. + if config.long.author { + let _ = write!(out, " {}", pad_right("?", padding.longest_uname_len)); + } + + let dfn = display_file_name(item, config, None, out).contents; + + let _ = writeln!( out, - " {}", - pad_right(&display_uname(md, config), padding.longest_uname_len) + " {} {} {}", + pad_left("?", padding.longest_size_len), + " ?", + dfn, ); } - - let _ = writeln!( - out, - " {} {} {}", - pad_left(&display_size_or_rdev(md, config), padding.longest_size_len), - display_date(md, config), - // unwrap is fine because it fails when metadata is not available - // but we already know that it is because it's checked at the - // start of the function. - display_file_name(item, config, None).unwrap().contents, - ); } #[cfg(unix)] @@ -1909,9 +2060,9 @@ fn display_size_or_rdev(metadata: &Metadata, config: &Config) -> String { let ft = metadata.file_type(); if ft.is_char_device() || ft.is_block_device() { let dev: u64 = metadata.rdev(); - let major = (dev >> 8) as u8; - let minor = dev as u8; - return format!("{}, {}", major, minor); + let major = (dev >> 24) as u8; + let minor = (dev & 0xff) as u8; + return format!("{},{:>5}", major, minor,); } } @@ -1952,7 +2103,7 @@ fn classify_file(path: &PathData) -> Option { Some('=') } else if file_type.is_fifo() { Some('|') - } else if file_type.is_file() && file_is_executable(path.md()?) { + } else if file_type.is_file() && file_is_executable(&path.md().unwrap()) { Some('*') } else { None @@ -1976,27 +2127,45 @@ fn classify_file(path: &PathData) -> Option { /// /// Note that non-unicode sequences in symlink targets are dealt with using /// [`std::path::Path::to_string_lossy`]. +#[allow(unused_variables)] fn display_file_name( path: &PathData, config: &Config, prefix_context: Option, -) -> Option { + out: &mut BufWriter, +) -> Cell { // This is our return value. We start by `&path.display_name` and modify it along the way. let mut name = escape_name(&path.display_name, &config.quoting_style); - #[cfg(unix)] - { - if config.format != Format::Long && config.inode { - name = path.md().map_or_else(|| "?".to_string(), get_inode) + " " + &name; - } - } - // We need to keep track of the width ourselves instead of letting term_grid // infer it because the color codes mess up term_grid's width calculation. let mut width = name.width(); if let Some(ls_colors) = &config.color { - name = color_name(ls_colors, &path.p_buf, name, path.md()?); + // quietly ignore any error here, we already printed the error above + if let Ok(metadata) = path.p_buf.symlink_metadata() { + name = color_name(ls_colors, &path.p_buf, name, &metadata); + } + } + + #[cfg(unix)] + { + if config.inode && config.format != Format::Long { + let inode = if let Some(md) = path.md() { + get_inode(&md) + } else { + let _ = out.flush(); + let show_error = show!(LsError::IOErrorContext( + std::io::Error::new(ErrorKind::NotFound, "NotFound"), + path.p_buf.as_os_str().to_owned() + )); + "?".to_string() + }; + // increment width here b/c name was given colors and name.width() is now the wrong + // size for display + width += inode.width(); + name = inode + " " + &name; + } } if config.indicator_style != IndicatorStyle::None { @@ -2027,45 +2196,12 @@ fn display_file_name( } } - if config.format == Format::Long && path.file_type()?.is_symlink() { - if let Ok(target) = path.p_buf.read_link() { - name.push_str(" -> "); - - // We might as well color the symlink output after the arrow. - // This makes extra system calls, but provides important information that - // people run `ls -l --color` are very interested in. - if let Some(ls_colors) = &config.color { - // We get the absolute path to be able to construct PathData with valid Metadata. - // This is because relative symlinks will fail to get_metadata. - let mut absolute_target = target.clone(); - if target.is_relative() { - if let Some(parent) = path.p_buf.parent() { - absolute_target = parent.join(absolute_target); - } - } - - let target_data = PathData::new(absolute_target, None, None, config, false); - - // If we have a symlink to a valid file, we use the metadata of said file. - // Because we use an absolute path, we can assume this is guaranteed to exist. - // Otherwise, we use path.md(), which will guarantee we color to the same - // color of non-existent symlinks according to style_for_path_with_metadata. - let target_metadata = match target_data.md() { - Some(md) => md, - None => path.md()?, - }; - - name.push_str(&color_name( - ls_colors, - &target_data.p_buf, - target.to_string_lossy().into_owned(), - target_metadata, - )); - } else { - // If no coloring is required, we just use target as is. - name.push_str(&target.to_string_lossy()); - } - } + if config.format == Format::Long + && path.file_type().is_some() + && path.file_type().unwrap().is_symlink() + { + name.push_str(" -> "); + name.push_str(&path.p_buf.read_link().unwrap().to_string_lossy()); } // Prepend the security context to the `name` and adjust `width` in order @@ -2082,10 +2218,10 @@ fn display_file_name( } } - Some(Cell { + Cell { contents: name, width, - }) + } } fn color_name(ls_colors: &LsColors, path: &Path, name: String, md: &Metadata) -> String { @@ -2110,12 +2246,12 @@ fn display_symlink_count(metadata: &Metadata) -> String { // This returns the SELinux security context as UTF8 `String`. // In the long term this should be changed to `OsStr`, see discussions at #2621/#2656 #[allow(unused_variables)] -fn get_security_context(config: &Config, p_buf: &Path, must_dereference: bool) -> String { +fn get_security_context(config: &Config, p_buf: &Path, must_dereference: &bool) -> String { let substitute_string = "?".to_string(); if config.selinux_supported { #[cfg(feature = "selinux")] { - match selinux::SecurityContext::of_path(p_buf, must_dereference, false) { + match selinux::SecurityContext::of_path(p_buf, must_dereference.to_owned(), false) { Err(_r) => { // TODO: show the actual reason why it failed show_warning!("failed to get security context of: {}", p_buf.quote()); diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index b3234ce5475..bdcbe02110d 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -11,7 +11,6 @@ use std::collections::HashMap; use std::path::Path; use std::thread::sleep; use std::time::Duration; - #[cfg(not(windows))] extern crate libc; #[cfg(not(windows))] @@ -39,6 +38,70 @@ fn test_ls_i() { new_ucmd!().arg("-il").succeeds(); } +#[test] +fn test_ls_ordering() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir("some-dir1"); + at.mkdir("some-dir2"); + at.mkdir("some-dir3"); + at.mkdir("some-dir4"); + at.mkdir("some-dir5"); + at.mkdir("some-dir6"); + + scene + .ucmd() + .arg("-i") + .succeeds() + .stdout_matches(&Regex::new(".*some-dir1.*some-dir3.*some-dir5\\n").unwrap()) + .stdout_matches(&Regex::new(".*some-dir2.*some-dir4.*some-dir6").unwrap()) + .stdout_does_not_match( + &Regex::new(".*some-dir1.*some-dir2.*some-dir3.*some-dir4.*some-dir5.*some-dir6") + .unwrap(), + ); + + scene + .ucmd() + .arg("-Rl") + .succeeds() + .stdout_matches(&Regex::new("some-dir1:\\ntotal 0").unwrap()); +} + +#[test] +fn test_ls_io_errors() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir("some-dir"); + at.symlink_file("does_not_exist", "some-dir/dangle"); + + #[cfg(any(target_os = "macos", target_os = "ios"))] + scene + .ucmd() + .arg("-1") + .arg("/var/root") + .fails() + .stderr_contains("cannot open directory") + .stderr_contains("Permission denied"); + + #[cfg(target_os = "linux")] + scene + .ucmd() + .arg("-1") + .arg("/sys/kernel/tracing") + .fails() + .stderr_contains("cannot open directory") + .stderr_contains("Permission denied"); + + scene + .ucmd() + .arg("-Li") + .arg("some-dir") + .fails() + .stderr_contains("cannot access") + .stderr_contains("No such file or directory") + .stdout_contains(if cfg!(windows) { "dangle" } else { "? dangle" }); +} + #[test] fn test_ls_walk_glob() { let scene = TestScenario::new(util_name!()); @@ -1269,6 +1332,22 @@ fn test_ls_color() { "{} test-color\nb {}\n", a_with_colors, z_with_colors )); + + // link resolution should not contain colors + at.mkdir("temp_dir"); + at.symlink_file("temp_dir/does_not_exist", "temp_dir/dangle"); + let dangle_colors = format!("\x1b[1;40;31m{}\x1b[0m", "dangle"); + scene + .ucmd() + .arg("--color") + .arg("-l") + .arg("temp_dir") + .succeeds() + .stdout_contains(format!( + "{} -> {}/temp_dir/does_not_exist", + dangle_colors, + at.subdir.to_string_lossy() + )); } #[cfg(unix)] @@ -2303,8 +2382,16 @@ fn test_ls_dangling_symlinks() { .ucmd() .arg("-Li") .arg("temp_dir") - .succeeds() // this should fail, though at the moment, ls lacks a way to propagate errors encountered during display + .fails() + .stderr_contains("cannot access") .stdout_contains(if cfg!(windows) { "dangle" } else { "? dangle" }); + + scene + .ucmd() + .arg("-Ll") + .arg("temp_dir") + .fails() + .stdout_contains("l?????????"); } #[test] From 8c043bf089cb45c21a9be22f0bd96aa032f78c77 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Tue, 28 Dec 2021 16:23:09 -0600 Subject: [PATCH 02/23] Fix store_md --- src/uu/ls/src/ls.rs | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 558b4357488..1b831e4c9e0 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1313,6 +1313,7 @@ impl PathData { String::new() }; + #[allow(clippy::match_like_matches_macro)] let store_md: bool = match config.sort { Sort::None => false, _ => true, @@ -1329,11 +1330,19 @@ impl PathData { } } + #[allow(clippy::redundant_clone)] fn md(&self) -> Option { - let res = get_metadata(&self.p_buf, &self.must_dereference).ok(); - if self.store_md { - self.md.get_or_init(|| res.to_owned()); - } + let res = if self.store_md { + self.md + .get_or_init(|| { + get_metadata(&self.p_buf.clone(), &self.must_dereference.clone()).ok() + }) + .to_owned() + } else { + get_metadata(&self.p_buf.clone(), &self.must_dereference.clone()) + .ok() + .to_owned() + }; res } @@ -1357,7 +1366,9 @@ fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { for loc in locs { let pd = PathData::new(PathBuf::from(loc), None, None, &config, true); - if get_metadata(&pd.p_buf, &pd.must_dereference).ok().is_none() { + // Getting metadata here is no big deal as it's just the CWD + // and we really just want to know if the strings exist as files/dirs + if pd.md().is_none() { let _ = out.flush(); show!(LsError::IOErrorContext( std::io::Error::new(ErrorKind::NotFound, "NotFound"), @@ -1404,11 +1415,14 @@ fn sort_entries(entries: &mut Vec, config: &Config) { Sort::Time => entries.sort_by_key(|k| { Reverse( k.md() + .as_ref() .and_then(|md| get_system_time(&md, config)) .unwrap_or(UNIX_EPOCH), ) }), - Sort::Size => entries.sort_by_key(|k| Reverse(k.md().map(|md| md.len()).unwrap_or(0))), + Sort::Size => { + entries.sort_by_key(|k| Reverse(k.md().as_ref().map(|md| md.len()).unwrap_or(0))) + } // The default sort in GNU ls is case insensitive Sort::Name => entries.sort_by(|a, b| a.display_name.cmp(&b.display_name)), Sort::Version => entries @@ -1572,7 +1586,9 @@ fn pad_right(string: &str, count: usize) -> String { fn display_total(items: &[PathData], config: &Config, out: &mut BufWriter) { let mut total_size = 0; for item in items { - total_size += get_metadata(&item.p_buf, &item.must_dereference) + total_size += item + .md() + .as_ref() .map_or(0, |md| get_block_size(&md, config)); } let _ = writeln!(out, "total {}", display_size(total_size, config)); @@ -2103,7 +2119,7 @@ fn classify_file(path: &PathData) -> Option { Some('=') } else if file_type.is_fifo() { Some('|') - } else if file_type.is_file() && file_is_executable(&path.md().unwrap()) { + } else if file_type.is_file() && file_is_executable(&path.md().as_ref().unwrap()) { Some('*') } else { None From f97acaf0d1ec13f81ebb7e4ee95f5a4e83290767 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Wed, 29 Dec 2021 17:28:27 -0600 Subject: [PATCH 03/23] Make requested changes -- cargo clippy is working again! --- src/uu/ls/src/ls.rs | 181 +++++++++++++++++++++------------------ tests/by-util/test_ls.rs | 41 ++++----- 2 files changed, 116 insertions(+), 106 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 1b831e4c9e0..2c015925ce3 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -668,23 +668,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let matches = app.get_matches_from(args); - let mut config = Config::from(&matches)?; - - #[cfg(unix)] - { - // There may be more formatting issues like this. - // Be on the look out! - if config.inode && config.format == Format::OneLine { - config.format = Format::Columns; - } - } + let config = Config::from(&matches)?; let locs = matches .values_of_os(options::PATHS) .map(|v| v.map(Path::new).collect()) .unwrap_or_else(|| vec![Path::new(".")]); - list(locs, &config) + list(locs, config) } pub fn uu_app() -> App<'static, 'static> { @@ -1262,7 +1253,6 @@ struct PathData { p_buf: PathBuf, must_dereference: bool, security_context: String, - store_md: bool, } impl PathData { @@ -1313,12 +1303,6 @@ impl PathData { String::new() }; - #[allow(clippy::match_like_matches_macro)] - let store_md: bool = match config.sort { - Sort::None => false, - _ => true, - }; - Self { md: OnceCell::new(), ft, @@ -1326,30 +1310,21 @@ impl PathData { p_buf, must_dereference, security_context, - store_md, } } - #[allow(clippy::redundant_clone)] - fn md(&self) -> Option { - let res = if self.store_md { - self.md - .get_or_init(|| { - get_metadata(&self.p_buf.clone(), &self.must_dereference.clone()).ok() - }) - .to_owned() - } else { - get_metadata(&self.p_buf.clone(), &self.must_dereference.clone()) - .ok() - .to_owned() - }; - res + fn md(&self) -> Option<&Metadata> { + self.md + .get_or_init(|| { + get_metadata(&self.p_buf, self.must_dereference).ok() + }) + .as_ref() } fn file_type(&self) -> Option<&FileType> { self.ft .get_or_init(|| { - get_metadata(&self.p_buf, &self.must_dereference) + get_metadata(&self.p_buf, self.must_dereference) .ok() .map(|md| md.file_type()) }) @@ -1357,29 +1332,29 @@ impl PathData { } } -fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { +fn list(locs: Vec<&Path>, config: Config) -> UResult<()> { let mut files = Vec::::new(); let mut dirs = Vec::::new(); let mut out = BufWriter::new(stdout()); let locs_len = locs.len(); for loc in locs { - let pd = PathData::new(PathBuf::from(loc), None, None, &config, true); + let path_data = PathData::new(PathBuf::from(loc), None, None, &config, true); // Getting metadata here is no big deal as it's just the CWD // and we really just want to know if the strings exist as files/dirs - if pd.md().is_none() { + if path_data.md().is_none() { let _ = out.flush(); show!(LsError::IOErrorContext( std::io::Error::new(ErrorKind::NotFound, "NotFound"), - pd.p_buf.into_os_string() + path_data.p_buf.into_os_string() )); //show!(err.map_err_context(|| pd.p_buf.to_string_lossy().into_owned())); continue; }; // don't do file_type() to save allocating metadata - let show_dir_contents = match pd.file_type() { + let show_dir_contents = match path_data.file_type() { Some(ft) => !config.directory && ft.is_dir(), None => { set_exit_code(1); @@ -1388,23 +1363,23 @@ fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { }; if show_dir_contents { - dirs.push(pd); + dirs.push(path_data); } else { - files.push(pd); + files.push(path_data); } } - sort_entries(&mut files, config); - sort_entries(&mut dirs, config); + sort_entries(&mut files, &config); + sort_entries(&mut dirs, &config); - display_items(&files, config, &mut out); + display_items(&files, &config, &mut out); 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); + enter_directory(dir, &config, &mut out); } Ok(()) @@ -1416,7 +1391,7 @@ fn sort_entries(entries: &mut Vec, config: &Config) { Reverse( k.md() .as_ref() - .and_then(|md| get_system_time(&md, config)) + .and_then(|md| get_system_time(md, config)) .unwrap_or(UNIX_EPOCH), ) }), @@ -1514,7 +1489,7 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) if should_display(&unwrapped, config) { // why check the DirEntry file_type()? B/c the call is // nearly free compared to a metadata() or file_type() call on a dir/file - let pd = match unwrapped.file_type() { + let path_data = match unwrapped.file_type() { Err(_err) => { let _ = out.flush(); show!(LsError::IOErrorContext( @@ -1525,10 +1500,19 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) continue; } Ok(dir_ft) => { - PathData::new(unwrapped.path(), Some(Ok(dir_ft)), None, &config, false) + let res = PathData::new(unwrapped.path(), Some(Ok(dir_ft)), None, config, false); + if dir_ft.is_symlink() && res.md().is_none() { + let _ = out.flush(); + show!(LsError::IOErrorContext( + std::io::Error::new(ErrorKind::NotFound, "NotFound"), + unwrapped.file_name() + )); + //show!(err.map_err_context(|| pd.p_buf.to_string_lossy().into_owned())); + } + res } }; - vec_path_data.push(pd); + vec_path_data.push(path_data); } } @@ -1553,8 +1537,8 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) } } -fn get_metadata(p_buf: &PathBuf, deref: &bool) -> std::io::Result { - if deref.to_owned() { +fn get_metadata(p_buf: &Path, dereference: bool) -> std::io::Result { + if dereference { p_buf.metadata() } else { p_buf.symlink_metadata() @@ -1565,10 +1549,10 @@ fn display_dir_entry_size(entry: &PathData, config: &Config) -> (usize, usize, u // TODO: Cache/memorize the display_* results so we don't have to recalculate them. if let Some(md) = entry.md() { ( - display_symlink_count(&md).len(), - display_uname(&md, config).len(), - display_group(&md, config).len(), - display_size_or_rdev(&md, config).len(), + display_symlink_count(md).len(), + display_uname(md, config).len(), + display_group(md, config).len(), + display_size_or_rdev(md, config).len(), ) } else { (0, 0, 0, 0) @@ -1589,7 +1573,7 @@ fn display_total(items: &[PathData], config: &Config, out: &mut BufWriter 1 { // GNU `ls` uses a "." character to indicate a file with a security context, // but not other alternate access method. @@ -1807,14 +1791,14 @@ fn display_item_long( } else { "" }, - pad_left(&display_symlink_count(&md), padding.longest_link_count_len), + pad_left(&display_symlink_count(md), padding.longest_link_count_len), ); if config.long.owner { let _ = write!( out, " {}", - pad_right(&display_uname(&md, config), padding.longest_uname_len), + pad_right(&display_uname(md, config), padding.longest_uname_len), ); } @@ -1822,7 +1806,7 @@ fn display_item_long( let _ = write!( out, " {}", - pad_right(&display_group(&md, config), padding.longest_group_len), + pad_right(&display_group(md, config), padding.longest_group_len), ); } @@ -1840,7 +1824,7 @@ fn display_item_long( let _ = write!( out, " {}", - pad_right(&display_uname(&md, config), padding.longest_uname_len), + pad_right(&display_uname(md, config), padding.longest_uname_len), ); } @@ -1849,24 +1833,19 @@ fn display_item_long( let _ = writeln!( out, " {} {} {}", - pad_left(&display_size_or_rdev(&md, config), padding.longest_size_len), - display_date(&md, config), + pad_left(&display_size_or_rdev(md, config), padding.longest_size_len), + display_date(md, config), dfn, ); } else { - // this is expressly for the case of a dangling symlink - let _ = out.flush(); - show!(LsError::IOErrorContext( - std::io::Error::new(ErrorKind::NotFound, "NotFound"), - item.p_buf.as_os_str().to_owned() - )); + // this 'else' is expressly for the case of a dangling symlink #[cfg(unix)] { if config.inode { if config.recursive { - let _ = write!(out, "{} ", "?".to_string()); + let _ = write!(out, " {} ", "?".to_string()); } else { - let _ = write!(out, " {} ", "?".to_string()); + let _ = write!(out, "{} ", "?".to_string()); } } } @@ -2119,7 +2098,7 @@ fn classify_file(path: &PathData) -> Option { Some('=') } else if file_type.is_fifo() { Some('|') - } else if file_type.is_file() && file_is_executable(&path.md().as_ref().unwrap()) { + } else if file_type.is_file() && file_is_executable(path.md().as_ref().unwrap()) { Some('*') } else { None @@ -2158,7 +2137,6 @@ fn display_file_name( let mut width = name.width(); if let Some(ls_colors) = &config.color { - // quietly ignore any error here, we already printed the error above if let Ok(metadata) = path.p_buf.symlink_metadata() { name = color_name(ls_colors, &path.p_buf, name, &metadata); } @@ -2168,7 +2146,7 @@ fn display_file_name( { if config.inode && config.format != Format::Long { let inode = if let Some(md) = path.md() { - get_inode(&md) + get_inode(md) } else { let _ = out.flush(); let show_error = show!(LsError::IOErrorContext( @@ -2212,12 +2190,53 @@ fn display_file_name( } } - if config.format == Format::Long - && path.file_type().is_some() - && path.file_type().unwrap().is_symlink() - { - name.push_str(" -> "); - name.push_str(&path.p_buf.read_link().unwrap().to_string_lossy()); + // if config.format == Format::Long + // && path.file_type().is_some() + // && path.file_type().unwrap().is_symlink() + // { + // name.push_str(" -> "); + // name.push_str(&path.p_buf.read_link().unwrap().to_string_lossy()); + // } + + if config.format == Format::Long && path.file_type().is_some() && path.file_type().unwrap().is_symlink() { + if let Ok(target) = path.p_buf.read_link() { + name.push_str(" -> "); + + // We might as well color the symlink output after the arrow. + // This makes extra system calls, but provides important information that + // people run `ls -l --color` are very interested in. + if let Some(ls_colors) = &config.color { + // We get the absolute path to be able to construct PathData with valid Metadata. + // This is because relative symlinks will fail to get_metadata. + let mut absolute_target = target.clone(); + if target.is_relative() { + if let Some(parent) = path.p_buf.parent() { + absolute_target = parent.join(absolute_target); + } + } + + let target_data = PathData::new(absolute_target, None, None, config, false); + + // If we have a symlink to a valid file, we use the metadata of said file. + // Because we use an absolute path, we can assume this is guaranteed to exist. + // Otherwise, we use path.md(), which will guarantee we color to the same + // color of non-existent symlinks according to style_for_path_with_metadata. + let target_metadata = match target_data.md() { + Some(md) => md, + None => path.md().unwrap(), + }; + + name.push_str(&color_name( + ls_colors, + &target_data.p_buf, + target.to_string_lossy().into_owned(), + target_metadata, + )); + } else { + // If no coloring is required, we just use target as is. + name.push_str(&target.to_string_lossy()); + } + } } // Prepend the security context to the `name` and adjust `width` in order diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index bdcbe02110d..17e4afbc3a8 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -49,17 +49,6 @@ fn test_ls_ordering() { at.mkdir("some-dir5"); at.mkdir("some-dir6"); - scene - .ucmd() - .arg("-i") - .succeeds() - .stdout_matches(&Regex::new(".*some-dir1.*some-dir3.*some-dir5\\n").unwrap()) - .stdout_matches(&Regex::new(".*some-dir2.*some-dir4.*some-dir6").unwrap()) - .stdout_does_not_match( - &Regex::new(".*some-dir1.*some-dir2.*some-dir3.*some-dir4.*some-dir5.*some-dir6") - .unwrap(), - ); - scene .ucmd() .arg("-Rl") @@ -1334,20 +1323,22 @@ fn test_ls_color() { )); // link resolution should not contain colors - at.mkdir("temp_dir"); - at.symlink_file("temp_dir/does_not_exist", "temp_dir/dangle"); - let dangle_colors = format!("\x1b[1;40;31m{}\x1b[0m", "dangle"); - scene - .ucmd() - .arg("--color") - .arg("-l") - .arg("temp_dir") - .succeeds() - .stdout_contains(format!( - "{} -> {}/temp_dir/does_not_exist", - dangle_colors, - at.subdir.to_string_lossy() - )); + // this is GNU behavior but is not the present behavior uutils + // keeping here as we may choose later to make an option + // at.mkdir("temp_dir"); + // at.symlink_file("temp_dir/does_not_exist", "temp_dir/dangle"); + // let dangle_colors = format!("\x1b[1;40;31m{}\x1b[0m", "dangle"); + // scene + // .ucmd() + // .arg("--color") + // .arg("-l") + // .arg("temp_dir") + // .succeeds() + // .stdout_contains(format!( + // "{} -> {}/temp_dir/does_not_exist", + // dangle_colors, + // at.subdir.to_string_lossy() + // )); } #[cfg(unix)] From f42640586e07d141f206c6518678f9b221622404 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Wed, 29 Dec 2021 19:31:24 -0600 Subject: [PATCH 04/23] cargo fmt and fix failing test --- src/uu/ls/src/ls.rs | 14 ++++++++------ tests/by-util/test_ls.rs | 28 +++++++++++++--------------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 2c015925ce3..93917bb21fe 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1315,9 +1315,7 @@ impl PathData { fn md(&self) -> Option<&Metadata> { self.md - .get_or_init(|| { - get_metadata(&self.p_buf, self.must_dereference).ok() - }) + .get_or_init(|| get_metadata(&self.p_buf, self.must_dereference).ok()) .as_ref() } @@ -1500,7 +1498,8 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) continue; } Ok(dir_ft) => { - let res = PathData::new(unwrapped.path(), Some(Ok(dir_ft)), None, config, false); + let res = + PathData::new(unwrapped.path(), Some(Ok(dir_ft)), None, config, false); if dir_ft.is_symlink() && res.md().is_none() { let _ = out.flush(); show!(LsError::IOErrorContext( @@ -1843,7 +1842,7 @@ fn display_item_long( { if config.inode { if config.recursive { - let _ = write!(out, " {} ", "?".to_string()); + let _ = write!(out, " {} ", "?".to_string()); } else { let _ = write!(out, "{} ", "?".to_string()); } @@ -2198,7 +2197,10 @@ fn display_file_name( // name.push_str(&path.p_buf.read_link().unwrap().to_string_lossy()); // } - if config.format == Format::Long && path.file_type().is_some() && path.file_type().unwrap().is_symlink() { + if config.format == Format::Long + && path.file_type().is_some() + && path.file_type().unwrap().is_symlink() + { if let Ok(target) = path.p_buf.read_link() { name.push_str(" -> "); diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 17e4afbc3a8..a3992714c3d 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -56,27 +56,25 @@ fn test_ls_ordering() { .stdout_matches(&Regex::new("some-dir1:\\ntotal 0").unwrap()); } +#[cfg(all(feature = "chmod"))] #[test] fn test_ls_io_errors() { let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; - at.mkdir("some-dir"); - at.symlink_file("does_not_exist", "some-dir/dangle"); - - #[cfg(any(target_os = "macos", target_os = "ios"))] - scene - .ucmd() - .arg("-1") - .arg("/var/root") - .fails() - .stderr_contains("cannot open directory") - .stderr_contains("Permission denied"); + at.mkdir("some-dir1"); + at.mkdir("some-dir2"); + at.symlink_file("does_not_exist", "some-dir2/dangle"); - #[cfg(target_os = "linux")] + scene.ccmd("chmod") + .arg("000") + .arg(format!("{}/", at.subdir.to_string_lossy())) + .arg("some-dir1") + .succeeds(); + scene .ucmd() .arg("-1") - .arg("/sys/kernel/tracing") + .arg("some-dir1") .fails() .stderr_contains("cannot open directory") .stderr_contains("Permission denied"); @@ -84,7 +82,7 @@ fn test_ls_io_errors() { scene .ucmd() .arg("-Li") - .arg("some-dir") + .arg("some-dir2") .fails() .stderr_contains("cannot access") .stderr_contains("No such file or directory") @@ -104,7 +102,7 @@ fn test_ls_walk_glob() { .to_str() .unwrap(), ); - + #[allow(clippy::trivial_regex)] let re_pwd = Regex::new(r"^\.\n").unwrap(); From 660e5e4147ea21df242f9eb4727f8b0141fdbbee Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Wed, 29 Dec 2021 19:42:32 -0600 Subject: [PATCH 05/23] Fix my fix! Ugh! --- tests/by-util/test_ls.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index a3992714c3d..962cfd953de 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -65,12 +65,8 @@ fn test_ls_io_errors() { at.mkdir("some-dir2"); at.symlink_file("does_not_exist", "some-dir2/dangle"); - scene.ccmd("chmod") - .arg("000") - .arg(format!("{}/", at.subdir.to_string_lossy())) - .arg("some-dir1") - .succeeds(); - + scene.ccmd("chmod").arg("000").arg("some-dir1").succeeds(); + scene .ucmd() .arg("-1") @@ -102,7 +98,7 @@ fn test_ls_walk_glob() { .to_str() .unwrap(), ); - + #[allow(clippy::trivial_regex)] let re_pwd = Regex::new(r"^\.\n").unwrap(); From 8030167d26912b5b80d640c86654a42cc1f4606f Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Thu, 30 Dec 2021 16:10:33 -0600 Subject: [PATCH 06/23] Fix panic on unwrap --- src/uu/ls/src/ls.rs | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 93917bb21fe..82b5b728c78 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2223,17 +2223,21 @@ fn display_file_name( // Because we use an absolute path, we can assume this is guaranteed to exist. // Otherwise, we use path.md(), which will guarantee we color to the same // color of non-existent symlinks according to style_for_path_with_metadata. - let target_metadata = match target_data.md() { - Some(md) => md, - None => path.md().unwrap(), - }; - - name.push_str(&color_name( - ls_colors, - &target_data.p_buf, - target.to_string_lossy().into_owned(), - target_metadata, - )); + if path.md().is_none() { + name.push_str(&path.p_buf.read_link().unwrap().to_string_lossy()); + } else { + let target_metadata = match target_data.md() { + Some(md) => md, + None => path.md().unwrap(), + }; + + name.push_str(&color_name( + ls_colors, + &target_data.p_buf, + target.to_string_lossy().into_owned(), + target_metadata, + )); + } } else { // If no coloring is required, we just use target as is. name.push_str(&target.to_string_lossy()); From 0bc6110322a36208286d381e7eb36fbfc2e172ed Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Fri, 31 Dec 2021 18:50:22 -0600 Subject: [PATCH 07/23] Fix inode padding, colors for dangling links --- src/uu/ls/src/ls.rs | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 82b5b728c78..64f99bbd4d2 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -299,6 +299,7 @@ struct PaddingCollection { longest_group_len: usize, longest_context_len: usize, longest_size_len: usize, + longest_inode_len: usize, } impl Config { @@ -1544,7 +1545,7 @@ fn get_metadata(p_buf: &Path, dereference: bool) -> std::io::Result { } } -fn display_dir_entry_size(entry: &PathData, config: &Config) -> (usize, usize, usize, usize) { +fn display_dir_entry_size(entry: &PathData, config: &Config) -> (usize, usize, usize, usize, usize) { // TODO: Cache/memorize the display_* results so we don't have to recalculate them. if let Some(md) = entry.md() { ( @@ -1552,9 +1553,10 @@ fn display_dir_entry_size(entry: &PathData, config: &Config) -> (usize, usize, u display_uname(md, config).len(), display_group(md, config).len(), display_size_or_rdev(md, config).len(), + display_inode(md).len(), ) } else { - (0, 0, 0, 0) + (0, 0, 0, 0, 0) } } @@ -1584,21 +1586,23 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter String { metadata.nlink().to_string() } +#[cfg(unix)] +fn display_inode(metadata: &Metadata) -> String { + metadata.ino().to_string() +} + // This returns the SELinux security context as UTF8 `String`. // In the long term this should be changed to `OsStr`, see discussions at #2621/#2656 #[allow(unused_variables)] From f25c66b946ccc0a407a4dc5bdfb8b25342c60f50 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Fri, 31 Dec 2021 19:04:42 -0600 Subject: [PATCH 08/23] Run rustfmt --- src/uu/ls/src/ls.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 64f99bbd4d2..a68fc17a7ec 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1545,7 +1545,10 @@ fn get_metadata(p_buf: &Path, dereference: bool) -> std::io::Result { } } -fn display_dir_entry_size(entry: &PathData, config: &Config) -> (usize, usize, usize, usize, usize) { +fn display_dir_entry_size( + entry: &PathData, + config: &Config, +) -> (usize, usize, usize, usize, usize) { // TODO: Cache/memorize the display_* results so we don't have to recalculate them. if let Some(md) = entry.md() { ( @@ -1781,10 +1784,11 @@ fn display_item_long( { if config.inode { let _ = write!( - out, - "{} ", - pad_left(&get_inode(md), padding.longest_inode_len), - );} + out, + "{} ", + pad_left(&get_inode(md), padding.longest_inode_len), + ); + } } let _ = write!( @@ -1849,11 +1853,8 @@ fn display_item_long( #[cfg(unix)] { if config.inode { - let _ = write!( - out, - "{} ", - pad_left("?", padding.longest_inode_len), - );} + let _ = write!(out, "{} ", pad_left("?", padding.longest_inode_len),); + } } let _ = write!( From 0c7e4c5e675d76cc86978163bf037c6bb57a41da Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Sat, 1 Jan 2022 11:41:24 -0600 Subject: [PATCH 09/23] Make requested changes --- src/uu/ls/src/ls.rs | 36 ++++++++++++++---------------------- tests/by-util/test_ls.rs | 18 ------------------ 2 files changed, 14 insertions(+), 40 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index a68fc17a7ec..cfcd60be789 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -661,7 +661,6 @@ impl Config { } #[uucore_procs::gen_uumain] -#[allow(unused_mut)] pub fn uumain(args: impl uucore::Args) -> UResult<()> { let usage = usage(); @@ -1299,7 +1298,7 @@ impl PathData { }; let security_context = if config.context { - get_security_context(config, &p_buf, &must_dereference) + get_security_context(config, &p_buf, must_dereference) } else { String::new() }; @@ -1322,11 +1321,7 @@ impl PathData { fn file_type(&self) -> Option<&FileType> { self.ft - .get_or_init(|| { - get_metadata(&self.p_buf, self.must_dereference) - .ok() - .map(|md| md.file_type()) - }) + .get_or_init(|| self.md().map(|md| md.file_type())) .as_ref() } } @@ -1389,14 +1384,11 @@ fn sort_entries(entries: &mut Vec, config: &Config) { Sort::Time => entries.sort_by_key(|k| { Reverse( k.md() - .as_ref() .and_then(|md| get_system_time(md, config)) .unwrap_or(UNIX_EPOCH), ) }), - Sort::Size => { - entries.sort_by_key(|k| Reverse(k.md().as_ref().map(|md| md.len()).unwrap_or(0))) - } + Sort::Size => entries.sort_by_key(|k| Reverse(k.md().map(|md| md.len()).unwrap_or(0))), // The default sort in GNU ls is case insensitive Sort::Name => entries.sort_by(|a, b| a.display_name.cmp(&b.display_name)), Sort::Version => entries @@ -1894,12 +1886,13 @@ fn display_item_long( } let dfn = display_file_name(item, config, None, out).contents; + let date_len = 12; let _ = writeln!( out, " {} {} {}", pad_left("?", padding.longest_size_len), - " ?", + pad_left("?", date_len), dfn, ); } @@ -2197,14 +2190,6 @@ fn display_file_name( } } - // if config.format == Format::Long - // && path.file_type().is_some() - // && path.file_type().unwrap().is_symlink() - // { - // name.push_str(" -> "); - // name.push_str(&path.p_buf.read_link().unwrap().to_string_lossy()); - // } - if config.format == Format::Long && path.file_type().is_some() && path.file_type().unwrap().is_symlink() @@ -2294,13 +2279,20 @@ fn display_symlink_count(metadata: &Metadata) -> String { #[cfg(unix)] fn display_inode(metadata: &Metadata) -> String { - metadata.ino().to_string() + #[cfg(unix)] + { + metadata.ino().to_string() + } + #[cfg(not(unix))] + { + "".to_string() + } } // This returns the SELinux security context as UTF8 `String`. // In the long term this should be changed to `OsStr`, see discussions at #2621/#2656 #[allow(unused_variables)] -fn get_security_context(config: &Config, p_buf: &Path, must_dereference: &bool) -> String { +fn get_security_context(config: &Config, p_buf: &Path, must_dereference: bool) -> String { let substitute_string = "?".to_string(); if config.selinux_supported { #[cfg(feature = "selinux")] diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 962cfd953de..76fbb4128bb 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -1315,24 +1315,6 @@ fn test_ls_color() { "{} test-color\nb {}\n", a_with_colors, z_with_colors )); - - // link resolution should not contain colors - // this is GNU behavior but is not the present behavior uutils - // keeping here as we may choose later to make an option - // at.mkdir("temp_dir"); - // at.symlink_file("temp_dir/does_not_exist", "temp_dir/dangle"); - // let dangle_colors = format!("\x1b[1;40;31m{}\x1b[0m", "dangle"); - // scene - // .ucmd() - // .arg("--color") - // .arg("-l") - // .arg("temp_dir") - // .succeeds() - // .stdout_contains(format!( - // "{} -> {}/temp_dir/does_not_exist", - // dangle_colors, - // at.subdir.to_string_lossy() - // )); } #[cfg(unix)] From 3c512c409f173a2c5499bad93231185c8db5b6d9 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Sat, 1 Jan 2022 11:46:15 -0600 Subject: [PATCH 10/23] Remove unused UIoError errors in comments --- src/uu/ls/src/ls.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index cfcd60be789..62e43d4f09c 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1343,7 +1343,6 @@ fn list(locs: Vec<&Path>, config: Config) -> UResult<()> { std::io::Error::new(ErrorKind::NotFound, "NotFound"), path_data.p_buf.into_os_string() )); - //show!(err.map_err_context(|| pd.p_buf.to_string_lossy().into_owned())); continue; }; @@ -1487,7 +1486,6 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) std::io::Error::new(ErrorKind::NotFound, "NotFound"), unwrapped.file_name() )); - //show!(err.map_err_context(|| pd.p_buf.to_string_lossy().into_owned())); continue; } Ok(dir_ft) => { @@ -1499,7 +1497,6 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) std::io::Error::new(ErrorKind::NotFound, "NotFound"), unwrapped.file_name() )); - //show!(err.map_err_context(|| pd.p_buf.to_string_lossy().into_owned())); } res } From e2995dfc003c4825f87b0fb747aedae56e648218 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Sat, 1 Jan 2022 11:57:16 -0600 Subject: [PATCH 11/23] Revert changes made to devices - must have made changes in another branch! --- src/uu/ls/src/ls.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 62e43d4f09c..304f8210c1a 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2052,9 +2052,9 @@ fn display_size_or_rdev(metadata: &Metadata, config: &Config) -> String { let ft = metadata.file_type(); if ft.is_char_device() || ft.is_block_device() { let dev: u64 = metadata.rdev(); - let major = (dev >> 24) as u8; - let minor = (dev & 0xff) as u8; - return format!("{},{:>5}", major, minor,); + let major = (dev >> 8) as u8; + let minor = dev as u8; + return format!("{}, {}", major, minor,); } } From 4d3ea3a314d6b8a5fbe5881caabfbb1902bf9df4 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Sat, 1 Jan 2022 12:41:12 -0600 Subject: [PATCH 12/23] Fix for Windows (no inode) --- src/uu/ls/src/ls.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 304f8210c1a..460dcbe4f09 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2274,7 +2274,6 @@ fn display_symlink_count(metadata: &Metadata) -> String { metadata.nlink().to_string() } -#[cfg(unix)] fn display_inode(metadata: &Metadata) -> String { #[cfg(unix)] { From d0ee7eb9e5be0e061cafefee0a35eb0068020f07 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Sat, 1 Jan 2022 13:55:17 -0600 Subject: [PATCH 13/23] Fix Windows lints --- src/uu/ls/src/ls.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 460dcbe4f09..7801c318c42 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2274,6 +2274,7 @@ fn display_symlink_count(metadata: &Metadata) -> String { metadata.nlink().to_string() } +#[allow(unused_variables)] fn display_inode(metadata: &Metadata) -> String { #[cfg(unix)] { From 71f3773f860c02a932141bfbc4c8a2951e96e158 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Sat, 1 Jan 2022 18:34:40 -0600 Subject: [PATCH 14/23] Fix Window lints --- src/uu/ls/src/ls.rs | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 7801c318c42..8e0b708c9a3 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -294,12 +294,13 @@ struct LongFormat { } struct PaddingCollection { + #[cfg(unix)] + longest_inode_len: usize, longest_link_count_len: usize, longest_uname_len: usize, longest_group_len: usize, longest_context_len: usize, longest_size_len: usize, - longest_inode_len: usize, } impl Config { @@ -1577,6 +1578,7 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter Date: Sat, 1 Jan 2022 19:39:56 -0600 Subject: [PATCH 15/23] Fix Windows lints --- src/uu/ls/src/ls.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 8e0b708c9a3..2b272206ccc 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1597,14 +1597,27 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter Date: Sun, 2 Jan 2022 00:37:11 -0600 Subject: [PATCH 16/23] Cleanup comments, small changes --- src/uu/ls/src/ls.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 2b272206ccc..6abaaa2b944 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1347,7 +1347,6 @@ fn list(locs: Vec<&Path>, config: Config) -> UResult<()> { continue; }; - // don't do file_type() to save allocating metadata let show_dir_contents = match path_data.file_type() { Some(ft) => !config.directory && ft.is_dir(), None => { @@ -1415,7 +1414,7 @@ fn is_hidden(file_path: &DirEntry) -> bool { let attr = metadata.file_attributes(); (attr & 0x2) > 0 } - #[cfg(unix)] + #[cfg(not(windows))] { file_path .file_name() @@ -2306,7 +2305,7 @@ fn display_symlink_count(metadata: &Metadata) -> String { fn display_inode(metadata: &Metadata) -> String { #[cfg(unix)] { - metadata.ino().to_string() + get_inode(metadata) } #[cfg(not(unix))] { From 155aa9b91cd951f42e23590272e3ec41e8e4b902 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Sun, 2 Jan 2022 08:39:49 -0600 Subject: [PATCH 17/23] Match GNU behavior: print no name heading, when no permissions on a dir, and test --- src/uu/ls/src/ls.rs | 23 ++++++++++++++--------- tests/by-util/test_ls.rs | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 6abaaa2b944..58b09bcbe8b 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1331,7 +1331,7 @@ fn list(locs: Vec<&Path>, config: Config) -> UResult<()> { let mut files = Vec::::new(); let mut dirs = Vec::::new(); let mut out = BufWriter::new(stdout()); - let locs_len = locs.len(); + let initial_locs_len = locs.len(); for loc in locs { let path_data = PathData::new(PathBuf::from(loc), None, None, &config, true); @@ -1368,11 +1368,7 @@ fn list(locs: Vec<&Path>, config: Config) -> UResult<()> { display_items(&files, &config, &mut out); 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); + enter_directory(dir, &config, initial_locs_len, &mut out); } Ok(()) @@ -1442,7 +1438,12 @@ fn should_display(entry: &DirEntry, config: &Config) -> bool { true } -fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) { +fn enter_directory( + dir: &PathData, + config: &Config, + initial_locs_len: usize, + out: &mut BufWriter, +) { // Create vec of entries with initial dot files let mut entries: Vec = if config.files == Files::All { vec![ @@ -1508,6 +1509,11 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) sort_entries(&mut vec_path_data, config); entries.append(&mut vec_path_data); + // Print dir heading - name... + if initial_locs_len > 1 || config.recursive { + let _ = writeln!(out, "\n{}:", dir.p_buf.display()); + } + // ...and total if config.format == Format::Long { display_total(&entries, config, out); } @@ -1520,8 +1526,7 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) .skip(if config.files == Files::All { 2 } else { 0 }) .filter(|p| p.file_type().map(|ft| ft.is_dir()).unwrap_or(false)) { - let _ = writeln!(out, "\n{}:", e.p_buf.display()); - enter_directory(e, config, out); + enter_directory(e, config, 0, out); } } } diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 76fbb4128bb..d9b6e165f8f 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -64,6 +64,12 @@ fn test_ls_io_errors() { at.mkdir("some-dir1"); at.mkdir("some-dir2"); at.symlink_file("does_not_exist", "some-dir2/dangle"); + at.mkdir("some-dir3"); + at.mkdir("some-dir3/some-dir4"); + at.mkdir("some-dir3/some-dir5"); + at.mkdir("some-dir3/some-dir6"); + at.mkdir("some-dir3/some-dir7"); + at.mkdir("some-dir3/some-dir8"); scene.ccmd("chmod").arg("000").arg("some-dir1").succeeds(); @@ -83,6 +89,32 @@ fn test_ls_io_errors() { .stderr_contains("cannot access") .stderr_contains("No such file or directory") .stdout_contains(if cfg!(windows) { "dangle" } else { "? dangle" }); + + scene + .ccmd("chmod") + .arg("000") + .arg("some-dir3/some-dir4") + .succeeds(); + + scene + .ucmd() + .arg("-la") + .arg("some-dir3/*") + .fails() + .stderr_contains("some-dir4") + .stderr_contains("cannot open directory") + .stderr_contains("Permission denied") + .stdout_does_not_contain("some-dir4"); + + scene + .ucmd() + .arg("-laR") + .arg("some-dir3") + .fails() + .stderr_contains("some-dir4") + .stderr_contains("cannot open directory") + .stderr_contains("Permission denied") + .stdout_does_not_contain("some-dir4"); } #[test] From 0b2e47332bb3002a1a8eafaaf4198cf50b933b4b Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Sun, 2 Jan 2022 15:40:20 -0600 Subject: [PATCH 18/23] Fix MinRustV lint --- tests/by-util/test_ls.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index d9b6e165f8f..21d5f9fcbb7 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -101,7 +101,6 @@ fn test_ls_io_errors() { .arg("-la") .arg("some-dir3/*") .fails() - .stderr_contains("some-dir4") .stderr_contains("cannot open directory") .stderr_contains("Permission denied") .stdout_does_not_contain("some-dir4"); From db4811f985ceefe88a3d42dbe66fa2a8074c340c Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Sun, 2 Jan 2022 18:10:30 -0600 Subject: [PATCH 19/23] Works on my machine! --- tests/by-util/test_ls.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 21d5f9fcbb7..d9b6e165f8f 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -101,6 +101,7 @@ fn test_ls_io_errors() { .arg("-la") .arg("some-dir3/*") .fails() + .stderr_contains("some-dir4") .stderr_contains("cannot open directory") .stderr_contains("Permission denied") .stdout_does_not_contain("some-dir4"); From a0ebc3d81af3c01487ae05ff4f5a1b865ca189d0 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Mon, 3 Jan 2022 10:59:49 -0600 Subject: [PATCH 20/23] Fix failing test: return PathBuf instead of OsString with error --- src/uu/ls/src/ls.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 58b09bcbe8b..079dbfb94f9 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -143,7 +143,7 @@ const DEFAULT_TERM_WIDTH: u16 = 80; enum LsError { InvalidLineWidth(String), IOError(std::io::Error), - IOErrorContext(std::io::Error, OsString), + IOErrorContext(std::io::Error, PathBuf), } impl UError for LsError { @@ -163,35 +163,34 @@ impl Display for LsError { match self { LsError::InvalidLineWidth(s) => write!(f, "invalid line width: {}", s.quote()), LsError::IOError(e) => write!(f, "general io error: {}", e), - LsError::IOErrorContext(e, s) => { - let path = Path::new(s); + LsError::IOErrorContext(e, p) => { let error_kind = e.kind(); match error_kind { ErrorKind::NotFound => write!( f, "cannot access '{}': No such file or directory", - s.to_string_lossy() + p.to_string_lossy() ), ErrorKind::PermissionDenied => { - if path.is_dir() { + if p.is_dir() { write!( f, "cannot open directory '{}': Permission denied", - s.to_string_lossy() + p.to_string_lossy() ) } else { write!( f, "cannot open file '{}': Permission denied", - s.to_string_lossy() + p.to_string_lossy() ) } } _ => write!( f, "unknown io error: '{:?}', '{:?}'", - s.to_string_lossy(), + p.to_string_lossy(), e ), } @@ -1342,7 +1341,7 @@ fn list(locs: Vec<&Path>, config: Config) -> UResult<()> { let _ = out.flush(); show!(LsError::IOErrorContext( std::io::Error::new(ErrorKind::NotFound, "NotFound"), - path_data.p_buf.into_os_string() + path_data.p_buf )); continue; }; @@ -1462,7 +1461,7 @@ fn enter_directory( Err(err) => { // flush buffer because the error may get printed in the wrong order let _ = out.flush(); - show!(LsError::IOErrorContext(err, dir.display_name.to_owned())); + show!(LsError::IOErrorContext(err, dir.p_buf.clone())); return; } Ok(res) => res, @@ -1485,7 +1484,7 @@ fn enter_directory( let _ = out.flush(); show!(LsError::IOErrorContext( std::io::Error::new(ErrorKind::NotFound, "NotFound"), - unwrapped.file_name() + unwrapped.path() )); continue; } @@ -1496,7 +1495,7 @@ fn enter_directory( let _ = out.flush(); show!(LsError::IOErrorContext( std::io::Error::new(ErrorKind::NotFound, "NotFound"), - unwrapped.file_name() + unwrapped.path() )); } res @@ -2180,7 +2179,7 @@ fn display_file_name( let _ = out.flush(); let show_error = show!(LsError::IOErrorContext( std::io::Error::new(ErrorKind::NotFound, "NotFound"), - path.p_buf.as_os_str().to_owned() + path.p_buf.clone(), )); "?".to_string() }; From 8fcda6b51f390b267d951b914237c5caf67c6dd5 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Mon, 3 Jan 2022 11:52:53 -0600 Subject: [PATCH 21/23] Stumped: Works on my machine! Testing the test machine to see what works. --- tests/by-util/test_ls.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index d9b6e165f8f..bbd40da1654 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -96,16 +96,6 @@ fn test_ls_io_errors() { .arg("some-dir3/some-dir4") .succeeds(); - scene - .ucmd() - .arg("-la") - .arg("some-dir3/*") - .fails() - .stderr_contains("some-dir4") - .stderr_contains("cannot open directory") - .stderr_contains("Permission denied") - .stdout_does_not_contain("some-dir4"); - scene .ucmd() .arg("-laR") From 21f544243bfcc53485bb8cd821f370803ffc7e54 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Mon, 3 Jan 2022 12:01:03 -0600 Subject: [PATCH 22/23] Issue was globbing and that the semantics are different for recursive, should be fixed?! --- tests/by-util/test_ls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index bbd40da1654..b02581dfc57 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -104,7 +104,7 @@ fn test_ls_io_errors() { .stderr_contains("some-dir4") .stderr_contains("cannot open directory") .stderr_contains("Permission denied") - .stdout_does_not_contain("some-dir4"); + .stdout_does_contain("some-dir4"); } #[test] From 9bb8ef05b501ff0f2941adacca25867d503943d8 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Mon, 3 Jan 2022 12:06:23 -0600 Subject: [PATCH 23/23] Ugh, got name of function wrong! --- tests/by-util/test_ls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index b02581dfc57..4749e2c29eb 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -104,7 +104,7 @@ fn test_ls_io_errors() { .stderr_contains("some-dir4") .stderr_contains("cannot open directory") .stderr_contains("Permission denied") - .stdout_does_contain("some-dir4"); + .stdout_contains("some-dir4"); } #[test]