From 9a894efa6e4239075a62b89afe1f629ecc8dd9d2 Mon Sep 17 00:00:00 2001 From: mhead Date: Tue, 8 Oct 2024 17:47:14 +0530 Subject: [PATCH] mv: linting errors and minor changes --- src/uu/mv/Cargo.toml | 2 +- src/uu/mv/src/mv.rs | 93 ++++++++++++++++++++----------- src/uucore/src/lib/features/fs.rs | 12 ++-- tests/by-util/test_mv.rs | 40 ++++++------- 4 files changed, 86 insertions(+), 61 deletions(-) diff --git a/src/uu/mv/Cargo.toml b/src/uu/mv/Cargo.toml index 3e0262e4bb..cf3051326f 100644 --- a/src/uu/mv/Cargo.toml +++ b/src/uu/mv/Cargo.toml @@ -28,7 +28,7 @@ uucore = { workspace = true, features = [ "fsxattr", "update-control", "perms", - "entries" + "entries", ] } [dev-dependencies] diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 6e4aa00102..9c528815b3 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (ToDO) sourcepath targetpath nushell canonicalized +// spell-checker:ignore (ToDO) sourcepath targetpath nushell canonicalized lred mod error; @@ -32,6 +32,7 @@ use uucore::fs::{ }; #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] use uucore::fsxattr; +#[cfg(unix)] use uucore::perms::{wrap_chown, Verbosity, VerbosityLevel}; use uucore::{show_error, update_control}; use walkdir::WalkDir; @@ -706,6 +707,7 @@ fn rename_with_fallback( } } else if !matches!(err.raw_os_error(),Some(err_code)if err_code == CROSSES_DEVICES_ERROR_CODE) { + // only try to copy if os reports an crosses devices error. return Err(err); } else if file_type.is_dir() { // We remove the destination directory if it exists to match the @@ -733,7 +735,6 @@ fn rename_with_fallback( } } - #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] let result = move_dir(from, to, progress_bar.as_ref(), verbose_context); if let Err(err) = result { @@ -913,14 +914,26 @@ fn move_dir( } } } + // if no error occurred try to remove source and copy metadata if !error_occurred { + // GNU's `mv` only reports an error when it fails to remove a directory + // entry. It doesn't print anything if it fails to remove the parent + // directory of that entry. + // in order to mimic that behavior, we need to remember where the last error occurred. let mut last_rem_err_depth: Option = None; while let Some((src_path, file_type, dest_path, src_md, depth)) = moved_entries.pop() { if let Some(src_metadata) = src_md { copy_metadata(&src_path, &dest_path, &src_metadata).ok(); } if matches!(last_rem_err_depth,Some(lred)if lred > depth) { + // This means current dir entry is parent directory of a child + // dir entry that couldn't be removed. + + // We mark current depth as the depth last error was occurred, this + // would ensure that we won't ignore sibling dir entries of the + // parent directory. last_rem_err_depth = Some(depth); + // there's no point trying to remove a non empty directory. continue; } let res = if src_path.is_dir() { @@ -976,30 +989,33 @@ fn copy_file( None }; - let md = from.metadata()?; - if cfg!(unix) && FileTypeExt::is_fifo(&md.file_type()) { - let file_size = md.len(); - uucore::fs::create_fifo(to)?; - if let Some(progress_bar) = progress_bar { - progress_bar.set_position(file_size + progress_bar_start_val); + #[cfg(all(unix, not(target_os = "redox")))] + { + let md = from.metadata()?; + if FileTypeExt::is_fifo(&md.file_type()) { + let file_size = md.len(); + uucore::fs::create_fifo(to)?; + if let Some(progress_bar) = progress_bar { + progress_bar.set_position(file_size + progress_bar_start_val); + } + return Ok(file_size); } - Ok(file_size) - } else if let Some(progress_handler) = progress_handler { + } + if let Some(progress_handler) = progress_handler { file::copy_with_progress(from, to, ©_options, progress_handler) } else { file::copy(from, to, ©_options) } } +#[allow(unused_variables)] fn copy_metadata(src: &Path, dest: &Path, src_metadata: &fs::Metadata) -> io::Result<()> { - // Get metadata of the dest file - let dest_metadata = fs::symlink_metadata(dest)?; - // Copy file permissions let permissions = src_metadata.permissions(); fs::set_permissions(dest, permissions)?; // Copy xattrs + #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] fsxattr::copy_xattrs(src, dest)?; // Copy the modified and accessed timestamps @@ -1010,6 +1026,8 @@ fn copy_metadata(src: &Path, dest: &Path, src_metadata: &fs::Metadata) -> io::Re // Copy ownership (if on Unix-like system) #[cfg(unix)] { + // Get metadata of the dest file + let dest_metadata = fs::symlink_metadata(dest)?; let uid = MetadataExt::uid(src_metadata); let gid = MetadataExt::gid(src_metadata); wrap_chown( @@ -1033,22 +1051,19 @@ fn copy_metadata(src: &Path, dest: &Path, src_metadata: &fs::Metadata) -> io::Re mod tests { use super::*; use crate::copy_file; - use crate::error::MvError; - use fs_extra::dir::get_size; + #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] use fsxattr::{apply_xattrs, retrieve_xattrs}; use indicatif::ProgressBar; - use std::collections::HashMap; - use std::fs; - use std::fs::metadata; - use std::fs::set_permissions; - use std::fs::{create_dir_all, File}; + use std::fs::{self, create_dir_all, File}; use std::io::Write; - use std::os::unix::fs::FileTypeExt; - use std::os::unix::fs::PermissionsExt; + #[cfg(unix)] + use std::os::unix::fs::{FileTypeExt, PermissionsExt}; use std::thread::sleep; use std::time::Duration; use tempfile::tempdir; + #[cfg(unix)] use uucore::fs::create_fifo; + use uucore::fs::disk_usage; #[test] fn move_all_files_and_directories() { @@ -1091,13 +1106,19 @@ mod tests { // 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 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"); + file.sync_all().unwrap(); + } + { + 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"); + file.sync_all().unwrap(); + } - let len = get_size(&from).expect("couldn't get the size of source dir"); + let len = disk_usage(&[&from], true).expect("couldn't get the size of source dir"); let pb = ProgressBar::new(len); // Call the function @@ -1113,6 +1134,7 @@ mod tests { assert_eq!(pb.position(), len) } + #[cfg(unix)] #[test] fn move_all_files_and_directories_without_src_permission() { let tempdir = tempdir().expect("couldn't create tempdir"); @@ -1131,10 +1153,10 @@ mod tests { 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 metadata = fs::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"); + fs::set_permissions(&from, permissions).expect("failed to set permissions"); // Call the function let result: MvResult = move_dir(&from, &to, None, None); @@ -1192,6 +1214,7 @@ mod tests { ); } + #[cfg(all(unix, not(target_os = "redox")))] #[test] fn test_copy_file_with_fifo() { let temp_dir = tempdir().expect("couldn't create tempdir"); @@ -1214,6 +1237,7 @@ mod tests { .is_fifo()) } + #[cfg(unix)] #[test] fn test_copy_metadata_copies_permissions() { let temp_dir = tempdir().unwrap(); @@ -1240,7 +1264,7 @@ mod tests { } #[test] - fn test_copy_metadata_copies_filetimes() { + fn test_copy_metadata_copies_file_times() { let temp_dir = tempdir().expect("couldn't create tempdir"); let src_path = temp_dir.path().join("src_file"); let dest_path = temp_dir.path().join("dest_file"); @@ -1258,7 +1282,7 @@ mod tests { .expect("couldn't get modified time for src file"); let accessed_time = src_metadata .accessed() - .expect("couldn't get accesssed time for src file"); + .expect("couldn't get accessed time for src file"); //Try to copy metadata copy_metadata(&src_path, &dest_path, &src_metadata).unwrap(); @@ -1270,13 +1294,14 @@ mod tests { .expect("couldn't get modified time for src file"); let dest_accessed_time = dest_metadata .accessed() - .expect("couldn't get accesssed time for src file"); + .expect("couldn't get accessed time for src file"); // Verify that the file times were copied assert_eq!(modified_time, dest_modified_time); assert_eq!(dest_accessed_time, accessed_time); } + #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] #[test] fn test_copy_metadata_copies_xattr() { let temp_dir = tempdir().expect("couldn't create tempdir"); @@ -1290,7 +1315,7 @@ mod tests { let src_metadata = fs::metadata(&src_path).unwrap(); // Set xattrs for the source file - let mut test_xattrs = HashMap::new(); + let mut test_xattrs = std::collections::HashMap::new(); let test_attr = "user.test_attr"; let test_value = b"test value"; test_xattrs.insert(OsString::from(test_attr), test_value.to_vec()); diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index d08f3864be..7064c7c6f7 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -791,7 +791,7 @@ pub fn get_filename(file: &Path) -> Option<&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)] +#[cfg(all(unix, not(target_os = "redox")))] pub fn create_fifo(dest: &Path) -> std::io::Result<()> { nix::unistd::mkfifo(dest, nix::sys::stat::Mode::S_IRUSR).map_err(|err| err.into()) } @@ -1074,7 +1074,7 @@ mod tests { assert!(matches!(get_filename(&file_path), Some("foo.txt"))); } - #[cfg(unix)] + #[cfg(all(unix, not(target_os = "redox")))] #[test] fn test_create_fifo_success() { let dir = tempdir().unwrap(); @@ -1144,16 +1144,16 @@ mod tests { let subdir_path = dir.path().join("subdir"); fs::create_dir(&subdir_path).unwrap(); - let subfile_path = subdir_path.join("file2.txt"); - let mut subfile = fs::File::create(&subfile_path).unwrap(); - writeln!(subfile, "Hello, again!").unwrap(); + let sub_file_path = subdir_path.join("file2.txt"); + let mut sub_file = fs::File::create(&sub_file_path).unwrap(); + writeln!(sub_file, "Hello, again!").unwrap(); let paths: Vec = vec![dir.path().to_path_buf()]; let total_size = disk_usage(&paths, true).unwrap(); assert_eq!( total_size, - file_path.metadata().unwrap().len() + subfile_path.metadata().unwrap().len() + file_path.metadata().unwrap().len() + sub_file_path.metadata().unwrap().len() ); } } diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index 73d5cc9b8d..78871c31a0 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -1884,7 +1884,7 @@ mod inter_partition_copying { } #[test] - fn test_inter_partition_copying_folder_without_read_permission_to_subfolders() { + fn test_inter_partition_copying_dir_without_read_permission_to_sub_dir() { let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; at.mkdir_all("a/b/c"); @@ -1905,7 +1905,7 @@ mod inter_partition_copying { .arg("a") .arg(other_fs_tempdir.path().to_str().unwrap()) .fails() - // check erorr occured + // check error occurred .stderr_contains("cannot access 'a/b/f': Permission denied"); // make sure mv kept on going after error @@ -1915,7 +1915,7 @@ mod inter_partition_copying { "h" ); - // make sure mv didn't remove src because an error occured + // make sure mv didn't remove src because an error occurred at.dir_exists("a"); at.file_exists("a/b/h"); } @@ -1943,7 +1943,7 @@ mod inter_partition_copying { .expect("couldn't get modified time for src file"); let accessed_time = src_metadata .accessed() - .expect("couldn't get accesssed time for src file"); + .expect("couldn't get accessed time for src file"); // create a folder in another partition. let other_fs_tempdir = TempDir::new_in("/dev/shm/").expect("Unable to create temp directory"); @@ -1991,7 +1991,7 @@ mod inter_partition_copying { } // this test would fail if progress bar flag is set to true because we need - // to traverse directory inorder to find the size that would change the + // to traverse directory in order to find the size that would change the // access time. #[test] fn test_inter_partition_copying_directory_metadata_for_directory() { @@ -2016,7 +2016,7 @@ mod inter_partition_copying { .expect("couldn't get modified time for src file"); let accessed_time = src_metadata .accessed() - .expect("couldn't get accesssed time for src file"); + .expect("couldn't get accessed time for src file"); // create a folder in another partition. let other_fs_tempdir = TempDir::new_in("/dev/shm/").expect("Unable to create temp directory"); @@ -2068,8 +2068,8 @@ mod inter_partition_copying { let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; at.mkdir_all("a/aa/"); - at.write("a/aa/aa1", "filecontents"); - //remove write permssion for the subdir + at.write("a/aa/aa1", "file contents"); + //remove write permission for the subdir at.set_mode("a/aa/", 0o555); // create a folder in another partition. let other_fs_tempdir = @@ -2081,7 +2081,7 @@ mod inter_partition_copying { .arg("a") .arg(other_fs_tempdir.path().to_str().unwrap()) .fails() - // check erorr occured + // check error occurred .stderr_contains("mv: cannot remove 'a/aa/aa1': Permission denied") //make sure mv doesn't print errors for the parent directories .stderr_does_not_contain("'a/aa'"); @@ -2095,9 +2095,9 @@ mod inter_partition_copying { let at = &scene.fixtures; at.mkdir_all("a/aa/"); at.mkdir_all("a/aa/aaa"); - at.write("a/aa/aa1", "filecontents"); - at.write("a/aa/aa2", "filecontents"); - //remove write permssion for the subdir + at.write("a/aa/aa1", "file contents"); + at.write("a/aa/aa2", "file contents"); + //remove write permission for the subdir at.set_mode("a/aa/", 0o555); // create a folder in another partition. let other_fs_tempdir = @@ -2124,9 +2124,9 @@ mod inter_partition_copying { let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; at.mkdir_all("a/aa/"); - at.write("a/a1", "filecontents"); - at.write("a/aa/aa1", "filecontents"); - //remove write permssion for the subdir + at.write("a/a1", "file contents"); + at.write("a/aa/aa1", "file contents"); + //remove write permission for the subdir at.set_mode("a/aa/", 0o555); // create a folder in another partition. let other_fs_tempdir = @@ -2140,7 +2140,7 @@ mod inter_partition_copying { .fails() .stderr_contains("mv: cannot remove 'a/aa/aa1': Permission denied"); assert!(at.file_exists("a/aa/aa1")); - // file that doesn't belong to the branch that error occured didn't got removed + // file that doesn't belong to the branch that error occurred didn't got removed assert!(!at.file_exists("a/a1")); assert!(other_fs_tempdir.path().join("a/aa/aa1").exists()); } @@ -2151,10 +2151,10 @@ mod inter_partition_copying { let at = &scene.fixtures; at.mkdir_all("a/aa/"); at.mkdir_all("a/ab/"); - at.write("a/aa/aa1", "filecontents"); - at.write("a/ab/ab1", "filecontents"); + at.write("a/aa/aa1", "file contents"); + at.write("a/ab/ab1", "file contents"); - //remove write permssion for the subdir + //remove write permission for the subdir at.set_mode("a/aa/", 0o555); // create a folder in another partition. let other_fs_tempdir = @@ -2168,7 +2168,7 @@ mod inter_partition_copying { .fails() .stderr_contains("mv: cannot remove 'a/aa/aa1': Permission denied"); assert!(at.file_exists("a/aa/aa1")); - // folder that doesn't belong to the branch that error occured didn't got removed + // folder that doesn't belong to the branch that error occurred didn't got removed assert!(!at.dir_exists("a/ab")); assert!(other_fs_tempdir.path().join("a/ab/ab1").exists()); }