From 131e2cfb4ce22348a97dad41d8bf4842796165f8 Mon Sep 17 00:00:00 2001 From: jansheikkinen Date: Wed, 14 Feb 2024 15:59:49 -0600 Subject: [PATCH 1/5] ln: add test for -snf --- tests/by-util/test_ln.rs | 58 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tests/by-util/test_ln.rs b/tests/by-util/test_ln.rs index 36baef640de..ed0a8a41c18 100644 --- a/tests/by-util/test_ln.rs +++ b/tests/by-util/test_ln.rs @@ -549,6 +549,64 @@ fn test_symlink_no_deref_dir() { assert_eq!(at.resolve_link(link), dir1); } +#[test] +fn test_symlink_no_deref_file_in_destination_dir() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let file1 = "foo"; + let file2 = "bar"; + + let dest = "baz"; + + let link1 = "baz/foo"; + let link2 = "baz/bar"; + + at.touch(file1); + at.touch(file2); + at.mkdir(dest); + + assert!(at.file_exists(file1)); + assert!(at.file_exists(file2)); + assert!(at.dir_exists(dest)); + + // -n and -f should work alone + scene + .ucmd() + .args(&["-sn", file1, dest]) + .succeeds() + .no_stderr(); + assert!(at.is_symlink(link1)); + assert_eq!(at.resolve_link(link1), file1); + + scene + .ucmd() + .args(&["-sf", file1, dest]) + .succeeds() + .no_stderr(); + assert!(at.is_symlink(link1)); + assert_eq!(at.resolve_link(link1), file1); + + // -nf should also work + scene + .ucmd() + .args(&["-snf", file1, dest]) + .succeeds() + .no_stderr(); + assert!(at.is_symlink(link1)); + assert_eq!(at.resolve_link(link1), file1); + + scene + .ucmd() + .args(&["-snf", file1, file2, dest]) + .succeeds() + .no_stderr(); + assert!(at.is_symlink(link1)); + assert_eq!(at.resolve_link(link1), file1); + assert!(at.is_symlink(link2)); + assert_eq!(at.resolve_link(link2), file2); +} + #[test] fn test_symlink_no_deref_file() { let scene = TestScenario::new(util_name!()); From eb5a1146844eb566cb22c0b8e8deba372ddc40cb Mon Sep 17 00:00:00 2001 From: jansheikkinen Date: Wed, 14 Feb 2024 16:11:16 -0600 Subject: [PATCH 2/5] ln: also check that -n fails if exists --- tests/by-util/test_ln.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/by-util/test_ln.rs b/tests/by-util/test_ln.rs index ed0a8a41c18..f869fcc0310 100644 --- a/tests/by-util/test_ln.rs +++ b/tests/by-util/test_ln.rs @@ -587,6 +587,9 @@ fn test_symlink_no_deref_file_in_destination_dir() { assert!(at.is_symlink(link1)); assert_eq!(at.resolve_link(link1), file1); + // -n alone should fail if destination exists already (it should now) + scene.ucmd().args(&["-sn", file1, dest]).fails(); + // -nf should also work scene .ucmd() From abae5d542f41cc2cf5085f2b67c42f4f82e311cf Mon Sep 17 00:00:00 2001 From: jansheikkinen Date: Wed, 14 Feb 2024 17:01:24 -0600 Subject: [PATCH 3/5] ln: new test passes now but code needs cleanup --- src/uu/ln/src/ln.rs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index a056ee256a1..bc56520bfc4 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -300,6 +300,7 @@ fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings) let mut linked_destinations: HashSet = HashSet::with_capacity(files.len()); let mut all_successful = true; + for srcpath in files { let targetpath = if settings.no_dereference && matches!(settings.overwrite, OverwriteMode::Force) { @@ -319,8 +320,30 @@ fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings) show_error!("Could not update {}: {}", target_dir.quote(), e); }; } + target_dir.to_path_buf() + } else { + if target_dir.is_dir() { + 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.quote()); + all_successful = false; + continue; + } + } + } else { + target_dir.to_path_buf() + } } - target_dir.to_path_buf() } else { match srcpath.as_os_str().to_str() { Some(name) => { From cdfc8a542dea1b65a4e6d27dfea4fa09f6669da3 Mon Sep 17 00:00:00 2001 From: jansheikkinen Date: Wed, 14 Feb 2024 17:22:38 -0600 Subject: [PATCH 4/5] ln: clean up --- src/uu/ln/src/ln.rs | 56 ++++++++++++++------------------------------- 1 file changed, 17 insertions(+), 39 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index bc56520bfc4..5ee94c8449a 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -300,50 +300,28 @@ fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings) let mut linked_destinations: HashSet = HashSet::with_capacity(files.len()); let mut all_successful = true; - for srcpath in files { let targetpath = - if settings.no_dereference && matches!(settings.overwrite, OverwriteMode::Force) { + if settings.no_dereference + && matches!(settings.overwrite, OverwriteMode::Force) + && target_dir.is_symlink() + { // In that case, we don't want to do link resolution // We need to clean the target - if target_dir.is_symlink() { - if target_dir.is_file() { - if let Err(e) = fs::remove_file(target_dir) { - show_error!("Could not update {}: {}", target_dir.quote(), 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.quote(), e); - }; - } - target_dir.to_path_buf() - } else { - if target_dir.is_dir() { - 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.quote()); - all_successful = false; - continue; - } - } - } else { - target_dir.to_path_buf() - } + if target_dir.is_file() { + if let Err(e) = fs::remove_file(target_dir) { + show_error!("Could not update {}: {}", target_dir.quote(), 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.quote(), e); + }; } + target_dir.to_path_buf() } else { match srcpath.as_os_str().to_str() { Some(name) => { From 10e9b3adb6de07ecd35dface03410ef90d141d0e Mon Sep 17 00:00:00 2001 From: jansheikkinen Date: Thu, 15 Feb 2024 08:12:15 -0600 Subject: [PATCH 5/5] ln: run rustfmt --- src/uu/ln/src/ln.rs | 75 ++++++++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 38 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index 5ee94c8449a..a19e137e7db 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -301,46 +301,45 @@ fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings) let mut all_successful = true; for srcpath in files { - let targetpath = - if settings.no_dereference - && matches!(settings.overwrite, OverwriteMode::Force) - && target_dir.is_symlink() - { - // In that case, we don't want to do link resolution - // We need to clean the target - if target_dir.is_file() { - if let Err(e) = fs::remove_file(target_dir) { - show_error!("Could not update {}: {}", target_dir.quote(), 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.quote(), 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), - } - } - None => { - show_error!("cannot stat {}: No such file or directory", srcpath.quote()); - all_successful = false; - continue; + let targetpath = if settings.no_dereference + && matches!(settings.overwrite, OverwriteMode::Force) + && target_dir.is_symlink() + { + // In that case, we don't want to do link resolution + // We need to clean the target + if target_dir.is_file() { + if let Err(e) = fs::remove_file(target_dir) { + show_error!("Could not update {}: {}", target_dir.quote(), 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.quote(), 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), } } - }; + None => { + show_error!("cannot stat {}: No such file or directory", srcpath.quote()); + all_successful = false; + continue; + } + } + }; if linked_destinations.contains(&targetpath) { // If the target file was already created in this ln call, do not overwrite