Repository: restic/restic
Pull request #5099 (2024-10-18 20:41:08)

Hackport "backup: read extended metadata from snapshot"
<!--
Thank you very much for contributing code or documentation to restic! Please
fill out the following questions to make it easier for us to review your
changes.
-->

What does this PR change? What problem does it solve?
-----------------------------------------------------
Backport https://github.com/restic/restic/pull/5097 to the `patch-release` branch. The main different to that PR is that the commit "fs/vss: properly create node from vss path" is reimplemented using a horrible hack. This is necessary as a clean solution requires the FS interface refactoring from https://github.com/restic/restic/issues/5021.

`internal/fs` cannot import `internal/restic` as the latter already imports the former. Thus `NodeFromFileInfo` cannot be easily moved to `internal/fs` as in #5097. The call to `MapFilename` could in principle also be integrated into `NodeFromFileInfo`, however, this requires more test fixes.

The majority of the remaining commits are just cherry-picked from #5097.
<!--
Describe the changes and their purpose here, as detailed as needed.
-->

P.S. The "hackport" typo is intentional.

Was the change previously discussed in an issue or on the forum?
----------------------------------------------------------------
Related to https://github.com/restic/restic/issues/5063
<!--
Link issues and relevant forum posts here.

If this PR resolves an issue on GitHub, use "Closes #1234" so that the issue
is closed automatically when this PR is merged.
-->

Checklist
---------

<!--
You do not need to check all the boxes below all at once. Feel free to take
your time and add more commits. If you're done and ready for review, please
check the last box. Enable a checkbox by replacing [ ] with [x].
-->

- [x] I have read the [contribution guidelines](https://github.com/restic/restic/blob/master/CONTRIBUTING.md#providing-patches).
- [x] I have [enabled maintainer edits](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork).
- [x] I have added tests for all code changes.
- ~~[ ] I have added documentation for relevant changes (in the manual).~~
- [x] There's a new file in `changelog/unreleased/` that describes the changes for our users (see [template](https://github.com/restic/restic/blob/master/changelog/TEMPLATE)).
- [x] I have run `gofmt` on the code in all commits.
- [x] All commit messages are formatted in the same style as [the other commits in the repo](https://github.com/restic/restic/blob/master/CONTRIBUTING.md#git-commits).
- [x] I'm done! This pull request is ready for review.



Patch:
diff --git a/changelog/unreleased/issue-5063 b/changelog/unreleased/issue-5063
new file mode 100644
index 000000000..65aa379e4
--- /dev/null
+++ b/changelog/unreleased/issue-5063
@@ -0,0 +1,11 @@
+Bugfix: Correctly `backup` extended metadata when using VSS on Windows
+
+On Windows, when creating a backup using the `--use-fs-snapshot` option,
+then the extended metadata was not read from the filesystem snapshot. This
+could result in errors when files have been removed in the meantime.
+
+This issue has been resolved.
+
+https://github.com/restic/restic/issues/5063
+https://github.com/restic/restic/pull/5097
+https://github.com/restic/restic/pull/5099
diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go
index 562108a33..c7c0bcc50 100644
--- a/cmd/restic/cmd_backup.go
+++ b/cmd/restic/cmd_backup.go
@@ -95,6 +95,7 @@ type BackupOptions struct {
 }
 
 var backupOptions BackupOptions
+var backupFSTestHook func(fs fs.FS) fs.FS
 
 // ErrInvalidSourceData is used to report an incomplete backup
 var ErrInvalidSourceData = errors.New("at least one source file could not be read")
@@ -598,6 +599,10 @@ func runBackup(ctx context.Context, opts BackupOptions, gopts GlobalOptions, ter
 		targets = []string{filename}
 	}
 
+	if backupFSTestHook != nil {
+		targetFS = backupFSTestHook(targetFS)
+	}
+
 	wg, wgCtx := errgroup.WithContext(ctx)
 	cancelCtx, cancel := context.WithCancel(wgCtx)
 	defer cancel()
diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go
index 839320816..03b3b9986 100644
--- a/internal/archiver/archiver.go
+++ b/internal/archiver/archiver.go
@@ -248,7 +248,8 @@ func (arch *Archiver) trackItem(item string, previous, current *restic.Node, s I
 
 // nodeFromFileInfo returns the restic node from an os.FileInfo.
 func (arch *Archiver) nodeFromFileInfo(snPath, filename string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) {
-	node, err := restic.NodeFromFileInfo(filename, fi, ignoreXattrListError)
+	mappedFilename := arch.FS.MapFilename(filename)
+	node, err := restic.NodeFromFileInfo(mappedFilename, fi, ignoreXattrListError)
 	if !arch.WithAtime {
 		node.AccessTime = node.ModTime
 	}
diff --git a/internal/archiver/file_saver.go b/internal/archiver/file_saver.go
index d10334301..70666506d 100644
--- a/internal/archiver/file_saver.go
+++ b/internal/archiver/file_saver.go
@@ -156,7 +156,7 @@ func (s *FileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPat
 
 	debug.Log("%v", snPath)
 
-	node, err := s.NodeFromFileInfo(snPath, f.Name(), fi, false)
+	node, err := s.NodeFromFileInfo(snPath, target, fi, false)
 	if err != nil {
 		_ = f.Close()
 		completeError(err)
diff --git a/internal/fs/fs_local.go b/internal/fs/fs_local.go
index 48c40dc90..06dbae9a0 100644
--- a/internal/fs/fs_local.go
+++ b/internal/fs/fs_local.go
@@ -18,6 +18,12 @@ func (fs Local) VolumeName(path string) string {
 	return filepath.VolumeName(path)
 }
 
+// MapFilename is a temporary hack to prepare a filename for usage with
+// NodeFromFileInfo. This is only relevant for LocalVss.
+func (fs Local) MapFilename(filename string) string {
+	return filename
+}
+
 // Open opens a file for reading.
 func (fs Local) Open(name string) (File, error) {
 	f, err := os.Open(fixpath(name))
diff --git a/internal/fs/fs_local_vss.go b/internal/fs/fs_local_vss.go
index 718dfc46d..db6c95155 100644
--- a/internal/fs/fs_local_vss.go
+++ b/internal/fs/fs_local_vss.go
@@ -145,6 +145,12 @@ func (fs *LocalVss) Lstat(name string) (os.FileInfo, error) {
 	return os.Lstat(fs.snapshotPath(name))
 }
 
+// MapFilename is a temporary hack to prepare a filename for usage with
+// NodeFromFileInfo. This is only relevant for LocalVss.
+func (fs *LocalVss) MapFilename(filename string) string {
+	return fs.snapshotPath(filename)
+}
+
 // isMountPointIncluded  is true if given mountpoint included by user.
 func (fs *LocalVss) isMountPointIncluded(mountPoint string) bool {
 	if fs.excludeVolumes == nil {
diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go
index 47af74245..57864c87b 100644
--- a/internal/fs/fs_reader.go
+++ b/internal/fs/fs_reader.go
@@ -39,6 +39,12 @@ func (fs *Reader) VolumeName(_ string) string {
 	return ""
 }
 
+// MapFilename is a temporary hack to prepare a filename for usage with
+// NodeFromFileInfo. This is only relevant for LocalVss.
+func (fs *Reader) MapFilename(filename string) string {
+	return filename
+}
+
 // Open opens a file for reading.
 func (fs *Reader) Open(name string) (f File, err error) {
 	switch name {
@@ -223,7 +229,7 @@ func (r *readerFile) Close() error {
 var _ File = &readerFile{}
 
 // fakeFile implements all File methods, but only returns errors for anything
-// except Stat() and Name().
+// except Stat()
 type fakeFile struct {
 	name string
 	os.FileInfo
@@ -260,10 +266,6 @@ func (f fakeFile) Stat() (os.FileInfo, error) {
 	return f.FileInfo, nil
 }
 
-func (f fakeFile) Name() string {
-	return f.name
-}
-
 // fakeDir implements Readdirnames and Readdir, everything else is delegated to fakeFile.
 type fakeDir struct {
 	entries []os.FileInfo
diff --git a/internal/fs/interface.go b/internal/fs/interface.go
index b26c56944..147773e2d 100644
--- a/internal/fs/interface.go
+++ b/internal/fs/interface.go
@@ -11,6 +11,7 @@ type FS interface {
 	OpenFile(name string, flag int, perm os.FileMode) (File, error)
 	Stat(name string) (os.FileInfo, error)
 	Lstat(name string) (os.FileInfo, error)
+	MapFilename(filename string) string
 
 	Join(elem ...string) string
 	Separator() string
@@ -33,5 +34,4 @@ type File interface {
 	Readdir(int) ([]os.FileInfo, error)
 	Seek(int64, int) (int64, error)
 	Stat() (os.FileInfo, error)
-	Name() string
 }


Test patch:
diff --git a/cmd/restic/cmd_backup_integration_test.go b/cmd/restic/cmd_backup_integration_test.go
index 5e00b84b0..5926fdd54 100644
--- a/cmd/restic/cmd_backup_integration_test.go
+++ b/cmd/restic/cmd_backup_integration_test.go
@@ -8,6 +8,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"testing"
+	"time"
 
 	"github.com/restic/restic/internal/fs"
 	"github.com/restic/restic/internal/restic"
@@ -111,6 +112,63 @@ func TestBackupWithRelativePath(t *testing.T) {
 	rtest.Assert(t, latestSn.Parent != nil && latestSn.Parent.Equal(firstSnapshotID), "second snapshot selected unexpected parent %v instead of %v", latestSn.Parent, firstSnapshotID)
 }
 
+type vssDeleteOriginalFS struct {
+	fs.FS
+	testdata   string
+	hasRemoved bool
+}
+
+func (f *vssDeleteOriginalFS) Lstat(name string) (os.FileInfo, error) {
+	if !f.hasRemoved {
+		// call Lstat to trigger snapshot creation
+		_, _ = f.FS.Lstat(name)
+		// nuke testdata
+		var err error
+		for i := 0; i < 3; i++ {
+			// The CI sometimes runs into "The process cannot access the file because it is being used by another process" errors
+			// thus try a few times to remove the data
+			err = os.RemoveAll(f.testdata)
+			if err == nil {
+				break
+			}
+			time.Sleep(10 * time.Millisecond)
+		}
+		if err != nil {
+			return nil, err
+		}
+		f.hasRemoved = true
+	}
+	return f.FS.Lstat(name)
+}
+
+func TestBackupVSS(t *testing.T) {
+	if runtime.GOOS != "windows" || fs.HasSufficientPrivilegesForVSS() != nil {
+		t.Skip("vss fs test can only be run on windows with admin privileges")
+	}
+
+	env, cleanup := withTestEnvironment(t)
+	defer cleanup()
+
+	testSetupBackupData(t, env)
+	opts := BackupOptions{UseFsSnapshot: true}
+
+	var testFS *vssDeleteOriginalFS
+	backupFSTestHook = func(fs fs.FS) fs.FS {
+		testFS = &vssDeleteOriginalFS{
+			FS:       fs,
+			testdata: env.testdata,
+		}
+		return testFS
+	}
+	defer func() {
+		backupFSTestHook = nil
+	}()
+
+	testRunBackup(t, filepath.Dir(env.testdata), []string{"testdata"}, opts, env.gopts)
+	testListSnapshots(t, env.gopts, 1)
+	rtest.Equals(t, true, testFS.hasRemoved, "testdata was not removed")
+}
+
 func TestBackupParentSelection(t *testing.T) {
 	env, cleanup := withTestEnvironment(t)
 	defer cleanup()
diff --git a/cmd/restic/integration_helpers_test.go b/cmd/restic/integration_helpers_test.go
index 978deab3d..8ae3bb78a 100644
--- a/cmd/restic/integration_helpers_test.go
+++ b/cmd/restic/integration_helpers_test.go
@@ -9,6 +9,7 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"strings"
 	"sync"
 	"testing"
 
@@ -168,6 +169,16 @@ type testEnvironment struct {
 	gopts                                   GlobalOptions
 }
 
+type logOutputter struct {
+	t testing.TB
+}
+
+func (l *logOutputter) Write(p []byte) (n int, err error) {
+	l.t.Helper()
+	l.t.Log(strings.TrimSuffix(string(p), "\n"))
+	return len(p), nil
+}
+
 // withTestEnvironment creates a test environment and returns a cleanup
 // function which removes it.
 func withTestEnvironment(t testing.TB) (env *testEnvironment, cleanup func()) {
@@ -200,8 +211,11 @@ func withTestEnvironment(t testing.TB) (env *testEnvironment, cleanup func()) {
 		Quiet:    true,
 		CacheDir: env.cache,
 		password: rtest.TestPassword,
-		stdout:   os.Stdout,
-		stderr:   os.Stderr,
+		// stdout and stderr are written to by Warnf etc. That is the written data
+		// usually consists of one or multiple lines and therefore can be handled well
+		// by t.Log.
+		stdout:   &logOutputter{t},
+		stderr:   &logOutputter{t},
 		extended: make(options.Options),
 
 		// replace this hook with "nil" if listing a filetype more than once is necessary