From 9abef3bf1a4d28f8195c8a470cf267050832d67a Mon Sep 17 00:00:00 2001 From: greatroar <@> Date: Mon, 17 Feb 2020 13:24:09 +0100 Subject: [PATCH 1/7] Move internal/fs.TestChdir to internal/test.Chdir --- cmd/restic/integration_test.go | 5 ++--- internal/archiver/archiver_test.go | 18 +++++++++--------- internal/archiver/scanner_test.go | 6 +++--- internal/archiver/testing_test.go | 2 +- internal/archiver/tree_test.go | 2 +- internal/dump/tar_test.go | 2 +- internal/fs/helpers.go | 25 ------------------------- internal/restorer/restorer_test.go | 2 +- internal/test/helpers.go | 26 ++++++++++++++++++++++++++ 9 files changed, 44 insertions(+), 44 deletions(-) diff --git a/cmd/restic/integration_test.go b/cmd/restic/integration_test.go index b279a75b0..9d1595e43 100644 --- a/cmd/restic/integration_test.go +++ b/cmd/restic/integration_test.go @@ -21,7 +21,6 @@ import ( "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/filter" - "github.com/restic/restic/internal/fs" "github.com/restic/restic/internal/repository" "github.com/restic/restic/internal/restic" rtest "github.com/restic/restic/internal/test" @@ -66,7 +65,7 @@ func testRunBackupAssumeFailure(t testing.TB, dir string, target []string, opts gopts.stdout = ioutil.Discard t.Logf("backing up %v in %v", target, dir) if dir != "" { - cleanup := fs.TestChdir(t, dir) + cleanup := rtest.Chdir(t, dir) defer cleanup() } @@ -1003,7 +1002,7 @@ func TestRestoreLatest(t *testing.T) { // chdir manually here so we can get the current directory. This is not the // same as the temp dir returned by ioutil.TempDir() on darwin. - back := fs.TestChdir(t, filepath.Dir(env.testdata)) + back := rtest.Chdir(t, filepath.Dir(env.testdata)) defer back() curdir, err := os.Getwd() diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index 6fc5ab648..643da5d24 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -803,7 +803,7 @@ func TestArchiverSaveDir(t *testing.T) { chdir = filepath.Join(chdir, test.chdir) } - back := fs.TestChdir(t, chdir) + back := restictest.Chdir(t, chdir) defer back() fi, err := fs.Lstat(test.target) @@ -1063,7 +1063,7 @@ func TestArchiverSaveTree(t *testing.T) { arch.runWorkers(ctx, &tmb) - back := fs.TestChdir(t, tempdir) + back := restictest.Chdir(t, tempdir) defer back() if test.prepare != nil { @@ -1353,7 +1353,7 @@ func TestArchiverSnapshot(t *testing.T) { chdir = filepath.Join(chdir, filepath.FromSlash(test.chdir)) } - back := fs.TestChdir(t, chdir) + back := restictest.Chdir(t, chdir) defer back() var targets []string @@ -1507,7 +1507,7 @@ func TestArchiverSnapshotSelect(t *testing.T) { arch := New(repo, fs.Track{FS: fs.Local{}}, Options{}) arch.Select = test.selFn - back := fs.TestChdir(t, tempdir) + back := restictest.Chdir(t, tempdir) defer back() targets := []string{"."} @@ -1614,7 +1614,7 @@ func TestArchiverParent(t *testing.T) { arch := New(repo, testFS, Options{}) - back := fs.TestChdir(t, tempdir) + back := restictest.Chdir(t, tempdir) defer back() _, firstSnapshotID, err := arch.Snapshot(ctx, []string{"."}, SnapshotOptions{Time: time.Now()}) @@ -1774,7 +1774,7 @@ func TestArchiverErrorReporting(t *testing.T) { tempdir, repo, cleanup := prepareTempdirRepoSrc(t, test.src) defer cleanup() - back := fs.TestChdir(t, tempdir) + back := restictest.Chdir(t, tempdir) defer back() if test.prepare != nil { @@ -1915,7 +1915,7 @@ func TestArchiverAbortEarlyOnError(t *testing.T) { tempdir, repo, cleanup := prepareTempdirRepoSrc(t, test.src) defer cleanup() - back := fs.TestChdir(t, tempdir) + back := restictest.Chdir(t, tempdir) defer back() testFS := &TrackFS{ @@ -2046,7 +2046,7 @@ func TestMetadataChanged(t *testing.T) { tempdir, repo, cleanup := prepareTempdirRepoSrc(t, files) defer cleanup() - back := fs.TestChdir(t, tempdir) + back := restictest.Chdir(t, tempdir) defer back() // get metadata @@ -2121,7 +2121,7 @@ func TestRacyFileSwap(t *testing.T) { tempdir, repo, cleanup := prepareTempdirRepoSrc(t, files) defer cleanup() - back := fs.TestChdir(t, tempdir) + back := restictest.Chdir(t, tempdir) defer back() // get metadata of current folder diff --git a/internal/archiver/scanner_test.go b/internal/archiver/scanner_test.go index a171df5f6..3d4cfacdd 100644 --- a/internal/archiver/scanner_test.go +++ b/internal/archiver/scanner_test.go @@ -88,7 +88,7 @@ func TestScanner(t *testing.T) { TestCreateFiles(t, tempdir, test.src) - back := fs.TestChdir(t, tempdir) + back := restictest.Chdir(t, tempdir) defer back() cur, err := os.Getwd() @@ -225,7 +225,7 @@ func TestScannerError(t *testing.T) { TestCreateFiles(t, tempdir, test.src) - back := fs.TestChdir(t, tempdir) + back := restictest.Chdir(t, tempdir) defer back() cur, err := os.Getwd() @@ -299,7 +299,7 @@ func TestScannerCancel(t *testing.T) { TestCreateFiles(t, tempdir, src) - back := fs.TestChdir(t, tempdir) + back := restictest.Chdir(t, tempdir) defer back() cur, err := os.Getwd() diff --git a/internal/archiver/testing_test.go b/internal/archiver/testing_test.go index 5d9c43363..e11b86250 100644 --- a/internal/archiver/testing_test.go +++ b/internal/archiver/testing_test.go @@ -495,7 +495,7 @@ func TestTestEnsureSnapshot(t *testing.T) { createFilesAt(t, targetDir, test.files) - back := fs.TestChdir(t, tempdir) + back := restictest.Chdir(t, tempdir) defer back() repo, cleanup := repository.TestRepository(t) diff --git a/internal/archiver/tree_test.go b/internal/archiver/tree_test.go index 00d0b6b82..73ace7bfc 100644 --- a/internal/archiver/tree_test.go +++ b/internal/archiver/tree_test.go @@ -444,7 +444,7 @@ func TestTree(t *testing.T) { TestCreateFiles(t, tempdir, test.src) - back := fs.TestChdir(t, tempdir) + back := restictest.Chdir(t, tempdir) defer back() tree, err := NewTree(fs.Local{}, test.targets) diff --git a/internal/dump/tar_test.go b/internal/dump/tar_test.go index fddbef7f0..a8ce336fd 100644 --- a/internal/dump/tar_test.go +++ b/internal/dump/tar_test.go @@ -79,7 +79,7 @@ func TestWriteTar(t *testing.T) { arch := archiver.New(repo, fs.Track{FS: fs.Local{}}, archiver.Options{}) - back := fs.TestChdir(t, tmpdir) + back := rtest.Chdir(t, tmpdir) defer back() sn, _, err := arch.Snapshot(ctx, []string{"."}, archiver.SnapshotOptions{}) diff --git a/internal/fs/helpers.go b/internal/fs/helpers.go index 787fe485a..7ce24afe9 100644 --- a/internal/fs/helpers.go +++ b/internal/fs/helpers.go @@ -16,31 +16,6 @@ func IsRegularFile(fi os.FileInfo) bool { return fi.Mode()&(os.ModeType|os.ModeCharDevice) == 0 } -// TestChdir changes the current directory to dest, the function back returns to the previous directory. -func TestChdir(t testing.TB, dest string) (back func()) { - t.Helper() - - prev, err := os.Getwd() - if err != nil { - t.Fatal(err) - } - - t.Logf("chdir to %v", dest) - err = os.Chdir(dest) - if err != nil { - t.Fatal(err) - } - - return func() { - t.Helper() - t.Logf("chdir back to %v", prev) - err = os.Chdir(prev) - if err != nil { - t.Fatal(err) - } - } -} - // TestTempFile returns a new temporary file, which is removed when cleanup() // is called. func TestTempFile(t testing.TB, prefix string) (File, func()) { diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 333d4598b..00c56bccd 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -442,7 +442,7 @@ func TestRestorerRelative(t *testing.T) { tempdir, cleanup := rtest.TempDir(t) defer cleanup() - cleanup = fs.TestChdir(t, tempdir) + cleanup = rtest.Chdir(t, tempdir) defer cleanup() errors := make(map[string]string) diff --git a/internal/test/helpers.go b/internal/test/helpers.go index 785189598..cd2d215cf 100644 --- a/internal/test/helpers.go +++ b/internal/test/helpers.go @@ -202,3 +202,29 @@ func TempDir(t testing.TB) (path string, cleanup func()) { RemoveAll(t, tempdir) } } + +// Chdir changes the current directory to dest. +// The function back returns to the previous directory. +func Chdir(t testing.TB, dest string) (back func()) { + t.Helper() + + prev, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + + t.Logf("chdir to %v", dest) + err = os.Chdir(dest) + if err != nil { + t.Fatal(err) + } + + return func() { + t.Helper() + t.Logf("chdir back to %v", prev) + err = os.Chdir(prev) + if err != nil { + t.Fatal(err) + } + } +} From 1b20f6beecc0ffb58f8f8448407bdb9bb857e14b Mon Sep 17 00:00:00 2001 From: greatroar <@> Date: Mon, 17 Feb 2020 00:20:38 +0100 Subject: [PATCH 2/7] Remove io.Writer from fs.File It was only used in a single test, which now uses plain *os.File instead. --- internal/fs/fs_reader.go | 4 ---- internal/fs/helpers.go | 25 +------------------- internal/fs/interface.go | 1 - internal/textfile/read_test.go | 43 ++++++++++++++++++++++++---------- 4 files changed, 32 insertions(+), 41 deletions(-) diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index 91fedb263..d991951aa 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -252,10 +252,6 @@ func (f fakeFile) Seek(int64, int) (int64, error) { return 0, os.ErrInvalid } -func (f fakeFile) Write(p []byte) (int, error) { - return 0, os.ErrInvalid -} - func (f fakeFile) Read(p []byte) (int, error) { return 0, os.ErrInvalid } diff --git a/internal/fs/helpers.go b/internal/fs/helpers.go index 7ce24afe9..b7f7ad6ba 100644 --- a/internal/fs/helpers.go +++ b/internal/fs/helpers.go @@ -1,10 +1,6 @@ package fs -import ( - "io/ioutil" - "os" - "testing" -) +import "os" // IsRegularFile returns true if fi belongs to a normal file. If fi is nil, // false is returned. @@ -15,22 +11,3 @@ func IsRegularFile(fi os.FileInfo) bool { return fi.Mode()&(os.ModeType|os.ModeCharDevice) == 0 } - -// TestTempFile returns a new temporary file, which is removed when cleanup() -// is called. -func TestTempFile(t testing.TB, prefix string) (File, func()) { - f, err := ioutil.TempFile("", prefix) - if err != nil { - t.Fatal(err) - } - - cleanup := func() { - _ = f.Close() - err = Remove(f.Name()) - if err != nil { - t.Fatal(err) - } - } - - return f, cleanup -} diff --git a/internal/fs/interface.go b/internal/fs/interface.go index 1c2260215..b26c56944 100644 --- a/internal/fs/interface.go +++ b/internal/fs/interface.go @@ -26,7 +26,6 @@ type FS interface { // File is an open file on a file system. type File interface { io.Reader - io.Writer io.Closer Fd() uintptr diff --git a/internal/textfile/read_test.go b/internal/textfile/read_test.go index 572a33ebe..8e8e659dc 100644 --- a/internal/textfile/read_test.go +++ b/internal/textfile/read_test.go @@ -3,25 +3,44 @@ package textfile import ( "bytes" "encoding/hex" + "io/ioutil" + "os" "testing" - - "github.com/restic/restic/internal/fs" ) -func writeTempfile(t testing.TB, data []byte) (fs.File, func()) { - f, removeTempfile := fs.TestTempFile(t, "restic-test-textfile-read-") +// writeTempfile writes data to a new temporary file and returns its name +// and a callback that removes it. +func writeTempfile(t testing.TB, data []byte) (string, func()) { + t.Helper() - _, err := f.Write(data) + f, err := ioutil.TempFile("", "restic-test-textfile-read-") + if err != nil { + t.Fatal(err) + } + name := f.Name() + + defer func() { + closeErr := f.Close() + if err == nil && closeErr != nil { + err = closeErr + } + if err != nil { + os.Remove(name) + t.Fatal(err) + } + }() + + _, err = f.Write(data) if err != nil { t.Fatal(err) } - err = f.Close() - if err != nil { - t.Fatal(err) + return name, func() { + err := os.Remove(name) + if err != nil { + t.Fatal(err) + } } - - return f, removeTempfile } func dec(s string) []byte { @@ -60,10 +79,10 @@ func TestRead(t *testing.T) { want = test.data } - f, cleanup := writeTempfile(t, test.data) + tempname, cleanup := writeTempfile(t, test.data) defer cleanup() - data, err := Read(f.Name()) + data, err := Read(tempname) if err != nil { t.Fatal(err) } From c504aa505c3155fb67353fb85cd0cca62f465a89 Mon Sep 17 00:00:00 2001 From: greatroar <@> Date: Mon, 2 Mar 2020 09:34:56 +0100 Subject: [PATCH 3/7] Remove unused fs.Rename --- internal/fs/file.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/internal/fs/file.go b/internal/fs/file.go index 86c519aff..f1ff451ed 100644 --- a/internal/fs/file.go +++ b/internal/fs/file.go @@ -32,14 +32,6 @@ func RemoveAll(path string) error { return os.RemoveAll(fixpath(path)) } -// Rename renames (moves) oldpath to newpath. -// If newpath already exists, Rename replaces it. -// OS-specific restrictions may apply when oldpath and newpath are in different directories. -// If there is an error, it will be of type *LinkError. -func Rename(oldpath, newpath string) error { - return os.Rename(fixpath(oldpath), fixpath(newpath)) -} - // Symlink creates newname as a symbolic link to oldname. // If there is an error, it will be of type *LinkError. func Symlink(oldname, newname string) error { From 1557c58eef9a029d2cb9fa113e457042a8bc1d3c Mon Sep 17 00:00:00 2001 From: greatroar <@> Date: Mon, 2 Mar 2020 21:33:39 +0100 Subject: [PATCH 4/7] Fix and simplify fs.Reader fakeDir.{Readdir,Readdirnames} weren't handling the case n == 0 correctly. fakeFileInfo.sys is always nil, so omit the field. --- internal/fs/fs_reader.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index d991951aa..73aca3230 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -275,7 +275,7 @@ type fakeDir struct { } func (d fakeDir) Readdirnames(n int) ([]string, error) { - if n >= 0 { + if n > 0 { return nil, errors.New("not implemented") } names := make([]string, 0, len(d.entries)) @@ -287,7 +287,7 @@ func (d fakeDir) Readdirnames(n int) ([]string, error) { } func (d fakeDir) Readdir(n int) ([]os.FileInfo, error) { - if n >= 0 { + if n > 0 { return nil, errors.New("not implemented") } return d.entries, nil @@ -299,7 +299,6 @@ type fakeFileInfo struct { size int64 mode os.FileMode modtime time.Time - sys interface{} } func (fi fakeFileInfo) Name() string { @@ -323,5 +322,5 @@ func (fi fakeFileInfo) IsDir() bool { } func (fi fakeFileInfo) Sys() interface{} { - return fi.sys + return nil } From 59b343a9bf4b62cec4cccb3350bf390612d339b9 Mon Sep 17 00:00:00 2001 From: greatroar <@> Date: Mon, 2 Mar 2020 21:54:53 +0100 Subject: [PATCH 5/7] Remove OS-specific versions of fs.MkdirAll Go has supported Windows paths correctly since 1.11, see https://github.com/golang/go/issues/10900 and the commit referenced there. --- internal/fs/file.go | 8 ++++++ internal/fs/file_unix.go | 8 ------ internal/fs/file_windows.go | 57 ------------------------------------- 3 files changed, 8 insertions(+), 65 deletions(-) diff --git a/internal/fs/file.go b/internal/fs/file.go index f1ff451ed..e438857b2 100644 --- a/internal/fs/file.go +++ b/internal/fs/file.go @@ -12,6 +12,14 @@ func Mkdir(name string, perm os.FileMode) error { return os.Mkdir(fixpath(name), perm) } +// MkdirAll creates a directory named path, along with any necessary parents, +// and returns nil, or else returns an error. The permission bits perm are used +// for all directories that MkdirAll creates. If path is already a directory, +// MkdirAll does nothing and returns nil. +func MkdirAll(path string, perm os.FileMode) error { + return os.MkdirAll(fixpath(path), perm) +} + // Readlink returns the destination of the named symbolic link. // If there is an error, it will be of type *PathError. func Readlink(name string) (string, error) { diff --git a/internal/fs/file_unix.go b/internal/fs/file_unix.go index 612465670..f5ea36696 100644 --- a/internal/fs/file_unix.go +++ b/internal/fs/file_unix.go @@ -14,14 +14,6 @@ func fixpath(name string) string { return name } -// MkdirAll creates a directory named path, along with any necessary parents, -// and returns nil, or else returns an error. The permission bits perm are used -// for all directories that MkdirAll creates. If path is already a directory, -// MkdirAll does nothing and returns nil. -func MkdirAll(path string, perm os.FileMode) error { - return os.MkdirAll(fixpath(path), perm) -} - // TempFile creates a temporary file which has already been deleted (on // supported platforms) func TempFile(dir, prefix string) (f *os.File, err error) { diff --git a/internal/fs/file_windows.go b/internal/fs/file_windows.go index dd53cde5e..8a4d01fb0 100644 --- a/internal/fs/file_windows.go +++ b/internal/fs/file_windows.go @@ -5,7 +5,6 @@ import ( "os" "path/filepath" "strings" - "syscall" ) // fixpath returns an absolute path on windows, so restic can open long file @@ -31,62 +30,6 @@ func fixpath(name string) string { return name } -// MkdirAll creates a directory named path, along with any necessary parents, -// and returns nil, or else returns an error. The permission bits perm are used -// for all directories that MkdirAll creates. If path is already a directory, -// MkdirAll does nothing and returns nil. -// -// Adapted from the stdlib MkdirAll, added test for volume name. -func MkdirAll(path string, perm os.FileMode) error { - // Fast path: if we can tell whether path is a directory or file, stop with success or error. - dir, err := os.Stat(path) - if err == nil { - if dir.IsDir() { - return nil - } - return &os.PathError{ - Op: "mkdir", - Path: path, - Err: syscall.ENOTDIR, - } - } - - // Slow path: make sure parent exists and then call Mkdir for path. - i := len(path) - for i > 0 && os.IsPathSeparator(path[i-1]) { // Skip trailing path separator. - i-- - } - - j := i - for j > 0 && !os.IsPathSeparator(path[j-1]) { // Scan backward over element. - j-- - } - - if j > 1 { - // Create parent - parent := path[0 : j-1] - if parent != filepath.VolumeName(parent) { - err = MkdirAll(parent, perm) - if err != nil { - return err - } - } - } - - // Parent now exists; invoke Mkdir and use its result. - err = os.Mkdir(path, perm) - if err != nil { - // Handle arguments like "foo/." by - // double-checking that directory doesn't exist. - dir, err1 := os.Lstat(path) - if err1 == nil && dir.IsDir() { - return nil - } - return err - } - return nil -} - // TempFile creates a temporary file. func TempFile(dir, prefix string) (f *os.File, err error) { return ioutil.TempFile(dir, prefix) From 95ebba85ff7e755e1f72c3e304f9ae9766efe9c6 Mon Sep 17 00:00:00 2001 From: greatroar <@> Date: Thu, 17 Sep 2020 10:11:31 +0200 Subject: [PATCH 6/7] Remove stray Printf from internal/fs --- internal/fs/fs_reader.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index 73aca3230..dac4aac9e 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -1,7 +1,6 @@ package fs import ( - "fmt" "io" "os" "path" @@ -208,7 +207,6 @@ func (r *readerFile) Read(p []byte) (int, error) { // return an error if we did not read any data if err == io.EOF && !r.AllowEmptyFile && !r.bytesRead { - fmt.Printf("reader: %d bytes read, err %v, bytesRead %v, allowEmpty %v\n", n, err, r.bytesRead, r.AllowEmptyFile) return n, &os.PathError{ Path: r.fakeFile.name, Op: "read", From 0d65b78168c01fba25f539d56b23a6004e9b3b57 Mon Sep 17 00:00:00 2001 From: greatroar <@> Date: Thu, 17 Sep 2020 10:22:15 +0200 Subject: [PATCH 7/7] Simplify os.ModeType|os.ModeCharDevice => os.ModeType Since Go 1.12, ModeCharDevice is included in ModeType: golang/go@a2a3dd00c934fa15ad880ee5fe1f64308cbc73a7 --- internal/fs/helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/fs/helpers.go b/internal/fs/helpers.go index b7f7ad6ba..4dd1e0e73 100644 --- a/internal/fs/helpers.go +++ b/internal/fs/helpers.go @@ -9,5 +9,5 @@ func IsRegularFile(fi os.FileInfo) bool { return false } - return fi.Mode()&(os.ModeType|os.ModeCharDevice) == 0 + return fi.Mode()&os.ModeType == 0 }