From 224be498afc64fc46cd367aa4e51e23dc66de7ca Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 8 Jan 2026 14:28:54 +0100 Subject: [PATCH 1/5] ls: remove redundant HashMap lookup in append_raw_style_code_for_indicator --- src/uu/ls/src/colors.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 8eb4b70970b..fc481eab0d4 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -129,14 +129,11 @@ impl<'a> StyleManager<'a> { indicator: Indicator, style_code: &mut String, ) { - if !self.indicator_codes.contains_key(&indicator) { - return; - } - style_code.push_str(self.reset(!self.initial_reset_is_done)); - style_code.push_str(ANSI_CSI); - if let Some(raw) = self.indicator_codes.get(&indicator) { + if let Some(raw) = self.indicator_codes.get(&indicator).cloned() { debug_assert!(!raw.is_empty()); - style_code.push_str(raw); + style_code.push_str(self.reset(!self.initial_reset_is_done)); + style_code.push_str(ANSI_CSI); + style_code.push_str(&raw); style_code.push_str(ANSI_SGR_END); } } From 71c3eb603ba42eb6048750de8e04e1045b17f703 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 8 Jan 2026 14:29:23 +0100 Subject: [PATCH 2/5] ls: replace magic numbers with named constants --- src/uu/ls/src/colors.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index fc481eab0d4..39955fc3c23 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -18,6 +18,14 @@ const ANSI_RESET: &str = "\x1b[0m"; const ANSI_CLEAR_EOL: &str = "\x1b[K"; const EMPTY_STYLE: &str = "\x1b[m"; +// Unix file mode bits +const MODE_SETUID: u32 = 0o4000; +const MODE_SETGID: u32 = 0o2000; +const MODE_EXECUTABLE: u32 = 0o0111; +const MODE_STICKY_OTHER_WRITABLE: u32 = 0o1002; +const MODE_OTHER_WRITABLE: u32 = 0o0002; +const MODE_STICKY: u32 = 0o1000; + enum RawIndicatorStyle { Empty, Code(Indicator), @@ -382,13 +390,13 @@ impl<'a> StyleManager<'a> { if self.needs_file_metadata() { if let Some(metadata) = path.metadata() { let mode = metadata.mode(); - if self.has_indicator_style(Indicator::Setuid) && mode & 0o4000 != 0 { + if self.has_indicator_style(Indicator::Setuid) && mode & MODE_SETUID != 0 { return Some(Indicator::Setuid); } - if self.has_indicator_style(Indicator::Setgid) && mode & 0o2000 != 0 { + if self.has_indicator_style(Indicator::Setgid) && mode & MODE_SETGID != 0 { return Some(Indicator::Setgid); } - if self.has_indicator_style(Indicator::ExecutableFile) && mode & 0o0111 != 0 { + if self.has_indicator_style(Indicator::ExecutableFile) && mode & MODE_EXECUTABLE != 0 { return Some(Indicator::ExecutableFile); } if self.has_indicator_style(Indicator::MultipleHardLinks) @@ -408,14 +416,14 @@ impl<'a> StyleManager<'a> { if let Some(metadata) = path.metadata() { let mode = metadata.mode(); if self.has_indicator_style(Indicator::StickyAndOtherWritable) - && mode & 0o1002 == 0o1002 + && mode & MODE_STICKY_OTHER_WRITABLE == MODE_STICKY_OTHER_WRITABLE { return Some(Indicator::StickyAndOtherWritable); } - if self.has_indicator_style(Indicator::OtherWritable) && mode & 0o0002 != 0 { + if self.has_indicator_style(Indicator::OtherWritable) && mode & MODE_OTHER_WRITABLE != 0 { return Some(Indicator::OtherWritable); } - if self.has_indicator_style(Indicator::Sticky) && mode & 0o1000 != 0 { + if self.has_indicator_style(Indicator::Sticky) && mode & MODE_STICKY != 0 { return Some(Indicator::Sticky); } } From c463561c8e361051a812fe1fe1ca4b1a3b58f449 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 8 Jan 2026 14:30:07 +0100 Subject: [PATCH 3/5] ls: refactor complex indicator_for_raw_code method into smaller helpers --- src/uu/ls/src/colors.rs | 169 +++++++++++++++++++++++----------------- 1 file changed, 96 insertions(+), 73 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 39955fc3c23..d97e5c42a3c 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -360,25 +360,7 @@ impl<'a> StyleManager<'a> { }; if file_type.is_symlink() { - let orphan_enabled = self.has_indicator_style(Indicator::OrphanedSymbolicLink); - let missing_enabled = self.has_indicator_style(Indicator::MissingFile); - let needs_target_state = self.ln_color_from_target || orphan_enabled; - let target_missing = needs_target_state && !entry_exists(); - - if target_missing { - let orphan_raw = self.indicator_codes.get(&Indicator::OrphanedSymbolicLink); - let orphan_raw_is_empty = orphan_raw.is_some_and(|value| value.is_empty()); - if orphan_enabled && (!orphan_raw_is_empty || self.ln_color_from_target) { - return Some(Indicator::OrphanedSymbolicLink); - } - if self.ln_color_from_target && missing_enabled { - return Some(Indicator::MissingFile); - } - } - if self.has_indicator_style(Indicator::SymbolicLink) { - return Some(Indicator::SymbolicLink); - } - return None; + return self.indicator_for_symlink(path, &mut entry_exists); } if self.has_indicator_style(Indicator::MissingFile) && !entry_exists() { @@ -386,72 +368,113 @@ impl<'a> StyleManager<'a> { } if file_type.is_file() { - #[cfg(unix)] - if self.needs_file_metadata() { - if let Some(metadata) = path.metadata() { - let mode = metadata.mode(); - if self.has_indicator_style(Indicator::Setuid) && mode & MODE_SETUID != 0 { - return Some(Indicator::Setuid); - } - if self.has_indicator_style(Indicator::Setgid) && mode & MODE_SETGID != 0 { - return Some(Indicator::Setgid); - } - if self.has_indicator_style(Indicator::ExecutableFile) && mode & MODE_EXECUTABLE != 0 { - return Some(Indicator::ExecutableFile); - } - if self.has_indicator_style(Indicator::MultipleHardLinks) - && metadata.nlink() > 1 - { - return Some(Indicator::MultipleHardLinks); - } - } - } + self.indicator_for_file(path) + } else if file_type.is_dir() { + self.indicator_for_directory(path) + } else { + self.indicator_for_special_file(file_type) + } + } - if self.has_indicator_style(Indicator::RegularFile) { - return Some(Indicator::RegularFile); + fn indicator_for_symlink( + &self, + _path: &PathData, + entry_exists: &mut dyn FnMut() -> bool, + ) -> Option { + let orphan_enabled = self.has_indicator_style(Indicator::OrphanedSymbolicLink); + let missing_enabled = self.has_indicator_style(Indicator::MissingFile); + let needs_target_state = self.ln_color_from_target || orphan_enabled; + let target_missing = needs_target_state && !entry_exists(); + + if target_missing { + let orphan_raw = self.indicator_codes.get(&Indicator::OrphanedSymbolicLink); + let orphan_raw_is_empty = orphan_raw.is_some_and(|value| value.is_empty()); + if orphan_enabled && (!orphan_raw_is_empty || self.ln_color_from_target) { + return Some(Indicator::OrphanedSymbolicLink); } - } else if file_type.is_dir() { - #[cfg(unix)] - if self.needs_dir_metadata() { - if let Some(metadata) = path.metadata() { - let mode = metadata.mode(); - if self.has_indicator_style(Indicator::StickyAndOtherWritable) - && mode & MODE_STICKY_OTHER_WRITABLE == MODE_STICKY_OTHER_WRITABLE - { - return Some(Indicator::StickyAndOtherWritable); - } - if self.has_indicator_style(Indicator::OtherWritable) && mode & MODE_OTHER_WRITABLE != 0 { - return Some(Indicator::OtherWritable); - } - if self.has_indicator_style(Indicator::Sticky) && mode & MODE_STICKY != 0 { - return Some(Indicator::Sticky); - } - } + if self.ln_color_from_target && missing_enabled { + return Some(Indicator::MissingFile); } + } + if self.has_indicator_style(Indicator::SymbolicLink) { + return Some(Indicator::SymbolicLink); + } + None + } - if self.has_indicator_style(Indicator::Directory) { - return Some(Indicator::Directory); - } - } else { - #[cfg(unix)] - { - if file_type.is_fifo() && self.has_indicator_style(Indicator::FIFO) { - return Some(Indicator::FIFO); + fn indicator_for_file(&self, path: &PathData) -> Option { + #[cfg(unix)] + if self.needs_file_metadata() { + if let Some(metadata) = path.metadata() { + let mode = metadata.mode(); + if self.has_indicator_style(Indicator::Setuid) && mode & MODE_SETUID != 0 { + return Some(Indicator::Setuid); } - if file_type.is_socket() && self.has_indicator_style(Indicator::Socket) { - return Some(Indicator::Socket); + if self.has_indicator_style(Indicator::Setgid) && mode & MODE_SETGID != 0 { + return Some(Indicator::Setgid); } - if file_type.is_block_device() && self.has_indicator_style(Indicator::BlockDevice) { - return Some(Indicator::BlockDevice); + if self.has_indicator_style(Indicator::ExecutableFile) + && mode & MODE_EXECUTABLE != 0 + { + return Some(Indicator::ExecutableFile); } - if file_type.is_char_device() - && self.has_indicator_style(Indicator::CharacterDevice) + if self.has_indicator_style(Indicator::MultipleHardLinks) && metadata.nlink() > 1 { + return Some(Indicator::MultipleHardLinks); + } + } + } + + if self.has_indicator_style(Indicator::RegularFile) { + Some(Indicator::RegularFile) + } else { + None + } + } + + fn indicator_for_directory(&self, path: &PathData) -> Option { + #[cfg(unix)] + if self.needs_dir_metadata() { + if let Some(metadata) = path.metadata() { + let mode = metadata.mode(); + if self.has_indicator_style(Indicator::StickyAndOtherWritable) + && mode & MODE_STICKY_OTHER_WRITABLE == MODE_STICKY_OTHER_WRITABLE { - return Some(Indicator::CharacterDevice); + return Some(Indicator::StickyAndOtherWritable); + } + if self.has_indicator_style(Indicator::OtherWritable) + && mode & MODE_OTHER_WRITABLE != 0 + { + return Some(Indicator::OtherWritable); + } + if self.has_indicator_style(Indicator::Sticky) && mode & MODE_STICKY != 0 { + return Some(Indicator::Sticky); } } } + if self.has_indicator_style(Indicator::Directory) { + Some(Indicator::Directory) + } else { + None + } + } + + fn indicator_for_special_file(&self, file_type: &std::fs::FileType) -> Option { + #[cfg(unix)] + { + if file_type.is_fifo() && self.has_indicator_style(Indicator::FIFO) { + return Some(Indicator::FIFO); + } + if file_type.is_socket() && self.has_indicator_style(Indicator::Socket) { + return Some(Indicator::Socket); + } + if file_type.is_block_device() && self.has_indicator_style(Indicator::BlockDevice) { + return Some(Indicator::BlockDevice); + } + if file_type.is_char_device() && self.has_indicator_style(Indicator::CharacterDevice) { + return Some(Indicator::CharacterDevice); + } + } None } From 1f247d156da1121b155d9d462418c771d3a5471e Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 8 Jan 2026 14:31:09 +0100 Subject: [PATCH 4/5] ls: optimize canonicalize_indicator_value to avoid unnecessary allocations --- src/uu/ls/src/colors.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index d97e5c42a3c..5f6f6a1df21 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -4,6 +4,7 @@ // file that was distributed with this source code. use super::PathData; use lscolors::{Indicator, LsColors, Style}; +use std::borrow::Cow; use std::collections::HashMap; use std::env; use std::ffi::OsString; @@ -758,7 +759,7 @@ fn parse_indicator_codes() -> (HashMap, bool) { } continue; } - indicator_codes.insert(indicator, canonicalize_indicator_value(value)); + indicator_codes.insert(indicator, canonicalize_indicator_value(value).into_owned()); } } } @@ -766,14 +767,14 @@ fn parse_indicator_codes() -> (HashMap, bool) { (indicator_codes, ln_color_from_target) } -fn canonicalize_indicator_value(value: &str) -> String { +fn canonicalize_indicator_value(value: &str) -> Cow<'_, str> { if value.len() == 1 && value.chars().all(|c| c.is_ascii_digit()) { let mut canonical = String::with_capacity(2); canonical.push('0'); canonical.push_str(value); - canonical + Cow::Owned(canonical) } else { - value.to_string() + Cow::Borrowed(value) } } From f07f5b9f2031a3d2b55e0938a38d94e0cf30fc20 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Fri, 9 Jan 2026 20:21:59 +0100 Subject: [PATCH 5/5] ls: fix Windows compilation by properly handling platform-specific code --- src/uu/ls/src/colors.rs | 90 +++++++++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 35 deletions(-) diff --git a/src/uu/ls/src/colors.rs b/src/uu/ls/src/colors.rs index 5f6f6a1df21..6b858ba65f3 100644 --- a/src/uu/ls/src/colors.rs +++ b/src/uu/ls/src/colors.rs @@ -19,13 +19,16 @@ const ANSI_RESET: &str = "\x1b[0m"; const ANSI_CLEAR_EOL: &str = "\x1b[K"; const EMPTY_STYLE: &str = "\x1b[m"; -// Unix file mode bits -const MODE_SETUID: u32 = 0o4000; -const MODE_SETGID: u32 = 0o2000; -const MODE_EXECUTABLE: u32 = 0o0111; -const MODE_STICKY_OTHER_WRITABLE: u32 = 0o1002; -const MODE_OTHER_WRITABLE: u32 = 0o0002; -const MODE_STICKY: u32 = 0o1000; +#[cfg(unix)] +mod mode { + // Unix file mode bits + pub const SETUID: u32 = 0o4000; + pub const SETGID: u32 = 0o2000; + pub const EXECUTABLE: u32 = 0o0111; + pub const STICKY_OTHER_WRITABLE: u32 = 0o1002; + pub const OTHER_WRITABLE: u32 = 0o0002; + pub const STICKY: u32 = 0o1000; +} enum RawIndicatorStyle { Empty, @@ -361,7 +364,7 @@ impl<'a> StyleManager<'a> { }; if file_type.is_symlink() { - return self.indicator_for_symlink(path, &mut entry_exists); + return self.indicator_for_symlink(&mut entry_exists); } if self.has_indicator_style(Indicator::MissingFile) && !entry_exists() { @@ -377,11 +380,7 @@ impl<'a> StyleManager<'a> { } } - fn indicator_for_symlink( - &self, - _path: &PathData, - entry_exists: &mut dyn FnMut() -> bool, - ) -> Option { + fn indicator_for_symlink(&self, entry_exists: &mut dyn FnMut() -> bool) -> Option { let orphan_enabled = self.has_indicator_style(Indicator::OrphanedSymbolicLink); let missing_enabled = self.has_indicator_style(Indicator::MissingFile); let needs_target_state = self.ln_color_from_target || orphan_enabled; @@ -403,19 +402,19 @@ impl<'a> StyleManager<'a> { None } + #[cfg(unix)] fn indicator_for_file(&self, path: &PathData) -> Option { - #[cfg(unix)] if self.needs_file_metadata() { if let Some(metadata) = path.metadata() { let mode = metadata.mode(); - if self.has_indicator_style(Indicator::Setuid) && mode & MODE_SETUID != 0 { + if self.has_indicator_style(Indicator::Setuid) && mode & mode::SETUID != 0 { return Some(Indicator::Setuid); } - if self.has_indicator_style(Indicator::Setgid) && mode & MODE_SETGID != 0 { + if self.has_indicator_style(Indicator::Setgid) && mode & mode::SETGID != 0 { return Some(Indicator::Setgid); } if self.has_indicator_style(Indicator::ExecutableFile) - && mode & MODE_EXECUTABLE != 0 + && mode & mode::EXECUTABLE != 0 { return Some(Indicator::ExecutableFile); } @@ -432,22 +431,31 @@ impl<'a> StyleManager<'a> { } } + #[cfg(not(unix))] + fn indicator_for_file(&self, _path: &PathData) -> Option { + if self.has_indicator_style(Indicator::RegularFile) { + Some(Indicator::RegularFile) + } else { + None + } + } + + #[cfg(unix)] fn indicator_for_directory(&self, path: &PathData) -> Option { - #[cfg(unix)] if self.needs_dir_metadata() { if let Some(metadata) = path.metadata() { let mode = metadata.mode(); if self.has_indicator_style(Indicator::StickyAndOtherWritable) - && mode & MODE_STICKY_OTHER_WRITABLE == MODE_STICKY_OTHER_WRITABLE + && mode & mode::STICKY_OTHER_WRITABLE == mode::STICKY_OTHER_WRITABLE { return Some(Indicator::StickyAndOtherWritable); } if self.has_indicator_style(Indicator::OtherWritable) - && mode & MODE_OTHER_WRITABLE != 0 + && mode & mode::OTHER_WRITABLE != 0 { return Some(Indicator::OtherWritable); } - if self.has_indicator_style(Indicator::Sticky) && mode & MODE_STICKY != 0 { + if self.has_indicator_style(Indicator::Sticky) && mode & mode::STICKY != 0 { return Some(Indicator::Sticky); } } @@ -460,25 +468,37 @@ impl<'a> StyleManager<'a> { } } + #[cfg(not(unix))] + fn indicator_for_directory(&self, _path: &PathData) -> Option { + if self.has_indicator_style(Indicator::Directory) { + Some(Indicator::Directory) + } else { + None + } + } + + #[cfg(unix)] fn indicator_for_special_file(&self, file_type: &std::fs::FileType) -> Option { - #[cfg(unix)] - { - if file_type.is_fifo() && self.has_indicator_style(Indicator::FIFO) { - return Some(Indicator::FIFO); - } - if file_type.is_socket() && self.has_indicator_style(Indicator::Socket) { - return Some(Indicator::Socket); - } - if file_type.is_block_device() && self.has_indicator_style(Indicator::BlockDevice) { - return Some(Indicator::BlockDevice); - } - if file_type.is_char_device() && self.has_indicator_style(Indicator::CharacterDevice) { - return Some(Indicator::CharacterDevice); - } + if file_type.is_fifo() && self.has_indicator_style(Indicator::FIFO) { + return Some(Indicator::FIFO); + } + if file_type.is_socket() && self.has_indicator_style(Indicator::Socket) { + return Some(Indicator::Socket); + } + if file_type.is_block_device() && self.has_indicator_style(Indicator::BlockDevice) { + return Some(Indicator::BlockDevice); + } + if file_type.is_char_device() && self.has_indicator_style(Indicator::CharacterDevice) { + return Some(Indicator::CharacterDevice); } None } + #[cfg(not(unix))] + fn indicator_for_special_file(&self, _file_type: &std::fs::FileType) -> Option { + None + } + #[cfg(unix)] fn needs_file_metadata(&self) -> bool { self.has_indicator_style(Indicator::Setuid)