This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7922

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

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

Details

Reviewers
pulkit
martinvonz
durin42
Group Reviewers
hg-reviewers
Commits
rHG52d40f8fb82d: rust-matchers: add function to generate a regex matcher function
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
In D7922#121720, @durin42 wrote:

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.EditedMar 9 2020, 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 added a comment.Mar 10 2020, 11:20 AM
In D7922#123057, @pulkit wrote:

@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.

pulkit added a comment.Mar 10 2020, 12:36 PM
In D7922#123193, @martinvonz wrote:
In D7922#123057, @pulkit wrote:

@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 added a comment.Mar 10 2020, 12:37 PM
In D7922#123207, @pulkit wrote:
In D7922#123193, @martinvonz wrote:
In D7922#123057, @pulkit wrote:

@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.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/lib.rs (7 lines)
Mrust/hg-core/src/matchers.rs (27 lines)

Diff 20712

rust/hg-core/src/lib.rs

Path(HgPathError), Path(HgPathError),
UnsupportedSyntax(String), UnsupportedSyntax(String),
UnsupportedSyntaxInFile(String, String, usize), UnsupportedSyntaxInFile(String, String, usize),
TooLong(usize), TooLong(usize),
IO(std::io::Error), IO(std::io::Error),
/// Needed a pattern that can be turned into a regex but got one that /// Needed a pattern that can be turned into a regex but got one that
/// can't. This should only happen through programmer error. /// can't. This should only happen through programmer error.
NonRegexPattern(IgnorePattern), NonRegexPattern(IgnorePattern),
/// This is temporary, see `re2/mod.rs`.
/// This will cause a fallback to Python.
Re2NotInstalled,
} }
impl ToString for PatternError { impl ToString for PatternError {
fn to_string(&self) -> String { fn to_string(&self) -> String {
match self { match self {
PatternError::UnsupportedSyntax(syntax) => { PatternError::UnsupportedSyntax(syntax) => {
format!("Unsupported syntax {}", syntax) format!("Unsupported syntax {}", syntax)
} }
PatternError::UnsupportedSyntaxInFile(syntax, file_path, line) => { PatternError::UnsupportedSyntaxInFile(syntax, file_path, line) => {
format!( format!(
"{}:{}: unsupported syntax {}", "{}:{}: unsupported syntax {}",
file_path, line, syntax file_path, line, syntax
) )
} }
PatternError::TooLong(size) => { PatternError::TooLong(size) => {
format!("matcher pattern is too long ({} bytes)", size) format!("matcher pattern is too long ({} bytes)", size)
} }
PatternError::IO(e) => e.to_string(), PatternError::IO(e) => e.to_string(),
PatternError::Path(e) => e.to_string(), PatternError::Path(e) => e.to_string(),
PatternError::NonRegexPattern(pattern) => { PatternError::NonRegexPattern(pattern) => {
format!("'{:?}' cannot be turned into a regex", pattern) format!("'{:?}' cannot be turned into a regex", pattern)
} }
PatternError::Re2NotInstalled => {
"Re2 is not installed, cannot use regex functionality."
.to_string()
}
} }
} }
} }
impl From<DirstateMapError> for DirstateError { impl From<DirstateMapError> for DirstateError {
fn from(e: DirstateMapError) -> Self { fn from(e: DirstateMapError) -> Self {
DirstateError::Map(e) DirstateError::Map(e)
} }

rust/hg-core/src/matchers.rs

// matchers.rs // matchers.rs
// //
// Copyright 2019 Raphaël Gomès <rgomes@octobus.net> // Copyright 2019 Raphaël Gomès <rgomes@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
//! Structs and types for matching files and directories. //! Structs and types for matching files and directories.
use crate::{utils::hg_path::HgPath, DirsMultiset, DirstateMapError}; #[cfg(feature = "with-re2")]
use crate::re2::Re2;
use crate::{
filepatterns::PatternResult, utils::hg_path::HgPath, DirsMultiset,
DirstateMapError, PatternError,
};
use std::collections::HashSet; use std::collections::HashSet;
use std::iter::FromIterator; use std::iter::FromIterator;
use std::ops::Deref; use std::ops::Deref;
#[derive(Debug, PartialEq)] #[derive(Debug, PartialEq)]
pub enum VisitChildrenSet<'a> { pub enum VisitChildrenSet<'a> {
/// Don't visit anything /// Don't visit anything
Empty, Empty,
} }
fn matches_everything(&self) -> bool { fn matches_everything(&self) -> bool {
false false
} }
fn is_exact(&self) -> bool { fn is_exact(&self) -> bool {
true true
} }
} }
#[cfg(feature = "with-re2")]
martinvonzUnsubmitted
Not Done

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.

martinvonz: Hmm, I don't like to replicate this into Rust. I argued for a long time with Boris over a year…
AlphareAuthorUnsubmitted
Done

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: I am having a little trouble reading the patchwork thread, but I gather that you want to get…
AlphareAuthorUnsubmitted
Done

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: I've looked into the behavior of Re2. It will return an error if the DFA runs out of memory…
martinvonzUnsubmitted
Not Done

Thanks for fixing the Rust side.

martinvonz: Thanks for fixing the Rust side.
/// Returns a function that matches an `HgPath` against the given regex
/// pattern.
///
/// This can fail when the pattern is invalid or not supported by the
/// underlying engine `Re2`, for instance anything with back-references.
fn re_matcher(
pattern: &[u8],
) -> PatternResult<impl Fn(&HgPath) -> bool + Sync> {
let regex = Re2::new(pattern);
let regex = regex.map_err(|e| PatternError::UnsupportedSyntax(e))?;
Ok(move |path: &HgPath| regex.is_match(path.as_bytes()))
}
#[cfg(not(feature = "with-re2"))]
fn re_matcher(_: &[u8]) -> PatternResult<Box<dyn Fn(&HgPath) -> bool + Sync>> {
Err(PatternError::Re2NotInstalled)
}
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::*; use super::*;
use pretty_assertions::assert_eq; use pretty_assertions::assert_eq;
#[test] #[test]
fn test_filematcher_visit_children_set() { fn test_filematcher_visit_children_set() {
// Visitchildrenset // Visitchildrenset