From 9bc14a239c9dfb826a8f8681da03f299390259dc Mon Sep 17 00:00:00 2001 From: Mahmoud Soltan Date: Sun, 5 Sep 2021 13:25:56 +0200 Subject: [PATCH] Added support for `ls -l --color` to color symlink targets as well. (#2627) * Fixed some documentation of display_item_long that was missed in #2623 * Implemented coloring of `ls -l` symlink targets( after the arrow `->`). * Documented display_file_name to some extent. * Ran rustfmt as part of mitigating CI chain errors. * Removed unused variables and code in test_ls_long_format as per #2623 specifically, as per https://github.com/uutils/coreutils/pull/2623#pullrequestreview-742386707 * Added a thorough test for `ls -laR --color` symlink coloring implemented in this branch. * renamed test files and dirs to shorter names and ran rustfmt. * Changed the order with which files are expected to match the change in their name. * Bettered some comments. * Removed needless borrow. Fixed that one clippy warning. * Moved the cfg not windows up to the function level because this function is meant to only run in non-windows OS (with groups and unames). Fixes the unused variable warning in CI. --- src/uu/ls/src/ls.rs | 63 +++++++--- tests/by-util/test_ls.rs | 247 +++++++++++++++++++++++++++++++++++---- 2 files changed, 273 insertions(+), 37 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index fcb0b28ff..6f63c2a4a 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1570,22 +1570,12 @@ fn display_grid( /// /// That's why we have the parameters: /// ```txt -/// max_links: usize, +/// longest_link_count_len: usize, /// longest_uname_len: usize, /// longest_group_len: usize, -/// max_size: usize, +/// longest_size_len: usize, /// ``` /// that decide the maximum possible character count of each field. -/// -/// [`get_inode`]: ls::get_inode -/// [`display_permissions`]: ls::display_permissions -/// [`display_symlink_count`]: ls::display_symlink_count -/// [`display_uname`]: ls::display_uname -/// [`display_group`]: ls::display_group -/// [`display_size_or_rdev`]: ls::display_size_or_rdev -/// [`get_system_time`]: ls::get_system_time -/// [`display_file_name`]: ls::display_file_name -/// [`pad_left`]: ls::pad_left fn display_item_long( item: &PathData, longest_link_count_len: usize, @@ -1866,7 +1856,20 @@ fn classify_file(path: &PathData) -> Option { } } +/// Takes a [`PathData`] struct and returns a cell with a name ready for displaying. +/// +/// This function relies on the following parameters in the provided `&Config`: +/// * `config.quoting_style` to decide how we will escape `name` using [`escape_name`]. +/// * `config.inode` decides whether to display inode numbers beside names using [`get_inode`]. +/// * `config.color` decides whether it's going to color `name` using [`color_name`]. +/// * `config.indicator_style` to append specific characters to `name` using [`classify_file`]. +/// * `config.format` to display symlink targets if `Format::Long`. This function is also +/// responsible for coloring symlink target names if `config.color` is specified. +/// +/// Note that non-unicode sequences in symlink targets are dealt with using +/// [`std::path::Path::to_string_lossy`]. fn display_file_name(path: &PathData, config: &Config) -> Option { + // This is our return value. We start by `&path.display_name` and modify it along the way. let mut name = escape_name(&path.display_name, &config.quoting_style); #[cfg(unix)] @@ -1919,7 +1922,41 @@ fn display_file_name(path: &PathData, config: &Config) -> Option { if config.format == Format::Long && path.file_type()?.is_symlink() { if let Ok(target) = path.p_buf.read_link() { name.push_str(" -> "); - name.push_str(&target.to_string_lossy()); + + // 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(ls_colors) = &config.color { + // 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() { + if let Some(parent) = path.p_buf.parent() { + absolute_target = parent.join(absolute_target); + } + } + + let target_data = PathData::new(absolute_target, None, None, config, false); + + // If we have a symlink to a valid file, we use the metadata of said file. + // 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. + let target_metadata = match target_data.md() { + Some(md) => md, + None => path.md()?, + }; + + name.push_str(&color_name( + ls_colors, + &target_data.p_buf, + target.to_string_lossy().into_owned(), + target_metadata, + )); + } else { + // If no coloring is required, we just use target as is. + name.push_str(&target.to_string_lossy()); + } } } diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index e6c23acc1..3d6092416 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -1,4 +1,4 @@ -// spell-checker:ignore (words) READMECAREFULLY birthtime doesntexist oneline somebackup somefile somegroup somehiddenbackup somehiddenfile +// spell-checker:ignore (words) READMECAREFULLY birthtime doesntexist oneline somebackup lrwx somefile somegroup somehiddenbackup somehiddenfile #[cfg(unix)] extern crate unix_socket; @@ -333,20 +333,9 @@ fn test_ls_long() { } } +#[cfg(not(windows))] #[test] fn test_ls_long_format() { - #[cfg(not(windows))] - let last; - #[cfg(not(windows))] - { - let _guard = UMASK_MUTEX.lock(); - last = unsafe { umask(0) }; - - unsafe { - umask(0o002); - } - } - let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; at.mkdir(&at.plus_as_string("test-long-dir")); @@ -354,8 +343,6 @@ fn test_ls_long_format() { at.mkdir(&at.plus_as_string("test-long-dir/test-long-dir")); for arg in &["-l", "--long", "--format=long", "--format=verbose"] { - #[allow(unused_variables)] - let result = scene.ucmd().arg(arg).arg("test-long-dir").succeeds(); // Assuming sane username do not have spaces within them. // A line of the output should be: // One of the characters -bcCdDlMnpPsStTx? @@ -368,26 +355,238 @@ fn test_ls_long_format() { // Either a year or a time, currently [0-9:]+, preceded by column whitespace, // and followed by a single space. // Whatever comes after is irrelevant to this specific test. - #[cfg(not(windows))] - result.stdout_matches(&Regex::new( + scene.ucmd().arg(arg).arg("test-long-dir").succeeds().stdout_matches(&Regex::new( r"\n[-bcCdDlMnpPsStTx?]([r-][w-][xt-]){3} +\d+ [^ ]+ +[^ ]+( +[^ ]+)? +\d+ [A-Z][a-z]{2} {0,2}\d{0,2} {0,2}[0-9:]+ " ).unwrap()); } - #[allow(unused_variables)] - let result = scene.ucmd().arg("-lan").arg("test-long-dir").succeeds(); // This checks for the line with the .. entry. The uname and group should be digits. - #[cfg(not(windows))] - result.stdout_matches(&Regex::new( + scene.ucmd().arg("-lan").arg("test-long-dir").succeeds().stdout_matches(&Regex::new( r"\nd([r-][w-][xt-]){3} +\d+ \d+ +\d+( +\d+)? +\d+ [A-Z][a-z]{2} {0,2}\d{0,2} {0,2}[0-9:]+ \.\." ).unwrap()); +} - #[cfg(not(windows))] +/// This test tests `ls -laR --color`. +/// This test is mainly about coloring, but, the recursion, symlink `->` processing, +/// and `.` and `..` being present in `-a` all need to work for the test to pass. +/// This test does not really test anything provided by `-l` but the file names and symlinks. +#[test] +fn test_ls_long_symlink_color() { + // If you break this test after breaking mkdir, touch, or ln, do not be alarmed! + // This test is made for ls, but it attempts to run those utils in the process. + + // Having Some([2, 0]) in a color basically means that "it has the same color as whatever + // is in the 2nd expected output, the 0th color", where the 0th color is the name color, and + // the 1st color is the target color, in a fixed-size array of size 2. + // Basically these are references to be used for indexing the `colors` vector defined below. + type ColorReference = Option<[usize; 2]>; + + // The string between \x1b[ and m + type Color = String; + + // The string between the color start and the color end is the file name itself. + type Name = String; + + let scene = TestScenario::new(util_name!()); + + // . + // ├── dir1 + // │ ├── file1 + // │ ├── dir2 + // │ │ └── dir3 + // │ ├── ln-dir-invalid -> dir1/dir2 + // │ ├── ln-up2 -> ../.. + // │ └── ln-root -> / + // ├── ln-file1 -> dir1/file1 + // ├── ln-file-invalid -> dir1/invalid-target + // └── ln-dir3 -> ./dir1/dir2/dir3 + prepare_folder_structure(&scene); + + // We memoize the colors so we can refer to them later. + // Each entry will be the colors of the link name and link target of a specific output. + let mut colors: Vec<[Color; 2]> = vec![]; + + // The contents of each tuple are the expected colors and names for the link and target. + // We will loop over the ls output and compare to those. + // None values mean that we do not know what color to expect yet, as LS_COLOR might + // be set differently, and as different implementations of ls may use different codes, + // for example, our ls uses `[1;36m` while the GNU ls uses `[01;36m`. + // + // These have been sorting according to default ls sort, and this affects the order of + // discovery of colors, so be very careful when changing directory/file names being created. + let expected_output: [(ColorReference, &str, ColorReference, &str); 6] = [ + // We don't know what colors are what the first time we meet a link. + (None, "ln-dir3", None, "./dir1/dir2/dir3"), + // We have acquired [0, 0], which should be the link color, + // and [0, 1], which should be the dir color, and we can compare to them from now on. + (None, "ln-file-invalid", Some([1, 1]), "dir1/invalid-target"), + // We acquired [1, 1], the non-existent color. + (Some([0, 0]), "ln-file1", None, "dir1/file1"), + (Some([1, 1]), "ln-dir-invalid", Some([1, 1]), "dir1/dir2"), + (Some([0, 0]), "ln-root", Some([0, 1]), "/"), + (Some([0, 0]), "ln-up2", Some([0, 1]), "../.."), + ]; + + // 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(); + let mut result_lines = result + .stdout_str() + .lines() + .filter_map(|line| match line.starts_with("lrwx") { + true => Some(line), + false => None, + }) + .enumerate(); + + // For each enumerated line, we assert that the output of ls matches the expected output. + // + // The unwraps within get_index_name_target will panic if a line starting lrwx does + // not have `colored_name -> target` within it. + while let Some((i, name, target)) = get_index_name_target(&mut result_lines) { + // The unwraps within capture_colored_string will panic if the name/target's color + // format is invalid. + let (matched_name_color, matched_name) = capture_colored_string(&name); + let (matched_target_color, matched_target) = capture_colored_string(&target); + + colors.push([matched_name_color, matched_target_color]); + + // We borrow them again after having moved them. This unwrap will never panic. + let [matched_name_color, matched_target_color] = colors.last().unwrap(); + + // We look up the Colors that are expected in `colors` using the ColorReferences + // stored in `expected_output`. + let expected_name_color = match expected_output[i].0 { + Some(color_reference) => Some(colors[color_reference[0]][color_reference[1]].as_str()), + None => None, + }; + let expected_target_color = match expected_output[i].2 { + Some(color_reference) => Some(colors[color_reference[0]][color_reference[1]].as_str()), + None => None, + }; + + // This is the important part. The asserts inside assert_names_and_colors_are_equal + // will panic if the colors or names do not match the expected colors or names. + // Keep in mind an expected color `Option<&str>` of None can mean either that we + // don't expect any color here, as in `expected_output[2], or don't know what specific + // color to expect yet, as in expected_output[0:1]. + assert_names_and_colors_are_equal( + &matched_name_color, + expected_name_color, + &matched_name, + expected_output[i].1, + &matched_target_color, + expected_target_color, + &matched_target, + expected_output[i].3, + ); + } + + // End of test, only definitions of the helper functions used above follows... + + fn get_index_name_target<'a, I>(lines: &mut I) -> Option<(usize, Name, Name)> + where + I: Iterator, { - unsafe { - umask(last); + match lines.next() { + Some((c, s)) => { + // `name` is whatever comes between \x1b (inclusive) and the arrow. + let name = String::from("\x1b") + + s.split(" -> ") + .next() + .unwrap() + .split(" \x1b") + .last() + .unwrap(); + // `target` is whatever comes after the arrow. + let target = s.split(" -> ").last().unwrap().to_string(); + Some((c, name, target)) + } + None => None, } } + + fn assert_names_and_colors_are_equal( + name_color: &str, + expected_name_color: Option<&str>, + name: &str, + expected_name: &str, + target_color: &str, + expected_target_color: Option<&str>, + target: &str, + expected_target: &str, + ) { + // Names are always compared. + assert_eq!(&name, &expected_name); + assert_eq!(&target, &expected_target); + + // Colors are only compared when we have inferred what color we are looking for. + if expected_name_color.is_some() { + assert_eq!(&name_color, &expected_name_color.unwrap()); + } + if expected_target_color.is_some() { + assert_eq!(&target_color, &expected_target_color.unwrap()); + } + } + + fn capture_colored_string(input: &str) -> (Color, Name) { + let colored_name = Regex::new(r"\x1b\[([0-9;]+)m(.+)\x1b\[0m").unwrap(); + match colored_name.captures(&input) { + Some(captures) => ( + captures.get(1).unwrap().as_str().to_string(), + captures.get(2).unwrap().as_str().to_string(), + ), + None => ("".to_string(), input.to_string()), + } + } + + fn prepare_folder_structure(scene: &TestScenario) { + // There is no way to change directory in the CI, so this is the best we can do. + // Also, keep in mind that windows might require privilege to symlink directories. + // + // We use scene.ccmd instead of scene.fixtures because we care about relative symlinks. + // So we're going to try out the built mkdir, touch, and ln here, and we expect them to succeed. + scene.ccmd("mkdir").arg("dir1").succeeds(); + scene.ccmd("mkdir").arg("dir1/dir2").succeeds(); + scene.ccmd("mkdir").arg("dir1/dir2/dir3").succeeds(); + scene.ccmd("touch").arg("dir1/file1").succeeds(); + + scene + .ccmd("ln") + .arg("-s") + .arg("dir1/dir2") + .arg("dir1/ln-dir-invalid") + .succeeds(); + scene + .ccmd("ln") + .arg("-s") + .arg("./dir1/dir2/dir3") + .arg("ln-dir3") + .succeeds(); + scene + .ccmd("ln") + .arg("-s") + .arg("../..") + .arg("dir1/ln-up2") + .succeeds(); + scene + .ccmd("ln") + .arg("-s") + .arg("/") + .arg("dir1/ln-root") + .succeeds(); + scene + .ccmd("ln") + .arg("-s") + .arg("dir1/file1") + .arg("ln-file1") + .succeeds(); + scene + .ccmd("ln") + .arg("-s") + .arg("dir1/invalid-target") + .arg("ln-file-invalid") + .succeeds(); + } } #[test]