Closed yut23 closed 9 months ago
Welcome @yut23. Now those are some high quality PRs right there. Thank you very much.
Now I'm wondering why I never just saved the current HEAD commit sha and compared against that though. I'm guessing it was too much bookkeeping when you are pulling multiple castles🤔 ...
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.
OK, I've figured out how the tag gets cleaned up. pull
is run inside a subshell in bin/homeshick, so err
will just exit from that subshell. As long as the exit code isn't EX_USAGE, symlink_cloned_files
will always be run, and the first thing it does is parse the tag and delete it.
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.
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 bin/homeshick
and only calls symlink_new_files on the repos that changed. I thought the tag method was more elegant initially, but it does introduce more edge cases and error handling.
*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
pull
checks 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_START
and the current second increments between$now
and 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.