From 15ef76fcbf9c50c7034f4c49ce94927b1afeb49a Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 31 Mar 2022 00:08:21 +0200 Subject: [PATCH 1/5] ln: when we get a failed to link, show which files --- 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 c09f7a606..08eef6ae7 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -53,7 +53,7 @@ pub enum OverwriteMode { enum LnError { TargetIsDirectory(PathBuf), SomeLinksFailed, - FailedToLink(String), + FailedToLink(PathBuf, PathBuf, String), SameFile(PathBuf, PathBuf), MissingDestination(PathBuf), ExtraOperand(OsString), @@ -63,7 +63,7 @@ impl Display for LnError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::TargetIsDirectory(s) => write!(f, "target {} is not a directory", s.quote()), - Self::FailedToLink(e) => write!(f, "failed to link: {}", e), + Self::FailedToLink(s, d, e) => write!(f, "failed to link {} to {}: {}", s.quote(), d.quote(), e), Self::SameFile(e, e2) => write!( f, "'{}' and '{}' are the same file", @@ -91,7 +91,7 @@ impl UError for LnError { match self { Self::TargetIsDirectory(_) | Self::SomeLinksFailed - | Self::FailedToLink(_) + | Self::FailedToLink(_, _, _) | Self::SameFile(_, _) | Self::MissingDestination(_) | Self::ExtraOperand(_) => 1, @@ -293,7 +293,7 @@ fn exec(files: &[PathBuf], settings: &Settings) -> UResult<()> { match link(&files[0], &files[1], settings) { Ok(_) => Ok(()), - Err(e) => Err(LnError::FailedToLink(e.to_string()).into()), + Err(e) => Err(LnError::FailedToLink(files[0].to_owned(), files[1].to_owned(), e.to_string()).into()), } } From 526370f922d340c3c55e312f31838cccc81db955 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 31 Mar 2022 00:11:02 +0200 Subject: [PATCH 2/5] ln: use the quote method instead of doing it by hand --- src/uu/ln/src/ln.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index 08eef6ae7..0d9d8579e 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -66,9 +66,9 @@ impl Display for LnError { Self::FailedToLink(s, d, e) => write!(f, "failed to link {} to {}: {}", s.quote(), d.quote(), e), Self::SameFile(e, e2) => write!( f, - "'{}' and '{}' are the same file", - e2.display(), - e.display() + "{} and {} are the same file", + e2.quote(), + e.quote() ), Self::SomeLinksFailed => write!(f, "some links failed to create"), Self::MissingDestination(s) => { From e5532417502e939df5bb4d21d515d1c9e9ae995f Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 31 Mar 2022 00:27:21 +0200 Subject: [PATCH 3/5] ln: rustfmt the recent changes --- src/uu/ln/src/ln.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index 0d9d8579e..4d0495e6d 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -63,13 +63,12 @@ impl Display for LnError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::TargetIsDirectory(s) => write!(f, "target {} is not a directory", s.quote()), - Self::FailedToLink(s, d, e) => write!(f, "failed to link {} to {}: {}", s.quote(), d.quote(), e), - Self::SameFile(e, e2) => write!( - f, - "{} and {} are the same file", - e2.quote(), - e.quote() - ), + Self::FailedToLink(s, d, e) => { + write!(f, "failed to link {} to {}: {}", s.quote(), d.quote(), e) + } + Self::SameFile(e, e2) => { + write!(f, "{} and {} are the same file", e2.quote(), e.quote()) + } Self::SomeLinksFailed => write!(f, "some links failed to create"), Self::MissingDestination(s) => { write!(f, "missing destination file operand after {}", s.quote()) @@ -293,7 +292,12 @@ fn exec(files: &[PathBuf], settings: &Settings) -> UResult<()> { match link(&files[0], &files[1], settings) { Ok(_) => Ok(()), - Err(e) => Err(LnError::FailedToLink(files[0].to_owned(), files[1].to_owned(), e.to_string()).into()), + Err(e) => { + Err( + LnError::FailedToLink(files[0].to_owned(), files[1].to_owned(), e.to_string()) + .into(), + ) + } } } From 676283ce93b8fcba81a4e2ab6088e16a86f2f7fc Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 31 Mar 2022 00:27:59 +0200 Subject: [PATCH 4/5] ln: implement fixes for tests/ln/backup-1.sh When doing ln b b~ ln -f --b=simple a b First, we create a backup of b Then, we force the override of a => b but we make sure that the backup is done. So, we had a bug in the ordering of the actions. we were first removing b. Therefore, losing the capability to do a backup of this. --- src/uu/ln/src/ln.rs | 28 ++++++++++++++++------------ tests/by-util/test_ln.rs | 26 ++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index 4d0495e6d..378c89872 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -402,18 +402,6 @@ fn link(src: &Path, dst: &Path, settings: &Settings) -> UResult<()> { }; if is_symlink(dst) || dst.exists() { - match settings.overwrite { - OverwriteMode::NoClobber => {} - OverwriteMode::Interactive => { - print!("{}: overwrite {}? ", uucore::util_name(), dst.quote()); - if !read_yes() { - return Ok(()); - } - fs::remove_file(dst)?; - } - OverwriteMode::Force => fs::remove_file(dst)?, - }; - backup_path = match settings.backup { BackupMode::NoBackup => None, BackupMode::SimpleBackup => Some(simple_backup_path(dst, &settings.suffix)), @@ -435,6 +423,22 @@ fn link(src: &Path, dst: &Path, settings: &Settings) -> UResult<()> { if let Some(ref p) = backup_path { fs::rename(dst, p)?; } + match settings.overwrite { + OverwriteMode::NoClobber => {} + OverwriteMode::Interactive => { + print!("{}: overwrite {}? ", uucore::util_name(), dst.quote()); + if !read_yes() { + return Ok(()); + } + + if fs::remove_file(dst).is_ok() {}; + // In case of error, don't do anything + } + OverwriteMode::Force => { + if fs::remove_file(dst).is_ok() {}; + // In case of error, don't do anything + } + }; } if settings.symbolic { diff --git a/tests/by-util/test_ln.rs b/tests/by-util/test_ln.rs index 9bda5d412..f54ac5f39 100644 --- a/tests/by-util/test_ln.rs +++ b/tests/by-util/test_ln.rs @@ -624,3 +624,29 @@ fn test_backup_same_file() { .fails() .stderr_contains("'file1' and './file1' are the same file"); } + +#[test] +fn test_backup_force() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.write("a", "a\n"); + at.write("b", "b2\n"); + + scene.ucmd().args(&["-s", "b", "b~"]).succeeds().no_stderr(); + assert!(at.file_exists("a")); + assert!(at.file_exists("b")); + assert!(at.file_exists("b~")); + scene + .ucmd() + .args(&["-f", "--b=simple", "a", "b"]) + .succeeds() + .no_stderr(); + assert!(at.file_exists("a")); + assert!(at.file_exists("b")); + assert!(at.file_exists("b~")); + assert_eq!(at.read("a"), "a\n"); + assert_eq!(at.read("b"), "a\n"); + // we should have the same content as b as we had time to do a backup + assert_eq!(at.read("b~"), "b2\n"); +} From b3d87b088d3b5ba5f881d4377bab4ab53b5db707 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 31 Mar 2022 13:45:41 +0200 Subject: [PATCH 5/5] ln: adjust the error messages --- src/uu/ln/src/ln.rs | 10 +++++----- tests/by-util/test_ln.rs | 2 +- util/build-gnu.sh | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index 378c89872..088edf219 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -54,7 +54,7 @@ enum LnError { TargetIsDirectory(PathBuf), SomeLinksFailed, FailedToLink(PathBuf, PathBuf, String), - SameFile(PathBuf, PathBuf), + SameFile(), MissingDestination(PathBuf), ExtraOperand(OsString), } @@ -66,8 +66,8 @@ impl Display for LnError { Self::FailedToLink(s, d, e) => { write!(f, "failed to link {} to {}: {}", s.quote(), d.quote(), e) } - Self::SameFile(e, e2) => { - write!(f, "{} and {} are the same file", e2.quote(), e.quote()) + Self::SameFile() => { + write!(f, "Same file") } Self::SomeLinksFailed => write!(f, "some links failed to create"), Self::MissingDestination(s) => { @@ -91,7 +91,7 @@ impl UError for LnError { Self::TargetIsDirectory(_) | Self::SomeLinksFailed | Self::FailedToLink(_, _, _) - | Self::SameFile(_, _) + | Self::SameFile() | Self::MissingDestination(_) | Self::ExtraOperand(_) => 1, } @@ -417,7 +417,7 @@ fn link(src: &Path, dst: &Path, settings: &Settings) -> UResult<()> { ResolveMode::Logical, )?; if dst_abs == source_abs { - return Err(LnError::SameFile(dst.to_path_buf(), source.to_path_buf()).into()); + return Err(LnError::SameFile().into()); } } if let Some(ref p) = backup_path { diff --git a/tests/by-util/test_ln.rs b/tests/by-util/test_ln.rs index f54ac5f39..0dcde3b35 100644 --- a/tests/by-util/test_ln.rs +++ b/tests/by-util/test_ln.rs @@ -622,7 +622,7 @@ fn test_backup_same_file() { at.touch("file1"); ucmd.args(&["--backup", "file1", "./file1"]) .fails() - .stderr_contains("'file1' and './file1' are the same file"); + .stderr_contains("n: failed to link 'file1' to './file1': Same file"); } #[test] diff --git a/util/build-gnu.sh b/util/build-gnu.sh index 4f02e8425..924d14d25 100755 --- a/util/build-gnu.sh +++ b/util/build-gnu.sh @@ -180,4 +180,4 @@ sed -i -e "s~ sed -n \"1s/'\\\/'/'OPT'/p\" < err >> pat || framework_failure_~ sed -i -e "s/rcexp=1$/rcexp=2\n case \"\$prg\" in chcon|dir|runcon|vdir) return;; esac/" tests/misc/usage_vs_getopt.sh # Update the GNU error message to match ours -sed -i -e "s/ln: 'f' and 'f' are the same file/ln: failed to link: 'f' and 'f' are the same file/g" tests/ln/hard-backup.sh +sed -i -e "s/ln: 'f' and 'f' are the same file/ln: failed to link 'f' to 'f': Same file/g" tests/ln/hard-backup.sh