diff --git a/CLAUDE.md b/CLAUDE.md index e627247..09a364f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -92,8 +92,9 @@ does on one that was aborted. So a caller must also put `saved_staged_patch` back on the paths `apply_abort()` never sees — the `RebaseOutcome::Completed` arm and its `after_continue` handler — through `git::restore_staged_after_rebase`, which applies three-way and never fails its -caller. `weave::run_rebase_or_abort` does it for its own callers (Specs 004 and -014). +caller, and on that arm goes before `transaction::delete` so a failed delete +leaves the index as the user had it — with one exception (Spec 014). +`weave::run_rebase_or_abort` does it for its own callers (Specs 004 and 014). A `weave::run_rebase_protecting` caller that can pause must also record its protected commits in `LoomState.protect`, as full object names; `loom continue` diff --git a/specs/014-continue-abort.md b/specs/014-continue-abort.md index a8d3df9..745c58a 100644 --- a/specs/014-continue-abort.md +++ b/specs/014-continue-abort.md @@ -481,14 +481,17 @@ entry either way.) A command that saves `saved_staged_patch` MUST therefore restore it on every exit: the `RebaseOutcome::Completed` arm, its `after_continue` handler, and the -abort. The restore applies three-way, never resetting the index first: the patch -is a HEAD-to-index diff, and a plain apply would refuse the whole of it over -either a staged *new* file whose entry survived the autostash or a hunk whose -context the rebase rewrote. Resetting to make it apply does not help and can -lose work: on the success path HEAD has moved, so the reset lands the index on -the *new* HEAD while the patch is against the old one — and it has already -dropped what the autostash preserved, which the failing apply then cannot put -back. +abort. On that arm the restore MUST precede `transaction::delete`, so a delete +that fails still leaves the index as the user had it; the exception is an arm +whose own work reads the index to undo itself, which fold's uncommit does, and +there the restore follows that work. The restore applies three-way, never +resetting the index first: the patch is a HEAD-to-index diff, and a plain apply +would refuse the whole of it over either a staged *new* file whose entry +survived the autostash or a hunk whose context the rebase rewrote. Resetting to +make it apply does not help and can lose work: on the success path HEAD has +moved, so the reset lands the index on the *new* HEAD while the patch is +against the old one — and it has already dropped what the autostash preserved, +which the failing apply then cannot put back. The restore is best-effort and MUST NOT fail its caller: it runs after that command's own rewrite has landed, so an error here would report a rewrite that diff --git a/src/absorb.rs b/src/absorb.rs index 62201f9..a6fdbd5 100644 --- a/src/absorb.rs +++ b/src/absorb.rs @@ -302,10 +302,10 @@ fn apply_plan(repo: &Repository, workdir: &Path, git_dir: &Path, plan: AbsorbPla .map_err(|e| transaction::roll_back_failed_rebase(workdir, git_dir, &state, e))?; match outcome { RebaseOutcome::Completed => { + git::restore_staged_after_rebase(workdir, &saved_staged); transaction::delete(git_dir)?; post_absorb( workdir, - &saved_staged, skipped_patch.as_deref(), plan.num_hunks, plan.num_files, @@ -422,9 +422,9 @@ pub fn after_continue( ) -> Result<()> { let ctx: AbsorbContext = serde_json::from_value(context.clone()).context("Failed to parse absorb resume context")?; + git::restore_staged_after_rebase(workdir, &rollback.saved_staged_patch); post_absorb( workdir, - &rollback.saved_staged_patch, ctx.skipped_patch.as_deref(), ctx.num_hunks, ctx.num_files, @@ -432,17 +432,16 @@ pub fn after_continue( ) } -/// Post-rebase work: restore staged/skipped patches and print success message. +/// Post-rebase work: re-apply the skipped patch and print the success message. +/// The staged patch goes back before the state file does, so both callers +/// restore it themselves. fn post_absorb( workdir: &Path, - saved_staged: &str, skipped_patch: Option<&str>, num_hunks: usize, num_files: usize, num_commits: usize, ) -> Result<()> { - git::restore_staged_after_rebase(workdir, saved_staged); - if let Some(patch) = skipped_patch && let Err(e) = git::apply_patch(workdir, patch) { diff --git a/src/commit.rs b/src/commit.rs index f0f3ff8..ef11598 100644 --- a/src/commit.rs +++ b/src/commit.rs @@ -188,8 +188,8 @@ pub fn run( protect: vec![head_oid.to_string()], }; transaction::save(&git_dir, &state)?; - // The state file owns the patch from here: `post_commit` puts it back on - // success, `Rollback` on abort. + // The state file owns the patch from here: the `Completed` arm puts it + // back on success, `Rollback` on abort. let saved_staged = staged_aside.release(); let base = graph.base_oid.to_string(); @@ -200,8 +200,9 @@ pub fn run( .map_err(|e| transaction::roll_back_failed_rebase(&workdir, &git_dir, &state, e))?; match outcome { RebaseOutcome::Completed => { + git::restore_staged_after_rebase(&workdir, &saved_staged); transaction::delete(&git_dir)?; - post_commit(&workdir, &branch_name, &saved_staged)?; + post_commit(&workdir, &branch_name)?; } RebaseOutcome::Stopped => { transaction::warn_paused(&workdir, "commit"); @@ -225,13 +226,13 @@ pub fn after_continue( let saved_staged = ctx .saved_staged .unwrap_or_else(|| rollback.saved_staged_patch.clone()); - post_commit(workdir, &ctx.branch_name, &saved_staged) + git::restore_staged_after_rebase(workdir, &saved_staged); + post_commit(workdir, &ctx.branch_name) } -/// Post-rebase work: restore staged changes and print success message. -fn post_commit(workdir: &Path, branch_name: &str, saved_staged: &str) -> Result<()> { - git::restore_staged_after_rebase(workdir, saved_staged); - +/// Post-rebase work: print the success message. The restore runs before the +/// state file goes, so both callers do it themselves. +fn post_commit(workdir: &Path, branch_name: &str) -> Result<()> { let new_hash = git::rev_parse(workdir, branch_name)?; msg::success(&format!( diff --git a/src/fold.rs b/src/fold.rs index 3999bb8..167319f 100644 --- a/src/fold.rs +++ b/src/fold.rs @@ -2575,6 +2575,10 @@ fn fold_commit_to_unstaged(repo: &Repository, commit_hash: &str) -> Result<()> { })?; let staged = match outcome { RebaseOutcome::Completed => { + // Not restore-then-delete as elsewhere (Spec 014): the apply + // below undoes itself by diffing the working tree against the + // index, so staged work put back first would change what it + // reads as its own. transaction::delete(&git_dir)?; if !diff.is_empty() && let Err(e) = git::apply_patch_to_worktree(workdir, &diff)