Fix pull not symlinking new and renamed files #219
Conversation
|
Welcome @yut23. Now those are some high quality PRs right there. Thank you very much. The one issue I see is that something goes wrong between the tag creation and deletion, resulting in the check failing later on because of an internal rather than external conflict. No matter how you look at it though, this actually makes things work better than before. So I'm inclined to just merge it unless you want to address that issue first? |
|
I originally wrote this code 2 years ago (!), and have been using it in my personal fork since then. I was thinking about that potential issue too, but I've never run into it, so it's probably not a huge problem. |
yut23
force-pushed
the
fix-pull-symlinking
branch
from
5e1d206 to
8004dc1
Compare
|
OK, I've figured out how the tag gets cleaned up. I think this could fail if the user pulls two repos, the second of which already has the tag. This would make pull return EX_USAGE, and the tag in the first repo wouldn't be deleted. I'll see if I can write a test for that. Returning EX_DATAERR instead of EX_USAGE should fix it, and probably makes a bit more sense. |
|
Hmm, that doesn't work. It looks like I was returning EX_USAGE specifically so symlink_cloned_files wouldn't delete a user-created tag. |
yut23
force-pushed
the
fix-pull-symlinking
branch
2 times, most recently
from
3883daf to
951cbe5
Compare
lib/commands/pull.sh
Outdated
| local now | ||
| now=$(date +%s) | ||
| if ! git_out=$(cd "$repo" && git diff --name-only --diff-filter=A "HEAD@{(($now-$T_START+1)).seconds.ago}" HEAD -- home 2>/dev/null | wc -l 2>&1); then | ||
| if ! git_out=$(cd "$repo" && git diff --name-only --diff-filter=AR "$before_pull" HEAD -- home 2>/dev/null | wc -l 2>&1); then |
There was a problem hiding this comment.
Why not just diff to the tag and then delete it after? It's one less command that can go wrong.
There was a problem hiding this comment.
I think it was to make sure the tag still gets deleted in case $repo/home isn't a directory, but it looks like we can just move that check after the diff.
|
I've also got an alternate implementation in https://github.com/yut23/homeshick/tree/fix-pull-symlinking-old, which checks the git hash before and after calling pull in |
pull creates a temporary tag in each castle before running git pull, and uses it to determine which files have been added (and renamed) in symlink_new_files, which also deletes it.
yut23
force-pushed
the
fix-pull-symlinking
branch
from
951cbe5 to
bed4cfb
Compare
|
*ping. Do you think this is ready or would you like some more time to work on the PR? |
|
I think it's ready. I've been using it for a while now with no issues. |
This PR reworks the way
pullchecks for new files for symlinking, which was broken before due to a missing$in front of the arithmetic expansion. This made it resolve to HEAD instead of HEAD@{1.seconds.ago}.It now creates a temporary tag in each castle before running git pull, and diffs against that in symlink_new_files. It also wasn't checking for renamed files, but that's easily fixed with
--diff-filter=AR.This should be much more robust than the previous time-based method, which broke intermittently in my local tests and once during actual use. If
$now == $T_STARTand the current second increments between$nowand the git diff call, then HEAD@{1.seconds.ago} will point to the pull itself rather than the commit before it.I've also added a whole bunch of tests covering all the error conditions and edge cases I could think of.