Address Windows symlink cp review feedback

Author: caomengxuan666

src/uu/cp/src/cp.rs

@@ -1936,8 +1936,7 @@ pub(crate) fn copy_attributes(
 fn symlink_file(
     source: &Path,
     dest: &Path,
-    #[cfg(windows)] source_type_hint: Option<&Path>,
-    #[cfg(not(windows))] _source_type_hint: Option<&Path>,
+    #[cfg(windows)] source_metadata: &Metadata,
     #[cfg(not(target_os = "wasi"))] symlinked_files: &mut HashSet<FileInformation>,
     #[cfg(target_os = "wasi")] _symlinked_files: &mut HashSet<FileInformation>,
 ) -> CopyResult<()> {
@@ -1963,7 +1962,7 @@ fn symlink_file(
     }
     #[cfg(windows)]
     {
-        let create_symlink = if source_type_hint.is_some_and(source_is_directory_for_symlink) {
+        let create_symlink = if source_is_directory_for_symlink(source_metadata) {
             std::os::windows::fs::symlink_dir
         } else {
             std::os::windows::fs::symlink_file
@@ -1987,18 +1986,12 @@ fn symlink_file(
     }
 }
 
+/// Returns true when `source_metadata` should be recreated as a directory symlink.
 #[cfg(windows)]
-fn source_is_directory_for_symlink(source: &Path) -> bool {
+fn source_is_directory_for_symlink(source_metadata: &Metadata) -> bool {
     const FILE_ATTRIBUTE_DIRECTORY: u32 = 0x10;
 
-    source.symlink_metadata().is_ok_and(|metadata| {
-        let file_type = metadata.file_type();
-        if file_type.is_symlink() {
-            metadata.file_attributes() & FILE_ATTRIBUTE_DIRECTORY != 0
-        } else {
-            file_type.is_dir()
-        }
-    })
+    source_metadata.file_attributes() & FILE_ATTRIBUTE_DIRECTORY != 0
 }
 
 fn context_for(src: &Path, dest: &Path) -> String {
@@ -2346,7 +2339,13 @@ fn handle_copy_mode(
             if dest.exists() && options.overwrite == OverwriteMode::Clobber(ClobberMode::Force) {
                 fs::remove_file(dest)?;
             }
-            symlink_file(source, dest, Some(source), symlinked_files)?;
+            symlink_file(
+                source,
+                dest,
+                #[cfg(windows)]
+                source_metadata,
+                symlinked_files,
+            )?;
         }
         CopyMode::Update => {
             if dest.exists() {
@@ -2821,7 +2820,14 @@ fn copy_helper(
     }
 
     if source_metadata.is_symlink() {
-        copy_link(source, dest, symlinked_files, options)?;
+        copy_link(
+            source,
+            dest,
+            #[cfg(windows)]
+            source_metadata,
+            symlinked_files,
+            options,
+        )?;
     } else {
         // Use O_NOFOLLOW on the source open iff cp is in no-dereference mode.
         // In that case source_metadata was obtained via lstat, so a path swap
@@ -2900,6 +2906,7 @@ fn copy_node(
 fn copy_link(
     source: &Path,
     dest: &Path,
+    #[cfg(windows)] source_metadata: &Metadata,
     symlinked_files: &mut HashSet<FileInformation>,
     options: &Options,
 ) -> CopyResult<()> {
@@ -2910,7 +2917,13 @@ fn copy_link(
     if dest.is_symlink() || dest.is_file() {
         delete_path(dest, options)?;
     }
-    symlink_file(&link, dest, Some(source), symlinked_files)?;
+    symlink_file(
+        &link,
+        dest,
+        #[cfg(windows)]
+        source_metadata,
+        symlinked_files,
+    )?;
     copy_attributes(
         source,
         dest,
@@ -2986,6 +2999,8 @@ fn disk_usage_directory(p: &Path) -> io::Result<u64> {
 #[cfg(test)]
 mod tests {
 
+    #[cfg(windows)]
+    use crate::source_is_directory_for_symlink;
     use crate::{Attributes, Preserve, aligned_ancestors, localize_to_target};
     use std::path::Path;
 
@@ -3056,4 +3071,36 @@ mod tests {
             }
         );
     }
+
+    #[test]
+    #[cfg(windows)]
+    fn test_source_is_directory_for_symlink_detects_directory_symlinks() {
+        let temp_dir = tempfile::tempdir().unwrap();
+        let target = temp_dir.path().join("target");
+        let symlink = temp_dir.path().join("target-link");
+
+        std::fs::create_dir(&target).unwrap();
+        if std::os::windows::fs::symlink_dir(&target, &symlink).is_err() {
+            return;
+        }
+
+        let metadata = symlink.symlink_metadata().unwrap();
+        assert!(source_is_directory_for_symlink(&metadata));
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn test_source_is_directory_for_symlink_rejects_file_symlinks() {
+        let temp_dir = tempfile::tempdir().unwrap();
+        let target = temp_dir.path().join("target");
+        let symlink = temp_dir.path().join("target-link");
+
+        std::fs::write(&target, b"data").unwrap();
+        if std::os::windows::fs::symlink_file(&target, &symlink).is_err() {
+            return;
+        }
+
+        let metadata = symlink.symlink_metadata().unwrap();
+        assert!(!source_is_directory_for_symlink(&metadata));
+    }
 }

tests/by-util/test_cp.rs

@@ -2874,6 +2874,16 @@ fn test_copy_dir_symlink() {
     at.symlink_dir("dir", "dir-link");
     ucmd.args(&["-r", "dir-link", "copy"]).succeeds();
     assert_eq!(at.resolve_link("copy"), "dir");
+}
+
+#[test]
+#[cfg(windows)]
+fn test_copy_windows_dir_symlink_preserves_directory_type() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.mkdir("dir");
+    at.symlink_dir("dir", "dir-link");
+    ucmd.args(&["-r", "dir-link", "copy"]).succeeds();
+    assert_eq!(at.resolve_link("copy"), "dir");
     assert!(at.plus("copy").is_dir());
 }