sync: fix directory modtimes being set twice

When syncing to a backend which supports directory modtimes, a
directory which needed its modtime updated and which had files
transferred into it would get its modtime set twice - once when the
directory was checked and once in the pass at the end of the sync.

Now, when the end of sync pass is in use (which it is for all default
syncs), directories which need their modtime (or metadata) updating
are marked for that pass instead of being updated immediately. This
halves the number of directory modtime updates in a typical sync and
makes the "Updated dirs" stat count each directory once.

fs/sync/sync.go

@@ -1128,13 +1128,25 @@ func (s *syncCopyMove) copyDirMetadata(ctx context.Context, f fs.Fs, dst fs.Dire
 	newDst = dst
 	if !equal {
 		if s.setDirMetadata && s.copyEmptySrcDirs {
-			newDst, err = operations.CopyDirMetadata(ctx, f, dst, dir, src)
+			if dst != nil && s.setDirModTime && s.setDirModTimeAfter {
+				// Update the metadata in the delayed pass at the end of
+				// the sync in case transfers into the directory change it
+				s.markDirModified(dst.Remote())
+			} else {
+				newDst, err = operations.CopyDirMetadata(ctx, f, dst, dir, src)
+			}
 		} else if dst == nil && s.setDirModTime && s.copyEmptySrcDirs {
 			newDst, err = operations.MkdirModTime(ctx, f, dir, src.ModTime(ctx))
 		} else if dst == nil && s.copyEmptySrcDirs {
 			err = operations.Mkdir(ctx, f, dir)
 		} else if dst != nil && s.setDirModTime {
-			newDst, err = operations.SetDirModTime(ctx, f, dst, dir, src.ModTime(ctx))
+			if s.setDirModTimeAfter {
+				// Set the modtime in the delayed pass at the end of the
+				// sync in case transfers into the directory change it
+				s.markDirModified(dst.Remote())
+			} else {
+				newDst, err = operations.SetDirModTime(ctx, f, dst, dir, src.ModTime(ctx))
+			}
 		}
 	}
 	if transform.Transforming(ctx) && newDst != nil && src.Remote() != newDst.Remote() {
@@ -1197,7 +1209,9 @@ func (s *syncCopyMove) setDelayedDirModTimes(ctx context.Context) error {
 				continue
 			}
 			if !s.copyEmptySrcDirs {
-				if _, isEmpty := s.srcEmptyDirs[item.dir]; isEmpty {
+				// Skip empty source directories which were never
+				// created on the destination
+				if _, isEmpty := s.srcEmptyDirs[item.dir]; isEmpty && item.dst == nil {
 					continue
 				}
 			}

fs/sync/sync_test.go

@@ -2868,6 +2868,39 @@ func TestSyncReplaceDirModTimeWithEmptyDirs(t *testing.T) {
 	testSyncReplaceDirModTime(t, true)
 }
 
+// Test that syncing a directory which needs its modtime updating and
+// which has files transferred into it only sets its modtime once
+func TestSyncSetDirModTimeOnce(t *testing.T) {
+	ctx := context.Background()
+	r := fstest.NewRun(t)
+	if r.Fremote.Features().DirSetModTime == nil {
+		t.Skip("Skipping test as remote does not support DirSetModTime")
+	}
+
+	file1 := r.WriteFile("dir/file1", "file1 contents", t2)
+	_, err := operations.SetDirModTime(ctx, r.Flocal, nil, "dir", t2)
+	require.NoError(t, err)
+
+	// Initial sync creates the directory and sets its modtime
+	accounting.GlobalStats().ResetCounters()
+	require.NoError(t, Sync(ctx, r.Fremote, r.Flocal, false))
+	r.CheckRemoteItems(t, file1)
+	assert.Equal(t, int64(1), accounting.GlobalStats().UpdatedDirs(0), "expected directory modtime to be set exactly once")
+	r.CheckDirectoryModTimes(t, "dir")
+
+	// Change the contents of the directory and its modtime
+	file1 = r.WriteFile("dir/file1", "file1 changed contents", t1)
+	_, err = operations.SetDirModTime(ctx, r.Flocal, nil, "dir", t1)
+	require.NoError(t, err)
+
+	// Sync again and check the modtime is only set once
+	accounting.GlobalStats().ResetCounters()
+	require.NoError(t, Sync(ctx, r.Fremote, r.Flocal, false))
+	r.CheckRemoteItems(t, file1)
+	assert.Equal(t, int64(1), accounting.GlobalStats().UpdatedDirs(0), "expected directory modtime to be set exactly once")
+	r.CheckDirectoryModTimes(t, "dir")
+}
+
 // Tests that nothing is transferred when src and dst already match
 // Run the same sync twice, ensure no action is taken the second time
 func testNothingToTransfer(t *testing.T, copyEmptySrcDirs bool) {