Merge pull request #4958 from mikix/restore-errors

restore: clean up error handling when restoring metadata

changelog/unreleased/pull-4958

@@ -0,0 +1,7 @@
+Bugfix: Don't ignore metadata-setting errors during restore
+
+Restic was accidentally ignoring errors when setting timestamps,
+attributes, or file modes during restore. It will now report those
+errors (unless it's just a permission error when not running as root).
+
+https://github.com/restic/restic/pull/4958

internal/restic/node.go

@@ -229,6 +229,13 @@ func (node *Node) CreateAt(ctx context.Context, path string, repo BlobLoader) er
 func (node Node) RestoreMetadata(path string, warn func(msg string)) error {
 	err := node.restoreMetadata(path, warn)
 	if err != nil {
+		// It is common to have permission errors for folders like /home
+		// unless you're running as root, so ignore those.
+		if os.Geteuid() > 0 && errors.Is(err, os.ErrPermission) {
+			debug.Log("not running as root, ignoring permission error for %v: %v",
+				path, err)
+			return nil
+		}
 		debug.Log("restoreMetadata(%s) error %v", path, err)
 	}
 
@@ -239,33 +246,26 @@ func (node Node) restoreMetadata(path string, warn func(msg string)) error {
 	var firsterr error
 
 	if err := lchown(path, int(node.UID), int(node.GID)); err != nil {
-		// Like "cp -a" and "rsync -a" do, we only report lchown permission errors
+		firsterr = errors.WithStack(err)
-		// if we run as root.
-		if os.Geteuid() > 0 && os.IsPermission(err) {
-			debug.Log("not running as root, ignoring lchown permission error for %v: %v",
-				path, err)
-		} else {
-			firsterr = errors.WithStack(err)
-		}
 	}
 
 	if err := node.RestoreTimestamps(path); err != nil {
 		debug.Log("error restoring timestamps for dir %v: %v", path, err)
-		if firsterr != nil {
+		if firsterr == nil {
 			firsterr = err
 		}
 	}
 
 	if err := node.restoreExtendedAttributes(path); err != nil {
 		debug.Log("error restoring extended attributes for %v: %v", path, err)
-		if firsterr != nil {
+		if firsterr == nil {
 			firsterr = err
 		}
 	}
 
 	if err := node.restoreGenericAttributes(path, warn); err != nil {
 		debug.Log("error restoring generic attributes for %v: %v", path, err)
-		if firsterr != nil {
+		if firsterr == nil {
 			firsterr = err
 		}
 	}
@@ -275,7 +275,7 @@ func (node Node) restoreMetadata(path string, warn func(msg string)) error {
 	// calls above would fail.
 	if node.Type != "symlink" {
 		if err := fs.Chmod(path, node.Mode); err != nil {
-			if firsterr != nil {
+			if firsterr == nil {
 				firsterr = errors.WithStack(err)
 			}
 		}

internal/restic/node_test.go

@@ -13,6 +13,7 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/test"
 	rtest "github.com/restic/restic/internal/test"
 )
@@ -382,3 +383,14 @@ func TestSymlinkSerializationFormat(t *testing.T) {
 		test.Assert(t, n2.LinkTargetRaw == nil, "quoted link target is just a helper field and must be unset after decoding")
 	}
 }
+
+func TestNodeRestoreMetadataError(t *testing.T) {
+	tempdir := t.TempDir()
+
+	node := nodeTests[0]
+	nodePath := filepath.Join(tempdir, node.Name)
+
+	// This will fail because the target file does not exist
+	err := node.RestoreMetadata(nodePath, func(msg string) { rtest.OK(t, fmt.Errorf("Warning triggered for path: %s: %s", nodePath, msg)) })
+	test.Assert(t, errors.Is(err, os.ErrNotExist), "failed for an unexpected reason")
+}