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

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

LGTM now, 1 minor nit below since the other patch needs another version
anyway...

On Mon, Aug 21, 2023 at 03:08:25PM +0200, Gabriel Goller wrote:
> Updated `matches()` function, which now takes the `GetFileMode` trait and returns a
> Result. `GetFileMode` is 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. `GetFileMode` is also implemented for `Option<u32>` (so that
> we can simply pass options of the `file_mode`) and `u32` (returning
> `std::convert::Infallible`, so that we can pass a u32 mode directly).
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> 
> changes v3:
>  - changed `matches()` and `matches_exact()` to retrieve the file_mode lazily.
>    Allowed these functions to take in a trait `GetFileMode`, so that we can pass
>    closures and options that contain the `file_mode`.
> 
> 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 | 335 ++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 315 insertions(+), 92 deletions(-)
> 
> diff --git a/src/match_list.rs b/src/match_list.rs
> index c5b14e0..69b1fdb 100644
> --- a/src/match_list.rs
> +++ b/src/match_list.rs
(...)
> @@ -383,30 +438,48 @@ where
>      <&'a T as IntoIterator>::IntoIter: DoubleEndedIterator,
>      <&'a T as IntoIterator>::Item: MatchListEntry,
>  {

Since we have a T here already...

> -    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::Error>

maybe use `P` for the `path` ;-)

> +    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
> +        Ok(None)
>      }
>  
> -    fn matches_exact_do(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType> {
> +    fn matches_exact<U, G>(&self, path: U, get_file_mode: G) -> Result<Option<MatchType>, G::Error>

^ here as well

> +    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)
>      }
>  }
>