From 21934341509684d1b2e6866a013d315960edf7b3 Mon Sep 17 00:00:00 2001 From: mhead Date: Wed, 11 Sep 2024 13:13:46 +0530 Subject: [PATCH 1/9] mv: copy dir --- Cargo.lock | 2 + src/uu/mv/Cargo.toml | 4 + src/uu/mv/src/mv.rs | 334 ++++++++++++++++++++++++++++-- src/uucore/src/lib/features/fs.rs | 7 + tests/by-util/test_mv.rs | 62 ++++++ 5 files changed, 390 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2af9bc78953..b266415d151 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2945,7 +2945,9 @@ dependencies = [ "clap", "fs_extra", "indicatif", + "tempfile", "uucore", + "walkdir", ] [[package]] diff --git a/src/uu/mv/Cargo.toml b/src/uu/mv/Cargo.toml index c484d5a77f3..8ee03bbfd06 100644 --- a/src/uu/mv/Cargo.toml +++ b/src/uu/mv/Cargo.toml @@ -20,6 +20,7 @@ path = "src/mv.rs" clap = { workspace = true } fs_extra = { workspace = true } indicatif = { workspace = true } +walkdir = { workspace = true } uucore = { workspace = true, features = [ "backup-control", "fs", @@ -27,6 +28,9 @@ uucore = { workspace = true, features = [ "update-control", ] } +[dev-dependencies] +tempfile = { workspace = true } + [[bin]] name = "mv" path = "src/main.rs" diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 5f1b717834b..5dff6124a1e 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -20,6 +20,8 @@ use std::os::unix; #[cfg(windows)] use std::os::windows; use std::path::{Path, PathBuf}; +#[cfg(unix)] +use unix::fs::FileTypeExt; use uucore::backup_control::{self, source_is_target_backup}; use uucore::display::Quotable; use uucore::error::{set_exit_code, FromIo, UResult, USimpleError, UUsageError}; @@ -30,15 +32,17 @@ use uucore::fs::{ #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] use uucore::fsxattr; use uucore::update_control; +use walkdir::WalkDir; // These are exposed for projects (e.g. nushell) that want to create an `Options` value, which // requires these enums pub use uucore::{backup_control::BackupMode, update_control::UpdateMode}; use uucore::{format_usage, help_about, help_section, help_usage, prompt_yes, show}; -use fs_extra::dir::{ - get_size as dir_get_size, move_dir, move_dir_with_progress, CopyOptions as DirCopyOptions, - TransitProcess, TransitProcessResult, +use fs_extra::{ + dir::{create_all, get_size as dir_get_size, remove}, + error::Result as FsXResult, + file::{self, CopyOptions}, }; use crate::error::MvError; @@ -630,13 +634,6 @@ fn rename_with_fallback( if to.exists() { fs::remove_dir_all(to)?; } - let options = DirCopyOptions { - // From the `fs_extra` documentation: - // "Recursively copy a directory with a new name or place it - // inside the destination. (same behaviors like cp -r in Unix)" - copy_inside: true, - ..DirCopyOptions::new() - }; // Calculate total size of directory // Silently degrades: @@ -663,15 +660,7 @@ fn rename_with_fallback( let xattrs = fsxattr::retrieve_xattrs(from).unwrap_or_else(|_| std::collections::HashMap::new()); - let result = if let Some(ref pb) = progress_bar { - move_dir_with_progress(from, to, &options, |process_info: TransitProcess| { - pb.set_position(process_info.copied_bytes); - pb.set_message(process_info.file_name); - TransitProcessResult::ContinueOrAbort - }) - } else { - move_dir(from, to, &options) - }; + let result = move_dir(from, to, progress_bar.as_ref()); #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] fsxattr::apply_xattrs(to, xattrs).unwrap(); @@ -751,3 +740,310 @@ fn is_empty_dir(path: &Path) -> bool { Err(_e) => false, } } + +/// Moves a directory from one location to another with progress tracking. +/// This function assumes that `from` is a directory and `to` does not exist. + +/// Returns: +/// - `Result`: The total number of bytes moved if successful. +fn move_dir(from: &Path, to: &Path, progress_bar: Option<&ProgressBar>) -> FsXResult { + // The return value that represents the number of bytes copied. + let mut result: u64 = 0; + let mut error_occured = false; + for dir_entry_result in WalkDir::new(from) { + match dir_entry_result { + Ok(dir_entry) => { + if dir_entry.file_type().is_dir() { + let path = dir_entry.into_path(); + let tmp_to = path.strip_prefix(from)?; + let dir = to.join(tmp_to); + if !dir.exists() { + create_all(&dir, false)?; + } + } else { + let file = dir_entry.path(); + let tp = file.strip_prefix(from)?; + let to_file = to.join(tp); + let result_file_copy = copy_file(file, &to_file, progress_bar, result)?; + result += result_file_copy; + } + } + Err(_) => { + error_occured = true; + } + } + } + if !error_occured { + remove(from)?; + } + Ok(result) +} + +/// Copies a file from one path to another, updating the progress bar if provided. +fn copy_file( + from: &Path, + to: &Path, + progress_bar: Option<&ProgressBar>, + progress_bar_start_val: u64, +) -> FsXResult { + let copy_options: CopyOptions = CopyOptions { + // We are overwriting here based on the assumption that the update and + // override options are handled by a parent function call. + overwrite: true, + ..Default::default() + }; + let progress_handler = if let Some(progress_bar) = progress_bar { + let display_file_name = from + .file_name() + .and_then(|file_name| file_name.to_str()) + .map(|file_name| file_name.to_string()) + .unwrap_or_default(); + let _progress_handler = |info: file::TransitProcess| { + let copied_bytes = progress_bar_start_val + info.copied_bytes; + progress_bar.set_position(copied_bytes); + }; + progress_bar.set_message(display_file_name); + Some(_progress_handler) + } else { + None + }; + let result_file_copy = { + let md = from.metadata()?; + if cfg!(unix) && FileTypeExt::is_fifo(&md.file_type()) { + let file_size = md.len(); + uucore::fs::copy_fifo(to)?; + if let Some(progress_bar) = progress_bar { + progress_bar.set_position(file_size + progress_bar_start_val); + } + Ok(file_size) + } else { + if let Some(progress_handler) = progress_handler { + file::copy_with_progress(from, to, ©_options, progress_handler) + } else { + file::copy(from, to, ©_options) + } + } + }; + result_file_copy +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + extern crate fs_extra; + use super::{copy_file, move_dir}; + use fs_extra::dir::*; + use indicatif::{ProgressBar, ProgressStyle}; + use tempfile::tempdir; + + // These tests are copied from the `fs_extra`'s repository + #[test] + fn it_move_work() { + for with_progress_bar in [false, true] { + let temp_dir = tempdir().unwrap(); + let mut path_from = PathBuf::from(temp_dir.path()); + let test_name = "sub"; + path_from.push("it_move_work"); + let mut path_to = path_from.clone(); + path_to.push("out"); + path_from.push(test_name); + + create_all(&path_from, true).unwrap(); + assert!(path_from.exists()); + create_all(&path_to, true).unwrap(); + assert!(path_to.exists()); + + let mut file1_path = path_from.clone(); + file1_path.push("test1.txt"); + let content1 = "content1"; + fs_extra::file::write_all(&file1_path, content1).unwrap(); + assert!(file1_path.exists()); + + let mut sub_dir_path = path_from.clone(); + sub_dir_path.push("sub"); + create(&sub_dir_path, true).unwrap(); + let mut file2_path = sub_dir_path.clone(); + file2_path.push("test2.txt"); + let content2 = "content2"; + fs_extra::file::write_all(&file2_path, content2).unwrap(); + assert!(file2_path.exists()); + + let pb = if with_progress_bar { + Some( + ProgressBar::new(16).with_style( + ProgressStyle::with_template( + "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", + ) + .unwrap(), + ), + ) + } else { + None + }; + + let result = move_dir(&path_from, &path_to, pb.as_ref()).unwrap(); + + assert_eq!(16, result); + assert!(path_to.exists()); + assert!(!path_from.exists()); + if let Some(pb) = pb { + assert_eq!(pb.position(), 16); + } + } + } + + #[test] + fn it_move_exist_overwrite() { + for with_progress_bar in [false, true] { + let temp_dir = tempdir().unwrap(); + let mut path_from = PathBuf::from(temp_dir.path()); + let test_name = "sub"; + path_from.push("it_move_exist_overwrite"); + let mut path_to = path_from.clone(); + path_to.push("out"); + path_from.push(test_name); + let same_file = "test.txt"; + + create_all(&path_from, true).unwrap(); + assert!(path_from.exists()); + create_all(&path_to, true).unwrap(); + assert!(path_to.exists()); + + let mut file1_path = path_from.clone(); + file1_path.push(same_file); + let content1 = "content1"; + fs_extra::file::write_all(&file1_path, content1).unwrap(); + assert!(file1_path.exists()); + + let mut sub_dir_path = path_from.clone(); + sub_dir_path.push("sub"); + create(&sub_dir_path, true).unwrap(); + let mut file2_path = sub_dir_path.clone(); + file2_path.push("test2.txt"); + let content2 = "content2"; + fs_extra::file::write_all(&file2_path, content2).unwrap(); + assert!(file2_path.exists()); + + let mut exist_path = path_to.clone(); + exist_path.push(test_name); + create(&exist_path, true).unwrap(); + assert!(exist_path.exists()); + exist_path.push(same_file); + let exist_content = "exist content"; + assert_ne!(exist_content, content1); + fs_extra::file::write_all(&exist_path, exist_content).unwrap(); + assert!(exist_path.exists()); + + let dir_size = get_size(&path_from).expect("failed to get dir size"); + let pb = if with_progress_bar { + Some( + ProgressBar::new(dir_size).with_style( + ProgressStyle::with_template( + "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", + ) + .unwrap(), + ), + ) + } else { + None + }; + move_dir(&path_from, &path_to, pb.as_ref()).unwrap(); + assert!(exist_path.exists()); + assert!(path_to.exists()); + assert!(!path_from.exists()); + if let Some(pb) = pb { + assert_eq!(pb.position(), dir_size); + } + } + } + + #[test] + fn it_move_inside_work_target_dir_not_exist() { + for with_progress_bar in [false, true] { + let temp_dir = tempdir().unwrap(); + let path_root = PathBuf::from(temp_dir.path()); + let root = path_root.join("it_move_inside_work_target_dir_not_exist"); + let root_dir1 = root.join("dir1"); + let root_dir1_sub = root_dir1.join("sub"); + let root_dir2 = root.join("dir2"); + let file1 = root_dir1.join("file1.txt"); + let file2 = root_dir1_sub.join("file2.txt"); + + create_all(&root_dir1_sub, true).unwrap(); + fs_extra::file::write_all(&file1, "content1").unwrap(); + fs_extra::file::write_all(&file2, "content2").unwrap(); + + if root_dir2.exists() { + remove(&root_dir2).unwrap(); + } + + assert!(root_dir1.exists()); + assert!(root_dir1_sub.exists()); + assert!(!root_dir2.exists()); + assert!(file1.exists()); + assert!(file2.exists()); + let dir_size = get_size(&root_dir1).expect("failed to get dir size"); + let pb = if with_progress_bar { + Some( + ProgressBar::new(dir_size).with_style( + ProgressStyle::with_template( + "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", + ) + .unwrap(), + ), + ) + } else { + None + }; + + let result = move_dir(&root_dir1, &root_dir2, pb.as_ref()).unwrap(); + + assert_eq!(16, result); + assert!(!root_dir1.exists()); + let root_dir2_sub = root_dir2.join("sub"); + let root_dir2_file1 = root_dir2.join("file1.txt"); + let root_dir2_sub_file2 = root_dir2_sub.join("file2.txt"); + assert!(root_dir2.exists()); + assert!(root_dir2_sub.exists()); + assert!(root_dir2_file1.exists()); + assert!(root_dir2_sub_file2.exists()); + if let Some(pb) = pb { + assert_eq!(pb.position(), dir_size); + } + } + } + + #[test] + fn copy_file_test() { + for with_progress_bar in [false, true] { + let temp_dir = tempdir().unwrap(); + let temp_dir_path = temp_dir.path(); + + let file1_path = temp_dir_path.join("file"); + let content = "content"; + fs_extra::file::write_all(&file1_path, content).unwrap(); + assert!(file1_path.exists()); + let path_to = temp_dir_path.join("file_out"); + let pb = if with_progress_bar { + Some( + ProgressBar::new(7).with_style( + ProgressStyle::with_template( + "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", + ) + .unwrap(), + ), + ) + } else { + None + }; + + let result = copy_file(&file1_path, &path_to, pb.as_ref(), 0).expect("move failed"); + + assert_eq!(7, result); + assert!(path_to.exists()); + if let Some(pb) = pb { + assert_eq!(pb.position(), 7); + } + } + } +} diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index 73c61e0a3e4..4c7ac1c12a3 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -789,6 +789,13 @@ pub fn get_filename(file: &Path) -> Option<&str> { file.file_name().and_then(|filename| filename.to_str()) } +// "Copies" a FIFO by creating a new one. This workaround is because Rust's +// built-in fs::copy does not handle FIFOs (see rust-lang/rust/issues/79390). +#[cfg(unix)] +pub fn copy_fifo(dest: &Path) -> std::io::Result<()> { + nix::unistd::mkfifo(dest, nix::sys::stat::Mode::S_IRUSR).map_err(|err| err.into()) +} + #[cfg(test)] mod tests { // Note this useful idiom: importing names from outer (for mod tests) scope. diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index 7b100fe5c3f..de043d88495 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -1323,6 +1323,33 @@ fn test_mv_verbose() { "renamed '{file_a}' -> '{file_b}' (backup: '{file_b}~')\n" )); } +#[test] +fn test_verbose_src_symlink() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + let file_a = "test_mv_verbose_file_a"; + let ln = "link"; + at.touch(file_a); + at.symlink_file(&file_a, &ln); + scene + .ucmd() + .arg("-v") + .arg(file_a) + .arg(ln) + .succeeds() + .stdout_only(format!("renamed '{file_a}' -> '{ln}'\n")); + + at.touch(file_a); + scene + .ucmd() + .arg("-vb") + .arg(file_a) + .arg(ln) + .succeeds() + .stdout_only(format!( + "renamed '{file_a}' -> '{ln}' (backup: '{ln}~')\n" + )); +} #[test] #[cfg(any(target_os = "linux", target_os = "android"))] // mkdir does not support -m on windows. Freebsd doesn't return a permission error either. @@ -1630,6 +1657,41 @@ mod inter_partition_copying { use std::fs::{read_to_string, set_permissions, write}; use std::os::unix::fs::{symlink, PermissionsExt}; use tempfile::TempDir; + // Ensure that the copying code used in an inter-partition move preserve dir structure. + #[test] + fn test_inter_partition_copying_folder() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir_all("a/b/c"); + at.write("a/b/d", "d"); + at.write("a/b/c/e", "e"); + + // create a folder in another partition. + let other_fs_tempdir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory"); + + // mv to other partition + scene + .ucmd() + .arg("a") + .arg(other_fs_tempdir.path().to_str().unwrap()) + .succeeds(); + + // make sure that a got removed. + assert!(!at.dir_exists("a")); + + // Ensure that the folder structure is preserved, files are copied, and their contents remain intact. + assert_eq!( + read_to_string(other_fs_tempdir.path().join("a/b/d"),) + .expect("Unable to read other_fs_file"), + "d" + ); + assert_eq!( + read_to_string(other_fs_tempdir.path().join("a/b/c/e"),) + .expect("Unable to read other_fs_file"), + "e" + ); + } // Ensure that the copying code used in an inter-partition move unlinks the destination symlink. #[test] From c6be3da6ec7f5461034b762aba78b3831e48daad Mon Sep 17 00:00:00 2001 From: mhead Date: Thu, 12 Sep 2024 11:15:58 +0530 Subject: [PATCH 2/9] mv: gnu test `backup-dir` regression fix --- src/uu/mv/src/mv.rs | 15 +++++++++++++++ tests/by-util/test_mv.rs | 21 +++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 5dff6124a1e..43a13e36181 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -531,6 +531,21 @@ fn rename( ) -> io::Result<()> { let mut backup_path = None; + // If `no-target-directory` is specified, we treat the destination as a file. + // In that case, if there is a trailing forward slash, we remove it. + let to = if path_ends_with_terminator(to) && opts.no_target_dir { + let _to = to.to_string_lossy(); + if cfg!(windows) { + Path::new(_to.trim_end_matches('\\')).to_path_buf() + } else { + Path::new(_to.trim_end_matches('/')).to_path_buf() + } + } else { + to.to_path_buf() + }; + + let to = &to; + if to.exists() { if opts.update == UpdateMode::ReplaceIfOlder && opts.overwrite == OverwriteMode::Interactive { diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index de043d88495..4bb90d85a8a 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -1779,3 +1779,24 @@ mod inter_partition_copying { .stderr_contains("Permission denied"); } } + +#[cfg(unix)] +#[test] +fn test_mv_backup_when_dest_has_trailing_slash() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir("D"); + at.mkdir("E"); + at.touch("D/d_file"); + at.touch("E/e_file"); + scene + .ucmd() + .arg("-T") + .arg("--backup=numbered") + .arg("D") + .arg("E/") + .succeeds(); + assert!(at.dir_exists("E.~1~"), "Backup folder not created"); + assert!(at.file_exists("E.~1~/e_file"), "Backup file not created"); + assert!(at.file_exists("E/d_file"), "source file didn't move"); +} From b7b96e77a13be28114d78b83645be5182125991c Mon Sep 17 00:00:00 2001 From: mhead Date: Thu, 12 Sep 2024 11:42:17 +0530 Subject: [PATCH 3/9] minor changes --- src/uu/mv/src/mv.rs | 36 ++++++++++++++++-------------------- tests/by-util/test_mv.rs | 27 --------------------------- 2 files changed, 16 insertions(+), 47 deletions(-) diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 43a13e36181..2b8bc8a4b9e 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -764,7 +764,7 @@ fn is_empty_dir(path: &Path) -> bool { fn move_dir(from: &Path, to: &Path, progress_bar: Option<&ProgressBar>) -> FsXResult { // The return value that represents the number of bytes copied. let mut result: u64 = 0; - let mut error_occured = false; + let mut error_occurred = false; for dir_entry_result in WalkDir::new(from) { match dir_entry_result { Ok(dir_entry) => { @@ -784,11 +784,11 @@ fn move_dir(from: &Path, to: &Path, progress_bar: Option<&ProgressBar>) -> FsXRe } } Err(_) => { - error_occured = true; + error_occurred = true; } } } - if !error_occured { + if !error_occurred { remove(from)?; } Ok(result) @@ -822,24 +822,20 @@ fn copy_file( } else { None }; - let result_file_copy = { - let md = from.metadata()?; - if cfg!(unix) && FileTypeExt::is_fifo(&md.file_type()) { - let file_size = md.len(); - uucore::fs::copy_fifo(to)?; - if let Some(progress_bar) = progress_bar { - progress_bar.set_position(file_size + progress_bar_start_val); - } - Ok(file_size) - } else { - if let Some(progress_handler) = progress_handler { - file::copy_with_progress(from, to, ©_options, progress_handler) - } else { - file::copy(from, to, ©_options) - } + + let md = from.metadata()?; + if cfg!(unix) && FileTypeExt::is_fifo(&md.file_type()) { + let file_size = md.len(); + uucore::fs::copy_fifo(to)?; + if let Some(progress_bar) = progress_bar { + progress_bar.set_position(file_size + progress_bar_start_val); } - }; - result_file_copy + Ok(file_size) + } else if let Some(progress_handler) = progress_handler { + file::copy_with_progress(from, to, ©_options, progress_handler) + } else { + file::copy(from, to, ©_options) + } } #[cfg(test)] diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index 4bb90d85a8a..8ed6689d4cf 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -1323,33 +1323,6 @@ fn test_mv_verbose() { "renamed '{file_a}' -> '{file_b}' (backup: '{file_b}~')\n" )); } -#[test] -fn test_verbose_src_symlink() { - let scene = TestScenario::new(util_name!()); - let at = &scene.fixtures; - let file_a = "test_mv_verbose_file_a"; - let ln = "link"; - at.touch(file_a); - at.symlink_file(&file_a, &ln); - scene - .ucmd() - .arg("-v") - .arg(file_a) - .arg(ln) - .succeeds() - .stdout_only(format!("renamed '{file_a}' -> '{ln}'\n")); - - at.touch(file_a); - scene - .ucmd() - .arg("-vb") - .arg(file_a) - .arg(ln) - .succeeds() - .stdout_only(format!( - "renamed '{file_a}' -> '{ln}' (backup: '{ln}~')\n" - )); -} #[test] #[cfg(any(target_os = "linux", target_os = "android"))] // mkdir does not support -m on windows. Freebsd doesn't return a permission error either. From 4f57607327b3ae4716a5e3dc163fb40742346d7a Mon Sep 17 00:00:00 2001 From: sreehari prasad <52113972+matrixhead@users.noreply.github.com> Date: Thu, 12 Sep 2024 14:06:22 +0530 Subject: [PATCH 4/9] Update src/uu/mv/src/mv.rs Co-authored-by: Sylvestre Ledru --- src/uu/mv/src/mv.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 2b8bc8a4b9e..1cab1c6f86a 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -534,12 +534,8 @@ fn rename( // If `no-target-directory` is specified, we treat the destination as a file. // In that case, if there is a trailing forward slash, we remove it. let to = if path_ends_with_terminator(to) && opts.no_target_dir { - let _to = to.to_string_lossy(); - if cfg!(windows) { - Path::new(_to.trim_end_matches('\\')).to_path_buf() - } else { - Path::new(_to.trim_end_matches('/')).to_path_buf() - } + let trimmed_to = to.to_string_lossy().trim_end_matches(MAIN_SEPARATOR.to_string()); + Path::new(trimmed_to).to_path_buf() } else { to.to_path_buf() }; From 35b7cb7bf86009b049f209cf6e4ab067a0687dc1 Mon Sep 17 00:00:00 2001 From: mhead Date: Thu, 12 Sep 2024 14:48:04 +0530 Subject: [PATCH 5/9] changed to MAIN_SEPARATOR --- src/uu/mv/src/mv.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 1cab1c6f86a..f6be2a8f131 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -19,7 +19,7 @@ use std::io; use std::os::unix; #[cfg(windows)] use std::os::windows; -use std::path::{Path, PathBuf}; +use std::path::{Path, PathBuf, MAIN_SEPARATOR}; #[cfg(unix)] use unix::fs::FileTypeExt; use uucore::backup_control::{self, source_is_target_backup}; @@ -534,8 +534,9 @@ fn rename( // If `no-target-directory` is specified, we treat the destination as a file. // In that case, if there is a trailing forward slash, we remove it. let to = if path_ends_with_terminator(to) && opts.no_target_dir { - let trimmed_to = to.to_string_lossy().trim_end_matches(MAIN_SEPARATOR.to_string()); - Path::new(trimmed_to).to_path_buf() + let to_str = to.to_string_lossy(); + let trimmed_to = to_str.trim_end_matches(MAIN_SEPARATOR); + Path::new(trimmed_to).to_path_buf() } else { to.to_path_buf() }; From ef9a5fabb681bc3cd97ade66c911cb8103ea160d Mon Sep 17 00:00:00 2001 From: mhead Date: Tue, 17 Sep 2024 14:49:03 +0530 Subject: [PATCH 6/9] added verbose context type to print verbose message --- src/uu/mv/src/mv.rs | 603 ++++++++++++++++++++++++++------------------ 1 file changed, 355 insertions(+), 248 deletions(-) diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index f6be2a8f131..d79e6a91dba 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -102,7 +102,68 @@ pub enum OverwriteMode { ///'-f' '--force' overwrite without prompt Force, } +struct VerboseContext<'a> { + backup: Option<&'a Path>, + pb: Option<&'a MultiProgress>, +} + +impl<'a> VerboseContext<'a> { + fn new(backup: Option<&'a Path>, pb: Option<&'a MultiProgress>) -> VerboseContext<'a> { + VerboseContext { backup, pb } + } + fn print_move_file(&self, from: &Path, to: &Path) { + let message = match self.backup.as_ref() { + Some(path) => format!( + "renamed {} -> {} (backup: {})", + from.quote(), + to.quote(), + path.quote() + ), + None => format!("renamed {} -> {}", from.quote(), to.quote()), + }; + match self.pb { + Some(pb) => pb.suspend(|| { + println!("{message}"); + }), + None => println!("{message}"), + }; + } + fn print_copy_file(&self, from: &Path, to: &Path, with_backup_message: bool) { + let message = match self.backup.as_ref() { + Some(path) if with_backup_message => format!( + "copied {} -> {} (backup: {})", + from.quote(), + to.quote(), + path.quote() + ), + _ => format!("copied {} -> {}", from.quote(), to.quote()), + }; + + match self.pb { + Some(pb) => pb.suspend(|| { + println!("{message}"); + }), + None => println!("{message}"), + }; + } + + fn create_folder(&self, path: &Path) { + println!( + "created directory {}", + path.to_string_lossy().trim_end_matches("/").quote() + ); + } + + fn remove_file(&self, from: &Path) { + println!("removed {}", from.quote()) + } + + fn remove_folder(&self, from: &Path) { + println!("removed directory {}", from.quote()) + } + +} const ABOUT: &str = help_about!("mv.md"); const USAGE: &str = help_usage!("mv.md"); const AFTER_HELP: &str = help_section!("after help", "mv.md"); @@ -586,7 +647,7 @@ fn rename( backup_path = backup_control::get_backup_path(opts.backup, to, &opts.suffix); if let Some(ref backup_path) = backup_path { - rename_with_fallback(to, backup_path, multi_progress)?; + rename_with_fallback(to, backup_path, multi_progress, None)?; } } @@ -602,26 +663,14 @@ fn rename( } } - rename_with_fallback(from, to, multi_progress)?; + let verbose_context = if opts.verbose { + Some(VerboseContext::new(backup_path.as_deref(), multi_progress)) + } else { + None + }; - if opts.verbose { - let message = match backup_path { - Some(path) => format!( - "renamed {} -> {} (backup: {})", - from.quote(), - to.quote(), - path.quote() - ), - None => format!("renamed {} -> {}", from.quote(), to.quote()), - }; + rename_with_fallback(from, to, multi_progress, verbose_context)?; - match multi_progress { - Some(pb) => pb.suspend(|| { - println!("{message}"); - }), - None => println!("{message}"), - }; - } Ok(()) } @@ -631,6 +680,7 @@ fn rename_with_fallback( from: &Path, to: &Path, multi_progress: Option<&MultiProgress>, + vebose_context: Option>, ) -> io::Result<()> { if fs::rename(from, to).is_err() { // Get metadata without following symlinks @@ -639,6 +689,9 @@ fn rename_with_fallback( if file_type.is_symlink() { rename_symlink_fallback(from, to)?; + if let Some(vc) = vebose_context { + vc.print_move_file(from, to); + } } else if file_type.is_dir() { // We remove the destination directory if it exists to match the // behavior of `fs::rename`. As far as I can tell, `fs_extra`'s @@ -672,7 +725,7 @@ fn rename_with_fallback( let xattrs = fsxattr::retrieve_xattrs(from).unwrap_or_else(|_| std::collections::HashMap::new()); - let result = move_dir(from, to, progress_bar.as_ref()); + let result = move_dir(from, to, progress_bar.as_ref(), vebose_context); #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] fsxattr::apply_xattrs(to, xattrs).unwrap(); @@ -702,10 +755,39 @@ fn rename_with_fallback( } #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] fs::copy(from, to) + .map(|v| { + if let Some(vc) = vebose_context.as_ref() { + vc.print_copy_file(from, to, true); + } + v + }) .and_then(|_| fsxattr::copy_xattrs(&from, &to)) - .and_then(|_| fs::remove_file(from))?; + .and_then(|_| { + let res = fs::remove_file(from); + if let Some(vc) = vebose_context { + vc.remove_file(from); + } + res + })?; #[cfg(any(target_os = "macos", target_os = "redox", not(unix)))] - fs::copy(from, to).and_then(|_| fs::remove_file(from))?; + fs::copy(from, to) + .map(|v| { + if let Some(vc) = vebose_context.as_ref() { + vc.print_copy_file(from, to, true); + } + v + }) + .and_then(|_| { + let res = fs::remove_file(from); + if let Some(vc) = vebose_context { + vc.remove_file(from); + } + res + })?; + } + } else { + if let Some(vb) = vebose_context { + vb.print_move_file(from, to); } } Ok(()) @@ -758,10 +840,16 @@ fn is_empty_dir(path: &Path) -> bool { /// Returns: /// - `Result`: The total number of bytes moved if successful. -fn move_dir(from: &Path, to: &Path, progress_bar: Option<&ProgressBar>) -> FsXResult { +fn move_dir( + from: &Path, + to: &Path, + progress_bar: Option<&ProgressBar>, + vebose_context: Option>, +) -> FsXResult { // The return value that represents the number of bytes copied. let mut result: u64 = 0; let mut error_occurred = false; + let mut moved_entries: Vec<(PathBuf, bool)> = vec![]; for dir_entry_result in WalkDir::new(from) { match dir_entry_result { Ok(dir_entry) => { @@ -769,14 +857,20 @@ fn move_dir(from: &Path, to: &Path, progress_bar: Option<&ProgressBar>) -> FsXRe let path = dir_entry.into_path(); let tmp_to = path.strip_prefix(from)?; let dir = to.join(tmp_to); - if !dir.exists() { - create_all(&dir, false)?; + create_all(&dir, false)?; + if let Some(vc) = vebose_context.as_ref() { + vc.create_folder(&dir) } + moved_entries.push((path, true)) } else { - let file = dir_entry.path(); + let file = dir_entry.into_path(); let tp = file.strip_prefix(from)?; let to_file = to.join(tp); - let result_file_copy = copy_file(file, &to_file, progress_bar, result)?; + let result_file_copy = copy_file(&file, &to_file, progress_bar, result)?; + if let Some(vc) = vebose_context.as_ref() { + vc.print_copy_file(&file, &to_file, false); + } + moved_entries.push((file, false)); result += result_file_copy; } } @@ -786,7 +880,20 @@ fn move_dir(from: &Path, to: &Path, progress_bar: Option<&ProgressBar>) -> FsXRe } } if !error_occurred { - remove(from)?; + while !moved_entries.is_empty() { + let (moved_path, is_dir) = moved_entries.pop().unwrap(); + if is_dir { + remove(&moved_path)?; + if let Some(vc) = vebose_context.as_ref() { + vc.remove_folder(&moved_path); + } + } else { + file::remove(&moved_path)?; + if let Some(vc) = vebose_context.as_ref() { + vc.remove_file(&moved_path); + } + } + } } Ok(result) } @@ -835,223 +942,223 @@ fn copy_file( } } -#[cfg(test)] -mod tests { - use std::path::PathBuf; - extern crate fs_extra; - use super::{copy_file, move_dir}; - use fs_extra::dir::*; - use indicatif::{ProgressBar, ProgressStyle}; - use tempfile::tempdir; - - // These tests are copied from the `fs_extra`'s repository - #[test] - fn it_move_work() { - for with_progress_bar in [false, true] { - let temp_dir = tempdir().unwrap(); - let mut path_from = PathBuf::from(temp_dir.path()); - let test_name = "sub"; - path_from.push("it_move_work"); - let mut path_to = path_from.clone(); - path_to.push("out"); - path_from.push(test_name); - - create_all(&path_from, true).unwrap(); - assert!(path_from.exists()); - create_all(&path_to, true).unwrap(); - assert!(path_to.exists()); - - let mut file1_path = path_from.clone(); - file1_path.push("test1.txt"); - let content1 = "content1"; - fs_extra::file::write_all(&file1_path, content1).unwrap(); - assert!(file1_path.exists()); - - let mut sub_dir_path = path_from.clone(); - sub_dir_path.push("sub"); - create(&sub_dir_path, true).unwrap(); - let mut file2_path = sub_dir_path.clone(); - file2_path.push("test2.txt"); - let content2 = "content2"; - fs_extra::file::write_all(&file2_path, content2).unwrap(); - assert!(file2_path.exists()); - - let pb = if with_progress_bar { - Some( - ProgressBar::new(16).with_style( - ProgressStyle::with_template( - "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", - ) - .unwrap(), - ), - ) - } else { - None - }; - - let result = move_dir(&path_from, &path_to, pb.as_ref()).unwrap(); - - assert_eq!(16, result); - assert!(path_to.exists()); - assert!(!path_from.exists()); - if let Some(pb) = pb { - assert_eq!(pb.position(), 16); - } - } - } - - #[test] - fn it_move_exist_overwrite() { - for with_progress_bar in [false, true] { - let temp_dir = tempdir().unwrap(); - let mut path_from = PathBuf::from(temp_dir.path()); - let test_name = "sub"; - path_from.push("it_move_exist_overwrite"); - let mut path_to = path_from.clone(); - path_to.push("out"); - path_from.push(test_name); - let same_file = "test.txt"; - - create_all(&path_from, true).unwrap(); - assert!(path_from.exists()); - create_all(&path_to, true).unwrap(); - assert!(path_to.exists()); - - let mut file1_path = path_from.clone(); - file1_path.push(same_file); - let content1 = "content1"; - fs_extra::file::write_all(&file1_path, content1).unwrap(); - assert!(file1_path.exists()); - - let mut sub_dir_path = path_from.clone(); - sub_dir_path.push("sub"); - create(&sub_dir_path, true).unwrap(); - let mut file2_path = sub_dir_path.clone(); - file2_path.push("test2.txt"); - let content2 = "content2"; - fs_extra::file::write_all(&file2_path, content2).unwrap(); - assert!(file2_path.exists()); - - let mut exist_path = path_to.clone(); - exist_path.push(test_name); - create(&exist_path, true).unwrap(); - assert!(exist_path.exists()); - exist_path.push(same_file); - let exist_content = "exist content"; - assert_ne!(exist_content, content1); - fs_extra::file::write_all(&exist_path, exist_content).unwrap(); - assert!(exist_path.exists()); - - let dir_size = get_size(&path_from).expect("failed to get dir size"); - let pb = if with_progress_bar { - Some( - ProgressBar::new(dir_size).with_style( - ProgressStyle::with_template( - "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", - ) - .unwrap(), - ), - ) - } else { - None - }; - move_dir(&path_from, &path_to, pb.as_ref()).unwrap(); - assert!(exist_path.exists()); - assert!(path_to.exists()); - assert!(!path_from.exists()); - if let Some(pb) = pb { - assert_eq!(pb.position(), dir_size); - } - } - } - - #[test] - fn it_move_inside_work_target_dir_not_exist() { - for with_progress_bar in [false, true] { - let temp_dir = tempdir().unwrap(); - let path_root = PathBuf::from(temp_dir.path()); - let root = path_root.join("it_move_inside_work_target_dir_not_exist"); - let root_dir1 = root.join("dir1"); - let root_dir1_sub = root_dir1.join("sub"); - let root_dir2 = root.join("dir2"); - let file1 = root_dir1.join("file1.txt"); - let file2 = root_dir1_sub.join("file2.txt"); - - create_all(&root_dir1_sub, true).unwrap(); - fs_extra::file::write_all(&file1, "content1").unwrap(); - fs_extra::file::write_all(&file2, "content2").unwrap(); - - if root_dir2.exists() { - remove(&root_dir2).unwrap(); - } - - assert!(root_dir1.exists()); - assert!(root_dir1_sub.exists()); - assert!(!root_dir2.exists()); - assert!(file1.exists()); - assert!(file2.exists()); - let dir_size = get_size(&root_dir1).expect("failed to get dir size"); - let pb = if with_progress_bar { - Some( - ProgressBar::new(dir_size).with_style( - ProgressStyle::with_template( - "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", - ) - .unwrap(), - ), - ) - } else { - None - }; - - let result = move_dir(&root_dir1, &root_dir2, pb.as_ref()).unwrap(); - - assert_eq!(16, result); - assert!(!root_dir1.exists()); - let root_dir2_sub = root_dir2.join("sub"); - let root_dir2_file1 = root_dir2.join("file1.txt"); - let root_dir2_sub_file2 = root_dir2_sub.join("file2.txt"); - assert!(root_dir2.exists()); - assert!(root_dir2_sub.exists()); - assert!(root_dir2_file1.exists()); - assert!(root_dir2_sub_file2.exists()); - if let Some(pb) = pb { - assert_eq!(pb.position(), dir_size); - } - } - } - - #[test] - fn copy_file_test() { - for with_progress_bar in [false, true] { - let temp_dir = tempdir().unwrap(); - let temp_dir_path = temp_dir.path(); - - let file1_path = temp_dir_path.join("file"); - let content = "content"; - fs_extra::file::write_all(&file1_path, content).unwrap(); - assert!(file1_path.exists()); - let path_to = temp_dir_path.join("file_out"); - let pb = if with_progress_bar { - Some( - ProgressBar::new(7).with_style( - ProgressStyle::with_template( - "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", - ) - .unwrap(), - ), - ) - } else { - None - }; - - let result = copy_file(&file1_path, &path_to, pb.as_ref(), 0).expect("move failed"); - - assert_eq!(7, result); - assert!(path_to.exists()); - if let Some(pb) = pb { - assert_eq!(pb.position(), 7); - } - } - } -} +// #[cfg(test)] +// mod tests { +// use std::path::PathBuf; +// extern crate fs_extra; +// use super::{copy_file, move_dir}; +// use fs_extra::dir::*; +// use indicatif::{ProgressBar, ProgressStyle}; +// use tempfile::tempdir; + +// // These tests are copied from the `fs_extra`'s repository +// #[test] +// fn it_move_work() { +// for with_progress_bar in [false, true] { +// let temp_dir = tempdir().unwrap(); +// let mut path_from = PathBuf::from(temp_dir.path()); +// let test_name = "sub"; +// path_from.push("it_move_work"); +// let mut path_to = path_from.clone(); +// path_to.push("out"); +// path_from.push(test_name); + +// create_all(&path_from, true).unwrap(); +// assert!(path_from.exists()); +// create_all(&path_to, true).unwrap(); +// assert!(path_to.exists()); + +// let mut file1_path = path_from.clone(); +// file1_path.push("test1.txt"); +// let content1 = "content1"; +// fs_extra::file::write_all(&file1_path, content1).unwrap(); +// assert!(file1_path.exists()); + +// let mut sub_dir_path = path_from.clone(); +// sub_dir_path.push("sub"); +// create(&sub_dir_path, true).unwrap(); +// let mut file2_path = sub_dir_path.clone(); +// file2_path.push("test2.txt"); +// let content2 = "content2"; +// fs_extra::file::write_all(&file2_path, content2).unwrap(); +// assert!(file2_path.exists()); + +// let pb = if with_progress_bar { +// Some( +// ProgressBar::new(16).with_style( +// ProgressStyle::with_template( +// "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", +// ) +// .unwrap(), +// ), +// ) +// } else { +// None +// }; + +// let result = move_dir(&path_from, &path_to, pb.as_ref()).unwrap(); + +// assert_eq!(16, result); +// assert!(path_to.exists()); +// assert!(!path_from.exists()); +// if let Some(pb) = pb { +// assert_eq!(pb.position(), 16); +// } +// } +// } + +// #[test] +// fn it_move_exist_overwrite() { +// for with_progress_bar in [false, true] { +// let temp_dir = tempdir().unwrap(); +// let mut path_from = PathBuf::from(temp_dir.path()); +// let test_name = "sub"; +// path_from.push("it_move_exist_overwrite"); +// let mut path_to = path_from.clone(); +// path_to.push("out"); +// path_from.push(test_name); +// let same_file = "test.txt"; + +// create_all(&path_from, true).unwrap(); +// assert!(path_from.exists()); +// create_all(&path_to, true).unwrap(); +// assert!(path_to.exists()); + +// let mut file1_path = path_from.clone(); +// file1_path.push(same_file); +// let content1 = "content1"; +// fs_extra::file::write_all(&file1_path, content1).unwrap(); +// assert!(file1_path.exists()); + +// let mut sub_dir_path = path_from.clone(); +// sub_dir_path.push("sub"); +// create(&sub_dir_path, true).unwrap(); +// let mut file2_path = sub_dir_path.clone(); +// file2_path.push("test2.txt"); +// let content2 = "content2"; +// fs_extra::file::write_all(&file2_path, content2).unwrap(); +// assert!(file2_path.exists()); + +// let mut exist_path = path_to.clone(); +// exist_path.push(test_name); +// create(&exist_path, true).unwrap(); +// assert!(exist_path.exists()); +// exist_path.push(same_file); +// let exist_content = "exist content"; +// assert_ne!(exist_content, content1); +// fs_extra::file::write_all(&exist_path, exist_content).unwrap(); +// assert!(exist_path.exists()); + +// let dir_size = get_size(&path_from).expect("failed to get dir size"); +// let pb = if with_progress_bar { +// Some( +// ProgressBar::new(dir_size).with_style( +// ProgressStyle::with_template( +// "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", +// ) +// .unwrap(), +// ), +// ) +// } else { +// None +// }; +// move_dir(&path_from, &path_to, pb.as_ref()).unwrap(); +// assert!(exist_path.exists()); +// assert!(path_to.exists()); +// assert!(!path_from.exists()); +// if let Some(pb) = pb { +// assert_eq!(pb.position(), dir_size); +// } +// } +// } + +// #[test] +// fn it_move_inside_work_target_dir_not_exist() { +// for with_progress_bar in [false, true] { +// let temp_dir = tempdir().unwrap(); +// let path_root = PathBuf::from(temp_dir.path()); +// let root = path_root.join("it_move_inside_work_target_dir_not_exist"); +// let root_dir1 = root.join("dir1"); +// let root_dir1_sub = root_dir1.join("sub"); +// let root_dir2 = root.join("dir2"); +// let file1 = root_dir1.join("file1.txt"); +// let file2 = root_dir1_sub.join("file2.txt"); + +// create_all(&root_dir1_sub, true).unwrap(); +// fs_extra::file::write_all(&file1, "content1").unwrap(); +// fs_extra::file::write_all(&file2, "content2").unwrap(); + +// if root_dir2.exists() { +// remove(&root_dir2).unwrap(); +// } + +// assert!(root_dir1.exists()); +// assert!(root_dir1_sub.exists()); +// assert!(!root_dir2.exists()); +// assert!(file1.exists()); +// assert!(file2.exists()); +// let dir_size = get_size(&root_dir1).expect("failed to get dir size"); +// let pb = if with_progress_bar { +// Some( +// ProgressBar::new(dir_size).with_style( +// ProgressStyle::with_template( +// "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", +// ) +// .unwrap(), +// ), +// ) +// } else { +// None +// }; + +// let result = move_dir(&root_dir1, &root_dir2, pb.as_ref()).unwrap(); + +// assert_eq!(16, result); +// assert!(!root_dir1.exists()); +// let root_dir2_sub = root_dir2.join("sub"); +// let root_dir2_file1 = root_dir2.join("file1.txt"); +// let root_dir2_sub_file2 = root_dir2_sub.join("file2.txt"); +// assert!(root_dir2.exists()); +// assert!(root_dir2_sub.exists()); +// assert!(root_dir2_file1.exists()); +// assert!(root_dir2_sub_file2.exists()); +// if let Some(pb) = pb { +// assert_eq!(pb.position(), dir_size); +// } +// } +// } + +// #[test] +// fn copy_file_test() { +// for with_progress_bar in [false, true] { +// let temp_dir = tempdir().unwrap(); +// let temp_dir_path = temp_dir.path(); + +// let file1_path = temp_dir_path.join("file"); +// let content = "content"; +// fs_extra::file::write_all(&file1_path, content).unwrap(); +// assert!(file1_path.exists()); +// let path_to = temp_dir_path.join("file_out"); +// let pb = if with_progress_bar { +// Some( +// ProgressBar::new(7).with_style( +// ProgressStyle::with_template( +// "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", +// ) +// .unwrap(), +// ), +// ) +// } else { +// None +// }; + +// let result = copy_file(&file1_path, &path_to, pb.as_ref(), 0).expect("move failed"); + +// assert_eq!(7, result); +// assert!(path_to.exists()); +// if let Some(pb) = pb { +// assert_eq!(pb.position(), 7); +// } +// } +// } +// } From bd275f8b4695761059938a615cc0407f261996ff Mon Sep 17 00:00:00 2001 From: mhead Date: Thu, 19 Sep 2024 14:13:25 +0530 Subject: [PATCH 7/9] mv: added dir copy error messages and types --- src/uu/mv/src/error.rs | 19 ++++++++- src/uu/mv/src/mv.rs | 97 +++++++++++++++++++++++++++++------------- 2 files changed, 86 insertions(+), 30 deletions(-) diff --git a/src/uu/mv/src/error.rs b/src/uu/mv/src/error.rs index f989d4e1332..cfc00a8e489 100644 --- a/src/uu/mv/src/error.rs +++ b/src/uu/mv/src/error.rs @@ -7,6 +7,8 @@ use std::fmt::{Display, Formatter, Result}; use uucore::error::UError; +use fs_extra::error::Error as FsXError; + #[derive(Debug)] pub enum MvError { NoSuchFile(String), @@ -19,6 +21,8 @@ pub enum MvError { NotADirectory(String), TargetNotADirectory(String), FailedToAccessNotADirectory(String), + FsXError(FsXError), + NotAllFilesMoved } impl Error for MvError {} @@ -48,7 +52,20 @@ impl Display for MvError { Self::FailedToAccessNotADirectory(t) => { write!(f, "failed to access {t}: Not a directory") - } + }, + Self::FsXError(err)=>{ + write!(f, "{err}") + }, + Self::NotAllFilesMoved => { + write!(f, "failed to move all files") + }, + } } } + +impl From for MvError{ + fn from(err: FsXError) -> Self { + MvError::FsXError(err) + } +} diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index d79e6a91dba..7b24e7e6e5a 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -11,15 +11,15 @@ use clap::builder::ValueParser; use clap::{crate_version, error::ErrorKind, Arg, ArgAction, ArgMatches, Command}; use indicatif::{MultiProgress, ProgressBar, ProgressStyle}; use std::collections::HashSet; -use std::env; use std::ffi::OsString; -use std::fs; +use std::fs::{self, FileType}; use std::io; #[cfg(unix)] use std::os::unix; #[cfg(windows)] use std::os::windows; use std::path::{Path, PathBuf, MAIN_SEPARATOR}; +use std::env; #[cfg(unix)] use unix::fs::FileTypeExt; use uucore::backup_control::{self, source_is_target_backup}; @@ -31,7 +31,7 @@ use uucore::fs::{ }; #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] use uucore::fsxattr; -use uucore::update_control; +use uucore::{show_error, update_control}; use walkdir::WalkDir; // These are exposed for projects (e.g. nushell) that want to create an `Options` value, which @@ -47,6 +47,8 @@ use fs_extra::{ use crate::error::MvError; +type MvResult = Result; + /// Options contains all the possible behaviors and flags for mv. /// /// All options are public so that the options can be programmatically @@ -162,7 +164,6 @@ impl<'a> VerboseContext<'a> { fn remove_folder(&self, from: &Path) { println!("removed directory {}", from.quote()) } - } const ABOUT: &str = help_about!("mv.md"); const USAGE: &str = help_usage!("mv.md"); @@ -731,11 +732,17 @@ fn rename_with_fallback( fsxattr::apply_xattrs(to, xattrs).unwrap(); if let Err(err) = result { - return match err.kind { - fs_extra::error::ErrorKind::PermissionDenied => Err(io::Error::new( + return match err { + MvError::FsXError(fs_extra::error::Error { + kind: fs_extra::error::ErrorKind::PermissionDenied, + .. + }) => Err(io::Error::new( io::ErrorKind::PermissionDenied, "Permission denied", )), + MvError::NotAllFilesMoved => { + Err(io::Error::new(io::ErrorKind::Other, format!(""))) + } _ => Err(io::Error::new(io::ErrorKind::Other, format!("{err:?}"))), }; } @@ -845,55 +852,87 @@ fn move_dir( to: &Path, progress_bar: Option<&ProgressBar>, vebose_context: Option>, -) -> FsXResult { +) -> MvResult { // The return value that represents the number of bytes copied. let mut result: u64 = 0; let mut error_occurred = false; - let mut moved_entries: Vec<(PathBuf, bool)> = vec![]; + let mut moved_entries: Vec<(PathBuf, FileType)> = vec![]; for dir_entry_result in WalkDir::new(from) { match dir_entry_result { Ok(dir_entry) => { - if dir_entry.file_type().is_dir() { - let path = dir_entry.into_path(); - let tmp_to = path.strip_prefix(from)?; - let dir = to.join(tmp_to); - create_all(&dir, false)?; + let file_type = dir_entry.file_type(); + let dir_entry_path = dir_entry.into_path(); + //comment why this is okay + let tmp_to = dir_entry_path.strip_prefix(&from).unwrap(); + let dir_entry_to = to.join(tmp_to); + if file_type.is_dir() { + // check this create all align with gnu + let res = create_all(&dir_entry_to, false); + if res.is_err() { + error_occurred = true; + show_error!("{:?}", res.unwrap_err()); + continue; + } if let Some(vc) = vebose_context.as_ref() { - vc.create_folder(&dir) + vc.create_folder(&dir_entry_to) } - moved_entries.push((path, true)) } else { - let file = dir_entry.into_path(); - let tp = file.strip_prefix(from)?; - let to_file = to.join(tp); - let result_file_copy = copy_file(&file, &to_file, progress_bar, result)?; + let result_file_copy = + copy_file(&dir_entry_path, &dir_entry_to, progress_bar, result); + + match result_file_copy { + Ok(result_file_copy) => { + result += result_file_copy; + } + Err(err) => { + let err_msg = match err.kind { + fs_extra::error::ErrorKind::Io(error) => { + format!("error writing {}: {}", dir_entry_to.quote(), error) + } + _ => { + format!("{:?}", err) + } + }; + error_occurred = true; + show_error!("{}", err_msg); + continue; + } + } if let Some(vc) = vebose_context.as_ref() { - vc.print_copy_file(&file, &to_file, false); + vc.print_copy_file(&dir_entry_path, &dir_entry_to, false); } - moved_entries.push((file, false)); - result += result_file_copy; } + moved_entries.push((dir_entry_path, file_type)); } - Err(_) => { + Err(err) => { + let err_msg = match (err.io_error(), err.path()) { + (Some(io_error), Some(path)) => { + format!("cannot access {}: {io_error}", path.quote()) + } + _ => err.to_string(), + }; + show_error!("{err_msg}"); error_occurred = true; } } } if !error_occurred { while !moved_entries.is_empty() { - let (moved_path, is_dir) = moved_entries.pop().unwrap(); - if is_dir { + let (moved_path, file_type) = moved_entries.pop().unwrap(); + if file_type.is_dir() { remove(&moved_path)?; if let Some(vc) = vebose_context.as_ref() { - vc.remove_folder(&moved_path); - } + vc.remove_folder(&moved_path); + } } else { file::remove(&moved_path)?; if let Some(vc) = vebose_context.as_ref() { - vc.remove_file(&moved_path); - } + vc.remove_file(&moved_path); + } } } + } else { + return Err(MvError::NotAllFilesMoved); } Ok(result) } From 06705661609682d4985d87d0c1e3dacecc7f2793 Mon Sep 17 00:00:00 2001 From: mhead Date: Sun, 22 Sep 2024 15:48:47 +0530 Subject: [PATCH 8/9] mv: tests and minor changes --- src/uu/mv/src/mv.rs | 471 ++++++++++++++++++++------------------- tests/by-util/test_mv.rs | 193 +++++++++++++++- 2 files changed, 433 insertions(+), 231 deletions(-) diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 7b24e7e6e5a..9749bc96b7c 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -11,6 +11,7 @@ use clap::builder::ValueParser; use clap::{crate_version, error::ErrorKind, Arg, ArgAction, ArgMatches, Command}; use indicatif::{MultiProgress, ProgressBar, ProgressStyle}; use std::collections::HashSet; +use std::env; use std::ffi::OsString; use std::fs::{self, FileType}; use std::io; @@ -19,7 +20,6 @@ use std::os::unix; #[cfg(windows)] use std::os::windows; use std::path::{Path, PathBuf, MAIN_SEPARATOR}; -use std::env; #[cfg(unix)] use unix::fs::FileTypeExt; use uucore::backup_control::{self, source_is_target_backup}; @@ -113,6 +113,7 @@ impl<'a> VerboseContext<'a> { fn new(backup: Option<&'a Path>, pb: Option<&'a MultiProgress>) -> VerboseContext<'a> { VerboseContext { backup, pb } } + fn print_move_file(&self, from: &Path, to: &Path) { let message = match self.backup.as_ref() { Some(path) => format!( @@ -131,6 +132,7 @@ impl<'a> VerboseContext<'a> { None => println!("{message}"), }; } + fn print_copy_file(&self, from: &Path, to: &Path, with_backup_message: bool) { let message = match self.backup.as_ref() { Some(path) if with_backup_message => format!( @@ -772,7 +774,9 @@ fn rename_with_fallback( .and_then(|_| { let res = fs::remove_file(from); if let Some(vc) = vebose_context { - vc.remove_file(from); + if res.is_ok() { + vc.remove_file(from); + } } res })?; @@ -919,19 +923,26 @@ fn move_dir( if !error_occurred { while !moved_entries.is_empty() { let (moved_path, file_type) = moved_entries.pop().unwrap(); - if file_type.is_dir() { - remove(&moved_path)?; - if let Some(vc) = vebose_context.as_ref() { - vc.remove_folder(&moved_path); - } + let res = if file_type.is_dir() { + remove(&moved_path) + } else { + file::remove(&moved_path) + }; + if let Err(err) = res { + error_occurred = true; + show_error!("cannot remove {}: {}", moved_path.quote(), err); } else { - file::remove(&moved_path)?; if let Some(vc) = vebose_context.as_ref() { - vc.remove_file(&moved_path); + if file_type.is_dir() { + vc.remove_folder(&moved_path); + } else { + vc.remove_file(&moved_path); + } } } } - } else { + } + if error_occurred { return Err(MvError::NotAllFilesMoved); } Ok(result) @@ -981,223 +992,223 @@ fn copy_file( } } -// #[cfg(test)] -// mod tests { -// use std::path::PathBuf; -// extern crate fs_extra; -// use super::{copy_file, move_dir}; -// use fs_extra::dir::*; -// use indicatif::{ProgressBar, ProgressStyle}; -// use tempfile::tempdir; - -// // These tests are copied from the `fs_extra`'s repository -// #[test] -// fn it_move_work() { -// for with_progress_bar in [false, true] { -// let temp_dir = tempdir().unwrap(); -// let mut path_from = PathBuf::from(temp_dir.path()); -// let test_name = "sub"; -// path_from.push("it_move_work"); -// let mut path_to = path_from.clone(); -// path_to.push("out"); -// path_from.push(test_name); - -// create_all(&path_from, true).unwrap(); -// assert!(path_from.exists()); -// create_all(&path_to, true).unwrap(); -// assert!(path_to.exists()); - -// let mut file1_path = path_from.clone(); -// file1_path.push("test1.txt"); -// let content1 = "content1"; -// fs_extra::file::write_all(&file1_path, content1).unwrap(); -// assert!(file1_path.exists()); - -// let mut sub_dir_path = path_from.clone(); -// sub_dir_path.push("sub"); -// create(&sub_dir_path, true).unwrap(); -// let mut file2_path = sub_dir_path.clone(); -// file2_path.push("test2.txt"); -// let content2 = "content2"; -// fs_extra::file::write_all(&file2_path, content2).unwrap(); -// assert!(file2_path.exists()); - -// let pb = if with_progress_bar { -// Some( -// ProgressBar::new(16).with_style( -// ProgressStyle::with_template( -// "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", -// ) -// .unwrap(), -// ), -// ) -// } else { -// None -// }; - -// let result = move_dir(&path_from, &path_to, pb.as_ref()).unwrap(); - -// assert_eq!(16, result); -// assert!(path_to.exists()); -// assert!(!path_from.exists()); -// if let Some(pb) = pb { -// assert_eq!(pb.position(), 16); -// } -// } -// } - -// #[test] -// fn it_move_exist_overwrite() { -// for with_progress_bar in [false, true] { -// let temp_dir = tempdir().unwrap(); -// let mut path_from = PathBuf::from(temp_dir.path()); -// let test_name = "sub"; -// path_from.push("it_move_exist_overwrite"); -// let mut path_to = path_from.clone(); -// path_to.push("out"); -// path_from.push(test_name); -// let same_file = "test.txt"; - -// create_all(&path_from, true).unwrap(); -// assert!(path_from.exists()); -// create_all(&path_to, true).unwrap(); -// assert!(path_to.exists()); - -// let mut file1_path = path_from.clone(); -// file1_path.push(same_file); -// let content1 = "content1"; -// fs_extra::file::write_all(&file1_path, content1).unwrap(); -// assert!(file1_path.exists()); - -// let mut sub_dir_path = path_from.clone(); -// sub_dir_path.push("sub"); -// create(&sub_dir_path, true).unwrap(); -// let mut file2_path = sub_dir_path.clone(); -// file2_path.push("test2.txt"); -// let content2 = "content2"; -// fs_extra::file::write_all(&file2_path, content2).unwrap(); -// assert!(file2_path.exists()); - -// let mut exist_path = path_to.clone(); -// exist_path.push(test_name); -// create(&exist_path, true).unwrap(); -// assert!(exist_path.exists()); -// exist_path.push(same_file); -// let exist_content = "exist content"; -// assert_ne!(exist_content, content1); -// fs_extra::file::write_all(&exist_path, exist_content).unwrap(); -// assert!(exist_path.exists()); - -// let dir_size = get_size(&path_from).expect("failed to get dir size"); -// let pb = if with_progress_bar { -// Some( -// ProgressBar::new(dir_size).with_style( -// ProgressStyle::with_template( -// "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", -// ) -// .unwrap(), -// ), -// ) -// } else { -// None -// }; -// move_dir(&path_from, &path_to, pb.as_ref()).unwrap(); -// assert!(exist_path.exists()); -// assert!(path_to.exists()); -// assert!(!path_from.exists()); -// if let Some(pb) = pb { -// assert_eq!(pb.position(), dir_size); -// } -// } -// } - -// #[test] -// fn it_move_inside_work_target_dir_not_exist() { -// for with_progress_bar in [false, true] { -// let temp_dir = tempdir().unwrap(); -// let path_root = PathBuf::from(temp_dir.path()); -// let root = path_root.join("it_move_inside_work_target_dir_not_exist"); -// let root_dir1 = root.join("dir1"); -// let root_dir1_sub = root_dir1.join("sub"); -// let root_dir2 = root.join("dir2"); -// let file1 = root_dir1.join("file1.txt"); -// let file2 = root_dir1_sub.join("file2.txt"); - -// create_all(&root_dir1_sub, true).unwrap(); -// fs_extra::file::write_all(&file1, "content1").unwrap(); -// fs_extra::file::write_all(&file2, "content2").unwrap(); - -// if root_dir2.exists() { -// remove(&root_dir2).unwrap(); -// } - -// assert!(root_dir1.exists()); -// assert!(root_dir1_sub.exists()); -// assert!(!root_dir2.exists()); -// assert!(file1.exists()); -// assert!(file2.exists()); -// let dir_size = get_size(&root_dir1).expect("failed to get dir size"); -// let pb = if with_progress_bar { -// Some( -// ProgressBar::new(dir_size).with_style( -// ProgressStyle::with_template( -// "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", -// ) -// .unwrap(), -// ), -// ) -// } else { -// None -// }; - -// let result = move_dir(&root_dir1, &root_dir2, pb.as_ref()).unwrap(); - -// assert_eq!(16, result); -// assert!(!root_dir1.exists()); -// let root_dir2_sub = root_dir2.join("sub"); -// let root_dir2_file1 = root_dir2.join("file1.txt"); -// let root_dir2_sub_file2 = root_dir2_sub.join("file2.txt"); -// assert!(root_dir2.exists()); -// assert!(root_dir2_sub.exists()); -// assert!(root_dir2_file1.exists()); -// assert!(root_dir2_sub_file2.exists()); -// if let Some(pb) = pb { -// assert_eq!(pb.position(), dir_size); -// } -// } -// } - -// #[test] -// fn copy_file_test() { -// for with_progress_bar in [false, true] { -// let temp_dir = tempdir().unwrap(); -// let temp_dir_path = temp_dir.path(); - -// let file1_path = temp_dir_path.join("file"); -// let content = "content"; -// fs_extra::file::write_all(&file1_path, content).unwrap(); -// assert!(file1_path.exists()); -// let path_to = temp_dir_path.join("file_out"); -// let pb = if with_progress_bar { -// Some( -// ProgressBar::new(7).with_style( -// ProgressStyle::with_template( -// "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", -// ) -// .unwrap(), -// ), -// ) -// } else { -// None -// }; - -// let result = copy_file(&file1_path, &path_to, pb.as_ref(), 0).expect("move failed"); - -// assert_eq!(7, result); -// assert!(path_to.exists()); -// if let Some(pb) = pb { -// assert_eq!(pb.position(), 7); -// } -// } -// } -// } +#[cfg(test)] +mod tests { + use std::path::PathBuf; + extern crate fs_extra; + use super::{copy_file, move_dir}; + use fs_extra::dir::*; + use indicatif::{ProgressBar, ProgressStyle}; + use tempfile::tempdir; + + // These tests are copied from the `fs_extra`'s repository + #[test] + fn it_move_work() { + for with_progress_bar in [false, true] { + let temp_dir = tempdir().unwrap(); + let mut path_from = PathBuf::from(temp_dir.path()); + let test_name = "sub"; + path_from.push("it_move_work"); + let mut path_to = path_from.clone(); + path_to.push("out"); + path_from.push(test_name); + + create_all(&path_from, true).unwrap(); + assert!(path_from.exists()); + create_all(&path_to, true).unwrap(); + assert!(path_to.exists()); + + let mut file1_path = path_from.clone(); + file1_path.push("test1.txt"); + let content1 = "content1"; + fs_extra::file::write_all(&file1_path, content1).unwrap(); + assert!(file1_path.exists()); + + let mut sub_dir_path = path_from.clone(); + sub_dir_path.push("sub"); + create(&sub_dir_path, true).unwrap(); + let mut file2_path = sub_dir_path.clone(); + file2_path.push("test2.txt"); + let content2 = "content2"; + fs_extra::file::write_all(&file2_path, content2).unwrap(); + assert!(file2_path.exists()); + + let pb = if with_progress_bar { + Some( + ProgressBar::new(16).with_style( + ProgressStyle::with_template( + "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", + ) + .unwrap(), + ), + ) + } else { + None + }; + + let result = move_dir(&path_from, &path_to, pb.as_ref(), None).unwrap(); + + assert_eq!(16, result); + assert!(path_to.exists()); + assert!(!path_from.exists()); + if let Some(pb) = pb { + assert_eq!(pb.position(), 16); + } + } + } + + #[test] + fn it_move_exist_overwrite() { + for with_progress_bar in [false, true] { + let temp_dir = tempdir().unwrap(); + let mut path_from = PathBuf::from(temp_dir.path()); + let test_name = "sub"; + path_from.push("it_move_exist_overwrite"); + let mut path_to = path_from.clone(); + path_to.push("out"); + path_from.push(test_name); + let same_file = "test.txt"; + + create_all(&path_from, true).unwrap(); + assert!(path_from.exists()); + create_all(&path_to, true).unwrap(); + assert!(path_to.exists()); + + let mut file1_path = path_from.clone(); + file1_path.push(same_file); + let content1 = "content1"; + fs_extra::file::write_all(&file1_path, content1).unwrap(); + assert!(file1_path.exists()); + + let mut sub_dir_path = path_from.clone(); + sub_dir_path.push("sub"); + create(&sub_dir_path, true).unwrap(); + let mut file2_path = sub_dir_path.clone(); + file2_path.push("test2.txt"); + let content2 = "content2"; + fs_extra::file::write_all(&file2_path, content2).unwrap(); + assert!(file2_path.exists()); + + let mut exist_path = path_to.clone(); + exist_path.push(test_name); + create(&exist_path, true).unwrap(); + assert!(exist_path.exists()); + exist_path.push(same_file); + let exist_content = "exist content"; + assert_ne!(exist_content, content1); + fs_extra::file::write_all(&exist_path, exist_content).unwrap(); + assert!(exist_path.exists()); + + let dir_size = get_size(&path_from).expect("failed to get dir size"); + let pb = if with_progress_bar { + Some( + ProgressBar::new(dir_size).with_style( + ProgressStyle::with_template( + "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", + ) + .unwrap(), + ), + ) + } else { + None + }; + move_dir(&path_from, &path_to, pb.as_ref(), None).unwrap(); + assert!(exist_path.exists()); + assert!(path_to.exists()); + assert!(!path_from.exists()); + if let Some(pb) = pb { + assert_eq!(pb.position(), dir_size); + } + } + } + + #[test] + fn it_move_inside_work_target_dir_not_exist() { + for with_progress_bar in [false, true] { + let temp_dir = tempdir().unwrap(); + let path_root = PathBuf::from(temp_dir.path()); + let root = path_root.join("it_move_inside_work_target_dir_not_exist"); + let root_dir1 = root.join("dir1"); + let root_dir1_sub = root_dir1.join("sub"); + let root_dir2 = root.join("dir2"); + let file1 = root_dir1.join("file1.txt"); + let file2 = root_dir1_sub.join("file2.txt"); + + create_all(&root_dir1_sub, true).unwrap(); + fs_extra::file::write_all(&file1, "content1").unwrap(); + fs_extra::file::write_all(&file2, "content2").unwrap(); + + if root_dir2.exists() { + remove(&root_dir2).unwrap(); + } + + assert!(root_dir1.exists()); + assert!(root_dir1_sub.exists()); + assert!(!root_dir2.exists()); + assert!(file1.exists()); + assert!(file2.exists()); + let dir_size = get_size(&root_dir1).expect("failed to get dir size"); + let pb = if with_progress_bar { + Some( + ProgressBar::new(dir_size).with_style( + ProgressStyle::with_template( + "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", + ) + .unwrap(), + ), + ) + } else { + None + }; + + let result = move_dir(&root_dir1, &root_dir2, pb.as_ref(), None).unwrap(); + + assert_eq!(16, result); + assert!(!root_dir1.exists()); + let root_dir2_sub = root_dir2.join("sub"); + let root_dir2_file1 = root_dir2.join("file1.txt"); + let root_dir2_sub_file2 = root_dir2_sub.join("file2.txt"); + assert!(root_dir2.exists()); + assert!(root_dir2_sub.exists()); + assert!(root_dir2_file1.exists()); + assert!(root_dir2_sub_file2.exists()); + if let Some(pb) = pb { + assert_eq!(pb.position(), dir_size); + } + } + } + + #[test] + fn copy_file_test() { + for with_progress_bar in [false, true] { + let temp_dir = tempdir().unwrap(); + let temp_dir_path = temp_dir.path(); + + let file1_path = temp_dir_path.join("file"); + let content = "content"; + fs_extra::file::write_all(&file1_path, content).unwrap(); + assert!(file1_path.exists()); + let path_to = temp_dir_path.join("file_out"); + let pb = if with_progress_bar { + Some( + ProgressBar::new(7).with_style( + ProgressStyle::with_template( + "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", + ) + .unwrap(), + ), + ) + } else { + None + }; + + let result = copy_file(&file1_path, &path_to, pb.as_ref(), 0).expect("move failed"); + + assert_eq!(7, result); + assert!(path_to.exists()); + if let Some(pb) = pb { + assert_eq!(pb.position(), 7); + } + } + } +} diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index 8ed6689d4cf..bca4659ce57 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -1628,8 +1628,9 @@ fn test_acl() { mod inter_partition_copying { use crate::common::util::TestScenario; use std::fs::{read_to_string, set_permissions, write}; - use std::os::unix::fs::{symlink, PermissionsExt}; + use std::os::unix::fs::{symlink, FileTypeExt, PermissionsExt}; use tempfile::TempDir; + use uucore::display::Quotable; // Ensure that the copying code used in an inter-partition move preserve dir structure. #[test] fn test_inter_partition_copying_folder() { @@ -1751,6 +1752,196 @@ mod inter_partition_copying { .stderr_contains("inter-device move failed:") .stderr_contains("Permission denied"); } + + #[cfg(unix)] + #[test] + fn test_inter_partition_copying_folder_with_fifo() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir_all("a/b/c"); + at.write("a/b/d", "d"); + at.write("a/b/c/e", "e"); + at.mkfifo("a/b/c/f"); + + // create a folder in another partition. + let other_fs_tempdir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory"); + + // mv to other partition + scene + .ucmd() + .arg("a") + .arg(other_fs_tempdir.path().to_str().unwrap()) + .succeeds(); + + // make sure that a got removed. + assert!(!at.dir_exists("a")); + + // Ensure that the folder structure is preserved, files are copied, and their contents remain intact. + assert_eq!( + read_to_string(other_fs_tempdir.path().join("a/b/d"),) + .expect("Unable to read other_fs_file"), + "d" + ); + assert_eq!( + read_to_string(other_fs_tempdir.path().join("a/b/c/e"),) + .expect("Unable to read other_fs_file"), + "e" + ); + assert!(other_fs_tempdir + .path() + .join("a/b/c/f") + .metadata() + .expect("") + .file_type() + .is_fifo()) + } + + #[test] + fn test_inter_partition_copying_folder_with_verbose() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir_all("a/b/c"); + at.write("a/b/d", "d"); + at.write("a/b/c/e", "e"); + + // create a folder in another partition. + let other_fs_tempdir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory"); + + // mv to other partition + scene + .ucmd() + .arg("-v") + .arg("a") + .arg(other_fs_tempdir.path().to_str().unwrap()) + .succeeds() + .stdout_contains(format!( + "created directory {}", + other_fs_tempdir.path().join("a").to_string_lossy().quote() + )) + .stdout_contains(format!( + "created directory {}", + other_fs_tempdir + .path() + .join("a/b/c") + .to_string_lossy() + .quote() + )) + .stdout_contains(format!( + "copied 'a/b/d' -> {}", + other_fs_tempdir + .path() + .join("a/b/d") + .to_string_lossy() + .quote() + )) + .stdout_contains("removed 'a/b/c/e'") + .stdout_contains("removed directory 'a/b'"); + } + #[test] + fn test_inter_partition_copying_file_with_verbose() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.write("a", "file_contents"); + + // create a folder in another partition. + let other_fs_tempdir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory"); + + // mv to other partition + scene + .ucmd() + .arg("-v") + .arg("a") + .arg(other_fs_tempdir.path().to_str().unwrap()) + .succeeds() + .stdout_contains(format!( + "copied 'a' -> {}", + other_fs_tempdir.path().join("a").to_string_lossy().quote() + )) + .stdout_contains("removed 'a'"); + + at.write("a", "file_contents"); + + // mv to other partition + scene + .ucmd() + .arg("-vb") + .arg("a") + .arg(other_fs_tempdir.path().to_str().unwrap()) + .succeeds() + .stdout_contains(format!( + "copied 'a' -> {} (backup: {})", + other_fs_tempdir.path().join("a").to_string_lossy().quote(), + other_fs_tempdir.path().join("a~").to_string_lossy().quote() + )) + .stdout_contains("removed 'a'"); + } + + #[test] + fn test_inter_partition_copying_folder_without_read_permission_to_subfolders() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir_all("a/b/c"); + at.write("a/b/d", "d"); + at.write("a/b/c/e", "e"); + at.mkdir_all("a/b/f"); + at.write("a/b/f/g", "g"); + at.write("a/b/h", "h"); + at.set_mode("a/b/f", 0); + // create a folder in another partition. + let other_fs_tempdir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory"); + + // mv to other partition + scene + .ucmd() + .arg("-v") + .arg("a") + .arg(other_fs_tempdir.path().to_str().unwrap()) + .fails() + // check erorr occured + .stderr_contains(format!("cannot access 'a/b/f': Permission denied")); + + // make sure mv kept on going after error + assert_eq!( + read_to_string(other_fs_tempdir.path().join("a/b/h"),) + .expect("Unable to read other_fs_file"), + "h" + ); + + // make sure mv didn't remove src because an error occured + at.dir_exists("a"); + at.file_exists("a/b/h"); + } + + #[test] + fn test_inter_partition_copying_folder_without_write_permission_to_subfolders() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir_all("a/b/c"); + at.write("a/b/d", "d"); + at.write("a/b/c/e", "e"); + at.mkdir_all("a/b/f"); + at.write("a/b/f/g", "g"); + at.write("a/b/h", "h"); + at.set_mode("a/b/f",0o555 ); + // create a folder in another partition. + let other_fs_tempdir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory"); + + // mv to other partition + scene + .ucmd() + .arg("-v") + .arg("a") + .arg(other_fs_tempdir.path().to_str().unwrap()) + .fails() + // check erorr occured + .stderr_contains(format!("mv: cannot remove 'a': Permission denied")); + + } } #[cfg(unix)] From 2b7448e8dcde0bd7041c098673f9b7ae50a4c38f Mon Sep 17 00:00:00 2001 From: mhead Date: Wed, 25 Sep 2024 19:01:20 +0530 Subject: [PATCH 9/9] mv: minor changes and unit tests --- src/uu/mv/src/error.rs | 15 +- src/uu/mv/src/mv.rs | 522 +++++++++++++++++---------------------- tests/by-util/test_mv.rs | 9 +- 3 files changed, 241 insertions(+), 305 deletions(-) diff --git a/src/uu/mv/src/error.rs b/src/uu/mv/src/error.rs index cfc00a8e489..9dc2974fdb7 100644 --- a/src/uu/mv/src/error.rs +++ b/src/uu/mv/src/error.rs @@ -22,7 +22,7 @@ pub enum MvError { TargetNotADirectory(String), FailedToAccessNotADirectory(String), FsXError(FsXError), - NotAllFilesMoved + NotAllFilesMoved, } impl Error for MvError {} @@ -52,20 +52,19 @@ impl Display for MvError { Self::FailedToAccessNotADirectory(t) => { write!(f, "failed to access {t}: Not a directory") - }, - Self::FsXError(err)=>{ + } + Self::FsXError(err) => { write!(f, "{err}") - }, + } Self::NotAllFilesMoved => { write!(f, "failed to move all files") - }, - + } } } } -impl From for MvError{ +impl From for MvError { fn from(err: FsXError) -> Self { - MvError::FsXError(err) + Self::FsXError(err) } } diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 9749bc96b7c..d771d884db5 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -40,8 +40,8 @@ pub use uucore::{backup_control::BackupMode, update_control::UpdateMode}; use uucore::{format_usage, help_about, help_section, help_usage, prompt_yes, show}; use fs_extra::{ - dir::{create_all, get_size as dir_get_size, remove}, - error::Result as FsXResult, + dir::{get_size as dir_get_size, remove}, + error::{ErrorKind as FsXErrorKind, Result as FsXResult}, file::{self, CopyOptions}, }; @@ -110,10 +110,19 @@ struct VerboseContext<'a> { } impl<'a> VerboseContext<'a> { - fn new(backup: Option<&'a Path>, pb: Option<&'a MultiProgress>) -> VerboseContext<'a> { + fn new(backup: Option<&'a Path>, pb: Option<&'a MultiProgress>) -> Self { VerboseContext { backup, pb } } + fn hide_pb_and_print(&self, msg: &str) { + match self.pb { + Some(pb) => pb.suspend(|| { + println!("{msg}"); + }), + None => println!("{msg}"), + }; + } + fn print_move_file(&self, from: &Path, to: &Path) { let message = match self.backup.as_ref() { Some(path) => format!( @@ -124,13 +133,7 @@ impl<'a> VerboseContext<'a> { ), None => format!("renamed {} -> {}", from.quote(), to.quote()), }; - - match self.pb { - Some(pb) => pb.suspend(|| { - println!("{message}"); - }), - None => println!("{message}"), - }; + self.hide_pb_and_print(&message); } fn print_copy_file(&self, from: &Path, to: &Path, with_backup_message: bool) { @@ -143,28 +146,27 @@ impl<'a> VerboseContext<'a> { ), _ => format!("copied {} -> {}", from.quote(), to.quote()), }; - - match self.pb { - Some(pb) => pb.suspend(|| { - println!("{message}"); - }), - None => println!("{message}"), - }; + self.hide_pb_and_print(&message); } fn create_folder(&self, path: &Path) { - println!( + let message = format!( "created directory {}", - path.to_string_lossy().trim_end_matches("/").quote() + path.to_string_lossy() + .trim_end_matches(MAIN_SEPARATOR) + .quote() ); + self.hide_pb_and_print(&message); } fn remove_file(&self, from: &Path) { - println!("removed {}", from.quote()) + let message = format!("removed {}", from.quote()); + self.hide_pb_and_print(&message); } fn remove_folder(&self, from: &Path) { - println!("removed directory {}", from.quote()) + let message = format!("removed directory {}", from.quote()); + self.hide_pb_and_print(&message); } } const ABOUT: &str = help_about!("mv.md"); @@ -672,9 +674,7 @@ fn rename( None }; - rename_with_fallback(from, to, multi_progress, verbose_context)?; - - Ok(()) + rename_with_fallback(from, to, multi_progress, verbose_context.as_ref()) } /// A wrapper around `fs::rename`, so that if it fails, we try falling back on @@ -683,7 +683,7 @@ fn rename_with_fallback( from: &Path, to: &Path, multi_progress: Option<&MultiProgress>, - vebose_context: Option>, + vebose_context: Option<&VerboseContext<'_>>, ) -> io::Result<()> { if fs::rename(from, to).is_err() { // Get metadata without following symlinks @@ -735,15 +735,8 @@ fn rename_with_fallback( if let Err(err) = result { return match err { - MvError::FsXError(fs_extra::error::Error { - kind: fs_extra::error::ErrorKind::PermissionDenied, - .. - }) => Err(io::Error::new( - io::ErrorKind::PermissionDenied, - "Permission denied", - )), MvError::NotAllFilesMoved => { - Err(io::Error::new(io::ErrorKind::Other, format!(""))) + Err(io::Error::new(io::ErrorKind::Other, String::new())) } _ => Err(io::Error::new(io::ErrorKind::Other, format!("{err:?}"))), }; @@ -765,41 +758,26 @@ fn rename_with_fallback( #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] fs::copy(from, to) .map(|v| { - if let Some(vc) = vebose_context.as_ref() { - vc.print_copy_file(from, to, true); - } - v - }) - .and_then(|_| fsxattr::copy_xattrs(&from, &to)) - .and_then(|_| { - let res = fs::remove_file(from); if let Some(vc) = vebose_context { - if res.is_ok() { - vc.remove_file(from); - } - } - res - })?; - #[cfg(any(target_os = "macos", target_os = "redox", not(unix)))] - fs::copy(from, to) - .map(|v| { - if let Some(vc) = vebose_context.as_ref() { vc.print_copy_file(from, to, true); } v }) - .and_then(|_| { - let res = fs::remove_file(from); - if let Some(vc) = vebose_context { - vc.remove_file(from); - } - res - })?; - } - } else { - if let Some(vb) = vebose_context { - vb.print_move_file(from, to); + .and_then(|_| fsxattr::copy_xattrs(&from, &to))?; + #[cfg(any(target_os = "macos", target_os = "redox", not(unix)))] + fs::copy(from, to).map(|v| { + if let Some(vc) = vebose_context { + vc.print_copy_file(from, to, true); + } + v + })?; + fs::remove_file(from)?; + if let Some(vc) = vebose_context { + vc.remove_file(from); + } } + } else if let Some(vb) = vebose_context { + vb.print_move_file(from, to); } Ok(()) } @@ -855,7 +833,7 @@ fn move_dir( from: &Path, to: &Path, progress_bar: Option<&ProgressBar>, - vebose_context: Option>, + verbose_context: Option<&VerboseContext<'_>>, ) -> MvResult { // The return value that represents the number of bytes copied. let mut result: u64 = 0; @@ -867,44 +845,45 @@ fn move_dir( let file_type = dir_entry.file_type(); let dir_entry_path = dir_entry.into_path(); //comment why this is okay - let tmp_to = dir_entry_path.strip_prefix(&from).unwrap(); + let tmp_to = dir_entry_path.strip_prefix(from).unwrap(); let dir_entry_to = to.join(tmp_to); if file_type.is_dir() { // check this create all align with gnu - let res = create_all(&dir_entry_to, false); - if res.is_err() { + let res = fs_extra::dir::create(&dir_entry_to, false); + if let Err(err) = res { + if let FsXErrorKind::NotFound = err.kind { + return Err(err.into()); + } error_occurred = true; - show_error!("{:?}", res.unwrap_err()); + show_error!("{:?}", err); continue; } - if let Some(vc) = vebose_context.as_ref() { - vc.create_folder(&dir_entry_to) + if let Some(vc) = verbose_context { + vc.create_folder(&dir_entry_to); } } else { - let result_file_copy = - copy_file(&dir_entry_path, &dir_entry_to, progress_bar, result); - - match result_file_copy { - Ok(result_file_copy) => { - result += result_file_copy; + let res = copy_file(&dir_entry_path, &dir_entry_to, progress_bar, result); + match res { + Ok(copied_bytes) => { + result += copied_bytes; + if let Some(vc) = verbose_context { + vc.print_copy_file(&dir_entry_path, &dir_entry_to, false); + } } Err(err) => { let err_msg = match err.kind { - fs_extra::error::ErrorKind::Io(error) => { + FsXErrorKind::Io(error) => { format!("error writing {}: {}", dir_entry_to.quote(), error) } _ => { format!("{:?}", err) } }; - error_occurred = true; show_error!("{}", err_msg); + error_occurred = true; continue; } } - if let Some(vc) = vebose_context.as_ref() { - vc.print_copy_file(&dir_entry_path, &dir_entry_to, false); - } } moved_entries.push((dir_entry_path, file_type)); } @@ -921,8 +900,7 @@ fn move_dir( } } if !error_occurred { - while !moved_entries.is_empty() { - let (moved_path, file_type) = moved_entries.pop().unwrap(); + while let Some((moved_path, file_type)) = moved_entries.pop() { let res = if file_type.is_dir() { remove(&moved_path) } else { @@ -931,13 +909,11 @@ fn move_dir( if let Err(err) = res { error_occurred = true; show_error!("cannot remove {}: {}", moved_path.quote(), err); - } else { - if let Some(vc) = vebose_context.as_ref() { - if file_type.is_dir() { - vc.remove_folder(&moved_path); - } else { - vc.remove_file(&moved_path); - } + } else if let Some(vc) = verbose_context { + if file_type.is_dir() { + vc.remove_folder(&moved_path); + } else { + vc.remove_file(&moved_path); } } } @@ -994,221 +970,183 @@ fn copy_file( #[cfg(test)] mod tests { - use std::path::PathBuf; - extern crate fs_extra; - use super::{copy_file, move_dir}; - use fs_extra::dir::*; - use indicatif::{ProgressBar, ProgressStyle}; + use crate::copy_file; + use crate::error::MvError; + + use super::move_dir; + use super::MvResult; + use fs_extra::dir::get_size; + use indicatif::ProgressBar; + use std::fs; + use std::fs::metadata; + use std::fs::set_permissions; + use std::fs::{create_dir_all, File}; + use std::io::Write; + use std::os::unix::fs::FileTypeExt; use tempfile::tempdir; + use uucore::fs::copy_fifo; - // These tests are copied from the `fs_extra`'s repository #[test] - fn it_move_work() { - for with_progress_bar in [false, true] { - let temp_dir = tempdir().unwrap(); - let mut path_from = PathBuf::from(temp_dir.path()); - let test_name = "sub"; - path_from.push("it_move_work"); - let mut path_to = path_from.clone(); - path_to.push("out"); - path_from.push(test_name); - - create_all(&path_from, true).unwrap(); - assert!(path_from.exists()); - create_all(&path_to, true).unwrap(); - assert!(path_to.exists()); - - let mut file1_path = path_from.clone(); - file1_path.push("test1.txt"); - let content1 = "content1"; - fs_extra::file::write_all(&file1_path, content1).unwrap(); - assert!(file1_path.exists()); - - let mut sub_dir_path = path_from.clone(); - sub_dir_path.push("sub"); - create(&sub_dir_path, true).unwrap(); - let mut file2_path = sub_dir_path.clone(); - file2_path.push("test2.txt"); - let content2 = "content2"; - fs_extra::file::write_all(&file2_path, content2).unwrap(); - assert!(file2_path.exists()); - - let pb = if with_progress_bar { - Some( - ProgressBar::new(16).with_style( - ProgressStyle::with_template( - "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", - ) - .unwrap(), - ), - ) - } else { - None - }; - - let result = move_dir(&path_from, &path_to, pb.as_ref(), None).unwrap(); - - assert_eq!(16, result); - assert!(path_to.exists()); - assert!(!path_from.exists()); - if let Some(pb) = pb { - assert_eq!(pb.position(), 16); - } - } + fn move_all_files_and_directories() { + let tempdir = tempdir().expect("couldn't create tempdir"); + let tempdir_path = tempdir.path(); + let mut from = tempdir_path.to_path_buf(); + from.push("test_src"); + let mut to = tempdir_path.to_path_buf(); + to.push("test_dest"); + + // Setup source directory with files and subdirectories + create_dir_all(from.join("subdir")).expect("couldn't create subdir"); + let mut file = File::create(from.join("file1.txt")).expect("couldn't create file1.txt"); + writeln!(file, "Hello, world!").expect("couldn't write to file1.txt"); + let mut file = + File::create(from.join("subdir/file2.txt")).expect("couldn't create subdir/file2.txt"); + writeln!(file, "Hello, subdir!").expect("couldn't write to subdir/file2.txt"); + + // Call the function + let result: MvResult = move_dir(&from, &to, None, None); + + // Assert the result + assert!(result.is_ok()); + assert!(to.join("file1.txt").exists()); + assert!(to.join("subdir/file2.txt").exists()); + assert!(!from.join("file1.txt").exists()); + assert!(!from.join("subdir/file2.txt").exists()); + assert!(!from.exists()); } #[test] - fn it_move_exist_overwrite() { - for with_progress_bar in [false, true] { - let temp_dir = tempdir().unwrap(); - let mut path_from = PathBuf::from(temp_dir.path()); - let test_name = "sub"; - path_from.push("it_move_exist_overwrite"); - let mut path_to = path_from.clone(); - path_to.push("out"); - path_from.push(test_name); - let same_file = "test.txt"; - - create_all(&path_from, true).unwrap(); - assert!(path_from.exists()); - create_all(&path_to, true).unwrap(); - assert!(path_to.exists()); - - let mut file1_path = path_from.clone(); - file1_path.push(same_file); - let content1 = "content1"; - fs_extra::file::write_all(&file1_path, content1).unwrap(); - assert!(file1_path.exists()); - - let mut sub_dir_path = path_from.clone(); - sub_dir_path.push("sub"); - create(&sub_dir_path, true).unwrap(); - let mut file2_path = sub_dir_path.clone(); - file2_path.push("test2.txt"); - let content2 = "content2"; - fs_extra::file::write_all(&file2_path, content2).unwrap(); - assert!(file2_path.exists()); - - let mut exist_path = path_to.clone(); - exist_path.push(test_name); - create(&exist_path, true).unwrap(); - assert!(exist_path.exists()); - exist_path.push(same_file); - let exist_content = "exist content"; - assert_ne!(exist_content, content1); - fs_extra::file::write_all(&exist_path, exist_content).unwrap(); - assert!(exist_path.exists()); - - let dir_size = get_size(&path_from).expect("failed to get dir size"); - let pb = if with_progress_bar { - Some( - ProgressBar::new(dir_size).with_style( - ProgressStyle::with_template( - "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", - ) - .unwrap(), - ), - ) - } else { - None - }; - move_dir(&path_from, &path_to, pb.as_ref(), None).unwrap(); - assert!(exist_path.exists()); - assert!(path_to.exists()); - assert!(!path_from.exists()); - if let Some(pb) = pb { - assert_eq!(pb.position(), dir_size); - } - } + fn move_dir_tracks_progress() { + // Create a temporary directory for testing + let tempdir = tempdir().expect("couldn't create tempdir"); + let tempdir_path = tempdir.path(); + let mut from = tempdir_path.to_path_buf(); + from.push("test_src"); + let mut to = tempdir_path.to_path_buf(); + to.push("test_dest"); + + // Setup source directory with files and subdirectories + create_dir_all(from.join("subdir")).expect("couldn't create subdir"); + let mut file = File::create(from.join("file1.txt")).expect("couldn't create file1.txt"); + writeln!(file, "Hello, world!").expect("couldn't write to file1.txt"); + let mut file = + File::create(from.join("subdir/file2.txt")).expect("couldn't create subdir/file2.txt"); + writeln!(file, "Hello, subdir!").expect("couldn't write to subdir/file2.txt"); + + let len = get_size(&from).expect("couldn't get the size of source dir"); + let pb = ProgressBar::new(len); + + // Call the function + let result: MvResult = move_dir(&from, &to, Some(&pb), None); + + // Assert the result + assert!(result.is_ok()); + assert!(to.join("file1.txt").exists()); + assert!(to.join("subdir/file2.txt").exists()); + assert!(!from.join("file1.txt").exists()); + assert!(!from.join("subdir/file2.txt").exists()); + assert!(!from.exists()); + assert_eq!(pb.position(), len) } #[test] - fn it_move_inside_work_target_dir_not_exist() { - for with_progress_bar in [false, true] { - let temp_dir = tempdir().unwrap(); - let path_root = PathBuf::from(temp_dir.path()); - let root = path_root.join("it_move_inside_work_target_dir_not_exist"); - let root_dir1 = root.join("dir1"); - let root_dir1_sub = root_dir1.join("sub"); - let root_dir2 = root.join("dir2"); - let file1 = root_dir1.join("file1.txt"); - let file2 = root_dir1_sub.join("file2.txt"); - - create_all(&root_dir1_sub, true).unwrap(); - fs_extra::file::write_all(&file1, "content1").unwrap(); - fs_extra::file::write_all(&file2, "content2").unwrap(); - - if root_dir2.exists() { - remove(&root_dir2).unwrap(); - } - - assert!(root_dir1.exists()); - assert!(root_dir1_sub.exists()); - assert!(!root_dir2.exists()); - assert!(file1.exists()); - assert!(file2.exists()); - let dir_size = get_size(&root_dir1).expect("failed to get dir size"); - let pb = if with_progress_bar { - Some( - ProgressBar::new(dir_size).with_style( - ProgressStyle::with_template( - "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", - ) - .unwrap(), - ), - ) - } else { - None - }; - - let result = move_dir(&root_dir1, &root_dir2, pb.as_ref(), None).unwrap(); - - assert_eq!(16, result); - assert!(!root_dir1.exists()); - let root_dir2_sub = root_dir2.join("sub"); - let root_dir2_file1 = root_dir2.join("file1.txt"); - let root_dir2_sub_file2 = root_dir2_sub.join("file2.txt"); - assert!(root_dir2.exists()); - assert!(root_dir2_sub.exists()); - assert!(root_dir2_file1.exists()); - assert!(root_dir2_sub_file2.exists()); - if let Some(pb) = pb { - assert_eq!(pb.position(), dir_size); - } - } + fn move_all_files_and_directories_without_src_permission() { + let tempdir = tempdir().expect("couldn't create tempdir"); + let tempdir_path = tempdir.path(); + let mut from = tempdir_path.to_path_buf(); + from.push("test_src"); + let mut to = tempdir_path.to_path_buf(); + to.push("test_dest"); + + // Setup source directory with files and subdirectories + create_dir_all(from.join("subdir")).expect("couldn't create subdir"); + + let mut file = File::create(from.join("file1.txt")).expect("couldn't create file1.txt"); + writeln!(file, "Hello, world!").expect("couldn't write to file1.txt"); + let mut file = + File::create(from.join("subdir/file2.txt")).expect("couldn't create subdir/file2.txt"); + writeln!(file, "Hello, subdir!").expect("couldn't write to subdir/file2.txt"); + + let metadata = metadata(&from).expect("failed to get metadata"); + let mut permissions = metadata.permissions(); + std::os::unix::fs::PermissionsExt::set_mode(&mut permissions, 0o222); + set_permissions(&from, permissions).expect("failed to set permissions"); + + // Call the function + let result: MvResult = move_dir(&from, &to, None, None); + assert!(matches!(result, Err(MvError::NotAllFilesMoved))); + assert!(from.exists()); } #[test] - fn copy_file_test() { - for with_progress_bar in [false, true] { - let temp_dir = tempdir().unwrap(); - let temp_dir_path = temp_dir.path(); - - let file1_path = temp_dir_path.join("file"); - let content = "content"; - fs_extra::file::write_all(&file1_path, content).unwrap(); - assert!(file1_path.exists()); - let path_to = temp_dir_path.join("file_out"); - let pb = if with_progress_bar { - Some( - ProgressBar::new(7).with_style( - ProgressStyle::with_template( - "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}", - ) - .unwrap(), - ), - ) - } else { - None - }; - - let result = copy_file(&file1_path, &path_to, pb.as_ref(), 0).expect("move failed"); + fn test_copy_file() { + let temp_dir = tempdir().expect("couldn't create tempdir"); + let from = temp_dir.path().join("test_source.txt"); + let to = temp_dir.path().join("test_destination.txt"); + + // Create a test source file + let mut file = File::create(&from).expect("couldn't create file1.txt"); + write!(file, "Hello, world!").expect("couldn't write to file1.txt"); + + // Call the function + let result = copy_file(&from, &to, None, 0); + + // Assert the result is Ok and the file was copied + assert!(result.is_ok()); + assert!(to.exists()); + assert_eq!( + fs::read_to_string(to).expect("couldn't read from to"), + "Hello, world!" + ); + } + #[test] + fn test_copy_file_with_progress() { + let temp_dir = tempdir().expect("couldn't create tempdir"); + let from = temp_dir.path().join("test_source.txt"); + let to = temp_dir.path().join("test_destination.txt"); + + // Create a test source file + let mut file = File::create(&from).expect("couldn't create file1.txt"); + write!(file, "Hello, world!").expect("couldn't write to file1.txt"); + + let len = file + .metadata() + .expect("couldn't get source file metadata") + .len(); + let pb = ProgressBar::new(len); + + // Call the function + let result = copy_file(&from, &to, Some(&pb), 0); + + // Assert the result is Ok and the file was copied + assert_eq!(pb.position(), len); + assert!(result.is_ok()); + assert!(to.exists()); + assert_eq!( + fs::read_to_string(to).expect("couldn't read from to"), + "Hello, world!" + ); + } - assert_eq!(7, result); - assert!(path_to.exists()); - if let Some(pb) = pb { - assert_eq!(pb.position(), 7); - } - } + #[test] + fn test_copy_file_with_fifo() { + let temp_dir = tempdir().expect("couldn't create tempdir"); + let from = temp_dir.path().join("test_source.txt"); + let to = temp_dir.path().join("test_destination.txt"); + + // Create a test source file + copy_fifo(&from).expect("couldn't create fifo"); + + // Call the function + let result = copy_file(&from, &to, None, 0); + + // Assert the result is Ok and the fifo was copied + assert!(result.is_ok()); + assert!(to.exists()); + assert!(to + .metadata() + .expect("couldn't get metadata") + .file_type() + .is_fifo()) } } diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index bca4659ce57..41b1aa641bf 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -1794,7 +1794,7 @@ mod inter_partition_copying { .metadata() .expect("") .file_type() - .is_fifo()) + .is_fifo()); } #[test] @@ -1902,7 +1902,7 @@ mod inter_partition_copying { .arg(other_fs_tempdir.path().to_str().unwrap()) .fails() // check erorr occured - .stderr_contains(format!("cannot access 'a/b/f': Permission denied")); + .stderr_contains("cannot access 'a/b/f': Permission denied"); // make sure mv kept on going after error assert_eq!( @@ -1926,7 +1926,7 @@ mod inter_partition_copying { at.mkdir_all("a/b/f"); at.write("a/b/f/g", "g"); at.write("a/b/h", "h"); - at.set_mode("a/b/f",0o555 ); + at.set_mode("a/b/f", 0o555); // create a folder in another partition. let other_fs_tempdir = TempDir::new_in("/dev/shm/").expect("Unable to create temp directory"); @@ -1939,8 +1939,7 @@ mod inter_partition_copying { .arg(other_fs_tempdir.path().to_str().unwrap()) .fails() // check erorr occured - .stderr_contains(format!("mv: cannot remove 'a': Permission denied")); - + .stderr_contains("mv: cannot remove 'a': Permission denied"); } }