Repository navigation
pr: report invalid numeric options and page ranges as GNU does - #15176
Merged
Merged
Conversation
Collaborator
|
Is there an existing issue describing the problem being addressed? |
Contributor
Author
|
GNU testsuite comparison: |
sylvestre
reviewed
Oct 8, 2026
sylvestre
reviewed
Oct 8, 2026
sylvestre
reviewed
Oct 8, 2026
sylvestre
reviewed
Oct 8, 2026
| /// Like [`parse_number`], for an option that wants a number from 1 up: | ||
| /// zero and negative numbers are numbers out of range. | ||
| fn parse_positive(raw: &str, option_message: &str) -> Result<usize, PrError> { | ||
| let negative = raw |
Contributor
There was a problem hiding this comment.
the same "is a negative number" check is in the -o parsing too, could be a small helper, no?
Contributor
Author
There was a problem hiding this comment.
Done @sylvestre , is_negative_number() for both.
sylvestre
reviewed
Oct 8, 2026
Contributor
Author
|
@sylvestre the xstrtol failure came from util/build-gnu.sh rewriting xstrtol.pl to the old wording; those three seds are gone since pr prints what the test expects now. The Windows test no longer depends on the OS error text. |
NdekoCode
force-pushed
the
pr-numeric-option-diagnostics
branch
from
October 20, 2026 19:31
efbfa5d to
32ba51e
Compare
31 of 76 tasks
The value of -l, -w, -W, -N and -o was dropped by the legacy operand scan when it looked like one: `pr -l -1 f` read `-1` as a column count and `f` as the page length, and `pr -l +5 f` took `+5` for a page range. A page range that was not one (`--pages=0`, `2:1`) was reported as a bad argument, a `+` operand that was not a range was taken for a file, and a zero or negative length, width or column count got a message of its own. Keep the value of an option out of the operand scan, and let clap take a value that starts with `-`. Report the ranges and numbers the way GNU 9.12 does: `invalid page range '0'`, `--pages argument '...' too large`, `invalid + argument 'x'`, `invalid suffix in + argument '1:3x'`, and for the numbers `'-l PAGE_LENGTH' invalid number of lines: '0': Numerical result out of range`. A `+` operand that GNU takes for a file name, digits followed by other characters or a range that is not one, stays a file name.
The list of options whose value is the next argument duplicated what clap knows and missed a cluster (`-tl -1`) and an abbreviation (`--len -1`). Ask the built Command instead: a long option, exact or the one option a prefix fits, or the last short option of a cluster, that takes at least one value and does not require `=`. The scan runs once, on the arguments after `-e` got its attached value, so clap and the operands see the same thing. `-h` and `-D` take a value that starts with `-`, as GNU does. One parse of `FIRST[:LAST]` serves both the `+` operand check and the messages. A number is the digits up to whatever follows them, so `--pages=1x` is an invalid page range and `+1x:3` a file name, and a number too large with characters after it is an invalid suffix, as GNU reports them. The numeric diagnostics go through translate!(). Drop the xstrtol.pl wording patch from build-gnu.sh, since pr now prints what the test expects, and make the file name test independent of the OS error text.
sylvestre
force-pushed
the
pr-numeric-option-diagnostics
branch
from
November 3, 2026 22:36
32ba51e to
16e7ca0
Compare
sylvestre
reviewed
Nov 4, 2026
| if arg == "--" { | ||
| clap_args.push(arg); | ||
| break; | ||
| } |
Contributor
There was a problem hiding this comment.
when arg is itself a value this still checks it as an option, so pr -h -l +2 f gives +2 to -l instead of the page operand
!is_value && takes_next_argument(...)? please add a test
doing it in a new PR is fine
sylvestre
reviewed
Nov 4, 2026
| }) | ||
| matches | ||
| .get_one::<String>(opt) | ||
| .map(|raw| parse_number(raw, option_message)) |
Contributor
There was a problem hiding this comment.
almost the same as parse_usize, do we need both wrappers?
doing it in a new PR is fine
sylvestre
reviewed
Nov 4, 2026
| let option_message = translate!("pr-error-invalid-line-offset"); | ||
| match parse_number(raw, &option_message) { | ||
| // A negative margin is reported like one that does not fit; | ||
| // a negative zero is zero. |
Contributor
There was a problem hiding this comment.
parsing as i64 only to detect -0 is a bit odd, checking that all the digits are 0 would be simpler, no?
sylvestre
enabled auto-merge (rebase)
November 4, 2026 07:03
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.
The value of
-l,-w,-W,-Nand-owas dropped by the legacy operand scan when it looked like one:pr -l -1 fread-1as a column count andfas the page length, andpr -l +5 ftook+5for a page range. A page range that is not one (--pages=0,--pages=2:1) was reported as a bad argument, a+operand that is not a range was taken for a file, and a zero or negative length, width or column count had a wording of its own.The value of an option now stays out of the operand scan, and clap takes a value that starts with
-. The messages follow GNU 9.12:invalid page range '0',--pages argument '…' too large,invalid + argument 'x',invalid suffix in + argument '1:3x', and for the numbers'-l PAGE_LENGTH' invalid number of lines: '0': Numerical result out of range(the errno text, asValue too large for defined data typealready is). A+operand that GNU takes for a file name, digits followed by other characters or numbers that make no page range, stays a file name.Every case in the tests was run against GNU 9.12; the pr suite passes. This covers
pr/options.shfrom #14705.Closes #15179.