Repository: ncw/rclone (go)

Issue:
Behaviour for HTTP API is not the same as with CLI API
<!--

Welcome :-) We understand you are having a problem with rclone; we want to help you with that!

If you've just got a question or aren't sure if you've found a bug then please use the rclone forum:

    https://forum.rclone.org/

instead of filing an issue for a quick response.

If you think you might have found a bug, please can you try to replicate it with the latest beta?

    https://beta.rclone.org/
    
If you can still replicate it with the latest beta, then please fill in the info below which makes our lives much easier.  A log with -vv will make our day :-)

Thank you

The Rclone Developers

-->

#### What is the problem you are having with rclone?

Sending requests that should fail (missing local dir, access denied to remote, etc.) to rclone HTTP server are not failing. They end with empty object and 200 code result. Tested with both curl and rclone rc.

Example:

```
a> rclone rcd -vv --rc-no-auth
b> rclone rc sync/move srcFs=/non-existent/ dstFs=created-but-inaccessible-remote:bucket/test
{}
b> curl --verbose -H "Content-Type: application/json" -d '{"srcFs":"local-dir-that-exists", "dstFs":"created-but-inaccessible-remote:bucket/test"}' http://localhost:5572/sync/move
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 5572 (#0)
> POST /sync/move HTTP/1.1
> Host: localhost:5572
> User-Agent: curl/7.58.0
> Accept: */*
> Content-Type: application/json
> Content-Length: 65
> 
* upload completely sent off: 65 out of 65 bytes
< HTTP/1.1 200 OK
< Access-Control-Allow-Headers: 
< Access-Control-Allow-Origin: *
< Date: Tue, 07 May 2019 08:11:03 GMT
< Content-Length: 3
< Content-Type: text/plain; charset=utf-8
< 
{}

```

Same request made with CLI is reporting an error (with retries) and exiting with `1`. I've taken a look at the code and it seems that CLI calls are wrapped with `cmd.Run` function which integrates with `accounting` and has retries. This functionality is not available within handler for HTTP server so requests never fail. Although available in the server error logs.

Which leads me to another "API design" question, why is for example `sync.MoveDir` not returning an error on failure? It seems currently the only way to know if there was a failure is to use `accounting.Stats`. From how these top level functions are used it seems like entire call chain depends on these top level functions to return errors.

#### What is your rclone version (output from `rclone version`)

rclone v1.47.0-DEV
- os/arch: linux/amd64
- go version: go1.12.4

Ubuntu 18.04

####  Which cloud storage system are you using? (eg Google Drive)

AWS S3 but it shouldn't matter to this issue.

#### A log from the command with the `-vv` flag (eg output from `rclone -vv copy /tmp remote:tmp`)

HTTP Server logs:

```
2019/05/07 10:28:09 DEBUG : rclone: Version "v1.47.0-DEV" starting with parameters ["rclone" "rcd" "-vv" "--rc-no-auth"]
2019/05/07 10:28:09 NOTICE: Serving remote control on http://127.0.0.1:5572/
2019/05/07 10:28:14 DEBUG : rc: "sync/move": with parameters map[dstFs:bla:bucket/test srcFs:/non-existent/]
2019/05/07 10:28:14 DEBUG : Using config file from "/home/alek/.config/rclone/rclone.conf"
2019/05/07 10:28:15 ERROR : S3 bucket bucket path test/: Failed to update region for bucket: reading bucket location failed: AccessDenied: Access Denied
	status code: 403, request id: 5F13BD3082D862ED, host id: FlUy7pvhPWARqV4qVNBGRhKbrIykPXQnFAfRoU/6CK6HGs3qyWaZq/u57YLYOjqbn0yAa0WyoKM=
2019/05/07 10:28:15 ERROR : S3 bucket bucket path test/: Failed to update region for bucket: reading bucket location failed: AccessDenied: Access Denied
	status code: 403, request id: F45BAF0FA7E4A5D9, host id: SRxpFTaW90nTzWMToU5XBAeHI4lepEBM3mtTJrBwHx2L99FMQhqz2rLEtrcX+qDzKm6bcPx4u4g=
2019/05/07 10:28:15 ERROR : : error reading source directory: directory not found
2019/05/07 10:28:15 INFO  : S3 bucket bucket path test/: Waiting for checks to finish
2019/05/07 10:28:15 INFO  : S3 bucket bucket path test/: Waiting for transfers to finish
2019/05/07 10:28:15 DEBUG : rc: "sync/move": reply map[]: <nil>
2019/05/07 10:28:57 DEBUG : rc: "sync/move": with parameters map[dstFs:bla:scylla-manager-aleksandar/bash srcFs:tmp]
2019/05/07 10:28:57 ERROR : : error reading source directory: directory not found
2019/05/07 10:28:57 INFO  : S3 bucket scylla-manager-aleksandar path bash/: Waiting for checks to finish
2019/05/07 10:28:57 INFO  : S3 bucket scylla-manager-aleksandar path bash/: Waiting for transfers to finish
2019/05/07 10:28:57 DEBUG : rc: "sync/move": reply map[]: <nil>
```


Pull request description:
march: return errors when listing dirs
#### Was the change discussed in an issue or in the forum before?

Partially fixes #3172

#### What is the purpose of this change?

As described in linked issue errors in file system access are not returned when copying/moving. This change corrects that by catching any listing errors.

#### Checklist

- [x] I have read the [contribution guidelines](https://github.com/ncw/rclone/blob/master/CONTRIBUTING.md#submitting-a-pull-request).
- [x] I have added tests for all changes in this PR if appropriate.
- [ ] I have added documentation for the changes if appropriate.
- [x] All commit messages are in [house style](https://github.com/ncw/rclone/blob/master/CONTRIBUTING.md#commit-messages).
- [x] I'm done, this Pull Request is ready for review :-)


Patch:
diff --git a/cmd/sync/sync.go b/cmd/sync/sync.go
index d840db248..aef4bd43d 100644
--- a/cmd/sync/sync.go
+++ b/cmd/sync/sync.go
@@ -4,6 +4,7 @@ import (
 	"context"
 
 	"github.com/ncw/rclone/cmd"
+	"github.com/ncw/rclone/fs/operations"
 	"github.com/ncw/rclone/fs/sync"
 	"github.com/spf13/cobra"
 )
@@ -44,9 +45,12 @@ go there.
 `,
 	Run: func(command *cobra.Command, args []string) {
 		cmd.CheckArgs(2, 2, command, args)
-		fsrc, fdst := cmd.NewFsSrcDst(args)
+		fsrc, srcFileName, fdst := cmd.NewFsSrcFileDst(args)
 		cmd.Run(true, true, command, func() error {
-			return sync.Sync(context.Background(), fdst, fsrc, createEmptySrcDirs)
+			if srcFileName == "" {
+				return sync.Sync(context.Background(), fdst, fsrc, createEmptySrcDirs)
+			}
+			return operations.CopyFile(context.Background(), fdst, fsrc, srcFileName, srcFileName)
 		})
 	},
 }
diff --git a/docs/content/authors.md b/docs/content/authors.md
index 8364450cd..341ebbd69 100644
--- a/docs/content/authors.md
+++ b/docs/content/authors.md
@@ -266,4 +266,6 @@ Contributors
   * Aleksandar Jankovic <office@ajankovic.com>
   * Maran <maran@protonmail.com>
   * nguyenhuuluan434 <nguyenhuuluan434@gmail.com>
-  * Laura <zotan@zotan.pw>
+  * Laura Hausmann <zotan@zotan.pw>
+  * yparitcher <y@paritcher.com>
+  * AbelThar <abela.tharen@gmail.com>
diff --git a/docs/content/docs.md b/docs/content/docs.md
index 79e782192..2e37b07cc 100644
--- a/docs/content/docs.md
+++ b/docs/content/docs.md
@@ -881,11 +881,23 @@ The default is `bytes`.
 
 ### --suffix=SUFFIX ###
 
-This is for use with `--backup-dir` only.  If this isn't set then
-`--backup-dir` will move files with their original name.  If it is set
-then the files will have SUFFIX added on to them.
+When using `sync`, `copy` or `move` any files which would have been
+overwritten or deleted will have the suffix added to them.  If there 
+is a file with the same path (after the suffix has been added), then 
+it will be overwritten.
+
+The remote in use must support server side move or copy and you must
+use the same remote as the destination of the sync.
+
+This is for use with files to add the suffix in the current directory 
+or with `--backup-dir`. See `--backup-dir` for more info.
 
-See `--backup-dir` for more info.
+For example
+
+    rclone sync /path/to/local/file remote:current --suffix .bak
+
+will sync `/path/to/local` to `remote:current`, but for any files
+which would have been updated or deleted have .bak added.
 
 ### --suffix-keep-extension ###
 
diff --git a/fs/accounting/stats.go b/fs/accounting/stats.go
index 8c7e2db8f..0dbcfaf5b 100644
--- a/fs/accounting/stats.go
+++ b/fs/accounting/stats.go
@@ -172,7 +172,7 @@ func etaString(done, total int64, rate float64) string {
 	if !ok {
 		return "-"
 	}
-	return d.String()
+	return fs.Duration(d).ReadableString()
 }
 
 // percent returns a/b as a percentage rounded to the nearest integer
diff --git a/fs/config/configflags/configflags.go b/fs/config/configflags/configflags.go
index f585e283d..97e5cbe11 100644
--- a/fs/config/configflags/configflags.go
+++ b/fs/config/configflags/configflags.go
@@ -68,7 +68,7 @@ func AddFlags(flagSet *pflag.FlagSet) {
 	flags.BoolVarP(flagSet, &fs.Config.NoTraverse, "no-traverse", "", fs.Config.NoTraverse, "Don't traverse destination file system on copy.")
 	flags.BoolVarP(flagSet, &fs.Config.NoUpdateModTime, "no-update-modtime", "", fs.Config.NoUpdateModTime, "Don't update destination mod-time if files identical.")
 	flags.StringVarP(flagSet, &fs.Config.BackupDir, "backup-dir", "", fs.Config.BackupDir, "Make backups into hierarchy based in DIR.")
-	flags.StringVarP(flagSet, &fs.Config.Suffix, "suffix", "", fs.Config.Suffix, "Suffix for use with --backup-dir.")
+	flags.StringVarP(flagSet, &fs.Config.Suffix, "suffix", "", fs.Config.Suffix, "Suffix to add to changed files.")
 	flags.BoolVarP(flagSet, &fs.Config.SuffixKeepExtension, "suffix-keep-extension", "", fs.Config.SuffixKeepExtension, "Preserve the extension when using --suffix.")
 	flags.BoolVarP(flagSet, &fs.Config.UseListR, "fast-list", "", fs.Config.UseListR, "Use recursive list if available. Uses more memory but fewer transactions.")
 	flags.Float64VarP(flagSet, &fs.Config.TPSLimit, "tpslimit", "", fs.Config.TPSLimit, "Limit HTTP transactions per second to this.")
@@ -152,10 +152,6 @@ func SetFlags() {
 		log.Fatalf(`Can't use --size-only and --ignore-size together.`)
 	}
 
-	if fs.Config.Suffix != "" && fs.Config.BackupDir == "" {
-		log.Fatalf(`Can only use --suffix with --backup-dir.`)
-	}
-
 	switch {
 	case len(fs.Config.StatsOneLineDateFormat) > 0:
 		fs.Config.StatsOneLineDate = true
diff --git a/fs/march/march.go b/fs/march/march.go
index 45eb4c9ca..b1f95a67c 100644
--- a/fs/march/march.go
+++ b/fs/march/march.go
@@ -8,6 +8,8 @@ import (
 	"strings"
 	"sync"
 
+	"github.com/pkg/errors"
+
 	"github.com/ncw/rclone/fs"
 	"github.com/ncw/rclone/fs/filter"
 	"github.com/ncw/rclone/fs/list"
@@ -110,7 +112,7 @@ type listDirJob struct {
 }
 
 // Run starts the matching process off
-func (m *March) Run() {
+func (m *March) Run() error {
 	m.init()
 
 	srcDepth := fs.Config.MaxDepth
@@ -122,6 +124,10 @@ func (m *March) Run() {
 		dstDepth = fs.MaxLevel
 	}
 
+	var mu sync.Mutex // Protects vars below
+	var jobError error
+	var errCount int
+
 	// Start some directory listing go routines
 	var wg sync.WaitGroup         // sync closing of go routines
 	var traversing sync.WaitGroup // running directory traversals
@@ -138,7 +144,16 @@ func (m *March) Run() {
 					if !ok {
 						return
 					}
-					jobs := m.processJob(job)
+					jobs, err := m.processJob(job)
+					if err != nil {
+						mu.Lock()
+						// Keep reference only to the first encountered error
+						if jobError == nil {
+							jobError = err
+						}
+						errCount++
+						mu.Unlock()
+					}
 					if len(jobs) > 0 {
 						traversing.Add(len(jobs))
 						go func() {
@@ -178,6 +193,11 @@ func (m *March) Run() {
 	traversing.Wait()
 	close(in)
 	wg.Wait()
+
+	if errCount > 1 {
+		return errors.Wrapf(jobError, "march failed with %d error(s): first error", errCount)
+	}
+	return jobError
 }
 
 // Check to see if the context has been cancelled
@@ -339,8 +359,9 @@ func matchListings(srcListEntries, dstListEntries fs.DirEntries, transforms []ma
 // more jobs
 //
 // returns errors using processError
-func (m *March) processJob(job listDirJob) (jobs []listDirJob) {
+func (m *March) processJob(job listDirJob) ([]listDirJob, error) {
 	var (
+		jobs                   []listDirJob
 		srcList, dstList       fs.DirEntries
 		srcListErr, dstListErr error
 		wg                     sync.WaitGroup
@@ -367,14 +388,14 @@ func (m *March) processJob(job listDirJob) (jobs []listDirJob) {
 	if srcListErr != nil {
 		fs.Errorf(job.srcRemote, "error reading source directory: %v", srcListErr)
 		fs.CountError(srcListErr)
-		return nil
+		return nil, srcListErr
 	}
 	if dstListErr == fs.ErrorDirNotFound {
 		// Copy the stuff anyway
 	} else if dstListErr != nil {
 		fs.Errorf(job.dstRemote, "error reading destination directory: %v", dstListErr)
 		fs.CountError(dstListErr)
-		return nil
+		return nil, dstListErr
 	}
 
 	// If NoTraverse is set, then try to find a matching object
@@ -395,7 +416,7 @@ func (m *March) processJob(job listDirJob) (jobs []listDirJob) {
 	srcOnly, dstOnly, matches := matchListings(srcList, dstList, m.transforms)
 	for _, src := range srcOnly {
 		if m.aborting() {
-			return nil
+			return nil, m.Ctx.Err()
 		}
 		recurse := m.Callback.SrcOnly(src)
 		if recurse && job.srcDepth > 0 {
@@ -409,7 +430,7 @@ func (m *March) processJob(job listDirJob) (jobs []listDirJob) {
 	}
 	for _, dst := range dstOnly {
 		if m.aborting() {
-			return nil
+			return nil, m.Ctx.Err()
 		}
 		recurse := m.Callback.DstOnly(dst)
 		if recurse && job.dstDepth > 0 {
@@ -422,7 +443,7 @@ func (m *March) processJob(job listDirJob) (jobs []listDirJob) {
 	}
 	for _, match := range matches {
 		if m.aborting() {
-			return nil
+			return nil, m.Ctx.Err()
 		}
 		recurse := m.Callback.Match(m.Ctx, match.dst, match.src)
 		if recurse && job.srcDepth > 0 && job.dstDepth > 0 {
@@ -434,5 +455,5 @@ func (m *March) processJob(job listDirJob) (jobs []listDirJob) {
 			})
 		}
 	}
-	return jobs
+	return jobs, nil
 }
diff --git a/fs/operations/multithread.go b/fs/operations/multithread.go
index 211b32102..56d7677ab 100644
--- a/fs/operations/multithread.go
+++ b/fs/operations/multithread.go
@@ -122,9 +122,9 @@ func multiThreadCopy(ctx context.Context, f fs.Fs, remote string, src fs.Object,
 		return nil, errors.New("multi-thread copy: can't copy zero sized file")
 	}
 
-	g, ctx := errgroup.WithContext(context.Background())
+	g, gCtx := errgroup.WithContext(ctx)
 	mc := &multiThreadCopyState{
-		ctx:     ctx,
+		ctx:     gCtx,
 		size:    src.Size(),
 		src:     src,
 		streams: streams,
@@ -136,7 +136,7 @@ func multiThreadCopy(ctx context.Context, f fs.Fs, remote string, src fs.Object,
 	defer fs.CheckClose(mc.acc, &err)
 
 	// create write file handle
-	mc.wc, err = openWriterAt(ctx, remote, mc.size)
+	mc.wc, err = openWriterAt(gCtx, remote, mc.size)
 	if err != nil {
 		return nil, errors.Wrap(err, "multpart copy: failed to open destination")
 	}
@@ -146,7 +146,7 @@ func multiThreadCopy(ctx context.Context, f fs.Fs, remote string, src fs.Object,
 	for stream := 0; stream < mc.streams; stream++ {
 		stream := stream
 		g.Go(func() (err error) {
-			return mc.copyStream(ctx, stream)
+			return mc.copyStream(gCtx, stream)
 		})
 	}
 	err = g.Wait()
diff --git a/fs/operations/operations.go b/fs/operations/operations.go
index 10c6e107d..b629c715c 100644
--- a/fs/operations/operations.go
+++ b/fs/operations/operations.go
@@ -513,13 +513,7 @@ func DeleteFileWithBackupDir(ctx context.Context, dst fs.Object, backupDir fs.Fs
 	if fs.Config.DryRun {
 		fs.Logf(dst, "Not %s as --dry-run", actioning)
 	} else if backupDir != nil {
-		if !SameConfig(dst.Fs(), backupDir) {
-			err = errors.New("parameter to --backup-dir has to be on the same remote as destination")
-		} else {
-			remoteWithSuffix := SuffixName(dst.Remote())
-			overwritten, _ := backupDir.NewObject(ctx, remoteWithSuffix)
-			_, err = Move(ctx, backupDir, overwritten, remoteWithSuffix, dst)
-		}
+		err = MoveBackupDir(ctx, backupDir, dst)
 	} else {
 		err = dst.Remove(ctx)
 	}
@@ -601,25 +595,41 @@ func Same(fdst, fsrc fs.Info) bool {
 	return SameConfig(fdst, fsrc) && strings.Trim(fdst.Root(), "/") == strings.Trim(fsrc.Root(), "/")
 }
 
+// fixRoot returns the Root with a trailing / if not empty. It is
+// aware of case insensitive filesystems.
+func fixRoot(f fs.Info) string {
+	s := strings.Trim(filepath.ToSlash(f.Root()), "/")
+	if s != "" {
+		s += "/"
+	}
+	if f.Features().CaseInsensitive {
+		s = strings.ToLower(s)
+	}
+	return s
+}
+
 // Overlapping returns true if fdst and fsrc point to the same
 // underlying Fs and they overlap.
 func Overlapping(fdst, fsrc fs.Info) bool {
 	if !SameConfig(fdst, fsrc) {
 		return false
 	}
-	// Return the Root with a trailing / if not empty
-	fixedRoot := func(f fs.Info) string {
-		s := strings.Trim(filepath.ToSlash(f.Root()), "/")
-		if s != "" {
-			s += "/"
-		}
-		return s
-	}
-	fdstRoot := fixedRoot(fdst)
-	fsrcRoot := fixedRoot(fsrc)
+	fdstRoot := fixRoot(fdst)
+	fsrcRoot := fixRoot(fsrc)
 	return strings.HasPrefix(fdstRoot, fsrcRoot) || strings.HasPrefix(fsrcRoot, fdstRoot)
 }
 
+// SameDir returns true if fdst and fsrc point to the same
+// underlying Fs and they are the same directory.
+func SameDir(fdst, fsrc fs.Info) bool {
+	if !SameConfig(fdst, fsrc) {
+		return false
+	}
+	fdstRoot := fixRoot(fdst)
+	fsrcRoot := fixRoot(fsrc)
+	return fdstRoot == fsrcRoot
+}
+
 // checkIdentical checks to see if dst and src are identical
 //
 // it returns true if differences were found
@@ -778,7 +788,7 @@ func CheckFn(ctx context.Context, fdst, fsrc fs.Fs, check checkFn, oneway bool)
 		Callback: c,
 	}
 	fs.Infof(fdst, "Waiting for checks to finish")
-	m.Run()
+	err := m.Run()
 
 	if c.dstFilesMissing > 0 {
 		fs.Logf(fdst, "%d files missing", c.dstFilesMissing)
@@ -797,7 +807,7 @@ func CheckFn(ctx context.Context, fdst, fsrc fs.Fs, check checkFn, oneway bool)
 	if c.differences > 0 {
 		return errors.Errorf("%d differences found", c.differences)
 	}
-	return nil
+	return err
 }
 
 // Check the files in fsrc and fdst according to Size and hash
@@ -1465,6 +1475,54 @@ func CopyURL(ctx context.Context, fdst fs.Fs, dstFileName string, url string) (d
 	return RcatSize(ctx, fdst, dstFileName, resp.Body, resp.ContentLength, time.Now())
 }
 
+// BackupDir returns the correctly configured --backup-dir
+func BackupDir(fdst fs.Fs, fsrc fs.Fs, srcFileName string) (backupDir fs.Fs, err error) {
+	if fs.Config.BackupDir != "" {
+		backupDir, err = cache.Get(fs.Config.BackupDir)
+		if err != nil {
+			return nil, fserrors.FatalError(errors.Errorf("Failed to make fs for --backup-dir %q: %v", fs.Config.BackupDir, err))
+		}
+		if !SameConfig(fdst, backupDir) {
+			return nil, fserrors.FatalError(errors.New("parameter to --backup-dir has to be on the same remote as destination"))
+		}
+		if srcFileName == "" {
+			if Overlapping(fdst, backupDir) {
+				return nil, fserrors.FatalError(errors.New("destination and parameter to --backup-dir mustn't overlap"))
+			}
+			if Overlapping(fsrc, backupDir) {
+				return nil, fserrors.FatalError(errors.New("source and parameter to --backup-dir mustn't overlap"))
+			}
+		} else {
+			if fs.Config.Suffix == "" {
+				if SameDir(fdst, backupDir) {
+					return nil, fserrors.FatalError(errors.New("destination and parameter to --backup-dir mustn't be the same"))
+				}
+				if SameDir(fsrc, backupDir) {
+					return nil, fserrors.FatalError(errors.New("source and parameter to --backup-dir mustn't be the same"))
+				}
+			}
+		}
+	} else {
+		if srcFileName == "" {
+			return nil, fserrors.FatalError(errors.New("--suffix must be used with a file or with --backup-dir"))
+		}
+		// --backup-dir is not set but --suffix is - use the destination as the backupDir
+		backupDir = fdst
+	}
+	if !CanServerSideMove(backupDir) {
+		return nil, fserrors.FatalError(errors.New("can't use --backup-dir on a remote which doesn't support server side move or copy"))
+	}
+	return backupDir, nil
+}
+
+// MoveBackupDir moves a file to the backup dir
+func MoveBackupDir(ctx context.Context, backupDir fs.Fs, dst fs.Object) (err error) {
+	remoteWithSuffix := SuffixName(dst.Remote())
+	overwritten, _ := backupDir.NewObject(ctx, remoteWithSuffix)
+	_, err = Move(ctx, backupDir, overwritten, remoteWithSuffix, dst)
+	return err
+}
+
 // moveOrCopyFile moves or copies a single file possibly to a new name
 func moveOrCopyFile(ctx context.Context, fdst fs.Fs, fsrc fs.Fs, dstFileName string, srcFileName string, cp bool) (err error) {
 	dstFilePath := path.Join(fdst.Root(), dstFileName)
@@ -1521,14 +1579,12 @@ func moveOrCopyFile(ctx context.Context, fdst fs.Fs, fsrc fs.Fs, dstFileName str
 
 	if NeedTransfer(ctx, dstObj, srcObj) {
 		// If destination already exists, then we must move it into --backup-dir if required
-		if dstObj != nil && fs.Config.BackupDir != "" {
-			backupDir, err := cache.Get(fs.Config.BackupDir)
+		if dstObj != nil && (fs.Config.BackupDir != "" || fs.Config.Suffix != "") {
+			backupDir, err := BackupDir(fdst, fsrc, srcFileName)
 			if err != nil {
 				return errors.Wrap(err, "creating Fs for --backup-dir failed")
 			}
-			remoteWithSuffix := SuffixName(dstObj.Remote())
-			overwritten, _ := backupDir.NewObject(ctx, remoteWithSuffix)
-			_, err = Move(ctx, backupDir, overwritten, remoteWithSuffix, dstObj)
+			err = MoveBackupDir(ctx, backupDir, dstObj)
 			if err != nil {
 				return errors.Wrap(err, "moving to --backup-dir failed")
 			}
@@ -1762,18 +1818,18 @@ func DirMove(ctx context.Context, f fs.Fs, srcRemote, dstRemote string) (err err
 		newPath string
 	}
 	renames := make(chan rename, fs.Config.Transfers)
-	g, ctx := errgroup.WithContext(context.Background())
+	g, gCtx := errgroup.WithContext(context.Background())
 	for i := 0; i < fs.Config.Transfers; i++ {
 		g.Go(func() error {
 			for job := range renames {
-				dstOverwritten, _ := f.NewObject(ctx, job.newPath)
-				_, err := Move(ctx, f, dstOverwritten, job.newPath, job.o)
+				dstOverwritten, _ := f.NewObject(gCtx, job.newPath)
+				_, err := Move(gCtx, f, dstOverwritten, job.newPath, job.o)
 				if err != nil {
 					return err
 				}
 				select {
-				case <-ctx.Done():
-					return ctx.Err()
+				case <-gCtx.Done():
+					return gCtx.Err()
 				default:
 				}
 
diff --git a/fs/parseduration.go b/fs/parseduration.go
index 94626a1fe..e79b323ee 100644
--- a/fs/parseduration.go
+++ b/fs/parseduration.go
@@ -78,6 +78,68 @@ func ParseDuration(age string) (time.Duration, error) {
 	return time.Duration(period), nil
 }
 
+// ReadableString parses d into a human readable duration.
+// Based on https://github.com/hako/durafmt
+func (d Duration) ReadableString() string {
+	switch d {
+	case DurationOff:
+		return "off"
+	case 0:
+		return "0s"
+	}
+
+	readableString := ""
+
+	// Check for minus durations.
+	if d < 0 {
+		readableString += "-"
+	}
+
+	duration := time.Duration(math.Abs(float64(d)))
+
+	// Convert duration.
+	seconds := int64(duration.Seconds()) % 60
+	minutes := int64(duration.Minutes()) % 60
+	hours := int64(duration.Hours()) % 24
+	days := int64(duration/(24*time.Hour)) % 365 % 7
+
+	// Edge case between 364 and 365 days.
+	// We need to calculate weeks from what is left from years
+	leftYearDays := int64(duration/(24*time.Hour)) % 365
+	weeks := leftYearDays / 7
+	if leftYearDays >= 364 && leftYearDays < 365 {
+		weeks = 52
+	}
+
+	years := int64(duration/(24*time.Hour)) / 365
+	milliseconds := int64(duration/time.Millisecond) -
+		(seconds * 1000) - (minutes * 60000) - (hours * 3600000) -
+		(days * 86400000) - (weeks * 604800000) - (years * 31536000000)
+
+	// Create a map of the converted duration time.
+	durationMap := map[string]int64{
+		"ms": milliseconds,
+		"s":  seconds,
+		"m":  minutes,
+		"h":  hours,
+		"d":  days,
+		"w":  weeks,
+		"y":  years,
+	}
+
+	// Construct duration string.
+	for _, u := range [...]string{"y", "w", "d", "h", "m", "s", "ms"} {
+		v := durationMap[u]
+		strval := strconv.FormatInt(v, 10)
+		if v == 0 {
+			continue
+		}
+		readableString += strval + u
+	}
+
+	return readableString
+}
+
 // Set a Duration
 func (d *Duration) Set(s string) error {
 	duration, err := ParseDuration(s)
diff --git a/fs/sync/sync.go b/fs/sync/sync.go
index c2ce37ae0..903998847 100644
--- a/fs/sync/sync.go
+++ b/fs/sync/sync.go
@@ -10,7 +10,6 @@ import (
 
 	"github.com/ncw/rclone/fs"
 	"github.com/ncw/rclone/fs/accounting"
-	"github.com/ncw/rclone/fs/cache"
 	"github.com/ncw/rclone/fs/filter"
 	"github.com/ncw/rclone/fs/fserrors"
 	"github.com/ncw/rclone/fs/hash"
@@ -62,7 +61,6 @@ type syncCopyMove struct {
 	trackRenamesCh chan fs.Object         // objects are pumped in here
 	renameCheck    []fs.Object            // accumulate files to check for rename here
 	backupDir      fs.Fs                  // place to store overwrites/deletes
-	suffix         string                 // suffix to add to files placed in backupDir
 }
 
 func newSyncCopyMove(ctx context.Context, fdst, fsrc fs.Fs, deleteMode fs.DeleteMode, DoMove bool, deleteEmptySrcDirs bool, copyEmptySrcDirs bool) (*syncCopyMove, error) {
@@ -122,25 +120,12 @@ func newSyncCopyMove(ctx context.Context, fdst, fsrc fs.Fs, deleteMode fs.Delete
 		}
 	}
 	// Make Fs for --backup-dir if required
-	if fs.Config.BackupDir != "" {
+	if fs.Config.BackupDir != "" || fs.Config.Suffix != "" {
 		var err error
-		s.backupDir, err = cache.Get(fs.Config.BackupDir)
+		s.backupDir, err = operations.BackupDir(fdst, fsrc, "")
 		if err != nil {
-			return nil, fserrors.FatalError(errors.Errorf("Failed to make fs for --backup-dir %q: %v", fs.Config.BackupDir, err))
+			return nil, err
 		}
-		if !operations.CanServerSideMove(s.backupDir) {
-			return nil, fserrors.FatalError(errors.New("can't use --backup-dir on a remote which doesn't support server side move or copy"))
-		}
-		if !operations.SameConfig(fdst, s.backupDir) {
-			return nil, fserrors.FatalError(errors.New("parameter to --backup-dir has to be on the same remote as destination"))
-		}
-		if operations.Overlapping(fdst, s.backupDir) {
-			return nil, fserrors.FatalError(errors.New("destination and parameter to --backup-dir mustn't overlap"))
-		}
-		if operations.Overlapping(fsrc, s.backupDir) {
-			return nil, fserrors.FatalError(errors.New("source and parameter to --backup-dir mustn't overlap"))
-		}
-		s.suffix = fs.Config.Suffix
 	}
 	return s, nil
 }
@@ -227,9 +212,7 @@ func (s *syncCopyMove) pairChecker(in *pipe, out *pipe, wg *sync.WaitGroup) {
 				} else {
 					// If destination already exists, then we must move it into --backup-dir if required
 					if pair.Dst != nil && s.backupDir != nil {
-						remoteWithSuffix := operations.SuffixName(pair.Dst.Remote())
-						overwritten, _ := s.backupDir.NewObject(s.ctx, remoteWithSuffix)
-						_, err := operations.Move(s.ctx, s.backupDir, overwritten, remoteWithSuffix, pair.Dst)
+						err := operations.MoveBackupDir(s.ctx, s.backupDir, pair.Dst)
 						if err != nil {
 							s.processError(err)
 						} else {
@@ -666,7 +649,7 @@ func (s *syncCopyMove) run() error {
 		Callback:      s,
 		DstIncludeAll: filter.Active.Opt.DeleteExcluded,
 	}
-	m.Run()
+	s.processError(m.Run())
 
 	s.stopTrackRenames()
 	if s.trackRenames {


Test patch:
diff --git a/fs/accounting/stats_test.go b/fs/accounting/stats_test.go
index af5eeebcd..8db584ed6 100644
--- a/fs/accounting/stats_test.go
+++ b/fs/accounting/stats_test.go
@@ -19,9 +19,20 @@ func TestETA(t *testing.T) {
 		wantOK      bool
 		wantString  string
 	}{
+		// Custom String Cases
+		{size: 0, total: 365 * 86400, rate: 1.0, wantETA: 365 * 86400 * time.Second, wantOK: true, wantString: "1y"},
+		{size: 0, total: 7 * 86400, rate: 1.0, wantETA: 7 * 86400 * time.Second, wantOK: true, wantString: "1w"},
+		{size: 0, total: 1 * 86400, rate: 1.0, wantETA: 1 * 86400 * time.Second, wantOK: true, wantString: "1d"},
+		{size: 0, total: 1110 * 86400, rate: 1.0, wantETA: 1110 * 86400 * time.Second, wantOK: true, wantString: "3y2w1d"},
+		{size: 0, total: 15 * 86400, rate: 1.0, wantETA: 15 * 86400 * time.Second, wantOK: true, wantString: "2w1d"},
+		// Composite Custom String Cases
+		{size: 0, total: 1.5 * 86400, rate: 1.0, wantETA: 1.5 * 86400 * time.Second, wantOK: true, wantString: "1d12h"},
+		{size: 0, total: 95000, rate: 1.0, wantETA: 95000 * time.Second, wantOK: true, wantString: "1d2h23m20s"},
+		// Standard Duration String Cases
 		{size: 0, total: 100, rate: 1.0, wantETA: 100 * time.Second, wantOK: true, wantString: "1m40s"},
 		{size: 50, total: 100, rate: 1.0, wantETA: 50 * time.Second, wantOK: true, wantString: "50s"},
 		{size: 100, total: 100, rate: 1.0, wantETA: 0 * time.Second, wantOK: true, wantString: "0s"},
+		// No String Cases
 		{size: -1, total: 100, rate: 1.0, wantETA: 0, wantOK: false, wantString: "-"},
 		{size: 200, total: 100, rate: 1.0, wantETA: 0, wantOK: false, wantString: "-"},
 		{size: 10, total: -1, rate: 1.0, wantETA: 0, wantOK: false, wantString: "-"},
diff --git a/fs/operations/operations_test.go b/fs/operations/operations_test.go
index 0e050ed20..11422e1cf 100644
--- a/fs/operations/operations_test.go
+++ b/fs/operations/operations_test.go
@@ -378,6 +378,19 @@ func TestCheck(t *testing.T) {
 	testCheck(t, operations.Check)
 }
 
+func TestCheckFsError(t *testing.T) {
+	dstFs, err := fs.NewFs("non-existent")
+	if err != nil {
+		t.Fatal(err)
+	}
+	srcFs, err := fs.NewFs("non-existent")
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = operations.Check(context.Background(), dstFs, srcFs, false)
+	require.Error(t, err)
+}
+
 func TestCheckDownload(t *testing.T) {
 	testCheck(t, operations.CheckDownload)
 }
diff --git a/fs/parseduration_test.go b/fs/parseduration_test.go
index 3a486549b..49ff164c9 100644
--- a/fs/parseduration_test.go
+++ b/fs/parseduration_test.go
@@ -84,6 +84,44 @@ func TestDurationString(t *testing.T) {
 	}
 }
 
+func TestDurationReadableString(t *testing.T) {
+	for _, test := range []struct {
+		negative bool
+		in       time.Duration
+		want     string
+	}{
+		// Edge Cases
+		{false, time.Duration(DurationOff), "off"},
+		// Base Cases
+		{false, time.Duration(0), "0s"},
+		{true, time.Millisecond, "1ms"},
+		{true, time.Second, "1s"},
+		{true, time.Minute, "1m"},
+		{true, (3 * time.Minute) / 2, "1m30s"},
+		{true, time.Hour, "1h"},
+		{true, time.Hour * 24, "1d"},
+		{true, time.Hour * 24 * 7, "1w"},
+		{true, time.Hour * 24 * 365, "1y"},
+		// Composite Cases
+		{true, time.Hour + 2*time.Minute + 3*time.Second, "1h2m3s"},
+		{true, time.Hour * 24 * (365 + 14), "1y2w"},
+		{true, time.Hour*24*4 + time.Hour*3 + time.Minute*2 + time.Second, "4d3h2m1s"},
+		{true, time.Hour * 24 * (365*3 + 7*2 + 1), "3y2w1d"},
+		{true, time.Hour*24*(365*3+7*2+1) + time.Hour*2 + time.Second, "3y2w1d2h1s"},
+		{true, time.Hour*24*(365*3+7*2+1) + time.Second, "3y2w1d1s"},
+		{true, time.Hour*24*(365+7*2+3) + time.Hour*4 + time.Minute*5 + time.Second*6 + time.Millisecond*7, "1y2w3d4h5m6s7ms"},
+	} {
+		got := Duration(test.in).ReadableString()
+		assert.Equal(t, test.want, got)
+
+		// Test Negative Case
+		if test.negative {
+			got = Duration(-test.in).ReadableString()
+			assert.Equal(t, "-"+test.want, got)
+		}
+	}
+}
+
 func TestDurationScan(t *testing.T) {
 	var v Duration
 	n, err := fmt.Sscan(" 17m ", &v)
diff --git a/fs/sync/sync_test.go b/fs/sync/sync_test.go
index 231c2d24a..0b28bd395 100644
--- a/fs/sync/sync_test.go
+++ b/fs/sync/sync_test.go
@@ -64,6 +64,20 @@ func TestCopy(t *testing.T) {
 	fstest.CheckItems(t, r.Fremote, file1)
 }
 
+func TestCopyMissingDirectory(t *testing.T) {
+	r := fstest.NewRun(t)
+	defer r.Finalise()
+	r.Mkdir(context.Background(), r.Fremote)
+
+	nonExistingFs, err := fs.NewFs("/non-existing")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = CopyDir(context.Background(), r.Fremote, nonExistingFs, false)
+	require.Error(t, err)
+}
+
 // Now with --no-traverse
 func TestCopyNoTraverse(t *testing.T) {
 	r := fstest.NewRun(t)
@@ -1281,6 +1295,94 @@ func TestSyncBackupDir(t *testing.T)                        { testSyncBackupDir(
 func TestSyncBackupDirWithSuffix(t *testing.T)              { testSyncBackupDir(t, ".bak", false) }
 func TestSyncBackupDirWithSuffixKeepExtension(t *testing.T) { testSyncBackupDir(t, "-2019-01-01", true) }
 
+// Test with Suffix set
+func testSyncSuffix(t *testing.T, suffix string, suffixKeepExtension bool) {
+	r := fstest.NewRun(t)
+	defer r.Finalise()
+
+	if !operations.CanServerSideMove(r.Fremote) {
+		t.Skip("Skipping test as remote does not support server side move")
+	}
+	r.Mkdir(context.Background(), r.Fremote)
+
+	fs.Config.Suffix = suffix
+	fs.Config.SuffixKeepExtension = suffixKeepExtension
+	defer func() {
+		fs.Config.BackupDir = ""
+		fs.Config.Suffix = ""
+		fs.Config.SuffixKeepExtension = false
+	}()
+
+	// Make the setup so we have one, two, three in the dest
+	// and one (different), two (same) in the source
+	file1 := r.WriteObject(context.Background(), "dst/one", "one", t1)
+	file2 := r.WriteObject(context.Background(), "dst/two", "two", t1)
+	file3 := r.WriteObject(context.Background(), "dst/three.txt", "three", t1)
+	file2a := r.WriteFile("two", "two", t1)
+	file1a := r.WriteFile("one", "oneA", t2)
+	file3a := r.WriteFile("three.txt", "threeA", t1)
+
+	fstest.CheckItems(t, r.Fremote, file1, file2, file3)
+	fstest.CheckItems(t, r.Flocal, file1a, file2a, file3a)
+
+	fdst, err := fs.NewFs(r.FremoteName + "/dst")
+	require.NoError(t, err)
+
+	accounting.Stats.ResetCounters()
+	err = operations.CopyFile(context.Background(), fdst, r.Flocal, "one", "one")
+	require.NoError(t, err)
+	err = operations.CopyFile(context.Background(), fdst, r.Flocal, "two", "two")
+	require.NoError(t, err)
+	err = operations.CopyFile(context.Background(), fdst, r.Flocal, "three.txt", "three.txt")
+	require.NoError(t, err)
+
+	// one should be moved to the backup dir and the new one installed
+	file1.Path = "dst/one" + suffix
+	file1a.Path = "dst/one"
+	// two should be unchanged
+	// three should be moved to the backup dir
+	if suffixKeepExtension {
+		file3.Path = "dst/three" + suffix + ".txt"
+	} else {
+		file3.Path = "dst/three.txt" + suffix
+	}
+	file3a.Path = "dst/three.txt"
+
+	fstest.CheckItems(t, r.Fremote, file1, file2, file3, file1a, file3a)
+
+	// Now check what happens if we do it again
+	// Restore a different three and update one in the source
+	file3b := r.WriteFile("three.txt", "threeB", t3)
+	file1b := r.WriteFile("one", "oneBB", t3)
+	fstest.CheckItems(t, r.Fremote, file1, file2, file3, file1a, file3a)
+
+	// This should delete three and overwrite one again, checking
+	// the files got overwritten correctly in backup-dir
+	accounting.Stats.ResetCounters()
+	err = operations.CopyFile(context.Background(), fdst, r.Flocal, "one", "one")
+	require.NoError(t, err)
+	err = operations.CopyFile(context.Background(), fdst, r.Flocal, "two", "two")
+	require.NoError(t, err)
+	err = operations.CopyFile(context.Background(), fdst, r.Flocal, "three.txt", "three.txt")
+	require.NoError(t, err)
+
+	// one should be moved to the backup dir and the new one installed
+	file1a.Path = "dst/one" + suffix
+	file1b.Path = "dst/one"
+	// two should be unchanged
+	// three should be moved to the backup dir
+	if suffixKeepExtension {
+		file3a.Path = "dst/three" + suffix + ".txt"
+	} else {
+		file3a.Path = "dst/three.txt" + suffix
+	}
+	file3b.Path = "dst/three.txt"
+
+	fstest.CheckItems(t, r.Fremote, file1b, file3b, file2, file3a, file1a)
+}
+func TestSyncSuffix(t *testing.T)              { testSyncSuffix(t, ".bak", false) }
+func TestSyncSuffixKeepExtension(t *testing.T) { testSyncSuffix(t, "-2019-01-01", true) }
+
 // Check we can sync two files with differing UTF-8 representations
 func TestSyncUTFNorm(t *testing.T) {
 	if runtime.GOOS == "darwin" {