Add option to link files instead of moving or copying them.

[Rovanion]

Hi,
I've made modifications to beets so that it links in the files instead of copying or moving them. Beets now links when both link and copy are set to no in the yaml since I haven't been able to get a link: yes option into the configure['import'] map.

So this works, as far as I can tell. But I would need help with what I should add where to have for command line flags and config file settings. It only works on systems where Python implements link i.e. not windows.

[sampsyo]

Cool; thanks for addressing this! For the record, this fixes #114.

Let's get this finished so we can merge it as soon as possible. Here's what we need to do:

• Add that configuration option. A simple if config['import']['link']: check should do fine. The default (no) will also need to be added to config_default.yaml. I'm guessing that this appeared not to work for you because you tried dumping out config['import'] as a dictionary, which will only contain the top-level source of the information. If you just use the right config option in a conditional or as config['import']['link'].get(bool), everything should work. See the Confit docs for details on how that stuff works.
• Tests.
• Documentation for the config option.
• Minor style issues in the new code. See Travis' output for a few line notes.

Thanks again for tackling this! It will be great to have in beets core.

[sampsyo]

Ah, forgot one more loose end:

• Sensible error message when symlinking is not supported. We need to raise a user-visible error and stop the process instead of printing a message and failing silently.

[Rovanion]

Right, what type of exception should I raise?

[sampsyo]

Good question. The utility function should perhaps raise a special-purpose exception? Or FilesystemError? That's up for debate. Then, the UI should catch this and raise a UserError instead to print a nice message. The tricky part is making sure that the UI code is what raises the UserError so the utility and library stay pure.

[Rovanion]

The python os library will raise an exception itself [0] if the function is not supported on the platform, perhaps not try-caching but just letting that exception go through would be better?

I need pointers as to what documentation and how I would write tests for the new code.

[0]https://docs.python.org/2/library/os.html

[sampsyo]

Looking great! Thanks for adding the config option.

It raises an AttributeError according to my VM:

>>> os.symlink('x', 'y')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'module' object has no attribute 'symlink'

which is unfortunately unspecific.

And good question about the tests. The harnesses are fairly well developed for importer tests; all you need to do is follow the template for one of the existing file-handling tests (e.g., test_import_copy_arrives and test_import_with_move_deletes_import_files). As you can see, the tests set configuration options, run an import, and then inspect the filesystem to see what happens. The test here should turn off moving and copying, enable linking, and make sure that the original file exists but the new location now contains a symlink to the existing file.

Thanks again for following up on this—looking better and better!

[frafall]

Nice job, I've tested it to replace my old python scripts and it works nicely with the symbolic links.

Only one issue though, I build the repository on a Debian system and share with Samba to all Windows machines in the house, but, as the symbolic links are absolute instead of relative the music files are not found.

Would it be possible to have an option to make symbolic links relative?

Best regards
frafall

[Rovanion]

Having the path be relative should absolutely be possible, though I have no idea how. Though it seems python comes with batteries included: https://docs.python.org/2/library/os.path.html#os.path.relpath

I've had a break from working on this firstly because I got a heavy work load on me at uni and now because I'm on vacation mostly without internet access, but secondly because writing tests was confusing.

But I'll hop onto irc and ask @sampsyo about the test writing at some point in the future.

[frafall]

Ye, most of the link stuff is easy, I used Unipath (https://pypi.python.org/pypi/Unipath/) in my old scripts which did the job of making the relative link.
Question is really if we need both, then how do we select which to use and so on....

[frafall]

Modifying util.init.py to the following did the trick for me:

def link(path, dest, replace=False, relative=True):
    """Create a symbolic 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)):
        return

    path = syspath(path)
    dest = syspath(dest)

    if relative:
        path = os.path.relpath(path, os.path.realpath(os.path.dirname(dest)))

    if os.path.exists(dest) and not replace:
        raise FilesystemError('file exists', 'rename', (path, dest),
                              traceback.format_exc())
    try:
        os.symlink(path, dest)
    except OSError:
        raise FilesystemError('Operating system does not support symbolic '
                              'links.', 'link', (path, dest),
                              traceback.format_exc())

[sampsyo]

Cool! Perhaps one of you can update this PR/open a new one?

[frafall]

Ill have a look, as an old svn but newbie git user Im still feeling my way around lol

[Rovanion]

Is there any point at which absolute links are preferable to relative? I can't think of one.

About updating or opening a new pull request: Either works. You pull my changes, add yours and then push your changes to github. After that I get and push your new changes to this branch or you open up a new pull request from your branch. Only difference being that the former keeps comments and changes all in one thread.

[sampsyo]

Here's one advantage of absolute symlinks to consider: they don't break when they move. If you every change your directory structure and move the symlinks to a different location, relative symlinks would need to be rewritten but absolute ones would not.

[frafall]

Well relative ones work within a network share, no matter where you mount it (SMB, NFS), absolute wont.

If we start moving the directories around chances are the original file location will change as well and then all bets are off...

But, it might be nice to have an option to control it as I can't see every method ppl like to use to organize.

[Rovanion]

Then we also have hard links which don't break on file system structure changes at all, you can move both the "source" and "destination" file, but require the files to be on the same fs. So then we have three types of links good for three different situations.

@frafall Though relative symlinks won't work if the destination isn't shared in the same tree as the link, right?

[sampsyo]

Right. There are advantages to both.

[frafall]

Lol so very true, so it all depends on how people prefer to organize their music.

That's the main reason I moved to Beets from my own Python scripts, it really is so flexible can accommodate pretty much everyone's needs :)

[Rovanion]

@sampsyo What wuold you say about just trying to land this, and then we could add the option for relative links later? I'm quite lost on how to write these test cases. D'you think you could lend me a hand with that?

[geigerzaehler]

@Rovanion, I can help you with the tests: Have a look at the NonAutotaggedImportTest class. This test case is concerned with the file operations on imports (we don’t need the autotagger). Basically you want to copy some of the test methods and check for the correct symlinks. A basic test should make the following assertions

• Database item created and path points to a symlink
• Symlink points to an audio file that

If your feeling adventurous you can also try to test it with the tagger to make sure that tags are updated in the original file the symlink points to.

[Rovanion]

The tagger should preferrably be disabled by default when the symlink
option is turned on since, at least in my mind, the goal is to keep the
original files untouched so that they can still be used by some other
service without duplicating data.

2014-09-11 11:10 GMT+02:00 geigerzaehler notifications@github.com:

@Rovanion https://github.com/Rovanion, I can help you with the tests:
Have a look at the NonAutotaggedImportTest class
https://github.com/sampsyo/beets/blob/master/test/test_importer.py#L212.
This test case is concerned with the file operations on imports (we don’t
need the autotagger). Basically you want to copy some of the test methods
and check for the correct symlinks. A basic test should make the following
assertions

• Database item created and path points to a symlink
• Symlink points to an audio file that

If your feeling adventurous you can also try to test it with the tagger to
make sure that tags are updated in the original file the symlink points to.

—
Reply to this email directly or view it on GitHub
#710 (comment).

[geigerzaehler]

The tagger should preferrably be disabled by default when the symlink option is turned on since, at least in my mind, the goal is to keep the original files untouched so that they can still be used by some other service without duplicating data.

Ah, ok. Nevertheless, if it is possible people will do it and we should test it.

[Rovanion]

Yeah that's correct.

2014-09-11 11:44 GMT+02:00 geigerzaehler notifications@github.com:

The tagger should preferrably be disabled by default when the symlink
option is turned on since, at least in my mind, the goal is to keep the
original files untouched so that they can still be used by some other
service without duplicating data.

Ah, ok. Nevertheless, if it is possible people will do it and we should
test it.

—
Reply to this email directly or view it on GitHub
#710 (comment).

[sampsyo]

Hmm, I'm a little confused—maybe I'm missing something, but the current diff does not seem to add anything to the import process. It just seems to add the utility functions and extra flags to the model methods. Maybe something got lost in the git munging?

[Rovanion]

@sampsyo I probably messed up my rebase on master.

[sampsyo]

Okay, cool—if you can sort out the rebase, then it does seem like this is worth merging!

[sampsyo]

Thanks again! I've finished this up, @Rovanion. Now merged.

[Rovanion]

Thank you!