From b676216f9b54f2fc38ad07bc6019417e306dd60d Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 30 Nov 2023 11:04:14 +0100 Subject: [PATCH 1/4] ls: use the gnu_legacy feature from lscolors --- Cargo.toml | 1 + src/uu/ls/Cargo.toml | 2 +- src/uu/ls/src/ls.rs | 6 ++- tests/by-util/test_ls.rs | 88 ++++++++++++++++++++++++---------------- 4 files changed, 60 insertions(+), 37 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f88285a1f..abc3f26b0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -290,6 +290,7 @@ itertools = "0.12.0" libc = "0.2.150" lscolors = { version = "0.16.0", default-features = false, features = [ "nu-ansi-term", + "gnu_legacy", ] } memchr = "2" memmap2 = "0.9" diff --git a/src/uu/ls/Cargo.toml b/src/uu/ls/Cargo.toml index a82a1f37e..56b642a1a 100644 --- a/src/uu/ls/Cargo.toml +++ b/src/uu/ls/Cargo.toml @@ -22,7 +22,7 @@ number_prefix = { workspace = true } uutils_term_grid = { workspace = true } terminal_size = { workspace = true } glob = { workspace = true } -lscolors = { workspace = true } +lscolors = { workspace = true, features = ["gnu_legacy"] } uucore = { workspace = true, features = [ "entries", "fs", diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index deb8aac3d..d93620967 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3140,7 +3140,11 @@ fn display_file_name( fn color_name(name: String, path: &Path, md: Option<&Metadata>, ls_colors: &LsColors) -> String { match ls_colors.style_for_path_with_metadata(path, md) { Some(style) => { - return style.to_nu_ansi_term_style().paint(name).to_string(); + return style + .to_nu_ansi_term_style() + .reset_before_style() + .paint(name) + .to_string(); } None => name, } diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 8bc2b75ac..5061778ae 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -2,7 +2,7 @@ // // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (words) READMECAREFULLY birthtime doesntexist oneline somebackup lrwx somefile somegroup somehiddenbackup somehiddenfile tabsize aaaaaaaa bbbb cccc dddddddd ncccc neee naaaaa nbcdef nfffff dired subdired tmpfs +// spell-checker:ignore (words) READMECAREFULLY birthtime doesntexist oneline somebackup lrwx somefile somegroup somehiddenbackup somehiddenfile tabsize aaaaaaaa bbbb cccc dddddddd ncccc neee naaaaa nbcdef nfffff dired subdired tmpfs mdir1 #[cfg(any(unix, feature = "feat_selinux"))] use crate::common::util::expected_result; @@ -864,11 +864,11 @@ fn test_ls_zero() { .succeeds() .stdout_only("\"0-test-zero\"\x00\"2-test-zero\"\x00\"3-test-zero\"\x00"); - scene - .ucmd() - .args(&["--zero", "--color=always"]) - .succeeds() - .stdout_only("\x1b[1;34m0-test-zero\x1b[0m\x002-test-zero\x003-test-zero\x00"); + let result = scene.ucmd().args(&["--zero", "--color=always"]).succeeds(); + assert_eq!( + result.stdout_str(), + "\u{1b}[0m\u{1b}[01;34m0-test-zero\x1b[0m\x002-test-zero\x003-test-zero\x00" + ); scene .ucmd() @@ -921,12 +921,9 @@ fn test_ls_zero() { "\"0-test-zero\"\x00\"1\\ntest-zero\"\x00\"2-test-zero\"\x00\"3-test-zero\"\x00", ); - scene - .ucmd() - .args(&["--zero", "--color=always"]) - .succeeds() - .stdout_only( - "\x1b[1;34m0-test-zero\x1b[0m\x001\ntest-zero\x002-test-zero\x003-test-zero\x00", + let result = scene.ucmd().args(&["--zero", "--color=always"]).succeeds(); + assert_eq!(result.stdout_str(), + "\u{1b}[0m\u{1b}[01;34m0-test-zero\x1b[0m\x001\ntest-zero\x002-test-zero\x003-test-zero\x00", ); scene @@ -1202,12 +1199,21 @@ fn test_ls_long_symlink_color() { } fn capture_colored_string(input: &str) -> (Color, Name) { - let colored_name = Regex::new(r"\x1b\[([0-9;]+)m(.+)\x1b\[0m").unwrap(); + // Input can be: + // \u{1b}[0m\u{1b}[01;36mln-dir3\u{1b}[0m + // \u{1b}[0m\u{1b}[01;34m./dir1/dir2/dir3\u{1b}[0m + // \u{1b}[0m\u{1b}[01;36mln-file-invalid\u{1b}[0m + // \u{1b}[01;36mdir1/invalid-target\u{1b}[0m + let colored_name = Regex::new(r"(?:\x1b\[0m\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(), - ), + Some(captures) => { + dbg!(captures.get(1).unwrap().as_str().to_string()); + dbg!(captures.get(2).unwrap().as_str().to_string()); + return ( + captures.get(1).unwrap().as_str().to_string(), + captures.get(2).unwrap().as_str().to_string(), + ); + } None => (String::new(), input.to_string()), } } @@ -1977,6 +1983,20 @@ fn test_ls_recursive_1() { .stdout_is(out); } +// Function to convert a string to its ASCII representation +fn to_ascii_representation(input: &str) -> String { + input + .chars() + .map(|c| { + if c.is_ascii_control() || !c.is_ascii() { + format!("\\x{:02x}", c as u32) + } else { + c.to_string() + } + }) + .collect::() +} + #[test] fn test_ls_color() { let scene = TestScenario::new(util_name!()); @@ -1995,9 +2015,9 @@ fn test_ls_color() { at.touch(nested_file); at.touch("test-color"); - let a_with_colors = "\x1b[1;34ma\x1b[0m"; - let z_with_colors = "\x1b[1;34mz\x1b[0m"; - let nested_dir_with_colors = "\x1b[1;34mnested_dir\x1b[0m"; // spell-checker:disable-line + let a_with_colors = "\x1b[0m\x1b[01;34ma\x1b[0m"; + let z_with_colors = "\x1b[01;34mz\x1b[0m\n"; + let nested_dir_with_colors = "\x1b[0m\x1b[01;34mnested_dir\x1b[0m\x0anested_file"; // spell-checker:disable-line // Color is disabled by default let result = scene.ucmd().succeeds(); @@ -2006,12 +2026,9 @@ fn test_ls_color() { // Color should be enabled for param in ["--color", "--col", "--color=always", "--col=always"] { - scene - .ucmd() - .arg(param) - .succeeds() - .stdout_contains(a_with_colors) - .stdout_contains(z_with_colors); + let result = scene.ucmd().arg(param).succeeds(); + assert!(result.stdout_str().contains(a_with_colors)); + assert!(result.stdout_str().contains(z_with_colors)); } // Color should be disabled @@ -2020,12 +2037,8 @@ fn test_ls_color() { assert!(!result.stdout_str().contains(z_with_colors)); // Nested dir should be shown and colored - scene - .ucmd() - .arg("--color") - .arg("a") - .succeeds() - .stdout_contains(nested_dir_with_colors); + let result = scene.ucmd().arg("--color").arg("a").succeeds(); + assert!(result.stdout_str().contains(nested_dir_with_colors)); // No output scene @@ -2037,13 +2050,18 @@ fn test_ls_color() { // The colors must not mess up the grid layout at.touch("b"); - scene + let result = scene .ucmd() .arg("--color") .arg("-w=15") .arg("-C") - .succeeds() - .stdout_only(format!("{a_with_colors} test-color\nb {z_with_colors}\n")); + .succeeds(); + let expected = format!("{} test-color\x0ab {}", a_with_colors, z_with_colors); + assert_eq!( + to_ascii_representation(result.stdout_str()), + to_ascii_representation(&expected) + ); + assert_eq!(result.stdout_str(), expected); } #[cfg(unix)] From 268b180416d0249b232a762ff408ffe8a29babb4 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sat, 9 Dec 2023 13:06:09 +0100 Subject: [PATCH 2/4] ls colors: create a stylemanager to carry the previous style to know if we need to reset or not --- src/uu/ls/src/ls.rs | 88 ++++++++++++++++++++++++++++++++-------- tests/by-util/test_ls.rs | 20 +++++++++ 2 files changed, 91 insertions(+), 17 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index d93620967..e3176ec72 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -10,7 +10,8 @@ use clap::{ crate_version, Arg, ArgAction, Command, }; use glob::{MatchOptions, Pattern}; -use lscolors::LsColors; +use lscolors::{LsColors, Style}; + use number_prefix::NumberPrefix; use std::{cell::OnceCell, num::IntErrorKind}; use std::{collections::HashSet, io::IsTerminal}; @@ -1900,6 +1901,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { let mut dirs = Vec::::new(); let mut out = BufWriter::new(stdout()); let mut dired = DiredOutput::default(); + let mut style_manager = StyleManager::new(); let initial_locs_len = locs.len(); for loc in locs { @@ -1933,7 +1935,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { sort_entries(&mut files, config, &mut out); sort_entries(&mut dirs, config, &mut out); - display_items(&files, config, &mut out, &mut dired)?; + display_items(&files, config, &mut out, &mut dired, &mut style_manager)?; for (pos, path_data) in dirs.iter().enumerate() { // Do read_dir call here to match GNU semantics by printing @@ -1985,6 +1987,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { &mut out, &mut listed_ancestors, &mut dired, + &mut style_manager, )?; } if config.dired { @@ -2101,6 +2104,7 @@ fn enter_directory( out: &mut BufWriter, listed_ancestors: &mut HashSet, dired: &mut DiredOutput, + style_manager: &mut StyleManager, ) -> UResult<()> { // Create vec of entries with initial dot files let mut entries: Vec = if config.files == Files::All { @@ -2153,7 +2157,7 @@ fn enter_directory( } } - display_items(&entries, config, out, dired)?; + display_items(&entries, config, out, dired, style_manager)?; if config.recursive { for e in entries @@ -2194,7 +2198,15 @@ fn enter_directory( show_dir_name(&e.p_buf, out); writeln!(out)?; - enter_directory(e, rd, config, out, listed_ancestors, dired)?; + enter_directory( + e, + rd, + config, + out, + listed_ancestors, + dired, + style_manager, + )?; listed_ancestors .remove(&FileInformation::from_path(&e.p_buf, e.must_dereference)?); } else { @@ -2316,6 +2328,7 @@ fn display_items( config: &Config, out: &mut BufWriter, dired: &mut DiredOutput, + style_manager: &mut StyleManager, ) -> UResult<()> { // `-Z`, `--context`: // Display the SELinux security context or '?' if none is found. When used with the `-l` @@ -2338,7 +2351,7 @@ fn display_items( display_additional_leading_info(item, &padding_collection, config, out)?; write!(out, "{more_info}")?; } - display_item_long(item, &padding_collection, config, out, dired)?; + display_item_long(item, &padding_collection, config, out, dired, style_manager)?; } } else { let mut longest_context_len = 1; @@ -2358,7 +2371,7 @@ fn display_items( for i in items { let more_info = display_additional_leading_info(i, &padding, config, out)?; - let cell = display_file_name(i, config, prefix_context, more_info, out); + let cell = display_file_name(i, config, prefix_context, more_info, out, style_manager); names_vec.push(cell); } @@ -2513,6 +2526,7 @@ fn display_item_long( config: &Config, out: &mut BufWriter, dired: &mut DiredOutput, + style_manager: &mut StyleManager, ) -> UResult<()> { let mut output_display: String = String::new(); if config.dired { @@ -2605,7 +2619,8 @@ fn display_item_long( write!(output_display, " {} ", display_date(md, config)).unwrap(); - let displayed_file = display_file_name(item, config, None, String::new(), out).contents; + let displayed_file = + display_file_name(item, config, None, String::new(), out, style_manager).contents; if config.dired { let (start, end) = dired::calculate_dired( &dired.dired_positions, @@ -2687,7 +2702,8 @@ fn display_item_long( write!(output_display, " {}", pad_right("?", padding.uname)).unwrap(); } - let displayed_file = display_file_name(item, config, None, String::new(), out).contents; + let displayed_file = + display_file_name(item, config, None, String::new(), out, style_manager).contents; let date_len = 12; write!( @@ -2985,6 +3001,7 @@ fn display_file_name( prefix_context: Option, more_info: String, out: &mut BufWriter, + style_manager: &mut StyleManager, ) -> Cell { // 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); @@ -3008,13 +3025,14 @@ fn display_file_name( if let Some(ls_colors) = &config.color { let md = path.md(out); name = if md.is_some() { - color_name(name, &path.p_buf, md, ls_colors) + color_name(name, &path.p_buf, md, ls_colors, style_manager) } else { color_name( name, &path.p_buf, path.p_buf.symlink_metadata().ok().as_ref(), ls_colors, + style_manager, ) }; } @@ -3103,6 +3121,7 @@ fn display_file_name( &target_data.p_buf, Some(&target_metadata), ls_colors, + style_manager, )); } } else { @@ -3137,15 +3156,50 @@ fn display_file_name( } } -fn color_name(name: String, path: &Path, md: Option<&Metadata>, ls_colors: &LsColors) -> String { - match ls_colors.style_for_path_with_metadata(path, md) { - Some(style) => { - return style - .to_nu_ansi_term_style() - .reset_before_style() - .paint(name) - .to_string(); +/// We need this struct to be able to store the previous style. +/// This because we need to check the previous value in case we don't need +/// the reset +struct StyleManager { + current_style: Option