From c832ca82b77a8cc9540486b50fbced3270015e79 Mon Sep 17 00:00:00 2001 From: joknarf Date: Tue, 31 Mar 2026 00:19:42 +0200 Subject: [PATCH 1/3] ls: ls -lF symlink target indicator --- src/uu/ls/src/display.rs | 130 ++++++++++++++++++++------------------- tests/by-util/test_ls.rs | 23 +++++++ 2 files changed, 89 insertions(+), 64 deletions(-) diff --git a/src/uu/ls/src/display.rs b/src/uu/ls/src/display.rs index 929ce7e4610..2d2eecdc834 100644 --- a/src/uu/ls/src/display.rs +++ b/src/uu/ls/src/display.rs @@ -93,7 +93,7 @@ pub(crate) struct DisplayItemName { pub(crate) dired_name_len: usize, } -#[derive(PartialEq, Eq)] +#[derive(Clone, Copy, PartialEq, Eq)] pub(crate) enum IndicatorStyle { None, Slash, @@ -704,29 +704,12 @@ fn display_item_name( } } - if config.indicator_style != IndicatorStyle::None { - let sym = classify_file(path); - - let char_opt = match config.indicator_style { - IndicatorStyle::Classify => sym, - IndicatorStyle::FileType => { - // Don't append an asterisk. - match sym { - Some('*') => None, - _ => sym, - } - } - IndicatorStyle::Slash => { - // Append only a slash. - match sym { - Some('/') => Some('/'), - _ => None, - } - } - IndicatorStyle::None => None, - }; + let is_long_symlink = config.format == Format::Long + && path.file_type().is_some_and(FileType::is_symlink) + && !path.must_dereference; - if let Some(c) = char_opt { + if !is_long_symlink { + if let Some(c) = indicator_char(path, config.indicator_style) { let _ = name.write_char(c); } } @@ -744,66 +727,68 @@ fn display_item_name( // We might as well color the symlink output after the arrow. // This makes extra system calls, but provides important information that // people run `ls -l --color` are very interested in. - if let Some(style_manager) = &mut style_manager { - let escaped_target = escape_name_with_locale(target_path.as_os_str(), config); - // 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 absolute_target = if target_path.is_relative() { - match path.path().parent() { - Some(p) => &p.join(&target_path), - None => &target_path, - } - } else { - &target_path - }; - - match fs::canonicalize(absolute_target) { - Ok(resolved_target) => { - let target_data = PathData::new( - resolved_target.as_path().into(), - None, - target_path.file_name().map(Cow::Borrowed), - config, - false, - ); - - // Check if the target actually needs coloring + let escaped_target = escape_name_with_locale(target_path.as_os_str(), config); + + // 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 absolute_target = if target_path.is_relative() { + match path.path().parent() { + Some(p) => p.join(&target_path), + None => target_path.clone(), + } + } else { + target_path.clone() + }; + + match fs::canonicalize(&absolute_target) { + Ok(resolved_target) => { + let target_data = PathData::new( + resolved_target.as_path().into(), + None, + target_path.file_name().map(Cow::Borrowed), + config, + false, + ); + + let mut target_display = escaped_target.clone(); + if let Some(style_manager) = &mut style_manager { let md_option: Option = target_data .metadata() .cloned() .or_else(|| target_data.p_buf.symlink_metadata().ok()); - let style = style_manager.colors.style_for_path_with_metadata( - &target_data.p_buf, - md_option.as_ref(), - ); - if style.is_some() { - // Only apply coloring if there's actually a style - name.push(color_name( - escaped_target, + if style_manager + .colors + .style_for_path_with_metadata(&target_data.p_buf, md_option.as_ref()) + .is_some() + { + target_display = color_name( + escaped_target.clone(), &target_data, style_manager, None, is_wrap(name.len()), - )); - } else { - // For regular files with no coloring, just use plain text - name.push(escaped_target); + ); } } - Err(_) => { + + name.push(target_display); + if let Some(c) = indicator_char(&target_data, config.indicator_style) { + let _ = name.write_char(c); + } + } + Err(_) => { + if let Some(style_manager) = &mut style_manager { name.push( style_manager.apply_missing_target_style( escaped_target, is_wrap(name.len()), ), ); + } else { + name.push(escaped_target); } } - } else { - // If no coloring is required, we just use target as is. - // Apply the right quoting - name.push(escape_name_with_locale(target_path.as_os_str(), config)); } } Err(err) => { @@ -1118,6 +1103,23 @@ fn display_item_long( Ok(()) } +fn indicator_char(path: &PathData, style: IndicatorStyle) -> Option { + let sym = classify_file(path); + + match style { + IndicatorStyle::Classify => sym, + IndicatorStyle::FileType => match sym { + Some('*') => None, + _ => sym, + }, + IndicatorStyle::Slash => match sym { + Some('/') => Some('/'), + _ => None, + }, + IndicatorStyle::None => None, + } +} + fn classify_file(path: &PathData) -> Option { let file_type = path.file_type()?; diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index fbbba1a0a22..5832a4dae49 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -3386,6 +3386,29 @@ fn test_ls_indicator_style() { } } +#[test] +#[cfg(not(windows))] +fn test_ls_indicator_style_symlink_target_long() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.mkdir("dir"); + assert!(at.dir_exists("dir")); + + at.symlink_dir("dir", "linkdir"); + assert!(at.is_symlink("linkdir")); + + scene + .ucmd() + .arg("--classify") + .arg("-l") + .arg("linkdir") + .succeeds() + .stdout_contains("linkdir -> ") + .stdout_does_not_contain("linkdir@ -> ") + .stdout_contains("/dir/"); +} + // Essentially the same test as above, but only test symlinks and directories, // not pipes or sockets. #[test] From e6fd64c384f1f395b4978d75dae26776f61cef72 Mon Sep 17 00:00:00 2001 From: joknarf Date: Tue, 31 Mar 2026 21:03:19 +0200 Subject: [PATCH 2/3] tests/ls: test for ls -lF symlink indicator --- tests/by-util/test_ls.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 5832a4dae49..12bd5e6d818 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -3395,17 +3395,17 @@ fn test_ls_indicator_style_symlink_target_long() { at.mkdir("dir"); assert!(at.dir_exists("dir")); - at.symlink_dir("dir", "linkdir"); - assert!(at.is_symlink("linkdir")); + at.symlink_dir("dir", "dir_link"); + assert!(at.is_symlink("dir_link")); scene .ucmd() .arg("--classify") .arg("-l") - .arg("linkdir") + .arg("dir_link") .succeeds() - .stdout_contains("linkdir -> ") - .stdout_does_not_contain("linkdir@ -> ") + .stdout_contains("dir_link -> ") + .stdout_does_not_contain("dir_link@ -> ") .stdout_contains("/dir/"); } From b5a119481a88bf86d91b4de0f218b371aeb4df88 Mon Sep 17 00:00:00 2001 From: joknarf Date: Tue, 31 Mar 2026 21:03:46 +0200 Subject: [PATCH 3/3] fix(ls): take care of performance regressions Co-authored-by: Guillem L. Jara <4lon3ly0@tutanota.com> --- src/uu/ls/src/config.rs | 36 ++++++++++++++------------------ src/uu/ls/src/display.rs | 44 +++++++++++++++++++++++----------------- 2 files changed, 40 insertions(+), 40 deletions(-) diff --git a/src/uu/ls/src/config.rs b/src/uu/ls/src/config.rs index e2e797f1628..7d1640c4fb6 100644 --- a/src/uu/ls/src/config.rs +++ b/src/uu/ls/src/config.rs @@ -172,7 +172,7 @@ pub struct Config { // Dir and vdir needs access to this field pub quoting_style: QuotingStyle, pub(crate) locale_quoting: Option, - pub(crate) indicator_style: IndicatorStyle, + pub(crate) indicator_style: Option, pub(crate) time_format_recent: String, // Time format for recent dates pub(crate) time_format_older: Option, // Time format for older dates (optional, if not present, time_format_recent is used) pub(crate) context: bool, @@ -554,34 +554,28 @@ fn extract_quoting_style( /// # Returns /// /// An [`IndicatorStyle`] variant representing the indicator style to use. -fn extract_indicator_style(options: &clap::ArgMatches) -> IndicatorStyle { +fn extract_indicator_style(options: &clap::ArgMatches) -> Option { if let Some(field) = options.get_one::(options::INDICATOR_STYLE) { match field.as_str() { - "none" => IndicatorStyle::None, - "file-type" => IndicatorStyle::FileType, - "classify" => IndicatorStyle::Classify, - "slash" => IndicatorStyle::Slash, - &_ => IndicatorStyle::None, + "none" => None, + "file-type" => Some(IndicatorStyle::FileType), + "classify" => Some(IndicatorStyle::Classify), + "slash" => Some(IndicatorStyle::Slash), + &_ => None, } } else if let Some(field) = options.get_one::(options::indicator_style::CLASSIFY) { match field.as_str() { - "never" | "no" | "none" => IndicatorStyle::None, - "always" | "yes" | "force" => IndicatorStyle::Classify, - "auto" | "tty" | "if-tty" => { - if stdout().is_terminal() { - IndicatorStyle::Classify - } else { - IndicatorStyle::None - } - } - &_ => IndicatorStyle::None, + "never" | "no" | "none" => None, + "always" | "yes" | "force" => Some(IndicatorStyle::Classify), + "auto" | "tty" | "if-tty" => stdout().is_terminal().then_some(IndicatorStyle::Classify), + &_ => None, } } else if options.get_flag(options::indicator_style::SLASH) { - IndicatorStyle::Slash + Some(IndicatorStyle::Slash) } else if options.get_flag(options::indicator_style::FILE_TYPE) { - IndicatorStyle::FileType + Some(IndicatorStyle::FileType) } else { - IndicatorStyle::None + None } } @@ -954,7 +948,7 @@ impl Config { } else if options.get_flag(options::dereference::DIR_ARGS) { Dereference::DirArgs } else if options.get_flag(options::DIRECTORY) - || indicator_style == IndicatorStyle::Classify + || indicator_style == Some(IndicatorStyle::Classify) || format == Format::Long { Dereference::None diff --git a/src/uu/ls/src/display.rs b/src/uu/ls/src/display.rs index 2d2eecdc834..7a30d1dd8a7 100644 --- a/src/uu/ls/src/display.rs +++ b/src/uu/ls/src/display.rs @@ -95,7 +95,6 @@ pub(crate) struct DisplayItemName { #[derive(Clone, Copy, PartialEq, Eq)] pub(crate) enum IndicatorStyle { - None, Slash, FileType, Classify, @@ -733,14 +732,14 @@ fn display_item_name( // This is because relative symlinks will fail to get_metadata. let absolute_target = if target_path.is_relative() { match path.path().parent() { - Some(p) => p.join(&target_path), - None => target_path.clone(), + Some(p) => &p.join(&target_path), + None => &target_path, } } else { - target_path.clone() + &target_path }; - match fs::canonicalize(&absolute_target) { + match fs::canonicalize(absolute_target) { Ok(resolved_target) => { let target_data = PathData::new( resolved_target.as_path().into(), @@ -750,27 +749,32 @@ fn display_item_name( false, ); - let mut target_display = escaped_target.clone(); - if let Some(style_manager) = &mut style_manager { - let md_option: Option = target_data - .metadata() - .cloned() - .or_else(|| target_data.p_buf.symlink_metadata().ok()); - + let target_display = if let Some(style_manager) = style_manager { + let md = match target_data.metadata() { + Some(md) => Some(Cow::Borrowed(md)), + None => target_data.p_buf.symlink_metadata().ok().map(Cow::Owned), + }; + // Check if the target actually needs coloring if style_manager .colors - .style_for_path_with_metadata(&target_data.p_buf, md_option.as_ref()) + .style_for_path_with_metadata(&target_data.p_buf, md.as_deref()) .is_some() { - target_display = color_name( - escaped_target.clone(), + // Only apply coloring if there's actually a style + &color_name( + escaped_target, &target_data, style_manager, None, is_wrap(name.len()), - ); + ) + } else { + // For regular files with no coloring, just use plain text + &escaped_target } - } + } else { + &escaped_target + }; name.push(target_display); if let Some(c) = indicator_char(&target_data, config.indicator_style) { @@ -786,6 +790,8 @@ fn display_item_name( ), ); } else { + // If no coloring is required, we just use target as is. + // with the right quoting name.push(escaped_target); } } @@ -1103,7 +1109,8 @@ fn display_item_long( Ok(()) } -fn indicator_char(path: &PathData, style: IndicatorStyle) -> Option { +fn indicator_char(path: &PathData, style: Option) -> Option { + let style = style?; let sym = classify_file(path); match style { @@ -1116,7 +1123,6 @@ fn indicator_char(path: &PathData, style: IndicatorStyle) -> Option { Some('/') => Some('/'), _ => None, }, - IndicatorStyle::None => None, } }