From 58e8ba16d13e73f06655c1546586940858e022e4 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 9 Aug 2026 00:40:21 +0800 Subject: [PATCH] feat(git): ask for three kinds of diff, and lift the row model out of the renderer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `probe` could only ever run `git diff HEAD`. A `DiffRequest` now names the source (worktree, staged, HEAD, one commit, a range), the pathspecs, the context width and the budget, and `DiffSource::args` is the single place any diff argv is built — which is also the single place to test it. Three things were verified against git 2.50.1 rather than assumed: - `-c core.quotePath=false` is a real bug fix, not tidiness. With quoting on, `diff --git "a/\344\270\255\346\226\207\345\220\215.txt" …`; off, `diff --git a/中文名.txt b/中文名.txt`. `parse_quoted_pair` does not decode octal, so every non-ASCII path in the overlay was simply wrong. `ls-files` quotes the same way, so the untracked listing gets the flag too. - `Commit` runs `log -p -1 --format= --first-parent`, not `diff-tree`. `diff-tree` does not honour `--first-parent` as a narrowing of a merge: over a merge of two branches that each added a file, `diff-tree -p -m --first-parent` emits both files (one patch per parent, concatenated) and dropping `-m` emits nothing at all. `log -p -1 --first-parent` gives the one first-parent patch for a merge, an ordinary commit and the initial commit alike, so there is no special case and no `--root`. - A type change is not one patch. git emits a deletion and a creation for the same path, back to back, with nothing in either header saying why; the pair is folded back into a single `TypeChanged` entry. `old mode`/`new mode` only appears for permission changes, which stay `Modified`. `FileStatus` also gains `Copied` and `Unmerged`. A conflicted path arrives as a combined diff (`diff --cc`, `@@@`, one marker column per parent), so the body parser reads its marker width from the hunk header instead of assuming one. The budget is a parameter now (`DiffBudget::PANEL`, `::SINGLE_FILE`) rather than three module constants read inside the parser. Defaults are unchanged. On the UI side, the overlay's reuse test was `(cwd, host)` — clicking a staged file while a worktree overlay was open took the "just move the focus" branch and went on showing the unstaged patch under the staged file's name. The source is part of an overlay's identity now, in that filter, in the panel-seed shortcut and in the in-flight de-duplication key. `maybe_refresh_diff_overlay` only compares HEAD snapshots against the cached `--numstat HEAD` counts, which are the only counts they are comparable to. `split_hunk` and tab expansion move to `ui::diff_rows` alongside a new `unified_rows`, so the two renderings of a hunk are built and tested in one place, without a window. The overlay's file list finally hangs on tty7's own scrollbar instead of a bare `overflow_y_scroll`. --- crates/tty7-core/src/core/git/diff.rs | 883 ++++++++++++++++++++++++-- src/ui/diff_overlay.rs | 350 ++++++---- src/ui/diff_rows.rs | 242 +++++++ 3 files changed, 1301 insertions(+), 174 deletions(-) diff --git a/crates/tty7-core/src/core/git/diff.rs b/crates/tty7-core/src/core/git/diff.rs index 5bf692a3..209c2991 100644 --- a/crates/tty7-core/src/core/git/diff.rs +++ b/crates/tty7-core/src/core/git/diff.rs @@ -19,15 +19,172 @@ pub const MAX_RENDERED_FILES: usize = 300; pub const MAX_UNTRACKED: usize = 500; +/// `-U`. Git's own default, spelled out because the request carries it. +pub const DEFAULT_CONTEXT: u32 = 3; + #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum Truncation { PerFile, Budget, } +/// Which patch to ask git for. +/// +/// The three working-tree variants are the same three questions the SCM panel +/// asks — `Worktree` is what is not staged, `Staged` is what is, and `Head` is +/// both at once, which is what the overlay has always shown. +#[derive(Clone, PartialEq, Eq, Hash, Debug, Default)] +pub enum DiffSource { + /// `git diff` — unstaged changes. + Worktree, + /// `git diff --cached` — what a commit right now would contain. + Staged, + /// `git diff HEAD` — staged and unstaged together. + #[default] + Head, + /// One commit against its first parent. + Commit { rev: String }, + /// `base...head`: what `head` added since the two diverged. + Range { base: String, head: String }, +} + +impl DiffSource { + /// The whole argv, minus pathspecs. Every diff tty7 runs is built here so + /// there is one place to read, and one place to test, what git is asked. + pub fn args(&self, context: u32, ignore_whitespace: bool) -> Vec { + // `core.quotePath=false` is not tidiness: with it on, git renders a + // non-ASCII path in the `diff --git` header as C octal escapes, and + // nothing downstream decodes those — the path would simply be wrong. + let mut argv = strings(&["-c", "core.quotePath=false"]); + match self { + DiffSource::Worktree => argv.push("diff".to_string()), + DiffSource::Staged => argv.extend(strings(&["diff", "--cached"])), + DiffSource::Head => argv.extend(strings(&["diff", "HEAD"])), + // `log -p -1`, not `diff-tree`: `diff-tree` does not honour + // `--first-parent` as a *narrowing* of a merge. Measured on git + // 2.50.1 against a merge of two branches that each added a file: + // `diff-tree -p -m --first-parent` emits both files (one patch per + // parent, concatenated), and dropping `-m` emits nothing at all. + // `log -p -1 --first-parent` gives the one first-parent patch for a + // merge, an ordinary commit and the initial commit alike, so there + // is no special case and no need for `--root`. `--format=` empties + // the commit header, at the cost of one blank line the parser + // ignores. + DiffSource::Commit { rev } => { + argv.extend(strings(&["log", "-p", "-1", "--format=", "--first-parent"])); + argv.push(rev.clone()); + } + // Three dots: measured from the merge base, so unrelated work on + // `base` does not show up as if `head` had reverted it. + DiffSource::Range { base, head } => { + argv.push("diff".to_string()); + argv.push(format!("{base}...{head}")); + } + } + argv.extend(strings(&[ + "--no-color", + "--no-ext-diff", + "--no-textconv", + "-M", + ])); + argv.push(format!("-U{context}")); + if ignore_whitespace { + argv.push("-w".to_string()); + } + argv + } + + /// Whether untracked files belong in the snapshot. They are a property of + /// the working tree, so a commit or a range has none, and a staged diff + /// does not either — an untracked file is by definition not in the index. + pub fn lists_untracked(&self) -> bool { + matches!(self, DiffSource::Worktree | DiffSource::Head) + } +} + +fn strings(args: &[&str]) -> Vec { + args.iter().map(|a| a.to_string()).collect() +} + +/// How much of a patch is worth keeping in memory. +/// +/// The line counts on [`FileDiff`] stay exact past every one of these; only the +/// retained text is bounded. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct DiffBudget { + pub max_lines_per_file: usize, + pub max_total_lines: usize, + pub max_files_with_hunks: usize, +} + +impl DiffBudget { + /// A whole tree at once: no single file may crowd out the rest. + pub const PANEL: DiffBudget = DiffBudget { + max_lines_per_file: MAX_LINES_PER_FILE, + max_total_lines: MAX_TOTAL_LINES, + max_files_with_hunks: MAX_FILES_WITH_HUNKS, + }; + + /// One file the user asked for by name. There is nothing to crowd out, so + /// the per-file cap rises to where it is a defence against a generated + /// file rather than a rationing rule. + pub const SINGLE_FILE: DiffBudget = DiffBudget { + max_lines_per_file: 50_000, + max_total_lines: MAX_TOTAL_LINES, + max_files_with_hunks: MAX_FILES_WITH_HUNKS, + }; +} + +/// The whole point of the second budget. Pinned here so that trimming +/// [`MAX_LINES_PER_FILE`] one day cannot silently make the two identical. +const _: () = + assert!(DiffBudget::SINGLE_FILE.max_lines_per_file > DiffBudget::PANEL.max_lines_per_file); + +impl Default for DiffBudget { + fn default() -> DiffBudget { + DiffBudget::PANEL + } +} + +/// One patch to fetch and parse. +pub struct DiffRequest<'a> { + pub source: DiffSource, + /// Empty means the whole tree. Every entry must already be a pathspec — + /// `:(literal)…` — because git globs a bare path and a file named `a[b].c` + /// would then match nothing. + pub paths: &'a [String], + pub context: u32, + pub budget: DiffBudget, + pub ignore_whitespace: bool, +} + +impl Default for DiffRequest<'_> { + fn default() -> DiffRequest<'static> { + DiffRequest { + source: DiffSource::default(), + paths: &[], + context: DEFAULT_CONTEXT, + budget: DiffBudget::PANEL, + ignore_whitespace: false, + } + } +} + +impl DiffRequest<'_> { + pub fn args(&self) -> Vec { + let mut argv = self.source.args(self.context, self.ignore_whitespace); + if !self.paths.is_empty() { + argv.push("--".to_string()); + argv.extend(self.paths.iter().cloned()); + } + argv + } +} + #[derive(Clone, PartialEq, Eq, Debug, Default)] pub struct DiffSnapshot { pub root: PathBuf, + pub source: DiffSource, pub branch: String, pub files: Vec, pub untracked: Vec, @@ -91,6 +248,12 @@ pub enum FileStatus { Modified, Deleted, Renamed, + Copied, + /// Regular file ↔ symlink ↔ submodule. Not a mode change: `100644` to + /// `100755` is still [`FileStatus::Modified`]. + TypeChanged, + /// A path with conflict markers, or one git refused to diff at all. + Unmerged, } #[derive(Clone, PartialEq, Eq, Debug)] @@ -126,43 +289,66 @@ pub struct DiffLine { pub text: String, } +/// The whole working tree against `HEAD`, on the panel's budget. pub fn probe(host: &dyn Host, cwd: &Path) -> Option { - let root = git::git(host, cwd, &["rev-parse", "--show-toplevel"])?; - let root = PathBuf::from(root.trim_end_matches(['\n', '\r'])); - let branch = git::branch_name(host, cwd)?; - let mut parser = DiffParser::default(); - let diffed = host.git_lines( - cwd, - &["diff", "--no-color", "--no-ext-diff", "-M", "HEAD"], - &mut |line| parser.push_line(line), - ); + probe_diff(host, cwd, &DiffRequest::default()) +} + +pub fn probe_diff(host: &dyn Host, root: &Path, req: &DiffRequest<'_>) -> Option { + let toplevel = git::git(host, root, &["rev-parse", "--show-toplevel"])?; + let toplevel = PathBuf::from(toplevel.trim_end_matches(['\n', '\r'])); + let branch = git::branch_name(host, root)?; + + let argv = req.args(); + let argv: Vec<&str> = argv.iter().map(String::as_str).collect(); + let mut parser = DiffParser::with_budget(req.budget); + let diffed = host.git_lines(root, &argv, &mut |line| parser.push_line(line)); let files = match diffed { Ok(Some(0)) => parser.finish(), _ => Vec::new(), }; + let mut untracked: Vec = Vec::new(); let mut untracked_total = 0usize; - let listed = host.git_lines( - cwd, - &["ls-files", "--others", "--exclude-standard", "--full-name"], - &mut |line| { - untracked_total += 1; - if untracked.len() < MAX_UNTRACKED { - untracked.push(line.to_string()); - } - }, - ); - if !matches!(listed, Ok(Some(0))) { + // A pathspec-limited request is answering "what changed in these files", + // so a list of everything else in the tree would be noise. + let list_untracked = req.source.lists_untracked() && req.paths.is_empty(); + let listed = list_untracked.then(|| { + host.git_lines( + root, + &[ + "-c", + "core.quotePath=false", + "ls-files", + "--others", + "--exclude-standard", + "--full-name", + ], + &mut |line| { + untracked_total += 1; + if untracked.len() < MAX_UNTRACKED { + untracked.push(line.to_string()); + } + }, + ) + }); + let untracked_ok = match &listed { + Some(listed) => matches!(listed, Ok(Some(0))), + None => true, + }; + if !untracked_ok { untracked.clear(); untracked_total = 0; } + Some(DiffSnapshot { - root, + root: toplevel, + source: req.source.clone(), branch, files, untracked, untracked_total, - read_failed: !matches!(diffed, Ok(Some(0))) || !matches!(listed, Ok(Some(0))), + read_failed: !matches!(diffed, Ok(Some(0))) || !untracked_ok, }) } @@ -177,6 +363,7 @@ pub fn parse_unified(out: &str) -> Vec { #[derive(Default)] pub struct DiffParser { + budget: DiffBudget, files: Vec, old_no: u32, new_no: u32, @@ -184,26 +371,44 @@ pub struct DiffParser { total_lines: usize, files_with_hunks: usize, in_hunk: bool, + /// How many marker columns the current hunk's body lines carry: one for an + /// ordinary patch, one per parent for a combined (`diff --cc`) one. + markers: usize, + old_mode: String, } impl DiffParser { + pub fn with_budget(budget: DiffBudget) -> DiffParser { + DiffParser { + budget, + ..DiffParser::default() + } + } + pub fn push_line(&mut self, line: &str) { if let Some(rest) = line.strip_prefix("diff --git ") { let (old_p, new_p) = parse_git_header_paths(rest); - self.files.push(FileDiff { - path: new_p.clone(), - old_path: (old_p != new_p).then_some(old_p), - status: FileStatus::Modified, - added: 0, - removed: 0, - binary: false, - truncated: None, - hunks: Vec::new(), - }); - self.file_lines = 0; - self.in_hunk = false; + self.start_file(new_p.clone(), (old_p != new_p).then_some(old_p), None); return; } + // A conflicted path comes out as a combined diff against every merge + // parent at once, which is git's only way of saying "unmerged" in a + // patch — the `diff --git` header has no status field. + if let Some(rest) = line + .strip_prefix("diff --cc ") + .or_else(|| line.strip_prefix("diff --combined ")) + { + let path = parse_combined_header_path(rest); + self.start_file(path, None, Some(FileStatus::Unmerged)); + return; + } + // `git diff --cached` cannot show a conflicted path at all and says so + // on its own line, before any patch. + if let Some(rest) = line.strip_prefix("* Unmerged path ") { + self.start_file(rest.to_string(), None, Some(FileStatus::Unmerged)); + return; + } + let old_mode = std::mem::take(&mut self.old_mode); let Some(file) = self.files.last_mut() else { return; }; @@ -219,6 +424,20 @@ impl DiffParser { file.status = FileStatus::Renamed; return; } + if line.starts_with("copy from ") { + file.status = FileStatus::Copied; + return; + } + if let Some(mode) = line.strip_prefix("old mode ") { + self.old_mode = mode.trim().to_string(); + return; + } + if let Some(mode) = line.strip_prefix("new mode ") { + if !old_mode.is_empty() && object_type(&old_mode) != object_type(mode.trim()) { + file.status = FileStatus::TypeChanged; + } + return; + } if line.starts_with("Binary files ") || line.starts_with("GIT binary patch") { file.binary = true; return; @@ -231,12 +450,16 @@ impl DiffParser { } if line.starts_with("@@") { self.in_hunk = true; + // `@@` is one marker column, `@@@` two — a combined diff carries + // one per merge parent. Set before any early return: the line + // counts below stay exact even for a file whose body was dropped. + self.markers = line.bytes().take_while(|b| *b == b'@').count().max(2) - 1; if file.truncated.is_some() { return; } let first_hunk = file.hunks.is_empty(); - if (first_hunk && self.files_with_hunks >= MAX_FILES_WITH_HUNKS) - || self.total_lines >= MAX_TOTAL_LINES + if (first_hunk && self.files_with_hunks >= self.budget.max_files_with_hunks) + || self.total_lines >= self.budget.max_total_lines { file.truncated = Some(Truncation::Budget); return; @@ -256,11 +479,8 @@ impl DiffParser { if !self.in_hunk { return; } - let (kind, text) = match line.as_bytes().first() { - Some(b'+') => (LineKind::Added, &line[1..]), - Some(b'-') => (LineKind::Removed, &line[1..]), - Some(b' ') => (LineKind::Context, &line[1..]), - _ => return, + let Some((kind, text)) = split_body_line(line, self.markers) else { + return; }; match kind { LineKind::Added => file.added += 1, @@ -271,11 +491,11 @@ impl DiffParser { return; } self.file_lines += 1; - if self.file_lines > MAX_LINES_PER_FILE { + if self.file_lines > self.budget.max_lines_per_file { file.truncated = Some(Truncation::PerFile); return; } - if self.total_lines >= MAX_TOTAL_LINES { + if self.total_lines >= self.budget.max_total_lines { file.truncated = Some(Truncation::Budget); return; } @@ -310,8 +530,76 @@ impl DiffParser { } pub fn finish(self) -> Vec { - self.files + fold_type_changes(self.files) } + + fn start_file(&mut self, path: String, old_path: Option, status: Option) { + self.files.push(FileDiff { + path, + old_path, + status: status.unwrap_or(FileStatus::Modified), + added: 0, + removed: 0, + binary: false, + truncated: None, + hunks: Vec::new(), + }); + self.file_lines = 0; + self.in_hunk = false; + self.markers = 1; + self.old_mode.clear(); + } +} + +/// A path whose *type* changed is not one patch: git emits the deletion and the +/// creation back to back, and the header of neither says why. The adjacent pair +/// over one path is the whole signal — git has no other reason to produce it — +/// so it is folded back into the single entry the user thinks of. +fn fold_type_changes(files: Vec) -> Vec { + let mut folded: Vec = Vec::with_capacity(files.len()); + for file in files { + let pair = folded.last().is_some_and(|prev| { + prev.status == FileStatus::Deleted + && file.status == FileStatus::Added + && prev.path == file.path + }); + match folded.last_mut().filter(|_| pair) { + Some(prev) => { + prev.status = FileStatus::TypeChanged; + prev.added += file.added; + prev.removed += file.removed; + prev.binary |= file.binary; + prev.truncated = prev.truncated.or(file.truncated); + prev.hunks.extend(file.hunks); + } + None => folded.push(file), + } + } + folded +} + +/// The `100`/`120`/`160` of a git file mode: regular file, symlink, submodule. +/// Permission bits are deliberately dropped — `100644` to `100755` is a +/// modification, not a type change. +fn object_type(mode: &str) -> &str { + &mode[..mode.len().min(3)] +} + +/// Splits a hunk body line into its kind and its text, given how many marker +/// columns the hunk carries. A combined diff marks a line per parent; one `+` +/// or `-` anywhere in those columns settles what happened to the line. +fn split_body_line(line: &str, markers: usize) -> Option<(LineKind, &str)> { + let head = line.get(..markers)?; + let kind = if head.contains('+') { + LineKind::Added + } else if head.contains('-') { + LineKind::Removed + } else if head.bytes().all(|b| b == b' ') { + LineKind::Context + } else { + return None; + }; + Some((kind, &line[markers..])) } fn is_hunk_line(line: &str) -> bool { @@ -366,13 +654,34 @@ fn strip_prefix_ab(p: &str) -> String { .to_string() } +/// A combined hunk header carries one `-` range per merge parent before the +/// single `+` range, so the ranges are read positionally rather than by a fixed +/// shape. The first `-` is the first parent's, which is the side tty7 numbers. fn parse_hunk_starts(line: &str) -> Option<(u32, u32)> { - let rest = line.strip_prefix("@@ -")?; - let (old_part, rest) = rest.split_once(" +")?; - let (new_part, _) = rest.split_once(" @@")?; - let old = old_part.split(',').next()?.parse().ok()?; - let new = new_part.split(',').next()?.parse().ok()?; - Some((old, new)) + let ranges = line.trim_start_matches('@'); + let ranges = &ranges[..ranges.find(" @@")?]; + let mut old = None; + let mut new = None; + for range in ranges.split_whitespace() { + let (sign, rest) = range.split_at_checked(1)?; + let start: u32 = rest.split(',').next()?.parse().ok()?; + match sign { + "-" if old.is_none() => old = Some(start), + "+" => new = Some(start), + _ => {} + } + } + Some((old?, new?)) +} + +/// `diff --cc `: one path, repo-relative, with no `a/` or `b/` prefix. +fn parse_combined_header_path(rest: &str) -> String { + match rest.starts_with('"') { + true => parse_quoted_pair(rest) + .pop() + .unwrap_or_else(|| rest.to_string()), + false => rest.to_string(), + } } #[cfg(test)] @@ -446,6 +755,176 @@ Binary files a/img.png and b/img.png differ assert!(b.hunks.is_empty()); } + fn argv(source: DiffSource) -> Vec { + source.args(DEFAULT_CONTEXT, false) + } + + #[test] + fn every_source_spells_out_its_own_argv() { + let common = ["--no-color", "--no-ext-diff", "--no-textconv", "-M", "-U3"]; + let cases: Vec<(DiffSource, Vec<&str>)> = vec![ + (DiffSource::Worktree, vec!["diff"]), + (DiffSource::Staged, vec!["diff", "--cached"]), + (DiffSource::Head, vec!["diff", "HEAD"]), + ( + DiffSource::Commit { + rev: "deadbeef".into(), + }, + vec!["log", "-p", "-1", "--format=", "--first-parent", "deadbeef"], + ), + ( + DiffSource::Range { + base: "main".into(), + head: "topic".into(), + }, + vec!["diff", "main...topic"], + ), + ]; + for (source, middle) in cases { + let want: Vec = ["-c", "core.quotePath=false"] + .into_iter() + .chain(middle) + .chain(common) + .map(str::to_string) + .collect(); + assert_eq!(argv(source.clone()), want, "{source:?}"); + } + } + + #[test] + fn quote_path_is_off_on_every_source() { + // Left on, a non-ASCII path arrives as C octal escapes that nothing + // downstream decodes — `parse_quoted_pair` would hand back the literal + // digits. Verified against git 2.50.1: `diff --git + // "a/\344\270\255\346\226\207\345\220\215.txt" …` becomes + // `diff --git a/中文名.txt b/中文名.txt` once this is off. + for source in [ + DiffSource::Worktree, + DiffSource::Staged, + DiffSource::Head, + DiffSource::Commit { rev: "HEAD".into() }, + DiffSource::Range { + base: "a".into(), + head: "b".into(), + }, + ] { + assert_eq!(&argv(source.clone())[..2], ["-c", "core.quotePath=false"]); + } + } + + #[test] + fn octal_escaped_paths_are_what_the_flag_prevents() { + let escaped = parse_unified( + "diff --git \"a/\\344\\270\\255\\346\\226\\207\\345\\220\\215.txt\" \ + \"b/\\344\\270\\255\\346\\226\\207\\345\\220\\215.txt\"\n", + ); + assert_ne!( + escaped[0].path, "中文名.txt", + "the escapes are not decoded here, which is why they must not be produced" + ); + + let raw = parse_unified("diff --git a/中文名.txt b/中文名.txt\n"); + assert_eq!(raw[0].path, "中文名.txt"); + } + + #[test] + fn a_request_appends_its_pathspecs_after_a_separator() { + let paths = [":(literal)src/a[b].rs".to_string()]; + let req = DiffRequest { + source: DiffSource::Staged, + paths: &paths, + context: 0, + budget: DiffBudget::SINGLE_FILE, + ignore_whitespace: true, + }; + assert_eq!( + req.args(), + [ + "-c", + "core.quotePath=false", + "diff", + "--cached", + "--no-color", + "--no-ext-diff", + "--no-textconv", + "-M", + "-U0", + "-w", + "--", + ":(literal)src/a[b].rs", + ] + ); + assert_eq!( + DiffRequest::default() + .args() + .iter() + .filter(|a| *a == "--") + .count(), + 0, + "a whole-tree request has nothing to separate" + ); + } + + #[test] + fn the_default_request_is_the_panel_probe() { + let req = DiffRequest::default(); + assert_eq!(req.source, DiffSource::Head); + assert_eq!(req.budget, DiffBudget::PANEL); + assert_eq!(req.args(), argv(DiffSource::Head)); + } + + #[test] + fn a_default_snapshot_still_reads_as_head() { + let snap = DiffSnapshot::default(); + assert_eq!( + snap.source, + DiffSource::Head, + "`..Default::default()` callers keep the source they always had" + ); + } + + #[test] + fn the_single_file_budget_only_lifts_the_per_file_cap() { + assert_eq!(DiffBudget::PANEL, DiffBudget::default()); + assert_eq!( + DiffBudget::SINGLE_FILE.max_total_lines, + DiffBudget::PANEL.max_total_lines + ); + assert_eq!( + DiffBudget::SINGLE_FILE.max_files_with_hunks, + DiffBudget::PANEL.max_files_with_hunks + ); + + let mut out = String::from( + "diff --git a/big.txt b/big.txt\nindex 1..2 100644\n--- a/big.txt\n+++ b/big.txt\n@@ -0,0 +1,3000 @@\n", + ); + for i in 0..3000 { + out.push_str(&format!("+line {i}\n")); + } + let mut parser = DiffParser::with_budget(DiffBudget::SINGLE_FILE); + for line in out.lines() { + parser.push_line(line); + } + let files = parser.finish(); + assert_eq!(files[0].truncated, None, "3000 lines fit under 50_000"); + assert_eq!(files[0].hunks[0].lines.len(), 3000); + } + + #[test] + fn untracked_files_belong_to_the_working_tree_only() { + assert!(DiffSource::Worktree.lists_untracked()); + assert!(DiffSource::Head.lists_untracked()); + assert!(!DiffSource::Staged.lists_untracked()); + assert!(!DiffSource::Commit { rev: "x".into() }.lists_untracked()); + assert!( + !DiffSource::Range { + base: "a".into(), + head: "b".into() + } + .lists_untracked() + ); + } + #[test] fn parses_renames() { let out = "\ @@ -462,6 +941,116 @@ rename to new/name.rs assert_eq!((files[0].added, files[0].removed), (0, 0)); } + #[test] + fn parses_copies() { + let out = "\ +diff --git a/tpl.rs b/copy.rs +similarity index 100% +copy from tpl.rs +copy to copy.rs +"; + let files = parse_unified(out); + assert_eq!(files[0].status, FileStatus::Copied); + assert_eq!(files[0].path, "copy.rs"); + assert_eq!(files[0].old_path.as_deref(), Some("tpl.rs")); + } + + #[test] + fn a_type_change_is_one_file_not_a_delete_and_an_add() { + // git 2.50.1, `git diff` over a regular file replaced by a symlink. + let out = "\ +diff --git a/t.txt b/t.txt +deleted file mode 100644 +index 587be6b..0000000 +--- a/t.txt ++++ /dev/null +@@ -1 +0,0 @@ +-x +diff --git a/t.txt b/t.txt +new file mode 120000 +index 0000000..1de5659 +--- /dev/null ++++ b/t.txt +@@ -0,0 +1 @@ ++target +\\ No newline at end of file +"; + let files = parse_unified(out); + assert_eq!(files.len(), 1, "one path, one row"); + assert_eq!(files[0].status, FileStatus::TypeChanged); + assert_eq!((files[0].added, files[0].removed), (1, 1)); + assert_eq!(files[0].hunks.len(), 2, "both halves stay readable"); + } + + #[test] + fn a_permission_change_is_still_a_modification() { + let files = parse_unified("diff --git a/t.sh b/t.sh\nold mode 100644\nnew mode 100755\n"); + assert_eq!(files[0].status, FileStatus::Modified); + } + + #[test] + fn a_mode_change_across_object_types_is_a_type_change() { + let files = parse_unified("diff --git a/t b/t\nold mode 100644\nnew mode 120000\n"); + assert_eq!(files[0].status, FileStatus::TypeChanged); + } + + #[test] + fn a_delete_and_an_add_of_different_paths_stay_two_files() { + let files = parse_unified( + "diff --git a/gone.txt b/gone.txt\ndeleted file mode 100644\n\ + diff --git a/new.txt b/new.txt\nnew file mode 100644\n", + ); + assert_eq!(files.len(), 2); + assert_eq!(files[0].status, FileStatus::Deleted); + assert_eq!(files[1].status, FileStatus::Added); + } + + #[test] + fn a_conflicted_file_parses_as_a_combined_diff() { + // git 2.50.1, `git diff` during a conflicted merge. Two marker columns, + // and a hunk header with one range per parent. + let out = "\ +diff --cc f.txt +index af70335,f794161..0000000 +--- a/f.txt ++++ b/f.txt +@@@ -1,3 -1,3 +1,7 @@@ + a +++<<<<<<< HEAD + +MAIN +++======= ++ SIDE +++>>>>>>> side + c +"; + let files = parse_unified(out); + assert_eq!(files.len(), 1); + assert_eq!(files[0].path, "f.txt"); + assert_eq!(files[0].status, FileStatus::Unmerged); + + let lines = &files[0].hunks[0].lines; + assert_eq!(lines.len(), 7); + assert_eq!(lines[0].kind, LineKind::Context); + assert_eq!(lines[0].text, "a", "both marker columns come off the text"); + assert_eq!((lines[0].old_no, lines[0].new_no), (Some(1), Some(1))); + assert_eq!(lines[1].kind, LineKind::Added); + assert_eq!(lines[1].text, "<<<<<<< HEAD"); + assert_eq!(lines[4].kind, LineKind::Added); + assert_eq!(lines[4].text, "SIDE", "added on one side is still added"); + assert_eq!(lines[6].text, "c"); + assert_eq!((files[0].added, files[0].removed), (5, 0)); + } + + #[test] + fn a_staged_diff_reports_the_paths_it_refused_to_show() { + let files = parse_unified("* Unmerged path f.txt\ndiff --git a/ok.rs b/ok.rs\n"); + assert_eq!(files.len(), 2); + assert_eq!(files[0].path, "f.txt"); + assert_eq!(files[0].status, FileStatus::Unmerged); + assert!(files[0].hunks.is_empty()); + assert_eq!(files[1].path, "ok.rs"); + } + #[test] fn parses_quoted_paths() { let out = "diff --git \"a/has space.txt\" \"b/has space.txt\"\n"; @@ -655,6 +1244,200 @@ index 1..2 100644 ); } + /// A repo with a merge whose two parents each added a file, plus an + /// ordinary commit and an initial one. Returns `None` if git is missing. + fn merge_repo(name: &str) -> Option { + let dir = std::env::temp_dir().join(format!("tty7-diff-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).ok()?; + let run = |args: &[&str]| { + git::git_output(&dir, args) + .ok() + .filter(|out| out.success()) + .is_some() + }; + if !run(&["init", "-q", "-b", "mainwork", "."]) { + return None; + } + run(&["config", "user.email", "t@tty7.test"]); + run(&["config", "user.name", "tty7 test"]); + std::fs::write(dir.join("base.txt"), "base\n").ok()?; + run(&["add", "-A"]); + run(&["commit", "-qm", "initial"]); + run(&["checkout", "-q", "-b", "side"]); + std::fs::write(dir.join("s.txt"), "side\n").ok()?; + run(&["add", "-A"]); + run(&["commit", "-qm", "side"]); + run(&["checkout", "-q", "mainwork"]); + std::fs::write(dir.join("m.txt"), "main\n").ok()?; + run(&["add", "-A"]); + run(&["commit", "-qm", "main"]); + run(&["merge", "-q", "--no-ff", "-m", "merge side", "side"]); + Some(dir) + } + + fn rev(dir: &Path, spec: &str) -> String { + git::git_output(dir, &["rev-parse", spec]) + .map(|out| String::from_utf8_lossy(&out.stdout).trim().to_string()) + .unwrap_or_default() + } + + fn commit_files(host: &dyn Host, dir: &Path, spec: &str) -> Vec { + let req = DiffRequest { + source: DiffSource::Commit { + rev: rev(dir, spec), + }, + ..Default::default() + }; + probe_diff(host, dir, &req) + .expect("the scratch repo answers") + .files + .into_iter() + .map(|f| f.path) + .collect() + } + + /// The reason `Commit` runs `log -p -1 --first-parent` and not `diff-tree`: + /// on git 2.50.1 `diff-tree -p -m --first-parent` emits *both* parents' + /// patches concatenated, and without `-m` it emits nothing for a merge at + /// all. Either would show a file the merge did not touch on this side. + #[test] + fn a_merge_commit_yields_only_its_first_parent_patch() { + let Some(dir) = merge_repo("merge") else { + return; + }; + let host = crate::host::local::LocalHost::new(); + + assert_eq!(commit_files(&*host, &dir, "HEAD"), ["s.txt"]); + assert_eq!(commit_files(&*host, &dir, "HEAD^"), ["m.txt"]); + assert_eq!( + commit_files(&*host, &dir, "HEAD^^"), + ["base.txt"], + "the initial commit diffs against the empty tree, not an error" + ); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn the_three_working_tree_sources_split_staged_from_unstaged() { + let Some(dir) = merge_repo("sources") else { + return; + }; + let host = crate::host::local::LocalHost::new(); + std::fs::write(dir.join("base.txt"), "base\nstaged\n").unwrap(); + let _ = git::git_output(&dir, &["add", "base.txt"]); + std::fs::write(dir.join("m.txt"), "main\nunstaged\n").unwrap(); + std::fs::write(dir.join("fresh.txt"), "new\n").unwrap(); + + let files = |source: DiffSource| -> Vec { + let req = DiffRequest { + source, + ..Default::default() + }; + probe_diff(&*host, &dir, &req) + .expect("the scratch repo answers") + .files + .into_iter() + .map(|f| f.path) + .collect() + }; + assert_eq!(files(DiffSource::Staged), ["base.txt"]); + assert_eq!(files(DiffSource::Worktree), ["m.txt"]); + assert_eq!(files(DiffSource::Head), ["base.txt", "m.txt"]); + + let staged = probe_diff( + &*host, + &dir, + &DiffRequest { + source: DiffSource::Staged, + ..Default::default() + }, + ) + .unwrap(); + assert!( + staged.untracked.is_empty(), + "an untracked file is by definition not staged" + ); + assert_eq!(probe(&*host, &dir).unwrap().untracked, ["fresh.txt"]); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn a_non_ascii_path_survives_the_round_trip() { + let Some(dir) = merge_repo("utf8") else { + return; + }; + let host = crate::host::local::LocalHost::new(); + std::fs::write(dir.join("中文名.txt"), "one\n").unwrap(); + let _ = git::git_output(&dir, &["add", "-A"]); + + let snap = probe_diff( + &*host, + &dir, + &DiffRequest { + source: DiffSource::Staged, + ..Default::default() + }, + ) + .unwrap(); + assert_eq!( + snap.files + .iter() + .map(|f| f.path.as_str()) + .collect::>(), + ["中文名.txt"], + "octal escapes would have made this `344\\270\\255…`" + ); + + std::fs::write(dir.join("未跟踪.txt"), "two\n").unwrap(); + assert!( + probe(&*host, &dir) + .unwrap() + .untracked + .contains(&"未跟踪.txt".to_string()), + "ls-files quotes the same way, and is unquoted the same way" + ); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn a_pathspec_narrows_the_patch_and_drops_the_untracked_list() { + let Some(dir) = merge_repo("pathspec") else { + return; + }; + let host = crate::host::local::LocalHost::new(); + std::fs::write(dir.join("base.txt"), "base\nedit\n").unwrap(); + std::fs::write(dir.join("m.txt"), "main\nedit\n").unwrap(); + std::fs::write(dir.join("fresh.txt"), "new\n").unwrap(); + + let paths = [":(literal)m.txt".to_string()]; + let snap = probe_diff( + &*host, + &dir, + &DiffRequest { + source: DiffSource::Worktree, + paths: &paths, + ..Default::default() + }, + ) + .unwrap(); + assert_eq!( + snap.files + .iter() + .map(|f| f.path.as_str()) + .collect::>(), + ["m.txt"] + ); + assert!(snap.untracked.is_empty()); + assert!(!snap.read_failed); + assert_eq!(snap.source, DiffSource::Worktree); + + let _ = std::fs::remove_dir_all(&dir); + } + #[test] #[ignore = "measurement, not an assertion"] fn bench_stream_vs_buffer() { diff --git a/src/ui/diff_overlay.rs b/src/ui/diff_overlay.rs index da52eaa7..17bd7aa8 100644 --- a/src/ui/diff_overlay.rs +++ b/src/ui/diff_overlay.rs @@ -9,14 +9,21 @@ use gpui_component::button::Button; use gpui_component::{ActiveTheme as _, Icon, IconName, Sizable as _, h_flex, v_flex}; use crate::terminal::git_diff::{ - self, AUTO_COLLAPSE_LINES, DiffSnapshot, DiffStats, FileDiff, FileStatus, LineKind, + self, AUTO_COLLAPSE_LINES, DiffSnapshot, DiffSource, DiffStats, FileDiff, FileStatus, MAX_RENDERED_FILES, Truncation, }; use crate::ui::app::Tty7App; +use crate::ui::diff_rows::{Side, SplitCell, SplitRow, split_hunk}; use crate::ui::i18n::{L10nKey, t, t_fmt, t_plural}; use crate::ui::rounding; use crate::ui::rounding::RoundedCorners as _; +/// What the right panel's shared probe still asks git for, and therefore what +/// an overlay opened from the panel has to ask for too: the seed below is only +/// sound while the two agree. Both move together when the panel splits into +/// staged and unstaged groups. +const PANEL_DIFF_SOURCE: DiffSource = DiffSource::Head; + pub(crate) enum DiffLoad { Loading, Ready(Arc), @@ -26,11 +33,15 @@ pub(crate) enum DiffLoad { pub(crate) struct DiffOverlayState { pub(crate) host_id: crate::ui::host_ops::HostId, pub(crate) cwd: PathBuf, + /// Which patch this overlay is showing. Part of its identity, not a + /// setting: two sources over one directory are two different overlays. + pub(crate) source: DiffSource, pub(crate) focus_handle: FocusHandle, pub(crate) load: DiffLoad, pub(crate) loading: bool, pub(crate) expanded: HashMap, pub(crate) focus: Option, + pub(crate) scroll: gpui::ScrollHandle, } impl Tty7App { @@ -51,6 +62,18 @@ impl Tty7App { focus: Option, window: &mut Window, cx: &mut Context, + ) { + self.open_diff_overlay(host, cwd, PANEL_DIFF_SOURCE, focus, window, cx) + } + + pub(crate) fn open_diff_overlay( + &mut self, + host: crate::ui::host_ops::HostId, + cwd: PathBuf, + source: DiffSource, + focus: Option, + window: &mut Window, + cx: &mut Context, ) { let active = self.active; let was_front = self.tabs.get(active).is_some_and(|t| { @@ -59,11 +82,14 @@ impl Tty7App { if let Some(tab) = self.tabs.get_mut(active) { tab.overlay_top = crate::ui::app::OverlayTop::Diff; } + // The source belongs in this filter: an open worktree overlay reused + // for a staged file would only move the focus, and go on showing the + // unstaged patch under the staged file's name. match self .tabs .get_mut(active) .and_then(|t| t.diff_overlay.as_mut()) - .filter(|o| o.cwd == cwd && o.host_id == host) + .filter(|o| o.cwd == cwd && o.host_id == host && o.source == source) { Some(o) if o.focus == focus && was_front => { self.close_diff_overlay(window, cx); @@ -78,8 +104,12 @@ impl Tty7App { } None => {} } + // Skipping the "Reading…" flash is only allowed when the panel's + // snapshot answers the same question this overlay is asking. let seed = match (&self.right_panel.diff_cwd, &self.right_panel.diff) { - (Some(panel_key), Some(Some(snap))) if *panel_key == (host, cwd.clone()) => { + (Some(panel_key), Some(Some(snap))) + if source == PANEL_DIFF_SOURCE && *panel_key == (host, cwd.clone()) => + { DiffLoad::Ready(Arc::clone(snap)) } _ => DiffLoad::Loading, @@ -92,11 +122,13 @@ impl Tty7App { tab.diff_overlay = Some(DiffOverlayState { host_id: host, cwd, + source, focus_handle: focus_handle.clone(), load: seed, loading: false, expanded: HashMap::new(), focus, + scroll: gpui::ScrollHandle::new(), }); window.focus(&focus_handle, cx); self.spawn_diff_probe(cx); @@ -137,12 +169,13 @@ impl Tty7App { return; } let cwd = overlay.cwd.clone(); + let source = overlay.source.clone(); let id = overlay.host_id; let Some(host) = crate::ui::host_registry::HostRegistry::lookup(cx, id) else { return; }; overlay.loading = true; - self.spawn_shared_diff_probe(host, cwd, cx); + self.spawn_diff_probe_for(host, cwd, source, cx); } pub(crate) fn spawn_shared_diff_probe( @@ -151,22 +184,40 @@ impl Tty7App { cwd: PathBuf, cx: &mut Context, ) { - let key = (host.id(), cwd.clone()); + self.spawn_diff_probe_for(host, cwd, PANEL_DIFF_SOURCE, cx) + } + + pub(crate) fn spawn_diff_probe_for( + &mut self, + host: crate::ui::host_ops::SharedHost, + cwd: PathBuf, + source: DiffSource, + cx: &mut Context, + ) { + let key = probe_key(host.id(), &cwd, &source); if !self.diff_probes_inflight.insert(key.clone()) { self.diff_probes_restale.insert(key); return; } let host_for_retry = host.clone(); let probe_cwd = cwd.clone(); + let probe_source = source.clone(); crate::ui::host_ops::HostOps::run( host, cx, - move |h| git_diff::probe(h, &probe_cwd), + move |h| { + let req = git_diff::DiffRequest { + source: probe_source, + ..Default::default() + }; + git_diff::probe_diff(h, &probe_cwd, &req) + }, move |app, result, cx| { + let id = key.0; app.diff_probes_inflight.remove(&key); - app.install_diff_snapshot(key.0, &cwd, result.map(Arc::new), cx); - if app.diff_probes_restale.remove(&(key.0, cwd.clone())) { - app.spawn_shared_diff_probe(host_for_retry, cwd, cx); + app.install_diff_snapshot(id, &cwd, &source, result.map(Arc::new), cx); + if app.diff_probes_restale.remove(&key) { + app.spawn_diff_probe_for(host_for_retry, cwd, source, cx); } }, ); @@ -176,6 +227,7 @@ impl Tty7App { &mut self, host: crate::ui::host_ops::HostId, cwd: &Path, + source: &DiffSource, snap: Option>, cx: &mut Context, ) { @@ -184,7 +236,7 @@ impl Tty7App { let Some(overlay) = tab .diff_overlay .as_mut() - .filter(|o| o.cwd == cwd && o.host_id == host) + .filter(|o| o.cwd == cwd && o.host_id == host && o.source == *source) else { continue; }; @@ -195,6 +247,12 @@ impl Tty7App { }; landed = true; } + if *source != PANEL_DIFF_SOURCE { + if landed { + cx.notify(); + } + return; + } let key = (host, cwd.to_path_buf()); if self.right_panel.diff_pending.as_ref() == Some(&key) { self.right_panel.diff_pending = None; @@ -220,6 +278,13 @@ impl Tty7App { if overlay.loading { return; } + // The cached counts come from `git diff --numstat HEAD`, so only a + // HEAD snapshot is comparable to them. A staged or unstaged snapshot + // would differ the moment anything is staged, and re-probe forever; a + // commit or a range cannot go stale at all. + if overlay.source != DiffSource::Head { + return; + } let DiffLoad::Ready(snap) = &overlay.load else { return; }; @@ -250,9 +315,13 @@ impl Tty7App { DiffLoad::Ready(snap) if empty_snapshot(snap) => { self.diff_message(t(L10nKey::DiffWorkingTreeClean), cx) } - DiffLoad::Ready(snap) => { - self.diff_file_list(snap, &overlay.expanded, focused_file(snap, overlay), cx) - } + DiffLoad::Ready(snap) => self.diff_file_list( + snap, + &overlay.expanded, + focused_file(snap, overlay), + &overlay.scroll, + cx, + ), }; let header = self.diff_header(overlay, window, cx); @@ -442,6 +511,7 @@ impl Tty7App { snap: &DiffSnapshot, expanded: &HashMap, focused: Option, + scroll: &gpui::ScrollHandle, cx: &mut Context, ) -> AnyElement { let stats = snap.stats(); @@ -480,13 +550,17 @@ impl Tty7App { if focused.is_none() && !snap.untracked.is_empty() { list = list.child(self.diff_untracked_section(snap, cx)); } - div() - .id("diff-overlay-scroll") - .flex_1() - .min_h_0() - .overflow_y_scroll() - .child(list) - .into_any_element() + crate::ui::scrollbar::with_vertical_scrollbar( + "diff-overlay-scrollbar", + div() + .id("diff-overlay-scroll") + .flex_1() + .min_h_0() + .overflow_y_scroll() + .track_scroll(scroll) + .child(list), + scroll, + ) } fn diff_oversized_notice( @@ -527,6 +601,9 @@ impl Tty7App { FileStatus::Modified => ("M", cx.theme().warning), FileStatus::Deleted => ("D", cx.theme().danger), FileStatus::Renamed => ("R", cx.theme().muted_foreground), + FileStatus::Copied => ("C", cx.theme().muted_foreground), + FileStatus::TypeChanged => ("T", cx.theme().warning), + FileStatus::Unmerged => ("U", cx.theme().danger), }; let shown_path = match &file.old_path { Some(old) => format!("{old} → {}", file.path), @@ -863,76 +940,19 @@ fn oversized_summary(snap: &DiffSnapshot, stats: &DiffStats) -> String { parts.join(", ") } -#[derive(Clone, Copy)] -enum Side { - Old, - New, -} - -struct SplitCell { - no: Option, - text: String, - changed: bool, -} - -struct SplitRow { - left: Option, - right: Option, -} - -fn split_hunk(lines: &[git_diff::DiffLine]) -> Vec { - fn clean(text: &str) -> String { - text.replace('\t', " ") - } - fn flush( - rows: &mut Vec, - rem: &mut Vec<&git_diff::DiffLine>, - add: &mut Vec<&git_diff::DiffLine>, - ) { - for i in 0..rem.len().max(add.len()) { - rows.push(SplitRow { - left: rem.get(i).map(|l| SplitCell { - no: l.old_no, - text: clean(&l.text), - changed: true, - }), - right: add.get(i).map(|l| SplitCell { - no: l.new_no, - text: clean(&l.text), - changed: true, - }), - }); - } - rem.clear(); - add.clear(); - } - - let mut rows = Vec::new(); - let mut rem: Vec<&git_diff::DiffLine> = Vec::new(); - let mut add: Vec<&git_diff::DiffLine> = Vec::new(); - for line in lines { - match line.kind { - LineKind::Removed => rem.push(line), - LineKind::Added => add.push(line), - LineKind::Context => { - flush(&mut rows, &mut rem, &mut add); - rows.push(SplitRow { - left: Some(SplitCell { - no: line.old_no, - text: clean(&line.text), - changed: false, - }), - right: Some(SplitCell { - no: line.new_no, - text: clean(&line.text), - changed: false, - }), - }); - } - } - } - flush(&mut rows, &mut rem, &mut add); - rows +/// The de-duplication sets on `Tty7App` are keyed by `(HostId, PathBuf)`, so +/// the source rides along inside the path: two sources over one directory are +/// two independent probes and must not cancel one another. `Debug` is what +/// makes the tag unique — it carries the rev of a `Commit` and both ends of a +/// `Range` — and the separator is a byte no path contains. +fn probe_key( + host: crate::ui::host_ops::HostId, + cwd: &Path, + source: &DiffSource, +) -> (crate::ui::host_ops::HostId, PathBuf) { + let mut tagged = std::ffi::OsString::from(format!("{source:?}\u{1}")); + tagged.push(cwd.as_os_str()); + (host, PathBuf::from(tagged)) } #[cfg(test)] @@ -951,40 +971,22 @@ mod tests { } #[test] - fn pairs_removed_and_added_side_by_side() { - let lines = vec![ - line(LineKind::Context, Some(1), Some(1), "a"), - line(LineKind::Removed, Some(2), None, "b"), - line(LineKind::Removed, Some(3), None, "c"), - line(LineKind::Added, None, Some(2), "B"), - line(LineKind::Context, Some(4), Some(3), "d"), - ]; - let rows = split_hunk(&lines); - assert_eq!(rows.len(), 4); - - let l = rows[0].left.as_ref().unwrap(); - let r = rows[0].right.as_ref().unwrap(); - assert_eq!((l.no, l.text.as_str(), l.changed), (Some(1), "a", false)); - assert_eq!((r.no, r.text.as_str(), r.changed), (Some(1), "a", false)); - - let l = rows[1].left.as_ref().unwrap(); - let r = rows[1].right.as_ref().unwrap(); - assert_eq!((l.no, l.text.as_str(), l.changed), (Some(2), "b", true)); - assert_eq!((r.no, r.text.as_str(), r.changed), (Some(2), "B", true)); - - assert_eq!(rows[2].left.as_ref().unwrap().text, "c"); - assert!(rows[2].right.is_none()); - - assert_eq!(rows[3].left.as_ref().unwrap().no, Some(4)); - assert_eq!(rows[3].right.as_ref().unwrap().no, Some(3)); - } - - #[test] - fn expands_tabs_in_cell_text() { - let lines = vec![line(LineKind::Added, None, Some(1), "\tindented")]; - let rows = split_hunk(&lines); - assert_eq!(rows[0].right.as_ref().unwrap().text, " indented"); - assert!(rows[0].left.is_none()); + fn the_probe_key_separates_the_sources_over_one_directory() { + let host = crate::ui::host_ops::HostId::LOCAL; + let cwd = Path::new("/repo"); + let worktree = probe_key(host, cwd, &DiffSource::Worktree); + assert_ne!(worktree, probe_key(host, cwd, &DiffSource::Staged)); + assert_ne!(worktree, probe_key(host, cwd, &DiffSource::Head)); + assert_ne!( + probe_key(host, cwd, &DiffSource::Commit { rev: "a".into() }), + probe_key(host, cwd, &DiffSource::Commit { rev: "b".into() }), + "two commits are two probes" + ); + assert_eq!(worktree, probe_key(host, cwd, &DiffSource::Worktree)); + assert_ne!( + worktree, + probe_key(host, Path::new("/other"), &DiffSource::Worktree) + ); } fn small_file(path: &str, added: u32) -> FileDiff { @@ -1376,3 +1378,103 @@ mod tests { } } } + +#[cfg(all(test, unix))] +mod overlay_gpui_tests { + use super::*; + use crate::ui::app::test_window; + use crate::ui::host_ops::HostId; + use gpui::{Entity, TestAppContext, VisualTestContext}; + + fn overlay_source_and_load( + app: &Entity, + vcx: &mut VisualTestContext, + ) -> (DiffSource, bool) { + app.update_in(vcx, |app, _, _| { + let overlay = app.tabs[app.active] + .diff_overlay + .as_ref() + .expect("an overlay is open"); + ( + overlay.source.clone(), + matches!(overlay.load, DiffLoad::Loading), + ) + }) + } + + /// Opening a staged file while a worktree overlay is up must re-probe. The + /// filter used to match on `(cwd, host)` alone, so it took the "just move + /// the focus" branch and left the unstaged patch on screen under the + /// staged file's name. + #[gpui::test] + fn a_second_source_over_one_directory_is_a_second_overlay(cx: &mut TestAppContext) { + let (app, mut vcx, _pane) = test_window::harness_with_tabs(cx, 1); + let cwd = std::path::PathBuf::from("/no/such/tty7/repo"); + + app.update_in(&mut vcx, |app, window, cx| { + app.open_diff_overlay( + HostId::LOCAL, + cwd.clone(), + DiffSource::Worktree, + Some("a.rs".to_string()), + window, + cx, + ); + }); + assert_eq!( + overlay_source_and_load(&app, &mut vcx), + (DiffSource::Worktree, true) + ); + + // Pretend the worktree probe landed, so a reused overlay would show it. + app.update_in(&mut vcx, |app, _, _| { + let active = app.active; + let overlay = app.tabs[active].diff_overlay.as_mut().unwrap(); + overlay.loading = false; + overlay.load = DiffLoad::Ready(Arc::new(DiffSnapshot { + source: DiffSource::Worktree, + branch: "main".into(), + ..Default::default() + })); + }); + + app.update_in(&mut vcx, |app, window, cx| { + app.open_diff_overlay( + HostId::LOCAL, + cwd.clone(), + DiffSource::Staged, + Some("a.rs".to_string()), + window, + cx, + ); + }); + assert_eq!( + overlay_source_and_load(&app, &mut vcx), + (DiffSource::Staged, true), + "the same file from a different source is a different question" + ); + } + + /// The same source and the same focus still toggles the overlay shut. + #[gpui::test] + fn the_same_source_twice_still_closes(cx: &mut TestAppContext) { + let (app, mut vcx, _pane) = test_window::harness_with_tabs(cx, 1); + let cwd = std::path::PathBuf::from("/no/such/tty7/repo"); + + for _ in 0..2 { + app.update_in(&mut vcx, |app, window, cx| { + app.open_diff_overlay( + HostId::LOCAL, + cwd.clone(), + DiffSource::Worktree, + None, + window, + cx, + ); + }); + } + app.update_in(&mut vcx, |app, _, _| { + assert!(app.tabs[app.active].diff_overlay.is_none()); + }); + } +} diff --git a/src/ui/diff_rows.rs b/src/ui/diff_rows.rs index f93f9f10..8a10215b 100644 --- a/src/ui/diff_rows.rs +++ b/src/ui/diff_rows.rs @@ -3,3 +3,245 @@ //! Side-by-side and unified are two renderings of the same `Vec`, so //! the pairing logic lives here — outside either renderer — and is unit tested //! without a window. + +use crate::terminal::git_diff::{DiffLine, LineKind}; + +/// A tab is worth this many columns. Not configurable: a diff is read next to +/// the file's other lines, not on its own, and the grid has to line up. +const TAB_WIDTH: usize = 4; + +/// Diff text is laid out as a single run, so a literal tab would advance to the +/// renderer's idea of a tab stop rather than the file's. Both views expand +/// them the same way, or the two halves of a split row would drift apart. +pub(crate) fn expand_tabs(text: &str) -> String { + text.replace('\t', &" ".repeat(TAB_WIDTH)) +} + +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub(crate) enum Side { + Old, + New, +} + +pub(crate) struct SplitCell { + pub(crate) no: Option, + pub(crate) text: String, + pub(crate) changed: bool, +} + +pub(crate) struct SplitRow { + pub(crate) left: Option, + pub(crate) right: Option, +} + +/// Pairs each run of removals with the run of additions that follows it, so a +/// rewritten line sits opposite the line it replaced. Whichever run is shorter +/// leaves empty cells at the bottom of the pair. +pub(crate) fn split_hunk(lines: &[DiffLine]) -> Vec { + fn flush(rows: &mut Vec, rem: &mut Vec<&DiffLine>, add: &mut Vec<&DiffLine>) { + for i in 0..rem.len().max(add.len()) { + rows.push(SplitRow { + left: rem.get(i).map(|l| SplitCell { + no: l.old_no, + text: expand_tabs(&l.text), + changed: true, + }), + right: add.get(i).map(|l| SplitCell { + no: l.new_no, + text: expand_tabs(&l.text), + changed: true, + }), + }); + } + rem.clear(); + add.clear(); + } + + let mut rows = Vec::new(); + let mut rem: Vec<&DiffLine> = Vec::new(); + let mut add: Vec<&DiffLine> = Vec::new(); + for line in lines { + match line.kind { + LineKind::Removed => rem.push(line), + LineKind::Added => add.push(line), + LineKind::Context => { + flush(&mut rows, &mut rem, &mut add); + rows.push(SplitRow { + left: Some(SplitCell { + no: line.old_no, + text: expand_tabs(&line.text), + changed: false, + }), + right: Some(SplitCell { + no: line.new_no, + text: expand_tabs(&line.text), + changed: false, + }), + }); + } + } + } + flush(&mut rows, &mut rem, &mut add); + rows +} + +// Read by the unified renderer, which lands with the view toggle. The rows +// themselves belong here now so that both shapes of the same hunk are built — +// and tested — in one place rather than growing a second copy later. +#[allow(dead_code)] +pub(crate) struct UnifiedRow { + pub(crate) old: Option, + pub(crate) new: Option, + pub(crate) kind: LineKind, + pub(crate) text: String, +} + +/// One row per line, in git's own order — every removal in a run first, then +/// every addition. That is the opposite of [`split_hunk`], and it is the whole +/// difference between the two views: unified shows the patch as it was written, +/// side-by-side re-pairs it into before and after. +#[allow(dead_code)] +pub(crate) fn unified_rows(lines: &[DiffLine]) -> Vec { + lines + .iter() + .map(|line| UnifiedRow { + old: line.old_no, + new: line.new_no, + kind: line.kind, + text: expand_tabs(&line.text), + }) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn line(kind: LineKind, old: Option, new: Option, text: &str) -> DiffLine { + DiffLine { + kind, + old_no: old, + new_no: new, + text: text.to_string(), + } + } + + /// A context line, two removals, one addition, a context line — the shape + /// where the two views visibly disagree. + fn hunk() -> Vec { + vec![ + line(LineKind::Context, Some(1), Some(1), "a"), + line(LineKind::Removed, Some(2), None, "b"), + line(LineKind::Removed, Some(3), None, "c"), + line(LineKind::Added, None, Some(2), "B"), + line(LineKind::Context, Some(4), Some(3), "d"), + ] + } + + #[test] + fn pairs_removed_and_added_side_by_side() { + let rows = split_hunk(&hunk()); + assert_eq!(rows.len(), 4); + + let l = rows[0].left.as_ref().unwrap(); + let r = rows[0].right.as_ref().unwrap(); + assert_eq!((l.no, l.text.as_str(), l.changed), (Some(1), "a", false)); + assert_eq!((r.no, r.text.as_str(), r.changed), (Some(1), "a", false)); + + let l = rows[1].left.as_ref().unwrap(); + let r = rows[1].right.as_ref().unwrap(); + assert_eq!((l.no, l.text.as_str(), l.changed), (Some(2), "b", true)); + assert_eq!((r.no, r.text.as_str(), r.changed), (Some(2), "B", true)); + + assert_eq!(rows[2].left.as_ref().unwrap().text, "c"); + assert!(rows[2].right.is_none()); + + assert_eq!(rows[3].left.as_ref().unwrap().no, Some(4)); + assert_eq!(rows[3].right.as_ref().unwrap().no, Some(3)); + } + + #[test] + fn expands_tabs_in_cell_text() { + let lines = vec![line(LineKind::Added, None, Some(1), "\tindented")]; + let rows = split_hunk(&lines); + assert_eq!(rows[0].right.as_ref().unwrap().text, " indented"); + assert!(rows[0].left.is_none()); + } + + #[test] + fn expand_tabs_is_a_fixed_width_substitution() { + assert_eq!(expand_tabs("plain"), "plain"); + assert_eq!(expand_tabs("\tone"), " one"); + assert_eq!(expand_tabs("\t\ttwo"), " two"); + assert_eq!( + expand_tabs("a\tb"), + "a b", + "a fixed width, not the next tab stop — the diff has no column grid" + ); + assert_eq!(expand_tabs(""), ""); + } + + #[test] + fn unified_keeps_gits_own_order() { + let rows = unified_rows(&hunk()); + let shape: Vec<(Option, Option, LineKind, &str)> = rows + .iter() + .map(|r| (r.old, r.new, r.kind, r.text.as_str())) + .collect(); + assert_eq!( + shape, + [ + (Some(1), Some(1), LineKind::Context, "a"), + (Some(2), None, LineKind::Removed, "b"), + (Some(3), None, LineKind::Removed, "c"), + (None, Some(2), LineKind::Added, "B"), + (Some(4), Some(3), LineKind::Context, "d"), + ], + "both removals come before the addition, unlike the split view" + ); + } + + #[test] + fn unified_numbers_each_column_from_the_side_it_belongs_to() { + let rows = unified_rows(&hunk()); + assert!( + rows.iter() + .all(|r| (r.old.is_some() && r.new.is_some()) == (r.kind == LineKind::Context)), + "a context line is the only kind that exists on both sides" + ); + assert!( + rows.iter() + .filter(|r| r.kind == LineKind::Added) + .all(|r| r.old.is_none()) + ); + assert!( + rows.iter() + .filter(|r| r.kind == LineKind::Removed) + .all(|r| r.new.is_none()) + ); + } + + #[test] + fn unified_expands_tabs_the_same_way_split_does() { + let lines = vec![line(LineKind::Added, None, Some(1), "\tindented")]; + assert_eq!(unified_rows(&lines)[0].text, " indented"); + } + + #[test] + fn both_views_render_every_line_exactly_once() { + let lines = hunk(); + let unified = unified_rows(&lines); + assert_eq!(unified.len(), lines.len()); + + let cells: usize = split_hunk(&lines) + .iter() + .map(|r| r.left.is_some() as usize + r.right.is_some() as usize) + .sum(); + let context = lines.iter().filter(|l| l.kind == LineKind::Context).count(); + assert_eq!( + cells, + lines.len() + context, + "a context line fills two cells, a change fills one" + ); + } +}