restore: clean up error handling when restoring metadata

- Fix a logic error that instead of reporting the *first*
  metadata-setting error that appears, we were instead reporting the
  *last* error (and only if the lchown call failed!).
- Don't show any errors when setting metadata for files in non-root
  mode (things like timestamps, attributes). Previously, only lchown
  errors were skipped. But other kinds of attribute errors make sense
  to skip as well. The code path happened to work correctly before
  because of the above logic error. But once that was fixed, this
  change needed to happen too.

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
-		// 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)
-		}
+		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")
+}