From 4b27c6719b636dbafde4fea03e47915ec7c78a3c Mon Sep 17 00:00:00 2001 From: forgems Date: Sun, 9 Jun 2019 16:57:05 +0200 Subject: [PATCH] fs: Allow sync of a file and a directory with the same name When sorting fs.DirEntries we sort by DirEntry type and when synchronizing files let the directories be before objects, so when the destintation fs doesn't support duplicate names, we will only lose duplicated object instead of whole directory. The enables synchronisation to work with a file and a directory of the same name which is reasonably common on bucket based remotes. --- fs/direntries.go | 27 +++++++++- fs/direntries_test.go | 26 ++++++++++ fs/march/march.go | 31 ++++++----- fs/march/march_test.go | 113 ++++++++++++++++++++++++++++++++--------- 4 files changed, 161 insertions(+), 36 deletions(-) create mode 100644 fs/direntries_test.go diff --git a/fs/direntries.go b/fs/direntries.go index b3ae9eaad..72839a417 100644 --- a/fs/direntries.go +++ b/fs/direntries.go @@ -17,7 +17,7 @@ func (ds DirEntries) Swap(i, j int) { // Less is part of sort.Interface. func (ds DirEntries) Less(i, j int) bool { - return ds[i].Remote() < ds[j].Remote() + return CompareDirEntries(ds[i], ds[j]) < 0 } // ForObject runs the function supplied on every object in the entries @@ -79,3 +79,28 @@ func DirEntryType(d DirEntry) string { } return fmt.Sprintf("unknown type %T", d) } + +// CompareDirEntries returns 1 if a > b, 0 if a == b and -1 if a < b +// If two dir entries have the same name, compare their types (directories are before objects) +func CompareDirEntries(a, b DirEntry) int { + aName := a.Remote() + bName := b.Remote() + + if aName > bName { + return 1 + } else if aName < bName { + return -1 + } + + typeA := DirEntryType(a) + typeB := DirEntryType(b) + + // same name, compare types + if typeA > typeB { + return 1 + } else if typeA < typeB { + return -1 + } + + return 0 +} diff --git a/fs/direntries_test.go b/fs/direntries_test.go new file mode 100644 index 000000000..b2c1e9ea4 --- /dev/null +++ b/fs/direntries_test.go @@ -0,0 +1,26 @@ +package fs_test + +import ( + "sort" + "testing" + + "github.com/ncw/rclone/fs" + "github.com/ncw/rclone/fstest/mockdir" + "github.com/ncw/rclone/fstest/mockobject" + "github.com/stretchr/testify/assert" +) + +func TestDirEntriesSort(t *testing.T) { + a := mockobject.New("a") + aDir := mockdir.New("a") + b := mockobject.New("b") + bDir := mockdir.New("b") + c := mockobject.New("c") + cDir := mockdir.New("c") + anotherc := mockobject.New("c") + dirEntries := fs.DirEntries{bDir, b, aDir, a, c, cDir, anotherc} + + sort.Stable(dirEntries) + + assert.Equal(t, fs.DirEntries{aDir, a, bDir, b, cDir, c, anotherc}, dirEntries) +} diff --git a/fs/march/march.go b/fs/march/march.go index 7b9ca5d53..0009d6695 100644 --- a/fs/march/march.go +++ b/fs/march/march.go @@ -213,7 +213,7 @@ func (es matchEntries) Less(i, j int) bool { ei, ej := &es[i], &es[j] if ei.name == ej.name { if ei.leaf == ej.leaf { - return ei.entry.Remote() < ej.entry.Remote() + return fs.CompareDirEntries(ei.entry, ej.entry) < 0 } return ei.leaf < ej.leaf } @@ -267,6 +267,7 @@ type matchTransformFn func(name string) string func matchListings(srcListEntries, dstListEntries fs.DirEntries, transforms []matchTransformFn) (srcOnly fs.DirEntries, dstOnly fs.DirEntries, matches []matchPair) { srcList := newMatchEntries(srcListEntries, transforms) dstList := newMatchEntries(dstListEntries, transforms) + for iSrc, iDst := 0, 0; ; iSrc, iDst = iSrc+1, iDst+1 { var src, dst fs.DirEntry var srcName, dstName string @@ -282,34 +283,40 @@ func matchListings(srcListEntries, dstListEntries fs.DirEntries, transforms []ma break } if src != nil && iSrc > 0 { - prev := srcList[iSrc-1].name - if srcName == prev { + prev := srcList[iSrc-1].entry + prevName := srcList[iSrc-1].name + if srcName == prevName && fs.DirEntryType(prev) == fs.DirEntryType(src) { fs.Logf(src, "Duplicate %s found in source - ignoring", fs.DirEntryType(src)) iDst-- // ignore the src and retry the dst continue - } else if srcName < prev { + } else if srcName < prevName { // this should never happen since we sort the listings panic("Out of order listing in source") } } if dst != nil && iDst > 0 { - prev := dstList[iDst-1].name - if dstName == prev { + prev := dstList[iDst-1].entry + prevName := dstList[iDst-1].name + if dstName == prevName && fs.DirEntryType(dst) == fs.DirEntryType(prev) { fs.Logf(dst, "Duplicate %s found in destination - ignoring", fs.DirEntryType(dst)) iSrc-- // ignore the dst and retry the src continue - } else if dstName < prev { + } else if dstName < prevName { // this should never happen since we sort the listings panic("Out of order listing in destination") } } if src != nil && dst != nil { - if srcName < dstName { - dst = nil - iDst-- // retry the dst - } else if srcName > dstName { + // we can't use CompareDirEntries because srcName, dstName could + // be different then src.Remote() or dst.Remote() + srcType := fs.DirEntryType(src) + dstType := fs.DirEntryType(dst) + if srcName > dstName || (srcName == dstName && srcType > dstType) { src = nil - iSrc-- // retry the src + iSrc-- + } else if srcName < dstName || (srcName == dstName && srcType < dstType) { + dst = nil + iDst-- } } // Debugf(nil, "src = %v, dst = %v", src, dst) diff --git a/fs/march/march_test.go b/fs/march/march_test.go index 19a8af8f9..3b85574d3 100644 --- a/fs/march/march_test.go +++ b/fs/march/march_test.go @@ -3,10 +3,12 @@ package march import ( + "fmt" "strings" "testing" "github.com/ncw/rclone/fs" + "github.com/ncw/rclone/fstest/mockdir" "github.com/ncw/rclone/fstest/mockobject" "github.com/stretchr/testify/assert" ) @@ -38,11 +40,13 @@ func TestNewMatchEntries(t *testing.T) { func TestMatchListings(t *testing.T) { var ( - a = mockobject.Object("a") - A = mockobject.Object("A") - b = mockobject.Object("b") - c = mockobject.Object("c") - d = mockobject.Object("d") + a = mockobject.Object("a") + A = mockobject.Object("A") + b = mockobject.Object("b") + c = mockobject.Object("c") + d = mockobject.Object("d") + dirA = mockdir.New("A") + dirb = mockdir.New("b") ) for _, test := range []struct { @@ -147,25 +151,88 @@ func TestMatchListings(t *testing.T) { }, transforms: []matchTransformFn{strings.ToLower}, }, + { + what: "File and directory are not duplicates - srcOnly", + input: fs.DirEntries{ + dirA, nil, + A, nil, + }, + srcOnly: fs.DirEntries{ + dirA, + A, + }, + }, + { + what: "File and directory are not duplicates - matches", + input: fs.DirEntries{ + dirA, dirA, + A, A, + }, + matches: []matchPair{ + {dirA, dirA}, + {A, A}, + }, + }, + { + what: "Sync with directory #1", + input: fs.DirEntries{ + dirA, nil, + A, nil, + b, b, + nil, c, + nil, d, + }, + srcOnly: fs.DirEntries{ + dirA, + A, + }, + dstOnly: fs.DirEntries{ + c, d, + }, + matches: []matchPair{ + {b, b}, + }, + }, + { + what: "Sync with 2 directories", + input: fs.DirEntries{ + dirA, dirA, + A, nil, + nil, dirb, + nil, b, + }, + srcOnly: fs.DirEntries{ + A, + }, + dstOnly: fs.DirEntries{ + dirb, + b, + }, + matches: []matchPair{ + {dirA, dirA}, + }, + }, } { - var srcList, dstList fs.DirEntries - for i := 0; i < len(test.input); i += 2 { - src, dst := test.input[i], test.input[i+1] - if src != nil { - srcList = append(srcList, src) + t.Run(fmt.Sprintf("TestMatchListings-%s", test.what), func(t *testing.T) { + var srcList, dstList fs.DirEntries + for i := 0; i < len(test.input); i += 2 { + src, dst := test.input[i], test.input[i+1] + if src != nil { + srcList = append(srcList, src) + } + if dst != nil { + dstList = append(dstList, dst) + } } - if dst != nil { - dstList = append(dstList, dst) - } - } - srcOnly, dstOnly, matches := matchListings(srcList, dstList, test.transforms) - assert.Equal(t, test.srcOnly, srcOnly, test.what) - assert.Equal(t, test.dstOnly, dstOnly, test.what) - assert.Equal(t, test.matches, matches, test.what) - // now swap src and dst - dstOnly, srcOnly, matches = matchListings(dstList, srcList, test.transforms) - assert.Equal(t, test.srcOnly, srcOnly, test.what) - assert.Equal(t, test.dstOnly, dstOnly, test.what) - assert.Equal(t, test.matches, matches, test.what) + srcOnly, dstOnly, matches := matchListings(srcList, dstList, test.transforms) + assert.Equal(t, test.srcOnly, srcOnly, test.what, "srcOnly differ") + assert.Equal(t, test.dstOnly, dstOnly, test.what, "dstOnly differ") + assert.Equal(t, test.matches, matches, test.what, "matches differ") + // now swap src and dst + dstOnly, srcOnly, matches = matchListings(dstList, srcList, test.transforms) + assert.Equal(t, test.srcOnly, srcOnly, test.what, "srcOnly differ") + assert.Equal(t, test.dstOnly, dstOnly, test.what, "dstOnly differ") + assert.Equal(t, test.matches, matches, test.what, "matches differ") + }) } }