chmod: only report umask-curtailed modes for option-like operands

src/uu/chmod/src/chmod.rs

@@ -136,6 +136,10 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     };
 
     let modes = matches.get_one::<String>(options::MODE);
+    // Whether the mode reached us as an option-like operand ("chmod -w f") rather than as an
+    // ordinary positional operand ("chmod -- -w f"). This decides whether the umask diagnostic
+    // below is emitted; see the comment on `option_like_mode`.
+    let option_like_mode = parsed_cmode.is_some();
     let cmode = if let Some(parsed_cmode) = parsed_cmode {
         parsed_cmode
     } else {
@@ -173,6 +177,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         recursive,
         fmode,
         cmode,
+        option_like_mode,
         traverse_symlinks,
         dereference,
     };
@@ -270,6 +275,11 @@ struct Chmoder {
     recursive: bool,
     fmode: Option<u32>,
     cmode: Option<String>,
+    /// Set when the mode was given as an option-like operand, e.g. `chmod -w f`, instead of as a
+    /// plain positional operand, e.g. `chmod -- -w f`. GNU only reports a mode whose effect was
+    /// curtailed by the umask for the first spelling: the second one is unambiguous, so there is
+    /// nothing to warn about.
+    option_like_mode: bool,
     traverse_symlinks: TraverseSymlinks,
     dereference: bool,
 }
@@ -787,8 +797,12 @@ impl Chmoder {
             } else {
                 self.change_file(fperm, new_mode, file)?;
             }
-            // if a permission would have been removed if umask was 0, but it wasn't because umask was not 0, print an error and fail
-            if (new_mode & !naively_expected_new_mode) != 0 {
+            // A bare mode such as `-w` is umask-relative, so the umask can keep permissions that
+            // the user asked to drop. GNU reports that as an error, but only when the mode was
+            // written in the option-like form (`chmod -w f`), where it doubles as a hint that the
+            // argument was consumed as a mode. After `--` the operand is unambiguous and GNU stays
+            // silent, so the diagnostic is suppressed here too.
+            if self.option_like_mode && (new_mode & !naively_expected_new_mode) != 0 {
                 return Err(ChmodError::NewPermissions(
                     file.into(),
                     display_permissions_unix(new_mode, false),

tests/by-util/test_chmod.rs

@@ -887,6 +887,141 @@ fn test_gnu_special_options() {
     scene.ucmd().arg("--").arg("--").fails();
 }
 
+/// Every row of the operand matrix from
+/// <https://github.com/uutils/coreutils/issues/3147>.
+///
+/// The left column is the argument list, the right column is the exact list of operands, in order,
+/// that must end up being changed. Three files named `f`, `--` and `-w` exist in every case, so a
+/// row that expects `[]` is asserting that the arguments are rejected outright rather than being
+/// resolved to some file that happens to exist. None of the rows are redundant: they only differ
+/// pairwise in where a `--` sits, which is precisely what decides whether a leading-hyphen argument
+/// is a mode or a file name.
+#[test]
+#[cfg(not(target_os = "android"))]
+fn test_gnu_usage_matrix() {
+    let matrix: &[(&[&str], &[&str])] = &[
+        (&["--"], &[]),
+        (&["--", "--"], &[]),
+        (&["--", "--", "--", "f"], &["--", "f"]),
+        (&["--", "--", "-w", "f"], &["-w", "f"]),
+        (&["--", "--", "f"], &["f"]),
+        (&["--", "-w"], &[]),
+        (&["--", "-w", "--", "f"], &["--", "f"]),
+        (&["--", "-w", "-w", "f"], &["-w", "f"]),
+        (&["--", "-w", "f"], &["f"]),
+        (&["--", "f"], &[]),
+        (&["-w"], &[]),
+        (&["-w", "--"], &[]),
+        (&["-w", "--", "--", "f"], &["--", "f"]),
+        (&["-w", "--", "-w", "f"], &["-w", "f"]),
+        (&["-w", "--", "f"], &["f"]),
+        (&["-w", "-w"], &[]),
+        (&["-w", "-w", "--", "f"], &["f"]),
+        (&["-w", "-w", "-w", "f"], &["f"]),
+        (&["-w", "-w", "f"], &["f"]),
+        (&["-w", "f"], &["f"]),
+        (&["f"], &[]),
+        (&["f", "--"], &[]),
+        (&["f", "-w"], &["f"]),
+        (&["f", "f"], &[]),
+        (&["u+gr", "f"], &[]),
+        (&["ug,+x", "f"], &[]),
+    ];
+
+    for (args, expected) in matrix {
+        let scene = TestScenario::new(util_name!());
+        let at = &scene.fixtures;
+        // 0o644 has no group or other write bit, so `-w` lands on 0o444 under any umask. This test
+        // is about which operands get picked, not about umask arithmetic.
+        for name in ["f", "--", "-w"] {
+            make_file(&at.plus_as_string(name), 0o644);
+        }
+
+        let result = scene.ucmd().arg("-v").args(args).run();
+        let context = format!("chmod -v {}", args.join(" "));
+
+        // `-v` names every operand it visits, whether or not the mode ends up changing anything.
+        let visited: Vec<&str> = result
+            .stdout_str()
+            .lines()
+            .filter_map(|line| line.strip_prefix("mode of '"))
+            .filter_map(|rest| rest.split('\'').next())
+            .collect();
+        assert_eq!(visited, *expected, "{context}: acted on the wrong operands");
+
+        // A row that names no operand is an error (a missing or invalid mode), never a silent
+        // no-op.
+        assert_eq!(
+            result.succeeded(),
+            !expected.is_empty(),
+            "{context}: unexpected exit status"
+        );
+
+        // Independently of `-v`, nothing outside the expected list may be touched. This is what
+        // catches an implementation that mistakes the file named `--` for a separator.
+        for name in ["f", "--", "-w"] {
+            if !expected.contains(&name) {
+                assert_eq!(
+                    at.metadata(name).permissions().mode() & 0o7777,
+                    0o644,
+                    "{context}: {name} should have been left alone"
+                );
+            }
+        }
+    }
+}
+
+/// `chmod` warns that the umask kept bits the mode asked to remove only when the mode was written
+/// in the option-like form, i.e. as a leading-hyphen argument before any `--`. Once `--` has been
+/// seen the operand is unambiguously a mode, and the change is applied silently.
+#[test]
+#[cfg(not(target_os = "android"))]
+fn test_umask_conflict_reported_only_for_option_like_mode() {
+    // (arguments, resulting permission bits, whether the umask conflict is reported)
+    let cases: &[(&[&str], u32, bool)] = &[
+        (&["-w", "file"], 0o466, true),
+        // A `--` after the mode does not retroactively make it an ordinary operand.
+        (&["-w", "--", "file"], 0o466, true),
+        (&["file", "-w"], 0o466, true),
+        (&["-w", "-w", "--", "file"], 0o466, true),
+        (&["--", "-w", "file"], 0o466, false),
+        (&["--", "-rw", "file"], 0o022, false),
+        // What matters is whether the argument itself began with a hyphen, not whether the mode
+        // contains an umask-relative clause: these two modes do the same thing, and only the one
+        // that looks like an option is reported.
+        (&["u+x,-w", "file"], 0o566, false),
+        (&["-w,u+x", "file"], 0o566, true),
+        (&["--", "-w,u+x", "file"], 0o566, false),
+    ];
+
+    for (args, expected_mode, reported) in cases {
+        let scene = TestScenario::new(util_name!());
+        let at = &scene.fixtures;
+        // 0o666 is required: on 0o644 the umask has nothing left to keep, so the conflict never
+        // arises and every one of these cases would pass vacuously.
+        make_file(&at.plus_as_string("file"), 0o666);
+
+        let result = scene.ucmd().umask(0o022).args(args).run();
+        let context = format!("chmod {}", args.join(" "));
+
+        assert_eq!(
+            at.metadata("file").permissions().mode() & 0o7777,
+            *expected_mode,
+            "{context}: wrong resulting permissions"
+        );
+        if *reported {
+            result.code_is(1);
+            assert!(
+                result.stderr_str().contains("new permissions are"),
+                "{context}: expected the umask conflict to be reported, got {:?}",
+                result.stderr_str()
+            );
+        } else {
+            result.success().no_stderr();
+        }
+    }
+}
+
 #[test]
 fn test_chmod_dereference_symlink() {
     let scene = TestScenario::new(util_name!());