ls: fix hyperlink functionality to use correct OSC 8 format and handle symlink targets

Should fix tests/ls/hyperlink.sh

Author: sylvestre

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<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()),
+                                ));
+                            } else {
+                                // For regular files with no coloring, just use plain text
+                                name.push(escaped_target);
+                            }
                         }
                         Err(_) => {
                             name.push(
@@ -3463,29 +3480,35 @@ 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();
 
+    // Use raw bytes to preserve invalid UTF-8 sequences instead of to_string_lossy()
+    use std::os::unix::ffi::OsStrExt;
+    let absolute_path_bytes = absolute_path.as_os_str().as_bytes();
+
+    // 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<u8> = "_-.~/".bytes().collect();
     #[cfg(target_os = "windows")]
-    let unencoded_chars = "_-.:~/\\";
-
-    // percentage encoding of path
-    let absolute_path: String = absolute_path
-        .chars()
-        .map(|c| {
-            if c.is_alphanumeric() || unencoded_chars.contains(c) {
-                c.to_string()
+    let unencoded_bytes: std::collections::HashSet<u8> = "_-.~/\\".bytes().collect();
+
+    // 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| {
+            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
 }
 

tests/by-util/test_ls.rs

@@ -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,89 @@ 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");
+    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"));
+    assert!(output.contains("file%3awith%3acolons.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!());