Skip to content

Commit e402404

Browse files
committed
Do not use .display()/.to_string_lossy() unnecessarily
This comes up a lot for quoted strings in messages: OS strings can be quoted directly and this prevents information loss. This commit removes ~60% of the calls to these methods (modulo tests). Some of the remaining calls are benign, for example because they convert solely to check for the presence of ASCII characters. Others are nontrivial to improve.
1 parent c804e51 commit e402404

File tree

74 files changed

+256
-272
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

74 files changed

+256
-272
lines changed

src/uu/base32/src/base_common.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ impl Config {
5252
if let Some(extra_op) = values.next() {
5353
return Err(UUsageError::new(
5454
BASE_CMD_PARSE_ERROR,
55-
translate!("base-common-extra-operand", "operand" => extra_op.to_string_lossy().quote()),
55+
translate!("base-common-extra-operand", "operand" => extra_op.quote()),
5656
));
5757
}
5858

src/uu/chcon/src/chcon.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -760,7 +760,7 @@ fn root_dev_ino_warn(dir_name: &Path) {
760760
} else {
761761
show_warning!(
762762
"{}",
763-
translate!("chcon-warning-dangerous-recursive-dir", "dir" => dir_name.to_string_lossy(), "option" => options::preserve_root::NO_PRESERVE_ROOT)
763+
translate!("chcon-warning-dangerous-recursive-dir", "dir" => dir_name.quote(), "option" => options::preserve_root::NO_PRESERVE_ROOT)
764764
);
765765
}
766766
}
@@ -782,7 +782,7 @@ fn cycle_warning_required(fts_options: c_int, entry: &fts::EntryRef) -> bool {
782782
fn emit_cycle_warning(file_name: &Path) {
783783
show_warning!(
784784
"{}",
785-
translate!("chcon-warning-circular-directory", "file" => file_name.to_string_lossy())
785+
translate!("chcon-warning-circular-directory", "file" => file_name.quote())
786786
);
787787
}
788788

src/uu/chmod/src/chmod.rs

Lines changed: 20 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use clap::{Arg, ArgAction, Command};
99
use std::ffi::OsString;
1010
use std::fs;
1111
use std::os::unix::fs::{MetadataExt, PermissionsExt};
12-
use std::path::Path;
12+
use std::path::{Path, PathBuf};
1313
use thiserror::Error;
1414
use uucore::display::Quotable;
1515
use uucore::error::{ExitCode, UError, UResult, USimpleError, UUsageError, set_exit_code};
@@ -27,17 +27,17 @@ use uucore::translate;
2727
#[derive(Debug, Error)]
2828
enum ChmodError {
2929
#[error("{}", translate!("chmod-error-cannot-stat", "file" => _0.quote()))]
30-
CannotStat(String),
30+
CannotStat(PathBuf),
3131
#[error("{}", translate!("chmod-error-dangling-symlink", "file" => _0.quote()))]
32-
DanglingSymlink(String),
32+
DanglingSymlink(PathBuf),
3333
#[error("{}", translate!("chmod-error-no-such-file", "file" => _0.quote()))]
34-
NoSuchFile(String),
34+
NoSuchFile(PathBuf),
3535
#[error("{}", translate!("chmod-error-preserve-root", "file" => _0.quote()))]
36-
PreserveRoot(String),
37-
#[error("{}", translate!("chmod-error-permission-denied", "file" => _0.quote()))]
38-
PermissionDenied(String),
39-
#[error("{}", translate!("chmod-error-new-permissions", "file" => _0.clone(), "actual" => _1.clone(), "expected" => _2.clone()))]
40-
NewPermissions(String, String, String),
36+
PreserveRoot(PathBuf),
37+
#[error("{}", translate!("chmod-error-permission-denied", "file" => _0.maybe_quote()))]
38+
PermissionDenied(PathBuf),
39+
#[error("{}", translate!("chmod-error-new-permissions", "file" => _0.maybe_quote(), "actual" => _1.clone(), "expected" => _2.clone()))]
40+
NewPermissions(PathBuf, String, String),
4141
}
4242

4343
impl UError for ChmodError {}
@@ -123,7 +123,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
123123
Some(fref) => match fs::metadata(fref) {
124124
Ok(meta) => Some(meta.mode() & 0o7777),
125125
Err(_) => {
126-
return Err(ChmodError::CannotStat(fref.to_string_lossy().to_string()).into());
126+
return Err(ChmodError::CannotStat(fref.into()).into());
127127
}
128128
},
129129
None => None,
@@ -384,22 +384,18 @@ impl Chmoder {
384384
}
385385

386386
if !self.quiet {
387-
show!(ChmodError::DanglingSymlink(
388-
filename.to_string_lossy().to_string()
389-
));
387+
show!(ChmodError::DanglingSymlink(filename.into()));
390388
set_exit_code(1);
391389
}
392390

393391
if self.verbose {
394392
println!(
395393
"{}",
396-
translate!("chmod-verbose-failed-dangling", "file" => filename.to_string_lossy().quote())
394+
translate!("chmod-verbose-failed-dangling", "file" => filename.quote())
397395
);
398396
}
399397
} else if !self.quiet {
400-
show!(ChmodError::NoSuchFile(
401-
filename.to_string_lossy().to_string()
402-
));
398+
show!(ChmodError::NoSuchFile(filename.into()));
403399
}
404400
// GNU exits with exit code 1 even if -q or --quiet are passed
405401
// So we set the exit code, because it hasn't been set yet if `self.quiet` is true.
@@ -412,7 +408,7 @@ impl Chmoder {
412408
continue;
413409
}
414410
if self.recursive && self.preserve_root && file == Path::new("/") {
415-
return Err(ChmodError::PreserveRoot("/".to_string()).into());
411+
return Err(ChmodError::PreserveRoot("/".into()).into());
416412
}
417413
if self.recursive {
418414
r = self.walk_dir_with_context(file, true);
@@ -474,10 +470,7 @@ impl Chmoder {
474470
Err(err) => {
475471
// Handle permission denied errors with proper file path context
476472
if err.kind() == std::io::ErrorKind::PermissionDenied {
477-
r = r.and(Err(ChmodError::PermissionDenied(
478-
file_path.to_string_lossy().to_string(),
479-
)
480-
.into()));
473+
r = r.and(Err(ChmodError::PermissionDenied(file_path.into()).into()));
481474
} else {
482475
r = r.and(Err(err.into()));
483476
}
@@ -504,7 +497,7 @@ impl Chmoder {
504497
// Handle permission denied with proper file path context
505498
let e = dir_meta.unwrap_err();
506499
let error = if e.kind() == std::io::ErrorKind::PermissionDenied {
507-
ChmodError::PermissionDenied(entry_path.to_string_lossy().to_string()).into()
500+
ChmodError::PermissionDenied(entry_path).into()
508501
} else {
509502
e.into()
510503
};
@@ -584,9 +577,7 @@ impl Chmoder {
584577
new_mode
585578
);
586579
}
587-
return Err(
588-
ChmodError::PermissionDenied(file_path.to_string_lossy().to_string()).into(),
589-
);
580+
return Err(ChmodError::PermissionDenied(file_path.into()).into());
590581
}
591582

592583
// Report the change using the helper method
@@ -625,9 +616,9 @@ impl Chmoder {
625616
} else if err.kind() == std::io::ErrorKind::PermissionDenied {
626617
// These two filenames would normally be conditionally
627618
// quoted, but GNU's tests expect them to always be quoted
628-
Err(ChmodError::PermissionDenied(file.to_string_lossy().to_string()).into())
619+
Err(ChmodError::PermissionDenied(file.into()).into())
629620
} else {
630-
Err(ChmodError::CannotStat(file.to_string_lossy().to_string()).into())
621+
Err(ChmodError::CannotStat(file.into()).into())
631622
};
632623
}
633624
};
@@ -657,7 +648,7 @@ impl Chmoder {
657648
// if a permission would have been removed if umask was 0, but it wasn't because umask was not 0, print an error and fail
658649
if (new_mode & !naively_expected_new_mode) != 0 {
659650
return Err(ChmodError::NewPermissions(
660-
file.to_string_lossy().to_string(),
651+
file.into(),
661652
display_permissions_unix(new_mode as mode_t, false),
662653
display_permissions_unix(naively_expected_new_mode as mode_t, false),
663654
)

src/uu/chroot/src/chroot.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
182182
}
183183

184184
if !options.newroot.is_dir() {
185-
return Err(ChrootError::NoSuchDirectory(format!("{}", options.newroot.display())).into());
185+
return Err(ChrootError::NoSuchDirectory(options.newroot).into());
186186
}
187187

188188
let commands = match matches.get_many::<String>(options::COMMAND) {
@@ -436,7 +436,7 @@ fn enter_chroot(root: &Path, skip_chdir: bool) -> UResult<()> {
436436
let err = unsafe {
437437
chroot(
438438
CString::new(root.as_os_str().as_bytes().to_vec())
439-
.map_err(|e| ChrootError::CannotEnter("root".to_string(), e.into()))?
439+
.map_err(|e| ChrootError::CannotEnter("root".into(), e.into()))?
440440
.as_bytes_with_nul()
441441
.as_ptr()
442442
.cast::<libc::c_char>(),
@@ -449,6 +449,6 @@ fn enter_chroot(root: &Path, skip_chdir: bool) -> UResult<()> {
449449
}
450450
Ok(())
451451
} else {
452-
Err(ChrootError::CannotEnter(format!("{}", root.display()), Error::last_os_error()).into())
452+
Err(ChrootError::CannotEnter(root.into(), Error::last_os_error()).into())
453453
}
454454
}

src/uu/chroot/src/error.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
// spell-checker:ignore NEWROOT Userspec userspec
66
//! Errors returned by chroot.
77
use std::io::Error;
8+
use std::path::PathBuf;
89
use thiserror::Error;
910
use uucore::display::Quotable;
1011
use uucore::error::UError;
@@ -16,7 +17,7 @@ use uucore::translate;
1617
pub enum ChrootError {
1718
/// Failed to enter the specified directory.
1819
#[error("{}", translate!("chroot-error-cannot-enter", "dir" => _0.quote(), "err" => _1))]
19-
CannotEnter(String, #[source] Error),
20+
CannotEnter(PathBuf, #[source] Error),
2021

2122
/// Failed to execute the specified command.
2223
#[error("{}", translate!("chroot-error-command-failed", "cmd" => _0.quote(), "err" => _1))]
@@ -52,7 +53,7 @@ pub enum ChrootError {
5253

5354
/// The given directory does not exist.
5455
#[error("{}", translate!("chroot-error-no-such-directory", "dir" => _0.quote()))]
55-
NoSuchDirectory(String),
56+
NoSuchDirectory(PathBuf),
5657

5758
/// The call to `setgid()` failed.
5859
#[error("{}", translate!("chroot-error-set-gid-failed", "gid" => _0, "err" => _1))]

src/uu/cksum/src/cksum.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ use uucore::checksum::{
1919
LEGACY_ALGORITHMS, SUPPORTED_ALGORITHMS, calculate_blake2b_length_str, detect_algo,
2020
digest_reader, perform_checksum_validation, sanitize_sha2_sha3_length_str,
2121
};
22+
use uucore::display::Quotable;
2223
use uucore::translate;
2324

2425
use uucore::{
@@ -200,7 +201,7 @@ where
200201
if filepath.is_dir() {
201202
show!(USimpleError::new(
202203
1,
203-
translate!("cksum-error-is-directory", "file" => filepath.display())
204+
translate!("cksum-error-is-directory", "file" => filepath.maybe_quote())
204205
));
205206
continue;
206207
}
@@ -213,7 +214,7 @@ where
213214
file_buf = match File::open(filepath) {
214215
Ok(file) => file,
215216
Err(err) => {
216-
show!(err.map_err_context(|| filepath.to_string_lossy().to_string()));
217+
show!(err.map_err_context(|| filepath.maybe_quote().to_string()));
217218
continue;
218219
}
219220
};

src/uu/comm/src/comm.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use std::ffi::OsString;
1010
use std::fs::{File, metadata};
1111
use std::io::{self, BufRead, BufReader, Read, StdinLock, stdin};
1212
use std::path::Path;
13+
use uucore::display::Quotable;
1314
use uucore::error::{FromIo, UResult, USimpleError};
1415
use uucore::format_usage;
1516
use uucore::fs::paths_refer_to_same_file;
@@ -310,9 +311,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
310311
let filename1 = matches.get_one::<OsString>(options::FILE_1).unwrap();
311312
let filename2 = matches.get_one::<OsString>(options::FILE_2).unwrap();
312313
let mut f1 = open_file(filename1, line_ending)
313-
.map_err_context(|| filename1.to_string_lossy().to_string())?;
314+
.map_err_context(|| filename1.maybe_quote().to_string())?;
314315
let mut f2 = open_file(filename2, line_ending)
315-
.map_err_context(|| filename2.to_string_lossy().to_string())?;
316+
.map_err_context(|| filename2.maybe_quote().to_string())?;
316317

317318
// Due to default_value(), there must be at least one value here, thus unwrap() must not panic.
318319
let all_delimiters = matches

src/uu/cp/src/cp.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1741,13 +1741,13 @@ pub(crate) fn copy_attributes(
17411741
if let Some(context) = context {
17421742
if let Err(e) = context.set_for_path(dest, false, false) {
17431743
return Err(CpError::Error(
1744-
translate!("cp-error-selinux-set-context", "path" => dest.display(), "error" => e),
1744+
translate!("cp-error-selinux-set-context", "path" => dest.quote(), "error" => e),
17451745
));
17461746
}
17471747
}
17481748
} else {
17491749
return Err(CpError::Error(
1750-
translate!("cp-error-selinux-get-context", "path" => source.display()),
1750+
translate!("cp-error-selinux-get-context", "path" => source.quote()),
17511751
));
17521752
}
17531753
Ok(())

src/uu/cp/src/platform/macos.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use std::os::unix::fs::OpenOptionsExt;
1010
use std::path::Path;
1111

1212
use uucore::buf_copy;
13+
use uucore::display::Quotable;
1314
use uucore::translate;
1415

1516
use uucore::mode::get_umask;
@@ -86,7 +87,7 @@ pub(crate) fn copy_on_write(
8687
// support COW).
8788
match reflink_mode {
8889
ReflinkMode::Always => {
89-
return Err(translate!("cp-error-failed-to-clone", "source" => source.display(), "dest" => dest.display(), "error" => error)
90+
return Err(translate!("cp-error-failed-to-clone", "source" => source.quote(), "dest" => dest.quote(), "error" => error)
9091
.into());
9192
}
9293
_ => {

src/uu/cut/src/cut.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,7 @@ fn cut_files(mut filenames: Vec<OsString>, mode: &Mode) {
374374
if path.is_dir() {
375375
show_error!(
376376
"{}: {}",
377-
filename.to_string_lossy().maybe_quote(),
377+
filename.maybe_quote(),
378378
translate!("cut-error-is-directory")
379379
);
380380
set_exit_code(1);
@@ -383,7 +383,7 @@ fn cut_files(mut filenames: Vec<OsString>, mode: &Mode) {
383383

384384
show_if_err!(
385385
File::open(path)
386-
.map_err_context(|| filename.to_string_lossy().to_string())
386+
.map_err_context(|| filename.maybe_quote().to_string())
387387
.and_then(|file| {
388388
match &mode {
389389
Mode::Bytes(ranges, opts) | Mode::Characters(ranges, opts) => {

0 commit comments

Comments
 (0)