From cab2ff60bcec8a5e715a7cc97ef380b4515f3b62 Mon Sep 17 00:00:00 2001 From: adehad <26027314+adehad@users.noreply.github.com> Date: Mon, 3 Aug 2026 10:48:26 +0100 Subject: [PATCH 1/9] feat(git): apply and revert diff content in the working tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Historical diffs are inert. PR #65 stopped a commit diff pretending to be unstaged working-tree content, which closed the hazard but left the other half of the bug: there is still nothing you can do with a hunk you are looking at in another commit, stash, or branch. Sublime Merge's answer is to write a .patch and apply it; git's is `git apply` / `git apply -R`. Add that operation at the git layer. `apply_worktree_patch` takes a file, a `WorktreePatchSource`, a `WorktreePatchScope` (whole file, one hunk, or a line selection) and a direction, and rewrites that file on disk. The source models a *pair of revisions*, not a commit. `Commit(oid)` resolves to first-parent → itself and `Compare { from, to }` to any two revparse-able revisions, and both then travel identical code. Applying across branches is therefore the same operation as applying a commit's hunk, generated from the correct tree pair — not from index→workdir, which is what would make a cross-branch apply silently target the wrong content. Neither obvious applier works here: * `repo.apply(.., ApplyLocation::WorkDir, ..)` matches hunk context literally and has no three-way fallback. Any uncommitted edit inside a hunk's context window defeats it, and a dirty working tree is the normal case for this feature — you compare against another branch precisely because you are mid-change. `libgit2s_own_apply_refuses_the_case_the_merge_handles` pins that. * `git apply --3way` merges into the *index* and refuses with "does not match index" whenever the working-tree file differs from its index entry — exactly the dirty tree the fallback exists for. It also leaves conflict markers and an unmerged index entry behind. So reconstruct both sides and let libgit2 merge them: `base` is the file on the starting side, `target` is `base` with just the selected hunk or lines rewritten (computed from the diff, so exact by construction), and `ours` is the working tree. A three-way merge of the three keeps unrelated local edits and conflicts only on genuine overlap, without touching the index. The rewrite itself is pure and unit-tested; within a run of adjacent -/+ lines the i-th removal pairs with the i-th insertion, matching the side-by-side view, because reading git's removals-then- insertions order literally puts a kept line on the wrong side of an applied one. Three failure modes get three messages: already applied (the merge folds back to what is on disk), a conflicting local edit (the merge conflicts and the file is dirty), and a context mismatch (the merge conflicts and the file is clean, so the file itself has moved on). Nothing is written in any of them. Unlike staging, these operations edit files on disk, so each returns a byte snapshot of what it overwrote and emits `WorktreePatchApplied`; `restore_worktree_files_at` puts those bytes back. A snapshot is exact even when the forward operation went through a merge, which a reverse patch would not be. Co-Authored-By: Claude Fable 5 --- crates/rgitui_git/src/project/mod.rs | 14 + .../rgitui_git/src/project/worktree_patch.rs | 2221 +++++++++++++++++ crates/rgitui_git/src/types.rs | 6 + 3 files changed, 2241 insertions(+) create mode 100644 crates/rgitui_git/src/project/worktree_patch.rs diff --git a/crates/rgitui_git/src/project/mod.rs b/crates/rgitui_git/src/project/mod.rs index 35f9913..db455f6 100644 --- a/crates/rgitui_git/src/project/mod.rs +++ b/crates/rgitui_git/src/project/mod.rs @@ -13,6 +13,7 @@ mod refresh; mod search; mod submodule; mod watcher; +mod worktree_patch; use anyhow::{Context as _, Result}; use git2::{Repository, StatusOptions}; @@ -79,6 +80,11 @@ pub use submodule::{ compute_submodules, submodule_init, submodule_init_all, submodule_update, submodule_update_all, SubmoduleInfo, }; +pub use worktree_patch::{ + apply_worktree_patch, restore_worktree_files, snapshots_fit_undo_stack, WorktreeFileSnapshot, + WorktreePatchDirection, WorktreePatchOutcome, WorktreePatchScope, WorktreePatchSource, + MAX_UNDO_SNAPSHOT_BYTES, +}; fn parse_remote_tracking_ref(name: &str) -> Option<(String, String)> { let trimmed = name.strip_prefix("refs/remotes/").unwrap_or(name); @@ -256,6 +262,14 @@ pub enum GitProjectEvent { /// Emitted after ahead/behind for all branches has been recomputed in the background. AheadBehindRefreshed, OperationUpdated(GitOperationUpdate), + /// An apply/revert rewrote working-tree files on disk. Carries what those + /// files held beforehand so the workspace can offer an exact undo; unlike + /// staging, these operations are not recoverable from git state alone. + WorktreePatchApplied { + /// Undo label, e.g. "Applied hunk 2 of src/main.rs from a1b2c3d". + label: String, + snapshots: Vec, + }, } /// The core Git project state holder. diff --git a/crates/rgitui_git/src/project/worktree_patch.rs b/crates/rgitui_git/src/project/worktree_patch.rs new file mode 100644 index 0000000..f9c7978 --- /dev/null +++ b/crates/rgitui_git/src/project/worktree_patch.rs @@ -0,0 +1,2221 @@ +//! Applying and reverting diff content in the working tree. +//! +//! Staging moves content between the working tree and the index, so it can be +//! expressed as `repo.apply(.., ApplyLocation::Index, ..)` over a patch sliced +//! out of the index→workdir diff. Applying *historical* content is a different +//! operation: the patch comes from a pair of trees that need not include the +//! working tree at all — a past commit against its parent, a stash entry, or +//! two arbitrary branches — and the result is written to files on disk. +//! +//! ## Why not `repo.apply(.., ApplyLocation::WorkDir, ..)` +//! +//! libgit2's apply is a plain patch applier: it matches each hunk's context +//! against the target and fails outright when the context does not line up. It +//! has no three-way fallback. The working tree being dirty is the *normal* case +//! for this feature — you compare against another branch precisely because you +//! are mid-change — and an uncommitted edit anywhere inside a hunk's context +//! window (three lines either side) is enough to make a context match fail. +//! +//! ## Why not `git apply --3way` +//! +//! `git apply --3way` is the CLI's answer to that, and it is what a patch-file +//! workflow effectively gets. But it merges into the *index*: it refuses with +//! `error: : does not match index` whenever the working-tree file differs +//! from its index entry. That is exactly the dirty working tree we need to +//! support, so the one case the fallback exists for is the case it rejects. +//! It also leaves conflict markers plus an unmerged index entry behind on +//! failure, which is a worse state to hand back than a refusal. +//! +//! ## What this does instead +//! +//! Reconstruct the two sides ourselves and let libgit2 merge them: +//! +//! * `base` — the file as it is on the side the patch starts from. +//! * `target` — `base` rewritten with exactly the selected hunk or lines +//! applied (or reverted). Computed from the diff, not by matching context, so +//! it is exact by construction. +//! * `ours` — the file as it is in the working tree right now. +//! +//! Then a three-way merge of (base, ours, target). Because base→target differs +//! only inside the selected region, unrelated local edits merge cleanly and an +//! overlapping edit conflicts — which is the behaviour `--3way` promises, +//! obtained without touching the index and without needing the working tree to +//! match it. + +use std::collections::HashSet; +use std::path::{Path, PathBuf}; + +use anyhow::{Context as _, Result}; +use git2::{IndexEntry, IndexTime, Oid, Repository}; + +/// Which side of a diff the working tree should be moved toward. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum WorktreePatchDirection { + /// Bring the diff's new-side content into the working tree — the + /// hunk-level equivalent of a cherry-pick. + Apply, + /// Restore the diff's old-side content in the working tree — the + /// hunk-level equivalent of a revert. + Revert, +} + +impl WorktreePatchDirection { + /// Verb used in progress and success messages ("Applying", "Applied"). + pub fn present_participle(self) -> &'static str { + match self { + WorktreePatchDirection::Apply => "Applying", + WorktreePatchDirection::Revert => "Reverting", + } + } + + /// Verb used in success messages. + pub fn past_tense(self) -> &'static str { + match self { + WorktreePatchDirection::Apply => "Applied", + WorktreePatchDirection::Revert => "Reverted", + } + } + + fn verb(self) -> &'static str { + match self { + WorktreePatchDirection::Apply => "apply", + WorktreePatchDirection::Revert => "revert", + } + } + + /// The direction that undoes this one. + pub fn inverse(self) -> Self { + match self { + WorktreePatchDirection::Apply => WorktreePatchDirection::Revert, + WorktreePatchDirection::Revert => WorktreePatchDirection::Apply, + } + } +} + +/// How much of a file's diff a working-tree apply or revert covers. +/// +/// The three variants are the three granularities the diff viewer offers: the +/// hunk under the cursor by default, a manual line selection when the user has +/// made one, and the whole file from the file-level menu. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum WorktreePatchScope { + /// Every hunk in the file. + File, + /// One hunk, indexed as the diff viewer displays it. + Hunk(usize), + /// Only the given change lines, as `(old_lineno, new_lineno)` pairs from + /// the diff viewer: an addition is `(None, Some(n))`, a deletion + /// `(Some(n), None)`. + Lines(Vec<(Option, Option)>), +} + +impl WorktreePatchScope { + /// Human-readable description used in operation summaries and toasts. + pub fn describe(&self) -> String { + match self { + WorktreePatchScope::File => "all changes".to_string(), + WorktreePatchScope::Hunk(index) => format!("hunk {}", index + 1), + WorktreePatchScope::Lines(pairs) => format!( + "{} line{}", + pairs.len(), + if pairs.len() == 1 { "" } else { "s" } + ), + } + } +} + +/// The pair of revisions whose difference is being applied or reverted. +/// +/// `Commit` is not special-cased into the apply machinery: it resolves to the +/// same `from`/`to` tree pair as any other comparison, so a cross-branch diff +/// and a historical commit diff travel identical code. That is what makes +/// "apply the difference between two branches into my working tree" work rather +/// than being a no-op bolted onto a commit-only feature. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum WorktreePatchSource { + /// The change one commit (or stash entry) introduced: its first parent → + /// itself. A root commit's `from` side is the empty tree. + Commit(Oid), + /// The difference between two arbitrary revisions, `from` → `to`. Both are + /// resolved with `revparse_single`, so branch names, tags, `HEAD~3`, + /// `origin/main` and raw OIDs all work. + Compare { from: String, to: String }, +} + +impl WorktreePatchSource { + /// Short label naming this source in user-facing messages. + pub fn label(&self) -> String { + match self { + WorktreePatchSource::Commit(oid) => short_oid(&oid.to_string()), + WorktreePatchSource::Compare { from, to } => format!("{from}...{to}"), + } + } + + /// The (from, to) trees this source compares. Either side may be absent — + /// a root commit has no parent tree, and an explicitly empty revision + /// resolves to no tree. + fn trees<'repo>( + &self, + repo: &'repo Repository, + ) -> Result<(Option>, Option>)> { + match self { + WorktreePatchSource::Commit(oid) => { + let commit = repo.find_commit(*oid).with_context(|| { + format!( + "Commit {} is not in this repository", + short_oid(&oid.to_string()) + ) + })?; + let to = commit.tree()?; + let from = if commit.parent_count() > 0 { + Some(commit.parent(0)?.tree()?) + } else { + None + }; + Ok((from, Some(to))) + } + WorktreePatchSource::Compare { from, to } => Ok(( + Some(resolve_tree(repo, from)?), + Some(resolve_tree(repo, to)?), + )), + } + } +} + +fn resolve_tree<'repo>(repo: &'repo Repository, rev: &str) -> Result> { + let object = repo.revparse_single(rev).with_context(|| { + format!("Can't resolve '{rev}' — check the branch, tag or commit name and try again.") + })?; + object + .peel_to_tree() + .with_context(|| format!("'{rev}' does not name a commit or tree.")) +} + +/// Shorten a hex OID for display, leaving anything already short untouched. +pub(crate) fn short_oid(oid_hex: &str) -> String { + oid_hex[..7.min(oid_hex.len())].to_string() +} + +/// The contents of one working-tree file before an apply or revert rewrote it. +/// +/// Apply and revert edit files on disk, unlike staging, so every operation +/// snapshots what it is about to overwrite. Undo restores these bytes verbatim +/// rather than re-deriving a reverse patch, so it is exact even when the forward +/// operation went through a three-way merge. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct WorktreeFileSnapshot { + /// Path relative to the worktree root. + pub path: PathBuf, + /// Contents before the operation; `None` when the file did not exist. + pub contents: Option>, +} + +/// Largest total snapshot the undo stack will hold for one operation. Past this +/// the operation still runs, but it is not offered as undoable rather than +/// parking tens of megabytes in a 20-deep history. +pub const MAX_UNDO_SNAPSHOT_BYTES: usize = 4 * 1024 * 1024; + +/// Whether `snapshots` are small enough to keep in the undo stack. +/// +/// Pure so the cap is testable without a repository. +pub fn snapshots_fit_undo_stack(snapshots: &[WorktreeFileSnapshot]) -> bool { + snapshots + .iter() + .filter_map(|s| s.contents.as_ref().map(Vec::len)) + .sum::() + <= MAX_UNDO_SNAPSHOT_BYTES +} + +/// Outcome of a successful working-tree apply or revert. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct WorktreePatchOutcome { + /// Pre-operation contents of every file the operation rewrote. + pub snapshots: Vec, + /// True when the change could not be dropped in verbatim and was merged + /// around unrelated local edits. Worth telling the user about: their file + /// now holds both their edit and the applied change. + pub merged_with_local_changes: bool, +} + +// ── Pure line rewriting ─────────────────────────────────────────────────────── + +/// Split `text` into lines, keeping each line's terminator so a file with no +/// trailing newline round-trips unchanged. +pub(crate) fn split_keeping_terminators(text: &str) -> Vec { + let mut lines = Vec::new(); + let mut current = String::new(); + for ch in text.chars() { + current.push(ch); + if ch == '\n' { + lines.push(std::mem::take(&mut current)); + } + } + if !current.is_empty() { + lines.push(current); + } + lines +} + +/// One line of a hunk, reduced to what the rewrite needs. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum ScopedLine { + Context { + old_lineno: usize, + new_lineno: usize, + }, + Addition { + new_lineno: usize, + text: String, + }, + Deletion { + old_lineno: usize, + text: String, + }, +} + +impl ScopedLine { + /// This line's position on `direction`'s base side, or `None` when it only + /// exists on the target side. + fn base_lineno(&self, direction: WorktreePatchDirection) -> Option { + match (self, direction) { + (ScopedLine::Context { old_lineno, .. }, WorktreePatchDirection::Apply) => { + Some(*old_lineno) + } + (ScopedLine::Context { new_lineno, .. }, WorktreePatchDirection::Revert) => { + Some(*new_lineno) + } + (ScopedLine::Deletion { old_lineno, .. }, WorktreePatchDirection::Apply) => { + Some(*old_lineno) + } + (ScopedLine::Addition { new_lineno, .. }, WorktreePatchDirection::Revert) => { + Some(*new_lineno) + } + _ => None, + } + } + + fn is_context(&self) -> bool { + matches!(self, ScopedLine::Context { .. }) + } +} + +/// Rewrite `base` — the file as it stands on `direction`'s starting side — into +/// the side the operation moves toward, covering only what the scope selects. +/// +/// `hunks` is the file's diff, hunk by hunk, in display order. `selected_hunks` +/// is `None` for "every hunk"; `selected_lines` is `None` for "every change line +/// of the selected hunks" and otherwise the `(old, new)` pairs the viewer +/// emitted. +/// +/// Within a run of consecutive `-`/`+` lines the *i*-th removal is treated as +/// paired with the *i*-th insertion, matching what the side-by-side view shows +/// the user. Git's unified output groups all removals ahead of all insertions, +/// so reading the rows in order would put a kept line on the wrong side of an +/// applied one when only part of a run is selected. +/// +/// Per pair the rule is uniform in both directions: emit the target-side line +/// when its row is selected, and keep the base-side line when its row is not. +/// Selecting both sides of a substitution therefore replaces the line, selecting +/// neither leaves it alone, selecting only the insertion adds a line beside the +/// original, and selecting only the removal drops it. +/// +/// Pure: no repository, no filesystem, no GPUI, and both directions run the same +/// walk with the two sides swapped, so an apply and a revert can never disagree +/// about which lines they touch. +pub(crate) fn rewrite_side( + base: &[String], + hunks: &[Vec], + direction: WorktreePatchDirection, + selected_hunks: Option<&HashSet>, + selected_lines: Option<&SelectedLines>, +) -> String { + /// Emit base lines from `cursor` up to (but not including) `upto`. + fn copy_base_through<'a>( + pieces: &mut Vec<&'a str>, + base: &'a [String], + cursor: &mut usize, + upto: usize, + ) { + while *cursor < upto && *cursor <= base.len() { + pieces.push(&base[*cursor - 1]); + *cursor += 1; + } + } + + let mut pieces: Vec<&str> = Vec::new(); + // Next base line number not yet emitted, 1-based. + let mut cursor = 1usize; + + for (hunk_index, lines) in hunks.iter().enumerate() { + let hunk_selected = selected_hunks.is_none_or(|set| set.contains(&hunk_index)); + let is_selected = |line: &ScopedLine| -> bool { + if !hunk_selected { + return false; + } + match (line, selected_lines) { + (_, None) => true, + (ScopedLine::Addition { new_lineno, .. }, Some(selected)) => { + selected.contains_addition(*new_lineno) + } + (ScopedLine::Deletion { old_lineno, .. }, Some(selected)) => { + selected.contains_deletion(*old_lineno) + } + (ScopedLine::Context { .. }, Some(_)) => false, + } + }; + + // Copy the untouched region ahead of this hunk. + if let Some(first) = lines.iter().filter_map(|l| l.base_lineno(direction)).min() { + copy_base_through(&mut pieces, base, &mut cursor, first); + } + + let mut index = 0; + while index < lines.len() { + if lines[index].is_context() { + if let Some(lineno) = lines[index].base_lineno(direction) { + copy_base_through(&mut pieces, base, &mut cursor, lineno); + if lineno <= base.len() { + pieces.push(&base[lineno - 1]); + } + cursor = lineno + 1; + } + index += 1; + continue; + } + + // A run of consecutive change lines, split into the two sides. + let run_end = lines[index..] + .iter() + .position(ScopedLine::is_context) + .map(|offset| index + offset) + .unwrap_or(lines.len()); + let run = &lines[index..run_end]; + let base_side: Vec<&ScopedLine> = run + .iter() + .filter(|line| line.base_lineno(direction).is_some()) + .collect(); + let target_side: Vec<&ScopedLine> = run + .iter() + .filter(|line| line.base_lineno(direction).is_none()) + .collect(); + + for pair_index in 0..base_side.len().max(target_side.len()) { + let base_line = base_side.get(pair_index); + let target_line = target_side.get(pair_index); + let kept_base = base_line + .filter(|line| !is_selected(line)) + .and_then(|line| line.base_lineno(direction)) + .filter(|lineno| *lineno <= base.len()) + .map(|lineno| base[lineno - 1].as_str()); + let applied_target = + target_line + .filter(|line| is_selected(line)) + .and_then(|line| match line { + ScopedLine::Addition { text, .. } + | ScopedLine::Deletion { text, .. } => Some(text.as_str()), + ScopedLine::Context { .. } => None, + }); + + // Unified diffs list removals before insertions, so applying + // emits the kept base line first and reverting emits the + // restored line first. + let ordered = match direction { + WorktreePatchDirection::Apply => [kept_base, applied_target], + WorktreePatchDirection::Revert => [applied_target, kept_base], + }; + pieces.extend(ordered.into_iter().flatten()); + } + + if let Some(last) = base_side + .iter() + .filter_map(|line| line.base_lineno(direction)) + .max() + { + cursor = last + 1; + } + index = run_end; + } + } + + copy_base_through(&mut pieces, base, &mut cursor, base.len() + 1); + + // Only the last piece may lack a newline: any earlier one that arrives + // without one gains it, so the other side's unterminated final line landing + // mid-file cannot glue two lines together. + let count = pieces.len(); + let mut out = String::new(); + for (index, piece) in pieces.into_iter().enumerate() { + out.push_str(piece); + if !piece.ends_with('\n') && index + 1 < count { + out.push('\n'); + } + } + out +} + +/// The viewer's line selection, split into the two sides it targets. +/// +/// Additions are matched on their new-side line number and deletions on their +/// old-side one, kept apart so an addition and a deletion that happen to share +/// a number are never confused. +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub(crate) struct SelectedLines { + additions: HashSet, + deletions: HashSet, +} + +impl SelectedLines { + pub(crate) fn from_pairs(pairs: &[(Option, Option)]) -> Self { + let mut selected = Self::default(); + for (old, new) in pairs { + match (old, new) { + (None, Some(new)) => { + selected.additions.insert(*new); + } + (Some(old), None) => { + selected.deletions.insert(*old); + } + // A context pair carries both numbers and a `(None, None)` row + // carries neither; neither selects a change line. + _ => {} + } + } + selected + } + + fn contains_addition(&self, new_lineno: usize) -> bool { + self.additions.contains(&new_lineno) + } + + fn contains_deletion(&self, old_lineno: usize) -> bool { + self.deletions.contains(&old_lineno) + } + + fn is_empty(&self) -> bool { + self.additions.is_empty() && self.deletions.is_empty() + } +} + +// ── Repository-facing work ──────────────────────────────────────────────────── + +/// One file's diff between the source's two trees, plus the blob on each side. +struct FileDiffSides { + hunks: Vec>, + /// Blob on the diff's old side, `None` when the file was added. + old_blob: Option, + /// Blob on the diff's new side, `None` when the file was deleted. + new_blob: Option, +} + +/// Read `file_path`'s diff out of `source` as hunks of [`ScopedLine`]s. +/// +/// The diff is computed with default options so hunk indices line up with the +/// `FileDiff` the viewer is displaying, which came from the same tree pair via +/// `parse_multi_file_diff`. +fn scoped_hunks( + repo: &Repository, + source: &WorktreePatchSource, + file_path: &Path, +) -> Result { + let (from_tree, to_tree) = source.trees(repo)?; + let diff = repo.diff_tree_to_tree(from_tree.as_ref(), to_tree.as_ref(), None)?; + + for delta_index in 0..diff.deltas().len() { + let patch = match git2::Patch::from_diff(&diff, delta_index) { + Ok(Some(patch)) => patch, + _ => continue, + }; + let old_path = patch.delta().old_file().path().map(Path::to_path_buf); + let new_path = patch.delta().new_file().path().map(Path::to_path_buf); + if old_path.as_deref() != Some(file_path) && new_path.as_deref() != Some(file_path) { + continue; + } + + let mut hunks = Vec::with_capacity(patch.num_hunks()); + for hunk_index in 0..patch.num_hunks() { + let mut lines = Vec::new(); + for line_index in 0..patch.num_lines_in_hunk(hunk_index)? { + let line = patch.line_in_hunk(hunk_index, line_index)?; + let text = String::from_utf8_lossy(line.content()).to_string(); + match line.origin() { + ' ' => { + if let (Some(old), Some(new)) = (line.old_lineno(), line.new_lineno()) { + lines.push(ScopedLine::Context { + old_lineno: old as usize, + new_lineno: new as usize, + }); + } + } + '+' => { + if let Some(new) = line.new_lineno() { + lines.push(ScopedLine::Addition { + new_lineno: new as usize, + text, + }); + } + } + '-' => { + if let Some(old) = line.old_lineno() { + lines.push(ScopedLine::Deletion { + old_lineno: old as usize, + text, + }); + } + } + // '=', '>' and '<' carry the "\ No newline at end of file" + // note, which annotates the preceding line rather than + // being a line of its own. The preceding line's content + // already lacks its terminator, so drop these. + _ => {} + } + } + hunks.push(lines); + } + + let old_blob = patch.delta().old_file().id(); + let new_blob = patch.delta().new_file().id(); + return Ok(FileDiffSides { + hunks, + old_blob: (!old_blob.is_zero()).then_some(old_blob), + new_blob: (!new_blob.is_zero()).then_some(new_blob), + }); + } + + anyhow::bail!( + "{} is unchanged between {}, so there is nothing to apply or revert. Pick a file that \ + differs between them.", + file_path.display(), + source.label() + ) +} + +fn blob_text(repo: &Repository, blob: Option) -> Result { + match blob { + None => Ok(String::new()), + Some(oid) => { + let blob = repo.find_blob(oid)?; + Ok(String::from_utf8_lossy(blob.content()).to_string()) + } + } +} + +/// Apply or revert part of `source`'s diff for one file in `worktree_path`. +/// +/// Returns the pre-operation snapshot so the caller can offer undo. Every error +/// is a sentence naming the file, the reason and what to do next. +pub fn apply_worktree_patch( + worktree_path: &Path, + file_path: &Path, + source: &WorktreePatchSource, + scope: &WorktreePatchScope, + direction: WorktreePatchDirection, +) -> Result { + let repo = Repository::open(worktree_path).with_context(|| { + format!( + "Failed to open the repository at {}", + worktree_path.display() + ) + })?; + let workdir = repo + .workdir() + .ok_or_else(|| { + anyhow::anyhow!( + "This is a bare repository, so there is no working tree to {} into.", + direction.verb() + ) + })? + .to_path_buf(); + + let FileDiffSides { + hunks, + old_blob, + new_blob, + } = scoped_hunks(&repo, source, file_path)?; + + let selected_hunks = match scope { + WorktreePatchScope::File | WorktreePatchScope::Lines(_) => None, + WorktreePatchScope::Hunk(index) => { + if *index >= hunks.len() { + anyhow::bail!( + "Hunk {} is no longer part of {}'s diff — reselect the hunk and try again.", + index + 1, + file_path.display() + ); + } + Some(HashSet::from([*index])) + } + }; + let selected_lines = match scope { + WorktreePatchScope::File | WorktreePatchScope::Hunk(_) => None, + WorktreePatchScope::Lines(pairs) => { + let selected = SelectedLines::from_pairs(pairs); + if selected.is_empty() { + anyhow::bail!( + "No added or removed lines are selected, so there is nothing to {}. Select \ + the lines you want first.", + direction.verb() + ); + } + Some(selected) + } + }; + + let (base_blob, target_side_blob) = match direction { + WorktreePatchDirection::Apply => (old_blob, new_blob), + WorktreePatchDirection::Revert => (new_blob, old_blob), + }; + + let base_text = blob_text(&repo, base_blob)?; + let base_lines = split_keeping_terminators(&base_text); + let target_text = rewrite_side( + &base_lines, + &hunks, + direction, + selected_hunks.as_ref(), + selected_lines.as_ref(), + ); + + if target_text == base_text { + anyhow::bail!( + "None of the selected lines are part of {}'s diff against {} — reselect them and \ + try again.", + file_path.display(), + source.label() + ); + } + + let absolute = workdir.join(file_path); + + // The whole file moving to a side that does not have it is a deletion, not + // an empty file. Only the file-level scope can express that. + let deletes_file = matches!(scope, WorktreePatchScope::File) + && target_side_blob.is_none() + && target_text.is_empty(); + + let existing = match std::fs::read(&absolute) { + Ok(bytes) => Some(bytes), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, + Err(error) => { + return Err(error).with_context(|| format!("Failed to read {}", absolute.display())) + } + }; + let existing_text = existing + .as_deref() + .map(|bytes| String::from_utf8_lossy(bytes).to_string()); + let snapshot = vec![WorktreeFileSnapshot { + path: file_path.to_path_buf(), + contents: existing.clone(), + }]; + + if deletes_file { + let Some(existing_text) = existing_text.as_deref() else { + anyhow::bail!( + "{} is already absent from the working tree, so there is nothing to {}.", + file_path.display(), + direction.verb() + ); + }; + if existing_text != base_text { + anyhow::bail!( + "Can't {} the deletion of {}: the file has uncommitted edits that would be lost. \ + Commit or stash them, then try again.", + direction.verb(), + file_path.display() + ); + } + std::fs::remove_file(&absolute) + .with_context(|| format!("Failed to delete {}", absolute.display()))?; + return Ok(WorktreePatchOutcome { + snapshots: snapshot, + merged_with_local_changes: false, + }); + } + + let ours_text = existing_text.clone().unwrap_or_default(); + + // Clean case: the working tree still matches the side the patch starts + // from, so the rewrite is exact and no merge is needed. + let merged_with_local_changes = ours_text != base_text; + let merged_text = if merged_with_local_changes { + match three_way_merge(&repo, file_path, &base_text, &ours_text, &target_text)? { + Some(merged) => merged, + None => { + anyhow::bail!(conflict_message(&repo, file_path, scope, source, direction)); + } + } + } else { + target_text + }; + + // The merge folding back to what is already on disk means the selected + // change is already present (or already gone). Reporting that beats writing + // the same bytes and claiming to have done something — and it is the general + // test, since a scope covering part of a file leaves the rest of `target` + // disagreeing with the working tree for reasons that are none of its + // business. + if merged_text == ours_text { + anyhow::bail!(already_applied_message(file_path, scope, source, direction)); + } + + if let Some(parent) = absolute.parent() { + std::fs::create_dir_all(parent) + .with_context(|| format!("Failed to create {}", parent.display()))?; + } + std::fs::write(&absolute, merged_text.as_bytes()) + .with_context(|| format!("Failed to write {}", absolute.display()))?; + + Ok(WorktreePatchOutcome { + snapshots: snapshot, + merged_with_local_changes, + }) +} + +/// Restore files to the contents captured before an apply or revert. +pub fn restore_worktree_files( + worktree_path: &Path, + snapshots: &[WorktreeFileSnapshot], +) -> Result<()> { + let repo = Repository::open(worktree_path).with_context(|| { + format!( + "Failed to open the repository at {}", + worktree_path.display() + ) + })?; + let workdir = repo + .workdir() + .ok_or_else(|| anyhow::anyhow!("This is a bare repository, so it has no working tree."))? + .to_path_buf(); + + for snapshot in snapshots { + let absolute = workdir.join(&snapshot.path); + match &snapshot.contents { + Some(bytes) => { + if let Some(parent) = absolute.parent() { + std::fs::create_dir_all(parent) + .with_context(|| format!("Failed to create {}", parent.display()))?; + } + std::fs::write(&absolute, bytes) + .with_context(|| format!("Failed to restore {}", absolute.display()))?; + } + None => match std::fs::remove_file(&absolute) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => { + return Err(error) + .with_context(|| format!("Failed to remove {}", absolute.display())) + } + }, + } + } + + Ok(()) +} + +/// Three-way merge three versions of one file's text. +/// +/// Returns `None` when the merge would need conflict markers. libgit2's file +/// merge works on blobs, so the three versions are written to the object +/// database first; they are unreferenced and collected by the next `git gc`. +fn three_way_merge( + repo: &Repository, + file_path: &Path, + ancestor: &str, + ours: &str, + theirs: &str, +) -> Result> { + let path_bytes = file_path.to_string_lossy().replace('\\', "/").into_bytes(); + let entry = |text: &str| -> Result { + Ok(IndexEntry { + ctime: IndexTime::new(0, 0), + mtime: IndexTime::new(0, 0), + dev: 0, + ino: 0, + mode: 0o100644, + uid: 0, + gid: 0, + file_size: text.len() as u32, + id: repo.blob(text.as_bytes())?, + flags: 0, + flags_extended: 0, + path: path_bytes.clone(), + }) + }; + + // Default (non-diff3) conflict style: we never keep a conflicted result, so + // the markers only ever feed `is_automergeable()`. + let mut options = git2::MergeFileOptions::new(); + options.style_standard(true); + let result = repo.merge_file_from_index( + &entry(ancestor)?, + &entry(ours)?, + &entry(theirs)?, + Some(&mut options), + )?; + + if !result.is_automergeable() { + return Ok(None); + } + Ok(Some(String::from_utf8_lossy(result.content()).to_string())) +} + +// ── Error messages ──────────────────────────────────────────────────────────── + +fn already_applied_message( + file_path: &Path, + scope: &WorktreePatchScope, + source: &WorktreePatchSource, + direction: WorktreePatchDirection, +) -> String { + match direction { + WorktreePatchDirection::Apply => format!( + "{} already matches {} for {} — there is nothing to apply. Press r to revert it \ + instead.", + file_path.display(), + source.label(), + scope.describe() + ), + WorktreePatchDirection::Revert => format!( + "{}'s {} from {} is already absent from the working tree — there is nothing to \ + revert. Press a to apply it instead.", + file_path.display(), + scope.describe(), + source.label() + ), + } +} + +fn conflict_message( + repo: &Repository, + file_path: &Path, + scope: &WorktreePatchScope, + source: &WorktreePatchSource, + direction: WorktreePatchDirection, +) -> String { + let dirty = repo + .status_file(file_path) + .map(|status| { + status.intersects( + git2::Status::WT_MODIFIED + | git2::Status::WT_NEW + | git2::Status::WT_TYPECHANGE + | git2::Status::INDEX_MODIFIED + | git2::Status::INDEX_NEW, + ) + }) + .unwrap_or(false); + + if dirty { + // A conflicting local edit: the user has uncommitted work in the same + // place. Naming the file tells them exactly what to park. + format!( + "Can't {} {} of {}: your uncommitted edits to that file overlap it. Commit or stash \ + {}, then try again.", + direction.verb(), + scope.describe(), + file_path.display(), + file_path.display() + ) + } else { + // A context mismatch: the file is clean, so the patch does not fit + // because the file itself has moved on since that revision. + format!( + "Can't {} {} of {}: the surrounding lines have changed since {}, so the patch no \ + longer fits. {} the whole file from the file menu to take that revision's version \ + wholesale.", + direction.verb(), + scope.describe(), + file_path.display(), + source.label(), + match direction { + WorktreePatchDirection::Apply => "Apply", + WorktreePatchDirection::Revert => "Revert", + } + ) + } +} + +// ── GitProject operations ───────────────────────────────────────────────────── + +use gpui::{AsyncApp, Context, Task, WeakEntity}; + +use super::refresh::gather_refresh_data_lightweight_cached; +use super::{GitProject, GitProjectEvent, RefreshData}; +use crate::types::GitOperationKind; + +impl GitProject { + /// Apply or revert part of another revision's diff in `worktree_path`. + /// + /// The heavy work — resolving trees, rewriting lines, merging and writing — + /// happens on the background executor; only the refresh snapshot and the + /// events come back to the UI thread. + #[allow(clippy::too_many_arguments)] + pub fn patch_worktree_at( + &mut self, + file_path: &Path, + source: WorktreePatchSource, + scope: WorktreePatchScope, + direction: WorktreePatchDirection, + worktree_path: &Path, + cx: &mut Context, + ) -> Task> { + let file_path = file_path.to_path_buf(); + let task_file_path = file_path.clone(); + let task_worktree_path = worktree_path.to_path_buf(); + let refresh_repo_path = self.repo_path.clone(); + let worktree_cache = self.worktree_status_cache.clone(); + let author_filter = self.commit_author_filter.clone(); + let commit_limit = self.commit_limit; + let branch_name = self.head_branch.clone(); + let kind = match direction { + WorktreePatchDirection::Apply => GitOperationKind::ApplyToWorktree, + WorktreePatchDirection::Revert => GitOperationKind::RevertInWorktree, + }; + let scope_text = scope.describe(); + let source_label = source.label(); + let operation_id = self.begin_operation( + kind, + format!( + "{} {} of {} from {}...", + direction.present_participle(), + scope_text, + file_path.display(), + source_label + ), + None, + branch_name.clone(), + cx, + ); + let task_source = source.clone(); + let task_scope = scope.clone(); + + cx.spawn(async move |this: WeakEntity, cx: &mut AsyncApp| { + let result: anyhow::Result<(WorktreePatchOutcome, RefreshData)> = cx + .background_executor() + .spawn(async move { + let outcome = apply_worktree_patch( + &task_worktree_path, + &task_file_path, + &task_source, + &task_scope, + direction, + )?; + let data = gather_refresh_data_lightweight_cached( + &refresh_repo_path, + commit_limit, + &worktree_cache, + author_filter.as_deref(), + )?; + Ok((outcome, data)) + }) + .await; + + cx.update(|cx| { + this.update(cx, |this, cx| { + match result { + Ok((outcome, data)) => { + this.apply_refresh_data(data); + let mut summary = format!( + "{} {} of {} from {}", + direction.past_tense(), + scope_text, + file_path.display(), + source_label + ); + if outcome.merged_with_local_changes { + summary.push_str( + " — merged around your uncommitted edits to that file", + ); + } + this.complete_op( + operation_id, + kind, + summary.clone(), + (None, None, branch_name.clone()), + cx, + ); + if snapshots_fit_undo_stack(&outcome.snapshots) { + cx.emit(GitProjectEvent::WorktreePatchApplied { + label: summary, + snapshots: outcome.snapshots, + }); + } + cx.emit(GitProjectEvent::StatusChanged); + } + Err(e) => { + this.fail_op( + operation_id, + kind, + format!("{} failed", kind.display_name()), + e.to_string(), + (None, branch_name.clone(), false), + cx, + ); + } + } + cx.notify(); + Ok(()) + }) + })? + }) + } + + /// Restore working-tree files to a snapshot taken before an apply or revert. + pub fn restore_worktree_files_at( + &mut self, + snapshots: Vec, + worktree_path: &Path, + cx: &mut Context, + ) -> Task> { + let task_worktree_path = worktree_path.to_path_buf(); + let refresh_repo_path = self.repo_path.clone(); + let worktree_cache = self.worktree_status_cache.clone(); + let author_filter = self.commit_author_filter.clone(); + let commit_limit = self.commit_limit; + let branch_name = self.head_branch.clone(); + let file_count = snapshots.len(); + let operation_id = self.begin_operation( + GitOperationKind::Discard, + format!( + "Restoring {} file{}...", + file_count, + if file_count == 1 { "" } else { "s" } + ), + None, + branch_name.clone(), + cx, + ); + + cx.spawn(async move |this: WeakEntity, cx: &mut AsyncApp| { + let result: anyhow::Result = cx + .background_executor() + .spawn(async move { + restore_worktree_files(&task_worktree_path, &snapshots)?; + gather_refresh_data_lightweight_cached( + &refresh_repo_path, + commit_limit, + &worktree_cache, + author_filter.as_deref(), + ) + }) + .await; + + cx.update(|cx| { + this.update(cx, |this, cx| { + match result { + Ok(data) => { + this.apply_refresh_data(data); + this.complete_op( + operation_id, + GitOperationKind::Discard, + format!( + "Restored {} file{}", + file_count, + if file_count == 1 { "" } else { "s" } + ), + (None, None, branch_name.clone()), + cx, + ); + cx.emit(GitProjectEvent::StatusChanged); + } + Err(e) => { + this.fail_op( + operation_id, + GitOperationKind::Discard, + "Restore failed", + e.to_string(), + (None, branch_name.clone(), false), + cx, + ); + } + } + cx.notify(); + Ok(()) + }) + })? + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn context(old: usize, new: usize) -> ScopedLine { + ScopedLine::Context { + old_lineno: old, + new_lineno: new, + } + } + + fn addition(new: usize, text: &str) -> ScopedLine { + ScopedLine::Addition { + new_lineno: new, + text: text.to_string(), + } + } + + fn deletion(old: usize, text: &str) -> ScopedLine { + ScopedLine::Deletion { + old_lineno: old, + text: text.to_string(), + } + } + + // ── split_keeping_terminators ───────────────────────────────── + + #[test] + fn split_keeps_line_terminators() { + assert_eq!(split_keeping_terminators("a\nb\n"), vec!["a\n", "b\n"]); + } + + #[test] + fn split_keeps_a_final_line_without_a_newline() { + assert_eq!(split_keeping_terminators("a\nb"), vec!["a\n", "b"]); + } + + #[test] + fn split_of_empty_text_has_no_lines() { + assert!(split_keeping_terminators("").is_empty()); + } + + // ── rewrite_side ────────────────────────────────────────────── + + fn rewrite( + base: &str, + hunks: &[Vec], + direction: WorktreePatchDirection, + selected_hunks: Option<&HashSet>, + selected_lines: Option<&SelectedLines>, + ) -> String { + rewrite_side( + &split_keeping_terminators(base), + hunks, + direction, + selected_hunks, + selected_lines, + ) + } + + /// `b` → `B2` as one hunk of a three-line file. + fn substitution_hunk() -> Vec> { + vec![vec![ + context(1, 1), + deletion(2, "b\n"), + addition(2, "B2\n"), + context(3, 3), + ]] + } + + #[test] + fn applying_a_substitution_replaces_the_base_line() { + let hunks = substitution_hunk(); + let out = rewrite( + "a\nb\nc\n", + &hunks, + WorktreePatchDirection::Apply, + None, + None, + ); + assert_eq!(out, "a\nB2\nc\n"); + } + + #[test] + fn reverting_a_substitution_restores_the_old_line() { + let hunks = substitution_hunk(); + let out = rewrite( + "a\nB2\nc\n", + &hunks, + WorktreePatchDirection::Revert, + None, + None, + ); + assert_eq!(out, "a\nb\nc\n"); + } + + #[test] + fn apply_then_revert_round_trips() { + let hunks = substitution_hunk(); + let original = "a\nb\nc\n"; + let applied = rewrite(original, &hunks, WorktreePatchDirection::Apply, None, None); + let reverted = rewrite(&applied, &hunks, WorktreePatchDirection::Revert, None, None); + assert_eq!(reverted, original); + } + + #[test] + fn a_hunk_scope_leaves_other_hunks_alone() { + // Two independent substitutions, far enough apart to be separate hunks. + let hunks = vec![ + vec![ + context(1, 1), + deletion(2, "b\n"), + addition(2, "B2\n"), + context(3, 3), + ], + vec![ + context(5, 5), + deletion(6, "f\n"), + addition(6, "F2\n"), + context(7, 7), + ], + ]; + let only_second = HashSet::from([1usize]); + let out = rewrite( + "a\nb\nc\nd\ne\nf\ng\n", + &hunks, + WorktreePatchDirection::Apply, + Some(&only_second), + None, + ); + assert_eq!(out, "a\nb\nc\nd\ne\nF2\ng\n"); + } + + /// Two adjacent substitutions with no context between them, which is how + /// git emits them: both removals, then both insertions. + fn adjacent_substitutions() -> Vec> { + vec![vec![ + context(1, 1), + deletion(2, "b\n"), + deletion(3, "c\n"), + addition(2, "B2\n"), + addition(3, "C2\n"), + context(4, 4), + ]] + } + + #[test] + fn a_line_scope_rewrites_only_the_selected_lines() { + let hunks = adjacent_substitutions(); + // The second substitution: old line 3 removed, new line 3 added. + let selected = SelectedLines::from_pairs(&[(Some(3), None), (None, Some(3))]); + let out = rewrite( + "a\nb\nc\nd\n", + &hunks, + WorktreePatchDirection::Apply, + None, + Some(&selected), + ); + assert_eq!(out, "a\nb\nC2\nd\n"); + } + + #[test] + fn a_line_scope_keeps_an_unselected_line_in_its_own_place() { + // Selecting the *first* substitution of a run is where reading the + // unified rows in order goes wrong: it would emit the kept `c` before + // the applied `B2`. + let hunks = adjacent_substitutions(); + let selected = SelectedLines::from_pairs(&[(Some(2), None), (None, Some(2))]); + let out = rewrite( + "a\nb\nc\nd\n", + &hunks, + WorktreePatchDirection::Apply, + None, + Some(&selected), + ); + assert_eq!(out, "a\nB2\nc\nd\n"); + } + + #[test] + fn reverting_the_first_of_two_adjacent_substitutions_keeps_the_order() { + let hunks = adjacent_substitutions(); + let selected = SelectedLines::from_pairs(&[(Some(2), None), (None, Some(2))]); + let out = rewrite( + "a\nB2\nC2\nd\n", + &hunks, + WorktreePatchDirection::Revert, + None, + Some(&selected), + ); + assert_eq!(out, "a\nb\nC2\nd\n"); + } + + #[test] + fn selecting_only_an_addition_leaves_the_deletion_in_place() { + let hunks = substitution_hunk(); + let selected = SelectedLines::from_pairs(&[(None, Some(2))]); + let out = rewrite( + "a\nb\nc\n", + &hunks, + WorktreePatchDirection::Apply, + None, + Some(&selected), + ); + assert_eq!(out, "a\nb\nB2\nc\n"); + } + + #[test] + fn selecting_only_a_deletion_drops_the_line() { + let hunks = substitution_hunk(); + let selected = SelectedLines::from_pairs(&[(Some(2), None)]); + let out = rewrite( + "a\nb\nc\n", + &hunks, + WorktreePatchDirection::Apply, + None, + Some(&selected), + ); + assert_eq!(out, "a\nc\n"); + } + + #[test] + fn selection_pairs_ignore_context_and_empty_rows() { + let selected = SelectedLines::from_pairs(&[(Some(1), Some(1)), (None, None)]); + assert!(selected.is_empty()); + } + + #[test] + fn pure_insertion_at_the_top_of_a_file_lands_first() { + let hunks = vec![vec![addition(1, "new\n"), context(1, 2)]]; + let out = rewrite("a\n", &hunks, WorktreePatchDirection::Apply, None, None); + assert_eq!(out, "new\na\n"); + } + + #[test] + fn insertion_without_a_trailing_newline_gains_one_when_lines_follow() { + // The other side's last line becoming a middle line must not glue two + // lines together. + let hunks = vec![vec![context(1, 1), addition(2, "tail"), context(2, 3)]]; + let out = rewrite("a\nb\n", &hunks, WorktreePatchDirection::Apply, None, None); + assert_eq!(out, "a\ntail\nb\n"); + } + + #[test] + fn a_final_line_keeps_its_missing_newline() { + let hunks = vec![vec![context(1, 1), deletion(2, "b"), addition(2, "B2")]]; + let out = rewrite("a\nb", &hunks, WorktreePatchDirection::Apply, None, None); + assert_eq!(out, "a\nB2"); + } + + #[test] + fn deleting_every_line_yields_empty_text() { + let hunks = vec![vec![deletion(1, "a\n"), deletion(2, "b\n")]]; + let out = rewrite("a\nb\n", &hunks, WorktreePatchDirection::Apply, None, None); + assert_eq!(out, ""); + } + + #[test] + fn an_unselected_hunk_contributes_its_base_lines_verbatim() { + let hunks = substitution_hunk(); + let nothing = HashSet::new(); + let out = rewrite( + "a\nb\nc\n", + &hunks, + WorktreePatchDirection::Apply, + Some(¬hing), + None, + ); + assert_eq!(out, "a\nb\nc\n"); + } + + // ── snapshot cap ────────────────────────────────────────────── + + #[test] + fn small_snapshots_fit_the_undo_stack() { + let snapshots = vec![WorktreeFileSnapshot { + path: PathBuf::from("a.txt"), + contents: Some(vec![0; 1024]), + }]; + assert!(snapshots_fit_undo_stack(&snapshots)); + } + + #[test] + fn oversized_snapshots_do_not_fit_the_undo_stack() { + let snapshots = vec![WorktreeFileSnapshot { + path: PathBuf::from("a.txt"), + contents: Some(vec![0; MAX_UNDO_SNAPSHOT_BYTES + 1]), + }]; + assert!(!snapshots_fit_undo_stack(&snapshots)); + } + + #[test] + fn a_deleted_file_snapshot_costs_nothing() { + let snapshots = vec![WorktreeFileSnapshot { + path: PathBuf::from("a.txt"), + contents: None, + }]; + assert!(snapshots_fit_undo_stack(&snapshots)); + } + + // ── labels ──────────────────────────────────────────────────── + + #[test] + fn scope_descriptions_read_as_prose() { + assert_eq!(WorktreePatchScope::File.describe(), "all changes"); + assert_eq!(WorktreePatchScope::Hunk(0).describe(), "hunk 1"); + assert_eq!( + WorktreePatchScope::Lines(vec![(None, Some(1))]).describe(), + "1 line" + ); + assert_eq!( + WorktreePatchScope::Lines(vec![(None, Some(1)), (Some(2), None)]).describe(), + "2 lines" + ); + } + + #[test] + fn a_commit_source_is_labelled_by_its_short_oid() { + let oid = Oid::from_str("1234567890abcdef1234567890abcdef12345678").unwrap(); + assert_eq!(WorktreePatchSource::Commit(oid).label(), "1234567"); + } + + #[test] + fn a_compare_source_is_labelled_by_both_endpoints() { + let source = WorktreePatchSource::Compare { + from: "main".to_string(), + to: "feature".to_string(), + }; + assert_eq!(source.label(), "main...feature"); + } + + #[test] + fn direction_inverts() { + assert_eq!( + WorktreePatchDirection::Apply.inverse(), + WorktreePatchDirection::Revert + ); + assert_eq!( + WorktreePatchDirection::Revert.inverse(), + WorktreePatchDirection::Apply + ); + } +} + +// ── Integration tests against real repositories ─────────────────────────────── +// +// Every assertion here reads the file back off disk. A test that only checked +// `is_ok()` would pass for a patch applied in the wrong direction, which is the +// one mistake this feature cannot afford. +#[cfg(test)] +mod worktree_patch_integration_tests { + use super::*; + use tempfile::TempDir; + + struct Fixture { + _dir: TempDir, + path: PathBuf, + repo: Repository, + } + + impl Fixture { + fn new() -> Self { + let dir = TempDir::new().unwrap(); + let path = dir.path().to_path_buf(); + let repo = Repository::init(&path).unwrap(); + let mut config = repo.config().unwrap(); + config.set_str("user.name", "Test").unwrap(); + config.set_str("user.email", "t@t.com").unwrap(); + // Keep line endings byte-exact so content assertions hold on + // Windows, where autocrlf would rewrite what we read back. + config.set_bool("core.autocrlf", false).unwrap(); + drop(config); + Self { + _dir: dir, + path, + repo, + } + } + + fn write(&self, name: &str, contents: &str) { + std::fs::write(self.path.join(name), contents).unwrap(); + } + + fn read(&self, name: &str) -> String { + std::fs::read_to_string(self.path.join(name)).unwrap() + } + + fn commit(&self, message: &str, files: &[&str]) -> Oid { + let signature = git2::Signature::now("Test", "t@t.com").unwrap(); + let mut index = self.repo.index().unwrap(); + for file in files { + index.add_path(Path::new(file)).unwrap(); + } + index.write().unwrap(); + let tree = self.repo.find_tree(index.write_tree().unwrap()).unwrap(); + let parents = match self.repo.head().ok().and_then(|h| h.peel_to_commit().ok()) { + Some(parent) => vec![parent], + None => Vec::new(), + }; + let parent_refs: Vec<&git2::Commit> = parents.iter().collect(); + self.repo + .commit( + Some("HEAD"), + &signature, + &signature, + message, + &tree, + &parent_refs, + ) + .unwrap() + } + + fn branch(&self, name: &str) { + let head = self.repo.head().unwrap().peel_to_commit().unwrap(); + self.repo.branch(name, &head, true).unwrap(); + } + + fn checkout(&self, name: &str) { + let reference = format!("refs/heads/{name}"); + let object = self.repo.revparse_single(&reference).unwrap(); + self.repo.checkout_tree(&object, None).unwrap(); + self.repo.set_head(&reference).unwrap(); + } + + fn head_branch_name(&self) -> String { + self.repo.head().unwrap().shorthand().unwrap().to_string() + } + } + + /// Numbered lines `l1..l20`, with `substitutions` replacing the given + /// 1-based line numbers. + fn numbered_lines(substitutions: &[(usize, &str)]) -> String { + (1..=20) + .map( + |lineno| match substitutions.iter().find(|(at, _)| *at == lineno) { + Some((_, text)) => format!("{text}\n"), + None => format!("l{lineno}\n"), + }, + ) + .collect() + } + + /// A repo whose HEAD commit changes lines 2 and 15 of a 20-line `f.txt`. + /// The two edits are more than twice the default context apart, so the diff + /// against the parent really has two hunks rather than one merged one. + fn two_hunk_commit() -> (Fixture, Oid) { + let fixture = Fixture::new(); + fixture.write("f.txt", &numbered_lines(&[])); + fixture.commit("base", &["f.txt"]); + fixture.write("f.txt", &numbered_lines(&[(2, "L2X"), (15, "L15X")])); + let oid = fixture.commit("two edits", &["f.txt"]); + // Leave the working tree matching the commit's parent so an apply has + // somewhere to land. + fixture.write("f.txt", &numbered_lines(&[])); + (fixture, oid) + } + + fn apply( + fixture: &Fixture, + file: &str, + source: &WorktreePatchSource, + scope: WorktreePatchScope, + direction: WorktreePatchDirection, + ) -> Result { + apply_worktree_patch(&fixture.path, Path::new(file), source, &scope, direction) + } + + // ── hunk apply / revert on a clean tree ─────────────────────── + + #[test] + fn applying_a_commits_hunk_writes_only_that_hunk_to_disk() { + let (fixture, oid) = two_hunk_commit(); + apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Apply, + ) + .unwrap(); + assert_eq!( + fixture.read("f.txt"), + numbered_lines(&[(2, "L2X")]), + "hunk 0 changes line 2 and must leave line 15 alone" + ); + } + + #[test] + fn applying_the_second_hunk_targets_the_second_change() { + let (fixture, oid) = two_hunk_commit(); + apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::Hunk(1), + WorktreePatchDirection::Apply, + ) + .unwrap(); + assert_eq!(fixture.read("f.txt"), numbered_lines(&[(15, "L15X")])); + } + + #[test] + fn reverting_a_hunk_restores_the_parents_line() { + let (fixture, oid) = two_hunk_commit(); + // Start from the committed content so there is something to revert. + fixture.write("f.txt", &numbered_lines(&[(2, "L2X"), (15, "L15X")])); + apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Revert, + ) + .unwrap(); + assert_eq!( + fixture.read("f.txt"), + numbered_lines(&[(15, "L15X")]), + "only the first hunk should be rolled back" + ); + } + + #[test] + fn applying_then_reverting_the_same_hunk_returns_the_original_bytes() { + let (fixture, oid) = two_hunk_commit(); + let original = fixture.read("f.txt"); + let source = WorktreePatchSource::Commit(oid); + apply( + &fixture, + "f.txt", + &source, + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Apply, + ) + .unwrap(); + apply( + &fixture, + "f.txt", + &source, + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Revert, + ) + .unwrap(); + assert_eq!(fixture.read("f.txt"), original); + } + + // ── whole file and line subsets ─────────────────────────────── + + #[test] + fn applying_the_whole_file_takes_every_hunk() { + let (fixture, oid) = two_hunk_commit(); + apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .unwrap(); + assert_eq!( + fixture.read("f.txt"), + numbered_lines(&[(2, "L2X"), (15, "L15X")]) + ); + } + + #[test] + fn applying_a_line_subset_rewrites_only_the_selected_lines() { + let fixture = Fixture::new(); + fixture.write("f.txt", "a\nb\nc\nd\n"); + fixture.commit("base", &["f.txt"]); + fixture.write("f.txt", "a\nB2\nC2\nd\n"); + let oid = fixture.commit("two lines", &["f.txt"]); + fixture.write("f.txt", "a\nb\nc\nd\n"); + + // Select only the `c` → `C2` substitution: old line 3 and new line 3. + apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::Lines(vec![(Some(3), None), (None, Some(3))]), + WorktreePatchDirection::Apply, + ) + .unwrap(); + assert_eq!(fixture.read("f.txt"), "a\nb\nC2\nd\n"); + } + + #[test] + fn reverting_a_line_subset_restores_only_the_selected_lines() { + let fixture = Fixture::new(); + fixture.write("f.txt", "a\nb\nc\nd\n"); + fixture.commit("base", &["f.txt"]); + fixture.write("f.txt", "a\nB2\nC2\nd\n"); + let oid = fixture.commit("two lines", &["f.txt"]); + + apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::Lines(vec![(Some(3), None), (None, Some(3))]), + WorktreePatchDirection::Revert, + ) + .unwrap(); + assert_eq!(fixture.read("f.txt"), "a\nB2\nc\nd\n"); + } + + // ── local edits ─────────────────────────────────────────────── + + #[test] + fn applying_over_an_unrelated_local_edit_keeps_both_changes() { + // The hunk changes line 2, so its trailing context runs to line 5. A + // local edit on line 5 is inside that context window, which is what + // makes a plain context-matching apply (libgit2's, and `git apply`'s) + // fail here. The three-way merge keeps both. + let (fixture, oid) = two_hunk_commit(); + fixture.write("f.txt", &numbered_lines(&[(5, "LOCAL5")])); + + let outcome = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Apply, + ) + .unwrap(); + + assert_eq!( + fixture.read("f.txt"), + numbered_lines(&[(2, "L2X"), (5, "LOCAL5")]), + "the applied hunk and the local edit must both survive" + ); + assert!( + outcome.merged_with_local_changes, + "the caller needs to know a merge happened so it can say so" + ); + } + + #[test] + fn libgit2s_own_apply_refuses_the_case_the_merge_handles() { + // The evidence behind not using `repo.apply(.., WorkDir, ..)`: the same + // hunk, as a patch, against the same working tree that + // `applying_over_an_unrelated_local_edit_keeps_both_changes` handles. + let (fixture, _) = two_hunk_commit(); + fixture.write("f.txt", &numbered_lines(&[(5, "LOCAL5")])); + let patch = "diff --git a/f.txt b/f.txt\n\ + --- a/f.txt\n\ + +++ b/f.txt\n\ + @@ -1,5 +1,5 @@\n\ + \x20l1\n\ + -l2\n\ + +L2X\n\ + \x20l3\n\ + \x20l4\n\ + \x20l5\n"; + let diff = git2::Diff::from_buffer(patch.as_bytes()).unwrap(); + fixture + .repo + .apply(&diff, git2::ApplyLocation::WorkDir, None) + .expect_err("libgit2 matches context literally, and line 5 no longer matches"); + assert_eq!( + fixture.read("f.txt"), + numbered_lines(&[(5, "LOCAL5")]), + "and it leaves the file alone, so there is nothing to fall back from" + ); + } + + #[test] + fn applying_over_an_overlapping_local_edit_is_refused_and_writes_nothing() { + let (fixture, oid) = two_hunk_commit(); + fixture.write("f.txt", &numbered_lines(&[(2, "MINE")])); + + let error = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Apply, + ) + .expect_err("an overlapping edit must not be silently merged"); + + let message = error.to_string(); + assert!( + message.contains("uncommitted edits"), + "expected the local-edit message, got: {message}" + ); + assert!( + message.contains("Commit or stash"), + "the message must say what to do, got: {message}" + ); + assert_eq!( + fixture.read("f.txt"), + numbered_lines(&[(2, "MINE")]), + "a refused apply must leave the file untouched" + ); + } + + #[test] + fn a_context_mismatch_on_a_clean_file_reports_the_revision_moving_on() { + // Commit a change, then commit again so the hunk's context no longer + // exists anywhere in the working tree. The file itself is clean. + let fixture = Fixture::new(); + fixture.write("f.txt", "a\nb\nc\n"); + fixture.commit("base", &["f.txt"]); + fixture.write("f.txt", "a\nB2\nc\n"); + let target = fixture.commit("change b", &["f.txt"]); + fixture.write("f.txt", "totally\ndifferent\nfile\n"); + fixture.commit("rewrite", &["f.txt"]); + + let error = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(target), + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Apply, + ) + .expect_err("the patch cannot fit"); + + let message = error.to_string(); + assert!( + message.contains("surrounding lines have changed"), + "expected the context-mismatch message, got: {message}" + ); + assert_eq!( + fixture.read("f.txt"), + "totally\ndifferent\nfile\n", + "a refused apply must leave the file untouched" + ); + } + + // ── already applied ─────────────────────────────────────────── + + #[test] + fn applying_an_already_applied_hunk_is_a_clear_error_not_a_silent_mess() { + let (fixture, oid) = two_hunk_commit(); + let source = WorktreePatchSource::Commit(oid); + apply( + &fixture, + "f.txt", + &source, + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Apply, + ) + .unwrap(); + let after_first = fixture.read("f.txt"); + + let error = apply( + &fixture, + "f.txt", + &source, + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Apply, + ) + .expect_err("a second apply has nothing to do"); + + let message = error.to_string(); + assert!( + message.contains("nothing to apply"), + "expected the already-applied message, got: {message}" + ); + assert!( + message.contains("press r to revert") || message.contains("Press r to revert"), + "the message must point at the way out, got: {message}" + ); + assert_eq!( + fixture.read("f.txt"), + after_first, + "the second attempt must not double-apply" + ); + } + + #[test] + fn reverting_something_already_absent_is_a_clear_error() { + let (fixture, oid) = two_hunk_commit(); + let error = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Revert, + ) + .expect_err("the change was never applied"); + + let message = error.to_string(); + assert!( + message.contains("nothing to \nrevert") || message.contains("nothing to revert"), + "expected the already-reverted message, got: {message}" + ); + } + + // ── cross-branch compare ────────────────────────────────────── + + /// Two branches that differ in `f.txt`, with `main` checked out. + fn two_branch_repo() -> Fixture { + let fixture = Fixture::new(); + fixture.write("f.txt", "shared\nours\nshared2\n"); + fixture.commit("base", &["f.txt"]); + let base_branch = fixture.head_branch_name(); + fixture.branch("feature"); + fixture.checkout("feature"); + fixture.write("f.txt", "shared\ntheirs\nshared2\n"); + fixture.commit("feature edit", &["f.txt"]); + fixture.checkout(&base_branch); + fixture.write("f.txt", "shared\nours\nshared2\n"); + fixture + } + + #[test] + fn applying_a_cross_branch_hunk_brings_the_other_branchs_content_in() { + let fixture = two_branch_repo(); + let base_branch = fixture.head_branch_name(); + apply( + &fixture, + "f.txt", + &WorktreePatchSource::Compare { + from: base_branch, + to: "feature".to_string(), + }, + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Apply, + ) + .unwrap(); + assert_eq!( + fixture.read("f.txt"), + "shared\ntheirs\nshared2\n", + "applying main→feature must pull feature's line into the working tree" + ); + } + + #[test] + fn the_compare_direction_decides_which_branch_wins() { + // Same two branches, endpoints swapped. Applying feature→main while on + // main is a no-op, which proves the patch really is generated from the + // endpoints and not from index→workdir. + let fixture = two_branch_repo(); + let base_branch = fixture.head_branch_name(); + let error = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Compare { + from: "feature".to_string(), + to: base_branch, + }, + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Apply, + ) + .expect_err("the working tree already holds main's content"); + assert!( + error.to_string().contains("nothing to apply"), + "got: {error}" + ); + assert_eq!(fixture.read("f.txt"), "shared\nours\nshared2\n"); + } + + #[test] + fn reverting_a_cross_branch_hunk_restores_this_branchs_content() { + let fixture = two_branch_repo(); + let base_branch = fixture.head_branch_name(); + let source = WorktreePatchSource::Compare { + from: base_branch, + to: "feature".to_string(), + }; + apply( + &fixture, + "f.txt", + &source, + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Apply, + ) + .unwrap(); + apply( + &fixture, + "f.txt", + &source, + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Revert, + ) + .unwrap(); + assert_eq!(fixture.read("f.txt"), "shared\nours\nshared2\n"); + } + + #[test] + fn a_compare_endpoint_that_does_not_resolve_names_itself() { + let fixture = two_branch_repo(); + let error = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Compare { + from: "no-such-branch".to_string(), + to: "feature".to_string(), + }, + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .expect_err("the endpoint does not exist"); + assert!(error.to_string().contains("no-such-branch"), "got: {error}"); + } + + // ── file creation and deletion ──────────────────────────────── + + #[test] + fn applying_a_commit_that_added_a_file_creates_it() { + let fixture = Fixture::new(); + fixture.write("keep.txt", "x\n"); + fixture.commit("base", &["keep.txt"]); + fixture.write("added.txt", "one\ntwo\n"); + let oid = fixture.commit("add a file", &["added.txt"]); + std::fs::remove_file(fixture.path.join("added.txt")).unwrap(); + + apply( + &fixture, + "added.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .unwrap(); + assert_eq!(fixture.read("added.txt"), "one\ntwo\n"); + } + + #[test] + fn reverting_a_commit_that_added_a_file_deletes_it() { + let fixture = Fixture::new(); + fixture.write("keep.txt", "x\n"); + fixture.commit("base", &["keep.txt"]); + fixture.write("added.txt", "one\ntwo\n"); + let oid = fixture.commit("add a file", &["added.txt"]); + + apply( + &fixture, + "added.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Revert, + ) + .unwrap(); + assert!( + !fixture.path.join("added.txt").exists(), + "reverting the addition of a file should remove it" + ); + } + + #[test] + fn a_file_unchanged_by_the_source_is_refused_by_name() { + let (fixture, oid) = two_hunk_commit(); + fixture.write("other.txt", "untouched\n"); + let error = apply( + &fixture, + "other.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .expect_err("that file is not part of the commit"); + assert!(error.to_string().contains("other.txt"), "got: {error}"); + } + + #[test] + fn a_hunk_index_past_the_end_is_refused() { + let (fixture, oid) = two_hunk_commit(); + let error = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::Hunk(9), + WorktreePatchDirection::Apply, + ) + .expect_err("there is no tenth hunk"); + assert!( + error.to_string().contains("reselect the hunk"), + "got: {error}" + ); + } + + // ── undo snapshots ──────────────────────────────────────────── + + #[test] + fn the_snapshot_captures_the_bytes_the_apply_overwrote() { + let (fixture, oid) = two_hunk_commit(); + let before = fixture.read("f.txt"); + let outcome = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .unwrap(); + + assert_eq!(outcome.snapshots.len(), 1); + assert_eq!(outcome.snapshots[0].path, PathBuf::from("f.txt")); + assert_eq!( + outcome.snapshots[0].contents.as_deref(), + Some(before.as_bytes()) + ); + } + + #[test] + fn restoring_a_snapshot_puts_the_original_bytes_back() { + let (fixture, oid) = two_hunk_commit(); + let before = fixture.read("f.txt"); + let outcome = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .unwrap(); + assert_ne!(fixture.read("f.txt"), before); + + restore_worktree_files(&fixture.path, &outcome.snapshots).unwrap(); + assert_eq!(fixture.read("f.txt"), before); + } + + #[test] + fn restoring_an_absent_snapshot_deletes_the_created_file() { + let fixture = Fixture::new(); + fixture.write("keep.txt", "x\n"); + fixture.commit("base", &["keep.txt"]); + fixture.write("added.txt", "one\n"); + let oid = fixture.commit("add", &["added.txt"]); + std::fs::remove_file(fixture.path.join("added.txt")).unwrap(); + + let outcome = apply( + &fixture, + "added.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .unwrap(); + assert!(fixture.path.join("added.txt").exists()); + assert_eq!(outcome.snapshots[0].contents, None); + + restore_worktree_files(&fixture.path, &outcome.snapshots).unwrap(); + assert!(!fixture.path.join("added.txt").exists()); + } + + #[test] + fn restoring_a_snapshot_of_a_deleted_file_recreates_it() { + let fixture = Fixture::new(); + fixture.write("keep.txt", "x\n"); + fixture.commit("base", &["keep.txt"]); + fixture.write("added.txt", "one\ntwo\n"); + let oid = fixture.commit("add", &["added.txt"]); + + let outcome = apply( + &fixture, + "added.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Revert, + ) + .unwrap(); + assert!(!fixture.path.join("added.txt").exists()); + + restore_worktree_files(&fixture.path, &outcome.snapshots).unwrap(); + assert_eq!(fixture.read("added.txt"), "one\ntwo\n"); + } + + // ── stash entries travel the commit path ────────────────────── + + #[test] + fn a_stash_entrys_hunk_applies_like_any_other_commit() { + let fixture = Fixture::new(); + fixture.write("f.txt", "a\nb\nc\n"); + fixture.commit("base", &["f.txt"]); + fixture.write("f.txt", "a\nSTASHED\nc\n"); + let mut repo = Repository::open(&fixture.path).unwrap(); + let signature = git2::Signature::now("Test", "t@t.com").unwrap(); + let stash_oid = repo + .stash_save(&signature, "wip", Some(git2::StashFlags::DEFAULT)) + .unwrap(); + assert_eq!(fixture.read("f.txt"), "a\nb\nc\n", "stash reset the file"); + + apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(stash_oid), + WorktreePatchScope::Hunk(0), + WorktreePatchDirection::Apply, + ) + .unwrap(); + assert_eq!(fixture.read("f.txt"), "a\nSTASHED\nc\n"); + } +} diff --git a/crates/rgitui_git/src/types.rs b/crates/rgitui_git/src/types.rs index fd7f71a..9dcfd89 100644 --- a/crates/rgitui_git/src/types.rs +++ b/crates/rgitui_git/src/types.rs @@ -331,6 +331,10 @@ pub enum GitOperationKind { ResolveConflict, Clean, Clone, + /// Writing diff content from another revision into the working tree. + ApplyToWorktree, + /// Removing diff content from another revision out of the working tree. + RevertInWorktree, } impl GitOperationKind { @@ -358,6 +362,8 @@ impl GitOperationKind { GitOperationKind::ResolveConflict => "Resolve conflict", GitOperationKind::Clean => "Clean", GitOperationKind::Clone => "Clone", + GitOperationKind::ApplyToWorktree => "Apply to working tree", + GitOperationKind::RevertInWorktree => "Revert in working tree", } } } From 595a77f7562ee5a470925dd39c0de20884ba1155 Mon Sep 17 00:00:00 2001 From: adehad <26027314+adehad@users.noreply.github.com> Date: Mon, 3 Aug 2026 11:20:22 +0100 Subject: [PATCH 2/9] feat: offer apply and revert on diffs from outside the working tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #65 made a commit diff stop lying about being unstaged, and the `DiffSource` enum it introduced suppressed the staging affordances that did the damage. What it left behind is a diff you can look at and nothing else: no button, no menu. The user's case is comparing against another branch and pulling the difference into their files, so an inert panel is only half a fix. Generalise `staging_action()` into `DiffSource::operations()`, the one place that decides what a source supports. Stage for the working tree, Unstage for the index, and Apply/Revert for everything else — a commit, a stash entry, or the new `Compare { from, to }` variant, which is how a branch-comparison view would describe its content. No source returns an empty list, so none is inert, and `reject_operation` (was `reject_staging`) now covers all four operations from that same list rather than a second hand-written table. `Compare` exists even though no comparison UI does yet, because the alternative is worse: treat "the difference between two revisions" as a commit diff and the patch gets generated from the wrong pair of trees. `DiffSource::patch_source` resolves both it and `Commit` to the tree pair the git layer wants, so whenever a compare view lands the apply path already serves it correctly. Three granularities, following the rule staging already uses: * the hunk by default — one button per operation in every hunk header, unified and side-by-side, both routed through `DiffViewerEvent::for_hunk` so a button and a keystroke cannot disagree; * a manual line selection when the user has turned on partial mode; * the whole file, from a new "File" menu in the viewer header. The hunk headers cannot express whole-file and the sidebar's Staged/Unstaged lists only cover staging, so this is the only home for it — and it is why the menu carries apply/revert and nothing else. Partial mode now works on committed content. It was blocked because line-level staging was meaningless there; line-level *apply* is not. The staging route stays shut, which `partial_mode_on_a_commit_diff_still_refuses_to_stage` pins. A selection spanning several hunks becomes one line-scoped request rather than one request per hunk: unlike staging, these read a file off disk and write it back, so two in flight over the same file would race. `apply_selection`, `revert_selection`, `apply_file` and `revert_file` join the staging entry points as the viewer's public commands; the next commit declares the actions that dispatch to them. The view tests drive those methods rather than keystrokes for the reason the staging ones already do: the bindings live in `rgitui_workspace`, which sits above this crate. The workspace maps the request to `patch_worktree_at`, taking the tree pair from the displayed source, and rejects any request the source does not offer — the same backstop the staging path got, now needed in both directions because apply and revert write to disk. On success `WorktreePatchApplied` carries the overwritten bytes into a new `UndoAction::RestoreWorktreeFiles`, the first undo action that restores content instead of issuing a reversing git command; a reverse patch would not be exact after a three-way merge. Co-Authored-By: Claude Fable 5 --- Cargo.lock | 1 + crates/rgitui_diff/Cargo.toml | 3 + crates/rgitui_diff/src/lib.rs | 1282 ++++++++++++++--- .../rgitui_workspace/src/workspace/events.rs | 66 +- crates/rgitui_workspace/src/workspace/undo.rs | 76 +- 5 files changed, 1219 insertions(+), 209 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 61c0969..98c04c7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5125,6 +5125,7 @@ name = "rgitui_diff" version = "0.3.2" dependencies = [ "anyhow", + "git2", "gpui", "log", "rgitui_git", diff --git a/crates/rgitui_diff/Cargo.toml b/crates/rgitui_diff/Cargo.toml index 5a68214..4c0b8fa 100644 --- a/crates/rgitui_diff/Cargo.toml +++ b/crates/rgitui_diff/Cargo.toml @@ -15,6 +15,9 @@ rgitui_ui.workspace = true rgitui_git.workspace = true rgitui_settings.workspace = true anyhow.workspace = true +# Only for parsing a hex OID out of a `DiffSource` when building the tree pair +# an apply/revert patch comes from. +git2.workspace = true log.workspace = true syntect = { version = "5.3", default-features = false, features = ["default-fancy"] } diff --git a/crates/rgitui_diff/src/lib.rs b/crates/rgitui_diff/src/lib.rs index 89578b2..d469d69 100644 --- a/crates/rgitui_diff/src/lib.rs +++ b/crates/rgitui_diff/src/lib.rs @@ -14,7 +14,10 @@ use gpui::{ MouseMoveEvent, MouseUpEvent, Render, ScrollStrategy, SharedString, StyledText, UniformListScrollHandle, WeakEntity, Window, }; -use rgitui_git::{DiffLine, FileDiff, ThreeWayFileDiff}; +use rgitui_git::{ + DiffLine, FileDiff, ThreeWayFileDiff, WorktreePatchDirection, WorktreePatchScope, + WorktreePatchSource, +}; use rgitui_theme::{ActiveTheme, Appearance, Color, StyledExt, ThemeState}; use rgitui_ui::{ Badge, Button, ButtonSize, ButtonStyle, EstimatedListScroll, Icon, IconName, IconSize, Label, @@ -28,7 +31,7 @@ use syntect::parsing::{SyntaxReference, SyntaxSet}; /// old_file_line is None for additions (new lines), new_file_line is None for deletions. pub type LineSelection = Vec<(Option, Option)>; -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub enum DiffViewerEvent { /// The displayed file changed. Workspace listeners use this to start /// blame/history prefetching for every path into the diff viewer. @@ -44,6 +47,36 @@ pub enum DiffViewerEvent { LineStageRequested(Vec<(Option, Option)>), /// Request to unstage only the given lines within the current staged diff. LineUnstageRequested(Vec<(Option, Option)>), + /// Request to write the displayed source's content into the working tree + /// (`Apply`) or take it back out (`Revert`), over `scope`. + /// + /// `scope` carries all three granularities — hunk, line selection, whole + /// file — because the handler treats them alike: resolve the source to a + /// tree pair and hand the scope to the git layer. + WorktreePatchRequested { + /// Always [`DiffOperation::Apply`] or [`DiffOperation::Revert`]. + operation: DiffOperation, + scope: WorktreePatchScope, + }, +} + +impl DiffViewerEvent { + /// The request `operation` raises against hunk `index`. + /// + /// The single mapping from operation to event, so a hunk-header button and a + /// key binding for the same operation cannot disagree. + pub fn for_hunk(operation: DiffOperation, index: usize) -> Self { + match operation { + DiffOperation::Stage => DiffViewerEvent::HunkStageRequested(index), + DiffOperation::Unstage => DiffViewerEvent::HunkUnstageRequested(index), + DiffOperation::Apply | DiffOperation::Revert => { + DiffViewerEvent::WorktreePatchRequested { + operation, + scope: WorktreePatchScope::Hunk(index), + } + } + } + } } #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] @@ -54,23 +87,81 @@ pub enum DiffDisplayMode { ThreeWay, } -/// The staging operation a mutable diff supports. +/// An operation the diff viewer can offer over the content it is showing. /// -/// A diff of the working tree can only be staged; a diff of the index can only -/// be unstaged. Historical content supports neither, which is why this is -/// returned as an `Option` from [`DiffSource::staging_action`]. +/// Which of these are available is a property of the content's provenance +/// alone, and [`DiffSource::operations`] is the single place that decides it. +/// A mutable working-tree source can be staged or unstaged; content from +/// anywhere else — a commit, a stash, a comparison of two revisions — cannot, +/// but its changes can be applied to or reverted in the working tree, which are +/// the hunk-level equivalents of a cherry-pick and a revert. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub enum StagingAction { +pub enum DiffOperation { + /// Move working-tree content into the index. Stage, + /// Move index content back out to the working tree. Unstage, + /// Write this source's version of the content into the working tree. + Apply, + /// Take this source's change back out of the working tree. + Revert, } -impl StagingAction { +impl DiffOperation { /// Label for the per-hunk button in the diff viewer's hunk headers. pub fn hunk_button_label(self) -> &'static str { match self { - StagingAction::Stage => "Stage Hunk", - StagingAction::Unstage => "Unstage Hunk", + DiffOperation::Stage => "Stage Hunk", + DiffOperation::Unstage => "Unstage Hunk", + DiffOperation::Apply => "Apply Hunk", + DiffOperation::Revert => "Revert Hunk", + } + } + + /// Label for the whole-file entry in the diff viewer's file menu. + pub fn file_menu_label(self) -> &'static str { + match self { + DiffOperation::Stage => "Stage File", + DiffOperation::Unstage => "Unstage File", + DiffOperation::Apply => "Apply File to Working Tree", + DiffOperation::Revert => "Revert File in Working Tree", + } + } + + /// The *default* key that invokes this operation while the diff viewer has + /// focus, for hints and element ids. + /// + /// A hint rather than a lookup: the binding lives in the `commands!` registry + /// in `rgitui_workspace`, which sits above this crate and so cannot be named + /// from here, and the user may have rebound it. + pub fn key(self) -> &'static str { + match self { + DiffOperation::Stage => "s", + DiffOperation::Unstage => "u", + DiffOperation::Apply => "a", + DiffOperation::Revert => "r", + } + } + + /// True for the two operations that move content between the working tree + /// and the index without editing any file. + pub fn is_staging(self) -> bool { + matches!(self, DiffOperation::Stage | DiffOperation::Unstage) + } + + /// True for the two operations that rewrite files on disk. Callers must put + /// these on the undo stack, since nothing else records the previous contents. + pub fn writes_files(self) -> bool { + matches!(self, DiffOperation::Apply | DiffOperation::Revert) + } + + /// The git-layer direction this operation runs in, or `None` for the two + /// staging operations, which do not go through the patch machinery. + pub fn patch_direction(self) -> Option { + match self { + DiffOperation::Stage | DiffOperation::Unstage => None, + DiffOperation::Apply => Some(WorktreePatchDirection::Apply), + DiffOperation::Revert => Some(WorktreePatchDirection::Revert), } } } @@ -78,19 +169,27 @@ impl StagingAction { /// Where the content currently shown in the diff viewer came from. /// /// Staged versus unstaged is a property of the two mutable sources only. A -/// commit or stash has no such distinction — its content is already recorded — -/// so it carries its OID instead, and offers no staging. +/// commit, stash or revision pair has no such distinction — its content is +/// already recorded — so it carries the revision it came from instead, and +/// offers no staging. #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum DiffSource { /// Unstaged working-tree changes (index → workdir). Can be staged. Worktree, /// Staged changes (HEAD → index). Can be unstaged. Index, - /// A file inside a commit, identified by its hex OID. Immutable. + /// A file inside a commit, identified by its hex OID. Commit(String), /// A file inside a stash entry, identified by the stash commit's hex OID. - /// Immutable. Stash(String), + /// The difference between two arbitrary revisions, `from` → `to`, as any + /// branch-comparison view would produce. Applying brings `to`'s content + /// into the working tree; reverting restores `from`'s. + /// + /// A commit diff carries [`Self::Commit`] rather than this, so that it can be + /// identified by OID for caching and blame; both resolve to a tree pair in + /// [`Self::patch_source`], so apply and revert treat them identically. + Compare { from: String, to: String }, } impl DiffSource { @@ -103,68 +202,130 @@ impl DiffSource { } } - /// The staging operation this content supports, or `None` when the content - /// is an immutable historical snapshot. - pub fn staging_action(&self) -> Option { + /// Every operation this content supports, in the order the UI offers them. + pub fn operations(&self) -> &'static [DiffOperation] { match self { - DiffSource::Worktree => Some(StagingAction::Stage), - DiffSource::Index => Some(StagingAction::Unstage), - DiffSource::Commit(_) | DiffSource::Stash(_) => None, + DiffSource::Worktree => &[DiffOperation::Stage], + DiffSource::Index => &[DiffOperation::Unstage], + DiffSource::Commit(_) | DiffSource::Stash(_) | DiffSource::Compare { .. } => { + &[DiffOperation::Apply, DiffOperation::Revert] + } } } - /// True when the content is already committed or stashed, so it has no - /// staged/unstaged distinction and cannot be staged or unstaged. + /// Whether `operation` is available for this content. + pub fn offers(&self, operation: DiffOperation) -> bool { + self.operations().contains(&operation) + } + + /// The staging operation this content supports, or `None` when it is not + /// mutable working-tree content. + pub fn staging_action(&self) -> Option { + self.operations() + .iter() + .copied() + .find(|operation| operation.is_staging()) + } + + /// True when the content is a snapshot from outside the working tree, so it + /// has no staged/unstaged distinction and cannot be staged or unstaged. pub fn is_historical(&self) -> bool { self.staging_action().is_none() } /// The commit-like OID backing this content, if any. Used for the display /// cache key, the `DiffChanged` payload, and blame/history view caching. + /// + /// A comparison has no single commit to attribute lines to — its endpoints + /// may be branch names rather than OIDs — so it reports `None` and is + /// treated like working-tree content by the blame/history prefetch. pub fn commit_id(&self) -> Option<&str> { match self { - DiffSource::Worktree | DiffSource::Index => None, + DiffSource::Worktree | DiffSource::Index | DiffSource::Compare { .. } => None, DiffSource::Commit(oid) | DiffSource::Stash(oid) => Some(oid), } } - /// Why `requested` cannot be applied to this content, as an actionable + /// Short label naming the revision (or pair) this content came from, for + /// menu entries and tooltips. `None` for working-tree content, which has no + /// revision to name. + pub fn revision_label(&self) -> Option { + match self { + DiffSource::Worktree | DiffSource::Index => None, + DiffSource::Commit(oid) | DiffSource::Stash(oid) => { + Some(oid[..7.min(oid.len())].to_string()) + } + DiffSource::Compare { from, to } => Some(format!("{from}...{to}")), + } + } + + /// The tree pair an apply or revert should be generated from, or `None` for + /// working-tree content, which supports staging instead. + pub fn patch_source(&self) -> Option { + match self { + DiffSource::Worktree | DiffSource::Index => None, + DiffSource::Commit(oid) | DiffSource::Stash(oid) => git2::Oid::from_str(oid) + .ok() + .map(WorktreePatchSource::Commit), + DiffSource::Compare { from, to } => Some(WorktreePatchSource::Compare { + from: from.clone(), + to: to.clone(), + }), + } + } + + /// Why `requested` cannot be performed on this content, as an actionable /// sentence, or `None` when the request is valid. /// /// The viewer already hides the affordances that would produce an invalid /// request, so this is a backstop for the case where the displayed content /// changes between a click being painted and being delivered. - pub fn reject_staging(&self, requested: StagingAction) -> Option { + pub fn reject_operation(&self, requested: DiffOperation) -> Option { + if self.offers(requested) { + return None; + } match (self, requested) { - (DiffSource::Worktree, StagingAction::Stage) - | (DiffSource::Index, StagingAction::Unstage) => None, - (DiffSource::Worktree, StagingAction::Unstage) => { + (DiffSource::Worktree, DiffOperation::Unstage) => { Some("These changes are not staged yet — press s to stage them first.".to_string()) } - (DiffSource::Index, StagingAction::Stage) => { + (DiffSource::Index, DiffOperation::Stage) => { Some("These changes are already staged — press u to unstage them.".to_string()) } + // A mutable source asked to apply or revert: that content is + // already in the working tree, so there is nothing to bring in. + (DiffSource::Worktree | DiffSource::Index, _) => Some( + "These changes are already in your working tree — use s or u to move them \ + between the index and the working tree." + .to_string(), + ), (DiffSource::Commit(oid), _) => Some(format!( - "Commit {} is already committed and cannot be staged — select the file under \ - Staged or Unstaged in the sidebar to change its working-tree state.", + "Commit {} is already committed and cannot be staged — press a to apply its \ + changes to your working tree, or select the file under Staged or Unstaged in \ + the sidebar.", &oid[..7.min(oid.len())] )), (DiffSource::Stash(_), _) => Some( - "Stashed changes cannot be staged directly — apply or pop the stash first, then \ - stage it from the sidebar." + "Stashed changes cannot be staged directly — press a to apply them to your \ + working tree, or pop the stash and stage it from the sidebar." .to_string(), ), + (DiffSource::Compare { from, to }, _) => Some(format!( + "{from}...{to} is a comparison of two revisions and cannot be staged — press a \ + to apply its changes to your working tree instead." + )), } } - /// Badge shown in the diff viewer header. Historical content is labelled by - /// its origin rather than by a staged/unstaged state it does not have. + /// Badge shown in the diff viewer header. Content from outside the working + /// tree is labelled by its origin rather than by a staged/unstaged state it + /// does not have. pub fn badge(&self) -> (&'static str, Color) { match self { DiffSource::Worktree => ("Unstaged", Color::Modified), DiffSource::Index => ("Staged", Color::Added), DiffSource::Commit(_) => ("Committed", Color::Muted), DiffSource::Stash(_) => ("Stashed", Color::Muted), + DiffSource::Compare { .. } => ("Compared", Color::Muted), } } } @@ -563,6 +724,10 @@ pub struct DiffViewer { error: Option, /// Bounded LRU cache for computed display rows. display_cache: DisplayCache, + /// Whether the header's file-level operations menu is open. It is the mouse + /// route to whole-file apply/revert, the granularity neither the hunk headers + /// nor a line selection can express. + file_menu_open: bool, /// Bumped when the selected diff content changes. Workspace-level diff /// refreshes use this to reject stale repository results. generation: u64, @@ -619,6 +784,7 @@ impl DiffViewer { loading: false, error: None, display_cache: DisplayCache::default(), + file_menu_open: false, generation: 0, preparation_generation: 0, } @@ -775,6 +941,7 @@ impl DiffViewer { self.partial_mode = false; self.selection_anchor = None; self.mouse_selecting = false; + self.file_menu_open = false; cx.emit(DiffViewerEvent::DiffChanged { path: path.clone(), @@ -846,6 +1013,7 @@ impl DiffViewer { self.partial_mode = false; self.selection_anchor = None; self.mouse_selecting = false; + self.file_menu_open = false; self.sync_wrap_list_state(); cx.notify(); } @@ -876,6 +1044,7 @@ impl DiffViewer { self.partial_mode = false; self.selection_anchor = None; self.mouse_selecting = false; + self.file_menu_open = false; cx.emit(DiffViewerEvent::DiffChanged { path: self.file_path.clone().unwrap_or_default(), commit_id: None, @@ -1220,9 +1389,11 @@ impl DiffViewer { /// Toggles line-level selection, clearing any selection when leaving it. pub fn toggle_partial_mode(&mut self, cx: &mut Context) { - // Meaningless for committed or stashed content, which cannot be staged - // at all. - if self.source.is_historical() { + // Partial mode exists to scope the source's line-level operation — + // staging for the working tree, apply/revert for anything else. The + // three-way conflict view has no line-level operation, so there is + // nothing for a line selection to act on. + if self.display_mode == DiffDisplayMode::ThreeWay { return; } self.partial_mode = !self.partial_mode; @@ -1254,7 +1425,7 @@ impl DiffViewer { pub fn stage_selection(&mut self, cx: &mut Context) { // Only the working tree can be staged; committed and stashed content is // historical and has nothing to stage. - if self.source.staging_action() != Some(StagingAction::Stage) { + if !self.source.offers(DiffOperation::Stage) { return; } if self.partial_mode { @@ -1280,7 +1451,7 @@ impl DiffViewer { /// Requests unstaging of the hunks — or lines — under the current selection. pub fn unstage_selection(&mut self, cx: &mut Context) { // Only the index can be unstaged. - if self.source.staging_action() != Some(StagingAction::Unstage) { + if !self.source.offers(DiffOperation::Unstage) { return; } if self.partial_mode { @@ -1300,7 +1471,7 @@ impl DiffViewer { /// Requests staging of just the hunk under the cursor. pub fn stage_current_hunk(&mut self, cx: &mut Context) { - if self.source.staging_action() != Some(StagingAction::Stage) { + if !self.source.offers(DiffOperation::Stage) { return; } if let Some(idx) = self.current_hunk_index() { @@ -1310,7 +1481,7 @@ impl DiffViewer { /// Requests unstaging of just the hunk under the cursor. pub fn unstage_current_hunk(&mut self, cx: &mut Context) { - if self.source.staging_action() != Some(StagingAction::Unstage) { + if !self.source.offers(DiffOperation::Unstage) { return; } if let Some(idx) = self.current_hunk_index() { @@ -1318,6 +1489,34 @@ impl DiffViewer { } } + /// Requests that the displayed source's version of the hunks — or, in partial + /// mode, the lines — under the current selection be written into the working + /// tree. + /// + /// The counterpart of [`Self::stage_selection`] for content that has no + /// staging route: a commit, a stash entry, or a comparison of two revisions. + pub fn apply_selection(&mut self, cx: &mut Context) { + self.request_worktree_patch(DiffOperation::Apply, cx); + } + + /// Requests that the displayed source's change be taken back out of the + /// working tree, over the same granularity [`Self::apply_selection`] uses. + pub fn revert_selection(&mut self, cx: &mut Context) { + self.request_worktree_patch(DiffOperation::Revert, cx); + } + + /// Requests that the whole file be brought to the displayed source's version, + /// ignoring the current selection. + pub fn apply_file(&mut self, cx: &mut Context) { + self.request_whole_file_patch(DiffOperation::Apply, cx); + } + + /// Requests that the displayed source's change be taken out of the whole + /// file, ignoring the current selection. + pub fn revert_file(&mut self, cx: &mut Context) { + self.request_whole_file_patch(DiffOperation::Revert, cx); + } + /// The change lines under the selection, or the cursor hunk's if there is none. fn change_lines_under_selection(&self) -> Vec<(Option, Option)> { match &self.selected_lines { @@ -1341,6 +1540,94 @@ impl DiffViewer { } } + /// The operations the file-level menu offers: the ones that act on the whole + /// file rather than on a hunk or a line selection. + /// + /// Whole-file staging is reached from the sidebar's Staged/Unstaged lists, so + /// the menu carries apply/revert only. + fn file_menu_operations(&self) -> Vec { + if self.display_mode == DiffDisplayMode::ThreeWay { + return Vec::new(); + } + self.source + .operations() + .iter() + .copied() + .filter(|operation| operation.writes_files()) + .collect() + } + + /// Ask the workspace to apply or revert `operation` over the whole file. + fn request_whole_file_patch(&mut self, operation: DiffOperation, cx: &mut Context) { + self.file_menu_open = false; + if !self.source.offers(operation) { + cx.notify(); + return; + } + cx.emit(DiffViewerEvent::WorktreePatchRequested { + operation, + scope: WorktreePatchScope::File, + }); + cx.notify(); + } + + /// Ask the workspace to apply or revert `operation` over the granularity the + /// current selection implies. Silently does nothing when the displayed + /// content does not support the operation, so the key is inert rather than + /// wrong on a working-tree diff. + fn request_worktree_patch(&mut self, operation: DiffOperation, cx: &mut Context) { + if !self.source.offers(operation) { + return; + } + if let Some(scope) = self.selected_patch_scope() { + cx.emit(DiffViewerEvent::WorktreePatchRequested { operation, scope }); + } + } + + /// The scope an apply/revert should cover: line-level when the user has turned + /// on partial mode, otherwise the hunk under the cursor, or the hunks the row + /// selection spans. + /// + /// Several selected hunks collapse into one `Lines` scope. These operations + /// read the file off disk and write it back, so two requests in flight over + /// the same file would race; one request covering every selected line cannot. + fn selected_patch_scope(&self) -> Option { + if self.partial_mode { + let lines = match &self.selected_lines { + Some(selection) => self + .lines_under_selection(selection.clone()) + .into_iter() + .filter(Self::is_change_line) + .collect(), + None => self.current_hunk_changes(), + }; + if lines.is_empty() { + return None; + } + return Some(WorktreePatchScope::Lines(lines)); + } + + let hunks = match &self.selected_lines { + Some(selection) => self.hunks_under_selection(selection.clone()), + None => self + .current_hunk_index() + .map(|i| vec![i]) + .unwrap_or_default(), + }; + match hunks.as_slice() { + [] => None, + [only] => Some(WorktreePatchScope::Hunk(*only)), + several => { + let lines = self.changes_in_hunks(several); + if lines.is_empty() { + None + } else { + Some(WorktreePatchScope::Lines(lines)) + } + } + } + } + /// Returns the hunk index at or before the currently highlighted row. /// If the highlighted row itself is a hunk header, returns its index. /// Otherwise searches backwards to find the nearest preceding hunk. @@ -1502,39 +1789,42 @@ impl DiffViewer { /// (old_num, new_num) pairs. Deletions are emitted as (Some, None) and /// additions as (None, Some) so the git layer can stage either side. fn current_hunk_changes(&self) -> Vec<(Option, Option)> { - let hunk_idx = match self.current_hunk_index() { - Some(i) => i, - None => return Vec::new(), - }; + match self.current_hunk_index() { + Some(index) => self.changes_in_hunks(&[index]), + None => Vec::new(), + } + } + /// The change lines of every hunk in `hunks`, as (old_num, new_num) pairs in + /// file order. Used to express a multi-hunk selection as a single line-scoped + /// request. + fn changes_in_hunks(&self, hunks: &[usize]) -> Vec<(Option, Option)> { + // Line numbers only come off the unified rows; the side-by-side rows + // split a modification across two columns. + if self.display_mode == DiffDisplayMode::ThreeWay { + return Vec::new(); + } + let wanted: HashSet = hunks.iter().copied().collect(); let mut lines = Vec::new(); - if self.display_mode == DiffDisplayMode::Unified { - let rows: &Vec = &self.display_rows; - let mut in_hunk = false; - for row in rows.iter() { - match row { - DisplayRow::HunkHeader { hunk_index, .. } => { - if *hunk_index == hunk_idx { - in_hunk = true; - } else if in_hunk { - break; // past our hunk - } - } - DisplayRow::Line { - old_num, - new_num, - kind, - .. - } => { - if in_hunk - && matches!(kind, DisplayLineKind::Addition | DisplayLineKind::Deletion) - { - lines.push((*old_num, *new_num)); - } + let mut current_hunk: Option = None; + for row in self.display_rows.iter() { + match row { + DisplayRow::HunkHeader { hunk_index, .. } => current_hunk = Some(*hunk_index), + DisplayRow::Line { + old_num, + new_num, + kind, + .. + } => { + let in_wanted_hunk = current_hunk.is_some_and(|hunk| wanted.contains(&hunk)); + if in_wanted_hunk + && matches!(kind, DisplayLineKind::Addition | DisplayLineKind::Deletion) + { + lines.push((*old_num, *new_num)); } } } - }; + } lines } @@ -2553,6 +2843,149 @@ impl DiffViewer { } } +/// Height of one row in the diff viewer's file-operations menu, in pixels. +const FILE_MENU_ITEM_HEIGHT: f32 = 28.0; +/// Minimum width of the file-operations menu, in pixels. +const FILE_MENU_WIDTH: f32 = 220.0; +/// Where the menu's dismiss backdrop starts, in pixels from the top of the +/// viewer. Must clear the header so the toggle button stays clickable. +const FILE_MENU_BACKDROP_TOP: f32 = 26.0; + +impl DiffViewer { + /// The header's file-operations toggle. Renders nothing when the displayed + /// content has no whole-file operation. + fn render_file_menu_button(&self, cx: &mut Context) -> AnyElement { + if self.file_menu_operations().is_empty() { + return div().into_any_element(); + } + let tooltip: SharedString = match self.source.revision_label() { + Some(revision) => format!("Apply or revert this whole file from {revision}").into(), + None => "Whole-file operations".into(), + }; + Button::new("diff-file-menu", "File") + .size(ButtonSize::Compact) + .style(ButtonStyle::Subtle) + .tooltip(tooltip) + .on_click(cx.listener(|this, _: &ClickEvent, _, cx| { + this.file_menu_open = !this.file_menu_open; + cx.notify(); + })) + .into_any_element() + } + + /// True when the file-operations menu is open and has something to show. + /// + /// Both the menu and its dismiss backdrop are gated on this, so the backdrop + /// can never outlive the menu and swallow clicks over the diff body. + fn file_menu_visible(&self) -> bool { + self.file_menu_open && !self.file_menu_operations().is_empty() + } + + /// A backdrop over the diff body that dismisses the open file menu. + /// + /// Starts below the header so the toggle button stays uncovered: a backdrop + /// over the button would eat its mouse-down, and the button would reopen the + /// menu it had just closed. + fn render_file_menu_backdrop(&self, cx: &mut Context) -> Option { + if !self.file_menu_visible() { + return None; + } + Some( + div() + .id("diff-file-menu-backdrop") + .absolute() + .top(px(FILE_MENU_BACKDROP_TOP)) + .left_0() + .right_0() + .bottom_0() + .on_mouse_down( + MouseButton::Left, + cx.listener(|this, _: &MouseDownEvent, _, cx| { + this.file_menu_open = false; + cx.notify(); + cx.stop_propagation(); + }), + ) + .into_any_element(), + ) + } + + /// The open file-operations menu, positioned under the header's toggle. + fn render_file_menu(&self, cx: &mut Context) -> Option { + if !self.file_menu_visible() { + return None; + } + let operations = self.file_menu_operations(); + let colors = cx.colors(); + let revision = self.source.revision_label(); + + let mut menu = div() + .id("diff-file-menu-popover") + .absolute() + .top(px(28.)) + .right(px(8.)) + .v_flex() + .min_w(px(FILE_MENU_WIDTH)) + .py(px(4.)) + .bg(colors.elevated_surface_background) + .border_1() + .border_color(colors.border) + .rounded(px(6.)) + .elevation_3(cx) + // Clicking inside the menu must not reach the viewer's own click + // handler, which would steal focus and re-render underneath. + .on_mouse_down( + MouseButton::Left, + |_: &MouseDownEvent, _: &mut Window, cx: &mut App| { + cx.stop_propagation(); + }, + ); + + if let Some(revision) = &revision { + menu = menu.child( + div().px(px(10.)).pb(px(2.)).child( + Label::new(SharedString::from(revision.clone())) + .size(LabelSize::XSmall) + .color(Color::Muted), + ), + ); + } + + for operation in operations { + let hover_bg = colors.ghost_element_hover; + let active_bg = colors.ghost_element_active; + let accent = colors.text_accent; + menu = menu.child( + div() + .id(SharedString::from(format!( + "diff-file-menu-{}", + operation.key() + ))) + .h_flex() + .w_full() + .h(px(FILE_MENU_ITEM_HEIGHT)) + .px(px(10.)) + .gap(px(6.)) + .items_center() + .rounded(px(4.)) + .cursor_pointer() + .hover(move |s| s.bg(hover_bg).border_l_2().border_color(accent)) + .active(move |s| s.bg(active_bg)) + .on_click(cx.listener(move |this, _: &ClickEvent, _, cx| { + this.request_whole_file_patch(operation, cx); + })) + .child( + Label::new(operation.file_menu_label()) + .size(LabelSize::Small) + .color(Color::Default), + ), + ); + } + + Some(menu.into_any_element()) + } +} + impl Render for DiffViewer { fn render(&mut self, window: &mut Window, cx: &mut Context) -> impl IntoElement { log::trace!( @@ -2652,9 +3085,9 @@ impl Render for DiffViewer { let display_rows = self.display_rows.clone(); let sbs_rows = self.sbs_rows.clone(); let three_way_rows = self.three_way_rows.clone(); - // `None` for committed/stashed content, which suppresses every per-hunk - // staging affordance below. - let staging_action = self.source.staging_action(); + // Stage/unstage for working-tree content, apply/revert for everything + // else; the hunk headers below render one button per entry. + let hunk_operations = self.source.operations(); let display_mode = self.display_mode; let view: WeakEntity = cx.weak_entity(); @@ -2767,12 +3200,10 @@ impl Render for DiffViewer { let ctx_name: SharedString = context_name.clone().into(); let has_context = !context_name.is_empty(); let idx = *hunk_index; - let view_clone = view.clone(); let view_hunk = view.clone(); let view_hunk_drag = view.clone(); - let is_hunk_selected = selected_lines - .as_ref() - .is_some_and(|r| r.contains(&i)); + let is_hunk_selected = + selected_lines.as_ref().is_some_and(|r| r.contains(&i)); let hunk_bg = if is_hunk_selected { selection_bg } else { @@ -2842,16 +3273,21 @@ impl Render for DiffViewer { hunk_row = hunk_row.child(div().flex_1()); - // Committed and stashed content is immutable — - // offer no staging button at all. - if let Some(action) = staging_action { + // One button per operation the source + // supports: Stage/Unstage for working-tree + // content, Apply/Revert for a commit, a + // stash, or a comparison. + for operation in hunk_operations { + let operation = *operation; + let button_view = view.clone(); hunk_row = hunk_row.child( Button::new( SharedString::from(format!( - "hunk-stage-{}", + "hunk-{}-{}", + operation.key(), idx )), - action.hunk_button_label(), + operation.hunk_button_label(), ) .size(ButtonSize::Compact) .style(ButtonStyle::Subtle) @@ -2859,12 +3295,11 @@ impl Render for DiffViewer { move |_: &ClickEvent, _: &mut Window, cx: &mut App| { - view_clone + button_view .update(cx, |_this, cx| { - cx.emit(match action { - StagingAction::Stage => DiffViewerEvent::HunkStageRequested(idx), - StagingAction::Unstage => DiffViewerEvent::HunkUnstageRequested(idx), - }); + cx.emit(DiffViewerEvent::for_hunk( + operation, idx, + )); }) .ok(); }, @@ -2880,8 +3315,7 @@ impl Render for DiffViewer { styled, kind, } => { - let (prefix, text_col, line_bg, gutter_accent) = match kind - { + let (prefix, text_col, line_bg, gutter_accent) = match kind { DisplayLineKind::Context => { (" ", text_color, editor_bg, gutter_bg) } @@ -2903,9 +3337,8 @@ impl Render for DiffViewer { .into(); let prefix_str: SharedString = prefix.into(); let is_highlighted = highlighted_row == Some(i); - let is_selected = selected_lines - .as_ref() - .is_some_and(|r| r.contains(&i)); + let is_selected = + selected_lines.as_ref().is_some_and(|r| r.contains(&i)); let effective_bg = if is_selected { selection_bg } else if is_highlighted { @@ -3016,11 +3449,7 @@ impl Render for DiffViewer { .font_family("Lilex") .text_color(text_col) .child(Self::render_styled_text( - window, - styled, - text_col, - row_height, - true, + window, styled, text_col, row_height, true, )) .into_any_element() } else { @@ -3033,11 +3462,7 @@ impl Render for DiffViewer { .font_family("Lilex") .text_color(text_col) .child(Self::render_styled_text( - window, - styled, - text_col, - row_height, - false, + window, styled, text_col, row_height, false, )) .into_any_element() }; @@ -3112,12 +3537,10 @@ impl Render for DiffViewer { let ctx_name: SharedString = context_name.clone().into(); let has_context = !context_name.is_empty(); let idx = *hunk_index; - let view_clone = view.clone(); let view_sbs_hunk = view.clone(); let view_sbs_hunk_drag = view.clone(); - let is_sbs_hunk_selected = selected_lines - .as_ref() - .is_some_and(|r| r.contains(&i)); + let is_sbs_hunk_selected = + selected_lines.as_ref().is_some_and(|r| r.contains(&i)); let sbs_hunk_bg = if is_sbs_hunk_selected { selection_bg } else { @@ -3187,16 +3610,19 @@ impl Render for DiffViewer { hunk_row = hunk_row.child(div().flex_1()); - // Committed and stashed content is immutable — - // offer no staging button at all. - if let Some(action) = staging_action { + // Same operations as the unified header, from + // the same source decision. + for operation in hunk_operations { + let operation = *operation; + let button_view = view.clone(); hunk_row = hunk_row.child( Button::new( SharedString::from(format!( - "sbs-hunk-stage-{}", + "sbs-hunk-{}-{}", + operation.key(), idx )), - action.hunk_button_label(), + operation.hunk_button_label(), ) .size(ButtonSize::Compact) .style(ButtonStyle::Subtle) @@ -3204,12 +3630,11 @@ impl Render for DiffViewer { move |_: &ClickEvent, _: &mut Window, cx: &mut App| { - view_clone + button_view .update(cx, |_this, cx| { - cx.emit(match action { - StagingAction::Stage => DiffViewerEvent::HunkStageRequested(idx), - StagingAction::Unstage => DiffViewerEvent::HunkUnstageRequested(idx), - }); + cx.emit(DiffViewerEvent::for_hunk( + operation, idx, + )); }) .ok(); }, @@ -3227,21 +3652,20 @@ impl Render for DiffViewer { right_styled, right_kind, } => { - let (left_bg, left_gutter_bg, left_text_col) = - match left_kind { - SideBySideLineKind::Context => { - (editor_bg, gutter_bg, text_color) - } - SideBySideLineKind::Deletion => { - (deleted_line_bg, deleted_gutter_bg, vc_deleted) - } - SideBySideLineKind::Addition => { - (added_line_bg, added_gutter_bg, vc_added) - } - SideBySideLineKind::Empty => { - (empty_fill_bg, gutter_bg, text_placeholder_color) - } - }; + let (left_bg, left_gutter_bg, left_text_col) = match left_kind { + SideBySideLineKind::Context => { + (editor_bg, gutter_bg, text_color) + } + SideBySideLineKind::Deletion => { + (deleted_line_bg, deleted_gutter_bg, vc_deleted) + } + SideBySideLineKind::Addition => { + (added_line_bg, added_gutter_bg, vc_added) + } + SideBySideLineKind::Empty => { + (empty_fill_bg, gutter_bg, text_placeholder_color) + } + }; let (right_bg, right_gutter_bg, right_text_col) = match right_kind { SideBySideLineKind::Context => { @@ -3267,9 +3691,8 @@ impl Render for DiffViewer { .unwrap_or_else(|| " ".to_string()) .into(); let is_highlighted = highlighted_row == Some(i); - let is_sbs_selected = selected_lines - .as_ref() - .is_some_and(|r| r.contains(&i)); + let is_sbs_selected = + selected_lines.as_ref().is_some_and(|r| r.contains(&i)); let effective_left_bg = if is_sbs_selected { selection_bg } else if is_highlighted { @@ -3436,10 +3859,8 @@ impl Render for DiffViewer { } }; - let mut divider = div() - .w(px(2.)) - .flex_shrink_0() - .bg(border_variant); + let mut divider = + div().w(px(2.)).flex_shrink_0().bg(border_variant); divider = if wrap_enabled { divider.min_h(px(row_height)) } else { @@ -3869,6 +4290,9 @@ impl Render for DiffViewer { .v_flex() .size_full() .overflow_hidden() + // `relative` so the file-operations menu can be positioned against + // this container rather than the window. + .relative() // Focus ring so j/k navigation is discoverable: an accent border when the // pane holds keyboard focus, a transparent border of the same width // otherwise so toggling focus never shifts layout or adds chrome. @@ -3958,23 +4382,32 @@ impl Render for DiffViewer { ) .child({ // Always-present partial-mode affordance so the line-level - // staging entry point is discoverable: emphasized while active, - // a muted "Partial (p)" hint otherwise. Hidden for the - // three-way conflict view, which has no line-level staging, - // and for committed/stashed content, which cannot be staged. - let partial_tooltip = if self.partial_mode { - "Line-level staging: select lines, then s / u. Press p to exit." + // entry point is discoverable: emphasized while active, a + // muted "Partial (p)" hint otherwise. The keys it points at + // depend on the source — s/u for working-tree content, a/r + // for anything else. Hidden only for the three-way conflict + // view, which has no line-level operation at all. + let keys = self + .source + .operations() + .iter() + .map(|operation| operation.key()) + .collect::>() + .join(" / "); + let partial_tooltip: SharedString = if self.partial_mode { + format!( + "Line-level changes: select lines, then {keys}. Press p to exit." + ) } else { - "Partial line-level staging — press p to toggle." - }; + format!("Partial line-level changes ({keys}) — press p to toggle.") + } + .into(); let (partial_label, partial_color) = if self.partial_mode { ("Partial", Color::Warning.color(cx)) } else { ("Partial (p)", text_placeholder_color) }; - if self.display_mode == DiffDisplayMode::ThreeWay - || self.source.is_historical() - { + if self.display_mode == DiffDisplayMode::ThreeWay { div().into_any_element() } else { div() @@ -3987,6 +4420,7 @@ impl Render for DiffViewer { .into_any_element() } }) + .child(self.render_file_menu_button(cx)) .child({ let toggle_tooltip = match self.display_mode { DiffDisplayMode::Unified => "Switch to side-by-side view (d)", @@ -4047,6 +4481,13 @@ impl Render for DiffViewer { body = body.child(Scrollbar::horizontal("diff-hscroll", h_handle)); } container = container.child(body); + // Backdrop first so the menu paints on top of it. + if let Some(backdrop) = self.render_file_menu_backdrop(cx) { + container = container.child(backdrop); + } + if let Some(menu) = self.render_file_menu(cx) { + container = container.child(menu); + } container.into_any_element() } @@ -4174,6 +4615,10 @@ mod tests { for source in [ DiffSource::Commit(OID.to_string()), DiffSource::Stash(OID.to_string()), + DiffSource::Compare { + from: "main".to_string(), + to: "feature".to_string(), + }, ] { assert!(source.is_historical(), "{source:?} should be historical"); assert_eq!( @@ -4181,6 +4626,32 @@ mod tests { None, "{source:?} must offer no stage/unstage button or key binding" ); + assert!( + !source.offers(DiffOperation::Stage) && !source.offers(DiffOperation::Unstage), + "{source:?} must offer neither staging operation" + ); + } + } + + #[test] + fn historical_sources_offer_apply_and_revert_instead() { + for source in [ + DiffSource::Commit(OID.to_string()), + DiffSource::Stash(OID.to_string()), + DiffSource::Compare { + from: "main".to_string(), + to: "feature".to_string(), + }, + ] { + assert_eq!( + source.operations(), + &[DiffOperation::Apply, DiffOperation::Revert], + "{source:?} must offer both working-tree operations" + ); + assert!( + source.patch_source().is_some(), + "{source:?} must resolve to a tree pair to generate the patch from" + ); } } @@ -4190,11 +4661,11 @@ mod tests { assert!(!DiffSource::Index.is_historical()); assert_eq!( DiffSource::Worktree.staging_action(), - Some(StagingAction::Stage) + Some(DiffOperation::Stage) ); assert_eq!( DiffSource::Index.staging_action(), - Some(StagingAction::Unstage) + Some(DiffOperation::Unstage) ); } @@ -4214,8 +4685,8 @@ mod tests { #[test] fn hunk_button_labels_match_the_action() { - assert_eq!(StagingAction::Stage.hunk_button_label(), "Stage Hunk"); - assert_eq!(StagingAction::Unstage.hunk_button_label(), "Unstage Hunk"); + assert_eq!(DiffOperation::Stage.hunk_button_label(), "Stage Hunk"); + assert_eq!(DiffOperation::Unstage.hunk_button_label(), "Unstage Hunk"); } #[test] @@ -4223,9 +4694,9 @@ mod tests { // A staging request from a commit diff would reach // `GitProject::stage_hunk_at`, which resolves the hunk index against the // working tree — staging unrelated uncommitted edits. - for action in [StagingAction::Stage, StagingAction::Unstage] { + for action in [DiffOperation::Stage, DiffOperation::Unstage] { let message = DiffSource::Commit(OID.to_string()) - .reject_staging(action) + .reject_operation(action) .expect("staging a commit diff must be rejected"); assert!( message.contains("9f2c1ab"), @@ -4241,26 +4712,171 @@ mod tests { #[test] fn staging_a_stash_diff_is_rejected_with_an_actionable_message() { - for action in [StagingAction::Stage, StagingAction::Unstage] { + for action in [DiffOperation::Stage, DiffOperation::Unstage] { let message = DiffSource::Stash(OID.to_string()) - .reject_staging(action) + .reject_operation(action) .expect("staging a stash diff must be rejected"); assert!( - message.contains("apply or pop"), + message.contains("pop the stash"), "message names the way forward: {message}" ); assert!(message.ends_with('.')); } } + #[test] + fn a_comparison_cannot_be_staged_but_says_what_to_do_instead() { + let source = DiffSource::Compare { + from: "main".to_string(), + to: "feature".to_string(), + }; + for action in [DiffOperation::Stage, DiffOperation::Unstage] { + let message = source + .reject_operation(action) + .expect("a comparison must not be staged"); + assert!( + message.contains("main...feature"), + "message names the comparison: {message}" + ); + assert!( + message.contains("press a to apply"), + "message points at the operation that does work: {message}" + ); + assert!(message.ends_with('.')); + } + } + + #[test] + fn mutable_sources_cannot_apply_or_revert() { + // Working-tree content is already in the working tree, so there is + // nothing to bring in — and `patch_source` has no tree pair for it. + for source in [DiffSource::Worktree, DiffSource::Index] { + for action in [DiffOperation::Apply, DiffOperation::Revert] { + assert!( + !source.offers(action), + "{source:?} must not offer {action:?}" + ); + let message = source + .reject_operation(action) + .expect("the request must be rejected"); + assert!( + message.contains("already in your working tree"), + "got: {message}" + ); + } + assert!(source.patch_source().is_none()); + } + } + + #[test] + fn every_source_offers_at_least_one_operation() { + // No displayable source may be inert: each one has an affordance to render. + for source in [ + DiffSource::Worktree, + DiffSource::Index, + DiffSource::Commit(OID.to_string()), + DiffSource::Stash(OID.to_string()), + DiffSource::Compare { + from: "main".to_string(), + to: "feature".to_string(), + }, + ] { + assert!( + !source.operations().is_empty(), + "{source:?} offers nothing at all" + ); + for operation in source.operations() { + assert_eq!( + source.reject_operation(*operation), + None, + "{source:?} offers {operation:?} but rejects it" + ); + } + } + } + + #[test] + fn a_commit_source_resolves_to_its_own_change() { + let source = DiffSource::Commit(OID.to_string()); + assert_eq!( + source.patch_source(), + Some(WorktreePatchSource::Commit( + git2::Oid::from_str(OID).unwrap() + )) + ); + } + + #[test] + fn a_comparison_resolves_to_both_of_its_endpoints() { + let source = DiffSource::Compare { + from: "main".to_string(), + to: "origin/feature".to_string(), + }; + assert_eq!( + source.patch_source(), + Some(WorktreePatchSource::Compare { + from: "main".to_string(), + to: "origin/feature".to_string(), + }), + "the endpoints must survive verbatim: their order decides which \ + side an apply pulls in" + ); + } + + #[test] + fn revision_labels_name_the_source() { + assert_eq!(DiffSource::Worktree.revision_label(), None); + assert_eq!(DiffSource::Index.revision_label(), None); + assert_eq!( + DiffSource::Commit(OID.to_string()) + .revision_label() + .unwrap(), + "9f2c1ab" + ); + assert_eq!( + DiffSource::Compare { + from: "main".to_string(), + to: "feature".to_string(), + } + .revision_label() + .unwrap(), + "main...feature" + ); + } + + #[test] + fn operation_labels_and_keys_are_distinct_per_operation() { + let operations = [ + DiffOperation::Stage, + DiffOperation::Unstage, + DiffOperation::Apply, + DiffOperation::Revert, + ]; + let keys: Vec<&str> = operations.iter().map(|o| o.key()).collect(); + let unique: HashSet<&&str> = keys.iter().collect(); + assert_eq!(unique.len(), keys.len(), "keys collide: {keys:?}"); + for operation in operations { + assert!(!operation.hunk_button_label().is_empty()); + assert!(!operation.file_menu_label().is_empty()); + } + assert!(DiffOperation::Stage.is_staging()); + assert!(DiffOperation::Unstage.is_staging()); + assert!(!DiffOperation::Apply.is_staging()); + assert!(!DiffOperation::Revert.is_staging()); + assert!(DiffOperation::Apply.writes_files()); + assert!(DiffOperation::Revert.writes_files()); + assert!(!DiffOperation::Stage.writes_files()); + assert!(!DiffOperation::Unstage.writes_files()); + } + #[test] fn valid_staging_requests_are_not_rejected() { assert_eq!( - DiffSource::Worktree.reject_staging(StagingAction::Stage), + DiffSource::Worktree.reject_operation(DiffOperation::Stage), None ); assert_eq!( - DiffSource::Index.reject_staging(StagingAction::Unstage), + DiffSource::Index.reject_operation(DiffOperation::Unstage), None ); } @@ -4268,10 +4884,10 @@ mod tests { #[test] fn mismatched_working_tree_requests_are_rejected() { assert!(DiffSource::Worktree - .reject_staging(StagingAction::Unstage) + .reject_operation(DiffOperation::Unstage) .is_some()); assert!(DiffSource::Index - .reject_staging(StagingAction::Stage) + .reject_operation(DiffOperation::Stage) .is_some()); } @@ -4964,7 +5580,7 @@ mod view_tests { use rgitui_git::{DiffHunk, DiffLine, FileChangeKind, FileDiff}; use rgitui_test_support::ViewTest; - use super::{DiffSource, DiffViewer, DiffViewerEvent}; + use super::{DiffOperation, DiffSource, DiffViewer, DiffViewerEvent, WorktreePatchScope}; const OID: &str = "9f2c1ab4d5e6f708192a3b4c5d6e7f8091a2b3c4"; const PATH: &str = "src/main.rs"; @@ -4996,17 +5612,36 @@ mod view_tests { } } - /// Emitted stage/unstage requests, ignoring the `DiffChanged` - /// notification that every `set_diff` produces. - fn staging_requests(&self) -> Vec { + /// Emitted requests, ignoring the `DiffChanged` notification that every + /// `set_diff` produces. + fn request_events(&self) -> Vec { self.events .iter() .filter(|event| !matches!(event, DiffViewerEvent::DiffChanged { .. })) + .cloned() + .collect() + } + + /// [`Self::request_events`] as debug strings, for assertions that only + /// care that nothing (or one named thing) came out. + fn requests(&self) -> Vec { + self.request_events() + .iter() .map(|event| format!("{event:?}")) .collect() } } + /// Shows `diff` from `source` in the probe's viewer and focuses it. + fn show(probe: &mut ViewTest, diff: FileDiff, source: DiffSource) { + probe.update(|probe, window, cx| { + probe.viewer.update(cx, |viewer, cx| { + viewer.set_diff(diff, PATH.to_string(), source, cx); + viewer.focus(window, cx); + }); + }); + } + impl Render for StagingProbe { fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { div().size_full().child(self.viewer.clone()) @@ -5045,13 +5680,7 @@ mod view_tests { /// the entry points the actions call refuse on historical content. fn staging_requests_after_stage_then_unstage(source: DiffSource) -> Vec { let mut probe = ViewTest::open(StagingProbe::new); - - probe.update(|probe, window, cx| { - probe.viewer.update(cx, |viewer, cx| { - viewer.set_diff(one_hunk_diff(), PATH.to_string(), source, cx); - viewer.focus(window, cx); - }); - }); + show(&mut probe, one_hunk_diff(), source); // Guard against a vacuous test: the rows must actually exist, or the // selection would find no hunk regardless of provenance. @@ -5072,7 +5701,7 @@ mod view_tests { }); }); - probe.read(|probe, _| probe.staging_requests()) + probe.read(|probe, _| probe.requests()) } #[test] @@ -5118,21 +5747,17 @@ mod view_tests { ); } + /// Partial mode is available on committed content, whose line-level operation + /// is applying or reverting those lines in the working tree. Staging is not, + /// in partial mode either. #[test] - fn partial_staging_mode_cannot_be_entered_on_a_commit_diff() { + fn partial_mode_on_a_commit_diff_still_refuses_to_stage() { let mut probe = ViewTest::open(StagingProbe::new); - - probe.update(|probe, window, cx| { - probe.viewer.update(cx, |viewer, cx| { - viewer.set_diff( - one_hunk_diff(), - PATH.to_string(), - DiffSource::Commit(OID.to_string()), - cx, - ); - viewer.focus(window, cx); - }); - }); + show( + &mut probe, + one_hunk_diff(), + DiffSource::Commit(OID.to_string()), + ); probe.update(|probe, _window, cx| { probe @@ -5141,8 +5766,23 @@ mod view_tests { }); probe.read(|probe, cx| { assert!( - !probe.viewer.read(cx).partial_mode, - "line-level staging must stay unavailable for committed content" + probe.viewer.read(cx).partial_mode, + "line-level apply/revert needs partial mode, so `p` must work here" + ); + }); + + probe.update(|probe, _window, cx| { + probe.viewer.update(cx, |viewer, cx| { + viewer.select_all_lines(cx); + viewer.stage_selection(cx); + viewer.unstage_selection(cx); + }); + }); + probe.read(|probe, _| { + assert!( + probe.requests().is_empty(), + "partial mode must not open a staging route for committed content: {:?}", + probe.requests() ); }); } @@ -5150,13 +5790,7 @@ mod view_tests { #[test] fn partial_staging_mode_still_toggles_on_a_worktree_diff() { let mut probe = ViewTest::open(StagingProbe::new); - - probe.update(|probe, window, cx| { - probe.viewer.update(cx, |viewer, cx| { - viewer.set_diff(one_hunk_diff(), PATH.to_string(), DiffSource::Worktree, cx); - viewer.focus(window, cx); - }); - }); + show(&mut probe, one_hunk_diff(), DiffSource::Worktree); probe.update(|probe, _window, cx| { probe @@ -5171,4 +5805,260 @@ mod view_tests { }); probe.read(|probe, cx| assert!(!probe.viewer.read(cx).partial_mode)); } + + // ── apply / revert affordances ──────────────────────────────── + + /// Shows `source` in a focused viewer, selects every row, then runs `act` + /// against the viewer and returns the requests that came back. + /// + /// `act` calls the same public methods the `diff::*` actions dispatch to + /// rather than simulating keystrokes: those bindings are gpui actions declared + /// in `rgitui_workspace`, which sits above this crate, so a keystroke pressed + /// here reaches no handler. The keystrokes are pinned by the keymap registry's + /// own tests. + fn requests_after( + source: DiffSource, + act: impl FnOnce(&mut DiffViewer, &mut Context), + ) -> Vec { + let mut probe = ViewTest::open(StagingProbe::new); + show(&mut probe, one_hunk_diff(), source); + probe.read(|probe, cx| { + assert!( + probe.viewer.read(cx).row_count() > 0, + "display rows should be prepared before the command is invoked" + ); + }); + probe.update(|probe, _window, cx| { + probe.viewer.update(cx, |viewer, cx| { + viewer.select_all_lines(cx); + act(viewer, cx); + }); + }); + probe.read(|probe, _| probe.request_events()) + } + + fn historical_sources() -> Vec { + vec![ + DiffSource::Commit(OID.to_string()), + DiffSource::Stash(OID.to_string()), + DiffSource::Compare { + from: "main".to_string(), + to: "feature".to_string(), + }, + ] + } + + #[test] + fn applying_content_from_outside_the_working_tree_requests_an_apply() { + for source in historical_sources() { + let requests = requests_after(source.clone(), |viewer, cx| viewer.apply_selection(cx)); + assert_eq!( + requests, + vec![DiffViewerEvent::WorktreePatchRequested { + operation: DiffOperation::Apply, + scope: WorktreePatchScope::Hunk(0), + }], + "{source:?} should offer to apply its hunk" + ); + } + } + + #[test] + fn reverting_content_from_outside_the_working_tree_requests_a_revert() { + for source in historical_sources() { + let requests = requests_after(source.clone(), |viewer, cx| viewer.revert_selection(cx)); + assert_eq!( + requests, + vec![DiffViewerEvent::WorktreePatchRequested { + operation: DiffOperation::Revert, + scope: WorktreePatchScope::Hunk(0), + }], + "{source:?} should offer to revert its hunk" + ); + } + } + + /// Working-tree content must not offer to apply or revert itself: it is + /// already in the working tree, and the git layer has no tree pair to + /// generate a patch from. + #[test] + fn applying_or_reverting_a_working_tree_diff_requests_nothing() { + for source in [DiffSource::Worktree, DiffSource::Index] { + let requests = requests_after(source.clone(), |viewer, cx| { + viewer.apply_selection(cx); + viewer.revert_selection(cx); + viewer.apply_file(cx); + viewer.revert_file(cx); + }); + assert!( + requests.is_empty(), + "{source:?} must not emit a working-tree patch request, got {requests:?}" + ); + } + } + + #[test] + fn a_line_selection_scopes_the_apply_to_those_lines() { + // Partial mode plus a whole-diff row selection; the one change line in + // the fixture is the addition at new line 2. + let requests = requests_after(DiffSource::Commit(OID.to_string()), |viewer, cx| { + viewer.toggle_partial_mode(cx); + viewer.select_all_lines(cx); + viewer.apply_selection(cx); + }); + assert_eq!( + requests, + vec![DiffViewerEvent::WorktreePatchRequested { + operation: DiffOperation::Apply, + scope: WorktreePatchScope::Lines(vec![(None, Some(2))]), + }], + "a manual line selection must narrow the scope to those lines" + ); + } + + #[test] + fn the_file_menu_carries_apply_and_revert_only_for_content_from_elsewhere() { + for source in historical_sources() { + let mut probe = ViewTest::open(StagingProbe::new); + show(&mut probe, one_hunk_diff(), source.clone()); + probe.read(|probe, cx| { + assert_eq!( + probe.viewer.read(cx).file_menu_operations(), + vec![DiffOperation::Apply, DiffOperation::Revert], + "{source:?} needs a whole-file route; the hunk headers cannot express one" + ); + }); + } + + for source in [DiffSource::Worktree, DiffSource::Index] { + let mut probe = ViewTest::open(StagingProbe::new); + show(&mut probe, one_hunk_diff(), source.clone()); + probe.read(|probe, cx| { + assert!( + probe.viewer.read(cx).file_menu_operations().is_empty(), + "{source:?} stages whole files from the sidebar, so it gets no menu" + ); + }); + } + } + + #[test] + fn the_dismiss_backdrop_only_exists_while_the_menu_does() { + // The backdrop covers the diff body to catch outside clicks, so it must + // never outlive the menu — otherwise it silently swallows clicks on the + // diff itself. `file_menu_open` alone is not enough: a source with no + // whole-file operations renders no menu even when the flag is set. + let mut probe = ViewTest::open(StagingProbe::new); + show(&mut probe, one_hunk_diff(), DiffSource::Worktree); + probe.update(|probe, _, cx| { + probe + .viewer + .update(cx, |viewer, _| viewer.file_menu_open = true); + }); + probe.read(|probe, cx| { + assert!( + !probe.viewer.read(cx).file_menu_visible(), + "the worktree has no menu to dismiss, so it must have no backdrop" + ); + }); + + let mut probe = ViewTest::open(StagingProbe::new); + show( + &mut probe, + one_hunk_diff(), + DiffSource::Commit(OID.to_string()), + ); + probe.read(|probe, cx| { + assert!( + !probe.viewer.read(cx).file_menu_visible(), + "the menu starts closed" + ); + }); + probe.update(|probe, _, cx| { + probe + .viewer + .update(cx, |viewer, _| viewer.file_menu_open = true); + }); + probe.read(|probe, cx| { + assert!(probe.viewer.read(cx).file_menu_visible()); + }); + } + + #[test] + fn choosing_apply_from_the_file_menu_requests_the_whole_file() { + let mut probe = ViewTest::open(StagingProbe::new); + show( + &mut probe, + one_hunk_diff(), + DiffSource::Commit(OID.to_string()), + ); + + probe.update(|probe, _, cx| { + probe.viewer.update(cx, |viewer, cx| { + viewer.file_menu_open = true; + viewer.request_whole_file_patch(DiffOperation::Apply, cx); + }); + }); + + probe.read(|probe, cx| { + assert_eq!( + probe.request_events(), + vec![DiffViewerEvent::WorktreePatchRequested { + operation: DiffOperation::Apply, + scope: WorktreePatchScope::File, + }] + ); + assert!( + !probe.viewer.read(cx).file_menu_open, + "choosing an entry should dismiss the menu" + ); + }); + } + + /// The whole-file commands ignore the row selection entirely, so the menu and + /// the keystroke reach the same scope from opposite starting states. + #[test] + fn the_whole_file_commands_request_file_scope_whatever_is_selected() { + for (operation, act) in [ + ( + DiffOperation::Apply, + &DiffViewer::apply_file as &dyn Fn(&mut DiffViewer, &mut Context), + ), + (DiffOperation::Revert, &DiffViewer::revert_file), + ] { + let requests = requests_after(DiffSource::Commit(OID.to_string()), |viewer, cx| { + act(viewer, cx) + }); + assert_eq!( + requests, + vec![DiffViewerEvent::WorktreePatchRequested { + operation, + scope: WorktreePatchScope::File, + }], + "{operation:?} over the whole file must not be narrowed by the selection" + ); + } + } + + #[test] + fn a_hunk_header_button_and_the_key_raise_the_same_request() { + // Both routes go through `for_hunk`, so neither can drift from the other. + for operation in [DiffOperation::Apply, DiffOperation::Revert] { + assert_eq!( + DiffViewerEvent::for_hunk(operation, 3), + DiffViewerEvent::WorktreePatchRequested { + operation, + scope: WorktreePatchScope::Hunk(3), + } + ); + } + assert_eq!( + DiffViewerEvent::for_hunk(DiffOperation::Stage, 3), + DiffViewerEvent::HunkStageRequested(3) + ); + assert_eq!( + DiffViewerEvent::for_hunk(DiffOperation::Unstage, 3), + DiffViewerEvent::HunkUnstageRequested(3) + ); + } } diff --git a/crates/rgitui_workspace/src/workspace/events.rs b/crates/rgitui_workspace/src/workspace/events.rs index 4722614..3879f78 100644 --- a/crates/rgitui_workspace/src/workspace/events.rs +++ b/crates/rgitui_workspace/src/workspace/events.rs @@ -7,7 +7,7 @@ use std::time::Instant; use futures::StreamExt; use gpui::{AppContext, Context, Entity, SharedString}; use rgitui_ai::{AiEvent, AiGenerator}; -use rgitui_diff::{DiffSource, DiffViewer, DiffViewerEvent, StagingAction}; +use rgitui_diff::{DiffOperation, DiffSource, DiffViewer, DiffViewerEvent}; use rgitui_git::{ CommitInfo, GitOperationKind, GitOperationState, GitProject, GitProjectEvent, RebaseEntryAction, RebasePlanEntry, Signature, @@ -866,6 +866,17 @@ pub(super) fn subscribe_project(cx: &mut Context, subs: ProjectSubscr tb.set_ahead_behind(ahead, behind, cx); }); } + GitProjectEvent::WorktreePatchApplied { label, snapshots } => { + // No git command reverses a working-tree rewrite, so the + // pre-operation bytes are what goes on the undo stack. + this.push_undo( + label.clone(), + UndoAction::RestoreWorktreeFiles { + snapshots: snapshots.clone(), + }, + cx, + ); + } GitProjectEvent::RepositoryChanged | GitProjectEvent::StatusChanged | GitProjectEvent::HeadChanged @@ -957,8 +968,7 @@ pub(super) fn subscribe_project(cx: &mut Context, subs: ProjectSubscr if dv.has_three_way_diff() || dv.source().is_historical() { None } else { - let is_staged = - dv.source().staging_action() == Some(StagingAction::Unstage); + let is_staged = dv.source().offers(DiffOperation::Unstage); dv.file_path().map(|p| (p.to_string(), is_staged)) } }; @@ -2326,20 +2336,24 @@ pub(super) fn subscribe_diff_viewer( .map(std::path::PathBuf::from); if let Some(path) = file_path { - // Backstop: never apply a staging request against content the - // viewer is not showing as mutable. `stage_hunk_at` and friends - // resolve the hunk index against the *working tree*, so honouring - // a request raised over a commit or stash diff would silently - // stage unrelated uncommitted changes. + // Backstop: never honour a request the displayed source does not + // offer. `stage_hunk_at` and friends resolve the hunk index + // against the *working tree*, and apply/revert rewrite files on + // disk, so a request that outlived the content it was raised over + // would hit changes the user is not looking at. let requested = match event { DiffViewerEvent::HunkStageRequested(_) - | DiffViewerEvent::LineStageRequested(_) => Some(StagingAction::Stage), + | DiffViewerEvent::LineStageRequested(_) => Some(DiffOperation::Stage), DiffViewerEvent::HunkUnstageRequested(_) - | DiffViewerEvent::LineUnstageRequested(_) => Some(StagingAction::Unstage), + | DiffViewerEvent::LineUnstageRequested(_) => Some(DiffOperation::Unstage), + DiffViewerEvent::WorktreePatchRequested { operation, .. } => Some(*operation), DiffViewerEvent::DiffChanged { .. } => None, }; if let Some(requested) = requested { - let rejection = diff_viewer_ref.read(cx).source().reject_staging(requested); + let rejection = diff_viewer_ref + .read(cx) + .source() + .reject_operation(requested); if let Some(message) = rejection { this.show_toast(message, ToastKind::Warning, cx); return; @@ -2379,6 +2393,36 @@ pub(super) fn subscribe_diff_viewer( .detach(); }); } + DiffViewerEvent::WorktreePatchRequested { operation, scope } => { + // The patch is generated from the displayed source's tree + // pair, so applying a hunk of a comparison brings the + // other revision's content in rather than the index's. + let source = diff_viewer_ref.read(cx).source().patch_source(); + let Some(source) = source else { + this.show_toast( + "This diff has no revision to apply from — select a commit, a \ + stash entry or a comparison first.", + ToastKind::Warning, + cx, + ); + return; + }; + let Some(direction) = operation.patch_direction() else { + return; + }; + let scope = scope.clone(); + project.update(cx, |proj, cx| { + proj.patch_worktree_at( + &path, + source, + scope, + direction, + &worktree_path, + cx, + ) + .detach(); + }); + } DiffViewerEvent::DiffChanged { .. } => {} } } diff --git a/crates/rgitui_workspace/src/workspace/undo.rs b/crates/rgitui_workspace/src/workspace/undo.rs index 536d68c..fd13b55 100644 --- a/crates/rgitui_workspace/src/workspace/undo.rs +++ b/crates/rgitui_workspace/src/workspace/undo.rs @@ -47,6 +47,15 @@ pub enum UndoAction { UnstageFiles { paths: Vec }, /// Undo unstage: stage the file paths. StageFiles { paths: Vec }, + /// Undo an apply/revert of diff content: write the files back byte for byte. + /// + /// The bytes travel with the entry because the forward operation may have gone + /// through a three-way merge, and a reverse patch would then not restore the + /// working tree exactly. `contents: None` means the file did not exist, so + /// undoing deletes it. + RestoreWorktreeFiles { + snapshots: Vec, + }, } impl UndoEntry { @@ -292,6 +301,17 @@ impl Workspace { cx, ); } + UndoAction::RestoreWorktreeFiles { snapshots } => { + project.update(cx, |proj, cx| { + proj.restore_worktree_files_at(snapshots, &worktree_path, cx) + .detach(); + }); + self.show_toast( + format!("Undid: {undo_label}{suffix}"), + ToastKind::Success, + cx, + ); + } } } @@ -492,8 +512,15 @@ mod tests { UndoAction::StageFiles { paths: vec!["c.rs".into()], }, + UndoAction::RestoreWorktreeFiles { + snapshots: vec![rgitui_git::WorktreeFileSnapshot { + path: PathBuf::from("d.rs"), + contents: Some(b"before\n".to_vec()), + }], + }, ]; + let count = actions.len(); let mut stack = UndoStack::new(); for (i, action) in actions.into_iter().enumerate() { stack.push(UndoEntry { @@ -504,11 +531,56 @@ mod tests { worktree_path: PathBuf::new(), }); } - assert_eq!(stack.count(), 7); + assert_eq!(stack.count(), count); // pop all back off without panicking - for _ in 0..7 { + for _ in 0..count { assert!(stack.pop().is_some()); } assert!(stack.pop().is_none()); } + + // ── worktree apply/revert undo ─────────────────────────────── + + #[test] + fn an_apply_undo_entry_carries_the_bytes_it_will_restore() { + let mut stack = UndoStack::new(); + stack.push(UndoEntry { + label: "Applied hunk 1 of src/main.rs from a1b2c3d".to_string(), + action: UndoAction::RestoreWorktreeFiles { + snapshots: vec![ + rgitui_git::WorktreeFileSnapshot { + path: PathBuf::from("src/main.rs"), + contents: Some(b"fn main() {}\n".to_vec()), + }, + rgitui_git::WorktreeFileSnapshot { + path: PathBuf::from("src/new.rs"), + contents: None, + }, + ], + }, + created_at: Instant::now(), + repo_path: PathBuf::from("/repo"), + worktree_path: PathBuf::from("/repo"), + }); + + let entry = stack.pop().expect("the entry should be undoable"); + let UndoAction::RestoreWorktreeFiles { snapshots } = entry.action else { + panic!("expected a worktree restore, got {:?}", entry.action); + }; + assert_eq!(snapshots.len(), 2); + assert_eq!(snapshots[0].path, PathBuf::from("src/main.rs")); + assert_eq!( + snapshots[0].contents.as_deref(), + Some(&b"fn main() {}\n"[..]) + ); + assert_eq!( + snapshots[1].contents, None, + "a file the apply created must be recorded as absent so undo deletes it" + ); + assert_eq!( + entry.worktree_path, + PathBuf::from("/repo"), + "undo must route back to the worktree the files were written in" + ); + } } From ce5bff5623da148cc782f0e341f700896859df55 Mon Sep 17 00:00:00 2001 From: adehad <26027314+adehad@users.noreply.github.com> Date: Mon, 3 Aug 2026 12:27:17 +0100 Subject: [PATCH 3/9] feat(keymap): make apply and revert rebindable commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The apply/revert affordances arrived as buttons and a menu, which leaves the feature mouse-only and absent from the shortcut reference. Declaring them in the `commands!` registry is what puts them on a keystroke, in `docs/KEYBINDINGS.md`, and under the user's control in `keymap.json`. Four commands in the `diff` namespace, split the way staging already is — selection-scoped and fixed-scope: * `diff::ApplySelection` / `diff::RevertSelection` on `a` and `r` (`shift-a` / `shift-r` too, matching `s`/`shift-s`), acting on the hunk under the cursor, the hunks the row selection spans, or the selected lines in partial mode. * `diff::ApplyFile` / `diff::RevertFile` on `alt-a` and `alt-r`, the whole file regardless of the selection. Alt for the fixed-scope variant is the convention `alt-s`/`alt-u` already set. Bare `a` and `r` are free in `DiffViewer && !modal && !TextInput`: the viewer's own `a` binding is `secondary-a` for select-all, and the only other bare `r` in the registry is the rebase editor's reword, which sits behind `modal` and so never competes. They are also the letters that read as apply and revert, and they are live exactly where `s` and `u` are inert. `a` and `r` join the list in `ambiguous_letters_resolve_to_one_action_per_ context` and the ownership table in `the_overloaded_letters_are_owned_by_the_expected_views`, so a later block cannot take either letter without saying so. Co-Authored-By: Claude Fable 5 --- .../rgitui_workspace/src/keymap/registry.rs | 23 ++++++++++++++++++- .../src/workspace/commands.rs | 8 +++++++ docs/KEYBINDINGS.md | 4 ++++ docs/keymap.schema.json | 16 +++++++++++++ 4 files changed, 50 insertions(+), 1 deletion(-) diff --git a/crates/rgitui_workspace/src/keymap/registry.rs b/crates/rgitui_workspace/src/keymap/registry.rs index 50e3841..efd078f 100644 --- a/crates/rgitui_workspace/src/keymap/registry.rs +++ b/crates/rgitui_workspace/src/keymap/registry.rs @@ -291,6 +291,21 @@ commands! { StageCurrentHunk "alt-s" [hidden]; /// Unstage the hunk under the diff cursor. UnstageCurrentHunk "alt-u" [hidden]; + // Bare `a` and `r`, live only for content from outside the working tree, + // which is where `s`/`u` above are inert. The diff viewer's own `a` is + // `secondary-a`, and the rebase editor's `r` sits behind `modal`, so + // neither collides. + /// Apply the hunks or lines under the diff selection to the working tree. + ApplySelection ["a", "shift-a"] [hidden]; + /// Revert the hunks or lines under the diff selection in the working tree. + RevertSelection ["r", "shift-r"] [hidden]; + // Alt for the fixed-scope variant, as with `alt-s`/`alt-u`: the selection + // is ignored and the whole file is rewritten. Both record the overwritten + // bytes on the undo stack. + /// Apply the whole file to the working tree. + ApplyFile "alt-a" [hidden]; + /// Revert the whole file in the working tree. + RevertFile "alt-r" [hidden]; /// Copy the selected diff lines to the clipboard. CopyDiffSelection "secondary-c" [hidden]; /// Select every line in the diff. @@ -883,7 +898,9 @@ mod tests { /// context — otherwise one of them is dead. #[test] fn ambiguous_letters_resolve_to_one_action_per_context() { - for keystrokes in ["d", "s", "p", "b", "h", "j", "k", "g", "y", "/", "[", "]"] { + for keystrokes in [ + "d", "s", "p", "b", "h", "j", "k", "g", "y", "r", "/", "[", "]", + ] { let bindings = bindings_for(keystrokes); assert!( !bindings.is_empty(), @@ -933,6 +950,10 @@ mod tests { ("p", &["diff::TogglePartialSelection", "rebase::RebasePick"]), ("b", &["history::HistoryShowBlame"]), ("h", &["blame::BlameShowHistory"]), + // The rebase editor's `r` sits behind `modal`, so it never meets a + // `DiffViewer && !modal` binding. + ("a", &["diff::ApplySelection"]), + ("r", &["diff::RevertSelection", "rebase::RebaseReword"]), ]; for (keystrokes, actions) in expected { diff --git a/crates/rgitui_workspace/src/workspace/commands.rs b/crates/rgitui_workspace/src/workspace/commands.rs index b840fa7..208d712 100644 --- a/crates/rgitui_workspace/src/workspace/commands.rs +++ b/crates/rgitui_workspace/src/workspace/commands.rs @@ -185,6 +185,10 @@ impl Workspace { CommandId::UnstageSelection => diff.unstage_selection(cx), CommandId::StageCurrentHunk => diff.stage_current_hunk(cx), CommandId::UnstageCurrentHunk => diff.unstage_current_hunk(cx), + CommandId::ApplySelection => diff.apply_selection(cx), + CommandId::RevertSelection => diff.revert_selection(cx), + CommandId::ApplyFile => diff.apply_file(cx), + CommandId::RevertFile => diff.revert_file(cx), CommandId::CopyDiffSelection => diff.copy_selection(cx), CommandId::SelectAllDiffLines => diff.select_all_lines(cx), _ => cx.propagate(), @@ -818,6 +822,10 @@ impl Workspace { | CommandId::UnstageSelection | CommandId::StageCurrentHunk | CommandId::UnstageCurrentHunk + | CommandId::ApplySelection + | CommandId::RevertSelection + | CommandId::ApplyFile + | CommandId::RevertFile | CommandId::CopyDiffSelection | CommandId::SelectAllDiffLines | CommandId::ToggleFileTree diff --git a/docs/KEYBINDINGS.md b/docs/KEYBINDINGS.md index 71a2389..28ddbdc 100644 --- a/docs/KEYBINDINGS.md +++ b/docs/KEYBINDINGS.md @@ -154,6 +154,10 @@ The file is reloaded when you save it. Bindings you add win over the defaults. T | `u` or `shift-u` | `DiffViewer && !modal && !TextInput` | `diff::UnstageSelection` | Unstage the hunks or lines under the diff selection. | | `alt-s` | `DiffViewer && !modal && !TextInput` | `diff::StageCurrentHunk` | Stage the hunk under the diff cursor. | | `alt-u` | `DiffViewer && !modal && !TextInput` | `diff::UnstageCurrentHunk` | Unstage the hunk under the diff cursor. | +| `a` or `shift-a` | `DiffViewer && !modal && !TextInput` | `diff::ApplySelection` | Apply the hunks or lines under the diff selection to the working tree. | +| `r` or `shift-r` | `DiffViewer && !modal && !TextInput` | `diff::RevertSelection` | Revert the hunks or lines under the diff selection in the working tree. | +| `alt-a` | `DiffViewer && !modal && !TextInput` | `diff::ApplyFile` | Apply the whole file to the working tree. | +| `alt-r` | `DiffViewer && !modal && !TextInput` | `diff::RevertFile` | Revert the whole file in the working tree. | | `secondary-c` | `DiffViewer && !modal && !TextInput` | `diff::CopyDiffSelection` | Copy the selected diff lines to the clipboard. | | `secondary-a` | `DiffViewer && !modal && !TextInput` | `diff::SelectAllDiffLines` | Select every line in the diff. | diff --git a/docs/keymap.schema.json b/docs/keymap.schema.json index 5d1255d..ed7c07c 100644 --- a/docs/keymap.schema.json +++ b/docs/keymap.schema.json @@ -456,6 +456,22 @@ "const": "diff::UnstageCurrentHunk", "description": "Unstage the hunk under the diff cursor. Command id: `unstage_current_hunk`." }, + { + "const": "diff::ApplySelection", + "description": "Apply the hunks or lines under the diff selection to the working tree. Command id: `apply_selection`." + }, + { + "const": "diff::RevertSelection", + "description": "Revert the hunks or lines under the diff selection in the working tree. Command id: `revert_selection`." + }, + { + "const": "diff::ApplyFile", + "description": "Apply the whole file to the working tree. Command id: `apply_file`." + }, + { + "const": "diff::RevertFile", + "description": "Revert the whole file in the working tree. Command id: `revert_file`." + }, { "const": "diff::CopyDiffSelection", "description": "Copy the selected diff lines to the clipboard. Command id: `copy_diff_selection`." From 9a805207840a6931feec3336c231ebf3c6edc810 Mon Sep 17 00:00:00 2001 From: adehad <26027314+adehad@users.noreply.github.com> Date: Mon, 3 Aug 2026 12:27:49 +0100 Subject: [PATCH 4/9] docs(git): state the worktree-patch constraints without arguing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `worktree_patch` docs were written as a case for the design: "Why not `repo.apply(..)`", "Why not `git apply --3way`", what staging does by contrast, what stops a cross-branch apply being a special case. The next reader has no memory of the alternatives being weighed, so that framing is rot; the facts inside it are not — libgit2's apply has no three-way fallback, and `git apply --3way` merges into the index and refuses when the working tree differs from it are both non-obvious dependency behaviours someone would otherwise rediscover against a dirty working tree. Keep the constraints, drop the argument. Same treatment for `WorktreePatchSource`, `WorktreeFileSnapshot` and `GitProjectEvent::WorktreePatchApplied`, whose docs justified themselves against staging rather than saying what they hold. Co-Authored-By: Claude Fable 5 --- crates/rgitui_git/src/project/mod.rs | 4 +- .../rgitui_git/src/project/worktree_patch.rs | 70 ++++++++----------- 2 files changed, 30 insertions(+), 44 deletions(-) diff --git a/crates/rgitui_git/src/project/mod.rs b/crates/rgitui_git/src/project/mod.rs index db455f6..9bc91d8 100644 --- a/crates/rgitui_git/src/project/mod.rs +++ b/crates/rgitui_git/src/project/mod.rs @@ -263,8 +263,8 @@ pub enum GitProjectEvent { AheadBehindRefreshed, OperationUpdated(GitOperationUpdate), /// An apply/revert rewrote working-tree files on disk. Carries what those - /// files held beforehand so the workspace can offer an exact undo; unlike - /// staging, these operations are not recoverable from git state alone. + /// files held beforehand, which is the only record of it: the previous + /// contents are not recoverable from git state. WorktreePatchApplied { /// Undo label, e.g. "Applied hunk 2 of src/main.rs from a1b2c3d". label: String, diff --git a/crates/rgitui_git/src/project/worktree_patch.rs b/crates/rgitui_git/src/project/worktree_patch.rs index f9c7978..51cebb6 100644 --- a/crates/rgitui_git/src/project/worktree_patch.rs +++ b/crates/rgitui_git/src/project/worktree_patch.rs @@ -1,46 +1,37 @@ //! Applying and reverting diff content in the working tree. //! -//! Staging moves content between the working tree and the index, so it can be -//! expressed as `repo.apply(.., ApplyLocation::Index, ..)` over a patch sliced -//! out of the index→workdir diff. Applying *historical* content is a different -//! operation: the patch comes from a pair of trees that need not include the -//! working tree at all — a past commit against its parent, a stash entry, or -//! two arbitrary branches — and the result is written to files on disk. +//! The patch comes from a pair of trees that need not include the working tree +//! at all — a past commit against its parent, a stash entry, or two arbitrary +//! branches — and the result is written to files on disk. A dirty working tree is +//! the normal case: you compare against another revision precisely because you +//! are mid-change. //! -//! ## Why not `repo.apply(.., ApplyLocation::WorkDir, ..)` +//! ## Constraints on the two obvious appliers //! -//! libgit2's apply is a plain patch applier: it matches each hunk's context -//! against the target and fails outright when the context does not line up. It -//! has no three-way fallback. The working tree being dirty is the *normal* case -//! for this feature — you compare against another branch precisely because you -//! are mid-change — and an uncommitted edit anywhere inside a hunk's context -//! window (three lines either side) is enough to make a context match fail. +//! Neither off-the-shelf applier tolerates that dirty working tree: //! -//! ## Why not `git apply --3way` +//! * `repo.apply(.., ApplyLocation::WorkDir, ..)` matches each hunk's context +//! against the target and fails outright when it does not line up, with no +//! three-way fallback. An uncommitted edit anywhere inside a hunk's context +//! window (three lines either side) defeats it. +//! * `git apply --3way` merges into the *index*, and refuses with +//! `error: : does not match index` whenever the working-tree file differs +//! from its index entry. On failure it also leaves conflict markers and an +//! unmerged index entry behind. //! -//! `git apply --3way` is the CLI's answer to that, and it is what a patch-file -//! workflow effectively gets. But it merges into the *index*: it refuses with -//! `error: : does not match index` whenever the working-tree file differs -//! from its index entry. That is exactly the dirty working tree we need to -//! support, so the one case the fallback exists for is the case it rejects. -//! It also leaves conflict markers plus an unmerged index entry behind on -//! failure, which is a worse state to hand back than a refusal. +//! ## What this module does //! -//! ## What this does instead -//! -//! Reconstruct the two sides ourselves and let libgit2 merge them: +//! Reconstructs the two sides and lets libgit2 merge them: //! //! * `base` — the file as it is on the side the patch starts from. //! * `target` — `base` rewritten with exactly the selected hunk or lines -//! applied (or reverted). Computed from the diff, not by matching context, so -//! it is exact by construction. +//! applied (or reverted). Computed from the diff rather than by matching +//! context, so it is exact by construction. //! * `ours` — the file as it is in the working tree right now. //! -//! Then a three-way merge of (base, ours, target). Because base→target differs -//! only inside the selected region, unrelated local edits merge cleanly and an -//! overlapping edit conflicts — which is the behaviour `--3way` promises, -//! obtained without touching the index and without needing the working tree to -//! match it. +//! A three-way merge of (base, ours, target) then keeps unrelated local edits +//! and conflicts only where an edit overlaps the selected region. The index is +//! never touched and never has to match the working tree. use std::collections::HashSet; use std::path::{Path, PathBuf}; @@ -126,11 +117,8 @@ impl WorktreePatchScope { /// The pair of revisions whose difference is being applied or reverted. /// -/// `Commit` is not special-cased into the apply machinery: it resolves to the -/// same `from`/`to` tree pair as any other comparison, so a cross-branch diff -/// and a historical commit diff travel identical code. That is what makes -/// "apply the difference between two branches into my working tree" work rather -/// than being a no-op bolted onto a commit-only feature. +/// Both variants resolve to a `from`/`to` tree pair, so a commit diff and a +/// cross-branch comparison travel identical code from there on. #[derive(Debug, Clone, PartialEq, Eq)] pub enum WorktreePatchSource { /// The change one commit (or stash entry) introduced: its first parent → @@ -198,10 +186,8 @@ pub(crate) fn short_oid(oid_hex: &str) -> String { /// The contents of one working-tree file before an apply or revert rewrote it. /// -/// Apply and revert edit files on disk, unlike staging, so every operation -/// snapshots what it is about to overwrite. Undo restores these bytes verbatim -/// rather than re-deriving a reverse patch, so it is exact even when the forward -/// operation went through a three-way merge. +/// Undo restores these bytes verbatim rather than deriving a reverse patch, which +/// would not be exact when the forward operation went through a three-way merge. #[derive(Debug, Clone, PartialEq, Eq)] pub struct WorktreeFileSnapshot { /// Path relative to the worktree root. @@ -1772,8 +1758,8 @@ mod worktree_patch_integration_tests { #[test] fn libgit2s_own_apply_refuses_the_case_the_merge_handles() { - // The evidence behind not using `repo.apply(.., WorkDir, ..)`: the same - // hunk, as a patch, against the same working tree that + // Pins the libgit2 constraint the module docs state: the same hunk, as a + // patch, against the same working tree that // `applying_over_an_unrelated_local_edit_keeps_both_changes` handles. let (fixture, _) = two_hunk_commit(); fixture.write("f.txt", &numbered_lines(&[(5, "LOCAL5")])); From 880f1aa4c3a17f6d802c939da997850cfc01250f Mon Sep 17 00:00:00 2001 From: Noah Clarkson Date: Sun, 9 Aug 2026 18:52:10 +1200 Subject: [PATCH 5/9] fix(git): harden working-tree patch mutations --- crates/rgitui_git/src/project/mod.rs | 10 +- .../rgitui_git/src/project/worktree_patch.rs | 1274 +++++++++++++++-- .../rgitui_workspace/src/workspace/events.rs | 11 +- crates/rgitui_workspace/src/workspace/undo.rs | 89 +- 4 files changed, 1251 insertions(+), 133 deletions(-) diff --git a/crates/rgitui_git/src/project/mod.rs b/crates/rgitui_git/src/project/mod.rs index 9bc91d8..da22ac5 100644 --- a/crates/rgitui_git/src/project/mod.rs +++ b/crates/rgitui_git/src/project/mod.rs @@ -81,9 +81,9 @@ pub use submodule::{ SubmoduleInfo, }; pub use worktree_patch::{ - apply_worktree_patch, restore_worktree_files, snapshots_fit_undo_stack, WorktreeFileSnapshot, - WorktreePatchDirection, WorktreePatchOutcome, WorktreePatchScope, WorktreePatchSource, - MAX_UNDO_SNAPSHOT_BYTES, + apply_worktree_patch, restore_worktree_files, snapshots_fit_undo_stack, + WorktreeFilePermissions, WorktreeFileSnapshot, WorktreeFileState, WorktreePatchDirection, + WorktreePatchOutcome, WorktreePatchScope, WorktreePatchSource, MAX_UNDO_SNAPSHOT_BYTES, }; fn parse_remote_tracking_ref(name: &str) -> Option<(String, String)> { @@ -269,6 +269,10 @@ pub enum GitProjectEvent { /// Undo label, e.g. "Applied hunk 2 of src/main.rs from a1b2c3d". label: String, snapshots: Vec, + /// Repository entity that started the asynchronous mutation. + repo_path: PathBuf, + /// Effective worktree captured when the operation started. + worktree_path: PathBuf, }, } diff --git a/crates/rgitui_git/src/project/worktree_patch.rs b/crates/rgitui_git/src/project/worktree_patch.rs index 51cebb6..42e193d 100644 --- a/crates/rgitui_git/src/project/worktree_patch.rs +++ b/crates/rgitui_git/src/project/worktree_patch.rs @@ -34,10 +34,19 @@ //! never touched and never has to match the working tree. use std::collections::HashSet; +use std::ffi::OsString; +use std::io::{Seek as _, Write as _}; use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; use anyhow::{Context as _, Result}; -use git2::{IndexEntry, IndexTime, Oid, Repository}; +use git2::{FileMode, IndexEntry, IndexTime, Oid, Repository}; + +/// Serializes every in-process filesystem transaction performed by this +/// module. The filesystem compare-and-swap checks still defend against edits +/// from editors and other processes; this lock prevents two background tasks +/// in rgitui from both accepting the same pre-image. +static WORKTREE_MUTATION_LOCK: parking_lot::Mutex<()> = parking_lot::Mutex::new(()); /// Which side of a diff the working tree should be moved toward. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -184,16 +193,39 @@ pub(crate) fn short_oid(oid_hex: &str) -> String { oid_hex[..7.min(oid_hex.len())].to_string() } -/// The contents of one working-tree file before an apply or revert rewrote it. +/// A regular working-tree file's exact on-disk state. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct WorktreeFileState { + /// Bytes as stored in the working tree, after Git's smudge/encoding/EOL + /// conversions rather than the canonical bytes stored in an object. + pub contents: Vec, + /// Permissions needed to restore the file without silently changing its + /// executable or read-only state. + pub permissions: WorktreeFilePermissions, +} + +/// Portable portion of a file's permissions, plus the complete Unix mode when +/// that platform makes it available. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct WorktreeFilePermissions { + pub readonly: bool, + #[cfg(unix)] + pub mode: u32, +} + +/// The state of one working-tree file around an apply or revert. /// -/// Undo restores these bytes verbatim rather than deriving a reverse patch, which -/// would not be exact when the forward operation went through a three-way merge. +/// Undo restores `before` only while the path still equals `expected_after`. +/// That compare-and-swap contract prevents Undo from destroying edits made +/// after the operation completed. #[derive(Debug, Clone, PartialEq, Eq)] pub struct WorktreeFileSnapshot { /// Path relative to the worktree root. pub path: PathBuf, - /// Contents before the operation; `None` when the file did not exist. - pub contents: Option>, + /// State before the operation; `None` when the file did not exist. + pub before: Option, + /// State written by the operation; `None` when it deleted the file. + pub expected_after: Option, } /// Largest total snapshot the undo stack will hold for one operation. Past this @@ -207,7 +239,9 @@ pub const MAX_UNDO_SNAPSHOT_BYTES: usize = 4 * 1024 * 1024; pub fn snapshots_fit_undo_stack(snapshots: &[WorktreeFileSnapshot]) -> bool { snapshots .iter() - .filter_map(|s| s.contents.as_ref().map(Vec::len)) + .flat_map(|snapshot| [snapshot.before.as_ref(), snapshot.expected_after.as_ref()]) + .flatten() + .map(|state| state.contents.len()) .sum::() <= MAX_UNDO_SNAPSHOT_BYTES } @@ -215,7 +249,7 @@ pub fn snapshots_fit_undo_stack(snapshots: &[WorktreeFileSnapshot]) -> bool { /// Outcome of a successful working-tree apply or revert. #[derive(Debug, Clone, PartialEq, Eq)] pub struct WorktreePatchOutcome { - /// Pre-operation contents of every file the operation rewrote. + /// Exact before/after states of every file the operation rewrote. pub snapshots: Vec, /// True when the change could not be dropped in verbatim and was merged /// around unrelated local edits. Worth telling the user about: their file @@ -491,6 +525,15 @@ struct FileDiffSides { old_blob: Option, /// Blob on the diff's new side, `None` when the file was deleted. new_blob: Option, + old_mode: FileMode, + new_mode: FileMode, +} + +fn is_regular_blob_mode(mode: FileMode) -> bool { + matches!( + mode, + FileMode::Blob | FileMode::BlobGroupWritable | FileMode::BlobExecutable + ) } /// Read `file_path`'s diff out of `source` as hunks of [`ScopedLine`]s. @@ -506,23 +549,41 @@ fn scoped_hunks( let (from_tree, to_tree) = source.trees(repo)?; let diff = repo.diff_tree_to_tree(from_tree.as_ref(), to_tree.as_ref(), None)?; - for delta_index in 0..diff.deltas().len() { - let patch = match git2::Patch::from_diff(&diff, delta_index) { - Ok(Some(patch)) => patch, - _ => continue, - }; - let old_path = patch.delta().old_file().path().map(Path::to_path_buf); - let new_path = patch.delta().new_file().path().map(Path::to_path_buf); + for (delta_index, delta) in diff.deltas().enumerate() { + let old_path = delta.old_file().path().map(Path::to_path_buf); + let new_path = delta.new_file().path().map(Path::to_path_buf); if old_path.as_deref() != Some(file_path) && new_path.as_deref() != Some(file_path) { continue; } + let old_blob = delta.old_file().id(); + let new_blob = delta.new_file().id(); + let old_blob = (!old_blob.is_zero()).then_some(old_blob); + let new_blob = (!new_blob.is_zero()).then_some(new_blob); + if old_blob.is_some() && !is_regular_blob_mode(delta.old_file().mode()) + || new_blob.is_some() && !is_regular_blob_mode(delta.new_file().mode()) + { + anyhow::bail!( + "Can't patch {} because one side is not a regular file. Symbolic links and submodules are not supported by working-tree content patches.", + file_path.display() + ); + } + // Validate both canonical sides before asking libgit2 for line hunks. + // Binary/invalid text must never be silently replacement-decoded. + blob_text(repo, old_blob, file_path)?; + blob_text(repo, new_blob, file_path)?; + let patch = git2::Patch::from_diff(&diff, delta_index)?.ok_or_else(|| { + anyhow::anyhow!( + "Can't patch {} because Git does not expose it as a textual diff.", + file_path.display() + ) + })?; + let mut hunks = Vec::with_capacity(patch.num_hunks()); for hunk_index in 0..patch.num_hunks() { let mut lines = Vec::new(); for line_index in 0..patch.num_lines_in_hunk(hunk_index)? { let line = patch.line_in_hunk(hunk_index, line_index)?; - let text = String::from_utf8_lossy(line.content()).to_string(); match line.origin() { ' ' => { if let (Some(old), Some(new)) = (line.old_lineno(), line.new_lineno()) { @@ -534,6 +595,14 @@ fn scoped_hunks( } '+' => { if let Some(new) = line.new_lineno() { + let text = std::str::from_utf8(line.content()) + .with_context(|| { + format!( + "Can't patch {} because its canonical Git content is not valid UTF-8.", + file_path.display() + ) + })? + .to_owned(); lines.push(ScopedLine::Addition { new_lineno: new as usize, text, @@ -542,6 +611,14 @@ fn scoped_hunks( } '-' => { if let Some(old) = line.old_lineno() { + let text = std::str::from_utf8(line.content()) + .with_context(|| { + format!( + "Can't patch {} because its canonical Git content is not valid UTF-8.", + file_path.display() + ) + })? + .to_owned(); lines.push(ScopedLine::Deletion { old_lineno: old as usize, text, @@ -558,12 +635,12 @@ fn scoped_hunks( hunks.push(lines); } - let old_blob = patch.delta().old_file().id(); - let new_blob = patch.delta().new_file().id(); return Ok(FileDiffSides { hunks, - old_blob: (!old_blob.is_zero()).then_some(old_blob), - new_blob: (!new_blob.is_zero()).then_some(new_blob), + old_blob, + new_blob, + old_mode: delta.old_file().mode(), + new_mode: delta.new_file().mode(), }); } @@ -575,16 +652,408 @@ fn scoped_hunks( ) } -fn blob_text(repo: &Repository, blob: Option) -> Result { +fn blob_text(repo: &Repository, blob: Option, file_path: &Path) -> Result { match blob { None => Ok(String::new()), Some(oid) => { let blob = repo.find_blob(oid)?; - Ok(String::from_utf8_lossy(blob.content()).to_string()) + std::str::from_utf8(blob.content()) + .map(str::to_owned) + .with_context(|| { + format!( + "Can't patch {} because its canonical Git content is not valid UTF-8.", + file_path.display() + ) + }) + } + } +} + +fn validate_relative_file_path(path: &Path) -> Result<()> { + use std::path::Component; + + let structurally_invalid = path.as_os_str().is_empty() + || path + .components() + .any(|component| !matches!(component, Component::Normal(_))); + #[cfg(windows)] + let structurally_invalid = structurally_invalid + || path.components().any(|component| { + matches!( + component, + Component::Normal(value) + if value.to_str().is_some_and(|value| value.contains(':')) + ) + }); + if structurally_invalid { + anyhow::bail!( + "Refusing to modify '{}': the path must contain only normal components relative to the working tree, with no root or parent traversal.", + path.display() + ); + } + Ok(()) +} + +#[cfg(windows)] +fn is_reparse_point(metadata: &std::fs::Metadata) -> bool { + use std::os::windows::fs::MetadataExt as _; + const FILE_ATTRIBUTE_REPARSE_POINT: u32 = 0x400; + metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT != 0 +} + +#[cfg(not(windows))] +fn is_reparse_point(_metadata: &std::fs::Metadata) -> bool { + false +} + +fn is_link_or_reparse(metadata: &std::fs::Metadata) -> bool { + metadata.file_type().is_symlink() || is_reparse_point(metadata) +} + +/// Verify every existing parent without following a link or Windows reparse +/// point. Missing parents are optionally created one component at a time and +/// inspected immediately after creation. +fn validate_parent_chain(workdir: &Path, path: &Path, create_missing: bool) -> Result { + let mut current = workdir.to_path_buf(); + let Some(parent) = path.parent() else { + return Ok(true); + }; + + for component in parent.components() { + current.push(component.as_os_str()); + let metadata = match std::fs::symlink_metadata(¤t) { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound && create_missing => { + match std::fs::create_dir(¤t) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(error) => { + return Err(error) + .with_context(|| format!("Failed to create {}", current.display())); + } + } + std::fs::symlink_metadata(¤t) + .with_context(|| format!("Failed to inspect {}", current.display()))? + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(false), + Err(error) => { + return Err(error) + .with_context(|| format!("Failed to inspect {}", current.display())); + } + }; + + if is_link_or_reparse(&metadata) { + anyhow::bail!( + "Refusing to modify '{}': parent '{}' is a symbolic link or reparse point.", + path.display(), + current.display() + ); + } + if !metadata.is_dir() { + anyhow::bail!( + "Refusing to modify '{}': parent '{}' is not a directory.", + path.display(), + current.display() + ); + } + } + + // A second, independent containment check catches paths whose parents were + // exchanged while they were being inspected. + if parent.as_os_str().is_empty() { + return Ok(true); + } + let resolved_parent = workdir + .join(parent) + .canonicalize() + .with_context(|| format!("Failed to resolve {}", workdir.join(parent).display()))?; + if !resolved_parent.starts_with(workdir) { + anyhow::bail!( + "Refusing to modify '{}': its parent resolves outside the working tree.", + path.display() + ); + } + Ok(true) +} + +fn permissions_from_metadata(metadata: &std::fs::Metadata) -> WorktreeFilePermissions { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + WorktreeFilePermissions { + readonly: metadata.permissions().readonly(), + mode: metadata.permissions().mode() & 0o7777, + } + } + #[cfg(not(unix))] + { + WorktreeFilePermissions { + readonly: metadata.permissions().readonly(), } } } +fn permissions_for_new_file(mode: FileMode) -> WorktreeFilePermissions { + #[cfg(unix)] + { + WorktreeFilePermissions { + readonly: false, + mode: match mode { + FileMode::BlobExecutable => 0o755, + FileMode::BlobGroupWritable => 0o664, + _ => 0o644, + }, + } + } + #[cfg(not(unix))] + { + let _ = mode; + WorktreeFilePermissions { readonly: false } + } +} + +fn apply_permissions(file: &std::fs::File, permissions: &WorktreeFilePermissions) -> Result<()> { + #[cfg(unix)] + let fs_permissions = { + use std::os::unix::fs::PermissionsExt as _; + std::fs::Permissions::from_mode(permissions.mode) + }; + #[cfg(not(unix))] + let fs_permissions = { + let mut value = file.metadata()?.permissions(); + value.set_readonly(permissions.readonly); + value + }; + file.set_permissions(fs_permissions) + .context("Failed to set permissions on the temporary working-tree file") +} + +/// Read a path only after lstat-style validation. `None` means the path or one +/// of its parents does not exist; links and non-regular files are errors. +fn read_worktree_file_state(workdir: &Path, path: &Path) -> Result> { + validate_relative_file_path(path)?; + if !validate_parent_chain(workdir, path, false)? { + return Ok(None); + } + let absolute = workdir.join(path); + let metadata = match std::fs::symlink_metadata(&absolute) { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(error) => { + return Err(error).with_context(|| format!("Failed to inspect {}", absolute.display())); + } + }; + if is_link_or_reparse(&metadata) { + anyhow::bail!( + "Refusing to modify '{}': it is a symbolic link or reparse point.", + path.display() + ); + } + if !metadata.is_file() { + anyhow::bail!( + "Refusing to modify '{}': it is not a regular file.", + path.display() + ); + } + + let contents = std::fs::read(&absolute) + .with_context(|| format!("Failed to read {}", absolute.display()))?; + let metadata_after = std::fs::symlink_metadata(&absolute) + .with_context(|| format!("Failed to revalidate {}", absolute.display()))?; + if is_link_or_reparse(&metadata_after) || !metadata_after.is_file() { + anyhow::bail!( + "Refusing to modify '{}': its filesystem type changed while it was being read.", + path.display() + ); + } + if metadata_after.len() != contents.len() as u64 { + anyhow::bail!( + "Refusing to modify '{}': it changed while it was being read. Try again after the other edit finishes.", + path.display() + ); + } + + Ok(Some(WorktreeFileState { + contents, + permissions: permissions_from_metadata(&metadata_after), + })) +} + +fn git_path_arg(path: &Path) -> OsString { + let mut argument = OsString::from("--path="); + argument.push(path.as_os_str()); + argument +} + +fn run_git_with_input( + workdir: &Path, + arguments: &[OsString], + input: &[u8], + description: &str, +) -> Result> { + // Feed stdin from a temporary file so a noisy required filter cannot + // deadlock us by filling Git's stderr pipe while this process is still + // synchronously writing a large input pipe. + let mut input_file = tempfile::tempfile() + .with_context(|| format!("Failed to prepare input for Git while {description}"))?; + input_file + .write_all(input) + .with_context(|| format!("Failed to prepare content for Git while {description}"))?; + input_file + .rewind() + .with_context(|| format!("Failed to rewind Git input while {description}"))?; + let output = Command::new("git") + .arg("-C") + .arg(workdir) + .args(arguments) + .stdin(Stdio::from(input_file)) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .output() + .with_context(|| format!("Failed to run Git while {description}"))?; + if !output.status.success() { + let details = String::from_utf8(output.stderr) + .unwrap_or_else(|_| "Git returned non-UTF-8 error output".to_string()); + anyhow::bail!("Git failed while {description}: {}", details.trim()); + } + Ok(output.stdout) +} + +fn git_object_bytes(workdir: &Path, oid: &str, description: &str) -> Result> { + run_git_with_input( + workdir, + &[ + OsString::from("cat-file"), + OsString::from("blob"), + OsString::from(oid), + ], + &[], + description, + ) +} + +/// Convert exact worktree bytes into canonical Git bytes, honoring text/EOL, +/// working-tree-encoding, clean filters, and LFS. +fn clean_worktree_bytes(workdir: &Path, path: &Path, bytes: &[u8]) -> Result> { + let oid_output = run_git_with_input( + workdir, + &[ + OsString::from("hash-object"), + OsString::from("-w"), + OsString::from("--stdin"), + git_path_arg(path), + ], + bytes, + &format!("cleaning {} through Git attributes", path.display()), + )?; + let oid = std::str::from_utf8(&oid_output) + .context("Git returned a non-UTF-8 object ID while cleaning the working-tree file")? + .trim(); + git_object_bytes( + workdir, + oid, + &format!("reading the cleaned content for {}", path.display()), + ) +} + +/// Convert canonical Git bytes into exact worktree bytes, honoring smudge +/// filters, working-tree-encoding and configured line endings. +fn smudge_canonical_bytes(workdir: &Path, path: &Path, bytes: &[u8]) -> Result> { + let oid_output = run_git_with_input( + workdir, + &[ + OsString::from("hash-object"), + OsString::from("-w"), + OsString::from("--stdin"), + ], + bytes, + &format!("storing the merged content for {}", path.display()), + )?; + let oid = std::str::from_utf8(&oid_output) + .context("Git returned a non-UTF-8 object ID while preparing working-tree content")? + .trim(); + run_git_with_input( + workdir, + &[ + OsString::from("cat-file"), + OsString::from("--filters"), + git_path_arg(path), + OsString::from(oid), + ], + &[], + &format!("smudging {} through Git attributes", path.display()), + ) +} + +fn changed_during_operation(path: &Path, purpose: &str) -> anyhow::Error { + anyhow::anyhow!( + "Can't {purpose} '{}': it changed after the operation started. No file was overwritten; retry after the other edit finishes.", + path.display() + ) +} + +/// Atomically replace or delete one path only if its exact current bytes and +/// permissions still match `expected`. +fn replace_file_if_unchanged( + workdir: &Path, + path: &Path, + expected: &Option, + desired: &Option, + purpose: &str, +) -> Result<()> { + if &read_worktree_file_state(workdir, path)? != expected { + return Err(changed_during_operation(path, purpose)); + } + + let absolute = workdir.join(path); + match desired { + Some(desired) => { + validate_parent_chain(workdir, path, true)?; + let parent = absolute + .parent() + .context("A validated working-tree path had no parent directory")?; + let mut temporary = tempfile::NamedTempFile::new_in(parent).with_context(|| { + format!("Failed to create a temporary file in {}", parent.display()) + })?; + temporary.write_all(&desired.contents).with_context(|| { + format!("Failed to write a temporary copy of {}", path.display()) + })?; + temporary.as_file().sync_all().with_context(|| { + format!("Failed to flush a temporary copy of {}", path.display()) + })?; + apply_permissions(temporary.as_file(), &desired.permissions)?; + + // Revalidate immediately before the rename. Renaming replaces a + // final symlink rather than following it, while parent validation + // protects the directory traversal itself. + validate_parent_chain(workdir, path, false)?; + if &read_worktree_file_state(workdir, path)? != expected { + return Err(changed_during_operation(path, purpose)); + } + temporary + .persist(&absolute) + .map_err(|error| error.error) + .with_context(|| format!("Failed to atomically replace {}", absolute.display()))?; + } + None => { + validate_parent_chain(workdir, path, false)?; + if &read_worktree_file_state(workdir, path)? != expected { + return Err(changed_during_operation(path, purpose)); + } + std::fs::remove_file(&absolute) + .with_context(|| format!("Failed to delete {}", absolute.display()))?; + } + } + + if &read_worktree_file_state(workdir, path)? != desired { + anyhow::bail!( + "The filesystem changed '{}' while the atomic update was being published. Check the file before retrying.", + path.display() + ); + } + Ok(()) +} + /// Apply or revert part of `source`'s diff for one file in `worktree_path`. /// /// Returns the pre-operation snapshot so the caller can offer undo. Every error @@ -610,12 +1079,17 @@ pub fn apply_worktree_patch( direction.verb() ) })? - .to_path_buf(); + .canonicalize() + .context("Failed to resolve the working-tree root")?; + + validate_relative_file_path(file_path)?; let FileDiffSides { hunks, old_blob, new_blob, + old_mode, + new_mode, } = scoped_hunks(&repo, source, file_path)?; let selected_hunks = match scope { @@ -646,12 +1120,12 @@ pub fn apply_worktree_patch( } }; - let (base_blob, target_side_blob) = match direction { - WorktreePatchDirection::Apply => (old_blob, new_blob), - WorktreePatchDirection::Revert => (new_blob, old_blob), + let (base_blob, target_side_blob, target_mode) = match direction { + WorktreePatchDirection::Apply => (old_blob, new_blob, new_mode), + WorktreePatchDirection::Revert => (new_blob, old_blob, old_mode), }; - let base_text = blob_text(&repo, base_blob)?; + let base_text = blob_text(&repo, base_blob, file_path)?; let base_lines = split_keeping_terminators(&base_text); let target_text = rewrite_side( &base_lines, @@ -670,28 +1144,30 @@ pub fn apply_worktree_patch( ); } - let absolute = workdir.join(file_path); - // The whole file moving to a side that does not have it is a deletion, not // an empty file. Only the file-level scope can express that. let deletes_file = matches!(scope, WorktreePatchScope::File) && target_side_blob.is_none() && target_text.is_empty(); - let existing = match std::fs::read(&absolute) { - Ok(bytes) => Some(bytes), - Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, - Err(error) => { - return Err(error).with_context(|| format!("Failed to read {}", absolute.display())) - } + let _mutation_guard = WORKTREE_MUTATION_LOCK.lock(); + let existing = read_worktree_file_state(&workdir, file_path)?; + let existing_canonical = match &existing { + Some(state) => Some(clean_worktree_bytes(&workdir, file_path, &state.contents)?), + None => None, }; - let existing_text = existing + let existing_text = existing_canonical .as_deref() - .map(|bytes| String::from_utf8_lossy(bytes).to_string()); - let snapshot = vec![WorktreeFileSnapshot { - path: file_path.to_path_buf(), - contents: existing.clone(), - }]; + .map(|bytes| { + std::str::from_utf8(bytes).map(str::to_owned).with_context(|| { + format!( + "Can't {} {} because its canonical working-tree content is not valid UTF-8. No file was changed.", + direction.verb(), + file_path.display() + ) + }) + }) + .transpose()?; if deletes_file { let Some(existing_text) = existing_text.as_deref() else { @@ -709,10 +1185,13 @@ pub fn apply_worktree_patch( file_path.display() ); } - std::fs::remove_file(&absolute) - .with_context(|| format!("Failed to delete {}", absolute.display()))?; + replace_file_if_unchanged(&workdir, file_path, &existing, &None, direction.verb())?; return Ok(WorktreePatchOutcome { - snapshots: snapshot, + snapshots: vec![WorktreeFileSnapshot { + path: file_path.to_path_buf(), + before: existing, + expected_after: None, + }], merged_with_local_changes: false, }); } @@ -743,15 +1222,22 @@ pub fn apply_worktree_patch( anyhow::bail!(already_applied_message(file_path, scope, source, direction)); } - if let Some(parent) = absolute.parent() { - std::fs::create_dir_all(parent) - .with_context(|| format!("Failed to create {}", parent.display()))?; - } - std::fs::write(&absolute, merged_text.as_bytes()) - .with_context(|| format!("Failed to write {}", absolute.display()))?; + let worktree_bytes = smudge_canonical_bytes(&workdir, file_path, merged_text.as_bytes())?; + let after = Some(WorktreeFileState { + contents: worktree_bytes, + permissions: existing + .as_ref() + .map(|state| state.permissions.clone()) + .unwrap_or_else(|| permissions_for_new_file(target_mode)), + }); + replace_file_if_unchanged(&workdir, file_path, &existing, &after, direction.verb())?; Ok(WorktreePatchOutcome { - snapshots: snapshot, + snapshots: vec![WorktreeFileSnapshot { + path: file_path.to_path_buf(), + before: existing, + expected_after: after, + }], merged_with_local_changes, }) } @@ -770,30 +1256,33 @@ pub fn restore_worktree_files( let workdir = repo .workdir() .ok_or_else(|| anyhow::anyhow!("This is a bare repository, so it has no working tree."))? - .to_path_buf(); + .canonicalize() + .context("Failed to resolve the working-tree root")?; + + let _mutation_guard = WORKTREE_MUTATION_LOCK.lock(); + // Validate every CAS before making the first mutation. A later external + // edit is checked again immediately before each atomic replace. for snapshot in snapshots { - let absolute = workdir.join(&snapshot.path); - match &snapshot.contents { - Some(bytes) => { - if let Some(parent) = absolute.parent() { - std::fs::create_dir_all(parent) - .with_context(|| format!("Failed to create {}", parent.display()))?; - } - std::fs::write(&absolute, bytes) - .with_context(|| format!("Failed to restore {}", absolute.display()))?; - } - None => match std::fs::remove_file(&absolute) { - Ok(()) => {} - Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} - Err(error) => { - return Err(error) - .with_context(|| format!("Failed to remove {}", absolute.display())) - } - }, + validate_relative_file_path(&snapshot.path)?; + if read_worktree_file_state(&workdir, &snapshot.path)? != snapshot.expected_after { + anyhow::bail!( + "Can't undo the change to '{}': the file was edited after the operation. Those later edits were left untouched.", + snapshot.path.display() + ); } } + for snapshot in snapshots { + replace_file_if_unchanged( + &workdir, + &snapshot.path, + &snapshot.expected_after, + &snapshot.before, + "undo the change to", + )?; + } + Ok(()) } @@ -809,7 +1298,11 @@ fn three_way_merge( ours: &str, theirs: &str, ) -> Result> { - let path_bytes = file_path.to_string_lossy().replace('\\', "/").into_bytes(); + let path_bytes = file_path + .to_str() + .context("Working-tree patch paths must be valid UTF-8")? + .replace('\\', "/") + .into_bytes(); let entry = |text: &str| -> Result { Ok(IndexEntry { ctime: IndexTime::new(0, 0), @@ -841,7 +1334,16 @@ fn three_way_merge( if !result.is_automergeable() { return Ok(None); } - Ok(Some(String::from_utf8_lossy(result.content()).to_string())) + Ok(Some( + std::str::from_utf8(result.content()) + .with_context(|| { + format!( + "Git produced invalid UTF-8 while merging {}. No working-tree file was changed.", + file_path.display() + ) + })? + .to_owned(), + )) } // ── Error messages ──────────────────────────────────────────────────────────── @@ -928,6 +1430,14 @@ use super::refresh::gather_refresh_data_lightweight_cached; use super::{GitProject, GitProjectEvent, RefreshData}; use crate::types::GitOperationKind; +fn run_mutation_then_refresh( + mutation: impl FnOnce() -> Result, + refresh: impl FnOnce() -> Result, +) -> Result<(M, Result)> { + let mutation = mutation()?; + Ok((mutation, refresh())) +} + impl GitProject { /// Apply or revert part of another revision's diff in `worktree_path`. /// @@ -947,6 +1457,8 @@ impl GitProject { let file_path = file_path.to_path_buf(); let task_file_path = file_path.clone(); let task_worktree_path = worktree_path.to_path_buf(); + let origin_worktree_path = task_worktree_path.clone(); + let origin_repo_path = self.repo_path.clone(); let refresh_repo_path = self.repo_path.clone(); let worktree_cache = self.worktree_status_cache.clone(); let author_filter = self.commit_author_filter.clone(); @@ -975,31 +1487,35 @@ impl GitProject { let task_scope = scope.clone(); cx.spawn(async move |this: WeakEntity, cx: &mut AsyncApp| { - let result: anyhow::Result<(WorktreePatchOutcome, RefreshData)> = cx + let result: anyhow::Result<(WorktreePatchOutcome, anyhow::Result)> = cx .background_executor() .spawn(async move { - let outcome = apply_worktree_patch( - &task_worktree_path, - &task_file_path, - &task_source, - &task_scope, - direction, - )?; - let data = gather_refresh_data_lightweight_cached( - &refresh_repo_path, - commit_limit, - &worktree_cache, - author_filter.as_deref(), - )?; - Ok((outcome, data)) + run_mutation_then_refresh( + || { + apply_worktree_patch( + &task_worktree_path, + &task_file_path, + &task_source, + &task_scope, + direction, + ) + }, + || { + gather_refresh_data_lightweight_cached( + &refresh_repo_path, + commit_limit, + &worktree_cache, + author_filter.as_deref(), + ) + }, + ) }) .await; cx.update(|cx| { this.update(cx, |this, cx| { match result { - Ok((outcome, data)) => { - this.apply_refresh_data(data); + Ok((outcome, refresh_result)) => { let mut summary = format!( "{} {} of {} from {}", direction.past_tense(), @@ -1012,6 +1528,27 @@ impl GitProject { " — merged around your uncommitted edits to that file", ); } + let undoable = snapshots_fit_undo_stack(&outcome.snapshots); + if !undoable { + summary.push_str( + "; Undo is unavailable because the before/after snapshot exceeds 4 MiB", + ); + } + let refresh_succeeded = match refresh_result { + Ok(data) => { + this.apply_refresh_data(data); + true + } + Err(error) => { + log::warn!( + "worktree patch succeeded but status refresh failed: {error:#}" + ); + summary.push_str( + "; the file changed successfully, but status refresh failed; refresh the repository to update the UI", + ); + false + } + }; this.complete_op( operation_id, kind, @@ -1019,13 +1556,17 @@ impl GitProject { (None, None, branch_name.clone()), cx, ); - if snapshots_fit_undo_stack(&outcome.snapshots) { + if undoable { cx.emit(GitProjectEvent::WorktreePatchApplied { label: summary, snapshots: outcome.snapshots, + repo_path: origin_repo_path.clone(), + worktree_path: origin_worktree_path.clone(), }); } - cx.emit(GitProjectEvent::StatusChanged); + if refresh_succeeded { + cx.emit(GitProjectEvent::StatusChanged); + } } Err(e) => { this.fail_op( @@ -1072,15 +1613,19 @@ impl GitProject { ); cx.spawn(async move |this: WeakEntity, cx: &mut AsyncApp| { - let result: anyhow::Result = cx + let result: anyhow::Result<((), anyhow::Result)> = cx .background_executor() .spawn(async move { - restore_worktree_files(&task_worktree_path, &snapshots)?; - gather_refresh_data_lightweight_cached( - &refresh_repo_path, - commit_limit, - &worktree_cache, - author_filter.as_deref(), + run_mutation_then_refresh( + || restore_worktree_files(&task_worktree_path, &snapshots), + || { + gather_refresh_data_lightweight_cached( + &refresh_repo_path, + commit_limit, + &worktree_cache, + author_filter.as_deref(), + ) + }, ) }) .await; @@ -1088,20 +1633,43 @@ impl GitProject { cx.update(|cx| { this.update(cx, |this, cx| { match result { - Ok(data) => { - this.apply_refresh_data(data); + Ok(((), refresh_result)) => { + let (summary, refresh_succeeded) = match refresh_result { + Ok(data) => { + this.apply_refresh_data(data); + ( + format!( + "Restored {} file{}", + file_count, + if file_count == 1 { "" } else { "s" } + ), + true, + ) + } + Err(error) => { + log::warn!( + "worktree restore succeeded but status refresh failed: {error:#}" + ); + ( + format!( + "Restored {} file{}; status refresh failed, so refresh the repository to update the UI", + file_count, + if file_count == 1 { "" } else { "s" } + ), + false, + ) + } + }; this.complete_op( operation_id, GitOperationKind::Discard, - format!( - "Restored {} file{}", - file_count, - if file_count == 1 { "" } else { "s" } - ), + summary, (None, None, branch_name.clone()), cx, ); - cx.emit(GitProjectEvent::StatusChanged); + if refresh_succeeded { + cx.emit(GitProjectEvent::StatusChanged); + } } Err(e) => { this.fail_op( @@ -1126,6 +1694,17 @@ impl GitProject { mod tests { use super::*; + fn state(contents: Vec) -> WorktreeFileState { + WorktreeFileState { + contents, + permissions: WorktreeFilePermissions { + readonly: false, + #[cfg(unix)] + mode: 0o644, + }, + } + } + fn context(old: usize, new: usize) -> ScopedLine { ScopedLine::Context { old_lineno: old, @@ -1398,7 +1977,8 @@ mod tests { fn small_snapshots_fit_the_undo_stack() { let snapshots = vec![WorktreeFileSnapshot { path: PathBuf::from("a.txt"), - contents: Some(vec![0; 1024]), + before: Some(state(vec![0; 1024])), + expected_after: None, }]; assert!(snapshots_fit_undo_stack(&snapshots)); } @@ -1407,7 +1987,8 @@ mod tests { fn oversized_snapshots_do_not_fit_the_undo_stack() { let snapshots = vec![WorktreeFileSnapshot { path: PathBuf::from("a.txt"), - contents: Some(vec![0; MAX_UNDO_SNAPSHOT_BYTES + 1]), + before: Some(state(vec![0; MAX_UNDO_SNAPSHOT_BYTES + 1])), + expected_after: None, }]; assert!(!snapshots_fit_undo_stack(&snapshots)); } @@ -1416,7 +1997,8 @@ mod tests { fn a_deleted_file_snapshot_costs_nothing() { let snapshots = vec![WorktreeFileSnapshot { path: PathBuf::from("a.txt"), - contents: None, + before: None, + expected_after: None, }]; assert!(snapshots_fit_undo_stack(&snapshots)); } @@ -1463,6 +2045,33 @@ mod tests { WorktreePatchDirection::Apply ); } + + #[test] + fn a_refresh_failure_does_not_turn_a_successful_mutation_into_a_failure() { + let result = run_mutation_then_refresh( + || Ok::<_, anyhow::Error>("mutated"), + || Err::<(), _>(anyhow::anyhow!("refresh unavailable")), + ) + .expect("the mutation itself succeeded"); + + assert_eq!(result.0, "mutated"); + assert!(result.1.is_err()); + } + + #[test] + fn a_mutation_failure_prevents_the_refresh_from_running() { + let refresh_ran = std::cell::Cell::new(false); + let result = run_mutation_then_refresh( + || Err::<(), _>(anyhow::anyhow!("mutation failed")), + || { + refresh_ran.set(true); + Ok::<_, anyhow::Error>(()) + }, + ); + + assert!(result.is_err()); + assert!(!refresh_ran.get()); + } } // ── Integration tests against real repositories ─────────────────────────────── @@ -1501,7 +2110,15 @@ mod worktree_patch_integration_tests { } fn write(&self, name: &str, contents: &str) { - std::fs::write(self.path.join(name), contents).unwrap(); + self.write_bytes(name, contents.as_bytes()); + } + + fn write_bytes(&self, name: &str, contents: &[u8]) { + let path = self.path.join(name); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(path, contents).unwrap(); } fn read(&self, name: &str) -> String { @@ -1548,6 +2165,10 @@ mod worktree_patch_integration_tests { fn head_branch_name(&self) -> String { self.repo.head().unwrap().shorthand().unwrap().to_string() } + + fn set_config(&self, name: &str, value: &str) { + self.repo.config().unwrap().set_str(name, value).unwrap(); + } } /// Numbered lines `l1..l20`, with `substitutions` replacing the given @@ -2110,9 +2731,19 @@ mod worktree_patch_integration_tests { assert_eq!(outcome.snapshots.len(), 1); assert_eq!(outcome.snapshots[0].path, PathBuf::from("f.txt")); assert_eq!( - outcome.snapshots[0].contents.as_deref(), + outcome.snapshots[0] + .before + .as_ref() + .map(|state| state.contents.as_slice()), Some(before.as_bytes()) ); + assert_eq!( + outcome.snapshots[0] + .expected_after + .as_ref() + .map(|state| state.contents.as_slice()), + Some(fixture.read("f.txt").as_bytes()) + ); } #[test] @@ -2151,7 +2782,8 @@ mod worktree_patch_integration_tests { ) .unwrap(); assert!(fixture.path.join("added.txt").exists()); - assert_eq!(outcome.snapshots[0].contents, None); + assert_eq!(outcome.snapshots[0].before, None); + assert!(outcome.snapshots[0].expected_after.is_some()); restore_worktree_files(&fixture.path, &outcome.snapshots).unwrap(); assert!(!fixture.path.join("added.txt").exists()); @@ -2181,6 +2813,424 @@ mod worktree_patch_integration_tests { // ── stash entries travel the commit path ────────────────────── + #[test] + fn undo_refuses_to_overwrite_an_edit_made_after_apply() { + let (fixture, oid) = two_hunk_commit(); + let outcome = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .unwrap(); + fixture.write("f.txt", "a later editor change\n"); + + let error = restore_worktree_files(&fixture.path, &outcome.snapshots) + .expect_err("undo must preserve the later edit"); + assert!(error.to_string().contains("edited after"), "got: {error}"); + assert_eq!(fixture.read("f.txt"), "a later editor change\n"); + } + + #[test] + fn undo_of_a_created_file_refuses_to_delete_later_content() { + let fixture = Fixture::new(); + fixture.write("keep.txt", "x\n"); + fixture.commit("base", &["keep.txt"]); + fixture.write("added.txt", "from commit\n"); + let oid = fixture.commit("add", &["added.txt"]); + std::fs::remove_file(fixture.path.join("added.txt")).unwrap(); + let outcome = apply( + &fixture, + "added.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .unwrap(); + fixture.write("added.txt", "new user content\n"); + + assert!(restore_worktree_files(&fixture.path, &outcome.snapshots).is_err()); + assert_eq!(fixture.read("added.txt"), "new user content\n"); + } + + #[test] + fn undo_of_a_deleted_file_refuses_to_replace_a_new_path() { + let fixture = Fixture::new(); + fixture.write("keep.txt", "x\n"); + fixture.commit("base", &["keep.txt"]); + fixture.write("added.txt", "from commit\n"); + let oid = fixture.commit("add", &["added.txt"]); + let outcome = apply( + &fixture, + "added.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Revert, + ) + .unwrap(); + fixture.write("added.txt", "new path\n"); + + assert!(restore_worktree_files(&fixture.path, &outcome.snapshots).is_err()); + assert_eq!(fixture.read("added.txt"), "new path\n"); + } + + #[test] + fn concurrent_restores_accept_the_expected_after_image_only_once() { + let (fixture, oid) = two_hunk_commit(); + let outcome = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .unwrap(); + let path = fixture.path.clone(); + let left_snapshots = outcome.snapshots.clone(); + let right_snapshots = outcome.snapshots; + let barrier = std::sync::Arc::new(std::sync::Barrier::new(3)); + let left_barrier = barrier.clone(); + let left_path = path.clone(); + let left = std::thread::spawn(move || { + left_barrier.wait(); + restore_worktree_files(&left_path, &left_snapshots) + }); + let right_barrier = barrier.clone(); + let right = std::thread::spawn(move || { + right_barrier.wait(); + restore_worktree_files(&path, &right_snapshots) + }); + barrier.wait(); + + let results = [left.join().unwrap(), right.join().unwrap()]; + assert_eq!(results.iter().filter(|result| result.is_ok()).count(), 1); + assert_eq!(results.iter().filter(|result| result.is_err()).count(), 1); + } + + #[test] + fn traversal_paths_are_rejected_before_any_filesystem_write() { + let (fixture, oid) = two_hunk_commit(); + let error = apply_worktree_patch( + &fixture.path, + Path::new("../outside.txt"), + &WorktreePatchSource::Commit(oid), + &WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .expect_err("parent traversal is never a working-tree file path"); + assert!(error.to_string().contains("relative"), "got: {error}"); + } + + #[test] + fn invalid_utf8_in_the_worktree_is_refused_without_rewriting_it() { + let (fixture, oid) = two_hunk_commit(); + let invalid = b"valid prefix\n\xff\xfe\n"; + fixture.write_bytes("f.txt", invalid); + + let error = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .expect_err("invalid canonical text cannot enter the merge"); + assert!(error.to_string().contains("valid UTF-8"), "got: {error}"); + assert_eq!(std::fs::read(fixture.path.join("f.txt")).unwrap(), invalid); + } + + #[test] + fn invalid_utf8_in_a_source_blob_is_refused_without_rewriting_the_worktree() { + let fixture = Fixture::new(); + fixture.write("f.txt", "base\n"); + fixture.commit("base", &["f.txt"]); + fixture.write_bytes("f.txt", b"target\n\xff\n"); + let oid = fixture.commit("invalid target", &["f.txt"]); + fixture.write("f.txt", "base\n"); + + let error = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .expect_err("invalid canonical source text must be refused"); + assert!(error.to_string().contains("valid UTF-8"), "got: {error}"); + assert_eq!(fixture.read("f.txt"), "base\n"); + } + + #[test] + fn git_eol_conversion_round_trips_canonical_and_worktree_bytes() { + let fixture = Fixture::new(); + fixture.write(".gitattributes", "*.txt text eol=crlf\n"); + + let canonical = b"one\ntwo\n"; + let worktree = smudge_canonical_bytes(&fixture.path, Path::new("f.txt"), canonical) + .expect("Git should apply the eol attribute"); + assert_eq!(worktree, b"one\r\ntwo\r\n"); + assert_eq!( + clean_worktree_bytes(&fixture.path, Path::new("f.txt"), &worktree).unwrap(), + canonical + ); + } + + #[test] + fn a_full_apply_and_undo_preserve_crlf_worktree_form() { + let fixture = Fixture::new(); + fixture.write(".gitattributes", "*.txt text eol=crlf\n"); + fixture.commit("attributes", &[".gitattributes"]); + fixture.write_bytes("f.txt", b"base\r\n"); + fixture.commit("base", &["f.txt"]); + fixture.write_bytes("f.txt", b"target\r\n"); + let oid = fixture.commit("target", &["f.txt"]); + fixture.write_bytes("f.txt", b"base\r\n"); + + let outcome = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .unwrap(); + assert_eq!( + std::fs::read(fixture.path.join("f.txt")).unwrap(), + b"target\r\n" + ); + + restore_worktree_files(&fixture.path, &outcome.snapshots).unwrap(); + assert_eq!( + std::fs::read(fixture.path.join("f.txt")).unwrap(), + b"base\r\n" + ); + } + + #[test] + fn git_working_tree_encoding_round_trips_through_canonical_utf8() { + let fixture = Fixture::new(); + fixture.write( + ".gitattributes", + "*.utf16 text working-tree-encoding=UTF-16LE\n", + ); + let canonical = b"one\ntwo\n"; + + let encoded = smudge_canonical_bytes(&fixture.path, Path::new("f.utf16"), canonical) + .expect("Git should encode the working-tree form"); + assert_ne!(encoded, canonical); + assert_eq!( + clean_worktree_bytes(&fixture.path, Path::new("f.utf16"), &encoded).unwrap(), + canonical + ); + } + + #[test] + fn required_clean_and_smudge_filters_are_honoured() { + let fixture = Fixture::new(); + fixture.write(".gitattributes", "*.flt filter=rgitui-test\n"); + fixture.set_config("filter.rgitui-test.required", "true"); + fixture.set_config("filter.rgitui-test.clean", "sed s/WORKTREE/CANONICAL/g"); + fixture.set_config("filter.rgitui-test.smudge", "sed s/CANONICAL/WORKTREE/g"); + + assert_eq!( + clean_worktree_bytes(&fixture.path, Path::new("f.flt"), b"WORKTREE\n").unwrap(), + b"CANONICAL\n" + ); + assert_eq!( + smudge_canonical_bytes(&fixture.path, Path::new("f.flt"), b"CANONICAL\n").unwrap(), + b"WORKTREE\n" + ); + } + + #[test] + fn a_required_filter_failure_writes_nothing() { + let (fixture, oid) = two_hunk_commit(); + fixture.write(".gitattributes", "f.txt filter=broken\n"); + fixture.set_config("filter.broken.required", "true"); + fixture.set_config("filter.broken.clean", "false"); + fixture.set_config("filter.broken.smudge", "false"); + let before = fixture.read("f.txt"); + + assert!(apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .is_err()); + assert_eq!(fixture.read("f.txt"), before); + } + + #[cfg(unix)] + #[test] + fn undo_restores_the_recorded_unix_mode() { + use std::os::unix::fs::PermissionsExt as _; + + let fixture = Fixture::new(); + fixture.write("f.txt", "after\n"); + std::fs::set_permissions( + fixture.path.join("f.txt"), + std::fs::Permissions::from_mode(0o600), + ) + .unwrap(); + let snapshot = WorktreeFileSnapshot { + path: PathBuf::from("f.txt"), + before: Some(WorktreeFileState { + contents: b"before\n".to_vec(), + permissions: WorktreeFilePermissions { + readonly: false, + mode: 0o751, + }, + }), + expected_after: read_worktree_file_state(&fixture.path, Path::new("f.txt")).unwrap(), + }; + + restore_worktree_files(&fixture.path, &[snapshot]).unwrap(); + assert_eq!(fixture.read("f.txt"), "before\n"); + assert_eq!( + std::fs::metadata(fixture.path.join("f.txt")) + .unwrap() + .permissions() + .mode() + & 0o777, + 0o751 + ); + } + + #[cfg(unix)] + #[test] + fn applying_a_new_executable_file_preserves_its_git_mode() { + use std::os::unix::fs::PermissionsExt as _; + + let fixture = Fixture::new(); + fixture.write("keep.txt", "base\n"); + fixture.commit("base", &["keep.txt"]); + fixture.write("script.sh", "#!/bin/sh\nexit 0\n"); + std::fs::set_permissions( + fixture.path.join("script.sh"), + std::fs::Permissions::from_mode(0o755), + ) + .unwrap(); + let oid = fixture.commit("script", &["script.sh"]); + std::fs::remove_file(fixture.path.join("script.sh")).unwrap(); + + apply( + &fixture, + "script.sh", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .unwrap(); + assert_eq!( + std::fs::metadata(fixture.path.join("script.sh")) + .unwrap() + .permissions() + .mode() + & 0o777, + 0o755 + ); + } + + #[cfg(any(unix, windows))] + #[test] + fn a_symlink_file_is_rejected_without_touching_its_target() { + let (fixture, oid) = two_hunk_commit(); + let outside = TempDir::new().unwrap(); + let target = outside.path().join("target.txt"); + std::fs::write(&target, "outside\n").unwrap(); + std::fs::remove_file(fixture.path.join("f.txt")).unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(&target, fixture.path.join("f.txt")).unwrap(); + #[cfg(windows)] + if std::os::windows::fs::symlink_file(&target, fixture.path.join("f.txt")).is_err() { + return; + } + + let error = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .expect_err("a final symlink must never be followed"); + assert!( + error.to_string().contains("symbolic link") + || error.to_string().contains("reparse point"), + "got: {error}" + ); + assert_eq!(std::fs::read_to_string(target).unwrap(), "outside\n"); + } + + #[cfg(any(unix, windows))] + #[test] + fn a_symlink_parent_is_rejected_without_touching_outside_files() { + let fixture = Fixture::new(); + fixture.write("dir/f.txt", "base\n"); + fixture.commit("base", &["dir/f.txt"]); + fixture.write("dir/f.txt", "changed\n"); + let oid = fixture.commit("change", &["dir/f.txt"]); + std::fs::remove_file(fixture.path.join("dir/f.txt")).unwrap(); + std::fs::remove_dir(fixture.path.join("dir")).unwrap(); + let outside = TempDir::new().unwrap(); + std::fs::write(outside.path().join("f.txt"), "outside\n").unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(outside.path(), fixture.path.join("dir")).unwrap(); + #[cfg(windows)] + if std::os::windows::fs::symlink_dir(outside.path(), fixture.path.join("dir")).is_err() { + return; + } + + let error = apply( + &fixture, + "dir/f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .expect_err("a linked parent must never be followed"); + assert!( + error.to_string().contains("symbolic link") + || error.to_string().contains("reparse point"), + "got: {error}" + ); + assert_eq!( + std::fs::read_to_string(outside.path().join("f.txt")).unwrap(), + "outside\n" + ); + } + + #[cfg(unix)] + #[test] + fn a_tracked_symlink_diff_is_not_materialized_as_a_regular_file() { + let fixture = Fixture::new(); + fixture.write("f.txt", "base\n"); + fixture.commit("base", &["f.txt"]); + let outside = TempDir::new().unwrap(); + let target = outside.path().join("target.txt"); + std::fs::write(&target, "outside\n").unwrap(); + std::fs::remove_file(fixture.path.join("f.txt")).unwrap(); + std::os::unix::fs::symlink(&target, fixture.path.join("f.txt")).unwrap(); + let oid = fixture.commit("replace with symlink", &["f.txt"]); + std::fs::remove_file(fixture.path.join("f.txt")).unwrap(); + fixture.write("f.txt", "base\n"); + + let error = apply( + &fixture, + "f.txt", + &WorktreePatchSource::Commit(oid), + WorktreePatchScope::File, + WorktreePatchDirection::Apply, + ) + .expect_err("a symlink diff needs link-aware semantics"); + assert!(error.to_string().contains("not a regular file")); + assert_eq!(fixture.read("f.txt"), "base\n"); + assert_eq!(std::fs::read_to_string(target).unwrap(), "outside\n"); + } + #[test] fn a_stash_entrys_hunk_applies_like_any_other_commit() { let fixture = Fixture::new(); diff --git a/crates/rgitui_workspace/src/workspace/events.rs b/crates/rgitui_workspace/src/workspace/events.rs index 3879f78..87896db 100644 --- a/crates/rgitui_workspace/src/workspace/events.rs +++ b/crates/rgitui_workspace/src/workspace/events.rs @@ -866,14 +866,21 @@ pub(super) fn subscribe_project(cx: &mut Context, subs: ProjectSubscr tb.set_ahead_behind(ahead, behind, cx); }); } - GitProjectEvent::WorktreePatchApplied { label, snapshots } => { + GitProjectEvent::WorktreePatchApplied { + label, + snapshots, + repo_path, + worktree_path, + } => { // No git command reverses a working-tree rewrite, so the // pre-operation bytes are what goes on the undo stack. - this.push_undo( + this.push_undo_for_paths( label.clone(), UndoAction::RestoreWorktreeFiles { snapshots: snapshots.clone(), }, + repo_path.clone(), + worktree_path.clone(), cx, ); } diff --git a/crates/rgitui_workspace/src/workspace/undo.rs b/crates/rgitui_workspace/src/workspace/undo.rs index fd13b55..56a8111 100644 --- a/crates/rgitui_workspace/src/workspace/undo.rs +++ b/crates/rgitui_workspace/src/workspace/undo.rs @@ -51,14 +51,29 @@ pub enum UndoAction { /// /// The bytes travel with the entry because the forward operation may have gone /// through a three-way merge, and a reverse patch would then not restore the - /// working tree exactly. `contents: None` means the file did not exist, so - /// undoing deletes it. + /// working tree exactly. `before: None` means the file did not exist, so + /// undoing deletes it; `expected_after` protects later user edits. RestoreWorktreeFiles { snapshots: Vec, }, } impl UndoEntry { + fn for_paths( + label: impl Into, + action: UndoAction, + repo_path: PathBuf, + worktree_path: PathBuf, + ) -> Self { + Self { + label: label.into(), + action, + created_at: Instant::now(), + repo_path, + worktree_path, + } + } + pub fn is_expired(&self) -> bool { self.created_at.elapsed().as_secs() > UNDO_EXPIRY_SECS } @@ -155,13 +170,26 @@ impl Workspace { }; let repo_path = tab.project.read(cx).repo_path().to_path_buf(); let worktree_path = self.effective_worktree_path(cx); - self.undo_stack.push(UndoEntry { - label: label.into(), + self.push_undo_for_paths(label, action, repo_path, worktree_path, cx); + } + + /// Push an undo entry stamped with paths captured by the operation itself. + /// Asynchronous completions must use this instead of consulting the active + /// tab, which may have changed while the filesystem work was running. + pub fn push_undo_for_paths( + &mut self, + label: impl Into, + action: UndoAction, + repo_path: PathBuf, + worktree_path: PathBuf, + cx: &mut Context, + ) { + self.undo_stack.push(UndoEntry::for_paths( + label, action, - created_at: Instant::now(), repo_path, worktree_path, - }); + )); self.schedule_undo_expiry(cx); cx.notify(); } @@ -306,11 +334,10 @@ impl Workspace { proj.restore_worktree_files_at(snapshots, &worktree_path, cx) .detach(); }); - self.show_toast( - format!("Undid: {undo_label}{suffix}"), - ToastKind::Success, - cx, - ); + // The restore performs a filesystem compare-and-swap in the + // background and can legitimately refuse after a later edit. + // Its operation event reports the actual result; do not show + // an optimistic success toast here. } } } @@ -341,6 +368,17 @@ impl Workspace { mod tests { use super::*; + fn file_state(contents: &[u8]) -> rgitui_git::WorktreeFileState { + rgitui_git::WorktreeFileState { + contents: contents.to_vec(), + permissions: rgitui_git::WorktreeFilePermissions { + readonly: false, + #[cfg(unix)] + mode: 0o644, + }, + } + } + fn make_entry(label: &str) -> UndoEntry { UndoEntry { label: label.to_string(), @@ -515,7 +553,8 @@ mod tests { UndoAction::RestoreWorktreeFiles { snapshots: vec![rgitui_git::WorktreeFileSnapshot { path: PathBuf::from("d.rs"), - contents: Some(b"before\n".to_vec()), + before: Some(file_state(b"before\n")), + expected_after: Some(file_state(b"after\n")), }], }, ]; @@ -550,11 +589,13 @@ mod tests { snapshots: vec![ rgitui_git::WorktreeFileSnapshot { path: PathBuf::from("src/main.rs"), - contents: Some(b"fn main() {}\n".to_vec()), + before: Some(file_state(b"fn main() {}\n")), + expected_after: Some(file_state(b"fn main() { run(); }\n")), }, rgitui_git::WorktreeFileSnapshot { path: PathBuf::from("src/new.rs"), - contents: None, + before: None, + expected_after: Some(file_state(b"pub fn new() {}\n")), }, ], }, @@ -570,11 +611,14 @@ mod tests { assert_eq!(snapshots.len(), 2); assert_eq!(snapshots[0].path, PathBuf::from("src/main.rs")); assert_eq!( - snapshots[0].contents.as_deref(), + snapshots[0] + .before + .as_ref() + .map(|state| state.contents.as_slice()), Some(&b"fn main() {}\n"[..]) ); assert_eq!( - snapshots[1].contents, None, + snapshots[1].before, None, "a file the apply created must be recorded as absent so undo deletes it" ); assert_eq!( @@ -583,4 +627,17 @@ mod tests { "undo must route back to the worktree the files were written in" ); } + + #[test] + fn asynchronous_undo_entry_keeps_its_captured_origin_paths() { + let entry = UndoEntry::for_paths( + "Applied a hunk", + UndoAction::RestoreWorktreeFiles { snapshots: vec![] }, + PathBuf::from("originating-repository"), + PathBuf::from("originating-worktree"), + ); + + assert_eq!(entry.repo_path, PathBuf::from("originating-repository")); + assert_eq!(entry.worktree_path, PathBuf::from("originating-worktree")); + } } From 1491418318030830e9528945ea4624c30f318caf Mon Sep 17 00:00:00 2001 From: Noah Clarkson Date: Sun, 9 Aug 2026 18:55:51 +1200 Subject: [PATCH 6/9] test(git): reuse the shared temp repository fixture --- .../rgitui_git/src/project/worktree_patch.rs | 77 +++++++------------ crates/rgitui_test_support/src/temp_repo.rs | 9 +++ 2 files changed, 36 insertions(+), 50 deletions(-) diff --git a/crates/rgitui_git/src/project/worktree_patch.rs b/crates/rgitui_git/src/project/worktree_patch.rs index 42e193d..469e279 100644 --- a/crates/rgitui_git/src/project/worktree_patch.rs +++ b/crates/rgitui_git/src/project/worktree_patch.rs @@ -2082,43 +2082,27 @@ mod tests { #[cfg(test)] mod worktree_patch_integration_tests { use super::*; + use rgitui_test_support::TempRepo; use tempfile::TempDir; struct Fixture { - _dir: TempDir, + repo: TempRepo, path: PathBuf, - repo: Repository, } impl Fixture { fn new() -> Self { - let dir = TempDir::new().unwrap(); - let path = dir.path().to_path_buf(); - let repo = Repository::init(&path).unwrap(); - let mut config = repo.config().unwrap(); - config.set_str("user.name", "Test").unwrap(); - config.set_str("user.email", "t@t.com").unwrap(); - // Keep line endings byte-exact so content assertions hold on - // Windows, where autocrlf would rewrite what we read back. - config.set_bool("core.autocrlf", false).unwrap(); - drop(config); - Self { - _dir: dir, - path, - repo, - } + let repo = TempRepo::init(); + let path = repo.path().to_path_buf(); + Self { repo, path } } fn write(&self, name: &str, contents: &str) { - self.write_bytes(name, contents.as_bytes()); + self.repo.write_file(name, contents); } fn write_bytes(&self, name: &str, contents: &[u8]) { - let path = self.path.join(name); - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent).unwrap(); - } - std::fs::write(path, contents).unwrap(); + self.repo.write_file_bytes(name, contents); } fn read(&self, name: &str) -> String { @@ -2126,48 +2110,40 @@ mod worktree_patch_integration_tests { } fn commit(&self, message: &str, files: &[&str]) -> Oid { - let signature = git2::Signature::now("Test", "t@t.com").unwrap(); - let mut index = self.repo.index().unwrap(); for file in files { - index.add_path(Path::new(file)).unwrap(); + self.repo.stage(file); } - index.write().unwrap(); - let tree = self.repo.find_tree(index.write_tree().unwrap()).unwrap(); - let parents = match self.repo.head().ok().and_then(|h| h.peel_to_commit().ok()) { - Some(parent) => vec![parent], - None => Vec::new(), - }; - let parent_refs: Vec<&git2::Commit> = parents.iter().collect(); - self.repo - .commit( - Some("HEAD"), - &signature, - &signature, - message, - &tree, - &parent_refs, - ) - .unwrap() + self.repo.commit(message) } fn branch(&self, name: &str) { - let head = self.repo.head().unwrap().peel_to_commit().unwrap(); - self.repo.branch(name, &head, true).unwrap(); + self.repo.branch(name); } fn checkout(&self, name: &str) { let reference = format!("refs/heads/{name}"); - let object = self.repo.revparse_single(&reference).unwrap(); - self.repo.checkout_tree(&object, None).unwrap(); - self.repo.set_head(&reference).unwrap(); + let object = self.repo.repo().revparse_single(&reference).unwrap(); + self.repo.repo().checkout_tree(&object, None).unwrap(); + self.repo.repo().set_head(&reference).unwrap(); } fn head_branch_name(&self) -> String { - self.repo.head().unwrap().shorthand().unwrap().to_string() + self.repo + .repo() + .head() + .unwrap() + .shorthand() + .unwrap() + .to_string() } fn set_config(&self, name: &str, value: &str) { - self.repo.config().unwrap().set_str(name, value).unwrap(); + self.repo + .repo() + .config() + .unwrap() + .set_str(name, value) + .unwrap(); } } @@ -2397,6 +2373,7 @@ mod worktree_patch_integration_tests { let diff = git2::Diff::from_buffer(patch.as_bytes()).unwrap(); fixture .repo + .repo() .apply(&diff, git2::ApplyLocation::WorkDir, None) .expect_err("libgit2 matches context literally, and line 5 no longer matches"); assert_eq!( diff --git a/crates/rgitui_test_support/src/temp_repo.rs b/crates/rgitui_test_support/src/temp_repo.rs index 1643ae0..530e050 100644 --- a/crates/rgitui_test_support/src/temp_repo.rs +++ b/crates/rgitui_test_support/src/temp_repo.rs @@ -131,6 +131,15 @@ impl TempRepo { /// Writes `contents` to `relative_path` in the working tree, creating parent /// directories. The change is left unstaged. pub fn write_file(&self, relative_path: impl AsRef, contents: &str) { + self.write_file_bytes(relative_path, contents.as_bytes()); + } + + /// Writes exact bytes to `relative_path` in the working tree, creating + /// parent directories. The change is left unstaged. + /// + /// Use this for encoding, binary-content, and invalid-UTF-8 tests; prefer + /// [`TempRepo::write_file`] for ordinary text fixtures. + pub fn write_file_bytes(&self, relative_path: impl AsRef, contents: &[u8]) { let path = self.path().join(relative_path); if let Some(parent) = path.parent() { std::fs::create_dir_all(parent).expect("failed to create parent dirs"); From 68c0cae18f72200ef2509dcc46245beae71511d0 Mon Sep 17 00:00:00 2001 From: Noah Clarkson Date: Sun, 9 Aug 2026 18:59:05 +1200 Subject: [PATCH 7/9] fix(workspace): route diff actions to their owning tab --- crates/rgitui_workspace/src/workspace/events.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/crates/rgitui_workspace/src/workspace/events.rs b/crates/rgitui_workspace/src/workspace/events.rs index 87896db..ce1f358 100644 --- a/crates/rgitui_workspace/src/workspace/events.rs +++ b/crates/rgitui_workspace/src/workspace/events.rs @@ -2367,11 +2367,15 @@ pub(super) fn subscribe_diff_viewer( } } - // Hunk/line staging must target the inspected worktree's index, not - // the main repo's. Route every op through the `_at` variant with the - // active tab's effective worktree (the main repo path when no - // worktree is being inspected). - let worktree_path = this.effective_worktree_path(cx); + // Route the request through the tab that owns the emitting diff + // viewer. Using the active tab here can target another worktree if + // focus changes while an input event is being delivered. + let worktree_path = this + .tabs + .iter() + .find(|tab| tab.diff_viewer == diff_viewer_ref) + .map(|tab| tab.effective_repo_path(cx)) + .unwrap_or_else(|| this.effective_worktree_path(cx)); match event { DiffViewerEvent::HunkStageRequested(hunk_idx) => { let idx = *hunk_idx; From 705abf1fdc9c2187f120702b657d3c57ecac5e1e Mon Sep 17 00:00:00 2001 From: Noah Clarkson Date: Sun, 9 Aug 2026 19:00:24 +1200 Subject: [PATCH 8/9] fix(git): keep patch-conflict guidance accurate --- crates/rgitui_git/src/project/worktree_patch.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/crates/rgitui_git/src/project/worktree_patch.rs b/crates/rgitui_git/src/project/worktree_patch.rs index 469e279..5fc01bc 100644 --- a/crates/rgitui_git/src/project/worktree_patch.rs +++ b/crates/rgitui_git/src/project/worktree_patch.rs @@ -1408,16 +1408,11 @@ fn conflict_message( // because the file itself has moved on since that revision. format!( "Can't {} {} of {}: the surrounding lines have changed since {}, so the patch no \ - longer fits. {} the whole file from the file menu to take that revision's version \ - wholesale.", + longer fits cleanly. Select a closer revision or make the change manually.", direction.verb(), scope.describe(), file_path.display(), - source.label(), - match direction { - WorktreePatchDirection::Apply => "Apply", - WorktreePatchDirection::Revert => "Revert", - } + source.label() ) } } From be524a534a2f807b0a6300d09d0d786f5214b37d Mon Sep 17 00:00:00 2001 From: Noah Clarkson Date: Sun, 9 Aug 2026 19:13:36 +1200 Subject: [PATCH 9/9] fix(git): reject split non-regular type changes --- .../rgitui_git/src/project/worktree_patch.rs | 56 +++++++++++++++---- 1 file changed, 46 insertions(+), 10 deletions(-) diff --git a/crates/rgitui_git/src/project/worktree_patch.rs b/crates/rgitui_git/src/project/worktree_patch.rs index 5fc01bc..3fc2023 100644 --- a/crates/rgitui_git/src/project/worktree_patch.rs +++ b/crates/rgitui_git/src/project/worktree_patch.rs @@ -538,16 +538,21 @@ fn is_regular_blob_mode(mode: FileMode) -> bool { /// Read `file_path`'s diff out of `source` as hunks of [`ScopedLine`]s. /// -/// The diff is computed with default options so hunk indices line up with the -/// `FileDiff` the viewer is displaying, which came from the same tree pair via -/// `parse_multi_file_diff`. +/// Textual deltas use the viewer's default diff shape so their hunk indices line +/// up with `parse_multi_file_diff`. Type changes are kept intact only so an +/// unsupported symlink or submodule cannot masquerade as a plain deletion. fn scoped_hunks( repo: &Repository, source: &WorktreePatchSource, file_path: &Path, ) -> Result { let (from_tree, to_tree) = source.trees(repo)?; - let diff = repo.diff_tree_to_tree(from_tree.as_ref(), to_tree.as_ref(), None)?; + // Keep regular-file/symlink/submodule transitions in one delta. With + // libgit2's default split form, the first matching delta can look like a + // harmless deletion and hide the non-regular addition that follows it. + let mut options = git2::DiffOptions::new(); + options.include_typechange(true); + let diff = repo.diff_tree_to_tree(from_tree.as_ref(), to_tree.as_ref(), Some(&mut options))?; for (delta_index, delta) in diff.deltas().enumerate() { let old_path = delta.old_file().path().map(Path::to_path_buf); @@ -2111,6 +2116,42 @@ mod worktree_patch_integration_tests { self.repo.commit(message) } + fn commit_symlink(&self, message: &str, name: &str, target: &Path) -> Oid { + let target = target.to_string_lossy(); + let blob = self.repo.repo().blob(target.as_bytes()).unwrap(); + let mut index = self.repo.repo().index().unwrap(); + index + .add(&IndexEntry { + ctime: IndexTime::new(0, 0), + mtime: IndexTime::new(0, 0), + dev: 0, + ino: 0, + mode: 0o120000, + uid: 0, + gid: 0, + file_size: target.len() as u32, + id: blob, + flags: 0, + flags_extended: 0, + path: name.as_bytes().to_vec(), + }) + .unwrap(); + index.write().unwrap(); + let tree = index.write_tree().unwrap(); + assert_eq!( + self.repo + .repo() + .find_tree(tree) + .unwrap() + .get_path(Path::new(name)) + .unwrap() + .filemode(), + 0o120000 + ); + self.repo + .commit_tree(Some("HEAD"), message, tree, &[self.repo.head_oid()]) + } + fn branch(&self, name: &str) { self.repo.branch(name); } @@ -3175,7 +3216,6 @@ mod worktree_patch_integration_tests { ); } - #[cfg(unix)] #[test] fn a_tracked_symlink_diff_is_not_materialized_as_a_regular_file() { let fixture = Fixture::new(); @@ -3184,11 +3224,7 @@ mod worktree_patch_integration_tests { let outside = TempDir::new().unwrap(); let target = outside.path().join("target.txt"); std::fs::write(&target, "outside\n").unwrap(); - std::fs::remove_file(fixture.path.join("f.txt")).unwrap(); - std::os::unix::fs::symlink(&target, fixture.path.join("f.txt")).unwrap(); - let oid = fixture.commit("replace with symlink", &["f.txt"]); - std::fs::remove_file(fixture.path.join("f.txt")).unwrap(); - fixture.write("f.txt", "base\n"); + let oid = fixture.commit_symlink("replace with symlink", "f.txt", &target); let error = apply( &fixture,