From a6056408ddb9e4300be97852791f4169f48c357a Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Mon, 11 Jul 2016 11:36:46 +0100 Subject: [PATCH] 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 | 18 +++++----- fs/fs.go | 1 + fs/operations.go | 6 ++++ fs/operations_test.go | 3 +- fs/sync.go | 54 ++++++++++++++-------------- fs/sync_test.go | 75 +++++++++++++++++++++++++++++++++------ fstest/fstest.go | 26 ++++++++------ fstest/fstests/fstests.go | 2 +- 8 files changed, 126 insertions(+), 59 deletions(-) diff --git a/docs/content/docs.md b/docs/content/docs.md index 4930b5bed..9c34981bf 100644 --- a/docs/content/docs.md +++ b/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. diff --git a/fs/fs.go b/fs/fs.go index dcbc80134..2e03736da 100644 --- a/fs/fs.go +++ b/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 diff --git a/fs/operations.go b/fs/operations.go index b759e2c35..48c149982 100644 --- a/fs/operations.go +++ b/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 diff --git a/fs/operations_test.go b/fs/operations_test.go index 7c10d9638..92799f458 100644 --- a/fs/operations_test.go +++ b/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) } diff --git a/fs/sync.go b/fs/sync.go index 37042f7a0..fe12afd9a 100644 --- a/fs/sync.go +++ b/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) } diff --git a/fs/sync_test.go b/fs/sync_test.go index b0d7e2180..c90ff513e 100644 --- a/fs/sync_test.go +++ b/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()) } diff --git a/fstest/fstest.go b/fstest/fstest.go index f5fbc117a..61af1f628 100644 --- a/fstest/fstest.go +++ b/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 diff --git a/fstest/fstests/fstests.go b/fstest/fstests/fstests.go index 8fe9b2589..86c5e1fd1 100644 --- a/fstest/fstests/fstests.go +++ b/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()