Fix #1212 restore code produces inconsistent timestamps/permissions.

Keep track of restored child status so parent and root directory not selected by filter will also restore metadata when traversing tree.

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) (somethingRestored 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 somethingRestored, 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 somethingRestored, 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 somethingRestored, err
 			}
 			continue
 		}
@@ -92,6 +92,10 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string,
 		selectedForRestore, childMayBeSelected := res.SelectFilter(nodeLocation, nodeTarget, node)
 		debug.Log("SelectFilter returned %v %v for %q", selectedForRestore, childMayBeSelected, nodeLocation)
 
+		if selectedForRestore {
+			somethingRestored = true
+		}
+
 		sanitizeError := func(err error) error {
 			if err != nil {
 				err = res.Error(nodeLocation, err)
@@ -101,27 +105,39 @@ 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 somethingRestored, 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 somethingRestored, err
 				}
 			}
 
+			// we need to keep track of restored child status
+			// so we can restore the metadata of the current directory on leaveDir
+			childRestored := false
+
 			if childMayBeSelected {
-				err = sanitizeError(res.traverseTree(ctx, nodeTarget, nodeLocation, *node.Subtree, visitor))
+				childRestored, err = res.traverseTree(ctx, nodeTarget, nodeLocation, *node.Subtree, visitor)
+				err = sanitizeError(err)
 				if err != nil {
-					return err
+					return somethingRestored, err
+				}
+				// we need to inform the parent directory
+				// to restore the metadata of the parent directory on leaveDir
+				if childRestored {
+					somethingRestored = true
 				}
 			}
 
-			if selectedForRestore {
+			// we check both because the current directory can be selected for restore
+			// or a child be restored in the any of the subtree so we need to restore metadata on each level of the hierarchy
+			if selectedForRestore || childRestored {
 				err = sanitizeError(visitor.leaveDir(node, nodeTarget, nodeLocation))
 				if err != nil {
-					return err
+					return somethingRestored, err
 				}
 			}
 
@@ -131,12 +147,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 somethingRestored, err
 			}
 		}
 	}
 
-	return nil
+	return somethingRestored, nil
 }
 
 func (res *Restorer) restoreNodeTo(ctx context.Context, node *restic.Node, target, location string) error {
@@ -205,7 +221,7 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error {
 	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
@@ -258,7 +274,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
-	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: func(node *restic.Node, target, location string) error {
 			debug.Log("second pass, enterDir noop %q", location)
 			return nil
@@ -284,10 +300,10 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error {
 			return res.restoreNodeMetadataTo(node, target, location)
 		},
 		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.
@@ -300,7 +316,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

@@ -691,7 +691,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, string(filepath.Separator), *sn.Tree, test.Visitor(t))
 			if err != nil {
 				t.Fatal(err)
 			}