-
-
Notifications
You must be signed in to change notification settings - Fork 71
Bugfix checkouting existing branch. Fixes issue #109 #149
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
base: master
Are you sure you want to change the base?
Bugfix checkouting existing branch. Fixes issue #109 #149
Conversation
|
Hi @brikis98 , @hongil0316 It's been 2 months since creation of this PR. Can you please review? |
|
@ceschae, @james00012, @ZachGoldberg Can you please have a look on this PR? |
| // Create new branch | ||
| checkoutNewBranchErr := worktree.Checkout( | ||
| &git.CheckoutOptions{ | ||
| Hash: ref.Hash(), |
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.
Create a new branch from the default HEAD, so any existing remote branch history is skipped
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.
Fist of all thanks @denis256 for review, then please let me describe how new code works.
Code first fetches remote branches, then it tries to checkout existing branch if exist and return from this function on line 204. If checkout of existing branch is not successful then it creates new branch from current HEAD here.
|
I think PR needs to be updated with changes from master, now it fails on CICD I also noticed that there are no tests - I think we need to add such tests, without tests we can't track that checkout of existing branches will continue to work over time |
|
I think there is a risk of losing changes from branches:
repository/repo-operations.go 207-231 |
|
Again thanks for the review, I will try this week to update tests and back-merge latest master to my branch. How code works I have described in conversation: #149 (comment). |
Description
Fixes #109.
Original code did equivalent of following:
new code is properly checkouting remote branch if exists
Problem with original code is not only that's not convenient, but it's as well bugy, it's silently supposing that
<branch name>was created on top of default remote branch name plus it's supposing remote branch name haven't changed in meanwhile until remote<branch name>was created. Ofc this conditions are often not met and thats the case when the bug #109 appears.TODOs
Read the Gruntwork contribution guidelines.
Release Notes
Fixed #109 checkout remote branch locally if exists.