1
Fork 0
mirror of https://github.com/RGBCube/uutils-coreutils synced 2025-07-28 11:37:44 +00:00

mv: improve move-to-self error handling (#6995)

- improve move-to-self detection, so this errors without data loss:

```diff
 mkdir mydir
 mv mydir mydir/subdir
-mv: No such file or directory (os error 2)
+mv: cannot move 'mydir' to a subdirectory of itself, 'mydir/subdir'
```

- align "cannot move source to a subdirectory of itself" and "same file"
  errors more closely with coreutils:

```diff
 mkdir mydir
 mv mydir/ mydir/..
-mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/../mydir/'
+mv: 'mydir/' and 'mydir/../mydir' are the same file
```

Causing: https://github.com/nushell/nushell/issues/13082
This commit is contained in:
Solomon 2024-12-26 12:48:29 -07:00 committed by GitHub
parent b4cdc36573
commit 98c9be5ec4
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 140 additions and 111 deletions

View file

@ -12,7 +12,6 @@ pub enum MvError {
NoSuchFile(String), NoSuchFile(String),
CannotStatNotADirectory(String), CannotStatNotADirectory(String),
SameFile(String, String), SameFile(String, String),
SelfSubdirectory(String),
SelfTargetSubdirectory(String, String), SelfTargetSubdirectory(String, String),
DirectoryToNonDirectory(String), DirectoryToNonDirectory(String),
NonDirectoryToDirectory(String, String), NonDirectoryToDirectory(String, String),
@ -29,14 +28,9 @@ impl Display for MvError {
Self::NoSuchFile(s) => write!(f, "cannot stat {s}: No such file or directory"), Self::NoSuchFile(s) => write!(f, "cannot stat {s}: No such file or directory"),
Self::CannotStatNotADirectory(s) => write!(f, "cannot stat {s}: Not a directory"), Self::CannotStatNotADirectory(s) => write!(f, "cannot stat {s}: Not a directory"),
Self::SameFile(s, t) => write!(f, "{s} and {t} are the same file"), Self::SameFile(s, t) => write!(f, "{s} and {t} are the same file"),
Self::SelfSubdirectory(s) => write!( Self::SelfTargetSubdirectory(s, t) => {
f, write!(f, "cannot move {s} to a subdirectory of itself, {t}")
"cannot move '{s}' to a subdirectory of itself, '{s}/{s}'" }
),
Self::SelfTargetSubdirectory(s, t) => write!(
f,
"cannot move '{s}' to a subdirectory of itself, '{t}/{s}'"
),
Self::DirectoryToNonDirectory(t) => { Self::DirectoryToNonDirectory(t) => {
write!(f, "cannot overwrite directory {t} with non-directory") write!(f, "cannot overwrite directory {t} with non-directory")
} }

View file

@ -19,13 +19,13 @@ use std::io;
use std::os::unix; use std::os::unix;
#[cfg(windows)] #[cfg(windows)]
use std::os::windows; use std::os::windows;
use std::path::{Path, PathBuf}; use std::path::{absolute, Path, PathBuf};
use uucore::backup_control::{self, source_is_target_backup}; use uucore::backup_control::{self, source_is_target_backup};
use uucore::display::Quotable; use uucore::display::Quotable;
use uucore::error::{set_exit_code, FromIo, UResult, USimpleError, UUsageError}; use uucore::error::{set_exit_code, FromIo, UResult, USimpleError, UUsageError};
use uucore::fs::{ use uucore::fs::{
are_hardlinks_or_one_way_symlink_to_same_file, are_hardlinks_to_same_file, are_hardlinks_or_one_way_symlink_to_same_file, are_hardlinks_to_same_file, canonicalize,
path_ends_with_terminator, path_ends_with_terminator, MissingHandling, ResolveMode,
}; };
#[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
use uucore::fsxattr; use uucore::fsxattr;
@ -322,20 +322,6 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
}); });
} }
if (source.eq(target)
|| are_hardlinks_to_same_file(source, target)
|| are_hardlinks_or_one_way_symlink_to_same_file(source, target))
&& opts.backup == BackupMode::NoBackup
{
if source.eq(Path::new(".")) || source.ends_with("/.") || source.is_file() {
return Err(
MvError::SameFile(source.quote().to_string(), target.quote().to_string()).into(),
);
} else {
return Err(MvError::SelfSubdirectory(source.display().to_string()).into());
}
}
let target_is_dir = target.is_dir(); let target_is_dir = target.is_dir();
let source_is_dir = source.is_dir(); let source_is_dir = source.is_dir();
@ -347,6 +333,8 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
return Err(MvError::FailedToAccessNotADirectory(target.quote().to_string()).into()); return Err(MvError::FailedToAccessNotADirectory(target.quote().to_string()).into());
} }
assert_not_same_file(source, target, target_is_dir, opts)?;
if target_is_dir { if target_is_dir {
if opts.no_target_dir { if opts.no_target_dir {
if source.is_dir() { if source.is_dir() {
@ -356,14 +344,6 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
} else { } else {
Err(MvError::DirectoryToNonDirectory(target.quote().to_string()).into()) Err(MvError::DirectoryToNonDirectory(target.quote().to_string()).into())
} }
// Check that source & target do not contain same subdir/dir when both exist
// mkdir dir1/dir2; mv dir1 dir1/dir2
} else if target.starts_with(source) {
Err(MvError::SelfTargetSubdirectory(
source.display().to_string(),
target.display().to_string(),
)
.into())
} else { } else {
move_files_into_dir(&[source.to_path_buf()], target, opts) move_files_into_dir(&[source.to_path_buf()], target, opts)
} }
@ -387,6 +367,88 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
} }
} }
fn assert_not_same_file(
source: &Path,
target: &Path,
target_is_dir: bool,
opts: &Options,
) -> UResult<()> {
// we'll compare canonicalized_source and canonicalized_target for same file detection
let canonicalized_source = match canonicalize(
absolute(source)?,
MissingHandling::Normal,
ResolveMode::Logical,
) {
Ok(source) if source.exists() => source,
_ => absolute(source)?, // file or symlink target doesn't exist but its absolute path is still used for comparison
};
// special case if the target exists, is a directory, and the `-T` flag wasn't used
let target_is_dir = target_is_dir && !opts.no_target_dir;
let canonicalized_target = if target_is_dir {
// `mv source_file target_dir` => target_dir/source_file
// canonicalize the path that exists (target directory) and join the source file name
canonicalize(
absolute(target)?,
MissingHandling::Normal,
ResolveMode::Logical,
)?
.join(source.file_name().unwrap_or_default())
} else {
// `mv source target_dir/target` => target_dir/target
// we canonicalize target_dir and join /target
match absolute(target)?.parent() {
Some(parent) if parent.to_str() != Some("") => {
canonicalize(parent, MissingHandling::Normal, ResolveMode::Logical)?
.join(target.file_name().unwrap_or_default())
}
// path.parent() returns Some("") or None if there's no parent
_ => absolute(target)?, // absolute paths should always have a parent, but we'll fall back just in case
}
};
let same_file = (canonicalized_source.eq(&canonicalized_target)
|| are_hardlinks_to_same_file(source, target)
|| are_hardlinks_or_one_way_symlink_to_same_file(source, target))
&& opts.backup == BackupMode::NoBackup;
// get the expected target path to show in errors
// this is based on the argument and not canonicalized
let target_display = match source.file_name() {
Some(file_name) if target_is_dir => {
// join target_dir/source_file in a platform-independent manner
let mut path = target
.display()
.to_string()
.trim_end_matches("/")
.to_owned();
path.push('/');
path.push_str(&file_name.to_string_lossy());
path.quote().to_string()
}
_ => target.quote().to_string(),
};
if same_file
&& (canonicalized_source.eq(&canonicalized_target)
|| source.eq(Path::new("."))
|| source.ends_with("/.")
|| source.is_file())
{
return Err(MvError::SameFile(source.quote().to_string(), target_display).into());
} else if (same_file || canonicalized_target.starts_with(canonicalized_source))
// don't error if we're moving a symlink of a directory into itself
&& !source.is_symlink()
{
return Err(
MvError::SelfTargetSubdirectory(source.quote().to_string(), target_display).into(),
);
}
Ok(())
}
fn handle_multiple_paths(paths: &[PathBuf], opts: &Options) -> UResult<()> { fn handle_multiple_paths(paths: &[PathBuf], opts: &Options) -> UResult<()> {
if opts.no_target_dir { if opts.no_target_dir {
return Err(UUsageError::new( return Err(UUsageError::new(
@ -425,10 +487,6 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options)
return Err(MvError::NotADirectory(target_dir.quote().to_string()).into()); return Err(MvError::NotADirectory(target_dir.quote().to_string()).into());
} }
let canonicalized_target_dir = target_dir
.canonicalize()
.unwrap_or_else(|_| target_dir.to_path_buf());
let multi_progress = options.progress_bar.then(MultiProgress::new); let multi_progress = options.progress_bar.then(MultiProgress::new);
let count_progress = if let Some(ref multi_progress) = multi_progress { let count_progress = if let Some(ref multi_progress) = multi_progress {
@ -479,24 +537,9 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options)
// Check if we have mv dir1 dir2 dir2 // Check if we have mv dir1 dir2 dir2
// And generate an error if this is the case // And generate an error if this is the case
if let Ok(canonicalized_source) = sourcepath.canonicalize() { if let Err(e) = assert_not_same_file(sourcepath, target_dir, true, options) {
if canonicalized_source == canonicalized_target_dir { show!(e);
// User tried to move directory to itself, warning is shown continue;
// and process of moving files is continued.
show!(USimpleError::new(
1,
format!(
"cannot move '{}' to a subdirectory of itself, '{}/{}'",
sourcepath.display(),
uucore::fs::normalize_path(target_dir).display(),
canonicalized_target_dir.components().last().map_or_else(
|| target_dir.display().to_string(),
|dir| { PathBuf::from(dir.as_os_str()).display().to_string() }
)
)
));
continue;
}
} }
match rename(sourcepath, &targetpath, options, multi_progress.as_ref()) { match rename(sourcepath, &targetpath, options, multi_progress.as_ref()) {

View file

@ -6,6 +6,7 @@
// spell-checker:ignore mydir // spell-checker:ignore mydir
use crate::common::util::TestScenario; use crate::common::util::TestScenario;
use filetime::FileTime; use filetime::FileTime;
use rstest::rstest;
use std::io::Write; use std::io::Write;
#[test] #[test]
@ -467,7 +468,31 @@ fn test_mv_same_symlink() {
.arg(file_c) .arg(file_c)
.arg(file_a) .arg(file_a)
.fails() .fails()
.stderr_is(format!("mv: '{file_c}' and '{file_a}' are the same file\n",)); .stderr_is(format!("mv: '{file_c}' and '{file_a}' are the same file\n"));
}
#[test]
#[cfg(all(unix, not(target_os = "android")))]
fn test_mv_same_broken_symlink() {
let (at, mut ucmd) = at_and_ucmd!();
at.symlink_file("missing-target", "broken");
ucmd.arg("broken")
.arg("broken")
.fails()
.stderr_is("mv: 'broken' and 'broken' are the same file\n");
}
#[test]
#[cfg(all(unix, not(target_os = "android")))]
fn test_mv_symlink_into_target() {
let (at, mut ucmd) = at_and_ucmd!();
at.mkdir("dir");
at.symlink_file("dir", "dir-link");
ucmd.arg("dir-link").arg("dir").succeeds();
} }
#[test] #[test]
@ -1389,24 +1414,6 @@ fn test_mv_interactive_error() {
.is_empty()); .is_empty());
} }
#[test]
fn test_mv_into_self() {
let scene = TestScenario::new(util_name!());
let at = &scene.fixtures;
let dir1 = "dir1";
let dir2 = "dir2";
at.mkdir(dir1);
at.mkdir(dir2);
scene
.ucmd()
.arg(dir1)
.arg(dir2)
.arg(dir2)
.fails()
.stderr_contains("mv: cannot move 'dir2' to a subdirectory of itself, 'dir2/dir2'");
}
#[test] #[test]
fn test_mv_arg_interactive_skipped() { fn test_mv_arg_interactive_skipped() {
let (at, mut ucmd) = at_and_ucmd!(); let (at, mut ucmd) = at_and_ucmd!();
@ -1456,27 +1463,32 @@ fn test_mv_into_self_data() {
assert!(!at.file_exists(file1)); assert!(!at.file_exists(file1));
} }
#[test] #[rstest]
fn test_mv_directory_into_subdirectory_of_itself_fails() { #[case(vec!["mydir"], vec!["mydir", "mydir"], "mv: cannot move 'mydir' to a subdirectory of itself, 'mydir/mydir'")]
#[case(vec!["mydir"], vec!["mydir/", "mydir/"], "mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir'")]
#[case(vec!["mydir"], vec!["./mydir", "mydir", "mydir/"], "mv: cannot move './mydir' to a subdirectory of itself, 'mydir/mydir'")]
#[case(vec!["mydir"], vec!["mydir/", "mydir/mydir_2/"], "mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir_2/'")]
#[case(vec!["mydir/mydir_2"], vec!["mydir", "mydir/mydir_2"], "mv: cannot move 'mydir' to a subdirectory of itself, 'mydir/mydir_2/mydir'\n")]
#[case(vec!["mydir/mydir_2"], vec!["mydir/", "mydir/mydir_2/"], "mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir_2/mydir'\n")]
#[case(vec!["mydir", "mydir_2"], vec!["mydir/", "mydir_2/", "mydir_2/"], "mv: cannot move 'mydir_2/' to a subdirectory of itself, 'mydir_2/mydir_2'")]
#[case(vec!["mydir"], vec!["mydir/", "mydir"], "mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir'")]
#[case(vec!["mydir"], vec!["-T", "mydir", "mydir"], "mv: 'mydir' and 'mydir' are the same file")]
#[case(vec!["mydir"], vec!["mydir/", "mydir/../"], "mv: 'mydir/' and 'mydir/../mydir' are the same file")]
fn test_mv_directory_self(
#[case] dirs: Vec<&str>,
#[case] args: Vec<&str>,
#[case] expected_error: &str,
) {
let scene = TestScenario::new(util_name!()); let scene = TestScenario::new(util_name!());
let at = &scene.fixtures; let at = &scene.fixtures;
let dir1 = "mydir"; for dir in dirs {
let dir2 = "mydir/mydir_2"; at.mkdir_all(dir);
at.mkdir(dir1); }
at.mkdir(dir2);
scene.ucmd().arg(dir1).arg(dir2).fails().stderr_contains(
"mv: cannot move 'mydir' to a subdirectory of itself, 'mydir/mydir_2/mydir'",
);
// check that it also errors out with /
scene scene
.ucmd() .ucmd()
.arg(format!("{dir1}/")) .args(&args)
.arg(dir2)
.fails() .fails()
.stderr_contains( .stderr_contains(expected_error);
"mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir_2/mydir/'",
);
} }
#[test] #[test]
@ -1755,23 +1767,3 @@ fn test_mv_error_msg_with_multiple_sources_that_does_not_exist() {
.stderr_contains("mv: cannot stat 'a': No such file or directory") .stderr_contains("mv: cannot stat 'a': No such file or directory")
.stderr_contains("mv: cannot stat 'b/': No such file or directory"); .stderr_contains("mv: cannot stat 'b/': No such file or directory");
} }
#[test]
fn test_mv_error_cant_move_itself() {
let scene = TestScenario::new(util_name!());
let at = &scene.fixtures;
at.mkdir("b");
scene
.ucmd()
.arg("b")
.arg("b/")
.fails()
.stderr_contains("mv: cannot move 'b' to a subdirectory of itself, 'b/b'");
scene
.ucmd()
.arg("./b")
.arg("b")
.arg("b/")
.fails()
.stderr_contains("mv: cannot move 'b' to a subdirectory of itself, 'b/b'");
}