Skip to content

mv: Fix stderr output mv file into dir and dir into file where both are files #5464

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
merged 6 commits into from
Oct 28, 2023
Merged

mv: Fix stderr output mv file into dir and dir into file where both are files #5464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e04c89c
Add tests mv file into dir and dir into file where both are files
mickvangelderen Oct 27, 2023
3a5476f
Fix test_mv_dir_into_file_where_both_are_files
mickvangelderen Oct 27, 2023
cadfb80
Fix test_mv_file_into_dir_where_both_are_files
mickvangelderen Oct 27, 2023
aa6f674
Store String in error instead of PathBuf
mickvangelderen Oct 27, 2023
54808af
Implement path_ends_with_terminator for windows
mickvangelderen Oct 27, 2023
4f051b4
Fix compilation on windows
mickvangelderen Oct 28, 2023
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
7 changes: 7 additions & 0 deletions src/uu/mv/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ use uucore::error::UError;
#[derive(Debug)]
pub enum MvError {
NoSuchFile(String),
CannotStatNotADirectory(String),
SameFile(String, String),
SelfSubdirectory(String),
SelfTargetSubdirectory(String, String),
DirectoryToNonDirectory(String),
NonDirectoryToDirectory(String, String),
NotADirectory(String),
TargetNotADirectory(String),
FailedToAccessNotADirectory(String),
}

impl Error for MvError {}
Expand All @@ -25,6 +27,7 @@ impl Display for MvError {
fn fmt(&self, f: &mut Formatter) -> Result {
match self {
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,
Expand All @@ -42,6 +45,10 @@ impl Display for MvError {
}
Self::NotADirectory(t) => write!(f, "target {t}: Not a directory"),
Self::TargetNotADirectory(t) => write!(f, "target directory {t}: Not a directory"),

Self::FailedToAccessNotADirectory(t) => {
write!(f, "failed to access {t}: Not a directory")
}
}
}
}
33 changes: 31 additions & 2 deletions src/uu/mv/src/mv.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,25 @@ static OPT_VERBOSE: &str = "verbose";
static OPT_PROGRESS: &str = "progress";
static ARG_FILES: &str = "files";

/// Returns true if the passed `path` ends with a path terminator.
#[cfg(unix)]
fn path_ends_with_terminator(path: &Path) -> bool {
use std::os::unix::prelude::OsStrExt;
path.as_os_str()
.as_bytes()
Copy link
Contributor

@cakebaker cakebaker Oct 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I guess you have seen the error from the CI that there is no as_bytes function under Windows?

Copy link
Contributor Author

@mickvangelderen mickvangelderen Oct 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The pipeline wouldn't run until someone from this project had approved it. Thanks for pointing out that they are running now.

Copy link
Contributor Author

@mickvangelderen mickvangelderen Oct 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@cakebaker can I run the workflows locally? They won't run until they're approved:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@mickvangelderen the approval is no longer needed and the workflows should run automatically in the future.

.last()
.map_or(false, |&byte| byte == b'/' || byte == b'\\')
}

#[cfg(windows)]
fn path_ends_with_terminator(path: &Path) -> bool {
use std::os::windows::prelude::OsStrExt;
path.as_os_str()
.encode_wide()
.last()
.map_or(false, |wide| wide == b'/'.into() || wide == b'\\'.into())
}

#[uucore::main]
pub fn uumain(args: impl uucore::Args) -> UResult<()> {
let mut app = uu_app();
Expand Down Expand Up @@ -299,7 +318,11 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
.into());
}
if source.symlink_metadata().is_err() {
return Err(MvError::NoSuchFile(source.quote().to_string()).into());
return Err(if path_ends_with_terminator(source) {
MvError::CannotStatNotADirectory(source.quote().to_string()).into()
} else {
MvError::NoSuchFile(source.quote().to_string()).into()
});
}

if (source.eq(target)
Expand All @@ -316,7 +339,13 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
}
}

if target.is_dir() {
let target_is_dir = target.is_dir();

if path_ends_with_terminator(target) && !target_is_dir {
return Err(MvError::FailedToAccessNotADirectory(target.quote().to_string()).into());
}

if target_is_dir {
if opts.no_target_dir {
if source.is_dir() {
rename(source, target, opts, None).map_err_context(|| {
Expand Down
29 changes: 29 additions & 0 deletions tests/by-util/test_mv.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1414,6 +1414,35 @@ fn test_mv_directory_into_subdirectory_of_itself_fails() {
"mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir_2/mydir/'",
);
}

#[test]
fn test_mv_file_into_dir_where_both_are_files() {
let scene = TestScenario::new(util_name!());
let at = &scene.fixtures;
at.touch("a");
at.touch("b");
scene
.ucmd()
.arg("a")
.arg("b/")
.fails()
.stderr_contains("mv: failed to access 'b/': Not a directory");
}

#[test]
fn test_mv_dir_into_file_where_both_are_files() {
let scene = TestScenario::new(util_name!());
let at = &scene.fixtures;
at.touch("a");
at.touch("b");
scene
.ucmd()
.arg("a/")
.arg("b")
.fails()
.stderr_contains("mv: cannot stat 'a/': Not a directory");
}

// Todo:

// $ at.touch a b
Expand Down