[pbs-devel] [PATCH proxmox-backup] fix #3613: catalog_shell: include matched dir's contents on restore

[pbs-devel] [PATCH proxmox-backup] fix #3613: catalog_shell: include matched dir's contents on restore

[Dylan Whyte]

Prior to this, during an interactive restore, if a directory was matched
via a pattern match or selection, only the empty directory would be
restored, and not its contents. Now the entire contents is restored on
directory match

Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
---
 pbs-client/src/catalog_shell.rs | 56 ++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/pbs-client/src/catalog_shell.rs b/pbs-client/src/catalog_shell.rs
index c9c9da67..d0658def 100644
--- a/pbs-client/src/catalog_shell.rs
+++ b/pbs-client/src/catalog_shell.rs
@@ -997,6 +997,9 @@ impl Shell {
             &self.accessor,
         )?;
 
+        // Clear match list following restore
+        self.selected.clear();
+
         extractor.extract().await
     }
 }
@@ -1007,9 +1010,7 @@ struct ExtractorState<'a> {
     path_len_stack: Vec<usize>,
 
     dir_stack: Vec<PathStackEntry>,
-
-    matches: bool,
-    matches_stack: Vec<bool>,
+    match_pos: Option<usize>,
 
     read_dir: <Vec<catalog::DirEntry> as IntoIterator>::IntoIter,
     read_dir_stack: Vec<<Vec<catalog::DirEntry> as IntoIterator>::IntoIter>,
@@ -1029,6 +1030,7 @@ impl<'a> ExtractorState<'a> {
         match_list: &'a [MatchEntry],
         accessor: &'a Accessor,
     ) -> Result<Self, Error> {
+        let match_pos = None;
         let read_dir = catalog
             .read_dir(&dir_stack.last().unwrap().catalog)?
             .into_iter();
@@ -1038,9 +1040,7 @@ impl<'a> ExtractorState<'a> {
             path_len_stack: Vec::new(),
 
             dir_stack,
-
-            matches: match_list.is_empty(),
-            matches_stack: Vec::new(),
+            match_pos,
 
             read_dir,
             read_dir_stack: Vec::new(),
@@ -1084,11 +1084,6 @@ impl<'a> ExtractorState<'a> {
             None => return Ok(ControlFlow::Break(())), // out of root directory
         };
 
-        self.matches = self
-            .matches_stack
-            .pop()
-            .ok_or_else(|| format_err!("internal iterator error (matches_stack)"))?;
-
         self.dir_stack
             .pop()
             .ok_or_else(|| format_err!("internal iterator error (dir_stack)"))?;
@@ -1106,14 +1101,14 @@ impl<'a> ExtractorState<'a> {
     async fn handle_new_directory(
         &mut self,
         entry: catalog::DirEntry,
-        match_result: Option<MatchType>,
+        create_dir: bool,
     ) -> Result<(), Error> {
         // enter a new directory:
         self.read_dir_stack.push(mem::replace(
             &mut self.read_dir,
             self.catalog.read_dir(&entry)?.into_iter(),
         ));
-        self.matches_stack.push(self.matches);
+
         self.dir_stack.push(PathStackEntry::new(entry));
         self.path_len_stack.push(self.path_len);
         self.path_len = self.path.len();
@@ -1121,23 +1116,46 @@ impl<'a> ExtractorState<'a> {
         Shell::walk_pxar_archive(self.accessor, &mut self.dir_stack).await?;
         let dir_pxar = self.dir_stack.last().unwrap().pxar.as_ref().unwrap();
         let dir_meta = dir_pxar.entry().metadata().clone();
-        let create = self.matches && match_result != Some(MatchType::Exclude);
-        self.extractor.enter_directory(dir_pxar.file_name().to_os_string(), dir_meta, create)?;
+        self.extractor.enter_directory(dir_pxar.file_name().to_os_string(), dir_meta, create_dir)?;
 
         Ok(())
     }
 
     pub async fn handle_entry(&mut self, entry: catalog::DirEntry) -> Result<(), Error> {
         let match_result = self.match_list.matches(&self.path, entry.get_file_mode());
+
+        let current_pos = self.dir_stack.len();
+
         let did_match = match match_result {
-            Some(MatchType::Include) => true,
-            Some(MatchType::Exclude) => false,
-            None => self.matches,
+            Some(MatchType::Include) => {
+                // Only update match position for items lower in the directory stack
+                if let Some(match_pos) = self.match_pos {
+                    self.match_pos = Some(usize::min(current_pos, match_pos));
+                } else {
+                    self.match_pos = Some(current_pos)
+                }
+                true
+            },
+            Some(MatchType::Exclude) => return Ok(()), // no need to continue for excluded item
+            None => {
+                // Restore items contained within a matched directory
+                if let Some(match_pos) = self.match_pos {
+                    if current_pos > match_pos {
+                        true
+                    } else {
+                        self.match_pos = None;
+                        false
+                    }
+                } else {
+                    // Restore everything if no matches were specified
+                    self.match_list.is_empty()
+                }
+            }
         };
 
         match (did_match, &entry.attr) {
             (_, DirEntryAttribute::Directory { .. }) => {
-                self.handle_new_directory(entry, match_result).await?;
+                self.handle_new_directory(entry, did_match).await?;
             }
             (true, DirEntryAttribute::File { .. }) => {
                 self.dir_stack.push(PathStackEntry::new(entry));
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup] fix #3613: catalog_shell: include matched dir's contents on restore

[Wolfgang Bumiller]

On Wed, Apr 06, 2022 at 12:09:51PM +0200, Dylan Whyte wrote:
> 
> On 4/6/22 11:30, Wolfgang Bumiller wrote:
> > On Wed, Apr 06, 2022 at 11:15:28AM +0200, Dylan Whyte wrote:
> > > On 4/6/22 10:26, Dietmar Maurer wrote:
> > > > > On 04/04/2022 6:19 PM Dylan Whyte<d.whyte@proxmox.com>  wrote:
> > > > > 
> > > > > Prior to this, during an interactive restore, if a directory was matched
> > > > > via a pattern match or selection, only the empty directory would be
> > > > > restored, and not its contents.
> > > > Why not simply use "**" if you want to restore a whole tree?
> > > I had originally thought about this, but there are some good reasons for the
> > > patch:
> > > 
> > >   * I believe there is an expectation when selecting a directory for
> > >     restore, that you would like for the entire directory to be restored
> > >     (unless any sub-directory is explicitly excluded).
> > >   * The 'select' command doesn't do pattern matching, so it wouldn't be
> > >     able to use '**' to restore the directory. This point doesn't apply
> > >     to 'find' and 'restore --pattern'.
> > Fair points.
> > I don't have particularly hard feelings about this behavior other than
> > that it's a change people who're already used to it might not expect.
> > 
> > >   * With the current implementation, '**' won't restore empty
> > >     sub-directories of a matched directory, in spite of the fact that
> > >     they appear in the match list.
> > That sounds like a bug.
> Just to clarify, would you like me to fix only this specific bug and
> otherwise leave the old behavior in place, so that the trailing '/**' is
> still required to restore a directory's contents?

Unless others object, either is fine with me.

> > 
> > Now, with your patch getting rid of the `matches_stack` to keep track
> > of whether or not we're currently extracting, have you tested nested
> > alternating include-excludes?
> > 
> > include a/
> > exclude a/b
> > include a/b/c
> > 
> > where upon leaving from 'c' to 'b' we need to be back in 'exclude' mode
> > and when leaving from 'b' to 'a' we need to be back in 'include' mode?
> Regarding this, the patch currently just skips any excluded items, so an
> excluded directory is not traversed. I decided that items inside excluded
> directories probably aren't meant to be matched, but if you'd like it to
> behave otherwise, I can rethink it :)

Well, if I explicitly exclude a/b and explicitly include a and a/b/c, I
do want a/b/c to be, well, included ;-)

But I suppose the functionality could still be there in a different way,
much like we need `foo/**` currently, we'd have to change the exclude to
be `a/b/?*` so it doesn't directly match `a/b/` as a directory...
So if that works alternatively, that's fine as well I suppose.
Nested alternating includes/excludes are nasty anyway...

Re: [pbs-devel] [PATCH proxmox-backup] fix #3613: catalog_shell: include matched dir's contents on restore

[Dylan Whyte]

On 4/6/22 11:30, Wolfgang Bumiller wrote:
> On Wed, Apr 06, 2022 at 11:15:28AM +0200, Dylan Whyte wrote:
>> On 4/6/22 10:26, Dietmar Maurer wrote:
>>>> On 04/04/2022 6:19 PM Dylan Whyte<d.whyte@proxmox.com>  wrote:
>>>>
>>>> Prior to this, during an interactive restore, if a directory was matched
>>>> via a pattern match or selection, only the empty directory would be
>>>> restored, and not its contents.
>>> Why not simply use "**" if you want to restore a whole tree?
>> I had originally thought about this, but there are some good reasons for the
>> patch:
>>
>>   * I believe there is an expectation when selecting a directory for
>>     restore, that you would like for the entire directory to be restored
>>     (unless any sub-directory is explicitly excluded).
>>   * The 'select' command doesn't do pattern matching, so it wouldn't be
>>     able to use '**' to restore the directory. This point doesn't apply
>>     to 'find' and 'restore --pattern'.
> Fair points.
> I don't have particularly hard feelings about this behavior other than
> that it's a change people who're already used to it might not expect.
>
>>   * With the current implementation, '**' won't restore empty
>>     sub-directories of a matched directory, in spite of the fact that
>>     they appear in the match list.
> That sounds like a bug.
Just to clarify, would you like me to fix only this specific bug and 
otherwise leave the old behavior in place, so that the trailing '/**' is 
still required to restore a directory's contents?
>
> Now, with your patch getting rid of the `matches_stack` to keep track
> of whether or not we're currently extracting, have you tested nested
> alternating include-excludes?
>
> include a/
> exclude a/b
> include a/b/c
>
> where upon leaving from 'c' to 'b' we need to be back in 'exclude' mode
> and when leaving from 'b' to 'a' we need to be back in 'include' mode?
Regarding this, the patch currently just skips any excluded items, so an 
excluded directory is not traversed. I decided that items inside 
excluded directories probably aren't meant to be matched, but if you'd 
like it to behave otherwise, I can rethink it :)

Re: [pbs-devel] [PATCH proxmox-backup] fix #3613: catalog_shell: include matched dir's contents on restore

[Wolfgang Bumiller]

On Wed, Apr 06, 2022 at 11:15:28AM +0200, Dylan Whyte wrote:
> On 4/6/22 10:26, Dietmar Maurer wrote:
> > > On 04/04/2022 6:19 PM Dylan Whyte<d.whyte@proxmox.com>  wrote:
> > > 
> > > Prior to this, during an interactive restore, if a directory was matched
> > > via a pattern match or selection, only the empty directory would be
> > > restored, and not its contents.
> > Why not simply use "**" if you want to restore a whole tree?
> 
> I had originally thought about this, but there are some good reasons for the
> patch:
> 
>  * I believe there is an expectation when selecting a directory for
>    restore, that you would like for the entire directory to be restored
>    (unless any sub-directory is explicitly excluded).
>  * The 'select' command doesn't do pattern matching, so it wouldn't be
>    able to use '**' to restore the directory. This point doesn't apply
>    to 'find' and 'restore --pattern'.

Fair points.
I don't have particularly hard feelings about this behavior other than
that it's a change people who're already used to it might not expect.

>  * With the current implementation, '**' won't restore empty
>    sub-directories of a matched directory, in spite of the fact that
>    they appear in the match list.

That sounds like a bug.

Now, with your patch getting rid of the `matches_stack` to keep track
of whether or not we're currently extracting, have you tested nested
alternating include-excludes?

include a/
exclude a/b
include a/b/c

where upon leaving from 'c' to 'b' we need to be back in 'exclude' mode
and when leaving from 'b' to 'a' we need to be back in 'include' mode?

Re: [pbs-devel] [PATCH proxmox-backup] fix #3613: catalog_shell: include matched dir's contents on restore

[Dylan Whyte]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

On 4/6/22 10:26, Dietmar Maurer wrote:
>> On 04/04/2022 6:19 PM Dylan Whyte<d.whyte@proxmox.com>  wrote:
>>
>>   
>> Prior to this, during an interactive restore, if a directory was matched
>> via a pattern match or selection, only the empty directory would be
>> restored, and not its contents.
> Why not simply use "**" if you want to restore a whole tree?

I had originally thought about this, but there are some good reasons for 
the patch:

  * I believe there is an expectation when selecting a directory for
    restore, that you would like for the entire directory to be restored
    (unless any sub-directory is explicitly excluded).
  * The 'select' command doesn't do pattern matching, so it wouldn't be
    able to use '**' to restore the directory. This point doesn't apply
    to 'find' and 'restore --pattern'.
  * With the current implementation, '**' won't restore empty
    sub-directories of a matched directory, in spite of the fact that
    they appear in the match list.

[-- Attachment #2: Type: text/html, Size: 1638 bytes --]

Re: [pbs-devel] [PATCH proxmox-backup] fix #3613: catalog_shell: include matched dir's contents on restore

[Dietmar Maurer]

> On 04/04/2022 6:19 PM Dylan Whyte <d.whyte@proxmox.com> wrote:
> 
>  
> Prior to this, during an interactive restore, if a directory was matched
> via a pattern match or selection, only the empty directory would be
> restored, and not its contents. 

Why not simply use "**" if you want to restore a whole tree?