Don't import files with missing permissions on move+write

[p3732]

When importing files with moving and metadata writing enabled [import: \n write: yes \n move: yes], files always get moved and added to the library. However when the user has no write access to these files, the files get imported anyway, but the metadata can not be written.

For me this occurs every now and then when importing from an unpacked archive, that was created with different permissions.

Archlinux, python 3.7, beets 1.4.7 plugins: bandcamp, beatport, bucket, discogs, fetchart, inline, the

[nathdwek]

What would the desired behaviour be? From the title I assume you would like to skip those? Should some kind of warning be emitted?

Also might be stupid, but what is the current behaviour and how is it problematic? Is the user informed that the metadata cannot be written? Is an exception not caught?

[p3732]

Yes, I think the files should be skipped, since the write operation will not successfully be able to complete.

Current behaviour is that the files get moved first and the tag writing fails afterwards, printing error writing xyz.mp3: [Errno 13] Permission denied: b'xyz.mp3' where xyz is the whole file path.

Performance-wise I don't know how much overhead looking up the file permissions beforehand is, but I suppose the bottleneck for most use-cases would still be the online tagging lookup.

[p3732]

Just to clarify, beets does not crash or anything with the current behaviour and the errors can be fixed by manually setting the permissions and then running 'beet write' (assuming no other uncommited changes to file tags would be overwritten)

[sampsyo]

Hmm… I'm not sure about this. Imagine the way that you'd need to clean up the problem afterward. In the current system, you just have to fix the permissions (chmod the files), use beet write to finish updating their tags, and you're done. If we just skipped the files with a warning, you could end up with a partial album—some files imported, some files left behind. Then it would be up to you to reassemble the pieces and merge them into a single album. Isn't that less convenient?

[p3732]

Oh, I did not consider this, since everytime I encountered this problem all files of an album were affected. Would skipping the whole album be an option, as soon as one file does not have write permissions? I do see that this seems like an awful lot of overhead for an uncommon use case...

[sampsyo]

Perhaps! One hard thing, though, is that it's never possible to tell 100% of the time whether a file will be movable or writable. It could lead to an unexpected error later in the pipeline even though the permissions look fine.

Maybe it would be worthwhile to extend the permissions plugin, which is meant for adjusting file permissions? Have you taken a look at that?

[p3732]

I learnt of the permissions plugin when i searched for issues to avoid a creating a duplicate, but as of now the permissions plugin sets file permissions after the import process (as stated in permissions.py: "Fixes file permissions after the file gets written on import.")

I just tried importing files with wrong permissions with the permissions plugin enabled and while the tags expectedly could not be written, a simple 'beet write' fixed the tags. So extending the plugin should be possible.

Therefore the config would need to be extended, I see two possible options:

1. Simply adding a set_before_write: true variable
2. Adding a full fledged before_write: {file: 644, dir: 755}, after_write: {file: 644, dir: 755}

The former seems more reasonable.

[p3732]

Am I correct in assuming that self.register_listener(import_task_created) would be the correct position in the pipeline to add the permission change?

[sampsyo]

Cool! That seems like a good idea. I think a single before_write: true flag would be fine; the more granular control doesn't seem too useful.

The import_task_created event would actually do it to the original files, before they're even read to kick off the import process. That would certainly be another option, but I don't think I'd call that before_write. For something that happens just before writing, I might recommend the write event. That actually happens not just during import but for every write operation—which might or might not be what you want (including beet write, for example). For something that happens specifically between copying/moving files and writing their metadata during the import process, I think we'd have to add a new event.

[p3732]

So I wanted to finally tackle this issue and by now also have some more insight on what events are in beets (calls to plugins.send(<event>)).

The write event is called from library.py and thus, as you said, is called every time something is written. This is not what I originally reported this for, but actually would be an interesting expansion of the permissions plug-in, in order to fix changed permissions in the library with a simple beet write.

Anyway, my question is, for adding an write_import event of some sorts, would it be sufficient to call this in importer.py / ImportTask / manipulate_files() or is there more to it? (Since there also is BaseImportTask, implying other code can create it's own ImportTask, but I could find any plugin doing so.)

[sampsyo]

Yep! That would do it. (BaseImportTask is not meant to be subclassed in plugins—it's just a generic base class for use in both album and item imports.)

[p3732]

How does https://github.com/p3732/beets/commit/9974cc447d1ca0a7672f55b5c7a94c49bcc47c71 look?

It worked for me when importing from files with faulty permissions.

[p3732]

Coming to think of it, maybe setting the permissions before and after is not necessary and the plugin should either listen to item_imported or write_import (when before_write is set).

[p3732]

Included in the pull request.

[p3732]

Found another mistake.

[stale[bot]]

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

This issue 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.