-
- Notifications
You must be signed in to change notification settings - Fork 674
Support choosing checkout branch method when status is not empty #2494
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
Conversation
Allow choosing a checkout branch method when status is not empty
extrawurst left a comment
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.
Thanks for looking into this! I left the first round of feedbacks
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.
hi, apologies for the intrusion. i have come across this repo just now and have been poking around. i hope you don't mind but i've left a few comments and questions i have, but don't feel obligated to reply
| @Fatpandac I compared our behavior when choosing |
extrawurst left a comment
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.
We should either fix this and stash staged and unstaged separately or flat out error if we detect that this will happen. Wdyt?
extrawurst left a comment
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.
Considering the findings I would further argue lets move the logic to checkout with different options to asyncgit and lets cover them with proper unit tests that recreate the various scenarios and the expected results/errors we want to achieve. this is a fundamental enough feature to warrant that quality and resilience for future changes not breaking it
| @Fatpandac if the stash staging and non-staged changes is gonna be hard (I assume so, I am not aware we have that machinery yet) then lets for now rip out the stashing option and support only |
| @extrawurst I agree. I'll remove the |
d8500df to 0db55e9 Compare | @Fatpandac please resolve conflicts and let me know when this is ready for a new round of reviews |
| @extrawurst Could you please take a look at this comment? I’m not sure how to resolve it. |
| @Fatpandac can you resolve the merge conflict, then we can merge |
extrawurst left a comment
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.
LGTM
Head branch was pushed to by a user without write access
| Done |
This Pull Request fixes/closes #2404.
It changes the following:
I followed the checklist:
make checkwithout errors