sync: don't set dir modtimes if already set

[nielash]

What is the purpose of this change?

Before this change, directory modtimes (and metadata) were always synced from src to dst, even if already in sync (i.e. their modtimes already matched.) This potentially required excessive API calls, made logs noisy, and was potentially problematic for backends that create "versions" or otherwise log activity updates when modtime/metadata is updated.

After this change, a new DirsEqual function is added to check whether dirs are equal based on a number of factors such as ModifyWindow and sync flags in use. If the dirs are equal, the modtime/metadata update is skipped.

For backends that require setDirModTimeAfter, the "after" sync is performed only for dirs that could have been changed by the sync (i.e. dirs containing files that were created/updated.)

Note that dir metadata (other than modtime) is not currently considered by DirsEqual, consistent with how object metadata is synced (only when objects are unequal for reasons other than metadata).

To sync dir modtimes and metadata unconditionally (the previous behavior), use --ignore-times.

Was the change discussed in an issue or in the forum before?

• rclone copy --metadata not working for folders #6685 (comment)

Checklist

• I have read the contribution guidelines.
• I have added tests for all changes in this PR if appropriate.
• I have added documentation for the changes if appropriate.
• All commit messages are in house style.
• I'm done, this Pull Request is ready for review :-)

[ncw]

This is looking really nice - thank you :-)

I put some specific nits inline.

General comments:

• Please kill path.ToSlash with fire :-) These should not appear in core sync routines - they are supposed to translated to / at the edges and be / within rclone. If \ are getting in to the core then that needs fixing, but not here!
• The bisync tests are quite annoying aren't they!
• I wonder if we could combine the s.modifiedDirs and s.setDirModTimes somehow... We are storing a lot of directory objects we don't need to potentially. Perhaps we don't know until the end of the sync.

[ncw]

This should not need filepath.ToSlash here - all paths should be unix / delimited in rclone internals.

If they aren't then that is a bug somewhere else :-)

Please remove them all from this file! Also see later suggestion for a function markDirModifiedObject...

[nielash]

I removed all the filepath usages here :) I think the error I was seeing the other day may just have been a transient fluke of GitHub Actions (I've been seeing more of those lately)

Here is an example of what I mean though about the path library causing issues on Windows with rclone's unix paths: this test fails only on Windows. (And I confirmed it also fails on a real PC, not just GH Actions)

I think the issue in this particular case is path.Join removing the double // and then rclone adding it back incorrectly. But there may be other issues as well.

I think it would be really handy to have a more comprehensive library of rclone-safe path manipulation functions!

[nielash]

• Please kill path.ToSlash with fire :-) These should not appear in core sync routines - they are supposed to translated to / at the edges and be / within rclone. If \ are getting in to the core then that needs fixing, but not here!

Haha, will do! There definitely is some issue, I think specific to Windows -- I've run into it multiple times on this and other projects. But I agree this is not the proper fix for it. I'll take these out and try to see if I can find the true cause.

• The bisync tests are quite annoying aren't they!

They sure are! I would estimate that my time on this PR was maybe 10% writing code and 90% debugging the bisync tests. But on the other hand, this is a good example of an issue worth knowing about that a more conventional unit test would probably miss.

• I wonder if we could combine the s.modifiedDirs and s.setDirModTimes somehow... We are storing a lot of directory objects we don't need to potentially. Perhaps we don't know until the end of the sync.

Yeah, that is basically the problem. At the point that s.setDirModTimes is set, we haven't yet recursed into the directory and so we don't know if there are changed files within it. It might be possible to do it after each directory is done (instead of waiting til the end of the sync), or at least release the ones we know we definitely won't need, but I'm not sure how much difference that would make, and would probably add a lot of complexity trying to coordinate that across channels. IMO it doesn't seem worth it.

[ncw]

I think this is looking great except for fspath.Dir which isn't correct!

See suggestions inline - thank you :-)

[ncw]

Perfect - thank you :-)

I'll merge this now.