From 63cf1cc6ad3e5b9aebab86acfe253e0fd1f2c209 Mon Sep 17 00:00:00 2001 From: arbelonson-source <269032023+arbelonson-source@users.noreply.github.com> Date: Mon, 31 Aug 2026 05:08:34 +0300 Subject: [PATCH] chmod: name the whole mode operand in an invalid-mode error GNU says the same thing for every way a `chmod` mode can be malformed: $ chmod 999 f chmod: invalid mode: '999' Try 'chmod --help' for more information. $ chmod a f chmod: invalid mode: 'a' Try 'chmod --help' for more information. uutils instead surfaced a message shaped like whatever failed to parse internally: $ chmod 999 f chmod: invalid digit found in string # a raw ParseIntError $ chmod a f chmod: invalid mode (a) # wrong punctuation, no hint and for a value that parsed as octal but was out of range: $ chmod 17777 f chmod: mode is too large (17777 > 7777) # ours, before chmod: invalid mode: '17777' # GNU GNU also always names the *whole* mode operand, not just the clause that broke: `chmod u+rwx,z+r f` reports `'u+rwx,z+r'`, not `'z+r'`. The fix is at the one place chmod already builds its plain error message, using the whole mode string it already has on hand, and switching from `USimpleError` to `UUsageError` so the "Try --help" hint that was missing gets added the same way every other usage error in the codebase gets it. The per-kind detail this replaces at the headline (which operator was invalid, which one was expected) isn't lost -- it moves to the label under the caret diagram chmod already draws for a `-m`/positional mode on a real terminal, which GNU has no equivalent of. `describe()` previously left that case unlabelled on the assumption that the headline already said it; now that the headline is uniform, it needs to say it instead. `install`, `mkdir`, `mkfifo` and `mknod` share this same mode parser and very likely have the identical bug in their own `-m` handling -- not touched here, since each has occasionally slightly different GNU wording (`mkdir` omits the colon: "invalid mode 'x'", not "invalid mode: 'x'") and deserves its own verification rather than assuming chmod's fix applies as-is. --- src/uu/chmod/locales/en-US.ftl | 1 + src/uu/chmod/locales/fr-FR.ftl | 1 + src/uu/chmod/src/chmod.rs | 17 ++++++++++----- src/uucore/src/lib/features/mode.rs | 19 ++++++++--------- tests/by-util/test_chmod.rs | 32 +++++++++++++++++++++-------- 5 files changed, 46 insertions(+), 24 deletions(-) diff --git a/src/uu/chmod/locales/en-US.ftl b/src/uu/chmod/locales/en-US.ftl index 2f3f6748461..0df78e83b01 100644 --- a/src/uu/chmod/locales/en-US.ftl +++ b/src/uu/chmod/locales/en-US.ftl @@ -15,6 +15,7 @@ chmod-error-permission-denied = cannot access {$file}: Permission denied chmod-error-new-permissions = {$file}: new permissions are {$actual}, not {$expected} chmod-error-changing-permissions = changing permissions of {$file}: {$err} chmod-error-missing-operand = missing operand +chmod-error-invalid-mode = invalid mode: '{$mode}' # Help messages chmod-help-print-help = Print help information. diff --git a/src/uu/chmod/locales/fr-FR.ftl b/src/uu/chmod/locales/fr-FR.ftl index e8116f08f9c..aa7580607c9 100644 --- a/src/uu/chmod/locales/fr-FR.ftl +++ b/src/uu/chmod/locales/fr-FR.ftl @@ -27,6 +27,7 @@ chmod-error-permission-denied = impossible d'accéder à {$file} : Permission re chmod-error-new-permissions = {$file} : les nouvelles permissions sont {$actual}, pas {$expected} chmod-error-changing-permissions = changement des permissions de {$file} : {$err} chmod-error-missing-operand = opérande manquant +chmod-error-invalid-mode = mode invalide : '{$mode}' # Messages verbeux/de statut chmod-verbose-failed-dangling = échec du changement de mode de {$file} de 0000 (---------) vers 1500 (r-x-----T) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 6875ac66b84..f90631cb68a 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -13,9 +13,7 @@ use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::{Path, PathBuf}; use thiserror::Error; use uucore::display::Quotable; -use uucore::error::{ - ExitCode, UError, UResult, USimpleError, UUsageError, set_exit_code, strip_errno, -}; +use uucore::error::{ExitCode, UError, UResult, UUsageError, set_exit_code, strip_errno}; use uucore::fs::{FileInformation, display_permissions_unix}; use uucore::mode; use uucore::perms::{TraverseSymlinks, configure_symlink_and_recursion}; @@ -349,15 +347,24 @@ impl Chmoder { if self.quiet { return Err(ExitCode::new(1)); } + // GNU always names the whole mode operand, not the + // clause that broke, and always with this one + // message regardless of what specifically went + // wrong -- the more specific `error` still supplies + // the caret diagram's label when one is rendered. + let message = translate!( + "chmod-error-invalid-mode", + "mode" => cmode_unwrapped.clone() + ); if let Some(args) = &self.args && let Some((index, operand, offset)) = self.locate_clause(args, &cmode_unwrapped, clause_start) - && error.render_at(args, index, &operand, offset, &error.to_string()) + && error.render_at(args, index, &operand, offset, &message) { // The diagnostic is already on stderr; exit quietly. return Err(ExitCode::new(1)); } - return Err(USimpleError::new(1, error.to_string())); + return Err(UUsageError::new(1, message)); } } } diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index 8dfeccb4c94..3a17fd87937 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -131,19 +131,18 @@ impl ModeError { /// The caret label for this error, translated, and the advice that goes /// under it. /// - /// Labelled only where a label would add to the message, per the - /// convention in [`crate::diagnostics`]. + /// The headline callers show is the same for every `ModeError` — GNU + /// itself only ever says "invalid mode: 'WHOLE_MODE'" — so the detail + /// that used to live there moves down to the label instead. fn describe(&self) -> (Option, Option) { let label = match self.kind { - // The message already names the expected operators. - ModeErrorKind::InvalidOperator => None, - ModeErrorKind::MissingOperator => Some("mode-diag-label-missing-operator"), - ModeErrorKind::InvalidNumber => Some("mode-diag-label-invalid-number"), + // `self.message` already names the operator that was expected + // and the one that was found instead. + ModeErrorKind::InvalidOperator => Some(self.message.clone()), + ModeErrorKind::MissingOperator => Some(translate!("mode-diag-label-missing-operator")), + ModeErrorKind::InvalidNumber => Some(translate!("mode-diag-label-invalid-number")), }; - ( - label.map(|label| translate!(label)), - Some(translate!("mode-diag-help-syntax")), - ) + (label, Some(translate!("mode-diag-help-syntax"))) } /// Where this error sits inside the whole mode, given where the clause it diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index ca13fdef473..30d7f47904f 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -285,6 +285,26 @@ fn test_chmod_error_permissions() { ); } +/// GNU says the same thing, "invalid mode: 'WHOLE_MODE'", for every way a +/// mode can be malformed -- a non-octal digit, a value too large, a missing +/// operator, or an unrecognized one -- so uutils does too, rather than +/// leaking the reason as a distinct message shaped like whatever failed to +/// parse it internally (a raw `ParseIntError`, for one). +#[test] +fn test_invalid_mode_names_the_whole_operand() { + let scenario = TestScenario::new(util_name!()); + let at = &scenario.fixtures; + at.touch("file"); + + for mode in ["999", "", "a", "u?rwx", "u+rwx,z+r", "12x"] { + scenario + .ucmd() + .args(&[mode, "file"]) + .fails_with_code(1) + .usage_error(format!("invalid mode: '{mode}'")); + } +} + #[test] fn test_chmod_permissions_too_large() { let scenario = TestScenario::new(util_name!()); @@ -296,19 +316,13 @@ fn test_chmod_permissions_too_large() { .ucmd() .args(&["10777", "file"]) .fails_with_code(1) - .stderr_is( - // spell-checker:disable-next-line - "chmod: mode is too large (10777 > 7777)\n", - ); + .usage_error("invalid mode: '10777'"); // test around the boundary of the acceptable octal mode scenario .ucmd() .args(&["10000", "file"]) .fails_with_code(1) - .stderr_is( - // spell-checker:disable-next-line - "chmod: mode is too large (10000 > 7777)\n", - ); + .usage_error("invalid mode: '10000'"); at.mkdir("dir"); scenario.ucmd().args(&["7777", "dir"]).succeeds(); } @@ -1820,7 +1834,7 @@ mod diagnostics { // The test harness pipes stderr, so the report must not appear. ucmd.args(&["g+rw?x", "probe"]) .fails_with_code(1) - .stderr_only("chmod: invalid operator (expected +, -, or =, but found ?)\n"); + .usage_error("invalid mode: 'g+rw?x'"); } #[cfg(unix)]