chmod: name the whole mode operand in an invalid-mode error - #14301
Open
arbelonson-source wants to merge 1 commit into
Open
chmod: name the whole mode operand in an invalid-mode error#14301arbelonson-source wants to merge 1 commit into
arbelonson-source wants to merge 1 commit into
Conversation
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.
|
GNU testsuite comparison: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GNU says the same thing for every way a
chmodmode can be malformed:uutils instead surfaced a message shaped like whatever failed internally:
GNU also always names the whole mode operand, not just the clause that broke:
chmod u+rwx,z+r freports'u+rwx,z+r', not'z+r'.What changed
chmodalready builds its plain error message now uses the whole mode string it already has on hand, with a newchmod-error-invalid-modemessage matching GNU's wording exactly.USimpleErrortoUUsageErrorso the "Try --help" hint — missing before — gets added the same way every other usage error in the codebase already gets it.chmodalready draws for a-m/positional mode on a real terminal (something GNU has no equivalent of).describe()previously left theInvalidOperatorcase 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,mkfifoandmknodshare this same mode parser (uucore::mode) and, from a quick check, have the identical class of bug in their own-mhandling: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
--helphint): all match.test_invalid_mode_names_the_whole_operand, verified to fail without the fix; updatedtest_chmod_permissions_too_largeand the five caret-diagnostic tests intest_chmod.rsto 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).install/mkdir/mkfifo/mknod's own suites (187 tests) since they shareuucore::mode: unaffected.cargo fmt --checkandcargo 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.