From 5a1d9cec7ec2b82fc7460f4d2b011b58dd3f83da Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 9 Apr 2023 21:10:31 +0200 Subject: [PATCH 1/8] du: an error code when done on a non existing file --- src/uu/du/src/du.rs | 3 ++- tests/by-util/test_du.rs | 25 ++++++++++++++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index cab9da227..fb490888a 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -33,7 +33,7 @@ use std::time::{Duration, UNIX_EPOCH}; use std::{error::Error, fmt::Display}; use uucore::display::{print_verbatim, Quotable}; use uucore::error::FromIo; -use uucore::error::{UError, UResult}; +use uucore::error::{set_exit_code, UError, UResult}; use uucore::parse_glob; use uucore::parse_size::{parse_size, ParseSizeError}; use uucore::{ @@ -655,6 +655,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { path_string.maybe_quote(), "No such file or directory" ); + set_exit_code(1); } } diff --git a/tests/by-util/test_du.rs b/tests/by-util/test_du.rs index d69eaaf99..73be82bf9 100644 --- a/tests/by-util/test_du.rs +++ b/tests/by-util/test_du.rs @@ -6,7 +6,6 @@ // spell-checker:ignore (paths) sublink subwords azerty azeaze xcwww azeaz amaz azea qzerty tazerty #[cfg(not(windows))] use regex::Regex; -#[cfg(not(windows))] use std::io::Write; #[cfg(any(target_os = "linux", target_os = "android"))] @@ -851,3 +850,27 @@ fn test_du_exclude_invalid_syntax() { .fails() .stderr_contains("du: Invalid exclude syntax"); } + +#[test] +fn test_du_symlink_fail() { + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + + at.symlink_file("non-existing.txt", "target.txt"); + + ts.ucmd().arg("-L").arg("target.txt").fails().code_is(1); +} + +#[test] +fn test_du_symlink_multiple_fail() { + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + + at.symlink_file("non-existing.txt", "target.txt"); + let mut file1 = at.make_file("file1"); + file1.write_all(b"azeaze").unwrap(); + + let result = ts.ucmd().arg("-L").arg("target.txt").arg("file1").fails(); + assert_eq!(result.code(), 1); + result.stdout_contains("4\tfile1\n"); +} From eea8d40e781f667fb66856d62cb7827df998deac Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 9 Apr 2023 21:12:39 +0200 Subject: [PATCH 2/8] du: add support of --dereference-args Should fix tests/du/deref.sh --- src/uu/du/src/du.rs | 40 ++++++++++++++++++++++++++++------------ tests/by-util/test_du.rs | 28 +++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 13 deletions(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index fb490888a..74d86219b 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -68,6 +68,7 @@ mod options { pub const TIME_STYLE: &str = "time-style"; pub const ONE_FILE_SYSTEM: &str = "one-file-system"; pub const DEREFERENCE: &str = "dereference"; + pub const DEREFERENCE_ARGS: &str = "dereference-args"; pub const INODES: &str = "inodes"; pub const EXCLUDE: &str = "exclude"; pub const EXCLUDE_FROM: &str = "exclude-from"; @@ -89,6 +90,7 @@ struct Options { separate_dirs: bool, one_file_system: bool, dereference: bool, + dereference_args: Vec, inodes: bool, verbose: bool, } @@ -113,7 +115,7 @@ struct Stat { impl Stat { fn new(path: PathBuf, options: &Options) -> Result { - let metadata = if options.dereference { + let metadata = if options.dereference || options.dereference_args.contains(&path) { fs::metadata(&path)? } else { fs::symlink_metadata(&path)? @@ -505,17 +507,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { summarize, )?; - let options = Options { - all: matches.get_flag(options::ALL), - max_depth, - total: matches.get_flag(options::TOTAL), - separate_dirs: matches.get_flag(options::SEPARATE_DIRS), - one_file_system: matches.get_flag(options::ONE_FILE_SYSTEM), - dereference: matches.get_flag(options::DEREFERENCE), - inodes: matches.get_flag(options::INODES), - verbose: matches.get_flag(options::VERBOSE), - }; - let files = match matches.get_one::(options::FILE) { Some(_) => matches .get_many::(options::FILE) @@ -525,6 +516,24 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { None => vec!["."], }; + let options = Options { + all: matches.get_flag(options::ALL), + max_depth, + total: matches.get_flag(options::TOTAL), + separate_dirs: matches.get_flag(options::SEPARATE_DIRS), + one_file_system: matches.get_flag(options::ONE_FILE_SYSTEM), + dereference: matches.get_flag(options::DEREFERENCE), + dereference_args: if matches.get_flag(options::DEREFERENCE_ARGS) { + // Convert into a pathbuf as we will use it in a function using Pathbuf + // We don't care about the cost as it is rarely used + files.iter().map(PathBuf::from).collect() + } else { + vec![] + }, + inodes: matches.get_flag(options::INODES), + verbose: matches.get_flag(options::VERBOSE), + }; + if options.inodes && (matches.get_flag(options::APPARENT_SIZE) || matches.get_flag(options::BYTES)) { @@ -789,6 +798,13 @@ pub fn uu_app() -> Command { .help("dereference all symbolic links") .action(ArgAction::SetTrue) ) + .arg( + Arg::new(options::DEREFERENCE_ARGS) + .short('D') + .long(options::DEREFERENCE_ARGS) + .help("dereference only symlinks that are listed on the command line") + .action(ArgAction::SetTrue) + ) // .arg( // Arg::new("no-dereference") // .short('P') diff --git a/tests/by-util/test_du.rs b/tests/by-util/test_du.rs index 73be82bf9..992f7f162 100644 --- a/tests/by-util/test_du.rs +++ b/tests/by-util/test_du.rs @@ -3,7 +3,7 @@ // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -// spell-checker:ignore (paths) sublink subwords azerty azeaze xcwww azeaz amaz azea qzerty tazerty +// spell-checker:ignore (paths) sublink subwords azerty azeaze xcwww azeaz amaz azea qzerty tazerty tsublink #[cfg(not(windows))] use regex::Regex; use std::io::Write; @@ -285,6 +285,30 @@ fn test_du_dereference() { _du_dereference(result.stdout_str()); } +#[cfg(not(windows))] +#[test] +fn test_du_dereference_args() { + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + + at.mkdir_all("subdir"); + let mut file1 = at.make_file("subdir/file-ignore1"); + file1.write_all(b"azeaze").unwrap(); + let mut file2 = at.make_file("subdir/file-ignore1"); + file2.write_all(b"amaz?ng").unwrap(); + at.symlink_dir("subdir", "sublink"); + + let result = ts.ucmd().arg("-D").arg("-s").arg("sublink").succeeds(); + let stdout = result.stdout_str(); + + assert!(!stdout.starts_with("0")); + assert!(stdout.contains("sublink")); + + // Without the option + let result = ts.ucmd().arg("-s").arg("sublink").succeeds(); + result.stdout_contains("0\tsublink\n"); +} + #[cfg(target_vendor = "apple")] fn _du_dereference(s: &str) { assert_eq!(s, "4\tsubdir/links/deeper_dir\n16\tsubdir/links\n"); @@ -851,6 +875,7 @@ fn test_du_exclude_invalid_syntax() { .stderr_contains("du: Invalid exclude syntax"); } +#[cfg(not(windows))] #[test] fn test_du_symlink_fail() { let ts = TestScenario::new(util_name!()); @@ -861,6 +886,7 @@ fn test_du_symlink_fail() { ts.ucmd().arg("-L").arg("target.txt").fails().code_is(1); } +#[cfg(not(windows))] #[test] fn test_du_symlink_multiple_fail() { let ts = TestScenario::new(util_name!()); From 47cb40c020605c62df85f8edc9ad26b0c799deee Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 9 Apr 2023 21:24:09 +0200 Subject: [PATCH 3/8] du: adjust test_du_basics_bad_name as we made it work --- tests/by-util/test_du.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/by-util/test_du.rs b/tests/by-util/test_du.rs index 992f7f162..a965d3fc6 100644 --- a/tests/by-util/test_du.rs +++ b/tests/by-util/test_du.rs @@ -121,7 +121,7 @@ fn test_du_invalid_size() { fn test_du_basics_bad_name() { new_ucmd!() .arg("bad_name") - .succeeds() // TODO: replace with ".fails()" once `du` is fixed + .fails() .stderr_only("du: bad_name: No such file or directory\n"); } From f08b8dab235604e2d79d3bdb4e0a76bb1ea61e81 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Tue, 11 Apr 2023 17:54:19 +0200 Subject: [PATCH 4/8] du: directly get a vec of pathbuf to avoid several type changes --- src/uu/du/src/du.rs | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index 74d86219b..2dcfbf2ac 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -114,12 +114,13 @@ struct Stat { } impl Stat { - fn new(path: PathBuf, options: &Options) -> Result { - let metadata = if options.dereference || options.dereference_args.contains(&path) { - fs::metadata(&path)? - } else { - fs::symlink_metadata(&path)? - }; + fn new(path: &Path, options: &Options) -> Result { + let metadata = + if options.dereference || options.dereference_args.contains(&path.to_path_buf()) { + fs::metadata(path)? + } else { + fs::symlink_metadata(path)? + }; #[cfg(not(windows))] let file_info = FileInfo { @@ -128,7 +129,7 @@ impl Stat { }; #[cfg(not(windows))] return Ok(Self { - path, + path: path.to_path_buf(), is_dir: metadata.is_dir(), size: metadata.len(), blocks: metadata.blocks(), @@ -140,12 +141,12 @@ impl Stat { }); #[cfg(windows)] - let size_on_disk = get_size_on_disk(&path); + let size_on_disk = get_size_on_disk(path); #[cfg(windows)] - let file_info = get_file_info(&path); + let file_info = get_file_info(path); #[cfg(windows)] Ok(Self { - path, + path: path.to_path_buf(), is_dir: metadata.is_dir(), size: metadata.len(), blocks: size_on_disk / 1024 * 2, @@ -298,7 +299,7 @@ fn du( 'file_loop: for f in read { match f { Ok(entry) => { - match Stat::new(entry.path(), options) { + match Stat::new(&entry.path(), options) { Ok(this_stat) => { // We have an exclude list for pattern in exclude { @@ -511,9 +512,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { Some(_) => matches .get_many::(options::FILE) .unwrap() - .map(|s| s.as_str()) + .map(PathBuf::from) .collect(), - None => vec!["."], + None => vec![PathBuf::from(".")], }; let options = Options { @@ -524,9 +525,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { one_file_system: matches.get_flag(options::ONE_FILE_SYSTEM), dereference: matches.get_flag(options::DEREFERENCE), dereference_args: if matches.get_flag(options::DEREFERENCE_ARGS) { - // Convert into a pathbuf as we will use it in a function using Pathbuf - // We don't care about the cost as it is rarely used - files.iter().map(PathBuf::from).collect() + // We don't care about the cost of cloning as it is rarely used + files.clone() } else { vec![] }, @@ -589,11 +589,12 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let excludes = build_exclude_patterns(&matches)?; let mut grand_total = 0; - 'loop_file: for path_string in files { + 'loop_file: for path in files { // Skip if we don't want to ignore anything if !&excludes.is_empty() { + let path_string = path.to_string_lossy(); for pattern in &excludes { - if pattern.matches(path_string) { + if pattern.matches(&path_string) { // if the directory is ignored, leave early if options.verbose { println!("{} ignored", path_string.quote()); @@ -603,9 +604,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } } - let path = PathBuf::from(&path_string); // Check existence of path provided in argument - if let Ok(stat) = Stat::new(path, &options) { + if let Ok(stat) = Stat::new(&path, &options) { // Kick off the computation of disk usage from the initial path let mut inodes: HashSet = HashSet::new(); if let Some(inode) = stat.inode { @@ -661,7 +661,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } else { show_error!( "{}: {}", - path_string.maybe_quote(), + path.to_string_lossy().maybe_quote(), "No such file or directory" ); set_exit_code(1); From f475a362042301319665f68c5869d5145cd38da4 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Tue, 11 Apr 2023 20:41:57 +0200 Subject: [PATCH 5/8] du: reduce the complexity by moving the time option mgmt into a function --- src/uu/du/src/du.rs | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index 2dcfbf2ac..c5aafb44e 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -625,20 +625,11 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { if matches.contains_id(options::TIME) { let tm = { - let secs = { - match matches.get_one::(options::TIME) { - Some(s) => match s.as_str() { - "ctime" | "status" => stat.modified, - "access" | "atime" | "use" => stat.accessed, - "birth" | "creation" => stat - .created - .ok_or_else(|| DuError::InvalidTimeArg(s.into()))?, - // below should never happen as clap already restricts the values. - _ => unreachable!("Invalid field for --time"), - }, - None => stat.modified, - } - }; + let secs = matches + .get_one::(options::TIME) + .map(|s| get_time_secs(s, &stat)) + .transpose()? + .unwrap_or(stat.modified); DateTime::::from(UNIX_EPOCH + Duration::from_secs(secs)) }; if !summarize || index == len - 1 { @@ -676,6 +667,19 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { Ok(()) } +fn get_time_secs(s: &str, stat: &Stat) -> std::result::Result { + let secs = match s { + "ctime" | "status" => stat.modified, + "access" | "atime" | "use" => stat.accessed, + "birth" | "creation" => stat + .created + .ok_or_else(|| DuError::InvalidTimeArg(s.into()))?, + // below should never happen as clap already restricts the values. + _ => unreachable!("Invalid field for --time"), + }; + Ok(secs) +} + fn parse_time_style(s: Option<&str>) -> UResult<&str> { match s { Some(s) => match s { From b2fd72878ee97f8b8f13e6dcb190df6cf4e50d2d Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Tue, 11 Apr 2023 21:03:24 +0200 Subject: [PATCH 6/8] du: reduce the complexity by moving get_convert_size_fn option mgmt into a function --- src/uu/du/src/du.rs | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index c5aafb44e..d7618d3e0 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -400,6 +400,20 @@ fn convert_size_other(size: u64, _multiplier: u64, block_size: u64) -> String { format!("{}", ((size as f64) / (block_size as f64)).ceil()) } +fn get_convert_size_fn(matches: &ArgMatches) -> Box String> { + if matches.get_flag(options::HUMAN_READABLE) || matches.get_flag(options::SI) { + Box::new(convert_size_human) + } else if matches.get_flag(options::BYTES) { + Box::new(convert_size_b) + } else if matches.get_flag(options::BLOCK_SIZE_1K) { + Box::new(convert_size_k) + } else if matches.get_flag(options::BLOCK_SIZE_1M) { + Box::new(convert_size_m) + } else { + Box::new(convert_size_other) + } +} + #[derive(Debug)] enum DuError { InvalidMaxDepthArg(String), @@ -556,19 +570,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } else { 1024 }; - let convert_size_fn = { - if matches.get_flag(options::HUMAN_READABLE) || matches.get_flag(options::SI) { - convert_size_human - } else if matches.get_flag(options::BYTES) { - convert_size_b - } else if matches.get_flag(options::BLOCK_SIZE_1K) { - convert_size_k - } else if matches.get_flag(options::BLOCK_SIZE_1M) { - convert_size_m - } else { - convert_size_other - } - }; + + let convert_size_fn = get_convert_size_fn(&matches); + let convert_size = |size: u64| { if options.inodes { size.to_string() From e82e6a3e4c075f1c30ef0cd3d26d87cb053d75f8 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Tue, 11 Apr 2023 22:33:34 +0200 Subject: [PATCH 7/8] du: remove cognitive_complexity clippy allow --- src/uu/du/src/du.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index d7618d3e0..4b1aa5a0e 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -507,7 +507,6 @@ fn build_exclude_patterns(matches: &ArgMatches) -> UResult> { } #[uucore::main] -#[allow(clippy::cognitive_complexity)] pub fn uumain(args: impl uucore::Args) -> UResult<()> { let args = args.collect_ignore(); From 00a866e1281e62c830038f829de0c98829a9b9f3 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Wed, 12 Apr 2023 21:25:42 +0200 Subject: [PATCH 8/8] du: move to use an enum --- src/uu/du/src/du.rs | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index 4b1aa5a0e..f78e0d4ae 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -89,12 +89,18 @@ struct Options { total: bool, separate_dirs: bool, one_file_system: bool, - dereference: bool, - dereference_args: Vec, + dereference: Deref, inodes: bool, verbose: bool, } +#[derive(PartialEq)] +enum Deref { + All, + Args(Vec), + None, +} + #[derive(PartialEq, Eq, Hash, Clone, Copy)] struct FileInfo { file_id: u128, @@ -115,12 +121,20 @@ struct Stat { impl Stat { fn new(path: &Path, options: &Options) -> Result { - let metadata = - if options.dereference || options.dereference_args.contains(&path.to_path_buf()) { - fs::metadata(path)? - } else { - fs::symlink_metadata(path)? - }; + // Determine whether to dereference (follow) the symbolic link + let should_dereference = match &options.dereference { + Deref::All => true, + Deref::Args(paths) => paths.contains(&path.to_path_buf()), + Deref::None => false, + }; + + let metadata = if should_dereference { + // Get metadata, following symbolic links if necessary + fs::metadata(path) + } else { + // Get metadata without following symbolic links + fs::symlink_metadata(path) + }?; #[cfg(not(windows))] let file_info = FileInfo { @@ -536,12 +550,13 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { total: matches.get_flag(options::TOTAL), separate_dirs: matches.get_flag(options::SEPARATE_DIRS), one_file_system: matches.get_flag(options::ONE_FILE_SYSTEM), - dereference: matches.get_flag(options::DEREFERENCE), - dereference_args: if matches.get_flag(options::DEREFERENCE_ARGS) { + dereference: if matches.get_flag(options::DEREFERENCE) { + Deref::All + } else if matches.get_flag(options::DEREFERENCE_ARGS) { // We don't care about the cost of cloning as it is rarely used - files.clone() + Deref::Args(files.clone()) } else { - vec![] + Deref::None }, inodes: matches.get_flag(options::INODES), verbose: matches.get_flag(options::VERBOSE),