diff --git a/CLAUDE.md b/CLAUDE.md index f937d1b..b6c1440 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,6 +76,13 @@ Every new resumable `weave::run_rebase` caller must populate `Rollback` before saving `LoomState` and register in `transaction::dispatch_after_continue`. There is no abort dispatcher; `Rollback::apply_abort()` owns cleanup. +`roll_back_failed_rebase` skips that cleanup when the rebase never started, +because nothing was autostashed — unless `reset_mixed_to` or `reset_hard_to` is +set, which marks a caller that moved HEAD before the rebase existed (`commit`, +`absorb`) and so has its own work to take back. A caller that unstages before +its rebase without moving HEAD keeps a `staging::StagedAside` guard instead +(Spec 014). + Every rebase autostashes, and that replay reaches the working tree only: a staged modification comes back unstaged on a rebase that completed just as it does on one that was aborted. So a caller must also put `saved_staged_patch` diff --git a/specs/014-continue-abort.md b/specs/014-continue-abort.md index d60ac68..a8d3df9 100644 --- a/specs/014-continue-abort.md +++ b/specs/014-continue-abort.md @@ -253,6 +253,16 @@ still in progress — a failed abort, or the refusal above declining to reset ov uncommitted work: the rollback would make that worse, so both stay for `loom abort`. +A refusal that never started the rebase is the other exception, and only in +part. Nothing was autostashed, so a command that moved nothing before it takes +back only the refs it made: the index and working tree are the user's, and +replaying a saved patch over them would double their staging. One that did move +something first MUST apply the whole rollback — its commits are already in +history and the state file recording the undo is deleted here. A recorded +`reset_mixed_to` or `reset_hard_to` is what tells the two apart. A command that +unstages before its rebase without moving HEAD MUST restore that itself +(Specs 006 and 007). + The same rule holds outside `loom abort`. Wherever a command aborts its own rebase and then cleans up after itself — deleting a temp branch, resetting refs, restoring a saved patch, removing the state file — the cleanup is skipped @@ -494,9 +504,10 @@ and the patch's context is untouched. So may one restoring after reset makes the index exactly what the patch expects, at the cost of dropping whatever the abort left staged outside it. -Two exceptions: a refusal that never started the rebase autostashed nothing, and -an abort that failed leaves the rebase on disk — neither index is loom's to -touch. An unmerged index is left alone for the same reason: the autostash replay +Two exceptions: a refusal that never started the rebase autostashed nothing, so +unless its rollback records a reset target that index is the user's, and an +abort that failed leaves the rebase on disk — neither index is loom's to touch. +An unmerged index is left alone for the same reason: the autostash replay conflicted, so those stages are the user's to resolve and git kept the stash. ### Rollback Restores Pre-Existing State diff --git a/src/absorb_test.rs b/src/absorb_test.rs index 824eaf8..687119e 100644 --- a/src/absorb_test.rs +++ b/src/absorb_test.rs @@ -466,3 +466,55 @@ fn absorb_abort_preserves_working_state() { ); assert_eq!(test_repo.read_file("new-file.txt"), "new-content"); } + +/// Absorb makes its `fixup!` commits before the rebase, so a rebase that +/// refuses to start — a branch it would move is checked out in another +/// worktree — still has them to take back, along with the working tree they +/// consumed. The state file holding that undo goes here, so nothing later can. +#[test] +fn absorb_rolls_back_when_the_rebase_refuses_to_start() { + let test_repo = TestRepo::new_with_remote(); + let workdir = test_repo.workdir(); + let base = test_repo + .find_remote_branch_target("origin/main") + .to_string(); + + test_repo.create_branch_at("feature", &base); + test_repo.switch_branch("feature"); + test_repo.commit("A1", "a1.txt"); + test_repo.switch_branch("integration"); + test_repo.merge_no_ff("feature"); + + let wt = workdir.parent().unwrap().join("wt"); + crate::git::run_git( + &workdir, + &["worktree", "add", wt.to_str().unwrap(), "feature"], + ) + .unwrap(); + + let head_before = test_repo.head_oid(); + test_repo.write_file( + "a1.txt", + "the change to absorb +", + ); + let worktree_before = test_repo.read_file("a1.txt"); + + let result = test_repo.in_dir(|| super::run(false, vec![])); + + assert!(result.is_err(), "the rebase cannot start, so absorb fails"); + assert_eq!( + test_repo.head_oid(), + head_before, + "the `fixup!` commits must be gone" + ); + assert_eq!( + test_repo.read_file("a1.txt"), + worktree_before, + "the change absorb took out of the working tree comes back" + ); + assert!( + !test_repo.repo.path().join("loom/state.json").exists(), + "the state file goes with the rollback" + ); +} diff --git a/src/commit_test.rs b/src/commit_test.rs index bb3e007..1e5beaf 100644 --- a/src/commit_test.rs +++ b/src/commit_test.rs @@ -1301,3 +1301,67 @@ fn commit_dry_run_does_not_amend_head() { assert_eq!(test_repo.head_oid(), head); assert!(!full_message(&test_repo, "HEAD").contains("Change-Id")); } + +/// A rebase that refuses to start — the target branch is checked out in another +/// worktree — still has `commit`'s own commit to take back: it was made before +/// the rebase existed, and the state file that holds the undo is deleted here, +/// so nothing later can. Guards the commit being stranded on integration with +/// the user's staging left behind it. +#[test] +fn commit_rolls_back_when_the_rebase_refuses_to_start() { + let test_repo = setup_with_two_branches(); + let workdir = test_repo.workdir(); + + let wt = workdir.parent().unwrap().join("wt"); + crate::git::run_git( + &workdir, + &["worktree", "add", wt.to_str().unwrap(), "feature-a"], + ) + .unwrap(); + + let head_before = test_repo.head_oid(); + test_repo.write_file( + "other.txt", + "staged by the user +", + ); + test_repo.stage_files(&["other.txt"]); + let staged_before = crate::git::diff_cached(&workdir).unwrap(); + test_repo.write_file( + "new.txt", + "for the commit +", + ); + + let result = test_repo.in_dir(|| { + run( + Some("feature-a".to_string()), + Some("Add new file".to_string()), + vec!["new.txt".to_string()], + ) + }); + + assert!( + result.is_err(), + "the rebase cannot start, so the commit fails" + ); + assert_eq!( + test_repo.head_oid(), + head_before, + "the commit must not be left on integration" + ); + assert_eq!( + crate::git::diff_cached(&workdir).unwrap(), + staged_before, + "the user's own staging comes back exactly as it was" + ); + assert_eq!( + test_repo.read_file("new.txt"), + "for the commit\n", + "the content loom committed comes back to the working tree" + ); + assert!( + !test_repo.repo.path().join("loom/state.json").exists(), + "the state file goes with the rollback" + ); +} diff --git a/src/core/transaction.rs b/src/core/transaction.rs index 7c5444d..17aa695 100644 --- a/src/core/transaction.rs +++ b/src/core/transaction.rs @@ -47,6 +47,13 @@ pub struct Rollback { } impl Rollback { + /// Whether this undo takes HEAD back, which only a caller that moved it + /// itself, before the rebase, records. A caller that unstages before the + /// rebase without moving HEAD must keep its own guard instead (Spec 014). + pub fn takes_head_back(&self) -> bool { + !self.reset_mixed_to.is_empty() || !self.reset_hard_to.is_empty() + } + /// Remove the refs the operation created, and nothing else. /// /// For a failure that rewrote nothing: the temp branches are still loom's @@ -432,10 +439,13 @@ pub fn roll_back_failed_rebase( ); return cause; } - // A pre-flight refusal never autostashed, so that index was never loom's: - // replaying a saved patch over it would put back staging the user still - // has. The refs loom made are still its own to take back. - if git::rebase_never_started(&cause) { + // A pre-flight refusal only has something to undo where loom moved HEAD + // itself before the rebase existed — `commit`'s commit, `absorb`'s fixups — + // which is what a recorded reset target means. With none, nothing was + // autostashed and the index is still the user's: replaying a saved patch + // over it would put back staging they never lost. The refs loom made are + // its own to take back either way. + if git::rebase_never_started(&cause) && !state.rollback.takes_head_back() { state.rollback.delete_temp_branches(workdir); if let Err(e) = delete(git_dir) { crate::core::msg::warn(&format!("could not remove the loom state file: {e}")); @@ -844,6 +854,41 @@ mod tests { git::rebase_abort(&workdir).unwrap(); } + /// The other half of the rule: with no reset recorded, loom moved nothing + /// before the rebase, so the index is still the user's and replaying the + /// saved patch over it would double their staging. + #[test] + fn a_pre_flight_refusal_leaves_an_untouched_index_alone() { + let t = crate::core::test_helpers::TestRepo::new(); + let workdir = t.workdir(); + let git_dir = t.repo.path().to_path_buf(); + t.write_file( + "staged.txt", + "the user's own +", + ); + t.stage_files(&["staged.txt"]); + let staged = git::diff_cached(&workdir).unwrap(); + + let state = LoomState { + command: "fold".to_string(), + rollback: Rollback { + saved_staged_patch: staged.clone(), + ..Default::default() + }, + context: serde_json::Value::Null, + protect: Vec::new(), + }; + save(&git_dir, &state).unwrap(); + + let cause = git::before_rebase_starts::<()>(Err(anyhow::anyhow!("checked out elsewhere"))) + .expect_err("tagged as raised before the rebase started"); + roll_back_failed_rebase(&workdir, &git_dir, &state, cause); + + assert_eq!(git::diff_cached(&workdir).unwrap(), staged); + assert!(!state_path(&git_dir).exists()); + } + /// A half-applied undo is `loom abort`'s to finish, so the state stays. #[test] fn a_failed_rollback_keeps_the_state_too() {