[Commented On] D10834: rust: Parse "subinclude"d files along the way, not later

baymax (Baymax, Your Personal Patch-care Companion) phabricator at mercurial-scm.org
Fri Jun 4 11:23:41 UTC 2021


baymax added a comment.
baymax updated this revision to Diff 28436.


  ✅ refresh by Heptapod after a successful CI run (🐙 💚)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D10834?vs=28433&id=28436

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D10834/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D10834

AFFECTED FILES
  rust/hg-core/src/filepatterns.rs
  rust/hg-core/src/matchers.rs
  rust/hg-cpython/src/dirstate/status.rs

CHANGE DETAILS

diff --git a/rust/hg-cpython/src/dirstate/status.rs b/rust/hg-cpython/src/dirstate/status.rs
--- a/rust/hg-cpython/src/dirstate/status.rs
+++ b/rust/hg-cpython/src/dirstate/status.rs
@@ -211,12 +211,9 @@
                 .collect();
 
             let ignore_patterns = ignore_patterns?;
-            let mut all_warnings = vec![];
 
-            let (matcher, warnings) =
-                IncludeMatcher::new(ignore_patterns, &root_dir)
-                    .map_err(|e| handle_fallback(py, e.into()))?;
-            all_warnings.extend(warnings);
+            let matcher = IncludeMatcher::new(ignore_patterns)
+                .map_err(|e| handle_fallback(py, e.into()))?;
 
             let (status_res, warnings) = dmap
                 .status(
@@ -234,9 +231,7 @@
                 )
                 .map_err(|e| handle_fallback(py, e))?;
 
-            all_warnings.extend(warnings);
-
-            build_response(py, status_res, all_warnings)
+            build_response(py, status_res, warnings)
         }
         e => Err(PyErr::new::<ValueError, _>(
             py,
diff --git a/rust/hg-core/src/matchers.rs b/rust/hg-core/src/matchers.rs
--- a/rust/hg-core/src/matchers.rs
+++ b/rust/hg-core/src/matchers.rs
@@ -11,7 +11,7 @@
     dirstate::dirs_multiset::DirsChildrenMultiset,
     filepatterns::{
         build_single_regex, filter_subincludes, get_patterns_from_file,
-        PatternFileWarning, PatternResult, SubInclude,
+        PatternFileWarning, PatternResult,
     },
     utils::{
         files::find_dirs,
@@ -237,7 +237,7 @@
 /// ///
 /// let ignore_patterns =
 /// vec![IgnorePattern::new(PatternSyntax::RootGlob, b"this*", Path::new(""))];
-/// let (matcher, _) = IncludeMatcher::new(ignore_patterns, "".as_ref()).unwrap();
+/// let matcher = IncludeMatcher::new(ignore_patterns).unwrap();
 /// ///
 /// assert_eq!(matcher.matches(HgPath::new(b"testing")), false);
 /// assert_eq!(matcher.matches(HgPath::new(b"this should work")), true);
@@ -341,8 +341,8 @@
 
 /// Returns the regex pattern and a function that matches an `HgPath` against
 /// said regex formed by the given ignore patterns.
-fn build_regex_match<'a>(
-    ignore_patterns: &'a [&'a IgnorePattern],
+fn build_regex_match(
+    ignore_patterns: &[IgnorePattern],
 ) -> PatternResult<(Vec<u8>, Box<dyn Fn(&HgPath) -> bool + Sync>)> {
     let mut regexps = vec![];
     let mut exact_set = HashSet::new();
@@ -478,32 +478,25 @@
 /// Returns a function that checks whether a given file (in the general sense)
 /// should be matched.
 fn build_match<'a, 'b>(
-    ignore_patterns: &'a [IgnorePattern],
-    root_dir: &Path,
-) -> PatternResult<(
-    Vec<u8>,
-    Box<dyn Fn(&HgPath) -> bool + 'b + Sync>,
-    Vec<PatternFileWarning>,
-)> {
+    ignore_patterns: Vec<IgnorePattern>,
+) -> PatternResult<(Vec<u8>, Box<dyn Fn(&HgPath) -> bool + 'b + Sync>)> {
     let mut match_funcs: Vec<Box<dyn Fn(&HgPath) -> bool + Sync>> = vec![];
     // For debugging and printing
     let mut patterns = vec![];
-    let mut all_warnings = vec![];
 
-    let (subincludes, ignore_patterns) =
-        filter_subincludes(ignore_patterns, root_dir)?;
+    let (subincludes, ignore_patterns) = filter_subincludes(ignore_patterns)?;
 
     if !subincludes.is_empty() {
         // Build prefix-based matcher functions for subincludes
         let mut submatchers = FastHashMap::default();
         let mut prefixes = vec![];
 
-        for SubInclude { prefix, root, path } in subincludes.into_iter() {
-            let (match_fn, warnings) =
-                get_ignore_function(vec![path.to_path_buf()], &root)?;
-            all_warnings.extend(warnings);
-            prefixes.push(prefix.to_owned());
-            submatchers.insert(prefix.to_owned(), match_fn);
+        for sub_include in subincludes {
+            let matcher = IncludeMatcher::new(sub_include.included_patterns)?;
+            let match_fn =
+                Box::new(move |path: &HgPath| matcher.matches(path));
+            prefixes.push(sub_include.prefix.clone());
+            submatchers.insert(sub_include.prefix.clone(), match_fn);
         }
 
         let match_subinclude = move |filename: &HgPath| {
@@ -556,14 +549,13 @@
     }
 
     Ok(if match_funcs.len() == 1 {
-        (patterns, match_funcs.remove(0), all_warnings)
+        (patterns, match_funcs.remove(0))
     } else {
         (
             patterns,
             Box::new(move |f: &HgPath| -> bool {
                 match_funcs.iter().any(|match_func| match_func(f))
             }),
-            all_warnings,
         )
     })
 }
@@ -588,8 +580,7 @@
         all_patterns.extend(patterns.to_owned());
         all_warnings.extend(warnings);
     }
-    let (matcher, warnings) = IncludeMatcher::new(all_patterns, root_dir)?;
-    all_warnings.extend(warnings);
+    let matcher = IncludeMatcher::new(all_patterns)?;
     Ok((
         Box::new(move |path: &HgPath| matcher.matches(path)),
         all_warnings,
@@ -597,34 +588,26 @@
 }
 
 impl<'a> IncludeMatcher<'a> {
-    pub fn new(
-        ignore_patterns: Vec<IgnorePattern>,
-        root_dir: &Path,
-    ) -> PatternResult<(Self, Vec<PatternFileWarning>)> {
-        let (patterns, match_fn, warnings) =
-            build_match(&ignore_patterns, root_dir)?;
+    pub fn new(ignore_patterns: Vec<IgnorePattern>) -> PatternResult<Self> {
         let RootsDirsAndParents {
             roots,
             dirs,
             parents,
         } = roots_dirs_and_parents(&ignore_patterns)?;
-
         let prefix = ignore_patterns.iter().any(|k| match k.syntax {
             PatternSyntax::Path | PatternSyntax::RelPath => true,
             _ => false,
         });
+        let (patterns, match_fn) = build_match(ignore_patterns)?;
 
-        Ok((
-            Self {
-                patterns,
-                match_fn,
-                prefix,
-                roots,
-                dirs,
-                parents,
-            },
-            warnings,
-        ))
+        Ok(Self {
+            patterns,
+            match_fn,
+            prefix,
+            roots,
+            dirs,
+            parents,
+        })
     }
 
     fn get_all_parents_children(&self) -> DirsChildrenMultiset {
@@ -810,14 +793,11 @@
     #[test]
     fn test_includematcher() {
         // VisitchildrensetPrefix
-        let (matcher, _) = IncludeMatcher::new(
-            vec![IgnorePattern::new(
-                PatternSyntax::RelPath,
-                b"dir/subdir",
-                Path::new(""),
-            )],
-            "".as_ref(),
-        )
+        let matcher = IncludeMatcher::new(vec![IgnorePattern::new(
+            PatternSyntax::RelPath,
+            b"dir/subdir",
+            Path::new(""),
+        )])
         .unwrap();
 
         let mut set = HashSet::new();
@@ -848,14 +828,11 @@
         );
 
         // VisitchildrensetRootfilesin
-        let (matcher, _) = IncludeMatcher::new(
-            vec![IgnorePattern::new(
-                PatternSyntax::RootFiles,
-                b"dir/subdir",
-                Path::new(""),
-            )],
-            "".as_ref(),
-        )
+        let matcher = IncludeMatcher::new(vec![IgnorePattern::new(
+            PatternSyntax::RootFiles,
+            b"dir/subdir",
+            Path::new(""),
+        )])
         .unwrap();
 
         let mut set = HashSet::new();
@@ -886,14 +863,11 @@
         );
 
         // VisitchildrensetGlob
-        let (matcher, _) = IncludeMatcher::new(
-            vec![IgnorePattern::new(
-                PatternSyntax::Glob,
-                b"dir/z*",
-                Path::new(""),
-            )],
-            "".as_ref(),
-        )
+        let matcher = IncludeMatcher::new(vec![IgnorePattern::new(
+            PatternSyntax::Glob,
+            b"dir/z*",
+            Path::new(""),
+        )])
         .unwrap();
 
         let mut set = HashSet::new();
diff --git a/rust/hg-core/src/filepatterns.rs b/rust/hg-core/src/filepatterns.rs
--- a/rust/hg-core/src/filepatterns.rs
+++ b/rust/hg-core/src/filepatterns.rs
@@ -41,7 +41,7 @@
 /// Appended to the regexp of globs
 const GLOB_SUFFIX: &[u8; 7] = b"(?:/|$)";
 
-#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub enum PatternSyntax {
     /// A regular expression
     Regexp,
@@ -65,6 +65,14 @@
     Include,
     /// A file of patterns to match against files under the same directory
     SubInclude,
+    /// SubInclude with the result of parsing the included file
+    ///
+    /// Note: there is no ExpandedInclude because that expansion can be done
+    /// in place by replacing the Include pattern by the included patterns.
+    /// SubInclude requires more handling.
+    ///
+    /// Note: `Box` is used to minimize size impact on other enum variants
+    ExpandedSubInclude(Box<SubInclude>),
 }
 
 /// Transforms a glob pattern into a regex
@@ -218,7 +226,9 @@
         PatternSyntax::Glob | PatternSyntax::RootGlob => {
             [glob_to_re(pattern).as_slice(), GLOB_SUFFIX].concat()
         }
-        PatternSyntax::Include | PatternSyntax::SubInclude => unreachable!(),
+        PatternSyntax::Include
+        | PatternSyntax::SubInclude
+        | PatternSyntax::ExpandedSubInclude(_) => unreachable!(),
     }
 }
 
@@ -441,10 +451,10 @@
 pub type PatternResult<T> = Result<T, PatternError>;
 
 /// Wrapper for `read_pattern_file` that also recursively expands `include:`
-/// patterns.
+/// and `subinclude:` patterns.
 ///
-/// `subinclude:` is not treated as a special pattern here: unraveling them
-/// needs to occur in the "ignore" phase.
+/// The former are expanded in place, while `PatternSyntax::ExpandedSubInclude`
+/// is used for the latter to form a tree of patterns.
 pub fn get_patterns_from_file(
     pattern_file: &Path,
     root_dir: &Path,
@@ -453,18 +463,35 @@
     let patterns = patterns
         .into_iter()
         .flat_map(|entry| -> PatternResult<_> {
-            let IgnorePattern {
-                syntax, pattern, ..
-            } = &entry;
-            Ok(match syntax {
+            Ok(match &entry.syntax {
                 PatternSyntax::Include => {
                     let inner_include =
-                        root_dir.join(get_path_from_bytes(&pattern));
+                        root_dir.join(get_path_from_bytes(&entry.pattern));
                     let (inner_pats, inner_warnings) =
                         get_patterns_from_file(&inner_include, root_dir)?;
                     warnings.extend(inner_warnings);
                     inner_pats
                 }
+                PatternSyntax::SubInclude => {
+                    let mut sub_include = SubInclude::new(
+                        &root_dir,
+                        &entry.pattern,
+                        &entry.source,
+                    )?;
+                    let (inner_patterns, inner_warnings) =
+                        get_patterns_from_file(
+                            &sub_include.path,
+                            &sub_include.root,
+                        )?;
+                    sub_include.included_patterns = inner_patterns;
+                    warnings.extend(inner_warnings);
+                    vec![IgnorePattern {
+                        syntax: PatternSyntax::ExpandedSubInclude(Box::new(
+                            sub_include,
+                        )),
+                        ..entry
+                    }]
+                }
                 _ => vec![entry],
             })
         })
@@ -475,6 +502,7 @@
 }
 
 /// Holds all the information needed to handle a `subinclude:` pattern.
+#[derive(Debug, PartialEq, Eq, Clone)]
 pub struct SubInclude {
     /// Will be used for repository (hg) paths that start with this prefix.
     /// It is relative to the current working directory, so comparing against
@@ -484,6 +512,8 @@
     pub path: PathBuf,
     /// Folder in the filesystem where this it applies
     pub root: PathBuf,
+
+    pub included_patterns: Vec<IgnorePattern>,
 }
 
 impl SubInclude {
@@ -513,29 +543,25 @@
             })?,
             path: path.to_owned(),
             root: new_root.to_owned(),
+            included_patterns: Vec::new(),
         })
     }
 }
 
 /// Separate and pre-process subincludes from other patterns for the "ignore"
 /// phase.
-pub fn filter_subincludes<'a>(
-    ignore_patterns: &'a [IgnorePattern],
-    root_dir: &Path,
-) -> Result<(Vec<SubInclude>, Vec<&'a IgnorePattern>), HgPathError> {
+pub fn filter_subincludes(
+    ignore_patterns: Vec<IgnorePattern>,
+) -> Result<(Vec<Box<SubInclude>>, Vec<IgnorePattern>), HgPathError> {
     let mut subincludes = vec![];
     let mut others = vec![];
 
-    for ignore_pattern in ignore_patterns.iter() {
-        let IgnorePattern {
-            syntax,
-            pattern,
-            source,
-        } = ignore_pattern;
-        if *syntax == PatternSyntax::SubInclude {
-            subincludes.push(SubInclude::new(root_dir, pattern, &source)?);
+    for pattern in ignore_patterns {
+        if let PatternSyntax::ExpandedSubInclude(sub_include) = pattern.syntax
+        {
+            subincludes.push(sub_include);
         } else {
-            others.push(ignore_pattern)
+            others.push(pattern)
         }
     }
     Ok((subincludes, others))



To: SimonSapin, #hg-reviewers
Cc: mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20210604/82693c6e/attachment-0002.html>


More information about the Mercurial-patches mailing list