From 1369658a32a62053530c82df2deae3f733bcf7ce Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 29 Jun 2024 17:15:29 +0200 Subject: [PATCH 01/12] archiver: extract Readdirnames to fs package --- internal/archiver/archiver.go | 25 ++----------------------- internal/archiver/scanner.go | 2 +- internal/archiver/tree.go | 2 +- internal/fs/file.go | 22 ++++++++++++++++++++++ 4 files changed, 26 insertions(+), 25 deletions(-) diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index 9a31911b9..19ad12ab8 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -304,7 +304,7 @@ func (arch *Archiver) saveDir(ctx context.Context, snPath string, dir string, fi return FutureNode{}, err } - names, err := readdirnames(arch.FS, dir, fs.O_NOFOLLOW) + names, err := fs.Readdirnames(arch.FS, dir, fs.O_NOFOLLOW) if err != nil { return FutureNode{}, err } @@ -707,27 +707,6 @@ func (arch *Archiver) saveTree(ctx context.Context, snPath string, atree *Tree, return fn, len(nodes), nil } -// flags are passed to fs.OpenFile. O_RDONLY is implied. -func readdirnames(filesystem fs.FS, dir string, flags int) ([]string, error) { - f, err := filesystem.OpenFile(dir, fs.O_RDONLY|flags, 0) - if err != nil { - return nil, errors.WithStack(err) - } - - entries, err := f.Readdirnames(-1) - if err != nil { - _ = f.Close() - return nil, errors.Wrapf(err, "Readdirnames %v failed", dir) - } - - err = f.Close() - if err != nil { - return nil, err - } - - return entries, nil -} - // resolveRelativeTargets replaces targets that only contain relative // directories ("." or "../../") with the contents of the directory. Each // element of target is processed with fs.Clean(). @@ -743,7 +722,7 @@ func resolveRelativeTargets(filesys fs.FS, targets []string) ([]string, error) { } debug.Log("replacing %q with readdir(%q)", target, target) - entries, err := readdirnames(filesys, target, fs.O_NOFOLLOW) + entries, err := fs.Readdirnames(filesys, target, fs.O_NOFOLLOW) if err != nil { return nil, err } diff --git a/internal/archiver/scanner.go b/internal/archiver/scanner.go index cc419b19e..d61e5ce47 100644 --- a/internal/archiver/scanner.go +++ b/internal/archiver/scanner.go @@ -124,7 +124,7 @@ func (s *Scanner) scan(ctx context.Context, stats ScanStats, target string) (Sca stats.Files++ stats.Bytes += uint64(fi.Size()) case fi.Mode().IsDir(): - names, err := readdirnames(s.FS, target, fs.O_NOFOLLOW) + names, err := fs.Readdirnames(s.FS, target, fs.O_NOFOLLOW) if err != nil { return stats, s.Error(target, err) } diff --git a/internal/archiver/tree.go b/internal/archiver/tree.go index 16a78ee70..cd03ba521 100644 --- a/internal/archiver/tree.go +++ b/internal/archiver/tree.go @@ -233,7 +233,7 @@ func unrollTree(f fs.FS, t *Tree) error { // nodes, add the contents of Path to the nodes. if t.Path != "" && len(t.Nodes) > 0 { debug.Log("resolve path %v", t.Path) - entries, err := readdirnames(f, t.Path, 0) + entries, err := fs.Readdirnames(f, t.Path, 0) if err != nil { return err } diff --git a/internal/fs/file.go b/internal/fs/file.go index 4a236ea09..929195f1c 100644 --- a/internal/fs/file.go +++ b/internal/fs/file.go @@ -1,6 +1,7 @@ package fs import ( + "fmt" "os" "path/filepath" "time" @@ -138,3 +139,24 @@ func ResetPermissions(path string) error { } return nil } + +// Readdirnames returns a list of file in a directory. Flags are passed to fs.OpenFile. O_RDONLY is implied. +func Readdirnames(filesystem FS, dir string, flags int) ([]string, error) { + f, err := filesystem.OpenFile(dir, O_RDONLY|flags, 0) + if err != nil { + return nil, fmt.Errorf("openfile for readdirnames failed: %w", err) + } + + entries, err := f.Readdirnames(-1) + if err != nil { + _ = f.Close() + return nil, fmt.Errorf("readdirnames %v failed: %w", dir, err) + } + + err = f.Close() + if err != nil { + return nil, err + } + + return entries, nil +} From a9a60f77ced8c8ecbca2d09795c7574c71715b13 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 29 Jun 2024 17:24:47 +0200 Subject: [PATCH 02/12] restore: optimize memory usage --- internal/restorer/restorer.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 650ad0731..3817414eb 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -120,7 +120,9 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, return hasRestored, res.Error(location, err) } - for _, node := range tree.Nodes { + for i, node := range tree.Nodes { + // allow GC of tree node + tree.Nodes[i] = nil // ensure that the node name does not contain anything that refers to a // top-level directory. From d762f4ee64ff22889b97f361aa5010c11f9ec87e Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 29 Jun 2024 17:53:47 +0200 Subject: [PATCH 03/12] restore: simplfy selectFilter arguments --- cmd/restic/cmd_restore.go | 8 ++++---- internal/restorer/restorer.go | 6 +++--- internal/restorer/restorer_test.go | 22 +++++++++++----------- internal/restorer/restorer_unix_test.go | 8 -------- 4 files changed, 18 insertions(+), 26 deletions(-) diff --git a/cmd/restic/cmd_restore.go b/cmd/restic/cmd_restore.go index aea6457bd..d10558c6a 100644 --- a/cmd/restic/cmd_restore.go +++ b/cmd/restic/cmd_restore.go @@ -161,7 +161,7 @@ func runRestore(ctx context.Context, opts RestoreOptions, gopts GlobalOptions, msg.E("Warning: %s\n", message) } - selectExcludeFilter := func(item string, _ string, node *restic.Node) (selectedForRestore bool, childMayBeSelected bool) { + selectExcludeFilter := func(item string, _ string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { matched := false for _, rejectFn := range excludePatternFns { matched = matched || rejectFn(item) @@ -178,12 +178,12 @@ func runRestore(ctx context.Context, opts RestoreOptions, gopts GlobalOptions, // therefore childMayMatch does not matter, but we should not go down // unless the dir is selected for restore selectedForRestore = !matched - childMayBeSelected = selectedForRestore && node.Type == "dir" + childMayBeSelected = selectedForRestore && isDir return selectedForRestore, childMayBeSelected } - selectIncludeFilter := func(item string, _ string, node *restic.Node) (selectedForRestore bool, childMayBeSelected bool) { + selectIncludeFilter := func(item string, _ string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { selectedForRestore = false childMayBeSelected = false for _, includeFn := range includePatternFns { @@ -195,7 +195,7 @@ func runRestore(ctx context.Context, opts RestoreOptions, gopts GlobalOptions, break } } - childMayBeSelected = childMayBeSelected && node.Type == "dir" + childMayBeSelected = childMayBeSelected && isDir return selectedForRestore, childMayBeSelected } diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 3817414eb..d13c3462c 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -27,7 +27,7 @@ type Restorer struct { Error func(location string, err error) error Warn func(message string) - SelectFilter func(item string, dstpath string, node *restic.Node) (selectedForRestore bool, childMayBeSelected bool) + SelectFilter func(item string, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) } var restorerAbortOnAllErrors = func(_ string, err error) error { return err } @@ -97,7 +97,7 @@ func NewRestorer(repo restic.Repository, sn *restic.Snapshot, opts Options) *Res opts: opts, fileList: make(map[string]bool), Error: restorerAbortOnAllErrors, - SelectFilter: func(string, string, *restic.Node) (bool, bool) { return true, true }, + SelectFilter: func(string, string, bool) (bool, bool) { return true, true }, sn: sn, } @@ -154,7 +154,7 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, continue } - selectedForRestore, childMayBeSelected := res.SelectFilter(nodeLocation, nodeTarget, node) + selectedForRestore, childMayBeSelected := res.SelectFilter(nodeLocation, nodeTarget, node.Type == "dir") debug.Log("SelectFilter returned %v %v for %q", selectedForRestore, childMayBeSelected, nodeLocation) if selectedForRestore { diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 5eca779c6..720b91368 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -192,7 +192,7 @@ func TestRestorer(t *testing.T) { Files map[string]string ErrorsMust map[string]map[string]struct{} ErrorsMay map[string]map[string]struct{} - Select func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) + Select func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) }{ // valid test cases { @@ -284,7 +284,7 @@ func TestRestorer(t *testing.T) { Files: map[string]string{ "dir/file": "content: file\n", }, - Select: func(item, dstpath string, node *restic.Node) (selectedForRestore bool, childMayBeSelected bool) { + Select: func(item, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { switch item { case filepath.FromSlash("/dir"): childMayBeSelected = true @@ -370,7 +370,7 @@ func TestRestorer(t *testing.T) { // make sure we're creating a new subdir of the tempdir tempdir = filepath.Join(tempdir, "target") - res.SelectFilter = func(item, dstpath string, node *restic.Node) (selectedForRestore bool, childMayBeSelected bool) { + res.SelectFilter = func(item, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { t.Logf("restore %v to %v", item, dstpath) if !fs.HasPathPrefix(tempdir, dstpath) { t.Errorf("would restore %v to %v, which is not within the target dir %v", @@ -379,7 +379,7 @@ func TestRestorer(t *testing.T) { } if test.Select != nil { - return test.Select(item, dstpath, node) + return test.Select(item, dstpath, isDir) } return true, true @@ -570,7 +570,7 @@ func checkVisitOrder(list []TreeVisit) TraverseTreeCheck { func TestRestorerTraverseTree(t *testing.T) { var tests = []struct { Snapshot - Select func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) + Select func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) Visitor TraverseTreeCheck }{ { @@ -586,7 +586,7 @@ func TestRestorerTraverseTree(t *testing.T) { "foo": File{Data: "content: foo\n"}, }, }, - Select: func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) { + Select: func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { return true, true }, Visitor: checkVisitOrder([]TreeVisit{ @@ -613,7 +613,7 @@ func TestRestorerTraverseTree(t *testing.T) { "foo": File{Data: "content: foo\n"}, }, }, - Select: func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) { + Select: func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { if item == "/foo" { return true, false } @@ -635,7 +635,7 @@ func TestRestorerTraverseTree(t *testing.T) { }}, }, }, - Select: func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) { + Select: func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { if item == "/aaa" { return true, false } @@ -659,7 +659,7 @@ func TestRestorerTraverseTree(t *testing.T) { "foo": File{Data: "content: foo\n"}, }, }, - Select: func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) { + Select: func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { if strings.HasPrefix(item, "/dir") { return true, true } @@ -688,7 +688,7 @@ func TestRestorerTraverseTree(t *testing.T) { "foo": File{Data: "content: foo\n"}, }, }, - Select: func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) { + Select: func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { switch item { case "/dir": return false, true @@ -788,7 +788,7 @@ func TestRestorerConsistentTimestampsAndPermissions(t *testing.T) { res := NewRestorer(repo, sn, Options{}) - res.SelectFilter = func(item string, dstpath string, node *restic.Node) (selectedForRestore bool, childMayBeSelected bool) { + res.SelectFilter = func(item string, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { switch filepath.ToSlash(item) { case "/dir": childMayBeSelected = true diff --git a/internal/restorer/restorer_unix_test.go b/internal/restorer/restorer_unix_test.go index 2ad28a0f6..febd43ace 100644 --- a/internal/restorer/restorer_unix_test.go +++ b/internal/restorer/restorer_unix_test.go @@ -13,7 +13,6 @@ import ( "time" "github.com/restic/restic/internal/repository" - "github.com/restic/restic/internal/restic" rtest "github.com/restic/restic/internal/test" restoreui "github.com/restic/restic/internal/ui/restore" ) @@ -34,10 +33,6 @@ func TestRestorerRestoreEmptyHardlinkedFields(t *testing.T) { res := NewRestorer(repo, sn, Options{}) - res.SelectFilter = func(item string, dstpath string, node *restic.Node) (selectedForRestore bool, childMayBeSelected bool) { - return true, true - } - tempdir := rtest.TempDir(t) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -108,9 +103,6 @@ func testRestorerProgressBar(t *testing.T, dryRun bool) { mock := &printerMock{} progress := restoreui.NewProgress(mock, 0) res := NewRestorer(repo, sn, Options{Progress: progress, DryRun: dryRun}) - res.SelectFilter = func(item string, dstpath string, node *restic.Node) (selectedForRestore bool, childMayBeSelected bool) { - return true, true - } tempdir := rtest.TempDir(t) ctx, cancel := context.WithCancel(context.Background()) From 144e2a451fb19753ec37f2543cc5ffb1b929a591 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 29 Jun 2024 18:58:17 +0200 Subject: [PATCH 04/12] restore: track expected filenames in a folder --- internal/restorer/restorer.go | 87 +++++++++++++++++++++++------- internal/restorer/restorer_test.go | 73 ++++++++++++++++--------- 2 files changed, 117 insertions(+), 43 deletions(-) diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index d13c3462c..62c486f02 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -37,6 +37,7 @@ type Options struct { Sparse bool Progress *restoreui.Progress Overwrite OverwriteBehavior + Delete bool } type OverwriteBehavior int @@ -107,22 +108,61 @@ func NewRestorer(repo restic.Repository, sn *restic.Snapshot, opts Options) *Res type treeVisitor struct { enterDir func(node *restic.Node, target, location string) error visitNode func(node *restic.Node, target, location string) error - leaveDir func(node *restic.Node, target, location string) error + // 'entries' contains all files the snapshot contains for this node. This also includes files + // ignored by the SelectFilter. + leaveDir func(node *restic.Node, target, location string, entries []string) error } // traverseTree traverses a tree from the repo and calls treeVisitor. // target is the path in the file system, location within the snapshot. -func (res *Restorer) traverseTree(ctx context.Context, target, location string, treeID restic.ID, visitor treeVisitor) (hasRestored bool, err error) { +func (res *Restorer) traverseTree(ctx context.Context, target string, treeID restic.ID, visitor treeVisitor) error { + location := string(filepath.Separator) + sanitizeError := func(err error) error { + switch err { + case nil, context.Canceled, context.DeadlineExceeded: + // Context errors are permanent. + return err + default: + return res.Error(location, err) + } + } + + if visitor.enterDir != nil { + err := sanitizeError(visitor.enterDir(nil, target, location)) + if err != nil { + return err + } + } + childFilenames, hasRestored, err := res.traverseTreeInner(ctx, target, location, treeID, visitor) + if err != nil { + return err + } + if hasRestored && visitor.leaveDir != nil { + err = sanitizeError(visitor.leaveDir(nil, target, location, childFilenames)) + } + + return err +} + +func (res *Restorer) traverseTreeInner(ctx context.Context, target, location string, treeID restic.ID, visitor treeVisitor) (filenames []string, hasRestored bool, err error) { debug.Log("%v %v %v", target, location, treeID) tree, err := restic.LoadTree(ctx, res.repo, treeID) if err != nil { debug.Log("error loading tree %v: %v", treeID, err) - return hasRestored, res.Error(location, err) + return nil, hasRestored, res.Error(location, err) } + if res.opts.Delete { + filenames = make([]string, 0, len(tree.Nodes)) + } for i, node := range tree.Nodes { // allow GC of tree node tree.Nodes[i] = nil + if res.opts.Delete { + // just track all files included in the tree node to simplify the control flow. + // tracking too many files does not matter except for a slightly elevated memory usage + filenames = append(filenames, node.Name) + } // ensure that the node name does not contain anything that refers to a // top-level directory. @@ -131,8 +171,10 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, debug.Log("node %q has invalid name %q", node.Name, nodeName) err := res.Error(location, errors.Errorf("invalid child node name %s", node.Name)) if err != nil { - return hasRestored, err + return nil, hasRestored, err } + // force disable deletion to prevent unexpected behavior + res.opts.Delete = false continue } @@ -144,8 +186,10 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, debug.Log("node %q has invalid target path %q", node.Name, nodeTarget) err := res.Error(nodeLocation, errors.New("node has invalid path")) if err != nil { - return hasRestored, err + return nil, hasRestored, err } + // force disable deletion to prevent unexpected behavior + res.opts.Delete = false continue } @@ -173,25 +217,26 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, if node.Type == "dir" { if node.Subtree == nil { - return hasRestored, errors.Errorf("Dir without subtree in tree %v", treeID.Str()) + return nil, hasRestored, errors.Errorf("Dir without subtree in tree %v", treeID.Str()) } if selectedForRestore && visitor.enterDir != nil { err = sanitizeError(visitor.enterDir(node, nodeTarget, nodeLocation)) if err != nil { - return hasRestored, err + return nil, hasRestored, err } } // keep track of restored child status // so metadata of the current directory are restored on leaveDir childHasRestored := false + var childFilenames []string if childMayBeSelected { - childHasRestored, err = res.traverseTree(ctx, nodeTarget, nodeLocation, *node.Subtree, visitor) + childFilenames, childHasRestored, err = res.traverseTreeInner(ctx, nodeTarget, nodeLocation, *node.Subtree, visitor) err = sanitizeError(err) if err != nil { - return hasRestored, err + return nil, hasRestored, err } // inform the parent directory to restore parent metadata on leaveDir if needed if childHasRestored { @@ -202,9 +247,9 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, // metadata need to be restore when leaving the directory in both cases // selected for restore or any child of any subtree have been restored if (selectedForRestore || childHasRestored) && visitor.leaveDir != nil { - err = sanitizeError(visitor.leaveDir(node, nodeTarget, nodeLocation)) + err = sanitizeError(visitor.leaveDir(node, nodeTarget, nodeLocation, childFilenames)) if err != nil { - return hasRestored, err + return nil, hasRestored, err } } @@ -214,12 +259,12 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, if selectedForRestore { err = sanitizeError(visitor.visitNode(node, nodeTarget, nodeLocation)) if err != nil { - return hasRestored, err + return nil, hasRestored, err } } } - return hasRestored, nil + return filenames, hasRestored, nil } func (res *Restorer) restoreNodeTo(ctx context.Context, node *restic.Node, target, location string) error { @@ -310,10 +355,12 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { var buf []byte // first tree pass: create directories and collect all files to restore - _, err = res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{ + err = res.traverseTree(ctx, dst, *res.sn.Tree, treeVisitor{ enterDir: func(_ *restic.Node, target, location string) error { debug.Log("first pass, enterDir: mkdir %q, leaveDir should restore metadata", location) - res.opts.Progress.AddFile(0) + if location != "/" { + res.opts.Progress.AddFile(0) + } return res.ensureDir(target) }, @@ -373,7 +420,7 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { debug.Log("second pass for %q", dst) // second tree pass: restore special files and filesystem metadata - _, err = res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{ + err = res.traverseTree(ctx, dst, *res.sn.Tree, treeVisitor{ visitNode: func(node *restic.Node, target, location string) error { debug.Log("second pass, visitNode: restore node %q", location) if node.Type != "file" { @@ -396,7 +443,11 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { // don't touch skipped files return nil }, - leaveDir: func(node *restic.Node, target, location string) error { + leaveDir: func(node *restic.Node, target, location string, expectedFilenames []string) error { + if node == nil { + return nil + } + err := res.restoreNodeMetadataTo(node, target, location) if err == nil { res.opts.Progress.AddProgress(location, restoreui.ActionDirRestored, 0, 0) @@ -493,7 +544,7 @@ func (res *Restorer) VerifyFiles(ctx context.Context, dst string) (int, error) { g.Go(func() error { defer close(work) - _, err := res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{ + err := res.traverseTree(ctx, dst, *res.sn.Tree, treeVisitor{ visitNode: func(node *restic.Node, target, location string) error { if node.Type != "file" { return nil diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 720b91368..d483872e0 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -8,6 +8,7 @@ import ( "math" "os" "path/filepath" + "reflect" "runtime" "strings" "syscall" @@ -527,16 +528,17 @@ func TestRestorerRelative(t *testing.T) { type TraverseTreeCheck func(testing.TB) treeVisitor type TreeVisit struct { - funcName string // name of the function - location string // location passed to the function + funcName string // name of the function + location string // location passed to the function + files []string // file list passed to the function } func checkVisitOrder(list []TreeVisit) TraverseTreeCheck { var pos int return func(t testing.TB) treeVisitor { - check := func(funcName string) func(*restic.Node, string, string) error { - return func(node *restic.Node, target, location string) error { + check := func(funcName string) func(*restic.Node, string, string, []string) error { + return func(node *restic.Node, target, location string, expectedFilenames []string) error { if pos >= len(list) { t.Errorf("step %v, %v(%v): expected no more than %d function calls", pos, funcName, location, len(list)) pos++ @@ -554,14 +556,24 @@ func checkVisitOrder(list []TreeVisit) TraverseTreeCheck { t.Errorf("step %v: want location %v, got %v", pos, list[pos].location, location) } + if !reflect.DeepEqual(expectedFilenames, v.files) { + t.Errorf("step %v: want files %v, got %v", pos, list[pos].files, expectedFilenames) + } + pos++ return nil } } + checkNoFilename := func(funcName string) func(*restic.Node, string, string) error { + f := check(funcName) + return func(node *restic.Node, target, location string) error { + return f(node, target, location, nil) + } + } return treeVisitor{ - enterDir: check("enterDir"), - visitNode: check("visitNode"), + enterDir: checkNoFilename("enterDir"), + visitNode: checkNoFilename("visitNode"), leaveDir: check("leaveDir"), } } @@ -590,13 +602,15 @@ func TestRestorerTraverseTree(t *testing.T) { return true, true }, Visitor: checkVisitOrder([]TreeVisit{ - {"enterDir", "/dir"}, - {"visitNode", "/dir/otherfile"}, - {"enterDir", "/dir/subdir"}, - {"visitNode", "/dir/subdir/file"}, - {"leaveDir", "/dir/subdir"}, - {"leaveDir", "/dir"}, - {"visitNode", "/foo"}, + {"enterDir", "/", nil}, + {"enterDir", "/dir", nil}, + {"visitNode", "/dir/otherfile", nil}, + {"enterDir", "/dir/subdir", nil}, + {"visitNode", "/dir/subdir/file", nil}, + {"leaveDir", "/dir/subdir", []string{"file"}}, + {"leaveDir", "/dir", []string{"otherfile", "subdir"}}, + {"visitNode", "/foo", nil}, + {"leaveDir", "/", []string{"dir", "foo"}}, }), }, @@ -620,7 +634,9 @@ func TestRestorerTraverseTree(t *testing.T) { return false, false }, Visitor: checkVisitOrder([]TreeVisit{ - {"visitNode", "/foo"}, + {"enterDir", "/", nil}, + {"visitNode", "/foo", nil}, + {"leaveDir", "/", []string{"dir", "foo"}}, }), }, { @@ -642,7 +658,9 @@ func TestRestorerTraverseTree(t *testing.T) { return false, false }, Visitor: checkVisitOrder([]TreeVisit{ - {"visitNode", "/aaa"}, + {"enterDir", "/", nil}, + {"visitNode", "/aaa", nil}, + {"leaveDir", "/", []string{"aaa", "dir"}}, }), }, @@ -666,12 +684,14 @@ func TestRestorerTraverseTree(t *testing.T) { return false, false }, Visitor: checkVisitOrder([]TreeVisit{ - {"enterDir", "/dir"}, - {"visitNode", "/dir/otherfile"}, - {"enterDir", "/dir/subdir"}, - {"visitNode", "/dir/subdir/file"}, - {"leaveDir", "/dir/subdir"}, - {"leaveDir", "/dir"}, + {"enterDir", "/", nil}, + {"enterDir", "/dir", nil}, + {"visitNode", "/dir/otherfile", nil}, + {"enterDir", "/dir/subdir", nil}, + {"visitNode", "/dir/subdir/file", nil}, + {"leaveDir", "/dir/subdir", []string{"file"}}, + {"leaveDir", "/dir", []string{"otherfile", "subdir"}}, + {"leaveDir", "/", []string{"dir", "foo"}}, }), }, @@ -699,8 +719,10 @@ func TestRestorerTraverseTree(t *testing.T) { } }, Visitor: checkVisitOrder([]TreeVisit{ - {"visitNode", "/dir/otherfile"}, - {"leaveDir", "/dir"}, + {"enterDir", "/", nil}, + {"visitNode", "/dir/otherfile", nil}, + {"leaveDir", "/dir", []string{"otherfile", "subdir"}}, + {"leaveDir", "/", []string{"dir", "foo"}}, }), }, } @@ -710,7 +732,8 @@ func TestRestorerTraverseTree(t *testing.T) { repo := repository.TestRepository(t) sn, _ := saveSnapshot(t, repo, test.Snapshot, noopGetGenericAttributes) - res := NewRestorer(repo, sn, Options{}) + // set Delete option to enable tracking filenames in a directory + res := NewRestorer(repo, sn, Options{Delete: true}) res.SelectFilter = test.Select @@ -721,7 +744,7 @@ func TestRestorerTraverseTree(t *testing.T) { // make sure we're creating a new subdir of the tempdir target := filepath.Join(tempdir, "target") - _, err := res.traverseTree(ctx, target, string(filepath.Separator), *sn.Tree, test.Visitor(t)) + err := res.traverseTree(ctx, target, *sn.Tree, test.Visitor(t)) if err != nil { t.Fatal(err) } From ac44bdf6dd2805c6f5f30f761a7893bc912e57c1 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 29 Jun 2024 19:02:57 +0200 Subject: [PATCH 05/12] restore: add --delete option to remove files that are not in snapshot --- cmd/restic/cmd_restore.go | 3 + internal/restorer/restorer.go | 56 ++++++++++++++- internal/restorer/restorer_test.go | 112 +++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 2 deletions(-) diff --git a/cmd/restic/cmd_restore.go b/cmd/restic/cmd_restore.go index d10558c6a..07290a05b 100644 --- a/cmd/restic/cmd_restore.go +++ b/cmd/restic/cmd_restore.go @@ -51,6 +51,7 @@ type RestoreOptions struct { Sparse bool Verify bool Overwrite restorer.OverwriteBehavior + Delete bool } var restoreOptions RestoreOptions @@ -69,6 +70,7 @@ func init() { flags.BoolVar(&restoreOptions.Sparse, "sparse", false, "restore files as sparse") flags.BoolVar(&restoreOptions.Verify, "verify", false, "verify restored files content") flags.Var(&restoreOptions.Overwrite, "overwrite", "overwrite behavior, one of (always|if-changed|if-newer|never) (default: always)") + flags.BoolVar(&restoreOptions.Delete, "delete", false, "delete files from target directory if they do not exist in snapshot. Use '--dry-run -vv' to check what would be deleted") } func runRestore(ctx context.Context, opts RestoreOptions, gopts GlobalOptions, @@ -149,6 +151,7 @@ func runRestore(ctx context.Context, opts RestoreOptions, gopts GlobalOptions, Sparse: opts.Sparse, Progress: progress, Overwrite: opts.Overwrite, + Delete: opts.Delete, }) totalErrors := 0 diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 62c486f02..5fd06098f 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -25,8 +25,10 @@ type Restorer struct { fileList map[string]bool - Error func(location string, err error) error - Warn func(message string) + Error func(location string, err error) error + Warn func(message string) + // SelectFilter determines whether the item is selectedForRestore or whether a childMayBeSelected. + // selectedForRestore must not depend on isDir as `removeUnexpectedFiles` always passes false to isDir. SelectFilter func(item string, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) } @@ -444,6 +446,12 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { return nil }, leaveDir: func(node *restic.Node, target, location string, expectedFilenames []string) error { + if res.opts.Delete { + if err := res.removeUnexpectedFiles(target, location, expectedFilenames); err != nil { + return err + } + } + if node == nil { return nil } @@ -458,6 +466,50 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { return err } +func (res *Restorer) removeUnexpectedFiles(target, location string, expectedFilenames []string) error { + if !res.opts.Delete { + panic("internal error") + } + + entries, err := fs.Readdirnames(fs.Local{}, target, fs.O_NOFOLLOW) + if errors.Is(err, os.ErrNotExist) { + return nil + } else if err != nil { + return err + } + + keep := map[string]struct{}{} + for _, name := range expectedFilenames { + keep[name] = struct{}{} + } + + for _, entry := range entries { + if _, ok := keep[entry]; ok { + continue + } + + nodeTarget := filepath.Join(target, entry) + nodeLocation := filepath.Join(location, entry) + + if target == nodeTarget || !fs.HasPathPrefix(target, nodeTarget) { + return fmt.Errorf("skipping deletion due to invalid filename: %v", entry) + } + + // TODO pass a proper value to the isDir parameter once this becomes relevant for the filters + selectedForRestore, _ := res.SelectFilter(nodeLocation, nodeTarget, false) + // only delete files that were selected for restore + if selectedForRestore { + if !res.opts.DryRun { + if err := fs.RemoveAll(nodeTarget); err != nil { + return err + } + } + } + } + + return nil +} + func (res *Restorer) trackFile(location string, metadataOnly bool) { res.fileList[location] = metadataOnly } diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index d483872e0..0dc2961fa 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -1219,3 +1219,115 @@ func TestRestoreDryRun(t *testing.T) { _, err := os.Stat(tempdir) rtest.Assert(t, errors.Is(err, os.ErrNotExist), "expected no file to be created, got %v", err) } + +func TestRestoreDelete(t *testing.T) { + repo := repository.TestRepository(t) + tempdir := rtest.TempDir(t) + + sn, _ := saveSnapshot(t, repo, Snapshot{ + Nodes: map[string]Node{ + "dir": Dir{ + Mode: normalizeFileMode(0755 | os.ModeDir), + Nodes: map[string]Node{ + "file1": File{Data: "content: file\n"}, + "anotherfile": File{Data: "content: file\n"}, + }, + }, + "dir2": Dir{ + Mode: normalizeFileMode(0755 | os.ModeDir), + Nodes: map[string]Node{ + "anotherfile": File{Data: "content: file\n"}, + }, + }, + "anotherfile": File{Data: "content: file\n"}, + }, + }, noopGetGenericAttributes) + + // should delete files that no longer exist in the snapshot + deleteSn, _ := saveSnapshot(t, repo, Snapshot{ + Nodes: map[string]Node{ + "dir": Dir{ + Mode: normalizeFileMode(0755 | os.ModeDir), + Nodes: map[string]Node{ + "file1": File{Data: "content: file\n"}, + }, + }, + }, + }, noopGetGenericAttributes) + + tests := []struct { + selectFilter func(item string, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) + fileState map[string]bool + }{ + { + selectFilter: nil, + fileState: map[string]bool{ + "dir": true, + filepath.Join("dir", "anotherfile"): false, + filepath.Join("dir", "file1"): true, + "dir2": false, + filepath.Join("dir2", "anotherfile"): false, + "anotherfile": false, + }, + }, + { + selectFilter: func(item, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { + return false, false + }, + fileState: map[string]bool{ + "dir": true, + filepath.Join("dir", "anotherfile"): true, + filepath.Join("dir", "file1"): true, + "dir2": true, + filepath.Join("dir2", "anotherfile"): true, + "anotherfile": true, + }, + }, + { + selectFilter: func(item, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { + switch item { + case filepath.FromSlash("/dir"): + selectedForRestore = true + case filepath.FromSlash("/dir2"): + selectedForRestore = true + } + return + }, + fileState: map[string]bool{ + "dir": true, + filepath.Join("dir", "anotherfile"): true, + filepath.Join("dir", "file1"): true, + "dir2": false, + filepath.Join("dir2", "anotherfile"): false, + "anotherfile": true, + }, + }, + } + + for _, test := range tests { + t.Run("", func(t *testing.T) { + res := NewRestorer(repo, sn, Options{}) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + err := res.RestoreTo(ctx, tempdir) + rtest.OK(t, err) + + res = NewRestorer(repo, deleteSn, Options{Delete: true}) + if test.selectFilter != nil { + res.SelectFilter = test.selectFilter + } + err = res.RestoreTo(ctx, tempdir) + rtest.OK(t, err) + + for fn, shouldExist := range test.fileState { + _, err := os.Stat(filepath.Join(tempdir, fn)) + if shouldExist { + rtest.OK(t, err) + } else { + rtest.Assert(t, errors.Is(err, os.ErrNotExist), "file %v: unexpected error got %v, expected ErrNotExist", fn, err) + } + } + }) + } +} From 013a6156bd2eba4d557548636b7db6e7cd3aee5d Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 29 Jun 2024 19:23:09 +0200 Subject: [PATCH 06/12] restore: remove unused parameter from SelectFilter --- cmd/restic/cmd_restore.go | 4 ++-- internal/restorer/restorer.go | 8 +++---- internal/restorer/restorer_test.go | 36 +++++++++++++----------------- 3 files changed, 21 insertions(+), 27 deletions(-) diff --git a/cmd/restic/cmd_restore.go b/cmd/restic/cmd_restore.go index 07290a05b..1835219ad 100644 --- a/cmd/restic/cmd_restore.go +++ b/cmd/restic/cmd_restore.go @@ -164,7 +164,7 @@ func runRestore(ctx context.Context, opts RestoreOptions, gopts GlobalOptions, msg.E("Warning: %s\n", message) } - selectExcludeFilter := func(item string, _ string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { + selectExcludeFilter := func(item string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { matched := false for _, rejectFn := range excludePatternFns { matched = matched || rejectFn(item) @@ -186,7 +186,7 @@ func runRestore(ctx context.Context, opts RestoreOptions, gopts GlobalOptions, return selectedForRestore, childMayBeSelected } - selectIncludeFilter := func(item string, _ string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { + selectIncludeFilter := func(item string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { selectedForRestore = false childMayBeSelected = false for _, includeFn := range includePatternFns { diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 5fd06098f..52d34c5ed 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -29,7 +29,7 @@ type Restorer struct { Warn func(message string) // SelectFilter determines whether the item is selectedForRestore or whether a childMayBeSelected. // selectedForRestore must not depend on isDir as `removeUnexpectedFiles` always passes false to isDir. - SelectFilter func(item string, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) + SelectFilter func(item string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) } var restorerAbortOnAllErrors = func(_ string, err error) error { return err } @@ -100,7 +100,7 @@ func NewRestorer(repo restic.Repository, sn *restic.Snapshot, opts Options) *Res opts: opts, fileList: make(map[string]bool), Error: restorerAbortOnAllErrors, - SelectFilter: func(string, string, bool) (bool, bool) { return true, true }, + SelectFilter: func(string, bool) (bool, bool) { return true, true }, sn: sn, } @@ -200,7 +200,7 @@ func (res *Restorer) traverseTreeInner(ctx context.Context, target, location str continue } - selectedForRestore, childMayBeSelected := res.SelectFilter(nodeLocation, nodeTarget, node.Type == "dir") + selectedForRestore, childMayBeSelected := res.SelectFilter(nodeLocation, node.Type == "dir") debug.Log("SelectFilter returned %v %v for %q", selectedForRestore, childMayBeSelected, nodeLocation) if selectedForRestore { @@ -496,7 +496,7 @@ func (res *Restorer) removeUnexpectedFiles(target, location string, expectedFile } // TODO pass a proper value to the isDir parameter once this becomes relevant for the filters - selectedForRestore, _ := res.SelectFilter(nodeLocation, nodeTarget, false) + selectedForRestore, _ := res.SelectFilter(nodeLocation, false) // only delete files that were selected for restore if selectedForRestore { if !res.opts.DryRun { diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 0dc2961fa..8a8f81ce0 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -193,7 +193,7 @@ func TestRestorer(t *testing.T) { Files map[string]string ErrorsMust map[string]map[string]struct{} ErrorsMay map[string]map[string]struct{} - Select func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) + Select func(item string, isDir bool) (selectForRestore bool, childMayBeSelected bool) }{ // valid test cases { @@ -285,7 +285,7 @@ func TestRestorer(t *testing.T) { Files: map[string]string{ "dir/file": "content: file\n", }, - Select: func(item, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { + Select: func(item string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { switch item { case filepath.FromSlash("/dir"): childMayBeSelected = true @@ -371,16 +371,10 @@ func TestRestorer(t *testing.T) { // make sure we're creating a new subdir of the tempdir tempdir = filepath.Join(tempdir, "target") - res.SelectFilter = func(item, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { - t.Logf("restore %v to %v", item, dstpath) - if !fs.HasPathPrefix(tempdir, dstpath) { - t.Errorf("would restore %v to %v, which is not within the target dir %v", - item, dstpath, tempdir) - return false, false - } - + res.SelectFilter = func(item string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { + t.Logf("restore %v", item) if test.Select != nil { - return test.Select(item, dstpath, isDir) + return test.Select(item, isDir) } return true, true @@ -582,7 +576,7 @@ func checkVisitOrder(list []TreeVisit) TraverseTreeCheck { func TestRestorerTraverseTree(t *testing.T) { var tests = []struct { Snapshot - Select func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) + Select func(item string, isDir bool) (selectForRestore bool, childMayBeSelected bool) Visitor TraverseTreeCheck }{ { @@ -598,7 +592,7 @@ func TestRestorerTraverseTree(t *testing.T) { "foo": File{Data: "content: foo\n"}, }, }, - Select: func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { + Select: func(item string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { return true, true }, Visitor: checkVisitOrder([]TreeVisit{ @@ -627,7 +621,7 @@ func TestRestorerTraverseTree(t *testing.T) { "foo": File{Data: "content: foo\n"}, }, }, - Select: func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { + Select: func(item string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { if item == "/foo" { return true, false } @@ -651,7 +645,7 @@ func TestRestorerTraverseTree(t *testing.T) { }}, }, }, - Select: func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { + Select: func(item string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { if item == "/aaa" { return true, false } @@ -677,7 +671,7 @@ func TestRestorerTraverseTree(t *testing.T) { "foo": File{Data: "content: foo\n"}, }, }, - Select: func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { + Select: func(item string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { if strings.HasPrefix(item, "/dir") { return true, true } @@ -708,7 +702,7 @@ func TestRestorerTraverseTree(t *testing.T) { "foo": File{Data: "content: foo\n"}, }, }, - Select: func(item string, dstpath string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { + Select: func(item string, isDir bool) (selectForRestore bool, childMayBeSelected bool) { switch item { case "/dir": return false, true @@ -811,7 +805,7 @@ func TestRestorerConsistentTimestampsAndPermissions(t *testing.T) { res := NewRestorer(repo, sn, Options{}) - res.SelectFilter = func(item string, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { + res.SelectFilter = func(item string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { switch filepath.ToSlash(item) { case "/dir": childMayBeSelected = true @@ -1256,7 +1250,7 @@ func TestRestoreDelete(t *testing.T) { }, noopGetGenericAttributes) tests := []struct { - selectFilter func(item string, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) + selectFilter func(item string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) fileState map[string]bool }{ { @@ -1271,7 +1265,7 @@ func TestRestoreDelete(t *testing.T) { }, }, { - selectFilter: func(item, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { + selectFilter: func(item string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { return false, false }, fileState: map[string]bool{ @@ -1284,7 +1278,7 @@ func TestRestoreDelete(t *testing.T) { }, }, { - selectFilter: func(item, dstpath string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { + selectFilter: func(item string, isDir bool) (selectedForRestore bool, childMayBeSelected bool) { switch item { case filepath.FromSlash("/dir"): selectedForRestore = true From 168fc09d5f25b34d72a25de5aaafb704e97035d4 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 29 Jun 2024 19:54:12 +0200 Subject: [PATCH 07/12] restore: use case insensitive file name comparison on windows --- internal/restorer/restorer.go | 4 +-- internal/restorer/restorer_unix.go | 10 +++++++ internal/restorer/restorer_windows.go | 13 +++++++++ internal/restorer/restorer_windows_test.go | 34 ++++++++++++++++++++++ 4 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 internal/restorer/restorer_unix.go create mode 100644 internal/restorer/restorer_windows.go diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 52d34c5ed..6e81812c2 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -480,11 +480,11 @@ func (res *Restorer) removeUnexpectedFiles(target, location string, expectedFile keep := map[string]struct{}{} for _, name := range expectedFilenames { - keep[name] = struct{}{} + keep[toComparableFilename(name)] = struct{}{} } for _, entry := range entries { - if _, ok := keep[entry]; ok { + if _, ok := keep[toComparableFilename(entry)]; ok { continue } diff --git a/internal/restorer/restorer_unix.go b/internal/restorer/restorer_unix.go new file mode 100644 index 000000000..7316f7b5d --- /dev/null +++ b/internal/restorer/restorer_unix.go @@ -0,0 +1,10 @@ +//go:build !windows +// +build !windows + +package restorer + +// toComparableFilename returns a filename suitable for equality checks. On Windows, it returns the +// uppercase version of the string. On all other systems, it returns the unmodified filename. +func toComparableFilename(path string) string { + return path +} diff --git a/internal/restorer/restorer_windows.go b/internal/restorer/restorer_windows.go new file mode 100644 index 000000000..72337d8ae --- /dev/null +++ b/internal/restorer/restorer_windows.go @@ -0,0 +1,13 @@ +//go:build windows +// +build windows + +package restorer + +import "strings" + +// toComparableFilename returns a filename suitable for equality checks. On Windows, it returns the +// uppercase version of the string. On all other systems, it returns the unmodified filename. +func toComparableFilename(path string) string { + // apparently NTFS internally uppercases filenames for comparision + return strings.ToUpper(path) +} diff --git a/internal/restorer/restorer_windows_test.go b/internal/restorer/restorer_windows_test.go index 61d075061..3f6c8472b 100644 --- a/internal/restorer/restorer_windows_test.go +++ b/internal/restorer/restorer_windows_test.go @@ -9,6 +9,7 @@ import ( "math" "os" "path" + "path/filepath" "syscall" "testing" "time" @@ -539,3 +540,36 @@ func TestDirAttributeCombinationsOverwrite(t *testing.T) { } } } + +func TestRestoreDeleteCaseInsensitive(t *testing.T) { + repo := repository.TestRepository(t) + tempdir := rtest.TempDir(t) + + sn, _ := saveSnapshot(t, repo, Snapshot{ + Nodes: map[string]Node{ + "anotherfile": File{Data: "content: file\n"}, + }, + }, noopGetGenericAttributes) + + // should delete files that no longer exist in the snapshot + deleteSn, _ := saveSnapshot(t, repo, Snapshot{ + Nodes: map[string]Node{ + "AnotherfilE": File{Data: "content: file\n"}, + }, + }, noopGetGenericAttributes) + + res := NewRestorer(repo, sn, Options{}) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + err := res.RestoreTo(ctx, tempdir) + rtest.OK(t, err) + + res = NewRestorer(repo, deleteSn, Options{Delete: true}) + err = res.RestoreTo(ctx, tempdir) + rtest.OK(t, err) + + // anotherfile must still exist + _, err = os.Stat(filepath.Join(tempdir, "anotherfile")) + rtest.OK(t, err) +} From f4b15fdd96389e704423b8f6a2dd67f7e6ac442f Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 29 Jun 2024 20:23:28 +0200 Subject: [PATCH 08/12] restore: allow deleting a directory to replace it with a file When the `--delete` option is specified, recursively delete directories that should be replaced with a file. --- internal/restorer/filerestorer.go | 24 ++++++++------ internal/restorer/filerestorer_test.go | 6 ++-- internal/restorer/fileswriter.go | 22 ++++++++----- internal/restorer/fileswriter_test.go | 43 ++++++++++++++++++++++++-- internal/restorer/restorer.go | 2 +- internal/restorer/restorer_test.go | 21 +++++++++++++ 6 files changed, 95 insertions(+), 23 deletions(-) diff --git a/internal/restorer/filerestorer.go b/internal/restorer/filerestorer.go index fd5b3c5db..56059cb16 100644 --- a/internal/restorer/filerestorer.go +++ b/internal/restorer/filerestorer.go @@ -53,6 +53,8 @@ type fileRestorer struct { sparse bool progress *restore.Progress + allowRecursiveDelete bool + dst string files []*fileInfo Error func(string, error) error @@ -63,21 +65,23 @@ func newFileRestorer(dst string, idx func(restic.BlobType, restic.ID) []restic.PackedBlob, connections uint, sparse bool, + allowRecursiveDelete bool, progress *restore.Progress) *fileRestorer { // as packs are streamed the concurrency is limited by IO workerCount := int(connections) return &fileRestorer{ - idx: idx, - blobsLoader: blobsLoader, - filesWriter: newFilesWriter(workerCount), - zeroChunk: repository.ZeroChunk(), - sparse: sparse, - progress: progress, - workerCount: workerCount, - dst: dst, - Error: restorerAbortOnAllErrors, + idx: idx, + blobsLoader: blobsLoader, + filesWriter: newFilesWriter(workerCount, allowRecursiveDelete), + zeroChunk: repository.ZeroChunk(), + sparse: sparse, + progress: progress, + allowRecursiveDelete: allowRecursiveDelete, + workerCount: workerCount, + dst: dst, + Error: restorerAbortOnAllErrors, } } @@ -207,7 +211,7 @@ func (r *fileRestorer) restoreFiles(ctx context.Context) error { } func (r *fileRestorer) restoreEmptyFileAt(location string) error { - f, err := createFile(r.targetPath(location), 0, false) + f, err := createFile(r.targetPath(location), 0, false, r.allowRecursiveDelete) if err != nil { return err } diff --git a/internal/restorer/filerestorer_test.go b/internal/restorer/filerestorer_test.go index d29c0dcea..f594760e4 100644 --- a/internal/restorer/filerestorer_test.go +++ b/internal/restorer/filerestorer_test.go @@ -144,7 +144,7 @@ func restoreAndVerify(t *testing.T, tempdir string, content []TestFile, files ma t.Helper() repo := newTestRepo(content) - r := newFileRestorer(tempdir, repo.loader, repo.Lookup, 2, sparse, nil) + r := newFileRestorer(tempdir, repo.loader, repo.Lookup, 2, sparse, false, nil) if files == nil { r.files = repo.files @@ -285,7 +285,7 @@ func TestErrorRestoreFiles(t *testing.T) { return loadError } - r := newFileRestorer(tempdir, repo.loader, repo.Lookup, 2, false, nil) + r := newFileRestorer(tempdir, repo.loader, repo.Lookup, 2, false, false, nil) r.files = repo.files err := r.restoreFiles(context.TODO()) @@ -326,7 +326,7 @@ func TestFatalDownloadError(t *testing.T) { }) } - r := newFileRestorer(tempdir, repo.loader, repo.Lookup, 2, false, nil) + r := newFileRestorer(tempdir, repo.loader, repo.Lookup, 2, false, false, nil) r.files = repo.files var errors []string diff --git a/internal/restorer/fileswriter.go b/internal/restorer/fileswriter.go index 034ed2725..962f66619 100644 --- a/internal/restorer/fileswriter.go +++ b/internal/restorer/fileswriter.go @@ -19,7 +19,8 @@ import ( // TODO I am not 100% convinced this is necessary, i.e. it may be okay // to use multiple os.File to write to the same target file type filesWriter struct { - buckets []filesWriterBucket + buckets []filesWriterBucket + allowRecursiveDelete bool } type filesWriterBucket struct { @@ -33,13 +34,14 @@ type partialFile struct { sparse bool } -func newFilesWriter(count int) *filesWriter { +func newFilesWriter(count int, allowRecursiveDelete bool) *filesWriter { buckets := make([]filesWriterBucket, count) for b := 0; b < count; b++ { buckets[b].files = make(map[string]*partialFile) } return &filesWriter{ - buckets: buckets, + buckets: buckets, + allowRecursiveDelete: allowRecursiveDelete, } } @@ -60,7 +62,7 @@ func openFile(path string) (*os.File, error) { return f, nil } -func createFile(path string, createSize int64, sparse bool) (*os.File, error) { +func createFile(path string, createSize int64, sparse bool, allowRecursiveDelete bool) (*os.File, error) { f, err := fs.OpenFile(path, fs.O_CREATE|fs.O_WRONLY|fs.O_NOFOLLOW, 0600) if err != nil && fs.IsAccessDenied(err) { // If file is readonly, clear the readonly flag by resetting the @@ -109,8 +111,14 @@ func createFile(path string, createSize int64, sparse bool) (*os.File, error) { } // not what we expected, try to get rid of it - if err := fs.Remove(path); err != nil { - return nil, err + if allowRecursiveDelete { + if err := fs.RemoveAll(path); err != nil { + return nil, err + } + } else { + if err := fs.Remove(path); err != nil { + return nil, err + } } // create a new file, pass O_EXCL to make sure there are no surprises f, err = fs.OpenFile(path, fs.O_CREATE|fs.O_WRONLY|fs.O_EXCL|fs.O_NOFOLLOW, 0600) @@ -169,7 +177,7 @@ func (w *filesWriter) writeToFile(path string, blob []byte, offset int64, create var f *os.File var err error if createSize >= 0 { - f, err = createFile(path, createSize, sparse) + f, err = createFile(path, createSize, sparse, w.allowRecursiveDelete) if err != nil { return nil, err } diff --git a/internal/restorer/fileswriter_test.go b/internal/restorer/fileswriter_test.go index 383a9e0d7..c69847927 100644 --- a/internal/restorer/fileswriter_test.go +++ b/internal/restorer/fileswriter_test.go @@ -13,7 +13,7 @@ import ( func TestFilesWriterBasic(t *testing.T) { dir := rtest.TempDir(t) - w := newFilesWriter(1) + w := newFilesWriter(1, false) f1 := dir + "/f1" f2 := dir + "/f2" @@ -39,6 +39,29 @@ func TestFilesWriterBasic(t *testing.T) { rtest.Equals(t, []byte{2, 2}, buf) } +func TestFilesWriterRecursiveOverwrite(t *testing.T) { + path := filepath.Join(t.TempDir(), "test") + + // create filled directory + rtest.OK(t, os.Mkdir(path, 0o700)) + rtest.OK(t, os.WriteFile(filepath.Join(path, "file"), []byte("data"), 0o400)) + + // must error if recursive delete is not allowed + w := newFilesWriter(1, false) + err := w.writeToFile(path, []byte{1}, 0, 2, false) + rtest.Assert(t, errors.Is(err, notEmptyDirError()), "unexepected error got %v", err) + rtest.Equals(t, 0, len(w.buckets[0].files)) + + // must replace directory + w = newFilesWriter(1, true) + rtest.OK(t, w.writeToFile(path, []byte{1, 1}, 0, 2, false)) + rtest.Equals(t, 0, len(w.buckets[0].files)) + + buf, err := os.ReadFile(path) + rtest.OK(t, err) + rtest.Equals(t, []byte{1, 1}, buf) +} + func TestCreateFile(t *testing.T) { basepath := filepath.Join(t.TempDir(), "test") @@ -110,7 +133,7 @@ func TestCreateFile(t *testing.T) { for j, test := range tests { path := basepath + fmt.Sprintf("%v%v", i, j) sc.create(t, path) - f, err := createFile(path, test.size, test.isSparse) + f, err := createFile(path, test.size, test.isSparse, false) if sc.err == nil { rtest.OK(t, err) fi, err := f.Stat() @@ -129,3 +152,19 @@ func TestCreateFile(t *testing.T) { }) } } + +func TestCreateFileRecursiveDelete(t *testing.T) { + path := filepath.Join(t.TempDir(), "test") + + // create filled directory + rtest.OK(t, os.Mkdir(path, 0o700)) + rtest.OK(t, os.WriteFile(filepath.Join(path, "file"), []byte("data"), 0o400)) + + // replace it + f, err := createFile(path, 42, false, true) + rtest.OK(t, err) + fi, err := f.Stat() + rtest.OK(t, err) + rtest.Assert(t, fi.Mode().IsRegular(), "wrong filetype %v", fi.Mode()) + rtest.OK(t, f.Close()) +} diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 6e81812c2..9efaa64df 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -349,7 +349,7 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { idx := NewHardlinkIndex[string]() filerestorer := newFileRestorer(dst, res.repo.LoadBlobsFromPack, res.repo.LookupBlob, - res.repo.Connections(), res.opts.Sparse, res.opts.Progress) + res.repo.Connections(), res.opts.Sparse, res.opts.Delete, res.opts.Progress) filerestorer.Error = res.Error debug.Log("first pass for %q", dst) diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 8a8f81ce0..3d2323d0f 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -1214,6 +1214,27 @@ func TestRestoreDryRun(t *testing.T) { rtest.Assert(t, errors.Is(err, os.ErrNotExist), "expected no file to be created, got %v", err) } +func TestRestoreOverwriteDirectory(t *testing.T) { + saveSnapshotsAndOverwrite(t, + Snapshot{ + Nodes: map[string]Node{ + "dir": Dir{ + Mode: normalizeFileMode(0755 | os.ModeDir), + Nodes: map[string]Node{ + "anotherfile": File{Data: "content: file\n"}, + }, + }, + }, + }, + Snapshot{ + Nodes: map[string]Node{ + "dir": File{Data: "content: file\n"}, + }, + }, + Options{Delete: true}, + ) +} + func TestRestoreDelete(t *testing.T) { repo := repository.TestRepository(t) tempdir := rtest.TempDir(t) From aa8e18cf32528435350d3a8e99b7d7f0cb49edba Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 29 Jun 2024 21:29:42 +0200 Subject: [PATCH 09/12] restore: add deletions to progress output --- doc/075_scripting.rst | 2 +- internal/restorer/restorer.go | 1 + internal/ui/restore/json.go | 2 ++ internal/ui/restore/json_test.go | 1 + internal/ui/restore/progress.go | 12 ++++++++++++ internal/ui/restore/progress_test.go | 2 ++ internal/ui/restore/text.go | 6 ++++-- internal/ui/restore/text_test.go | 1 + 8 files changed, 24 insertions(+), 3 deletions(-) diff --git a/doc/075_scripting.rst b/doc/075_scripting.rst index b83fe5eb5..e11f280db 100644 --- a/doc/075_scripting.rst +++ b/doc/075_scripting.rst @@ -520,7 +520,7 @@ Only printed if `--verbose=2` is specified. +----------------------+-----------------------------------------------------------+ | ``message_type`` | Always "verbose_status" | +----------------------+-----------------------------------------------------------+ -| ``action`` | Either "restored", "updated" or "unchanged" | +| ``action`` | Either "restored", "updated", "unchanged" or "deleted" | +----------------------+-----------------------------------------------------------+ | ``item`` | The item in question | +----------------------+-----------------------------------------------------------+ diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 9efaa64df..37072d9a9 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -499,6 +499,7 @@ func (res *Restorer) removeUnexpectedFiles(target, location string, expectedFile selectedForRestore, _ := res.SelectFilter(nodeLocation, false) // only delete files that were selected for restore if selectedForRestore { + res.opts.Progress.ReportDeletedFile(nodeLocation) if !res.opts.DryRun { if err := fs.RemoveAll(nodeTarget); err != nil { return err diff --git a/internal/ui/restore/json.go b/internal/ui/restore/json.go index ebc217176..7db2e21a3 100644 --- a/internal/ui/restore/json.go +++ b/internal/ui/restore/json.go @@ -56,6 +56,8 @@ func (t *jsonPrinter) CompleteItem(messageType ItemAction, item string, size uin action = "updated" case ActionFileUnchanged: action = "unchanged" + case ActionDeleted: + action = "deleted" default: panic("unknown message type") } diff --git a/internal/ui/restore/json_test.go b/internal/ui/restore/json_test.go index 1a749b933..06a70d5dc 100644 --- a/internal/ui/restore/json_test.go +++ b/internal/ui/restore/json_test.go @@ -53,6 +53,7 @@ func TestJSONPrintCompleteItem(t *testing.T) { {ActionFileRestored, 123, "{\"message_type\":\"verbose_status\",\"action\":\"restored\",\"item\":\"test\",\"size\":123}\n"}, {ActionFileUpdated, 123, "{\"message_type\":\"verbose_status\",\"action\":\"updated\",\"item\":\"test\",\"size\":123}\n"}, {ActionFileUnchanged, 123, "{\"message_type\":\"verbose_status\",\"action\":\"unchanged\",\"item\":\"test\",\"size\":123}\n"}, + {ActionDeleted, 0, "{\"message_type\":\"verbose_status\",\"action\":\"deleted\",\"item\":\"test\",\"size\":0}\n"}, } { term, printer := createJSONProgress() printer.CompleteItem(data.action, "test", data.size) diff --git a/internal/ui/restore/progress.go b/internal/ui/restore/progress.go index 04274b7ea..71a46e9dd 100644 --- a/internal/ui/restore/progress.go +++ b/internal/ui/restore/progress.go @@ -51,6 +51,7 @@ const ( ActionFileRestored ItemAction = "file restored" ActionFileUpdated ItemAction = "file updated" ActionFileUnchanged ItemAction = "file unchanged" + ActionDeleted ItemAction = "deleted" ) func NewProgress(printer ProgressPrinter, interval time.Duration) *Progress { @@ -126,6 +127,17 @@ func (p *Progress) AddSkippedFile(name string, size uint64) { p.printer.CompleteItem(ActionFileUnchanged, name, size) } +func (p *Progress) ReportDeletedFile(name string) { + if p == nil { + return + } + + p.m.Lock() + defer p.m.Unlock() + + p.printer.CompleteItem(ActionDeleted, name, 0) +} + func (p *Progress) Finish() { p.updater.Done() } diff --git a/internal/ui/restore/progress_test.go b/internal/ui/restore/progress_test.go index eda1b05c0..4a6304741 100644 --- a/internal/ui/restore/progress_test.go +++ b/internal/ui/restore/progress_test.go @@ -181,10 +181,12 @@ func TestProgressTypes(t *testing.T) { progress.AddFile(0) progress.AddProgress("dir", ActionDirRestored, fileSize, fileSize) progress.AddProgress("new", ActionFileRestored, 0, 0) + progress.ReportDeletedFile("del") return true }) test.Equals(t, itemTrace{ itemTraceEntry{ActionDirRestored, "dir", fileSize}, itemTraceEntry{ActionFileRestored, "new", 0}, + itemTraceEntry{ActionDeleted, "del", 0}, }, items) } diff --git a/internal/ui/restore/text.go b/internal/ui/restore/text.go index 77c2f2d15..2f0b3c01f 100644 --- a/internal/ui/restore/text.go +++ b/internal/ui/restore/text.go @@ -48,12 +48,14 @@ func (t *textPrinter) CompleteItem(messageType ItemAction, item string, size uin action = "updated" case ActionFileUnchanged: action = "unchanged" + case ActionDeleted: + action = "deleted" default: panic("unknown message type") } - if messageType == ActionDirRestored { - t.terminal.Print(fmt.Sprintf("restored %v", item)) + if messageType == ActionDirRestored || messageType == ActionDeleted { + t.terminal.Print(fmt.Sprintf("%-9v %v", action, item)) } else { t.terminal.Print(fmt.Sprintf("%-9v %v with size %v", action, item, ui.FormatBytes(size))) } diff --git a/internal/ui/restore/text_test.go b/internal/ui/restore/text_test.go index c7d173422..eddc0d1ca 100644 --- a/internal/ui/restore/text_test.go +++ b/internal/ui/restore/text_test.go @@ -65,6 +65,7 @@ func TestPrintCompleteItem(t *testing.T) { {ActionFileRestored, 123, "restored test with size 123 B"}, {ActionFileUpdated, 123, "updated test with size 123 B"}, {ActionFileUnchanged, 123, "unchanged test with size 123 B"}, + {ActionDeleted, 0, "deleted test"}, } { term, printer := createTextProgress() printer.CompleteItem(data.action, "test", data.size) From 868219aad1873dc703163855bf9687782898a622 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 29 Jun 2024 21:53:07 +0200 Subject: [PATCH 10/12] restore: test --dry-run plus --delete --- internal/restorer/restorer_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 3d2323d0f..a343bda2c 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -1214,6 +1214,32 @@ func TestRestoreDryRun(t *testing.T) { rtest.Assert(t, errors.Is(err, os.ErrNotExist), "expected no file to be created, got %v", err) } +func TestRestoreDryRunDelete(t *testing.T) { + snapshot := Snapshot{ + Nodes: map[string]Node{ + "foo": File{Data: "content: foo\n"}, + }, + } + + repo := repository.TestRepository(t) + tempdir := filepath.Join(rtest.TempDir(t), "target") + tempfile := filepath.Join(tempdir, "existing") + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + rtest.OK(t, os.Mkdir(tempdir, 0o755)) + f, err := os.Create(tempfile) + rtest.OK(t, err) + rtest.OK(t, f.Close()) + + sn, _ := saveSnapshot(t, repo, snapshot, noopGetGenericAttributes) + res := NewRestorer(repo, sn, Options{DryRun: true, Delete: true}) + rtest.OK(t, res.RestoreTo(ctx, tempdir)) + + _, err = os.Stat(tempfile) + rtest.Assert(t, err == nil, "expected file to still exist, got error %v", err) +} + func TestRestoreOverwriteDirectory(t *testing.T) { saveSnapshotsAndOverwrite(t, Snapshot{ From 569f111cb176459931fd5b83dd1c34c9dba1f8d0 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 29 Jun 2024 22:21:27 +0200 Subject: [PATCH 11/12] restore: document --delete option --- doc/050_restore.rst | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/doc/050_restore.rst b/doc/050_restore.rst index 497488241..5f351ae1d 100644 --- a/doc/050_restore.rst +++ b/doc/050_restore.rst @@ -111,6 +111,27 @@ values are supported: newer modification time (mtime). * ``--overwrite never``: never overwrite existing files. +Delete files not in snapshot +---------------------------- + +When restoring into a directory that already contains files, it can be useful to remove all +files that do not exist in the snapshot. For this, pass the ``--delete`` option to the ``restore`` +command. The command will then **delete all files** from the target directory that do not +exist in the snapshot. + +The ``--delete`` option also allows overwriting a non-empty directory if the snapshot contains a +file with the same name. + +.. warning:: + + Always use the ``--dry-run -vv`` option to verify what would be deleted before running the actual + command. + +When specifying ``--include`` or ``--exclude`` options, only files or directories matched by those +options will be deleted. For example, the command +``restic -r /srv/restic-repo restore 79766175:/work --target /tmp/restore-work --include /foo --delete`` +would only delete files within ``/tmp/restore-work/foo``. + Dry run ------- From 54316978cd1fe42bb0ba9251d24595f68c38dc99 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 29 Jun 2024 22:31:19 +0200 Subject: [PATCH 12/12] add restore --delete changelog --- changelog/unreleased/issue-2348 | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 changelog/unreleased/issue-2348 diff --git a/changelog/unreleased/issue-2348 b/changelog/unreleased/issue-2348 new file mode 100644 index 000000000..a8a0849fe --- /dev/null +++ b/changelog/unreleased/issue-2348 @@ -0,0 +1,10 @@ +Enhancement: Add `--delete` option to `restore` command + +The `restore` command now supports a `--delete` option that allows removing files and directories +from the target directory that do not exist in the snapshot. This option also allows files in the +snapshot to replace non-empty directories. + +To check that only the expected files are deleted add the `--dry-run --verbose=2` options. + +https://github.com/restic/restic/issues/2348 +https://github.com/restic/restic/pull/4881