Merge pull request #45 from sireliah/directories-even-more-edge-cases

Directories even more edge cases

Author: sireliah

src/p2p/transfer/directory.rs

@@ -46,6 +46,11 @@ impl ZipStream {
                 let entry = entry?;
                 let file_path = entry.path();
                 debug!("{:?}", file_path);
+
+                if !file_path.exists() {
+                    continue;
+                }
+
                 let rel_path = match base_path {
                     Some(base) => file_path
                         .strip_prefix(base)
@@ -60,10 +65,16 @@ impl ZipStream {
 
                 // Only files and empty directories are supported for now. Symlinks are ignored.
                 if file_path.is_file() {
-                    debug!("Writing file: {}", path_string);
-                    Self::write_file(&mut zip, path_string, &file_path).await?;
+                    if file_path.metadata()?.len() > 0 {
+                        debug!("Writing file: {}", path_string);
+                        Self::write_file(&mut zip, path_string, &file_path).await?;
+                    } else {
+                        debug!("Writing empty file: {}", path_string);
+                        Self::write_empty_file(&mut zip, path_string).await?;
+                    }
                 } else {
                     if file_path.read_dir()?.next().is_none() {
+                        debug!("Writing empty directory: {}", path_string);
                         Self::write_empty_dir(&mut zip, path_string).await?;
                     }
                 }
@@ -95,6 +106,19 @@ impl ZipStream {
         Ok(())
     }
 
+    async fn write_empty_file(
+        zip: &mut ZipFileWriter<&mut DuplexStream>,
+        rel_path: String,
+    ) -> Result<(), Error> {
+        // Trying to unzip the empty file at the reader end makes tokio error with "early eof".
+        // This might be an async-zip bug, but as a workaround it's enough to create empty entry here.
+        let opts = EntryOptions::new(rel_path, DEFAULT_COMPRESSION);
+        zip.write_entry_whole(opts, &[])
+            .await
+            .map_err(|err| zip_error(err))?;
+        Ok(())
+    }
+
     async fn write_file(
         zip: &mut ZipFileWriter<&mut DuplexStream>,
         rel_path: String,
@@ -141,13 +165,19 @@ async fn get_compression(path: &Path) -> Result<Compression, Error> {
     let mut file = File::open(&path).await?;
     file.read_exact(&mut buf).await?;
     match buf {
+        // Zip
         [80, 75, 3, 4] => Ok(Compression::Bz),
-        _ => Ok(DEFAULT_COMPRESSION),
+        // Gzip
+        [31, 139, 8, 0] => Ok(Compression::Bz),
+        v => {
+            debug!("Compression: {:?}, {:?}", v, path);
+            Ok(DEFAULT_COMPRESSION)
+        },
     }
 }
 
 fn zip_error(err: ZipError) -> Error {
-    Error::new(ErrorKind::Other, err.to_string())
+    Error::new(ErrorKind::Other, format!("Zip error: {}", err.to_string()))
 }
 
 fn is_zip_dir(path: &Path) -> bool {

src/p2p/transfer/file.rs

@@ -168,8 +168,10 @@ fn check_directory_size(path: &str) -> Result<u64, io::Error> {
     let mut total_size = 0;
     for entry in WalkDir::new(path) {
         let entry = entry?;
-        let meta = metadata(entry.path())?;
-        total_size += meta.len();
+        match metadata(entry.path()) {
+            Ok(meta) => total_size += meta.len(),
+            Err(e) => warn!("Can't estimate size of {:?}, {}", entry.path(), e),
+        };
     }
     Ok(total_size)
 }

src/user_data/mod.rs

@@ -18,47 +18,40 @@ fn get_timestamp() -> u64 {
         .as_secs()
 }
 
-fn extend_dir(path: &Path, time: u64) -> PathBuf {
-    let dir = match path.file_name() {
-        Some(dir_name) => dir_name.to_string_lossy().to_string(),
-        None => "directory".to_string(),
-    };
-    match path.parent() {
-        Some(parent_path) => parent_path.join(format!("{}_{}", dir, time)),
-        // Probably not best idea to use this application to move your whole root dir (｡•̀ᴗ-)
-        None => Path::new(&format!("/directory_{}", time)).to_path_buf(),
-    }
+fn extend_dir(path: &Path, dir_name: &str, time: u64) -> PathBuf {
+    path.join(format!("{}_{}", dir_name, time))
 }
 
-fn extend_file(path: &Path, time: u64) -> PathBuf {
-    let extension: String = match path.extension() {
+fn extend_file(path: &Path, name: &str, time: u64) -> PathBuf {
+    let file_name_path = Path::new(name);
+    let extension: String = match file_name_path.extension() {
         Some(v) => v.to_string_lossy().to_string(),
         None => "".to_string(),
     };
-    let basename = match path.file_stem() {
+    let basename = match file_name_path.file_stem() {
         Some(v) => v.to_string_lossy().to_string(),
         None => "file".to_string(),
     };
-    let name = format!("{}_{}", basename, time);
-    let mut path = path.join(&name);
+    let new_name = format!("{}_{}", basename, time);
+    let mut path = path.join(&new_name);
     path.set_extension(extension);
     path
 }
 
-fn generate_full_path<F>(name: &str, path: &Path, timestamp: F) -> Result<String, Error>
+fn generate_full_path<F>(path: &Path, name: &str, timestamp: F) -> Result<String, Error>
 where
     F: Fn() -> u64,
 {
     // If file or dir already exists in the target directory, create a path extended with a timestamp
-    let path = Path::new(&path);
     let joined = path.join(&name);
     let time = timestamp();
 
     let joined = if joined.exists() {
+        info!("File already exists: {:?}", joined);
         if joined.is_file() {
-            extend_file(&joined, time)
+            extend_file(path, name, time)
         } else {
-            extend_dir(&joined, time)
+            extend_dir(path, name, time)
         }
     } else {
         joined
@@ -76,12 +69,12 @@ pub fn get_target_path(name: &str, target_path: Option<&String>) -> Result<Strin
     match target_path {
         Some(path) => {
             let path = Path::new(path);
-            generate_full_path(name, path, get_timestamp)
+            generate_full_path(path, name, get_timestamp)
         }
         None => {
             let config = UserConfig::new()?;
             let dir = config.get_downloads_dir();
-            generate_full_path(name, dir.as_path(), get_timestamp)
+            generate_full_path(dir.as_path(), name, get_timestamp)
         }
     }
 }
@@ -208,34 +201,51 @@ impl UserConfig {
 #[cfg(test)]
 mod tests {
     use crate::user_data::{extend_dir, generate_full_path};
+    use std::fs::{create_dir_all, File};
     use std::path::Path;
+    use tempfile::tempdir;
 
     #[test]
     fn test_extend_dir_should_extend_name_with_timestamp() {
-        let result = extend_dir(Path::new("/home/user/directory/"), 1111);
+        let result = extend_dir(Path::new("/home/user/"), "directory", 1111);
 
         assert_eq!(result, Path::new("/home/user/directory_1111"))
     }
 
     #[test]
-    fn test_extend_dir_root_edge_case() {
-        let result = extend_dir(Path::new("/"), 1111);
+    fn test_generate_full_file_path() {
+        let result = generate_full_path(Path::new("/home/user/"), "a-file.txt", || 1111).unwrap();
 
-        assert_eq!(result, Path::new("/directory_1111"))
+        assert_eq!(result, "/home/user/a-file.txt");
     }
 
     #[test]
-    fn test_generate_full_file_path() {
-        let result = generate_full_path("a-file.txt", Path::new("/home/user/"), || 1111).unwrap();
+    fn test_generate_full_file_path_when_file_exists() {
+        let dir = tempdir().unwrap();
+        let path = dir.path();
+        let received_file_name = "a-file.txt";
+        File::create(path.join(received_file_name)).unwrap();
 
-        assert_eq!(result, "/home/user/a-file.txt");
+        let result = generate_full_path(path, received_file_name, || 1111).unwrap();
+
+        assert_eq!(result, path.join("a-file_1111.txt").to_string_lossy());
     }
 
     #[test]
     fn test_generate_full_dir_path() {
         let result =
-            generate_full_path("some_directory", Path::new("/home/user/"), || 1111).unwrap();
+            generate_full_path(Path::new("/home/user/"), "some_directory", || 1111).unwrap();
 
         assert_eq!(result, "/home/user/some_directory");
     }
+    #[test]
+    fn test_generate_full_dir_path_when_dir_exists() {
+        let dir = tempdir().unwrap();
+        let path = dir.path();
+        let received_dir_name = "some_directory";
+        create_dir_all(path.join(received_dir_name)).unwrap();
+        let result = generate_full_path(path, received_dir_name, || 1111).unwrap();
+
+        assert_eq!(result, path.join("some_directory_1111").to_string_lossy());
+    }
 }

tests/data/test_dir/dir/deleted_symlink_file

@@ -0,0 +1 @@
+../deleted_symlink_file

tests/data/test_dir/empty_file

@@ -144,6 +144,12 @@ fn test_directory_transfer() {
                     .len(),
                 659903
             );
+            assert_eq!(
+                fs::metadata(Path::new(&path).join("test_dir/empty_file"))
+                    .unwrap()
+                    .len(),
+                0
+            );
             assert!(fs::metadata(Path::new(&path).join("test_dir/empty_dir/"))
                 .unwrap()
                 .is_dir());