diff --git a/src/uu/ls/src/config.rs b/src/uu/ls/src/config.rs index 383d3645148..3a879b83735 100644 --- a/src/uu/ls/src/config.rs +++ b/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, - 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, @@ -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 { 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 } } @@ -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 diff --git a/src/uu/ls/src/display.rs b/src/uu/ls/src/display.rs index 8823969b4f3..d258d2d9c67 100644 --- a/src/uu/ls/src/display.rs +++ b/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 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); } } 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 = 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) -> Option { + 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 { let file_type = path.file_type()?; diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 7afedc35a42..55e9b22e112 100644 --- a/tests/by-util/test_ls.rs +++ b/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[0-9;]*)[m]link\x1b\[0m -> \x1b\[(?P[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();