From 4009fb67c80086ae220cbccc6a6ca1d7aa2af621 Mon Sep 17 00:00:00 2001 From: Stefan Date: Sun, 3 Jun 2018 20:45:34 +0200 Subject: [PATCH] fs: calculate ModifyWindow each time on the fly instead of relying on global state - see #2319, #2328 --- cmd/cmd.go | 6 ----- cmd/mountlib/mounttest/fs.go | 30 ------------------------ fs/fs.go | 21 +++++++---------- fs/operations/operations.go | 10 ++++---- fs/operations/operations_test.go | 8 +++---- fs/sync/sync_test.go | 40 ++++++++++++++++---------------- fstest/fstest.go | 2 +- fstest/fstests/fstests.go | 6 ++--- fstest/run.go | 3 +-- 9 files changed, 43 insertions(+), 83 deletions(-) diff --git a/cmd/cmd.go b/cmd/cmd.go index 9ada78a57..d1dbab5bd 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -196,7 +196,6 @@ func newFsFileAddFilter(remote string) (fs.Fs, string) { // limit the Fs to a single file. func NewFsSrc(args []string) fs.Fs { fsrc, _ := newFsFileAddFilter(args[0]) - fs.CalculateModifyWindow(fsrc) return fsrc } @@ -217,7 +216,6 @@ func newFsDir(remote string) fs.Fs { // The argument must point a directory func NewFsDir(args []string) fs.Fs { fdst := newFsDir(args[0]) - fs.CalculateModifyWindow(fdst) return fdst } @@ -225,7 +223,6 @@ func NewFsDir(args []string) fs.Fs { func NewFsSrcDst(args []string) (fs.Fs, fs.Fs) { fsrc, _ := newFsFileAddFilter(args[0]) fdst := newFsDir(args[1]) - fs.CalculateModifyWindow(fdst, fsrc) return fsrc, fdst } @@ -235,7 +232,6 @@ func NewFsSrcDst(args []string) (fs.Fs, fs.Fs) { func NewFsSrcFileDst(args []string) (fsrc fs.Fs, srcFileName string, fdst fs.Fs) { fsrc, srcFileName = NewFsFile(args[0]) fdst = newFsDir(args[1]) - fs.CalculateModifyWindow(fdst, fsrc) return fsrc, srcFileName, fdst } @@ -266,7 +262,6 @@ func NewFsSrcDstFiles(args []string) (fsrc fs.Fs, srcFileName string, fdst fs.Fs fs.CountError(err) log.Fatalf("Failed to create file system for destination %q: %v", dstRemote, err) } - fs.CalculateModifyWindow(fdst, fsrc) return } @@ -280,7 +275,6 @@ func NewFsDstFile(args []string) (fdst fs.Fs, dstFileName string) { log.Fatalf("%q is a directory", args[0]) } fdst = newFsDir(dstRemote) - fs.CalculateModifyWindow(fdst) return } diff --git a/cmd/mountlib/mounttest/fs.go b/cmd/mountlib/mounttest/fs.go index f29a4e3b9..06a82505d 100644 --- a/cmd/mountlib/mounttest/fs.go +++ b/cmd/mountlib/mounttest/fs.go @@ -18,12 +18,10 @@ import ( "time" _ "github.com/ncw/rclone/backend/all" // import all the backends - "github.com/ncw/rclone/cmd/mountlib" "github.com/ncw/rclone/fs" "github.com/ncw/rclone/fs/walk" "github.com/ncw/rclone/fstest" "github.com/ncw/rclone/vfs" - "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -49,7 +47,6 @@ func RunTests(t *testing.T, fn MountFn) { vfs.CacheModeWrites, vfs.CacheModeFull, } - t.Run("TestModifyWindow", TestModifyWindow) run = newRun() for _, cacheMode := range cacheModes { run.cacheMode(cacheMode) @@ -410,30 +407,3 @@ func TestRoot(t *testing.T) { assert.True(t, fi.IsDir()) assert.Equal(t, run.vfs.Opt.DirPerms&os.ModePerm, fi.Mode().Perm()) } - -// TestModifyWindow ensures that the modify window is calculated upon running -// the mount cobra command -func TestModifyWindow(t *testing.T) { - // create local remote - remoteName, _, err := fstest.RandomRemoteName(*fstest.RemoteName) - require.NoError(t, err) - - // find valid path to mount to - mountPath := findMountPath() - defer func() { - err := os.RemoveAll(mountPath) - require.NoError(t, err) - }() - - // no-op mount command, since we only care about FS creation - fakeMount := func(f fs.Fs, mountpoint string) error { - return nil - } - cmd := mountlib.NewMountCommand("modify-window-test", fakeMount) - - // running the command should recalculate the modify window. The modify window - // is greater than 0 for all local FS, which is why the test works. - fs.Config.ModifyWindow = 0 * time.Second - cmd.Run(&cobra.Command{}, []string{remoteName, mountPath}) - assert.NotEqual(t, 0*time.Second, fs.Config.ModifyWindow) -} diff --git a/fs/fs.go b/fs/fs.go index 9325a6b85..0c7d81ab0 100644 --- a/fs/fs.go +++ b/fs/fs.go @@ -862,23 +862,20 @@ func FileExists(fs Fs, remote string) (bool, error) { return true, nil } -// CalculateModifyWindow works out modify window for Fses passed in - -// sets Config.ModifyWindow -// -// This is the largest modify window of all the fses in use, and the -// user configured value -func CalculateModifyWindow(fss ...Fs) { +// GetModifyWindow calculates the maximum modify window between the given Fses +// and the Config.ModifyWindow parameter. +func GetModifyWindow(fss ...Info) time.Duration { + window := Config.ModifyWindow for _, f := range fss { if f != nil { precision := f.Precision() - if precision > Config.ModifyWindow { - Config.ModifyWindow = precision - } if precision == ModTimeNotSupported { - Infof(f, "Modify window not supported") - return + return ModTimeNotSupported + } + if precision > window { + window = precision } } } - Infof(fss[0], "Modify window is %s", Config.ModifyWindow) + return window } diff --git a/fs/operations/operations.go b/fs/operations/operations.go index 5903b15f9..ec8bcca75 100644 --- a/fs/operations/operations.go +++ b/fs/operations/operations.go @@ -132,16 +132,16 @@ func equal(src fs.ObjectInfo, dst fs.Object, sizeOnly, checkSum bool) bool { } // Sizes the same so check the mtime - if fs.Config.ModifyWindow == fs.ModTimeNotSupported { + modifyWindow := fs.GetModifyWindow(src.Fs(), dst.Fs()) + if modifyWindow == fs.ModTimeNotSupported { fs.Debugf(src, "Sizes identical") return true } srcModTime := src.ModTime() dstModTime := dst.ModTime() dt := dstModTime.Sub(srcModTime) - ModifyWindow := fs.Config.ModifyWindow - if dt < ModifyWindow && dt > -ModifyWindow { - fs.Debugf(src, "Size and modification time the same (differ by %s, within tolerance %s)", dt, ModifyWindow) + if dt < modifyWindow && dt > -modifyWindow { + fs.Debugf(src, "Size and modification time the same (differ by %s, within tolerance %s)", dt, modifyWindow) return true } @@ -1285,7 +1285,7 @@ func NeedTransfer(dst, src fs.Object) bool { dstModTime := dst.ModTime() dt := dstModTime.Sub(srcModTime) // If have a mutually agreed precision then use that - modifyWindow := fs.Config.ModifyWindow + modifyWindow := fs.GetModifyWindow(dst.Fs(), src.Fs()) if modifyWindow == fs.ModTimeNotSupported { // Otherwise use 1 second as a safe default as // the resolution of the time a file was diff --git a/fs/operations/operations_test.go b/fs/operations/operations_test.go index 37e28436d..4e1ff4cb4 100644 --- a/fs/operations/operations_test.go +++ b/fs/operations/operations_test.go @@ -411,7 +411,7 @@ func TestRmdirsNoLeaveRoot(t *testing.T) { "A3/B3", "A3/B3/C4", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) require.NoError(t, operations.Rmdirs(r.Fremote, "", false)) @@ -427,7 +427,7 @@ func TestRmdirsNoLeaveRoot(t *testing.T) { "A1/B1", "A1/B1/C1", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) } @@ -452,7 +452,7 @@ func TestRmdirsLeaveRoot(t *testing.T) { "A1/B1", "A1/B1/C1", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) require.NoError(t, operations.Rmdirs(r.Fremote, "A1", true)) @@ -464,7 +464,7 @@ func TestRmdirsLeaveRoot(t *testing.T) { []string{ "A1", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) } diff --git a/fs/sync/sync_test.go b/fs/sync/sync_test.go index 256d32353..c8d81129b 100644 --- a/fs/sync/sync_test.go +++ b/fs/sync/sync_test.go @@ -306,7 +306,7 @@ func TestSyncIgnoreErrors(t *testing.T) { "a", "c", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) fstest.CheckListingWithPrecision( t, @@ -320,7 +320,7 @@ func TestSyncIgnoreErrors(t *testing.T) { "c", "d", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) accounting.Stats.ResetCounters() @@ -338,7 +338,7 @@ func TestSyncIgnoreErrors(t *testing.T) { "a", "c", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) fstest.CheckListingWithPrecision( t, @@ -351,7 +351,7 @@ func TestSyncIgnoreErrors(t *testing.T) { "a", "c", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) } @@ -413,11 +413,11 @@ func TestSyncAfterChangingModtimeOnlyWithNoUpdateModTime(t *testing.T) { } func TestSyncDoesntUpdateModtime(t *testing.T) { - if fs.Config.ModifyWindow == fs.ModTimeNotSupported { - t.Skip("Can't run this test on fs which doesn't support mod time") - } r := fstest.NewRun(t) defer r.Finalise() + if fs.GetModifyWindow(r.Fremote) == fs.ModTimeNotSupported { + t.Skip("Can't run this test on fs which doesn't support mod time") + } file1 := r.WriteFile("foo", "foo", t2) file2 := r.WriteObject("foo", "bar", t1) @@ -546,7 +546,7 @@ func TestSyncAfterRemovingAFileAndAddingAFileSubDir(t *testing.T) { "a", "c", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) fstest.CheckListingWithPrecision( t, @@ -561,7 +561,7 @@ func TestSyncAfterRemovingAFileAndAddingAFileSubDir(t *testing.T) { "d", "d/e", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) accounting.Stats.ResetCounters() @@ -579,7 +579,7 @@ func TestSyncAfterRemovingAFileAndAddingAFileSubDir(t *testing.T) { "a", "c", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) fstest.CheckListingWithPrecision( t, @@ -592,7 +592,7 @@ func TestSyncAfterRemovingAFileAndAddingAFileSubDir(t *testing.T) { "a", "c", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) } @@ -616,7 +616,7 @@ func TestSyncAfterRemovingAFileAndAddingAFileSubDirWithErrors(t *testing.T) { "a", "c", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) fstest.CheckListingWithPrecision( t, @@ -630,7 +630,7 @@ func TestSyncAfterRemovingAFileAndAddingAFileSubDirWithErrors(t *testing.T) { "c", "d", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) accounting.Stats.ResetCounters() @@ -649,7 +649,7 @@ func TestSyncAfterRemovingAFileAndAddingAFileSubDirWithErrors(t *testing.T) { "a", "c", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) fstest.CheckListingWithPrecision( t, @@ -665,7 +665,7 @@ func TestSyncAfterRemovingAFileAndAddingAFileSubDirWithErrors(t *testing.T) { "c", "d", }, - fs.Config.ModifyWindow, + fs.GetModifyWindow(r.Fremote), ) } @@ -779,11 +779,11 @@ func TestSyncWithExcludeAndDeleteExcluded(t *testing.T) { // Test with UpdateOlder set func TestSyncWithUpdateOlder(t *testing.T) { - if fs.Config.ModifyWindow == fs.ModTimeNotSupported { - t.Skip("Can't run this test on fs which doesn't support mod time") - } r := fstest.NewRun(t) defer r.Finalise() + if fs.GetModifyWindow(r.Fremote) == fs.ModTimeNotSupported { + t.Skip("Can't run this test on fs which doesn't support mod time") + } t2plus := t2.Add(time.Second / 2) t2minus := t2.Add(time.Second / 2) oneF := r.WriteFile("one", "one", t1) @@ -887,7 +887,7 @@ func testServerSideMove(t *testing.T, r *fstest.Run, withFilter, testDeleteEmpty } if testDeleteEmptyDirs { - fstest.CheckListingWithPrecision(t, r.Fremote, nil, []string{}, fs.Config.ModifyWindow) + fstest.CheckListingWithPrecision(t, r.Fremote, nil, []string{}, fs.GetModifyWindow(r.Fremote)) } fstest.CheckItems(t, FremoteMove, file2, file1, file3u) @@ -916,7 +916,7 @@ func testServerSideMove(t *testing.T, r *fstest.Run, withFilter, testDeleteEmpty } if testDeleteEmptyDirs { - fstest.CheckListingWithPrecision(t, FremoteMove, nil, []string{}, fs.Config.ModifyWindow) + fstest.CheckListingWithPrecision(t, FremoteMove, nil, []string{}, fs.GetModifyWindow(r.Fremote)) } } diff --git a/fstest/fstest.go b/fstest/fstest.go index c9cb7d871..6361c6e7e 100644 --- a/fstest/fstest.go +++ b/fstest/fstest.go @@ -343,7 +343,7 @@ func CheckListing(t *testing.T, f fs.Fs, items []Item) { // CheckItems checks the fs to see if it has only the items passed in // using a precision of fs.Config.ModifyWindow func CheckItems(t *testing.T, f fs.Fs, items ...Item) { - CheckListingWithPrecision(t, f, items, nil, fs.Config.ModifyWindow) + CheckListingWithPrecision(t, f, items, nil, fs.GetModifyWindow(f)) } // Time parses a time string or logs a fatal error diff --git a/fstest/fstests/fstests.go b/fstest/fstests/fstests.go index 90034274d..9a4f180b5 100644 --- a/fstest/fstests/fstests.go +++ b/fstest/fstests/fstests.go @@ -302,15 +302,15 @@ func Run(t *testing.T, opt *Opt) { dir := "dir/subdir" err := operations.Mkdir(remote, dir) require.NoError(t, err) - fstest.CheckListingWithPrecision(t, remote, []fstest.Item{}, []string{"dir", "dir/subdir"}, fs.Config.ModifyWindow) + fstest.CheckListingWithPrecision(t, remote, []fstest.Item{}, []string{"dir", "dir/subdir"}, fs.GetModifyWindow(remote)) err = operations.Rmdir(remote, dir) require.NoError(t, err) - fstest.CheckListingWithPrecision(t, remote, []fstest.Item{}, []string{"dir"}, fs.Config.ModifyWindow) + fstest.CheckListingWithPrecision(t, remote, []fstest.Item{}, []string{"dir"}, fs.GetModifyWindow(remote)) err = operations.Rmdir(remote, "dir") require.NoError(t, err) - fstest.CheckListingWithPrecision(t, remote, []fstest.Item{}, []string{}, fs.Config.ModifyWindow) + fstest.CheckListingWithPrecision(t, remote, []fstest.Item{}, []string{}, fs.GetModifyWindow(remote)) }) // TestFsListEmpty tests listing an empty directory diff --git a/fstest/run.go b/fstest/run.go index 1bf0ddc8b..23872e1b5 100644 --- a/fstest/run.go +++ b/fstest/run.go @@ -103,7 +103,6 @@ func newRun() *Run { if err != nil { r.Fatalf("Failed to make %q: %v", r.LocalName, err) } - fs.CalculateModifyWindow(r.Fremote, r.Flocal) return r } @@ -173,7 +172,7 @@ func NewRun(t *testing.T) *Run { } r.Logf = t.Logf r.Fatalf = t.Fatalf - r.Logf("Remote %q, Local %q, Modify Window %q", r.Fremote, r.Flocal, fs.Config.ModifyWindow) + r.Logf("Remote %q, Local %q, Modify Window %q", r.Fremote, r.Flocal, fs.GetModifyWindow(r.Fremote)) return r }