Make default for mv safer, require -f to overwrite (#6904)

* fix:  "saner" default for mv

fixes #6747

As highlighted in the issue, the default behavior of nu currently
is to overwrite the destination file without notice.
This is not a "standard" expectation users that want this behavior
can create a dedicated alias.

* fix: 📝 edit the comment

* fix:  updated the tests

* fix: 🚧 use --force for case test
This commit is contained in:
Mel Massadian 2022-10-29 22:16:29 +02:00 committed by GitHub
parent ce4ae00a6f
commit 843d8c2242
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 30 additions and 7 deletions

View file

@ -50,8 +50,8 @@ impl Command for Mv {
"make mv to be verbose, showing files been moved.", "make mv to be verbose, showing files been moved.",
Some('v'), Some('v'),
) )
.switch("force", "overwrite the destination.", Some('f'))
.switch("interactive", "ask user to confirm action", Some('i')) .switch("interactive", "ask user to confirm action", Some('i'))
// .switch("force", "suppress error when no file", Some('f'))
.category(Category::FileSystem) .category(Category::FileSystem)
} }
@ -76,7 +76,7 @@ impl Command for Mv {
let spanned_destination: Spanned<String> = call.req(engine_state, stack, 1)?; let spanned_destination: Spanned<String> = call.req(engine_state, stack, 1)?;
let verbose = call.has_flag("verbose"); let verbose = call.has_flag("verbose");
let interactive = call.has_flag("interactive"); let interactive = call.has_flag("interactive");
// let force = call.has_flag("force"); let force = call.has_flag("force");
let ctrlc = engine_state.ctrlc.clone(); let ctrlc = engine_state.ctrlc.clone();
@ -101,12 +101,22 @@ impl Command for Mv {
// //
// First, the destination exists. // First, the destination exists.
// - If a directory, move everything into that directory, otherwise // - If a directory, move everything into that directory, otherwise
// - if only a single source, overwrite the file, otherwise // - if only a single source, and --force (or -f) is provided overwrite the file,
// - error. // - otherwise error.
// //
// Second, the destination doesn't exist, so we can only rename a single source. Otherwise // Second, the destination doesn't exist, so we can only rename a single source. Otherwise
// it's an error. // it's an error.
if destination.exists() && !force && !destination.is_dir() && !source.is_dir() {
return Err(ShellError::GenericError(
"Destination file already exists".into(),
"you can use -f, --force to force overwriting the destination".into(),
Some(spanned_destination.span),
None,
Vec::new(),
));
}
if (destination.exists() && !destination.is_dir() && sources.len() > 1) if (destination.exists() && !destination.is_dir() && sources.len() > 1)
|| (!destination.exists() && sources.len() > 1) || (!destination.exists() && sources.len() > 1)
{ {

View file

@ -23,7 +23,7 @@ fn moves_a_file() {
} }
#[test] #[test]
fn overwrites_if_moving_to_existing_file() { fn overwrites_if_moving_to_existing_file_and_force_provided() {
Playground::setup("mv_test_2", |dirs, sandbox| { Playground::setup("mv_test_2", |dirs, sandbox| {
sandbox.with_files(vec![EmptyFile("andres.txt"), EmptyFile("jonathan.txt")]); sandbox.with_files(vec![EmptyFile("andres.txt"), EmptyFile("jonathan.txt")]);
@ -32,7 +32,7 @@ fn overwrites_if_moving_to_existing_file() {
nu!( nu!(
cwd: dirs.test(), cwd: dirs.test(),
"mv andres.txt jonathan.txt" "mv andres.txt -f jonathan.txt"
); );
assert!(!original.exists()); assert!(!original.exists());
@ -207,6 +207,19 @@ fn errors_if_source_doesnt_exist() {
}) })
} }
#[test]
fn error_if_moving_to_existing_file_without_force() {
Playground::setup("mv_test_10_0", |dirs, sandbox| {
sandbox.with_files(vec![EmptyFile("andres.txt"), EmptyFile("jonathan.txt")]);
let actual = nu!(
cwd: dirs.test(),
"mv andres.txt jonathan.txt"
);
assert!(actual.err.contains("file already exists"))
})
}
#[test] #[test]
fn errors_if_destination_doesnt_exist() { fn errors_if_destination_doesnt_exist() {
Playground::setup("mv_test_10_1", |dirs, sandbox| { Playground::setup("mv_test_10_1", |dirs, sandbox| {
@ -438,7 +451,7 @@ fn mv_change_case_of_file() {
nu!( nu!(
cwd: dirs.test(), cwd: dirs.test(),
format!("mv {original_file_name} {new_file_name}") format!("mv {original_file_name} -f {new_file_name}")
); );
// Doing this instead of `Path::exists()` because we need to check file existence in // Doing this instead of `Path::exists()` because we need to check file existence in