Skip to content

fix(staging): never drop the staging set aside for a rewrite - #294

Merged
narnaud merged 1 commit into
mainfrom
fix-patch-restore
Sep 22, 2026
Merged

narnaud merged 1 commit into
mainfrom
fix-patch-restore

Conversation

@narnaud

@narnaud narnaud commented Sep 21, 2026

Copy link
Copy Markdown
Owner

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

Summary by CodeRabbit

  • Bug Fixes
    • Preserved staged files when commit, fold, split, or rebase operations fail or are cancelled.
    • Restored staged changes reliably after successful and unsuccessful fold operations.
    • Improved recovery when failures occur after a commit is created but before operation state is saved.
    • Prevented staged work from being lost during interrupted or paused operations.
    • Safely preserved staged changes when recovery cannot complete immediately, allowing them to be restored later.

@coderabbitai

coderabbitai Bot commented Sep 21, 2026

Copy link
Copy Markdown

Review in Change Stack →

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 configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 09308e06-284f-4772-8c69-ce46feb15334

📥 Commits

Reviewing files that changed from the base of the PR and between de0670c and 6502c32.

📒 Files selected for processing (8)
  • CLAUDE.md
  • specs/007-fold.md
  • src/core/staging.rs
  • src/core/staging_test.rs
  • src/fold.rs
  • src/fold_test.rs
  • src/git/git_apply.rs
  • src/git/git_apply_test.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The change adds StagedAside ownership for saved staged work. Commit, fold, split, and rebase recovery paths now restore or transfer staged changes across success, pause, and failure paths. Regression tests cover failures before and after commit creation.

Changes

Staged change recovery

Layer / File(s) Summary
StagedAside guard contract
src/core/staging.rs, src/core/staging_test.rs, CLAUDE.md
StagedAside owns saved staged patches and restores them on drop unless ownership is released or handed over. Tests cover restoration, release, handoff, and failed cleanup. The guidance defines when handoff is safe.
Commit staging recovery
src/commit.rs, src/commit_test.rs, specs/006-commit.md
Commit flows use StagedAside across staging, commit creation, transaction-state persistence, and failure cleanup. Tests cover failures before state persistence and before commit creation.
Fold and split recovery
src/fold.rs, src/fold_test.rs, src/split.rs, specs/007-fold.md
Fold and split flows restore or transfer staged changes across rebases, pauses, successful completion, and failure cleanup. The fold specification requires restoration on every failure path.
Rebase restoration helper
src/git/git_apply.rs, src/git/git_apply_test.rs, src/git/mod.rs
restore_loom_unstaged restores staged patches when no rebase remains and parks them when rebase state remains. The public re-export and tests use the new helper.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Suggested reviewers: dfaure-kdab

Merge Risk: 🟡 Moderate · up to 6502c

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)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing staged content from being lost during rewrite operations.
Docstring Coverage ✅ Passed Docstring coverage is 92.11% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 10 files. (2 skipped: 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@dfaure-kdab

Copy link
Copy Markdown
Collaborator

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:

  • src/fold.rs, run_patch_fold_commit_to_unstaged non-head branch: staged_aside.handed_over(); immediately before rebase_abort_then_cleanup(workdir, e, || rollback_fold(...)).
  • src/fold.rs, fold_selected_hunks_to_commit: let saved_staged = staged.release();, then two rebase_abort_then_cleanup(workdir, e, rollback) sites.

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
LoomState, so loom abort has nothing to read either. What's lost is index-only content (staged X, worktree then edited to Y); worktree content survives, the staged side does not.

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,
which is exactly this case. Disarming inside the cleanup closure instead of before it (a &self disarm over a Cell, since handed_over consumes) makes a skipped cleanup fall through to the park.

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
@narnaud
narnaud merged commit eb1aeac into main Sep 22, 2026
6 checks passed
@narnaud
narnaud deleted the fix-patch-restore branch September 22, 2026 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants