Skip to content

Commit

Permalink
mv: improve move-to-self error handling
Browse files Browse the repository at this point in the history 
- 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
```
  • Loading branch information
@sgvictorino
sgvictorino committed Dec 24, 2024
1 parent 609eee6 commit ddbb07b
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 111 deletions.
12 changes: 3 additions & 9 deletions src/uu/mv/src/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ pub enum MvError {
NoSuchFile(String),
CannotStatNotADirectory(String),
SameFile(String, String),
SelfSubdirectory(String),
SelfTargetSubdirectory(String, String),
DirectoryToNonDirectory(String),
NonDirectoryToDirectory(String, String),
Expand All @@ -29,14 +28,9 @@ impl Display for MvError {
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::SameFile(s, t) => write!(f, "{s} and {t} are the same file"),
Self::SelfSubdirectory(s) => write!(
f,
"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::SelfTargetSubdirectory(s, t) => {
write!(f, "cannot move {s} to a subdirectory of itself, {t}")
}
Self::DirectoryToNonDirectory(t) => {
write!(f, "cannot overwrite directory {t} with non-directory")
}
Expand Down
120 changes: 73 additions & 47 deletions src/uu/mv/src/mv.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ use std::io;
use std::os::unix;
#[cfg(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::display::Quotable;
use uucore::error::{set_exit_code, FromIo, UResult, USimpleError, UUsageError};
use uucore::fs::{
are_hardlinks_or_one_way_symlink_to_same_file, are_hardlinks_to_same_file,
path_ends_with_terminator,
are_hardlinks_or_one_way_symlink_to_same_file, are_hardlinks_to_same_file, canonicalize,
path_ends_with_terminator, MissingHandling, ResolveMode,
};
#[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
use uucore::fsxattr;
Expand Down Expand Up @@ -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 source_is_dir = source.is_dir();

Expand All @@ -347,6 +333,8 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
return Err(MvError::FailedToAccessNotADirectory(target.quote().to_string()).into());
}

assert_not_same_file(source, target, target_is_dir, opts)?;

if target_is_dir {
if opts.no_target_dir {
if source.is_dir() {
Expand All @@ -356,14 +344,6 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
} else {
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 {
move_files_into_dir(&[source.to_path_buf()], target, opts)
}
Expand All @@ -387,6 +367,71 @@ 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<()> {
let target_is_dir = target_is_dir && !opts.no_target_dir;

let canonicalized_source =
match canonicalize(source, MissingHandling::Normal, ResolveMode::Logical) {
Ok(source) if source.exists() => source,
_ => absolute(source)?,
};

let canonicalized_target = if target_is_dir {
canonicalize(target, MissingHandling::Normal, ResolveMode::Logical)?
.join(source.file_name().unwrap_or_default())
} else {
match target.parent() {
Some(parent) if parent.to_str() != Some("") => {
canonicalize(parent, MissingHandling::Normal, ResolveMode::Logical)?
.join(target.file_name().unwrap_or_default())
}
_ => absolute(target)?,
}
};

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;

let target_display = match source.file_name() {
Some(file_name) if target_is_dir => {
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 == 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))
&& !source.is_symlink()
{
return Err(
MvError::SelfTargetSubdirectory(source.quote().to_string(), target_display).into(),
);
}
Ok(())
}

fn handle_multiple_paths(paths: &[PathBuf], opts: &Options) -> UResult<()> {
if opts.no_target_dir {
return Err(UUsageError::new(
Expand Down Expand Up @@ -425,10 +470,6 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options)
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 count_progress = if let Some(ref multi_progress) = multi_progress {
Expand Down Expand Up @@ -479,24 +520,9 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options)

// Check if we have mv dir1 dir2 dir2
// And generate an error if this is the case
if let Ok(canonicalized_source) = sourcepath.canonicalize() {
if canonicalized_source == canonicalized_target_dir {
// User tried to move directory to itself, warning is shown
// 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;
}
if let Err(e) = assert_not_same_file(sourcepath, target_dir, true, options) {
show!(e);
continue;
}

match rename(sourcepath, &targetpath, options, multi_progress.as_ref()) {
Expand Down
102 changes: 47 additions & 55 deletions tests/by-util/test_mv.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// spell-checker:ignore mydir
use crate::common::util::TestScenario;
use filetime::FileTime;
use rstest::rstest;
use std::io::Write;

#[test]
Expand Down Expand Up @@ -467,7 +468,31 @@ fn test_mv_same_symlink() {
.arg(file_c)
.arg(file_a)
.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]
Expand Down Expand Up @@ -1389,24 +1414,6 @@ fn test_mv_interactive_error() {
.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]
fn test_mv_arg_interactive_skipped() {
let (at, mut ucmd) = at_and_ucmd!();
Expand Down Expand Up @@ -1456,27 +1463,32 @@ fn test_mv_into_self_data() {
assert!(!at.file_exists(file1));
}

#[test]
fn test_mv_directory_into_subdirectory_of_itself_fails() {
#[rstest]
#[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 at = &scene.fixtures;
let dir1 = "mydir";
let dir2 = "mydir/mydir_2";
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 /
for dir in dirs {
at.mkdir_all(dir);
}
scene
.ucmd()
.arg(format!("{dir1}/"))
.arg(dir2)
.args(&args)
.fails()
.stderr_contains(
"mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir_2/mydir/'",
);
.stderr_contains(expected_error);
}

#[test]
Expand Down Expand Up @@ -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 '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'");
}

0 comments on commit ddbb07b

Please sign in to comment.