Combine remote decrypts files when moving to --backup-dir... expected?

[ncw (Nick Craig-Wood)]

@nielash

When using --backup-dir with a combine remote that combines crypt and non-crypt (on the same base remote), instead of the files getting server-side moved into the backup-dir in their encrypted form, they get decrypted and copied into the backup-dir, but with metadata that reveals their previous encrypted names.

I wouldn't expect an encrypted file to arrive in an unencrypted remote.

@nielash

And on the underlying remote, the file is no longer encrypted.

I think that is what I would expect.

@nielash

Additionally, on at least Google Drive, the file retains metadata that reveals its former encrypted name:

Hmm, interesting!

We do set the Description to be the leaf name when upload files the first time. Can't remember why we do that!

Because you've used the -M flag the description is being preserved on the copy.

You could overwrite this with --metadata-set description= I think.

@nielash

--backup-dir normally requires server-side move, so my initial assumption was that the base remote would move the files in their existing form.

This is because it is wrapped in a combine. The combine backend advertises server side move (the Move method) because all of its parts support Move.

However when rclone comes to do the server side move, the combine backend figures it can't do it and returns fs.ErrorCantMove which then causes the fallback download/upload that you are seeing.

@nielash

Relatedly, I am wondering if this is a mistake, or if I'm somehow misunderstanding it:

rclone/fs/operations/operations.go at 519fe98e6e7798c3ae14ffd60af6ce2316b126a5 · rclone/rclone · GitHub

The comment is definitely wrong - it doesn't use the backup dir as you surmised!

Looking at the places it is called some of those maybe should be using the backupdir

cmd/bisync/bisync_test.go
491: 		return operations.DeleteFile(ctx, obj)
cmd/deletefile/deletefile.go
41: 			return operations.DeleteFile(context.Background(), fileObj)
cmd/ncdu/ncdu.go
581: 			err := operations.DeleteFile(ctx, obj)
636: 				err = operations.DeleteFile(ctx, obj)
fs/operations/dedupe.go
 66: 		err := DeleteFile(ctx, o)
130: 				err := DeleteFile(ctx, o)
fs/operations/operations.go
 374: 			err = DeleteFile(ctx, dst)
 410: 	return newDst, DeleteFile(ctx, src)
1812: 				err = DeleteFile(ctx, srcObj)
fs/operations/rc.go
276: 		return nil, DeleteFile(ctx, o)
fs/sync/sync.go
397: 						s.processError(operations.DeleteFile(s.ctx, src))
443: 				err = operations.DeleteFile(ctx, src)

To get the backup dir you call operations.BackupDir which is a relatively expensive operation so we should encourage the callers to call operations.DeleteFileWithBackupDir and look up the backupdir outside the loop.

So perhaps what we should do is

1. leave operations.DeleteFile alone but fix the comment to say call DeleteFileWIthBackup if --backup-dir support is required
2. add a note to DeleteFileWithBackup noting that you use BackupDir to find the --backup-dir and that it is relatively expensive so don't put it in a loop
3. audit the existings call sides for operations.DeleteFile and try to figure out whether they should be obeying --backup-dir or not.

I think this is enough stuff that we should probably make an issue about it - do you mind doing the honours?