rclone/rclone

`operations.DeleteFile` does not use `--backup-dir`, despite comment

Open

#7,566 opened on Jan 14, 2024

View on GitHub
 (0 comments) (0 reactions) (0 assignees)Go (5,090 forks)batch import
buggood first issuemaintenance

Repository metrics

Stars
 (57,185 stars)
PR merge metrics
 (Avg merge 32d 20h) (23 merged PRs in 30d)

Description

The associated forum post URL from https://forum.rclone.org

https://forum.rclone.org/t/combine-remote-decrypts-files-when-moving-to-backup-dir-expected/43986

What is the problem you are having with rclone?

The comment for operations.DeleteFile is incorrect. It states that --backup-dir is respected, but in fact, the only possible outcome is for backupDir to be nil.

https://github.com/rclone/rclone/blob/519fe98e6e7798c3ae14ffd60af6ce2316b126a5/fs/operations/operations.go#L489-L495

Some of its callers should probably be using backupDir, and so should probably be calling operations.DeleteFileWithBackupDir instead.

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)

operations.DeleteFileWithBackupDir requires callers to have already looked up the backupDir with operations.BackupDir which is a relatively expensive operation, so it should be done outside the loop when it is necessary.

ncw's suggested fix, which LGTM:

  1. leave operations.DeleteFile alone but fix the comment to say call DeleteFileWithBackupDir if --backup-dir support is required
  2. add a note to DeleteFileWithBackupDir 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 existing call sides for operations.DeleteFile and try to figure out whether they should be obeying --backup-dir or not.

How to use GitHub

  • Please use the 👍 reaction to show that you are affected by the same issue.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Contributor guide