Page MenuHomePhabricator

rust-matchers: add function to generate a regex matcher function
ClosedPublic

Authored by Alphare on Jan 17 2020, 5:41 AM.

Details

Summary

This function will be used to help build the upcoming IncludeMatcher. While
Re2 is still used and behind a feature flag, this function returns an error
meant for fallback in the default case.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Jan 17 2020, 5:41 AM
pulkit accepted this revision.Feb 7 2020, 4:12 PM
This revision is now accepted and ready to land.Feb 7 2020, 4:12 PM
pulkit added a comment.Feb 7 2020, 4:47 PM

This one failed to apply on latest default, needs rebase.

martinvonz requested changes to this revision.Feb 10 2020, 7:48 PM
martinvonz added a subscriber: martinvonz.
martinvonz added inline comments.
rust/hg-core/src/matchers.rs
224

Hmm, I don't like to replicate this into Rust. I argued for a long time with Boris over a year ago that we should see if we can remove it from Python. He said they (Octobus, I think) would look into that if I would just queue the workaround for the time being. Could you see if you can simplify the Python code first instead?

See https://patchwork.mercurial-scm.org/patch/36755/ for discussion.

This revision now requires changes to proceed.Feb 10 2020, 7:48 PM
Alphare added inline comments.Feb 11 2020, 5:09 AM
rust/hg-core/src/matchers.rs
224

I am having a little trouble reading the patchwork thread, but I gather that you want to get rid of the preventive splitting of patterns and just let the regex engine handle it on its own? Was this measure taken because of a bug in Python's re or because its exceptions were too coarse/unusable?
I'll look into the behavior of Re2, in that case.

Alphare updated this revision to Diff 20154.Feb 11 2020, 5:54 AM
Alphare added inline comments.Feb 11 2020, 5:58 AM
rust/hg-core/src/matchers.rs
224

I've looked into the behavior of Re2. It will return an error if the DFA runs out of memory, which seems perfectly reasonable.
I will simplify the Rust code, however I feel like you're better suited than I am to fix the Python side of things, since I don't really understand the ins-and-outs of the problem.

Alphare updated this revision to Diff 20160.Feb 11 2020, 6:13 AM
durin42 accepted this revision as: durin42.Feb 28 2020, 11:56 AM

I think this looks good, but I want Martin to ack that his concern is adequately resolved.

martinvonz accepted this revision.Feb 28 2020, 12:38 PM

I think this looks good, but I want Martin to ack that his concern is adequately resolved.

Yes, looks good to me now. Thanks.

rust/hg-core/src/matchers.rs
224

Thanks for fixing the Rust side.

This revision is now accepted and ready to land.Feb 28 2020, 12:38 PM
pulkit added a comment.EditedMon, Mar 9, 5:06 AM

@martinvonz this (and next few patches) seems accepted by you and Augie. Should I go ahead and push it or I missed some discussion on IRC about it?

@martinvonz this (and next few patches) seems accepted by you and Augie. Should I go ahead and push it or I missed some discussion on IRC about it?

You can push it. I wanted to finish the cleanup to the regex grouping that I mentioned earlier, but that can be done separately. But that's the only reason I haven't queued this patch anyway.

@martinvonz this (and next few patches) seems accepted by you and Augie. Should I go ahead and push it or I missed some discussion on IRC about it?

You can push it. I wanted to finish the cleanup to the regex grouping that I mentioned earlier, but that can be done separately. But that's the only reason I haven't queued this patch anyway.

You mean cleanup on the Python side or on the rust side? IIUC, the rust side is fine now.

@martinvonz this (and next few patches) seems accepted by you and Augie. Should I go ahead and push it or I missed some discussion on IRC about it?

You can push it. I wanted to finish the cleanup to the regex grouping that I mentioned earlier, but that can be done separately. But that's the only reason I haven't queued this patch anyway.

You mean cleanup on the Python side or on the rust side? IIUC, the rust side is fine now.

On the Python side.