`operations.DeleteFile` does not use `--backup-dir`, despite comment
Nobody has claimed this yet.
- 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.
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:
- leave
operations.DeleteFilealone but fix the comment to say callDeleteFileWithBackupDirif--backup-dirsupport is required - add a note to
DeleteFileWithBackupDirnoting that you useBackupDirto find the--backup-dirand that it is relatively expensive so don't put it in a loop - audit the existing call sides for
operations.DeleteFileand try to figure out whether they should be obeying--backup-diror 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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