Skip to content

chmod: name the whole mode operand in an invalid-mode error - #14301

Open
arbelonson-source wants to merge 1 commit into
uutils:mainfrom
arbelonson-source:fix/chmod-invalid-mode-message
Open

chmod: name the whole mode operand in an invalid-mode error#14301
arbelonson-source wants to merge 1 commit into
uutils:mainfrom
arbelonson-source:fix/chmod-invalid-mode-message

Conversation

@arbelonson-source

Copy link
Copy Markdown
Contributor

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 internally:

$ chmod 999 f       # ours, before: a raw ParseIntError leaking through
chmod: invalid digit found in string
$ chmod a f          # ours, before: wrong punctuation, no --help hint
chmod: invalid mode (a)
$ chmod 17777 f      # ours, before: a value that parsed as octal but was out of range
chmod: mode is too large (17777 > 7777)

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'.

What changed

  • The one place chmod already builds its plain error message now uses the whole mode string it already has on hand, with a new chmod-error-invalid-mode message matching GNU's wording exactly.
  • Switched from USimpleError to UUsageError so the "Try --help" hint — missing before — gets added the same way every other usage error in the codebase already 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 (something GNU has no equivalent of). describe() previously left the InvalidOperator case unlabelled on the assumption the headline already said it; now that the headline is uniform, the label says it instead, reusing the exact text the headline used to show.

Not covered

install, mkdir, mkfifo and mknod share this same mode parser (uucore::mode) and, from a quick check, have the identical class of bug in their own -m handling:

$ mkdir -m 999 d      # GNU: mkdir: invalid mode '999'   (no colon — different from chmod!)
mkdir: invalid digit found in string

I didn't fold those in here: each utility's exact GNU wording needs its own verification (mkdir already differs from chmod by one punctuation mark) rather than assuming chmod's message applies as-is, and this keeps the PR to the one utility I fully checked.

Testing

  • 17-case differential sweep against GNU 9.11 (bad digits, too-large octal, missing/invalid operators, empty mode, multi-clause mode, all normalized for the binary path in the --help hint): all match.
  • New test_invalid_mode_names_the_whole_operand, verified to fail without the fix; updated test_chmod_permissions_too_large and the five caret-diagnostic tests in test_chmod.rs to the corrected wording (the caret tests already passed unchanged — the label fix keeps their content, just relocates it).
  • cargo test --features chmod --test tests -- test_chmod: 63 passed, 0 failed (62 pre-existing — 5 adjusted, not deleted — plus 1 new).
  • Also ran install/mkdir/mkfifo/mknod's own suites (187 tests) since they share uucore::mode: unaffected.
  • cargo fmt --check and cargo clippy -p uu_chmod --all-targets -- -D warnings: clean.

Disclosure

Prepared with AI assistance (Claude Opus 5, via Claude Code), per the AI policy in CONTRIBUTING.md. GNU's behaviour was established by running the installed GNU binary as a black box; I did not read GNU coreutils source. All testing was run locally.

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.
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/tail/symlink (fails in this run but passes in the 'main' branch)
Note: The gnu test tests/printf/printf-surprise is now being skipped but was previously passing.
Skip an intermittent issue tests/pr/bounded-memory (was skipped on 'main', now failing)

Comment thread src/uu/chmod/src/chmod.rs
// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the comment could be one line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants