Repository: rclone/rclone (language: go)

Issue:
Rclone sync doesn't sync directory mtimes
# Info 
Rclone version: v1.69.0 Stable
OS: Linux Mint virginia 21.3 x86_64 (Linux 6.5.0-44-generic)

# Problem 
As per title, rclone doesn't seem to sync mtimes if these have changed in the destinantion. Directory syncing was merged as of v1.66, so it should be possible to keep modtime synced between two folders.

# Reproduction
```
$ ls -F
test/
$ tree -D ./test
[Jan 17 13:42]  test
├── [Aug  8  2023]  file8.txt
├── [Sep  9  2023]  file9.txt
├── [Jan 17  2022]  test_dir1
│   ├── [Jan  1  2023]  file1.txt
│   ├── [Feb  2  2023]  file2.txt
│   └── [Mar  3  2023]  file3.txt
├── [Dec 18  2020]  test_dir2
│   ├── [Apr  4  2023]  file4.txt
│   └── [May  5  2023]  file5.txt
└── [Dec  9  2018]  test_dir3
    ├── [Jun  6  2023]  file6.txt
    ├── [Jul  7  2023]  file7.txt
    └── [Dec  9  2018]  sub_dir
        └── [Dec  9  2018]  file10.txt

4 directories, 10 files
```

When I sync `./test` to `./test2` everything's working as expected
```
$ rclone sync ./test ./test2
$ tree -D ./test2
[Jan 17 15:17]  ./test2
├── [Aug  8  2023]  file8.txt
├── [Sep  9  2023]  file9.txt
├── [Jan 17  2022]  test_dir1
│   ├── [Jan  1  2023]  file1.txt
│   ├── [Feb  2  2023]  file2.txt
│   └── [Mar  3  2023]  file3.txt
├── [Dec 18  2020]  test_dir2
│   ├── [Apr  4  2023]  file4.txt
│   └── [May  5  2023]  file5.txt
└── [Dec  9  2018]  test_dir3
    ├── [Jun  6  2023]  file6.txt
    ├── [Jul  7  2023]  file7.txt
    └── [Dec  9  2018]  sub_dir
        └── [Dec  9  2018]  file10.txt

4 directories, 10 files
```

Now, suppose for some reason the timestamps in the destination change 
```
$ find ./test2 -exec touch -m {} +
$ tree -D test2
[Jan 17 15:18]  test2
├── [Jan 17 15:18]  file8.txt
├── [Jan 17 15:18]  file9.txt
├── [Jan 17 15:18]  test_dir1
│   ├── [Jan 17 15:18]  file1.txt
│   ├── [Jan 17 15:18]  file2.txt
│   └── [Jan 17 15:18]  file3.txt
├── [Jan 17 15:18]  test_dir2
│   ├── [Jan 17 15:18]  file4.txt
│   └── [Jan 17 15:18]  file5.txt
└── [Jan 17 15:18]  test_dir3
    ├── [Jan 17 15:18]  file6.txt
    ├── [Jan 17 15:18]  file7.txt
    └── [Jan 17 15:18]  sub_dir
        └── [Jan 17 15:18]  file10.txt

4 directories, 10 files
```

After I sync again with `rclone sync ./test ./test2`, I get
```
$ tree -D test2
[Jan 17 15:18]  test2
├── [Aug  8  2023]  file8.txt
├── [Sep  9  2023]  file9.txt
├── [Jan 17 15:18]  test_dir1
│   ├── [Jan  1  2023]  file1.txt
│   ├── [Feb  2  2023]  file2.txt
│   └── [Mar  3  2023]  file3.txt
├── [Jan 17 15:18]  test_dir2
│   ├── [Apr  4  2023]  file4.txt
│   └── [May  5  2023]  file5.txt
└── [Jan 17 15:18]  test_dir3
    ├── [Jun  6  2023]  file6.txt
    ├── [Jul  7  2023]  file7.txt
    └── [Jan 17 15:18]  sub_dir
        └── [Dec  9  2018]  file10.txt

4 directories, 10 files
```

In other words, **only file timestamps are synced**, folder timestamps are left as they were in the source.

# Expected behaviour
After a sync, both file and folder mtimes are updated to match the source folder.




Rclone sync doesn't sync directory mtimes
# Info 
Rclone version: v1.69.0 Stable
OS: Linux Mint virginia 21.3 x86_64 (Linux 6.5.0-44-generic)

# Problem 
As per title, rclone doesn't seem to sync mtimes if these have changed in the destinantion. Directory syncing was merged as of v1.66, so it should be possible to keep modtime synced between two folders.

# Reproduction
```
$ ls -F
test/
$ tree -D ./test
[Jan 17 13:42]  test
├── [Aug  8  2023]  file8.txt
├── [Sep  9  2023]  file9.txt
├── [Jan 17  2022]  test_dir1
│   ├── [Jan  1  2023]  file1.txt
│   ├── [Feb  2  2023]  file2.txt
│   └── [Mar  3  2023]  file3.txt
├── [Dec 18  2020]  test_dir2
│   ├── [Apr  4  2023]  file4.txt
│   └── [May  5  2023]  file5.txt
└── [Dec  9  2018]  test_dir3
    ├── [Jun  6  2023]  file6.txt
    ├── [Jul  7  2023]  file7.txt
    └── [Dec  9  2018]  sub_dir
        └── [Dec  9  2018]  file10.txt

4 directories, 10 files
```

When I sync `./test` to `./test2` everything's working as expected
```
$ rclone sync ./test ./test2
$ tree -D ./test2
[Jan 17 15:17]  ./test2
├── [Aug  8  2023]  file8.txt
├── [Sep  9  2023]  file9.txt
├── [Jan 17  2022]  test_dir1
│   ├── [Jan  1  2023]  file1.txt
│   ├── [Feb  2  2023]  file2.txt
│   └── [Mar  3  2023]  file3.txt
├── [Dec 18  2020]  test_dir2
│   ├── [Apr  4  2023]  file4.txt
│   └── [May  5  2023]  file5.txt
└── [Dec  9  2018]  test_dir3
    ├── [Jun  6  2023]  file6.txt
    ├── [Jul  7  2023]  file7.txt
    └── [Dec  9  2018]  sub_dir
        └── [Dec  9  2018]  file10.txt

4 directories, 10 files
```

Now, suppose for some reason the timestamps in the destination change 
```
$ find ./test2 -exec touch -m {} +
$ tree -D test2
[Jan 17 15:18]  test2
├── [Jan 17 15:18]  file8.txt
├── [Jan 17 15:18]  file9.txt
├── [Jan 17 15:18]  test_dir1
│   ├── [Jan 17 15:18]  file1.txt
│   ├── [Jan 17 15:18]  file2.txt
│   └── [Jan 17 15:18]  file3.txt
├── [Jan 17 15:18]  test_dir2
│   ├── [Jan 17 15:18]  file4.txt
│   └── [Jan 17 15:18]  file5.txt
└── [Jan 17 15:18]  test_dir3
    ├── [Jan 17 15:18]  file6.txt
    ├── [Jan 17 15:18]  file7.txt
    └── [Jan 17 15:18]  sub_dir
        └── [Jan 17 15:18]  file10.txt

4 directories, 10 files
```

After I sync again with `rclone sync ./test ./test2`, I get
```
$ tree -D test2
[Jan 17 15:18]  test2
├── [Aug  8  2023]  file8.txt
├── [Sep  9  2023]  file9.txt
├── [Jan 17 15:18]  test_dir1
│   ├── [Jan  1  2023]  file1.txt
│   ├── [Feb  2  2023]  file2.txt
│   └── [Mar  3  2023]  file3.txt
├── [Jan 17 15:18]  test_dir2
│   ├── [Apr  4  2023]  file4.txt
│   └── [May  5  2023]  file5.txt
└── [Jan 17 15:18]  test_dir3
    ├── [Jun  6  2023]  file6.txt
    ├── [Jul  7  2023]  file7.txt
    └── [Jan 17 15:18]  sub_dir
        └── [Dec  9  2018]  file10.txt

4 directories, 10 files
```

In other words, **only file timestamps are synced**, folder timestamps are left as they were in the source.

# Expected behaviour
After a sync, both file and folder mtimes are updated to match the source folder.






Discussion / hints:
I think this will do what you want if you use `--create-empty-src-dirs`

I recall that we originally did it your way by default, but there was significant user feedback from people unhappy with this change (see https://github.com/rclone/rclone/issues/7689 and https://forum.rclone.org/t/how-to-ignore-empty-directories-when-uploading-from-windows/45057/9) so it was changed to require the flag.

I tend to agree that your way makes more sense and would be the natural expectation (as whether to sync directory modtimes is really a separate issue from whether to sync empty directories), and I [ranted about it here](https://forum.rclone.org/t/how-to-ignore-empty-directories-when-uploading-from-windows/45057/17?u=nielash). But backward compatibility won the day, for better or worse. ¯\_(ツ)_/¯
I see. `--create-empty-src-dirs` works as expected, but to be honest I don't feel great about it. 

Both issues seem to interest backwards compatibilty with creating empty dirs, and, as I understand it, this behaivour popped up as a result. But, regardless of defaults, I really feel these should be two separate things. Suppose I want to sync modtimes but _not_ add empty dirs. Is empty dir creation so tightly coupled to modtimes that it can't be done?

It feels very weird that sync takes a `--no-update-dir-modtime` option (inherited from copy)  yet totally disregards them anyways.

At the very least, it should be added to the docs, as it's very unintuitive to expect  `--create-empty-src-dirs` to have anything to do with modtimes. 
> It feels very weird that sync takes a `--no-update-dir-modtime` option (inherited from copy) yet totally disregards them anyways.

@ncw remind me... is it intended behavior that `--no-update-dir-modtime` has no effect when `--create-empty-src-dirs` is not also in use? In other words, `--no-update-dir-modtime` does not imply that dir modtimes _would_ otherwise always be updated?

It has been long enough now that I don't clearly remember what was decided (and, to @ll3006's point, the docs are quite vague on this.)

```
--no-update-dir-modtime                       Don't update directory modification times
```

Here it seems to suggest the opposite:
https://github.com/rclone/rclone/blob/dc9c87279b989d4c9c8baec9adb259aba8b1da08/docs/content/docs.md?plain=1#L1879-L1882
@nielash 
> is it intended behavior that `--no-update-dir-modtime` has no effect when `--create-empty-src-dirs` is not also in use? In other words, `--no-update-dir-modtime` does not imply that dir modtimes _would_ otherwise always be updated?

I'd say that is a bug. I tried lots of differerent rclone versions to try to find one where this worked, but I couldn't find one.

This looks relevant 10eb4742dd9da47342ef768576fefd7c5f149443 - it looks like it was trying to fix this problem but failed.

Do you fancy having a look at what it would take to fix @nielash ?

I got as far as writing a bisect script but I couldn't find a working rclone to anchor it with!

[bisect-rclone.zip](https://github.com/user-attachments/files/18660589/bisect-rclone.zip)
> I'd say that is a bug.

I agree.

> Do you fancy having a look at what it would take to fix [@nielash](https://github.com/nielash) ?

I can, unless @ll3006 would like to have the honor?
> I can, unless @ll3006 would like to have the honor?

I am working on a prototype. I'll see if I can get it done, but I should mention I'm not overly familiar with Go.

Commit f5f86786b has the original behaviour of 'sync everything, including empty dirs'. It's also the first commit to "disrespect" `copyEmptySrcDirs`, adding directories even if they're empty. I've been working on top of this as I find it's the cleanest state before all the emptydir fixes.

Currently, my implementation idea is as follows:
- Keep a reference to all dirs that might have their modtime set (much like `setDirModTimes` does)
- If a dir is verified to be not empty (i.e. when a file from that dir is added)
    - try doing mkdir on it with metadata/modtime
    - Otherwise defer metadata/modtime for later 
- Set all deferred metadata/modtimes
- Eventually also copy empty dirs

I'll have to see if special handling is needed for dirs that have _become_ empty, that is, if all the files in a directory are deleted between two syncs. Since rclone's behaviour is to keep the directory empty, I would expect dir modtimes to also be updated in this case, if changed.

It will need extensive testing to ensure current behaviour and performance is not impacted.

Do you think this would work? 
I haven't given this a close look yet and so may be wrong, but my gut says this might be overcomplicating it -- and that perhaps all that is needed is to adjust the conditional logic here:

https://github.com/rclone/rclone/blob/dc9c87279b989d4c9c8baec9adb259aba8b1da08/fs/sync/sync.go#L1114-L1129

to ensure that we call `SetDirModTime` (not `MkdirModTime`) when `!ci.NoUpdateDirModTime && !s.copyEmptySrcDirs`
I think this will do what you want if you use `--create-empty-src-dirs`

I recall that we originally did it your way by default, but there was significant user feedback from people unhappy with this change (see https://github.com/rclone/rclone/issues/7689 and https://forum.rclone.org/t/how-to-ignore-empty-directories-when-uploading-from-windows/45057/9) so it was changed to require the flag.

I tend to agree that your way makes more sense and would be the natural expectation (as whether to sync directory modtimes is really a separate issue from whether to sync empty directories), and I [ranted about it here](https://forum.rclone.org/t/how-to-ignore-empty-directories-when-uploading-from-windows/45057/17?u=nielash). But backward compatibility won the day, for better or worse. ¯\_(ツ)_/¯
I see. `--create-empty-src-dirs` works as expected, but to be honest I don't feel great about it. 

Both issues seem to interest backwards compatibilty with creating empty dirs, and, as I understand it, this behaivour popped up as a result. But, regardless of defaults, I really feel these should be two separate things. Suppose I want to sync modtimes but _not_ add empty dirs. Is empty dir creation so tightly coupled to modtimes that it can't be done?

It feels very weird that sync takes a `--no-update-dir-modtime` option (inherited from copy)  yet totally disregards them anyways.

At the very least, it should be added to the docs, as it's very unintuitive to expect  `--create-empty-src-dirs` to have anything to do with modtimes. 
> It feels very weird that sync takes a `--no-update-dir-modtime` option (inherited from copy) yet totally disregards them anyways.

@ncw remind me... is it intended behavior that `--no-update-dir-modtime` has no effect when `--create-empty-src-dirs` is not also in use? In other words, `--no-update-dir-modtime` does not imply that dir modtimes _would_ otherwise always be updated?

It has been long enough now that I don't clearly remember what was decided (and, to @ll3006's point, the docs are quite vague on this.)

```
--no-update-dir-modtime                       Don't update directory modification times
```

Here it seems to suggest the opposite:
https://github.com/rclone/rclone/blob/dc9c87279b989d4c9c8baec9adb259aba8b1da08/docs/content/docs.md?plain=1#L1879-L1882
@nielash 
> is it intended behavior that `--no-update-dir-modtime` has no effect when `--create-empty-src-dirs` is not also in use? In other words, `--no-update-dir-modtime` does not imply that dir modtimes _would_ otherwise always be updated?

I'd say that is a bug. I tried lots of differerent rclone versions to try to find one where this worked, but I couldn't find one.

This looks relevant 10eb4742dd9da47342ef768576fefd7c5f149443 - it looks like it was trying to fix this problem but failed.

Do you fancy having a look at what it would take to fix @nielash ?

I got as far as writing a bisect script but I couldn't find a working rclone to anchor it with!

[bisect-rclone.zip](https://github.com/user-attachments/files/18660589/bisect-rclone.zip)
> I'd say that is a bug.

I agree.

> Do you fancy having a look at what it would take to fix [@nielash](https://github.com/nielash) ?

I can, unless @ll3006 would like to have the honor?
> I can, unless @ll3006 would like to have the honor?

I am working on a prototype. I'll see if I can get it done, but I should mention I'm not overly familiar with Go.

Commit f5f86786b has the original behaviour of 'sync everything, including empty dirs'. It's also the first commit to "disrespect" `copyEmptySrcDirs`, adding directories even if they're empty. I've been working on top of this as I find it's the cleanest state before all the emptydir fixes.

Currently, my implementation idea is as follows:
- Keep a reference to all dirs that might have their modtime set (much like `setDirModTimes` does)
- If a dir is verified to be not empty (i.e. when a file from that dir is added)
    - try doing mkdir on it with metadata/modtime
    - Otherwise defer metadata/modtime for later 
- Set all deferred metadata/modtimes
- Eventually also copy empty dirs

I'll have to see if special handling is needed for dirs that have _become_ empty, that is, if all the files in a directory are deleted between two syncs. Since rclone's behaviour is to keep the directory empty, I would expect dir modtimes to also be updated in this case, if changed.

It will need extensive testing to ensure current behaviour and performance is not impacted.

Do you think this would work? 
I haven't given this a close look yet and so may be wrong, but my gut says this might be overcomplicating it -- and that perhaps all that is needed is to adjust the conditional logic here:

https://github.com/rclone/rclone/blob/dc9c87279b989d4c9c8baec9adb259aba8b1da08/fs/sync/sync.go#L1114-L1129

to ensure that we call `SetDirModTime` (not `MkdirModTime`) when `!ci.NoUpdateDirModTime && !s.copyEmptySrcDirs`

Fix patch:
diff --git a/fs/sync/sync.go b/fs/sync/sync.go
index 7feedfcd770e9..e30c86ed2ff00 100644
--- a/fs/sync/sync.go
+++ b/fs/sync/sync.go
@@ -1109,23 +1109,17 @@ func (s *syncCopyMove) copyDirMetadata(ctx context.Context, f fs.Fs, dst fs.Dire
 	if !s.setDirModTimeAfter && equal {
 		return nil
 	}
-	if s.setDirModTimeAfter && equal {
-		newDst = dst
-	} else if s.copyEmptySrcDirs {
-		if s.setDirMetadata {
+	newDst = dst
+	if !equal {
+		if s.setDirMetadata && s.copyEmptySrcDirs {
 			newDst, err = operations.CopyDirMetadata(ctx, f, dst, dir, src)
-		} else if s.setDirModTime {
-			if dst == nil {
-				newDst, err = operations.MkdirModTime(ctx, f, dir, src.ModTime(ctx))
-			} else {
-				newDst, err = operations.SetDirModTime(ctx, f, dst, dir, src.ModTime(ctx))
-			}
-		} else if dst == nil {
-			// Create the directory if it doesn't exist
+		} 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))
 		}
-	} else {
-		newDst = dst
 	}
 	// If we need to set modtime after and we created a dir, then save it for later
 	if s.setDirModTime && s.setDirModTimeAfter && err == nil {


Test patch:
diff --git a/fs/sync/sync_test.go b/fs/sync/sync_test.go
index f4f1c556a9c86..9c24407b791b6 100644
--- a/fs/sync/sync_test.go
+++ b/fs/sync/sync_test.go
@@ -2762,6 +2762,81 @@ func TestSyncConcurrentTruncate(t *testing.T) {
 	testSyncConcurrent(t, "truncate")
 }
 
+// Test that sync replaces dir modtimes in dst if they've changed
+func testSyncReplaceDirModTime(t *testing.T, copyEmptySrcDirs bool) {
+	accounting.GlobalStats().ResetCounters()
+	ctx, _ := fs.AddConfig(context.Background())
+	r := fstest.NewRun(t)
+
+	file1 := r.WriteFile("file1", "file1", t2)
+	file2 := r.WriteFile("test_dir1/file2", "file2", t2)
+	file3 := r.WriteFile("test_dir2/sub_dir/file3", "file3", t2)
+	r.CheckLocalItems(t, file1, file2, file3)
+
+	_, err := operations.MkdirModTime(ctx, r.Flocal, "empty_dir", t2)
+	require.NoError(t, err)
+
+	// A directory that's empty on both src and dst
+	_, err = operations.MkdirModTime(ctx, r.Flocal, "empty_on_remote", t2)
+	require.NoError(t, err)
+	_, err = operations.MkdirModTime(ctx, r.Fremote, "empty_on_remote", t2)
+	require.NoError(t, err)
+
+	// set logging
+	// (this checks log output as DirModtime operations do not yet have stats, and r.CheckDirectoryModTimes also does not tell us what actions were taken)
+	oldLogLevel := fs.GetConfig(context.Background()).LogLevel
+	defer func() { fs.GetConfig(context.Background()).LogLevel = oldLogLevel }() // reset to old val after test
+	// need to do this as fs.Infof only respects the globalConfig
+	fs.GetConfig(context.Background()).LogLevel = fs.LogLevelInfo
+
+	// First run
+	accounting.GlobalStats().ResetCounters()
+	ctx = predictDstFromLogger(ctx)
+	output := bilib.CaptureOutput(func() {
+		err := CopyDir(ctx, r.Fremote, r.Flocal, copyEmptySrcDirs)
+		require.NoError(t, err)
+	})
+	require.NotNil(t, output)
+	testLoggerVsLsf(ctx, r.Fremote, operations.GetLoggerOpt(ctx).JSON, t)
+
+	// Save all dirs
+	dirs := []string{"test_dir1", "test_dir2", "test_dir2/sub_dir", "empty_on_remote"}
+	if copyEmptySrcDirs {
+		dirs = append(dirs, "empty_dir")
+	}
+
+	// Change dir modtimes
+	for _, dir := range dirs {
+		_, err := operations.SetDirModTime(ctx, r.Flocal, nil, dir, t1)
+		if err != nil && !errors.Is(err, fs.ErrorNotImplemented) {
+			require.NoError(t, err)
+		}
+	}
+
+	// Run again
+	accounting.GlobalStats().ResetCounters()
+	ctx = predictDstFromLogger(ctx)
+	output = bilib.CaptureOutput(func() {
+		err := CopyDir(ctx, r.Fremote, r.Flocal, copyEmptySrcDirs)
+		require.NoError(t, err)
+	})
+	require.NotNil(t, output)
+	testLoggerVsLsf(ctx, r.Fremote, operations.GetLoggerOpt(ctx).JSON, t)
+	r.CheckLocalItems(t, file1, file2, file3)
+	r.CheckRemoteItems(t, file1, file2, file3)
+
+	// Check that the modtimes of the directories are as expected
+	r.CheckDirectoryModTimes(t, dirs...)
+}
+
+func TestSyncReplaceDirModTime(t *testing.T) {
+	testSyncReplaceDirModTime(t, false)
+}
+
+func TestSyncReplaceDirModTimeWithEmptyDirs(t *testing.T) {
+	testSyncReplaceDirModTime(t, true)
+}
+
 // 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) {