Add log message on verbose, when beets renames destination file during move. #4924
Conversation
beets/library.py
Outdated
| 'File already exists at dest, changing final path slightly, ' | ||
| "to be unique, from '{}', to '{}'.", |
There was a problem hiding this comment.
Just a suggestion to slightly simplify the wording here… how about this?
File exists at
{}. Avoiding conflict with new name:{}
| dest = util.unique_path(dest) | ||
| log.debug( |
There was a problem hiding this comment.
I think you may need a if dest != old_dest here. This may also need some careful testing to make sure it doesn't fire spuriously when these point to the same path but are just normalized differently… hopefully that doesn't happen, but a few test runs should reveal the answer!
There was a problem hiding this comment.
Normalized differently? I admit I had a big assumption that the stuff passed in would be comparable. Should both incoming paths go through util.bytestring_path, would that normalize them the same way? I'm obviously not familiar enough with the codebase to understand where paths might be normalized different ways. I'll dig further.
There was a problem hiding this comment.
If you have any specific suggestions of test runs, and have the time to, please let me know. All tests pass, so I'm assuming you mean something outside of those.
There was a problem hiding this comment.
About being "normalized differently," I'm just thinking about all the various ways in which different paths can refer to the same file. For example, os.path.normpath or os.path.abspath are different ways of transforming paths so they still refer to the same file. It's just worth double-checking to make sure that we're not doing that kind of thing when transforming the path (or that the original path is already normalized in the same way, which it probably is).
I suggested if dest != old_dest so that (normalization aside) this message only gets printed when there is an actual change.
For testing, all I mean is running this on a few files—perhaps in the same cases that presumably motivated you to add this log message—and make sure that the message shows up when it's supposed to (and doesn't appear when it isn't).
|
Is there something to do here still @fleur? Was the |
|
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. |
Description
First step in fix of #2895. Add log message on verbose, when beets renames destination file during move. The issue says it may be trickier than it looks. It did not seem tricky, so only doing the first step suggested, to see if I missed something.
To Do