From 4f6d2502f7dbaa3249ca280de3704fa13a529bda Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 21 Jul 2018 14:06:19 +0200 Subject: [PATCH 1/5] restorer: Add test for restore with include filter --- internal/restorer/restorer_test.go | 31 ++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index b57b6f409..83a178150 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -134,6 +134,7 @@ func TestRestorer(t *testing.T) { Files map[string]string ErrorsMust map[string]string ErrorsMay map[string]string + Select func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) }{ // valid test cases { @@ -212,6 +213,31 @@ func TestRestorer(t *testing.T) { "topfile": "top-level file", }, }, + { + Snapshot: Snapshot{ + Nodes: map[string]Node{ + "dir": Dir{ + Nodes: map[string]Node{ + "file": File{"content: file\n"}, + }, + }, + }, + }, + Files: map[string]string{ + "dir/file": "content: file\n", + }, + Select: func(item, dstpath string, node *restic.Node) (selectedForRestore bool, childMayBeSelected bool) { + switch item { + case filepath.FromSlash("/dir"): + childMayBeSelected = true + case filepath.FromSlash("/dir/file"): + selectedForRestore = true + childMayBeSelected = true + } + + return selectedForRestore, childMayBeSelected + }, + }, // test cases with invalid/constructed names { @@ -293,6 +319,11 @@ func TestRestorer(t *testing.T) { item, dstpath, tempdir) return false, false } + + if test.Select != nil { + return test.Select(item, dstpath, node) + } + return true, true } From 57636a45733256e71b9331885e4ee779d0bc954b Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 21 Jul 2018 20:56:24 +0200 Subject: [PATCH 2/5] restorer: Run tests in the same package --- internal/restorer/restorer_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 83a178150..1957df14a 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -1,4 +1,4 @@ -package restorer_test +package restorer import ( "bytes" @@ -13,7 +13,6 @@ import ( "github.com/restic/restic/internal/fs" "github.com/restic/restic/internal/repository" "github.com/restic/restic/internal/restic" - "github.com/restic/restic/internal/restorer" rtest "github.com/restic/restic/internal/test" ) @@ -301,7 +300,7 @@ func TestRestorer(t *testing.T) { _, id := saveSnapshot(t, repo, test.Snapshot) t.Logf("snapshot saved as %v", id.Str()) - res, err := restorer.NewRestorer(repo, id) + res, err := NewRestorer(repo, id) if err != nil { t.Fatal(err) } @@ -422,7 +421,7 @@ func TestRestorerRelative(t *testing.T) { _, id := saveSnapshot(t, repo, test.Snapshot) t.Logf("snapshot saved as %v", id.Str()) - res, err := restorer.NewRestorer(repo, id) + res, err := NewRestorer(repo, id) if err != nil { t.Fatal(err) } From 74016d59813fa838b71c6be60c387026b4d18ab3 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 21 Jul 2018 20:56:38 +0200 Subject: [PATCH 3/5] restorer: Fix return of saveSnapshot --- internal/restorer/restorer_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 1957df14a..02fc74a3c 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -91,7 +91,7 @@ func saveDir(t testing.TB, repo restic.Repository, nodes map[string]Node) restic return id } -func saveSnapshot(t testing.TB, repo restic.Repository, snapshot Snapshot) (restic.Repository, restic.ID) { +func saveSnapshot(t testing.TB, repo restic.Repository, snapshot Snapshot) (*restic.Snapshot, restic.ID) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -118,7 +118,7 @@ func saveSnapshot(t testing.TB, repo restic.Repository, snapshot Snapshot) (rest t.Fatal(err) } - return repo, id + return sn, id } // toSlash converts the OS specific path dir to a slash-separated path. From ce19f2694872054a5727e4d931315edd39b1a9c9 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 21 Jul 2018 21:55:57 +0200 Subject: [PATCH 4/5] restorer: Add tests for traverseTree --- internal/restorer/restorer_test.go | 210 +++++++++++++++++++++++++++++ 1 file changed, 210 insertions(+) diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 02fc74a3c..c5fdd6cb8 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -466,3 +466,213 @@ 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 +} + +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 { + if pos >= len(list) { + t.Errorf("step %v, %v(%v): expected no more than %d function calls", pos, funcName, location, len(list)) + pos++ + return nil + } + + v := list[pos] + + if v.funcName != funcName { + t.Errorf("step %v, location %v: want function %v, but %v was called", + pos, location, v.funcName, funcName) + } + + if location != filepath.FromSlash(v.location) { + t.Errorf("step %v: want location %v, got %v", pos, list[pos].location, location) + } + + pos++ + return nil + } + } + + return treeVisitor{ + enterDir: check("enterDir"), + visitNode: check("visitNode"), + leaveDir: check("leaveDir"), + } + } +} + +func TestRestorerTraverseTree(t *testing.T) { + var tests = []struct { + Snapshot + Select func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) + Visitor TraverseTreeCheck + }{ + { + // select everything + Snapshot: Snapshot{ + Nodes: map[string]Node{ + "dir": Dir{Nodes: map[string]Node{ + "otherfile": File{"x"}, + "subdir": Dir{Nodes: map[string]Node{ + "file": File{"content: file\n"}, + }}, + }}, + "foo": File{"content: foo\n"}, + }, + }, + Select: func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) { + 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"}, + }), + }, + + // select only the top-level file + { + Snapshot: Snapshot{ + Nodes: map[string]Node{ + "dir": Dir{Nodes: map[string]Node{ + "otherfile": File{"x"}, + "subdir": Dir{Nodes: map[string]Node{ + "file": File{"content: file\n"}, + }}, + }}, + "foo": File{"content: foo\n"}, + }, + }, + Select: func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) { + if item == "/foo" { + return true, false + } + return false, false + }, + Visitor: checkVisitOrder([]TreeVisit{ + {"visitNode", "/foo"}, + }), + }, + { + Snapshot: Snapshot{ + Nodes: map[string]Node{ + "aaa": File{"content: foo\n"}, + "dir": Dir{Nodes: map[string]Node{ + "otherfile": File{"x"}, + "subdir": Dir{Nodes: map[string]Node{ + "file": File{"content: file\n"}, + }}, + }}, + }, + }, + Select: func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) { + if item == "/aaa" { + return true, false + } + return false, false + }, + Visitor: checkVisitOrder([]TreeVisit{ + {"visitNode", "/aaa"}, + }), + }, + + // select dir/ + { + Snapshot: Snapshot{ + Nodes: map[string]Node{ + "dir": Dir{Nodes: map[string]Node{ + "otherfile": File{"x"}, + "subdir": Dir{Nodes: map[string]Node{ + "file": File{"content: file\n"}, + }}, + }}, + "foo": File{"content: foo\n"}, + }, + }, + Select: func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) { + if strings.HasPrefix(item, "/dir") { + return true, true + } + return false, false + }, + Visitor: checkVisitOrder([]TreeVisit{ + {"enterDir", "/dir"}, + {"visitNode", "/dir/otherfile"}, + {"enterDir", "/dir/subdir"}, + {"visitNode", "/dir/subdir/file"}, + {"leaveDir", "/dir/subdir"}, + {"leaveDir", "/dir"}, + }), + }, + + // select only dir/otherfile + { + Snapshot: Snapshot{ + Nodes: map[string]Node{ + "dir": Dir{Nodes: map[string]Node{ + "otherfile": File{"x"}, + "subdir": Dir{Nodes: map[string]Node{ + "file": File{"content: file\n"}, + }}, + }}, + "foo": File{"content: foo\n"}, + }, + }, + Select: func(item string, dstpath string, node *restic.Node) (selectForRestore bool, childMayBeSelected bool) { + switch item { + case "/dir": + return false, true + case "/dir/otherfile": + return true, false + default: + return false, false + } + }, + Visitor: checkVisitOrder([]TreeVisit{ + {"visitNode", "/dir/otherfile"}, + }), + }, + } + + for _, test := range tests { + t.Run("", func(t *testing.T) { + repo, cleanup := repository.TestRepository(t) + defer cleanup() + sn, id := saveSnapshot(t, repo, test.Snapshot) + + res, err := NewRestorer(repo, id) + if err != nil { + t.Fatal(err) + } + + res.SelectFilter = test.Select + + tempdir, cleanup := rtest.TempDir(t) + defer cleanup() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // 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)) + if err != nil { + t.Fatal(err) + } + }) + } +} From 44924ba043eb3367ef29a69c6694c716031cd83b Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 21 Jul 2018 22:39:12 +0200 Subject: [PATCH 5/5] restorer: Fix traverseTree traverseTree() was meant to call enterDir() whenever a directory is selected for restore, either explicitly or implicitly (=contains a file which is to be restored). After restoring a file, leaveDir() is called in reverse order for all intermediate directories so that the metadata can be restored. When a directory is selected implicitly, the metadata for it is restored. This is different from the previous restorer behavior, which created implicitly selected intermediate directories with permissions 0700 (only user can read/write it). This commit changes the behavior back to the old one. Only a directory is explicitly selected for restore, enterDir()/leaveDir() are called for it. Otherwise, only visitNode() is called, so visitNode() needs to make sure the parent directory exists. If the directory is explicitly included, leaveDir() will then restore the metadata correctly. When we decide to change the behavior (restore metadata for all intermediate directories, even if selected implicitly), we should do that in the selection functions, not here. This finally resolves #1870 --- internal/restorer/restorer.go | 45 +++++++++++------------------------ 1 file changed, 14 insertions(+), 31 deletions(-) diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 160497110..73e844ac0 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -99,36 +99,16 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, return err } - enteredDir := false if node.Type == "dir" { if node.Subtree == nil { return errors.Errorf("Dir without subtree in tree %v", treeID.Str()) } - // ifedorenko: apparently a dir can be selected explicitly or implicitly when a child is selected - // to support implicit selection, visit the directory from within visitor#visitNode if selectedForRestore { - enteredDir = true err = sanitizeError(visitor.enterDir(node, nodeTarget, nodeLocation)) if err != nil { return err } - } else { - _visitor := visitor - visitor = treeVisitor{ - enterDir: _visitor.enterDir, - visitNode: func(node *restic.Node, nodeTarget, nodeLocation string) error { - if !enteredDir { - enteredDir = true - derr := sanitizeError(_visitor.enterDir(node, nodeTarget, nodeLocation)) - if derr != nil { - return derr - } - } - return _visitor.visitNode(node, nodeTarget, nodeLocation) - }, - leaveDir: _visitor.leaveDir, - } } if childMayBeSelected { @@ -137,25 +117,21 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, return err } } - } - if selectedForRestore && node.Type != "dir" { - err = sanitizeError(visitor.visitNode(node, nodeTarget, nodeLocation)) - if err != nil { - err = res.Error(nodeLocation, node, errors.Wrap(err, "restoreNodeTo")) + if selectedForRestore { + err = sanitizeError(visitor.leaveDir(node, nodeTarget, nodeLocation)) if err != nil { return err } } + + continue } - if enteredDir { - err = sanitizeError(visitor.leaveDir(node, nodeTarget, nodeLocation)) + if selectedForRestore { + err = sanitizeError(visitor.visitNode(node, nodeTarget, nodeLocation)) if err != nil { - err = res.Error(nodeLocation, node, errors.Wrap(err, "RestoreTimestamps")) - if err != nil { - return err - } + return err } } } @@ -211,6 +187,13 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { return fs.MkdirAll(target, 0700) }, visitNode: func(node *restic.Node, target, location string) error { + // create parent dir with default permissions + // #leaveDir restores dir metadata after visiting all children + err := fs.MkdirAll(filepath.Dir(target), 0700) + if err != nil { + return err + } + return res.restoreNodeTo(ctx, node, target, location, idx) }, leaveDir: func(node *restic.Node, target, location string) error {