mkdir, install, mkfifo, mknod: fix -m's invalid-mode message and an empty-clause bug

src/uu/install/locales/en-US.ftl

@@ -45,7 +45,7 @@ install-error-same-file = { $file1 } and { $file2 } are the same file
 install-error-extra-operand = extra operand { $operand }
 { $usage }
 install-error-not-permitted = cannot remove { $path }: Operation not permitted
-install-error-invalid-mode = Invalid mode string: { $error }
+install-error-invalid-mode = invalid mode '{ $mode }'
 install-error-mutually-exclusive-target = Options --target-directory and --no-target-directory are mutually exclusive
 install-error-mutually-exclusive-compare-strip = Options --compare and --strip are mutually exclusive
 install-error-missing-file-operand = missing file operand

src/uu/install/locales/fr-FR.ftl

@@ -44,7 +44,7 @@ install-error-override-directory-failed = impossible d'écraser le répertoire {
 install-error-same-file = { $file1 } et { $file2 } sont le même fichier
 install-error-extra-operand = opérande supplémentaire { $operand }
   { $usage }
-install-error-invalid-mode = Chaîne de mode invalide : { $error }
+install-error-invalid-mode = mode invalide '{ $mode }'
 install-error-mutually-exclusive-target = Les options --target-directory et --no-target-directory sont mutuellement exclusives
 install-error-mutually-exclusive-compare-strip = Les options --compare et --strip sont mutuellement exclusives
 install-error-missing-file-operand = opérande de fichier manquant

src/uu/install/src/install.rs

@@ -374,7 +374,9 @@ fn behavior(matches: &ArgMatches, diag_args: Option<&[OsString]>) -> UResult<Beh
     let specified_mode: Option<u32> = if matches.contains_id(OPT_MODE) {
         let x = matches.get_one::<String>(OPT_MODE).ok_or(1)?;
         Some(uucore::mode::parse(x, considering_dir, 0).map_err(|err| {
-            let message = translate!("install-error-invalid-mode", "error" => err.to_string());
+            // GNU always says the same thing regardless of what specifically
+            // went wrong, and does not add a "Try --help" hint here.
+            let message = translate!("install-error-invalid-mode", "mode" => x.clone());
             // When the diagnostic is rendered it is already on stderr; exit quietly.
             if !diag_args.is_some_and(|args| err.render_mode_value(args, x, 0, &message)) {
                 show_error!("{message}");

src/uu/mkdir/locales/en-US.ftl

@@ -15,6 +15,7 @@ mkdir-error-file-exists = { $path }: File exists
 mkdir-error-failed-to-create-tree = failed to create whole tree
 mkdir-error-cannot-set-permissions = cannot set permissions { $path }
 mkdir-error-cannot-create-directory = cannot create directory '{ $path }': { $error }
+mkdir-error-invalid-mode = invalid mode '{ $mode }'
 
 # Verbose output
 mkdir-verbose-created-directory = { $util_name }: created directory { $path }

src/uu/mkdir/locales/fr-FR.ftl

@@ -15,6 +15,7 @@ mkdir-error-file-exists = { $path } : Le fichier existe
 mkdir-error-failed-to-create-tree = échec de la création de l'arborescence complète
 mkdir-error-cannot-set-permissions = impossible de définir les permissions { $path }
 mkdir-error-cannot-create-directory = impossible de créer le répertoire '{ $path }': { $error }
+mkdir-error-invalid-mode = mode invalide '{ $mode }'
 
 # Sortie détaillée
 mkdir-verbose-created-directory = { $util_name } : répertoire créé { $path }

src/uu/mkdir/src/mkdir.rs

@@ -70,11 +70,15 @@ fn get_mode(matches: &ArgMatches, diag_args: Option<&[OsString]>) -> UResult<Opt
     mode::parse_chmod(DEFAULT_PERM, m, true, mode::get_umask())
         .map(Some)
         .map_err(|err| {
-            if diag_args.is_some_and(|args| err.render_mode_value(args, m, 0, &err.to_string())) {
+            // GNU always says the same thing regardless of what specifically
+            // went wrong, and unlike `chmod` does not add a "Try --help"
+            // hint here.
+            let message = translate!("mkdir-error-invalid-mode", "mode" => m.clone());
+            if diag_args.is_some_and(|args| err.render_mode_value(args, m, 0, &message)) {
                 // The diagnostic is already on stderr; exit quietly.
                 ExitCode::new(1)
             } else {
-                USimpleError::new(1, err.to_string())
+                USimpleError::new(1, message)
             }
         })
 }

src/uu/mkfifo/locales/en-US.ftl

@@ -7,7 +7,7 @@ mkfifo-help-selinux = set the SELinux security context to default type
 mkfifo-help-context = like -Z, or if CTX is specified then set the SELinux or SMACK security context to CTX
 
 # Error messages
-mkfifo-error-invalid-mode = invalid mode: { $error }
+mkfifo-error-invalid-mode = invalid mode
 mkfifo-error-cannot-create-fifo = cannot create fifo { $path }: { $error }
 mkfifo-error-cannot-set-permissions = cannot set permissions on { $path }: { $error }
 mkfifo-error-non-file-permission = mode must specify only file permission bits

src/uu/mkfifo/locales/fr-FR.ftl

@@ -7,7 +7,7 @@ mkfifo-help-selinux = définir le contexte de sécurité SELinux au type par dé
 mkfifo-help-context = comme -Z, ou si CTX est spécifié, définir le contexte de sécurité SELinux ou SMACK à CTX
 
 # Messages d'erreur
-mkfifo-error-invalid-mode = mode invalide : { $error }
+mkfifo-error-invalid-mode = mode invalide
 mkfifo-error-cannot-create-fifo = impossible de créer le fifo { $path } : { $error }
 mkfifo-error-cannot-set-permissions = impossible de définir les permissions sur { $path } : { $error }
 mkfifo-error-non-file-permission = le mode ne doit spécifier que des bits de permission de fichier

src/uu/mkfifo/src/mkfifo.rs

@@ -28,7 +28,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let matches = uucore::clap_localization::handle_clap_result(uu_app(), args)?;
 
     let mode = calculate_mode(matches.get_one::<String>(options::MODE)).map_err(|err| {
-        let message = translate!("mkfifo-error-invalid-mode", "error" => err.to_string());
+        // Unlike `chmod`/`mkdir`/`install`, GNU does not name the mode
+        // operand here at all, regardless of what went wrong with it.
+        let message = translate!("mkfifo-error-invalid-mode");
         if let Some(args) = &diag_args
             && let Some(mode) = matches.get_one::<String>(options::MODE)
             && err.render_mode_value(args, mode, 0, &message)

src/uu/mknod/locales/en-US.ftl

@@ -28,7 +28,7 @@ mknod-help-context = like -Z, or if CTX is specified then set the SELinux or SMA
 # Error messages
 mknod-error-fifo-no-major-minor = Fifos do not have major and minor device numbers.
 mknod-error-special-require-major-minor = Special files require major and minor device numbers.
-mknod-error-invalid-mode = invalid mode ({ $error })
+mknod-error-invalid-mode = invalid mode
 mknod-error-mode-permission-bits-only = mode must specify only file permission bits
 mknod-error-missing-device-type = missing device type
 mknod-error-invalid-device-type = invalid device type { $type }

src/uu/mknod/locales/fr-FR.ftl

@@ -28,7 +28,7 @@ mknod-help-context = comme -Z, ou si CTX est spécifié, définir le contexte de
 # Messages d'erreur
 mknod-error-fifo-no-major-minor = Les fifos n'ont pas de numéros de périphérique majeur et mineur.
 mknod-error-special-require-major-minor = Les fichiers spéciaux nécessitent des numéros de périphérique majeur et mineur.
-mknod-error-invalid-mode = mode invalide ({ $error })
+mknod-error-invalid-mode = mode invalide
 mknod-error-mode-permission-bits-only = le mode ne doit spécifier que les bits de permission de fichier
 mknod-error-missing-device-type = type de périphérique manquant
 mknod-error-invalid-device-type = type de périphérique invalide { $type }

src/uu/mknod/src/mknod.rs

@@ -155,8 +155,10 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
             let mode =
                 uucore::mode::parse_chmod(MODE_RW_UGO, str_mode, true, uucore::mode::get_umask())
                     .map_err(|err| {
-                    let message =
-                        translate!("mknod-error-invalid-mode", "error" => err.to_string());
+                    // Unlike `chmod`/`mkdir`/`install`, GNU does not name the
+                    // mode operand here at all, regardless of what went wrong
+                    // with it.
+                    let message = translate!("mknod-error-invalid-mode");
                     if let Some(args) = &diag_args
                         && err.render_mode_value(args, str_mode, 0, &message)
                     {

src/uucore/src/lib/features/mode.rs

@@ -131,19 +131,19 @@ 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`].
+    /// Every caller shows the same headline for every `ModeError` -- GNU
+    /// itself only ever says "invalid mode" (with or without the operand,
+    /// depending on the utility) -- so the detail that used to live there
+    /// lives in the label instead.
     fn describe(&self) -> (Option<String>, Option<String>) {
         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
@@ -335,10 +335,12 @@ pub fn parse_chmod(
         offset += raw_part.len() + 1;
 
         let mode_part = raw_part.trim();
-        if mode_part.is_empty() {
-            continue;
-        }
 
+        // An empty clause -- the whole mode, a leading, trailing or doubled
+        // comma -- is not tolerated: GNU rejects `chmod u+rwx, f` the same
+        // way it rejects `chmod , f`. `parse_symbolic` already reports that
+        // correctly for an empty string, so it is not special-cased away
+        // here.
         new_mode = if mode_part.chars().any(|c| c.is_ascii_digit()) {
             parse_numeric(new_mode, mode_part, considering_dir)
         } else {
@@ -469,11 +471,14 @@ mod tests {
     }
 
     #[test]
-    fn test_parse_empty_string() {
-        // Empty string should return 0
-        assert_eq!(parse("", false, 0).unwrap(), 0);
-        assert_eq!(parse("   ", false, 0).unwrap(), 0);
-        assert_eq!(parse(",,", false, 0).unwrap(), 0);
+    fn test_parse_empty_string_is_invalid() {
+        // GNU rejects an empty, all-whitespace, or comma-only mode string
+        // (verified against chmod/install/mkdir 9.11: `chmod '' f`,
+        // `chmod '   ' f`, `chmod ',' f`, `install -m '' ...`,
+        // `mkdir -m '' ...` all fail with "invalid mode").
+        assert!(parse("", false, 0).is_err());
+        assert!(parse("   ", false, 0).is_err());
+        assert!(parse(",,", false, 0).is_err());
     }
 
     #[test]

tests/by-util/test_install.rs

@@ -339,13 +339,28 @@ fn test_install_mode_failing() {
         .arg(dir)
         .arg(mode_arg)
         .fails()
-        .stderr_contains("Invalid mode string: invalid digit found in string");
+        .stderr_contains("invalid mode '999'");
 
     let dest_file = &format!("{dir}/{file}");
     assert!(at.file_exists(file));
     assert!(!at.file_exists(dest_file));
 }
 
+/// GNU says the same thing for any way `-m`'s mode can be malformed, and it
+/// is not the message chmod uses for the same failure: install's has no
+/// colon before the quoted mode, and no "Try --help" hint.
+#[test]
+fn test_install_invalid_mode_names_the_operand() {
+    // The mode is validated before install so much as looks for the source,
+    // so a nonexistent one does not confuse this with a different error.
+    for mode in ["999", "", "a", "u?rwx"] {
+        new_ucmd!()
+            .args(&["source_file", "dest", "--mode", mode])
+            .fails_with_code(1)
+            .stderr_only(format!("install: invalid mode '{mode}'\n"));
+    }
+}
+
 #[test]
 fn test_install_mode_directories() {
     let (at, mut ucmd) = at_and_ucmd!();

tests/by-util/test_mkdir.rs

@@ -108,6 +108,40 @@ fn test_mkdir_mode() {
     new_ucmd!().arg("-m").arg("755").arg("test_dir").succeeds();
 }
 
+/// GNU says the same thing for any way `-m`'s mode can be malformed, and it
+/// is not the message chmod uses for the same failure: mkdir's has no
+/// colon before the quoted mode, and no "Try --help" hint.
+///
+/// `-m` is a no-op on Windows (`get_mode` always returns `None` there), so
+/// there is nothing to validate and nothing to fail on that platform.
+#[test]
+#[cfg(not(windows))]
+fn test_mkdir_invalid_mode_names_the_operand() {
+    for mode in ["999", "", "a", "u?rwx"] {
+        new_ucmd!()
+            .args(&["-m", mode, "test_dir"])
+            .fails_with_code(1)
+            .stderr_only(format!("mkdir: invalid mode '{mode}'\n"));
+    }
+}
+
+/// A comma-separated mode with an empty clause -- leading, trailing, or
+/// doubled -- is not tolerated as a no-op clause: GNU rejects the whole
+/// thing, the same as it rejects an empty mode outright.
+///
+/// `-m` is a no-op on Windows (`get_mode` always returns `None` there), so
+/// there is nothing to validate and nothing to fail on that platform.
+#[test]
+#[cfg(not(windows))]
+fn test_mkdir_rejects_an_empty_clause() {
+    for mode in ["u+rwx,", ",u+rwx", "u+rwx,,g+r"] {
+        new_ucmd!()
+            .args(&["-m", mode, "test_dir"])
+            .fails_with_code(1)
+            .stderr_only(format!("mkdir: invalid mode '{mode}'\n"));
+    }
+}
+
 #[test]
 fn test_mkdir_parent() {
     let scene = TestScenario::new(util_name!());

tests/by-util/test_mkfifo.rs

@@ -83,6 +83,18 @@ fn test_create_one_fifo_already_exists() {
         .stderr_is("mkfifo: cannot create fifo 'abcdef': File exists\n");
 }
 
+/// Unlike `chmod`/`mkdir`/`install`, GNU does not name the mode operand at
+/// all here, regardless of what specifically was wrong with it.
+#[test]
+fn test_invalid_mode_is_not_named() {
+    for mode in ["999", "", "a", "u?rwx"] {
+        new_ucmd!()
+            .args(&["-m", mode, "probe"])
+            .fails_with_code(1)
+            .stderr_only("mkfifo: invalid mode\n");
+    }
+}
+
 #[test]
 fn test_create_fifo_with_mode_and_umask() {
     use uucore::fs::display_permissions;

tests/by-util/test_mknod.rs

@@ -135,7 +135,19 @@ fn test_mknod_invalid_mode() {
         .fails()
         .no_stdout()
         .code_is(1)
-        .stderr_contains("invalid mode");
+        .stderr_only("mknod: invalid mode\n");
+}
+
+/// Unlike `chmod`/`mkdir`/`install`, GNU does not name the mode operand at
+/// all here, regardless of what specifically was wrong with it.
+#[test]
+fn test_invalid_mode_is_not_named() {
+    for mode in ["999", "", "a", "u?rwx"] {
+        new_ucmd!()
+            .args(&["--mode", mode, "probe", "p"])
+            .fails_with_code(1)
+            .stderr_only("mknod: invalid mode\n");
+    }
 }
 
 #[test]