From c200e3b9e855e337011084422c2e4ee1084b849c Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Mon, 9 Feb 2026 23:36:28 +0100 Subject: [PATCH] ls: fix hyperlink functionality to use correct OSC 8 format and handle symlink targets (#10824) --- src/uu/ls/src/ls.rs | 62 ++++++++++++++------- tests/by-util/test_ls.rs | 114 ++++++++++++++++++++++++++++++++------- 2 files changed, 136 insertions(+), 40 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 0ec9beb40..7365cb4dc 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -3406,13 +3406,30 @@ fn display_item_name( config, false, ); - name.push(color_name( - escaped_target, - &target_data, - style_manager, - None, - is_wrap(name.len()), - )); + + // 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()), + )); + } else { + // For regular files with no coloring, just use plain text + name.push(escaped_target); + } } Err(_) => { name.push( @@ -3463,29 +3480,34 @@ fn create_hyperlink(name: &OsStr, path: &PathData) -> OsString { let hostname = hostname.to_string_lossy(); let absolute_path = fs::canonicalize(path.path()).unwrap_or_default(); - let absolute_path = absolute_path.to_string_lossy(); + // Get bytes for URL encoding in a cross-platform way + let absolute_path_bytes = os_str_as_bytes_lossy(absolute_path.as_os_str()); + + // Create a set of safe ASCII bytes that don't need encoding #[cfg(not(target_os = "windows"))] - let unencoded_chars = "_-.:~/"; + let unencoded_bytes: std::collections::HashSet = "_-.~/".bytes().collect(); #[cfg(target_os = "windows")] - let unencoded_chars = "_-.:~/\\"; + let unencoded_bytes: std::collections::HashSet = "_-.~/\\:".bytes().collect(); - // percentage encoding of path - let absolute_path: String = absolute_path - .chars() - .map(|c| { - if c.is_alphanumeric() || unencoded_chars.contains(c) { - c.to_string() + // Encode at byte level to properly handle UTF-8 sequences and preserve invalid UTF-8 + let full_encoded_path: String = absolute_path_bytes + .iter() + .map(|&b: &u8| { + if b.is_ascii_alphanumeric() || unencoded_bytes.contains(&b) { + (b as char).to_string() } else { - format!("%{:02x}", c as u8) + format!("%{b:02x}") } }) .collect(); - // \x1b = ESC, \x07 = BEL - let mut ret: OsString = format!("\x1b]8;;file://{hostname}{absolute_path}\x07").into(); + // OSC 8 hyperlink format: \x1b]8;;URL\x1b\\TEXT\x1b]8;;\x1b\\ + // \x1b = ESC, \x1b\\ = ESC backslash + let mut ret: OsString = format!("\x1b]8;;file://{hostname}{full_encoded_path}\x1b\\").into(); ret.push(name); - ret.push("\x1b]8;;\x07"); + ret.push("\x1b]8;;\x1b\\"); + ret } diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 56215eac4..37492d80a 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -3,7 +3,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 mdir COLORTERM mexe bcdef mfoo timefile -// spell-checker:ignore (words) fakeroot setcap drwxr bcdlps mdangling mentry +// spell-checker:ignore (words) fakeroot setcap drwxr bcdlps mdangling mentry awith acolons #![allow( clippy::similar_names, clippy::too_many_lines, @@ -5701,19 +5701,15 @@ fn test_ls_hyperlink() { let result = scene.ucmd().arg("--hyperlink").succeeds(); assert!(result.stdout_str().contains("\x1b]8;;file://")); - assert!( - result - .stdout_str() - .contains(&format!("{path}{separator}{file}\x07{file}\x1b]8;;\x07")) - ); + assert!(result.stdout_str().contains(&format!( + "{path}{separator}{file}\x1b\\{file}\x1b]8;;\x1b\\" + ))); let result = scene.ucmd().arg("--hyperlink=always").succeeds(); assert!(result.stdout_str().contains("\x1b]8;;file://")); - assert!( - result - .stdout_str() - .contains(&format!("{path}{separator}{file}\x07{file}\x1b]8;;\x07")) - ); + assert!(result.stdout_str().contains(&format!( + "{path}{separator}{file}\x1b\\{file}\x1b]8;;\x1b\\" + ))); for argument in [ "--hyperlink=never", @@ -5748,23 +5744,23 @@ fn test_ls_hyperlink_encode_link() { assert!( result .stdout_str() - .contains("back%5cslash\x07back\\slash\x1b]8;;\x07") + .contains("back%5cslash\x1b\\back\\slash\x1b]8;;\x1b\\") ); assert!( result .stdout_str() - .contains("ques%3ftion\x07ques?tion\x1b]8;;\x07") + .contains("ques%3ftion\x1b\\ques?tion\x1b]8;;\x1b\\") ); } assert!( result .stdout_str() - .contains("encoded%253Fquestion\x07encoded%3Fquestion\x1b]8;;\x07") + .contains("encoded%253Fquestion\x1b\\encoded%3Fquestion\x1b]8;;\x1b\\") ); assert!( result .stdout_str() - .contains("sp%20ace\x07sp ace\x1b]8;;\x07") + .contains("sp%20ace\x1b\\sp ace\x1b]8;;\x1b\\") ); } // spell-checker: enable @@ -5796,7 +5792,9 @@ fn test_ls_hyperlink_dirs() { .lines() .next() .unwrap() - .contains(&format!("{path}{separator}{dir_a}\x07{dir_a}\x1b]8;;\x07:")) + .contains(&format!( + "{path}{separator}{dir_a}\x1b\\{dir_a}\x1b]8;;\x1b\\:" + )) ); assert_eq!(result.stdout_str().lines().nth(1).unwrap(), ""); assert!( @@ -5805,7 +5803,9 @@ fn test_ls_hyperlink_dirs() { .lines() .nth(2) .unwrap() - .contains(&format!("{path}{separator}{dir_b}\x07{dir_b}\x1b]8;;\x07:")) + .contains(&format!( + "{path}{separator}{dir_b}\x1b\\{dir_b}\x1b]8;;\x1b\\:" + )) ); } @@ -5837,21 +5837,95 @@ fn test_ls_hyperlink_recursive_dirs() { let mut lines = result.stdout_str().lines(); assert_hyperlink!( lines.next(), - &format!("{path}{separator}{dir_a}\x07{dir_a}\x1b]8;;\x07:") + &format!("{path}{separator}{dir_a}\x1b\\{dir_a}\x1b]8;;\x1b\\:") ); assert_hyperlink!( lines.next(), - &format!("{path}{separator}{dir_a}{separator}{dir_b}\x07{dir_b}\x1b]8;;\x07") + &format!("{path}{separator}{dir_a}{separator}{dir_b}\x1b\\{dir_b}\x1b]8;;\x1b\\") ); assert!(matches!(lines.next(), Some(l) if l.is_empty())); assert_hyperlink!( lines.next(), &format!( - "{path}{separator}{dir_a}{separator}{dir_b}\x07{dir_a}{separator}{dir_b}\x1b]8;;\x07:" + "{path}{separator}{dir_a}{separator}{dir_b}\x1b\\{dir_a}{separator}{dir_b}\x1b]8;;\x1b\\:" ) ); } +#[test] +fn test_ls_hyperlink_symlink_target_handling() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.mkdir("target_dir"); + at.touch("target_file.txt"); + at.symlink_file("target_file.txt", "link_to_file"); + at.symlink_dir("target_dir", "link_to_dir"); + at.symlink_file("nonexistent", "link_to_missing"); + + let result = scene + .ucmd() + .args(&["-l", "--hyperlink", "--color"]) + .succeeds(); + let output = result.stdout_str(); + + assert!(output.contains("\x1b]8;;file://")); + assert!(!output.contains('\x07')); + assert!(output.contains("\x1b\\")); + + let file_link_line = output + .lines() + .find(|line| line.contains("link_to_file")) + .unwrap(); + assert!(file_link_line.contains(" -> ")); + let target_part = file_link_line.split(" -> ").nth(1).unwrap(); + assert!(!target_part.contains("\x1b[")); + assert!(!target_part.ends_with("\x1b[K")); + + let dir_link_line = output + .lines() + .find(|line| line.contains("link_to_dir")) + .unwrap(); + assert!(dir_link_line.contains(" -> ")); + let target_part = dir_link_line.split(" -> ").nth(1).unwrap(); + assert!(target_part.contains("\x1b[")); + + let missing_link_line = output + .lines() + .find(|line| line.contains("link_to_missing")) + .unwrap(); + assert!(missing_link_line.contains(" -> ")); + let missing_target_part = missing_link_line.split(" -> ").nth(1).unwrap(); + assert!(missing_target_part.contains("\x1b[")); +} + +#[test] +fn test_ls_hyperlink_utf8_encoding() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.touch("café.txt"); + #[cfg(not(target_os = "windows"))] + at.touch("file:with:colons.txt"); + #[cfg(target_os = "windows")] + at.touch("file-with-colons.txt"); + at.touch("file with spaces.txt"); + + let result = scene.ucmd().args(&["--hyperlink"]).succeeds(); + let output = result.stdout_str(); + + assert!(output.contains("caf%c3%a9.txt")); + #[cfg(not(target_os = "windows"))] + assert!(output.contains("file%3awith%3acolons.txt")); + #[cfg(target_os = "windows")] + assert!(output.contains("file-with-colons.txt")); + assert!(output.contains("file%20with%20spaces.txt")); + + let hyperlink_count = output.matches("\x1b]8;;file://").count(); + let terminator_count = output.matches("\x1b\\").count(); + assert_eq!(terminator_count, hyperlink_count * 2); +} + #[test] fn test_ls_color_do_not_reset() { let scene: TestScenario = TestScenario::new(util_name!());