rclone / rclone/rclone

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

Open
#7,566 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug good first issue maintenance
Dominant language
Go
Stars
59.8k
Forks
5.4k
Avg merge
2d 14h
Merged PRs (30d)
48

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in fs/operations/operations.go by reading DeleteFile, DeleteFileWithBackupDir, and BackupDir, then review the listed DeleteFile callers across cmd, fs/operations, and fs/sync. Determine which call sites should honor --backup-dir and whether the lookup can occur outside loops. Done means the comments accurately describe the behavior and the relevant callers use the appropriate backup-aware path.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.