-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't use git tags in pull #226
Conversation
As seen in andsens#225, this approach was more complicated than it needed to be and caused confusion for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! This seems way more robust. I commented with some minor nitpicks, and some ideas for improving resililency.
bin/homeshick
Outdated
@@ -182,7 +183,13 @@ case $cmd in | |||
refresh) | |||
(refresh $threshhold "$param") ;; | |||
pull) | |||
(pull "$param") && pull_completed+=("$param") ;; | |||
prev_hash=$(cd "$repos/$param" && git rev-parse HEAD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At work we pre-initialize the castles with git init <castle>; git remote add origin ...
when provisioning servers and put a homeshick pull --batch; homeshick link -f; exec $SHELL
in an otherwise empty .bashrc
. This way the dotfiles are initialized on first login. However, in that case HEAD
does not exist and rev-parse
will fail.
Maybe just bail when rev-parse
fails and assume the link
call is handled in another way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a test for this later, but it should print something different from $curr_hash
and we're not checking the exit code, so this should behave the same as a normal pull.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, though maybe we should silence the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get that pre-initialization to work. After running those commands in a clean shell, I get this error message on the current master:
no upstream Could not pull dotfiles, it has no upstream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the actual commands we're running. I believe the last line is what does the trick.
git init $HOME/.homesick/repos/dotfiles
(
cd $HOME/.homesick/repos/dotfiles
git remote add origin [email protected]:user/dotfiles
printf '[branch "master"]\n remote = origin\n merge = refs/heads/master' >> .git/config)
)
Also fix minor style nit.
I added a test for a pre-initialized castle and silenced the rev-parse error. It currently goes through with the pull and runs |
Brilliant. Thank you for all the work Eric! I just noticed that I can't merge though, the PR is for |
As seen in #225, this approach was more complicated than it needed to be and caused confusion for users.