Fix move command - stop it running for overlapping fses - fixes #577

* Make move command check for overlapping remotes and refuse to run
  * Do copy/delete rather than all the copies then all the deletes
  * Doesn't purge the source - this was unexpected behaviour see #512 and #416
  * Add -list-retries flag to test suite to control retries

This changes the semantics of `move` slightly.  However it now errs on
the side of not deleting stuff.

docs/content/docs.md

@@ -113,16 +113,18 @@ go there.
 
 ### move source:path dest:path ###
 
-Moves the source to the destination.
+Moves the contents of the source directory to the destination
+directory. Rclone will error if the source and destination overlap.
 
-If there are no filters in use this is equivalent to a copy followed
-by a purge, but may use server side operations to speed it up if
-possible.
+If no filters are in use and if possible this will server side move
+`source:path` into `dest:path`. After this `source:path` will no
+longer longer exist.
 
-If filters are in use then it is equivalent to a copy followed by
-delete, followed by an rmdir (which only removes the directory if
-empty).  The individual file moves will be moved with server side
-operations if possible.
+Otherwise for each file in `source:path` selected by the filters (if
+any) this will move it into `dest:path`.  If possible a server side
+move will be used, otherwise it will copy it (server side if possible)
+into `dest:path` then delete the original (if no errors on copy) in
+`source:path`.
 
 **Important**: Since this can cause data loss, test first with the
 --dry-run flag.

fs/fs.go

@@ -45,6 +45,7 @@ var (
 	ErrorListOnlyRoot         = errors.New("can only list from root")
 	ErrorIsFile               = errors.New("is a file not a directory")
 	ErrorNotDeleting          = errors.New("not deleting files as there were IO errors")
+	ErrorCantMoveOverlapping  = errors.New("can't move files on overlapping remotes")
 )
 
 // RegInfo provides information about a filesystem

fs/operations.go

@@ -452,6 +452,12 @@ func Same(fdst, fsrc Fs) bool {
 	return fdst.Name() == fsrc.Name() && fdst.Root() == fsrc.Root()
 }
 
+// Overlapping returns true if fdst and fsrc point to the same
+// underlying Fs or they overlap.
+func Overlapping(fdst, fsrc Fs) bool {
+	return fdst.Name() == fsrc.Name() && (strings.HasPrefix(fdst.Root(), fsrc.Root()) || strings.HasPrefix(fsrc.Root(), fdst.Root()))
+}
+
 // checkIdentical checks to see if dst and src are identical
 //
 // it returns true if differences were found

fs/operations_test.go

@@ -75,6 +75,7 @@ type Run struct {
 	localName    string
 	flocal       fs.Fs
 	fremote      fs.Fs
+	fremoteName  string
 	cleanRemote  func()
 	mkdir        map[string]bool // whether the remote has been made yet for the fs name
 	Logf, Fatalf func(text string, args ...interface{})
@@ -108,7 +109,7 @@ func newRun() *Run {
 	fs.Config.DumpBodies = *DumpBodies
 	fs.Config.LowLevelRetries = *LowLevelRetries
 	var err error
-	r.fremote, r.cleanRemote, err = fstest.RandomRemote(*RemoteName, *SubDir)
+	r.fremote, r.fremoteName, r.cleanRemote, err = fstest.RandomRemote(*RemoteName, *SubDir)
 	if err != nil {
 		r.Fatalf("Failed to open remote %q: %v", *RemoteName, err)
 	}

fs/sync.go

@@ -264,14 +264,10 @@ func (s *syncCopyMove) pairMover(in ObjectPairChan, fdst Fs, wg *sync.WaitGroup)
 				Log(src, "Not moving as --dry-run")
 			} else if haveMover && src.Fs().Name() == fdst.Name() {
 				// Delete destination if it exists
-				if pair.dst != nil {
-					err := dst.Remove()
-					if err != nil {
-						Stats.Error()
-						ErrorLog(dst, "Couldn't delete: %v", err)
-						s.processError(err)
-					}
+				if dst != nil {
+					s.processError(DeleteFile(src))
 				}
+				// Move dst <- src
 				_, err := fdstMover.Move(src, src.Remote())
 				if err != nil {
 					Stats.Error()
@@ -281,7 +277,15 @@ func (s *syncCopyMove) pairMover(in ObjectPairChan, fdst Fs, wg *sync.WaitGroup)
 					Debug(src, "Moved")
 				}
 			} else {
-				s.processError(Copy(fdst, pair.dst, src))
+				// Copy dst <- src
+				err := Copy(fdst, dst, src)
+				s.processError(err)
+				if err != nil {
+					ErrorLog(src, "Not deleting as copy failed: %v", err)
+				} else {
+					// Delete src if no error on copy
+					s.processError(DeleteFile(src))
+				}
 			}
 			Stats.DoneTransferring(src.Remote())
 		case <-s.abort:
@@ -491,14 +495,24 @@ func MoveDir(fdst, fsrc Fs) error {
 		ErrorLog(fdst, "Nothing to do as source and destination are the same")
 		return nil
 	}
+	// The two remotes mustn't overlap
+	if Overlapping(fdst, fsrc) {
+		err := ErrorCantMoveOverlapping
+		ErrorLog(fdst, "%v", err)
+		return err
+	}
 
 	// First attempt to use DirMover if exists, same Fs and no filters are active
 	if fdstDirMover, ok := fdst.(DirMover); ok && fsrc.Name() == fdst.Name() && Config.Filter.InActive() {
-		err := fdstDirMover.DirMove(fsrc)
+		if Config.DryRun {
+			Log(fdst, "Not doing server side directory move as --dry-run")
+			return nil
+		}
 		Debug(fdst, "Using server side directory move")
+		err := fdstDirMover.DirMove(fsrc)
 		switch err {
 		case ErrorCantDirMove, ErrorDirExists:
-			Debug(fdst, "Server side directory move failed - fallback to copy/delete: %v", err)
+			Debug(fdst, "Server side directory move failed - fallback to file moves: %v", err)
 		case nil:
 			Debug(fdst, "Server side directory move succeeded")
 			return nil
@@ -509,22 +523,6 @@ func MoveDir(fdst, fsrc Fs) error {
 		}
 	}
 
-	// Now move the files
-	err := moveDir(fdst, fsrc)
-	if err != nil || Stats.Errored() {
-		ErrorLog(fdst, "Not deleting files as there were IO errors")
-		return err
-	}
-	// If no filters then purge
-	if Config.Filter.InActive() {
-		return Purge(fsrc)
-	}
-	// Otherwise remove any remaining files obeying filters
-	err = Delete(fsrc)
-	if err != nil {
-		return err
-	}
-	// and try to remove the directory if empty - ignoring error
-	_ = TryRmdir(fsrc)
-	return nil
+	// Otherwise move the files one by one
+	return moveDir(fdst, fsrc)
 }

fs/sync_test.go

@@ -100,7 +100,7 @@ func TestServerSideCopy(t *testing.T) {
 	file1 := r.WriteObject("sub dir/hello world", "hello world", t1)
 	fstest.CheckItems(t, r.fremote, file1)
 
-	fremoteCopy, finaliseCopy, err := fstest.RandomRemote(*RemoteName, *SubDir)
+	fremoteCopy, _, finaliseCopy, err := fstest.RandomRemote(*RemoteName, *SubDir)
 	require.NoError(t, err)
 	defer finaliseCopy()
 	t.Logf("Server side copy (if possible) %v -> %v", r.fremote, fremoteCopy)
@@ -563,17 +563,12 @@ func TestSyncWithUpdateOlder(t *testing.T) {
 }
 
 // Test a server side move if possible, or the backup path if not
-func TestServerSideMove(t *testing.T) {
-	r := NewRun(t)
-	defer r.Finalise()
+func testServerSideMove(t *testing.T, r *Run, fremoteMove fs.Fs, withFilter bool) {
 	file1 := r.WriteBoth("potato2", "------------------------------------------------------------", t1)
 	file2 := r.WriteBoth("empty space", "", t2)
 
 	fstest.CheckItems(t, r.fremote, file2, file1)
 
-	fremoteMove, finaliseMove, err := fstest.RandomRemote(*RemoteName, *SubDir)
-	require.NoError(t, err)
-	defer finaliseMove()
 	t.Logf("Server side move (if possible) %v -> %v", r.fremote, fremoteMove)
 
 	// Write just one file in the new remote
@@ -582,17 +577,75 @@ func TestServerSideMove(t *testing.T) {
 
 	// Do server side move
 	fs.Stats.ResetCounters()
-	err = fs.MoveDir(fremoteMove, r.fremote)
+	err := fs.MoveDir(fremoteMove, r.fremote)
 	require.NoError(t, err)
 
-	fstest.CheckItems(t, r.fremote)
+	fstest.CheckItems(t, r.fremote, file2)
 	fstest.CheckItems(t, fremoteMove, file2, file1)
 
+	// Purge the original before moving
+	require.NoError(t, fs.Purge(r.fremote))
+
 	// Move it back again, dst does not exist this time
 	fs.Stats.ResetCounters()
 	err = fs.MoveDir(r.fremote, fremoteMove)
 	require.NoError(t, err)
 
-	fstest.CheckItems(t, r.fremote, file2, file1)
-	fstest.CheckItems(t, fremoteMove)
+	if withFilter {
+		fstest.CheckItems(t, r.fremote, file1)
+		fstest.CheckItems(t, fremoteMove, file2)
+	} else {
+		fstest.CheckItems(t, r.fremote, file2, file1)
+		fstest.CheckItems(t, fremoteMove)
+	}
+}
+
+// Test a server side move if possible, or the backup path if not
+func TestServerSideMove(t *testing.T) {
+	r := NewRun(t)
+	defer r.Finalise()
+	fremoteMove, _, finaliseMove, err := fstest.RandomRemote(*RemoteName, *SubDir)
+	require.NoError(t, err)
+	defer finaliseMove()
+	testServerSideMove(t, r, fremoteMove, false)
+}
+
+// Test a server side move if possible, or the backup path if not
+func TestServerSideMoveWithFilter(t *testing.T) {
+	r := NewRun(t)
+	defer r.Finalise()
+
+	fs.Config.Filter.MinSize = 40
+	defer func() {
+		fs.Config.Filter.MinSize = -1
+	}()
+
+	fremoteMove, _, finaliseMove, err := fstest.RandomRemote(*RemoteName, *SubDir)
+	require.NoError(t, err)
+	defer finaliseMove()
+	testServerSideMove(t, r, fremoteMove, true)
+}
+
+// Test a server side move with overlap
+func TestServerSideMoveOverlap(t *testing.T) {
+	r := NewRun(t)
+	defer r.Finalise()
+	subRemoteName := r.fremoteName + "/rclone-move-test"
+	fremoteMove, err := fs.NewFs(subRemoteName)
+	require.NoError(t, err)
+
+	file1 := r.WriteObject("potato2", "------------------------------------------------------------", t1)
+	fstest.CheckItems(t, r.fremote, file1)
+
+	// Subdir move with no filters should return ErrorCantMoveOverlapping
+	err = fs.MoveDir(fremoteMove, r.fremote)
+	assert.EqualError(t, err, fs.ErrorCantMoveOverlapping.Error())
+
+	// Now try with a filter which should also fail with ErrorCantMoveOverlapping
+	fs.Config.Filter.MinSize = 40
+	defer func() {
+		fs.Config.Filter.MinSize = -1
+	}()
+	err = fs.MoveDir(fremoteMove, r.fremote)
+	assert.EqualError(t, err, fs.ErrorCantMoveOverlapping.Error())
 }

fstest/fstest.go

@@ -5,6 +5,7 @@ package fstest
 
 import (
 	"bytes"
+	"flag"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -25,6 +26,7 @@ import (
 var (
 	// MatchTestRemote matches the remote names used for testing
 	MatchTestRemote = regexp.MustCompile(`^rclone-test-[abcdefghijklmnopqrstuvwxyz0123456789]{24}$`)
+	listRetries     = flag.Int("list-retries", 6, "Number or times to retry listing")
 )
 
 // Seed the random number generator
@@ -123,9 +125,11 @@ func (is *Items) Find(t *testing.T, obj fs.Object, precision time.Duration) {
 		i, ok = is.byNameAlt[obj.Remote()]
 		assert.True(t, ok, fmt.Sprintf("Unexpected file %q", obj.Remote()))
 	}
-	delete(is.byName, i.Path)
-	delete(is.byName, i.WinPath)
-	i.Check(t, obj, precision)
+	if i != nil {
+		delete(is.byName, i.Path)
+		delete(is.byName, i.WinPath)
+		i.Check(t, obj, precision)
+	}
 }
 
 // Done checks all finished
@@ -134,8 +138,8 @@ func (is *Items) Done(t *testing.T) {
 		for name := range is.byName {
 			t.Logf("Not found %q", name)
 		}
-		t.Errorf("%d objects not found", len(is.byName))
 	}
+	assert.Equal(t, 0, len(is.byName), fmt.Sprintf("%d objects not found", len(is.byName)))
 }
 
 // CheckListingWithPrecision checks the fs to see if it has the
@@ -145,7 +149,7 @@ func CheckListingWithPrecision(t *testing.T, f fs.Fs, items []Item, precision ti
 	oldErrors := fs.Stats.GetErrors()
 	var objs []fs.Object
 	var err error
-	const retries = 6
+	var retries = *listRetries
 	sleep := time.Second / 2
 	for i := 1; i <= retries; i++ {
 		objs, err = fs.NewLister().Start(f, "").GetObjects()
@@ -257,26 +261,28 @@ func RandomRemoteName(remoteName string) (string, string, error) {
 //
 // Call the finalise function returned to Purge the fs at the end (and
 // the parent if necessary)
-func RandomRemote(remoteName string, subdir bool) (fs.Fs, func(), error) {
+//
+// Returns the remote, its url, a finaliser and an error
+func RandomRemote(remoteName string, subdir bool) (fs.Fs, string, func(), error) {
 	var err error
 	var parentRemote fs.Fs
 
 	remoteName, _, err = RandomRemoteName(remoteName)
 	if err != nil {
-		return nil, nil, err
+		return nil, "", nil, err
 	}
 
 	if subdir {
 		parentRemote, err = fs.NewFs(remoteName)
 		if err != nil {
-			return nil, nil, err
+			return nil, "", nil, err
 		}
 		remoteName += "/rclone-test-subdir-" + RandomString(8)
 	}
 
 	remote, err := fs.NewFs(remoteName)
 	if err != nil {
-		return nil, nil, err
+		return nil, "", nil, err
 	}
 
 	finalise := func() {
@@ -289,7 +295,7 @@ func RandomRemote(remoteName string, subdir bool) (fs.Fs, func(), error) {
 		}
 	}
 
-	return remote, finalise, nil
+	return remote, remoteName, finalise, nil
 }
 
 // TestMkdir tests Mkdir works

fstest/fstests/fstests.go

@@ -395,7 +395,7 @@ func TestFsDirMove(t *testing.T) {
 	require.Equal(t, fs.ErrorDirExists, err)
 
 	// new remote
-	newRemote, removeNewRemote, err := fstest.RandomRemote(RemoteName, false)
+	newRemote, _, removeNewRemote, err := fstest.RandomRemote(RemoteName, false)
 	require.NoError(t, err)
 	defer removeNewRemote()