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(
@@ -3458,34 +3475,53 @@ fn display_item_name(
     name
 }
 
+/// Cross-platform helper to get bytes from OsStr
+fn get_os_str_bytes(os_str: &OsStr) -> Vec<u8> {
+    #[cfg(unix)]
+    {
+        use std::os::unix::ffi::OsStrExt;
+        os_str.as_bytes().to_vec()
+    }
+    #[cfg(windows)]
+    {
+        // On Windows, convert to UTF-8 bytes, handling invalid UTF-8 gracefully
+        os_str.to_string_lossy().as_bytes().to_vec()
+    }
+}
+
 fn create_hyperlink(name: &OsStr, path: &PathData) -> OsString {
     let hostname = hostname::get().unwrap_or_else(|_| OsString::from(""));
     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 = get_os_str_bytes(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<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: &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
 }
 

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,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!());