From c2ef996e4c67e3098237a62dc90ce5a7b4998645 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Thu, 18 Sep 2025 19:19:53 -0500 Subject: [PATCH 01/65] Initial commit --- src/uu/ls/src/ls.rs | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index ba7f26f1aad..a36ba03539d 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1828,26 +1828,21 @@ impl PathData { // Why prefer to check the DirEntry file_type()? B/c the call is // nearly free compared to a metadata() call on a Path - fn get_file_type( - de: &DirEntry, - p_buf: &Path, - must_dereference: bool, - ) -> OnceCell> { + fn get_file_type(de: &DirEntry, must_dereference: bool) -> Option { if must_dereference { - if let Ok(md_pb) = p_buf.metadata() { - return OnceCell::from(Some(md_pb.file_type())); - } + // wait for metadata call to populate file type + return None; } + if let Ok(ft_de) = de.file_type() { - OnceCell::from(Some(ft_de)) - } else if let Ok(md_pb) = p_buf.symlink_metadata() { - OnceCell::from(Some(md_pb.file_type())) - } else { - OnceCell::new() + return Some(ft_de); } + + None } + let ft = match de { - Some(ref de) => get_file_type(de, &p_buf, must_dereference), + Some(ref de) => OnceCell::from(get_file_type(de, must_dereference)), None => OnceCell::new(), }; From 4f16995d9b2d179ddea357c4927c5d732af41706 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Thu, 18 Sep 2025 21:20:30 -0500 Subject: [PATCH 02/65] Fix first test --- src/uu/ls/src/ls.rs | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index a36ba03539d..a3ac0cb7149 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1828,10 +1828,10 @@ impl PathData { // Why prefer to check the DirEntry file_type()? B/c the call is // nearly free compared to a metadata() call on a Path - fn get_file_type(de: &DirEntry, must_dereference: bool) -> Option { + fn get_file_type(de: &DirEntry, p_buf: &Path, must_dereference: bool) -> Option { if must_dereference { // wait for metadata call to populate file type - return None; + return p_buf.metadata().ok().map(|md| md.file_type()); } if let Ok(ft_de) = de.file_type() { @@ -1842,7 +1842,7 @@ impl PathData { } let ft = match de { - Some(ref de) => OnceCell::from(get_file_type(de, must_dereference)), + Some(ref de) => OnceCell::from(get_file_type(de, &p_buf, must_dereference)), None => OnceCell::new(), }; @@ -1899,9 +1899,19 @@ impl PathData { .as_ref() } - fn file_type(&self, out: &mut BufWriter) -> Option<&FileType> { + fn file_type(&self) -> Option<&FileType> { self.ft - .get_or_init(|| self.get_metadata(out).map(|md| md.file_type())) + .get_or_init(|| { + self.md + .get_or_init(|| { + match get_metadata_with_deref_opt(&self.p_buf, self.must_dereference) { + Ok(md) => Some(md), + Err(_) => self.de.as_ref().and_then(|de| de.metadata().ok()), + } + }) + .as_ref() + .map(|md| md.file_type()) + }) .as_ref() } } @@ -1985,7 +1995,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { continue; } - let show_dir_contents = match path_data.file_type(&mut state.out) { + let show_dir_contents = match path_data.file_type() { Some(ft) => !config.directory && ft.is_dir(), None => { set_exit_code(1); @@ -2244,10 +2254,7 @@ fn enter_directory( for e in entries .iter() .skip(if config.files == Files::All { 2 } else { 0 }) - .filter(|p| { - p.ft.get() - .is_some_and(|o_ft| o_ft.is_some_and(|ft| ft.is_dir())) - }) + .filter(|p| p.file_type().is_some_and(|ft| ft.is_dir())) { match fs::read_dir(&e.p_buf) { Err(err) => { @@ -2801,7 +2808,7 @@ fn display_item_long( } else { #[cfg(unix)] let leading_char = { - if let Some(Some(ft)) = item.ft.get() { + if let Some(ft) = item.file_type() { if ft.is_char_device() { "c" } else if ft.is_block_device() { @@ -2819,8 +2826,8 @@ fn display_item_long( }; #[cfg(not(unix))] let leading_char = { - if let Some(Some(ft)) = item.ft.get() { - if ft.is_symlink() { + if let Some(ft) = item.file_type() { + if item.is_symlink() { "l" } else if ft.is_dir() { "d" @@ -3013,7 +3020,7 @@ fn file_is_executable(md: &Metadata) -> bool { } fn classify_file(path: &PathData, out: &mut BufWriter) -> Option { - let file_type = path.file_type(out)?; + let file_type = path.file_type()?; if file_type.is_dir() { Some('/') @@ -3120,8 +3127,8 @@ fn display_item_name( } if config.format == Format::Long - && path.file_type(&mut state.out).is_some() - && path.file_type(&mut state.out).unwrap().is_symlink() + && path.file_type().is_some() + && path.file_type().unwrap().is_symlink() && !path.must_dereference { match path.p_buf.read_link() { @@ -3154,7 +3161,7 @@ fn display_item_name( ) .is_err() { - name.push(path.p_buf.read_link().unwrap()); + name.push(target); } else { name.push(color_name( locale_aware_escape_name(target.as_os_str(), config.quoting_style), From d8dc0cb33f9bedcfbf334f532cb160d3336778a5 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Thu, 18 Sep 2025 21:55:58 -0500 Subject: [PATCH 03/65] Fix 2nd test for broken links --- src/uu/ls/src/ls.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index a3ac0cb7149..9af9164953e 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1914,6 +1914,10 @@ impl PathData { }) .as_ref() } + + fn is_broken_link(&self) -> bool { + self.de.is_some() && get_metadata_with_deref_opt(&self.p_buf, false).is_ok() + } } /// Show the directory name in the case where several arguments are given to ls @@ -2821,7 +2825,7 @@ fn display_item_long( "-" } } else { - "-" + if item.is_broken_link() { "l" } else { "-" } } }; #[cfg(not(unix))] @@ -2835,7 +2839,7 @@ fn display_item_long( "-" } } else { - "-" + if item.is_broken_link() { "l" } else { "-" } } }; From 3918da74b26ff1c73d713b8ac9fcfdde814fed23 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Thu, 18 Sep 2025 22:34:33 -0500 Subject: [PATCH 04/65] Cleanup --- src/uu/ls/src/ls.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 9af9164953e..ef4dfe71de6 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1915,8 +1915,12 @@ impl PathData { .as_ref() } - fn is_broken_link(&self) -> bool { - self.de.is_some() && get_metadata_with_deref_opt(&self.p_buf, false).is_ok() + fn is_dangling_link(&self) -> bool { + // deref enabled, self is real dir entry, self has metadata associated with link, but not with target + self.must_dereference + && self.de.is_some() + && self.file_type().is_none() + && get_metadata_with_deref_opt(&self.p_buf, false).is_ok() } } @@ -2825,7 +2829,7 @@ fn display_item_long( "-" } } else { - if item.is_broken_link() { "l" } else { "-" } + if item.is_dangling_link() { "l" } else { "-" } } }; #[cfg(not(unix))] @@ -2839,7 +2843,7 @@ fn display_item_long( "-" } } else { - if item.is_broken_link() { "l" } else { "-" } + if item.is_dangling_link() { "l" } else { "-" } } }; From 61270177b1d0c3dce3ac3dd6069e8a1e548b1bb9 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Thu, 18 Sep 2025 23:25:20 -0500 Subject: [PATCH 05/65] Fix lints --- src/uu/ls/src/ls.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index ef4dfe71de6..98a93bdcf59 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1904,10 +1904,9 @@ impl PathData { .get_or_init(|| { self.md .get_or_init(|| { - match get_metadata_with_deref_opt(&self.p_buf, self.must_dereference) { - Ok(md) => Some(md), - Err(_) => self.de.as_ref().and_then(|de| de.metadata().ok()), - } + get_metadata_with_deref_opt(&self.p_buf, self.must_dereference) + .ok() + .or_else(|| self.de.as_ref().and_then(|de| de.metadata().ok())) }) .as_ref() .map(|md| md.file_type()) @@ -2828,8 +2827,10 @@ fn display_item_long( } else { "-" } + } else if item.is_dangling_link() { + "l" } else { - if item.is_dangling_link() { "l" } else { "-" } + "-" } }; #[cfg(not(unix))] @@ -2842,8 +2843,10 @@ fn display_item_long( } else { "-" } + } else if item.is_dangling_link() { + "l" } else { - if item.is_dangling_link() { "l" } else { "-" } + "-" } }; From ba59811c833612124ff31f854d324d3c3098d353 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Thu, 18 Sep 2025 23:35:17 -0500 Subject: [PATCH 06/65] Cleanup --- 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 98a93bdcf59..216ac040820 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2836,7 +2836,7 @@ fn display_item_long( #[cfg(not(unix))] let leading_char = { if let Some(ft) = item.file_type() { - if item.is_symlink() { + if ft.is_symlink() { "l" } else if ft.is_dir() { "d" From 23553696f45bf4cef02b222498f40ec85d95e1f7 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Fri, 19 Sep 2025 00:35:39 -0500 Subject: [PATCH 07/65] Remove the need to pass around out bufwriter --- src/uu/ls/src/colors.rs | 4 +-- src/uu/ls/src/ls.rs | 68 ++++++++++++++++++----------------------- 2 files changed, 30 insertions(+), 42 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 4affbbf8ca2..8a22a8d105d 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -7,7 +7,6 @@ use super::get_metadata_with_deref_opt; use lscolors::{Indicator, LsColors, Style}; use std::ffi::OsString; use std::fs::{DirEntry, Metadata}; -use std::io::{BufWriter, Stdout}; /// We need this struct to be able to store the previous style. /// This because we need to check the previous value in case we don't need @@ -153,7 +152,6 @@ pub(crate) fn color_name( name: OsString, path: &PathData, style_manager: &mut StyleManager, - out: &mut BufWriter, target_symlink: Option<&PathData>, wrap: bool, ) -> OsString { @@ -193,7 +191,7 @@ pub(crate) fn color_name( let md = md_res.or_else(|_| path.p_buf.symlink_metadata()); style_manager.apply_style_based_on_metadata(path, md.ok().as_ref(), name, wrap) } else { - let md_option = path.get_metadata(out); + let md_option = path.get_metadata(); let symlink_metadata = path.p_buf.symlink_metadata().ok(); let md = md_option.or(symlink_metadata.as_ref()); style_manager.apply_style_based_on_metadata(path, md, name, wrap) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 216ac040820..c6fa84b1fdc 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1860,7 +1860,7 @@ impl PathData { } } - fn get_metadata(&self, out: &mut BufWriter) -> Option<&Metadata> { + fn get_metadata(&self) -> Option<&Metadata> { self.md .get_or_init(|| { // check if we can use DirEntry metadata @@ -1875,6 +1875,7 @@ impl PathData { match get_metadata_with_deref_opt(self.p_buf.as_path(), self.must_dereference) { Err(err) => { // FIXME: A bit tricky to propagate the result here + let mut out = stdout(); out.flush().unwrap(); let errno = err.raw_os_error().unwrap_or(1i32); // a bad fd will throw an error when dereferenced, @@ -1914,6 +1915,11 @@ impl PathData { .as_ref() } + fn is_executable_file(&self) -> bool { + self.file_type().is_some_and(|f| f.is_file()) + && self.get_metadata().is_some_and(|md| file_is_executable(md)) + } + fn is_dangling_link(&self) -> bool { // deref enabled, self is real dir entry, self has metadata associated with link, but not with target self.must_dereference @@ -1998,7 +2004,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { // Proper GNU handling is don't show if dereferenced symlink DNE // but only for the base dir, for a child dir show, and print ?s // in long format - if path_data.get_metadata(&mut state.out).is_none() { + if path_data.get_metadata().is_none() { continue; } @@ -2017,8 +2023,8 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { } } - sort_entries(&mut files, config, &mut state.out); - sort_entries(&mut dirs, config, &mut state.out); + sort_entries(&mut files, config); + sort_entries(&mut dirs, config); if let Some(style_manager) = state.style_manager.as_mut() { // ls will try to write a reset before anything is written if normal @@ -2090,17 +2096,17 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { Ok(()) } -fn sort_entries(entries: &mut [PathData], config: &Config, out: &mut BufWriter) { +fn sort_entries(entries: &mut [PathData], config: &Config) { match config.sort { Sort::Time => entries.sort_by_key(|k| { Reverse( - k.get_metadata(out) + k.get_metadata() .and_then(|md| metadata_get_time(md, config.time)) .unwrap_or(UNIX_EPOCH), ) }), Sort::Size => { - entries.sort_by_key(|k| Reverse(k.get_metadata(out).map_or(0, |md| md.len()))); + entries.sort_by_key(|k| Reverse(k.get_metadata().map_or(0, |md| md.len()))); } // The default sort in GNU ls is case insensitive Sort::Name => entries.sort_by(|a, b| a.display_name.cmp(&b.display_name)), @@ -2244,7 +2250,7 @@ fn enter_directory( } } - sort_entries(&mut entries, config, &mut state.out); + sort_entries(&mut entries, config); // Print total after any error display if config.format == Format::Long || config.alloc_size { @@ -2322,7 +2328,7 @@ fn display_dir_entry_size( state: &mut ListState, ) -> (usize, usize, usize, usize, usize, usize) { // TODO: Cache/memorize the display_* results so we don't have to recalculate them. - if let Some(md) = entry.get_metadata(&mut state.out) { + if let Some(md) = entry.get_metadata() { let (size_len, major_len, minor_len) = match display_len_or_rdev(md, config) { SizeOrDeviceId::Device(major, minor) => { (major.len() + minor.len() + 2usize, major.len(), minor.len()) @@ -2379,7 +2385,7 @@ fn return_total( let mut total_size = 0; for item in items { total_size += item - .get_metadata(out) + .get_metadata() .as_ref() .map_or(0, |md| get_block_size(md, config)); } @@ -2397,13 +2403,12 @@ fn display_additional_leading_info( item: &PathData, padding: &PaddingCollection, config: &Config, - out: &mut BufWriter, ) -> UResult { let mut result = String::new(); #[cfg(unix)] { if config.inode { - let i = if let Some(md) = item.get_metadata(out) { + let i = if let Some(md) = item.get_metadata() { get_inode(md) } else { "?".to_owned() @@ -2413,7 +2418,7 @@ fn display_additional_leading_info( } if config.alloc_size { - let s = if let Some(md) = item.get_metadata(out) { + let s = if let Some(md) = item.get_metadata() { display_size(get_block_size(md, config), config) } else { "?".to_owned() @@ -2454,12 +2459,7 @@ fn display_items( let should_display_leading_info = config.alloc_size; if should_display_leading_info { - let more_info = display_additional_leading_info( - item, - &padding_collection, - config, - &mut state.out, - )?; + let more_info = display_additional_leading_info(item, &padding_collection, config)?; write!(state.out, "{more_info}")?; } @@ -2487,7 +2487,7 @@ fn display_items( let mut names_vec = Vec::new(); for i in items { - let more_info = display_additional_leading_info(i, &padding, config, &mut state.out)?; + let more_info = display_additional_leading_info(i, &padding, config)?; // it's okay to set current column to zero which is used to decide // whether text will wrap or not, because when format is grid or // column ls will try to place the item name in a new line if it @@ -2711,7 +2711,7 @@ fn display_item_long( if config.dired { output_display.extend(b" "); } - if let Some(md) = item.get_metadata(&mut state.out) { + if let Some(md) = item.get_metadata() { #[cfg(any(not(unix), target_os = "android", target_os = "macos"))] // TODO: See how Mac should work here let is_acl_set = false; @@ -3030,7 +3030,7 @@ fn file_is_executable(md: &Metadata) -> bool { return md.mode() & ((S_IXUSR | S_IXGRP | S_IXOTH) as u32) != 0; } -fn classify_file(path: &PathData, out: &mut BufWriter) -> Option { +fn classify_file(path: &PathData) -> Option { let file_type = path.file_type()?; if file_type.is_dir() { @@ -3044,11 +3044,9 @@ fn classify_file(path: &PathData, out: &mut BufWriter) -> Option { Some('=') } else if file_type.is_fifo() { Some('|') - } else if file_type.is_file() // Safe unwrapping if the file was removed between listing and display // See https://github.com/uutils/coreutils/issues/5371 - && path.get_metadata(out).is_some_and(file_is_executable) - { + } else if path.is_executable_file() { Some('*') } else { None @@ -3094,14 +3092,7 @@ fn display_item_name( if let Some(style_manager) = &mut state.style_manager { let len = name.len(); - name = color_name( - name, - path, - style_manager, - &mut state.out, - None, - is_wrap(len), - ); + name = color_name(name, path, style_manager, None, is_wrap(len)); } if config.format != Format::Long && !more_info.is_empty() { @@ -3111,7 +3102,7 @@ fn display_item_name( } if config.indicator_style != IndicatorStyle::None { - let sym = classify_file(path, &mut state.out); + let sym = classify_file(path); let char_opt = match config.indicator_style { IndicatorStyle::Classify => sym, @@ -3165,7 +3156,7 @@ fn display_item_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. - if path.get_metadata(&mut state.out).is_none() + if path.get_metadata().is_none() && get_metadata_with_deref_opt( target_data.p_buf.as_path(), target_data.must_dereference, @@ -3178,7 +3169,6 @@ fn display_item_name( locale_aware_escape_name(target.as_os_str(), config.quoting_style), path, style_manager, - &mut state.out, Some(&target_data), is_wrap(name.len()), )); @@ -3340,7 +3330,7 @@ fn calculate_padding_collection( for item in items { #[cfg(unix)] if config.inode { - let inode_len = if let Some(md) = item.get_metadata(&mut state.out) { + let inode_len = if let Some(md) = item.get_metadata() { display_inode(md).len() } else { continue; @@ -3349,7 +3339,7 @@ fn calculate_padding_collection( } if config.alloc_size { - if let Some(md) = item.get_metadata(&mut state.out) { + if let Some(md) = item.get_metadata() { let block_size_len = display_size(get_block_size(md, config), config).len(); padding_collections.block_size = block_size_len.max(padding_collections.block_size); } @@ -3399,7 +3389,7 @@ fn calculate_padding_collection( for item in items { if config.alloc_size { - if let Some(md) = item.get_metadata(&mut state.out) { + if let Some(md) = item.get_metadata() { let block_size_len = display_size(get_block_size(md, config), config).len(); padding_collections.block_size = block_size_len.max(padding_collections.block_size); } From 3c42725432a3f71f1c04687c04e6e94ee664a73e Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Fri, 19 Sep 2025 00:48:32 -0500 Subject: [PATCH 08/65] Cleanup --- src/uu/ls/src/ls.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index c6fa84b1fdc..4e114173596 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1875,8 +1875,8 @@ impl PathData { match get_metadata_with_deref_opt(self.p_buf.as_path(), self.must_dereference) { Err(err) => { // FIXME: A bit tricky to propagate the result here - let mut out = stdout(); - out.flush().unwrap(); + let mut out = stdout().lock(); + let _ = out.flush(); let errno = err.raw_os_error().unwrap_or(1i32); // a bad fd will throw an error when dereferenced, // but GNU will not throw an error until a bad fd "dir" @@ -1915,6 +1915,7 @@ impl PathData { .as_ref() } + #[cfg(unix)] fn is_executable_file(&self) -> bool { self.file_type().is_some_and(|f| f.is_file()) && self.get_metadata().is_some_and(|md| file_is_executable(md)) From f8fc4a9a378bca3ab61f700935f3413734de628e Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Fri, 19 Sep 2025 01:18:18 -0500 Subject: [PATCH 09/65] Cleanup --- src/uu/ls/src/ls.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 4e114173596..d5f370df942 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1787,7 +1787,7 @@ struct PathData { impl PathData { fn new( p_buf: PathBuf, - dir_entry: Option>, + dir_entry: Option, file_name: Option, config: &Config, command_line: bool, @@ -1804,6 +1804,7 @@ impl PathData { .unwrap_or_else(|| p_buf.iter().next_back().unwrap()) .to_owned() }; + let must_dereference = match &config.dereference { Dereference::All => true, Dereference::Args => command_line, @@ -1821,10 +1822,7 @@ impl PathData { Dereference::None => false, }; - let de: Option = match dir_entry { - Some(de) => de.ok(), - None => None, - }; + let de: Option = dir_entry; // Why prefer to check the DirEntry file_type()? B/c the call is // nearly free compared to a metadata() call on a Path @@ -2246,7 +2244,7 @@ fn enter_directory( if should_display(&dir_entry, config) { let entry_path_data = - PathData::new(dir_entry.path(), Some(Ok(dir_entry)), None, config, false); + PathData::new(dir_entry.path(), Some(dir_entry), None, config, false); entries.push(entry_path_data); } } From 18ab2e0c4dc4e6594ea122bbb0065b99f493e05f Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Fri, 19 Sep 2025 01:29:25 -0500 Subject: [PATCH 10/65] Cleanup --- src/uu/ls/src/colors.rs | 2 +- src/uu/ls/src/ls.rs | 49 +++++++++++++++++------------------------ 2 files changed, 21 insertions(+), 30 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 8a22a8d105d..9219fd04619 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -191,7 +191,7 @@ pub(crate) fn color_name( let md = md_res.or_else(|_| path.p_buf.symlink_metadata()); style_manager.apply_style_based_on_metadata(path, md.ok().as_ref(), name, wrap) } else { - let md_option = path.get_metadata(); + let md_option = path.metadata(); let symlink_metadata = path.p_buf.symlink_metadata().ok(); let md = md_option.or(symlink_metadata.as_ref()); style_manager.apply_style_based_on_metadata(path, md, name, wrap) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index d5f370df942..967ad870dfe 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1858,7 +1858,7 @@ impl PathData { } } - fn get_metadata(&self) -> Option<&Metadata> { + fn metadata(&self) -> Option<&Metadata> { self.md .get_or_init(|| { // check if we can use DirEntry metadata @@ -1900,31 +1900,22 @@ impl PathData { fn file_type(&self) -> Option<&FileType> { self.ft - .get_or_init(|| { - self.md - .get_or_init(|| { - get_metadata_with_deref_opt(&self.p_buf, self.must_dereference) - .ok() - .or_else(|| self.de.as_ref().and_then(|de| de.metadata().ok())) - }) - .as_ref() - .map(|md| md.file_type()) - }) + .get_or_init(|| self.metadata().map(|md| md.file_type())) .as_ref() } - #[cfg(unix)] - fn is_executable_file(&self) -> bool { - self.file_type().is_some_and(|f| f.is_file()) - && self.get_metadata().is_some_and(|md| file_is_executable(md)) - } - fn is_dangling_link(&self) -> bool { // deref enabled, self is real dir entry, self has metadata associated with link, but not with target self.must_dereference && self.de.is_some() && self.file_type().is_none() - && get_metadata_with_deref_opt(&self.p_buf, false).is_ok() + && self.metadata().is_none() + } + + #[cfg(unix)] + fn is_executable_file(&self) -> bool { + self.file_type().is_some_and(|f| f.is_file()) + && self.metadata().is_some_and(|md| file_is_executable(md)) } } @@ -2003,7 +1994,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { // Proper GNU handling is don't show if dereferenced symlink DNE // but only for the base dir, for a child dir show, and print ?s // in long format - if path_data.get_metadata().is_none() { + if path_data.metadata().is_none() { continue; } @@ -2099,13 +2090,13 @@ fn sort_entries(entries: &mut [PathData], config: &Config) { match config.sort { Sort::Time => entries.sort_by_key(|k| { Reverse( - k.get_metadata() + k.metadata() .and_then(|md| metadata_get_time(md, config.time)) .unwrap_or(UNIX_EPOCH), ) }), Sort::Size => { - entries.sort_by_key(|k| Reverse(k.get_metadata().map_or(0, |md| md.len()))); + entries.sort_by_key(|k| Reverse(k.metadata().map_or(0, |md| md.len()))); } // The default sort in GNU ls is case insensitive Sort::Name => entries.sort_by(|a, b| a.display_name.cmp(&b.display_name)), @@ -2327,7 +2318,7 @@ fn display_dir_entry_size( state: &mut ListState, ) -> (usize, usize, usize, usize, usize, usize) { // TODO: Cache/memorize the display_* results so we don't have to recalculate them. - if let Some(md) = entry.get_metadata() { + if let Some(md) = entry.metadata() { let (size_len, major_len, minor_len) = match display_len_or_rdev(md, config) { SizeOrDeviceId::Device(major, minor) => { (major.len() + minor.len() + 2usize, major.len(), minor.len()) @@ -2384,7 +2375,7 @@ fn return_total( let mut total_size = 0; for item in items { total_size += item - .get_metadata() + .metadata() .as_ref() .map_or(0, |md| get_block_size(md, config)); } @@ -2407,7 +2398,7 @@ fn display_additional_leading_info( #[cfg(unix)] { if config.inode { - let i = if let Some(md) = item.get_metadata() { + let i = if let Some(md) = item.metadata() { get_inode(md) } else { "?".to_owned() @@ -2417,7 +2408,7 @@ fn display_additional_leading_info( } if config.alloc_size { - let s = if let Some(md) = item.get_metadata() { + let s = if let Some(md) = item.metadata() { display_size(get_block_size(md, config), config) } else { "?".to_owned() @@ -2710,7 +2701,7 @@ fn display_item_long( if config.dired { output_display.extend(b" "); } - if let Some(md) = item.get_metadata() { + if let Some(md) = item.metadata() { #[cfg(any(not(unix), target_os = "android", target_os = "macos"))] // TODO: See how Mac should work here let is_acl_set = false; @@ -3155,7 +3146,7 @@ fn display_item_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. - if path.get_metadata().is_none() + if path.metadata().is_none() && get_metadata_with_deref_opt( target_data.p_buf.as_path(), target_data.must_dereference, @@ -3329,7 +3320,7 @@ fn calculate_padding_collection( for item in items { #[cfg(unix)] if config.inode { - let inode_len = if let Some(md) = item.get_metadata() { + let inode_len = if let Some(md) = item.metadata() { display_inode(md).len() } else { continue; @@ -3338,7 +3329,7 @@ fn calculate_padding_collection( } if config.alloc_size { - if let Some(md) = item.get_metadata() { + if let Some(md) = item.metadata() { let block_size_len = display_size(get_block_size(md, config), config).len(); padding_collections.block_size = block_size_len.max(padding_collections.block_size); } From 0914fca6613c4d929d5cec5ff77aae4381a821e2 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Fri, 19 Sep 2025 02:24:57 -0500 Subject: [PATCH 11/65] Do not re request metadata for coloring --- src/uu/ls/src/colors.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 9219fd04619..24acb80115d 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -187,13 +187,17 @@ pub(crate) fn color_name( // use the optional target_symlink // Use fn get_metadata_with_deref_opt instead of get_metadata() here because ls // should not exit with an err, if we are unable to obtain the target_metadata - let md_res = get_metadata_with_deref_opt(&target.p_buf, path.must_dereference); - let md = md_res.or_else(|_| path.p_buf.symlink_metadata()); - style_manager.apply_style_based_on_metadata(path, md.ok().as_ref(), name, wrap) + let md_option = get_metadata_with_deref_opt(&target.p_buf, path.must_dereference) + .ok() + .or_else(|| path.p_buf.symlink_metadata().ok()); + + style_manager.apply_style_based_on_metadata(path, md_option.as_ref(), name, wrap) } else { - let md_option = path.metadata(); - let symlink_metadata = path.p_buf.symlink_metadata().ok(); - let md = md_option.or(symlink_metadata.as_ref()); - style_manager.apply_style_based_on_metadata(path, md, name, wrap) + let md_option: Option = path + .metadata() + .cloned() + .or_else(|| path.p_buf.symlink_metadata().ok()); + + style_manager.apply_style_based_on_metadata(path, md_option.as_ref(), name, wrap) } } From 106dfd2380c191a9940785ef1f6a4f7c581ece53 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Fri, 19 Sep 2025 02:29:04 -0500 Subject: [PATCH 12/65] Using DirEntry forces more metadata syscalls --- src/uu/ls/src/colors.rs | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 24acb80115d..182dc35d87e 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -133,16 +133,6 @@ impl<'a> StyleManager<'a> { .style_for_path_with_metadata(&path.p_buf, md_option); self.apply_style(style, name, wrap) } - - pub(crate) fn apply_style_based_on_dir_entry( - &mut self, - dir_entry: &DirEntry, - name: OsString, - wrap: bool, - ) -> OsString { - let style = self.colors.style_for(dir_entry); - self.apply_style(style, name, wrap) - } } /// Colors the provided name based on the style determined for the given path @@ -175,14 +165,6 @@ pub(crate) fn color_name( } } - if !path.must_dereference { - // If we need to dereference (follow) a symlink, we will need to get the metadata - if let Some(de) = &path.de { - // There is a DirEntry, we don't need to get the metadata for the color - return style_manager.apply_style_based_on_dir_entry(de, name, wrap); - } - } - if let Some(target) = target_symlink { // use the optional target_symlink // Use fn get_metadata_with_deref_opt instead of get_metadata() here because ls From 70cf0b15dde68a7c27f23051d6c67270ee7c16ad Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Fri, 19 Sep 2025 02:29:15 -0500 Subject: [PATCH 13/65] Cleanup --- src/uu/ls/src/colors.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 182dc35d87e..1369f2b31dc 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -6,7 +6,7 @@ use super::PathData; use super::get_metadata_with_deref_opt; use lscolors::{Indicator, LsColors, Style}; use std::ffi::OsString; -use std::fs::{DirEntry, Metadata}; +use std::fs::Metadata; /// We need this struct to be able to store the previous style. /// This because we need to check the previous value in case we don't need From a97caddd99de572ec4562c183d58036a1c4b4701 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Fri, 19 Sep 2025 12:48:53 -0500 Subject: [PATCH 14/65] Cleanup lints --- src/uu/ls/src/colors.rs | 2 -- src/uu/ls/src/ls.rs | 7 +++---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 1369f2b31dc..b75c7f41854 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -136,8 +136,6 @@ impl<'a> StyleManager<'a> { } /// Colors the provided name based on the style determined for the given path -/// This function is quite long because it tries to leverage [`DirEntry`] to avoid -/// unnecessary calls to stat and manages the symlink errors pub(crate) fn color_name( name: OsString, path: &PathData, diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 967ad870dfe..a49afbbb62e 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1915,7 +1915,7 @@ impl PathData { #[cfg(unix)] fn is_executable_file(&self) -> bool { self.file_type().is_some_and(|f| f.is_file()) - && self.metadata().is_some_and(|md| file_is_executable(md)) + && self.metadata().is_some_and(file_is_executable) } } @@ -3119,8 +3119,7 @@ fn display_item_name( } if config.format == Format::Long - && path.file_type().is_some() - && path.file_type().unwrap().is_symlink() + && path.file_type().is_some_and(|ft| ft.is_symlink()) && !path.must_dereference { match path.p_buf.read_link() { @@ -3379,7 +3378,7 @@ fn calculate_padding_collection( for item in items { if config.alloc_size { - if let Some(md) = item.get_metadata() { + if let Some(md) = item.metadata() { let block_size_len = display_size(get_block_size(md, config), config).len(); padding_collections.block_size = block_size_len.max(padding_collections.block_size); } From 8bd40c8f982437dfd3bf9128c8639a03d9da3db6 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Sun, 21 Sep 2025 13:57:08 -0500 Subject: [PATCH 15/65] Lazily obtain security context --- src/uu/ls/src/ls.rs | 55 +++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index a49afbbb62e..98a83a20ad7 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1780,7 +1780,7 @@ struct PathData { // PathBuf that all above data corresponds to p_buf: PathBuf, must_dereference: bool, - security_context: String, + security_context: OnceCell, command_line: bool, } @@ -1844,8 +1844,6 @@ impl PathData { None => OnceCell::new(), }; - let security_context = get_security_context(config, &p_buf, must_dereference); - Self { md: OnceCell::new(), ft, @@ -1853,7 +1851,7 @@ impl PathData { display_name, p_buf, must_dereference, - security_context, + security_context: OnceCell::new(), command_line, } } @@ -1917,6 +1915,12 @@ impl PathData { self.file_type().is_some_and(|f| f.is_file()) && self.metadata().is_some_and(file_is_executable) } + + fn security_context(&self, config: &Config) -> &String { + self.security_context.get_or_init(|| { + get_security_context(config, &self.p_buf, self.metadata(), self.must_dereference) + }) + } } /// Show the directory name in the case where several arguments are given to ls @@ -2460,7 +2464,7 @@ fn display_items( let mut longest_context_len = 1; let prefix_context = if config.context { for item in items { - let context_len = item.security_context.len(); + let context_len = item.security_context(config).len(); longest_context_len = context_len.max(longest_context_len); } Some(longest_context_len) @@ -2708,7 +2712,7 @@ fn display_item_long( #[cfg(all(unix, not(any(target_os = "android", target_os = "macos"))))] let is_acl_set = has_acl(item.display_name.as_os_str()); output_display.extend(display_permissions(md, true).as_bytes()); - if item.security_context.len() > 1 { + if item.security_context(config).len() > 1 { // GNU `ls` uses a "." character to indicate a file with a security context, // but not other alternate access method. output_display.extend(b"."); @@ -2730,7 +2734,7 @@ fn display_item_long( if config.context { output_display.extend(b" "); - output_display.extend_pad_right(&item.security_context, padding.context); + output_display.extend_pad_right(&item.security_context(config), padding.context); } // Author is only different from owner on GNU/Hurd, so we reuse @@ -2842,7 +2846,7 @@ fn display_item_long( output_display.extend(leading_char.as_bytes()); output_display.extend(b"?????????"); - if item.security_context.len() > 1 { + if item.security_context(config).len() > 1 { // GNU `ls` uses a "." character to indicate a file with a security context, // but not other alternate access method. output_display.extend(b"."); @@ -2862,7 +2866,7 @@ fn display_item_long( if config.context { output_display.extend(b" "); - output_display.extend_pad_right(&item.security_context, padding.context); + output_display.extend_pad_right(item.security_context(config), padding.context); } // Author is only different from owner on GNU/Hurd, so we reuse @@ -3182,9 +3186,9 @@ fn display_item_name( if config.context { if let Some(pad_count) = prefix_context { let security_context = if matches!(config.format, Format::Commas) { - path.security_context.clone() + path.security_context(config).to_owned() } else { - pad_left(&path.security_context, pad_count) + pad_left(path.security_context(config), pad_count).to_owned() }; let old_name = name; name = format!("{security_context} ").into(); @@ -3245,23 +3249,30 @@ fn display_inode(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 -fn get_security_context(config: &Config, p_buf: &Path, must_dereference: bool) -> String { +fn get_security_context( + config: &Config, + p_buf: &Path, + opt_metadata: Option<&Metadata>, + must_dereference: bool, +) -> String { let substitute_string = "?".to_string(); // If we must dereference, ensure that the symlink is actually valid even if the system // does not support SELinux. // Conforms to the GNU coreutils where a dangling symlink results in exit code 1. if must_dereference { - match get_metadata_with_deref_opt(p_buf, must_dereference) { - Err(err) => { - // The Path couldn't be dereferenced, so return early and set exit code 1 - // to indicate a minor error - // Only show error when context display is requested to avoid duplicate messages - if config.context { - show!(LsError::IOErrorContext(p_buf.to_path_buf(), err, false)); + if opt_metadata.is_none() { + match get_metadata_with_deref_opt(p_buf, must_dereference) { + Err(err) => { + // The Path couldn't be dereferenced, so return early and set exit code 1 + // to indicate a minor error + // Only show error when context display is requested to avoid duplicate messages + if config.context { + show!(LsError::IOErrorContext(p_buf.to_path_buf(), err, false)); + } + return substitute_string; } - return substitute_string; + Ok(_md) => (), } - Ok(_md) => (), } } if config.selinux_supported { @@ -3335,7 +3346,7 @@ fn calculate_padding_collection( } if config.format == Format::Long { - let context_len = item.security_context.len(); + let context_len = item.security_context(config).len(); let (link_count_len, uname_len, group_len, size_len, major_len, minor_len) = display_dir_entry_size(item, config, state); padding_collections.link_count = link_count_len.max(padding_collections.link_count); From 7a95cb304a9f55f93239c5801bbb0bf56769f364 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Sun, 21 Sep 2025 14:00:42 -0500 Subject: [PATCH 16/65] Cleanup --- src/uu/ls/src/ls.rs | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 98a83a20ad7..5a2e9710c87 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3259,20 +3259,15 @@ fn get_security_context( // If we must dereference, ensure that the symlink is actually valid even if the system // does not support SELinux. // Conforms to the GNU coreutils where a dangling symlink results in exit code 1. - if must_dereference { - if opt_metadata.is_none() { - match get_metadata_with_deref_opt(p_buf, must_dereference) { - Err(err) => { - // The Path couldn't be dereferenced, so return early and set exit code 1 - // to indicate a minor error - // Only show error when context display is requested to avoid duplicate messages - if config.context { - show!(LsError::IOErrorContext(p_buf.to_path_buf(), err, false)); - } - return substitute_string; - } - Ok(_md) => (), + if must_dereference && opt_metadata.is_none() { + if let Err(err) = get_metadata_with_deref_opt(p_buf, must_dereference) { + // The Path couldn't be dereferenced, so return early and set exit code 1 + // to indicate a minor error + // Only show error when context display is requested to avoid duplicate messages + if config.context { + show!(LsError::IOErrorContext(p_buf.to_path_buf(), err, false)); } + return substitute_string; } } if config.selinux_supported { From cd15f48cda9dfcb87d04e415681a23e007373a33 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Sun, 21 Sep 2025 14:03:42 -0500 Subject: [PATCH 17/65] Cleanup --- src/uu/ls/src/ls.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 5a2e9710c87..caca692f20f 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1772,6 +1772,7 @@ struct PathData { // Result got from symlink_metadata() or metadata() based on config md: OnceCell>, ft: OnceCell>, + security_context: OnceCell, // can be used to avoid reading the metadata. Can be also called d_type: // https://www.gnu.org/software/libc/manual/html_node/Directory-Entries.html de: Option, @@ -1780,7 +1781,6 @@ struct PathData { // PathBuf that all above data corresponds to p_buf: PathBuf, must_dereference: bool, - security_context: OnceCell, command_line: bool, } @@ -1847,11 +1847,11 @@ impl PathData { Self { md: OnceCell::new(), ft, + security_context: OnceCell::new(), de, display_name, p_buf, must_dereference, - security_context: OnceCell::new(), command_line, } } From c42295afa6a60f2c5c22d91b764e67f639380a96 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Sun, 21 Sep 2025 15:09:55 -0500 Subject: [PATCH 18/65] Fix lints --- src/uu/ls/src/ls.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index caca692f20f..fe85801c7f6 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2734,7 +2734,7 @@ fn display_item_long( if config.context { output_display.extend(b" "); - output_display.extend_pad_right(&item.security_context(config), padding.context); + output_display.extend_pad_right(item.security_context(config), padding.context); } // Author is only different from owner on GNU/Hurd, so we reuse @@ -3186,9 +3186,9 @@ fn display_item_name( if config.context { if let Some(pad_count) = prefix_context { let security_context = if matches!(config.format, Format::Commas) { - path.security_context(config).to_owned() + path.security_context(config).clone() } else { - pad_left(path.security_context(config), pad_count).to_owned() + pad_left(path.security_context(config), pad_count).clone() }; let old_name = name; name = format!("{security_context} ").into(); @@ -3390,7 +3390,7 @@ fn calculate_padding_collection( } } - let context_len = item.security_context.len(); + let context_len = item.security_context().len(); let (link_count_len, uname_len, group_len, size_len, _major_len, _minor_len) = display_dir_entry_size(item, config, state); padding_collections.link_count = link_count_len.max(padding_collections.link_count); From 758022aa7bf28ec0f5dfb7bce892975639a7cf09 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Sun, 21 Sep 2025 21:51:34 -0500 Subject: [PATCH 19/65] Fix lints --- 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 fe85801c7f6..dc7fe856f71 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3390,7 +3390,7 @@ fn calculate_padding_collection( } } - let context_len = item.security_context().len(); + let context_len = item.security_context(config).len(); let (link_count_len, uname_len, group_len, size_len, _major_len, _minor_len) = display_dir_entry_size(item, config, state); padding_collections.link_count = link_count_len.max(padding_collections.link_count); From 34cbcb3f380a760390a0476019d691c733ec07cb Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 00:49:20 -0500 Subject: [PATCH 20/65] Perhaps fix GNU test --- src/uu/ls/src/colors.rs | 6 +++--- src/uu/ls/src/ls.rs | 38 ++++++++++++++++++-------------------- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index b75c7f41854..de3328c3f79 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -3,7 +3,6 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. use super::PathData; -use super::get_metadata_with_deref_opt; use lscolors::{Indicator, LsColors, Style}; use std::ffi::OsString; use std::fs::Metadata; @@ -167,8 +166,9 @@ pub(crate) fn color_name( // use the optional target_symlink // Use fn get_metadata_with_deref_opt instead of get_metadata() here because ls // should not exit with an err, if we are unable to obtain the target_metadata - let md_option = get_metadata_with_deref_opt(&target.p_buf, path.must_dereference) - .ok() + let md_option = target + .metadata() + .cloned() .or_else(|| path.p_buf.symlink_metadata().ok()); style_manager.apply_style_based_on_metadata(path, md_option.as_ref(), name, wrap) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index dc7fe856f71..61c4b8a71ec 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1775,7 +1775,7 @@ struct PathData { security_context: OnceCell, // can be used to avoid reading the metadata. Can be also called d_type: // https://www.gnu.org/software/libc/manual/html_node/Directory-Entries.html - de: Option, + de_md: Option, // Name of the file - will be empty for . or .. display_name: OsString, // PathBuf that all above data corresponds to @@ -1799,10 +1799,10 @@ impl PathData { } else if command_line { p_buf.clone().into() } else { - p_buf - .file_name() - .unwrap_or_else(|| p_buf.iter().next_back().unwrap()) - .to_owned() + dir_entry + .as_ref() + .map(|de| de.file_name()) + .unwrap_or_default() }; let must_dereference = match &config.dereference { @@ -1822,24 +1822,26 @@ impl PathData { Dereference::None => false, }; - let de: Option = dir_entry; - // Why prefer to check the DirEntry file_type()? B/c the call is // nearly free compared to a metadata() call on a Path - fn get_file_type(de: &DirEntry, p_buf: &Path, must_dereference: bool) -> Option { + fn get_file_type( + dir_entry: &DirEntry, + p_buf: &Path, + must_dereference: bool, + ) -> Option { if must_dereference { // wait for metadata call to populate file type return p_buf.metadata().ok().map(|md| md.file_type()); } - if let Ok(ft_de) = de.file_type() { + if let Ok(ft_de) = dir_entry.file_type() { return Some(ft_de); } None } - let ft = match de { + let ft = match dir_entry.as_ref() { Some(ref de) => OnceCell::from(get_file_type(de, &p_buf, must_dereference)), None => OnceCell::new(), }; @@ -1848,7 +1850,7 @@ impl PathData { md: OnceCell::new(), ft, security_context: OnceCell::new(), - de, + de_md: dir_entry.as_ref().and_then(|de| de.metadata().ok()), display_name, p_buf, must_dereference, @@ -1861,10 +1863,8 @@ impl PathData { .get_or_init(|| { // check if we can use DirEntry metadata // it will avoid a call to stat() - if !self.must_dereference { - if let Some(dir_entry) = &self.de { - return dir_entry.metadata().ok(); - } + if !self.must_dereference && self.de_md.is_some() { + return self.de_md.clone(); } // if not, check if we can use Path metadata @@ -1878,10 +1878,8 @@ impl PathData { // but GNU will not throw an error until a bad fd "dir" // is entered, here we match that GNU behavior, by handing // back the non-dereferenced metadata upon an EBADF - if self.must_dereference && errno == 9i32 { - if let Some(dir_entry) = &self.de { - return dir_entry.metadata().ok(); - } + if self.must_dereference && errno == 9i32 && self.de_md.is_some() { + return self.de_md.clone(); } show!(LsError::IOErrorContext( self.p_buf.clone(), @@ -1905,7 +1903,7 @@ impl PathData { fn is_dangling_link(&self) -> bool { // deref enabled, self is real dir entry, self has metadata associated with link, but not with target self.must_dereference - && self.de.is_some() + && self.de_md.is_some() && self.file_type().is_none() && self.metadata().is_none() } From 264a4310161618f0572a8a934a916c85e60fbf87 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 01:14:57 -0500 Subject: [PATCH 21/65] Revert "Perhaps fix GNU test" This reverts commit 34cbcb3f380a760390a0476019d691c733ec07cb. --- src/uu/ls/src/colors.rs | 6 +++--- src/uu/ls/src/ls.rs | 38 ++++++++++++++++++++------------------ 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index de3328c3f79..b75c7f41854 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -3,6 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. use super::PathData; +use super::get_metadata_with_deref_opt; use lscolors::{Indicator, LsColors, Style}; use std::ffi::OsString; use std::fs::Metadata; @@ -166,9 +167,8 @@ pub(crate) fn color_name( // use the optional target_symlink // Use fn get_metadata_with_deref_opt instead of get_metadata() here because ls // should not exit with an err, if we are unable to obtain the target_metadata - let md_option = target - .metadata() - .cloned() + let md_option = get_metadata_with_deref_opt(&target.p_buf, path.must_dereference) + .ok() .or_else(|| path.p_buf.symlink_metadata().ok()); style_manager.apply_style_based_on_metadata(path, md_option.as_ref(), name, wrap) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 61c4b8a71ec..dc7fe856f71 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1775,7 +1775,7 @@ struct PathData { security_context: OnceCell, // can be used to avoid reading the metadata. Can be also called d_type: // https://www.gnu.org/software/libc/manual/html_node/Directory-Entries.html - de_md: Option, + de: Option, // Name of the file - will be empty for . or .. display_name: OsString, // PathBuf that all above data corresponds to @@ -1799,10 +1799,10 @@ impl PathData { } else if command_line { p_buf.clone().into() } else { - dir_entry - .as_ref() - .map(|de| de.file_name()) - .unwrap_or_default() + p_buf + .file_name() + .unwrap_or_else(|| p_buf.iter().next_back().unwrap()) + .to_owned() }; let must_dereference = match &config.dereference { @@ -1822,26 +1822,24 @@ impl PathData { Dereference::None => false, }; + let de: Option = dir_entry; + // Why prefer to check the DirEntry file_type()? B/c the call is // nearly free compared to a metadata() call on a Path - fn get_file_type( - dir_entry: &DirEntry, - p_buf: &Path, - must_dereference: bool, - ) -> Option { + fn get_file_type(de: &DirEntry, p_buf: &Path, must_dereference: bool) -> Option { if must_dereference { // wait for metadata call to populate file type return p_buf.metadata().ok().map(|md| md.file_type()); } - if let Ok(ft_de) = dir_entry.file_type() { + if let Ok(ft_de) = de.file_type() { return Some(ft_de); } None } - let ft = match dir_entry.as_ref() { + let ft = match de { Some(ref de) => OnceCell::from(get_file_type(de, &p_buf, must_dereference)), None => OnceCell::new(), }; @@ -1850,7 +1848,7 @@ impl PathData { md: OnceCell::new(), ft, security_context: OnceCell::new(), - de_md: dir_entry.as_ref().and_then(|de| de.metadata().ok()), + de, display_name, p_buf, must_dereference, @@ -1863,8 +1861,10 @@ impl PathData { .get_or_init(|| { // check if we can use DirEntry metadata // it will avoid a call to stat() - if !self.must_dereference && self.de_md.is_some() { - return self.de_md.clone(); + if !self.must_dereference { + if let Some(dir_entry) = &self.de { + return dir_entry.metadata().ok(); + } } // if not, check if we can use Path metadata @@ -1878,8 +1878,10 @@ impl PathData { // but GNU will not throw an error until a bad fd "dir" // is entered, here we match that GNU behavior, by handing // back the non-dereferenced metadata upon an EBADF - if self.must_dereference && errno == 9i32 && self.de_md.is_some() { - return self.de_md.clone(); + if self.must_dereference && errno == 9i32 { + if let Some(dir_entry) = &self.de { + return dir_entry.metadata().ok(); + } } show!(LsError::IOErrorContext( self.p_buf.clone(), @@ -1903,7 +1905,7 @@ impl PathData { fn is_dangling_link(&self) -> bool { // deref enabled, self is real dir entry, self has metadata associated with link, but not with target self.must_dereference - && self.de_md.is_some() + && self.de.is_some() && self.file_type().is_none() && self.metadata().is_none() } From 1b85fd858c5ee1977c7610a8272899dfdea3b6ef Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 12:52:17 -0500 Subject: [PATCH 22/65] Fix GNU test --- src/uu/ls/src/colors.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index b75c7f41854..f6293ddc70e 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -163,6 +163,14 @@ pub(crate) fn color_name( } } + if !path.must_dereference { + // If we need to dereference (follow) a symlink, we will need to get the metadata + if let Some(md) = path.de.as_ref().and_then(|de| de.metadata().ok()) { + // There is a DirEntry, we don't need to get the metadata for the color + return style_manager.apply_style_based_on_metadata(path, Some(&md), name, wrap); + } + } + if let Some(target) = target_symlink { // use the optional target_symlink // Use fn get_metadata_with_deref_opt instead of get_metadata() here because ls From c9fa58127d7f1f39ef1cd8f0b004078cda466f85 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 13:28:15 -0500 Subject: [PATCH 23/65] Try again to pass GNU test --- src/uu/ls/src/colors.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index f6293ddc70e..6767bcd0ef8 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -165,9 +165,9 @@ pub(crate) fn color_name( if !path.must_dereference { // If we need to dereference (follow) a symlink, we will need to get the metadata - if let Some(md) = path.de.as_ref().and_then(|de| de.metadata().ok()) { + if let Some(de) = &path.de { // There is a DirEntry, we don't need to get the metadata for the color - return style_manager.apply_style_based_on_metadata(path, Some(&md), name, wrap); + return style_manager.apply_style_based_on_dir_entry(de, name, wrap); } } From 025136a544cdc19f5fbf6c858ade9766a3c563b5 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 13:28:17 -0500 Subject: [PATCH 24/65] Cleanup --- src/uu/ls/src/colors.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 6767bcd0ef8..7f6a3a4be36 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -4,7 +4,7 @@ // file that was distributed with this source code. use super::PathData; use super::get_metadata_with_deref_opt; -use lscolors::{Indicator, LsColors, Style}; +use lscolors::{Indicator, LsColors, Style, apply_style_based_on_dir_entry}; use std::ffi::OsString; use std::fs::Metadata; From aed04451818dea417e104374e83942cf274d306f Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 13:32:54 -0500 Subject: [PATCH 25/65] Cleanup --- src/uu/ls/src/colors.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 7f6a3a4be36..1d60d183eb3 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -4,7 +4,7 @@ // file that was distributed with this source code. use super::PathData; use super::get_metadata_with_deref_opt; -use lscolors::{Indicator, LsColors, Style, apply_style_based_on_dir_entry}; +use lscolors::{Indicator, LsColors, Style}; use std::ffi::OsString; use std::fs::Metadata; @@ -133,6 +133,16 @@ impl<'a> StyleManager<'a> { .style_for_path_with_metadata(&path.p_buf, md_option); self.apply_style(style, name, wrap) } + + pub(crate) fn apply_style_based_on_dir_entry( + &mut self, + dir_entry: &DirEntry, + name: OsString, + wrap: bool, + ) -> OsString { + let style = self.colors.style_for(dir_entry); + self.apply_style(style, name, wrap) + } } /// Colors the provided name based on the style determined for the given path From ba222c42028ee08dc349fd88a876124090ad636f Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 13:35:10 -0500 Subject: [PATCH 26/65] Cleanup --- src/uu/ls/src/colors.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 1d60d183eb3..2cf326191bd 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -6,6 +6,7 @@ use super::PathData; use super::get_metadata_with_deref_opt; use lscolors::{Indicator, LsColors, Style}; use std::ffi::OsString; +use std::fs::DirEntry; use std::fs::Metadata; /// We need this struct to be able to store the previous style. From 1a7ead96fb4af9bb5623ce9b11792bbcb66d195e Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 15:01:42 -0500 Subject: [PATCH 27/65] Reduce unnecessary metadata calls re: symlink targets --- src/uu/ls/src/ls.rs | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index dc7fe856f71..65dd923d873 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3127,7 +3127,7 @@ fn display_item_name( && !path.must_dereference { match path.p_buf.read_link() { - Ok(target) => { + Ok(target_path) => { name.push(" -> "); // We might as well color the symlink output after the arrow. @@ -3136,8 +3136,8 @@ fn display_item_name( if let Some(style_manager) = &mut state.style_manager { // 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() { + let mut absolute_target = target_path.clone(); + if target_path.is_relative() { if let Some(parent) = path.p_buf.parent() { absolute_target = parent.join(absolute_target); } @@ -3149,17 +3149,11 @@ fn display_item_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. - if path.metadata().is_none() - && get_metadata_with_deref_opt( - target_data.p_buf.as_path(), - target_data.must_dereference, - ) - .is_err() - { - name.push(target); + if path.metadata().is_none() && target_data.metadata().is_none() { + name.push(target_path); } else { name.push(color_name( - locale_aware_escape_name(target.as_os_str(), config.quoting_style), + locale_aware_escape_name(target_path.as_os_str(), config.quoting_style), path, style_manager, Some(&target_data), @@ -3170,7 +3164,7 @@ fn display_item_name( // If no coloring is required, we just use target as is. // Apply the right quoting name.push(locale_aware_escape_name( - target.as_os_str(), + target_path.as_os_str(), config.quoting_style, )); } From c1446ddf70c77dd2e3127d93006d362db4cf62fd Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 16:09:26 -0500 Subject: [PATCH 28/65] No need to call functions twice --- src/uu/ls/src/colors.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 2cf326191bd..e079bc6aab3 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -3,7 +3,6 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. use super::PathData; -use super::get_metadata_with_deref_opt; use lscolors::{Indicator, LsColors, Style}; use std::ffi::OsString; use std::fs::DirEntry; @@ -182,13 +181,11 @@ pub(crate) fn color_name( } } - if let Some(target) = target_symlink { + if let Some(_target) = target_symlink { // use the optional target_symlink - // Use fn get_metadata_with_deref_opt instead of get_metadata() here because ls + // Use fn symlink_metadata directly instead of get_metadata() here because ls // should not exit with an err, if we are unable to obtain the target_metadata - let md_option = get_metadata_with_deref_opt(&target.p_buf, path.must_dereference) - .ok() - .or_else(|| path.p_buf.symlink_metadata().ok()); + let md_option = path.p_buf.symlink_metadata().ok(); style_manager.apply_style_based_on_metadata(path, md_option.as_ref(), name, wrap) } else { From 16bca97f1014544b764e997ea91b5a5ac46b7cc2 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 17:11:18 -0500 Subject: [PATCH 29/65] Reduce size of PathData struct by more than 1000 bytes --- src/uu/ls/src/colors.rs | 17 ++------- src/uu/ls/src/ls.rs | 79 +++++++++++++++++++---------------------- 2 files changed, 38 insertions(+), 58 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index e079bc6aab3..e639b279fb4 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -5,7 +5,6 @@ use super::PathData; use lscolors::{Indicator, LsColors, Style}; use std::ffi::OsString; -use std::fs::DirEntry; use std::fs::Metadata; /// We need this struct to be able to store the previous style. @@ -133,16 +132,6 @@ impl<'a> StyleManager<'a> { .style_for_path_with_metadata(&path.p_buf, md_option); self.apply_style(style, name, wrap) } - - pub(crate) fn apply_style_based_on_dir_entry( - &mut self, - dir_entry: &DirEntry, - name: OsString, - wrap: bool, - ) -> OsString { - let style = self.colors.style_for(dir_entry); - self.apply_style(style, name, wrap) - } } /// Colors the provided name based on the style determined for the given path @@ -175,10 +164,8 @@ pub(crate) fn color_name( if !path.must_dereference { // If we need to dereference (follow) a symlink, we will need to get the metadata - if let Some(de) = &path.de { - // There is a DirEntry, we don't need to get the metadata for the color - return style_manager.apply_style_based_on_dir_entry(de, name, wrap); - } + // There is a DirEntry, we don't need to get the metadata for the color + return style_manager.apply_style_based_on_metadata(path, path.metadata(), name, wrap); } if let Some(_target) = target_symlink { diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 65dd923d873..346ca22fd47 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1773,9 +1773,6 @@ struct PathData { md: OnceCell>, ft: OnceCell>, security_context: OnceCell, - // can be used to avoid reading the metadata. Can be also called d_type: - // https://www.gnu.org/software/libc/manual/html_node/Directory-Entries.html - de: Option, // Name of the file - will be empty for . or .. display_name: OsString, // PathBuf that all above data corresponds to @@ -1797,12 +1794,13 @@ impl PathData { let display_name = if let Some(name) = file_name { name } else if command_line { - p_buf.clone().into() + p_buf.as_os_str().to_os_string() } else { - p_buf - .file_name() - .unwrap_or_else(|| p_buf.iter().next_back().unwrap()) - .to_owned() + dir_entry + .as_ref() + .map(|de| de.file_name()) + .or_else(|| p_buf.file_name().map(|inner| inner.to_os_string())) + .unwrap_or_default() }; let must_dereference = match &config.dereference { @@ -1822,8 +1820,6 @@ impl PathData { Dereference::None => false, }; - let de: Option = dir_entry; - // Why prefer to check the DirEntry file_type()? B/c the call is // nearly free compared to a metadata() call on a Path fn get_file_type(de: &DirEntry, p_buf: &Path, must_dereference: bool) -> Option { @@ -1839,16 +1835,26 @@ impl PathData { None } - let ft = match de { + let ft = match dir_entry.as_ref() { Some(ref de) => OnceCell::from(get_file_type(de, &p_buf, must_dereference)), None => OnceCell::new(), }; + let md = match dir_entry.as_ref() { + Some(ref de) if !must_dereference => { + // check if we can use DirEntry metadata + // it will avoid a call to stat() + OnceCell::from(de.metadata().ok()) + } + _ => OnceCell::new(), + }; + + let security_context = OnceCell::new(); + Self { - md: OnceCell::new(), + md, ft, - security_context: OnceCell::new(), - de, + security_context, display_name, p_buf, must_dereference, @@ -1859,14 +1865,6 @@ impl PathData { fn metadata(&self) -> Option<&Metadata> { self.md .get_or_init(|| { - // check if we can use DirEntry metadata - // it will avoid a call to stat() - if !self.must_dereference { - if let Some(dir_entry) = &self.de { - return dir_entry.metadata().ok(); - } - } - // if not, check if we can use Path metadata match get_metadata_with_deref_opt(self.p_buf.as_path(), self.must_dereference) { Err(err) => { @@ -1879,8 +1877,8 @@ impl PathData { // is entered, here we match that GNU behavior, by handing // back the non-dereferenced metadata upon an EBADF if self.must_dereference && errno == 9i32 { - if let Some(dir_entry) = &self.de { - return dir_entry.metadata().ok(); + if let Ok(file) = self.p_buf.read_link() { + return file.symlink_metadata().ok(); } } show!(LsError::IOErrorContext( @@ -1904,10 +1902,7 @@ impl PathData { fn is_dangling_link(&self) -> bool { // deref enabled, self is real dir entry, self has metadata associated with link, but not with target - self.must_dereference - && self.de.is_some() - && self.file_type().is_none() - && self.metadata().is_none() + self.must_dereference && self.file_type().is_none() && self.metadata().is_none() } #[cfg(unix)] @@ -1917,9 +1912,8 @@ impl PathData { } fn security_context(&self, config: &Config) -> &String { - self.security_context.get_or_init(|| { - get_security_context(config, &self.p_buf, self.metadata(), self.must_dereference) - }) + self.security_context + .get_or_init(|| get_security_context(self, config)) } } @@ -3243,23 +3237,22 @@ fn display_inode(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 -fn get_security_context( - config: &Config, - p_buf: &Path, - opt_metadata: Option<&Metadata>, - must_dereference: bool, -) -> String { +fn get_security_context(path: &PathData, config: &Config) -> String { let substitute_string = "?".to_string(); // If we must dereference, ensure that the symlink is actually valid even if the system // does not support SELinux. // Conforms to the GNU coreutils where a dangling symlink results in exit code 1. - if must_dereference && opt_metadata.is_none() { - if let Err(err) = get_metadata_with_deref_opt(p_buf, must_dereference) { + if path.must_dereference && path.metadata().is_none() { + if let Err(err) = get_metadata_with_deref_opt(&path.p_buf, path.must_dereference) { // The Path couldn't be dereferenced, so return early and set exit code 1 // to indicate a minor error // Only show error when context display is requested to avoid duplicate messages if config.context { - show!(LsError::IOErrorContext(p_buf.to_path_buf(), err, false)); + show!(LsError::IOErrorContext( + path.p_buf.to_path_buf(), + err, + false + )); } return substitute_string; } @@ -3267,10 +3260,10 @@ fn get_security_context( if config.selinux_supported { #[cfg(feature = "selinux")] { - match selinux::SecurityContext::of_path(p_buf, must_dereference.to_owned(), false) { + match selinux::SecurityContext::of_path(path.p_buf, path.must_dereference, false) { Err(_r) => { // TODO: show the actual reason why it failed - show_warning!("failed to get security context of: {}", p_buf.quote()); + show_warning!("failed to get security context of: {}", path.p_buf.quote()); substitute_string } Ok(None) => substitute_string, @@ -3281,7 +3274,7 @@ fn get_security_context( String::from_utf8(context.to_vec()).unwrap_or_else(|e| { show_warning!( "getting security context of: {}: {}", - p_buf.quote(), + path.p_buf.quote(), e.to_string() ); String::from_utf8_lossy(context).into_owned() From fdd99f6b8268904042ff3e03f8873987dbb54241 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 17:18:06 -0500 Subject: [PATCH 30/65] Fix lints --- src/uu/ls/src/ls.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 346ca22fd47..b47302df98c 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1836,12 +1836,12 @@ impl PathData { } let ft = match dir_entry.as_ref() { - Some(ref de) => OnceCell::from(get_file_type(de, &p_buf, must_dereference)), + Some(de) => OnceCell::from(get_file_type(de, &p_buf, must_dereference)), None => OnceCell::new(), }; let md = match dir_entry.as_ref() { - Some(ref de) if !must_dereference => { + Some(de) if !must_dereference => { // check if we can use DirEntry metadata // it will avoid a call to stat() OnceCell::from(de.metadata().ok()) @@ -3248,11 +3248,7 @@ fn get_security_context(path: &PathData, config: &Config) -> String { // to indicate a minor error // Only show error when context display is requested to avoid duplicate messages if config.context { - show!(LsError::IOErrorContext( - path.p_buf.to_path_buf(), - err, - false - )); + show!(LsError::IOErrorContext(path.p_buf.clone(), err, false)); } return substitute_string; } @@ -3260,7 +3256,11 @@ fn get_security_context(path: &PathData, config: &Config) -> String { if config.selinux_supported { #[cfg(feature = "selinux")] { - match selinux::SecurityContext::of_path(path.p_buf, path.must_dereference, false) { + match selinux::SecurityContext::of_path( + path.p_buf.clone(), + path.must_dereference, + false, + ) { Err(_r) => { // TODO: show the actual reason why it failed show_warning!("failed to get security context of: {}", path.p_buf.quote()); From a56e6b48dd3a1b26e372c99fa69bec62d02f3f3d Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 18:50:35 -0500 Subject: [PATCH 31/65] Fix GNU test? --- src/uu/ls/src/colors.rs | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index e639b279fb4..ce7604aed8f 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -3,10 +3,25 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. use super::PathData; -use lscolors::{Indicator, LsColors, Style}; +use lscolors::{Colorable, Indicator, LsColors, Style}; use std::ffi::OsString; use std::fs::Metadata; +impl Colorable for PathData { + fn file_name(&self) -> OsString { + self.display_name.clone() + } + fn file_type(&self) -> Option { + self.file_type().cloned() + } + fn metadata(&self) -> Option { + self.metadata().cloned() + } + fn path(&self) -> std::path::PathBuf { + self.p_buf.clone() + } +} + /// We need this struct to be able to store the previous style. /// This because we need to check the previous value in case we don't need /// the reset @@ -132,6 +147,16 @@ impl<'a> StyleManager<'a> { .style_for_path_with_metadata(&path.p_buf, md_option); self.apply_style(style, name, wrap) } + + pub(crate) fn apply_style_based_on_colorable( + &mut self, + path: &T, + name: OsString, + wrap: bool, + ) -> OsString { + let style = self.colors.style_for(path); + self.apply_style(style, name, wrap) + } } /// Colors the provided name based on the style determined for the given path @@ -165,7 +190,7 @@ pub(crate) fn color_name( if !path.must_dereference { // If we need to dereference (follow) a symlink, we will need to get the metadata // There is a DirEntry, we don't need to get the metadata for the color - return style_manager.apply_style_based_on_metadata(path, path.metadata(), name, wrap); + return style_manager.apply_style_based_on_colorable(path, name, wrap); } if let Some(_target) = target_symlink { From 8cc6772833b7546f681a5e78e709f7dde702e560 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 18:58:27 -0500 Subject: [PATCH 32/65] Fix lints --- src/uu/ls/src/colors.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index ce7604aed8f..24a6ace69a1 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -12,7 +12,7 @@ impl Colorable for PathData { self.display_name.clone() } fn file_type(&self) -> Option { - self.file_type().cloned() + self.file_type().copied() } fn metadata(&self) -> Option { self.metadata().cloned() From 5cfc35175b502e79d47d1781d32b4b61aad70113 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 19:39:48 -0500 Subject: [PATCH 33/65] Cleanup --- src/uu/ls/src/ls.rs | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index b47302df98c..5d92f75ce22 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1772,7 +1772,7 @@ struct PathData { // Result got from symlink_metadata() or metadata() based on config md: OnceCell>, ft: OnceCell>, - security_context: OnceCell, + security_context: OnceCell>, // Name of the file - will be empty for . or .. display_name: OsString, // PathBuf that all above data corresponds to @@ -1911,9 +1911,9 @@ impl PathData { && self.metadata().is_some_and(file_is_executable) } - fn security_context(&self, config: &Config) -> &String { + fn security_context(&self, config: &Config) -> &str { self.security_context - .get_or_init(|| get_security_context(self, config)) + .get_or_init(|| get_security_context(self, config).into()) } } @@ -3174,10 +3174,11 @@ fn display_item_name( if config.context { if let Some(pad_count) = prefix_context { let security_context = if matches!(config.format, Format::Commas) { - path.security_context(config).clone() + path.security_context(config).to_string() } else { - pad_left(path.security_context(config), pad_count).clone() + pad_left(path.security_context(config), pad_count) }; + let old_name = name; name = format!("{security_context} ").into(); name.push(old_name); @@ -3237,8 +3238,9 @@ fn display_inode(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 -fn get_security_context(path: &PathData, config: &Config) -> String { - let substitute_string = "?".to_string(); +fn get_security_context<'a>(path: &'a PathData, config: &'a Config) -> &'a str { + static SUBSTITUTE_STRING: &'static str = "?"; + // If we must dereference, ensure that the symlink is actually valid even if the system // does not support SELinux. // Conforms to the GNU coreutils where a dangling symlink results in exit code 1. @@ -3250,9 +3252,10 @@ fn get_security_context(path: &PathData, config: &Config) -> String { if config.context { show!(LsError::IOErrorContext(path.p_buf.clone(), err, false)); } - return substitute_string; + return SUBSTITUTE_STRING; } } + if config.selinux_supported { #[cfg(feature = "selinux")] { @@ -3264,31 +3267,27 @@ fn get_security_context(path: &PathData, config: &Config) -> String { Err(_r) => { // TODO: show the actual reason why it failed show_warning!("failed to get security context of: {}", path.p_buf.quote()); - substitute_string + return SUBSTITUTE_STRING; } - Ok(None) => substitute_string, + Ok(None) => return SUBSTITUTE_STRING, Ok(Some(context)) => { let context = context.as_bytes(); let context = context.strip_suffix(&[0]).unwrap_or(context); - String::from_utf8(context.to_vec()).unwrap_or_else(|e| { + return String::from_utf8(context.to_vec()).unwrap_or_else(|e| { show_warning!( "getting security context of: {}: {}", path.p_buf.quote(), e.to_string() ); String::from_utf8_lossy(context).into_owned() - }) + }); } } } - #[cfg(not(feature = "selinux"))] - { - substitute_string - } - } else { - substitute_string } + + SUBSTITUTE_STRING } #[cfg(unix)] From 313b8d6ff035ad46c247274ed544c2f2f56f82c8 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 19:47:05 -0500 Subject: [PATCH 34/65] Fix lint --- 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 5d92f75ce22..36cbc7dd349 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3239,7 +3239,7 @@ fn display_inode(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 fn get_security_context<'a>(path: &'a PathData, config: &'a Config) -> &'a str { - static SUBSTITUTE_STRING: &'static str = "?"; + static SUBSTITUTE_STRING: &str = "?"; // If we must dereference, ensure that the symlink is actually valid even if the system // does not support SELinux. From c10a6f8da70efe58064f8cc4c67b2d4e2c9d84d0 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 20:13:21 -0500 Subject: [PATCH 35/65] Cleanup --- src/uu/ls/src/ls.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 36cbc7dd349..da55cec6383 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -5,6 +5,7 @@ // spell-checker:ignore (ToDO) somegroup nlink tabsize dired subdired dtype colorterm stringly nohash strtime +use std::borrow::Cow; #[cfg(unix)] use std::collections::HashMap; #[cfg(unix)] @@ -3238,7 +3239,7 @@ fn display_inode(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 -fn get_security_context<'a>(path: &'a PathData, config: &'a Config) -> &'a str { +fn get_security_context<'a>(path: &'a PathData, config: &'a Config) -> Cow<'a, str> { static SUBSTITUTE_STRING: &str = "?"; // If we must dereference, ensure that the symlink is actually valid even if the system @@ -3252,7 +3253,7 @@ fn get_security_context<'a>(path: &'a PathData, config: &'a Config) -> &'a str { if config.context { show!(LsError::IOErrorContext(path.p_buf.clone(), err, false)); } - return SUBSTITUTE_STRING; + return Cow::Borrowed(SUBSTITUTE_STRING); } } @@ -3267,27 +3268,31 @@ fn get_security_context<'a>(path: &'a PathData, config: &'a Config) -> &'a str { Err(_r) => { // TODO: show the actual reason why it failed show_warning!("failed to get security context of: {}", path.p_buf.quote()); - return SUBSTITUTE_STRING; + return Cow::Borrowed(SUBSTITUTE_STRING); } - Ok(None) => return SUBSTITUTE_STRING, + Ok(None) => return Cow::Borrowed(SUBSTITUTE_STRING), Ok(Some(context)) => { let context = context.as_bytes(); let context = context.strip_suffix(&[0]).unwrap_or(context); - return String::from_utf8(context.to_vec()).unwrap_or_else(|e| { + + let res: String = String::from_utf8(context.to_vec()).unwrap_or_else(|e| { show_warning!( "getting security context of: {}: {}", path.p_buf.quote(), e.to_string() ); - String::from_utf8_lossy(context).into_owned() + + String::from_utf8_lossy(context).to_string() }); + + return Cow::Owned(res); } } } } - SUBSTITUTE_STRING + Cow::Borrowed(SUBSTITUTE_STRING) } #[cfg(unix)] From 924e183daeab9bedcb236dfc285da6e468eff449 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 20:56:50 -0500 Subject: [PATCH 36/65] Never make a syscall when some non-deref metadata is available --- src/uu/ls/src/ls.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index da55cec6383..0dbc20190fd 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1867,7 +1867,8 @@ impl PathData { self.md .get_or_init(|| { // if not, check if we can use Path metadata - match get_metadata_with_deref_opt(self.p_buf.as_path(), self.must_dereference) { + match get_metadata_with_deref_opt(self.p_buf.as_path(), None, self.must_dereference) + { Err(err) => { // FIXME: A bit tricky to propagate the result here let mut out = stdout().lock(); @@ -2136,7 +2137,7 @@ fn sort_entries(entries: &mut [PathData], config: &Config) { !match md { None | Some(None) => { // If it metadata cannot be determined, treat as a file. - get_metadata_with_deref_opt(p.p_buf.as_path(), true) + get_metadata_with_deref_opt(p.p_buf.as_path(), p.metadata(), true) .map_or_else(|_| false, |m| m.is_dir()) } Some(Some(m)) => m.is_dir(), @@ -2303,10 +2304,18 @@ fn enter_directory( Ok(()) } -fn get_metadata_with_deref_opt(p_buf: &Path, dereference: bool) -> std::io::Result { +fn get_metadata_with_deref_opt( + p_buf: &Path, + opt_metadata: Option<&Metadata>, + dereference: bool, +) -> std::io::Result { if dereference { p_buf.metadata() } else { + if let Some(md) = opt_metadata { + return Ok(md.clone()); + } + p_buf.symlink_metadata() } } @@ -3245,8 +3254,10 @@ fn get_security_context<'a>(path: &'a PathData, config: &'a Config) -> Cow<'a, s // If we must dereference, ensure that the symlink is actually valid even if the system // does not support SELinux. // Conforms to the GNU coreutils where a dangling symlink results in exit code 1. - if path.must_dereference && path.metadata().is_none() { - if let Err(err) = get_metadata_with_deref_opt(&path.p_buf, path.must_dereference) { + if path.must_dereference { + if let Err(err) = + get_metadata_with_deref_opt(&path.p_buf, path.metadata(), path.must_dereference) + { // The Path couldn't be dereferenced, so return early and set exit code 1 // to indicate a minor error // Only show error when context display is requested to avoid duplicate messages From e72dd91eaf61cabf135a703ae6d3f642c09b7c4b Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Mon, 22 Sep 2025 23:35:20 -0500 Subject: [PATCH 37/65] Lazily wait for file_type and metadata --- src/uu/ls/src/ls.rs | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 0dbc20190fd..b5e03a794dd 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1823,29 +1823,26 @@ impl PathData { // Why prefer to check the DirEntry file_type()? B/c the call is // nearly free compared to a metadata() call on a Path - fn get_file_type(de: &DirEntry, p_buf: &Path, must_dereference: bool) -> Option { - if must_dereference { - // wait for metadata call to populate file type - return p_buf.metadata().ok().map(|md| md.file_type()); - } - - if let Ok(ft_de) = de.file_type() { - return Some(ft_de); - } - - None - } - - let ft = match dir_entry.as_ref() { - Some(de) => OnceCell::from(get_file_type(de, &p_buf, must_dereference)), - None => OnceCell::new(), - }; - let md = match dir_entry.as_ref() { Some(de) if !must_dereference => { // check if we can use DirEntry metadata // it will avoid a call to stat() - OnceCell::from(de.metadata().ok()) + if let Ok(md) = de.metadata() { + OnceCell::from(Some(md)) + } else { + OnceCell::new() + } + } + _ => OnceCell::new(), + }; + + let ft = match dir_entry.as_ref() { + Some(de) if !must_dereference => { + if let Ok(ft) = de.file_type() { + OnceCell::from(Some(ft)) + } else { + OnceCell::new() + } } _ => OnceCell::new(), }; From 41afd566391fe69d5a8988ec6045ba19a266c2cf Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Tue, 23 Sep 2025 00:05:59 -0500 Subject: [PATCH 38/65] Cleanup --- src/uu/ls/src/ls.rs | 55 +++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index b5e03a794dd..6f22d6edf0e 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1773,7 +1773,7 @@ struct PathData { // Result got from symlink_metadata() or metadata() based on config md: OnceCell>, ft: OnceCell>, - security_context: OnceCell>, + security_context: Box, // Name of the file - will be empty for . or .. display_name: OsString, // PathBuf that all above data corresponds to @@ -1847,7 +1847,10 @@ impl PathData { _ => OnceCell::new(), }; - let security_context = OnceCell::new(); + let security_context: Box = md + .get() + .map(|md| get_security_context(&p_buf, md, must_dereference, config).into()) + .unwrap_or_default(); Self { md, @@ -1910,9 +1913,8 @@ impl PathData { && self.metadata().is_some_and(file_is_executable) } - fn security_context(&self, config: &Config) -> &str { - self.security_context - .get_or_init(|| get_security_context(self, config).into()) + fn security_context(&self) -> &str { + &self.security_context } } @@ -2465,7 +2467,7 @@ fn display_items( let mut longest_context_len = 1; let prefix_context = if config.context { for item in items { - let context_len = item.security_context(config).len(); + let context_len = item.security_context().len(); longest_context_len = context_len.max(longest_context_len); } Some(longest_context_len) @@ -2713,7 +2715,7 @@ fn display_item_long( #[cfg(all(unix, not(any(target_os = "android", target_os = "macos"))))] let is_acl_set = has_acl(item.display_name.as_os_str()); output_display.extend(display_permissions(md, true).as_bytes()); - if item.security_context(config).len() > 1 { + 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. output_display.extend(b"."); @@ -2735,7 +2737,7 @@ fn display_item_long( if config.context { output_display.extend(b" "); - output_display.extend_pad_right(item.security_context(config), padding.context); + output_display.extend_pad_right(item.security_context(), padding.context); } // Author is only different from owner on GNU/Hurd, so we reuse @@ -2847,7 +2849,7 @@ fn display_item_long( output_display.extend(leading_char.as_bytes()); output_display.extend(b"?????????"); - if item.security_context(config).len() > 1 { + 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. output_display.extend(b"."); @@ -2867,7 +2869,7 @@ fn display_item_long( if config.context { output_display.extend(b" "); - output_display.extend_pad_right(item.security_context(config), padding.context); + output_display.extend_pad_right(item.security_context(), padding.context); } // Author is only different from owner on GNU/Hurd, so we reuse @@ -3181,9 +3183,9 @@ fn display_item_name( if config.context { if let Some(pad_count) = prefix_context { let security_context = if matches!(config.format, Format::Commas) { - path.security_context(config).to_string() + path.security_context().to_string() } else { - pad_left(path.security_context(config), pad_count) + pad_left(path.security_context(), pad_count) }; let old_name = name; @@ -3245,21 +3247,24 @@ fn display_inode(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 -fn get_security_context<'a>(path: &'a PathData, config: &'a Config) -> Cow<'a, str> { +fn get_security_context<'a>( + path: &'a Path, + md: &'a Option, + must_dereference: bool, + config: &'a Config, +) -> Cow<'a, str> { static SUBSTITUTE_STRING: &str = "?"; // If we must dereference, ensure that the symlink is actually valid even if the system // does not support SELinux. // Conforms to the GNU coreutils where a dangling symlink results in exit code 1. - if path.must_dereference { - if let Err(err) = - get_metadata_with_deref_opt(&path.p_buf, path.metadata(), path.must_dereference) - { + if must_dereference && md.is_none() { + if let Err(err) = get_metadata_with_deref_opt(&path, md.as_ref(), must_dereference) { // The Path couldn't be dereferenced, so return early and set exit code 1 // to indicate a minor error // Only show error when context display is requested to avoid duplicate messages if config.context { - show!(LsError::IOErrorContext(path.p_buf.clone(), err, false)); + show!(LsError::IOErrorContext(path.to_path_buf(), err, false)); } return Cow::Borrowed(SUBSTITUTE_STRING); } @@ -3268,14 +3273,10 @@ fn get_security_context<'a>(path: &'a PathData, config: &'a Config) -> Cow<'a, s if config.selinux_supported { #[cfg(feature = "selinux")] { - match selinux::SecurityContext::of_path( - path.p_buf.clone(), - path.must_dereference, - false, - ) { + match selinux::SecurityContext::of_path(path.clone(), must_dereference, false) { Err(_r) => { // TODO: show the actual reason why it failed - show_warning!("failed to get security context of: {}", path.p_buf.quote()); + show_warning!("failed to get security context of: {}", path.quote()); return Cow::Borrowed(SUBSTITUTE_STRING); } Ok(None) => return Cow::Borrowed(SUBSTITUTE_STRING), @@ -3287,7 +3288,7 @@ fn get_security_context<'a>(path: &'a PathData, config: &'a Config) -> Cow<'a, s let res: String = String::from_utf8(context.to_vec()).unwrap_or_else(|e| { show_warning!( "getting security context of: {}: {}", - path.p_buf.quote(), + path.quote(), e.to_string() ); @@ -3340,7 +3341,7 @@ fn calculate_padding_collection( } if config.format == Format::Long { - let context_len = item.security_context(config).len(); + let context_len = item.security_context().len(); let (link_count_len, uname_len, group_len, size_len, major_len, minor_len) = display_dir_entry_size(item, config, state); padding_collections.link_count = link_count_len.max(padding_collections.link_count); @@ -3389,7 +3390,7 @@ fn calculate_padding_collection( } } - let context_len = item.security_context(config).len(); + let context_len = item.security_context().len(); let (link_count_len, uname_len, group_len, size_len, _major_len, _minor_len) = display_dir_entry_size(item, config, state); padding_collections.link_count = link_count_len.max(padding_collections.link_count); From 3360ae4d4d5dee91cae0e641e6ca711bf0b21db4 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Tue, 23 Sep 2025 00:11:21 -0500 Subject: [PATCH 39/65] Cleanup --- src/uu/ls/src/ls.rs | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 6f22d6edf0e..5df654aac61 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1847,10 +1847,8 @@ impl PathData { _ => OnceCell::new(), }; - let security_context: Box = md - .get() - .map(|md| get_security_context(&p_buf, md, must_dereference, config).into()) - .unwrap_or_default(); + let security_context: Box = + get_security_context(&p_buf, must_dereference, config).into(); Self { md, @@ -1867,8 +1865,7 @@ impl PathData { self.md .get_or_init(|| { // if not, check if we can use Path metadata - match get_metadata_with_deref_opt(self.p_buf.as_path(), None, self.must_dereference) - { + match get_metadata_with_deref_opt(self.p_buf.as_path(), self.must_dereference) { Err(err) => { // FIXME: A bit tricky to propagate the result here let mut out = stdout().lock(); @@ -2136,7 +2133,7 @@ fn sort_entries(entries: &mut [PathData], config: &Config) { !match md { None | Some(None) => { // If it metadata cannot be determined, treat as a file. - get_metadata_with_deref_opt(p.p_buf.as_path(), p.metadata(), true) + get_metadata_with_deref_opt(p.p_buf.as_path(), true) .map_or_else(|_| false, |m| m.is_dir()) } Some(Some(m)) => m.is_dir(), @@ -2303,18 +2300,10 @@ fn enter_directory( Ok(()) } -fn get_metadata_with_deref_opt( - p_buf: &Path, - opt_metadata: Option<&Metadata>, - dereference: bool, -) -> std::io::Result { +fn get_metadata_with_deref_opt(p_buf: &Path, dereference: bool) -> std::io::Result { if dereference { p_buf.metadata() } else { - if let Some(md) = opt_metadata { - return Ok(md.clone()); - } - p_buf.symlink_metadata() } } @@ -3249,7 +3238,6 @@ fn display_inode(metadata: &Metadata) -> String { /// In the long term this should be changed to [`OsStr`], see discussions at #2621/#2656 fn get_security_context<'a>( path: &'a Path, - md: &'a Option, must_dereference: bool, config: &'a Config, ) -> Cow<'a, str> { @@ -3258,8 +3246,8 @@ fn get_security_context<'a>( // If we must dereference, ensure that the symlink is actually valid even if the system // does not support SELinux. // Conforms to the GNU coreutils where a dangling symlink results in exit code 1. - if must_dereference && md.is_none() { - if let Err(err) = get_metadata_with_deref_opt(&path, md.as_ref(), must_dereference) { + if must_dereference { + if let Err(err) = get_metadata_with_deref_opt(&path, must_dereference) { // The Path couldn't be dereferenced, so return early and set exit code 1 // to indicate a minor error // Only show error when context display is requested to avoid duplicate messages From ed63bf5b751f4ba71292998cd30032c7fc31f5bf Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Tue, 23 Sep 2025 00:21:43 -0500 Subject: [PATCH 40/65] Cleanup --- src/uu/ls/src/colors.rs | 4 +-- src/uu/ls/src/ls.rs | 80 +++++++++++++++++++++++------------------ 2 files changed, 48 insertions(+), 36 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 24a6ace69a1..fbc35518701 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -9,7 +9,7 @@ use std::fs::Metadata; impl Colorable for PathData { fn file_name(&self) -> OsString { - self.display_name.clone() + self.display_name().to_os_string() } fn file_type(&self) -> Option { self.file_type().copied() @@ -18,7 +18,7 @@ impl Colorable for PathData { self.metadata().cloned() } fn path(&self) -> std::path::PathBuf { - self.p_buf.clone() + self.path().to_path_buf() } } diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 5df654aac61..c727ca68f08 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1865,7 +1865,7 @@ impl PathData { self.md .get_or_init(|| { // if not, check if we can use Path metadata - match get_metadata_with_deref_opt(self.p_buf.as_path(), self.must_dereference) { + match get_metadata_with_deref_opt(self.path(), self.must_dereference) { Err(err) => { // FIXME: A bit tricky to propagate the result here let mut out = stdout().lock(); @@ -1876,12 +1876,12 @@ impl PathData { // is entered, here we match that GNU behavior, by handing // back the non-dereferenced metadata upon an EBADF if self.must_dereference && errno == 9i32 { - if let Ok(file) = self.p_buf.read_link() { + if let Ok(file) = self.path().read_link() { return file.symlink_metadata().ok(); } } show!(LsError::IOErrorContext( - self.p_buf.clone(), + self.path().to_path_buf(), err, self.command_line )); @@ -1913,6 +1913,14 @@ impl PathData { fn security_context(&self) -> &str { &self.security_context } + + fn path<'a>(&'a self) -> &'a Path { + &self.p_buf + } + + fn display_name<'a>(&'a self) -> &'a OsStr { + &self.display_name + } } /// Show the directory name in the case where several arguments are given to ls @@ -1932,7 +1940,7 @@ fn show_dir_name( config: &Config, ) -> std::io::Result<()> { let escaped_name = - locale_aware_escape_dir_name(path_data.p_buf.as_os_str(), config.quoting_style); + locale_aware_escape_dir_name(path_data.path().as_os_str(), config.quoting_style); let name = if config.hyperlink && !config.dired { create_hyperlink(&escaped_name, path_data) @@ -2026,12 +2034,12 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { for (pos, path_data) in dirs.iter().enumerate() { // Do read_dir call here to match GNU semantics by printing // read_dir errors before directory headings, names and totals - let read_dir = match fs::read_dir(&path_data.p_buf) { + let read_dir = match fs::read_dir(path_data.path()) { Err(err) => { // flush stdout buffer before the error to preserve formatting and order state.out.flush()?; show!(LsError::IOErrorContext( - path_data.p_buf.clone(), + path_data.path().to_path_buf(), err, path_data.command_line )); @@ -2050,7 +2058,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { writeln!(state.out)?; if config.dired { // First directory displayed - let dir_len = path_data.display_name.len(); + let dir_len = path_data.display_name().len(); // add the //SUBDIRED// coordinates dired::calculate_subdired(&mut dired, dir_len); // Add the padding for the dir name @@ -2064,7 +2072,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { } let mut listed_ancestors = HashSet::new(); listed_ancestors.insert(FileInformation::from_path( - &path_data.p_buf, + path_data.path(), path_data.must_dereference, )?); enter_directory( @@ -2095,25 +2103,25 @@ fn sort_entries(entries: &mut [PathData], config: &Config) { entries.sort_by_key(|k| Reverse(k.metadata().map_or(0, |md| md.len()))); } // The default sort in GNU ls is case insensitive - Sort::Name => entries.sort_by(|a, b| a.display_name.cmp(&b.display_name)), + Sort::Name => entries.sort_by(|a, b| a.display_name().cmp(&b.display_name())), Sort::Version => entries.sort_by(|a, b| { version_cmp( - os_str_as_bytes_lossy(a.p_buf.as_os_str()).as_ref(), - os_str_as_bytes_lossy(b.p_buf.as_os_str()).as_ref(), + os_str_as_bytes_lossy(a.path().as_os_str()).as_ref(), + os_str_as_bytes_lossy(b.path().as_os_str()).as_ref(), ) - .then(a.p_buf.to_string_lossy().cmp(&b.p_buf.to_string_lossy())) + .then(a.path().to_string_lossy().cmp(&b.path().to_string_lossy())) }), Sort::Extension => entries.sort_by(|a, b| { - a.p_buf + a.path() .extension() - .cmp(&b.p_buf.extension()) - .then(a.p_buf.file_stem().cmp(&b.p_buf.file_stem())) + .cmp(&b.path().extension()) + .then(a.path().file_stem().cmp(&b.path().file_stem())) }), Sort::Width => entries.sort_by(|a, b| { - a.display_name + a.display_name() .len() - .cmp(&b.display_name.len()) - .then(a.display_name.cmp(&b.display_name)) + .cmp(&b.display_name().len()) + .then(a.display_name().cmp(&b.display_name())) }), Sort::None => {} } @@ -2133,7 +2141,7 @@ fn sort_entries(entries: &mut [PathData], config: &Config) { !match md { None | Some(None) => { // If it metadata cannot be determined, treat as a file. - get_metadata_with_deref_opt(p.p_buf.as_path(), true) + get_metadata_with_deref_opt(p.path(), true) .map_or_else(|_| false, |m| m.is_dir()) } Some(Some(m)) => m.is_dir(), @@ -2200,14 +2208,14 @@ fn enter_directory( let mut entries: Vec = if config.files == Files::All { vec![ PathData::new( - path_data.p_buf.clone(), + path_data.path().to_path_buf(), None, Some(".".into()), config, false, ), PathData::new( - path_data.p_buf.join(".."), + path_data.path().join(".."), None, Some("..".into()), config, @@ -2255,18 +2263,18 @@ fn enter_directory( .skip(if config.files == Files::All { 2 } else { 0 }) .filter(|p| p.file_type().is_some_and(|ft| ft.is_dir())) { - match fs::read_dir(&e.p_buf) { + match fs::read_dir(e.path()) { Err(err) => { state.out.flush()?; show!(LsError::IOErrorContext( - e.p_buf.clone(), + e.path().to_path_buf(), err, e.command_line )); } Ok(rd) => { if listed_ancestors - .insert(FileInformation::from_path(&e.p_buf, e.must_dereference)?) + .insert(FileInformation::from_path(e.path(), e.must_dereference)?) { // when listing several directories in recursive mode, we show // "dirname:" at the beginning of the file list @@ -2277,7 +2285,7 @@ fn enter_directory( // 2 = \n + \n dired.padding = 2; dired::indent(&mut state.out)?; - let dir_name_size = e.p_buf.to_string_lossy().len(); + let dir_name_size = e.path().to_string_lossy().len(); dired::calculate_subdired(dired, dir_name_size); // inject dir name dired::add_dir_name(dired, dir_name_size); @@ -2287,10 +2295,10 @@ fn enter_directory( writeln!(state.out)?; enter_directory(e, rd, config, state, listed_ancestors, dired)?; listed_ancestors - .remove(&FileInformation::from_path(&e.p_buf, e.must_dereference)?); + .remove(&FileInformation::from_path(e.path(), e.must_dereference)?); } else { state.out.flush()?; - show!(LsError::AlreadyListedError(e.p_buf.clone())); + show!(LsError::AlreadyListedError(e.path().to_path_buf())); } } } @@ -2431,7 +2439,7 @@ fn display_items( // option, print the security context to the left of the size column. let quoted = items.iter().any(|item| { - let name = locale_aware_escape_name(&item.display_name, config.quoting_style); + let name = locale_aware_escape_name(&item.display_name(), config.quoting_style); os_str_starts_with(&name, b"'") }); @@ -2702,7 +2710,7 @@ fn display_item_long( // TODO: See how Mac should work here let is_acl_set = false; #[cfg(all(unix, not(any(target_os = "android", target_os = "macos"))))] - let is_acl_set = has_acl(item.display_name.as_os_str()); + let is_acl_set = has_acl(item.display_name()); output_display.extend(display_permissions(md, true).as_bytes()); if item.security_context().len() > 1 { // GNU `ls` uses a "." character to indicate a file with a security context, @@ -3067,7 +3075,7 @@ fn display_item_name( current_column: LazyCell usize + '_>>, ) -> OsString { // This is our return value. We start by `&path.display_name` and modify it along the way. - let mut name = locale_aware_escape_name(&path.display_name, config.quoting_style); + let mut name = locale_aware_escape_name(&path.display_name(), config.quoting_style); let is_wrap = |namelen: usize| config.width != 0 && *current_column + namelen > config.width.into(); @@ -3118,7 +3126,7 @@ fn display_item_name( && path.file_type().is_some_and(|ft| ft.is_symlink()) && !path.must_dereference { - match path.p_buf.read_link() { + match path.path().read_link() { Ok(target_path) => { name.push(" -> "); @@ -3130,7 +3138,7 @@ fn display_item_name( // This is because relative symlinks will fail to get_metadata. let mut absolute_target = target_path.clone(); if target_path.is_relative() { - if let Some(parent) = path.p_buf.parent() { + if let Some(parent) = path.path().parent() { absolute_target = parent.join(absolute_target); } } @@ -3162,7 +3170,11 @@ fn display_item_name( } } Err(err) => { - show!(LsError::IOErrorContext(path.p_buf.clone(), err, false)); + show!(LsError::IOErrorContext( + path.path().to_path_buf(), + err, + false + )); } } } @@ -3190,7 +3202,7 @@ fn create_hyperlink(name: &OsStr, path: &PathData) -> OsString { let hostname = hostname::get().unwrap_or_else(|_| OsString::from("")); let hostname = hostname.to_string_lossy(); - let absolute_path = fs::canonicalize(&path.p_buf).unwrap_or_default(); + let absolute_path = fs::canonicalize(path.path()).unwrap_or_default(); let absolute_path = absolute_path.to_string_lossy(); #[cfg(not(target_os = "windows"))] From e504572a47e92fb6a46cb5fee58da8c93bafd071 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Tue, 23 Sep 2025 00:23:15 -0500 Subject: [PATCH 41/65] Cleanup --- src/uu/ls/src/ls.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index c727ca68f08..fdd978515a5 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1914,11 +1914,11 @@ impl PathData { &self.security_context } - fn path<'a>(&'a self) -> &'a Path { + fn path(&self) -> &Path { &self.p_buf } - fn display_name<'a>(&'a self) -> &'a OsStr { + fn display_name(&self) -> &OsStr { &self.display_name } } From 1970a943524790a0dc1a34d4ef6d83eda36c6224 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Tue, 23 Sep 2025 00:33:31 -0500 Subject: [PATCH 42/65] Fix lints --- src/uu/ls/src/ls.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index fdd978515a5..78db727c948 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2103,25 +2103,25 @@ fn sort_entries(entries: &mut [PathData], config: &Config) { entries.sort_by_key(|k| Reverse(k.metadata().map_or(0, |md| md.len()))); } // The default sort in GNU ls is case insensitive - Sort::Name => entries.sort_by(|a, b| a.display_name().cmp(&b.display_name())), + Sort::Name => entries.sort_by(|a, b| a.display_name().cmp(b.display_name())), Sort::Version => entries.sort_by(|a, b| { version_cmp( os_str_as_bytes_lossy(a.path().as_os_str()).as_ref(), os_str_as_bytes_lossy(b.path().as_os_str()).as_ref(), ) - .then(a.path().to_string_lossy().cmp(&b.path().to_string_lossy())) + .then(a.path().to_string_lossy().cmp(b.path().to_string_lossy())) }), Sort::Extension => entries.sort_by(|a, b| { a.path() .extension() - .cmp(&b.path().extension()) - .then(a.path().file_stem().cmp(&b.path().file_stem())) + .cmp(b.path().extension()) + .then(a.path().file_stem().cmp(b.path().file_stem())) }), Sort::Width => entries.sort_by(|a, b| { a.display_name() .len() - .cmp(&b.display_name().len()) - .then(a.display_name().cmp(&b.display_name())) + .cmp(b.display_name().len()) + .then(a.display_name().cmp(b.display_name())) }), Sort::None => {} } @@ -2439,7 +2439,7 @@ fn display_items( // option, print the security context to the left of the size column. let quoted = items.iter().any(|item| { - let name = locale_aware_escape_name(&item.display_name(), config.quoting_style); + let name = locale_aware_escape_name(item.display_name(), config.quoting_style); os_str_starts_with(&name, b"'") }); @@ -3075,7 +3075,7 @@ fn display_item_name( current_column: LazyCell usize + '_>>, ) -> OsString { // This is our return value. We start by `&path.display_name` and modify it along the way. - let mut name = locale_aware_escape_name(&path.display_name(), config.quoting_style); + let mut name = locale_aware_escape_name(path.display_name(), config.quoting_style); let is_wrap = |namelen: usize| config.width != 0 && *current_column + namelen > config.width.into(); From c316a161cf905aa6f5dbb34be7bd2e90f0dc766d Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Tue, 23 Sep 2025 00:37:51 -0500 Subject: [PATCH 43/65] Fix lints --- src/uu/ls/src/ls.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 78db727c948..bba1989aeb4 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2103,25 +2103,25 @@ fn sort_entries(entries: &mut [PathData], config: &Config) { entries.sort_by_key(|k| Reverse(k.metadata().map_or(0, |md| md.len()))); } // The default sort in GNU ls is case insensitive - Sort::Name => entries.sort_by(|a, b| a.display_name().cmp(b.display_name())), + Sort::Name => entries.sort_by(|a, b| a.display_name().cmp(&b.display_name())), Sort::Version => entries.sort_by(|a, b| { version_cmp( os_str_as_bytes_lossy(a.path().as_os_str()).as_ref(), os_str_as_bytes_lossy(b.path().as_os_str()).as_ref(), ) - .then(a.path().to_string_lossy().cmp(b.path().to_string_lossy())) + .then(a.path().to_string_lossy().cmp(&b.path().to_string_lossy())) }), Sort::Extension => entries.sort_by(|a, b| { a.path() .extension() - .cmp(b.path().extension()) - .then(a.path().file_stem().cmp(b.path().file_stem())) + .cmp(&b.path().extension()) + .then(a.path().file_stem().cmp(&b.path().file_stem())) }), Sort::Width => entries.sort_by(|a, b| { a.display_name() .len() - .cmp(b.display_name().len()) - .then(a.display_name().cmp(b.display_name())) + .cmp(&b.display_name().len()) + .then(a.display_name().cmp(&b.display_name())) }), Sort::None => {} } From 290e8b43e477ec443316b648d31f4dcbac6e3b36 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Tue, 23 Sep 2025 00:57:25 -0500 Subject: [PATCH 44/65] Fix lints --- 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 bba1989aeb4..9b7d9e7cf81 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2103,7 +2103,7 @@ fn sort_entries(entries: &mut [PathData], config: &Config) { entries.sort_by_key(|k| Reverse(k.metadata().map_or(0, |md| md.len()))); } // The default sort in GNU ls is case insensitive - Sort::Name => entries.sort_by(|a, b| a.display_name().cmp(&b.display_name())), + Sort::Name => entries.sort_by(|a, b| a.display_name().cmp(b.display_name())), Sort::Version => entries.sort_by(|a, b| { version_cmp( os_str_as_bytes_lossy(a.path().as_os_str()).as_ref(), @@ -2121,7 +2121,7 @@ fn sort_entries(entries: &mut [PathData], config: &Config) { a.display_name() .len() .cmp(&b.display_name().len()) - .then(a.display_name().cmp(&b.display_name())) + .then(a.display_name().cmp(b.display_name())) }), Sort::None => {} } @@ -3259,7 +3259,7 @@ fn get_security_context<'a>( // does not support SELinux. // Conforms to the GNU coreutils where a dangling symlink results in exit code 1. if must_dereference { - if let Err(err) = get_metadata_with_deref_opt(&path, must_dereference) { + if let Err(err) = get_metadata_with_deref_opt(path, must_dereference) { // The Path couldn't be dereferenced, so return early and set exit code 1 // to indicate a minor error // Only show error when context display is requested to avoid duplicate messages From 3b904128514e204d7453e3f59463d4257d3038d3 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Tue, 23 Sep 2025 01:02:20 -0500 Subject: [PATCH 45/65] Fix lints --- 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 9b7d9e7cf81..5f5c25d998b 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3273,7 +3273,7 @@ fn get_security_context<'a>( if config.selinux_supported { #[cfg(feature = "selinux")] { - match selinux::SecurityContext::of_path(path.clone(), must_dereference, false) { + match selinux::SecurityContext::of_path(path.to_path_buf(), must_dereference, false) { Err(_r) => { // TODO: show the actual reason why it failed show_warning!("failed to get security context of: {}", path.quote()); From dd78a352d73b82e1c23b00c7a0ba95ad7ecaa208 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Tue, 23 Sep 2025 01:06:29 -0500 Subject: [PATCH 46/65] Fix lints --- 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 5f5c25d998b..ef1c5a39d9c 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3273,7 +3273,7 @@ fn get_security_context<'a>( if config.selinux_supported { #[cfg(feature = "selinux")] { - match selinux::SecurityContext::of_path(path.to_path_buf(), must_dereference, false) { + match selinux::SecurityContext::of_path(path, must_dereference, false) { Err(_r) => { // TODO: show the actual reason why it failed show_warning!("failed to get security context of: {}", path.quote()); From e205d760870129a50fc087f47c7b5994d4fe69a2 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 03:50:23 -0500 Subject: [PATCH 47/65] Initial commit --- src/uu/ls/src/ls.rs | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index ba7f26f1aad..e080877995f 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2320,10 +2320,31 @@ fn display_dir_entry_size( } SizeOrDeviceId::Size(size) => (size.len(), 0usize, 0usize), }; + + let long_format = &config.long; + + let display_symlink_count = if long_format.numeric_uid_gid { + display_symlink_count(md).len() + } else { + 0 + }; + + let display_uname = if long_format.owner { + display_uname(md, config, state).len() + } else { + 0 + }; + + let display_group = if long_format.group { + display_group(md, config, state).len() + } else { + 0 + }; + ( - display_symlink_count(md).len(), - display_uname(md, config, state).len(), - display_group(md, config, state).len(), + display_symlink_count, + display_uname, + display_group, size_len, major_len, minor_len, From d8ef736fbac9e0c5a6a4583f0245fb9059616916 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 10:29:59 -0500 Subject: [PATCH 48/65] Cleanup hot path --- src/uu/ls/src/ls.rs | 61 ++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index e080877995f..406d3bb741a 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2173,10 +2173,8 @@ fn should_display(entry: &DirEntry, config: &Config) -> bool { // https://github.com/rust-lang/glob/issues/23 // https://github.com/rust-lang/glob/issues/78 // https://github.com/BurntSushi/ripgrep/issues/1250 - let file_name = match file_name.to_str() { - Some(s) => s.to_string(), - None => file_name.to_string_lossy().into_owned(), - }; + let file_name = file_name.to_string_lossy(); + !config .ignore_patterns .iter() @@ -2379,7 +2377,7 @@ impl ExtendPad for Vec { // TODO: Consider converting callers to use ExtendPad instead, as it avoids // additional copies. -fn pad_left(string: &str, count: usize) -> String { +fn pad_left(string: &T, count: usize) -> String { format!("{string:>count$}") } @@ -2416,19 +2414,20 @@ fn display_additional_leading_info( { if config.inode { let i = if let Some(md) = item.get_metadata(out) { - get_inode(md) + &display_inode(md) } else { - "?".to_owned() + "?" }; + write!(result, "{} ", pad_left(&i, padding.inode)).unwrap(); } } if config.alloc_size { let s = if let Some(md) = item.get_metadata(out) { - display_size(get_block_size(md, config), config) + &display_size(get_block_size(md, config), config) } else { - "?".to_owned() + "?" }; // extra space is insert to align the sizes, as needed for all formats, except for the comma format. if config.format == Format::Commas { @@ -3038,26 +3037,26 @@ fn file_is_executable(md: &Metadata) -> bool { return md.mode() & ((S_IXUSR | S_IXGRP | S_IXOTH) as u32) != 0; } -fn classify_file(path: &PathData, out: &mut BufWriter) -> Option { +fn classify_file<'a>(path: &'a PathData, out: &mut BufWriter) -> Option<&'a str> { let file_type = path.file_type(out)?; if file_type.is_dir() { - Some('/') + Some("/") } else if file_type.is_symlink() { - Some('@') + Some("@") } else { #[cfg(unix)] { if file_type.is_socket() { - Some('=') + Some("=") } else if file_type.is_fifo() { - Some('|') + Some("|") } else if file_type.is_file() // Safe unwrapping if the file was removed between listing and display // See https://github.com/uutils/coreutils/issues/5371 && path.get_metadata(out).is_some_and(file_is_executable) { - Some('*') + Some("*") } else { None } @@ -3121,19 +3120,19 @@ fn display_item_name( if config.indicator_style != IndicatorStyle::None { let sym = classify_file(path, &mut state.out); - let char_opt = match config.indicator_style { + let char_opt: Option<&str> = match config.indicator_style { IndicatorStyle::Classify => sym, IndicatorStyle::FileType => { // Don't append an asterisk. match sym { - Some('*') => None, + Some("*") => None, _ => sym, } } IndicatorStyle::Slash => { // Append only a slash. match sym { - Some('/') => Some('/'), + Some("/") => Some("/"), _ => None, } } @@ -3141,7 +3140,7 @@ fn display_item_name( }; if let Some(c) = char_opt { - name.push(OsStr::new(&c.to_string())); + name.push(c); } } @@ -3211,9 +3210,9 @@ fn display_item_name( if config.context { if let Some(pad_count) = prefix_context { let security_context = if matches!(config.format, Format::Commas) { - path.security_context.clone() + &path.security_context } else { - pad_left(&path.security_context, pad_count) + &pad_left(&path.security_context, pad_count) }; let old_name = name; name = format!("{security_context} ").into(); @@ -3237,16 +3236,16 @@ fn create_hyperlink(name: &OsStr, path: &PathData) -> OsString { let unencoded_chars = "_-.:~/\\"; // percentage encoding of path - let absolute_path: String = absolute_path - .chars() - .map(|c| { - if c.is_alphanumeric() || unencoded_chars.contains(c) { - c.to_string() - } else { - format!("%{:02x}", c as u8) - } - }) - .collect(); + let absolute_path: String = absolute_path.chars().fold(String::new(), |mut acc, c| { + if c.is_alphanumeric() || unencoded_chars.contains(c) { + acc.push(c); + } else { + let x = format!("%{:02x}", c as u8); + acc.push_str(&x); + }; + + acc + }); // \x1b = ESC, \x07 = BEL let mut ret: OsString = format!("\x1b]8;;file://{hostname}{absolute_path}\x07").into(); From f3c2b83c0f5000964fc42690a3cac49005657d8f Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 10:33:59 -0500 Subject: [PATCH 49/65] Cleanup --- 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 406d3bb741a..c85c3470232 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3214,9 +3214,8 @@ fn display_item_name( } else { &pad_left(&path.security_context, pad_count) }; - let old_name = name; - name = format!("{security_context} ").into(); - name.push(old_name); + let old_name = name.to_string_lossy(); + name = format!("{security_context} {old_name}").into(); } } From 958f950889f480c4bc5858a54b3735283ff7beed Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 10:51:03 -0500 Subject: [PATCH 50/65] Fix GNU test? --- src/uu/ls/src/colors.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index fbc35518701..b700309a94d 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -193,11 +193,14 @@ pub(crate) fn color_name( return style_manager.apply_style_based_on_colorable(path, name, wrap); } - if let Some(_target) = target_symlink { + if let Some(target) = target_symlink { // use the optional target_symlink // Use fn symlink_metadata directly instead of get_metadata() here because ls // should not exit with an err, if we are unable to obtain the target_metadata - let md_option = path.p_buf.symlink_metadata().ok(); + let md_option: Option = target + .metadata() + .cloned() + .or_else(|| path.p_buf.symlink_metadata().ok()); style_manager.apply_style_based_on_metadata(path, md_option.as_ref(), name, wrap) } else { From 14aff2bee1596ebe5d5bd190468f30b88d2f0456 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 11:08:34 -0500 Subject: [PATCH 51/65] Fix lints --- src/uu/ls/src/ls.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index c85c3470232..04190aa871b 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2320,8 +2320,19 @@ fn display_dir_entry_size( }; let long_format = &config.long; + let numeric_uid_gid; - let display_symlink_count = if long_format.numeric_uid_gid { + #[cfg(unix)] + { + numeric_uid_gid = long_format.numeric_uid_gid; + } + + #[cfg(not(unix))] + { + numeric_uid_gid = false; + } + + let display_symlink_count = if numeric_uid_gid { display_symlink_count(md).len() } else { 0 From 42c756803def160304fcf6f2a5240a16b3ead5a0 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 11:25:36 -0500 Subject: [PATCH 52/65] No need to convert to strings --- src/uu/ls/src/ls.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 04190aa871b..a8ac3227463 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -5,6 +5,7 @@ // spell-checker:ignore (ToDO) somegroup nlink tabsize dired subdired dtype colorterm stringly nohash strtime +use std::borrow::Cow; #[cfg(unix)] use std::collections::HashMap; #[cfg(unix)] @@ -2660,10 +2661,7 @@ fn display_grid( }; // FIXME: the Grid crate only supports &str, so can't display raw bytes - let names: Vec<_> = names - .into_iter() - .map(|s| s.to_string_lossy().into_owned()) - .collect(); + let names: Vec> = names.iter().map(|s| s.to_string_lossy()).collect(); // Since tab_size=0 means no \t, use Spaces separator for optimization. let filling = match tab_size { From 3c0e32fabcf25610d7f29deccf0163539d38d445 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 12:01:17 -0500 Subject: [PATCH 53/65] Cleanup --- src/uu/ls/src/colors.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index b700309a94d..6920318ef13 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -197,12 +197,7 @@ pub(crate) fn color_name( // use the optional target_symlink // Use fn symlink_metadata directly instead of get_metadata() here because ls // should not exit with an err, if we are unable to obtain the target_metadata - let md_option: Option = target - .metadata() - .cloned() - .or_else(|| path.p_buf.symlink_metadata().ok()); - - style_manager.apply_style_based_on_metadata(path, md_option.as_ref(), name, wrap) + style_manager.apply_style_based_on_colorable(target, name, wrap) } else { let md_option: Option = path .metadata() From 47756086b1c73fc98a7f777e4502c7e604035b4e Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 17:33:49 -0500 Subject: [PATCH 54/65] Remove dep for non-Windows platforms which causes additional statx calls --- src/uu/ls/src/ls.rs | 68 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 56 insertions(+), 12 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index ef1c5a39d9c..8fb6bf1eb51 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -12,6 +12,7 @@ use std::collections::HashMap; use std::os::unix::fs::{FileTypeExt, MetadataExt}; #[cfg(windows)] use std::os::windows::fs::MetadataExt; + use std::{ cell::{LazyCell, OnceCell}, cmp::Reverse, @@ -39,6 +40,8 @@ use thiserror::Error; #[cfg(unix)] use uucore::entries; +#[cfg(windows)] +use uucore::fs::FileInformation; #[cfg(all(unix, not(any(target_os = "android", target_os = "macos"))))] use uucore::fsxattr::has_acl; #[cfg(unix)] @@ -61,7 +64,6 @@ use uucore::{ error::{UError, UResult, set_exit_code}, format::human::{SizeFormat, human_readable}, format_usage, - fs::FileInformation, fs::display_permissions, fsext::{MetadataTimeField, metadata_get_time}, line_ending::LineEnding, @@ -1768,7 +1770,7 @@ pub fn uu_app() -> Command { /// 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)] +#[derive(Debug, Clone)] struct PathData { // Result got from symlink_metadata() or metadata() based on config md: OnceCell>, @@ -1923,6 +1925,54 @@ impl PathData { } } +impl std::hash::Hash for PathData { + fn hash(&self, state: &mut H) { + #[cfg(windows)] + { + return FileInformation::from_path(self.p_buf).hash(state); + } + #[cfg(not(windows))] + match self.metadata() { + Some(md) => { + md.ino().hash(state); + md.dev().hash(state); + } + None => match self.path().symlink_metadata() { + Ok(md) => { + md.ino().hash(state); + md.dev().hash(state); + } + Err(_) => { + self.path().hash(state); + } + }, + } + } +} + +impl PartialEq for PathData { + fn eq(&self, other: &Self) -> bool { + #[cfg(windows)] + { + return FileInformation::from_path(self.p_buf) + == FileInformation::from_path(other.p_buf); + } + #[cfg(not(windows))] + { + if let Some(self_md) = self.metadata() { + if let Some(other_md) = other.metadata() { + { + return self_md.ino() == other_md.ino() && self_md.dev() == other_md.dev(); + } + } + } + self.path() == other.path() + } + } +} + +impl Eq for PathData {} + /// Show the directory name in the case where several arguments are given to ls /// or the recursive flag is passed. /// @@ -2071,10 +2121,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { } } let mut listed_ancestors = HashSet::new(); - listed_ancestors.insert(FileInformation::from_path( - path_data.path(), - path_data.must_dereference, - )?); + listed_ancestors.insert(path_data.clone()); enter_directory( path_data, read_dir, @@ -2201,7 +2248,7 @@ fn enter_directory( read_dir: ReadDir, config: &Config, state: &mut ListState, - listed_ancestors: &mut HashSet, + listed_ancestors: &mut HashSet, dired: &mut DiredOutput, ) -> UResult<()> { // Create vec of entries with initial dot files @@ -2273,9 +2320,7 @@ fn enter_directory( )); } Ok(rd) => { - if listed_ancestors - .insert(FileInformation::from_path(e.path(), e.must_dereference)?) - { + if !listed_ancestors.contains(e) { // when listing several directories in recursive mode, we show // "dirname:" at the beginning of the file list writeln!(state.out)?; @@ -2294,8 +2339,7 @@ fn enter_directory( show_dir_name(e, &mut state.out, config)?; writeln!(state.out)?; enter_directory(e, rd, config, state, listed_ancestors, dired)?; - listed_ancestors - .remove(&FileInformation::from_path(e.path(), e.must_dereference)?); + listed_ancestors.insert(e.clone()); } else { state.out.flush()?; show!(LsError::AlreadyListedError(e.path().to_path_buf())); From 6bb52bbc041b4190984ccd7910f759be768f094f Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 17:40:36 -0500 Subject: [PATCH 55/65] Fix lints --- src/uu/ls/src/ls.rs | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 8fb6bf1eb51..5f2d7737b9b 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2120,7 +2120,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { writeln!(state.out)?; } } - let mut listed_ancestors = HashSet::new(); + let mut listed_ancestors: HashSet = HashSet::new(); listed_ancestors.insert(path_data.clone()); enter_directory( path_data, @@ -2318,32 +2318,34 @@ fn enter_directory( err, e.command_line )); + continue; } Ok(rd) => { if !listed_ancestors.contains(e) { - // when listing several directories in recursive mode, we show - // "dirname:" at the beginning of the file list - writeln!(state.out)?; - if config.dired { - // We already injected the first dir - // Continue with the others - // 2 = \n + \n - dired.padding = 2; - dired::indent(&mut state.out)?; - let dir_name_size = e.path().to_string_lossy().len(); - dired::calculate_subdired(dired, dir_name_size); - // inject dir name - dired::add_dir_name(dired, dir_name_size); - } - - show_dir_name(e, &mut state.out, config)?; - writeln!(state.out)?; - enter_directory(e, rd, config, state, listed_ancestors, dired)?; - listed_ancestors.insert(e.clone()); - } else { state.out.flush()?; show!(LsError::AlreadyListedError(e.path().to_path_buf())); + continue; } + + // when listing several directories in recursive mode, we show + // "dirname:" at the beginning of the file list + writeln!(state.out)?; + if config.dired { + // We already injected the first dir + // Continue with the others + // 2 = \n + \n + dired.padding = 2; + dired::indent(&mut state.out)?; + let dir_name_size = e.path().to_string_lossy().len(); + dired::calculate_subdired(dired, dir_name_size); + // inject dir name + dired::add_dir_name(dired, dir_name_size); + } + + show_dir_name(e, &mut state.out, config)?; + writeln!(state.out)?; + enter_directory(e, rd, config, state, listed_ancestors, dired)?; + listed_ancestors.insert(e.clone()); } } } From 77fa4751e6e7be3c4801f2dbf025b4eab198f606 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 17:50:22 -0500 Subject: [PATCH 56/65] Fix lints --- src/uu/ls/src/ls.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 5f2d7737b9b..4204d4695af 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2019,6 +2019,7 @@ struct ListState<'a> { } #[allow(clippy::cognitive_complexity)] +#[allow(clippy::mutable_key_type)] pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { let mut files = Vec::::new(); let mut dirs = Vec::::new(); @@ -2243,6 +2244,7 @@ fn should_display(entry: &DirEntry, config: &Config) -> bool { } #[allow(clippy::cognitive_complexity)] +#[allow(clippy::mutable_key_type)] fn enter_directory( path_data: &PathData, read_dir: ReadDir, From 20ffda347b5df86016e4f6e8bd1a846d7e25a19c Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 18:00:56 -0500 Subject: [PATCH 57/65] Fix 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 4204d4695af..fcb037eb6b0 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1929,7 +1929,11 @@ impl std::hash::Hash for PathData { fn hash(&self, state: &mut H) { #[cfg(windows)] { - return FileInformation::from_path(self.p_buf).hash(state); + if Ok(self_fi) = FileInformation::from_path(self.p_buf).hash(state) { + return self_fi.hash(state); + } + + self.path().hash(state); } #[cfg(not(windows))] match self.metadata() { @@ -1954,8 +1958,17 @@ impl PartialEq for PathData { fn eq(&self, other: &Self) -> bool { #[cfg(windows)] { - return FileInformation::from_path(self.p_buf) - == FileInformation::from_path(other.p_buf); + if let Ok(self_fi) = FileInformation::from_path(self.p_buf, self.must_dereference) { + if let Ok(other_fi) = + FileInformation::from_path(other.p_buf, other.must_dereference) + { + { + return self_fi == other_fi; + } + } + } + + self.path() == other.path() } #[cfg(not(windows))] { From dac83bdcd292640b857df2d3c1a6889492259afa Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 18:09:07 -0500 Subject: [PATCH 58/65] Fix lints --- 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 fcb037eb6b0..7ad15e28a7f 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1929,7 +1929,7 @@ impl std::hash::Hash for PathData { fn hash(&self, state: &mut H) { #[cfg(windows)] { - if Ok(self_fi) = FileInformation::from_path(self.p_buf).hash(state) { + if let Ok(self_fi) = FileInformation::from_path(self.p_buf).hash(state) { return self_fi.hash(state); } From 1eb4193b913bf71c8c034cb67a4d3604a0275fe8 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 18:16:19 -0500 Subject: [PATCH 59/65] Revert "Fix lints" This reverts commit dac83bdcd292640b857df2d3c1a6889492259afa. --- 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 7ad15e28a7f..fcb037eb6b0 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1929,7 +1929,7 @@ impl std::hash::Hash for PathData { fn hash(&self, state: &mut H) { #[cfg(windows)] { - if let Ok(self_fi) = FileInformation::from_path(self.p_buf).hash(state) { + if Ok(self_fi) = FileInformation::from_path(self.p_buf).hash(state) { return self_fi.hash(state); } From 99f0fed704c27713cd90bfe5614e0881c0c007c9 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 18:16:33 -0500 Subject: [PATCH 60/65] Revert "Fix lints" This reverts commit 20ffda347b5df86016e4f6e8bd1a846d7e25a19c. --- src/uu/ls/src/ls.rs | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index fcb037eb6b0..4204d4695af 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1929,11 +1929,7 @@ impl std::hash::Hash for PathData { fn hash(&self, state: &mut H) { #[cfg(windows)] { - if Ok(self_fi) = FileInformation::from_path(self.p_buf).hash(state) { - return self_fi.hash(state); - } - - self.path().hash(state); + return FileInformation::from_path(self.p_buf).hash(state); } #[cfg(not(windows))] match self.metadata() { @@ -1958,17 +1954,8 @@ impl PartialEq for PathData { fn eq(&self, other: &Self) -> bool { #[cfg(windows)] { - if let Ok(self_fi) = FileInformation::from_path(self.p_buf, self.must_dereference) { - if let Ok(other_fi) = - FileInformation::from_path(other.p_buf, other.must_dereference) - { - { - return self_fi == other_fi; - } - } - } - - self.path() == other.path() + return FileInformation::from_path(self.p_buf) + == FileInformation::from_path(other.p_buf); } #[cfg(not(windows))] { From d064a9f7a636715e33d4e41aef48ad9b7edc9283 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 18:16:38 -0500 Subject: [PATCH 61/65] Revert "Fix lints" This reverts commit 77fa4751e6e7be3c4801f2dbf025b4eab198f606. --- src/uu/ls/src/ls.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 4204d4695af..5f2d7737b9b 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2019,7 +2019,6 @@ struct ListState<'a> { } #[allow(clippy::cognitive_complexity)] -#[allow(clippy::mutable_key_type)] pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { let mut files = Vec::::new(); let mut dirs = Vec::::new(); @@ -2244,7 +2243,6 @@ fn should_display(entry: &DirEntry, config: &Config) -> bool { } #[allow(clippy::cognitive_complexity)] -#[allow(clippy::mutable_key_type)] fn enter_directory( path_data: &PathData, read_dir: ReadDir, From e9447a4f9051a541b9f8ce0033126f6bcc021ffd Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 18:16:42 -0500 Subject: [PATCH 62/65] Revert "Fix lints" This reverts commit 6bb52bbc041b4190984ccd7910f759be768f094f. --- src/uu/ls/src/ls.rs | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 5f2d7737b9b..8fb6bf1eb51 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -2120,7 +2120,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { writeln!(state.out)?; } } - let mut listed_ancestors: HashSet = HashSet::new(); + let mut listed_ancestors = HashSet::new(); listed_ancestors.insert(path_data.clone()); enter_directory( path_data, @@ -2318,34 +2318,32 @@ fn enter_directory( err, e.command_line )); - continue; } Ok(rd) => { if !listed_ancestors.contains(e) { + // when listing several directories in recursive mode, we show + // "dirname:" at the beginning of the file list + writeln!(state.out)?; + if config.dired { + // We already injected the first dir + // Continue with the others + // 2 = \n + \n + dired.padding = 2; + dired::indent(&mut state.out)?; + let dir_name_size = e.path().to_string_lossy().len(); + dired::calculate_subdired(dired, dir_name_size); + // inject dir name + dired::add_dir_name(dired, dir_name_size); + } + + show_dir_name(e, &mut state.out, config)?; + writeln!(state.out)?; + enter_directory(e, rd, config, state, listed_ancestors, dired)?; + listed_ancestors.insert(e.clone()); + } else { state.out.flush()?; show!(LsError::AlreadyListedError(e.path().to_path_buf())); - continue; } - - // when listing several directories in recursive mode, we show - // "dirname:" at the beginning of the file list - writeln!(state.out)?; - if config.dired { - // We already injected the first dir - // Continue with the others - // 2 = \n + \n - dired.padding = 2; - dired::indent(&mut state.out)?; - let dir_name_size = e.path().to_string_lossy().len(); - dired::calculate_subdired(dired, dir_name_size); - // inject dir name - dired::add_dir_name(dired, dir_name_size); - } - - show_dir_name(e, &mut state.out, config)?; - writeln!(state.out)?; - enter_directory(e, rd, config, state, listed_ancestors, dired)?; - listed_ancestors.insert(e.clone()); } } } From cb90d1d0954249e3c28ff7b0ebd8f35e3b299d53 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 18:16:48 -0500 Subject: [PATCH 63/65] Revert "Remove dep for non-Windows platforms which causes additional statx calls" This reverts commit 47756086b1c73fc98a7f777e4502c7e604035b4e. --- src/uu/ls/src/ls.rs | 68 ++++++++------------------------------------- 1 file changed, 12 insertions(+), 56 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 8fb6bf1eb51..ef1c5a39d9c 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -12,7 +12,6 @@ use std::collections::HashMap; use std::os::unix::fs::{FileTypeExt, MetadataExt}; #[cfg(windows)] use std::os::windows::fs::MetadataExt; - use std::{ cell::{LazyCell, OnceCell}, cmp::Reverse, @@ -40,8 +39,6 @@ use thiserror::Error; #[cfg(unix)] use uucore::entries; -#[cfg(windows)] -use uucore::fs::FileInformation; #[cfg(all(unix, not(any(target_os = "android", target_os = "macos"))))] use uucore::fsxattr::has_acl; #[cfg(unix)] @@ -64,6 +61,7 @@ use uucore::{ error::{UError, UResult, set_exit_code}, format::human::{SizeFormat, human_readable}, format_usage, + fs::FileInformation, fs::display_permissions, fsext::{MetadataTimeField, metadata_get_time}, line_ending::LineEnding, @@ -1770,7 +1768,7 @@ pub fn uu_app() -> Command { /// 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, Clone)] +#[derive(Debug)] struct PathData { // Result got from symlink_metadata() or metadata() based on config md: OnceCell>, @@ -1925,54 +1923,6 @@ impl PathData { } } -impl std::hash::Hash for PathData { - fn hash(&self, state: &mut H) { - #[cfg(windows)] - { - return FileInformation::from_path(self.p_buf).hash(state); - } - #[cfg(not(windows))] - match self.metadata() { - Some(md) => { - md.ino().hash(state); - md.dev().hash(state); - } - None => match self.path().symlink_metadata() { - Ok(md) => { - md.ino().hash(state); - md.dev().hash(state); - } - Err(_) => { - self.path().hash(state); - } - }, - } - } -} - -impl PartialEq for PathData { - fn eq(&self, other: &Self) -> bool { - #[cfg(windows)] - { - return FileInformation::from_path(self.p_buf) - == FileInformation::from_path(other.p_buf); - } - #[cfg(not(windows))] - { - if let Some(self_md) = self.metadata() { - if let Some(other_md) = other.metadata() { - { - return self_md.ino() == other_md.ino() && self_md.dev() == other_md.dev(); - } - } - } - self.path() == other.path() - } - } -} - -impl Eq for PathData {} - /// Show the directory name in the case where several arguments are given to ls /// or the recursive flag is passed. /// @@ -2121,7 +2071,10 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { } } let mut listed_ancestors = HashSet::new(); - listed_ancestors.insert(path_data.clone()); + listed_ancestors.insert(FileInformation::from_path( + path_data.path(), + path_data.must_dereference, + )?); enter_directory( path_data, read_dir, @@ -2248,7 +2201,7 @@ fn enter_directory( read_dir: ReadDir, config: &Config, state: &mut ListState, - listed_ancestors: &mut HashSet, + listed_ancestors: &mut HashSet, dired: &mut DiredOutput, ) -> UResult<()> { // Create vec of entries with initial dot files @@ -2320,7 +2273,9 @@ fn enter_directory( )); } Ok(rd) => { - if !listed_ancestors.contains(e) { + if listed_ancestors + .insert(FileInformation::from_path(e.path(), e.must_dereference)?) + { // when listing several directories in recursive mode, we show // "dirname:" at the beginning of the file list writeln!(state.out)?; @@ -2339,7 +2294,8 @@ fn enter_directory( show_dir_name(e, &mut state.out, config)?; writeln!(state.out)?; enter_directory(e, rd, config, state, listed_ancestors, dired)?; - listed_ancestors.insert(e.clone()); + listed_ancestors + .remove(&FileInformation::from_path(e.path(), e.must_dereference)?); } else { state.out.flush()?; show!(LsError::AlreadyListedError(e.path().to_path_buf())); From 90812622d108ee6a8ae919cf41006bbd063ecf68 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 18:50:33 -0500 Subject: [PATCH 64/65] Revert "Merge branch 'uutils:main' into create_fewer_small_strings" This reverts commit bc7e8785cad3d8f74ec27e5c96e7f9b0e79b920d, reversing changes made to 42c756803def160304fcf6f2a5240a16b3ead5a0. --- .github/workflows/CICD.yml | 2 +- .github/workflows/GnuTests.yml | 2 +- .../acronyms+names.wordlist.txt | 20 +-- Cargo.lock | 2 - src/uu/id/locales/en-US.ftl | 1 - src/uu/id/locales/fr-FR.ftl | 1 - src/uu/id/src/id.rs | 8 -- src/uu/numfmt/Cargo.toml | 9 -- src/uu/numfmt/benches/numfmt_bench.rs | 120 ---------------- src/uu/seq/src/seq.rs | 39 +++--- src/uu/stat/src/stat.rs | 132 ++---------------- tests/by-util/test_id.rs | 5 - tests/by-util/test_seq.rs | 46 ------ tests/by-util/test_stat.rs | 46 ------ util/build-gnu.sh | 16 +-- 15 files changed, 53 insertions(+), 396 deletions(-) delete mode 100644 src/uu/numfmt/benches/numfmt_bench.rs diff --git a/.github/workflows/CICD.yml b/.github/workflows/CICD.yml index de8fa5998aa..61e5eb4c068 100644 --- a/.github/workflows/CICD.yml +++ b/.github/workflows/CICD.yml @@ -1327,6 +1327,6 @@ jobs: echo "Running benchmarks for packages: ${{ steps.benchmark_list.outputs.benchmark_packages }}" for package in ${{ steps.benchmark_list.outputs.benchmark_packages }}; do echo "Running benchmarks for $package" - cargo codspeed run -p $package > /dev/null + cargo codspeed run -p $package done token: ${{ secrets.CODSPEED_TOKEN }} diff --git a/.github/workflows/GnuTests.yml b/.github/workflows/GnuTests.yml index dcac7eaf2bf..0a98a36914e 100644 --- a/.github/workflows/GnuTests.yml +++ b/.github/workflows/GnuTests.yml @@ -29,7 +29,7 @@ env: TEST_ROOT_FULL_SUMMARY_FILE: 'gnu-root-full-result.json' TEST_SELINUX_FULL_SUMMARY_FILE: 'selinux-gnu-full-result.json' TEST_SELINUX_ROOT_FULL_SUMMARY_FILE: 'selinux-root-gnu-full-result.json' - REPO_GNU_REF: "v9.8" + REPO_GNU_REF: "v9.7" jobs: native: diff --git a/.vscode/cspell.dictionaries/acronyms+names.wordlist.txt b/.vscode/cspell.dictionaries/acronyms+names.wordlist.txt index e611b5954df..2724d2b1fa2 100644 --- a/.vscode/cspell.dictionaries/acronyms+names.wordlist.txt +++ b/.vscode/cspell.dictionaries/acronyms+names.wordlist.txt @@ -1,5 +1,4 @@ # * abbreviations / acronyms -aarch AIX ASLR # address space layout randomization AST # abstract syntax tree @@ -10,27 +9,23 @@ DevOps Ext3 FIFO FIFOs -flac FQDN # fully qualified domain name GID # group ID GIDs GNU GNUEABI GNUEABIhf -impls JFS -loongarch -lzma MSRV # minimum supported rust version MSVC NixOS POSIX POSIXLY -ReiserFS RISC RISCV RNG # random number generator RNGs +ReiserFS Solaris UID # user ID UIDs @@ -38,6 +33,11 @@ UUID # universally unique identifier WASI WASM XFS +aarch +flac +impls +lzma +loongarch # * names BusyBox @@ -48,23 +48,25 @@ Deno EditorConfig EPEL FreeBSD -genric Gmail +GNU Illumos Irix libfuzzer +MS-DOS +MSDOS MacOS MinGW Minix -MS-DOS -MSDOS NetBSD Novell Nushell OpenBSD +POSIX PowerPC SELinux SkyPack +Solaris SysV Xenix Yargs diff --git a/Cargo.lock b/Cargo.lock index d2cf4d7af1d..e5f2b501c8e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3731,9 +3731,7 @@ name = "uu_numfmt" version = "0.2.2" dependencies = [ "clap", - "codspeed-divan-compat", "fluent", - "tempfile", "thiserror 2.0.16", "uucore", ] diff --git a/src/uu/id/locales/en-US.ftl b/src/uu/id/locales/en-US.ftl index a6b4ac2256f..37b477c17a8 100644 --- a/src/uu/id/locales/en-US.ftl +++ b/src/uu/id/locales/en-US.ftl @@ -25,7 +25,6 @@ id-error-cannot-find-user-name = cannot find name for user ID { $uid } id-error-audit-retrieve = couldn't retrieve information # Help text for command-line arguments -id-help-ignore = ignore, for compatibility with other versions id-help-audit = Display the process audit user ID and other process audit properties, which requires privilege (not available on Linux). id-help-user = Display only the effective user ID as a number. diff --git a/src/uu/id/locales/fr-FR.ftl b/src/uu/id/locales/fr-FR.ftl index b606f520757..f69df611e22 100644 --- a/src/uu/id/locales/fr-FR.ftl +++ b/src/uu/id/locales/fr-FR.ftl @@ -25,7 +25,6 @@ id-error-cannot-find-user-name = impossible de trouver le nom pour l'ID utilisat id-error-audit-retrieve = impossible de récupérer les informations # Texte d'aide pour les arguments de ligne de commande -id-help-ignore = ignore, pour compatibilité avec d'autres versions id-help-audit = Affiche l'ID utilisateur d'audit du processus et autres propriétés d'audit, ce qui nécessite des privilèges (non disponible sous Linux). id-help-user = Affiche uniquement l'ID utilisateur effectif sous forme de nombre. diff --git a/src/uu/id/src/id.rs b/src/uu/id/src/id.rs index dcdc692435d..7c1e1c12c5a 100644 --- a/src/uu/id/src/id.rs +++ b/src/uu/id/src/id.rs @@ -69,7 +69,6 @@ fn get_context_help_text() -> String { } mod options { - pub const OPT_IGNORE: &str = "ignore"; pub const OPT_AUDIT: &str = "audit"; // GNU's id does not have this pub const OPT_CONTEXT: &str = "context"; pub const OPT_EFFECTIVE_USER: &str = "user"; @@ -354,13 +353,6 @@ pub fn uu_app() -> Command { .infer_long_args(true) .args_override_self(true) .after_help(translate!("id-after-help")) - .arg( - Arg::new(options::OPT_IGNORE) - .short('a') - .long(options::OPT_IGNORE) - .help(translate!("id-help-ignore")) - .action(ArgAction::SetTrue), - ) .arg( Arg::new(options::OPT_AUDIT) .short('A') diff --git a/src/uu/numfmt/Cargo.toml b/src/uu/numfmt/Cargo.toml index 177f2e3b8ac..5cfd2dc4697 100644 --- a/src/uu/numfmt/Cargo.toml +++ b/src/uu/numfmt/Cargo.toml @@ -23,15 +23,6 @@ uucore = { workspace = true, features = ["parser", "ranges"] } thiserror = { workspace = true } fluent = { workspace = true } -[dev-dependencies] -divan = { workspace = true } -tempfile = { workspace = true } -uucore = { workspace = true, features = ["benchmark"] } - [[bin]] name = "numfmt" path = "src/main.rs" - -[[bench]] -name = "numfmt_bench" -harness = false diff --git a/src/uu/numfmt/benches/numfmt_bench.rs b/src/uu/numfmt/benches/numfmt_bench.rs deleted file mode 100644 index ee09b7d0fc7..00000000000 --- a/src/uu/numfmt/benches/numfmt_bench.rs +++ /dev/null @@ -1,120 +0,0 @@ -// This file is part of the uutils coreutils package. -// -// For the full copyright and license information, please view the LICENSE -// file that was distributed with this source code. - -use divan::{Bencher, black_box}; -use tempfile::TempDir; -use uu_numfmt::uumain; -use uucore::benchmark::{create_test_file, run_util_function}; - -/// Generate numeric data for benchmarking -fn generate_numbers(count: usize) -> String { - (1..=count) - .map(|n| n.to_string()) - .collect::>() - .join("\n") -} - -/// Setup benchmark environment with test data -fn setup_benchmark(data: String) -> (TempDir, String) { - let temp_dir = tempfile::tempdir().unwrap(); - let file_path = create_test_file(data.as_bytes(), temp_dir.path()); - let file_path_str = file_path.to_str().unwrap().to_string(); - (temp_dir, file_path_str) -} - -/// Benchmark SI formatting with different number counts -#[divan::bench(args = [1_000_000])] -fn numfmt_to_si(bencher: Bencher, count: usize) { - let (_temp_dir, file_path_str) = setup_benchmark(generate_numbers(count)); - - bencher.bench(|| { - black_box(run_util_function(uumain, &["--to=si", &file_path_str])); - }); -} - -/// Benchmark SI formatting with precision format -#[divan::bench(args = [1_000_000])] -fn numfmt_to_si_precision(bencher: Bencher, count: usize) { - let (_temp_dir, file_path_str) = setup_benchmark(generate_numbers(count)); - - bencher.bench(|| { - black_box(run_util_function( - uumain, - &["--to=si", "--format=%.6f", &file_path_str], - )); - }); -} - -/// Benchmark IEC (binary) formatting -#[divan::bench(args = [1_000_000])] -fn numfmt_to_iec(bencher: Bencher, count: usize) { - let (_temp_dir, file_path_str) = setup_benchmark(generate_numbers(count)); - - bencher.bench(|| { - black_box(run_util_function(uumain, &["--to=iec", &file_path_str])); - }); -} - -/// Benchmark parsing from SI format back to raw numbers -#[divan::bench(args = [1_000_000])] -fn numfmt_from_si(bencher: Bencher, count: usize) { - // Generate SI formatted data (e.g., "1.0K", "2.0K", etc.) - let data = (1..=count) - .map(|n| format!("{:.1}K", n as f64 / 1000.0)) - .collect::>() - .join("\n"); - let (_temp_dir, file_path_str) = setup_benchmark(data); - - bencher.bench(|| { - black_box(run_util_function(uumain, &["--from=si", &file_path_str])); - }); -} - -/// Benchmark large numbers with SI formatting -#[divan::bench(args = [1_000_000])] -fn numfmt_large_numbers_si(bencher: Bencher, count: usize) { - // Generate larger numbers (millions to billions range) - let data = (1..=count) - .map(|n| (n * 1_000_000).to_string()) - .collect::>() - .join("\n"); - let (_temp_dir, file_path_str) = setup_benchmark(data); - - bencher.bench(|| { - black_box(run_util_function(uumain, &["--to=si", &file_path_str])); - }); -} - -/// Benchmark different padding widths -#[divan::bench(args = [(1_000_000, 5), (1_000_000, 50)])] -fn numfmt_padding(bencher: Bencher, (count, padding): (usize, usize)) { - let (_temp_dir, file_path_str) = setup_benchmark(generate_numbers(count)); - let padding_arg = format!("--padding={padding}"); - - bencher.bench(|| { - black_box(run_util_function( - uumain, - &["--to=si", &padding_arg, &file_path_str], - )); - }); -} - -/// Benchmark round modes with SI formatting -#[divan::bench(args = [("up", 100_000), ("down", 1_000_000), ("towards-zero", 1_000_000)])] -fn numfmt_round_modes(bencher: Bencher, (round_mode, count): (&str, usize)) { - let (_temp_dir, file_path_str) = setup_benchmark(generate_numbers(count)); - let round_arg = format!("--round={round_mode}"); - - bencher.bench(|| { - black_box(run_util_function( - uumain, - &["--to=si", &round_arg, &file_path_str], - )); - }); -} - -fn main() { - divan::main(); -} diff --git a/src/uu/seq/src/seq.rs b/src/uu/seq/src/seq.rs index 4c050c2c735..a489e54b9a9 100644 --- a/src/uu/seq/src/seq.rs +++ b/src/uu/seq/src/seq.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. // spell-checker:ignore (ToDO) bigdecimal extendedbigdecimal numberparse hexadecimalfloat biguint -use std::ffi::{OsStr, OsString}; +use std::ffi::OsString; use std::io::{BufWriter, ErrorKind, Write, stdout}; use clap::{Arg, ArgAction, Command}; @@ -39,8 +39,8 @@ const ARG_NUMBERS: &str = "numbers"; #[derive(Clone)] struct SeqOptions<'a> { - separator: OsString, - terminator: OsString, + separator: String, + terminator: String, equal_width: bool, format: Option<&'a str>, } @@ -105,11 +105,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let options = SeqOptions { separator: matches - .get_one::(OPT_SEPARATOR) - .map_or(OsString::from("\n"), |s| s.to_os_string()), + .get_one::(OPT_SEPARATOR) + .map_or("\n", |s| s.as_str()) + .to_string(), terminator: matches - .get_one::(OPT_TERMINATOR) - .map_or(OsString::from("\n"), |s| s.to_os_string()), + .get_one::(OPT_TERMINATOR) + .map(|s| s.as_str()) + .unwrap_or("\n") + .to_string(), equal_width: matches.get_flag(OPT_EQUAL_WIDTH), format: matches.get_one::(OPT_FORMAT).map(|s| s.as_str()), }; @@ -226,15 +229,13 @@ pub fn uu_app() -> Command { Arg::new(OPT_SEPARATOR) .short('s') .long("separator") - .help(translate!("seq-help-separator")) - .value_parser(clap::value_parser!(OsString)), + .help(translate!("seq-help-separator")), ) .arg( Arg::new(OPT_TERMINATOR) .short('t') .long("terminator") - .help(translate!("seq-help-terminator")) - .value_parser(clap::value_parser!(OsString)), + .help(translate!("seq-help-terminator")), ) .arg( Arg::new(OPT_EQUAL_WIDTH) @@ -266,8 +267,8 @@ fn fast_print_seq( first: &BigUint, increment: u64, last: &BigUint, - separator: &OsStr, - terminator: &OsStr, + separator: &str, + terminator: &str, padding: usize, ) -> std::io::Result<()> { // Nothing to do, just return. @@ -304,7 +305,7 @@ fn fast_print_seq( // Initialize buf with first and separator. buf[start..num_end].copy_from_slice(first_str.as_bytes()); - buf[num_end..].copy_from_slice(separator.as_encoded_bytes()); + buf[num_end..].copy_from_slice(separator.as_bytes()); // Normally, if padding is > 0, it should be equal to last_length, // so start would be == 0, but there are corner cases. @@ -320,7 +321,7 @@ fn fast_print_seq( } // Write the last number without separator, but with terminator. stdout.write_all(&buf[start..num_end])?; - stdout.write_all(terminator.as_encoded_bytes())?; + write!(stdout, "{terminator}")?; stdout.flush()?; Ok(()) } @@ -336,8 +337,8 @@ fn done_printing(next: &T, increment: &T, last: &T) -> boo /// Arbitrary precision decimal number code path ("slow" path) fn print_seq( range: RangeFloat, - separator: &OsStr, - terminator: &OsStr, + separator: &str, + terminator: &str, format: &Format, fast_allowed: bool, padding: usize, // Used by fast path only @@ -374,7 +375,7 @@ fn print_seq( let mut is_first_iteration = true; while !done_printing(&value, &increment, &last) { if !is_first_iteration { - stdout.write_all(separator.as_encoded_bytes())?; + stdout.write_all(separator.as_bytes())?; } format.fmt(&mut stdout, &value)?; // TODO Implement augmenting addition. @@ -382,7 +383,7 @@ fn print_seq( is_first_iteration = false; } if !is_first_iteration { - stdout.write_all(terminator.as_encoded_bytes())?; + stdout.write_all(terminator.as_bytes())?; } stdout.flush()?; Ok(()) diff --git a/src/uu/stat/src/stat.rs b/src/uu/stat/src/stat.rs index 5f9e8841793..797e6971be7 100644 --- a/src/uu/stat/src/stat.rs +++ b/src/uu/stat/src/stat.rs @@ -111,56 +111,9 @@ fn pad_and_print(result: &str, left: bool, width: usize, padding: Padding) { } } -/// Pads and prints raw bytes (Unix-specific) or falls back to string printing -/// -/// On Unix systems, this preserves non-UTF8 data by printing raw bytes -/// On other platforms, falls back to lossy string conversion -fn pad_and_print_bytes( - mut writer: W, - bytes: &[u8], - left: bool, - width: usize, - precision: Precision, -) -> Result<(), std::io::Error> { - let display_bytes = match precision { - Precision::Number(p) if p < bytes.len() => &bytes[..p], - _ => bytes, - }; - - let display_len = display_bytes.len(); - let padding_needed = width.saturating_sub(display_len); - - let (left_pad, right_pad) = if left { - (0, padding_needed) - } else { - (padding_needed, 0) - }; - - if left_pad > 0 { - print_padding(&mut writer, left_pad)?; - } - writer.write_all(display_bytes)?; - if right_pad > 0 { - print_padding(&mut writer, right_pad)?; - } - - Ok(()) -} - -/// print padding based on a writer W and n size -/// writer is genric to be any buffer like: `std::io::stdout` -/// n is the calculated padding size -fn print_padding(writer: &mut W, n: usize) -> Result<(), std::io::Error> { - for _ in 0..n { - writer.write_all(b" ")?; - } - Ok(()) -} - #[derive(Debug)] -pub enum OutputType<'a> { +pub enum OutputType { Str(String), - OsStr(&'a OsString), Integer(i64), Unsigned(u64), UnsignedHex(u64), @@ -353,7 +306,6 @@ fn print_it(output: &OutputType, flags: Flags, width: usize, precision: Precisio match output { OutputType::Str(s) => print_str(s, &flags, width, precision), - OutputType::OsStr(s) => print_os_str(s, &flags, width, precision), OutputType::Integer(num) => print_integer(*num, &flags, width, precision, padding_char), OutputType::Unsigned(num) => print_unsigned(*num, &flags, width, precision, padding_char), OutputType::UnsignedOct(num) => { @@ -402,37 +354,6 @@ fn print_str(s: &str, flags: &Flags, width: usize, precision: Precision) { pad_and_print(s, flags.left, width, Padding::Space); } -/// Prints a `OsString` value based on the provided flags, width, and precision. -/// for unix it converts it to bytes then tries to print it if failed print the lossy string version -/// for windows, `OsString` uses UTF-16 internally which doesn't map directly to bytes like Unix, -/// so we fall back to lossy string conversion to handle invalid UTF-8 sequences gracefully -/// -/// # Arguments -/// -/// * `s` - The `OsString` to be printed. -/// * `flags` - A reference to the Flags struct containing formatting flags. -/// * `width` - The width of the field for the printed string. -/// * `precision` - How many digits of precision, if any. -fn print_os_str(s: &OsString, flags: &Flags, width: usize, precision: Precision) { - #[cfg(unix)] - { - use std::os::unix::ffi::OsStrExt; - - let bytes = s.as_bytes(); - - if pad_and_print_bytes(std::io::stdout(), bytes, flags.left, width, precision).is_err() { - // if an error occurred while trying to print bytes fall back to normal lossy string so it can be printed - let fallback_string = s.to_string_lossy(); - print_str(&fallback_string, flags, width, precision); - } - } - #[cfg(not(unix))] - { - let lossy_string = s.to_string_lossy(); - print_str(&lossy_string, flags, width, precision); - } -} - fn quote_file_name(file_name: &str, quoting_style: &QuotingStyle) -> String { match quoting_style { QuotingStyle::Locale | QuotingStyle::Shell => { @@ -969,12 +890,16 @@ impl Stater { }) } - fn find_mount_point>(&self, p: P) -> Option<&OsString> { + fn find_mount_point>(&self, p: P) -> Option { let path = p.as_ref().canonicalize().ok()?; - self.mount_list - .as_ref()? - .iter() - .find(|root| path.starts_with(root)) + + for root in self.mount_list.as_ref()? { + if path.starts_with(root) { + // TODO: This is probably wrong, we should pass the OsString + return Some(root.to_string_lossy().into_owned()); + } + } + None } fn exec(&self) -> i32 { @@ -1068,11 +993,8 @@ impl Stater { 'h' => OutputType::Unsigned(meta.nlink()), // inode number 'i' => OutputType::Unsigned(meta.ino()), - // mount point - 'm' => match self.find_mount_point(file) { - Some(s) => OutputType::OsStr(s), - None => OutputType::Str(String::new()), - }, + // mount point: TODO: This should be an OsStr + 'm' => OutputType::Str(self.find_mount_point(file).unwrap()), // file name 'n' => OutputType::Str(display_name.to_string()), // quoted file name with dereference if symbolic link @@ -1378,8 +1300,6 @@ fn pretty_time(meta: &Metadata, md_time_field: MetadataTimeField) -> String { #[cfg(test)] mod tests { - use crate::{pad_and_print_bytes, print_padding}; - use super::{Flags, Precision, ScanUtil, Stater, Token, group_num, precision_trunc}; #[test] @@ -1501,32 +1421,4 @@ mod tests { assert_eq!(precision_trunc(123.456, Precision::Number(4)), "123.4560"); assert_eq!(precision_trunc(123.456, Precision::Number(5)), "123.45600"); } - - #[test] - fn test_pad_and_print_bytes() { - // testing non-utf8 with normal settings - let mut buffer = Vec::new(); - let bytes = b"\x80\xFF\x80"; - pad_and_print_bytes(&mut buffer, bytes, false, 3, Precision::NotSpecified).unwrap(); - assert_eq!(&buffer, b"\x80\xFF\x80"); - - // testing left padding - let mut buffer = Vec::new(); - let bytes = b"\x80\xFF\x80"; - pad_and_print_bytes(&mut buffer, bytes, false, 5, Precision::NotSpecified).unwrap(); - assert_eq!(&buffer, b" \x80\xFF\x80"); - - // testing right padding - let mut buffer = Vec::new(); - let bytes = b"\x80\xFF\x80"; - pad_and_print_bytes(&mut buffer, bytes, true, 5, Precision::NotSpecified).unwrap(); - assert_eq!(&buffer, b"\x80\xFF\x80 "); - } - - #[test] - fn test_print_padding() { - let mut buffer = Vec::new(); - print_padding(&mut buffer, 5).unwrap(); - assert_eq!(&buffer, b" "); - } } diff --git a/tests/by-util/test_id.rs b/tests/by-util/test_id.rs index 169fbe2abcf..7a7d5e9a169 100644 --- a/tests/by-util/test_id.rs +++ b/tests/by-util/test_id.rs @@ -17,11 +17,6 @@ fn test_invalid_arg() { new_ucmd!().arg("--definitely-invalid").fails_with_code(1); } -#[test] -fn test_id_ignore() { - new_ucmd!().arg("-a").succeeds(); -} - #[test] #[allow(unused_mut)] fn test_id_no_specified_user() { diff --git a/tests/by-util/test_seq.rs b/tests/by-util/test_seq.rs index f82a6228fe1..a4f49ea4149 100644 --- a/tests/by-util/test_seq.rs +++ b/tests/by-util/test_seq.rs @@ -228,52 +228,6 @@ fn test_separator_and_terminator() { .stdout_is("2\\n3\\n4\\n5\\n6\n"); } -#[test] -#[cfg(target_os = "linux")] -fn test_separator_non_utf8() { - use std::{ffi::OsString, os::unix::ffi::OsStringExt}; - - fn create_arg(prefix: &[u8]) -> OsString { - let separator = [0xFF, 0xFE]; - OsString::from_vec([prefix, &separator].concat()) - } - - let short = create_arg(b"-s"); - let long = create_arg(b"--separator="); - let expected = [b'1', 0xFF, 0xFE, b'2', b'\n']; - - for arg in [short, long] { - new_ucmd!() - .arg(&arg) - .arg("2") - .succeeds() - .stdout_is_bytes(expected); - } -} - -#[test] -#[cfg(target_os = "linux")] -fn test_terminator_non_utf8() { - use std::{ffi::OsString, os::unix::ffi::OsStringExt}; - - fn create_arg(prefix: &[u8]) -> OsString { - let terminator = [0xFF, 0xFE]; - OsString::from_vec([prefix, &terminator].concat()) - } - - let short = create_arg(b"-t"); - let long = create_arg(b"--terminator="); - let expected = [b'1', b'\n', b'2', 0xFF, 0xFE]; - - for arg in [short, long] { - new_ucmd!() - .arg(&arg) - .arg("2") - .succeeds() - .stdout_is_bytes(expected); - } -} - #[test] fn test_equalize_widths() { let args = ["-w", "--equal-width"]; diff --git a/tests/by-util/test_stat.rs b/tests/by-util/test_stat.rs index aeabf88fd43..6c4258189bd 100644 --- a/tests/by-util/test_stat.rs +++ b/tests/by-util/test_stat.rs @@ -514,49 +514,3 @@ fn test_stat_selinux() { let s: Vec<_> = result.stdout_str().split(':').collect(); assert!(s.len() == 4); } - -#[cfg(unix)] -#[test] -fn test_mount_point_basic() { - let ts = TestScenario::new(util_name!()); - let result = ts.ucmd().args(&["-c", "%m", "/"]).succeeds(); - let output = result.stdout_str().trim(); - assert!(!output.is_empty(), "Mount point should not be empty"); - assert_eq!(output, "/"); -} - -#[cfg(unix)] -#[test] -fn test_mount_point_width_and_alignment() { - let ts = TestScenario::new(util_name!()); - - // Right-aligned, width 15 - let result = ts.ucmd().args(&["-c", "%15m", "/"]).succeeds(); - let output = result.stdout_str(); - assert!( - output.trim().len() <= 15 && output.len() >= 15, - "Output should be padded to width 15" - ); - - // Left-aligned, width 15 - let result = ts.ucmd().args(&["-c", "%-15m", "/"]).succeeds(); - let output = result.stdout_str(); - - assert!( - output.trim().len() <= 15 && output.len() >= 15, - "Output should be padded to width 15 (left-aligned)" - ); -} - -#[cfg(unix)] -#[test] -fn test_mount_point_combined_with_other_specifiers() { - let ts = TestScenario::new(util_name!()); - let result = ts.ucmd().args(&["-c", "%m %n %s", "/bin/sh"]).succeeds(); - let output = result.stdout_str(); - let parts: Vec<&str> = output.split_whitespace().collect(); - assert!( - parts.len() >= 3, - "Should print mount point, file name, and size" - ); -} diff --git a/util/build-gnu.sh b/util/build-gnu.sh index db58321414d..4da34b4b324 100755 --- a/util/build-gnu.sh +++ b/util/build-gnu.sh @@ -70,18 +70,18 @@ fi ### -release_tag_GNU="v9.8" +release_tag_GNU="v9.7" # check if the GNU coreutils has been cloned, if not print instructions # note: the ${path_GNU} might already exist, so we check for the .git directory if test ! -d "${path_GNU}/.git"; then - echo "Could not find the GNU coreutils (expected at '${path_GNU}')" - echo "Download them to the expected path:" - echo " git clone --recurse-submodules https://github.com/coreutils/coreutils.git \"${path_GNU}\"" - echo "Afterwards, checkout the latest release tag:" - echo " cd \"${path_GNU}\"" - echo " git fetch --all --tags" - echo " git checkout tags/${release_tag_GNU}" + echo "Could not find GNU coreutils (expected at '${path_GNU}')" + echo "Run the following to download into the expected path:" + echo "git clone --recurse-submodules https://github.com/coreutils/coreutils.git \"${path_GNU}\"" + echo "After downloading GNU coreutils to \"${path_GNU}\" run the following commands to checkout latest release tag" + echo "cd \"${path_GNU}\"" + echo "git fetch --all --tags" + echo "git checkout tags/${release_tag_GNU}" exit 1 fi From 3f0fe766458db1b48c7aa0f717979d9b6593820c Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+kimono-koans@users.noreply.github.com> Date: Wed, 24 Sep 2025 23:03:55 -0500 Subject: [PATCH 65/65] Revert "Merge branch 'create_fewer_small_strings' of https://github.com/kimono-koans/coreutils into create_fewer_small_strings" This reverts commit 905173e1e078d8537dce4d2eaeee0d6a99fb05ff, reversing changes made to 90812622d108ee6a8ae919cf41006bbd063ecf68. --- src/uu/ls/src/colors.rs | 52 +++--- src/uu/ls/src/ls.rs | 346 ++++++++++++++++++++-------------------- 2 files changed, 195 insertions(+), 203 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 6920318ef13..4affbbf8ca2 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -3,24 +3,11 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. use super::PathData; -use lscolors::{Colorable, Indicator, LsColors, Style}; +use super::get_metadata_with_deref_opt; +use lscolors::{Indicator, LsColors, Style}; use std::ffi::OsString; -use std::fs::Metadata; - -impl Colorable for PathData { - fn file_name(&self) -> OsString { - self.display_name().to_os_string() - } - fn file_type(&self) -> Option { - self.file_type().copied() - } - fn metadata(&self) -> Option { - self.metadata().cloned() - } - fn path(&self) -> std::path::PathBuf { - self.path().to_path_buf() - } -} +use std::fs::{DirEntry, Metadata}; +use std::io::{BufWriter, Stdout}; /// We need this struct to be able to store the previous style. /// This because we need to check the previous value in case we don't need @@ -148,22 +135,25 @@ impl<'a> StyleManager<'a> { self.apply_style(style, name, wrap) } - pub(crate) fn apply_style_based_on_colorable( + pub(crate) fn apply_style_based_on_dir_entry( &mut self, - path: &T, + dir_entry: &DirEntry, name: OsString, wrap: bool, ) -> OsString { - let style = self.colors.style_for(path); + let style = self.colors.style_for(dir_entry); self.apply_style(style, name, wrap) } } /// Colors the provided name based on the style determined for the given path +/// This function is quite long because it tries to leverage [`DirEntry`] to avoid +/// unnecessary calls to stat and manages the symlink errors pub(crate) fn color_name( name: OsString, path: &PathData, style_manager: &mut StyleManager, + out: &mut BufWriter, target_symlink: Option<&PathData>, wrap: bool, ) -> OsString { @@ -189,21 +179,23 @@ pub(crate) fn color_name( if !path.must_dereference { // If we need to dereference (follow) a symlink, we will need to get the metadata - // There is a DirEntry, we don't need to get the metadata for the color - return style_manager.apply_style_based_on_colorable(path, name, wrap); + if let Some(de) = &path.de { + // There is a DirEntry, we don't need to get the metadata for the color + return style_manager.apply_style_based_on_dir_entry(de, name, wrap); + } } if let Some(target) = target_symlink { // use the optional target_symlink - // Use fn symlink_metadata directly instead of get_metadata() here because ls + // Use fn get_metadata_with_deref_opt instead of get_metadata() here because ls // should not exit with an err, if we are unable to obtain the target_metadata - style_manager.apply_style_based_on_colorable(target, name, wrap) + let md_res = get_metadata_with_deref_opt(&target.p_buf, path.must_dereference); + let md = md_res.or_else(|_| path.p_buf.symlink_metadata()); + style_manager.apply_style_based_on_metadata(path, md.ok().as_ref(), name, wrap) } else { - let md_option: Option = path - .metadata() - .cloned() - .or_else(|| path.p_buf.symlink_metadata().ok()); - - style_manager.apply_style_based_on_metadata(path, md_option.as_ref(), name, wrap) + let md_option = path.get_metadata(out); + let symlink_metadata = path.p_buf.symlink_metadata().ok(); + let md = md_option.or(symlink_metadata.as_ref()); + style_manager.apply_style_based_on_metadata(path, md, name, wrap) } } diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 3f4e76886fb..a8ac3227463 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1773,19 +1773,22 @@ struct PathData { // Result got from symlink_metadata() or metadata() based on config md: OnceCell>, ft: OnceCell>, - security_context: Box, + // can be used to avoid reading the metadata. Can be also called d_type: + // https://www.gnu.org/software/libc/manual/html_node/Directory-Entries.html + de: Option, // Name of the file - will be empty for . or .. display_name: OsString, // PathBuf that all above data corresponds to p_buf: PathBuf, must_dereference: bool, + security_context: String, command_line: bool, } impl PathData { fn new( p_buf: PathBuf, - dir_entry: Option, + dir_entry: Option>, file_name: Option, config: &Config, command_line: bool, @@ -1795,15 +1798,13 @@ impl PathData { let display_name = if let Some(name) = file_name { name } else if command_line { - p_buf.as_os_str().to_os_string() + p_buf.clone().into() } else { - dir_entry - .as_ref() - .map(|de| de.file_name()) - .or_else(|| p_buf.file_name().map(|inner| inner.to_os_string())) - .unwrap_or_default() + p_buf + .file_name() + .unwrap_or_else(|| p_buf.iter().next_back().unwrap()) + .to_owned() }; - let must_dereference = match &config.dereference { Dereference::All => true, Dereference::Args => command_line, @@ -1821,67 +1822,78 @@ impl PathData { Dereference::None => false, }; + let de: Option = match dir_entry { + Some(de) => de.ok(), + None => None, + }; + // Why prefer to check the DirEntry file_type()? B/c the call is // nearly free compared to a metadata() call on a Path - let md = match dir_entry.as_ref() { - Some(de) if !must_dereference => { - // check if we can use DirEntry metadata - // it will avoid a call to stat() - if let Ok(md) = de.metadata() { - OnceCell::from(Some(md)) - } else { - OnceCell::new() + fn get_file_type( + de: &DirEntry, + p_buf: &Path, + must_dereference: bool, + ) -> OnceCell> { + if must_dereference { + if let Ok(md_pb) = p_buf.metadata() { + return OnceCell::from(Some(md_pb.file_type())); } } - _ => OnceCell::new(), - }; - - let ft = match dir_entry.as_ref() { - Some(de) if !must_dereference => { - if let Ok(ft) = de.file_type() { - OnceCell::from(Some(ft)) - } else { - OnceCell::new() - } + if let Ok(ft_de) = de.file_type() { + OnceCell::from(Some(ft_de)) + } else if let Ok(md_pb) = p_buf.symlink_metadata() { + OnceCell::from(Some(md_pb.file_type())) + } else { + OnceCell::new() } - _ => OnceCell::new(), + } + let ft = match de { + Some(ref de) => get_file_type(de, &p_buf, must_dereference), + None => OnceCell::new(), }; - let security_context: Box = - get_security_context(&p_buf, must_dereference, config).into(); + let security_context = get_security_context(config, &p_buf, must_dereference); Self { - md, + md: OnceCell::new(), ft, - security_context, + de, display_name, p_buf, must_dereference, + security_context, command_line, } } - fn metadata(&self) -> Option<&Metadata> { + fn get_metadata(&self, out: &mut BufWriter) -> Option<&Metadata> { self.md .get_or_init(|| { + // check if we can use DirEntry metadata + // it will avoid a call to stat() + if !self.must_dereference { + if let Some(dir_entry) = &self.de { + return dir_entry.metadata().ok(); + } + } + // if not, check if we can use Path metadata - match get_metadata_with_deref_opt(self.path(), self.must_dereference) { + match get_metadata_with_deref_opt(self.p_buf.as_path(), self.must_dereference) { Err(err) => { // FIXME: A bit tricky to propagate the result here - let mut out = stdout().lock(); - let _ = out.flush(); + out.flush().unwrap(); let errno = err.raw_os_error().unwrap_or(1i32); // a bad fd will throw an error when dereferenced, // but GNU will not throw an error until a bad fd "dir" // is entered, here we match that GNU behavior, by handing // back the non-dereferenced metadata upon an EBADF if self.must_dereference && errno == 9i32 { - if let Ok(file) = self.path().read_link() { - return file.symlink_metadata().ok(); + if let Some(dir_entry) = &self.de { + return dir_entry.metadata().ok(); } } show!(LsError::IOErrorContext( - self.path().to_path_buf(), + self.p_buf.clone(), err, self.command_line )); @@ -1893,34 +1905,11 @@ impl PathData { .as_ref() } - fn file_type(&self) -> Option<&FileType> { + fn file_type(&self, out: &mut BufWriter) -> Option<&FileType> { self.ft - .get_or_init(|| self.metadata().map(|md| md.file_type())) + .get_or_init(|| self.get_metadata(out).map(|md| md.file_type())) .as_ref() } - - fn is_dangling_link(&self) -> bool { - // deref enabled, self is real dir entry, self has metadata associated with link, but not with target - self.must_dereference && self.file_type().is_none() && self.metadata().is_none() - } - - #[cfg(unix)] - fn is_executable_file(&self) -> bool { - self.file_type().is_some_and(|f| f.is_file()) - && self.metadata().is_some_and(file_is_executable) - } - - fn security_context(&self) -> &str { - &self.security_context - } - - fn path(&self) -> &Path { - &self.p_buf - } - - fn display_name(&self) -> &OsStr { - &self.display_name - } } /// Show the directory name in the case where several arguments are given to ls @@ -1940,7 +1929,7 @@ fn show_dir_name( config: &Config, ) -> std::io::Result<()> { let escaped_name = - locale_aware_escape_dir_name(path_data.path().as_os_str(), config.quoting_style); + locale_aware_escape_dir_name(path_data.p_buf.as_os_str(), config.quoting_style); let name = if config.hyperlink && !config.dired { create_hyperlink(&escaped_name, path_data) @@ -1998,11 +1987,11 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { // Proper GNU handling is don't show if dereferenced symlink DNE // but only for the base dir, for a child dir show, and print ?s // in long format - if path_data.metadata().is_none() { + if path_data.get_metadata(&mut state.out).is_none() { continue; } - let show_dir_contents = match path_data.file_type() { + let show_dir_contents = match path_data.file_type(&mut state.out) { Some(ft) => !config.directory && ft.is_dir(), None => { set_exit_code(1); @@ -2017,8 +2006,8 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { } } - sort_entries(&mut files, config); - sort_entries(&mut dirs, config); + sort_entries(&mut files, config, &mut state.out); + sort_entries(&mut dirs, config, &mut state.out); if let Some(style_manager) = state.style_manager.as_mut() { // ls will try to write a reset before anything is written if normal @@ -2034,12 +2023,12 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { for (pos, path_data) in dirs.iter().enumerate() { // Do read_dir call here to match GNU semantics by printing // read_dir errors before directory headings, names and totals - let read_dir = match fs::read_dir(path_data.path()) { + let read_dir = match fs::read_dir(&path_data.p_buf) { Err(err) => { // flush stdout buffer before the error to preserve formatting and order state.out.flush()?; show!(LsError::IOErrorContext( - path_data.path().to_path_buf(), + path_data.p_buf.clone(), err, path_data.command_line )); @@ -2058,7 +2047,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { writeln!(state.out)?; if config.dired { // First directory displayed - let dir_len = path_data.display_name().len(); + let dir_len = path_data.display_name.len(); // add the //SUBDIRED// coordinates dired::calculate_subdired(&mut dired, dir_len); // Add the padding for the dir name @@ -2072,7 +2061,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { } let mut listed_ancestors = HashSet::new(); listed_ancestors.insert(FileInformation::from_path( - path_data.path(), + &path_data.p_buf, path_data.must_dereference, )?); enter_directory( @@ -2090,38 +2079,38 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { Ok(()) } -fn sort_entries(entries: &mut [PathData], config: &Config) { +fn sort_entries(entries: &mut [PathData], config: &Config, out: &mut BufWriter) { match config.sort { Sort::Time => entries.sort_by_key(|k| { Reverse( - k.metadata() + k.get_metadata(out) .and_then(|md| metadata_get_time(md, config.time)) .unwrap_or(UNIX_EPOCH), ) }), Sort::Size => { - entries.sort_by_key(|k| Reverse(k.metadata().map_or(0, |md| md.len()))); + entries.sort_by_key(|k| Reverse(k.get_metadata(out).map_or(0, |md| md.len()))); } // The default sort in GNU ls is case insensitive - Sort::Name => entries.sort_by(|a, b| a.display_name().cmp(b.display_name())), + Sort::Name => entries.sort_by(|a, b| a.display_name.cmp(&b.display_name)), Sort::Version => entries.sort_by(|a, b| { version_cmp( - os_str_as_bytes_lossy(a.path().as_os_str()).as_ref(), - os_str_as_bytes_lossy(b.path().as_os_str()).as_ref(), + os_str_as_bytes_lossy(a.p_buf.as_os_str()).as_ref(), + os_str_as_bytes_lossy(b.p_buf.as_os_str()).as_ref(), ) - .then(a.path().to_string_lossy().cmp(&b.path().to_string_lossy())) + .then(a.p_buf.to_string_lossy().cmp(&b.p_buf.to_string_lossy())) }), Sort::Extension => entries.sort_by(|a, b| { - a.path() + a.p_buf .extension() - .cmp(&b.path().extension()) - .then(a.path().file_stem().cmp(&b.path().file_stem())) + .cmp(&b.p_buf.extension()) + .then(a.p_buf.file_stem().cmp(&b.p_buf.file_stem())) }), Sort::Width => entries.sort_by(|a, b| { - a.display_name() + a.display_name .len() - .cmp(&b.display_name().len()) - .then(a.display_name().cmp(b.display_name())) + .cmp(&b.display_name.len()) + .then(a.display_name.cmp(&b.display_name)) }), Sort::None => {} } @@ -2141,7 +2130,7 @@ fn sort_entries(entries: &mut [PathData], config: &Config) { !match md { None | Some(None) => { // If it metadata cannot be determined, treat as a file. - get_metadata_with_deref_opt(p.path(), true) + get_metadata_with_deref_opt(p.p_buf.as_path(), true) .map_or_else(|_| false, |m| m.is_dir()) } Some(Some(m)) => m.is_dir(), @@ -2206,14 +2195,14 @@ fn enter_directory( let mut entries: Vec = if config.files == Files::All { vec![ PathData::new( - path_data.path().to_path_buf(), + path_data.p_buf.clone(), None, Some(".".into()), config, false, ), PathData::new( - path_data.path().join(".."), + path_data.p_buf.join(".."), None, Some("..".into()), config, @@ -2237,12 +2226,12 @@ fn enter_directory( if should_display(&dir_entry, config) { let entry_path_data = - PathData::new(dir_entry.path(), Some(dir_entry), None, config, false); + PathData::new(dir_entry.path(), Some(Ok(dir_entry)), None, config, false); entries.push(entry_path_data); } } - sort_entries(&mut entries, config); + sort_entries(&mut entries, config, &mut state.out); // Print total after any error display if config.format == Format::Long || config.alloc_size { @@ -2259,20 +2248,23 @@ fn enter_directory( for e in entries .iter() .skip(if config.files == Files::All { 2 } else { 0 }) - .filter(|p| p.file_type().is_some_and(|ft| ft.is_dir())) + .filter(|p| { + p.ft.get() + .is_some_and(|o_ft| o_ft.is_some_and(|ft| ft.is_dir())) + }) { - match fs::read_dir(e.path()) { + match fs::read_dir(&e.p_buf) { Err(err) => { state.out.flush()?; show!(LsError::IOErrorContext( - e.path().to_path_buf(), + e.p_buf.clone(), err, e.command_line )); } Ok(rd) => { if listed_ancestors - .insert(FileInformation::from_path(e.path(), e.must_dereference)?) + .insert(FileInformation::from_path(&e.p_buf, e.must_dereference)?) { // when listing several directories in recursive mode, we show // "dirname:" at the beginning of the file list @@ -2283,7 +2275,7 @@ fn enter_directory( // 2 = \n + \n dired.padding = 2; dired::indent(&mut state.out)?; - let dir_name_size = e.path().to_string_lossy().len(); + let dir_name_size = e.p_buf.to_string_lossy().len(); dired::calculate_subdired(dired, dir_name_size); // inject dir name dired::add_dir_name(dired, dir_name_size); @@ -2293,10 +2285,10 @@ fn enter_directory( writeln!(state.out)?; enter_directory(e, rd, config, state, listed_ancestors, dired)?; listed_ancestors - .remove(&FileInformation::from_path(e.path(), e.must_dereference)?); + .remove(&FileInformation::from_path(&e.p_buf, e.must_dereference)?); } else { state.out.flush()?; - show!(LsError::AlreadyListedError(e.path().to_path_buf())); + show!(LsError::AlreadyListedError(e.p_buf.clone())); } } } @@ -2320,7 +2312,7 @@ fn display_dir_entry_size( state: &mut ListState, ) -> (usize, usize, usize, usize, usize, usize) { // TODO: Cache/memorize the display_* results so we don't have to recalculate them. - if let Some(md) = entry.metadata() { + if let Some(md) = entry.get_metadata(&mut state.out) { let (size_len, major_len, minor_len) = match display_len_or_rdev(md, config) { SizeOrDeviceId::Device(major, minor) => { (major.len() + minor.len() + 2usize, major.len(), minor.len()) @@ -2409,7 +2401,7 @@ fn return_total( let mut total_size = 0; for item in items { total_size += item - .metadata() + .get_metadata(out) .as_ref() .map_or(0, |md| get_block_size(md, config)); } @@ -2427,6 +2419,7 @@ fn display_additional_leading_info( item: &PathData, padding: &PaddingCollection, config: &Config, + out: &mut BufWriter, ) -> UResult { let mut result = String::new(); #[cfg(unix)] @@ -2470,7 +2463,7 @@ fn display_items( // option, print the security context to the left of the size column. let quoted = items.iter().any(|item| { - let name = locale_aware_escape_name(item.display_name(), config.quoting_style); + let name = locale_aware_escape_name(&item.display_name, config.quoting_style); os_str_starts_with(&name, b"'") }); @@ -2484,7 +2477,12 @@ fn display_items( let should_display_leading_info = config.alloc_size; if should_display_leading_info { - let more_info = display_additional_leading_info(item, &padding_collection, config)?; + let more_info = display_additional_leading_info( + item, + &padding_collection, + config, + &mut state.out, + )?; write!(state.out, "{more_info}")?; } @@ -2495,7 +2493,7 @@ fn display_items( let mut longest_context_len = 1; let prefix_context = if config.context { for item in items { - let context_len = item.security_context().len(); + let context_len = item.security_context.len(); longest_context_len = context_len.max(longest_context_len); } Some(longest_context_len) @@ -2512,7 +2510,7 @@ fn display_items( let mut names_vec = Vec::new(); for i in items { - let more_info = display_additional_leading_info(i, &padding, config)?; + let more_info = display_additional_leading_info(i, &padding, config, &mut state.out)?; // it's okay to set current column to zero which is used to decide // whether text will wrap or not, because when format is grid or // column ls will try to place the item name in a new line if it @@ -2733,14 +2731,14 @@ fn display_item_long( if config.dired { output_display.extend(b" "); } - if let Some(md) = item.metadata() { + if let Some(md) = item.get_metadata(&mut state.out) { #[cfg(any(not(unix), target_os = "android", target_os = "macos"))] // TODO: See how Mac should work here let is_acl_set = false; #[cfg(all(unix, not(any(target_os = "android", target_os = "macos"))))] - let is_acl_set = has_acl(item.display_name()); + let is_acl_set = has_acl(item.display_name.as_os_str()); output_display.extend(display_permissions(md, true).as_bytes()); - if item.security_context().len() > 1 { + 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. output_display.extend(b"."); @@ -2762,7 +2760,7 @@ fn display_item_long( if config.context { output_display.extend(b" "); - output_display.extend_pad_right(item.security_context(), padding.context); + output_display.extend_pad_right(&item.security_context, padding.context); } // Author is only different from owner on GNU/Hurd, so we reuse @@ -2837,7 +2835,7 @@ fn display_item_long( } else { #[cfg(unix)] let leading_char = { - if let Some(ft) = item.file_type() { + if let Some(Some(ft)) = item.ft.get() { if ft.is_char_device() { "c" } else if ft.is_block_device() { @@ -2849,15 +2847,13 @@ fn display_item_long( } else { "-" } - } else if item.is_dangling_link() { - "l" } else { "-" } }; #[cfg(not(unix))] let leading_char = { - if let Some(ft) = item.file_type() { + if let Some(Some(ft)) = item.ft.get() { if ft.is_symlink() { "l" } else if ft.is_dir() { @@ -2865,8 +2861,6 @@ fn display_item_long( } else { "-" } - } else if item.is_dangling_link() { - "l" } else { "-" } @@ -2874,7 +2868,7 @@ fn display_item_long( output_display.extend(leading_char.as_bytes()); output_display.extend(b"?????????"); - if item.security_context().len() > 1 { + 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. output_display.extend(b"."); @@ -2894,7 +2888,7 @@ fn display_item_long( if config.context { output_display.extend(b" "); - output_display.extend_pad_right(item.security_context(), padding.context); + output_display.extend_pad_right(&item.security_context, padding.context); } // Author is only different from owner on GNU/Hurd, so we reuse @@ -3052,8 +3046,8 @@ fn file_is_executable(md: &Metadata) -> bool { return md.mode() & ((S_IXUSR | S_IXGRP | S_IXOTH) as u32) != 0; } -fn classify_file(path: &PathData) -> Option { - let file_type = path.file_type()?; +fn classify_file<'a>(path: &'a PathData, out: &mut BufWriter) -> Option<&'a str> { + let file_type = path.file_type(out)?; if file_type.is_dir() { Some("/") @@ -3072,7 +3066,6 @@ fn classify_file(path: &PathData) -> Option { && path.get_metadata(out).is_some_and(file_is_executable) { Some("*") - } else { None } @@ -3106,7 +3099,7 @@ fn display_item_name( current_column: LazyCell usize + '_>>, ) -> OsString { // This is our return value. We start by `&path.display_name` and modify it along the way. - let mut name = locale_aware_escape_name(path.display_name(), config.quoting_style); + let mut name = locale_aware_escape_name(&path.display_name, config.quoting_style); let is_wrap = |namelen: usize| config.width != 0 && *current_column + namelen > config.width.into(); @@ -3117,7 +3110,14 @@ fn display_item_name( if let Some(style_manager) = &mut state.style_manager { let len = name.len(); - name = color_name(name, path, style_manager, None, is_wrap(len)); + name = color_name( + name, + path, + style_manager, + &mut state.out, + None, + is_wrap(len), + ); } if config.format != Format::Long && !more_info.is_empty() { @@ -3127,7 +3127,7 @@ fn display_item_name( } if config.indicator_style != IndicatorStyle::None { - let sym = classify_file(path); + let sym = classify_file(path, &mut state.out); let char_opt: Option<&str> = match config.indicator_style { IndicatorStyle::Classify => sym, @@ -3154,11 +3154,12 @@ fn display_item_name( } if config.format == Format::Long - && path.file_type().is_some_and(|ft| ft.is_symlink()) + && path.file_type(&mut state.out).is_some() + && path.file_type(&mut state.out).unwrap().is_symlink() && !path.must_dereference { - match path.path().read_link() { - Ok(target_path) => { + match path.p_buf.read_link() { + Ok(target) => { name.push(" -> "); // We might as well color the symlink output after the arrow. @@ -3167,9 +3168,9 @@ fn display_item_name( if let Some(style_manager) = &mut state.style_manager { // 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_path.clone(); - if target_path.is_relative() { - if let Some(parent) = path.path().parent() { + let mut absolute_target = target.clone(); + if target.is_relative() { + if let Some(parent) = path.p_buf.parent() { absolute_target = parent.join(absolute_target); } } @@ -3180,13 +3181,20 @@ fn display_item_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. - if path.metadata().is_none() && target_data.metadata().is_none() { - name.push(target_path); + if path.get_metadata(&mut state.out).is_none() + && get_metadata_with_deref_opt( + target_data.p_buf.as_path(), + target_data.must_dereference, + ) + .is_err() + { + name.push(path.p_buf.read_link().unwrap()); } else { name.push(color_name( - locale_aware_escape_name(target_path.as_os_str(), config.quoting_style), + locale_aware_escape_name(target.as_os_str(), config.quoting_style), path, style_manager, + &mut state.out, Some(&target_data), is_wrap(name.len()), )); @@ -3195,17 +3203,13 @@ fn display_item_name( // If no coloring is required, we just use target as is. // Apply the right quoting name.push(locale_aware_escape_name( - target_path.as_os_str(), + target.as_os_str(), config.quoting_style, )); } } Err(err) => { - show!(LsError::IOErrorContext( - path.path().to_path_buf(), - err, - false - )); + show!(LsError::IOErrorContext(path.p_buf.clone(), err, false)); } } } @@ -3221,7 +3225,6 @@ fn display_item_name( }; let old_name = name.to_string_lossy(); name = format!("{security_context} {old_name}").into(); - } } @@ -3232,7 +3235,7 @@ fn create_hyperlink(name: &OsStr, path: &PathData) -> OsString { let hostname = hostname::get().unwrap_or_else(|_| OsString::from("")); let hostname = hostname.to_string_lossy(); - let absolute_path = fs::canonicalize(path.path()).unwrap_or_default(); + let absolute_path = fs::canonicalize(&path.p_buf).unwrap_or_default(); let absolute_path = absolute_path.to_string_lossy(); #[cfg(not(target_os = "windows"))] @@ -3278,60 +3281,57 @@ fn display_inode(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 -fn get_security_context<'a>( - path: &'a Path, - must_dereference: bool, - config: &'a Config, -) -> Cow<'a, str> { - static SUBSTITUTE_STRING: &str = "?"; - +fn get_security_context(config: &Config, p_buf: &Path, must_dereference: bool) -> String { + let substitute_string = "?".to_string(); // If we must dereference, ensure that the symlink is actually valid even if the system // does not support SELinux. // Conforms to the GNU coreutils where a dangling symlink results in exit code 1. if must_dereference { - if let Err(err) = get_metadata_with_deref_opt(path, must_dereference) { - // The Path couldn't be dereferenced, so return early and set exit code 1 - // to indicate a minor error - // Only show error when context display is requested to avoid duplicate messages - if config.context { - show!(LsError::IOErrorContext(path.to_path_buf(), err, false)); + match get_metadata_with_deref_opt(p_buf, must_dereference) { + Err(err) => { + // The Path couldn't be dereferenced, so return early and set exit code 1 + // to indicate a minor error + // Only show error when context display is requested to avoid duplicate messages + if config.context { + show!(LsError::IOErrorContext(p_buf.to_path_buf(), err, false)); + } + return substitute_string; } - return Cow::Borrowed(SUBSTITUTE_STRING); + Ok(_md) => (), } } - if config.selinux_supported { #[cfg(feature = "selinux")] { - match selinux::SecurityContext::of_path(path, 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: {}", path.quote()); - return Cow::Borrowed(SUBSTITUTE_STRING); + show_warning!("failed to get security context of: {}", p_buf.quote()); + substitute_string } - Ok(None) => return Cow::Borrowed(SUBSTITUTE_STRING), + Ok(None) => substitute_string, Ok(Some(context)) => { let context = context.as_bytes(); let context = context.strip_suffix(&[0]).unwrap_or(context); - - let res: String = String::from_utf8(context.to_vec()).unwrap_or_else(|e| { + String::from_utf8(context.to_vec()).unwrap_or_else(|e| { show_warning!( "getting security context of: {}: {}", - path.quote(), + p_buf.quote(), e.to_string() ); - - String::from_utf8_lossy(context).to_string() - }); - - return Cow::Owned(res); + String::from_utf8_lossy(context).into_owned() + }) } } } + #[cfg(not(feature = "selinux"))] + { + substitute_string + } + } else { + substitute_string } - - Cow::Borrowed(SUBSTITUTE_STRING) } #[cfg(unix)] @@ -3355,7 +3355,7 @@ fn calculate_padding_collection( for item in items { #[cfg(unix)] if config.inode { - let inode_len = if let Some(md) = item.metadata() { + let inode_len = if let Some(md) = item.get_metadata(&mut state.out) { display_inode(md).len() } else { continue; @@ -3364,14 +3364,14 @@ fn calculate_padding_collection( } if config.alloc_size { - if let Some(md) = item.metadata() { + if let Some(md) = item.get_metadata(&mut state.out) { let block_size_len = display_size(get_block_size(md, config), config).len(); padding_collections.block_size = block_size_len.max(padding_collections.block_size); } } if config.format == Format::Long { - let context_len = item.security_context().len(); + let context_len = item.security_context.len(); let (link_count_len, uname_len, group_len, size_len, major_len, minor_len) = display_dir_entry_size(item, config, state); padding_collections.link_count = link_count_len.max(padding_collections.link_count); @@ -3414,13 +3414,13 @@ fn calculate_padding_collection( for item in items { if config.alloc_size { - if let Some(md) = item.metadata() { + if let Some(md) = item.get_metadata(&mut state.out) { let block_size_len = display_size(get_block_size(md, config), config).len(); padding_collections.block_size = block_size_len.max(padding_collections.block_size); } } - let context_len = item.security_context().len(); + let context_len = item.security_context.len(); let (link_count_len, uname_len, group_len, size_len, _major_len, _minor_len) = display_dir_entry_size(item, config, state); padding_collections.link_count = link_count_len.max(padding_collections.link_count);