sync: --update/-u not transfer files that haven't changed - fixes #3232

Before this change --update would transfer any file which was newer
than the destination regardless of whether it had changed or not.
This is needlessly wasteful of bandwidth.

After this change --update will only transfer files if they are newer
**and** they are different (checked with checksum and size).

docs/content/docs.md

@@ -1098,9 +1098,18 @@ The default is to run 4 file transfers in parallel.
 This forces rclone to skip any files which exist on the destination
 and have a modified time that is newer than the source file.
 
+This can be useful when transferring to a remote which doesn't support
+mod times directly (or when using `--use-server-mod-time` to avoid extra
+API calls) as it is more accurate than a `--size-only` check and faster
+than using `--checksum`.
+
 If an existing destination file has a modification time equal (within
 the computed modify window precision) to the source file's, it will be
-updated if the sizes are different.
+updated if the sizes are different.  If `--checksum` is set then
+rclone will update the destination if the checksums differ too.
+
+If an existing destination file is older than the source file then
+it will be updated if the size or checksum differs from the source file.
 
 On remotes which don't support mod time directly (or when using
 `--use-server-mod-time`) the time checked will be the uploaded time.
@@ -1108,11 +1117,6 @@ This means that if uploading to one of these remotes, rclone will skip
 any files which exist on the destination and have an uploaded time that
 is newer than the modification time of the source file.
 
-This can be useful when transferring to a remote which doesn't support
-mod times directly (or when using `--use-server-mod-time` to avoid extra
-API calls) as it is more accurate than a `--size-only` check and faster
-than using `--checksum`.
-
 ### --use-mmap ###
 
 If this flag is set then rclone will use anonymous memory allocated by

fs/operations/operations.go

@@ -114,7 +114,7 @@ func checkHashes(ctx context.Context, src fs.ObjectInfo, dst fs.Object, ht hash.
 // Otherwise the file is considered to be not equal including if there
 // were errors reading info.
 func Equal(ctx context.Context, src fs.ObjectInfo, dst fs.Object) bool {
-	return equal(ctx, src, dst, fs.Config.SizeOnly, fs.Config.CheckSum, !fs.Config.NoUpdateModTime)
+	return equal(ctx, src, dst, defaultEqualOpt())
 }
 
 // sizeDiffers compare the size of src and dst taking into account the
@@ -128,12 +128,30 @@ func sizeDiffers(src, dst fs.ObjectInfo) bool {
 
 var checksumWarning sync.Once
 
-func equal(ctx context.Context, src fs.ObjectInfo, dst fs.Object, sizeOnly, checkSum, UpdateModTime bool) bool {
+// options for equal function()
+type equalOpt struct {
+	sizeOnly          bool // if set only check size
+	checkSum          bool // if set check checksum+size instead of modtime+size
+	updateModTime     bool // if set update the modtime if hashes identical and checking with modtime+size
+	forceModTimeMatch bool // if set assume modtimes match
+}
+
+// default set of options for equal()
+func defaultEqualOpt() equalOpt {
+	return equalOpt{
+		sizeOnly:          fs.Config.SizeOnly,
+		checkSum:          fs.Config.CheckSum,
+		updateModTime:     !fs.Config.NoUpdateModTime,
+		forceModTimeMatch: false,
+	}
+}
+
+func equal(ctx context.Context, src fs.ObjectInfo, dst fs.Object, opt equalOpt) bool {
 	if sizeDiffers(src, dst) {
 		fs.Debugf(src, "Sizes differ (src %d vs dst %d)", src.Size(), dst.Size())
 		return false
 	}
-	if sizeOnly {
+	if opt.sizeOnly {
 		fs.Debugf(src, "Sizes identical")
 		return true
 	}
@@ -141,7 +159,7 @@ func equal(ctx context.Context, src fs.ObjectInfo, dst fs.Object, sizeOnly, chec
 	// Assert: Size is equal or being ignored
 
 	// If checking checksum and not modtime
-	if checkSum {
+	if opt.checkSum {
 		// Check the hash
 		same, ht, _ := CheckHashes(ctx, src, dst)
 		if !same {
@@ -159,21 +177,23 @@ func equal(ctx context.Context, src fs.ObjectInfo, dst fs.Object, sizeOnly, chec
 		return true
 	}
 
-	// Sizes the same so check the mtime
-	modifyWindow := fs.GetModifyWindow(src.Fs(), dst.Fs())
-	if modifyWindow == fs.ModTimeNotSupported {
-		fs.Debugf(src, "Sizes identical")
-		return true
-	}
 	srcModTime := src.ModTime(ctx)
-	dstModTime := dst.ModTime(ctx)
-	dt := dstModTime.Sub(srcModTime)
-	if dt < modifyWindow && dt > -modifyWindow {
-		fs.Debugf(src, "Size and modification time the same (differ by %s, within tolerance %s)", dt, modifyWindow)
-		return true
-	}
+	if !opt.forceModTimeMatch {
+		// Sizes the same so check the mtime
+		modifyWindow := fs.GetModifyWindow(src.Fs(), dst.Fs())
+		if modifyWindow == fs.ModTimeNotSupported {
+			fs.Debugf(src, "Sizes identical")
+			return true
+		}
+		dstModTime := dst.ModTime(ctx)
+		dt := dstModTime.Sub(srcModTime)
+		if dt < modifyWindow && dt > -modifyWindow {
+			fs.Debugf(src, "Size and modification time the same (differ by %s, within tolerance %s)", dt, modifyWindow)
+			return true
+		}
 
-	fs.Debugf(src, "Modification times differ by %s: %v, %v", dt, srcModTime, dstModTime)
+		fs.Debugf(src, "Modification times differ by %s: %v, %v", dt, srcModTime, dstModTime)
+	}
 
 	// Check if the hashes are the same
 	same, ht, _ := CheckHashes(ctx, src, dst)
@@ -187,7 +207,7 @@ func equal(ctx context.Context, src fs.ObjectInfo, dst fs.Object, sizeOnly, chec
 	}
 
 	// mod time differs but hash is the same to reset mod time if required
-	if UpdateModTime {
+	if opt.updateModTime {
 		if fs.Config.DryRun {
 			fs.Logf(src, "Not updating modification time as --dry-run")
 		} else {
@@ -1444,7 +1464,9 @@ func copyDest(ctx context.Context, fdst fs.Fs, dst, src fs.Object, CopyDest, bac
 	default:
 		return false, err
 	}
-	if equal(ctx, src, CopyDestFile, fs.Config.SizeOnly, fs.Config.CheckSum, false) {
+	opt := defaultEqualOpt()
+	opt.updateModTime = false
+	if equal(ctx, src, CopyDestFile, opt) {
 		if dst == nil || !Equal(ctx, src, dst) {
 			if dst != nil && backupDir != nil {
 				err = MoveBackupDir(ctx, backupDir, dst)
@@ -1520,13 +1542,22 @@ func NeedTransfer(ctx context.Context, dst, src fs.Object) bool {
 			fs.Debugf(src, "Destination is newer than source, skipping")
 			return false
 		case dt <= -modifyWindow:
-			fs.Debugf(src, "Destination is older than source, transferring")
-		default:
-			if !sizeDiffers(src, dst) {
-				fs.Debugf(src, "Destination mod time is within %v of source and sizes identical, skipping", modifyWindow)
+			// force --checksum on for the check and do update modtimes by default
+			opt := defaultEqualOpt()
+			opt.forceModTimeMatch = true
+			if equal(ctx, src, dst, opt) {
+				fs.Debugf(src, "Unchanged skipping")
 				return false
 			}
-			fs.Debugf(src, "Destination mod time is within %v of source but sizes differ, transferring", modifyWindow)
+		default:
+			// Do a size only compare unless --checksum is set
+			opt := defaultEqualOpt()
+			opt.sizeOnly = !fs.Config.CheckSum
+			if equal(ctx, src, dst, opt) {
+				fs.Debugf(src, "Destination mod time is within %v of source and files identical, skipping", modifyWindow)
+				return false
+			}
+			fs.Debugf(src, "Destination mod time is within %v of source but files differ, transferring", modifyWindow)
 		}
 	} else {
 		// Check to see if changed or not

fs/sync/sync_test.go

@@ -972,10 +972,22 @@ func TestSyncWithUpdateOlder(t *testing.T) {
 		fs.Config.ModifyWindow = oldModifyWindow
 	}()
 
-	accounting.GlobalStats().ResetCounters()
 	err := Sync(context.Background(), r.Fremote, r.Flocal, false)
 	require.NoError(t, err)
 	fstest.CheckItems(t, r.Fremote, oneO, twoF, threeO, fourF, fiveF)
+
+	if r.Fremote.Hashes().Count() == 0 {
+		t.Logf("Skip test with --checksum as no hashes supported")
+		return
+	}
+
+	// now enable checksum
+	fs.Config.CheckSum = true
+	defer func() { fs.Config.CheckSum = false }()
+
+	err = Sync(context.Background(), r.Fremote, r.Flocal, false)
+	require.NoError(t, err)
+	fstest.CheckItems(t, r.Fremote, oneO, twoF, threeF, fourF, fiveF)
 }
 
 // Test with TrackRenames set