Merge pull request #2906 from kitone/fix-inconsistent-timestamps-permissions

Restore inconsistent timestamps permissions

changelog/unreleased/issue-1212

@@ -0,0 +1,11 @@
+Bugfix: Restore timestamps and permissions on intermediate directories
+
+When using the `--include` option of the restore command, restic restored
+timestamps and permissions only on directories selected by the include pattern.
+Intermediate directories, which are necessary to restore files located in sub-
+directories, were created with default permissions. We've fixed the restore
+command to restore timestamps and permissions for these directories as well.
+
+https://github.com/restic/restic/issues/1212
+https://github.com/restic/restic/issues/1402
+https://github.com/restic/restic/pull/2906

internal/restorer/restorer.go

@@ -49,12 +49,12 @@ type treeVisitor struct {
 
 // 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) error {
+func (res *Restorer) traverseTree(ctx context.Context, target, location string, treeID restic.ID, visitor treeVisitor) (hasRestored bool, err error) {
 	debug.Log("%v %v %v", target, location, treeID)
 	tree, err := res.repo.LoadTree(ctx, treeID)
 	if err != nil {
 		debug.Log("error loading tree %v: %v", treeID, err)
-		return res.Error(location, err)
+		return hasRestored, res.Error(location, err)
 	}
 
 	for _, node := range tree.Nodes {
@@ -66,7 +66,7 @@ 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 err
+				return hasRestored, err
 			}
 			continue
 		}
@@ -79,7 +79,7 @@ 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 err
+				return hasRestored, err
 			}
 			continue
 		}
@@ -90,7 +90,11 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string,
 		}
 
 		selectedForRestore, childMayBeSelected := res.SelectFilter(nodeLocation, nodeTarget, node)
-		debug.Log("SelectFilter returned %v %v", selectedForRestore, childMayBeSelected)
+		debug.Log("SelectFilter returned %v %v for %q", selectedForRestore, childMayBeSelected, nodeLocation)
+
+		if selectedForRestore {
+			hasRestored = true
+		}
 
 		sanitizeError := func(err error) error {
 			if err != nil {
@@ -101,27 +105,38 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string,
 
 		if node.Type == "dir" {
 			if node.Subtree == nil {
-				return errors.Errorf("Dir without subtree in tree %v", treeID.Str())
+				return hasRestored, errors.Errorf("Dir without subtree in tree %v", treeID.Str())
 			}
 
 			if selectedForRestore {
 				err = sanitizeError(visitor.enterDir(node, nodeTarget, nodeLocation))
 				if err != nil {
-					return err
+					return hasRestored, err
 				}
 			}
 
+			// keep track of restored child status
+			// so metadata of the current directory are restored on leaveDir
+			childHasRestored := false
+
 			if childMayBeSelected {
-				err = sanitizeError(res.traverseTree(ctx, nodeTarget, nodeLocation, *node.Subtree, visitor))
+				childHasRestored, err = res.traverseTree(ctx, nodeTarget, nodeLocation, *node.Subtree, visitor)
+				err = sanitizeError(err)
 				if err != nil {
-					return err
+					return hasRestored, err
+				}
+				// inform the parent directory to restore parent metadata on leaveDir if needed
+				if childHasRestored {
+					hasRestored = true
 				}
 			}
 
-			if selectedForRestore {
+			// 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 {
 				err = sanitizeError(visitor.leaveDir(node, nodeTarget, nodeLocation))
 				if err != nil {
-					return err
+					return hasRestored, err
 				}
 			}
 
@@ -131,12 +146,12 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string,
 		if selectedForRestore {
 			err = sanitizeError(visitor.visitNode(node, nodeTarget, nodeLocation))
 			if err != nil {
-				return err
+				return hasRestored, err
 			}
 		}
 	}
 
-	return nil
+	return hasRestored, nil
 }
 
 func (res *Restorer) restoreNodeTo(ctx context.Context, node *restic.Node, target, location string) error {
@@ -198,24 +213,23 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error {
 		}
 	}
 
-	restoreNodeMetadata := func(node *restic.Node, target, location string) error {
-		return res.restoreNodeMetadataTo(node, target, location)
-	}
-	noop := func(node *restic.Node, target, location string) error { return nil }
-
 	idx := restic.NewHardlinkIndex()
 
 	filerestorer := newFileRestorer(dst, res.repo.Backend().Load, res.repo.Key(), res.repo.Index().Lookup)
 
+	debug.Log("first pass for %q", dst)
+
 	// 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, string(filepath.Separator), *res.sn.Tree, treeVisitor{
 		enterDir: func(node *restic.Node, target, location string) error {
+			debug.Log("first pass, enterDir: mkdir %q, leaveDir should restore metadata", location)
 			// create dir with default permissions
 			// #leaveDir restores dir metadata after visiting all children
 			return fs.MkdirAll(target, 0700)
 		},
 
 		visitNode: func(node *restic.Node, target, location string) error {
+			debug.Log("first pass, visitNode: mkdir %q, leaveDir on second pass should restore metadata", location)
 			// create parent dir with default permissions
 			// second pass #leaveDir restores dir metadata after visiting/restoring all children
 			err := fs.MkdirAll(filepath.Dir(target), 0700)
@@ -242,7 +256,9 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error {
 
 			return nil
 		},
-		leaveDir: noop,
+		leaveDir: func(node *restic.Node, target, location string) error {
+			return nil
+		},
 	})
 	if err != nil {
 		return err
@@ -253,10 +269,15 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error {
 		return err
 	}
 
+	debug.Log("second pass for %q", dst)
+
 	// second tree pass: restore special files and filesystem metadata
-	return res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{
+	_, err = res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{
-		enterDir: noop,
+		enterDir: func(node *restic.Node, target, location string) error {
+			return nil
+		},
 		visitNode: func(node *restic.Node, target, location string) error {
+			debug.Log("second pass, visitNode: restore node %q", location)
 			if node.Type != "file" {
 				return res.restoreNodeTo(ctx, node, target, location)
 			}
@@ -275,8 +296,12 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error {
 
 			return res.restoreNodeMetadataTo(node, target, location)
 		},
-		leaveDir: restoreNodeMetadata,
+		leaveDir: func(node *restic.Node, target, location string) error {
+			debug.Log("second pass, leaveDir restore metadata %q", location)
+			return res.restoreNodeMetadataTo(node, target, location)
+		},
 	})
+	return err
 }
 
 // Snapshot returns the snapshot this restorer is configured to use.
@@ -289,7 +314,7 @@ func (res *Restorer) VerifyFiles(ctx context.Context, dst string) (int, error) {
 	// TODO multithreaded?
 
 	count := 0
-	err := res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{
+	_, err := res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{
 		enterDir: func(node *restic.Node, target, location string) error { return nil },
 		visitNode: func(node *restic.Node, target, location string) error {
 			if node.Type != "file" {

internal/restorer/restorer_test.go

@@ -6,6 +6,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 	"time"
@@ -23,14 +24,17 @@ type Snapshot struct {
 }
 
 type File struct {
-	Data  string
+	Data    string
-	Links uint64
+	Links   uint64
-	Inode uint64
+	Inode   uint64
+	Mode    os.FileMode
+	ModTime time.Time
 }
 
 type Dir struct {
-	Nodes map[string]Node
+	Nodes   map[string]Node
-	Mode  os.FileMode
+	Mode    os.FileMode
+	ModTime time.Time
 }
 
 func saveFile(t testing.TB, repo restic.Repository, node File) restic.ID {
@@ -66,9 +70,14 @@ func saveDir(t testing.TB, repo restic.Repository, nodes map[string]Node, inode
 			if len(n.(File).Data) > 0 {
 				fc = append(fc, saveFile(t, repo, node))
 			}
+			mode := node.Mode
+			if mode == 0 {
+				mode = 0644
+			}
 			tree.Insert(&restic.Node{
 				Type:    "file",
-				Mode:    0644,
+				Mode:    mode,
+				ModTime: node.ModTime,
 				Name:    name,
 				UID:     uint32(os.Getuid()),
 				GID:     uint32(os.Getgid()),
@@ -88,6 +97,7 @@ func saveDir(t testing.TB, repo restic.Repository, nodes map[string]Node, inode
 			tree.Insert(&restic.Node{
 				Type:    "dir",
 				Mode:    mode,
+				ModTime: node.ModTime,
 				Name:    name,
 				UID:     uint32(os.Getuid()),
 				GID:     uint32(os.Getgid()),
@@ -655,6 +665,7 @@ func TestRestorerTraverseTree(t *testing.T) {
 			},
 			Visitor: checkVisitOrder([]TreeVisit{
 				{"visitNode", "/dir/otherfile"},
+				{"leaveDir", "/dir"},
 			}),
 		},
 	}
@@ -681,10 +692,115 @@ 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, string(filepath.Separator), *sn.Tree, test.Visitor(t))
 			if err != nil {
 				t.Fatal(err)
 			}
 		})
 	}
 }
+
+func normalizeFileMode(mode os.FileMode) os.FileMode {
+	if runtime.GOOS == "windows" {
+		if mode.IsDir() {
+			return 0555 | os.ModeDir
+		}
+		return os.FileMode(0444)
+	}
+	return mode
+}
+
+func checkConsistentInfo(t testing.TB, file string, fi os.FileInfo, modtime time.Time, mode os.FileMode) {
+	if fi.Mode() != mode {
+		t.Errorf("checking %q, Mode() returned wrong value, want 0%o, got 0%o", file, mode, fi.Mode())
+	}
+
+	if !fi.ModTime().Equal(modtime) {
+		t.Errorf("checking %s, ModTime() returned wrong value, want %v, got %v", file, modtime, fi.ModTime())
+	}
+}
+
+// test inspired from test case https://github.com/restic/restic/issues/1212
+func TestRestorerConsistentTimestampsAndPermissions(t *testing.T) {
+	timeForTest := time.Date(2019, time.January, 9, 1, 46, 40, 0, time.UTC)
+
+	repo, cleanup := repository.TestRepository(t)
+	defer cleanup()
+
+	_, id := saveSnapshot(t, repo, Snapshot{
+		Nodes: map[string]Node{
+			"dir": Dir{
+				Mode:    normalizeFileMode(0750 | os.ModeDir),
+				ModTime: timeForTest,
+				Nodes: map[string]Node{
+					"file1": File{
+						Mode:    normalizeFileMode(os.FileMode(0700)),
+						ModTime: timeForTest,
+						Data:    "content: file\n",
+					},
+					"anotherfile": File{
+						Data: "content: file\n",
+					},
+					"subdir": Dir{
+						Mode:    normalizeFileMode(0700 | os.ModeDir),
+						ModTime: timeForTest,
+						Nodes: map[string]Node{
+							"file2": File{
+								Mode:    normalizeFileMode(os.FileMode(0666)),
+								ModTime: timeForTest,
+								Links:   2,
+								Inode:   1,
+							},
+						},
+					},
+				},
+			},
+		},
+	})
+
+	res, err := NewRestorer(repo, id)
+	rtest.OK(t, err)
+
+	res.SelectFilter = func(item string, dstpath string, node *restic.Node) (selectedForRestore bool, childMayBeSelected bool) {
+		switch filepath.ToSlash(item) {
+		case "/dir":
+			childMayBeSelected = true
+		case "/dir/file1":
+			selectedForRestore = true
+			childMayBeSelected = false
+		case "/dir/subdir":
+			selectedForRestore = true
+			childMayBeSelected = true
+		case "/dir/subdir/file2":
+			selectedForRestore = true
+			childMayBeSelected = false
+		}
+		return selectedForRestore, childMayBeSelected
+	}
+
+	tempdir, cleanup := rtest.TempDir(t)
+	defer cleanup()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	err = res.RestoreTo(ctx, tempdir)
+	rtest.OK(t, err)
+
+	var testPatterns = []struct {
+		path    string
+		modtime time.Time
+		mode    os.FileMode
+	}{
+		{"dir", timeForTest, normalizeFileMode(0750 | os.ModeDir)},
+		{filepath.Join("dir", "file1"), timeForTest, normalizeFileMode(os.FileMode(0700))},
+		{filepath.Join("dir", "subdir"), timeForTest, normalizeFileMode(0700 | os.ModeDir)},
+		{filepath.Join("dir", "subdir", "file2"), timeForTest, normalizeFileMode(os.FileMode(0666))},
+	}
+
+	for _, test := range testPatterns {
+		f, err := os.Stat(filepath.Join(tempdir, test.path))
+		rtest.OK(t, err)
+		checkConsistentInfo(t, test.path, f, test.modtime, test.mode)
+	}
+}