ls: ls -lF symlink target indicators (#11554)

---------

Co-authored-by: Guillem L. Jara <4lon3ly0@tutanota.com>
Co-authored-by: Sylvestre Ledru <sylvestre@debian.org>

src/uu/ls/src/config.rs

@@ -216,7 +216,7 @@ pub struct Config {
     // Dir and vdir needs access to this field
     pub quoting_style: QuotingStyle,
     pub(crate) locale_quoting: Option<LocaleQuoting>,
-    pub(crate) indicator_style: IndicatorStyle,
+    pub(crate) indicator_style: Option<IndicatorStyle>,
     pub(crate) time_format_recent: String, // Time format for recent dates
     pub(crate) time_format_older: Option<String>, // Time format for older dates (optional, if not present, time_format_recent is used)
     pub(crate) context: bool,
@@ -598,34 +598,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<IndicatorStyle> {
     if let Some(field) = options.get_one::<String>(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::<String>(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
     }
 }
 
@@ -957,7 +951,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

src/uu/ls/src/display.rs

@@ -52,6 +52,7 @@ use crate::colors::{StyleManager, color_name};
 use crate::config::Files;
 use crate::dired::{self, DiredOutput};
 use crate::{Config, ListState, LsError, PathData, get_block_size};
+use lscolors::Indicator;
 
 /// Width of the standard Unix permissions string (one file-type char plus
 /// nine permission bits, e.g. `drwxr-xr-x`).
@@ -91,9 +92,8 @@ pub(crate) struct DisplayItemName {
     pub(crate) dired_name_len: usize,
 }
 
-#[derive(PartialEq, Eq)]
+#[derive(Clone, Copy, PartialEq, Eq)]
 pub(crate) enum IndicatorStyle {
-    None,
     Slash,
     FileType,
     Classify,
@@ -705,35 +705,31 @@ fn display_item_name(
         }
     }
 
-    if config.indicator_style != IndicatorStyle::None {
-        let sym = classify_file(path);
+    let is_long_symlink = config.format == Format::Long
+        && path.file_type().is_some_and(FileType::is_symlink)
+        && !path.must_dereference;
 
-        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,
-        };
-
-        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);
         }
     }
 
     let dired_name_len = if config.dired { name.len() } else { 0 };
 
+    let has_mi_or_or = style_manager.as_ref().is_some_and(|sm| {
+        sm.has_indicator_style(Indicator::OrphanedSymbolicLink)
+            || sm.has_indicator_style(Indicator::MissingFile)
+    });
+    // Only stat symlink target when:
+    // 1. Color is enabled AND LS_COLORS has mi= or or=, OR
+    // 2. Long format AND (--classify or --file-type)
+    let should_stat_target = has_mi_or_or
+        || matches!(
+            config.indicator_style,
+            Some(IndicatorStyle::Classify) | Some(IndicatorStyle::FileType)
+        );
+
     if config.format == Format::Long
         && path.file_type().is_some_and(FileType::is_symlink)
         && !path.must_dereference
@@ -745,10 +741,11 @@ 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 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.
+                if should_stat_target {
                     let absolute_target = if target_path.is_relative() {
                         match path.path().parent() {
                             Some(p) => &p.join(&target_path),
@@ -757,7 +754,6 @@ fn display_item_name(
                     } else {
                         &target_path
                     };
-
                     match fs::canonicalize(absolute_target) {
                         Ok(resolved_target) => {
                             let target_data = PathData::new(
@@ -769,43 +765,62 @@ fn display_item_name(
                                 false,
                             );
 
-                            // Check if the target actually needs coloring
-                            let md_option: Option<Metadata> = 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,
-                                    &target_data,
-                                    style_manager,
-                                    None,
-                                    is_wrap(name.len()),
-                                ));
+                            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.as_deref())
+                                    .is_some()
+                                {
+                                    // 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 {
-                                // For regular files with no coloring, just use plain text
-                                name.push(escaped_target);
+                                escaped_target
+                            };
+                            name.push(target_display);
+                            // Add appropriate indicator based on indicator_style
+                            if let Some(c) = indicator_char(&target_data, config.indicator_style) {
+                                if matches!(
+                                    config.indicator_style,
+                                    Some(IndicatorStyle::Classify)
+                                        | Some(IndicatorStyle::FileType)
+                                        | Some(IndicatorStyle::Slash)
+                                ) {
+                                    let _ = name.write_char(c);
+                                }
                             }
                         }
                         Err(_) => {
-                            name.push(
-                                style_manager.apply_missing_target_style(
+                            if let Some(style_manager) = &mut style_manager {
+                                name.push(style_manager.apply_missing_target_style(
                                     escaped_target,
                                     is_wrap(name.len()),
-                                ),
-                            );
+                                ));
+                            } else {
+                                // If no coloring is required, we just use target as is.
+                                // with the right quoting
+                                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));
+                    name.push(&escaped_target);
                 }
             }
             Err(err) => {
@@ -1118,6 +1133,23 @@ fn display_item_long(
     Ok(())
 }
 
+fn indicator_char(path: &PathData, style: Option<IndicatorStyle>) -> Option<char> {
+    let style = style?;
+    let sym = classify_file(path);
+
+    match style {
+        IndicatorStyle::Classify => sym,
+        IndicatorStyle::FileType => match sym {
+            Some('*') => None,
+            _ => sym,
+        },
+        IndicatorStyle::Slash => match sym {
+            Some('/') => Some('/'),
+            _ => None,
+        },
+    }
+}
+
 fn classify_file(path: &PathData) -> Option<char> {
     let file_type = path.file_type()?;
 

tests/by-util/test_ls.rs

@@ -1280,7 +1280,14 @@ fn test_ls_long_symlink_color() {
     ];
 
     // We are only interested in lines or the ls output that are symlinks. These start with "lrwx".
-    let result = scene.ucmd().arg("-laR").arg("--color").arg(".").succeeds();
+    // Use --file-type to ensure symlink targets are stat'd and colored
+    let result = scene
+        .ucmd()
+        .arg("-laR")
+        .arg("--color")
+        .arg("--file-type")
+        .arg(".")
+        .succeeds();
     let mut result_lines = result
         .stdout_str()
         .lines()
@@ -1521,6 +1528,41 @@ fn test_ls_dangling_symlink_or_and_missing_colors() {
     assert_eq!(captures.name("target").unwrap().as_str(), "34");
 }
 
+#[test]
+#[cfg(unix)]
+fn test_ls_symlink_to_dir_with_mi_colors() {
+    // When LS_COLORS contains mi=, ln=, di=, ls -lp should stat the symlink target,
+    // color the link with ln color, the target with di color, and append '/' to the target.
+    use std::os::unix::fs::symlink;
+
+    let ts = TestScenario::new(util_name!());
+    let at = &ts.fixtures;
+    at.mkdir("target_dir");
+    symlink("target_dir", at.plus("link")).unwrap();
+
+    let stdout = ts
+        .ucmd()
+        .env("LS_COLORS", "mi=41:ln=1;36:di=1;34")
+        .arg("-lp")
+        .arg("--color=always")
+        .arg("link")
+        .succeeds()
+        .stdout_str()
+        .to_string();
+
+    // Regex to capture link color and target color
+    let color_regex = Regex::new(
+        r"\x1b\[0m\x1b\[(?P<link>[0-9;]*)[m]link\x1b\[0m -> \x1b\[(?P<target>[0-9;]*)[m]target_dir\x1b\[0m/",
+    )
+    .unwrap();
+    let captures = color_regex
+        .captures(&stdout)
+        .expect("failed to capture symlink colors");
+
+    assert_eq!(captures.name("link").unwrap().as_str(), "1;36");
+    assert_eq!(captures.name("target").unwrap().as_str(), "1;34");
+}
+
 #[test]
 /// Mirrors GNU `tests/ls/ls-misc.pl::sl-dangle4`.
 fn test_ls_dangling_symlink_ln_or_priority() {
@@ -3400,6 +3442,110 @@ 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", "dir_link");
+    assert!(at.is_symlink("dir_link"));
+
+    scene
+        .ucmd()
+        .arg("--classify")
+        .arg("-l")
+        .arg("dir_link")
+        .succeeds()
+        .stdout_contains("dir_link -> ")
+        .stdout_does_not_contain("dir_link@ -> ")
+        .stdout_contains("/dir/");
+}
+
+#[test]
+#[cfg(unix)]
+fn test_ls_indicator_style_filetype_symlink_target_long() {
+    // GNU `ls -l --file-type` does append `/` to a symlink target that resolves to a
+    // directory unlike `ls -lp`
+    use std::os::unix::fs::symlink;
+
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.mkdir("dir");
+    assert!(at.dir_exists("dir"));
+
+    // Use a relative-path symlink so the displayed target matches GNU output.
+    symlink("dir", at.plus("dir_link")).unwrap();
+    assert!(at.is_symlink("dir_link"));
+
+    scene
+        .ucmd()
+        .arg("--file-type")
+        .arg("-l")
+        .arg("dir_link")
+        .succeeds()
+        .stdout_contains("dir_link -> dir/")
+        .stdout_does_not_contain("dir_link/");
+}
+
+#[test]
+#[cfg(unix)]
+fn test_ls_indicator_style_filetype_symlink_to_executable_target_long() {
+    use std::fs;
+    use std::os::unix::fs::{PermissionsExt, symlink};
+
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.touch("exec_target");
+    let mut perms = fs::metadata(at.plus("exec_target")).unwrap().permissions();
+    perms.set_mode(0o755);
+    fs::set_permissions(at.plus("exec_target"), perms).unwrap();
+
+    symlink("exec_target", at.plus("link")).unwrap();
+    assert!(at.is_symlink("link"));
+
+    scene
+        .ucmd()
+        .arg("--file-type")
+        .arg("-l")
+        .arg("link")
+        .succeeds()
+        .stdout_contains("link -> exec_target")
+        .stdout_does_not_contain("exec_target*");
+}
+
+#[test]
+#[cfg(unix)]
+fn test_ls_indicator_style_slash_symlink_target_long() {
+    // GNU `ls -lp` does NOT append `/` to a symlink target that resolves to a
+    // directory — the slash indicator style only applies to real directories.
+    use std::os::unix::fs::symlink;
+
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.mkdir("dir");
+    assert!(at.dir_exists("dir"));
+
+    // Use a relative-path symlink so the displayed target matches GNU output.
+    symlink("dir", at.plus("dir_link")).unwrap();
+    assert!(at.is_symlink("dir_link"));
+
+    scene
+        .ucmd()
+        .arg("-lp")
+        .arg("dir_link")
+        .succeeds()
+        .stdout_contains("dir_link -> dir\n")
+        .stdout_does_not_contain("dir_link/")
+        .stdout_does_not_contain("-> dir/");
+}
+
 // Essentially the same test as above, but only test symlinks and directories,
 // not pipes or sockets.
 #[test]
@@ -5024,7 +5170,7 @@ fn test_symlink_target_extension_color() {
     at.touch("archive.tar.gz");
     at.relative_symlink_file("archive.tar.gz", "link");
     let out = ucmd
-        .env("LS_COLORS", "*.tar.gz=31")
+        .env("LS_COLORS", "*.tar.gz=31:or=33")
         .args(&["-l", "--color=always", "link"])
         .succeeds()
         .stdout_move_str();
@@ -6045,7 +6191,7 @@ fn test_ls_hyperlink_symlink_target_handling() {
 
     let result = scene
         .ucmd()
-        .args(&["-l", "--hyperlink", "--color"])
+        .args(&["-l", "--hyperlink", "--color", "--file-type"])
         .succeeds();
     let output = result.stdout_str();