Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sylvestre merged 1 commit into from
Dec 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions src/uu/mv/src/error.rs
View file
Open in desktop
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
137 changes: 90 additions & 47 deletions src/uu/mv/src/mv.rs
View file
Open in desktop
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,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(
Copy link
Contributor

@sylvestre sylvestre Dec 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please add some comments explaining the different cases ?

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<()> {
if opts.no_target_dir {
return Err(UUsageError::new(
Expand Down Expand Up @@ -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());
}

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 +537,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
Open in desktop
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'");
}
Loading