Mercurial > hg
changeset 51570:b39057b713b1 stable
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.
| author | Arseniy Alekseyev <aalekseyev@janestreet.com> |
|---|---|
| date | Fri, 12 Apr 2024 16:09:45 +0100 |
| parents | b32c3146ec34 |
| children | 74230abb2504 |
| files | rust/hg-core/src/dirstate/dirs_multiset.rs rust/hg-core/src/matchers.rs tests/test-status.t |
| diffstat | 3 files changed, 46 insertions(+), 54 deletions(-) [+] |
line wrap: on
line diff
--- 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.