diff: don't panic on an operand ending in --width=N - #279
Conversation
The `--width` regex was missing the `^` anchor that its `--tabsize` sibling
one line above already has, so it matched any argument whose lossy form ended
in `--width=<digits>` rather than the option itself. Two symptoms follow, and
the quieter one is worse:
diff $'\xff--width=5' A B # aborts, exit 134
diff xx--width=5 A B # taken as a width, the operand is discarded
The first reaches `into_string().unwrap()` with a non-UTF-8 argument and, under
`panic = "abort"`, takes the process down. The second does not crash at all: the
operand is swallowed as a width and diff compares the remaining two files,
exiting 1. GNU treats both as file operands, reports the extra operand, exits 2.
Anchoring restores the invariant the `--tabsize` block documents, that a match
implies valid UTF-8, so the existing `unwrap` is sound rather than merely
unlikely to fire. I mirrored that block rather than reworking the `unwrap`
separately, to keep the two option paths reading the same way.
`--width` had no test coverage, which is how this survived. Added a `width` test
alongside `tabsize` covering valid values and the invalid forms, plus both cases
above. The non-UTF-8 one is `cfg(unix)`, since it needs bytes an `OsString`
cannot hold on Windows.
Exit codes now match GNU diffutils 3.12 for `--width=5`, `--width=5x`,
`xx--width=5` and the non-UTF-8 form. The error text still differs, because uu
prints its usage line where GNU names the extra operand, but that gap predates
this change and affects any three-operand invocation.
Closes uutils#247
Merging this PR will degrade performance by 3.47%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing |
|
I don't think the CodSpeed report is attributable to this change, and part of it may be pointing at the benchmark suite rather than the PR. Two of the five regressions, All six benchmarks that moved are subprocess spawns, and none of the in-process ones moved. That includes The base run was also on a different runner image, 20260729.566 against 20260819.586 for this branch, and spawn cost is mostly exec and dynamic linking. Happy to look again if you read it differently. |
--widthis parsed with a regex that is missing the^anchor its--tabsizeneighbour has one line above it, so it matches any argument whose lossy form ends in--width=<digits>rather than the option itself.That produces two symptoms, and the quieter one is worse than the reported crash:
The first is the abort from the issue: a non-UTF-8 argument reaches
into_string().unwrap()and, underpanic = "abort", takes the process down. The second does not crash at all.xx--width=5is accepted as a width, the operand is quietly discarded, and diff compares the two files that are left. GNU treats both as file operands and exits 2 withextra operand 'B'.The fix
Anchoring the regex restores the invariant the
--tabsizeblock documents a little further down, that a match implies valid UTF-8. I added that same comment to the--widthblock and left the existingunwrapin place rather than reworking it separately, so the two option paths keep reading the same way. To be precise about what this does: the anchor makes thatunwrapunreachable, it does not remove it.Tests
--widthhad no test coverage, which is how this survived. There is now awidthtest next totabsize, covering valid values and the same invalid formstabsizealready checks, plus both cases above. The non-UTF-8 case iscfg(unix), since it needs bytes anOsStringcannot hold on Windows.Both new assertions fail without the anchor:
width_non_utf8_operandaborts atinto_string().unwrap(), andwidthfails onxx--width=5.Verification
Exit codes compared against GNU diffutils 3.12:
$'\xff--width=5'xx--width=5--width=5--width=5xcargo testgoes from 272 to 274 passing, andcargo fmt --all -- --check,cargo clippy -- -D warningsandcargo test --all-features --no-fail-fastare clean. The GNU upstream test suite gives identical per-test results before and after. I ran it on macOS, where much of it fails for unrelated environment reasons, so I compared per test against a cleanmainbuild rather than reading the totals.Out of scope
The error text still differs from GNU here: this prints
Usage: diff <from> <to>where GNU printsextra operand 'B'followed by aTry --helpline. That is not specific to--width, it happens for any three-operand invocation, so I left it alone.Closes #247