Skip to content
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

Merged
merged 4 commits into from
Sep 29, 2024
Merged

Don't use git tags in pull #226

merged 4 commits into from
Sep 29, 2024

Conversation

yut23
Copy link
Contributor

@yut23 yut23 commented Sep 25, 2024

As seen in #225, this approach was more complicated than it needed to be and caused confusion for users.

As seen in andsens#225, this approach was more complicated
than it needed to be and caused confusion for users.
Copy link
Owner

@andsens andsens left a 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)
Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

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?

Copy link
Contributor Author

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

Copy link
Owner

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 git@github.com:user/dotfiles
  printf '[branch "master"]\n  remote = origin\n  merge = refs/heads/master' >> .git/config)
)

lib/commands/pull.sh Show resolved Hide resolved
bin/homeshick Outdated Show resolved Hide resolved
@yut23
Copy link
Contributor Author

yut23 commented Sep 27, 2024

I added a test for a pre-initialized castle and silenced the rev-parse error. It currently goes through with the pull and runs symlink_new_files, but the git diff fails as @{1} doesn't exist. We could have it check for that case and list all the files in home/ instead, but that would mean more special cases.

@andsens
Copy link
Owner

andsens commented Sep 28, 2024

Brilliant. Thank you for all the work Eric! I just noticed that I can't merge though, the PR is for master rather than development, could you quickly fix that?
After that I'll run with it for a short week and make sure nothing is amiss and then merge it.

@yut23 yut23 changed the base branch from master to development September 28, 2024 13:46
@andsens andsens merged commit 9768486 into andsens:development Sep 29, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants