Skip to content

rm: match GNU's error for a bad --preserve-root value - #14310

Closed
arbelonson-source wants to merge 1 commit into
uutils:mainfrom
arbelonson-source:fix/rm-preserve-root-invalid-value
Closed

rm: match GNU's error for a bad --preserve-root value#14310
arbelonson-source wants to merge 1 commit into
uutils:mainfrom
arbelonson-source:fix/rm-preserve-root-invalid-value

Conversation

@arbelonson-source

Copy link
Copy Markdown
Contributor

What

--preserve-root validates its value with clap's plain
value_parser(["all"]), so an unrecognized value produces clap's own
generic wording instead of GNU's:

$ rm --preserve-root=bogus -rf anything

# GNU
rm: unrecognized --preserve-root argument: 'bogus'

# uutils, before this PR
error: invalid value 'bogus' for '--preserve-root[=<all>]'
  [possible values: all]

For more information, try '--help'.

Found this one while working on --interactive in the same file
(#14309) -- it's the same underlying problem (clap's own wording
instead of GNU's) but not the same class of bug as the rest of this
series (#14293, #14303-#14309): those all had a ShortcutValueParser
producing a "invalid/ambiguous argument ... Valid arguments are: ..."
shape with abbreviation support. --preserve-root uses clap's built-in
possible-values parser instead, GNU's real message is a single,
simpler line with no "Try --help" hint, and no abbreviation is
accepted at all -- GNU rejects even a for all.

Fix

Resolve the value by hand (an exact match against "all", nothing
else) and report GNU's own wording exactly.

Also fixes the existing test_preserve_root_rejects_unknown_value
test, which was asserting clap's wrong wording ("invalid value 'bogus'") as though it were correct -- it had been written to match
the bug rather than GNU.

Testing

  • cargo test -p uu_rm / full tests/by-util/test_rm.rs suite: 84 passed, 0 failed.
  • Fixed the existing test to assert GNU's actual message, and added a regression test for the no-abbreviations-accepted case (--preserve-root=a).
  • Manually diffed --preserve-root with all, a, bogus, empty, All, ALL, and no value at all against GNU rm 9.11 under LC_ALL=C.
  • cargo clippy -p uu_rm --all-targets -- -D warnings and cargo fmt --check: clean.

This PR was written with AI assistance (Claude Opus 5, via Claude Code). I've tested the changes but please review the code carefully.

--preserve-root validated its value with clap's plain
value_parser(["all"]), so an unrecognized value produced clap's own
generic wording instead of GNU's single-line 'unrecognized
--preserve-root argument: ...' -- notably not the same
'invalid/ambiguous argument ... Valid arguments are: ...' shape used
by the other options in this series (uutils#14293, uutils#14303-uutils#14309), and with
no abbreviation accepted at all (GNU rejects even 'a').

Resolve the value by hand and report GNU's own, simpler wording. Also
fixes the existing test_preserve_root_rejects_unknown_value test,
which was asserting clap's wrong wording ('invalid value ...') as
though it were correct.

AI-assisted-by: Claude Opus 5, via Claude Code
@oech3

oech3 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

In this case, our message has more info.

@oech3

oech3 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Also note that aligning error message with GNU means we introduce a source extracted from GPL program.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

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

@sylvestre

Copy link
Copy Markdown
Contributor

our error message is better, let's keep it

@sylvestre sylvestre closed this Aug 31, 2026
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.

3 participants