mkdir, install, mkfifo, mknod: fix -m's invalid-mode message and an empty-clause bug - #14302
Open
arbelonson-source wants to merge 1 commit into
Open
Conversation
…mpty-clause bug
Follow-up to the chmod fix: `mkdir`/`install`/`mkfifo`/`mknod` share the
same mode parser chmod uses, and had the identical class of bug in their
own `-m`/`--mode` handling -- each leaking a message shaped like whatever
failed to parse internally, rather than GNU's own wording:
$ mkdir -m 999 d # ours, before: a raw ParseIntError
mkdir: invalid digit found in string
$ mkdir -m 999 d # GNU
mkdir: invalid mode '999'
GNU's wording is not identical across these, so each got its own fix
rather than reusing chmod's message: `mkdir`/`install` name the operand
but, unlike `chmod`, use no colon and add no "Try --help" hint; `mkfifo`/
`mknod` don't name the operand at all, just a bare "invalid mode".
A second, more serious bug turned up in the process, in the parser these
four share (`chmod`'s own inline loop does not use it, which is why this
one didn't affect chmod): an empty clause in a comma-separated mode --
the whole mode being empty, or a leading, trailing, or doubled comma --
was silently treated as a no-op instead of an error, so `mkdir -m ',' d`
and `mkdir -m 'u+rwx,' d` both succeeded where GNU rejects both. Verified
against GNU that no comma-separated form tolerates an empty clause.
`describe()`'s caret label for `InvalidOperator` picks up the same fix
`mode.rs` needed for chmod's PR: now that the headline never explains the
operator, the label does, reusing the message that used to be the
headline. (Independent of, but touching the same function as, the chmod
fix in uutils#14301 -- expect a trivial rebase against whichever lands second.)
|
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.
Follow-up to #14301 (chmod).
mkdir/install/mkfifo/mknodshare the same mode parser chmod uses, and — as flagged in that PR's body — have the identical class of bug in their own-m/--modehandling: each leaked a message shaped like whatever failed to parse internally, rather than GNU's own wording.Three different messages, verified individually — not assumed
GNU's wording is not identical across these four, so I checked each rather than assuming chmod's fix applies as-is:
mkdir/installname the operand, but unlikechmoduse no colon before the quote and add no "Try --help" hint:mkdir: invalid mode '999',install: invalid mode '999'.mkfifo/mknoddon't name the operand at all, regardless of what was wrong with it: justmkfifo: invalid mode/mknod: invalid mode.A second, more serious bug
Investigating turned up something beyond message wording, in the parser these four share but
chmod's own inline loop does not use (which is why it didn't affect chmod): an empty clause in a comma-separated mode was silently skipped as a no-op instead of erroring.Checked against GNU with a leading, trailing, and doubled comma — none are tolerated, so
parse_chmod'sif mode_part.is_empty() { continue; }was simply wrong; removing it letsparse_symbolic's existing empty-string handling report it the way it already does forchmod.Also touches
describe()mode.rs'sdescribe()needed the same fix chmod's PR made: now that the headline never explains which operator was invalid or expected, that detail lives in the caret-diagram label instead. This function is shared, so both PRs touch the same three lines with the same final text — whichever of the two merges second will need a trivial, content-identical rebase.Testing
cargo test --features mkdir,install,mkfifo,mknod,chmod --test tests -- test_mkdir test_install test_mkfifo test_mknod test_chmod: 257 passed, 0 failed (all pre-existing tests unaffected; chmod's own suite included since it sharesmode.rs).moz-fluent-linton all touched locale directories: no errors.cargo fmt --checkandcargo clippy -p uu_mkdir -p uu_install -p uu_mkfifo -p uu_mknod -p uucore --features mode --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.