Merge pull request #3730 from niyaznigmatullin/ln_error_messages

ln: error messages

src/uu/ln/src/ln.rs

@@ -12,17 +12,17 @@ extern crate uucore;
 
 use clap::{crate_version, Arg, Command};
 use uucore::display::Quotable;
-use uucore::error::{UError, UResult};
+use uucore::error::{FromIo, UError, UResult};
 use uucore::format_usage;
-use uucore::fs::{is_symlink, paths_refer_to_same_file};
+use uucore::fs::{is_symlink, make_path_relative_to, paths_refer_to_same_file};
 
 use std::borrow::Cow;
 use std::error::Error;
-use std::ffi::{OsStr, OsString};
+use std::ffi::OsString;
 use std::fmt::Display;
 use std::fs;
 
-use std::io::{stdin, Result};
+use std::io::stdin;
 #[cfg(any(unix, target_os = "redox"))]
 use std::os::unix::fs::symlink;
 #[cfg(windows)]
@@ -55,8 +55,7 @@ pub enum OverwriteMode {
 enum LnError {
     TargetIsDirectory(PathBuf),
     SomeLinksFailed,
-    FailedToLink(PathBuf, PathBuf, String),
+    SameFile(PathBuf, PathBuf),
-    SameFile(),
     MissingDestination(PathBuf),
     ExtraOperand(OsString),
 }
@@ -65,13 +64,10 @@ 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) => {
+            Self::SameFile(s, d) => {
-                write!(f, "failed to link {} to {}: {}", s.quote(), d.quote(), e)
+                write!(f, "{} and {} are the same file", s.quote(), d.quote())
             }
-            Self::SameFile() => {
+            Self::SomeLinksFailed => Ok(()),
-                write!(f, "Same file")
-            }
-            Self::SomeLinksFailed => write!(f, "some links failed to create"),
             Self::MissingDestination(s) => {
                 write!(f, "missing destination file operand after {}", s.quote())
             }
@@ -89,14 +85,7 @@ impl Error for LnError {}
 
 impl UError for LnError {
     fn code(&self) -> i32 {
-        match self {
+        1
-            Self::TargetIsDirectory(_)
-            | Self::SomeLinksFailed
-            | Self::FailedToLink(_, _, _)
-            | Self::SameFile()
-            | Self::MissingDestination(_)
-            | Self::ExtraOperand(_) => 1,
-        }
     }
 }
 
@@ -315,15 +304,7 @@ fn exec(files: &[PathBuf], settings: &Settings) -> UResult<()> {
     }
     assert!(!files.is_empty());
 
-    match link(&files[0], &files[1], settings) {
+    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(),
-            )
-        }
-    }
 }
 
 fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings) -> UResult<()> {
@@ -374,12 +355,7 @@ fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings)
             };
 
         if let Err(e) = link(srcpath, &targetpath, settings) {
-            show_error!(
+            show_error!("{}", e);
-                "cannot link {} to {}: {}",
-                targetpath.quote(),
-                srcpath.quote(),
-                e
-            );
             all_successful = false;
         }
     }
@@ -390,38 +366,23 @@ fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings)
     }
 }
 
-fn relative_path<'a>(src: &Path, dst: &Path) -> Result<Cow<'a, Path>> {
+fn relative_path<'a>(src: &'a Path, dst: &Path) -> Cow<'a, Path> {
-    let src_abs = canonicalize(src, MissingHandling::Normal, ResolveMode::Logical)?;
+    if let Ok(src_abs) = canonicalize(src, MissingHandling::Missing, ResolveMode::Physical) {
-    let mut dst_abs = canonicalize(
+        if let Ok(dst_abs) = canonicalize(
-        dst.parent().unwrap(),
+            dst.parent().unwrap(),
-        MissingHandling::Normal,
+            MissingHandling::Missing,
-        ResolveMode::Logical,
+            ResolveMode::Physical,
-    )?;
+        ) {
-    dst_abs.push(dst.components().last().unwrap());
+            return make_path_relative_to(src_abs, dst_abs).into();
-    let suffix_pos = src_abs
+        }
-        .components()
-        .zip(dst_abs.components())
-        .take_while(|(s, d)| s == d)
-        .count();
-
-    let src_iter = src_abs.components().skip(suffix_pos).map(|x| x.as_os_str());
-
-    let mut result: PathBuf = dst_abs
-        .components()
-        .skip(suffix_pos + 1)
-        .map(|_| OsStr::new(".."))
-        .chain(src_iter)
-        .collect();
-    if result.as_os_str().is_empty() {
-        result.push(".");
     }
-    Ok(result.into())
+    src.into()
 }
 
 fn link(src: &Path, dst: &Path, settings: &Settings) -> UResult<()> {
     let mut backup_path = None;
     let source: Cow<'_, Path> = if settings.relative {
-        relative_path(src, dst)?
+        relative_path(src, dst)
     } else {
         src.into()
     };
@@ -436,11 +397,11 @@ fn link(src: &Path, dst: &Path, settings: &Settings) -> UResult<()> {
         if settings.backup == BackupMode::ExistingBackup && !settings.symbolic {
             // when ln --backup f f, it should detect that it is the same file
             if paths_refer_to_same_file(src, dst, true) {
-                return Err(LnError::SameFile().into());
+                return Err(LnError::SameFile(src.to_owned(), dst.to_owned()).into());
             }
         }
         if let Some(ref p) = backup_path {
-            fs::rename(dst, p)?;
+            fs::rename(dst, p).map_err_context(|| format!("cannot backup {}", dst.quote()))?;
         }
         match settings.overwrite {
             OverwriteMode::NoClobber => {}
@@ -455,7 +416,7 @@ fn link(src: &Path, dst: &Path, settings: &Settings) -> UResult<()> {
             }
             OverwriteMode::Force => {
                 if !is_symlink(dst) && paths_refer_to_same_file(src, dst, true) {
-                    return Err(LnError::SameFile().into());
+                    return Err(LnError::SameFile(src.to_owned(), dst.to_owned()).into());
                 }
                 if fs::remove_file(dst).is_ok() {};
                 // In case of error, don't do anything
@@ -470,11 +431,18 @@ fn link(src: &Path, dst: &Path, settings: &Settings) -> UResult<()> {
             // if we want to have an hard link,
             // source is a symlink and -L is passed
             // we want to resolve the symlink to create the hardlink
-            std::fs::canonicalize(&source)?
+            fs::canonicalize(&source)
+                .map_err_context(|| format!("failed to access {}", source.quote()))?
         } else {
             source.to_path_buf()
         };
-        fs::hard_link(&p, dst)?;
+        fs::hard_link(&p, dst).map_err_context(|| {
+            format!(
+                "failed to create hard link {} => {}",
+                source.quote(),
+                dst.quote()
+            )
+        })?;
     }
 
     if settings.verbose {
@@ -524,7 +492,7 @@ fn existing_backup_path(path: &Path, suffix: &str) -> PathBuf {
 }
 
 #[cfg(windows)]
-pub fn symlink<P1: AsRef<Path>, P2: AsRef<Path>>(src: P1, dst: P2) -> Result<()> {
+pub fn symlink<P1: AsRef<Path>, P2: AsRef<Path>>(src: P1, dst: P2) -> std::io::Result<()> {
     if src.as_ref().is_dir() {
         symlink_dir(src, dst)
     } else {

src/uu/realpath/src/realpath.rs

@@ -11,12 +11,12 @@
 extern crate uucore;
 
 use clap::{crate_version, Arg, ArgMatches, Command};
-use std::path::Component;
 use std::{
     io::{stdout, Write},
     path::{Path, PathBuf},
 };
 use uucore::error::UClapError;
+use uucore::fs::make_path_relative_to;
 use uucore::{
     display::{print_verbatim, Quotable},
     error::{FromIo, UResult},
@@ -274,36 +274,13 @@ fn process_relative(
 ) -> PathBuf {
     if let Some(base) = relative_base {
         if path.starts_with(base) {
-            make_path_relative_to(&path, relative_to.unwrap_or(base))
+            make_path_relative_to(path, relative_to.unwrap_or(base))
         } else {
             path
         }
     } else if let Some(to) = relative_to {
-        make_path_relative_to(&path, to)
+        make_path_relative_to(path, to)
     } else {
         path
     }
 }
-
-/// Converts absolute `path` to be relative to absolute `to` path.
-fn make_path_relative_to(path: &Path, to: &Path) -> PathBuf {
-    let common_prefix_size = path
-        .components()
-        .zip(to.components())
-        .take_while(|(first, second)| first == second)
-        .count();
-    let path_suffix = path
-        .components()
-        .skip(common_prefix_size)
-        .map(|x| x.as_os_str());
-    let mut components: Vec<_> = to
-        .components()
-        .skip(common_prefix_size)
-        .map(|_| Component::ParentDir.as_os_str())
-        .chain(path_suffix)
-        .collect();
-    if components.is_empty() {
-        components.push(Component::CurDir.as_os_str());
-    }
-    components.iter().collect()
-}

src/uucore/src/lib/features/fs.rs

@@ -488,6 +488,31 @@ pub fn paths_refer_to_same_file<P: AsRef<Path>>(p1: P, p2: P, dereference: bool)
     false
 }
 
+/// Converts absolute `path` to be relative to absolute `to` path.
+pub fn make_path_relative_to<P1: AsRef<Path>, P2: AsRef<Path>>(path: P1, to: P2) -> PathBuf {
+    let path = path.as_ref();
+    let to = to.as_ref();
+    let common_prefix_size = path
+        .components()
+        .zip(to.components())
+        .take_while(|(first, second)| first == second)
+        .count();
+    let path_suffix = path
+        .components()
+        .skip(common_prefix_size)
+        .map(|x| x.as_os_str());
+    let mut components: Vec<_> = to
+        .components()
+        .skip(common_prefix_size)
+        .map(|_| Component::ParentDir.as_os_str())
+        .chain(path_suffix)
+        .collect();
+    if components.is_empty() {
+        components.push(Component::CurDir.as_os_str());
+    }
+    components.iter().collect()
+}
+
 #[cfg(test)]
 mod tests {
     // Note this useful idiom: importing names from outer (for mod tests) scope.

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("n: failed to link 'file1' to './file1': Same file");
+        .stderr_contains("n: 'file1' and './file1' are the same file");
 }
 
 #[test]
@@ -676,14 +676,13 @@ fn test_hard_logical_non_exit_fail() {
     let file_a = "/no-such-dir";
     let link = "hard-to-dangle";
 
-    scene.ucmd().args(&["-s", file_a]);
+    at.relative_symlink_dir(file_a, "no-such-dir");
-    assert!(!at.is_symlink("no-such-dir"));
 
     scene
         .ucmd()
         .args(&["-L", "no-such-dir", link])
         .fails()
-        .stderr_contains("failed to link 'no-such-dir'");
+        .stderr_contains("failed to access 'no-such-dir'");
 }
 
 #[test]
@@ -700,7 +699,7 @@ fn test_hard_logical_dir_fail() {
         .ucmd()
         .args(&["-L", target, "hard-to-dir-link"])
         .fails()
-        .stderr_contains("failed to link 'link-to-dir'");
+        .stderr_contains("failed to create hard link 'link-to-dir'");
 }
 
 #[test]
@@ -711,7 +710,7 @@ fn test_symlink_remove_existing_same_src_and_dest() {
     ucmd.args(&["-sf", "a", "a"])
         .fails()
         .code_is(1)
-        .stderr_contains("Same file");
+        .stderr_contains("'a' and 'a' are the same file");
     assert!(at.file_exists("a") && !at.symlink_exists("a"));
     assert_eq!(at.read("a"), "sample");
 }

util/build-gnu.sh

@@ -205,8 +205,7 @@ sed -i -e "s/cat opts/sed -i -e \"s| <.\*>$||g\" opts/" tests/misc/usage_vs_geto
 sed -i -e "s/provoked error./provoked error\ncat pat |sort -u > pat/" 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' to 'f': Same file/g" tests/ln/hard-backup.sh
+sed -i -e "s/link-to-dir: hard link not allowed for directory/failed to create hard link 'link-to-dir' =>/" -e "s|link-to-dir/: hard link not allowed for directory|failed to create hard link 'link-to-dir/' =>|" tests/ln/hard-to-sym.sh
-sed -i -e "s/failed to access 'no-such-dir'\":/failed to link 'no-such-dir'\"/" -e "s/link-to-dir: hard link not allowed for directory/failed to link 'link-to-dir' to/" -e "s|link-to-dir/: hard link not allowed for directory|failed to link 'link-to-dir/' to|" tests/ln/hard-to-sym.sh
 
 
 # GNU sleep accepts some crazy string, not sure we should match this behavior