Skip to content

Commit

Permalink
mv: linting errors and minor changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@matrixhead
matrixhead committed Oct 8, 2024
1 parent ed4bd99 commit ef3a5eb
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 65 deletions.
2 changes: 1 addition & 1 deletion src/uu/mv/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ uucore = { workspace = true, features = [
"fsxattr",
"update-control",
"perms",
"entries"
"entries",
] }

[dev-dependencies]
Expand Down
105 changes: 67 additions & 38 deletions src/uu/mv/src/mv.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@
// 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;

use clap::builder::ValueParser;
use clap::{crate_version, error::ErrorKind, Arg, ArgAction, ArgMatches, Command};
use filetime::set_symlink_file_times;
use filetime::{set_file_times, set_symlink_file_times};
use indicatif::{MultiProgress, ProgressBar, ProgressStyle};
use std::collections::HashSet;
use std::env;
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -733,7 +735,6 @@ fn rename_with_fallback(
}
}

Check warning on line 736 in src/uu/mv/src/mv.rs

View check run for this annotation

Codecov / codecov/patch

src/uu/mv/src/mv.rs#L736 

Added line #L736 was not covered by tests

#[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 {
Expand Down Expand Up @@ -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<usize> = 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() {
Expand Down Expand Up @@ -976,37 +989,37 @@ 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);

Check warning on line 999 in src/uu/mv/src/mv.rs

View check run for this annotation

Codecov / codecov/patch

src/uu/mv/src/mv.rs#L999 

Added line #L999 was not covered by tests
}
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, &copy_options, progress_handler)
} else {
file::copy(from, to, &copy_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
let modified_time = src_metadata.modified()?;
let accessed_time = src_metadata.accessed()?;
set_symlink_file_times(dest, accessed_time.into(), modified_time.into())?;

// Copy ownership (if on Unix-like system)
#[cfg(unix)]
{
Expand All @@ -1026,29 +1039,35 @@ fn copy_metadata(src: &Path, dest: &Path, src_metadata: &fs::Metadata) -> io::Re
.map_err(|err| io::Error::new(io::ErrorKind::Other, err))?;
}

// Copy the modified and accessed timestamps
let modified_time = src_metadata.modified()?;
let accessed_time = src_metadata.accessed()?;
if dest_metadata.is_symlink() {
set_symlink_file_times(dest, accessed_time.into(), modified_time.into())?;
} else {
set_file_times(dest, accessed_time.into(), modified_time.into())?;
}

Ok(())
}

#[cfg(test)]
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() {
Expand Down Expand Up @@ -1091,13 +1110,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
Expand All @@ -1113,6 +1138,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");
Expand All @@ -1131,10 +1157,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<u64> = move_dir(&from, &to, None, None);
Expand Down Expand Up @@ -1192,6 +1218,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");
Expand All @@ -1214,6 +1241,7 @@ mod tests {
.is_fifo())
}

#[cfg(unix)]
#[test]
fn test_copy_metadata_copies_permissions() {
let temp_dir = tempdir().unwrap();
Expand All @@ -1240,7 +1268,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");
Expand All @@ -1258,7 +1286,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();
Expand All @@ -1270,13 +1298,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");
Expand All @@ -1290,7 +1319,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());
Expand Down
12 changes: 6 additions & 6 deletions src/uucore/src/lib/features/fs.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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<PathBuf> = 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()
);
}
}
Loading

0 comments on commit ef3a5eb

Please sign in to comment.