From e30f191579c4fe1b5c64614cf9712feb9888dc66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Wed, 3 Jan 2024 18:37:51 +0100 Subject: [PATCH 01/13] ls: Handle the use of QUOTING_STYLE variable --- src/uu/ls/src/ls.rs | 111 +++++++++++++++++++++++++++++--------------- 1 file changed, 73 insertions(+), 38 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 952083d2a..d56929be8 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -621,7 +621,52 @@ fn extract_hyperlink(options: &clap::ArgMatches) -> bool { } } +/// Match the argument given to --quoting-style or the QUOTING_STYLE env variable. +/// +/// # Arguments +/// +/// * `style`: the actual argument string +/// * `show_control` - A boolean value representing whether or not to show control characters. +/// +/// # Returns +/// +/// * An option with None if the style string is invalid, or a `QuotingStyle` wrapped in `Some`. +fn match_quoting_style_name(style: &str, show_control: bool) -> Option { + match style { + "literal" => Some(QuotingStyle::Literal { show_control }), + "shell" => Some(QuotingStyle::Shell { + escape: false, + always_quote: false, + show_control, + }), + "shell-always" => Some(QuotingStyle::Shell { + escape: false, + always_quote: true, + show_control, + }), + "shell-escape" => Some(QuotingStyle::Shell { + escape: true, + always_quote: false, + show_control, + }), + "shell-escape-always" => Some(QuotingStyle::Shell { + escape: true, + always_quote: true, + show_control, + }), + "c" => Some(QuotingStyle::C { + quotes: quoting_style::Quotes::Double, + }), + "escape" => Some(QuotingStyle::C { + quotes: quoting_style::Quotes::None, + }), + _ => None, + } +} + /// Extracts the quoting style to use based on the options provided. +/// If no options are given, it looks if a default quoting style is provided +/// through the QUOTING_STYLE environment variable. /// /// # Arguments /// @@ -632,38 +677,12 @@ fn extract_hyperlink(options: &clap::ArgMatches) -> bool { /// /// A QuotingStyle variant representing the quoting style to use. fn extract_quoting_style(options: &clap::ArgMatches, show_control: bool) -> QuotingStyle { - let opt_quoting_style = options.get_one::(options::QUOTING_STYLE).cloned(); + let opt_quoting_style = options.get_one::(options::QUOTING_STYLE); if let Some(style) = opt_quoting_style { - match style.as_str() { - "literal" => QuotingStyle::Literal { show_control }, - "shell" => QuotingStyle::Shell { - escape: false, - always_quote: false, - show_control, - }, - "shell-always" => QuotingStyle::Shell { - escape: false, - always_quote: true, - show_control, - }, - "shell-escape" => QuotingStyle::Shell { - escape: true, - always_quote: false, - show_control, - }, - "shell-escape-always" => QuotingStyle::Shell { - escape: true, - always_quote: true, - show_control, - }, - "c" => QuotingStyle::C { - quotes: quoting_style::Quotes::Double, - }, - "escape" => QuotingStyle::C { - quotes: quoting_style::Quotes::None, - }, - _ => unreachable!("Should have been caught by Clap"), + match match_quoting_style_name(style, show_control) { + Some(qs) => qs, + None => unreachable!("Should have been caught by Clap"), } } else if options.get_flag(options::quoting::LITERAL) { QuotingStyle::Literal { show_control } @@ -675,16 +694,32 @@ fn extract_quoting_style(options: &clap::ArgMatches, show_control: bool) -> Quot QuotingStyle::C { quotes: quoting_style::Quotes::Double, } - } else if options.get_flag(options::DIRED) || !std::io::stdout().is_terminal() { - // By default, `ls` uses Literal quoting when - // writing to a non-terminal file descriptor + } else if options.get_flag(options::DIRED) { QuotingStyle::Literal { show_control } } else { - // TODO: use environment variable if available - QuotingStyle::Shell { - escape: true, - always_quote: false, - show_control, + // If set, the QUOTING_STYLE environment variable specifies a default style. + if let Ok(style) = std::env::var("QUOTING_STYLE") { + match match_quoting_style_name(style.as_str(), show_control) { + Some(qs) => return qs, + None => eprintln!( + "{}: Ignoring invalid value of environment variable QUOTING_STYLE: '{}'", + std::env::args().next().unwrap_or("ls".to_string()), + style + ), + } + } + + // By default, `ls` uses Literal quoting when + // writing to a non-terminal file descriptor + if !std::io::stdout().is_terminal() { + QuotingStyle::Literal { show_control } + } else { + // TODO: use environment variable if available + QuotingStyle::Shell { + escape: true, + always_quote: false, + show_control, + } } } } From 6760d63539cd2bb8ac1e5ec345baea84a1c902a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Thu, 4 Jan 2024 16:51:30 +0100 Subject: [PATCH 02/13] ls: Fix clippy warning --- src/uu/ls/src/ls.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index d56929be8..0e9b25722 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -709,17 +709,16 @@ fn extract_quoting_style(options: &clap::ArgMatches, show_control: bool) -> Quot } } - // By default, `ls` uses Literal quoting when - // writing to a non-terminal file descriptor - if !std::io::stdout().is_terminal() { - QuotingStyle::Literal { show_control } - } else { - // TODO: use environment variable if available + // By default, `ls` uses Shell escape quoting style when writing to a terminal file + // descriptor and Literal otherwise. + if std::io::stdout().is_terminal() { QuotingStyle::Shell { escape: true, always_quote: false, show_control, } + } else { + QuotingStyle::Literal { show_control } } } } From c58575edaaa208622077f4c30cf80645e06e1c90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Fri, 5 Jan 2024 02:07:09 +0100 Subject: [PATCH 03/13] tests/ls: Add tests to ensure env var is used as a last resort --- tests/by-util/test_ls.rs | 65 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 72a303ef3..476704660 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -2622,6 +2622,71 @@ fn test_ls_quoting_style() { } } +#[test] +fn test_ls_quoting_style_env_var_default() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.touch(at.plus_as_string("foo-1")); + at.touch(at.plus_as_string("bar-2")); + + // If no quoting style argument is provided, the QUOTING_STYLE environment variable + // shall be used. + + let correct_c = "\"bar-2\"\n\"foo-1\""; + scene + .ucmd() + .env("QUOTING_STYLE", "c") + .succeeds() + .stdout_only(format!("{correct_c}\n")); +} + +#[test] +fn test_ls_quoting_style_arg_overrides_env_var() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.touch(at.plus_as_string("foo-1")); + at.touch(at.plus_as_string("bar-2")); + + // The quoting style given by the env variable should be + // overriden by any escape style provided by argument. + for (arg, correct) in [ + ("--quoting-style=literal", "foo-1"), + ("-N", "foo-1"), + ("--quoting-style=escape", "foo-1"), + ("-b", "foo-1"), + ("--quoting-style=shell-escape", "foo-1"), + ("--quoting-style=shell-escape-always", "'foo-1'"), + ("--quoting-style=shell", "foo-1"), + ("--quoting-style=shell-always", "'foo-1'"), + ] { + scene + .ucmd() + .env("QUOTING_STYLE", "c") + .arg("--hide-control-chars") + .arg(arg) + .arg("foo-1") + .succeeds() + .stdout_only(format!("{correct}\n")); + } + + // Another loop to check for the C quoting style that is used as a default above. + for (arg, correct) in [ + ("--quoting-style=c", "\"foo-1\""), + ("-Q", "\"foo-1\""), + ("--quote-name", "\"foo-1\""), + ] { + scene + .ucmd() + .env("QUOTING_STYLE", "literal") + .arg("--hide-control-chars") + .arg(arg) + .arg("foo-1") + .succeeds() + .stdout_only(format!("{correct}\n")); + } + +} + #[test] fn test_ls_quoting_and_color() { let scene = TestScenario::new(util_name!()); From 4c698d58e09dc7d46dc47f5b88e1726374939e8c Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Wed, 3 Jan 2024 23:43:02 +0100 Subject: [PATCH 04/13] mv: support the case mkdir a && mv a e/ --- src/uu/mv/src/mv.rs | 5 +++-- tests/by-util/test_mv.rs | 13 +++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 855afcc1f..19a7c274f 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -334,10 +334,11 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()> } } - let target_is_dir = target.is_dir(); + let target_is_dir: bool = target.is_dir(); + let source_is_dir: bool = source.is_dir(); if path_ends_with_terminator(target) - && !target_is_dir + && (!target_is_dir && !source_is_dir) && !opts.no_target_dir && opts.update != UpdateMode::ReplaceIfOlder { diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index 4b923767b..175b91e7d 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -1556,6 +1556,19 @@ fn test_mv_dir_into_file_where_both_are_files() { .stderr_contains("mv: cannot stat 'a/': Not a directory"); } +#[test] +fn test_mv_dir_into_path_slash() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir("a"); + scene.ucmd().arg("a").arg("e/").succeeds(); + assert!(at.dir_exists("e")); + at.mkdir("b"); + at.mkdir("f"); + scene.ucmd().arg("b").arg("f/").succeeds(); + assert!(at.dir_exists("f/b")); +} + // Todo: // $ at.touch a b From 108dc4a0cddcabae16e009d7d9c71fdc05a40aac Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 4 Jan 2024 00:24:08 +0100 Subject: [PATCH 05/13] Move path_ends_with_terminator from mv into uucore --- src/uu/mv/src/mv.rs | 24 ++++-------------------- src/uucore/src/lib/features/fs.rs | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 19a7c274f..c95e54028 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -23,7 +23,10 @@ use std::path::{Path, PathBuf}; use uucore::backup_control::{self, source_is_target_backup}; use uucore::display::Quotable; use uucore::error::{set_exit_code, FromIo, UResult, USimpleError, UUsageError}; -use uucore::fs::{are_hardlinks_or_one_way_symlink_to_same_file, are_hardlinks_to_same_file}; +use uucore::fs::{ + are_hardlinks_or_one_way_symlink_to_same_file, are_hardlinks_to_same_file, + path_ends_with_terminator, +}; use uucore::update_control; // These are exposed for projects (e.g. nushell) that want to create an `Options` value, which // requires these enums @@ -104,25 +107,6 @@ static OPT_VERBOSE: &str = "verbose"; static OPT_PROGRESS: &str = "progress"; static ARG_FILES: &str = "files"; -/// Returns true if the passed `path` ends with a path terminator. -#[cfg(unix)] -fn path_ends_with_terminator(path: &Path) -> bool { - use std::os::unix::prelude::OsStrExt; - path.as_os_str() - .as_bytes() - .last() - .map_or(false, |&byte| byte == b'/' || byte == b'\\') -} - -#[cfg(windows)] -fn path_ends_with_terminator(path: &Path) -> bool { - use std::os::windows::prelude::OsStrExt; - path.as_os_str() - .encode_wide() - .last() - .map_or(false, |wide| wide == b'/'.into() || wide == b'\\'.into()) -} - #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { let mut app = uu_app(); diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index 6eb809e6d..7033646b6 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -714,6 +714,25 @@ pub fn are_hardlinks_or_one_way_symlink_to_same_file(source: &Path, target: &Pat source_metadata.ino() == target_metadata.ino() && source_metadata.dev() == target_metadata.dev() } +/// Returns true if the passed `path` ends with a path terminator. +#[cfg(unix)] +pub fn path_ends_with_terminator(path: &Path) -> bool { + use std::os::unix::prelude::OsStrExt; + path.as_os_str() + .as_bytes() + .last() + .map_or(false, |&byte| byte == b'/' || byte == b'\\') +} + +#[cfg(windows)] +pub fn path_ends_with_terminator(path: &Path) -> bool { + use std::os::windows::prelude::OsStrExt; + path.as_os_str() + .encode_wide() + .last() + .map_or(false, |wide| wide == b'/'.into() || wide == b'\\'.into()) +} + #[cfg(test)] mod tests { // Note this useful idiom: importing names from outer (for mod tests) scope. From cb27b9c9c35c133f2aae11dd71123c99ba32ccb8 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 4 Jan 2024 00:27:44 +0100 Subject: [PATCH 06/13] path_ends_with_terminator: rustdoc + unittest --- src/uucore/src/lib/features/fs.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index 7033646b6..c9eaa1e01 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -715,7 +715,16 @@ pub fn are_hardlinks_or_one_way_symlink_to_same_file(source: &Path, target: &Pat } /// Returns true if the passed `path` ends with a path terminator. +/// +/// This function examines the last character of the path to determine +/// if it is a directory separator. It supports both Unix-style (`/`) +/// and Windows-style (`\`) separators. +/// +/// # Arguments +/// +/// * `path` - A reference to the path to be checked. #[cfg(unix)] + pub fn path_ends_with_terminator(path: &Path) -> bool { use std::os::unix::prelude::OsStrExt; path.as_os_str() @@ -940,4 +949,24 @@ mod tests { assert_eq!(get_file_display(S_IFSOCK | 0o600), 's'); assert_eq!(get_file_display(0o777), '?'); } + + #[test] + fn test_path_ends_with_terminator() { + // Path ends with a forward slash + assert!(path_ends_with_terminator(Path::new("/some/path/"))); + + // Path ends with a backslash + assert!(path_ends_with_terminator(Path::new("C:\\some\\path\\"))); + + // Path does not end with a terminator + assert!(!path_ends_with_terminator(Path::new("/some/path"))); + assert!(!path_ends_with_terminator(Path::new("C:\\some\\path"))); + + // Empty path + assert!(!path_ends_with_terminator(Path::new(""))); + + // Root path + assert!(path_ends_with_terminator(Path::new("/"))); + assert!(path_ends_with_terminator(Path::new("C:\\"))); + } } From aabf5fa577fc6cdc9b08a5cc92c0083ee53367c6 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 4 Jan 2024 00:41:54 +0100 Subject: [PATCH 07/13] cp: manages target with trailing '/' --- src/uu/cp/src/copydir.rs | 13 +++++++++++-- src/uu/cp/src/cp.rs | 8 ++++++-- tests/by-util/test_cp.rs | 20 ++++++++++++++++++++ util/build-gnu.sh | 3 +++ 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index a903ed2aa..dd3fced53 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -17,7 +17,9 @@ use std::path::{Path, PathBuf, StripPrefixError}; use indicatif::ProgressBar; use uucore::display::Quotable; use uucore::error::UIoError; -use uucore::fs::{canonicalize, FileInformation, MissingHandling, ResolveMode}; +use uucore::fs::{ + canonicalize, path_ends_with_terminator, FileInformation, MissingHandling, ResolveMode, +}; use uucore::show; use uucore::show_error; use uucore::uio_error; @@ -170,7 +172,14 @@ impl Entry { let mut descendant = get_local_to_root_parent(&source_absolute, context.root_parent.as_deref())?; if no_target_dir { - descendant = descendant.strip_prefix(context.root)?.to_path_buf(); + let source_is_dir: bool = direntry.path().is_dir(); + if path_ends_with_terminator(context.target) && source_is_dir { + if let Err(e) = std::fs::create_dir_all(context.target) { + eprintln!("Failed to create directory: {}", e); + } + } else { + descendant = descendant.strip_prefix(context.root)?.to_path_buf(); + } } let local_to_target = context.target.join(descendant); diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 332bb5785..8a4c5623a 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -32,8 +32,8 @@ use platform::copy_on_write; use uucore::display::Quotable; use uucore::error::{set_exit_code, UClapError, UError, UResult, UUsageError}; use uucore::fs::{ - are_hardlinks_to_same_file, canonicalize, is_symlink_loop, paths_refer_to_same_file, - FileInformation, MissingHandling, ResolveMode, + are_hardlinks_to_same_file, canonicalize, is_symlink_loop, path_ends_with_terminator, + paths_refer_to_same_file, FileInformation, MissingHandling, ResolveMode, }; use uucore::{backup_control, update_control}; // These are exposed for projects (e.g. nushell) that want to create an `Options` value, which @@ -1994,6 +1994,10 @@ fn copy_helper( fs::create_dir_all(parent)?; } + if path_ends_with_terminator(dest) && !dest.is_dir() { + return Err(Error::NotADirectory(dest.to_path_buf())); + } + if source.as_os_str() == "/dev/null" { /* workaround a limitation of fs::copy * https://github.com/rust-lang/rust/issues/79390 diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index d166243ed..cb6b7a8dc 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -3681,3 +3681,23 @@ fn test_cp_seen_file() { assert!(at.plus("c").join("f").exists()); assert!(at.plus("c").join("f.~1~").exists()); } + +#[test] +fn test_cp_path_ends_with_terminator() { + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + at.mkdir("a"); + ts.ucmd().arg("-r").arg("-T").arg("a").arg("e/").succeeds(); +} + +#[test] +fn test_cp_no_such() { + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + at.touch("b"); + ts.ucmd() + .arg("b") + .arg("no-such/") + .fails() + .stderr_is("cp: 'no-such/' is not a directory\n"); +} diff --git a/util/build-gnu.sh b/util/build-gnu.sh index 4209b7710..915577c55 100755 --- a/util/build-gnu.sh +++ b/util/build-gnu.sh @@ -206,6 +206,9 @@ sed -i "s|cp: target directory 'symlink': Permission denied|cp: 'symlink' is not # to transform an ERROR into FAIL sed -i 's|xargs mkdir )|xargs mkdir -p )|' tests/cp/link-heap.sh +# Our message is a bit better +sed -i "s|cannot create regular file 'no-such/': Not a directory|'no-such/' is not a directory|" tests/mv/trailing-slash.sh + sed -i 's|cp |/usr/bin/cp |' tests/mv/hard-2.sh sed -i 's|paste |/usr/bin/paste |' tests/od/od-endian.sh sed -i 's|timeout |'"${SYSTEM_TIMEOUT}"' |' tests/tail/follow-stdin.sh From e64a0b4a2637a5b84165acf729f41da98b6bb8cf Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Fri, 5 Jan 2024 10:11:35 +0100 Subject: [PATCH 08/13] Various fixes Co-authored-by: Daniel Hofstetter --- src/uu/cp/src/copydir.rs | 2 +- src/uu/mv/src/mv.rs | 4 ++-- src/uucore/src/lib/features/fs.rs | 1 - 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index dd3fced53..7a9d797e8 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -172,7 +172,7 @@ impl Entry { let mut descendant = get_local_to_root_parent(&source_absolute, context.root_parent.as_deref())?; if no_target_dir { - let source_is_dir: bool = direntry.path().is_dir(); + let source_is_dir = direntry.path().is_dir(); if path_ends_with_terminator(context.target) && source_is_dir { if let Err(e) = std::fs::create_dir_all(context.target) { eprintln!("Failed to create directory: {}", e); diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index c95e54028..223ac9119 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -318,8 +318,8 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()> } } - let target_is_dir: bool = target.is_dir(); - let source_is_dir: bool = source.is_dir(); + let target_is_dir = target.is_dir(); + let source_is_dir = source.is_dir(); if path_ends_with_terminator(target) && (!target_is_dir && !source_is_dir) diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index c9eaa1e01..20cc9e13d 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -724,7 +724,6 @@ pub fn are_hardlinks_or_one_way_symlink_to_same_file(source: &Path, target: &Pat /// /// * `path` - A reference to the path to be checked. #[cfg(unix)] - pub fn path_ends_with_terminator(path: &Path) -> bool { use std::os::unix::prelude::OsStrExt; path.as_os_str() From 4372908e8487416b93836ba5d926b5cc441cd11a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Fri, 5 Jan 2024 13:51:28 +0100 Subject: [PATCH 09/13] fix: cargo fmt + fix spelling mistake --- tests/by-util/test_ls.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 476704660..0162b0170 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -2648,7 +2648,7 @@ fn test_ls_quoting_style_arg_overrides_env_var() { at.touch(at.plus_as_string("bar-2")); // The quoting style given by the env variable should be - // overriden by any escape style provided by argument. + // overridden by any escape style provided by argument. for (arg, correct) in [ ("--quoting-style=literal", "foo-1"), ("-N", "foo-1"), @@ -2684,7 +2684,6 @@ fn test_ls_quoting_style_arg_overrides_env_var() { .succeeds() .stdout_only(format!("{correct}\n")); } - } #[test] From 5de030f119791a873b5a6bfb990c8e1c049aa4d7 Mon Sep 17 00:00:00 2001 From: Michel Lind Date: Fri, 5 Jan 2024 22:20:59 -0600 Subject: [PATCH 10/13] uuhelp_parser: add links to homepage and repo When viewing the crate right now, apart from via looking at the dependencies it's hard to find the associated project and repository. Add the missing info. Signed-off-by: Michel Lind --- src/uuhelp_parser/Cargo.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/uuhelp_parser/Cargo.toml b/src/uuhelp_parser/Cargo.toml index c07bce631..cbf049f9d 100644 --- a/src/uuhelp_parser/Cargo.toml +++ b/src/uuhelp_parser/Cargo.toml @@ -5,3 +5,6 @@ version = "0.0.23" edition = "2021" license = "MIT" description = "A collection of functions to parse the markdown code of help files" + +homepage = "https://github.com/uutils/coreutils" +repository = "https://github.com/uutils/coreutils/tree/main/src/uuhelp_parser" From 9f4330f94cc471d880df7d9089ee1105b27fd321 Mon Sep 17 00:00:00 2001 From: Fabrice Fontaine Date: Sat, 6 Jan 2024 10:26:54 +0100 Subject: [PATCH 11/13] uucore: add support for sparc64 Add support for sparc64 in uucore to avoid the following build failure with nushell: error[E0308]: mismatched types --> /home/autobuild/autobuild/instance-7/output-1/build/nushell-0.85.0/VENDOR/uucore/src/lib/features/fs.rs:121:16 | 111 | pub fn number_of_links(&self) -> u64 { | --- expected `u64` because of return type ... 121 | return self.0.st_nlink; | ^^^^^^^^^^^^^^^ expected `u64`, found `u32` | help: you can convert a `u32` to a `u64` | 121 | return self.0.st_nlink.into(); | +++++++ For more information about this error, try `rustc --explain E0308`. error: could not compile `uucore` (lib) due to previous error Fixes: - http://autobuild.buildroot.org/results/f9f0287a8e39c65895014ca513ed25071f020add Signed-off-by: Fabrice Fontaine --- src/uucore/src/lib/features/fs.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index 20cc9e13d..3b9170bc3 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -121,6 +121,7 @@ impl FileInformation { not(target_arch = "aarch64"), not(target_arch = "riscv64"), not(target_arch = "loongarch64"), + not(target_arch = "sparc64"), target_pointer_width = "64" ))] return self.0.st_nlink; @@ -137,6 +138,7 @@ impl FileInformation { target_arch = "aarch64", target_arch = "riscv64", target_arch = "loongarch64", + target_arch = "sparc64", not(target_pointer_width = "64") ) ))] From 247f2e55bdd5a6da3d34604eb7033fbd7df69b66 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sat, 6 Jan 2024 16:54:29 +0100 Subject: [PATCH 12/13] seq: adjust some error messages. GNU's are better (#5798) * seq: adjust some error messages. GNU's are better tested by tests/seq/seq.pl * uucore: remove todo --------- Co-authored-by: Daniel Hofstetter --- src/uucore/src/lib/features/format/mod.rs | 23 ++++++++++++++++------- tests/by-util/test_seq.rs | 14 ++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/uucore/src/lib/features/format/mod.rs b/src/uucore/src/lib/features/format/mod.rs index d213d0359..4d30753d6 100644 --- a/src/uucore/src/lib/features/format/mod.rs +++ b/src/uucore/src/lib/features/format/mod.rs @@ -57,8 +57,8 @@ pub enum FormatError { IoError(std::io::Error), NoMoreArguments, InvalidArgument(FormatArgument), - TooManySpecs, - NeedAtLeastOneSpec, + TooManySpecs(Vec), + NeedAtLeastOneSpec(Vec), WrongSpecType, } @@ -79,9 +79,16 @@ impl Display for FormatError { "%{}: invalid conversion specification", String::from_utf8_lossy(s) ), - // TODO: The next two should print the spec as well - Self::TooManySpecs => write!(f, "format has too many % directives"), - Self::NeedAtLeastOneSpec => write!(f, "format has no % directive"), + Self::TooManySpecs(s) => write!( + f, + "format '{}' has too many % directives", + String::from_utf8_lossy(s) + ), + Self::NeedAtLeastOneSpec(s) => write!( + f, + "format '{}' has no % directive", + String::from_utf8_lossy(s) + ), // TODO: Error message below needs some work Self::WrongSpecType => write!(f, "wrong % directive type was given"), Self::IoError(_) => write!(f, "io error"), @@ -303,7 +310,9 @@ impl Format { } let Some(spec) = spec else { - return Err(FormatError::NeedAtLeastOneSpec); + return Err(FormatError::NeedAtLeastOneSpec( + format_string.as_ref().to_vec(), + )); }; let formatter = F::try_from_spec(spec)?; @@ -312,7 +321,7 @@ impl Format { for item in &mut iter { match item? { FormatItem::Spec(_) => { - return Err(FormatError::TooManySpecs); + return Err(FormatError::TooManySpecs(format_string.as_ref().to_vec())); } FormatItem::Char(c) => suffix.push(c), } diff --git a/tests/by-util/test_seq.rs b/tests/by-util/test_seq.rs index da28181eb..9b0f9acea 100644 --- a/tests/by-util/test_seq.rs +++ b/tests/by-util/test_seq.rs @@ -766,3 +766,17 @@ fn test_invalid_zero_increment_value() { .no_stdout() .usage_error("invalid Zero increment value: '0'"); } + +#[test] +fn test_invalid_format() { + new_ucmd!() + .args(&["-f", "%%g", "1"]) + .fails() + .no_stdout() + .stderr_contains("format '%%g' has no % directive"); + new_ucmd!() + .args(&["-f", "%g%g", "1"]) + .fails() + .no_stdout() + .stderr_contains("format '%g%g' has too many % directives"); +} From c867d6bfb1695721f1093ce967ebc21fbd847b74 Mon Sep 17 00:00:00 2001 From: Kostiantyn Hryshchuk Date: Sat, 6 Jan 2024 22:50:21 +0100 Subject: [PATCH 13/13] shred: implemented "--remove" arg (#5790) --- src/uu/shred/src/shred.rs | 105 +++++++++++++++++++++++++++--------- tests/by-util/test_shred.rs | 77 ++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 24 deletions(-) diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index a77bfe5e1..b142e2e94 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -28,10 +28,17 @@ pub mod options { pub const FILE: &str = "file"; pub const ITERATIONS: &str = "iterations"; pub const SIZE: &str = "size"; + pub const WIPESYNC: &str = "u"; pub const REMOVE: &str = "remove"; pub const VERBOSE: &str = "verbose"; pub const EXACT: &str = "exact"; pub const ZERO: &str = "zero"; + + pub mod remove { + pub const UNLINK: &str = "unlink"; + pub const WIPE: &str = "wipe"; + pub const WIPESYNC: &str = "wipesync"; + } } // This block size seems to match GNU (2^16 = 65536) @@ -81,6 +88,14 @@ enum PassType { Random, } +#[derive(PartialEq, Clone, Copy)] +enum RemoveMethod { + None, // Default method. Only obfuscate the file data + Unlink, // The same as 'None' + unlink the file + Wipe, // The same as 'Unlink' + obfuscate the file name before unlink + WipeSync, // The same as 'Wipe' sync the file name changes +} + /// Iterates over all possible filenames of a certain length using NAME_CHARSET as an alphabet struct FilenameIter { // Store the indices of the letters of our filename in NAME_CHARSET @@ -219,17 +234,25 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { None => unreachable!(), }; - // TODO: implement --remove HOW - // The optional HOW parameter indicates how to remove a directory entry: - // - 'unlink' => use a standard unlink call. - // - 'wipe' => also first obfuscate bytes in the name. - // - 'wipesync' => also sync each obfuscated byte to disk. - // The default mode is 'wipesync', but note it can be expensive. - // TODO: implement --random-source + let remove_method = if matches.get_flag(options::WIPESYNC) { + RemoveMethod::WipeSync + } else if matches.contains_id(options::REMOVE) { + match matches + .get_one::(options::REMOVE) + .map(AsRef::as_ref) + { + Some(options::remove::UNLINK) => RemoveMethod::Unlink, + Some(options::remove::WIPE) => RemoveMethod::Wipe, + Some(options::remove::WIPESYNC) => RemoveMethod::WipeSync, + _ => unreachable!("should be caught by clap"), + } + } else { + RemoveMethod::None + }; + let force = matches.get_flag(options::FORCE); - let remove = matches.get_flag(options::REMOVE); let size_arg = matches .get_one::(options::SIZE) .map(|s| s.to_string()); @@ -240,7 +263,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { for path_str in matches.get_many::(options::FILE).unwrap() { show_if_err!(wipe_file( - path_str, iterations, remove, size, exact, zero, verbose, force, + path_str, + iterations, + remove_method, + size, + exact, + zero, + verbose, + force, )); } Ok(()) @@ -276,12 +306,26 @@ pub fn uu_app() -> Command { .help("shred this many bytes (suffixes like K, M, G accepted)"), ) .arg( - Arg::new(options::REMOVE) + Arg::new(options::WIPESYNC) .short('u') - .long(options::REMOVE) - .help("truncate and remove file after overwriting; See below") + .help("deallocate and remove file after overwriting") .action(ArgAction::SetTrue), ) + .arg( + Arg::new(options::REMOVE) + .long(options::REMOVE) + .value_name("HOW") + .value_parser([ + options::remove::UNLINK, + options::remove::WIPE, + options::remove::WIPESYNC, + ]) + .num_args(0..=1) + .require_equals(true) + .default_missing_value(options::remove::WIPESYNC) + .help("like -u but give control on HOW to delete; See below") + .action(ArgAction::Set), + ) .arg( Arg::new(options::VERBOSE) .long(options::VERBOSE) @@ -340,7 +384,7 @@ fn pass_name(pass_type: &PassType) -> String { fn wipe_file( path_str: &str, n_passes: usize, - remove: bool, + remove_method: RemoveMethod, size: Option, exact: bool, zero: bool, @@ -457,8 +501,8 @@ fn wipe_file( .map_err_context(|| format!("{}: File write pass failed", path.maybe_quote()))); } - if remove { - do_remove(path, path_str, verbose) + if remove_method != RemoveMethod::None { + do_remove(path, path_str, verbose, remove_method) .map_err_context(|| format!("{}: failed to remove file", path.maybe_quote()))?; } Ok(()) @@ -501,7 +545,7 @@ fn get_file_size(path: &Path) -> Result { // Repeatedly renames the file with strings of decreasing length (most likely all 0s) // Return the path of the file after its last renaming or None if error -fn wipe_name(orig_path: &Path, verbose: bool) -> Option { +fn wipe_name(orig_path: &Path, verbose: bool, remove_method: RemoveMethod) -> Option { let file_name_len = orig_path.file_name().unwrap().to_str().unwrap().len(); let mut last_path = PathBuf::from(orig_path); @@ -526,12 +570,14 @@ fn wipe_name(orig_path: &Path, verbose: bool) -> Option { ); } - // Sync every file rename - let new_file = OpenOptions::new() - .write(true) - .open(new_path.clone()) - .expect("Failed to open renamed file for syncing"); - new_file.sync_all().expect("Failed to sync renamed file"); + if remove_method == RemoveMethod::WipeSync { + // Sync every file rename + let new_file = OpenOptions::new() + .write(true) + .open(new_path.clone()) + .expect("Failed to open renamed file for syncing"); + new_file.sync_all().expect("Failed to sync renamed file"); + } last_path = new_path; break; @@ -552,12 +598,23 @@ fn wipe_name(orig_path: &Path, verbose: bool) -> Option { Some(last_path) } -fn do_remove(path: &Path, orig_filename: &str, verbose: bool) -> Result<(), io::Error> { +fn do_remove( + path: &Path, + orig_filename: &str, + verbose: bool, + remove_method: RemoveMethod, +) -> Result<(), io::Error> { if verbose { show_error!("{}: removing", orig_filename.maybe_quote()); } - if let Some(rp) = wipe_name(path, verbose) { + let remove_path = if remove_method == RemoveMethod::Unlink { + Some(path.with_file_name(orig_filename)) + } else { + wipe_name(path, verbose, remove_method) + }; + + if let Some(rp) = remove_path { fs::remove_file(rp)?; } diff --git a/tests/by-util/test_shred.rs b/tests/by-util/test_shred.rs index 83d2890ed..d5de7882f 100644 --- a/tests/by-util/test_shred.rs +++ b/tests/by-util/test_shred.rs @@ -2,6 +2,9 @@ // // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. + +// spell-checker:ignore wipesync + use crate::common::util::TestScenario; #[test] @@ -9,8 +12,82 @@ fn test_invalid_arg() { new_ucmd!().arg("--definitely-invalid").fails().code_is(1); } +#[test] +fn test_invalid_remove_arg() { + new_ucmd!().arg("--remove=unknown").fails().code_is(1); +} + +#[test] +fn test_shred() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file = "test_shred"; + let file_original_content = "test_shred file content"; + + at.write(file, file_original_content); + + ucmd.arg(file).succeeds(); + + // File exists + assert!(at.file_exists(file)); + // File is obfuscated + assert!(at.read_bytes(file) != file_original_content.as_bytes()); +} + #[test] fn test_shred_remove() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file = "test_shred_remove"; + at.touch(file); + + ucmd.arg("--remove").arg(file).succeeds(); + + // File was deleted + assert!(!at.file_exists(file)); +} + +#[test] +fn test_shred_remove_unlink() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file = "test_shred_remove_unlink"; + at.touch(file); + + ucmd.arg("--remove=unlink").arg(file).succeeds(); + + // File was deleted + assert!(!at.file_exists(file)); +} + +#[test] +fn test_shred_remove_wipe() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file = "test_shred_remove_wipe"; + at.touch(file); + + ucmd.arg("--remove=wipe").arg(file).succeeds(); + + // File was deleted + assert!(!at.file_exists(file)); +} + +#[test] +fn test_shred_remove_wipesync() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file = "test_shred_remove_wipesync"; + at.touch(file); + + ucmd.arg("--remove=wipesync").arg(file).succeeds(); + + // File was deleted + assert!(!at.file_exists(file)); +} + +#[test] +fn test_shred_u() { let scene = TestScenario::new(util_name!()); let at = &scene.fixtures;