Merge pull request #60 from edmorley/fix-dir-get_size

Fix `dir::get_size`'s handling of symlinks

src/dir.rs

@@ -269,6 +269,7 @@ where
     let metadata = path.metadata()?;
     get_details_entry_with_meta(path, config, metadata)
 }
+
 fn get_details_entry_with_meta<P>(
     path: P,
     config: &HashSet<DirEntryAttr>,
@@ -758,11 +759,13 @@ where
     })
 }
 
-/// Returns the size of the file or directory in bytes
+/// Returns the size of the file or directory in bytes.(!important: folders size not count)
 ///
 /// If used on a directory, this function will recursively iterate over every file and every
 /// directory inside the directory. This can be very time consuming if used on large directories.
 ///
+/// Does not follow symlinks.
+///
 /// # Errors
 ///
 /// This function will return an error in the following situations, but is not limited to just
@@ -784,20 +787,32 @@ pub fn get_size<P>(path: P) -> Result<u64>
 where
     P: AsRef<Path>,
 {
-    let mut result = 0;
+    // Using `fs::symlink_metadata` since we don't want to follow symlinks,
+    // as we're calculating the exact size of the requested path itself.
+    let path_metadata = path.as_ref().symlink_metadata()?;
 
-    if path.as_ref().is_dir() {
+    let mut size_in_bytes = 0;
+
+    if path_metadata.is_dir() {
         for entry in read_dir(&path)? {
-            let _path = entry?.path();
-            result += _path.metadata()?.len();
-            if _path.is_dir() {
-                result += get_size(_path)?;
+            let entry = entry?;
+            // `DirEntry::metadata` does not follow symlinks (unlike `fs::metadata`), so in the
+            // case of symlinks, this is the size of the symlink itself, not its target.
+            let entry_metadata = entry.metadata()?;
+
+            if entry_metadata.is_dir() {
+                // The size of the directory entry itself will be counted inside the `get_size()` call,
+                // so we intentionally don't also add `entry_metadata.len()` to the total here.
+                size_in_bytes += get_size(entry.path())?;
+            } else {
+                size_in_bytes += entry_metadata.len();
             }
         }
     } else {
-        result = path.as_ref().metadata()?.len();
+        size_in_bytes = path_metadata.len();
     }
-    Ok(result)
+
+    Ok(size_in_bytes)
 }
 
 /// Copies the directory contents from one place to another using recursive method,

tests/dir.rs

@@ -1,5 +1,5 @@
 use std::collections::HashSet;
-use std::fs::read_dir;
+use std::fs::{self, read_dir};
 use std::path::{Path, PathBuf};
 use std::sync::mpsc::{self, TryRecvError};
 use std::thread;
@@ -70,6 +70,22 @@ fn get_dir_size() -> u64 {
         .len()
 }
 
+#[cfg(unix)]
+fn create_file_symlink<P: AsRef<Path>, Q: AsRef<Path>>(
+    original: P,
+    link: Q,
+) -> std::io::Result<()> {
+    std::os::unix::fs::symlink(original.as_ref(), link.as_ref())
+}
+
+#[cfg(windows)]
+fn create_file_symlink<P: AsRef<Path>, Q: AsRef<Path>>(
+    original: P,
+    link: Q,
+) -> std::io::Result<()> {
+    std::os::windows::fs::symlink_file(original.as_ref(), link.as_ref())
+}
+
 const TEST_FOLDER: &'static str = "./tests/temp/dir";
 
 #[test]
@@ -2821,22 +2837,42 @@ fn it_get_folder_size() {
 
     let mut file1 = path.clone();
     file1.push("test1.txt");
-    fs_extra::file::write_all(&file1, "content1").unwrap();
+    fs_extra::file::write_all(&file1, &"A".repeat(100)).unwrap();
     assert!(file1.exists());
 
     let mut sub_dir_path = path.clone();
     sub_dir_path.push("sub");
     create(&sub_dir_path, true).unwrap();
+
     let mut file2 = sub_dir_path.clone();
     file2.push("test2.txt");
-    fs_extra::file::write_all(&file2, "content2").unwrap();
+    fs_extra::file::write_all(&file2, &"B".repeat(300)).unwrap();
     assert!(file2.exists());
 
-    let result = get_size(&path).unwrap();
+    let symlink_file = sub_dir_path.join("symlink_file.txt");
+
+    // Rust stdlib APIs for creating a symlinked file only exist for Unix and Windows.
+    #[cfg(any(unix, windows))]
+    {
+        // Only passing the filename since we want this to be a relative symlink.
+        create_file_symlink("test2.txt", &symlink_file).unwrap();
+        assert!(symlink_file.exists());
+    }
+
+    // Total size comprises of:
+    // - 100 bytes for the standard file "test1.txt"
+    // - 300 bytes for the standard file "test2.txt"
+    // - (On supported platforms) 1 x symlink whose whose size varies by filesystem, so is dynamically calculated.
+    let mut expected_size = 100 + 300;
 
-    let test_size: u64 = 16 + get_dir_size();
+    if symlink_file.exists() {
+        // `fs::symlink_metadata` does not follow symlinks, so this is the size of the symlink itself, not its target.
+        expected_size += fs::symlink_metadata(&symlink_file).unwrap().len();
+    }
+
+    let result = get_size(&path).unwrap();
 
-    assert_eq!(test_size, result);
+    assert_eq!(expected_size, result);
 }
 
 #[test]

tests/lib.rs

@@ -59,16 +59,6 @@ where
     true
 }
 
-// Returns the size of a directory. On Linux with ext4 this can be about 4kB.
-// Since the directory size can vary, we need to calculate is dynamically.
-fn get_dir_size() -> u64 {
-    std::fs::create_dir_all("./tests/temp").expect("Couldn't create test folder");
-
-    std::fs::metadata("./tests/temp")
-        .expect("Couldn't receive metadata of tests/temp folder")
-        .len()
-}
-
 const TEST_FOLDER: &'static str = "./tests/temp/lib";
 
 #[test]
@@ -922,10 +912,10 @@ fn it_copy_progress_work() {
             Ok(process_info) => {
                 if process_info.file_name == "file2.txt" {
                     assert_eq!(9, process_info.file_total_bytes);
-                    assert_eq!(get_dir_size() + 41, process_info.total_bytes);
+                    assert_eq!(41, process_info.total_bytes);
                 } else if process_info.file_name == "file1.txt" {
                     assert_eq!(8, process_info.file_total_bytes);
-                    assert_eq!(get_dir_size() + 41, process_info.total_bytes);
+                    assert_eq!(41, process_info.total_bytes);
                 }
             }
             Err(TryRecvError::Disconnected) => {
@@ -1033,14 +1023,14 @@ fn it_copy_with_progress_work_dif_buf_size() {
             assert_eq!(i * 2, process_info.file_bytes_copied);
             assert_eq!(i * 2, process_info.copied_bytes);
             assert_eq!(8, process_info.file_total_bytes);
-            assert_eq!(get_dir_size() + 40, process_info.total_bytes);
+            assert_eq!(40, process_info.total_bytes);
         }
         for i in 1..5 {
             let process_info: TransitProcess = rx.recv().unwrap();
             assert_eq!(i * 2 + 8, process_info.copied_bytes);
             assert_eq!(i * 2, process_info.file_bytes_copied);
             assert_eq!(8, process_info.file_total_bytes);
-            assert_eq!(get_dir_size() + 40, process_info.total_bytes);
+            assert_eq!(40, process_info.total_bytes);
         }
 
         match result {
@@ -1055,14 +1045,14 @@ fn it_copy_with_progress_work_dif_buf_size() {
         assert_eq!(i, process_info.file_bytes_copied);
         assert_eq!(i, process_info.copied_bytes);
         assert_eq!(8, process_info.file_total_bytes);
-        assert_eq!(get_dir_size() + 40, process_info.total_bytes);
+        assert_eq!(40, process_info.total_bytes);
     }
     for i in 1..9 {
         let process_info: TransitProcess = rx.recv().unwrap();
         assert_eq!(i + 8, process_info.copied_bytes);
         assert_eq!(i, process_info.file_bytes_copied);
         assert_eq!(8, process_info.file_total_bytes);
-        assert_eq!(get_dir_size() + 40, process_info.total_bytes);
+        assert_eq!(40, process_info.total_bytes);
     }
 
     match result {
@@ -2385,10 +2375,10 @@ fn it_move_progress_work() {
             Ok(process_info) => {
                 if process_info.file_name == "file2.txt" {
                     assert_eq!(9, process_info.file_total_bytes);
-                    assert_eq!(get_dir_size() + 41, process_info.total_bytes);
+                    assert_eq!(41, process_info.total_bytes);
                 } else if process_info.file_name == "file1.txt" {
                     assert_eq!(8, process_info.file_total_bytes);
-                    assert_eq!(get_dir_size() + 41, process_info.total_bytes);
+                    assert_eq!(41, process_info.total_bytes);
                 }
             }
             Err(TryRecvError::Disconnected) => {