From 18cc7a663b63e8eaec036eee7b5b07fa4b95ec46 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Tue, 16 Aug 2016 10:01:58 +0000 Subject: [PATCH] lib: Remove osutil.Remove & osutil.RemoveAll (fixes #3513) These are no longer required with Go 1.7. Change made by removing the functions, doing a global s/osutil.Remove/os.Remove/. GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3514 --- lib/fs/mtimefs_test.go | 6 +-- lib/model/model_test.go | 8 ++-- lib/model/rwfolder.go | 20 ++++----- lib/model/sorter.go | 4 +- lib/osutil/osutil.go | 88 -------------------------------------- lib/osutil/osutil_test.go | 47 ++++++++++++++++---- lib/scanner/walk_test.go | 4 +- lib/versioner/staggered.go | 2 +- lib/versioner/trashcan.go | 6 +-- test/util.go | 2 +- 10 files changed, 64 insertions(+), 123 deletions(-) diff --git a/lib/fs/mtimefs_test.go b/lib/fs/mtimefs_test.go index a0b860a5d..57c841d4b 100644 --- a/lib/fs/mtimefs_test.go +++ b/lib/fs/mtimefs_test.go @@ -12,13 +12,11 @@ import ( "os" "testing" "time" - - "github.com/syncthing/syncthing/lib/osutil" ) func TestMtimeFS(t *testing.T) { - osutil.RemoveAll("testdata") - defer osutil.RemoveAll("testdata") + os.RemoveAll("testdata") + defer os.RemoveAll("testdata") os.Mkdir("testdata", 0755) ioutil.WriteFile("testdata/exists0", []byte("hello"), 0644) ioutil.WriteFile("testdata/exists1", []byte("hello"), 0644) diff --git a/lib/model/model_test.go b/lib/model/model_test.go index c6c89643b..e244aa3ba 100644 --- a/lib/model/model_test.go +++ b/lib/model/model_test.go @@ -1343,8 +1343,8 @@ func TestIssue3028(t *testing.T) { } func TestIssue3164(t *testing.T) { - osutil.RemoveAll("testdata/issue3164") - defer osutil.RemoveAll("testdata/issue3164") + os.RemoveAll("testdata/issue3164") + defer os.RemoveAll("testdata/issue3164") if err := os.MkdirAll("testdata/issue3164/oktodelete/foobar", 0777); err != nil { t.Fatal(err) @@ -1445,7 +1445,7 @@ func TestIssue2782(t *testing.T) { testName := ".syncthing-test." + srand.String(16) testDir := filepath.Join(home, testName) - if err := osutil.RemoveAll(testDir); err != nil { + if err := os.RemoveAll(testDir); err != nil { t.Skip(err) } if err := osutil.MkdirAll(testDir+"/syncdir", 0755); err != nil { @@ -1457,7 +1457,7 @@ func TestIssue2782(t *testing.T) { if err := os.Symlink("syncdir", testDir+"/synclink"); err != nil { t.Skip(err) } - defer osutil.RemoveAll(testDir) + defer os.RemoveAll(testDir) db := db.OpenMemory() m := NewModel(defaultConfig, protocol.LocalDeviceID, "device", "syncthing", "dev", db, nil) diff --git a/lib/model/rwfolder.go b/lib/model/rwfolder.go index dcaf09115..8b73f886a 100644 --- a/lib/model/rwfolder.go +++ b/lib/model/rwfolder.go @@ -602,7 +602,7 @@ func (f *rwFolder) handleDir(file protocol.FileInfo) { // Most likely a file/link is getting replaced with a directory. // Remove the file/link and fall through to directory creation. case err == nil && (!info.IsDir() || info.Mode()&os.ModeSymlink != 0): - err = osutil.InWritableDir(osutil.Remove, realName) + err = osutil.InWritableDir(os.Remove, realName) if err != nil { l.Infof("Puller (folder %q, dir %q): %v", f.folderID, file.Name, err) f.newError(file.Name, err) @@ -687,13 +687,13 @@ func (f *rwFolder) deleteDir(file protocol.FileInfo, matcher *ignore.Matcher) { for _, dirFile := range files { fullDirFile := filepath.Join(file.Name, dirFile) if defTempNamer.IsTemporary(dirFile) || (matcher != nil && matcher.Match(fullDirFile).IsDeletable()) { - osutil.RemoveAll(filepath.Join(f.dir, fullDirFile)) + os.RemoveAll(filepath.Join(f.dir, fullDirFile)) } } dir.Close() } - err = osutil.InWritableDir(osutil.Remove, realName) + err = osutil.InWritableDir(os.Remove, realName) if err == nil || os.IsNotExist(err) { // It was removed or it doesn't exist to start with f.dbUpdates <- dbUpdateJob{file, dbUpdateDeleteDir} @@ -740,7 +740,7 @@ func (f *rwFolder) deleteFile(file protocol.FileInfo) { } else if f.versioner != nil { err = osutil.InWritableDir(f.versioner.Archive, realName) } else { - err = osutil.InWritableDir(osutil.Remove, realName) + err = osutil.InWritableDir(os.Remove, realName) } if err == nil || os.IsNotExist(err) { @@ -825,7 +825,7 @@ func (f *rwFolder) renameFile(source, target protocol.FileInfo) { // get rid of. Attempt to delete it instead so that we make *some* // progress. The target is unhandled. - err = osutil.InWritableDir(osutil.Remove, from) + err = osutil.InWritableDir(os.Remove, from) if err != nil { l.Infof("Puller (folder %q, file %q): delete %q after failed rename: %v", f.folderID, target.Name, source.Name, err) f.newError(target.Name, err) @@ -976,7 +976,7 @@ func (f *rwFolder) handleFile(file protocol.FileInfo, copyChan chan<- copyBlocks // Otherwise, discard the file ourselves in order for the // sharedpuller not to panic when it fails to exclusively create a // file which already exists - osutil.InWritableDir(osutil.Remove, tempName) + osutil.InWritableDir(os.Remove, tempName) } } else { // Copy the blocks, as we don't want to shuffle them on the FileInfo @@ -1262,7 +1262,7 @@ func (f *rwFolder) performFinish(state *sharedPullerState) error { // and future hard ignores before attempting a directory delete. // Should share code with f.deletDir(). - if err = osutil.InWritableDir(osutil.Remove, state.realName); err != nil { + if err = osutil.InWritableDir(os.Remove, state.realName); err != nil { return err } @@ -1458,14 +1458,14 @@ func removeAvailability(availabilities []Availability, availability Availability func (f *rwFolder) moveForConflict(name string) error { if strings.Contains(filepath.Base(name), ".sync-conflict-") { l.Infoln("Conflict for", name, "which is already a conflict copy; not copying again.") - if err := osutil.Remove(name); err != nil && !os.IsNotExist(err) { + if err := os.Remove(name); err != nil && !os.IsNotExist(err) { return err } return nil } if f.maxConflicts == 0 { - if err := osutil.Remove(name); err != nil && !os.IsNotExist(err) { + if err := os.Remove(name); err != nil && !os.IsNotExist(err) { return err } return nil @@ -1487,7 +1487,7 @@ func (f *rwFolder) moveForConflict(name string) error { if gerr == nil && len(matches) > f.maxConflicts { sort.Sort(sort.Reverse(sort.StringSlice(matches))) for _, match := range matches[f.maxConflicts:] { - gerr = osutil.Remove(match) + gerr = os.Remove(match) if gerr != nil { l.Debugln(f, "removing extra conflict", gerr) } diff --git a/lib/model/sorter.go b/lib/model/sorter.go index b488e8ba7..de33c5c13 100644 --- a/lib/model/sorter.go +++ b/lib/model/sorter.go @@ -9,9 +9,9 @@ package model import ( "encoding/binary" "io/ioutil" + "os" "sort" - "github.com/syncthing/syncthing/lib/osutil" "github.com/syncthing/syncthing/lib/protocol" "github.com/syndtr/goleveldb/leveldb" "github.com/syndtr/goleveldb/leveldb/opt" @@ -186,7 +186,7 @@ func (s *onDiskIndexSorter) Sorted(fn func(protocol.FileInfo) bool) { func (s *onDiskIndexSorter) Close() { l.Debugf("onDiskIndexSorter %p closes", s) s.db.Close() - osutil.RemoveAll(s.dir) + os.RemoveAll(s.dir) } func (s *onDiskIndexSorter) full() bool { diff --git a/lib/osutil/osutil.go b/lib/osutil/osutil.go index f2ecd07d8..a2ab05c5a 100644 --- a/lib/osutil/osutil.go +++ b/lib/osutil/osutil.go @@ -15,7 +15,6 @@ import ( "path/filepath" "runtime" "strings" - "syscall" "github.com/calmh/du" "github.com/syncthing/syncthing/lib/sync" @@ -93,93 +92,6 @@ func InWritableDir(fn func(string) error, path string) error { return fn(path) } -// Remove removes the given path. On Windows, removes the read-only attribute -// from the target prior to deletion. -func Remove(path string) error { - if runtime.GOOS == "windows" { - info, err := os.Stat(path) - if err != nil { - return err - } - if info.Mode()&0200 == 0 { - os.Chmod(path, 0700) - } - } - return os.Remove(path) -} - -// RemoveAll is a copy of os.RemoveAll, but uses osutil.Remove. -// RemoveAll removes path and any children it contains. -// It removes everything it can but returns the first error -// it encounters. If the path does not exist, RemoveAll -// returns nil (no error). -func RemoveAll(path string) error { - // Simple case: if Remove works, we're done. - err := Remove(path) - if err == nil || os.IsNotExist(err) { - return nil - } - - // Otherwise, is this a directory we need to recurse into? - dir, serr := os.Lstat(path) - if serr != nil { - if serr, ok := serr.(*os.PathError); ok && (os.IsNotExist(serr.Err) || serr.Err == syscall.ENOTDIR) { - return nil - } - return serr - } - if !dir.IsDir() { - // Not a directory; return the error from Remove. - return err - } - - // Directory. - fd, err := os.Open(path) - if err != nil { - if os.IsNotExist(err) { - // Race. It was deleted between the Lstat and Open. - // Return nil per RemoveAll's docs. - return nil - } - return err - } - - // Remove contents & return first error. - err = nil - for { - names, err1 := fd.Readdirnames(100) - for _, name := range names { - err1 := RemoveAll(path + string(os.PathSeparator) + name) - if err == nil { - err = err1 - } - } - if err1 == io.EOF { - break - } - // If Readdirnames returned an error, use it. - if err == nil { - err = err1 - } - if len(names) == 0 { - break - } - } - - // Close directory, because windows won't remove opened directory. - fd.Close() - - // Remove directory. - err1 := Remove(path) - if err1 == nil || os.IsNotExist(err1) { - return nil - } - if err == nil { - err = err1 - } - return err -} - func ExpandTilde(path string) (string, error) { if path == "~" { return getHomeDir() diff --git a/lib/osutil/osutil_test.go b/lib/osutil/osutil_test.go index 96f0d98a2..e5b54a95e 100644 --- a/lib/osutil/osutil_test.go +++ b/lib/osutil/osutil_test.go @@ -71,6 +71,8 @@ func TestInWriteableDir(t *testing.T) { } func TestInWritableDirWindowsRemove(t *testing.T) { + // os.Remove should remove read only things on windows + if runtime.GOOS != "windows" { t.Skipf("Tests not required") return @@ -100,20 +102,49 @@ func TestInWritableDirWindowsRemove(t *testing.T) { os.Chmod("testdata/windows/ro/readonly", 0500) for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} { - err := os.Remove(path) - if err == nil { - t.Errorf("Expected error %s", path) - } - } - - for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} { - err := osutil.InWritableDir(osutil.Remove, path) + err := osutil.InWritableDir(os.Remove, path) if err != nil { t.Errorf("Unexpected error %s: %s", path, err) } } } +func TestInWritableDirWindowsRemoveAll(t *testing.T) { + // os.RemoveAll should remove read only things on windows + + if runtime.GOOS != "windows" { + t.Skipf("Tests not required") + return + } + + err := os.RemoveAll("testdata") + if err != nil { + t.Fatal(err) + } + defer os.Chmod("testdata/windows/ro/readonlynew", 0700) + defer os.RemoveAll("testdata") + + create := func(name string) error { + fd, err := os.Create(name) + if err != nil { + return err + } + fd.Close() + return nil + } + + os.Mkdir("testdata", 0700) + + os.Mkdir("testdata/windows", 0500) + os.Mkdir("testdata/windows/ro", 0500) + create("testdata/windows/ro/readonly") + os.Chmod("testdata/windows/ro/readonly", 0500) + + if err := os.RemoveAll("testdata/windows"); err != nil { + t.Errorf("Unexpected error: %s", err) + } +} + func TestInWritableDirWindowsRename(t *testing.T) { if runtime.GOOS != "windows" { t.Skipf("Tests not required") diff --git a/lib/scanner/walk_test.go b/lib/scanner/walk_test.go index 489846e3b..469a5fe7d 100644 --- a/lib/scanner/walk_test.go +++ b/lib/scanner/walk_test.go @@ -289,8 +289,8 @@ func TestWalkSymlink(t *testing.T) { // Create a folder with a symlink in it - osutil.RemoveAll("_symlinks") - defer osutil.RemoveAll("_symlinks") + os.RemoveAll("_symlinks") + defer os.RemoveAll("_symlinks") os.Mkdir("_symlinks", 0755) symlinks.Create("_symlinks/link", "destination", symlinks.TargetUnknown) diff --git a/lib/versioner/staggered.go b/lib/versioner/staggered.go index 2517b34d2..4dcacbcd7 100644 --- a/lib/versioner/staggered.go +++ b/lib/versioner/staggered.go @@ -165,7 +165,7 @@ func (v Staggered) expire(versions []string) { continue } - if err := osutil.Remove(file); err != nil { + if err := os.Remove(file); err != nil { l.Warnf("Versioner: can't remove %q: %v", file, err) } } diff --git a/lib/versioner/trashcan.go b/lib/versioner/trashcan.go index 144a6388c..edb1ea9d8 100644 --- a/lib/versioner/trashcan.go +++ b/lib/versioner/trashcan.go @@ -144,7 +144,7 @@ func (t *Trashcan) cleanoutArchive() error { // directory was empty and try to remove it. We ignore failure for // the time being. if currentDir != "" && filesInDir == 0 { - osutil.Remove(currentDir) + os.Remove(currentDir) } currentDir = path filesInDir = 0 @@ -153,7 +153,7 @@ func (t *Trashcan) cleanoutArchive() error { if info.ModTime().Before(cutoff) { // The file is too old; remove it. - osutil.Remove(path) + os.Remove(path) } else { // Keep this file, and remember it so we don't unnecessarily try // to remove this directory. @@ -169,7 +169,7 @@ func (t *Trashcan) cleanoutArchive() error { // The last directory seen by the walkFn may not have been removed as it // should be. if currentDir != "" && filesInDir == 0 { - osutil.Remove(currentDir) + os.Remove(currentDir) } return nil } diff --git a/test/util.go b/test/util.go index 71191eb01..b2ed3ab9c 100644 --- a/test/util.go +++ b/test/util.go @@ -208,7 +208,7 @@ func alterFiles(dir string) error { return err } } else { - err := osutil.Remove(path) + err := os.Remove(path) if err != nil { return err }