This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7528

rust-matchers: add `FileMatcher` implementation
ClosedPublic

Authored by Alphare on Nov 29 2019, 12:57 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG69c4f3cf2cdf: rust-matchers: add `FileMatcher` implementation
Summary

Mercurial defines an exactmatcher, I find FileMatcher to be clearer, but
am not opposed to using the old name.
This change also switched the order of assert_eq arguments as it is clearer
that way for most people.

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.Nov 29 2019, 12:57 PM
Alphare updated this revision to Diff 18418.Dec 2 2019, 6:49 AM
martinvonz added a subscriber: martinvonz.Dec 10 2019, 4:34 PM
martinvonz added inline comments.
rust/hg-core/src/matchers.rs
128–131

Same comment as on another (already submitted) patch: the argument order feels backwards. Do you mind changing it?

135

It would be slightly simpler if the paths were owned. It doesn't seem too expensive to take ownership of the paths. Do we have any call sites in this series that we can look at?

Alphare added inline comments.Dec 11 2019, 4:29 AM
rust/hg-core/src/matchers.rs
135

There is a single call site for now in D7529, it's really straightforward.

Automated tools like CI can produce matchers with a lot of files and it seems to me not worth changing existing no-allocation code to get rid of a few lifetime annotations. Can we see if it gets too unwieldy?

Alphare edited the summary of this revision. (Show Details)Dec 12 2019, 10:40 AM
Alphare updated this revision to Diff 18634.
Alphare added a commit: rHG69c4f3cf2cdf: rust-matchers: add `FileMatcher` implementation.Dec 17 2019, 9:49 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG69c4f3cf2cdf: rust-matchers: add `FileMatcher` implementation (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/matchers.rs (72 lines)

Diff 18811

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; use crate::{utils::hg_path::HgPath, DirsMultiset, DirstateMapError};
use std::collections::HashSet; use std::collections::HashSet;
use std::iter::FromIterator;
pub enum VisitChildrenSet<'a> { pub enum VisitChildrenSet<'a> {
/// Don't visit anything /// Don't visit anything
Empty, Empty,
/// Only visit this directory /// Only visit this directory
This, This,
/// Visit this directory and these subdirectories /// Visit this directory and these subdirectories
/// TODO Should we implement a `NonEmptyHashSet`? /// TODO Should we implement a `NonEmptyHashSet`?
} }
/// Matches everything. /// Matches everything.
///``` ///```
/// use hg::{ matchers::{Matcher, AlwaysMatcher}, utils::hg_path::HgPath }; /// use hg::{ matchers::{Matcher, AlwaysMatcher}, utils::hg_path::HgPath };
/// ///
/// let matcher = AlwaysMatcher; /// let matcher = AlwaysMatcher;
/// ///
/// assert_eq!(true, matcher.matches(HgPath::new(b"whatever"))); /// assert_eq!(matcher.matches(HgPath::new(b"whatever")), true);
/// assert_eq!(true, matcher.matches(HgPath::new(b"b.txt"))); /// assert_eq!(matcher.matches(HgPath::new(b"b.txt")), true);
/// assert_eq!(true, matcher.matches(HgPath::new(b"main.c"))); /// assert_eq!(matcher.matches(HgPath::new(b"main.c")), true);
/// assert_eq!(true, matcher.matches(HgPath::new(br"re:.*\.c$"))); /// assert_eq!(matcher.matches(HgPath::new(br"re:.*\.c$")), true);
/// ``` /// ```
#[derive(Debug)] #[derive(Debug)]
pub struct AlwaysMatcher; pub struct AlwaysMatcher;
impl Matcher for AlwaysMatcher { impl Matcher for AlwaysMatcher {
fn file_set(&self) -> Option<&HashSet<&HgPath>> { fn file_set(&self) -> Option<&HashSet<&HgPath>> {
None None
} }
} }
fn matches_everything(&self) -> bool { fn matches_everything(&self) -> bool {
true true
} }
fn is_exact(&self) -> bool { fn is_exact(&self) -> bool {
false false
} }
} }
/// Matches the input files exactly. They are interpreted as paths, not
/// patterns.
///
///```
/// use hg::{ matchers::{Matcher, FileMatcher}, utils::hg_path::HgPath };
///
/// let files = [HgPath::new(b"a.txt"), HgPath::new(br"re:.*\.c$")];
/// let matcher = FileMatcher::new(&files).unwrap();
///
/// assert_eq!(matcher.matches(HgPath::new(b"a.txt")), true);
/// assert_eq!(matcher.matches(HgPath::new(b"b.txt")), false);
/// assert_eq!(matcher.matches(HgPath::new(b"main.c")), false);
/// assert_eq!(matcher.matches(HgPath::new(br"re:.*\.c$")), true);
martinvonzUnsubmitted
Not Done

Same comment as on another (already submitted) patch: the argument order feels backwards. Do you mind changing it?

martinvonz: Same comment as on another (already submitted) patch: the argument order feels backwards. Do…
/// ```
#[derive(Debug)]
pub struct FileMatcher<'a> {
files: HashSet<&'a HgPath>,
martinvonzUnsubmitted
Not Done

It would be slightly simpler if the paths were owned. It doesn't seem too expensive to take ownership of the paths. Do we have any call sites in this series that we can look at?

martinvonz: It would be slightly simpler if the paths were owned. It doesn't seem too expensive to take…
AlphareAuthorUnsubmitted
Done

There is a single call site for now in D7529, it's really straightforward.

Automated tools like CI can produce matchers with a lot of files and it seems to me not worth changing existing no-allocation code to get rid of a few lifetime annotations. Can we see if it gets too unwieldy?

Alphare: There is a single call site for now in D7529, it's really straightforward. Automated tools…
dirs: DirsMultiset,
}
impl<'a> FileMatcher<'a> {
pub fn new(
files: &'a [impl AsRef<HgPath>],
) -> Result<Self, DirstateMapError> {
Ok(Self {
files: HashSet::from_iter(files.iter().map(|f| f.as_ref())),
dirs: DirsMultiset::from_manifest(files)?,
})
}
fn inner_matches(&self, filename: impl AsRef<HgPath>) -> bool {
self.files.contains(filename.as_ref())
}
}
impl<'a> Matcher for FileMatcher<'a> {
fn file_set(&self) -> Option<&HashSet<&HgPath>> {
Some(&self.files)
}
fn exact_match(&self, filename: impl AsRef<HgPath>) -> bool {
self.inner_matches(filename)
}
fn matches(&self, filename: impl AsRef<HgPath>) -> bool {
self.inner_matches(filename)
}
fn visit_children_set(
&self,
_directory: impl AsRef<HgPath>,
) -> VisitChildrenSet {
// TODO implement once we have `status.traverse`
// This is useless until unknown files are taken into account
// Which will not need to happen before the `IncludeMatcher`.
unimplemented!()
}
fn matches_everything(&self) -> bool {
false
}
fn is_exact(&self) -> bool {
true
}
}