fix(staging): never drop the staging set aside for a rewrite - #294
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe change adds ChangesStaged change recovery
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: 🟡 Moderate · up to Rare fold recovery failures can discard staged work or leave the repository in an unsafe rebase state; these paths should be corrected before merging. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
|
major: a failed abort still drops the set-aside index, with nothing parked The guard is right, but three exits hand it to a rollback that may never run:
rebase_abort_then_cleanup skips its closure when git rebase --abort fails — so rollback_fold, the new owner of the patch, never runs, the guard is already disarmed, and these -p forms write no Not a regression — main is the same — but this is the PR that makes ownership explicit, and the guard already knows the right answer: restore_loom_unstaged parks when a rebase is still on disk, |
Commands that rewrite history unstage the files they are not about, so those cannot join the commit — and every `?` before the state file that takes them over was a way to lose them: a hunk picker that failed rather than cancelled, `stage_files` on the non-patch path, `is_branch_at_merge_base`, and the exits after the commit is made, where no state file exists yet for `loom abort` to read. `fold` had the same holes, in both its own staging paths. `core::staging::StagedAside` restores on drop, and the two helpers that set work aside return it, so a new exit cannot forget. It is handed on where something else owns the restore: a state file, or a worktree snapshot taken before the unstaging. A guard dropping cannot see the error that ended the call, so `restore_loom_unstaged_after_abort` loses that argument and becomes `restore_loom_unstaged`: a rebase left on disk by a failed abort is read from the git dir instead. `restore_or_park_after_abort` goes through it too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Change-Id: I3e4568dcccae36b4a3b51059044d2cddd129a1cb
de0670c to
6502c32
Compare
Commands that rewrite history unstage the files they are not about, so those
cannot join the commit — and every
?before the state file that takes themover was a way to lose them: a hunk picker that failed rather than cancelled,
stage_fileson the non-patch path,is_branch_at_merge_base, and the exitsafter the commit is made, where no state file exists yet for
loom aborttoread.
foldhad the same holes, in both its own staging paths.core::staging::StagedAsiderestores on drop, and the two helpers that setwork aside return it, so a new exit cannot forget. It is handed on where
something else owns the restore: a state file, or a worktree snapshot taken
before the unstaging.
A guard dropping cannot see the error that ended the call, so
restore_loom_unstaged_after_abortloses that argument and becomesrestore_loom_unstaged: a rebase left on disk by a failed abort is read fromthe git dir instead.
restore_or_park_after_abortgoes through it too.Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Change-Id: I3e4568dcccae36b4a3b51059044d2cddd129a1cb
Summary by CodeRabbit