From 2cc5952c3742d193283be14bb8f807140aeb2d26 Mon Sep 17 00:00:00 2001 From: Stefan Stanciulescu <71919805+onthebridgetonowhere@users.noreply.github.com> Date: Fri, 20 May 2022 14:49:29 -0700 Subject: [PATCH] Fix cp bug (#5462) * Cleanup - remove old commented code * Force a / or \ to distinguish between folders and files for cp * Force a / or \ to distinguish between folders and files for cp * Remove unneeded code * Add cp test for checking copy to non existing directory * Fix warning in test --- crates/nu-command/src/filesystem/cp.rs | 188 ++----------------------- crates/nu-command/tests/commands/cp.rs | 14 ++ crates/nu-path/src/tilde.rs | 7 + 3 files changed, 30 insertions(+), 179 deletions(-) diff --git a/crates/nu-command/src/filesystem/cp.rs b/crates/nu-command/src/filesystem/cp.rs index 8684ca014f..90b25a89bd 100644 --- a/crates/nu-command/src/filesystem/cp.rs +++ b/crates/nu-command/src/filesystem/cp.rs @@ -74,6 +74,15 @@ impl Command for Cp { let source = path.join(src.item.as_str()); let destination = path.join(dst.item.as_str()); + // check if destination is a dir and it exists + let path_last_char = destination.as_os_str().to_string_lossy().chars().last(); + let is_directory = path_last_char == Some('/') || path_last_char == Some('\\'); + if is_directory && !destination.exists() { + return Err(ShellError::DirectoryNotFound( + dst.span, + Some("destination directory does not exist".to_string()), + )); + } let ctrlc = engine_state.ctrlc.clone(); let span = call.head; @@ -254,185 +263,6 @@ impl Command for Cp { }, ] } - - // let mut sources = - // nu_glob::glob(&source.to_string_lossy()).map_or_else(|_| Vec::new(), Iterator::collect); - // if sources.is_empty() { - // return Err(ShellError::FileNotFound(call.positional[0].span)); - // } - - // if sources.len() > 1 && !destination.is_dir() { - // return Err(ShellError::MoveNotPossible { - // source_message: "Can't move many files".to_string(), - // source_span: call.positional[0].span, - // destination_message: "into single file".to_string(), - // destination_span: call.positional[1].span, - // }); - // } - - // let any_source_is_dir = sources.iter().any(|f| matches!(f, Ok(f) if f.is_dir())); - // let recursive: bool = call.has_flag("recursive"); - // if any_source_is_dir && !recursive { - // return Err(ShellError::MoveNotPossibleSingle( - // "Directories must be copied using \"--recursive\"".to_string(), - // call.positional[0].span, - // )); - // } - - // if interactive && !force { - // let mut remove: Vec = vec![]; - // for (index, file) in sources.iter().enumerate() { - // let prompt = format!( - // "Are you shure that you want to copy {} to {}?", - // file.as_ref() - // .map_err(|err| ShellError::SpannedLabeledError( - // "Reference error".into(), - // err.to_string(), - // call.head - // ))? - // .file_name() - // .ok_or_else(|| ShellError::SpannedLabeledError( - // "File name error".into(), - // "Unable to get file name".into(), - // call.head - // ))? - // .to_str() - // .ok_or_else(|| ShellError::SpannedLabeledError( - // "Unable to get str error".into(), - // "Unable to convert to str file name".into(), - // call.head - // ))?, - // destination - // .file_name() - // .ok_or_else(|| ShellError::SpannedLabeledError( - // "File name error".into(), - // "Unable to get file name".into(), - // call.head - // ))? - // .to_str() - // .ok_or_else(|| ShellError::SpannedLabeledError( - // "Unable to get str error".into(), - // "Unable to convert to str file name".into(), - // call.head - // ))?, - // ); - - // let input = get_interactive_confirmation(prompt)?; - - // if !input { - // remove.push(index); - // } - // } - - // remove.reverse(); - - // for index in remove { - // sources.remove(index); - // } - - // if sources.is_empty() { - // return Err(ShellError::NoFileToBeCopied()); - // } - // } - - // for entry in sources.into_iter().flatten() { - // let mut sources = FileStructure::new(); - // sources.walk_decorate(&entry, engine_state, stack)?; - - // if entry.is_file() { - // let sources = sources.paths_applying_with(|(source_file, _depth_level)| { - // if destination.is_dir() { - // let mut dest = canonicalize_with(&destination, &path)?; - // if let Some(name) = entry.file_name() { - // dest.push(name); - // } - // Ok((source_file, dest)) - // } else { - // Ok((source_file, destination.clone())) - // } - // })?; - - // for (src, dst) in sources { - // if src.is_file() { - // std::fs::copy(&src, dst).map_err(|e| { - // ShellError::MoveNotPossibleSingle( - // format!( - // "failed to move containing file \"{}\": {}", - // src.to_string_lossy(), - // e - // ), - // call.positional[0].span, - // ) - // })?; - // } - // } - // } else if entry.is_dir() { - // let destination = if !destination.exists() { - // destination.clone() - // } else { - // match entry.file_name() { - // Some(name) => destination.join(name), - // None => { - // return Err(ShellError::FileNotFoundCustom( - // format!("containing \"{:?}\" is not a valid path", entry), - // call.positional[0].span, - // )) - // } - // } - // }; - - // std::fs::create_dir_all(&destination).map_err(|e| { - // ShellError::MoveNotPossibleSingle( - // format!("failed to recursively fill destination: {}", e), - // call.positional[1].span, - // ) - // })?; - - // let sources = sources.paths_applying_with(|(source_file, depth_level)| { - // let mut dest = destination.clone(); - // let path = canonicalize_with(&source_file, &path)?; - // let components = path - // .components() - // .map(|fragment| fragment.as_os_str()) - // .rev() - // .take(1 + depth_level); - - // components.for_each(|fragment| dest.push(fragment)); - // Ok((PathBuf::from(&source_file), dest)) - // })?; - - // for (src, dst) in sources { - // if src.is_dir() && !dst.exists() { - // std::fs::create_dir_all(&dst).map_err(|e| { - // ShellError::MoveNotPossibleSingle( - // format!( - // "failed to create containing directory \"{}\": {}", - // dst.to_string_lossy(), - // e - // ), - // call.positional[1].span, - // ) - // })?; - // } - - // if src.is_file() { - // std::fs::copy(&src, &dst).map_err(|e| { - // ShellError::MoveNotPossibleSingle( - // format!( - // "failed to move containing file \"{}\": {}", - // src.to_string_lossy(), - // e - // ), - // call.positional[0].span, - // ) - // })?; - // } - // } - // } - // } - - // Ok(PipelineData::new(call.head)) - // } } fn interactive_copy_file(interactive: bool, src: PathBuf, dst: PathBuf, span: Span) -> Value { diff --git a/crates/nu-command/tests/commands/cp.rs b/crates/nu-command/tests/commands/cp.rs index e49f519ef1..3fb0b97490 100644 --- a/crates/nu-command/tests/commands/cp.rs +++ b/crates/nu-command/tests/commands/cp.rs @@ -235,3 +235,17 @@ fn copy_file_and_dir_from_two_parents_up_using_multiple_dots_to_current_dir_recu assert!(files_exist_at(vec!["hello_there", "hello_again"], expected)); }) } + +#[test] +fn copy_to_non_existing_dir() { + Playground::setup("cp_test_11", |_dirs, sandbox| { + sandbox.with_files(vec![EmptyFile("empty_file")]); + + let actual = nu!( + cwd: sandbox.cwd(), + "cp empty_file ~/not_a_dir/", + ); + assert!(actual.err.contains("directory not found")); + assert!(actual.err.contains("destination directory does not exist")); + }); +} diff --git a/crates/nu-path/src/tilde.rs b/crates/nu-path/src/tilde.rs index c51ea52d03..88d733ab28 100644 --- a/crates/nu-path/src/tilde.rs +++ b/crates/nu-path/src/tilde.rs @@ -14,6 +14,9 @@ fn expand_tilde_with_home(path: impl AsRef, home: Option) -> Path }; } + let path_last_char = path.as_os_str().to_string_lossy().chars().last(); + let need_trailing_slash = path_last_char == Some('/') || path_last_char == Some('\\'); + match home { None => path.into(), Some(mut h) => { @@ -31,6 +34,10 @@ fn expand_tilde_with_home(path: impl AsRef, home: Option) -> Path if p != Path::new("") { h.push(p) } + + if need_trailing_slash { + h.push(""); + } } h }