From 75edcbc0d0a12aa4a21458fb7ddd99d196e7117f Mon Sep 17 00:00:00 2001 From: Jason Gedge Date: Tue, 12 May 2020 16:40:45 -0400 Subject: [PATCH] Simplify `mv` in `FilesystemShell` (#1587) --- crates/nu-cli/src/shell/filesystem_shell.rs | 450 ++++---------------- crates/nu-cli/tests/commands/mv.rs | 79 ++-- 2 files changed, 136 insertions(+), 393 deletions(-) diff --git a/crates/nu-cli/src/shell/filesystem_shell.rs b/crates/nu-cli/src/shell/filesystem_shell.rs index cb863705d6..decb42c802 100644 --- a/crates/nu-cli/src/shell/filesystem_shell.rs +++ b/crates/nu-cli/src/shell/filesystem_shell.rs @@ -15,7 +15,7 @@ use crate::utils::FileStructure; use rustyline::completion::FilenameCompleter; use rustyline::hint::{Hinter, HistoryHinter}; use std::collections::HashMap; -use std::path::{Component, Path, PathBuf}; +use std::path::{Path, PathBuf}; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; @@ -429,390 +429,51 @@ impl Shell for FilesystemShell { fn mv( &self, MoveArgs { src, dst }: MoveArgs, - name: Tag, + _name: Tag, path: &str, ) -> Result { - let name_tag = name; - let path = Path::new(path); let source = path.join(&src.item); - let mut destination = path.join(&dst.item); + let destination = path.join(&dst.item); - let sources: Vec<_> = match glob::glob(&source.to_string_lossy()) { - Ok(files) => files.collect(), - Err(_) => { - return Err(ShellError::labeled_error( - "Invalid pattern.", - "invalid pattern", - src.tag, - )) - } - }; + let sources = + glob::glob(&source.to_string_lossy()).map_or_else(|_| Vec::new(), Iterator::collect); if sources.is_empty() { return Err(ShellError::labeled_error( - "Invalid file or pattern.", + "Invalid file or pattern", "invalid file or pattern", src.tag, )); } - let destination_file_name = { - match destination.file_name() { - Some(name) => PathBuf::from(name), - None => { - let name_maybe = - destination.components().next_back().and_then( - |component| match component { - Component::RootDir => Some(PathBuf::from("/")), - Component::ParentDir => destination - .parent() - .and_then(|parent| parent.file_name()) - .map(PathBuf::from), - _ => None, - }, - ); + // We have two possibilities. + // + // First, the destination exists. + // - If a directory, move everything into that directory, otherwise + // - if only a single source, overwrite the file, otherwise + // - error. + // + // Second, the destination doesn't exist, so we can only rename a single source. Otherwise + // it's an error. - if let Some(name) = name_maybe { - name - } else { - return Err(ShellError::labeled_error( - "Rename aborted. Not a valid destination", - "not a valid destination", - dst.tag, - )); - } - } - } - }; - - if sources.is_empty() { + if (destination.exists() && !destination.is_dir() && sources.len() > 1) + || (!destination.exists() && sources.len() > 1) + { return Err(ShellError::labeled_error( - "Move aborted. Not a valid destination", - "not a valid destination", - src.tag, + "Can only move multiple sources if destination is a directory", + "destination must be a directory when multiple sources", + dst.tag, )); } - if sources.len() == 1 { - if let Ok(entry) = &sources[0] { - let entry_file_name = match entry.file_name() { - Some(name) => name, - None => { - return Err(ShellError::labeled_error( - "Rename aborted. Not a valid entry name", - "not a valid entry name", - src.tag, - )) - } - }; - - if destination.exists() && destination.is_dir() { - destination = match dunce::canonicalize(&destination) { - Ok(path) => path, - Err(e) => { - return Err(ShellError::labeled_error( - format!("Rename aborted. {:}", e.to_string()), - e.to_string(), - dst.tag, - )) - } - }; - - destination.push(entry_file_name); - } - - if entry.is_file() { - #[cfg(not(windows))] - { - match std::fs::rename(&entry, &destination) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Rename {:?} to {:?} aborted. {:}", - entry_file_name, - destination_file_name, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(o) => o, - }; - } - #[cfg(windows)] - { - match std::fs::copy(&entry, &destination) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Rename {:?} to {:?} aborted. {:}", - entry_file_name, - destination_file_name, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(_) => match std::fs::remove_file(&entry) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Rename {:?} to {:?} aborted. {:}", - entry_file_name, - destination_file_name, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(o) => o, - }, - }; - } - } - - if entry.is_dir() { - match std::fs::create_dir_all(&destination) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Rename {:?} to {:?} aborted. {:}", - entry_file_name, - destination_file_name, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(o) => o, - }; - #[cfg(not(windows))] - { - match std::fs::rename(&entry, &destination) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Rename {:?} to {:?} aborted. {:}", - entry_file_name, - destination_file_name, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(o) => o, - }; - } - #[cfg(windows)] - { - let mut sources: FileStructure = FileStructure::new(); - - sources.walk_decorate(&entry)?; - - let strategy = |(source_file, depth_level)| { - let mut new_dst = destination.clone(); - - let path = dunce::canonicalize(&source_file)?; - - let mut comps: Vec<_> = path - .components() - .map(|fragment| fragment.as_os_str()) - .rev() - .take(1 + depth_level) - .collect(); - - comps.reverse(); - - for fragment in comps.iter() { - new_dst.push(fragment); - } - - Ok((PathBuf::from(&source_file), new_dst)) - }; - - let sources = sources.paths_applying_with(strategy)?; - - for (ref src, ref dst) in sources { - if src.is_dir() && !dst.exists() { - match std::fs::create_dir_all(dst) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Rename {:?} to {:?} aborted. {:}", - entry_file_name, - destination_file_name, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(o) => o, - } - } else if src.is_file() { - match std::fs::copy(src, dst) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Moving file {:?} to {:?} aborted. {:}", - src, - dst, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(_o) => (), - } - } - } - - if src.is_file() { - match std::fs::copy(&src, &dst) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Rename {:?} to {:?} aborted. {:}", - src, - destination_file_name, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(_) => match std::fs::remove_file(&src) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Rename {:?} to {:?} aborted. {:}", - entry_file_name, - destination_file_name, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(o) => o, - }, - }; - } - - match std::fs::remove_dir_all(entry) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Rename {:?} to {:?} aborted. {:}", - entry_file_name, - destination_file_name, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(o) => o, - }; - } - } + for entry in sources { + if let Ok(entry) = entry { + move_file( + TaggedPathBuf(&entry, &src.tag), + TaggedPathBuf(&destination, &dst.tag), + )? } - } else if destination.exists() { - let is_file = |x: &Result| { - x.as_ref().map(|entry| entry.is_file()).unwrap_or_default() - }; - - if !sources.iter().all(is_file) { - return Err(ShellError::labeled_error( - "Rename aborted (directories found). Renaming in patterns not supported yet (try moving the directory directly)", - "renaming in patterns not supported yet (try moving the directory directly)", - src.tag, - )); - } - - for entry in sources { - if let Ok(entry) = entry { - let entry_file_name = match entry.file_name() { - Some(name) => name, - None => { - return Err(ShellError::labeled_error( - "Rename aborted. Not a valid entry name", - "not a valid entry name", - src.tag, - )) - } - }; - - let mut to = PathBuf::from(&destination); - to.push(entry_file_name); - - if entry.is_file() { - #[cfg(not(windows))] - { - match std::fs::rename(&entry, &to) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Rename {:?} to {:?} aborted. {:}", - entry_file_name, - destination_file_name, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(o) => o, - }; - } - #[cfg(windows)] - { - match std::fs::copy(&entry, &to) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Rename {:?} to {:?} aborted. {:}", - entry_file_name, - destination_file_name, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(_) => match std::fs::remove_file(&entry) { - Err(e) => { - return Err(ShellError::labeled_error( - format!( - "Remove {:?} to {:?} aborted. {:}", - entry_file_name, - destination_file_name, - e.to_string(), - ), - e.to_string(), - name_tag, - )); - } - Ok(o) => o, - }, - }; - } - } - } - } - } else { - return Err(ShellError::labeled_error( - format!("Rename aborted. (Does {:?} exist?)", destination_file_name), - format!("rename aborted (does {:?} exist?)", destination_file_name), - dst.tag, - )); } Ok(OutputStream::empty()) @@ -1034,6 +695,63 @@ impl Shell for FilesystemShell { } } +struct TaggedPathBuf<'a>(&'a PathBuf, &'a Tag); + +fn move_file(from: TaggedPathBuf, to: TaggedPathBuf) -> Result<(), ShellError> { + let TaggedPathBuf(from, from_tag) = from; + let TaggedPathBuf(to, to_tag) = to; + + if to.exists() && from.is_dir() && to.is_file() { + return Err(ShellError::labeled_error( + "Cannot rename a directory to a file", + "invalid destination", + to_tag, + )); + } + + let destination_dir_exists = if to.is_dir() { + true + } else { + to.parent().map(Path::exists).unwrap_or(true) + }; + + if !destination_dir_exists { + return Err(ShellError::labeled_error( + "Destination directory does not exist", + "destination does not exist", + to_tag, + )); + } + + let mut to = to.clone(); + if to.is_dir() { + let from_file_name = match from.file_name() { + Some(name) => name, + None => { + return Err(ShellError::labeled_error( + "Not a valid entry name", + "not a valid entry name", + from_tag, + )) + } + }; + + to.push(from_file_name); + } + + // We first try a rename, which is a quick operation. If that doesn't work, we'll try a copy + // and remove the old file. This is necessary if we're moving across filesystems. + std::fs::rename(&from, &to) + .or_else(|_| std::fs::copy(&from, &to).and_then(|_| std::fs::remove_file(&from))) + .map_err(|e| { + ShellError::labeled_error( + format!("Could not move {:?} to {:?}. {:}", from, to, e.to_string()), + "could not move", + from_tag, + ) + }) +} + fn is_empty_dir(dir: impl AsRef) -> bool { match dir.as_ref().read_dir() { Err(_) => true, diff --git a/crates/nu-cli/tests/commands/mv.rs b/crates/nu-cli/tests/commands/mv.rs index 268aac7d92..41c0e99c43 100644 --- a/crates/nu-cli/tests/commands/mv.rs +++ b/crates/nu-cli/tests/commands/mv.rs @@ -100,30 +100,6 @@ fn moves_the_directory_inside_directory_if_path_to_move_is_existing_directory() }) } -#[test] -fn moves_the_directory_inside_directory_if_path_to_move_is_nonexistent_directory() { - Playground::setup("mv_test_6", |dirs, sandbox| { - sandbox - .within("contributors") - .with_files(vec![EmptyFile("jonathan.txt")]) - .mkdir("expected"); - - let original_dir = dirs.test().join("contributors"); - - nu!( - cwd: dirs.test(), - "mv contributors expected/this_dir_exists_now/los_tres_amigos" - ); - - let expected = dirs - .test() - .join("expected/this_dir_exists_now/los_tres_amigos"); - - assert!(!original_dir.exists()); - assert!(expected.exists()); - }) -} - #[test] fn moves_using_path_with_wildcard() { Playground::setup("mv_test_7", |dirs, sandbox| { @@ -224,13 +200,63 @@ fn errors_if_source_doesnt_exist() { Playground::setup("mv_test_10", |dirs, sandbox| { sandbox.mkdir("test_folder"); let actual = nu!( - cwd: dirs.root(), + cwd: dirs.test(), "mv non-existing-file test_folder/" ); assert!(actual.err.contains("Invalid file or pattern")); }) } +#[test] +fn errors_if_destination_doesnt_exist() { + Playground::setup("mv_test_10_1", |dirs, sandbox| { + sandbox.with_files(vec![EmptyFile("empty.txt")]); + + let actual = nu!( + cwd: dirs.test(), + "mv empty.txt does/not/exist" + ); + + assert!(actual.err.contains("Destination directory does not exist")); + }) +} + +#[test] +fn errors_if_multiple_sources_but_destination_not_a_directory() { + Playground::setup("mv_test_10_2", |dirs, sandbox| { + sandbox.with_files(vec![ + EmptyFile("file1.txt"), + EmptyFile("file2.txt"), + EmptyFile("file3.txt"), + ]); + + let actual = nu!( + cwd: dirs.test(), + "mv file?.txt not_a_dir" + ); + + assert!(actual + .err + .contains("Can only move multiple sources if destination is a directory")); + }) +} + +#[test] +fn errors_if_renaming_directory_to_an_existing_file() { + Playground::setup("mv_test_10_3", |dirs, sandbox| { + sandbox + .mkdir("mydir") + .with_files(vec![EmptyFile("empty.txt")]); + + let actual = nu!( + cwd: dirs.test(), + "mv mydir empty.txt" + ); + + assert!(actual.err.contains("Cannot rename a directory to a file")); + }) +} + #[test] fn does_not_error_on_relative_parent_path() { Playground::setup("mv_test_11", |dirs, sandbox| { @@ -252,10 +278,9 @@ fn does_not_error_on_relative_parent_path() { } #[test] -#[ignore] // Temporarily failling, see https://github.com/nushell/nushell/issues/1523 fn move_files_using_glob_two_parents_up_using_multiple_dots() { Playground::setup("mv_test_12", |dirs, sandbox| { - sandbox.within("foo").mkdir("bar").with_files(vec![ + sandbox.within("foo").within("bar").with_files(vec![ EmptyFile("jonathan.json"), EmptyFile("andres.xml"), EmptyFile("yehuda.yaml"),