andsens / homeshick

git dotfiles synchronizer written in bash
MIT License
2.11k stars 145 forks source link

fix link command for symlink directory #34

Closed ttddyy closed 11 years ago

ttddyy commented 11 years ago

When there is a symlink directory under my castle's home directory, homeshick link my_castle command failds to create a symlink.(It just creates an empty directory.)

In pull request, I added a symlink directory check. So that, the else clause, which is for symlink directory, get called appropriately.

andsens commented 11 years ago

Could you maybe write a positive and negative testcase? I am not entirely sure I understand the scenario, a testcase would explain it quite clearly and also ensure that future changes don't break this behavior.

ttddyy commented 11 years ago

Hi, I added unittest.

My use case is that I want to link a symlink-directory under castle's home to become a symlink under my home directory created after homeshick link action.

example is:

in my castle:

~/.homesick/repos/my-dotfiles/
  `-- my-module/  # git submodule or just directory
  `-- home/
    `-- .my-module  # symlink to ../my-module

after homeshick link my-dotfiles, I want

~/.my-module  # a symlink to the  "~/.homesick/repos/my-dotfiles/home/.my-module" which is a symlink to "../my-module"

Thanks,

andsens commented 11 years ago

Aha! I see now, this makes sense. I don't see how this could break anything, so I'll merge. You will need to fix the test case though: Don't modify existing test cases, each test case only tests one single functionality. If it tests multiple things, it's hard to tell which functionality breaks by looking at the function name and refactoring gets harder as well (actually testDeepLinking should have been split into two already, it does more than one thing).

ttddyy commented 11 years ago

ok, i'll update the test case and ping you. :)

ttddyy commented 11 years ago

Aside from test cases, do you prefer to have completely separate test repos for this use-case, or want to kind of reuse existing repos(rc-files, dotfiles)?

ttddyy commented 11 years ago

ok, updated the pull-req test-case to create own test repos: my-module for being submoduled, module-files to have symlink inside and be used by test case. Also, added an independent test case in test/link.sh: testSymlinkDirectory()

Let me know some more fixes are needed. :)

andsens commented 11 years ago

OK, this is really annoying... I was waiting for github to notify me via mail on your update, I never got the email, so I check in here only to discover you have already fixed everything. I am sorry about the delay, your tests check out and the fix is working, thank you for the contribution! I manually integrated your changes into the main branch, so it will show up as a normal commit and not a merge.