From 29f44566d4f80c03f7e6c889c8fe3bf4af41abbb Mon Sep 17 00:00:00 2001 From: arbelonson-source <269032023+arbelonson-source@users.noreply.github.com> Date: Mon, 31 Aug 2026 06:28:43 +0300 Subject: [PATCH] shred: match GNU's error for a bad --remove value --remove validated its value with a plain ShortcutValueParser, so an unrecognized or ambiguous value produced clap's own generic wording instead of GNU's ('invalid argument ... for --remove / Valid arguments are: / - 'unlink' / ...'). Resolve the value against the option's own choice list by hand, accepting any unambiguous abbreviation the way GNU does, checking for an exact match first since one choice ('wipe') is itself a prefix of another ('wipesync') -- without that check, --remove=wipe would wrongly report itself as ambiguous. Moved this resolution above the missing-file-operand check: GNU validates an option's own value during option parsing, before it looks at operands at all, so 'shred --remove=wip' (no file) reports the ambiguous argument, not a missing operand. AI-assisted-by: Claude Opus 5, via Claude Code --- src/uu/shred/locales/en-US.ftl | 8 ++++ src/uu/shred/locales/fr-FR.ftl | 8 ++++ src/uu/shred/src/shred.rs | 74 ++++++++++++++++++++++++---------- tests/by-util/test_shred.rs | 24 ++++++++++- 4 files changed, 90 insertions(+), 24 deletions(-) diff --git a/src/uu/shred/locales/en-US.ftl b/src/uu/shred/locales/en-US.ftl index b2f749a0b82..911b15162d0 100644 --- a/src/uu/shred/locales/en-US.ftl +++ b/src/uu/shred/locales/en-US.ftl @@ -45,6 +45,14 @@ shred-no-such-file-or-directory = {$file}: No such file or directory shred-failed-to-open-for-writing-not-a-directory = {$file}: failed to open for writing: Not a directory shred-failed-to-open-for-writing-is-a-directory = {$file}: failed to open for writing: Is a directory shred-not-a-file = {$file}: Not a file +shred-invalid-remove-choice = invalid argument '{$arg}' for '--remove' + Valid arguments are: + {$choices} + Try 'shred --help' for more information. +shred-ambiguous-remove-choice = ambiguous argument '{$arg}' for '--remove' + Valid arguments are: + {$choices} + Try 'shred --help' for more information. # Option help text shred-force-help = change permissions to allow writing if necessary diff --git a/src/uu/shred/locales/fr-FR.ftl b/src/uu/shred/locales/fr-FR.ftl index d03b9d74ad8..7fb8809d25a 100644 --- a/src/uu/shred/locales/fr-FR.ftl +++ b/src/uu/shred/locales/fr-FR.ftl @@ -44,6 +44,14 @@ shred-no-such-file-or-directory = {$file} : Aucun fichier ou répertoire de ce t shred-failed-to-open-for-writing-not-a-directory = {$file} : impossible d'ouvrir en écriture : N'est pas un répertoire shred-failed-to-open-for-writing-is-a-directory = {$file} : impossible d'ouvrir en écriture : Est un répertoire shred-not-a-file = {$file} : N'est pas un fichier +shred-invalid-remove-choice = argument '{$arg}' invalide pour '--remove' + Les arguments valides sont : + {$choices} + Essayez 'shred --help' pour plus d'informations. +shred-ambiguous-remove-choice = argument '{$arg}' ambigu pour '--remove' + Les arguments valides sont : + {$choices} + Essayez 'shred --help' pour plus d'informations. # Texte d'aide des options shred-force-help = modifier les permissions pour permettre l'écriture si nécessaire diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index 84037891828..ea2845e8411 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -20,7 +20,6 @@ use uucore::diagnostics::OptionValue; use uucore::display::Quotable; use uucore::error::{FromIo, UResult, USimpleError, UUsageError}; use uucore::parser::parse_size::parse_size_u64; -use uucore::parser::shortcut_value_parser::ShortcutValueParser; use uucore::translate; use uucore::{format_usage, show_error, show_if_err}; @@ -104,6 +103,42 @@ enum RemoveMethod { WipeSync, // The same as 'Wipe' sync the file name changes } +/// The choices `--remove`'s value accepts, in the order GNU lists them. +const REMOVE_CHOICES: &[&str] = &[ + options::remove::UNLINK, + options::remove::WIPE, + options::remove::WIPESYNC, +]; + +/// The choice `value` names among `REMOVE_CHOICES`, accepting any +/// unambiguous abbreviation the way GNU does. +fn resolve_remove_choice(value: &str) -> UResult<&'static str> { + let list = || { + REMOVE_CHOICES + .iter() + .map(|name| format!(" - '{name}'")) + .collect::>() + .join("\n") + }; + if !value.is_empty() + && let Some(&exact) = REMOVE_CHOICES.iter().find(|name| **name == value) + { + return Ok(exact); + } + let mut named = REMOVE_CHOICES.iter().filter(|name| name.starts_with(value)); + match (named.next(), named.next()) { + (Some(name), None) if !value.is_empty() => Ok(*name), + (Some(_), Some(_)) => Err(USimpleError::new( + 1, + translate!("shred-ambiguous-remove-choice", "arg" => value.to_string(), "choices" => list()), + )), + _ => Err(USimpleError::new( + 1, + translate!("shred-invalid-remove-choice", "arg" => value.to_string(), "choices" => list()), + )), + } +} + /// 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 @@ -253,6 +288,22 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { 1, )?; + // GNU validates an option's own value (e.g. a bad --remove argument) + // during option parsing, before it ever looks at the operands, so this + // must run before the missing-file-operand check below. + let remove_method = if matches.get_flag(options::WIPESYNC) { + RemoveMethod::WipeSync + } else if let Some(value) = matches.get_one::(options::REMOVE) { + match resolve_remove_choice(value)? { + options::remove::UNLINK => RemoveMethod::Unlink, + options::remove::WIPE => RemoveMethod::Wipe, + options::remove::WIPESYNC => RemoveMethod::WipeSync, + _ => unreachable!("resolve_remove_choice only returns a valid choice"), + } + } else { + RemoveMethod::None + }; + if !matches.contains_id(options::FILE) { return Err(UUsageError::new( 1, @@ -277,22 +328,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { None => None, }; - 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 size_arg = matches .get_one::(options::SIZE) @@ -358,11 +393,6 @@ pub fn uu_app() -> Command { Arg::new(options::REMOVE) .long(options::REMOVE) .value_name("HOW") - .value_parser(ShortcutValueParser::new([ - options::remove::UNLINK, - options::remove::WIPE, - options::remove::WIPESYNC, - ])) .num_args(0..=1) .require_equals(true) .default_missing_value(options::remove::WIPESYNC) diff --git a/tests/by-util/test_shred.rs b/tests/by-util/test_shred.rs index efbf4466581..d5182456313 100644 --- a/tests/by-util/test_shred.rs +++ b/tests/by-util/test_shred.rs @@ -23,12 +23,32 @@ fn test_invalid_arg() { #[test] fn test_invalid_remove_arg() { - new_ucmd!().arg("--remove=unknown").fails_with_code(1); + new_ucmd!() + .arg("--remove=unknown") + .fails_with_code(1) + .stderr_is(concat!( + "shred: invalid argument 'unknown' for '--remove'\n", + "Valid arguments are:\n", + " - 'unlink'\n", + " - 'wipe'\n", + " - 'wipesync'\n", + "Try 'shred --help' for more information.\n", + )); } #[test] fn test_ambiguous_remove_arg() { - new_ucmd!().arg("--remove=wip").fails_with_code(1); + new_ucmd!() + .arg("--remove=wip") + .fails_with_code(1) + .stderr_is(concat!( + "shred: ambiguous argument 'wip' for '--remove'\n", + "Valid arguments are:\n", + " - 'unlink'\n", + " - 'wipe'\n", + " - 'wipesync'\n", + "Try 'shred --help' for more information.\n", + )); } #[test]