Fix to properly revert after aborting modify#167
Open
skarim wants to merge 3 commits into
Open
Conversation
Introduce IsCherryPickInProgress() (detects .git/CHERRY_PICK_HEAD) and split the existing cherry-pick reset into two distinct operations: - CherryPickQuit() runs `git cherry-pick --quit`, clearing the sequencer state without touching the index (used to clear stale state before starting a fresh cherry-pick). - CherryPickAbort() now runs `git cherry-pick --abort`, which fully restores the working tree and index to the pre-cherry-pick state. The previous CherryPickAbort() ran --quit, which leaves an unmerged index and therefore cannot recover a conflicted fold-down. Integration tests cover the in-progress detection, the full abort restore, and the --quit-leaves-index behavior.
When `gh stack modify` hit a rebase or cherry-pick conflict it saved state with phase "conflict" and told the user to run `gh stack modify --abort` to restore. But runModifyAbort had no case for PhaseConflict, so it fell into the default branch that merely printed "unexpected modify state phase" and deleted the state file without unwinding. The in-flight rebase/cherry-pick stayed active, branches were left partially rewritten, and the deleted state file also made --continue impossible: the stack was stuck in limbo. Fixes: - runModifyAbort now unwinds on PhaseConflict (same recovery as PhaseApplying), aborting the in-progress operation, resetting branch tips to their pre-modify SHAs, restoring stack metadata, and clearing state. - Unwind now also aborts an in-progress cherry-pick (fold-down conflicts), not just a rebase. Without this the restore checkouts would fail on the unmerged cherry-pick index. - ContinueApply now records a subsequent cascade-rebase conflict as ConflictType "rebase" instead of leaving a stale "cherry_pick", so the next --continue calls RebaseContinue rather than failing in CherryPickContinue. Adds coverage for the conflict-phase abort, pending-submit no-op abort, Unwind aborting an active cherry-pick, and the cherry-pick to rebase ConflictType transition.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes gh stack modify --abort so that aborting after a rebase/cherry-pick conflict actually restores the stack instead of deleting the state file and leaving the repository in a broken, unrecoverable state. runModifyAbort previously had no case for the conflict phase, so it fell through to the default branch that printed "unexpected modify state phase" and cleared state without unwinding. The change fits into the internal/modify recovery flow and the internal/git Ops abstraction.
Changes:
runModifyAbortnow treats theconflictphase likeapplyingand unwinds the stack (abort in-progress git op, reset branch tips, restore metadata, clear state).Unwindnow aborts an in-progress cherry-pick (git cherry-pick --abort) in addition to a rebase, and the git layer splits the old abort intoCherryPickQuit()(--quit) andCherryPickAbort()(--abort), addingIsCherryPickInProgress().- A follow-on cascade conflict after a fold-down now updates
ConflictTypefromcherry_picktorebaseso the next--continueresumes correctly.
Show a summary per file
| File | Description |
|---|---|
cmd/modify.go |
Handle PhaseConflict in --abort by unwinding instead of falling into the default branch. |
cmd/modify_test.go |
New tests for conflict-phase unwind and pending-submit no-op abort. |
internal/modify/apply.go |
Unwind aborts active cherry-picks; fold uses CherryPickQuit; cascade conflict records ConflictType="rebase". |
internal/modify/apply_test.go |
New tests for cherry-pick abort during unwind and the cherry-pick→rebase transition; mock alignment. |
internal/git/gitops.go |
Adds CherryPickQuit, CherryPickAbort (--abort), and IsCherryPickInProgress. |
internal/git/git.go |
Package-level wrappers; CherryPickAbort now returns an error. |
internal/git/mock_ops.go |
New mock fields/methods for the added Ops methods. |
internal/git/gitops_test.go |
Integration tests for cherry-pick in-progress detection, --abort restore, and --quit behavior. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Medium
ContinueApply removes the folded branch from the in-memory stack after a fold-down cherry-pick is resolved, but a subsequent cascade rebase conflict only saved the modify state file, not the stack metadata. On the next --continue the on-disk metadata (folded branch still present) was re-read, and because ConflictType is now "rebase" the fold-removal block was skipped, so the final save resurrected the folded branch as a phantom entry pointing at an orphaned tip. Persist the stack file alongside the state file on a cascade-rebase conflict, mirroring ApplyPlan's save-on-conflict, so the fold removal survives recovery. Adds an end-to-end regression test covering the fold-then-cascade-conflict path across two --continue calls.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix
gh stack modify --abortleaving the stack in a broken state after a conflictSummary
When
gh stack modifyruns into a rebase or cherry-pick conflict, it stops and tells the user they can restore their stack withgh stack modify --abort. Following that advice did not actually restore anything. The abort deleted the modify state file without unwinding, so the in-progress rebase or cherry-pick stayed active, branches were left partially rewritten, and — because the state file was gone —gh stack modify --continueno longer worked either. The stack was stuck in a broken, unrecoverable state.This PR makes
--abortfrom a conflict do what it promises, and fixes two related recovery gaps found along the way.Root cause
runModifyAbortswitched on the saved modify phase but had no case for theconflictphase. It handledapplying(unwind) andpending_submit(guide to submit), and everything else — includingconflict— fell into thedefaultbranch, which printed "unexpected modify state phase" and deleted the state file without restoring the stack.What this changes
runModifyAborttreats theconflictphase the same asapplying: it aborts the in-progress git operation, resets every branch tip back to its pre-modify SHA, restores the stack metadata, and clears the state file.git cherry-pick --quit(what the abort previously ran) only clears the sequencer state and leaves the index unmerged; a fullgit cherry-pick --abortis required to get back to a clean tree.cherry_picktorebase, so the next--continueresumes the rebase instead of failing while trying to continue a cherry-pick that is no longer in progress.To support the above, the git layer gains
IsCherryPickInProgress()and splits the old cherry-pick reset intoCherryPickQuit()(--quit, used to clear stale state before starting a fold) andCherryPickAbort()(--abort, used for recovery).Not affected
gh stack rebase --abort,gh stack sync, andgh stack submitwere checked and do not share this problem. Rebase restores refs on abort, sync auto-aborts and restores on conflict because it is non-interactive, and submit does not rebase or cherry-pick.Testing
go vet ./...is clean and the fullgo test -race -count=1 ./...suite passes.Unwindaborts an active cherry-pick, and the cherry-pick to rebase conflict-type transition.--abortrestore, and the fact that--quitintentionally leaves the index unmerged.Stack created with GitHub Stacks CLI • Give Feedback 💬