Skip to content

Commit

Permalink
Merge pull request #3730 from niyaznigmatullin/ln_error_messages
Browse files Browse the repository at this point in the history 
ln: error messages
  • Loading branch information
@sylvestre
sylvestre authored Jul 20, 2022
2 parents b9d8ee6 + 9f2a9fa commit c5f35aa
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 101 deletions.
102 changes: 35 additions & 67 deletions src/uu/ln/src/ln.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -55,8 +55,7 @@ pub enum OverwriteMode {
enum LnError {
TargetIsDirectory(PathBuf),
SomeLinksFailed,
FailedToLink(PathBuf, PathBuf, String),
SameFile(),
SameFile(PathBuf, PathBuf),
MissingDestination(PathBuf),
ExtraOperand(OsString),
}
Expand All @@ -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) => {
write!(f, "failed to link {} to {}: {}", s.quote(), d.quote(), e)
Self::SameFile(s, d) => {
write!(f, "{} and {} are the same file", s.quote(), d.quote())
}
Self::SameFile() => {
write!(f, "Same file")
}
Self::SomeLinksFailed => write!(f, "some links failed to create"),
Self::SomeLinksFailed => Ok(()),
Self::MissingDestination(s) => {
write!(f, "missing destination file operand after {}", s.quote())
}
Expand All @@ -89,14 +85,7 @@ impl Error for LnError {}

impl UError for LnError {
fn code(&self) -> i32 {
match self {
Self::TargetIsDirectory(_)
| Self::SomeLinksFailed
| Self::FailedToLink(_, _, _)
| Self::SameFile()
| Self::MissingDestination(_)
| Self::ExtraOperand(_) => 1,
}
1
}
}

Expand Down Expand Up @@ -315,15 +304,7 @@ fn exec(files: &[PathBuf], settings: &Settings) -> UResult<()> {
}
assert!(!files.is_empty());

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(),
)
}
}
link(&files[0], &files[1], settings)
}

fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings) -> UResult<()> {
Expand Down Expand Up @@ -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!(
"cannot link {} to {}: {}",
targetpath.quote(),
srcpath.quote(),
e
);
show_error!("{}", e);
all_successful = false;
}
}
Expand All @@ -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>> {
let src_abs = canonicalize(src, MissingHandling::Normal, ResolveMode::Logical)?;
let mut dst_abs = canonicalize(
dst.parent().unwrap(),
MissingHandling::Normal,
ResolveMode::Logical,
)?;
dst_abs.push(dst.components().last().unwrap());
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(".");
fn relative_path<'a>(src: &'a Path, dst: &Path) -> Cow<'a, Path> {
if let Ok(src_abs) = canonicalize(src, MissingHandling::Missing, ResolveMode::Physical) {
if let Ok(dst_abs) = canonicalize(
dst.parent().unwrap(),
MissingHandling::Missing,
ResolveMode::Physical,
) {
return make_path_relative_to(src_abs, dst_abs).into();
}
}
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()
};
Expand All @@ -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 => {}
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
29 changes: 3 additions & 26 deletions src/uu/realpath/src/realpath.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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()
}
25 changes: 25 additions & 0 deletions src/uucore/src/lib/features/fs.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
11 changes: 5 additions & 6 deletions tests/by-util/test_ln.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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]);
assert!(!at.is_symlink("no-such-dir"));
at.relative_symlink_dir(file_a, "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]
Expand All @@ -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]
Expand All @@ -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");
}
3 changes: 1 addition & 2 deletions util/build-gnu.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/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
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


# GNU sleep accepts some crazy string, not sure we should match this behavior
Expand Down

0 comments on commit c5f35aa

Please sign in to comment.