match: fix the rust-side bug in visit_children_set for rootfilesin matchers stable
authorArseniy Alekseyev <aalekseyev@janestreet.com>
Fri, 12 Apr 2024 16:09:45 +0100
branchstable
changeset 51570 b39057b713b1
parent 51569 b32c3146ec34
child 51571 74230abb2504
match: fix the rust-side bug in visit_children_set for rootfilesin matchers The fix is checked by `test_pattern_matcher_visit_children_set` test, which is what caught the bug in the first place, but also by an end-to-end test that I made for this purpose. Accept the new results of Cargo tests Many of these were already annotated with "FIXME", which is a good sign.
rust/hg-core/src/dirstate/dirs_multiset.rs
rust/hg-core/src/matchers.rs
tests/test-status.t
--- a/rust/hg-core/src/dirstate/dirs_multiset.rs	Fri Apr 12 15:39:21 2024 +0100
+++ b/rust/hg-core/src/dirstate/dirs_multiset.rs	Fri Apr 12 16:09:45 2024 +0100
@@ -158,14 +158,13 @@
 }
 
 impl<'a> DirsChildrenMultiset<'a> {
-    pub fn new(
+    pub fn new<I: Iterator<Item = &'a HgPathBuf>>(
         paths: impl Iterator<Item = &'a HgPathBuf>,
-        only_include: Option<&'a HashSet<impl AsRef<HgPath> + 'a>>,
+        only_include: Option<I>,
     ) -> Self {
         let mut new = Self {
             inner: HashMap::default(),
-            only_include: only_include
-                .map(|s| s.iter().map(AsRef::as_ref).collect()),
+            only_include: only_include.map(|s| s.map(AsRef::as_ref).collect()),
         };
 
         for path in paths {
--- a/rust/hg-core/src/matchers.rs	Fri Apr 12 15:39:21 2024 +0100
+++ b/rust/hg-core/src/matchers.rs	Fri Apr 12 16:09:45 2024 +0100
@@ -299,6 +299,7 @@
     /// Whether all the patterns match a prefix (i.e. recursively)
     prefix: bool,
     files: HashSet<HgPathBuf>,
+    dirs_explicit: HashSet<HgPathBuf>,
     dirs: DirsMultiset,
 }
 
@@ -315,8 +316,13 @@
 
 impl<'a> PatternMatcher<'a> {
     pub fn new(ignore_patterns: Vec<IgnorePattern>) -> PatternResult<Self> {
-        let (files, _) = roots_and_dirs(&ignore_patterns);
-        let dirs = DirsMultiset::from_manifest(&files)?;
+        let RootsDirsAndParents {
+            roots,
+            dirs: dirs_explicit,
+            parents,
+        } = roots_dirs_and_parents(&ignore_patterns)?;
+        let files = roots;
+        let dirs = parents;
         let files: HashSet<HgPathBuf> = HashSet::from_iter(files);
 
         let prefix = ignore_patterns.iter().all(|k| {
@@ -330,6 +336,7 @@
             prefix,
             files,
             dirs,
+            dirs_explicit,
         })
     }
 }
@@ -357,9 +364,10 @@
         if self.dirs.contains(directory) {
             return VisitChildrenSet::This;
         }
-        if dir_ancestors(directory)
-            .any(|parent_dir| self.files.contains(parent_dir))
-        {
+        if dir_ancestors(directory).any(|parent_dir| {
+            self.files.contains(parent_dir)
+                || self.dirs_explicit.contains(parent_dir)
+        }) {
             VisitChildrenSet::This
         } else {
             VisitChildrenSet::Empty
@@ -410,7 +418,7 @@
     prefix: bool,
     roots: HashSet<HgPathBuf>,
     dirs: HashSet<HgPathBuf>,
-    parents: HashSet<HgPathBuf>,
+    parents: DirsMultiset,
 }
 
 impl core::fmt::Debug for IncludeMatcher<'_> {
@@ -890,7 +898,7 @@
     /// Directories to match non-recursively
     pub dirs: HashSet<HgPathBuf>,
     /// Implicitly required directories to go to items in either roots or dirs
-    pub parents: HashSet<HgPathBuf>,
+    pub parents: DirsMultiset,
 }
 
 /// Extract roots, dirs and parents from patterns.
@@ -899,18 +907,11 @@
 ) -> PatternResult<RootsDirsAndParents> {
     let (roots, dirs) = roots_and_dirs(ignore_patterns);
 
-    let mut parents = HashSet::new();
+    let mut parents = DirsMultiset::from_manifest(&dirs)?;
 
-    parents.extend(
-        DirsMultiset::from_manifest(&dirs)?
-            .iter()
-            .map(ToOwned::to_owned),
-    );
-    parents.extend(
-        DirsMultiset::from_manifest(&roots)?
-            .iter()
-            .map(ToOwned::to_owned),
-    );
+    for path in &roots {
+        parents.add_path(path)?
+    }
 
     Ok(RootsDirsAndParents {
         roots: HashSet::from_iter(roots),
@@ -1082,7 +1083,7 @@
             .iter()
             .chain(self.roots.iter())
             .chain(self.parents.iter());
-        DirsChildrenMultiset::new(thing, Some(&self.parents))
+        DirsChildrenMultiset::new(thing, Some(self.parents.iter()))
     }
 
     pub fn debug_get_patterns(&self) -> &[u8] {
@@ -1149,9 +1150,12 @@
 
         let dirs = HashSet::new();
 
-        let mut parents = HashSet::new();
-        parents.insert(HgPathBuf::new());
-        parents.insert(HgPathBuf::from_bytes(b"g"));
+        let parents = DirsMultiset::from_manifest(&[
+            HgPathBuf::from_bytes(b"x"),
+            HgPathBuf::from_bytes(b"g/x"),
+            HgPathBuf::from_bytes(b"g/y"),
+        ])
+        .unwrap();
 
         assert_eq!(
             roots_dirs_and_parents(&pats).unwrap(),
@@ -1331,24 +1335,23 @@
         .unwrap();
         assert_eq!(
             m.visit_children_set(HgPath::new(b"dir/subdir/x")),
-            VisitChildrenSet::Empty
+            VisitChildrenSet::This
         );
         assert_eq!(
             m.visit_children_set(HgPath::new(b"folder")),
             VisitChildrenSet::Empty
         );
-        // FIXME: These should probably be This.
         assert_eq!(
             m.visit_children_set(HgPath::new(b"")),
-            VisitChildrenSet::Empty
+            VisitChildrenSet::This
         );
         assert_eq!(
             m.visit_children_set(HgPath::new(b"dir")),
-            VisitChildrenSet::Empty
+            VisitChildrenSet::This
         );
         assert_eq!(
             m.visit_children_set(HgPath::new(b"dir/subdir")),
-            VisitChildrenSet::Empty
+            VisitChildrenSet::This
         );
 
         // VisitchildrensetRootfilesin
@@ -1360,25 +1363,25 @@
         .unwrap();
         assert_eq!(
             m.visit_children_set(HgPath::new(b"dir/subdir/x")),
-            VisitChildrenSet::Empty
+            VisitChildrenSet::This
         );
         assert_eq!(
             m.visit_children_set(HgPath::new(b"folder")),
             VisitChildrenSet::Empty
         );
         // FIXME: These should probably be {'dir'}, {'subdir'} and This,
-        // respectively, or at least This for all three.
+        // respectively
         assert_eq!(
             m.visit_children_set(HgPath::new(b"")),
-            VisitChildrenSet::Empty
+            VisitChildrenSet::This
         );
         assert_eq!(
             m.visit_children_set(HgPath::new(b"dir")),
-            VisitChildrenSet::Empty
+            VisitChildrenSet::This
         );
         assert_eq!(
             m.visit_children_set(HgPath::new(b"dir/subdir")),
-            VisitChildrenSet::Empty
+            VisitChildrenSet::This
         );
 
         // VisitdirGlob
@@ -1392,10 +1395,9 @@
             m.visit_children_set(HgPath::new(b"")),
             VisitChildrenSet::This
         );
-        // FIXME: This probably should be This
         assert_eq!(
             m.visit_children_set(HgPath::new(b"dir")),
-            VisitChildrenSet::Empty
+            VisitChildrenSet::This
         );
         assert_eq!(
             m.visit_children_set(HgPath::new(b"folder")),
@@ -1426,10 +1428,9 @@
             m.visit_children_set(HgPath::new(b"folder")),
             VisitChildrenSet::Empty
         );
-        // FIXME: This probably should be This
         assert_eq!(
             m.visit_children_set(HgPath::new(b"dir")),
-            VisitChildrenSet::Empty
+            VisitChildrenSet::This
         );
         // OPT: these should probably be Empty
         assert_eq!(
@@ -2341,7 +2342,7 @@
             HgPathBuf::from_bytes(b"a/b/c/d/file.txt"),
         ];
         let file_abcdfile = FileMatcher::new(files).unwrap();
-        let _rootfilesin_dir = PatternMatcher::new(vec![IgnorePattern::new(
+        let rootfilesin_dir = PatternMatcher::new(vec![IgnorePattern::new(
             PatternSyntax::RootFilesIn,
             b"dir",
             Path::new(""),
@@ -2419,12 +2420,7 @@
         );
         tree.check_matcher(&file_dir_subdir_b, 1);
         tree.check_matcher(&file_abcdfile, 4);
-        //        // TODO: re-enable this test when the corresponding bug is
-        // fixed
-        //
-        //        if false {
-        //            tree.check_matcher(&rootfilesin_dir, 6);
-        //        }
+        tree.check_matcher(&rootfilesin_dir, 8);
         tree.check_matcher(&pattern_filepath_dir_subdir, 1);
         tree.check_matcher(&include_dir_subdir, 9);
         tree.check_matcher(&more_includematchers[0], 17);
--- a/tests/test-status.t	Fri Apr 12 15:39:21 2024 +0100
+++ b/tests/test-status.t	Fri Apr 12 16:09:45 2024 +0100
@@ -856,14 +856,11 @@
   ! subdir/deleted
   ? subdir/unknown
 
-FIXME: it's a bug (both in rhg and in Python) that the status below is wrong,
-in rhg it's empty, in Python it's missing the unknown file:
-
   $ hg status rootfilesin:subdir
-  M subdir/modified (no-rhg !)
-  R subdir/removed (no-rhg !)
-  ! subdir/deleted (no-rhg !)
-  ? subdir/unknown (no-rhg !)
+  M subdir/modified
+  R subdir/removed
+  ! subdir/deleted
+  ? subdir/unknown
 
 Note: `hg status some-name` creates a patternmatcher which is not supported
 yet by the Rust implementation of status, but includematcher is supported.