From d556c9e3989ed3dc24a6e73e54971a1a6da4417e Mon Sep 17 00:00:00 2001 From: Shinichiro Hamaji Date: Fri, 7 Apr 2017 13:10:24 +0900 Subject: [PATCH] ln: Fix how it selects the form ln had a bunch of problems: 1. `ln -s target` didn't work (2nd form in help). 2. `ln -t tmp` wasn't an error. We should check if files are empty first. 3. `ln -s file dir` didn't create dir/file. 4. `ln -s -T file dir` was removing `dir`. 5. Test cases for 4 say this is for compatibility with GNU coreutils but I couldn't find this feature. --- src/ln/ln.rs | 72 ++++++++++++++++++++------------------- tests/test_ln.rs | 87 +++++++++++++++++++++++++++++++----------------- 2 files changed, 94 insertions(+), 65 deletions(-) diff --git a/src/ln/ln.rs b/src/ln/ln.rs index 339efba05..c14831bb4 100644 --- a/src/ln/ln.rs +++ b/src/ln/ln.rs @@ -143,39 +143,45 @@ pub fn uumain(args: Vec) -> i32 { } fn exec(files: &[PathBuf], settings: &Settings) -> i32 { - match settings.target_dir { - Some(ref name) => return link_files_in_dir(files, &PathBuf::from(name), &settings), - None => {} + if files.len() == 0 { + show_error!("missing file operand\nTry '{} --help' for more information.", NAME); + return 1; } - match files.len() { - 0 => { - show_error!("missing file operand\nTry '{} --help' for more information.", NAME); + + // Handle cases where we create links in a directory first. + if let Some(ref name) = settings.target_dir { + // 4th form: a directory is specified by -t. + return link_files_in_dir(files, &PathBuf::from(name), &settings); + } + if !settings.no_target_dir { + if files.len() == 1 { + // 2nd form: the target directory is the current directory. + return link_files_in_dir(files, &PathBuf::from("."), &settings); + } + let last_file = &PathBuf::from(files.last().unwrap()); + if files.len() > 2 || last_file.is_dir() { + // 3rd form: create links in the last argument. + return link_files_in_dir(&files[0..files.len()-1], last_file, &settings); + } + } + + // 1st form. Now there should be only two operands, but if -T is + // specified we may have a wrong number of operands. + if files.len() == 1 { + show_error!("missing destination file operand after '{}'", files[0].to_string_lossy()); + return 1; + } + if files.len() > 2 { + show_error!("extra operand '{}'\nTry '{} --help' for more information.", files[2].display(), NAME); + return 1; + } + assert!(files.len() != 0); + + match link(&files[0], &files[1], settings) { + Ok(_) => 0, + Err(e) => { + show_error!("{}", e); 1 - }, - 1 => match link(&files[0], &files[0], settings) { - Ok(_) => 0, - Err(e) => { - show_error!("{}", e); - 1 - } - }, - 2 => match link(&files[0], &files[1], settings) { - Ok(_) => 0, - Err(e) => { - show_error!("{}", e); - 1 - } - }, - _ => { - if settings.no_target_dir { - show_error!("extra operand '{}'\nTry '{} --help' for more information.", files[2].display(), NAME); - return 1; - } - let (targets, dir) = match settings.target_dir { - Some(ref dir) => (files, PathBuf::from(dir.clone())), - None => (&files[0..files.len()-1], files[files.len()-1].clone()) - }; - link_files_in_dir(targets, &dir, settings) } } } @@ -219,10 +225,6 @@ fn link_files_in_dir(files: &[PathBuf], target_dir: &PathBuf, settings: &Setting fn link(src: &PathBuf, dst: &PathBuf, settings: &Settings) -> Result<()> { let mut backup_path = None; - if dst.is_dir() && settings.no_target_dir { - try!(fs::remove_dir(dst)); - } - if is_symlink(dst) || dst.exists() { match settings.overwrite { OverwriteMode::NoClobber => {}, diff --git a/tests/test_ln.rs b/tests/test_ln.rs index fad8ad4ce..25465f88a 100644 --- a/tests/test_ln.rs +++ b/tests/test_ln.rs @@ -274,7 +274,7 @@ fn test_symlink_target_dir_from_dir() { } #[test] -fn test_symlink_overwrite_dir() { +fn test_symlink_overwrite_dir_fail() { let (at, mut ucmd) = at_and_ucmd!(); let path_a = "test_symlink_overwrite_dir_a"; let path_b = "test_symlink_overwrite_dir_b"; @@ -282,35 +282,7 @@ fn test_symlink_overwrite_dir() { at.touch(path_a); at.mkdir(path_b); - ucmd.args(&["-s", "-T", path_a, path_b]).succeeds().no_stderr(); - - assert!(at.file_exists(path_a)); - assert!(at.is_symlink(path_b)); - assert_eq!(at.resolve_link(path_b), path_a); -} - -#[test] -fn test_symlink_overwrite_nonempty_dir() { - let (at, mut ucmd) = at_and_ucmd!(); - let path_a = "test_symlink_overwrite_nonempty_dir_a"; - let path_b = "test_symlink_overwrite_nonempty_dir_b"; - let dummy = "test_symlink_overwrite_nonempty_dir_b/file"; - - at.touch(path_a); - at.mkdir(path_b); - at.touch(dummy); - - // Not same error as GNU; the error message is a Rust builtin - // TODO: test (and implement) correct error message (or at least decide whether to do so) - // Current: "ln: error: Directory not empty (os error 66)" - // GNU: "ln: cannot link 'a' to 'b': Directory not empty" - - // Verbose output for the link should not be shown on failure - assert!(ucmd.args(&["-v", "-T", "-s", path_a, path_b]) - .fails().no_stdout().stderr.len() > 0); - - assert!(at.file_exists(path_a)); - assert!(at.dir_exists(path_b)); + assert!(ucmd.args(&["-s", "-T", path_a, path_b]).fails().stderr.len() > 0); } #[test] @@ -348,3 +320,58 @@ fn test_symlink_verbose() { scene.ucmd().args(&["-v", "-b", file_a, file_b]) .succeeds().stdout_only(format!("'{}' -> '{}' (backup: '{}~')\n", file_b, file_a, file_b)); } + +#[test] +fn test_symlink_target_only() { + let (at, mut ucmd) = at_and_ucmd!(); + let dir = "test_symlink_target_only"; + + at.mkdir(dir); + + assert!(ucmd.args(&["-s", "-t", dir]).fails().stderr.len() > 0); +} + +#[test] +fn test_symlink_implicit_target_dir() { + let (at, mut ucmd) = at_and_ucmd!(); + let dir = "test_symlink_implicit_target_dir"; + let file = "test_symlink_implicit_target_dir/file"; + + at.mkdir(dir); + at.touch(file); + + ucmd.args(&["-s", file]).succeeds().no_stderr(); + + assert!(at.file_exists("file")); + assert!(at.is_symlink("file")); + assert_eq!(at.resolve_link("file"), file); +} + +#[test] +fn test_symlink_to_dir_2args() { + let (at, mut ucmd) = at_and_ucmd!(); + let filename = "test_symlink_to_dir_2args_file"; + let from_file = &format!("{}/{}", at.as_string(), filename); + let to_dir = "test_symlink_to_dir_2args_to_dir"; + let to_file = &format!("{}/{}", to_dir, filename); + + at.mkdir(to_dir); + at.touch(from_file); + + ucmd.args(&["-s", from_file, to_dir]).succeeds().no_stderr(); + + assert!(at.file_exists(to_file)); + assert!(at.is_symlink(to_file)); + assert_eq!(at.resolve_link(to_file), filename); +} + +#[test] +fn test_symlink_missing_destination() { + let (at, mut ucmd) = at_and_ucmd!(); + let file = "test_symlink_missing_destination"; + + at.touch(file); + + ucmd.args(&["-s", "-T", file]).fails() + .stderr_is(format!("ln: error: missing destination file operand after '{}'", file)); +}