chmod: fix GNU test 'chmod/usage'

src/uu/chmod/src/chmod.rs

@@ -8,6 +8,7 @@
 // spell-checker:ignore (ToDO) Chmoder cmode fmode fperm fref ugoa RFILE RFILE's
 
 use clap::{crate_version, Arg, ArgAction, Command};
+use std::ffi::OsString;
 use std::fs;
 use std::os::unix::fs::{MetadataExt, PermissionsExt};
 use std::path::Path;
@@ -35,14 +36,64 @@ mod options {
     pub const FILE: &str = "FILE";
 }
 
+/// Extract negative modes (starting with '-') from the rest of the arguments.
+///
+/// This is mainly required for GNU compatibility, where "non-positional negative" modes are used
+/// as the actual positional MODE. Some examples of these cases are:
+/// * "chmod -w -r file", which is the same as "chmod -w,-r file"
+/// * "chmod -w file -r", which is the same as "chmod -w,-r file"
+///
+/// These can currently not be handled by clap.
+/// Therefore it might be possible that a pseudo MODE is inserted to pass clap parsing.
+/// The pseudo MODE is later replaced by the extracted (and joined) negative modes.
+fn extract_negative_modes(mut args: impl uucore::Args) -> (Option<String>, Vec<OsString>) {
+    // we look up the args until "--" is found
+    // "-mode" will be extracted into parsed_cmode_vec
+    let (parsed_cmode_vec, pre_double_hyphen_args): (Vec<OsString>, Vec<OsString>) =
+        args.by_ref().take_while(|a| a != "--").partition(|arg| {
+            let arg = if let Some(arg) = arg.to_str() {
+                arg.to_string()
+            } else {
+                return false;
+            };
+            arg.len() >= 2
+                && arg.starts_with('-')
+                && matches!(
+                    arg.chars().nth(1).unwrap(),
+                    'r' | 'w' | 'x' | 'X' | 's' | 't' | 'u' | 'g' | 'o' | '0'..='7'
+                )
+        });
+
+    let mut clean_args = Vec::new();
+    if !parsed_cmode_vec.is_empty() {
+        // we need a pseudo cmode for clap, which won't be used later.
+        // this is required because clap needs the default "chmod MODE FILE" scheme.
+        clean_args.push("w".into());
+    }
+    clean_args.extend(pre_double_hyphen_args);
+
+    if let Some(arg) = args.next() {
+        // as there is still something left in the iterator, we previously consumed the "--"
+        // -> add it to the args again
+        clean_args.push("--".into());
+        clean_args.push(arg);
+    }
+    clean_args.extend(args);
+
+    let parsed_cmode = Some(
+        parsed_cmode_vec
+            .iter()
+            .map(|s| s.to_str().unwrap())
+            .collect::<Vec<&str>>()
+            .join(","),
+    )
+    .filter(|s| !s.is_empty());
+    (parsed_cmode, clean_args)
+}
+
 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
-    let mut args = args.collect_lossy();
-
-    // Before we can parse 'args' with clap (and previously getopts),
-    // a possible MODE prefix '-' needs to be removed (e.g. "chmod -x FILE").
-    let mode_had_minus_prefix = mode::strip_minus_from_mode(&mut args);
-
+    let (parsed_cmode, args) = extract_negative_modes(args.skip(1)); // skip binary name
     let matches = uu_app().after_help(LONG_USAGE).try_get_matches_from(args)?;
 
     let changes = matches.get_flag(options::CHANGES);
@@ -62,13 +113,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         },
         None => None,
     };
-    let modes = matches.get_one::<String>(options::MODE).unwrap(); // should always be Some because required
-    let cmode = if mode_had_minus_prefix {
-        // clap parsing is finished, now put prefix back
-        format!("-{modes}")
+
+    let modes = matches.get_one::<String>(options::MODE);
+    let cmode = if let Some(parsed_cmode) = parsed_cmode {
+        parsed_cmode
     } else {
-        modes.to_string()
+        modes.unwrap().to_string() // modes is required
     };
+    // FIXME: enable non-utf8 paths
     let mut files: Vec<String> = matches
         .get_many::<String>(options::FILE)
         .map(|v| v.map(ToString::to_string).collect())
@@ -107,6 +159,7 @@ pub fn uu_app() -> Command {
         .override_usage(format_usage(USAGE))
         .args_override_self(true)
         .infer_long_args(true)
+        .no_binary_name(true)
         .arg(
             Arg::new(options::CHANGES)
                 .long(options::CHANGES)
@@ -376,3 +429,34 @@ impl Chmoder {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_extract_negative_modes() {
+        // "chmod -w -r file" becomes "chmod -w,-r file". clap does not accept "-w,-r" as MODE.
+        // Therefore, "w" is added as pseudo mode to pass clap.
+        let (c, a) = extract_negative_modes(vec!["-w", "-r", "file"].iter().map(OsString::from));
+        assert_eq!(c, Some("-w,-r".to_string()));
+        assert_eq!(a, vec!["w", "file"]);
+
+        // "chmod -w file -r" becomes "chmod -w,-r file". clap does not accept "-w,-r" as MODE.
+        // Therefore, "w" is added as pseudo mode to pass clap.
+        let (c, a) = extract_negative_modes(vec!["-w", "file", "-r"].iter().map(OsString::from));
+        assert_eq!(c, Some("-w,-r".to_string()));
+        assert_eq!(a, vec!["w", "file"]);
+
+        // "chmod -w -- -r file" becomes "chmod -w -r file", where "-r" is interpreted as file.
+        // Again, "w" is needed as pseudo mode.
+        let (c, a) = extract_negative_modes(vec!["-w", "--", "-r", "f"].iter().map(OsString::from));
+        assert_eq!(c, Some("-w".to_string()));
+        assert_eq!(a, vec!["w", "--", "-r", "f"]);
+
+        // "chmod -- -r file" becomes "chmod -r file".
+        let (c, a) = extract_negative_modes(vec!["--", "-r", "file"].iter().map(OsString::from));
+        assert_eq!(c, None);
+        assert_eq!(a, vec!["--", "-r", "file"]);
+    }
+}

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

@@ -122,6 +122,15 @@ fn parse_change(mode: &str, fperm: u32, considering_dir: bool) -> (u32, usize) {
             'o' => srwx = ((fperm << 6) & 0o700) | ((fperm << 3) & 0o070) | (fperm & 0o007),
             _ => break,
         };
+        if ch == 'u' || ch == 'g' || ch == 'o' {
+            // symbolic modes only allows perms to be a single letter of 'ugo'
+            // therefore this must either be the first char or it is unexpected
+            if pos != 0 {
+                break;
+            }
+            pos = 1;
+            break;
+        }
         pos += 1;
     }
     if pos == 0 {

tests/by-util/test_chmod.rs

@@ -4,9 +4,8 @@ use std::fs::{metadata, set_permissions, OpenOptions, Permissions};
 use std::os::unix::fs::{OpenOptionsExt, PermissionsExt};
 use std::sync::Mutex;
 
-extern crate libc;
-use uucore::mode::strip_minus_from_mode;
 extern crate chmod;
+extern crate libc;
 use self::libc::umask;
 
 static TEST_FILE: &str = "file";
@@ -503,35 +502,6 @@ fn test_chmod_symlink_non_existing_file_recursive() {
         .no_stderr();
 }
 
-#[test]
-fn test_chmod_strip_minus_from_mode() {
-    let tests = vec![
-        // ( before, after )
-        ("chmod -v -xw -R FILE", "chmod -v xw -R FILE"),
-        ("chmod g=rwx FILE -c", "chmod g=rwx FILE -c"),
-        (
-            "chmod -c -R -w,o+w FILE --preserve-root",
-            "chmod -c -R w,o+w FILE --preserve-root",
-        ),
-        ("chmod -c -R +w FILE ", "chmod -c -R +w FILE "),
-        ("chmod a=r,=xX FILE", "chmod a=r,=xX FILE"),
-        (
-            "chmod -v --reference REF_FILE -R FILE",
-            "chmod -v --reference REF_FILE -R FILE",
-        ),
-        ("chmod -Rvc -w-x FILE", "chmod -Rvc w-x FILE"),
-        ("chmod 755 -v FILE", "chmod 755 -v FILE"),
-        ("chmod -v +0004 FILE -R", "chmod -v +0004 FILE -R"),
-        ("chmod -v -0007 FILE -R", "chmod -v 0007 FILE -R"),
-    ];
-
-    for test in tests {
-        let mut args: Vec<String> = test.0.split(' ').map(|v| v.to_string()).collect();
-        let _mode_had_minus_prefix = strip_minus_from_mode(&mut args);
-        assert_eq!(test.1, args.join(" "));
-    }
-}
-
 #[test]
 fn test_chmod_keep_setgid() {
     for (from, arg, to) in [
@@ -691,7 +661,7 @@ fn test_gnu_options() {
 }
 
 #[test]
-fn test_gnu_reoccuring_options() {
+fn test_gnu_repeating_options() {
     let scene = TestScenario::new(util_name!());
     let at = &scene.fixtures;
     at.touch("file");