Repository: rclone/rclone
Pull request #5495 (2021-07-29 07:25:27)

fs: move with --ignore-existing will not delete skipped files - #5463
<!--
Thank you very much for contributing code or documentation to rclone! Please
fill out the following questions to make it easier for us to review your
changes.

You do not need to check all the boxes below all at once, feel free to take
your time and add more commits. If you're done and ready for review, please
check the last box.
-->

#### What is the purpose of this change?
Proposed solution for issue #5463 

<!--
Describe the changes here
-->

#### Was the change discussed in an issue or in the forum before?
Yes. See issue referenced.
<!--
Link issues and relevant forum posts here.
-->

#### Checklist

- [x] I have read the [contribution guidelines](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#submitting-a-pull-request).
- [x] I have added tests for all changes in this PR if appropriate.
- [x] I have added documentation for the changes if appropriate.
- [x] All commit messages are in [house style](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#commit-messages).
- [x] I'm done, this Pull Request is ready for review :-)


natecollins: @ncw I've updated the pull branch to add the tests and put in a small docs update for the flag. Once of the builds looks to have timed out on something seemingly unrelated. Could you rerun the build to check if was just a transitory issue?

Patch:
diff --git a/VERSION b/VERSION
index e7e45c829..7b701c25c 100644
--- a/VERSION
+++ b/VERSION
@@ -1,1 +1,1 @@
-v1.56.0
+v1.57.0
diff --git a/backend/s3/s3.go b/backend/s3/s3.go
index c2f315518..8d315e450 100644
--- a/backend/s3/s3.go
+++ b/backend/s3/s3.go
@@ -629,6 +629,10 @@ func init() {
 				Value:    "s3.eu-central-1.wasabisys.com",
 				Help:     "Wasabi EU Central endpoint",
 				Provider: "Wasabi",
+			}, {
+				Value:    "s3.ap-northeast-1.wasabisys.com",
+				Help:     "Wasabi AP Northeast endpoint",
+				Provider: "Wasabi",
 			}},
 		}, {
 			Name:     "location_constraint",
diff --git a/backend/sftp/sftp.go b/backend/sftp/sftp.go
index ea003cda5..3167a8c65 100644
--- a/backend/sftp/sftp.go
+++ b/backend/sftp/sftp.go
@@ -429,10 +429,6 @@ func (f *Fs) newSftpClient(conn *ssh.Client, opts ...sftp.ClientOption) (*sftp.C
 		sftp.UseConcurrentReads(!f.opt.DisableConcurrentReads),
 		sftp.UseConcurrentWrites(!f.opt.DisableConcurrentWrites),
 	)
-	if f.opt.DisableConcurrentReads { // FIXME
-		fs.Errorf(f, "Ignoring disable_concurrent_reads after library reversion - see #5197")
-	}
-
 	return sftp.NewClientPipe(pr, pw, opts...)
 }
 
diff --git a/docs/content/authors.md b/docs/content/authors.md
index 65e6a5226..277df19c6 100644
--- a/docs/content/authors.md
+++ b/docs/content/authors.md
@@ -511,3 +511,5 @@ put them back in again.` >}}
   * Michael Hanselmann <public@hansmi.ch>
   * Chuan Zh <zhchuan7@gmail.com>
   * Antoine GIRARD <antoine.girard@sapk.fr>
+  * Justin Winokur (Jwink3101) <Jwink3101@users.noreply.github.com>
+  * Mariano Absatz (git) <scm@baby.com.ar>
diff --git a/docs/content/docs.md b/docs/content/docs.md
index f18a12a58..3483b7b3f 100644
--- a/docs/content/docs.md
+++ b/docs/content/docs.md
@@ -910,6 +910,10 @@ While this isn't a generally recommended option, it can be useful
 in cases where your files change due to encryption. However, it cannot
 correct partial transfers in case a transfer was interrupted.
 
+When performing a `move`/`moveto` command, this flag will leave skipped
+files in the source location unchanged when a file with the same name
+exists on the destination.
+
 ### --ignore-size ###
 
 Normally rclone will look at modification time and size of files to
diff --git a/docs/content/drive.md b/docs/content/drive.md
index f9cf5cec9..cc661a379 100644
--- a/docs/content/drive.md
+++ b/docs/content/drive.md
@@ -258,7 +258,7 @@ client_secret>   # Can be left blank
 scope>           # Select your scope, 1 for example
 root_folder_id>  # Can be left blank
 service_account_file> /home/foo/myJSONfile.json # This is where the JSON file goes!
-y/n>             # Auto config, y
+y/n>             # Auto config, n
 
 ```
 
diff --git a/docs/content/s3.md b/docs/content/s3.md
index 904aa6462..2de3ee60e 100644
--- a/docs/content/s3.md
+++ b/docs/content/s3.md
@@ -1048,6 +1048,8 @@ Required when using an S3 clone.
         - Wasabi US West endpoint
     - "s3.eu-central-1.wasabisys.com"
         - Wasabi EU Central endpoint
+    - "s3.ap-northeast-1.wasabisys.com"
+        - Wasabi AP Northeast endpoint
 
 #### --s3-location-constraint
 
diff --git a/docs/layouts/partials/version.html b/docs/layouts/partials/version.html
index 7e5a271f6..9e5cd4e30 100644
--- a/docs/layouts/partials/version.html
+++ b/docs/layouts/partials/version.html
@@ -1,1 +1,1 @@
-v1.56.0
\ No newline at end of file
+v1.57.0
\ No newline at end of file
diff --git a/fs/operations/operations.go b/fs/operations/operations.go
index 3c2b681d7..6ee7be2bf 100644
--- a/fs/operations/operations.go
+++ b/fs/operations/operations.go
@@ -1811,7 +1811,11 @@ func moveOrCopyFile(ctx context.Context, fdst fs.Fs, fsrc fs.Fs, dstFileName str
 	} else {
 		tr := accounting.Stats(ctx).NewCheckingTransfer(srcObj)
 		if !cp {
-			err = DeleteFile(ctx, srcObj)
+			if ci.IgnoreExisting {
+				fs.Debugf(srcObj, "Not removing source file as destination file exists and --ignore-existing is set")
+			} else {
+				err = DeleteFile(ctx, srcObj)
+			}
 		}
 		tr.Done(ctx, err)
 	}
diff --git a/fs/sync/sync.go b/fs/sync/sync.go
index 7ac9a8a59..96290e0a1 100644
--- a/fs/sync/sync.go
+++ b/fs/sync/sync.go
@@ -354,6 +354,8 @@ func (s *syncCopyMove) pairChecker(in *pipe, out *pipe, fraction int, wg *sync.W
 					// Delete src if no error on copy
 					if operations.SameObject(src, pair.Dst) {
 						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 {
 						s.processError(operations.DeleteFile(s.ctx, src))
 					}
diff --git a/fs/version.go b/fs/version.go
index 66a14dee1..64576f059 100644
--- a/fs/version.go
+++ b/fs/version.go
@@ -1,4 +1,4 @@
 package fs
 
 // Version of rclone
-var Version = "v1.56.0-DEV"
+var Version = "v1.57.0-DEV"
diff --git a/vfs/vfscache/writeback/writeback.go b/vfs/vfscache/writeback/writeback.go
index 54d664baa..5db10c603 100644
--- a/vfs/vfscache/writeback/writeback.go
+++ b/vfs/vfscache/writeback/writeback.go
@@ -276,13 +276,12 @@ func (wb *WriteBack) Add(id Handle, name string, modified bool, putFn PutFn) Han
 	return wbItem.id
 }
 
-// Remove should be called when a file should be removed from the
+// _remove should be called when a file should be removed from the
 // writeback queue. This cancels a writeback if there is one and
 // doesn't return the item to the queue.
-func (wb *WriteBack) Remove(id Handle) (found bool) {
-	wb.mu.Lock()
-	defer wb.mu.Unlock()
-
+//
+// This should be called with the lock held
+func (wb *WriteBack) _remove(id Handle) (found bool) {
 	wbItem, found := wb.lookup[id]
 	if found {
 		fs.Debugf(wbItem.name, "vfs cache: cancelling writeback (uploading %v) %p item %d", wbItem.uploading, wbItem, wbItem.id)
@@ -299,6 +298,16 @@ func (wb *WriteBack) Remove(id Handle) (found bool) {
 	return found
 }
 
+// Remove should be called when a file should be removed from the
+// writeback queue. This cancels a writeback if there is one and
+// doesn't return the item to the queue.
+func (wb *WriteBack) Remove(id Handle) (found bool) {
+	wb.mu.Lock()
+	defer wb.mu.Unlock()
+
+	return wb._remove(id)
+}
+
 // Rename should be called when a file might be uploading and it gains
 // a new name. This will cancel the upload and put it back in the
 // queue.
@@ -314,6 +323,15 @@ func (wb *WriteBack) Rename(id Handle, name string) {
 		// We are uploading already so cancel the upload
 		wb._cancelUpload(wbItem)
 	}
+
+	// Check to see if there are any uploads with the existing
+	// name and remove them
+	for existingID, existingItem := range wb.lookup {
+		if existingID != id && existingItem.name == name {
+			wb._remove(existingID)
+		}
+	}
+
 	wbItem.name = name
 	// Kick the timer on
 	wb.items._update(wbItem, wb._newExpiry())


Test patch:
diff --git a/fs/operations/operations_test.go b/fs/operations/operations_test.go
index 3f39eb6fc..06883a7fe 100644
--- a/fs/operations/operations_test.go
+++ b/fs/operations/operations_test.go
@@ -811,6 +811,32 @@ func TestMoveFile(t *testing.T) {
 	fstest.CheckItems(t, r.Fremote, file2)
 }
 
+func TestMoveFileWithIgnoreExisting(t *testing.T) {
+	ctx := context.Background()
+	ctx, ci := fs.AddConfig(ctx)
+	r := fstest.NewRun(t)
+	defer r.Finalise()
+	file1 := r.WriteFile("file1", "file1 contents", t1)
+	fstest.CheckItems(t, r.Flocal, file1)
+
+	ci.IgnoreExisting = true
+
+	err := operations.MoveFile(ctx, r.Fremote, r.Flocal, file1.Path, file1.Path)
+	require.NoError(t, err)
+	fstest.CheckItems(t, r.Flocal)
+	fstest.CheckItems(t, r.Fremote, file1)
+
+	// Recreate file with updated content
+	file1b := r.WriteFile("file1", "file1 modified", t2)
+	fstest.CheckItems(t, r.Flocal, file1b)
+
+	// Ensure modified file did not transfer and was not deleted
+	err = operations.MoveFile(ctx, r.Fremote, r.Flocal, file1.Path, file1b.Path)
+	require.NoError(t, err)
+	fstest.CheckItems(t, r.Flocal, file1b)
+	fstest.CheckItems(t, r.Fremote, file1)
+}
+
 func TestCaseInsensitiveMoveFile(t *testing.T) {
 	ctx := context.Background()
 	r := fstest.NewRun(t)
diff --git a/fs/sync/sync_test.go b/fs/sync/sync_test.go
index f41efe914..ebe55da92 100644
--- a/fs/sync/sync_test.go
+++ b/fs/sync/sync_test.go
@@ -1342,6 +1342,65 @@ func TestMoveWithoutDeleteEmptySrcDirs(t *testing.T) {
 	fstest.CheckItems(t, r.Fremote, file1, file2)
 }
 
+func TestMoveWithIgnoreExisting(t *testing.T) {
+	ctx := context.Background()
+	ctx, ci := fs.AddConfig(ctx)
+	r := fstest.NewRun(t)
+	defer r.Finalise()
+	file1 := r.WriteFile("existing", "potato", t1)
+	file2 := r.WriteFile("existing-b", "tomato", t1)
+
+	ci.IgnoreExisting = true
+
+	accounting.GlobalStats().ResetCounters()
+	err := MoveDir(ctx, r.Fremote, r.Flocal, false, false)
+	require.NoError(t, err)
+	fstest.CheckListingWithPrecision(
+		t,
+		r.Flocal,
+		[]fstest.Item{},
+		[]string{},
+		fs.GetModifyWindow(ctx, r.Flocal),
+	)
+	fstest.CheckListingWithPrecision(
+		t,
+		r.Fremote,
+		[]fstest.Item{
+			file1,
+			file2,
+		},
+		[]string{},
+		fs.GetModifyWindow(ctx, r.Fremote),
+	)
+
+	// Recreate first file with modified content
+	file1b := r.WriteFile("existing", "newpotatoes", t2)
+	accounting.GlobalStats().ResetCounters()
+	err = MoveDir(ctx, r.Fremote, r.Flocal, false, false)
+	require.NoError(t, err)
+	// Source items should still exist in modified state
+	fstest.CheckListingWithPrecision(
+		t,
+		r.Flocal,
+		[]fstest.Item{
+			file1b,
+		},
+		[]string{},
+		fs.GetModifyWindow(ctx, r.Flocal),
+	)
+	// Dest items should not have changed
+	fstest.CheckListingWithPrecision(
+		t,
+		r.Fremote,
+		[]fstest.Item{
+			file1,
+			file2,
+		},
+		[]string{},
+		fs.GetModifyWindow(ctx, r.Fremote),
+	)
+}
+
 // Test a server-side move if possible, or the backup path if not
 func TestServerSideMove(t *testing.T) {
 	ctx := context.Background()
diff --git a/vfs/vfscache/writeback/writeback_test.go b/vfs/vfscache/writeback/writeback_test.go
index 25a41d9e6..6b0398b50 100644
--- a/vfs/vfscache/writeback/writeback_test.go
+++ b/vfs/vfscache/writeback/writeback_test.go
@@ -585,6 +585,53 @@ func TestWriteBackRename(t *testing.T) {
 	assert.Equal(t, wbItem.name, "three")
 }
 
+// TestWriteBackRenameDuplicates checks that if we rename an entry and
+// make a duplicate, we remove the duplicate.
+func TestWriteBackRenameDuplicates(t *testing.T) {
+	wb, cancel := newTestWriteBack(t)
+	defer cancel()
+
+	// add item "one"
+	pi1 := newPutItem(t)
+	id1 := wb.Add(0, "one", true, pi1.put)
+	wbItem1 := wb.lookup[id1]
+	checkOnHeap(t, wb, wbItem1)
+	checkInLookup(t, wb, wbItem1)
+	assert.Equal(t, wbItem1.name, "one")
+
+	<-pi1.started
+	checkNotOnHeap(t, wb, wbItem1)
+	checkInLookup(t, wb, wbItem1)
+
+	// add item "two"
+	pi2 := newPutItem(t)
+	id2 := wb.Add(0, "two", true, pi2.put)
+	wbItem2 := wb.lookup[id2]
+	checkOnHeap(t, wb, wbItem2)
+	checkInLookup(t, wb, wbItem2)
+	assert.Equal(t, wbItem2.name, "two")
+
+	<-pi2.started
+	checkNotOnHeap(t, wb, wbItem2)
+	checkInLookup(t, wb, wbItem2)
+
+	// rename "two" to "one"
+	wb.Rename(id2, "one")
+
+	// check "one" is cancelled and removed from heap and lookup
+	checkNotOnHeap(t, wb, wbItem1)
+	checkNotInLookup(t, wb, wbItem1)
+	assert.True(t, pi1.cancelled)
+	assert.Equal(t, wbItem1.name, "one")
+
+	// check "two" (now called "one"!) has been cancelled and will
+	// be retried
+	checkOnHeap(t, wb, wbItem2)
+	checkInLookup(t, wb, wbItem2)
+	assert.True(t, pi2.cancelled)
+	assert.Equal(t, wbItem2.name, "one")
+}
+
 func TestWriteBackCancelUpload(t *testing.T) {
 	wb, cancel := newTestWriteBack(t)
 	defer cancel()