Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ln: error messages #3730

Merged
merged 5 commits into from
Jul 20, 2022
Merged

ln: error messages #3730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
80ff3b3
ln: change error messages, extract common code
niyaznigmatullin Jul 18, 2022
406d393
tests/ln: fix tests according to new messages
niyaznigmatullin Jul 18, 2022
f220c86
util/build-gnu.sh: remove some test changes + change some messages
niyaznigmatullin Jul 18, 2022
b76c53c
ln: fix windows non-compiling code
niyaznigmatullin Jul 18, 2022
9f2a9fa
uucore/fs: make function more generic
niyaznigmatullin Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 35 additions & 67 deletions src/uu/ln/src/ln.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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