From 58d7ef5838304580972a8cf51abb531cfc486dca Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 10 Aug 2026 12:31:58 +0800 Subject: [PATCH] fix(scm): close out the review's minor findings across the data and UI layers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second pass over the branch review: every remaining finding verified against the code, the real ones fixed. Data layer: - A truncated log parse is never called complete: RecordSplitter drops an overlong record whole and reports the count (delivered cut short, a commit body cut mid-way reads as the real message), parse_log carries a truncated flag past MAX_LOG_BYTES, and load_page only says "end of history" when the parse read everything git returned. - Every scope pins symbolic revs to shas before walking, so a commit landing between two pages can no longer shift where page two starts under Head and Refs scopes; unresolvable names read as "no history" rather than as a load failure. --parents was doing nothing and is gone; edge sort is stable so a merge's Outs keep first-parent order. - The lane model's central invariant now names the join case — a merge whose second parent already has a lane reserved sends its Out onto that lane, one line below the cut, not two — with a golden test for the commonest merge topology of all, which no golden covered. - DiffSource revs get the same could-be-an-option guard log already had; C-quoted paths decode the full escape set (a tab decoded to a literal t broke the :(literal) re-probe); rename from/to lines override the ambiguous diff --git header; combined-diff line numbers follow the sides rather than the colour, so a " +" line no longer drifts every number below it. - A rename's old path stays out of the per-file decoration map, where it outranked a file re-created at that path; ignored records decorate as Ignored, not Modified; checkout gains the trailing -- that keeps a stale name from falling back to a worktree-clobbering path checkout; unstage before the first commit takes -f (worktree- safe with --cached); batches split by bytes as well as count for Windows' 32K command line; a deadline expiry reports Timeout, not "git could not be run"; error details keep both streams. - probe_status distinguishes "not a repository" from "could not ask": a dropped link keeps the cached status (stale beats blank) and rests 10s instead of erasing the panel, while a definitive not-a-repo also drops the cwd→root mappings so the panel stops drawing Loading for a repository that is gone. Probe and watch work are wrapped against panics that would wedge their in-flight bookkeeping forever, watch landings check the wipe counter, superseded probes relaunch through the debounce, and a refused network slot says so instead of eating the click. UI: - Reset --hard confirms with its own words (commits fall off the branch), not the discard dialog's; a merge commit whose prefilled message the user cleared is committable again; the disabled commit button distinguishes "nothing to commit" from "write a message". - Selection highlight matches on the diff source too, so a file staged and edited again no longer lights both of its rows for one overlay. - The graph materializes only the rows in the viewport window (5000 flex children per frame was most of a frame), row clicks carry the page Arc and an index instead of a deep Commit clone per row per frame, filter results are cached per (page, query), and a selected merge ring's hole matches the selection band under it. - A failed commit_files read says the list could not be read instead of "0 files changed"; the STAGED chip and the graph's relative times go through the i18n table; the keys-awaiting-a-caller list is pruned to the seven that still are; the orphaned PanelUntracked key is gone; the zh commit placeholder reads naturally. 2398 tests, 0 failures. Known flake: daemon::singleton's second-claim test, untouched by this branch, fails ~1 in 3 full parallel runs and passes alone. --- crates/tty7-core/src/core/git/diff.rs | 252 +++++++++++++++++++---- crates/tty7-core/src/core/git/log.rs | 208 +++++++++++++++---- crates/tty7-core/src/core/git/mod.rs | 126 ++++++++++-- crates/tty7-core/src/core/git/ops.rs | 137 +++++++++--- crates/tty7-core/src/core/git/status.rs | 163 ++++++++++++--- crates/tty7-core/src/host/conformance.rs | 42 ++-- crates/tty7-core/src/host/local.rs | 5 +- src/terminal/git_data.rs | 149 +++++++++++--- src/ui/diff_overlay.rs | 22 +- src/ui/host_ops.rs | 8 + src/ui/i18n/en.rs | 19 +- src/ui/i18n/ja.rs | 20 +- src/ui/i18n/mod.rs | 80 ++----- src/ui/i18n/zh.rs | 21 +- src/ui/scm/actions.rs | 13 +- src/ui/scm/detail.rs | 16 +- src/ui/scm/graph.rs | 96 +++++++-- src/ui/scm/panel.rs | 34 ++- src/ui/scm/path.rs | 33 ++- src/ui/scm/state.rs | 35 ++++ 20 files changed, 1168 insertions(+), 311 deletions(-) diff --git a/crates/tty7-core/src/core/git/diff.rs b/crates/tty7-core/src/core/git/diff.rs index c5eb5e0c..953c7415 100644 --- a/crates/tty7-core/src/core/git/diff.rs +++ b/crates/tty7-core/src/core/git/diff.rs @@ -163,6 +163,23 @@ impl DiffSource { pub fn lists_untracked(&self) -> bool { matches!(self, DiffSource::Worktree | DiffSource::Head) } + + /// Whether every rev this source carries can be handed to git as an + /// argument. The same rule log's `is_rev` applies: a rev the caller made + /// up is still a rev git will be handed, and anything that could be read + /// as an option — `--output=…` most damningly — is refused before it + /// reaches an argv. Checked by [`probe_diff`], so no in-tree caller can + /// forget it. + fn revs_are_arguments(&self) -> bool { + let ok = |rev: &str| { + !rev.is_empty() && !rev.starts_with('-') && !rev.contains(|c: char| c.is_control()) + }; + match self { + DiffSource::Worktree | DiffSource::Staged | DiffSource::Head => true, + DiffSource::Commit { rev, .. } => ok(rev), + DiffSource::Range { base, head } => ok(base) && ok(head), + } + } } fn strings(args: &[&str]) -> Vec { @@ -358,6 +375,9 @@ pub fn probe(host: &dyn Host, cwd: &Path) -> Option { } pub fn probe_diff(host: &dyn Host, root: &Path, req: &DiffRequest<'_>) -> Option { + if !req.source.revs_are_arguments() { + return None; + } 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)?; @@ -483,12 +503,25 @@ impl DiffParser { file.status = FileStatus::Deleted; return; } - if line.starts_with("rename from ") { + // These four carry one unambiguous path each — unlike the `diff --git` + // header, where two unquoted paths containing ` b/` cannot be split + // reliably (git does not quote a path for a mere space). They land + // after the header, so what they say overrides what it guessed. + if let Some(old) = line.strip_prefix("rename from ") { file.status = FileStatus::Renamed; + file.old_path = Some(unquote_path(old)); return; } - if line.starts_with("copy from ") { + if let Some(old) = line.strip_prefix("copy from ") { file.status = FileStatus::Copied; + file.old_path = Some(unquote_path(old)); + return; + } + if let Some(new) = line + .strip_prefix("rename to ") + .or_else(|| line.strip_prefix("copy to ")) + { + file.path = unquote_path(new); return; } if let Some(mode) = line.strip_prefix("old mode ") { @@ -542,7 +575,7 @@ impl DiffParser { if !self.in_hunk { return; } - let Some((kind, text)) = split_body_line(line, self.markers) else { + let Some((kind, sides, text)) = split_body_line(line, self.markers) else { return; }; match kind { @@ -565,24 +598,19 @@ impl DiffParser { let Some(hunk) = file.hunks.last_mut() else { return; }; - let (o, n) = match kind { - LineKind::Added => { - let n = self.new_no; - self.new_no += 1; - (None, Some(n)) - } - LineKind::Removed => { - let o = self.old_no; - self.old_no += 1; - (Some(o), None) - } - LineKind::Context => { - let (o, n) = (self.old_no, self.new_no); - self.old_no += 1; - self.new_no += 1; - (Some(o), Some(n)) - } - }; + // Numbered by side, not by colour: in a combined diff a ` +` line is + // painted as an addition but *exists* in the first parent, and the + // old-side counter has to walk past it or every number below drifts. + let o = sides.in_old.then(|| { + let o = self.old_no; + self.old_no += 1; + o + }); + let n = sides.in_new.then(|| { + let n = self.new_no; + self.new_no += 1; + n + }); hunk.lines.push(DiffLine { kind, old_no: o, @@ -648,10 +676,19 @@ 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)> { +/// Splits a hunk body line into its kind, which sides it exists on, 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 the +/// *colour*, but the line numbers come from the sides: +/// +/// - the line is in the result iff no column says `-`; +/// - the line is in the first parent — the side tty7 numbers — iff its own +/// column says `-`, or says ` ` on a line that is in the result. (` ` on a +/// line outside the result is the other parent's removal; the first parent +/// never had it.) +/// +/// For an ordinary one-column diff this reduces to exactly `+`/`-`/context. +fn split_body_line(line: &str, markers: usize) -> Option<(LineKind, LineSides, &str)> { let head = line.get(..markers)?; let kind = if head.contains('+') { LineKind::Added @@ -662,7 +699,17 @@ fn split_body_line(line: &str, markers: usize) -> Option<(LineKind, &str)> { } else { return None; }; - Some((kind, &line[markers..])) + let in_new = !head.contains('-'); + let first = head.as_bytes().first().copied(); + let in_old = first == Some(b'-') || (first == Some(b' ') && in_new); + Some((kind, LineSides { in_old, in_new }, &line[markers..])) +} + +/// Which sides of the diff a body line exists on. See [`split_body_line`]. +#[derive(Clone, Copy)] +struct LineSides { + in_old: bool, + in_new: bool, } fn is_hunk_line(line: &str) -> bool { @@ -696,10 +743,15 @@ fn parse_quoted_pair(s: &str) -> Vec { continue; } match ch { - '\\' if in_quote => escaped = true, + // The backslash stays in `cur`: the scanner only needs to know the + // next `"` does not close the quote; `c_unescape` does the decode. + '\\' if in_quote => { + cur.push('\\'); + escaped = true; + } '"' => { if in_quote { - parts.push(std::mem::take(&mut cur)); + parts.push(c_unescape(&std::mem::take(&mut cur))); } in_quote = !in_quote; } @@ -710,6 +762,67 @@ fn parse_quoted_pair(s: &str) -> Vec { parts } +/// A single path as written after `rename from ` and friends: C-quoted when it +/// carries a character git always quotes (a control character or a `"` — +/// `core.quotePath=false` stops the quoting of non-ASCII only), bare +/// otherwise. +fn unquote_path(s: &str) -> String { + let s = s.trim_end_matches(['\n', '\r']); + match s + .strip_prefix('"') + .and_then(|inner| inner.strip_suffix('"')) + { + Some(inner) => c_unescape(inner), + None => s.to_string(), + } +} + +/// Decodes the C escapes git writes inside a quoted path — the full set, not +/// just `\"` and `\\`: a path with a real tab arrives as `\t`, and decoding it +/// to a literal `t` breaks the `:(literal)` re-probe for that row. Octal +/// escapes are *bytes* — a multi-byte character arrives as several — so the +/// value is assembled as bytes and read back as UTF-8 at the end. +fn c_unescape(s: &str) -> String { + let mut bytes: Vec = Vec::with_capacity(s.len()); + let mut push_char = |bytes: &mut Vec, ch: char| { + let mut buf = [0u8; 4]; + bytes.extend_from_slice(ch.encode_utf8(&mut buf).as_bytes()); + }; + let mut it = s.chars().peekable(); + while let Some(ch) = it.next() { + if ch != '\\' { + push_char(&mut bytes, ch); + continue; + } + match it.next() { + Some(digit @ '0'..='7') => { + let mut value = digit as u32 - '0' as u32; + for _ in 0..2 { + match it.peek() { + Some(&next @ '0'..='7') => { + value = value * 8 + (next as u32 - '0' as u32); + it.next(); + } + _ => break, + } + } + bytes.push(value as u8); + } + Some('n') => bytes.push(b'\n'), + Some('t') => bytes.push(b'\t'), + Some('r') => bytes.push(b'\r'), + Some('a') => bytes.push(0x07), + Some('b') => bytes.push(0x08), + Some('f') => bytes.push(0x0c), + Some('v') => bytes.push(0x0b), + // `\"`, `\\`, and anything git never writes: the character itself. + Some(other) => push_char(&mut bytes, other), + None => {} + } + } + String::from_utf8_lossy(&bytes).into_owned() +} + fn strip_prefix_ab(p: &str) -> String { p.strip_prefix("a/") .or_else(|| p.strip_prefix("b/")) @@ -922,9 +1035,9 @@ Binary files a/img.png and b/img.png differ #[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 + // Left on, every non-ASCII path arrives as C octal escapes — decodable + // (see below), but the raw spelling needs no decode at all. 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 [ @@ -941,21 +1054,74 @@ Binary files a/img.png and b/img.png differ } } + /// `core.quotePath=false` stops the quoting of non-ASCII only. A path + /// with a control character or a `"` is *always* C-quoted, so the decoder + /// has to speak the whole escape set — a tab decoded to a literal `t` + /// names a path that does not exist, and the `:(literal)` re-probe for + /// that row comes back empty. #[test] - fn octal_escaped_paths_are_what_the_flag_prevents() { + fn c_quoted_paths_decode_the_full_escape_set() { 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" - ); + assert_eq!(escaped[0].path, "中文名.txt"); + + let control = parse_unified("diff --git \"a/x\\ty.rs\" \"b/x\\ty.rs\"\n"); + assert_eq!(control[0].path, "x\ty.rs"); + + let quote = + parse_unified("diff --git \"a/he said \\\"hi\\\".md\" \"b/he said \\\"hi\\\".md\"\n"); + assert_eq!(quote[0].path, "he said \"hi\".md"); let raw = parse_unified("diff --git a/中文名.txt b/中文名.txt\n"); assert_eq!(raw[0].path, "中文名.txt"); } + /// git never quotes a path for a mere space, so a `diff --git` header + /// whose paths contain ` b/` cannot be split reliably — but the `rename + /// from`/`rename to` lines that follow name one path each, and they win. + #[test] + fn rename_lines_override_an_ambiguous_header() { + let files = parse_unified( + "diff --git a/my b/old.rs b/my b/new.rs\n\ + similarity index 90%\n\ + rename from my b/old.rs\n\ + rename to my b/new.rs\n", + ); + assert_eq!(files[0].path, "my b/new.rs"); + assert_eq!(files[0].old_path.as_deref(), Some("my b/old.rs")); + assert_eq!(files[0].status, FileStatus::Renamed); + } + + /// A rev that could be read as an option never reaches git — same guard + /// log's `is_rev` applies, on the module that calls itself the one place + /// to read what git is asked. + #[test] + fn a_rev_shaped_like_an_option_never_reaches_git() { + let host = crate::host::local::LocalHost::new(); + for source in [ + DiffSource::commit("--output=/tmp/pwned"), + DiffSource::Range { + base: "--output=/tmp/pwned".into(), + head: "main".into(), + }, + DiffSource::Range { + base: "main".into(), + head: "".into(), + }, + ] { + let req = DiffRequest { + source, + ..DiffRequest::default() + }; + assert!( + probe_diff(&*host, Path::new("/"), &req).is_none(), + "refused before any git runs" + ); + } + } + #[test] fn a_request_appends_its_pathspecs_after_a_separator() { let paths = [":(literal)src/a[b].rs".to_string()]; @@ -1168,6 +1334,20 @@ index af70335,f794161..0000000 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)); + + // Numbers follow the *sides*, not the colour: ` +MAIN` is painted as + // an addition but exists in the first parent (it is HEAD's own line), + // so the old counter walks past it — and `c` lands on old line 3, + // exactly the `-1,3` the hunk header promises. `+ SIDE` is the other + // parent's line: no old number. + assert_eq!( + (lines[2].old_no, lines[2].new_no), + (Some(2), Some(3)), + "MAIN: {:?}", + lines[2] + ); + assert_eq!((lines[4].old_no, lines[4].new_no), (None, Some(5))); + assert_eq!((lines[6].old_no, lines[6].new_no), (Some(3), Some(7))); } #[test] diff --git a/crates/tty7-core/src/core/git/log.rs b/crates/tty7-core/src/core/git/log.rs index 45d164e8..c34840c9 100644 --- a/crates/tty7-core/src/core/git/log.rs +++ b/crates/tty7-core/src/core/git/log.rs @@ -47,6 +47,14 @@ pub const MAX_LOG_BYTES: usize = 16 * 1024 * 1024; /// told apart by counting fields — and one NUL inside a commit message (git /// objects allow it) would desynchronise the whole stream. RS and US cannot /// occur in a sha, a refname, an ISO date or an address. +/// +/// A commit *message* can still carry RS or US — nothing git accepts is out of +/// bounds there. The failure is contained, not eliminated: the body truncates +/// at the stray separator, `is_hex_oid` throws the tail away unless it is +/// deliberately shaped like a full record, and a deliberately shaped one can +/// fabricate at worst a bogus row in the graph — whose `git show` then fails. +/// Sealing that needs length-prefixed reads (`cat-file --batch`), a different +/// data path entirely. pub const REC_SEP: u8 = 0x1e; pub const FIELD_SEP: u8 = 0x1f; @@ -326,7 +334,9 @@ impl LaneAlloc { // No parents: a root. `slots[node]` was released above and nothing // claimed it, so the lane simply ends here. - edges.sort_unstable_by_key(Edge::paint_rank); + // Stable: two edges of one rank (a merge's several `Out`s) keep their + // insertion order — first parent first — which the golden tests pin. + edges.sort_by_key(Edge::paint_rank); GraphRow { node, // Colour is the lane number, fixed when the lane is created and @@ -394,17 +404,28 @@ const LOG_FIELDS: usize = 11; pub const REF_FORMAT: &str = "--format=%(objectname)%x1f%(refname)%x1f%(refname:short)%x1f%(upstream)%x1f%(HEAD)%x1f%(objecttype)%x1f%(*objectname)"; +/// What [`parse_log`] read, and whether it read all of it. +pub struct ParsedLog { + pub commits: Vec, + /// The stream was cut short — by [`MAX_LOG_BYTES`], or by a record past + /// `MAX_RECORD` being dropped whole. The caller must not present the + /// commits as "all of history": git returned more than was parsed. + pub truncated: bool, +} + /// Parses the output of the `log` invocation [`LOG_PRETTY`] belongs to. /// /// Records are split on RS and fields on US. Fields are taken with `splitn`, so /// the body — the only field that can contain anything at all — absorbs every /// separator past the tenth instead of shifting the parse. -pub fn parse_log(stdout: &[u8]) -> Vec { +pub fn parse_log(stdout: &[u8]) -> ParsedLog { let mut commits = Vec::new(); let mut used = 0usize; + let mut clipped = false; let mut on_record = |record: &[u8]| { used = used.saturating_add(record.len()); if used > MAX_LOG_BYTES { + clipped = true; return; } if let Some(commit) = parse_record(record) { @@ -413,8 +434,11 @@ pub fn parse_log(stdout: &[u8]) -> Vec { }; let mut split = RecordSplitter::new(REC_SEP); split.push(stdout, &mut on_record); - split.finish(&mut on_record); - commits + let dropped = split.finish(&mut on_record); + ParsedLog { + commits, + truncated: clipped || dropped > 0, + } } fn parse_record(record: &[u8]) -> Option { @@ -646,7 +670,7 @@ pub fn load_commit(host: &dyn Host, root: &Path, rev: &str) -> Option { if !out.success() { return None; } - parse_log(&out.stdout).into_iter().next() + parse_log(&out.stdout).commits.into_iter().next() } /// One path a commit touched, with the line counts beside it. @@ -723,7 +747,10 @@ fn records(stdout: &[u8]) -> Vec { let mut on_record = |record: &[u8]| out.push(String::from_utf8_lossy(record).into_owned()); let mut split = RecordSplitter::new(0); split.push(stdout, &mut on_record); - split.finish(&mut on_record); + // A dropped record here is a >1 MiB *pathname* — losing that one row from + // a commit's file list is the same answer `MAX_COMMIT_FILES` already gives + // for lists that are merely long. + let _ = split.finish(&mut on_record); out } @@ -886,7 +913,6 @@ pub fn load_page( // of its children, and dates do not guarantee that. A rebase or a // cherry-pick across timezones is enough to invert a pair. "--topo-order", - "--parents", "--decorate=full", "--no-color", LOG_PRETTY, @@ -905,8 +931,13 @@ pub fn load_page( if !out.success() { return None; } - let mut commits = parse_log(&out.stdout); - let complete = commits.len() < count; + let parsed = parse_log(&out.stdout); + let mut commits = parsed.commits; + // "End of history" needs both halves: git answered with fewer than asked + // for, *and* the parse read everything git answered with. A stream cut at + // `MAX_LOG_BYTES` also has fewer commits than `count` — calling that + // complete would freeze paging on a truncated graph. + let complete = !parsed.truncated && commits.len() < count; let page: Vec<(Oid, SmallVec<[Oid; 2]>)> = commits .iter() @@ -950,27 +981,24 @@ pub fn load_page( /// The revs to walk for a scope. /// -/// `HeadAndUpstream` resolves to shas first. Paging re-runs the walk with a -/// larger `-n`, and a symbolic `HEAD` would let a commit pushed between the two -/// runs change where page two starts — the second page would no longer be a -/// superset of the first, which is the one thing paging here relies on. +/// Every symbolic name resolves to a sha first. Paging re-runs the walk with a +/// larger `-n`, and a symbolic `HEAD` or branch name would let a commit pushed +/// between the two runs change where page two starts — the second page would +/// no longer be a superset of the first, which is the one thing paging here +/// relies on. (`--all` cannot be pinned; that scope accepts the reflow.) +/// A name that no longer resolves — a deleted branch, an unborn HEAD — simply +/// contributes nothing, which reads as "no history" rather than as a failure. fn scope_revs(host: &dyn Host, root: &Path, scope: &GraphScope) -> Vec { match scope { - GraphScope::Head => vec!["HEAD".to_string()], + GraphScope::Head => rev(host, root, "HEAD^{commit}").into_iter().collect(), GraphScope::All => vec!["--all".to_string()], - GraphScope::Refs(refs) => { - let mut revs: Vec = refs - .iter() - // A refname cannot begin with `-`, so anything that does is - // either a mistake or an option smuggled in through a scope. - .filter(|r| !r.is_empty() && !r.starts_with('-')) - .cloned() - .collect(); - if revs.is_empty() { - revs.push("HEAD".to_string()); - } - revs - } + GraphScope::Refs(refs) => refs + .iter() + // A refname cannot begin with `-`, so anything that does is + // either a mistake or an option smuggled in through a scope. + .filter(|r| !r.is_empty() && !r.starts_with('-')) + .filter_map(|r| rev(host, root, &format!("{r}^{{commit}}"))) + .collect(), GraphScope::HeadAndUpstream => { let mut revs = Vec::new(); if let Some(head) = rev(host, root, "HEAD^{commit}") { @@ -1127,7 +1155,14 @@ mod tests { lanes } - /// Lanes crossing the row's bottom edge, sorted. + /// Lanes crossing the row's bottom edge, sorted and folded. + /// + /// Folded, because one lane can legally carry a `Pass` *and* an `Out`: a + /// merge whose second parent already has a lane reserved by another child + /// sends its `Out` onto that lane, joining the line rather than opening a + /// second one. Below the row that is a single line in a single colour (an + /// `Out`'s colour is its lane), so the cut sees one line — which the + /// assertions below verify before folding. fn bottom(row: &GraphRow) -> Vec { let mut lanes: Vec = row .edges @@ -1139,20 +1174,45 @@ mod tests { }) .collect(); lanes.sort_unstable(); + lanes.dedup(); lanes } /// The property the whole layout rests on: at any horizontal cut through - /// the graph a lane carries at most one line, and what leaves a row's - /// bottom is exactly what enters the next row's top. Together those two - /// mean colour-by-lane can never put two visible lines in one colour. + /// the graph a lane carries at most one visible line, and what leaves a + /// row's bottom is exactly what enters the next row's top. Together those + /// two mean colour-by-lane can never put two visible lines in one colour. + /// + /// "Visible" carries the one nuance: an `Out` may land on a lane a `Pass` + /// already crosses — a join, see [`bottom`] — and that pair is one line. + /// Two `Pass`es or two `Out`s on one lane are still bugs. fn assert_lanes_line_up(rows: &[GraphRow]) { for (i, row) in rows.iter().enumerate() { - for edges in [top(row), bottom(row)] { - let mut once = edges.clone(); - once.dedup(); - assert_eq!(once, edges, "row {i} has two lines on one lane: {row:?}"); - } + let once = |mut lanes: Vec| { + lanes.sort_unstable(); + let len = lanes.len(); + lanes.dedup(); + assert_eq!(lanes.len(), len, "row {i} doubles up a lane: {row:?}"); + }; + once(top(row)); + once( + row.edges + .iter() + .filter_map(|e| match *e { + Edge::Pass { lane, .. } => Some(lane), + _ => None, + }) + .collect(), + ); + once( + row.edges + .iter() + .filter_map(|e| match *e { + Edge::Out { to, .. } => Some(to), + _ => None, + }) + .collect(), + ); } for (i, pair) in rows.windows(2).enumerate() { assert_eq!( @@ -1330,6 +1390,37 @@ mod tests { assert_lanes_line_up(&rows); } + /// The commonest merge topology of all: "merge main into topic after main + /// advanced". The merge's second parent (`c`) already has a lane reserved + /// by another child (`x`), so the merge's `Out` *joins* that lane instead + /// of opening a second one to the same commit — the row legally carries a + /// `Pass` and an `Out` on lane 0, one line below the cut, not two. + #[test] + fn a_second_parent_joins_a_line_another_child_opened() { + let page = [ + commit("x", &["c"]), + commit("m", &["a", "c"]), + commit("a", &["c"]), + commit("c", &[]), + ]; + let rows = lay_out(&page); + + assert_eq!(rows[0].edges.as_slice(), [out_at(0)]); + let merge = &rows[1]; + assert_eq!(merge.node, 1, "the merge tips a lane of its own"); + assert_eq!( + merge.edges.as_slice(), + [pass_at(0), out_at(1), out_at(0)], + "first parent inherits the node's lane; the second joins lane 0" + ); + assert_eq!( + rows[3].edges.as_slice(), + [in_at(0), in_at(1)], + "both lines still converge on the shared parent" + ); + assert_lanes_line_up(&rows); + } + #[test] fn more_parents_than_lanes_truncates_instead_of_panicking() { let parents: Vec = (0..40).map(|i| format!("p{i}")).collect(); @@ -1374,6 +1465,35 @@ mod tests { ]) } + /// A parse that could not read everything must say so — `load_page` turns + /// `truncated` into `complete: false`, and a truncated graph that claimed + /// to be the end of history would freeze paging on it forever. + #[test] + fn a_stream_the_parse_cannot_finish_is_never_called_complete() { + // One record past MAX_RECORD: dropped whole by the splitter. + let huge_body = "x".repeat(super::super::MAX_RECORD + 1); + let stream = [ + one(SHA_A, SHA_B, "", "kept", ""), + one(SHA_B, "", "", "monster", &huge_body), + ] + .concat(); + let parsed = parse_log(stream.as_bytes()); + assert_eq!(parsed.commits.len(), 1, "the readable record survives"); + assert!(parsed.truncated); + + // Cumulative bytes past MAX_LOG_BYTES: the tail is clipped. + let body = "y".repeat(512 * 1024); + let stream: String = (0..40) + .map(|i| one(&format!("{i:040}"), "", "", "big", &body)) + .collect(); + let parsed = parse_log(stream.as_bytes()); + assert!(parsed.commits.len() < 40); + assert!(parsed.truncated); + + let parsed = parse_log(one(SHA_A, "", "", "small", "fine").as_bytes()); + assert!(!parsed.truncated, "an ordinary stream is read in full"); + } + #[test] fn a_multi_line_body_survives_the_record_split() { let stream = [ @@ -1387,7 +1507,7 @@ mod tests { one(SHA_B, "", "", "second", ""), ] .join("\n"); - let commits = parse_log(stream.as_bytes()); + let commits = parse_log(stream.as_bytes()).commits; assert_eq!(commits.len(), 2); assert_eq!(commits[0].summary, "first"); @@ -1402,7 +1522,7 @@ mod tests { #[test] fn a_merge_records_both_parents() { let stream = one(SHA_A, &format!("{SHA_B} {SHA_C}"), "", "merge", ""); - let commits = parse_log(stream.as_bytes()); + let commits = parse_log(stream.as_bytes()).commits; assert_eq!(commits[0].parents.as_slice(), [SHA_B, SHA_C]); assert!(commits[0].is_merge()); @@ -1413,7 +1533,7 @@ mod tests { fn decorations_map_to_their_ref_kinds() { let deco = "HEAD -> refs/heads/main, refs/remotes/origin/main, tag: refs/tags/v1.0"; let stream = one(SHA_A, "", deco, "subject", ""); - let refs = parse_log(stream.as_bytes()).remove(0).refs; + let refs = parse_log(stream.as_bytes()).commits.remove(0).refs; assert_eq!(refs.len(), 3); assert_eq!(refs[0].kind, RefKind::LocalBranch); @@ -1427,7 +1547,7 @@ mod tests { assert_eq!(refs[2].short, "v1.0"); let detached = one(SHA_A, "", "HEAD, refs/tags/v2", "subject", ""); - let refs = parse_log(detached.as_bytes()).remove(0).refs; + let refs = parse_log(detached.as_bytes()).commits.remove(0).refs; assert_eq!(refs[0].kind, RefKind::Head); assert!(refs[0].is_head); } @@ -1436,7 +1556,7 @@ mod tests { fn a_unit_separator_inside_a_body_does_not_shift_fields() { let body = "before\x1fafter\x1fand\x1fmore"; let stream = one(SHA_A, "", "", "subject", body); - let commits = parse_log(stream.as_bytes()); + let commits = parse_log(stream.as_bytes()).commits; assert_eq!( commits[0].summary, "subject", @@ -1453,7 +1573,7 @@ mod tests { record(&[SHA_B, "only two fields"]), ] .join("\n"); - let commits = parse_log(stream.as_bytes()); + let commits = parse_log(stream.as_bytes()).commits; assert_eq!(commits.len(), 1, "{commits:?}"); assert_eq!(commits[0].summary, "real"); @@ -1517,7 +1637,7 @@ mod tests { let subject = "提".repeat(400); let body = "交".repeat(4000); let stream = one(SHA_A, "", "", &subject, &body); - let commit = parse_log(stream.as_bytes()).remove(0); + let commit = parse_log(stream.as_bytes()).commits.remove(0); assert_eq!( commit.summary.len(), @@ -1573,7 +1693,7 @@ mod tests { assert_eq!(by_oid[SHA_C][0].upstream, None); // `%D` cannot carry an upstream at all, so a decoration parsed out of // a log record must not claim one. - let logged = parse_log(one(SHA_A, "", "refs/heads/main", "s", "").as_bytes()); + let logged = parse_log(one(SHA_A, "", "refs/heads/main", "s", "").as_bytes()).commits; assert_eq!(logged[0].refs[0].upstream, None); } diff --git a/crates/tty7-core/src/core/git/mod.rs b/crates/tty7-core/src/core/git/mod.rs index 5d792a2f..73536ea1 100644 --- a/crates/tty7-core/src/core/git/mod.rs +++ b/crates/tty7-core/src/core/git/mod.rs @@ -265,6 +265,9 @@ impl LineSplitter { pub struct RecordSplitter { sep: u8, tail: Vec, + /// The record being assembled overran [`MAX_RECORD`] and is now being + /// discarded up to its separator. + discarding: bool, dropped: usize, } @@ -275,6 +278,7 @@ impl RecordSplitter { RecordSplitter { sep, tail: Vec::new(), + discarding: false, dropped: 0, } } @@ -283,40 +287,63 @@ impl RecordSplitter { let mut rest = chunk; while let Some(at) = rest.iter().position(|b| *b == self.sep) { let (record, after) = rest.split_at(at); - if self.tail.is_empty() && self.dropped == 0 && record.len() <= MAX_RECORD { - on_record(record); + // An overlong record is dropped whole, never delivered cut short: + // a truncated record still parses — a commit body cut mid-way + // reads as the real message — and a wrong record is worse than a + // missing one the caller is told about. + if self.discarding { + self.discarding = false; + self.dropped += 1; + } else if self.tail.is_empty() { + // The common case — a whole record inside one chunk — is + // borrowed straight from the input, no copy. + if record.len() <= MAX_RECORD { + on_record(record); + } else { + self.dropped += 1; + } } else { self.keep(record); - let joined = std::mem::take(&mut self.tail); - self.dropped = 0; - on_record(&joined); + if self.discarding { + self.discarding = false; + self.dropped += 1; + } else { + let joined = std::mem::take(&mut self.tail); + on_record(&joined); + } } rest = &after[1..]; } self.keep(rest); } - /// Emits a trailing record only if one was actually started. Unlike lines, - /// well-formed `-z` output ends *with* a separator, so the common case here - /// is emitting nothing. - pub fn finish(mut self, mut on_record: impl FnMut(&[u8])) { - if !self.tail.is_empty() { + /// Emits a trailing record only if one was actually started — unlike + /// lines, well-formed `-z` output ends *with* a separator, so the common + /// case here is emitting nothing. Returns how many records were dropped + /// whole for overrunning [`MAX_RECORD`]; non-zero means the parse is + /// incomplete and the caller must not present it as the full answer. + #[must_use] + pub fn finish(mut self, mut on_record: impl FnMut(&[u8])) -> usize { + if self.discarding { + self.dropped += 1; + } else if !self.tail.is_empty() { let joined = std::mem::take(&mut self.tail); on_record(&joined); } - } - - /// How many bytes were discarded for overrunning [`MAX_RECORD`]. Non-zero - /// means the parse is incomplete and the caller should say so. - pub fn dropped(&self) -> usize { self.dropped } fn keep(&mut self, bytes: &[u8]) { - let room = MAX_RECORD.saturating_sub(self.tail.len()); - let take = room.min(bytes.len()); - self.tail.extend_from_slice(&bytes[..take]); - self.dropped += bytes.len() - take; + if self.discarding { + return; + } + if self.tail.len() + bytes.len() > MAX_RECORD { + // Free what was buffered too — nobody will ever see this record. + self.tail.clear(); + self.discarding = true; + return; + } + self.tail.extend_from_slice(bytes); } } @@ -476,6 +503,67 @@ mod tests { assert!(got[0].starts_with("caf"), "{:?}", got[0]); } + #[test] + fn record_splitter_rejoins_across_chunks_and_emits_a_trailing_record() { + let mut split = RecordSplitter::new(0); + let mut got = Vec::new(); + split.push(b"alpha\0be", |r| got.push(r.to_vec())); + assert_eq!(got, [b"alpha".to_vec()], "only the complete record so far"); + split.push(b"ta\0gamma", |r| got.push(r.to_vec())); + // Well-formed `-z` output ends with a separator; a trailing record + // without one still comes out at `finish`. + assert_eq!(split.finish(|r| got.push(r.to_vec())), 0); + assert_eq!( + got, + [b"alpha".to_vec(), b"beta".to_vec(), b"gamma".to_vec()] + ); + } + + /// An overlong record is dropped whole and *counted* — delivered cut + /// short it would still parse, and a commit body cut mid-way reads as the + /// real message. + #[test] + fn record_splitter_drops_an_absurd_record_whole_and_says_so() { + let mut split = RecordSplitter::new(0); + let mut got = Vec::new(); + let huge = vec![b'x'; MAX_RECORD + 5_000]; + split.push(b"before\0", |r| got.push(r.to_vec())); + for piece in huge.chunks(64 * 1024) { + split.push(piece, |r| got.push(r.to_vec())); + } + split.push(b"\0after\0", |r| got.push(r.to_vec())); + let dropped = split.finish(|r| got.push(r.to_vec())); + + assert_eq!(dropped, 1); + assert_eq!( + got, + [b"before".to_vec(), b"after".to_vec()], + "no truncated ghost between the two, and the next record survives" + ); + + // A single-chunk oversized record takes the borrow fast path and must + // be counted the same way. + let mut split = RecordSplitter::new(0); + let mut got: Vec> = Vec::new(); + let mut one = vec![b'y'; MAX_RECORD + 1]; + one.push(0); + one.extend_from_slice(b"tail\0"); + split.push(&one, |r| got.push(r.to_vec())); + assert_eq!(split.finish(|r| got.push(r.to_vec())), 1); + assert_eq!(got, [b"tail".to_vec()]); + } + + /// A stream that *ends* mid-way through an oversized record still reports + /// the drop. + #[test] + fn record_splitter_counts_a_truncated_trailing_record() { + let mut split = RecordSplitter::new(0); + let mut got: Vec> = Vec::new(); + split.push(&vec![b'z'; MAX_RECORD + 1], |r| got.push(r.to_vec())); + assert_eq!(split.finish(|r| got.push(r.to_vec())), 1); + assert!(got.is_empty()); + } + #[test] fn line_splitter_caps_one_absurd_line() { let mut split = LineSplitter::default(); diff --git a/crates/tty7-core/src/core/git/ops.rs b/crates/tty7-core/src/core/git/ops.rs index 26af6a7a..fe883838 100644 --- a/crates/tty7-core/src/core/git/ops.rs +++ b/crates/tty7-core/src/core/git/ops.rs @@ -19,6 +19,13 @@ use crate::host::Host; /// macOS), so a big stage is split into several calls. pub const MAX_PATHSPECS_PER_CALL: usize = 200; +/// …and only so many *bytes*. The binding limit is not macOS's 256 KiB but +/// Windows' `CreateProcess`, which caps the whole command line at 32,767 +/// UTF-16 units — 200 deep-tree paths at 200+ characters each sail past it. +/// Sized with room for the prefix and the per-argument quoting the Windows +/// join adds. +pub const MAX_PATHSPEC_BYTES_PER_CALL: usize = 24 * 1024; + /// Long enough for a push over a slow link. Only applied to network operations /// — the local path has no deadline at all. pub const GIT_NETWORK_DEADLINE: std::time::Duration = std::time::Duration::from_secs(600); @@ -202,10 +209,13 @@ impl GitOp { GitOp::DiscardWorktree { .. } => Destructive::LosesWorktreeEdits, GitOp::DiscardUntracked { .. } => Destructive::LosesUntrackedFiles, GitOp::DeleteBranch { .. } => Destructive::LosesCommits, + // The stronger of the two truths: a hard reset clobbers worktree + // edits *and* — pointed at an older commit — drops commits off + // the branch. The dialog has to warn about the worse one. GitOp::Reset { mode: ResetMode::Hard, .. - } => Destructive::LosesWorktreeEdits, + } => Destructive::LosesCommits, GitOp::Commit { amend: true, .. } => Destructive::RewritesHistory, GitOp::Push { force_with_lease: true, @@ -243,8 +253,12 @@ impl GitOp { /// 2.23 (2019), is still documented as EXPERIMENTAL, and has had its /// behaviour adjusted across releases; the two older forms have not moved /// in over a decade. tty7's whole point is that a remote host behaves like - /// the local one, and a dev box on CentOS 7 (git 1.8) is a real thing — - /// a version fork here would have to be tested twice forever. + /// the local one, and ancient dev boxes are real. (The honest floor is + /// git 1.8.5, not older: every path-carrying op spells its pathspecs + /// `:(literal)`, which is where that magic arrived — CentOS 7's 1.8.3 + /// fails those with a clean "Invalid pathspec magic" rather than doing + /// anything wrong.) A version fork here would have to be tested twice + /// forever. /// /// So there is no version probing at all. The one case that genuinely /// needs a different command is an unborn HEAD, and that needs no probe @@ -296,7 +310,12 @@ impl GitOp { } vec![out] } - GitOp::CheckoutBranch { name } => vec![argv(&["checkout", name])], + // The trailing `--` forces ref interpretation: without it a name + // that no longer resolves (deleted out from under a stale branch + // list) but matches a tracked *file* falls back to a path + // checkout — which silently discards worktree edits to that file, + // under an op whose `destructive()` says nothing is at risk. + GitOp::CheckoutBranch { name } => vec![argv(&["checkout", name, "--"])], GitOp::CheckoutDetached { rev } => vec![argv(&["checkout", "--detach", rev])], GitOp::CreateBranch { name, @@ -511,25 +530,46 @@ fn unstage_prefix(head: &HeadState) -> &'static [&'static str] { &["reset", "-q", "HEAD"] } else { // There is no HEAD to reset against before the first commit — git - // fails outright — so the index entry is dropped instead. - &["rm", "--cached", "-r", "-q"] + // fails outright — so the index entry is dropped instead. `-f`, + // because without it git refuses a file whose staged content differs + // from the file on disk (staged, then edited again) — and with + // `--cached` the worktree is never touched, so nothing is at risk. + &["rm", "--cached", "-r", "-q", "-f"] } } -/// `prefix -- `, split so no single argv can hit `E2BIG`. +/// `prefix -- `, split so no single argv can hit `E2BIG` on unix or +/// the 32,767-unit command-line cap on Windows — by count *and* by bytes, +/// whichever fills first. /// /// The `--` is not optional: without it a file named `HEAD` reads as a rev and /// one named `-f` reads as an option. fn batched(prefix: &[&str], specs: &[String]) -> Vec> { - specs - .chunks(MAX_PATHSPECS_PER_CALL) - .map(|chunk| { - let mut out = argv(prefix); - out.push("--".into()); - out.extend(chunk.iter().cloned()); - out - }) - .collect() + let mut out = Vec::new(); + let mut chunk: Vec = Vec::new(); + let mut bytes = 0usize; + let flush = |chunk: Vec, out: &mut Vec>| { + let mut call = argv(prefix); + call.push("--".into()); + call.extend(chunk); + out.push(call); + }; + for spec in specs { + // Room for the quotes and the space the Windows argv join adds. + let cost = spec.len() + 3; + if !chunk.is_empty() + && (chunk.len() >= MAX_PATHSPECS_PER_CALL || bytes + cost > MAX_PATHSPEC_BYTES_PER_CALL) + { + flush(std::mem::take(&mut chunk), &mut out); + bytes = 0; + } + bytes += cost; + chunk.push(spec.clone()); + } + if !chunk.is_empty() { + flush(chunk, &mut out); + } + out } /// What a failure means, from git's own words. @@ -652,7 +692,13 @@ pub fn run_op( let spawned = host.git_with_deadline(root, &borrowed, deadline); let out = spawned.map_err(|err| GitOpError { op: label, - kind: GitOpErrorKind::Spawn, + // A deadline expiry is its own kind: "git could not be run" tells + // the user to check their install, when the truth is the job was + // running — and on a remote host may still be. + kind: match err.kind() { + std::io::ErrorKind::TimedOut => GitOpErrorKind::Timeout, + _ => GitOpErrorKind::Spawn, + }, message: err.to_string(), detail: err.to_string(), rerun_argv: rerun(), @@ -679,7 +725,13 @@ pub fn run_op( op: label, kind, message, - detail: if stderr.is_empty() { stdout } else { stderr }, + // Both streams: a pull explains itself across the two, and + // showing only one buries half the reason. + detail: match (stderr.is_empty(), stdout.is_empty()) { + (false, false) => format!("{stderr}\n{stdout}"), + (false, true) => stderr, + _ => stdout, + }, rerun_argv: rerun(), cwd: root.to_path_buf(), }); @@ -865,11 +917,21 @@ mod tests { paths: vec![p("x")], } .commands(&unborn()), - vec![vec!["rm", "--cached", "-r", "-q", "--", ":(literal)x"]], + // `-f` because a staged-then-edited file otherwise refuses to + // unstage before the first commit; `--cached` keeps it worktree-safe. + vec![vec![ + "rm", + "--cached", + "-r", + "-q", + "-f", + "--", + ":(literal)x" + ]], ); assert_eq!( GitOp::UnstageAll.commands(&unborn()), - vec![vec!["rm", "--cached", "-r", "-q", "--", "."]], + vec![vec!["rm", "--cached", "-r", "-q", "-f", "--", "."]], ); } @@ -918,6 +980,28 @@ mod tests { assert_eq!(batches[1][2], ":(literal)f200.txt"); } + /// The count cap alone is not enough: 200 deep-tree paths at 200+ + /// characters each sail past Windows' 32,767-unit command line. Bytes + /// split a batch before the count does. + #[test] + fn long_paths_split_a_batch_by_bytes_before_the_count_cap() { + let long = "d/".repeat(150) + "file.rs"; // ~300 bytes each + let paths: Vec = (0..MAX_PATHSPECS_PER_CALL).map(|_| p(&long)).collect(); + let batches = GitOp::Stage { paths }.commands(&born()); + + assert!(batches.len() > 1, "200 × ~300B has to split"); + for batch in &batches { + let bytes: usize = batch[2..].iter().map(|s| s.len() + 3).sum(); + assert!( + bytes <= MAX_PATHSPEC_BYTES_PER_CALL, + "batch of {bytes} bytes would overflow a Windows command line" + ); + assert!(batch.len() >= 3, "no batch goes out empty"); + } + let total: usize = batches.iter().map(|b| b.len() - 2).sum(); + assert_eq!(total, MAX_PATHSPECS_PER_CALL, "every path is still sent"); + } + #[test] fn a_file_named_head_or_dash_f_is_never_read_as_a_rev_or_an_option() { for op in [ @@ -1029,7 +1113,9 @@ mod tests { name: "feature".into(), } .commands(&born()), - vec![vec!["checkout", "feature"]], + // The trailing `--` keeps a stale branch name from falling back + // to a worktree-clobbering *path* checkout. + vec![vec!["checkout", "feature", "--"]], ); assert_eq!( GitOp::CheckoutDetached { @@ -1210,10 +1296,13 @@ mod tests { for op in every_op() { for head in [born(), unborn()] { for batch in op.commands(&head) { + // The two sanctioned `-f`s: `clean` (that is the verb's + // whole meaning, and it is gated as destructive) and + // `rm --cached` (never touches the worktree). + let exempt = batch[0] == "clean" + || (batch[0] == "rm" && batch.iter().any(|a| a == "--cached")); assert!( - !batch - .iter() - .any(|a| a == "--force" || a == "-f" && batch[0] != "clean"), + !batch.iter().any(|a| a == "--force" || a == "-f" && !exempt), "{:?} would force: {batch:?}", op.label(), ); diff --git a/crates/tty7-core/src/core/git/status.rs b/crates/tty7-core/src/core/git/status.rs index b7854096..00999163 100644 --- a/crates/tty7-core/src/core/git/status.rs +++ b/crates/tty7-core/src/core/git/status.rs @@ -2,7 +2,8 @@ //! control panel, the file tree's decorations, and every button that is only //! enabled for some file states. //! -//! One `git status --porcelain=v2 --branch -z` answers all of it. That format +//! One `git status --porcelain=v2 --branch --show-stash -uall -z` answers all +//! of it. That format //! is the only one that carries the staged and unstaged halves *separately* //! (the `XY` pair), a rename's old path, unmerged stages, submodule sub-state, //! and the branch header — getting the same picture out of `git diff` takes @@ -222,6 +223,12 @@ impl StatusEntry { if self.is_untracked() { return DecoStatus::Untracked; } + // Explicit, not via the code match below: an ignored record carries no + // change codes, so it would otherwise fall through to `Modified` the + // day `--ignored` is passed — and light the whole ignored tree up. + if matches!(self.kind, EntryKind::Ignored) { + return DecoStatus::Ignored; + } let worse = if code_rank(self.worktree) >= code_rank(self.index) { self.worktree } else { @@ -407,11 +414,13 @@ impl StatusIndex { for entry in &status.entries { let deco = entry.deco(); index.insert(entry.path.as_str(), deco); - // A rename's old path is no longer on disk, so no tree row will ask - // for it — but the directory it left did lose a file, and the - // rollup is the only place that can say so. + // A rename's old path goes into the directory rollup only — the + // directory it left did lose a file. Not into `files`: the path + // can be occupied again (`git mv a b && echo x > a` emits an + // untracked record for `a`), and that row belongs to whatever + // occupies it now, not to the rename it outranks. if let Some(orig) = &entry.orig_path { - index.insert(orig.as_str(), deco); + index.rollup(orig.as_str(), deco); } } if index.files.len() > MAX_DECORATED_FILES { @@ -439,6 +448,12 @@ impl StatusIndex { .entry(repo_rel.to_string()) .and_modify(|slot| *slot = (*slot).max(status)) .or_insert(status); + self.rollup(repo_rel, status); + } + + /// Only the ancestor walk — for a path that must not claim a file row of + /// its own, like the old half of a rename. + fn rollup(&mut self, repo_rel: &str, status: DecoStatus) { let mut cut = repo_rel; while let Some((parent, _)) = cut.rsplit_once('/') { self.dirs @@ -531,8 +546,12 @@ pub fn parse_porcelain_v2(stdout: &[u8]) -> ParsedStatus { let mut parser = Parser::default(); let mut split = RecordSplitter::new(0); split.push(stdout, |record| parser.record(record)); - split.finish(|record| parser.record(record)); - parser.finish() + let dropped = split.finish(|record| parser.record(record)); + let mut parsed = parser.finish(); + // A record past `MAX_RECORD` (a pathological pathname) is dropped whole; + // the status must say it is not the full picture, same as the entry cap. + parsed.truncated |= dropped > 0; + parsed } #[derive(Default)] @@ -793,15 +812,28 @@ fn head_state(oid: Option, head_name: Option) -> HeadState { } } -/// The whole working tree state for the repository containing `cwd`, or `None` -/// if there is no repository there. +/// What a status probe learned. The middle answer is the load-bearing one: +/// `Unreachable` is not an answer *about the repository* — the question could +/// not be asked — and treating it as "no repository here" made a dropped link +/// erase a panel that was showing perfectly good (if stale) data. +#[derive(Clone, PartialEq, Debug)] +pub enum StatusProbe { + Status(Box), + /// git ran and said so — an ordinary directory. + NotARepo, + /// git could not run, or ran and failed for a reason that is not "no + /// repository": a dead link, a timeout, an `index.lock` held by someone + /// else. Keep what is cached and ask again later. + Unreachable, +} + +/// The whole working tree state for the repository containing `cwd`. /// /// Three round trips in the common case — `rev-parse`, `status`, `read_dir` — /// and each one is an RPC on a remote workspace, which is why none of them is /// split into the several calls that would read more naturally. -pub fn probe_status(host: &dyn Host, cwd: &Path) -> Option { - let paths = super::git( - host, +pub fn probe_status(host: &dyn Host, cwd: &Path) -> StatusProbe { + let Ok(out) = host.git( cwd, &[ "rev-parse", @@ -810,16 +842,36 @@ pub fn probe_status(host: &dyn Host, cwd: &Path) -> Option { "--git-dir", "--git-common-dir", ], - )?; + ) else { + return StatusProbe::Unreachable; + }; + if !out.success() { + // Exit 128 with this phrase is the ordinary answer for an ordinary + // directory; the phrase has been stable (modulo case) since git 1.x. + // Anything else is a repository that could not be read. + let stderr = String::from_utf8_lossy(&out.stderr).to_ascii_lowercase(); + return if stderr.contains("not a git repository") { + StatusProbe::NotARepo + } else { + StatusProbe::Unreachable + }; + } + let paths = String::from_utf8_lossy(&out.stdout).into_owned(); let mut lines = paths.lines().map(|l| l.trim_end_matches(['\n', '\r'])); - let root = PathBuf::from(lines.next()?); + let Some(root) = lines.next().map(PathBuf::from) else { + return StatusProbe::Unreachable; + }; let git_dir = lines.next(); let home = super::repo_home(&root, git_dir, lines.next()); - let git_dir = PathBuf::from(git_dir?); + let Some(git_dir) = git_dir.map(PathBuf::from) else { + return StatusProbe::Unreachable; + }; - let out = host.git(cwd, STATUS_ARGS).ok()?; + let Ok(out) = host.git(cwd, STATUS_ARGS) else { + return StatusProbe::Unreachable; + }; if !out.success() { - return None; + return StatusProbe::Unreachable; } let mut parsed = parse_porcelain_v2(&out.stdout); if parsed.ahead_behind.is_none() @@ -832,7 +884,12 @@ pub fn probe_status(host: &dyn Host, cwd: &Path) -> Option { let operation = detect_operation(host, &git_dir, &listing); let prefilled_message = operation.and_then(|_| read_prefilled_message(host, &git_dir, &listing)); - Some(parsed.into_status(root, home, operation, prefilled_message)) + StatusProbe::Status(Box::new(parsed.into_status( + root, + home, + operation, + prefilled_message, + ))) } /// Ask for ahead/behind again when the header could not say. @@ -1436,6 +1493,48 @@ mod tests { index.dir("old").unwrap().changed, "the directory it left lost a file" ); + assert_eq!( + index.file("old/home.rs"), + None, + "the old path holds no file row of its own" + ); + } + + /// `git mv a b && echo x > a`: the rename record's old path and a fresh + /// untracked file share a spelling. The row on disk is the untracked file; + /// the rename must not outrank it just because `Renamed > Untracked`. + #[test] + fn a_file_recreated_at_a_renames_old_path_decorates_as_itself() { + let status = status_of( + &[ + head_records(), + rec(&[ + "2 R. N... 100644 100644 100644 ", + SHA, + " ", + SHA, + " R090 b.rs", + ]), + rec(&["a.rs"]), + rec(&["? a.rs"]), + ] + .concat(), + ); + let index = StatusIndex::build(&status); + + assert_eq!(index.file("a.rs"), Some(DecoStatus::Untracked)); + assert_eq!(index.file("b.rs"), Some(DecoStatus::Renamed)); + } + + /// `--ignored` is not passed today; the parser is future-proofed for it, + /// and the decoration must be too — an ignored record carries no change + /// codes and used to fall through to `Modified`. + #[test] + fn an_ignored_record_decorates_as_ignored_not_modified() { + let parsed = parse_porcelain_v2(&[head_records(), rec(&["! target"])].concat()); + let entry = &parsed.entries[0]; + assert_eq!(entry.kind, EntryKind::Ignored); + assert_eq!(entry.deco(), DecoStatus::Ignored); } #[test] @@ -1465,6 +1564,14 @@ mod tests { } } + /// A probe that must have found a repository, unwrapped with a reason. + fn probed(probe: StatusProbe, why: &str) -> WorkingTreeStatus { + match probe { + StatusProbe::Status(status) => *status, + other => panic!("{why}: {other:?}"), + } + } + fn scratch(name: &str) -> Option { // The pid keeps two concurrent `cargo test` runs off each other's // fixture, since the directory is wiped on the way in — the same @@ -1524,7 +1631,10 @@ mod tests { assert!(run(&*host, repo, &["mv", "moved.txt", "renamed.txt"])); std::fs::write(repo.join("untracked.txt"), "loose\n").unwrap(); - let status = probe_status(&*host, repo).expect("a repository was just created here"); + let status = probed( + probe_status(&*host, repo), + "a repository was just created here", + ); match &status.head { HeadState::Branch { name, oid } => { @@ -1606,7 +1716,7 @@ mod tests { // Expected to fail — that is the point. run(&*host, repo, &["merge", "other"]); - let status = probe_status(&*host, repo).expect("still a repository mid-merge"); + let status = probed(probe_status(&*host, repo), "still a repository mid-merge"); assert_eq!(status.operation, Some(RepoOperation::Merge)); assert!( status @@ -1657,7 +1767,7 @@ mod tests { std::fs::write(repo.join("f.txt"), "two\n").unwrap(); assert!(run(&*host, &repo, &["commit", "--quiet", "-am", "two"])); - let status = probe_status(&*host, &repo).expect("a repository with a remote"); + let status = probed(probe_status(&*host, &repo), "a repository with a remote"); assert_eq!(status.upstream.as_deref(), Some("origin/main")); // Straight from `# branch.ab`; the `rev-list` fallback never runs here. assert_eq!( @@ -1668,13 +1778,20 @@ mod tests { assert!(status.is_clean()); } + /// The two negative answers stay distinct: an ordinary directory is + /// `NotARepo` (record it, stop asking), a directory git could not even be + /// run in is `Unreachable` (keep what is cached, ask again later). #[test] fn outside_a_repository_there_is_no_status() { let host = crate::host::local::LocalHost::new(); let Some(scratch) = scratch("not-a-repo") else { return; }; - assert_eq!(probe_status(&*host, &scratch.0), None); - assert_eq!(probe_status(&*host, Path::new("/no/such/tty7/path")), None); + assert_eq!(probe_status(&*host, &scratch.0), StatusProbe::NotARepo); + assert_eq!( + probe_status(&*host, Path::new("/no/such/tty7/path")), + StatusProbe::Unreachable, + "a cwd that cannot be entered is not an answer about a repository" + ); } } diff --git a/crates/tty7-core/src/host/conformance.rs b/crates/tty7-core/src/host/conformance.rs index cc76fe8e..0328ae66 100644 --- a/crates/tty7-core/src/host/conformance.rs +++ b/crates/tty7-core/src/host/conformance.rs @@ -669,24 +669,32 @@ pub fn git_terminal_prompt_is_disabled(h: &dyn Host, sb: &dyn Sandbox) { mkdir(h, &repo); let Some(()) = git_repo(h, &repo) else { return }; - let configured = h.git( - &repo, - &[ - "config", - "alias.tty7prompt", - "!echo PROMPT=[$GIT_TERMINAL_PROMPT] REQUIRE=[$SSH_ASKPASS_REQUIRE]", - ], + // Failures past this point assert rather than return: a broken alias or a + // failing run is exactly a git that misbehaved, and returning would make + // this case pass vacuously in precisely that situation. + let out = h + .git( + &repo, + &[ + "config", + "alias.tty7prompt", + "!echo PROMPT=[$GIT_TERMINAL_PROMPT] REQUIRE=[$SSH_ASKPASS_REQUIRE]", + ], + ) + .unwrap(); + assert!( + out.success(), + "config exited {:?}: {:?}", + out.status, + out.stderr_trimmed() + ); + let out = h.git(&repo, &["tty7prompt"]).unwrap(); + assert!( + out.success(), + "alias run exited {:?}: {:?}", + out.status, + out.stderr_trimmed() ); - let Ok(out) = configured else { return }; - if !out.success() { - return; - } - let Ok(out) = h.git(&repo, &["tty7prompt"]) else { - return; - }; - if !out.success() { - return; - } // A `push` that stops to ask for a username never comes back — and on the // far side of a control link there is no terminal to answer at anyway. The // remote host inherits this from the server's own local host, so both ends diff --git a/crates/tty7-core/src/host/local.rs b/crates/tty7-core/src/host/local.rs index 3564342e..be391fa7 100644 --- a/crates/tty7-core/src/host/local.rs +++ b/crates/tty7-core/src/host/local.rs @@ -50,7 +50,10 @@ const NO_PROMPT_ENV: &[(&str, Option<&str>)] = &[ /// call, and this is the read path too. fn no_prompt_env() -> Vec<(&'static str, Option<&'static str>)> { let mut env = NO_PROMPT_ENV.to_vec(); - if std::env::var_os("GIT_SSH_COMMAND").is_none() { + // `GIT_SSH` too: it is the older spelling of the same choice (plink on + // Windows, most commonly), and `GIT_SSH_COMMAND` outranks it — forcing + // ours would silently swap their transport out. + if std::env::var_os("GIT_SSH_COMMAND").is_none() && std::env::var_os("GIT_SSH").is_none() { env.push(("GIT_SSH_COMMAND", Some("ssh -o BatchMode=yes"))); } env diff --git a/src/terminal/git_data.rs b/src/terminal/git_data.rs index e9d1ad59..3b7e8793 100644 --- a/src/terminal/git_data.rs +++ b/src/terminal/git_data.rs @@ -12,13 +12,6 @@ //! entries a `git add` touched is a losing game; bumping a counter for the //! repository and letting readers notice they are behind is not. -// The watcher and the subscription gate now use this module, but the panel and -// the file tree — the things that read the status and run the writes — are -// still landing alongside it, so `status_of`, `index_of`, `run_git_op`, -// `shell_quote` and the `FileTree`/`Editor` subscribers have no callers yet. -// Take the allow off with the last of them; anything still unused then is. -#![allow(dead_code)] - use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -28,7 +21,7 @@ use std::time::{Duration, Instant}; use gpui::{Context, Window}; use crate::core::git::ops::{GitOp, GitOpError, GitOpErrorKind, GitOpOutcome, run_op}; -use crate::core::git::status::{StatusIndex, WorkingTreeStatus, probe_status}; +use crate::core::git::status::{StatusIndex, StatusProbe, WorkingTreeStatus, probe_status}; use crate::ui::app::Tty7App; use crate::ui::host_ops::{ByHost, Host, HostId, HostOps, InFlight, SharedHost, WatchSub}; @@ -44,6 +37,12 @@ use crate::ui::host_ops::{ByHost, Host, HostId, HostOps, InFlight, SharedHost, W /// (one repository on screen) keeps theoretical. pub const MAX_CONCURRENT_NETWORK_OPS: usize = 2; +/// How long a probe that could not reach its host rests before it is asked +/// again. Only the render-driven retry waits this out — any real invalidation +/// (a watcher event, a write, the Refresh button) bumps the epoch, which +/// clears the rest and retries at once. +pub const PROBE_FAILURE_RETRY: Duration = Duration::from_secs(10); + /// How long a failed watch open rests before it is tried again. /// /// Without this the retry runs at frame rate: a failed open leaves the @@ -307,6 +306,14 @@ impl GitSubscriptions { } } +/// What a probe hands back to the UI thread: [`StatusProbe`], with the +/// decoration index pre-built off-thread and the panic case folded in. +enum ProbeLanding { + Status(Arc, Arc), + NotARepo, + Unreachable, +} + /// One repository's `.git` watch and the burst it is feeding. #[derive(Default)] struct RepoWatch { @@ -331,6 +338,11 @@ pub struct ScmData { epoch: ByHost, /// repo root → the epoch the cached status was read at. read_at: ByHost, + /// repo root → when a probe last came back *unreachable* — not "not a + /// repository", but "the question could not be asked". The held status + /// stays (stale beats blank), and `is_stale` sits out + /// [`PROBE_FAILURE_RETRY`] so a dead link is not probed at frame rate. + failed_at: ByHost, probes: InFlight<(HostId, PathBuf)>, network: ByHost>, /// repo root → its `.git` watch, once someone is looking. A plain map @@ -380,19 +392,28 @@ impl ScmData { } /// Whether what we hold was read before the last thing that changed it. - /// A repository we have never probed counts as stale. + /// A repository we have never probed counts as stale — unless the last + /// attempt could not reach the host and its rest has not passed yet. pub fn is_stale(&self, host: HostId, root: &Path) -> bool { - match self.read_at.get(host, root) { + let stale = match self.read_at.get(host, root) { Some(read) => *read < self.epoch(host, root), None => true, - } + }; + stale + && !self + .failed_at + .get(host, root) + .is_some_and(|at| at.elapsed() < PROBE_FAILURE_RETRY) } /// Mark a repository changed. Every write, every `.git` watcher event and /// every command boundary lands here; readers reprobe on their next look. + /// A real change also ends a failure's rest: whatever made the epoch move + /// is evidence the host is alive again. pub fn bump(&mut self, host: HostId, root: &Path) { let next = self.epoch(host, root) + 1; self.epoch.insert(host, root.to_path_buf(), next); + self.failed_at.remove(host, root); self.probes.invalidate(&(host, root.to_path_buf())); } @@ -403,9 +424,14 @@ impl ScmData { self.index.clear_host(host); self.epoch.clear_host(host); self.read_at.clear_host(host); + self.failed_at.clear_host(host); self.network.clear_host(host); self.watches.retain(|(held, _), _| *held != host); self.subs.clear_host(host); + // In-flight probe bookkeeping too: a probe whose landing never runs + // (its work panicked, say) would otherwise hold `begin` false for + // this key for the life of the process. + self.probes.retain(|(held, _)| *held != host); self.wipe += 1; } @@ -571,14 +597,24 @@ impl Tty7App { let probe_root = root.clone(); let this = cx.weak_entity(); - let again = host.clone(); HostOps::run_detached( host, cx, move |h| { - let status = probe_status(h, &probe_root)?; - let index = StatusIndex::build(&status); - Some((Arc::new(status), Arc::new(index))) + // `catch_unwind` because a panic on the pool thread would skip + // the landing entirely — and with it `probes.finish`, wedging + // this repository's refresh for the life of the process. + let probe = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + probe_status(h, &probe_root) + })); + match probe { + Ok(StatusProbe::Status(status)) => { + let index = StatusIndex::build(&status); + ProbeLanding::Status(Arc::new(*status), Arc::new(index)) + } + Ok(StatusProbe::NotARepo) => ProbeLanding::NotARepo, + Ok(StatusProbe::Unreachable) | Err(_) => ProbeLanding::Unreachable, + } }, move |cx, result| { let data = cx.default_global::(); @@ -591,8 +627,13 @@ impl Tty7App { if data.wipe != wipe || data.generation(id, &root) != sub_gen { return; } + // Only a definitive answer counts as a read; an unreachable + // host leaves what is cached (stale beats blank) and rests + // before the next try — see `is_stale`. + let definitive = !matches!(result, ProbeLanding::Unreachable); + let mut not_a_repo = false; let changed = match result { - Some((status, index)) => { + ProbeLanding::Status(status, index) => { let same = data .status .get(id, root.as_path()) @@ -606,13 +647,27 @@ impl Tty7App { // is what stops the next frame asking again: a pane whose // cwd is an ordinary directory would otherwise spawn a // `rev-parse` per frame, forever. - None => { + ProbeLanding::NotARepo => { + not_a_repo = true; let held = data.status.remove(id, root.as_path()).is_some(); data.index.remove(id, root.as_path()); held } + ProbeLanding::Unreachable => { + data.failed_at.insert(id, root.clone(), Instant::now()); + false + } }; - data.read_at.insert(id, root.clone(), at); + if definitive { + data.failed_at.remove(id, root.as_path()); + data.read_at.insert(id, root.clone(), at); + } + if not_a_repo { + // Every cwd that resolved to this root must re-ask, or the + // panel keeps drawing the Loading state of a repository + // that is gone (`rm -rf .git` being the honest test). + let _ = this.update(cx, |app, _| app.scm.forget_root(id, &root)); + } // `run_detached` lands with an `App` and no view, and writing // a global marks nothing dirty, so without this the panel and // the decorations wait for the next unrelated repaint. @@ -627,7 +682,13 @@ impl Tty7App { cx.refresh_windows(); } if superseded { - let _ = this.update(cx, |app, cx| app.scm_refresh(again, root, cx)); + // Through the debounce, not straight back into a probe: + // during sustained churn on a repository whose status read + // outlives the event interval, a direct relaunch runs + // probes back to back for the whole of it. The bump-and- + // wait path coalesces the retry with whatever is still + // landing. + let _ = this.update(cx, |app, cx| app.scm_invalidate(id, &root, cx)); } }, ); @@ -832,6 +893,9 @@ impl Tty7App { cx.default_global::().clear_host(host); cx.default_global::() .clear_host(host); + // The panel's own per-cwd caches too — `roots` grows one entry + // per directory ever visited on the dead link otherwise. + self.scm.forget_host(host); } } @@ -846,22 +910,29 @@ impl Tty7App { return; } let sub_gen = cx.default_global::().generation(id, &root); + let wipe = cx.default_global::().wipe; let probe_root = root.clone(); HostOps::run( host, cx, move |h| { - let dirs = scm_watch_dirs(h, &probe_root)?; - match h.watch(&dirs) { - Ok(sub) => Some(Arc::new(sub)), - Err(e) => { - log::warn!("source control: no watch for {probe_root:?}: {e}"); - None + // `catch_unwind` for the same reason the status probe carries + // it: a panic here would skip the landing, and with it + // `finish_watch_open` — `opening` would stay true forever. + std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let dirs = scm_watch_dirs(h, &probe_root)?; + match h.watch(&dirs) { + Ok(sub) => Some(Arc::new(sub)), + Err(e) => { + log::warn!("source control: no watch for {probe_root:?}: {e}"); + None + } } - } + })) + .unwrap_or(None) }, move |app, sub: Option>, cx| { - app.scm_watch_opened(id, root, sub_gen, sub, cx) + app.scm_watch_opened(id, root, sub_gen, wipe, sub, cx) }, ); } @@ -871,13 +942,20 @@ impl Tty7App { host: HostId, root: PathBuf, sub_gen: u64, + wipe: u64, sub: Option>, cx: &mut Context, ) { let data = cx.default_global::(); // Letting go while the watch was opening leaves the only `Arc` here, - // so returning closes it. - if data.generation(host, &root) != sub_gen || !data.is_subscribed(host, &root) { + // so returning closes it. `wipe` closes the one gap `generation` + // cannot: a disconnect resets generations to their default, so a + // watch opened against the *previous* connection could otherwise be + // installed for the re-subscribed repository. + if data.wipe != wipe + || data.generation(host, &root) != sub_gen + || !data.is_subscribed(host, &root) + { data.finish_watch_open(host, &root, None, Instant::now()); return; } @@ -914,7 +992,7 @@ impl Tty7App { root: PathBuf, op: GitOp, then: Option, - window: &Window, + window: &mut Window, cx: &mut Context, ) { let Some(status) = status_of(cx, host.id(), &root) else { @@ -929,7 +1007,16 @@ impl Tty7App { let slot = if op.is_network() { match cx.default_global::().take_network_slot(id, &root) { Some(slot) => Some(slot), - None => return, + None => { + // Said out loud: a swallowed click on Push looks exactly + // like a push that finished instantly. + gpui_component::WindowExt::push_notification( + window, + crate::ui::i18n::t(crate::ui::i18n::L10nKey::ScmNetworkBusy).to_string(), + cx, + ); + return; + } } } else { None diff --git a/src/ui/diff_overlay.rs b/src/ui/diff_overlay.rs index 8a15eb98..5caf1658 100644 --- a/src/ui/diff_overlay.rs +++ b/src/ui/diff_overlay.rs @@ -146,11 +146,6 @@ impl Tty7App { } None => {} } - let seed = DiffLoad::Loading; - let epoch = match &seed { - DiffLoad::Ready(snap) => Some(scm_epoch(cx, host, &snap.root)), - _ => None, - }; self.remember_active_pane(window, cx); let Some(tab) = self.tabs.get_mut(active) else { return; @@ -161,25 +156,34 @@ impl Tty7App { cwd, source, focus_handle: focus_handle.clone(), - load: seed, + // Every open starts at Loading until its own probe lands. The old + // panel-snapshot seeding died with the panel that held a snapshot + // per source; re-seeding would need the caller to carry one. + load: DiffLoad::Loading, loading: false, expanded: HashMap::new(), focus, scroll: gpui::ScrollHandle::new(), - epoch, + epoch: None, }); window.focus(&focus_handle, cx); self.spawn_diff_probe(cx); cx.notify(); } + /// Which file the open overlay is focused on — for the row that asked, + /// which means the *source* has to match too: a file staged and edited + /// again sits in two panel groups, and only the row whose patch is + /// actually on screen may draw itself selected. pub(crate) fn diff_overlay_focus( &self, host: crate::ui::host_ops::HostId, cwd: &std::path::Path, + source: &crate::terminal::git_diff::DiffSource, ) -> Option<&str> { let overlay = self.tabs.get(self.active)?.diff_overlay.as_ref()?; - (overlay.cwd == cwd && overlay.host_id == host).then_some(overlay.focus.as_deref())? + (overlay.cwd == cwd && overlay.host_id == host && overlay.source == *source) + .then_some(overlay.focus.as_deref())? } pub(crate) fn close_diff_overlay(&mut self, window: &mut Window, cx: &mut Context) { @@ -1149,7 +1153,7 @@ fn source_subject(source: &DiffSource, branch: String) -> SourceSubject { DiffSource::Worktree | DiffSource::Head => branch_of(None), // Staged is the branch too, but a patch that does not match the files // on disk — without the chip it is indistinguishable from the above. - DiffSource::Staged => branch_of(Some("STAGED")), + DiffSource::Staged => branch_of(Some(t(L10nKey::ScmChipStaged))), DiffSource::Commit { rev, label } => SourceSubject { icon: "icons/git-commit.svg", text: short_rev(rev), diff --git a/src/ui/host_ops.rs b/src/ui/host_ops.rs index a72ca16c..60771ad3 100644 --- a/src/ui/host_ops.rs +++ b/src/ui/host_ops.rs @@ -241,6 +241,14 @@ impl InFlight { self.stale.extend(self.in_flight.iter().cloned()); } + /// Drop every key the predicate rejects — bookkeeping for work that will + /// never land (a host cleared away under an in-flight job). If the job + /// does land after all, its `finish` is a no-op rather than a poison. + pub fn retain(&mut self, keep: impl Fn(&K) -> bool) { + self.in_flight.retain(|k| keep(k)); + self.stale.retain(|k| keep(k)); + } + pub fn finish(&mut self, key: &K) -> bool { self.in_flight.remove(key); !self.stale.remove(key) diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 3611e1b0..34234025 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -845,6 +845,21 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::ScmCommitStaged => "Commit Staged", L10nKey::ScmStashAll => "Stash All", L10nKey::ScmNothingToCommit => "Nothing to commit", + L10nKey::ScmNetworkBusy => "Another network operation is still running for this repository", + L10nKey::ScmCommitNeedsMessage => "Write a commit message first", + L10nKey::ScmDetailFilesFailed => "The file list could not be read", + L10nKey::ScmTimeNow => "now", + L10nKey::ScmTimeMinutes => "{n}m", + L10nKey::ScmTimeHours => "{n}h", + L10nKey::ScmTimeDays => "{n}d", + L10nKey::ScmTimeMonths => "{n}mo", + L10nKey::ScmTimeYears => "{n}y", + L10nKey::ScmResetHardConfirm => { + "Reset the branch to this commit? Commits after it fall off the branch, \ + and uncommitted changes are discarded." + } + L10nKey::ScmReset => "Reset", + L10nKey::ScmChipStaged => "STAGED", L10nKey::ScmStage => "Stage Changes", L10nKey::ScmStageAll => "Stage All Changes", L10nKey::ScmUnstage => "Unstage Changes", @@ -1314,7 +1329,6 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::PanelMoreChangedFiles => { "… and {count} more changed files — run `git diff` to see them." } - L10nKey::PanelUntracked => "{count} untracked", L10nKey::ScmFilesChanged => "{count} files changed", L10nKey::ScmStagedFileCount => "{count} files staged", L10nKey::AppMenuAbout => "About tty7", @@ -1425,9 +1439,6 @@ pub fn translate_variant_en(key: L10nKey, branch: &'static str) -> Option<&'stat (L10nKey::ScmStagedFileCount, "zero") => "No staged changes", (L10nKey::ScmStagedFileCount, "one") => "1 file staged", (L10nKey::ScmStagedFileCount, "other") => "{count} files staged", - (L10nKey::PanelUntracked, "zero") => "0 untracked", - (L10nKey::PanelUntracked, "one") => "1 untracked", - (L10nKey::PanelUntracked, "other") => "{count} untracked", (L10nKey::PanelMoreChangedFiles, "zero") => { "… and 0 more changed files — run `git diff` to see them." } diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 0189df6d..1d0af07d 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -895,6 +895,22 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::ScmCommitStaged => "ステージ済みをコミット", L10nKey::ScmStashAll => "すべてスタッシュ", L10nKey::ScmNothingToCommit => "コミットするものがありません", + L10nKey::ScmNetworkBusy => "このリポジトリでは別のネットワーク操作が実行中です", + L10nKey::ScmCommitNeedsMessage => "先にコミットメッセージを入力してください", + L10nKey::ScmDetailFilesFailed => "ファイル一覧を読み込めませんでした", + L10nKey::ScmTimeNow => "今", + L10nKey::ScmTimeMinutes => "{n}分", + // 「{n}時」は時刻に読めるので「時間」のまま。 + L10nKey::ScmTimeHours => "{n}時間", + L10nKey::ScmTimeDays => "{n}日", + L10nKey::ScmTimeMonths => "{n}か月", + L10nKey::ScmTimeYears => "{n}年", + L10nKey::ScmResetHardConfirm => { + "ブランチをこのコミットへリセットしますか?それ以降のコミットはブランチから外れ、\ + 未コミットの変更は破棄されます。" + } + L10nKey::ScmReset => "リセット", + L10nKey::ScmChipStaged => "ステージ済み", L10nKey::ScmStage => "変更をステージ", L10nKey::ScmStageAll => "すべての変更をステージ", L10nKey::ScmUnstage => "ステージを取り消す", @@ -1359,7 +1375,6 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::PanelMoreChangedFiles => { "… さらに変更されたファイル {count} 個 — 表示するには `git diff` を実行してください" } - L10nKey::PanelUntracked => "未追跡 {count}", L10nKey::ScmFilesChanged => "{count} 個のファイルが変更されました", L10nKey::ScmStagedFileCount => "{count} 個のファイルがステージされました", L10nKey::AppMenuAbout => "tty7 について", @@ -1468,9 +1483,6 @@ pub fn translate_variant_ja(key: L10nKey, branch: &'static str) -> Option<&'stat (L10nKey::ScmStagedFileCount, "zero") => "ステージされた変更はありません", (L10nKey::ScmStagedFileCount, "one") => "1 個のファイルがステージされました", (L10nKey::ScmStagedFileCount, "other") => "{count} 個のファイルがステージされました", - (L10nKey::PanelUntracked, "zero") => "未追跡 0", - (L10nKey::PanelUntracked, "one") => "未追跡 1", - (L10nKey::PanelUntracked, "other") => "未追跡 {count}", (L10nKey::PanelMoreChangedFiles, "zero") => { "… さらに変更されたファイル 0 個 — 表示するには `git diff` を実行してください" } diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 99869077..738b074b 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -630,7 +630,6 @@ pub enum L10nKey { PanelNoChanges, PanelNoChangesHint, PanelMoreChangedFiles, - PanelUntracked, PanelSessionSubtitle, PanelProcessesSubtitle, PanelPortsSubtitle, @@ -660,6 +659,18 @@ pub enum L10nKey { ScmCommitStaged, ScmStashAll, ScmNothingToCommit, + ScmNetworkBusy, + ScmCommitNeedsMessage, + ScmDetailFilesFailed, + ScmTimeNow, + ScmTimeMinutes, + ScmTimeHours, + ScmTimeDays, + ScmTimeMonths, + ScmTimeYears, + ScmResetHardConfirm, + ScmReset, + ScmChipStaged, ScmStage, ScmStageAll, ScmUnstage, @@ -1122,70 +1133,13 @@ pub enum L10nKey { /// **Delete a key from this list as soon as something renders it.** #[allow(dead_code)] const SCM_KEYS_AWAITING_A_CALLER: &[L10nKey] = &[ - L10nKey::ScmGroupMerge, - L10nKey::ScmGroupStaged, - L10nKey::ScmGroupChanges, - L10nKey::ScmGroupUntracked, - L10nKey::ScmCommitPlaceholder, - L10nKey::ScmCommitButton, - L10nKey::ScmCommitAllButton, - L10nKey::ScmCommitAmendButton, - L10nKey::ScmCommitAndPush, - L10nKey::ScmCommitAndSync, - L10nKey::ScmAmendLastCommit, - L10nKey::ScmCommitStaged, - L10nKey::ScmStashAll, - L10nKey::ScmNothingToCommit, - L10nKey::ScmStage, - L10nKey::ScmStageAll, - L10nKey::ScmUnstage, - L10nKey::ScmUnstageAll, - L10nKey::ScmDiscard, - L10nKey::ScmDiscardAll, - L10nKey::ScmDiscardConfirm, - L10nKey::ScmOpenConflict, - L10nKey::ScmMarkResolved, - L10nKey::ScmUnrepresentablePath, - L10nKey::ScmPublishBranch, - L10nKey::ScmDetached, - L10nKey::ScmAmendBadge, - L10nKey::ScmSync, - L10nKey::ScmPush, - L10nKey::ScmPull, - L10nKey::ScmFetch, L10nKey::ScmCheckoutBranch, - L10nKey::ScmCreateBranch, + L10nKey::ScmCommitDetailTitle, + L10nKey::ScmCommitStaged, + L10nKey::ScmRefresh, + L10nKey::ScmResetToCommit, L10nKey::ScmSearchBranches, L10nKey::ScmStashAndSwitch, - L10nKey::ScmGraphTitle, - L10nKey::ScmGraphLoadMore, - L10nKey::ScmGraphFilterPlaceholder, - L10nKey::ScmGraphAllBranches, - L10nKey::ScmGraphEmpty, - L10nKey::ScmGraphCurrentBranch, - L10nKey::ScmCheckoutCommit, - L10nKey::ScmCreateBranchHere, - L10nKey::ScmResetSoft, - L10nKey::ScmResetMixed, - L10nKey::ScmResetHard, - L10nKey::ScmCommitDetailTitle, - L10nKey::ScmCherryPick, - L10nKey::ScmRevertCommit, - L10nKey::ScmResetToCommit, - L10nKey::ScmRefresh, - L10nKey::ScmTooManyChanges, - L10nKey::ScmOpenChanges, - L10nKey::ScmDiscardAllConfirm, - L10nKey::ScmAmendConfirm, - L10nKey::ScmOpMerge, - L10nKey::ScmOpRebase, - L10nKey::ScmOpCherryPick, - L10nKey::ScmOpRevert, - L10nKey::ScmOpBisect, - L10nKey::ScmOpAm, - L10nKey::ScmSwitchRepository, - L10nKey::DiffViewSplit, - L10nKey::DiffViewUnified, ]; pub fn set_locale(gui_language: &str) { @@ -1844,7 +1798,6 @@ mod tests { L10nKey::PanelNoChanges, L10nKey::PanelNoChangesHint, L10nKey::PanelMoreChangedFiles, - L10nKey::PanelUntracked, L10nKey::PanelSessionSubtitle, L10nKey::PanelProcessesSubtitle, L10nKey::PanelPortsSubtitle, @@ -2263,7 +2216,6 @@ mod tests { L10nKey::SettingsAliasesLinked, L10nKey::SettingsRulesOpenedWithConnection, L10nKey::SettingsOfflineMachines, - L10nKey::PanelUntracked, L10nKey::PanelMoreChangedFiles, L10nKey::ScmFilesChanged, L10nKey::ScmStagedFileCount, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index a7bf0395..cea743e6 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -808,7 +808,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::ScmGroupStaged => "暂存的更改", L10nKey::ScmGroupChanges => "更改", L10nKey::ScmGroupUntracked => "未跟踪", - L10nKey::ScmCommitPlaceholder => "写点什么改了…", + L10nKey::ScmCommitPlaceholder => "说说改了什么…", L10nKey::ScmCommitButton => "提交", L10nKey::ScmCommitAllButton => "提交全部", L10nKey::ScmCommitAmendButton => "提交（修订）", @@ -818,6 +818,21 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::ScmCommitStaged => "提交已暂存的更改", L10nKey::ScmStashAll => "全部贮藏", L10nKey::ScmNothingToCommit => "没有可提交的内容", + L10nKey::ScmNetworkBusy => "这个仓库还有一个网络操作在进行中", + L10nKey::ScmCommitNeedsMessage => "先写一条提交信息", + L10nKey::ScmDetailFilesFailed => "无法读取文件列表", + L10nKey::ScmTimeNow => "刚刚", + L10nKey::ScmTimeMinutes => "{n}分", + L10nKey::ScmTimeHours => "{n}时", + L10nKey::ScmTimeDays => "{n}天", + // "个月" 而不是 "月":"3月" 会被读成月份名。 + L10nKey::ScmTimeMonths => "{n}个月", + L10nKey::ScmTimeYears => "{n}年", + L10nKey::ScmResetHardConfirm => { + "把分支重置到这个提交?之后的提交会从分支上消失,未提交的更改会被丢弃。" + } + L10nKey::ScmReset => "重置", + L10nKey::ScmChipStaged => "已暂存", L10nKey::ScmStage => "暂存更改", L10nKey::ScmStageAll => "暂存全部更改", L10nKey::ScmUnstage => "取消暂存", @@ -1251,7 +1266,6 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::SftpErrorUnsafeRemoteName => "拒绝不安全的远程名称 {name}", L10nKey::SftpErrorInvalidOctalMode => "无效的八进制模式", L10nKey::PanelMoreChangedFiles => "…还有 {count} 个变更文件——运行 `git diff` 查看。", - L10nKey::PanelUntracked => "{count} 个未跟踪文件", L10nKey::ScmFilesChanged => "{count} 个文件改动", L10nKey::ScmStagedFileCount => "已暂存 {count} 个文件", L10nKey::AppMenuAbout => "关于 tty7", @@ -1358,9 +1372,6 @@ pub fn translate_variant_zh(key: L10nKey, branch: &'static str) -> Option<&'stat (L10nKey::ScmStagedFileCount, "zero") => "没有暂存的更改", (L10nKey::ScmStagedFileCount, "one") => "已暂存 1 个文件", (L10nKey::ScmStagedFileCount, "other") => "已暂存 {count} 个文件", - (L10nKey::PanelUntracked, "zero") => "0 个未跟踪文件", - (L10nKey::PanelUntracked, "one") => "1 个未跟踪文件", - (L10nKey::PanelUntracked, "other") => "{count} 个未跟踪文件", (L10nKey::PanelMoreChangedFiles, "zero") => "…还有 0 个变更文件——运行 `git diff` 查看。", (L10nKey::PanelMoreChangedFiles, "one") => "…还有 1 个变更文件——运行 `git diff` 查看。", (L10nKey::PanelMoreChangedFiles, "other") => { diff --git a/src/ui/scm/actions.rs b/src/ui/scm/actions.rs index a91441f0..07f59a2a 100644 --- a/src/ui/scm/actions.rs +++ b/src/ui/scm/actions.rs @@ -124,7 +124,7 @@ impl Tty7App { PromptLevel::Warning, &confirm_question(&op, loss), None, - &[t(L10nKey::Cancel), confirm_verb(loss)], + &[t(L10nKey::Cancel), confirm_verb(&op, loss)], cx, ); cx.spawn_in(window, async move |app, cx| { @@ -465,6 +465,12 @@ pub(crate) fn split_upstream(upstream: &str) -> Option<(&str, &str)> { /// The question a destructive operation has to answer before it runs. fn confirm_question(op: &GitOp, loss: Destructive) -> String { + // Its own question, not the discard one: a hard reset to an older commit + // drops commits off the branch, and a dialog that says "Discard every + // change in this repository?" never mentions the part that hurts. + if matches!(op, GitOp::Reset { .. }) { + return t(L10nKey::ScmResetHardConfirm).to_string(); + } match loss { Destructive::RewritesHistory => t(L10nKey::ScmAmendConfirm).to_string(), // One file gets named; a whole group does not, because a list of two @@ -476,7 +482,10 @@ fn confirm_question(op: &GitOp, loss: Destructive) -> String { } } -fn confirm_verb(loss: Destructive) -> &'static str { +fn confirm_verb(op: &GitOp, loss: Destructive) -> &'static str { + if matches!(op, GitOp::Reset { .. }) { + return t(L10nKey::ScmReset); + } match loss { Destructive::RewritesHistory => t(L10nKey::ScmAmendLastCommit), _ => t(L10nKey::ScmDiscard), diff --git a/src/ui/scm/detail.rs b/src/ui/scm/detail.rs index 40b4d969..5395faf6 100644 --- a/src/ui/scm/detail.rs +++ b/src/ui/scm/detail.rs @@ -241,7 +241,12 @@ impl Tty7App { if let Some(commit) = commit { open.commit = Some(Arc::new(commit)); } - open.files = Some(Arc::new(files.unwrap_or_default())); + match files { + Some(files) => open.files = Some(Arc::new(files)), + // A failed read is not an empty commit — see + // `CommitDetailView::files_failed`. + None => open.files_failed = true, + } cx.notify(); }, ); @@ -531,7 +536,12 @@ impl Tty7App { cx: &mut Context, ) -> AnyElement { let Some(files) = detail.files.clone() else { - return self.detail_note(t(L10nKey::PanelLoading).to_string(), cx); + let note = if detail.files_failed { + L10nKey::ScmDetailFilesFailed + } else { + L10nKey::PanelLoading + }; + return self.detail_note(t(note).to_string(), cx); }; let list = v_flex().child(self.detail_summary(&files, mono, cx)); // The label rides along on the source so the overlay's header can say @@ -649,7 +659,7 @@ impl Tty7App { let sf = panel_surface(cx); let deco = crate::ui::diff_overlay::deco_status(file.status); let (name, dir) = split_display_path(&file.path); - let selected = self.diff_overlay_focus(detail.repo.host, &detail.repo.root) + let selected = self.diff_overlay_focus(detail.repo.host, &detail.repo.root, source) == Some(file.path.as_str()); h_flex() diff --git a/src/ui/scm/graph.rs b/src/ui/scm/graph.rs index b12c3e37..52e3c097 100644 --- a/src/ui/scm/graph.rs +++ b/src/ui/scm/graph.rs @@ -74,6 +74,10 @@ use crate::ui::scm::state::RepoKey; /// phi, so 12px occupies `round(12 × 1.618) = 19px`, and 20 is the first even /// pitch above it. const GRAPH_ROW_H: f32 = 20.; +/// Rows materialized above and below the visible band, so a fast scroll never +/// outruns the window into blank space, and the "load more" band — laid out +/// one row past the window's end — stays below the fold until it is real. +const GRAPH_WINDOW_MARGIN: usize = 4; /// Header of the section itself: fold, title, count, filter tile, scope picker. /// @@ -314,6 +318,14 @@ struct GraphPaint { /// transparency when one is configured, which would let the lane line show /// straight down the middle of the node. surface: Hsla, + /// The selected row and the fill its band paints under the node, so a + /// hollow node's hole matches the selection band it sits on instead of + /// punching through to the resting surface. Hover is not covered — it + /// lives in gpui's element state, which a paint closure cannot read — so + /// a hovered ring keeps the resting hole; one step of fill under a 3px + /// hole, against a whole selected band showing the wrong colour. + selected: Option, + selected_surface: Hsla, /// Whether a "load more" band follows the last row. more: bool, } @@ -419,6 +431,11 @@ fn paint_graph(p: &GraphPaint, bounds: Bounds, window: &mut Window) { // because the border is part of that same SDF — stacking would blend the // inner edge over the outer one's already-blended edge, and a 3px hole // is where that shows. + let hole = if p.selected == Some(i) { + p.selected_surface + } else { + p.surface + }; if row.parents > 1 { // A merge is a ring. It is the one row shape a reader scans for, // and an outline reads at 8px where a second fill colour does not. @@ -426,7 +443,7 @@ fn paint_graph(p: &GraphPaint, bounds: Bounds, window: &mut Window) { window.paint_quad(quad( dot(r), Corners::all(px(r)), - p.surface, + hole, Edges::all(px(GRAPH_LINE_W)), ink, BorderStyle::Solid, @@ -437,7 +454,7 @@ fn paint_graph(p: &GraphPaint, bounds: Bounds, window: &mut Window) { window.paint_quad(quad( dot(GRAPH_DOT_R), Corners::all(px(GRAPH_DOT_R)), - p.surface, + hole, Edges::all(px(GRAPH_LINE_W)), ink, BorderStyle::Solid, @@ -527,7 +544,7 @@ impl Tty7App { Some(page) if page.commits.is_empty() => { self.panel_empty(t(L10nKey::ScmGraphEmpty), None, cx) } - Some(page) => self.graph_body(repo, &page, query.as_deref(), cx), + Some(page) => self.graph_body(repo, &page, query.as_deref(), height, cx), }; let (backing, handle) = self.graph_resize(ceiling, cx); @@ -631,6 +648,29 @@ impl Tty7App { ); } + /// Which commit indices the list shows for this page and query, resolved + /// through the cache on `GraphState` — see its doc for why it exists. + fn graph_visible_rows( + &mut self, + page: &Arc, + query: Option<&str>, + ) -> Arc> { + let key = Arc::as_ptr(page) as usize; + if let Some((held_query, held_page, rows)) = &self.scm.graph.filter_cache { + if *held_page == key && held_query.as_deref() == query { + return rows.clone(); + } + } + let rows: Arc> = Arc::new(match query { + None => (0..page.commits.len()).collect(), + Some(q) => (0..page.commits.len()) + .filter(|i| matches_query(&page.commits[*i], q)) + .collect(), + }); + self.scm.graph.filter_cache = Some((query.map(str::to_string), key, rows.clone())); + rows + } + /// The filter box's text, if it has any. fn graph_query(&self, cx: &Context) -> Option { let input = self.scm.graph.search.as_ref()?; @@ -693,11 +733,15 @@ impl Tty7App { impl Tty7App { /// The scrolling list: rows underneath, one canvas over the gutter. + /// + /// `height` is the section's height — the ceiling on how much of the list + /// can be on screen, and so on how many rows become elements. fn graph_body( &mut self, repo: &RepoKey, page: &Arc, query: Option<&str>, + height: f32, cx: &mut Context, ) -> AnyElement { let panel_w = cx.global::().right_panel_width; @@ -710,22 +754,31 @@ impl Tty7App { // So the filter hides the gutter entirely and the list becomes a flat // search result — which is what it actually is. let filtering = query.is_some(); - let rows: Vec = match query { - None => (0..page.commits.len()).collect(), - Some(q) => (0..page.commits.len()) - .filter(|i| matches_query(&page.commits[*i], q)) - .collect(), - }; + let rows = self.graph_visible_rows(page, query); // No row at the cap either: `load_page` clamps there, so "load more" // past it could only refetch what is already on screen. let more = query.is_none() && !page.complete && page.commits.len() < MAX_GRAPH_COMMITS; let bands = rows.len() + usize::from(more); + // Only the rows that can be on screen become elements — the row count + // is bounded by the cap at 5000, and a taffy pass over 5000 flex + // children per frame is most of a frame. The stack below keeps its + // full fixed height so the scroll range is unchanged; a top padding + // stands in for everything scrolled past. The canvas needs no such + // treatment: its paint is already clipped to the content mask. + let scrolled = (-self.scm.graph.scroll.offset().y.as_f32()).max(0.); + let first = ((scrolled / GRAPH_ROW_H) as usize) + .saturating_sub(GRAPH_WINDOW_MARGIN) + .min(rows.len()); + let visible = (height / GRAPH_ROW_H).ceil() as usize + GRAPH_WINDOW_MARGIN * 2; + let last = first.saturating_add(visible).min(rows.len()); + // With the gutter gone the text takes the panel's own inset, so a // search result does not sit in a column of empty space. let indent = if filtering { CONTENT_INSET } else { gutter }; - let list = v_flex().children( - rows.iter() + let list = v_flex().pt(px(first as f32 * GRAPH_ROW_H)).children( + rows[first..last] + .iter() .map(|i| self.graph_row(repo, page, *i, indent, now, cx)), ); let mut stack = div() @@ -736,6 +789,7 @@ impl Tty7App { .children(more.then(|| self.graph_load_more(gutter, cx))); if !filtering { + let sf = cx.global::().sidebar; let paint = GraphPaint { page: page.clone(), max_lanes: cap, @@ -744,7 +798,14 @@ impl Tty7App { // The hole in a hollow node has to be the exact fill behind it, // or the lane line running underneath shows through. The // section is flush on the panel, so that fill is the sidebar's. - surface: gpui::rgb(cx.global::().sidebar.base).into(), + surface: gpui::rgb(sf.base).into(), + selected: self + .scm + .graph + .selected + .as_deref() + .and_then(|oid| page.commits.iter().position(|c| c.oid == oid)), + selected_surface: gpui::rgb(sf.selected).into(), more, }; stack = stack.child( @@ -866,12 +927,15 @@ impl Tty7App { }) .on_click(cx.listener({ let repo = repo.clone(); - let oid = oid.clone(); // The row already holds everything the detail view renders, so - // it hands its own commit over and no `git show` is run. - let seed = commit.clone(); + // it hands its own commit over and no `git show` is run. The + // listener carries the page `Arc` and an index, not a clone of + // the commit: with up to 5000 rows a frame, one deep `Commit` + // clone per row (an 8KB body, refs) was most of the frame. + let page = page.clone(); move |this, _, _, cx| { - this.graph_open_commit(repo.clone(), oid.clone(), Some(seed.clone()), cx) + let seed = page.commits[i].clone(); + this.graph_open_commit(repo.clone(), seed.oid.clone(), Some(seed), cx) } })) .context_menu({ diff --git a/src/ui/scm/panel.rs b/src/ui/scm/panel.rs index e7be760f..31cf96b9 100644 --- a/src/ui/scm/panel.rs +++ b/src/ui/scm/panel.rs @@ -218,6 +218,19 @@ impl Tty7App { host: host.id(), root, }); + // An override whose host has left the registry is dropped, not worked + // around: falling back to the *pane's* host while keeping the + // override's root would probe the wrong machine for that path and + // cache the answer under a mismatched key. + if self + .scm + .repo_override + .as_ref() + .is_some_and(|k| crate::ui::host_registry::HostRegistry::get(cx, k.host).is_none()) + { + self.scm.repo_override = None; + self.scm.override_tab = None; + } // An explicit pick from the switcher wins over the pane's own // repository, so everything below reads through `active_repo`. let repo = self @@ -869,7 +882,7 @@ impl Tty7App { // does. .text_color(if live { fg } else { muted }) .disabled(!live) - .when(!live, |b| b.tooltip(t(L10nKey::ScmNothingToCommit))) + .when(!live, |b| b.tooltip(t(plan.reason))) .on_click(cx.listener(move |this, _, window, cx| { this.scm_commit( repo_for_button.clone(), @@ -1292,8 +1305,9 @@ impl Tty7App { let path = entry.path.as_str().to_string(); let (name, dir) = split_display_path(&path); let (letter, deco) = row_status(entry, group); - let selected = self.diff_overlay_focus(repo.host, &repo.root) == Some(path.as_str()); let source = group_diff_source(group); + let selected = + self.diff_overlay_focus(repo.host, &repo.root, &source) == Some(path.as_str()); let id = SharedString::from(format!("scm-row-{group:?}-{path}")); let actions = self.scm_row_actions( &id, @@ -1875,6 +1889,10 @@ pub(crate) fn operation_label(op: RepoOperation) -> L10nKey { pub(crate) struct CommitPlan { pub(crate) label: L10nKey, pub(crate) enabled: bool, + /// Why the button is disabled, when it is. "Nothing to commit" and "write + /// a message" call for opposite actions, and one tooltip for both sends + /// the user staging files they already staged. + pub(crate) reason: L10nKey, } /// Decide both from the state of the index. @@ -1894,9 +1912,19 @@ pub(crate) fn commit_plan(status: &WorkingTreeStatus, amend: bool, message: &str L10nKey::ScmCommitAllButton }; let has_message = !message.trim().is_empty(); + // Mid-merge, an empty message is still committable: `ops` sends + // `--allow-empty-message` for exactly this, because refusing would strand + // the merge behind a message the user deliberately cleared. + let merging = matches!(status.operation, Some(RepoOperation::Merge)); + let something = staged || tracked_edits || amend; CommitPlan { label, - enabled: (staged || tracked_edits || amend) && (has_message || amend), + enabled: something && (has_message || amend || merging), + reason: if something { + L10nKey::ScmCommitNeedsMessage + } else { + L10nKey::ScmNothingToCommit + }, } } diff --git a/src/ui/scm/path.rs b/src/ui/scm/path.rs index 4f730595..c8485d46 100644 --- a/src/ui/scm/path.rs +++ b/src/ui/scm/path.rs @@ -62,19 +62,25 @@ const YEAR: i64 = DAY * 365; /// `"2h"` / `"3d"` / `"5mo"` — a graph row has about 26px for this. /// +/// Through the i18n table like `home::relative_time`, in the compact spelling +/// this column's width demands — "now" is still an English word, and the row, +/// the tooltip, the detail byline and the overlay header all read it. +/// /// `now` is a parameter rather than a clock read so the whole thing stays a /// pure function, and so a test can sit exactly on a boundary. pub(crate) fn relative_time(now_unix: i64, then_unix: i64) -> String { + use crate::ui::i18n::{L10nKey, t, t_fmt}; // A commit stamped in the future (clock skew across machines is routine in // a shared repo) reads as "now" rather than as a negative age. let delta = (now_unix - then_unix).max(0); + let unit = |key: L10nKey, n: i64| t_fmt(key, &[("n", &n.to_string())]); match delta { - d if d < MINUTE => "now".to_string(), - d if d < HOUR => format!("{}m", d / MINUTE), - d if d < DAY => format!("{}h", d / HOUR), - d if d < MONTH => format!("{}d", d / DAY), - d if d < YEAR => format!("{}mo", d / MONTH), - d => format!("{}y", d / YEAR), + d if d < MINUTE => t(L10nKey::ScmTimeNow).to_string(), + d if d < HOUR => unit(L10nKey::ScmTimeMinutes, d / MINUTE), + d if d < DAY => unit(L10nKey::ScmTimeHours, d / HOUR), + d if d < MONTH => unit(L10nKey::ScmTimeDays, d / DAY), + d if d < YEAR => unit(L10nKey::ScmTimeMonths, d / MONTH), + d => unit(L10nKey::ScmTimeYears, d / YEAR), } } @@ -155,6 +161,7 @@ mod tests { #[test] fn relative_time_covers_every_bucket() { + crate::ui::i18n::set_locale("en"); let now = 1_800_000_000i64; let ago = |secs: i64| relative_time(now, now - secs); assert_eq!(ago(0), "now"); @@ -174,7 +181,21 @@ mod tests { #[test] fn relative_time_clamps_commits_from_the_future() { + crate::ui::i18n::set_locale("en"); let now = 1_800_000_000i64; assert_eq!(relative_time(now, now + DAY), "now"); } + + /// The row, the tooltip and the overlay byline all read this — it goes + /// through the i18n table like `home::relative_time`, in compact form. + #[test] + fn relative_time_speaks_the_ui_language() { + crate::ui::i18n::set_locale("zh-CN"); + let now = 1_800_000_000i64; + assert_eq!(relative_time(now, now), "刚刚"); + assert_eq!(relative_time(now, now - 2 * HOUR), "2时"); + // “3月”会被读成月份名,所以是“个月”。 + assert_eq!(relative_time(now, now - 3 * MONTH), "3个月"); + crate::ui::i18n::set_locale("en"); + } } diff --git a/src/ui/scm/state.rs b/src/ui/scm/state.rs index a4b1f8b3..d31e34f0 100644 --- a/src/ui/scm/state.rs +++ b/src/ui/scm/state.rs @@ -123,6 +123,30 @@ impl ScmPanelState { self.repo_override.as_ref().or(self.repo.as_ref()) } + /// Drop everything keyed to a host that left the registry. Without this, + /// `roots` and friends grow one entry per directory ever visited on a + /// link that no longer exists. Drafts survive on purpose: a reconnect + /// brings the same repositories back, and unsent messages with them. + pub(crate) fn forget_host(&mut self, host: HostId) { + self.roots.retain(|(h, _), _| *h != host); + self.root_lookups.retain(|(h, _)| *h != host); + self.probe_attempt.retain(|(h, _), _| *h != host); + self.branches.retain(|k, _| k.host != host); + self.branches_loading.retain(|k| k.host != host); + if self.repo_override.as_ref().is_some_and(|k| k.host == host) { + self.repo_override = None; + self.override_tab = None; + } + } + + /// A probe just answered "no repository" for `root`: every directory that + /// resolved to it has to re-ask. Left cached, the panel keeps mapping the + /// cwd to a repository that is gone and draws its Loading state forever. + pub(crate) fn forget_root(&mut self, host: HostId, root: &std::path::Path) { + self.roots + .retain(|(h, _), (_, held)| !(*h == host && held.as_deref() == Some(root))); + } + pub(crate) fn draft(&self, repo: &RepoKey) -> &str { self.drafts.get(repo).map(String::as_str).unwrap_or("") } @@ -175,6 +199,11 @@ pub(crate) struct GraphState { /// own entity; without this the box would take text the list never sees. pub(crate) search: Option>, pub(crate) search_sub: Option, + /// Which commit indices the list shows, cached per (page identity, query). + /// The filter case-folds every subject and author; re-running that over + /// 5000 commits on every frame while the box is open is real work, and + /// even the unfiltered identity list is 40KB of indices a frame. + pub(crate) filter_cache: Option<(Option, usize, Arc>)>, /// An open "name a branch at this commit" input, and the rev it starts /// from. The panel's own naming row cannot serve this: it always creates /// at HEAD, and the whole point here is the commit under the cursor. @@ -219,6 +248,11 @@ pub(crate) struct CommitDetailView { /// that is not in this repository. pub(crate) commit: Option>, pub(crate) files: Option>>, + /// The file read came back with nothing — git errored, or the link + /// dropped mid-read. Distinct from "still loading", and from an empty + /// list: "0 files changed" for a commit whose files could not be read + /// would be a confident lie. + pub(crate) files_failed: bool, /// A long body starts folded — a merge from a bot can run to fifty lines, /// and the file list is what the reader came for. pub(crate) body_expanded: bool, @@ -238,6 +272,7 @@ impl CommitDetailView { loaded: false, commit: seed.map(Arc::new), files: None, + files_failed: false, body_expanded: false, } }