Merge pull request #4216 from jfinkels/cp-preserve-hardlinks-readability

cp: improve readability of preserve_hardlinks()

src/uu/cp/src/copydir.rs

@@ -213,9 +213,8 @@ fn copy_direntry(
     // If the source is not a directory, then we need to copy the file.
     if !source_absolute.is_dir() {
         if preserve_hard_links {
-            let mut found_hard_link = false;
             let dest = local_to_target.as_path().to_path_buf();
-            preserve_hardlinks(hard_links, &source_absolute, &dest, &mut found_hard_link)?;
+            let found_hard_link = preserve_hardlinks(hard_links, &source_absolute, &dest)?;
             if !found_hard_link {
                 match copy_file(
                     progress_bar,

src/uu/cp/src/cp.rs

@@ -891,6 +891,16 @@ fn parse_path_args(path_args: &[String], options: &Options) -> CopyResult<(Vec<S
     Ok((paths, target))
 }
 
+/// Get the inode information for a file.
+fn get_inode(file_info: &FileInformation) -> u64 {
+    #[cfg(unix)]
+    let result = file_info.inode();
+    #[cfg(windows)]
+    let result = file_info.file_index();
+    result
+}
+
+#[cfg(target_os = "redox")]
 fn preserve_hardlinks(
     hard_links: &mut Vec<(String, u64)>,
     source: &std::path::Path,
@@ -898,53 +908,47 @@ fn preserve_hardlinks(
     found_hard_link: &mut bool,
 ) -> CopyResult<()> {
     // Redox does not currently support hard links
-    #[cfg(not(target_os = "redox"))]
+    Ok(())
-    {
+}
-        if !source.is_dir() {
-            let info = match FileInformation::from_path(source, false) {
-                Ok(info) => info,
-                Err(e) => {
-                    return Err(format!("cannot stat {}: {}", source.quote(), e,).into());
-                }
-            };
 
-            #[cfg(unix)]
+/// Hard link a pair of files if needed _and_ record if this pair is a new hard link.
-            let inode = info.inode();
+#[cfg(not(target_os = "redox"))]
-
+fn preserve_hardlinks(
-            #[cfg(windows)]
+    hard_links: &mut Vec<(String, u64)>,
-            let inode = info.file_index();
+    source: &std::path::Path,
-
+    dest: &std::path::Path,
-            let nlinks = info.number_of_links();
+) -> CopyResult<bool> {
-
+    let info = FileInformation::from_path(source, false)
-            for hard_link in hard_links.iter() {
+        .context(format!("cannot stat {}", source.quote()))?;
-                if hard_link.1 == inode {
+    let inode = get_inode(&info);
-                    // Consider the following files:
+    let nlinks = info.number_of_links();
-                    //
+    let mut found_hard_link = false;
-                    // * `src/f` - a regular file
+    for (link, link_inode) in hard_links.iter() {
-                    // * `src/link` - a hard link to `src/f`
+        if *link_inode == inode {
-                    // * `dest/src/f` - a different regular file
+            // Consider the following files:
-                    //
+            //
-                    // In this scenario, if we do `cp -a src/ dest/`, it is
+            // * `src/f` - a regular file
-                    // possible that the order of traversal causes `src/link`
+            // * `src/link` - a hard link to `src/f`
-                    // to get copied first (to `dest/src/link`). In that case,
+            // * `dest/src/f` - a different regular file
-                    // in order to make sure `dest/src/link` is a hard link to
+            //
-                    // `dest/src/f` and `dest/src/f` has the contents of
+            // In this scenario, if we do `cp -a src/ dest/`, it is
-                    // `src/f`, we delete the existing file to allow the hard
+            // possible that the order of traversal causes `src/link`
-                    // linking.
+            // to get copied first (to `dest/src/link`). In that case,
-                    if file_or_link_exists(dest) && file_or_link_exists(Path::new(&hard_link.0)) {
+            // in order to make sure `dest/src/link` is a hard link to
-                        std::fs::remove_file(dest)?;
+            // `dest/src/f` and `dest/src/f` has the contents of
-                    }
+            // `src/f`, we delete the existing file to allow the hard
-
+            // linking.
-                    std::fs::hard_link(hard_link.0.clone(), dest).unwrap();
+            if file_or_link_exists(dest) && file_or_link_exists(Path::new(link)) {
-                    *found_hard_link = true;
+                std::fs::remove_file(dest)?;
-                }
-            }
-            if !(*found_hard_link) && nlinks > 1 {
-                hard_links.push((dest.to_str().unwrap().to_string(), inode));
             }
+            std::fs::hard_link(link, dest).unwrap();
+            found_hard_link = true;
         }
     }
-    Ok(())
+    if !found_hard_link && nlinks > 1 {
+        hard_links.push((dest.to_str().unwrap().to_string(), inode));
+    }
+    Ok(found_hard_link)
 }
 
 /// Copy all `sources` to `target`.  Returns an
@@ -986,11 +990,12 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu
             // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases)
             show_warning!("source {} specified more than once", source.quote());
         } else {
-            let mut found_hard_link = false;
+            let found_hard_link = if preserve_hard_links && !source.is_dir() {
-            if preserve_hard_links {
                 let dest = construct_dest_path(source, target, &target_type, options)?;
-                preserve_hardlinks(&mut hard_links, source, &dest, &mut found_hard_link)?;
+                preserve_hardlinks(&mut hard_links, source, &dest)?
-            }
+            } else {
+                false
+            };
             if !found_hard_link {
                 if let Err(error) = copy_source(
                     &progress_bar,