From e241f3ad69c9895330c0555963b8be9d7f96edb8 Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Thu, 22 Apr 2021 22:45:24 +0200 Subject: [PATCH 1/8] ls: skip reading metadata --- src/uu/ls/src/ls.rs | 176 +++++++++++++++++++++++--------------------- 1 file changed, 93 insertions(+), 83 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index e0aa3ec4b..d744e2914 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -19,11 +19,11 @@ mod version_cmp; use clap::{App, Arg}; use globset::{self, Glob, GlobSet, GlobSetBuilder}; use number_prefix::NumberPrefix; +use once_cell::unsync::OnceCell; use quoting_style::{escape_name, QuotingStyle}; #[cfg(unix)] use std::collections::HashMap; -use std::fs; -use std::fs::{DirEntry, FileType, Metadata}; +use std::fs::{self, DirEntry, FileType, Metadata}; #[cfg(unix)] use std::os::unix::fs::FileTypeExt; #[cfg(any(unix, target_os = "redox"))] @@ -492,6 +492,10 @@ impl Config { } } + if files != Files::Normal { + ignore_patterns.add(Glob::new(".*").unwrap()); + } + let ignore_patterns = ignore_patterns.build().unwrap(); let dereference = if options.is_present(options::dereference::ALL) { @@ -1043,29 +1047,66 @@ pub fn uumain(args: impl uucore::Args) -> i32 { /// Caching data here helps eliminate redundant syscalls to fetch same information struct PathData { // Result got from symlink_metadata() or metadata() based on config - md: std::io::Result, - // String formed from get_lossy_string() for the path - lossy_string: String, + md: OnceCell>, + ft: OnceCell>, // Name of the file - will be empty for . or .. file_name: String, // PathBuf that all above data corresponds to p_buf: PathBuf, + must_dereference: bool, } impl PathData { - fn new(p_buf: PathBuf, config: &Config, command_line: bool) -> Self { - let md = get_metadata(&p_buf, config, command_line); - let lossy_string = p_buf.to_string_lossy().into_owned(); + fn new( + p_buf: PathBuf, + file_type: Option>, + config: &Config, + command_line: bool, + ) -> Self { let name = p_buf .file_name() .map_or(String::new(), |s| s.to_string_lossy().into_owned()); + let must_dereference = match &config.dereference { + Dereference::All => true, + Dereference::Args => command_line, + Dereference::DirArgs => { + if command_line { + if let Ok(md) = p_buf.metadata() { + md.is_dir() + } else { + false + } + } else { + false + } + } + Dereference::None => false, + }; + let ft = match file_type { + Some(ft) => OnceCell::from(ft.ok()), + None => OnceCell::new(), + }; + Self { - md, - lossy_string, + md: OnceCell::new(), + ft, file_name: name, p_buf, + must_dereference, } } + + 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(|| self.md().map(|md| md.file_type())) + .as_ref() + } } fn list(locs: Vec, config: Config) -> i32 { @@ -1085,10 +1126,10 @@ fn list(locs: Vec, config: Config) -> i32 { continue; } - let path_data = PathData::new(p, &config, true); + let path_data = PathData::new(p, None, &config, true); - let show_dir_contents = if let Ok(md) = path_data.md.as_ref() { - !config.directory && md.is_dir() + let show_dir_contents = if let Some(ft) = path_data.file_type() { + !config.directory && ft.is_dir() } else { has_failed = true; false @@ -1106,7 +1147,7 @@ fn list(locs: Vec, config: Config) -> i32 { sort_entries(&mut dirs, &config); for dir in dirs { if number_of_locs > 1 { - println!("\n{}:", dir.lossy_string); + println!("\n{}:", dir.p_buf.display()); } enter_directory(&dir, &config); } @@ -1121,17 +1162,16 @@ fn sort_entries(entries: &mut Vec, config: &Config) { match config.sort { Sort::Time => entries.sort_by_key(|k| { Reverse( - k.md.as_ref() - .ok() + k.md() .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))) + 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_key(|k| k.file_name.to_lowercase()), + Sort::Name => entries.sort_by_key(|k| k.file_name.to_ascii_lowercase()), Sort::Version => entries.sort_by(|k, j| version_cmp::version_cmp(&k.p_buf, &j.p_buf)), Sort::None => {} } @@ -1169,8 +1209,8 @@ fn should_display(entry: &DirEntry, config: &Config) -> bool { fn enter_directory(dir: &PathData, config: &Config) { let mut entries: Vec<_> = if config.files == Files::All { vec![ - PathData::new(dir.p_buf.join("."), config, false), - PathData::new(dir.p_buf.join(".."), config, false), + PathData::new(dir.p_buf.join("."), None, config, false), + PathData::new(dir.p_buf.join(".."), None, config, false), ] } else { vec![] @@ -1179,7 +1219,7 @@ fn enter_directory(dir: &PathData, config: &Config) { let mut temp: Vec<_> = safe_unwrap!(fs::read_dir(&dir.p_buf)) .map(|res| safe_unwrap!(res)) .filter(|e| should_display(e, config)) - .map(|e| PathData::new(DirEntry::path(&e), config, false)) + .map(|e| PathData::new(DirEntry::path(&e), Some(e.file_type()), config, false)) .collect(); sort_entries(&mut temp, config); @@ -1192,31 +1232,15 @@ fn enter_directory(dir: &PathData, config: &Config) { for e in entries .iter() .skip(if config.files == Files::All { 2 } else { 0 }) - .filter(|p| p.md.as_ref().map(|md| md.is_dir()).unwrap_or(false)) + .filter(|p| p.file_type().map(|ft| ft.is_dir()).unwrap_or(false)) { - println!("\n{}:", e.lossy_string); + println!("\n{}:", e.p_buf.display()); enter_directory(&e, config); } } } -fn get_metadata(entry: &Path, config: &Config, command_line: bool) -> std::io::Result { - let dereference = match &config.dereference { - Dereference::All => true, - Dereference::Args => command_line, - Dereference::DirArgs => { - if command_line { - if let Ok(md) = entry.metadata() { - md.is_dir() - } else { - false - } - } else { - false - } - } - Dereference::None => false, - }; +fn get_metadata(entry: &Path, dereference: bool) -> std::io::Result { if dereference { entry.metadata().or_else(|_| entry.symlink_metadata()) } else { @@ -1225,7 +1249,7 @@ fn get_metadata(entry: &Path, config: &Config, command_line: bool) -> std::io::R } fn display_dir_entry_size(entry: &PathData, config: &Config) -> (usize, usize) { - if let Ok(md) = entry.md.as_ref() { + if let Some(md) = entry.md() { ( display_symlink_count(&md).len(), display_file_size(&md, config).len(), @@ -1251,17 +1275,9 @@ fn display_items(items: &[PathData], strip: Option<&Path>, config: &Config) { display_item_long(item, strip, max_links, max_size, config); } } else { - let names = items.iter().filter_map(|i| { - let md = i.md.as_ref(); - match md { - Err(e) => { - let filename = get_file_name(&i.p_buf, strip); - show_error!("'{}': {}", filename, e); - None - } - Ok(md) => Some(display_file_name(&i.p_buf, strip, &md, config)), - } - }); + let names = items + .iter() + .filter_map(|i| display_file_name(&i, strip, config)); match (&config.format, config.width) { (Format::Columns, Some(width)) => display_grid(names, width, Direction::TopToBottom), @@ -1325,13 +1341,13 @@ fn display_item_long( max_size: usize, config: &Config, ) { - let md = match &item.md { - Err(e) => { + let md = match item.md() { + None => { let filename = get_file_name(&item.p_buf, strip); - show_error!("{}: {}", filename, e); + show_error!("could not show file: {}", filename); return; } - Ok(md) => md, + Some(md) => md, }; #[cfg(unix)] @@ -1366,7 +1382,10 @@ fn display_item_long( " {} {} {}", pad_left(display_file_size(&md, config), max_size), display_date(&md, config), - display_file_name(&item.p_buf, strip, &md, config).contents, + // 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, strip, config).unwrap().contents, ); } @@ -1537,14 +1556,9 @@ fn get_file_name(name: &Path, strip: Option<&Path>) -> String { } #[cfg(not(unix))] -fn display_file_name( - path: &Path, - strip: Option<&Path>, - metadata: &Metadata, - config: &Config, -) -> Cell { - let mut name = escape_name(get_file_name(path, strip), &config.quoting_style); - let file_type = metadata.file_type(); +fn display_file_name(path: &PathData, strip: Option<&Path>, config: &Config) -> Option { + let mut name = escape_name(get_file_name(&path.p_buf, strip), &config.quoting_style); + let file_type = path.file_type()?; match config.indicator_style { IndicatorStyle::Classify | IndicatorStyle::FileType => { @@ -1563,8 +1577,8 @@ fn display_file_name( _ => (), }; - if config.format == Format::Long && metadata.file_type().is_symlink() { - if let Ok(target) = path.read_link() { + if config.format == Format::Long && path.file_type()?.is_symlink() { + if let Ok(target) = path.p_buf.read_link() { // We don't bother updating width here because it's not used for long listings let target_name = target.to_string_lossy().to_string(); name.push_str(" -> "); @@ -1572,7 +1586,7 @@ fn display_file_name( } } - name.into() + Some(name.into()) } #[cfg(unix)] @@ -1604,26 +1618,22 @@ macro_rules! has { #[cfg(unix)] #[allow(clippy::cognitive_complexity)] -fn display_file_name( - path: &Path, - strip: Option<&Path>, - metadata: &Metadata, - config: &Config, -) -> Cell { - let mut name = escape_name(get_file_name(path, strip), &config.quoting_style); +fn display_file_name(path: &PathData, strip: Option<&Path>, config: &Config) -> Option { + let mut name = escape_name(get_file_name(&path.p_buf, strip), &config.quoting_style); if config.format != Format::Long && config.inode { - name = get_inode(metadata) + " " + &name; + name = get_inode(path.md()?) + " " + &name; } let mut width = UnicodeWidthStr::width(&*name); let ext; if config.color || config.indicator_style != IndicatorStyle::None { - let file_type = metadata.file_type(); + let metadata = path.md()?; + let file_type = path.file_type()?; let (code, sym) = if file_type.is_dir() { ("di", Some('/')) } else if file_type.is_symlink() { - if path.exists() { + if path.p_buf.exists() { ("ln", Some('@')) } else { ("or", Some('@')) @@ -1657,7 +1667,7 @@ fn display_file_name( ("ex", sym) } else if metadata.nlink() > 1 { ("mh", sym) - } else if let Some(e) = path.extension() { + } else if let Some(e) = path.p_buf.extension() { ext = format!("*.{}", e.to_string_lossy()); (ext.as_str(), None) } else { @@ -1696,8 +1706,8 @@ fn display_file_name( } } - if config.format == Format::Long && metadata.file_type().is_symlink() { - if let Ok(target) = path.read_link() { + if config.format == Format::Long && path.file_type()?.is_symlink() { + if let Ok(target) = path.p_buf.read_link() { // We don't bother updating width here because it's not used for long listings let code = if target.exists() { "fi" } else { "mi" }; let target_name = color_name(target.to_string_lossy().to_string(), code); @@ -1706,10 +1716,10 @@ fn display_file_name( } } - Cell { + Some(Cell { contents: name, width, - } + }) } #[cfg(not(unix))] From a114f855f08426ab33a36ac81d062fca23d5e2ba Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Thu, 22 Apr 2021 23:43:00 +0200 Subject: [PATCH 2/8] ls: revert to_ascii_lowercase --- src/uu/ls/src/ls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index d744e2914..f820ffffe 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1171,7 +1171,7 @@ fn sort_entries(entries: &mut Vec, config: &Config) { 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_key(|k| k.file_name.to_ascii_lowercase()), + Sort::Name => entries.sort_by_key(|k| k.file_name.to_lowercase()), Sort::Version => entries.sort_by(|k, j| version_cmp::version_cmp(&k.p_buf, &j.p_buf)), Sort::None => {} } From 3874a2445744a853608ef9854a3db5d44289f549 Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Fri, 23 Apr 2021 00:35:45 +0200 Subject: [PATCH 3/8] ls: add once_cell to Cargo.toml --- src/uu/ls/Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/uu/ls/Cargo.toml b/src/uu/ls/Cargo.toml index dacdc7cd9..292a8b9d5 100644 --- a/src/uu/ls/Cargo.toml +++ b/src/uu/ls/Cargo.toml @@ -25,6 +25,7 @@ unicode-width = "0.1.5" globset = "0.4.6" uucore = { version=">=0.0.8", package="uucore", path="../../uucore", features=["entries", "fs"] } uucore_procs = { version=">=0.0.5", package="uucore_procs", path="../../uucore_procs" } +once_cell = "1.7.2" [target.'cfg(unix)'.dependencies] atty = "0.2" From 72bf7afe5b048e50dfe45558ceaa7e39e6345d2c Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Fri, 23 Apr 2021 00:49:05 +0200 Subject: [PATCH 4/8] ls: also update Cargo.lock --- Cargo.lock | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 461716b1b..0a07b83b0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -897,6 +897,12 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b8f8bdf33df195859076e54ab11ee78a1b208382d3a26ec40d142ffc1ecc49ef" +[[package]] +name = "once_cell" +version = "1.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "af8b08b04175473088b46763e51ee54da5f9a164bc162f615b91bc179dbf15a3" + [[package]] name = "onig" version = "4.3.3" @@ -1991,6 +1997,7 @@ dependencies = [ "globset", "lazy_static", "number_prefix", + "once_cell", "term_grid", "termsize", "time", From eccb86c9edc1b36d6733de66c4e37d305090d26d Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Fri, 23 Apr 2021 08:26:20 +0200 Subject: [PATCH 5/8] ls: fix -a test --- src/uu/ls/src/ls.rs | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index f820ffffe..19097edda 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -492,7 +492,7 @@ impl Config { } } - if files != Files::Normal { + if files == Files::Normal { ignore_patterns.add(Glob::new(".*").unwrap()); } @@ -1185,25 +1185,21 @@ fn sort_entries(entries: &mut Vec, config: &Config) { fn is_hidden(file_path: &DirEntry) -> bool { let metadata = fs::metadata(file_path.path()).unwrap(); let attr = metadata.file_attributes(); - ((attr & 0x2) > 0) || file_path.file_name().to_string_lossy().starts_with('.') -} - -#[cfg(unix)] -fn is_hidden(file_path: &DirEntry) -> bool { - file_path.file_name().to_string_lossy().starts_with('.') + ((attr & 0x2) > 0) } fn should_display(entry: &DirEntry, config: &Config) -> bool { let ffi_name = entry.file_name(); - if config.files == Files::Normal && is_hidden(entry) { - return false; + // For unix, the hidden files are already included in the ignore pattern + #[cfg(windows)] + { + if config.files == Files::Normal && is_hidden(entry) { + return false; + } } - if config.ignore_patterns.is_match(&ffi_name) { - return false; - } - true + !config.ignore_patterns.is_match(&ffi_name) } fn enter_directory(dir: &PathData, config: &Config) { From 9f79db287bb6323de4b0eec7d670ffb6aa9da674 Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Sat, 24 Apr 2021 09:55:24 +0200 Subject: [PATCH 6/8] Update src/uu/ls/src/ls.rs Co-authored-by: Sylvestre Ledru --- src/uu/ls/src/ls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 19097edda..11d8b261f 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1185,7 +1185,7 @@ fn sort_entries(entries: &mut Vec, config: &Config) { fn is_hidden(file_path: &DirEntry) -> bool { let metadata = fs::metadata(file_path.path()).unwrap(); let attr = metadata.file_attributes(); - ((attr & 0x2) > 0) + (attr & 0x2) > 0 } fn should_display(entry: &DirEntry, config: &Config) -> bool { From 728f0bd61d792a6263375b809567d92f50e291ce Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Sat, 24 Apr 2021 10:47:36 +0200 Subject: [PATCH 7/8] ls: remove redundant parentheses --- src/uu/ls/src/ls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 1129d55c4..31e3de31c 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1161,7 +1161,7 @@ fn sort_entries(entries: &mut Vec, config: &Config) { fn is_hidden(file_path: &DirEntry) -> bool { let metadata = fs::metadata(file_path.path()).unwrap(); let attr = metadata.file_attributes(); - ((attr & 0x2) > 0) + (attr & 0x2) > 0 } fn should_display(entry: &DirEntry, config: &Config) -> bool { From 1328d1887884eabb45fce7612f08f0fac817ae46 Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Sat, 24 Apr 2021 13:19:50 +0200 Subject: [PATCH 8/8] ls: remove outdated comment --- 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 31e3de31c..2baf93193 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1606,7 +1606,6 @@ fn display_file_name(path: &PathData, strip: Option<&Path>, config: &Config) -> if config.format == Format::Long && path.file_type()?.is_symlink() { if let Ok(target) = path.p_buf.read_link() { - // We don't bother updating width here because it's not used for long listings name.push_str(" -> "); name.push_str(&target.to_string_lossy()); }