Re: [pbs-devel] [PATCH pathpatterns v3] match_list: updated `matches()`, to only retrieve file mode if necessary

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Fri, Aug 18, 2023 at 04:32:11PM +0200, Gabriel Goller wrote:
> Updated `matches()` function, which now takes the `GetFileMode` trait, that
> has a function that should return the `file_mode`. `matches()` will go
> through the patterns and match by path only until it finds a pattern which
> does not have `MatchFlag::ANY_FILE_TYPE`, in case it will call the
> `get_file_mode`, which will return a `file_mode`. This will ensure that the
> `get()` (which in our case executes `stat()`) will only be run once(at most),
> every pattern will only be processed once and that the order of the patterns
> will be respected.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> 
> changes v2:
>  - updated the `matches_path()` function to take a closure and renamed
>    it to `matches_mode_lazy()`. This allows us to only match once, so we
>    don't need to match before and after `stat()` (without and with
>    `file_mode`) anymore.
> 
> changes v1:
>  - added `matches_path()` function, which matches by path only and returns an
>    error if the `file_mode` is required.
> 
>  src/lib.rs        |  72 +++++-----
>  src/match_list.rs | 337 ++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 315 insertions(+), 94 deletions(-)
> 
> diff --git a/src/lib.rs b/src/lib.rs
> index cb89917..998cdae 100644
> --- a/src/lib.rs
> +++ b/src/lib.rs
> @@ -31,17 +31,17 @@
>  //!     MatchEntry::include(Pattern::path("bananas/curved.*")?),
>  //! ];
>  //!
> -//! assert_eq!(list.matches("/things", None), None);
> -//! assert_eq!(list.matches("/things/shop", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop/bananas", None), Some(MatchType::Exclude));
> -//! assert_eq!(list.matches("/things/shop/bananas/curved.txt", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop/bananas/curved.bak", None), Some(MatchType::Include));
> +//! assert_eq!(list.matches("/things", None), Ok(None));
> +//! assert_eq!(list.matches("/things/shop", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop/bananas", None), Ok(Some(MatchType::Exclude)));
> +//! assert_eq!(list.matches("/things/shop/bananas/curved.txt", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop/bananas/curved.bak", None), Ok(Some(MatchType::Include)));
>  //!
>  //! // this will exclude the curved.bak file
>  //! list.push(MatchEntry::exclude(Pattern::path("curved.bak")?));
> -//! assert_eq!(list.matches("/things/shop/bananas/curved.bak", None), Some(MatchType::Exclude));
> +//! assert_eq!(list.matches("/things/shop/bananas/curved.bak", None), Ok(Some(MatchType::Exclude)));
>  //! list.pop();
> -//! # assert_eq!(list.matches("/things/shop/bananas/curved.bak", None), Some(MatchType::Include));
> +//! # assert_eq!(list.matches("/things/shop/bananas/curved.bak", None), Ok(Some(MatchType::Include)));
>  //!
>  //! // but this will not:
>  //! list.push(
> @@ -49,40 +49,40 @@
>  //!         .flags(MatchFlag::ANCHORED)
>  //! );
>  //! // or: list.push
> -//! assert_eq!(list.matches("/things/shop/bananas/curved.bak", None), Some(MatchType::Include));
> +//! assert_eq!(list.matches("/things/shop/bananas/curved.bak", None), Ok(Some(MatchType::Include)));
>  //! list.pop();
>  //!
>  //! // let's check some patterns, anything starting with a 'c', 'f' or 's':
>  //! let mut list = vec![MatchEntry::include(Pattern::path("[cfs]*")?)];
> -//! assert_eq!(list.matches("/things", None), None);
> -//! assert_eq!(list.matches("/things/file1.dat", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/file2.dat", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop/info.txt", None), None);
> -//! assert_eq!(list.matches("/things/shop/apples", None), None);
> -//! assert_eq!(list.matches("/things/shop/apples/gala.txt", None), None);
> -//! assert_eq!(list.matches("/things/shop/apples/golden-delicious.txt", None), None);
> -//! assert_eq!(list.matches("/things/shop/bananas", None), None);
> -//! assert_eq!(list.matches("/things/shop/bananas/straight.txt", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop/bananas/curved.txt", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/shop/bananas/curved.bak", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop/bananas/other.txt", None), None);
> +//! assert_eq!(list.matches("/things", None), Ok(None));
> +//! assert_eq!(list.matches("/things/file1.dat", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/file2.dat", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop/info.txt", None), Ok(None));
> +//! assert_eq!(list.matches("/things/shop/apples", None), Ok(None));
> +//! assert_eq!(list.matches("/things/shop/apples/gala.txt", None), Ok(None));
> +//! assert_eq!(list.matches("/things/shop/apples/golden-delicious.txt", None), Ok(None));
> +//! assert_eq!(list.matches("/things/shop/bananas", None), Ok(None));
> +//! assert_eq!(list.matches("/things/shop/bananas/straight.txt", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop/bananas/curved.txt", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/shop/bananas/curved.bak", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop/bananas/other.txt", None), Ok(None));
>  //!
>  //! // If we add `**` we end up including the entire `shop/` subtree:
>  //! list.push(MatchEntry::include(Pattern::path("[cfs]*/**")?));
> -//! assert_eq!(list.matches("/things", None), None);
> -//! assert_eq!(list.matches("/things/file1.dat", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/file2.dat", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop/info.txt", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop/apples", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop/apples/gala.txt", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/shop/apples/golden-delicious.txt", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop/bananas", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop/bananas/straight.txt", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/things/shop/bananas/curved.txt", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/shop/bananas/curved.bak", None), Some(MatchType::Include));
> -//! assert_eq!(list.matches("/shop/bananas/other.txt", None), Some(MatchType::Include));
> +//! assert_eq!(list.matches("/things", None), Ok(None));
> +//! assert_eq!(list.matches("/things/file1.dat", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/file2.dat", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop/info.txt", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop/apples", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop/apples/gala.txt", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/shop/apples/golden-delicious.txt", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop/bananas", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop/bananas/straight.txt", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/things/shop/bananas/curved.txt", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/shop/bananas/curved.bak", None), Ok(Some(MatchType::Include)));
> +//! assert_eq!(list.matches("/shop/bananas/other.txt", None), Ok(Some(MatchType::Include)));
>  //! # Ok(())
>  //! # }
>  //! # test().unwrap()
> @@ -92,7 +92,9 @@ mod match_list;
>  mod pattern;
>  
>  #[doc(inline)]
> -pub use match_list::{MatchEntry, MatchFlag, MatchList, MatchPattern, MatchType};
> +pub use match_list::{
> +    FileModeRequired, GetFileMode, MatchEntry, MatchFlag, MatchList, MatchPattern, MatchType,
> +};
>  
>  #[doc(inline)]
>  pub use pattern::{ParseError, Pattern, PatternFlag};
> diff --git a/src/match_list.rs b/src/match_list.rs
> index c5b14e0..51aa65d 100644
> --- a/src/match_list.rs
> +++ b/src/match_list.rs
> @@ -1,5 +1,4 @@
>  //! Helpers for include/exclude lists.
> -
>  use bitflags::bitflags;
>  
>  use crate::PatternFlag;
> @@ -39,6 +38,37 @@ impl Default for MatchFlag {
>      }
>  }
>  
> +#[derive(Debug, PartialEq)]

Aaaah shoot, can't put the `PartialEq` behind `cfg(test)` due to
doctests :(

Well, those doctests look ugly in the docs anyway, so I suppose a future
todo-item would be to move those into regular tests and guard this
derive.
But for now this is fine (definitely out of scope of this series)!

> +pub struct FileModeRequired {}

^ Why does this have an empty body now? We don't need that, please drop
it :-)

> +impl std::fmt::Display for FileModeRequired {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        write!(f, "File mode is required for matching")
> +    }
> +}
> +impl std::error::Error for FileModeRequired {}
> +
> +pub trait GetFileMode {
> +    type Error;
> +    fn get(self) -> Result<u32, Self::Error>;
> +}
> +
> +impl<T, E> GetFileMode for T
> +where
> +    T: FnOnce() -> Result<u32, E>,
> +{
> +    type Error = E;
> +    fn get(self) -> Result<u32, Self::Error> {
> +        self()
> +    }
> +}
> +
> +impl GetFileMode for Option<u32> {
> +    type Error = FileModeRequired;
> +    fn get(self) -> Result<u32, Self::Error> {
> +        self.ok_or(FileModeRequired {})
> +    }
> +}
> +
>  /// A pattern entry. (Glob patterns or literal patterns.)
>  // Note:
>  // For regex we'd likely use the POSIX extended REs via `regexec(3)`, since we're targetting
> @@ -211,7 +241,7 @@ impl MatchEntry {
>      pub fn matches_mode(&self, file_mode: u32) -> bool {
>          // bitflags' `.contains` means ALL bits must be set, if they are all set we don't
>          // need to check the mode...
> -        if self.flags.contains(MatchFlag::ANY_FILE_TYPE) {
> +        if !self.needs_file_mode() {
>              return true;
>          }
>  
> @@ -304,12 +334,21 @@ impl MatchEntry {
>  
>          self.matches_path_exact(path)
>      }
> +
> +    pub fn needs_file_mode(&self) -> bool {
> +        let flags = (self.flags & MatchFlag::ANY_FILE_TYPE).bits();
> +        if flags != 0 && flags != MatchFlag::ANY_FILE_TYPE.bits() {
> +            return true;
> +        }
> +        false
> +    }
>  }
>  
>  #[doc(hidden)]
>  pub trait MatchListEntry {
>      fn entry_matches(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
>      fn entry_matches_exact(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
> +    fn entry_needs_file_mode(&self) -> bool;
>  }
>  
>  impl MatchListEntry for &'_ MatchEntry {
> @@ -328,6 +367,10 @@ impl MatchListEntry for &'_ MatchEntry {
>              None
>          }
>      }
> +
> +    fn entry_needs_file_mode(&self) -> bool {
> +        self.needs_file_mode()
> +    }
>  }
>  
>  impl MatchListEntry for &'_ &'_ MatchEntry {
> @@ -346,6 +389,10 @@ impl MatchListEntry for &'_ &'_ MatchEntry {
>              None
>          }
>      }
> +
> +    fn entry_needs_file_mode(&self) -> bool {
> +        self.needs_file_mode()
> +    }
>  }
>  
>  /// This provides [`matches`](MatchList::matches) and [`matches_exact`](MatchList::matches_exact)
> @@ -361,19 +408,26 @@ impl MatchListEntry for &'_ &'_ MatchEntry {
>  /// This makes it easier to use slices over entries or references to entries.
>  pub trait MatchList {
>      /// Check whether this list contains anything matching a prefix of the specified path, and the
> -    /// specified file mode.
> -    fn matches<T: AsRef<[u8]>>(&self, path: T, file_mode: Option<u32>) -> Option<MatchType> {
> -        self.matches_do(path.as_ref(), file_mode)
> -    }
> -
> -    fn matches_do(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
> -
> -    /// Check whether this list contains anything exactly matching the path and mode.
> -    fn matches_exact<T: AsRef<[u8]>>(&self, path: T, file_mode: Option<u32>) -> Option<MatchType> {
> -        self.matches_exact_do(path.as_ref(), file_mode)
> -    }
> -
> -    fn matches_exact_do(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
> +    /// specified file mode. Gets the file_mode lazily, only if needed.
> +    fn matches<T, U>(
> +        &self,
> +        path: T,
> +        get_file_mode: U,
> +    ) -> Result<Option<MatchType>, <U as GetFileMode>::Error>

Just use `U::Error`, formats cleaner.

> +    where
> +        T: AsRef<[u8]>,
> +        U: GetFileMode;
> +
> +    /// Check whether this list contains anything exactly matching the path and mode. Gets the
> +    /// file_mode lazily, only if needed.
> +    fn matches_exact<T, U>(
> +        &self,
> +        path: T,
> +        get_file_mode: U,
> +    ) -> Result<Option<MatchType>, <U as GetFileMode>::Error>
> +    where
> +        T: AsRef<[u8]>,
> +        U: GetFileMode;
>  }
>  
>  impl<'a, T> MatchList for T
> @@ -383,30 +437,56 @@ where
>      <&'a T as IntoIterator>::IntoIter: DoubleEndedIterator,
>      <&'a T as IntoIterator>::Item: MatchListEntry,
>  {
> -    fn matches_do(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType> {
> +    fn matches<U, G>(
> +        &self,
> +        path: U,
> +        get_file_mode: G,
> +    ) -> Result<Option<MatchType>, <G as GetFileMode>::Error>
> +    where
> +        U: AsRef<[u8]>,
> +        G: GetFileMode,
> +    {
>          // This is an &self method on a `T where T: 'a`.
>          let this: &'a Self = unsafe { std::mem::transmute(self) };
>  
> +        let mut get_file_mode = Some(get_file_mode);
> +        let mut file_mode = None;
> +
>          for m in this.into_iter().rev() {
> -            if let Some(mt) = m.entry_matches(path, file_mode) {
> -                return Some(mt);
> +            if file_mode.is_none() && m.entry_needs_file_mode() {
> +                file_mode = Some(get_file_mode.take().unwrap().get()?);
> +            }
> +            if let Some(mt) = m.entry_matches(path.as_ref(), file_mode) {
> +                return Ok(Some(mt));
>              }
>          }
> -
> -        None
> -    }
> -
> -    fn matches_exact_do(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType> {
> +        Ok(None)
> +    }
> +
> +    fn matches_exact<U, G>(
> +        &self,
> +        path: U,
> +        get_file_mode: G,
> +    ) -> Result<Option<MatchType>, <G as GetFileMode>::Error>
> +    where
> +        U: AsRef<[u8]>,
> +        G: GetFileMode,
> +    {
>          // This is an &self method on a `T where T: 'a`.
>          let this: &'a Self = unsafe { std::mem::transmute(self) };
>  
> +        let mut get_file_mode = Some(get_file_mode);
> +        let mut file_mode = None;
> +
>          for m in this.into_iter().rev() {
> -            if let Some(mt) = m.entry_matches_exact(path, file_mode) {
> -                return Some(mt);
> +            if file_mode.is_none() && m.entry_needs_file_mode() {
> +                file_mode = Some(get_file_mode.take().unwrap().get()?);
> +            }
> +            if let Some(mt) = m.entry_matches_exact(path.as_ref(), file_mode) {
> +                return Ok(Some(mt));
>              }
>          }
> -
> -        None
> +        Ok(None)
>      }
>  }
>  
> @@ -415,20 +495,20 @@ fn assert_containers_implement_match_list() {
>      use std::iter::FromIterator;
>  
>      let vec = vec![MatchEntry::include(crate::Pattern::path("a*").unwrap())];
> -    assert_eq!(vec.matches("asdf", None), Some(MatchType::Include));
> +    assert_eq!(vec.matches("asdf", None), Ok(Some(MatchType::Include)));
>  
>      // FIXME: ideally we can make this work as well!
>      let vd = std::collections::VecDeque::<MatchEntry>::from_iter(vec.clone());
> -    assert_eq!(vd.matches("asdf", None), Some(MatchType::Include));
> +    assert_eq!(vd.matches("asdf", None), Ok(Some(MatchType::Include)));
>  
>      let list: &[MatchEntry] = &vec[..];
> -    assert_eq!(list.matches("asdf", None), Some(MatchType::Include));
> +    assert_eq!(list.matches("asdf", None), Ok(Some(MatchType::Include)));
>  
>      let list: Vec<&MatchEntry> = vec.iter().collect();
> -    assert_eq!(list.matches("asdf", None), Some(MatchType::Include));
> +    assert_eq!(list.matches("asdf", None), Ok(Some(MatchType::Include)));
>  
>      let list: &[&MatchEntry] = &list[..];
> -    assert_eq!(list.matches("asdf", None), Some(MatchType::Include));
> +    assert_eq!(list.matches("asdf", None), Ok(Some(MatchType::Include)));
>  }
>  
>  #[test]
> @@ -443,25 +523,28 @@ fn test_file_type_matches() {
>      ];
>      assert_eq!(
>          matchlist.matches("a_dir", Some(libc::S_IFDIR)),
> -        Some(MatchType::Include)
> +        Ok(Some(MatchType::Include))
>      );
>      assert_eq!(
>          matchlist.matches("/a_dir", Some(libc::S_IFDIR)),
> -        Some(MatchType::Include)
> +        Ok(Some(MatchType::Include))
>      );
> -    assert_eq!(matchlist.matches("/a_dir", Some(libc::S_IFREG)), None);
> +    assert_eq!(matchlist.matches("/a_dir", Some(libc::S_IFREG)), Ok(None));
>  
>      assert_eq!(
>          matchlist.matches("/a_file", Some(libc::S_IFREG)),
> -        Some(MatchType::Exclude)
> +        Ok(Some(MatchType::Exclude))
>      );
> -    assert_eq!(matchlist.matches("/a_file", Some(libc::S_IFDIR)), None);
> +    assert_eq!(matchlist.matches("/a_file", Some(libc::S_IFDIR)), Ok(None));
>  
>      assert_eq!(
>          matchlist.matches("/another_dir", Some(libc::S_IFDIR)),
> -        Some(MatchType::Exclude)
> +        Ok(Some(MatchType::Exclude))
> +    );
> +    assert_eq!(
> +        matchlist.matches("/another_dir", Some(libc::S_IFREG)),
> +        Ok(None)
>      );
> -    assert_eq!(matchlist.matches("/another_dir", Some(libc::S_IFREG)), None);
>  }
>  
>  #[test]
> @@ -471,22 +554,25 @@ fn test_anchored_matches() {
>      let matchlist = vec![
>          MatchEntry::new(Pattern::path("file-a").unwrap(), MatchType::Include),
>          MatchEntry::new(Pattern::path("some/path").unwrap(), MatchType::Include)
> -            .flags(MatchFlag::ANCHORED),
> +            .add_flags(MatchFlag::ANCHORED),
>      ];
>  
> -    assert_eq!(matchlist.matches("file-a", None), Some(MatchType::Include));
> +    assert_eq!(
> +        matchlist.matches("file-a", None),
> +        Ok(Some(MatchType::Include))
> +    );
>      assert_eq!(
>          matchlist.matches("another/file-a", None),
> -        Some(MatchType::Include)
> +        Ok(Some(MatchType::Include))
>      );
>  
> -    assert_eq!(matchlist.matches("some", None), None);
> -    assert_eq!(matchlist.matches("path", None), None);
> +    assert_eq!(matchlist.matches("some", None), Ok(None));
> +    assert_eq!(matchlist.matches("path", None), Ok(None));
>      assert_eq!(
>          matchlist.matches("some/path", None),
> -        Some(MatchType::Include)
> +        Ok(Some(MatchType::Include))
>      );
> -    assert_eq!(matchlist.matches("another/some/path", None), None);
> +    assert_eq!(matchlist.matches("another/some/path", None), Ok(None));
>  }
>  
>  #[test]
> @@ -495,7 +581,10 @@ fn test_literal_matches() {
>          MatchPattern::Literal(b"/bin/mv".to_vec()),
>          MatchType::Include,
>      )];
> -    assert_eq!(matchlist.matches("/bin/mv", None), Some(MatchType::Include));
> +    assert_eq!(
> +        matchlist.matches("/bin/mv", None),
> +        Ok(Some(MatchType::Include))
> +    );
>  }
>  
>  #[test]
> @@ -504,29 +593,159 @@ fn test_path_relativity() {
>      let matchlist = vec![
>          MatchEntry::new(Pattern::path("noslash").unwrap(), MatchType::Include),
>          MatchEntry::new(Pattern::path("noslash-a").unwrap(), MatchType::Include)
> -            .flags(MatchFlag::ANCHORED),
> +            .add_flags(MatchFlag::ANCHORED),
>          MatchEntry::new(Pattern::path("/slash").unwrap(), MatchType::Include),
>          MatchEntry::new(Pattern::path("/slash-a").unwrap(), MatchType::Include)
> -            .flags(MatchFlag::ANCHORED),
> +            .add_flags(MatchFlag::ANCHORED),
>      ];
> -    assert_eq!(matchlist.matches("noslash", None), Some(MatchType::Include));
> +    assert_eq!(
> +        matchlist.matches("noslash", None),
> +        Ok(Some(MatchType::Include))
> +    );
>      assert_eq!(
>          matchlist.matches("noslash-a", None),
> -        Some(MatchType::Include)
> +        Ok(Some(MatchType::Include))
>      );
> -    assert_eq!(matchlist.matches("slash", None), None);
> -    assert_eq!(matchlist.matches("/slash", None), Some(MatchType::Include));
> -    assert_eq!(matchlist.matches("slash-a", None), None);
> +    assert_eq!(matchlist.matches("slash", None), Ok(None));
> +    assert_eq!(
> +        matchlist.matches("/slash", None),
> +        Ok(Some(MatchType::Include))
> +    );
> +    assert_eq!(matchlist.matches("slash-a", None), Ok(None));
>      assert_eq!(
>          matchlist.matches("/slash-a", None),
> -        Some(MatchType::Include)
> +        Ok(Some(MatchType::Include))
>      );
>  
>      assert_eq!(
>          matchlist.matches("foo/noslash", None),
> -        Some(MatchType::Include)
> +        Ok(Some(MatchType::Include))
>      );
> -    assert_eq!(matchlist.matches("foo/noslash-a", None), None);
> -    assert_eq!(matchlist.matches("foo/slash", None), None);
> -    assert_eq!(matchlist.matches("foo/slash-a", None), None);
> +    assert_eq!(matchlist.matches("foo/noslash-a", None), Ok(None));
> +    assert_eq!(matchlist.matches("foo/slash", None), Ok(None));
> +    assert_eq!(matchlist.matches("foo/slash-a", None), Ok(None));
> +}
> +
> +#[test]
> +fn matches_path() {
> +    use crate::Pattern;
> +
> +    let matchlist = vec![
> +        MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude),
> +        MatchEntry::new(Pattern::path("b*").unwrap(), MatchType::Exclude),
> +    ];
> +
> +    assert_eq!(
> +        matchlist.matches("ahsjdj", || Err(FileModeRequired {})),
> +        Ok(Some(MatchType::Exclude))
> +    );
> +    let mut _test = MatchFlag::ANY_FILE_TYPE;

^ Not sure what the point of this is. If it's only to test the
assignment/mutability, maybe don't use `MatchFlag` for the contents,
that's a bit confusing/misleading, and maybe include a test that would
actually trigger the access as well and also actually validate the final
value.

> +    assert_eq!(
> +        matchlist.matches("bhshdf", || {
> +            _test = MatchFlag::MATCH_DIRECTORIES;
> +            Ok::<u32, FileModeRequired>(libc::S_IFDIR)
> +        }),
> +        Ok(Some(MatchType::Exclude))
> +    );
> +
> +    let matchlist = vec![
> +        MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
> +            .flags(MatchFlag::MATCH_DIRECTORIES),
> +        MatchEntry::new(Pattern::path("b*").unwrap(), MatchType::Exclude),
> +    ];
> +
> +    assert_eq!(
> +        matchlist.matches("ahsjdj", || Err(FileModeRequired {})),
> +        Err(FileModeRequired {})
> +    );
> +    assert_eq!(
> +        matchlist.matches("bhshdf", || Err(FileModeRequired {})),
> +        Ok(Some(MatchType::Exclude))
> +    );
> +    assert_eq!(
> +        matchlist.matches("ahsjdj", || Ok::<u32, FileModeRequired>(libc::S_IFDIR)),
> +        Ok(Some(MatchType::Exclude))
> +    );

Could also have a case with a different mode here.

> +
> +    let matchlist = vec![
> +        MatchEntry::new(Pattern::path("b*").unwrap(), MatchType::Include),
> +        MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
> +            .flags(MatchFlag::MATCH_DIRECTORIES),
> +    ];
> +
> +    assert_eq!(
> +        matchlist.matches("ahsjdj", || Err(FileModeRequired {})),
> +        Err(FileModeRequired {})
> +    );
> +    assert_eq!(
> +        matchlist.matches("bhshdf", || Err(FileModeRequired {})),
> +        Err(FileModeRequired {})
> +    );
> +
> +    assert_eq!(
> +        matchlist.matches("ahsjdj", || Ok::<u32, FileModeRequired>(libc::S_IFDIR)),
> +        Ok(Some(MatchType::Exclude))
> +    );
> +    assert_eq!(
> +        matchlist.matches("bhshdf", || Err(FileModeRequired {})),
> +        Err(FileModeRequired {})
> +    );

^ This case is a duplicate.

> +    assert_eq!(
> +        matchlist.matches("bbb", || Ok::<u32, FileModeRequired>(libc::S_IFDIR)),
> +        Ok(Some(MatchType::Include))
> +    );
> +
> +    let matchlist = vec![
> +        MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
> +            .flags(MatchFlag::MATCH_DIRECTORIES),
> +    ];
> +
> +    assert_eq!(
> +        matchlist.matches("bbb", || Ok::<u32, FileModeRequired>(libc::S_IFDIR)),
> +        Ok(None)
> +    );
> +
> +    let matchlist = vec![
> +        MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
> +            .flags(MatchFlag::MATCH_DIRECTORIES),
> +        MatchEntry::new(Pattern::path("b*").unwrap(), MatchType::Exclude)
> +            .flags(MatchFlag::MATCH_REGULAR_FILES),
> +    ];
> +
> +    assert_eq!(
> +        matchlist.matches("ahsjdj", || Ok::<u32, FileModeRequired>(libc::S_IFDIR)),
> +        Ok(Some(MatchType::Exclude))
> +    );
> +    assert_eq!(
> +        matchlist.matches("ahsjdj", || Ok::<u32, FileModeRequired>(libc::S_IFREG)),
> +        Ok(None)
> +    );
> +    assert_eq!(
> +        matchlist.matches("bhsjdj", || Ok::<u32, FileModeRequired>(libc::S_IFREG)),
> +        Ok(Some(MatchType::Exclude))
> +    );
> +}
> +
> +#[test]
> +fn match_entry_needs_flag() {
> +    use crate::Pattern;
> +    let match_entry = MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
> +        .flags(MatchFlag::MATCH_DIRECTORIES);

could replace the `.flags` call here with using `a*/` as a pattern
(trailing slash).

> +    assert_eq!(match_entry.needs_file_mode(), true);
> +
> +    let match_entry = MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
> +        .flags(MatchFlag::MATCH_REGULAR_FILES);
> +    assert_eq!(match_entry.needs_file_mode(), true);
> +
> +    let match_entry = MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
> +        .flags(MatchFlag::MATCH_DIRECTORIES | MatchFlag::MATCH_REGULAR_FILES);
> +    assert_eq!(match_entry.needs_file_mode(), true);
> +
> +    let match_entry = MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
> +        .flags(MatchFlag::ANCHORED | MatchFlag::MATCH_DIRECTORIES);
> +    assert_eq!(match_entry.needs_file_mode(), true);
> +
> +    let match_entry = MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
> +        .flags(MatchFlag::ANY_FILE_TYPE);
> +    assert_eq!(match_entry.needs_file_mode(), false);
>  }
> -- 
> 2.39.2