Skip to content

Commit

Permalink
sync: fix creation of empty directories when --create-empty-src-dirs=…
Browse files Browse the repository at this point in the history 
…false

In v1.66.0 the changes to enable metadata preservation on directories
introduced a regression, namely that empty directories were created
despite the state of the --create-empty-src-dirs flag.

This patch fixes the problem by letting the normal rclone directory
creation create the directories and fixing up their timestamps and
metadata afterwards if --create-empty-src-dirs=false.

Fixes rclone#7689
See: https://forum.rclone.org/t/empty-dirs-not-wanted/45059/
See: https://forum.rclone.org/t/how-to-ignore-empty-directories-when-uploading-from-windows/45057/
  • Loading branch information
@ncw @Fornax96
ncw authored and Fornax96 committed Jul 30, 2024
1 parent 56a4dbe commit 0e1ab6e
Show file tree
Hide file tree
Showing 4 changed files with 162 additions and 22 deletions.
2 changes: 2 additions & 0 deletions cmd/bisync/bisync_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ var logReplacements = []string{
`^(INFO : .*?: (Made directory with|Set directory) (metadata|modification time)).*$`, dropMe,
// ignore sizes in directory time updates
`^(NOTICE: .*?: Skipped set directory modification time as --dry-run is set).*$`, dropMe,
// ignore sizes in directory metadata updates
`^(NOTICE: .*?: Skipped update directory metadata as --dry-run is set).*$`, dropMe,
}

// Some dry-run messages differ depending on the particular remote.
Expand Down
39 changes: 30 additions & 9 deletions fs/sync/sync.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ type syncCopyMove struct {

// For keeping track of delayed modtime sets
type setDirModTime struct {
src fs.Directory // if set the metadata should be set too
dst fs.Directory
dir string
modTime time.Time
Expand Down Expand Up @@ -157,7 +158,7 @@ func newSyncCopyMove(ctx context.Context, fdst, fsrc fs.Fs, deleteMode fs.Delete
checkFirst: ci.CheckFirst,
setDirMetadata: ci.Metadata && fsrc.Features().ReadDirMetadata && fdst.Features().WriteDirMetadata,
setDirModTime: (!ci.NoUpdateDirModTime && fsrc.Features().CanHaveEmptyDirectories) && (fdst.Features().WriteDirSetModTime || fdst.Features().MkdirMetadata != nil || fdst.Features().DirSetModTime != nil),
setDirModTimeAfter: !ci.NoUpdateDirModTime && fsrc.Features().CanHaveEmptyDirectories && fdst.Features().DirModTimeUpdatesOnWrite,
setDirModTimeAfter: !ci.NoUpdateDirModTime && (!copyEmptySrcDirs || fsrc.Features().CanHaveEmptyDirectories && fdst.Features().DirModTimeUpdatesOnWrite),
modifiedDirs: make(map[string]struct{}),
}

Expand Down Expand Up @@ -1128,9 +1129,10 @@ func (s *syncCopyMove) copyDirMetadata(ctx context.Context, f fs.Fs, dst fs.Dire
if !s.setDirModTimeAfter && equal {
return nil
}
setMeta := true
if s.setDirModTimeAfter && equal {
newDst = dst
} else {
} else if s.copyEmptySrcDirs {
if s.setDirMetadata {
newDst, err = operations.CopyDirMetadata(ctx, f, dst, dir, src)
} else if s.setDirModTime {
Expand All @@ -1143,6 +1145,8 @@ func (s *syncCopyMove) copyDirMetadata(ctx context.Context, f fs.Fs, dst fs.Dire
// Create the directory if it doesn't exist
err = operations.Mkdir(ctx, f, dir)
}
} else {
setMeta = s.setDirMetadata
}
// If we need to set modtime after and we created a dir, then save it for later
if s.setDirModTime && s.setDirModTimeAfter && err == nil {
Expand All @@ -1159,12 +1163,16 @@ func (s *syncCopyMove) copyDirMetadata(ctx context.Context, f fs.Fs, dst fs.Dire
if level > s.setDirModTimesMaxLevel {
s.setDirModTimesMaxLevel = level
}
s.setDirModTimes = append(s.setDirModTimes, setDirModTime{
set := setDirModTime{
dst: newDst,
dir: dir,
modTime: src.ModTime(ctx),
level: level,
})
}
if setMeta {
set.src = src
}
s.setDirModTimes = append(s.setDirModTimes, set)
s.setDirModTimeMu.Unlock()
fs.Debugf(nil, "Added delayed dir = %q, newDst=%v", dir, newDst)
}
Expand Down Expand Up @@ -1195,15 +1203,28 @@ func (s *syncCopyMove) setDelayedDirModTimes(ctx context.Context) error {
if gCtx.Err() != nil {
break
}
item := item
if _, ok := s.modifiedDirs[item.dir]; !ok {
continue
if item.src == nil {
if _, ok := s.modifiedDirs[item.dir]; !ok {
continue
}
}
if !s.copyEmptySrcDirs {
if _, isEmpty := s.srcEmptyDirs[item.dir]; isEmpty {
continue
}
}
item := item
g.Go(func() error {
_, err := operations.SetDirModTime(gCtx, s.fdst, item.dst, item.dir, item.modTime)
var err error
// if item.src is set must copy full metadata
if item.src != nil {
_, err = operations.CopyDirMetadata(gCtx, s.fdst, item.dst, item.dir, item.src)
} else {
_, err = operations.SetDirModTime(gCtx, s.fdst, item.dst, item.dir, item.modTime)
}
if err != nil {
err = fs.CountError(err)
fs.Errorf(item.dir, "Failed to timestamp directory: %v", err)
fs.Errorf(item.dir, "Failed to update directory timestamp or metadata: %v", err)
errCount.Add(err)
}
return nil // don't return errors, just count them
Expand Down
115 changes: 110 additions & 5 deletions fs/sync/sync_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func TestCopy(t *testing.T) {
r.CheckDirectoryModTimes(t, "sub dir")
}

func TestCopyMetadata(t *testing.T) {
func testCopyMetadata(t *testing.T, createEmptySrcDirs bool) {
ctx := context.Background()
ctx, ci := fs.AddConfig(ctx)
ci.Metadata = true
Expand All @@ -99,6 +99,7 @@ func TestCopyMetadata(t *testing.T) {

const content = "hello metadata world!"
const dirPath = "metadata sub dir"
const emptyDirPath = "empty metadata sub dir"
const filePath = dirPath + "/hello metadata world"

fileMetadata := fs.Metadata{
Expand All @@ -119,6 +120,10 @@ func TestCopyMetadata(t *testing.T) {
_, err := operations.MkdirMetadata(ctx, r.Flocal, dirPath, dirMetadata)
require.NoError(t, err)

// Make the empty directory with metadata - may fall back to Mkdir
_, err = operations.MkdirMetadata(ctx, r.Flocal, emptyDirPath, dirMetadata)
require.NoError(t, err)

// Upload the file with metadata
in := io.NopCloser(bytes.NewBufferString(content))
_, err = operations.Rcat(ctx, r.Flocal, filePath, in, t1, fileMetadata)
Expand All @@ -132,7 +137,7 @@ func TestCopyMetadata(t *testing.T) {
}

ctx = predictDstFromLogger(ctx)
err = CopyDir(ctx, r.Fremote, r.Flocal, false)
err = CopyDir(ctx, r.Fremote, r.Flocal, createEmptySrcDirs)
require.NoError(t, err)
testLoggerVsLsf(ctx, r.Fremote, operations.GetLoggerOpt(ctx).JSON, t)

Expand All @@ -149,6 +154,26 @@ func TestCopyMetadata(t *testing.T) {
if features.ReadDirMetadata {
fstest.CheckEntryMetadata(ctx, t, r.Fremote, fstest.NewDirectory(ctx, t, r.Fremote, dirPath), dirMetadata)
}
if !createEmptySrcDirs {
// dir must not exist
_, err := fstest.NewDirectoryRetries(ctx, t, r.Fremote, emptyDirPath, 1)
assert.Error(t, err, "Not expecting to find empty directory")
assert.True(t, errors.Is(err, fs.ErrorDirNotFound), fmt.Sprintf("expecting wrapped %#v not: %#v", fs.ErrorDirNotFound, err))
} else {
// dir must exist
dir := fstest.NewDirectory(ctx, t, r.Fremote, emptyDirPath)
if features.ReadDirMetadata {
fstest.CheckEntryMetadata(ctx, t, r.Fremote, dir, dirMetadata)
}
}
}

func TestCopyMetadata(t *testing.T) {
testCopyMetadata(t, true)
}

func TestCopyMetadataNoEmptyDirs(t *testing.T) {
testCopyMetadata(t, false)
}

func TestCopyMissingDirectory(t *testing.T) {
Expand Down Expand Up @@ -309,6 +334,29 @@ func TestCopyEmptyDirectories(t *testing.T) {
r.CheckDirectoryModTimes(t, "sub dir", "sub dir2")
}

// Test copy empty directories when we are configured not to create them
func TestCopyNoEmptyDirectories(t *testing.T) {
ctx := context.Background()
r := fstest.NewRun(t)
file1 := r.WriteFile("sub dir/hello world", "hello world", t1)
err := operations.Mkdir(ctx, r.Flocal, "sub dir2")
require.NoError(t, err)
r.Mkdir(ctx, r.Fremote)

err = CopyDir(ctx, r.Fremote, r.Flocal, false)
require.NoError(t, err)

r.CheckRemoteListing(
t,
[]fstest.Item{
file1,
},
[]string{
"sub dir",
},
)
}

// Test move empty directories
func TestMoveEmptyDirectories(t *testing.T) {
ctx := context.Background()
Expand Down Expand Up @@ -383,6 +431,29 @@ func TestSyncNoUpdateDirModtime(t *testing.T) {
fstest.AssertTimeEqualWithPrecision(t, name, wantT, gotT, r.Fremote.Precision())
}

// Test move empty directories when we are not configured to create them
func TestMoveNoEmptyDirectories(t *testing.T) {
ctx := context.Background()
r := fstest.NewRun(t)
file1 := r.WriteFile("sub dir/hello world", "hello world", t1)
err := operations.Mkdir(ctx, r.Flocal, "sub dir2")
require.NoError(t, err)
r.Mkdir(ctx, r.Fremote)

err = MoveDir(ctx, r.Fremote, r.Flocal, false, false)
require.NoError(t, err)

r.CheckRemoteListing(
t,
[]fstest.Item{
file1,
},
[]string{
"sub dir",
},
)
}

// Test sync empty directories
func TestSyncEmptyDirectories(t *testing.T) {
ctx := context.Background()
Expand Down Expand Up @@ -474,6 +545,29 @@ func TestSyncSetDelayedModTimes(t *testing.T) {
r.CheckDirectoryModTimes(t, dirs...)
}

// Test sync empty directories when we are not configured to create them
func TestSyncNoEmptyDirectories(t *testing.T) {
ctx := context.Background()
r := fstest.NewRun(t)
file1 := r.WriteFile("sub dir/hello world", "hello world", t1)
err := operations.Mkdir(ctx, r.Flocal, "sub dir2")
require.NoError(t, err)
r.Mkdir(ctx, r.Fremote)

err = Sync(ctx, r.Fremote, r.Flocal, false)
require.NoError(t, err)

r.CheckRemoteListing(
t,
[]fstest.Item{
file1,
},
[]string{
"sub dir",
},
)
}

// Test a server-side copy if possible, or the backup path if not
func TestServerSideCopy(t *testing.T) {
ctx := context.Background()
Expand Down Expand Up @@ -2541,7 +2635,7 @@ func TestSyncConcurrentTruncate(t *testing.T) {

// 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) {
func testNothingToTransfer(t *testing.T, copyEmptySrcDirs bool) {
accounting.GlobalStats().ResetCounters()
ctx, _ := fs.AddConfig(context.Background())
r := fstest.NewRun(t)
Expand All @@ -2566,7 +2660,7 @@ func TestNothingToTransfer(t *testing.T) {
accounting.GlobalStats().ResetCounters()
ctx = predictDstFromLogger(ctx)
output := bilib.CaptureOutput(func() {
err = CopyDir(ctx, r.Fremote, r.Flocal, true)
err = CopyDir(ctx, r.Fremote, r.Flocal, copyEmptySrcDirs)
require.NoError(t, err)
})
require.NotNil(t, output)
Expand All @@ -2580,6 +2674,7 @@ func TestNothingToTransfer(t *testing.T) {
assert.True(t, strings.Contains(string(output), "Copied"), `expected to find at least one "Copied" log: `+string(output))
if r.Fremote.Features().DirSetModTime != nil || r.Fremote.Features().MkdirMetadata != nil {
assert.True(t, strings.Contains(string(output), "Set directory modification time"), `expected to find at least one "Set directory modification time" log: `+string(output))
assert.True(t, strings.Contains(string(output), "Made directory with metadata"), `expected to find at least one "Made directory with metadata" log: `+string(output))
}
assert.False(t, strings.Contains(string(output), "There was nothing to transfer"), `expected to find no "There was nothing to transfer" logs, but found one: `+string(output))
assert.True(t, accounting.GlobalStats().GetTransfers() >= 2)
Expand All @@ -2588,7 +2683,7 @@ func TestNothingToTransfer(t *testing.T) {
accounting.GlobalStats().ResetCounters()
ctx = predictDstFromLogger(ctx)
output = bilib.CaptureOutput(func() {
err = CopyDir(ctx, r.Fremote, r.Flocal, true)
err = CopyDir(ctx, r.Fremote, r.Flocal, copyEmptySrcDirs)
require.NoError(t, err)
})
require.NotNil(t, output)
Expand All @@ -2602,11 +2697,21 @@ func TestNothingToTransfer(t *testing.T) {
assert.False(t, strings.Contains(string(output), "Copied"), `expected to find no "Copied" logs, but found one: `+string(output))
if r.Fremote.Features().DirSetModTime != nil || r.Fremote.Features().MkdirMetadata != nil {
assert.False(t, strings.Contains(string(output), "Set directory modification time"), `expected to find no "Set directory modification time" logs, but found one: `+string(output))
assert.False(t, strings.Contains(string(output), "Updated directory metadata"), `expected to find no "Updated directory metadata" logs, but found one: `+string(output))
assert.False(t, strings.Contains(string(output), "directory"), `expected to find no "directory"-related logs, but found one: `+string(output)) // catch-all
}
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())
}

func TestNothingToTransferWithEmptyDirs(t *testing.T) {
testNothingToTransfer(t, true)
}

func TestNothingToTransferWithoutEmptyDirs(t *testing.T) {
testNothingToTransfer(t, false)
}

// for testing logger:
func predictDstFromLogger(ctx context.Context) context.Context {
opt := operations.NewLoggerOpt()
Expand Down
28 changes: 20 additions & 8 deletions fstest/fstest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -539,18 +539,20 @@ func NewObject(ctx context.Context, t *testing.T, f fs.Fs, remote string) fs.Obj
return obj
}

// NewDirectory finds the directory with remote in f
// NewDirectoryRetries finds the directory with remote in f
//
// If directory can't be found it returns an error wrapping fs.ErrorDirNotFound
//
// One day this will be an rclone primitive
func NewDirectory(ctx context.Context, t *testing.T, f fs.Fs, remote string) fs.Directory {
func NewDirectoryRetries(ctx context.Context, t *testing.T, f fs.Fs, remote string, retries int) (fs.Directory, error) {
var err error
var dir fs.Directory
sleepTime := 1 * time.Second
root := path.Dir(remote)
if root == "." {
root = ""
}
for i := 1; i <= *ListRetries; i++ {
for i := 1; i <= retries; i++ {
var entries fs.DirEntries
entries, err = f.List(ctx, root)
if err != nil {
Expand All @@ -560,14 +562,24 @@ func NewDirectory(ctx context.Context, t *testing.T, f fs.Fs, remote string) fs.
var ok bool
dir, ok = entry.(fs.Directory)
if ok && dir.Remote() == remote {
return dir
return dir, nil
}
}
err = fmt.Errorf("directory %q not found in %q", remote, root)
t.Logf("Sleeping for %v for findDir eventual consistency: %d/%d (%v)", sleepTime, i, *ListRetries, err)
time.Sleep(sleepTime)
sleepTime = (sleepTime * 3) / 2
err = fmt.Errorf("directory %q not found in %q: %w", remote, root, fs.ErrorDirNotFound)
if i < retries {
t.Logf("Sleeping for %v for NewDirectoryRetries eventual consistency: %d/%d (%v)", sleepTime, i, retries, err)
time.Sleep(sleepTime)
sleepTime = (sleepTime * 3) / 2
}
}
return dir, err
}

// NewDirectory finds the directory with remote in f
//
// One day this will be an rclone primitive
func NewDirectory(ctx context.Context, t *testing.T, f fs.Fs, remote string) fs.Directory {
dir, err := NewDirectoryRetries(ctx, t, f, remote, *ListRetries)
require.NoError(t, err)
return dir
}
Expand Down

0 comments on commit 0e1ab6e

Please sign in to comment.