components: Improve regexes in DiffViewer component - #52582
Conversation
A sequence like `\s+(.*)` can be inefficient if it winds up not matching, as it'll backtrack every possibility of whitespace matching the `\s` versus the `.`. We can fix it easily enough by doing like `\s+(\S.*|)`, preventing the group from having leading whitespace. The other regex here is pretty much impossible without atomic groups or possessive quantifiers. But since the intent is to get everything after a prefix, we can match just the prefix (which can be done efficiently) and then use `.substring()` to extract everything after.
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
|
|
||
| // Diff index | ||
| const header = /^(?:Index:|diff(?: -r \w+)+)\s+(.+?)\s*$/.exec( line ); | ||
| const header = /^(?:Index:|diff(?: -r \w+)+)\s+/.exec( line ); |
There was a problem hiding this comment.
I was playing with this a bit, and it seems an elegant solution.
I'll note this seems to be expecting a very specific diff format; it doesn't work with a standard git diff and the component actually crashes. Not sure it's worth addressing here.
Also, it seems the ensuing index.index is never actually used, so we could probably get away with removing this logic altogether, but again, that's perhaps out of scope.
There was a problem hiding this comment.
As long as it crashes the same way with trunk, I'm not concerned about it for this PR. 😀
Following a comment above to where they got this from suggests that the diff -r something -r something format is for Mercurial. 🤷
Also, it seems the ensuing
index.indexis never actually used
Yeah, I noticed that too. I agree with "out of scope".
There was a problem hiding this comment.
When I tried pasting in a git diff, it didn't crash. Worked fine. 🤷
| */ | ||
| function parseFileHeader( index: Index ) { | ||
| const fileHeader = /^(---|\+\+\+)\s+(.*)\r?$/.exec( diffstr[ i ] ); | ||
| const fileHeader = /^(---|\+\+\+)\s+(\S.*?|)\r?$/.exec( diffstr[ i ] ); |
There was a problem hiding this comment.
Shouldn't hurt anything, but why do we need the ? here?
There was a problem hiding this comment.
Hmm. We don't really, I'll take that out.
In PCRE the .* would match a \r making the \r? pointless, but JS . excludes \r, \u2028, and \u2029 as well as \n.
JS regex, unlike PCRE, excludes `\r` from `.` as well as `\n`.
github-actions
Bot
removed
the
[Status] Needs Review
2 participants
Closes MONOREP-949
Proposed changes
A sequence like
\s+(.*)can be inefficient if it winds up not matching, as it'll backtrack every possibility of whitespace matching the\sversus the.. We can fix it easily enough by doing like\s+(\S.*|), preventing the group from having leading whitespace.The other regex here is pretty much impossible without atomic groups or possessive quantifiers. But since the intent is to get everything after a prefix, we can match just the prefix (which can be done efficiently) and then use
.substring()to extract everything after.Related product discussion/links
p1790010272417069-slack-C05Q5HSS013
Does this pull request change what data or activity we track or use?
No
Testing instructions
projects/js-packages/storybook, runpnpm run storybook:dev.