ptx: handle invalid regex arguments gracefully instead of panicking#9825
ptx: handle invalid regex arguments gracefully instead of panicking#9825ChrisDryden merged 4 commits intouutils:mainfrom
Conversation
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
CrazyRoka
force-pushed
the
ptx-fix-regex-panic
branch
2 times, most recently
from
30a281f to
07c63a1
Compare
CrazyRoka
force-pushed
the
ptx-fix-regex-panic
branch
from
07c63a1 to
237d483
Compare
|
GNU testsuite comparison: |
|
Whats going to be tricky here is that some of the testing to compare the two implementations is going to not work because whats considered invalid to rust's regex does not line up 100% with the GNU regex. Some examples: bar\ and [abc GNU allows trailing backslashes, but uutils would show a warning for bar\ whereas if theres an unmatched [ it fails completely for gnu ptx but would succeed with an error for the uutils implementation. I think for the time being it would be best to just have this as an open issue to track some of the edge cases and merge the PR's you have with the fixes that pass all of the test cases and then the deviation with GNU can be handled when uutils figures out what it wants to do about the whole regex library situation |
CrazyRoka
force-pushed
the
ptx-fix-regex-panic
branch
from
95d53e2 to
13fab7e
Compare
CrazyRoka
force-pushed
the
ptx-fix-regex-panic
branch
from
13fab7e to
89747e2
Compare
|
GNU testsuite comparison: |
src/uu/ptx/src/ptx.rs
Outdated
|
|
||
| /// Try to compile a regex, printing a warning and returning None on failure. | ||
| fn try_compile_regex(pattern: &str) -> Option<Regex> { | ||
| match Regex::new(pattern) { |
There was a problem hiding this comment.
please consider using .inspect_err().ok() pattern instead of explicit match for more concise error handling
There was a problem hiding this comment.
I'm provided that feedback previously but then when I was comparing it to the gnu implementation, it seemingly silently succeeds. When you combine the duplicate file pr with this, it's the only PTX gnu test remaining to silently succeeds when passed invalid regex. I'm not sure what would be put in the 'inspect_error'
There was a problem hiding this comment.
As @ChrisDryden suggested, I removed the logging entirely and just used .ok() to return None silently on failure.
Merging this PR will degrade performance by 3.26%
Performance Changes
Comparing 
Footnotes
|
|
GNU testsuite comparison: |
|
@sylvestre are there any settings that could make the macOS queue not be as long? Both the ptx prs are just waiting on those |
3 participants
Description
This PR fixes a panic in
ptxwhen an invalid regular expression is passed to the-W(--word-regexp) argument.Previously, running:
ptx -W 'bar\\\'would result in a Rust panic (
called Result::unwrap() on an Err value).This change catches the
regex::Errorand prints a user-friendly message to stderr.Testing
tests/by-util/test_ptx.rsto verify:bar\) return exit code 1.(wrong) return exit code 1.