Skip to content

rm: match GNU's error for a bad --interactive value - #14309

Open
arbelonson-source wants to merge 1 commit into
uutils:mainfrom
arbelonson-source:fix/rm-interactive-invalid-choice
Open

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

Conversation

@arbelonson-source

Copy link
Copy Markdown
Contributor

What

--interactive validates its value with a plain ShortcutValueParser,
so an unrecognized or ambiguous value produces clap's own generic
wording instead of GNU's grouped-alias wording:

$ rm --interactive=bogus f

# GNU
rm: invalid argument 'bogus' for '--interactive'
Valid arguments are:
  - 'never', 'no', 'none'
  - 'once'
  - 'always', 'yes'
Try 'rm --help' for more information.

# uutils, before this PR
error: invalid value 'bogus' for '--interactive[=<WHEN>]'

  [possible values: always, once, never]

For more information, try '--help'.

Fix

Same fix already shipped for wc --total, date --iso-8601/--rfc-3339,
du --time, sort --parallel, uniq --group/--all-repeated, shred --remove, and tee --output-error (#14293, #14303-#14308): resolve
the value against the option's own alias groups by hand, accepting any
unambiguous abbreviation the way GNU does -- including one ambiguous
only between aliases of the same choice, e.g. n among
never/no/none, or a/y for always/yes -- and report GNU's
wording when it doesn't resolve, in GNU's own group order
(never/no/none, once, always/yes -- notably not the
order this option's choices were declared in).

Testing

  • cargo test -p uu_rm / full tests/by-util/test_rm.rs suite: 85 passed, 0 failed.
  • Added 2 regression tests: an invalid value's exact message, and that an ambiguous-but-same-meaning abbreviation (n) is still accepted.
  • Manually diffed every --interactive value (all real choices/aliases/abbreviations, invalid, ambiguous, empty) 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.

--interactive validated its value with a plain ShortcutValueParser, so
an unrecognized or ambiguous value produced clap's own generic wording
instead of GNU's grouped-alias wording ('invalid argument ... for
--interactive / Valid arguments are: / - 'never', 'no', 'none' / ...').

Resolve the value against the option's own alias groups by hand,
accepting any unambiguous abbreviation the way GNU does -- including
one ambiguous only between aliases of the *same* choice, e.g. 'n'
among 'never'/'no'/'none', or 'a'/'y' for 'always'/'yes' -- and report
GNU's wording, in GNU's own group order (never/no/none, once,
always/yes), when it does not resolve.

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

oech3 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Sorry, but I really don't hope increacing complexity just for matching error message with GNU by removing clap's control.

@codspeed-hq

codspeed-hq Bot commented Aug 31, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 3.26%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
✅ 7 untouched benchmarks
⏩ 404 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation rm_single_file 104 ms 100.7 ms +3.26%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing arbelonson-source:fix/rm-interactive-invalid-choice (d67cbb2) with main (3251833)

Open in CodSpeed

Footnotes

  1. 404 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@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)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Skip an intermittent issue tests/pr/bounded-memory (was skipped on 'main', now failing)

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