From 6502c32c6510e2d53de89b639bb7d123ffc7509e Mon Sep 17 00:00:00 2001 From: Nicolas Arnaud-Cormos Date: Sun, 20 Sep 2026 21:14:03 +0200 Subject: [PATCH] fix(staging): never drop the staging set aside for a rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Change-Id: I3e4568dcccae36b4a3b51059044d2cddd129a1cb --- CLAUDE.md | 5 +- specs/006-commit.md | 4 ++ specs/007-fold.md | 4 ++ src/commit.rs | 60 ++++++++++-------------- src/commit_test.rs | 60 ++++++++++++++++++++++++ src/core/staging.rs | 94 +++++++++++++++++++++++++++++++------ src/core/staging_test.rs | 96 +++++++++++++++++++++++++++++++++++++- src/fold.rs | 97 +++++++++++++++++++-------------------- src/fold_test.rs | 77 ++++++++++++++++++++++++++++++- src/git/git_apply.rs | 66 +++++++++++++++----------- src/git/git_apply_test.rs | 42 +++++++++++++---- src/git/mod.rs | 2 +- src/split.rs | 4 +- 13 files changed, 466 insertions(+), 145 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b6c1440f..e6272475 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -81,7 +81,10 @@ because nothing was autostashed — unless `reset_mixed_to` or `reset_hard_to` i 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). +(Spec 014). `StagedAside::handed_over()` may only name an owner that is durable +or has already run: called before a `rebase_abort_then_cleanup` closure it +drops the patch on exactly the path that skips that closure, so it goes inside +one. 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 diff --git a/specs/006-commit.md b/specs/006-commit.md index 08d16814..919075b2 100644 --- a/specs/006-commit.md +++ b/specs/006-commit.md @@ -72,6 +72,10 @@ Create the commit at HEAD, then relocate it to the target via one Weave operatio If relocation conflicts, pause and save `.git/loom/state.json`. The new commit remains recoverable in the working tree through mixed reset; original staged changes MUST be restored on abort. +Staged files set aside so they cannot join this commit MUST come back on every +failure path, including the ones after the commit is created but before +`state.json` exists, where no rollback can return them. + - `loom continue` resolves/completes relocation. - `loom abort` cancels and restores original history and staged state. diff --git a/specs/007-fold.md b/specs/007-fold.md index fa2147a5..ba5170d1 100644 --- a/specs/007-fold.md +++ b/specs/007-fold.md @@ -216,6 +216,10 @@ These operations pause on rebase conflict and save the listed `LoomState.context `loom continue` dispatches `after_continue`, removes `_loom-track`, restores pre-existing staged changes from `LoomState.rollback`, and prints the operation's success message. `loom abort` restores original history, staged state, and working-tree state (Spec 014). The post-continue unapplied-patch exception is defined above. +Staged files set aside so they cannot join the fold MUST come back on every +failure path, including the ones reached before `state.json` exists, where no +rollback can return them. + Multiple moves (to a branch or next to a commit), all `-c` moves, all `-p` forms, and CommitFile move failures save no resumable state and auto-rollback as specified in their sections. ## General invariants and prerequisites diff --git a/src/commit.rs b/src/commit.rs index add1c473..f0f3ff85 100644 --- a/src/commit.rs +++ b/src/commit.rs @@ -9,7 +9,7 @@ use crate::core::changeid; use crate::core::graph; use crate::core::msg; use crate::core::repo; -use crate::core::staging; +use crate::core::staging::{self, StagedAside}; use crate::core::transaction::{self, LoomState, Rollback}; use crate::core::weave::{self, RebaseOutcome, Weave}; use crate::git; @@ -94,17 +94,13 @@ pub fn run( // Stage files, saving aside any pre-existing staged files not in the // target list so they don't accidentally end up in this commit. - let saved_staged = if patch { + let staged_aside = if patch { resolve_staging_patch(&repo, &workdir, &files, theme)? } else { resolve_staging(&repo, &workdir, &files)? }; - // Restore the saved staged work if the index turns out to be empty. - if let Err(e) = repo::verify_has_staged_changes(&repo) { - git::restore_staged_patch(&workdir, &saved_staged); - return Err(e); - } + repo::verify_has_staged_changes(&repo)?; let git_opts: Vec<&str> = git_args.iter().map(String::as_str).collect(); let do_commit = || -> Result<()> { @@ -124,7 +120,9 @@ pub fn run( // "origin/main"). if loose { let result = do_commit(); - git::restore_staged_patch(&workdir, &saved_staged); + // Put back either way: a loose commit writes no state file, so there + // is no later owner for the patch and no rollback to hold it. + staged_aside.restore(); result?; let new_head = repo::head_oid(&repo)?; msg::success(&format!( @@ -139,25 +137,15 @@ pub fn run( // Resolve branch target (may create a new branch at merge-base). // Returns whether the branch was newly created — only newly-created // branches are deleted on rollback (not pre-existing empty ones). - // A cancelled prompt here must put the saved-aside files back in the index. let (branch_name, branch_is_new) = - match resolve_branch_target(&repo, &info, &workdir, branch.as_deref()) { - Ok(resolved) => resolved, - Err(e) => { - git::restore_staged_patch(&workdir, &saved_staged); - return Err(e); - } - }; + resolve_branch_target(&repo, &info, &workdir, branch.as_deref())?; // Empty branches (pointing at merge-base) need a branch section and // merge entry created in the Weave before moving the commit there. let branch_is_empty = is_branch_at_merge_base(&repo, &branch_name, info.upstream.merge_base_oid)?; - if let Err(e) = do_commit() { - git::restore_staged_patch(&workdir, &saved_staged); - return Err(e); - } + do_commit()?; let head_oid = repo::head_oid(&repo)?; @@ -184,7 +172,7 @@ pub fn run( } let ctx = CommitContext { branch_name: branch_name.clone(), - saved_staged: Some(saved_staged.clone()), + saved_staged: Some(staged_aside.patch().to_string()), }; let state = LoomState { command: "commit".to_string(), @@ -200,6 +188,9 @@ 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. + let saved_staged = staged_aside.release(); let base = graph.base_oid.to_string(); @@ -256,14 +247,14 @@ fn post_commit(workdir: &Path, branch_name: &str, saved_staged: &str) -> Result< /// /// With specific files, other staged files are saved aside and unstaged first, /// so they neither show in the picker nor leak into this commit. Returns that -/// saved patch for restoration after the commit; a cancelled picker restores -/// it and errors. -fn resolve_staging_patch( +/// saved patch for restoration after the commit; a picker that is cancelled or +/// fails puts it back and errors. +fn resolve_staging_patch<'a>( repo: &Repository, - workdir: &std::path::Path, + workdir: &'a Path, files: &[String], theme: &graph::Theme, -) -> Result { +) -> Result> { // Save aside other staged files when specific files are targeted. let filter = staging::filter_paths(repo, files)?; let saved_staged = match &filter { @@ -271,15 +262,12 @@ fn resolve_staging_patch( let path_refs: Vec<&str> = paths.iter().map(|s| s.as_str()).collect(); staging::save_and_unstage_other_staged(repo, workdir, &path_refs)? } - None => String::new(), + None => StagedAside::none(workdir), }; - let confirmed = staging::run_hunk_picker(repo, workdir, filter.as_deref(), theme)?; - if confirmed.is_none() { - git::restore_staged_patch(workdir, &saved_staged); + if staging::run_hunk_picker(repo, workdir, filter.as_deref(), theme)?.is_none() { return Err(msg::cancelled()); } - Ok(saved_staged) } @@ -287,18 +275,18 @@ fn resolve_staging_patch( /// as-is, `zz` stages everything, and named files are staged after any other /// pre-existing staged file is saved aside and unstaged so it cannot leak into /// this commit. Returns that saved patch for later restoration. -fn resolve_staging( +fn resolve_staging<'a>( repo: &Repository, - workdir: &std::path::Path, + workdir: &'a Path, files: &[String], -) -> Result { +) -> Result> { if files.is_empty() { - return Ok(String::new()); + return Ok(StagedAside::none(workdir)); } if files.iter().any(|f| f == "zz") { git::stage_all(workdir)?; - return Ok(String::new()); + return Ok(StagedAside::none(workdir)); } let resolved_paths = resolve_file_args(repo, files)?; diff --git a/src/commit_test.rs b/src/commit_test.rs index 1e5beafd..c154e6bb 100644 --- a/src/commit_test.rs +++ b/src/commit_test.rs @@ -177,6 +177,66 @@ fn commit_to_non_woven_branch_fails() { assert!(result.unwrap_err().to_string().contains("not woven")); } +/// The worst window: the commit is made, but the state file that would carry +/// the set-aside work is not written yet, so nothing else can return it — +/// `loom abort` has no rollback to read. A file where `.git/loom` must be a +/// directory is what makes `transaction::save` fail here — which also blocks +/// the last-resort patch dump the restore falls back on, so the one asserted +/// below is the real one, not the fallback. +#[test] +fn a_commit_failing_after_it_is_created_puts_back_the_staging() { + let test_repo = setup_with_woven_branch(); + test_repo.write_file("kept.txt", "staged before the commit"); + test_repo.stage_files(&["kept.txt"]); + test_repo.write_file("file.txt", "content"); + std::fs::write(test_repo.repo.path().join("loom"), "not a directory").unwrap(); + let before = test_repo.head_oid(); + + let result = test_repo.in_dir(|| { + run( + Some("feature-a".to_string()), + Some("Message".to_string()), + vec!["file.txt".to_string()], + ) + }); + + assert!(result.is_err(), "the state file cannot be written"); + // Without this the test still passes if the failure ever moves earlier, + // leaving the window it is named after unguarded. + assert_ne!( + test_repo.head_oid(), + before, + "the commit must already exist" + ); + let status = test_repo.status_porcelain(); + assert!(status.contains("A kept.txt"), "{status}"); +} + +/// A failure between staging and the commit puts the set-aside work back too: +/// which files were staged is state git keeps no second copy of. +#[test] +fn a_failed_commit_puts_back_the_staging_it_set_aside() { + let test_repo = TestRepo::new_with_remote(); + test_repo.commit("A1", "a1.txt"); + test_repo.create_branch_tracking("not-woven", "origin/main"); + + test_repo.write_file("kept.txt", "staged before the commit"); + test_repo.stage_files(&["kept.txt"]); + test_repo.write_file("file.txt", "content"); + + let result = test_repo.in_dir(|| { + run( + Some("not-woven".to_string()), + Some("Message".to_string()), + vec!["file.txt".to_string()], + ) + }); + + assert!(result.is_err()); + let status = test_repo.status_porcelain(); + assert!(status.contains("A kept.txt"), "{status}"); +} + #[test] fn commit_to_new_branch_creates_and_weaves() { let test_repo = TestRepo::new_with_remote(); diff --git a/src/core/staging.rs b/src/core/staging.rs index 8fd21b4f..580b9d58 100644 --- a/src/core/staging.rs +++ b/src/core/staging.rs @@ -1,5 +1,6 @@ use anyhow::{Result, bail}; use git2::Repository; +use std::cell::Cell; use std::path::Path; use crate::core::diff::{self, parse_hunk_start}; @@ -499,31 +500,94 @@ pub(crate) fn collect_commit_hunks( Ok(entries) } -/// Save and unstage all currently staged changes, returning a patch to restore them later. +/// Staged work an operation set aside, put back into the index when this +/// drops — every `?` in between included, which is what keeps it (Spec 006). /// -/// Returns an empty string if nothing is staged. Callers must call -/// `git::restore_staged_patch` with the returned patch when the operation -/// completes (or is rolled back), so pre-existing staged work is never lost. -pub(crate) fn save_and_unstage_staged(repo: &Repository, workdir: &Path) -> Result { +/// Hand it on where someone else owns the restore: a state file, or a worktree +/// snapshot taken before the unstaging. That owner has to be one this guard +/// cannot outlive uncollected — see [`StagedAside::handed_over`]. +#[must_use = "dropping the guard right away puts the work straight back, undoing the unstage"] +pub(crate) struct StagedAside<'a> { + workdir: &'a Path, + patch: String, + armed: Cell, +} + +impl<'a> StagedAside<'a> { + fn new(workdir: &'a Path, patch: String) -> Self { + StagedAside { + workdir, + patch, + armed: Cell::new(true), + } + } + + /// A guard over nothing: the operation set no staged work aside. + pub(crate) fn none(workdir: &'a Path) -> Self { + StagedAside::new(workdir, String::new()) + } + + pub(crate) fn patch(&self) -> &str { + &self.patch + } + + /// Put it back now, rather than wherever this would have dropped. Does + /// nothing once [`StagedAside::handed_over`] has run. + pub(crate) fn restore(self) {} + + /// Take the patch back: the caller restores it from here. + #[must_use = "the set-aside patch is lost unless a new owner keeps it"] + pub(crate) fn release(mut self) -> String { + self.armed.set(false); + std::mem::take(&mut self.patch) + } + + /// Something else holds the patch now, so this must not put it back a + /// second time. + /// + /// Data safety: that owner must be durable (a state file `loom abort` + /// reads) or already have run. A rollback closure is neither until it + /// runs, and [`git::rebase_abort_then_cleanup`] skips its closure when the + /// abort fails — so call this from inside such a closure, never before it. + /// Disarming early there drops the patch on the one path that cannot get + /// it back; leaving the guard armed parks it instead. + pub(crate) fn handed_over(&self) { + self.armed.set(false); + } +} + +impl Drop for StagedAside<'_> { + fn drop(&mut self) { + if self.armed.get() { + git::restore_loom_unstaged(self.workdir, &self.patch); + } + } +} + +/// Save and unstage all currently staged changes, so an amend or rebase below +/// leaves them out. Restored when the returned guard drops. +pub(crate) fn save_and_unstage_staged<'a>( + repo: &Repository, + workdir: &'a Path, +) -> Result> { let staged = repo::get_staged_files(repo)?; if staged.is_empty() { - return Ok(String::new()); + return Ok(StagedAside::new(workdir, String::new())); } let refs: Vec<&str> = staged.iter().map(|s| s.as_str()).collect(); let patch = git::diff_cached_files(workdir, &refs)?; git::unstage_files(workdir, &refs)?; - Ok(patch) + Ok(StagedAside::new(workdir, patch)) } /// Save the staged diff for files that are staged but NOT in `target_files`, -/// then unstage them so they don't leak into the upcoming commit. -/// -/// Returns the patch as a string (may be empty if nothing to save). -pub(crate) fn save_and_unstage_other_staged( +/// then unstage them so they don't leak into the upcoming commit. Restored +/// when the returned guard drops. +pub(crate) fn save_and_unstage_other_staged<'a>( repo: &Repository, - workdir: &Path, + workdir: &'a Path, target_files: &[&str], -) -> Result { +) -> Result> { let staged = repo::get_staged_files(repo)?; let other: Vec<&str> = staged .iter() @@ -531,11 +595,11 @@ pub(crate) fn save_and_unstage_other_staged( .map(|s| s.as_str()) .collect(); if other.is_empty() { - return Ok(String::new()); + return Ok(StagedAside::new(workdir, String::new())); } let patch = git::diff_cached_files(workdir, &other)?; git::unstage_files(workdir, &other)?; - Ok(patch) + Ok(StagedAside::new(workdir, patch)) } /// The single entry a submodule contributes to a picker: one object id, with no diff --git a/src/core/staging_test.rs b/src/core/staging_test.rs index 8b409a97..8406026f 100644 --- a/src/core/staging_test.rs +++ b/src/core/staging_test.rs @@ -1,4 +1,4 @@ -use super::{filter_paths, selected_paths}; +use super::{filter_paths, save_and_unstage_staged, selected_paths}; use crate::core::diff::DiffHunk; use crate::core::repo; use crate::core::test_helpers::TestRepo; @@ -155,3 +155,97 @@ fn only_the_picked_files_are_folded_and_order_is_kept() { vec!["a.rs".to_string(), "c.rs".to_string()] ); } + +/// The guard is what every `?` between the unstaging and the end of an +/// operation relies on, so its two states are pinned here rather than only +/// through the commands that hold it. +#[test] +fn the_set_aside_staging_comes_back_when_the_guard_drops() { + let test_repo = TestRepo::new(); + test_repo.commit("A1", "a1.txt"); + test_repo.write_file("kept.txt", "staged"); + test_repo.stage_files(&["kept.txt"]); + + let workdir = test_repo.repo.workdir().unwrap().to_path_buf(); + { + let guard = save_and_unstage_staged(&test_repo.repo, &workdir).unwrap(); + assert!(!guard.patch().is_empty()); + let status = test_repo.status_porcelain(); + assert!(!status.contains("A kept.txt"), "{status}"); + } + + let status = test_repo.status_porcelain(); + assert!(status.contains("A kept.txt"), "{status}"); +} + +#[test] +fn a_released_guard_leaves_the_index_to_its_new_owner() { + let test_repo = TestRepo::new(); + test_repo.commit("A1", "a1.txt"); + test_repo.write_file("kept.txt", "staged"); + test_repo.stage_files(&["kept.txt"]); + + let workdir = test_repo.repo.workdir().unwrap().to_path_buf(); + let patch = { + let guard = save_and_unstage_staged(&test_repo.repo, &workdir).unwrap(); + guard.release() + }; + + let status = test_repo.status_porcelain(); + assert!(!status.contains("A kept.txt"), "{status}"); + assert!(patch.contains("kept.txt"), "{patch}"); +} + +#[test] +fn a_handed_over_guard_leaves_the_index_to_its_new_owner() { + let test_repo = TestRepo::new(); + test_repo.commit("A1", "a1.txt"); + test_repo.write_file("kept.txt", "staged"); + test_repo.stage_files(&["kept.txt"]); + + let workdir = test_repo.repo.workdir().unwrap().to_path_buf(); + save_and_unstage_staged(&test_repo.repo, &workdir) + .unwrap() + .handed_over(); + + let status = test_repo.status_porcelain(); + assert!(!status.contains("A kept.txt"), "{status}"); +} + +/// The handover a failed abort never reaches. `rebase_abort_then_cleanup` +/// skips its closure when the abort fails, so disarming before the call +/// dropped the patch on the one path that cannot get it back: the `-p` folds +/// save no `LoomState` for `loom abort` to read it out of, and the snapshot +/// that owns it only replays from inside that closure. +#[test] +fn a_handover_the_cleanup_never_reached_parks_the_patch() { + let test_repo = TestRepo::new(); + test_repo.commit("A1", "a1.txt"); + test_repo.write_file("kept.txt", "staged"); + test_repo.stage_files(&["kept.txt"]); + + let workdir = test_repo.repo.workdir().unwrap().to_path_buf(); + let guard = save_and_unstage_staged(&test_repo.repo, &workdir).unwrap(); + // A rebase dir with no rebase under it: the abort git tries here fails. + std::fs::create_dir_all(test_repo.repo.path().join("rebase-merge")).unwrap(); + + let err = + crate::git::rebase_abort_then_cleanup(&workdir, anyhow::anyhow!("the fold failed"), || { + guard.handed_over() + }); + assert!(err.to_string().contains("The abort failed"), "{err}"); + + drop(guard); + + let parked = crate::git::git_path(&workdir, "loom").unwrap(); + assert!( + std::fs::read_dir(&parked) + .expect("the patch is parked under the git dir") + .filter_map(|e| e.ok()) + .any(|e| e + .file_name() + .to_string_lossy() + .starts_with("unrestored-staged")), + "a cleanup that never ran leaves the guard to hand the patch over" + ); +} diff --git a/src/fold.rs b/src/fold.rs index 3fcded5f..3999bb80 100644 --- a/src/fold.rs +++ b/src/fold.rs @@ -1065,7 +1065,7 @@ fn fold_selected_hunks_to_commit( let saved_worktree = WorktreeSnapshot::take(workdir)?; // Unstage pre-existing staged changes so the amends below leave them out. - let saved_staged = staging::save_and_unstage_staged(repo, workdir)?; + let staged = staging::save_and_unstage_staged(repo, workdir)?; // Phase 1: edit source, remove selected hunks. let mut graph = Weave::from_repo(repo)?; @@ -1081,7 +1081,6 @@ fn fold_selected_hunks_to_commit( &[target_hash], ) { let _ = git::branch_delete(workdir, TRACK_BRANCH); - git::restore_loom_unstaged_after_abort(workdir, &saved_staged, &e); return Err(e); } @@ -1093,22 +1092,16 @@ fn fold_selected_hunks_to_commit( true, git_opts, ) { - // Outside the cleanup closure: a failed abort skips it, and there is no - // `LoomState` yet for `loom abort` to find the patch in. - let e = git::rebase_abort_then_cleanup(workdir, e, || { + return Err(git::rebase_abort_then_cleanup(workdir, e, || { let _ = git::branch_delete(workdir, TRACK_BRANCH); - }); - git::restore_loom_unstaged_after_abort(workdir, &saved_staged, &e); - return Err(e); + })); } let phase1_source_hash = git::rev_parse(workdir, "HEAD").map_err(|e| { // Still inside the paused rebase, so the abort comes first. - let e = git::rebase_abort_then_cleanup(workdir, e, || { + git::rebase_abort_then_cleanup(workdir, e, || { let _ = git::branch_delete(workdir, TRACK_BRANCH); - }); - git::restore_loom_unstaged_after_abort(workdir, &saved_staged, &e); - e + }) })?; // The source replays during this continue; dropping it as empty would @@ -1118,15 +1111,16 @@ fn fold_selected_hunks_to_commit( git::continue_rebase_expecting_edit(workdir, git::AfterStop::nothing().protecting(&protect)) { let _ = git::branch_delete(workdir, TRACK_BRANCH); - git::restore_loom_unstaged_after_abort(workdir, &saved_staged, &e); return Err(e); } // Phase 1 is already committed, so undoing anything from here means // resetting over a working tree its rebase has restored. The snapshot - // predates `save_and_unstage_staged`, so it puts `saved_staged` back - // along with it. + // predates the unstaging, so a rollback puts the set-aside work back with + // the rest — but only once it runs, and a failed abort skips it, so the + // handover is inside the closure rather than before it. let rollback = || { + staged.handed_over(); let _ = git::branch_delete(workdir, TRACK_BRANCH); rollback_fold(workdir, &saved_head, Some(&saved_refs), &saved_worktree); }; @@ -1196,7 +1190,7 @@ fn fold_selected_hunks_to_commit( // emptied, and it goes back whichever way this ends. let tracked = git::rev_parse(workdir, TRACK_BRANCH); let _ = git::branch_delete(workdir, TRACK_BRANCH); - git::restore_staged_after_rebase(workdir, &saved_staged); + staged.restore(); let new_source_hash = tracked?; Ok((new_source_hash, new_target_hash)) @@ -1235,14 +1229,17 @@ fn run_patch_fold_commit_to_unstaged( let saved_worktree = WorktreeSnapshot::take(workdir)?; // Unstage pre-existing staged changes so the amend below leaves them out. - let saved_staged = staging::save_and_unstage_staged(repo, workdir)?; + // Held armed throughout, so every exit below puts it back; only the + // `rollback_fold` paths hand it over, to the snapshot above, which predates + // the unstaging. + let staged_aside = staging::save_and_unstage_staged(repo, workdir)?; let new_hash; if is_head { let pre_amend_hash = head_oid.to_string(); // Same rollback as below: a failure part-way through leaves the hunks - // reverse-applied in the working tree, and `saved_staged` unstaged. + // reverse-applied in the working tree, and the set-aside work unstaged. if let Err(e) = apply_and_amend( workdir, &selections, @@ -1251,13 +1248,13 @@ fn run_patch_fold_commit_to_unstaged( true, git_opts, ) { + staged_aside.handed_over(); rollback_fold(workdir, &pre_amend_hash, None, &saved_worktree); return Err(e).context("Failed to remove hunks from the commit, operation rolled back"); } new_hash = git::rev_parse(workdir, "HEAD")?; if let Err(e) = restore_to_worktree(workdir, &selected_patch, &whole_files) { - // The snapshot predates `save_and_unstage_staged`, so the rollback - // puts `saved_staged` back along with the rest. + staged_aside.handed_over(); rollback_fold(workdir, &pre_amend_hash, None, &saved_worktree); return Err(e) .context("Failed to restore hunks to working directory, operation rolled back"); @@ -1269,16 +1266,13 @@ fn run_patch_fold_commit_to_unstaged( let mut graph = Weave::from_repo(repo)?; let _ = graph.edit_commit(target_oid); let todo = graph.to_todo(); - if let Err(e) = weave::run_rebase_expecting_edit( + weave::run_rebase_expecting_edit( workdir, Some(&graph.base_oid.to_string()), &todo, target_oid, &[], - ) { - git::restore_loom_unstaged_after_abort(workdir, &saved_staged, &e); - return Err(e); - } + )?; if let Err(e) = apply_and_amend( workdir, @@ -1288,33 +1282,29 @@ fn run_patch_fold_commit_to_unstaged( true, git_opts, ) { - // Outside the cleanup closure: a failed abort skips it, and there is - // no `LoomState` yet for `loom abort` to find the patch in. - let e = git::rebase_abort_then_cleanup(workdir, e, || {}); - git::restore_loom_unstaged_after_abort(workdir, &saved_staged, &e); - return Err(e); + return Err(git::rebase_abort_then_cleanup(workdir, e, || {})); } new_hash = git::rev_parse(workdir, "HEAD").map_err(|e| { // Still inside the paused rebase, so the abort comes first. - let e = git::rebase_abort_then_cleanup(workdir, e, || {}); - git::restore_loom_unstaged_after_abort(workdir, &saved_staged, &e); - e + git::rebase_abort_then_cleanup(workdir, e, || {}) })?; if let Err(e) = git::continue_rebase_expecting_edit(workdir, git::AfterStop::nothing()) { return Err(git::rebase_abort_then_cleanup(workdir, e, || { + staged_aside.handed_over(); rollback_fold(workdir, &saved_head, Some(&saved_refs), &saved_worktree); })); } if let Err(e) = restore_to_worktree(workdir, &selected_patch, &whole_files) { + staged_aside.handed_over(); rollback_fold(workdir, &saved_head, Some(&saved_refs), &saved_worktree); return Err(e) .context("Failed to apply changes to working directory, operation rolled back"); } } - git::restore_staged_after_rebase(workdir, &saved_staged); + staged_aside.restore(); let mut staged: Vec = whole_files .iter() @@ -1569,7 +1559,7 @@ fn fold_files_into_commit( // Unstage pre-existing staged files outside the target list, so they do // not end up in this commit/amend. - let saved_staged = staging::save_and_unstage_other_staged(repo, workdir, &file_refs)?; + let staged = staging::save_and_unstage_other_staged(repo, workdir, &file_refs)?; let new_hash; @@ -1581,10 +1571,10 @@ fn fold_files_into_commit( // An amend that got as far as replacing HEAD and then failed leaves // it on a commit the user never asked for, so this takes HEAD back // too. - undo_commit_attempt(workdir, head_oid, staged_by_loom, &saved_staged); + undo_commit_attempt(workdir, head_oid, staged_by_loom, staged); return Err(e); } - git::restore_staged_patch(workdir, &saved_staged); + staged.restore(); new_hash = git::rev_parse(workdir, "HEAD")?; } else { // Create a fixup commit on HEAD with only the changed files, then @@ -1599,7 +1589,7 @@ fn fold_files_into_commit( git::stage_files(workdir, &file_refs)?; } if let Err(e) = git::commit_captured(workdir, &message, git_opts) { - undo_commit_attempt(workdir, head_oid, staged_by_loom, &saved_staged); + undo_commit_attempt(workdir, head_oid, staged_by_loom, staged); return Err(e); } @@ -1608,7 +1598,7 @@ fn fold_files_into_commit( // would then feed the user's own HEAD commit into the target and lose // it, so check what git actually did before anything is rewritten. if !committed_onto(workdir, head_oid) { - undo_commit_attempt(workdir, head_oid, staged_by_loom, &saved_staged); + undo_commit_attempt(workdir, head_oid, staged_by_loom, staged); let blame = if git_opts.is_empty() { "" } else { @@ -1621,7 +1611,7 @@ fn fold_files_into_commit( // the result through. The squash would then rewrite the target with // nothing in it and report the fold as done. if !git_opts.is_empty() && committed_the_same_tree(workdir, head_oid) { - undo_commit_attempt(workdir, head_oid, staged_by_loom, &saved_staged); + undo_commit_attempt(workdir, head_oid, staged_by_loom, staged); bail!( "`git commit` made an empty `fixup!` commit, so nothing was folded\n\ An argument after `--` kept the staged changes out of it" @@ -1629,28 +1619,34 @@ fn fold_files_into_commit( } // From here the repository carries a commit the user never asked for, - // and their other staged files live only in `saved_staged`. + // and their other staged files live only in the guard. let git_dir = repo.path().to_path_buf(); - match squash_fixup_into_commit( + // Not in the match scrutinee: that would hold the borrow of `staged` + // across arms that consume it. + let outcome = squash_fixup_into_commit( &git_dir, workdir, target_oid, head_oid, files, - &saved_staged, - ) { + staged.patch(), + ); + match outcome { // The rebase is over, so the finishing steps run outside the // rollback: undoing a rewrite that succeeded would leave the // integration branch behind its own feature branches. Ok(FixupOutcome::Rebased) => { - git::restore_staged_after_rebase(workdir, &saved_staged); + staged.restore(); transaction::delete(&git_dir)?; new_hash = git::rev_parse(workdir, TRACK_BRANCH)?; let _ = git::branch_delete(workdir, TRACK_BRANCH); } // `loom continue` and `loom abort` own the rest, through the state - // file the rebase left behind. - Ok(FixupOutcome::Paused) => return Ok(()), + // file the rebase left behind — the patch is in it. + Ok(FixupOutcome::Paused) => { + staged.handed_over(); + return Ok(()); + } Err(e) => { return Err(git::rebase_abort_then_cleanup(workdir, e, || { // Take the commit back first: the saved patch was made @@ -1661,7 +1657,6 @@ fn fold_files_into_commit( // had these files modified, not staged. let _ = git::unstage_files(workdir, &file_refs); } - git::restore_staged_patch(workdir, &saved_staged); let _ = git::branch_delete(workdir, TRACK_BRANCH); let _ = transaction::delete(&git_dir); })); @@ -1707,7 +1702,7 @@ fn undo_commit_attempt( workdir: &Path, head_oid: git2::Oid, staged_by_loom: &[&str], - saved_staged: &str, + staged: staging::StagedAside<'_>, ) { // A `--amend` moved HEAD instead of adding to it; the reset puts the // commit back and leaves what it held staged, for the two steps below. A @@ -1724,7 +1719,7 @@ fn undo_commit_attempt( git::save_or_warn( workdir, "unrestored-staged", - saved_staged, + &staged.release(), git::Replay::Cached, ); return; @@ -1732,7 +1727,7 @@ fn undo_commit_attempt( if !staged_by_loom.is_empty() { let _ = git::unstage_files(workdir, staged_by_loom); } - git::restore_staged_patch(workdir, saved_staged); + staged.restore(); } /// How far [`squash_fixup_into_commit`] got. diff --git a/src/fold_test.rs b/src/fold_test.rs index 7b0c0f1d..9907403f 100644 --- a/src/fold_test.rs +++ b/src/fold_test.rs @@ -304,6 +304,81 @@ fn fold_patch_only_staged_hunk_is_folded_into_non_head() { ); } +/// Files staged outside the fold are set aside so they cannot join it, and +/// must be staged again afterwards — here across the fixup path, where a whole +/// rebase runs in between. +#[test] +fn fold_into_a_non_head_commit_stages_the_other_files_again() { + let test_repo = TestRepo::new_with_remote(); + test_repo.write_file("file.txt", "target\n"); + test_repo.stage_files(&["file.txt"]); + test_repo.commit_staged("target commit"); + let target_oid = test_repo.head_oid(); + + test_repo.write_file("later.txt", "later\n"); + test_repo.stage_files(&["later.txt"]); + test_repo.commit_staged("second commit"); + + test_repo.write_file("file.txt", "target amended\n"); + test_repo.write_file("kept.txt", "staged, and none of the fold's business\n"); + test_repo.stage_files(&["file.txt", "kept.txt"]); + + let result = super::fold_files_into_commit( + &test_repo.repo, + &["file.txt".to_string()], + &target_oid.to_string(), + true, + &[], + ); + assert!(result.is_ok(), "fold failed: {result:?}"); + + let status = test_repo.status_porcelain(); + assert!(status.contains("A kept.txt"), "{status}"); +} + +/// The same set-aside work, on the path where the fold fails after the fixup +/// commit exists: nothing durable holds the patch yet, so only the guard can +/// bring it back. A file where `.git/loom` must be a directory is what makes +/// `transaction::save` fail this late. +#[test] +fn a_fold_failing_after_the_fixup_commit_puts_back_the_staging() { + let test_repo = TestRepo::new_with_remote(); + test_repo.write_file("file.txt", "target\n"); + test_repo.stage_files(&["file.txt"]); + test_repo.commit_staged("target commit"); + let target_oid = test_repo.head_oid(); + + test_repo.write_file("later.txt", "later\n"); + test_repo.stage_files(&["later.txt"]); + test_repo.commit_staged("second commit"); + + test_repo.write_file("file.txt", "target amended\n"); + test_repo.write_file("kept.txt", "staged, and none of the fold's business\n"); + test_repo.stage_files(&["file.txt", "kept.txt"]); + std::fs::write(test_repo.repo.path().join("loom"), "not a directory").unwrap(); + + let result = super::fold_files_into_commit( + &test_repo.repo, + &["file.txt".to_string()], + &target_oid.to_string(), + true, + &[], + ); + + assert!(result.is_err(), "the state file cannot be written"); + // The rollback resets over the fixup commit, so only the reflog still shows + // it. Without this the test passes just as green if the failure ever moves + // earlier, leaving the window it is named after unguarded. + let reflog = crate::git::run_git_stdout(&test_repo.workdir(), &["reflog", "--format=%gs"]) + .expect("reflog"); + assert!( + reflog.contains("fixup! target commit"), + "the fixup commit must already exist: {reflog}" + ); + let status = test_repo.status_porcelain(); + assert!(status.contains("A kept.txt"), "{status}"); +} + // ── Case 2: Commit + Commit (Fixup) ───────────────────────────────────── #[test] @@ -4275,7 +4350,7 @@ fn an_amend_that_replaced_head_is_taken_back() { crate::git::run_git(&workdir, &["commit", "--amend", "--no-edit"]).unwrap(); assert_ne!(t.head_oid(), head, "the amend should have moved HEAD"); - super::undo_commit_attempt(&workdir, head, &["file1.txt"], &saved); + super::undo_commit_attempt(&workdir, head, &["file1.txt"], saved); assert_eq!(t.head_oid(), head); assert_eq!(t.get_message(0), "Second"); diff --git a/src/git/git_apply.rs b/src/git/git_apply.rs index 3cbcd5f1..f6690cf2 100644 --- a/src/git/git_apply.rs +++ b/src/git/git_apply.rs @@ -255,25 +255,27 @@ fn run_apply( /// non-zero. Left in the real one those read as `UU` with no merge in progress /// and — the apply being `--cached` — no markers in the files to resolve. /// -/// An unmerged index is left alone: the autostash replay conflicted, so those -/// stages are the user's own merge to finish, and git keeps the stash it could -/// not replay. Best-effort otherwise — every caller runs this after its own -/// rewrite has landed, so a failure here must not turn that into a command -/// reporting failure; one caller deletes the branch it just wove on `Err`. +/// An unmerged index is left alone: those stages are a merge the user has to +/// finish, and writing over them would bury it. After a rebase they come from +/// an autostash replay that conflicted, and git keeps that stash behind them; +/// the guard's exits reach this having stashed nothing, so the parked patch is +/// the staged side either way. +/// +/// Best-effort otherwise, and it must never fail its caller: the rebase +/// callers run it once their own rewrite has landed, one of which deletes the +/// branch it just wove on `Err`, and [`restore_loom_unstaged`] brings in the +/// guard's exits, where nothing landed and no rebase ever ran. pub fn restore_staged_after_rebase(workdir: &Path, patch: &str) { if patch.is_empty() { return; } - // Git keeps the stash it could not replay, so the content is in there and - // the stages are the user's merge to finish. if super::has_unmerged_paths(workdir) { - // `git stash pop --index` is refused while the index is unmerged, so - // pointing at the stash would be advice that fails when followed. The - // patch is the staged side, so it is handed over instead. + // Not the stash a rebase would have kept: `git stash pop --index` is + // refused while the index is unmerged, and the guard's exits never made + // one. The patch is the staged side, so it is handed over instead. msg::warn( "the index has unmerged paths, so your staged changes could not go back \ - — resolve them, then either replay the patch below or take the staged \ - side from the stash git kept", + — resolve them, then replay the patch below", ); park(workdir, patch); return; @@ -313,8 +315,10 @@ pub fn restore_staged_after_rebase(workdir: &Path, patch: &str) { if super::diff_cached(workdir).is_ok_and(|current| current == patch) { return; } + // Not "what the rebase wrote": the guard reaches this from exits + // that never ran one. msg::warn(&format!( - "your staged changes no longer apply over what the rebase wrote: {e}" + "your staged changes no longer apply over what is in the index now: {e}" )); park(workdir, patch); } @@ -329,17 +333,28 @@ pub fn restore_staged_after_rebase(workdir: &Path, patch: &str) { } } -/// Put back a patch loom unstaged itself, after a call that aborted its own -/// rebase. +/// Put back a patch loom unstaged itself, wherever the call ended. /// -/// Unlike [`restore_or_park_after_abort`], a refusal from before the rebase -/// started still restores: no autostash ever held this staged side, because -/// loom emptied the index before the rebase existed. -pub fn restore_loom_unstaged_after_abort(workdir: &Path, patch: &str, err: &anyhow::Error) { - if super::rebase_never_started(err) { - restore_staged_patch(workdir, patch); +/// For the owner that cannot see the error — the guard restoring on drop — so +/// a rebase left on disk by a failed abort is recognised from the git dir +/// instead. Restoring into a live rebase's index would only be dropped again +/// by the `loom abort` that follows, so the patch goes to the user. The guard +/// emptied the index itself, so it restores after a refusal from before the +/// rebase started too: no autostash ever held that staged side. +/// [`restore_or_park_after_abort`] is the other way in and filters that case +/// out before here — its patch is autostashed work, which loom never unstaged. +pub fn restore_loom_unstaged(workdir: &Path, patch: &str) { + // Before the git dir is asked for: a guard over nothing is the common case, + // and it must neither warn about work that does not exist nor pay for a git + // call per command. + if patch.is_empty() { + return; + } + if super::rebase_is_over(workdir) { + restore_staged_after_rebase(workdir, patch); } else { - restore_or_park_after_abort(workdir, patch, err); + msg::warn("the rebase is still on disk, so your staged changes could not be put back"); + park(workdir, patch); } } @@ -353,12 +368,7 @@ pub fn restore_or_park_after_abort(workdir: &Path, patch: &str, err: &anyhow::Er if super::rebase_never_started(err) { return; } - if super::rebase_is_over(workdir) { - restore_staged_after_rebase(workdir, patch); - } else { - msg::warn("the rebase is still on disk, so your staged changes could not be put back"); - park(workdir, patch); - } + restore_loom_unstaged(workdir, patch); } /// Hand the staged patch to the user: a clean autostash replay puts the diff --git a/src/git/git_apply_test.rs b/src/git/git_apply_test.rs index a023eee9..9f048d51 100644 --- a/src/git/git_apply_test.rs +++ b/src/git/git_apply_test.rs @@ -374,7 +374,7 @@ fn restore_or_park_after_abort_parks_when_the_rebase_survived() { std::fs::create_dir_all(t.repo.path().join("rebase-merge")).unwrap(); let err = anyhow::anyhow!("the abort failed too"); - git::restore_loom_unstaged_after_abort(&workdir, &patch, &err); + git::restore_or_park_after_abort(&workdir, &patch, &err); let parked = git::git_path(&workdir, "loom").unwrap(); assert!( @@ -564,11 +564,39 @@ fn restore_staged_after_rebase_parks_a_patch_naming_a_removed_path() { ); } -/// `loom fold -p` empties the index itself before rebasing, so a refusal from -/// before the rebase started has no autostash behind it: the staged side is -/// only in the patch, and must go back even though nothing was ever stashed. +/// A refusal raised before the rebase started autostashed nothing and left the +/// index where it was, so it is the user's and this must not write to it. The +/// patch names work the index does not hold, which is what would show if the +/// early return went. #[test] -fn restore_loom_unstaged_after_abort_restores_when_the_rebase_never_started() { +fn restore_or_park_after_abort_leaves_the_index_alone_before_the_rebase_starts() { + let t = TestRepo::new(); + t.write_file("a.txt", "committed\n"); + t.stage_files(&["a.txt"]); + t.commit_staged("base"); + + let workdir = t.workdir(); + t.write_file("aside.txt", "set aside\n"); + t.stage_files(&["aside.txt"]); + let patch = git::diff_cached(&workdir).unwrap(); + git::run_git(&workdir, &["reset", "-q", "HEAD"]).unwrap(); + + let err = git::before_rebase_starts::<()>(Err(anyhow::anyhow!("checked out elsewhere"))) + .expect_err("tagged as raised before the rebase started"); + git::restore_or_park_after_abort(&workdir, &patch, &err); + + assert_eq!(git::diff_cached(&workdir).unwrap(), ""); + assert!( + !git::git_path(&workdir, "loom").unwrap().exists(), + "nothing to park either" + ); +} + +/// `loom fold -p` empties the index itself before rebasing, so a failure with +/// no rebase left on disk has no autostash behind it: the staged side is only +/// in the patch, and must go back even though nothing was ever stashed. +#[test] +fn restore_loom_unstaged_restores_when_no_rebase_is_on_disk() { let t = TestRepo::new(); t.write_file("a.txt", "committed\n"); t.stage_files(&["a.txt"]); @@ -581,9 +609,7 @@ fn restore_loom_unstaged_after_abort_restores_when_the_rebase_never_started() { // What `staging::save_and_unstage_staged` leaves behind. git::run_git(&workdir, &["reset", "-q", "HEAD"]).unwrap(); - let err = git::before_rebase_starts::<()>(Err(anyhow::anyhow!("checked out elsewhere"))) - .expect_err("tagged as raised before the rebase started"); - git::restore_loom_unstaged_after_abort(&workdir, &patch, &err); + git::restore_loom_unstaged(&workdir, &patch); assert_eq!(git::diff_cached(&workdir).unwrap(), patch); } diff --git a/src/git/mod.rs b/src/git/mod.rs index 727fd435..df3215f9 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -9,7 +9,7 @@ pub mod git_worktree; pub use git_apply::{ Replay, apply_cached_patch, apply_cached_patch_reverse, apply_patch, apply_patch_reverse, apply_patch_to_worktree, apply_patch_with_index, apply_patch_with_index_reverse, - restore_loom_unstaged_after_abort, restore_or_park_after_abort, restore_staged_after_rebase, + restore_loom_unstaged, restore_or_park_after_abort, restore_staged_after_rebase, restore_staged_patch, save_or_warn, save_patch_aside, }; pub use git_branch::{ diff --git a/src/split.rs b/src/split.rs index 9a3fe758..bc29e91b 100644 --- a/src/split.rs +++ b/src/split.rs @@ -234,12 +234,10 @@ fn run_split( let oid_str = commit_oid.to_string(); let short_hash = git::short_hash(&oid_str); // Save pre-existing staged changes so `reset --mixed` does not discard them. - // Unstaging them first also keeps the restore below a plain apply: a split - // leaves HEAD's tree as it was, so the patch still applies over it. let saved_staged = staging::save_and_unstage_staged(repo, workdir)?; let split_result = do_split(is_head); // Restore pre-existing staged changes regardless of outcome. - git::restore_staged_patch(workdir, &saved_staged); + saved_staged.restore(); let (h1, h2) = split_result?; msg::success(&format!( "Split `{}` into {} and {}",