fs: calculate ModifyWindow each time on the fly instead of relying on global state - see #2319, #2328

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
 }
 

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)
-}

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 -
+// GetModifyWindow calculates the maximum modify window between the given Fses
-// sets Config.ModifyWindow
+// and the Config.ModifyWindow parameter.
-//
+func GetModifyWindow(fss ...Info) time.Duration {
-// This is the largest modify window of all the fses in use, and the
+	window := Config.ModifyWindow
-// user configured value
-func CalculateModifyWindow(fss ...Fs) {
 	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 ModTimeNotSupported
-				return
+			}
+			if precision > window {
+				window = precision
 			}
 		}
 	}
-	Infof(fss[0], "Modify window is %s", Config.ModifyWindow)
+	return window
 }

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 {
-	if dt < ModifyWindow && dt > -ModifyWindow {
+		fs.Debugf(src, "Size and modification time the same (differ by %s, within tolerance %s)", 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

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),
 	)
 }
 

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))
 	}
 }
 

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

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

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
 }