#7566 change the comments in the operations.DeleteFile as it does not use --backup-dir, despite comment

[RONGALI-TARUN]

What is the purpose of this change?

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

Was the change discussed in an issue or in the forum before?

yes
(#7566)

Checklist

• I have read the contribution guidelines.
• I have added tests for all changes in this PR if appropriate.
• I have added documentation for the changes if appropriate.
• All commit messages are in house style.
• I'm done, this Pull Request is ready for review :-)

[nielash]

@RONGALI-TARUN thank you for working on this! This addresses 2 of the 3 items outlined on #7566. Are you interested in working on number 3 as well?

3. audit the existing call sides for operations.DeleteFile and try to figure out whether they should be obeying --backup-dir or not.

Also note that commit messages should be in this format.

[RONGALI-TARUN]

@RONGALI-TARUN thank you for working on this! This addresses 2 of the 3 items outlined on #7566. Are you interested in working on number 3 as well?

3. audit the existing call sides for operations.DeleteFile and try to figure out whether they should be obeying --backup-dir or not.

Also note that commit messages should be in this format.

hi @nielash
changed the msg format to the commit style
3. as i look through the code as far as i understand the function DeleteFileWithBackupDir is specifically created to address when we have provided the --backup-dir or else if we wanted to delete the file itself then we use the function DeleteFile.

[nielash]

3. as i look through the code as far as i understand the function DeleteFileWithBackupDir is specifically created to address when we have provided the --backup-dir or else if we wanted to delete the file itself then we use the function DeleteFile.

Right, to clarify -- the issue is that some functions that are currently calling operations.DeleteFile should really be calling operations.DeleteFileWithBackupDir instead. They may have assumed that they were using the backup-dir because of the incorrect comment on operations.DeleteFile, and now files are not getting backed up when they should be.

Here is an example:

rclone/fs/sync/sync.go

Line 423 in 24fdecf

deleteFileErr := operations.DeleteFile(s.ctx, src)

We actually don't even need to call operations.BackupDir here, as we already have it in s.backupDir, which we used a few lines earlier:

rclone/fs/sync/sync.go

Lines 385 to 387 in 24fdecf

	// If destination already exists, then we must move it into --backup-dir if required
	if pair.Dst != nil && s.backupDir != nil {
	err := operations.MoveBackupDir(s.ctx, s.backupDir, pair.Dst)

But we may need to do a check to see if src and dst use the same remote and skip it if not.

The list of callers is included on #7566. Probably not all of them will need to be changed, but some of them will. The task is to go through the list and decide which ones need to be changed, and then change them.

[RONGALI-TARUN]

3. as i look through the code as far as i understand the function DeleteFileWithBackupDir is specifically created to address when we have provided the --backup-dir or else if we wanted to delete the file itself then we use the function DeleteFile.

Right, to clarify -- the issue is that some functions that are currently calling operations.DeleteFile should really be calling operations.DeleteFileWithBackupDir instead. They may have assumed that they were using the backup-dir because of the incorrect comment on operations.DeleteFile, and now files are not getting backed up when they should be.

Here is an example:

rclone/fs/sync/sync.go

Line 423 in 24fdecf

deleteFileErr := operations.DeleteFile(s.ctx, src)

We actually don't even need to call operations.BackupDir here, as we already have it in s.backupDir, which we used a few lines earlier:

rclone/fs/sync/sync.go

Lines 385 to 387 in 24fdecf

	// If destination already exists, then we must move it into --backup-dir if required
	if pair.Dst != nil && s.backupDir != nil {
	err := operations.MoveBackupDir(s.ctx, s.backupDir, pair.Dst)

But we may need to do a check to see if src and dst use the same remote and skip it if not.

The list of callers is included on #7566. Probably not all of them will need to be changed, but some of them will. The task is to go through the list and decide which ones need to be changed, and then change them.

sure will work on this

[RONGALI-TARUN]

md/bisync/bisync_test.go
491: 	return operations.DeleteFile(ctx, obj) ----> this dont require backupDir
cmd/deletefile/deletefile.go
41: 		return operations.DeleteFile(context.Background(), fileObj)  ----> this dont require backupDir
cmd/ncdu/ncdu.go
581: 	err := operations.DeleteFile(ctx, obj) ----> this dont require backupDir
636: 	err = operations.DeleteFile(ctx, obj) ----> this dont require backupDir
fs/operations/dedupe.go
 66: 		err := DeleteFile(ctx, o) ----> this dont require backupDir
130: 	err := DeleteFile(ctx, o) ----> this dont require backupDir
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) ----> this dont require backupDir
fs/sync/sync.go
397: 	s.processError(operations.DeleteFile(s.ctx, src)) -->changed as the src and dst are from same remote
443: 	err = operations.DeleteFile(ctx, src)