-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New checkpoints algorithm #939
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
|
| } | ||
|
|
||
| if (stashSha) { | ||
| console.log(`[restoreMain] applying stash ${stashSha}`) |
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.
Use the injected logging function (this.log) instead of console.log for consistency & centralized logging (see our Dev Standards).
| console.log(`[restoreMain] applying stash ${stashSha}`) | |
| this.log(`[restoreMain] applying stash ${stashSha}`) |
mrubens
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.
Nice!
Description
I evaluated a bunch of options, including detecting locked files, and ultimately I think the best option is to stop using
git stashand instead using something which won't put your repo in a bad state when git errors occur for any reason.The
git stashoperation is fundamentally unsafe because it is not guaranteed to be atomic; a failure in the middle of stashing can cause havoc. Ideally a failure should return the git workspace and index. The good news is that git has such an atomic operation; namely a commit. So if we simulate a stash by creating a "stash" branch and committing to it then a failure will leave the index exactly as it was before you tried to commit, and this is much safer.To that end, I changed the
saveCheckpointlogic to work exactly like it was before, but without using a destructive stash operation (note that I am using a non-destructivegit stash createoption in order to preserve the state of your staged and partially staged files).Overall this is more complex, but safer.
The unit tests are pretty good and they pass without any modifications 💪
Type of change
How Has This Been Tested?
Checklist:
Additional context
Related Issues
Reviewers
Important
Replaces
git stashwith a safer temporary branch commit method inCheckpointServicefor atomic checkpoint operations.git stashwith temporary branch commits inCheckpointServicefor atomic operations.saveCheckpointnow creates a temporary branch, stages changes, commits them, and cherry-picks to a hidden branch.pushStash,applyStash, andpopStashfunctions.saveCheckpointby restoring the main branch and deleting temporary branches on failure.CHECKPOINT_BRANCHandSTASH_BRANCHconstants toCheckpointService.This description was created by
for 6dbb596. It will automatically update as commits are pushed.