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

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

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...