From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id E3137D70F for ; Mon, 21 Aug 2023 09:57:46 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C5B11144E6 for ; Mon, 21 Aug 2023 09:57:46 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 21 Aug 2023 09:57:44 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id A5E9F429E6 for ; Mon, 21 Aug 2023 09:57:44 +0200 (CEST) Date: Mon, 21 Aug 2023 09:57:41 +0200 From: Wolfgang Bumiller To: Gabriel Goller Cc: pbs-devel@lists.proxmox.com Message-ID: References: <20230818143212.91113-1-g.goller@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230818143212.91113-1-g.goller@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.104 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH pathpatterns v3] match_list: updated `matches()`, to only retrieve file mode if necessary X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Aug 2023 07:57:47 -0000 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 > --- > > 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; > +} > + > +impl GetFileMode for T > +where > + T: FnOnce() -> Result, > +{ > + type Error = E; > + fn get(self) -> Result { > + self() > + } > +} > + > +impl GetFileMode for Option { > + type Error = FileModeRequired; > + fn get(self) -> Result { > + 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) -> Option; > fn entry_matches_exact(&self, path: &[u8], file_mode: Option) -> Option; > + 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>(&self, path: T, file_mode: Option) -> Option { > - self.matches_do(path.as_ref(), file_mode) > - } > - > - fn matches_do(&self, path: &[u8], file_mode: Option) -> Option; > - > - /// Check whether this list contains anything exactly matching the path and mode. > - fn matches_exact>(&self, path: T, file_mode: Option) -> Option { > - self.matches_exact_do(path.as_ref(), file_mode) > - } > - > - fn matches_exact_do(&self, path: &[u8], file_mode: Option) -> Option; > + /// specified file mode. Gets the file_mode lazily, only if needed. > + fn matches( > + &self, > + path: T, > + get_file_mode: U, > + ) -> Result, ::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( > + &self, > + path: T, > + get_file_mode: U, > + ) -> Result, ::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) -> Option { > + fn matches( > + &self, > + path: U, > + get_file_mode: G, > + ) -> Result, ::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) -> Option { > + Ok(None) > + } > + > + fn matches_exact( > + &self, > + path: U, > + get_file_mode: G, > + ) -> Result, ::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::::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::(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::(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::(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::(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::(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::(libc::S_IFDIR)), > + Ok(Some(MatchType::Exclude)) > + ); > + assert_eq!( > + matchlist.matches("ahsjdj", || Ok::(libc::S_IFREG)), > + Ok(None) > + ); > + assert_eq!( > + matchlist.matches("bhsjdj", || Ok::(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