Add option to hardlink when importing #2445
Conversation
beets/util/__init__.py
Outdated
| raise FilesystemError(exc, 'link', (path, dest), | ||
| traceback.format_exc()) | ||
|
|
||
|
|
There was a problem hiding this comment.
this part about windows version less than 6 needs to go. I don't think they are supported on windows at all, on any version.
|
I think we also need to add a check to make sure both the source and the target are on the same device, otherwise the hardlink will fail. Simply copying what symlinks do is not enough. |
|
Writing a proper test for this will probaby require creating a fake device using tmpfs or the like |
|
I added a catch for the cross-device hardlink error so that the error's a bit prettier, however I'm not sure how to handle the test. tmpfs would indeed work, but it doesn't exist on OS X. Perhaps the implementation is simple enough at this point that we could get away without a cross-device hardlink test, we'd essentially be testing the Python implementation of |
beets/util/__init__.py
Outdated
| """Create a hard link from path to `dest`. Raises an OSError if | ||
| `dest` already exists, unless `replace` is True. Does nothing if | ||
| `path` == `dest`.""" | ||
| if (samefile(path, dest)): |
There was a problem hiding this comment.
No parentheses are necessary around the function call.
beets/util/__init__.py
Outdated
| def hardlink(path, dest, replace=False): | ||
| """Create a hard link from path to `dest`. Raises an OSError if | ||
| `dest` already exists, unless `replace` is True. Does nothing if | ||
| `path` == `dest`.""" |
There was a problem hiding this comment.
Please place the closing """ on a line by itself (that's PEP8 style).
beets/util/__init__.py
Outdated
| return | ||
|
|
||
| path = syspath(path) | ||
| dest = syspath(dest) |
There was a problem hiding this comment.
It might seem like a shortcut, but doing this once and overwriting the variables will cause problems: namely, the exceptions below use path and dest, which will now be system paths. This will produce strange results on Windows, where a ridiculous prefix needs to be added for syspath results.
docs/reference/config.rst
Outdated
| It will fail on Windows. | ||
|
|
||
| It's likely that you'll also want to set ``write`` to ``no`` if you use this | ||
| option to preserve the metadata on the linked files. |
There was a problem hiding this comment.
Instead of copying and pasting the above two paragraphs, can you please make this a little more human by saying something like:
As with symbolic links (see :ref:
link, above), this will not work on Windows and you'll want to setwritetono.
|
Thanks for the feedback - most of your suggestions applied to the original |
|
Ah, great! I hadn't realized that we had the same issues just above in the symlink function. Thanks for fixing that too. |
|
Thank you again! This is all merged up. ✨ |
Add option to hardlink when importing
|
Sweet, thanks for the fast response 🎉 |
This PR adds an option to hardlink files when importing, similar to the existing
linkoption that enables symlinks. Tests in the same style as the symlink tests are included.