From efa89de4636220d94ba9d077a4f2f0d2f2e64029 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Wed, 2 Jun 2021 19:58:29 +0200 Subject: [PATCH 1/5] ln: fix LINK_NAME in help output --- src/uu/ln/src/ln.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index 2a14f3c9c..950270872 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -54,7 +54,7 @@ pub enum BackupMode { fn get_usage() -> String { format!( - "{0} [OPTION]... [-T] TARGET LINK_executable!() (1st form) + "{0} [OPTION]... [-T] TARGET LINK_NAME (1st form) {0} [OPTION]... TARGET (2nd form) {0} [OPTION]... TARGET... DIRECTORY (3rd form) {0} [OPTION]... -t DIRECTORY TARGET... (4th form)", @@ -64,7 +64,7 @@ fn get_usage() -> String { fn get_long_usage() -> String { String::from( - " In the 1st form, create a link to TARGET with the name LINK_executable!(). + " In the 1st form, create a link to TARGET with the name LINK_NAME. In the 2nd form, create a link to TARGET in the current directory. In the 3rd and 4th forms, create links to each TARGET in DIRECTORY. Create hard links by default, symbolic links with --symbolic. @@ -144,7 +144,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .short("n") .long(OPT_NO_DEREFERENCE) .help( - "treat LINK_executable!() as a normal file if it is a \ + "treat LINK_NAME as a normal file if it is a \ symbolic link to a directory", ), ) @@ -179,7 +179,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { Arg::with_name(OPT_NO_TARGET_DIRECTORY) .short("T") .long(OPT_NO_TARGET_DIRECTORY) - .help("treat LINK_executable!() as a normal file always"), + .help("treat LINK_NAME as a normal file always"), ) .arg( Arg::with_name(OPT_RELATIVE) From 87570bbc1041b211b30b1f72fe661d4b497b8da2 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Wed, 2 Jun 2021 20:56:37 +0200 Subject: [PATCH 2/5] ln: remove redundant `force` flag This information is already encoded in the `OverwriteMode` enum. --- src/uu/ln/src/ln.rs | 82 ++++++++++++++++++++++----------------------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index 950270872..0c60405f6 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -27,7 +27,6 @@ use uucore::fs::{canonicalize, CanonicalizeMode}; pub struct Settings { overwrite: OverwriteMode, backup: BackupMode, - force: bool, suffix: String, symbolic: bool, relative: bool, @@ -244,7 +243,6 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let settings = Settings { overwrite: overwrite_mode, backup: backup_mode, - force: matches.is_present(OPT_FORCE), suffix: backup_suffix.to_string(), symbolic: matches.is_present(OPT_SYMBOLIC), relative: matches.is_present(OPT_RELATIVE), @@ -311,47 +309,48 @@ fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings) let mut all_successful = true; for srcpath in files.iter() { - let targetpath = if settings.no_dereference && settings.force { - // In that case, we don't want to do link resolution - // We need to clean the target - if is_symlink(target_dir) { - if target_dir.is_file() { - if let Err(e) = fs::remove_file(target_dir) { - show_error!("Could not update {}: {}", target_dir.display(), e) - }; - } - if target_dir.is_dir() { - // Not sure why but on Windows, the symlink can be - // considered as a dir - // See test_ln::test_symlink_no_deref_dir - if let Err(e) = fs::remove_dir(target_dir) { - show_error!("Could not update {}: {}", target_dir.display(), e) - }; - } - } - target_dir.to_path_buf() - } else { - match srcpath.as_os_str().to_str() { - Some(name) => { - match Path::new(name).file_name() { - Some(basename) => target_dir.join(basename), - // This can be None only for "." or "..". Trying - // to create a link with such name will fail with - // EEXIST, which agrees with the behavior of GNU - // coreutils. - None => target_dir.join(name), + let targetpath = + if settings.no_dereference && matches!(settings.overwrite, OverwriteMode::Force) { + // In that case, we don't want to do link resolution + // We need to clean the target + if is_symlink(target_dir) { + if target_dir.is_file() { + if let Err(e) = fs::remove_file(target_dir) { + show_error!("Could not update {}: {}", target_dir.display(), e) + }; + } + if target_dir.is_dir() { + // Not sure why but on Windows, the symlink can be + // considered as a dir + // See test_ln::test_symlink_no_deref_dir + if let Err(e) = fs::remove_dir(target_dir) { + show_error!("Could not update {}: {}", target_dir.display(), e) + }; } } - None => { - show_error!( - "cannot stat '{}': No such file or directory", - srcpath.display() - ); - all_successful = false; - continue; + target_dir.to_path_buf() + } else { + match srcpath.as_os_str().to_str() { + Some(name) => { + match Path::new(name).file_name() { + Some(basename) => target_dir.join(basename), + // This can be None only for "." or "..". Trying + // to create a link with such name will fail with + // EEXIST, which agrees with the behavior of GNU + // coreutils. + None => target_dir.join(name), + } + } + None => { + show_error!( + "cannot stat '{}': No such file or directory", + srcpath.display() + ); + all_successful = false; + continue; + } } - } - }; + }; if let Err(e) = link(srcpath, &targetpath, settings) { show_error!( @@ -422,7 +421,8 @@ fn link(src: &Path, dst: &Path, settings: &Settings) -> Result<()> { } } - if settings.no_dereference && settings.force && dst.exists() { + if settings.no_dereference && matches!(settings.overwrite, OverwriteMode::Force) && dst.exists() + { fs::remove_file(dst)?; } From ed69d797b526bf4a01f676d591a688664cb1b281 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Wed, 2 Jun 2021 21:02:12 +0200 Subject: [PATCH 3/5] ln: reject --relative without --symbolic --- src/uu/ln/src/ln.rs | 3 ++- tests/by-util/test_ln.rs | 19 +++++-------------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index 0c60405f6..4bd3310cc 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -184,7 +184,8 @@ pub fn uumain(args: impl uucore::Args) -> i32 { Arg::with_name(OPT_RELATIVE) .short("r") .long(OPT_RELATIVE) - .help("create symbolic links relative to link location"), + .help("create symbolic links relative to link location") + .requires(OPT_SYMBOLIC), ) .arg( Arg::with_name(OPT_VERBOSE) diff --git a/tests/by-util/test_ln.rs b/tests/by-util/test_ln.rs index e475e3608..00ea85ecd 100644 --- a/tests/by-util/test_ln.rs +++ b/tests/by-util/test_ln.rs @@ -428,20 +428,6 @@ fn test_symlink_relative() { assert_eq!(at.resolve_link(link), file_a); } -#[test] -fn test_hardlink_relative() { - let (at, mut ucmd) = at_and_ucmd!(); - let file_a = "test_hardlink_relative_a"; - let link = "test_hardlink_relative_link"; - - at.touch(file_a); - - // relative hardlink - ucmd.args(&["-r", "-v", file_a, link]) - .succeeds() - .stdout_only(format!("'{}' -> '{}'\n", link, file_a)); -} - #[test] fn test_symlink_relative_path() { let (at, mut ucmd) = at_and_ucmd!(); @@ -571,3 +557,8 @@ fn test_symlink_no_deref_file() { assert!(at.is_symlink(link)); assert_eq!(at.resolve_link(link), file1); } + +#[test] +fn test_relative_requires_symbolic() { + new_ucmd!().args(&["-r", "foo", "bar"]).fails(); +} From af8f47ea6a6c21836f88032217932cb15fba5ecb Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Thu, 3 Jun 2021 15:05:20 +0200 Subject: [PATCH 4/5] ln: remove redundant check if `dst.exists()` and `settings.overwrite` is `OverwriteMode::Force`, we already delete the file in the match above. --- src/uu/ln/src/ln.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index 4bd3310cc..4087716bd 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -422,11 +422,6 @@ fn link(src: &Path, dst: &Path, settings: &Settings) -> Result<()> { } } - if settings.no_dereference && matches!(settings.overwrite, OverwriteMode::Force) && dst.exists() - { - fs::remove_file(dst)?; - } - if settings.symbolic { symlink(&source, dst)?; } else { From 6c46d09397759118343a3e7fdd21310aa99debac Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Wed, 2 Jun 2021 21:27:08 +0200 Subject: [PATCH 5/5] ln: canonicalize the parent directory of dst, not dst dst may or may not exist. In case it exists it might already be a symlink. In neither case we should try to canonicalize dst, only its parent directory. https://www.gnu.org/software/coreutils/manual/html_node/ln-invocation.html > Relative symbolic links are generated based on their canonicalized > **containing directory**, and canonicalized targets. --- src/uu/ln/src/ln.rs | 3 ++- tests/by-util/test_ln.rs | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index 4087716bd..33ec5f449 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -372,7 +372,8 @@ fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings) fn relative_path<'a>(src: &Path, dst: &Path) -> Result> { let src_abs = canonicalize(src, CanonicalizeMode::Normal)?; - let dst_abs = canonicalize(dst, CanonicalizeMode::Normal)?; + let mut dst_abs = canonicalize(dst.parent().unwrap(), CanonicalizeMode::Normal)?; + dst_abs.push(dst.components().last().unwrap()); let suffix_pos = src_abs .components() .zip(dst_abs.components()) diff --git a/tests/by-util/test_ln.rs b/tests/by-util/test_ln.rs index 00ea85ecd..fc97ff779 100644 --- a/tests/by-util/test_ln.rs +++ b/tests/by-util/test_ln.rs @@ -562,3 +562,21 @@ fn test_symlink_no_deref_file() { fn test_relative_requires_symbolic() { new_ucmd!().args(&["-r", "foo", "bar"]).fails(); } + +#[test] +fn test_relative_dst_already_symlink() { + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("file1"); + at.symlink_file("file1", "file2"); + ucmd.arg("-srf").arg("file1").arg("file2").succeeds(); + at.is_symlink("file2"); +} + +#[test] +fn test_relative_src_already_symlink() { + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("file1"); + at.symlink_file("file1", "file2"); + ucmd.arg("-sr").arg("file2").arg("file3").succeeds(); + assert!(at.resolve_link("file3").ends_with("file1")); +}