From dd6e229327c7a34005c29af4194bdad9c6ced21d Mon Sep 17 00:00:00 2001
From: Nick Craig-Wood <nick@craig-wood.com>
Date: Wed, 8 Mar 2023 13:03:05 +0000
Subject: [PATCH] move: if --check-first and --order-by are set then delete
 with perfect ordering

If using rclone move and --check-first and --order-by then rclone uses
the transfer routine to delete files to ensure perfect ordering.

This will cause the transfer stats to have a larger than expected
number of items in it so we don't enable this by default.

Fixes #6033
---
 docs/content/docs.md |  6 ++++++
 fs/sync/pipe.go      |  6 ++++--
 fs/sync/pipe_test.go | 12 ++++++++++++
 fs/sync/sync.go      | 18 ++++++++++++++++--
 4 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/docs/content/docs.md b/docs/content/docs.md
index ff198e50d..5a9f441fb 100644
--- a/docs/content/docs.md
+++ b/docs/content/docs.md
@@ -789,6 +789,12 @@ interfere with checking.
 It can also be useful to ensure perfect ordering when using
 `--order-by`.
 
+If both `--check-first` and `--order-by` are set when doing `rclone move`
+then rclone will use the transfer thread to delete source files which
+don't need transferring. This will enable perfect ordering of the
+transfers and deletes but will cause the transfer stats to have more
+items in than expected.
+
 Using this flag can use more memory as it effectively sets
 `--max-backlog` to infinite. This means that all the info on the
 objects to transfer is held in memory before the transfers start.
diff --git a/fs/sync/pipe.go b/fs/sync/pipe.go
index a2aa701bc..a4a6fa621 100644
--- a/fs/sync/pipe.go
+++ b/fs/sync/pipe.go
@@ -85,6 +85,8 @@ func (p *pipe) Pop() interface{} {
 // It returns ok = false if the context was cancelled
 //
 // It will panic if you call it after Close()
+//
+// Note that pairs where src==dst aren't counted for stats
 func (p *pipe) Put(ctx context.Context, pair fs.ObjectPair) (ok bool) {
 	if ctx.Err() != nil {
 		return false
@@ -97,7 +99,7 @@ func (p *pipe) Put(ctx context.Context, pair fs.ObjectPair) (ok bool) {
 		deheap.Push(p, pair)
 	}
 	size := pair.Src.Size()
-	if size > 0 {
+	if size > 0 && pair.Src != pair.Dst {
 		p.totalSize += size
 	}
 	p.stats(len(p.queue), p.totalSize)
@@ -141,7 +143,7 @@ func (p *pipe) GetMax(ctx context.Context, fraction int) (pair fs.ObjectPair, ok
 		pair = deheap.PopMax(p).(fs.ObjectPair)
 	}
 	size := pair.Src.Size()
-	if size > 0 {
+	if size > 0 && pair.Src != pair.Dst {
 		p.totalSize -= size
 	}
 	if p.totalSize < 0 {
diff --git a/fs/sync/pipe_test.go b/fs/sync/pipe_test.go
index 84c9129e5..2d56506ce 100644
--- a/fs/sync/pipe_test.go
+++ b/fs/sync/pipe_test.go
@@ -42,12 +42,18 @@ func TestPipe(t *testing.T) {
 	obj1 := mockobject.New("potato").WithContent([]byte("hello"), mockobject.SeekModeNone)
 
 	pair1 := fs.ObjectPair{Src: obj1, Dst: nil}
+	pairD := fs.ObjectPair{Src: obj1, Dst: obj1} // this object should not count to the stats
 
 	// Put an object
 	ok := p.Put(ctx, pair1)
 	assert.Equal(t, true, ok)
 	checkStats(1, 5)
 
+	// Put an object to be deleted
+	ok = p.Put(ctx, pairD)
+	assert.Equal(t, true, ok)
+	checkStats(2, 5)
+
 	// Close the pipe showing reading on closed pipe is OK
 	p.Close()
 
@@ -55,6 +61,12 @@ func TestPipe(t *testing.T) {
 	pair2, ok := p.Get(ctx)
 	assert.Equal(t, pair1, pair2)
 	assert.Equal(t, true, ok)
+	checkStats(1, 0)
+
+	// Read from pipe
+	pair2, ok = p.Get(ctx)
+	assert.Equal(t, pairD, pair2)
+	assert.Equal(t, true, ok)
 	checkStats(0, 0)
 
 	// Check read on closed pipe
diff --git a/fs/sync/sync.go b/fs/sync/sync.go
index 66eeca611..95a49ab2f 100644
--- a/fs/sync/sync.go
+++ b/fs/sync/sync.go
@@ -377,6 +377,14 @@ func (s *syncCopyMove) pairChecker(in *pipe, out *pipe, fraction int, wg *sync.W
 						fs.Logf(src, "Not removing source file as it is the same file as the destination")
 					} else if s.ci.IgnoreExisting {
 						fs.Debugf(src, "Not removing source file as destination file exists and --ignore-existing is set")
+					} else if s.checkFirst && s.ci.OrderBy != "" {
+						// If we want perfect ordering then use the transfers to delete the file
+						//
+						// We send src == dst, to say we want the src deleted
+						ok = out.Put(s.ctx, fs.ObjectPair{Src: src, Dst: src})
+						if !ok {
+							return
+						}
 					} else {
 						s.processError(operations.DeleteFile(s.ctx, src))
 					}
@@ -417,10 +425,16 @@ func (s *syncCopyMove) pairCopyOrMove(ctx context.Context, in *pipe, fdst fs.Fs,
 			return
 		}
 		src := pair.Src
+		dst := pair.Dst
 		if s.DoMove {
-			_, err = operations.Move(ctx, fdst, pair.Dst, src.Remote(), src)
+			if src != dst {
+				_, err = operations.Move(ctx, fdst, dst, src.Remote(), src)
+			} else {
+				// src == dst signals delete the src
+				err = operations.DeleteFile(ctx, src)
+			}
 		} else {
-			_, err = operations.Copy(ctx, fdst, pair.Dst, src.Remote(), src)
+			_, err = operations.Copy(ctx, fdst, dst, src.Remote(), src)
 		}
 		s.processError(err)
 	}