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") + }) } }