Use shutil.move() to move files.

[aereaux]

Description

Fixes #4622.

I don't think this changes other behavior of the function, but it fixes the problem with the moved files being created with the wrong permissions for me.

To Do

• Documentation. (Changed the docstring)
• Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
• Tests. (Very much encouraged but not strictly required.)

[wisp3rwind]

Hmm, one of the commits that popped up in the git blame of the move function is this:

commit b04096de25fe9549f8f96e9a610a4936aa6b4a92
Author: Adrian Sampson <[email protected]>
Date:   Sat May 19 11:52:53 2012 -0700

    do not preserve metadata during copy-move (GC-383)
    
    The shutil.move() function attempts to copy metadata (e.g., permissions and
    mtime) when copying a file across filesystems. This always fails on Samba shares
    because the utime() call is never permitted by normal users. We don't care about
    preserving mtimes across moves, though, so this commit eschews shutil and
    reimplements the move algorithm.

I didn't check thoroughly, so there may be more differences to shutil.move() that we may want to preserve.

[aereaux]

Good catch. We do want to copy file permissions though, that's causing several problems. I'll see if I can setup a samba share to test on as well. If that still causes problems maybe trying shutil.move() and then the more complicated algorithm may be the way to go unless anyone has any other ideas.

[Serene-Arc]

I think the more complicated version is there for a reason. I don't want to remove a large amount of code without a good justification and a fair amount of testing. Perhaps a separate rule for files originating in /tmp or changing the permissions after the move if necessary.

Also, I am the one that made fetchart write to a temp file iirc. The reason for that doesn't matter too much but it's not security related; if needed, the process of creating the file can be altered, which would remove the necessity of changing the move function at all.

[aereaux]

Thanks for the feedback! I'm getting this problem with more than just fetchart, music files imported normally (I think it's mostly when importing across filesystems or from a .zip file) also are created with the wrong permissions. So just changing the logic in fetchart wouldn't fix this other problem I'm having.

Maybe the best solution here then would be to make sure that this function preserves permissions when moving. I may need help with testing any changes in more esoteric situations. And then possibly changing fetchart to create the temp file with more normal permissions if still needed.

Does that sound like a good solution? Are there any other concerns you have here with any changes?

[Serene-Arc]

Preserving or changing permissions seems like a good fix, since it seems according to that commit message that it's only a specific part of the metadata that SAMBA rejects.

The fetchart temp file was implemented because, when loading from a file already on disk, if there were modifications specified by the settings (such as resizing) then those would first be performed on the original file before it was moved, changing it. So I made it copy everything to a file in memory (which is what /tmp is) to do all those changes. But there's no specific reason that I chose the method that I did besides standard practices.

If possible, reading the umask might be a good option. If the user has that set then it should be honoured wherever possible. There's a bit of wiggle room with technically copying files but even specifying that if there's no umask value, we use 755 (the default umask on Linux) then I think that's a fine compromise. That won't cause any problems for users beyond possibly being too wide, and if it is, then the user would have the system knowledge to fix that. If you're running beets on a system where those file permissions matter and you haven't set umask to something else, then you can't really be helped.

I think the general priority should be:

1. Use the existing file permissions
2. use the permissions that umask indicates
3. Maybe a fallback of 500? Not sure, but this shouldn't be reached often.

What do you think about that?

(Also, if you rebase off master, the formatting CI will work :) )

[aereaux]

OK, this should be the bare minimum to try to copy file permissions. I think that this shouldn't raise any errors based on the docs, but if someone could test what happens when you use it on windows or with a samba share that would be useful.

[snejus]

@aereaux given that copying the metadata is a somewhat optional action, we probably do not want any possibility of exceptions here. Consider wrapping the copystat call with contextlib.suppress which will deal with any (potential) issues with Windows or a Samba share.

[aereaux]

Consider wrapping the copystat call with contextlib.suppress which will deal with any (potential) issues with Windows or a Samba share.

Do you know what possible error types that could raise? It would probably be good to just suppress those error types.

[snejus]

Consider wrapping the copystat call with contextlib.suppress which will deal with any (potential) issues with Windows or a Samba share.

Do you know what possible error types that could raise? It would probably be good to just suppress those error types.

Good point. We should probably respect users pressing CTRL-C meanwhile 😁 I think OSError should be a safe bet.

[aereaux]

Just pushed this change. Is there anything else that would need to be added for this to get merged?