Merge pull request #2252 from restic/fix-2249

Read fresh metadata for unmodified files

changelog/unreleased/issue-2249

@@ -0,0 +1,6 @@
+Bugfix: Read fresh metadata for unmodified files
+
+Restic took all metadata for files which were detected as unmodified, not taking into account changed metadata (ownership, mode). This is now corrected.
+
+https://github.com/restic/restic/issues/2249
+https://github.com/restic/restic/pull/2252

internal/archiver/archiver.go

@@ -384,12 +384,19 @@ func (arch *Archiver) Save(ctx context.Context, snPath, target string, previous
 			return FutureNode{}, true, nil
 		}
 
-		// use previous node if the file hasn't changed
+		// use previous list of blobs if the file hasn't changed
 		if previous != nil && !fileChanged(fi, previous, arch.IgnoreInode) {
-			debug.Log("%v hasn't changed, returning old node", target)
+			debug.Log("%v hasn't changed, using old list of blobs", target)
 			arch.CompleteItem(snPath, previous, previous, ItemStats{}, time.Since(start))
 			arch.CompleteBlob(snPath, previous.Size)
-			fn.node = previous
+			fn.node, err = arch.nodeFromFileInfo(target, fi)
+			if err != nil {
+				return FutureNode{}, false, err
+			}
+
+			// copy list of blobs
+			fn.node.Content = previous.Content
+
 			_ = file.Close()
 			return fn, false, nil
 		}

internal/archiver/archiver_test.go

@@ -14,6 +14,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/restic/restic/internal/checker"
 	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/fs"
@@ -1930,3 +1931,176 @@ func TestArchiverAbortEarlyOnError(t *testing.T) {
 		})
 	}
 }
+
+func snapshot(t testing.TB, repo restic.Repository, fs fs.FS, parent restic.ID, filename string) (restic.ID, *restic.Node) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	arch := New(repo, fs, Options{})
+
+	sopts := SnapshotOptions{
+		Time:           time.Now(),
+		ParentSnapshot: parent,
+	}
+	snapshot, snapshotID, err := arch.Snapshot(ctx, []string{filename}, sopts)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tree, err := repo.LoadTree(ctx, *snapshot.Tree)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	node := tree.Find(filename)
+	if node == nil {
+		t.Fatalf("unable to find node for testfile in snapshot")
+	}
+
+	return snapshotID, node
+}
+
+func chmod(t testing.TB, filename string, mode os.FileMode) {
+	err := os.Chmod(filename, mode)
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
+// StatFS allows overwriting what is returned by the Lstat function.
+type StatFS struct {
+	fs.FS
+
+	OverrideLstat map[string]os.FileInfo
+}
+
+func (fs *StatFS) Lstat(name string) (os.FileInfo, error) {
+	if fi, ok := fs.OverrideLstat[name]; ok {
+		return fi, nil
+	}
+
+	return fs.FS.Lstat(name)
+}
+
+func (fs *StatFS) OpenFile(name string, flags int, perm os.FileMode) (fs.File, error) {
+	if fi, ok := fs.OverrideLstat[name]; ok {
+		f, err := fs.FS.OpenFile(name, flags, perm)
+		if err != nil {
+			return nil, err
+		}
+
+		wrappedFile := fileStat{
+			File: f,
+			fi:   fi,
+		}
+		return wrappedFile, nil
+	}
+
+	return fs.FS.OpenFile(name, flags, perm)
+}
+
+type fileStat struct {
+	fs.File
+	fi os.FileInfo
+}
+
+func (f fileStat) Stat() (os.FileInfo, error) {
+	return f.fi, nil
+}
+
+type wrappedFileInfo struct {
+	os.FileInfo
+	sys  interface{}
+	mode os.FileMode
+}
+
+func (fi wrappedFileInfo) Sys() interface{} {
+	return fi.sys
+}
+
+func (fi wrappedFileInfo) Mode() os.FileMode {
+	return fi.mode
+}
+
+func TestMetadataChanged(t *testing.T) {
+	files := TestDir{
+		"testfile": TestFile{
+			Content: "foo bar test file",
+		},
+	}
+
+	tempdir, repo, cleanup := prepareTempdirRepoSrc(t, files)
+	defer cleanup()
+
+	back := fs.TestChdir(t, tempdir)
+	defer back()
+
+	// get metadata
+	fi := lstat(t, "testfile")
+	want, err := restic.NodeFromFileInfo("testfile", fi)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	fs := &StatFS{
+		FS: fs.Local{},
+		OverrideLstat: map[string]os.FileInfo{
+			"testfile": fi,
+		},
+	}
+
+	snapshotID, node2 := snapshot(t, repo, fs, restic.ID{}, "testfile")
+
+	// set some values so we can then compare the nodes
+	want.Content = node2.Content
+	want.Path = ""
+	want.ExtendedAttributes = nil
+
+	// make sure that metadata was recorded successfully
+	if !cmp.Equal(want, node2) {
+		t.Fatalf("metadata does not match:\n%v", cmp.Diff(want, node2))
+	}
+
+	// modify the mode
+	stat, ok := fi.Sys().(*syscall.Stat_t)
+	if ok {
+		// change a few values
+		stat.Mode = 0400
+		stat.Uid = 1234
+		stat.Gid = 1235
+
+		// wrap the os.FileInfo so we can return a modified stat_t
+		fi = wrappedFileInfo{
+			FileInfo: fi,
+			sys:      stat,
+			mode:     0400,
+		}
+		fs.OverrideLstat["testfile"] = fi
+	} else {
+		// skip the test on this platform
+		t.Skipf("unable to modify os.FileInfo, Sys() returned %T", fi.Sys())
+	}
+
+	want, err = restic.NodeFromFileInfo("testfile", fi)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// make another snapshot
+	snapshotID, node3 := snapshot(t, repo, fs, snapshotID, "testfile")
+
+	// set some values so we can then compare the nodes
+	want.Content = node2.Content
+	want.Path = ""
+	want.ExtendedAttributes = nil
+
+	// make sure that metadata was recorded successfully
+	if !cmp.Equal(want, node3) {
+		t.Fatalf("metadata does not match:\n%v", cmp.Diff(want, node2))
+	}
+
+	// make sure the content matches
+	TestEnsureFileContent(context.Background(), t, repo, "testfile", node3, files["testfile"].(TestFile))
+
+	checker.TestCheckRepo(t, repo)
+}