From 47735d8fe1a0ad601d1720e7ee9693bfae2294e8 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Wed, 17 Apr 2024 16:55:17 +0100 Subject: [PATCH] sync: fix failed to update directory timestamp or metadata: directory not found See: https://forum.rclone.org/t/empty-dirs-not-wanted/45059/14 Co-authored-by: nielash --- fs/sync/sync.go | 35 +++++++++++++++++++++++++++++------ fs/sync/sync_test.go | 29 +++++++++++++++++++++++------ 2 files changed, 52 insertions(+), 12 deletions(-) diff --git a/fs/sync/sync.go b/fs/sync/sync.go index 7a690bbf5..5d0bee896 100644 --- a/fs/sync/sync.go +++ b/fs/sync/sync.go @@ -733,7 +733,8 @@ func (s *syncCopyMove) markParentNotEmpty(entry fs.DirEntry) { s.srcEmptyDirsMu.Lock() defer s.srcEmptyDirsMu.Unlock() // Mark entry as potentially empty if it is a directory - if _, isDir := entry.(fs.Directory); isDir { + _, isDir := entry.(fs.Directory) + if isDir { s.srcEmptyDirs[entry.Remote()] = entry // if DoMove and --delete-empty-src-dirs flag is set then record the parent but // don't remove any as we are about to move files out of them them making the @@ -742,12 +743,27 @@ func (s *syncCopyMove) markParentNotEmpty(entry fs.DirEntry) { s.srcMoveEmptyDirs[entry.Remote()] = entry } } - // Mark its parent as not empty parentDir := path.Dir(entry.Remote()) - if parentDir == "." { - parentDir = "" + if isDir && s.copyEmptySrcDirs { + // Mark its parent as not empty + if parentDir == "." { + parentDir = "" + } + delete(s.srcEmptyDirs, parentDir) + } + if !isDir { + // Mark ALL its parents as not empty + for { + if parentDir == "." { + parentDir = "" + } + delete(s.srcEmptyDirs, parentDir) + if parentDir == "" { + break + } + parentDir = path.Dir(parentDir) + } } - delete(s.srcEmptyDirs, parentDir) } // parseTrackRenamesStrategy turns a config string into a trackRenamesStrategy @@ -1213,6 +1229,13 @@ func (s *syncCopyMove) setDelayedDirModTimes(ctx context.Context) error { } } item := item + if s.setDirModTimeAfter { // mark dir's parent as modified + dir := path.Dir(item.dir) + if dir == "." { + dir = "" + } + s.modifiedDirs[dir] = struct{}{} // lock is already held + } g.Go(func() error { var err error // if item.src is set must copy full metadata @@ -1277,8 +1300,8 @@ func (s *syncCopyMove) SrcOnly(src fs.DirEntry) (recurse bool) { s.logger(s.ctx, operations.MissingOnDst, src, nil, fs.ErrorIsDir) // Create the directory and make sure the Metadata/ModTime is correct - s.markDirModified(x.Remote()) s.copyDirMetadata(s.ctx, s.fdst, nil, x.Remote(), x) + s.markDirModified(x.Remote()) return true default: panic("Bad object in DirEntries") diff --git a/fs/sync/sync_test.go b/fs/sync/sync_test.go index 1287901e4..5516e5d91 100644 --- a/fs/sync/sync_test.go +++ b/fs/sync/sync_test.go @@ -305,7 +305,9 @@ func TestCopyEmptyDirectories(t *testing.T) { ctx := context.Background() r := fstest.NewRun(t) file1 := r.WriteFile("sub dir/hello world", "hello world", t1) - _, err := operations.MkdirModTime(ctx, r.Flocal, "sub dir2", t2) + _, err := operations.MkdirModTime(ctx, r.Flocal, "sub dir2/sub sub dir2", t2) + require.NoError(t, err) + _, err = operations.SetDirModTime(ctx, r.Flocal, nil, "sub dir2", t2) require.NoError(t, err) r.Mkdir(ctx, r.Fremote) @@ -327,11 +329,12 @@ func TestCopyEmptyDirectories(t *testing.T) { []string{ "sub dir", "sub dir2", + "sub dir2/sub sub dir2", }, ) // Check that the modtimes of the directories are as expected - r.CheckDirectoryModTimes(t, "sub dir", "sub dir2") + r.CheckDirectoryModTimes(t, "sub dir", "sub dir2", "sub dir2/sub sub dir2") } // Test copy empty directories when we are configured not to create them @@ -341,6 +344,8 @@ func TestCopyNoEmptyDirectories(t *testing.T) { file1 := r.WriteFile("sub dir/hello world", "hello world", t1) err := operations.Mkdir(ctx, r.Flocal, "sub dir2") require.NoError(t, err) + _, err = operations.MkdirModTime(ctx, r.Flocal, "sub dir2/sub sub dir2", t2) + require.NoError(t, err) r.Mkdir(ctx, r.Fremote) err = CopyDir(ctx, r.Fremote, r.Flocal, false) @@ -2668,7 +2673,7 @@ func testNothingToTransfer(t *testing.T, copyEmptySrcDirs bool) { r.CheckLocalItems(t, file1, file2) r.CheckRemoteItems(t, file1, file2) // Check that the modtimes of the directories are as expected - r.CheckDirectoryModTimes(t, "sub dir") + r.CheckDirectoryModTimes(t, "sub dir", "sub dir2", "sub dir2/very", "sub dir2/very/very", "sub dir2/very/very/very/very/very/nested/subdir") // check that actions were taken assert.True(t, strings.Contains(string(output), "Copied"), `expected to find at least one "Copied" log: `+string(output)) @@ -2690,7 +2695,7 @@ func testNothingToTransfer(t *testing.T, copyEmptySrcDirs bool) { r.CheckLocalItems(t, file1, file2) r.CheckRemoteItems(t, file1, file2) // Check that the modtimes of the directories are as expected - r.CheckDirectoryModTimes(t, "sub dir") + r.CheckDirectoryModTimes(t, "sub dir", "sub dir2", "sub dir2/very", "sub dir2/very/very", "sub dir2/very/very/very/very/very/nested/subdir") // check that actions were NOT taken assert.False(t, strings.Contains(string(output), "Copied"), `expected to find no "Copied" logs, but found one: `+string(output)) @@ -2702,7 +2707,7 @@ func testNothingToTransfer(t *testing.T, copyEmptySrcDirs bool) { assert.True(t, strings.Contains(string(output), "There was nothing to transfer"), `expected to find a "There was nothing to transfer" log: `+string(output)) assert.Equal(t, int64(0), accounting.GlobalStats().GetTransfers()) - // make a change in one dir and check that parent isn't changed + // check nested empty dir behavior (FIXME: probably belongs in a separate test) if r.Fremote.Features().DirSetModTime == nil && r.Fremote.Features().MkdirMetadata == nil { return } @@ -2711,6 +2716,10 @@ func testNothingToTransfer(t *testing.T, copyEmptySrcDirs bool) { assert.NoError(t, err) _, err = operations.SetDirModTime(ctx, r.Fremote, nil, "sub dir2", t1) assert.NoError(t, err) + _, err = operations.MkdirModTime(ctx, r.Flocal, "sub dirEmpty/sub dirEmpty2", t2) + assert.NoError(t, err) + _, err = operations.SetDirModTime(ctx, r.Flocal, nil, "sub dirEmpty", t2) + assert.NoError(t, err) accounting.GlobalStats().ResetCounters() ctx = predictDstFromLogger(ctx) @@ -2722,8 +2731,16 @@ func testNothingToTransfer(t *testing.T, copyEmptySrcDirs bool) { 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, "sub dir", "sub dir2", "sub dir2/very", "sub dir2/very/very", "sub dir2/very/very/very/very/very/nested/subdir", "sub dir2/sub dir3") + if copyEmptySrcDirs { + r.CheckDirectoryModTimes(t, "sub dirEmpty", "sub dirEmpty/sub dirEmpty2") + assert.True(t, strings.Contains(string(output), "sub dirEmpty:"), `expected to find at least one "sub dirEmpty:" log: `+string(output)) + } else { + assert.False(t, strings.Contains(string(output), "sub dirEmpty:"), `expected to find no "sub dirEmpty:" logs, but found one (empty dir was synced and shouldn't have been): `+string(output)) + } assert.True(t, strings.Contains(string(output), "sub dir3:"), `expected to find at least one "sub dir3:" log: `+string(output)) - assert.False(t, strings.Contains(string(output), "sub dir2:"), `expected to find no "sub dir2:" logs, but found one (unmodified dir was marked modified): `+string(output)) + assert.False(t, strings.Contains(string(output), "sub dir2/very:"), `expected to find no "sub dir2/very:" logs, but found one (unmodified dir was marked modified): `+string(output)) } func TestNothingToTransferWithEmptyDirs(t *testing.T) {