Fix data-loss and consistency findings from the final review

- amending into an immutable commit fails fast instead of silently
  no-opping - previously the worktree discard fallback then DELETED the
  changes it believed were committed (data loss)
- the whole-file discard fallback content-verifies each file against its
  pre-amend hash and leaves mid-operation edits in place (warn + skip),
  making the never-loss invariant hold for whole-file specs
- amends whose rebase would leave a worktree tip conflict-encoded are
  refused before materializing (plain git worktrees have no conflict UI)
- materialize_without_checkout now runs worktree checkouts too - its
  contract only exempts the editor's own worktree
- but worktree amend resolves worktree-only commits via rev-parse
- worktree chips never collide with change-id chips and the no-window
  fallback is a deterministic name-seeded probe (no more migration when
  the uncommitted-file set changes)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: Caleb-T-Owens

crates/but-api/src/worktrees.rs

@@ -10,6 +10,7 @@ use but_core::{DiffSpec, DryRun};
 use but_rebase::graph_rebase::{Editor, GraphEditorOptions, LookupStep as _};
 use but_workspace::worktrees::{WorktreeListing, WorktreeSource};
 use gix::bstr::BStr;
+use gix::prelude::ObjectIdExt as _;
 use tracing::instrument;
 
 use crate::{WorkspaceState, commit::types::CommitCreateResult};
@@ -92,12 +93,20 @@ pub fn linked_worktree_changes(
 /// the branch of any active worktree (including `name`'s own).
 ///
 /// The worktree's branch is rebased if the target is in its history, and its
-/// checkout follows with the consumed changes cancelled out. When the worktree's
-/// tip doesn't move (the target lives elsewhere), the consumed changes are
-/// discarded from the worktree after the commit and all ref edits are durable -
-/// so every failure window leaves a harmless duplicate of the changes, never a
-/// loss. Consumed changes that no longer match the worktree's live state at that
-/// point are left in place with a warning.
+/// checkout follows with the consumed changes cancelled out. When the rewrite
+/// would leave any linked worktree's branch on a conflict-encoded commit (the
+/// amend conflicts with later commits on that branch), this fails before
+/// anything is materialized - zero mutation.
+///
+/// When the worktree's tip doesn't move (the target lives elsewhere), the
+/// consumed changes are discarded from the worktree after the commit and all
+/// ref edits are durable, and each file is only discarded after re-verifying
+/// that its content still matches what was amended - so every failure window
+/// leaves a harmless duplicate of the changes, never a loss. Consumed changes
+/// whose file content changed in the meantime, or that no longer match the
+/// worktree's live state, are left in place with a `tracing` warning only;
+/// [`CommitCreateResult`] does not (yet) report which ones were left behind,
+/// so callers can merely tell users that this may happen.
 ///
 /// Note that unlike [`commit_amend`](crate::commit::amend::commit_amend), no
 /// oplog snapshot is recorded yet - oplog coverage of linked worktrees is
@@ -124,6 +133,19 @@ pub fn worktree_commit_amend(
     // Captured before any mutation: an unchanged tip afterwards means the
     // worktree's checkout didn't participate in the rewrite.
     let old_worktree_head = wt_repo.head_id()?.detach();
+    // Content identity of every requested file, captured before the amend so
+    // the discard fallback below can verify nothing wrote to them in between
+    // (editor autosave, formatters, watchers - the amend takes long enough for
+    // that race to be real).
+    let content_snapshots: std::collections::BTreeMap<_, _> = changes
+        .iter()
+        .map(|spec| {
+            (
+                spec.path.clone(),
+                snapshot_worktree_file(&wt_repo, spec.path.as_ref()),
+            )
+        })
+        .collect();
 
     // Worktree branches are seeded into the graph as extra tips but aren't
     // reachable from `HEAD`, which leaves them immutable by default - without
@@ -164,6 +186,30 @@ pub fn worktree_commit_amend(
     let new_commit = commit_selector
         .map(|commit_selector| rebase.lookup_pick(commit_selector))
         .transpose()?;
+
+    // Refuse to leave any linked worktree's branch on a conflict-encoded
+    // commit: its checkout would be skipped (conflicted trees are never
+    // written into plain worktrees) while the ref still moves, silently
+    // stranding a stale checkout on a GitButler-internal commit. Nothing has
+    // been materialized yet, so bailing here mutates nothing.
+    if new_commit.is_some() {
+        for (wt_name, tip) in rebase.worktree_checkout_tips()? {
+            if but_core::Commit::from_id(tip.attach(rebase.repo()))?.is_conflicted() {
+                bail!(
+                    "amending into {commit_id} would conflict with later commits on the branch \
+                     checked out in worktree {wt_name} - aborting without any changes"
+                );
+            }
+        }
+    }
+    if let Some(new_commit) = new_commit
+        && but_core::Commit::from_id(new_commit.attach(rebase.repo()))?.is_conflicted()
+    {
+        bail!(
+            "amending into {commit_id} would produce a conflicted commit - aborting without any changes"
+        );
+    }
+
     let is_dry_run: bool = dry_run.into();
     let workspace = WorkspaceState::from_successful_rebase(rebase, &repo, dry_run)?;
 
@@ -173,15 +219,37 @@ pub fn worktree_commit_amend(
     if !is_dry_run && new_commit.is_some() && !consumed_specs.is_empty() {
         let wt_repo = but_workspace::worktrees::open_worktree_repo(&repo, name)?;
         if wt_repo.head_id()?.detach() == old_worktree_head {
-            let dropped =
-                but_workspace::discard_workspace_changes(&wt_repo, consumed_specs, context_lines)?;
-            if !dropped.is_empty() {
+            // Only discard files whose content still matches the pre-amend
+            // snapshot - anything written in between is in neither the amended
+            // commit nor the snapshot, so discarding it would destroy it.
+            let (verified_specs, changed_specs): (Vec<_>, Vec<_>) =
+                consumed_specs.into_iter().partition(|spec| {
+                    content_snapshots.get(&spec.path).is_some_and(|before| {
+                        before.is_verifiable()
+                            && *before == snapshot_worktree_file(&wt_repo, spec.path.as_ref())
+                    })
+                });
+            for spec in &changed_specs {
                 tracing::warn!(
                     worktree = %name,
-                    ?dropped,
-                    "some committed changes no longer matched the worktree state - leaving them in place"
+                    path = %spec.path,
+                    "file content changed while amending - leaving it in the worktree"
                 );
             }
+            if !verified_specs.is_empty() {
+                let dropped = but_workspace::discard_workspace_changes(
+                    &wt_repo,
+                    verified_specs,
+                    context_lines,
+                )?;
+                if !dropped.is_empty() {
+                    tracing::warn!(
+                        worktree = %name,
+                        ?dropped,
+                        "some committed changes no longer matched the worktree state - leaving them in place"
+                    );
+                }
+            }
         }
     }
 
@@ -191,3 +259,62 @@ pub fn worktree_commit_amend(
         workspace,
     })
 }
+
+/// The content identity of a worktree file at one point in time, used to
+/// verify it didn't change between two reads.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum FileSnapshot {
+    /// Nothing exists at the path.
+    Missing,
+    /// A regular file or symlink whose content hashes to this blob id
+    /// (symlinks hash their target path, like Git does).
+    Blob(gix::ObjectId),
+    /// The path exists but could not be read, or isn't a file/symlink.
+    /// Never treated as matching anything, not even another unreadable state.
+    Unreadable,
+}
+
+impl FileSnapshot {
+    /// Whether this snapshot pins down actual content that a later read can be
+    /// compared against.
+    fn is_verifiable(&self) -> bool {
+        !matches!(self, FileSnapshot::Unreadable)
+    }
+}
+
+/// Hash the file at worktree-relative `rela_path` in `wt_repo`'s working
+/// directory as a git blob, without writing any object.
+///
+/// The raw on-disk bytes are hashed (no filters applied) - the result is only
+/// meant to be compared against another snapshot taken the same way.
+fn snapshot_worktree_file(wt_repo: &gix::Repository, rela_path: &BStr) -> FileSnapshot {
+    let Some(workdir) = wt_repo.workdir() else {
+        return FileSnapshot::Unreadable;
+    };
+    let path = workdir.join(gix::path::from_bstr(rela_path));
+    let md = match std::fs::symlink_metadata(&path) {
+        Ok(md) => md,
+        Err(err) if err.kind() == std::io::ErrorKind::NotFound => return FileSnapshot::Missing,
+        Err(_) => return FileSnapshot::Unreadable,
+    };
+    let bytes = if md.is_symlink() {
+        match std::fs::read_link(&path)
+            .map_err(anyhow::Error::from)
+            .and_then(|target| Ok(gix::path::os_string_into_bstring(target.into())?))
+        {
+            Ok(target) => Vec::from(target),
+            Err(_) => return FileSnapshot::Unreadable,
+        }
+    } else if md.is_file() {
+        match std::fs::read(&path) {
+            Ok(bytes) => bytes,
+            Err(_) => return FileSnapshot::Unreadable,
+        }
+    } else {
+        return FileSnapshot::Unreadable;
+    };
+    match gix::objs::compute_hash(wt_repo.object_hash(), gix::object::Kind::Blob, &bytes) {
+        Ok(id) => FileSnapshot::Blob(id),
+        Err(_) => FileSnapshot::Unreadable,
+    }
+}

crates/but-rebase/src/graph_rebase/materialize.rs

@@ -13,7 +13,8 @@ use gix::{
 };
 
 use crate::graph_rebase::{
-    Checkout, MaterializeOutcome, Pick, Step, SuccessfulRebase, util::collect_ordered_parents,
+    Checkout, MaterializeOutcome, Pick, Selector, Step, SuccessfulRebase,
+    util::collect_ordered_parents,
 };
 
 /// Check out `new_tip` in the linked worktree named `worktree_name`, doing nothing
@@ -56,61 +57,105 @@ fn checkout_worktree(
 }
 
 impl<'ws, 'graph, M: RefMetadata> SuccessfulRebase<'ws, 'graph, M> {
+    /// Resolve the tip a worktree checkout `selector` points to after the rebase,
+    /// or `None` when the branch step was removed from the graph.
+    fn resolve_worktree_checkout_tip(&self, selector: Selector) -> Result<Option<gix::ObjectId>> {
+        let selector = self.history.normalize_selector(selector)?;
+        Ok(match &self.graph[selector.id] {
+            Step::None => None,
+            Step::Pick(Pick { id, .. }) => Some(*id),
+            Step::Reference { .. } => {
+                let parents = collect_ordered_parents(&self.graph, selector.id);
+                let parent_step_id = parents.first().context("No first parent to reference")?;
+                let Step::Pick(Pick { id, .. }) = self.graph[*parent_step_id] else {
+                    bail!("collect_ordered_parents should always return a commit pick");
+                };
+                Some(id)
+            }
+        })
+    }
+
+    /// The tip each linked-worktree checkout will point to once this rebase is
+    /// materialized, as `(worktree_name, commit_id)` pairs.
+    ///
+    /// Worktrees whose checked-out branch was removed from the graph are skipped.
+    /// Objects referenced by the returned ids may exist only in the in-memory
+    /// repository, see [`Self::repo()`].
+    pub fn worktree_checkout_tips(&self) -> Result<Vec<(gix::bstr::BString, gix::ObjectId)>> {
+        let mut tips = Vec::new();
+        for checkout in &self.checkouts {
+            let Checkout::Worktree {
+                selector,
+                worktree_name,
+                ..
+            } = checkout
+            else {
+                continue;
+            };
+            if let Some(tip) = self.resolve_worktree_checkout_tip(*selector)? {
+                tips.push((worktree_name.clone(), tip));
+            }
+        }
+        Ok(tips)
+    }
+
+    /// Run the checkout of every linked worktree whose branch this rebase moves.
+    ///
+    /// All objects must be persisted beforehand - the worktree repositories are
+    /// opened from disk. A broken linked worktree degrades to today's
+    /// stale-checkout behavior with a warning instead of failing the whole
+    /// operation.
+    fn checkout_worktrees(&self, repo: &gix::Repository) -> Result<()> {
+        for checkout in &self.checkouts {
+            let Checkout::Worktree {
+                selector,
+                worktree_name,
+                merge_base_override,
+            } = checkout
+            else {
+                continue;
+            };
+            let Some(new_tip) = self.resolve_worktree_checkout_tip(*selector)? else {
+                tracing::warn!(
+                    worktree = %worktree_name,
+                    "the branch this worktree checks out was removed - leaving its checkout as is"
+                );
+                continue;
+            };
+            if let Err(err) = checkout_worktree(
+                repo,
+                worktree_name.as_ref(),
+                new_tip,
+                *merge_base_override,
+            ) {
+                tracing::warn!(
+                    worktree = %worktree_name,
+                    err = %err,
+                    "failed to check out linked worktree - its branch still moves"
+                );
+            }
+        }
+        Ok(())
+    }
+
     /// Materializes a history rewrite
     pub fn materialize(mut self) -> Result<MaterializeOutcome<'ws, 'graph, M>> {
         let repo = self.repo.clone();
         if let Some(memory) = self.repo.objects.take_object_memory() {
-            memory.persist(self.repo)?;
+            memory.persist(&self.repo)?;
         }
 
+        self.checkout_worktrees(&repo)?;
+
         let mut head_reference_update = None;
-        for checkout in self.checkouts {
+        for checkout in &self.checkouts {
             match checkout {
-                Checkout::Worktree {
-                    selector,
-                    worktree_name,
-                    merge_base_override,
-                } => {
-                    let selector = self.history.normalize_selector(selector)?;
-                    let new_tip = match &self.graph[selector.id] {
-                        Step::None => {
-                            tracing::warn!(
-                                worktree = %worktree_name,
-                                "the branch this worktree checks out was removed - leaving its checkout as is"
-                            );
-                            continue;
-                        }
-                        Step::Pick(Pick { id, .. }) => *id,
-                        Step::Reference { .. } => {
-                            let parents = collect_ordered_parents(&self.graph, selector.id);
-                            let parent_step_id =
-                                parents.first().context("No first parent to reference")?;
-                            let Step::Pick(Pick { id, .. }) = self.graph[*parent_step_id] else {
-                                bail!("collect_ordered_parents should always return a commit pick");
-                            };
-                            id
-                        }
-                    };
-                    // A broken linked worktree degrades to today's stale-checkout
-                    // behavior instead of failing the whole operation.
-                    if let Err(err) = checkout_worktree(
-                        &repo,
-                        worktree_name.as_ref(),
-                        new_tip,
-                        merge_base_override,
-                    ) {
-                        tracing::warn!(
-                            worktree = %worktree_name,
-                            err = %err,
-                            "failed to check out linked worktree - its branch still moves"
-                        );
-                    }
-                }
+                Checkout::Worktree { .. } => {}
                 Checkout::Head {
                     selector,
                     merge_base_override,
                 } => {
-                    let selector = self.history.normalize_selector(selector)?;
+                    let selector = self.history.normalize_selector(*selector)?;
                     let step = self.graph[selector.id].clone();
 
                     let (new_head, new_head_refname) = match step {
@@ -135,7 +180,7 @@ impl<'ws, 'graph, M: RefMetadata> SuccessfulRebase<'ws, 'graph, M> {
                         &repo,
                         Options {
                             skip_head_update: true,
-                            merge_base_override,
+                            merge_base_override: *merge_base_override,
                             allow_conflicted_commit_checkout: true,
                         },
                     )?;
@@ -180,7 +225,8 @@ impl<'ws, 'graph, M: RefMetadata> SuccessfulRebase<'ws, 'graph, M> {
         })
     }
 
-    /// Materializes a rebase without performing a checkout.
+    /// Materializes a rebase without performing a checkout of the editor's own
+    /// (`HEAD`) worktree.
     ///
     /// For the vast majority of operations you want to use
     /// [`Self::materialize`]. This is intended to be used in niche cases like
@@ -195,12 +241,19 @@ impl<'ws, 'graph, M: RefMetadata> SuccessfulRebase<'ws, 'graph, M> {
     ///
     /// If I instead called [`Self::materialize`], the changes would instead be
     /// gone from disk.
+    ///
+    /// Note that linked worktrees whose branches the rebase moves are still
+    /// checked out - "without checkout" is strictly about the editor's own
+    /// worktree; skipping the linked ones would leave their checkouts stale
+    /// behind their moved branches.
     pub fn materialize_without_checkout(mut self) -> Result<MaterializeOutcome<'ws, 'graph, M>> {
         let repo = self.repo.clone();
         if let Some(memory) = self.repo.objects.take_object_memory() {
-            memory.persist(self.repo)?;
+            memory.persist(&self.repo)?;
         }
 
+        self.checkout_worktrees(&repo)?;
+
         repo.edit_references(self.ref_edits.clone())?;
 
         let project_meta = self.workspace.graph.project_meta.clone();