Move files to trash instead of deleting via new config option

[justinmayer]

Description

While it doesn't happen often, some folks have had experiences in which Beets unexpectedly deletes a song. All software has bugs, and data loss sometimes happens. But as someone who once lost an entire music collection due to data loss (not Beets-related), I think we should do everything we can to prevent unexpected file deletion, regardless of whether that happens via bugs or user mistakes.

This pull request enhances the util.remove() function to move the specified file to the trash by default if there is a trash executable on $PATH. Otherwise, the file is deleted. This approach provides the option for a safer user experience without adding yet another Python dependency to the project.

The executable to be used for these safer file removal operations (default: trash) is configurable via a new remove_command configuration option.

Fixes #149 #2499

[RollingStar]

FYI:

Note: The Linux implementation is not very good and not maintained. Help welcome. If no one steps up to help maintain it, I will eventually remove Linux support.

[justinmayer]

I saw that notice before and selected the project (Trash) as an example nonetheless. Reasons include:

• There are many projects that provide a trash command, but few of them are cross-platform.
• Trash is merely one such project, an example. Providing an exhaustive list is out-of-scope.
• Providing solid support across Linux distributions is non-trivial, so this note is unsurprising.
• Linux users are statistically the best-equipped to find an alternative that suits them, if necessary.
• Perhaps some Linux folks will step forward and help out with Linux support in Trash.

[sampsyo]

Yo! This is a good idea. Can I make these suggestions, however?

• Can we make the docs and the code changes separate PRs? They will both require separate consideration, although we probably want both of them.
• For "trashing" things, I think it would be great to make this a config option rather than the default. This would also let people configure the tool they want to use, if it does not happen to be the first trash program on $PATH.

[justinmayer]

Hey Adrian!

Can we make the docs and the code changes separate PRs? They will both require separate consideration, although we probably want both of them.

Certainly. They were related enough that it seemed sensible to submit them together, but I'm happy to separate them.

For "trashing" things, I think it would be great to make this a config option rather than the default. This would also let people configure the tool they want to use, if it does not happen to be the first trash program on $PATH.

Sure, I can see the benefit in that. I'll have another pass at it.

[ssssam]

On some (most?) Linux systems, the gio trash command provided by gio is a good way to send files to the trash. Perhaps we could also try that if available.

If not, we could mention in the docs that creating a script like this can enable the feature on Linux:

cat > ~/.local.bin/trash <<"EOF"
#!/bin/sh
gio trash $@
EOF
chmod +x ~/.local/bin/trash

[sampsyo]

Nice; great point. All the more reason we should probably make the command configurable—so one could just configure it directly to do gio trash.

[justinmayer]

Hey @sampsyo. Based on your feedback, I addressed the points you raised:

Can we make the docs and the code changes separate PRs? They will both require separate consideration, although we probably want both of them.

I removed the documentation changes relating to backups, which I will submit in a separate pull request.

For "trashing" things, I think it would be great to make this a config option rather than the default. This would also let people configure the tool they want to use, if it does not happen to be the first trash program on $PATH.

The executable is now configurable via the remove_command setting.

Anything left to do here before merging? ✨

[wisp3rwind]

I added a few remarks, I think the most important one would be the handling of invalid config values. I haven't been part of the discussion on trashing and the backup recommendation so far, so if this has been discussed before feel free to dismiss my comments.

It might be useful to grep the codebase for uses of util.remove; I could imagine it being used on temporary files that should not go to trash.

[justinmayer]

Hey @wisp3rwind. Thanks for the comments. I did grep through the code, and I didn't see any areas in which util.remove() not deleting immediately would pose an issue.

[wisp3rwind]

I did grep through the code, and I didn't see any areas in which util.remove() not deleting immediately would pose an issue.

I've just been doing the same, but I do think that there are a few cases in test/ and beetsplug/ that should be exempt. I'll open a PR relative to this one with the changes (I probably won't make it today anymore, though).

[wisp3rwind]

I left one more comment about a slight simplification that can be made; I'll have a look at callers which need to opt-out of trash handling later today.

Otherwise, this is looking good 👍

[sampsyo]

Hi, folks! Thanks for making so much progress on this; it looks fantastic so far.

Just to check, however: it looks like the current implementation will attempt to use trash by default, if the user doesn't do anything else? I think we should probably not do that—unless I'm reading it wrong, this would lead to errors on machines without trash installed in the current setup. And even if we turn this into a silent fallback, it will change the behavior for current users. Perhaps I'm misreading things, but maybe the default for remove_command should just be empty to preserve the current behavior?

It also occurs to me that the current structure of the remove function now does ~completely separate things depending on whether trash=True. Maybe it would be a little more straightforward to instead just have a separate function called trash, and to leave remove to mean "actually delete"? The removal "mode" would then be dictated by the client, which is what we already do for moving/copying/etc.:

beets/beets/library.py

Line 793 in 84f6d37

if operation == MoveOperation.MOVE:

[justinmayer]

Hey Adrian! Thanks for the kind words.

It looks like the current implementation will attempt to use trash by default, if the user doesn't do anything else?

Yes — the idea is that this is a safer default for end users, eliminating potential foot-guns caused by unexpected and/or aberrant behavior, which can often result in catastrophic data loss. (See related linked issue in the description.)

Unless I'm reading it wrong, this would lead to errors on machines without trash installed in the current setup.

I think you might be reading it wrong? There shouldn't be any errors in that situation. If trash (or another command defined in remove_command) is not found on $PATH, then os.remove() is used to immediately delete the file (i.e., the current default behavior).

[…] it will change the behavior for current users. Perhaps I'm misreading things, but maybe the default for remove_command should just be empty to preserve the current behavior?

I believe that (1) the safest default is the best default and (2) this safety outweighs preserving the less-safe current behavior. Plus, it only changes the behavior for current users who have gone out of their way to install a trash tool, and those are likely the kind of users who will value the extra safety this new default removal behavior provides.

[wisp3rwind]

Personally, I agree with @nathdwek in #149

As a mainly linux guy, I didn't and still don't really feel the need for recycle-binning.

and TBH, I don't care what the default is, which is why I didn't press the point here. For my own workflow, I'm rarely in a situation where beets is deleting files anyway. This feature request (and the alleged footguns when using beets) seems to be brought up persistently (by a small number of people, however), so if trashing by default helps calming the waves, I'll just add remove_command="" to my configuration.

Just to check, however: it looks like the current implementation will attempt to use trash by default, if the user doesn't do anything else? I think we should probably not do that—unless I'm reading it wrong, this would lead to errors on machines without trash installed in the current setup. And even if we turn this into a silent fallback, it will change the behavior for current users. Perhaps I'm misreading things, but maybe the default for remove_command should just be empty to preserve the current behavior?

I think @justinmayer is right in that this is currently already a silent fallback. I was suprised about that, too, see #3796 (comment), but as outlined above, I won't be investing energy into discussing this point. Ideally of course, that would be polled on the forum.

It also occurs to me that the current structure of the remove function now does ~completely separate things depending on whether trash=True. Maybe it would be a little more straightforward to instead just have a separate function called trash, and to leave remove to mean "actually delete"? The removal "mode" would then be dictated by the client, which is what we already do for moving/copying/etc.:

I'm not sure that is useful: In the example you made, there is still a def move_file(self, dest, operation=MoveOperation.MOVE) top-level function, that switches between the various modes. That appears very similar to def remove(..., use_trash=True) with a boolean instead of the enum.

In any case, this needs to go with #3876, lest we litter the trash with unexpected temporary files. I'd appreciate if someone (...) could have a brief look whether the additional commit in that PR looks sensible.

[sampsyo]

Awesome; thanks. Glad that I totally misread the fallback thing! Sorry about that.

As for the other two issues:

• Thanks for clarifying about the intent to trash by default. I am still leaning toward not doing this and deleting by default. We already have several safeguards in place, such as not actually deleting unless you add -d to beet rm and then requiring a Y to confirm the action. In a sort of classically Unixy UI sense, I think it would be somewhat surprising that an action that's supposed to delete files actually calls a binary named trash instead. I really like having this as an option, but I think it makes the most sense to have it off by default.
• About whether to have the "trash decision" internal to remove or external in the client: thanks to @wisp3rwind for putting together Trash exemptions #3876! As that PR illustrates, it seems like the places in the code where we actually want to remove files may actually be more numerous than the places where we want to conditionally remove-or-trash files (which I imagine are only in library.py's remove methods, which are already meant to centralize logic for removing actual "content" files, i.e., music and album art). So I'm worried that having use_trash=True be the default will lead to confusing bugs in the future when we add code that wants to do "normal" removals. I don't really object to having the check in util instead of library, but it still seems marginally more natural to me to do the latter? I dunno, no need to bikeshed and I don't feel terribly strongly about it, but maybe the above reasons will make a little bit of sense.

[RollingStar]

Unless we're sure that beets' trash-related code is 100% bug-free, it strikes me as arrogant to not make recycle bin / trash the default. #2499

Add in an unbounded number of plugins that may also want to delete files, and it's a big source of errors for no benefit.

If your OS is that good, just automate the trash removal. I think mine does that already so there's no overhead to me as an end user, just more safety.

I skimmed the above conversation, so maybe I missed it: what is gained by not having trash functionality? If it just tries to call an outside program trash and silently falls back to the old behavior, what's the harm? Why do we need to avoid this?

[sampsyo]

My argument for having this off by default boils down to the principle of least astonishment: is that beets is a Unix CLI tool, and the "normal" thing for a Unix CLI tool to do is to delete files when you type the rm command. Invoking an external trash binary is undeniably a useful thing to do for some users, but it is also surprising—no other Unix CLI tool I am familiar with works that way. If you have a weird binary sitting around in your $PATH called trash that does something else entirely, I think it would be pretty surprising for a seemingly-minor beets feature to suddenly start invoking it. And if the trash program you have fails for some reason, that's not really something we can recover from—so it's an added layer of complexity.

So I'm very enthusiastic about the functionality being available, but I still think it should be off by default.

[wisp3rwind]

Having thought about it again, I probably care more than I realised, and I think I'm seeing a clear path now on how this should be approached:

• Change the interface to def remove(..., use_trash=False), and in turn adapt Trash exemptions #3876
• Make remove_command="" the default for now
• Merge this and Trash exemptions #3876
• Open a new issue about changing the default for remove_command, which we review only after trash functionality has been in a release or two, so that we can have some more confidence in the correctness of the trash-related code. From what I can tell, that hasn't seen any relevant real-world usage right now.

The basic idea behind this would be that we can always change the defaults to a "safer" value, but should really avoid doing the reverse (e.g. because we found some regression). The latter would break expectations of anyone who comes to rely on trashing over direct deletion. I'm also under the impression that we're not going to agree on the default here, and I'd like to avoid blocking to merge this PR. There's of course the risk that when reviewing that new issue we will not be able to resolve the disagreement.

---

Thanks for clarifying about the intent to trash by default. I am still leaning toward not doing this and deleting by default. We already have several safeguards in place, such as not actually deleting unless you add -d to beet rm and then requiring a Y to confirm the action.

I didn't even think about those safeguards, sounds like a good reason 👍

In a sort of classically Unixy UI sense, I think it would be somewhat surprising that an action that's supposed to delete files actually calls a binary named trash instead. I really like having this as an option, but I think it makes the most sense to have it off by default.

I'm not sure how large the actual risk is that there's indeed another program named trash that does not have the intended effect (how about Windows, though?). Still, one more reason to roll this out slowly.

About whether to have the "trash decision" internal to remove or external in the client: thanks to @wisp3rwind for putting together #3876! As that PR illustrates, it seems like the places in the code where we actually want to remove files may actually be more numerous than the places where we want to conditionally remove-or-trash files (which I imagine are only in library.py's remove methods, which are already meant to centralize logic for removing actual "content" files, i.e., music and album art). So I'm worried that having use_trash=True be the default will lead to confusing bugs in the future when we add code that wants to do "normal" removals.

Yes, I now agree that trash should be opt-in, for the reasons scattered throughout this comment.

I don't really object to having the check in util instead of library, but it still seems marginally more natural to me to do the latter? I dunno, no need to bikeshed and I don't feel terribly strongly about it, but maybe the above reasons will make a little bit of sense.

I believe that having it in util is useful, since it makes the trashing code available to plugins. There is one user already: The convert plugin, which might delete music on import when delete_originals=True (although one might argue that precisely because of this setting, trash should be bypassed...).

---

Unless we're sure that beets' trash-related code is 100% bug-free, it strikes me as arrogant to not make recycle bin / trash the default. #2499

That seems like a contradiction. I'm quite confident about the current "trash-related" PR. I'm by no means 100% sure that there's not a problem with an edge case we didn't think about (where, say, beets crashes and leaves you with some half-imported items in the database). There is a value to introducing it somewhat conservatively.

Add in an unbounded number of plugins that may also want to delete files, and it's a big source of errors for no benefit.

There is a very significant benefit: We should not ever move temporary files that we created ourselves to the trash. Such files with semi-random names popping up in the recycle bin are absolutely a cause for confusion, and exactly that will happen when implementing def remove(..., use_trash=True).
I'm not convinced that preventing external plugins from doing dangerous things is a relevant concern. We can't prevent them from os.removeing your data directly, anyway. Any (or at the least, most) plugins trying to delete your music directly is wrong already, since it should have used the Item.remove method. It could be worthwhile to add a section to https://github.com/beetbox/beets/blob/master/docs/dev/plugins.rst to explain usage of the remove function.

I skimmed the above conversation, so maybe I missed it: what is gained by not having trash functionality? If it just tries to call an outside program trash and silently falls back to the old behavior, what's the harm? Why do we need to avoid this?

In addition to @sampsyo's answer, I think there simply is no solution here that will suit everyone's workflow out of the box. Why should "absolutely everything needs to go to the trash" be the only viable way forward?

---

FWIW, if we don't go by the steps outlined above, I think this PR needs one more change: I did account for direct calls to util.remove in the tests in #3876, but that misses anything going through, for example, the importer. For our tests, we should ensure that running the them doesn't move anything to the actual trash on the machine.

[RollingStar]

Windows doesn't have that "rm = instant delete" paradigm to my knowledge, even in the CLI. Admittedly, Windows is half-supported right now (who on the dev team is enthusiastically using beets on Windows?). IIRC, there has been some discussion of just moving to the Windows Subsystem for Linux.

For temp files, Windows has the temporary files folder. This is all much lower level than I'm familiar with, but for temp files wouldn't the program be doing this anyway? https://en.wikipedia.org/wiki/Temporary_file#Creation

There is a value to introducing it somewhat conservatively.

Fair enough. I guess what I mean is, what does this look like 1-3 years from now? For me, trash-by-default is the most user-friendly and indeed "least astonishing" behavior. rm == delete on unix, but for someone not used to the unix command line (ex. Windows user), it could just as easily be remove. Even for me, knowing the rm command on Linux, I wouldn't have guessed that it deletes files irreversibly. That's not something I'm used to non-system programs doing (with limited Linux command line experience). Like, nothing I can do in Firefox Ubuntu will end with me deleting files without stopping in the trash first. It's splitting hairs though, if the config supports trashing, I can't complain much.

In my 4 years of using beets, I've had bugs cause beets to remove files that shouldn't have been removed, but I've never thought "I need these music files off my PC now, no backsies".

[sampsyo]

Thanks, @wisp3rwind; that plan sounds great!

I believe that having it in util is useful, since it makes the trashing code available to plugins. There is one user already: The convert plugin, which might delete music on import when delete_originals=True (although one might argue that precisely because of this setting, trash should be bypassed...).

That's a good reason! I too am not exactly sure what to do about that convert setting, but it is very easy to imagine other scenarios in plugins that want the same behavior.

FWIW, if we don't go by the steps outlined above, I think this PR needs one more change: I did account for direct calls to util.remove in the tests in #3876, but that misses anything going through, for example, the importer. For our tests, we should ensure that running the them doesn't move anything to the actual trash on the machine.

Also a very good point (and a reason to keep util.remove as actually-deleting by default).

In my 4 years of using beets, I've had bugs cause beets to remove files that shouldn't have been removed, but I've never thought "I need these music files off my PC now, no backsies".

Unfortunately, @RollingStar, I think there's a chance that these frustrating accidental deletions would not have been solved by a "trash" precaution. It's just as easy for a bug in beets to lose data by overwriting a file via a move, for instance, that would have slipped by this specific change to the util.remove function. So as inconvenient as it may be, I think the real solution here is to continue fixing those bugs…

[wisp3rwind]

Unfortunately, @RollingStar, I think there's a chance that these frustrating accidental deletions would not have been solved by a "trash" precaution. It's just as easy for a bug in beets to lose data by overwriting a file via a move, for instance, that would have slipped by this specific change to the util.remove function. So as inconvenient as it may be, I think the real solution here is to continue fixing those bugs…

Now you could argue that file moves should do backups first :) I don't have an overview over the situations we do moves in, so that might actually be worth discussing in a future issue/PR. In any case, let's get the uncontroversial part of the current PR merged. @justinmayer, how does the plan above sound to you? Do you have the time and energy to modify the defaults accordingly?

[justinmayer]

How does the plan above sound to you? Do you have the time and energy to modify the defaults accordingly?

@wisp3rwind: Assuming we're talking about the first two bullets points in your previous comments, then yes — I should be able to make time to update this PR with those changes. I'm traveling at the moment but will do my best to get that done by the end of June.

[wisp3rwind]

How does the plan above sound to you? Do you have the time and energy to modify the defaults accordingly?

@wisp3rwind: Assuming we're talking about the first two bullets points in your previous comments, then yes — I should be able to make time to update this PR with those changes. I'm traveling at the moment but will do my best to get that done by the end of June.

Yep, exactly, that'd be great! Don't hurry, enjoy the travelling!

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[justinmayer]

@sampsyo: Can you re-open this? I'll work on it this week.

[sampsyo]

Certainly!