Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
17 changes: 14 additions & 3 deletions specs/014-continue-abort.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
52 changes: 52 additions & 0 deletions src/absorb_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
);
}
64 changes: 64 additions & 0 deletions src/commit_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
);
}
53 changes: 49 additions & 4 deletions src/core/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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}"));
Expand Down Expand Up @@ -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() {
Expand Down
Loading