diff --git a/CHANGELOG.md b/CHANGELOG.md index c8de8edb..f9c89c0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,7 +65,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 stays for the many reads that answer in bytes. A stream that goes silent for two minutes while the link stays up ends with a timeout rather than parking its reader forever — the wait is between chunks, not on the whole read, so a - slow-but-alive `git diff` still runs to completion. (#239) + slow-but-alive `git diff` still runs to completion. And the queue *between* + the two ends is bounded as well, not just the reads at either end: a peer that + pushes faster than this side can parse is cut off at 32 MiB of arrears with an + error, rather than quietly reassembling the whole diff in a channel. (#239) - **Sidebar diff preview is optional** — clicking a sidebar row's `+N −N` working-tree counts opens the diff overlay, which is the point of them for @@ -144,15 +147,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The untracked list escaped all of the above: `git ls-files --others` reports every path not yet ignored, and one un-ignored `node_modules` reached the overlay as tens of thousands of rows without touching the diff at all. It is - now streamed and capped like the diff, counts toward the oversized - threshold, and renders a bounded number of rows — while the count shown - stays the true total, so a capped list never reads as files having vanished. - - The 400-line auto-collapse rule was per-file, so sixty forty-line files all - opened at once (2400 side-by-side rows, none of them individually large). - Past a repo-wide total the overlay now opens with every file collapsed — - zero rows built — leads with a summary saying the diff is too large to - render efficiently, and points at expanding individual files or `git diff` - in the terminal. + now streamed, capped where it is retained and again where it is rendered — + while the count shown stays the true total, so a capped list never reads as + files having vanished. It deliberately does *not* drive the collapse- + everything rule below: folding file bodies shut removes no untracked rows, + so a tree with an un-ignored dependency directory and three edited files + would have hidden the three cheap things and kept the expensive one. + - The 400-line auto-collapse rule was per-file, so sixty medium files all + opened at once (thousands of side-by-side rows, none of them individually + large). Past a repo-wide total the overlay now opens with every file + collapsed — zero rows built — leads with a summary saying the diff is too + large to render efficiently, and points at expanding individual files or + `git diff` in the terminal. That total counts the context lines git prints + around every hunk, which is what is actually rendered, so it sits several + times above the `+N −N` figure at which it would otherwise fire on an + ordinary afternoon's work. - **One `git diff` per repository, not two** — the Changes panel and the diff overlay each ran their own full-diff probe and kept their own snapshot of the @@ -162,6 +171,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **An unsubscribed directory watch stops delivering immediately** — dropping a + local watch handle now closes its delivery channel rather than only asking the + OS backend to stand down. Tearing that backend down is not instantaneous, and + on Windows a `ReadDirectoryChangesW` completion can fire *during* teardown and + reach a consumer that has already unsubscribed. Batches already queued stay + readable, which is the one thing a consumer racing its own drop may + legitimately still see. (#239) + - **Rounded UI controls no longer square off their corners** ([#236](https://github.com/l0ng-ai/tty7/issues/236)) — the cursor-shape toggles (Block / Bar / Underline) are the clearest case: the selected diff --git a/crates/tty7-core/src/host/remote.rs b/crates/tty7-core/src/host/remote.rs index f26ccb40..447afc96 100644 --- a/crates/tty7-core/src/host/remote.rs +++ b/crates/tty7-core/src/host/remote.rs @@ -35,7 +35,7 @@ use std::collections::HashMap; use std::io::{self, Read, Write}; use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; use std::sync::{Arc, Mutex, Weak, mpsc}; use std::time::{Duration, Instant}; @@ -390,7 +390,14 @@ impl Host for RemoteHost { // with nowhere to go — see `ControlRequest::GitStream`. let id = self.next_stream.fetch_add(1, Ordering::Relaxed); let (tx, rx) = mpsc::channel(); - self.streams.insert(id, tx); + let queued = Arc::new(AtomicUsize::new(0)); + self.streams.insert( + id, + StreamSink { + tx, + queued: Arc::clone(&queued), + }, + ); // The receiver comes off the registry however this returns: an early // `?` below would otherwise leak the entry for the life of the // connection. @@ -408,7 +415,7 @@ impl Host for RemoteHost { other => return Err(wrong_shape("an accepted git stream", &other)), } - drain_git_stream(&rx, GIT_STREAM_IDLE_TIMEOUT, on_line) + drain_git_stream(&rx, &queued, GIT_STREAM_IDLE_TIMEOUT, on_line) } /// Safe to send unguarded: the request landed in control v2, and the @@ -474,15 +481,45 @@ impl std::fmt::Debug for RemoteHost { // Watches // --------------------------------------------------------------------------- -/// Reassemble one git stream's pushes into lines, ending on `GitEnd`, on a link -/// that died, or on `idle` elapsing between chunks. +/// Bytes one stream may have sitting between the reader thread and the thread +/// draining it. /// -/// Split out from [`RemoteHost::git_lines`] so the three ways a stream ends are +/// The queue below is unbounded and its `send` never waits, deliberately: the +/// reader thread serves the *whole* connection, so parking it there would stall +/// every other reply, every watch event and the keepalive pongs — a peer that +/// out-runs one diff reader would take the link down with it. The cost of not +/// waiting is that nothing throttles the sender, and "streaming" would bound +/// what each end reads at once while letting the queue between them grow to the +/// size of the whole diff — the exact peak this path exists to remove, one +/// container further along. +/// +/// So the queue is *bounded* instead of back-pressured: past this the stream is +/// failed with [`GitStreamMsg::Overrun`] rather than served, which turns an +/// unbounded allocation into a read that says what happened. Real back-pressure +/// would need credit-based flow control in the dialect — the client telling the +/// server how much more it may push — which is a protocol change, not a +/// buffering policy, and is not what this is. +/// +/// Set far above any healthy gap. The drainer only splits lines and parses, at +/// roughly 8 MB per 12 ms, so it stays within a chunk or two of a link that is +/// merely fast; reaching 32 MiB of arrears means the consumer is wedged, not +/// busy. +const GIT_STREAM_QUEUE_BUDGET: usize = 32 * 1024 * 1024; + +/// Reassemble one git stream's pushes into lines, ending on `GitEnd`, on a link +/// that died, on the queue budget blowing, or on `idle` elapsing between chunks. +/// +/// Split out from [`RemoteHost::git_lines`] so the ways a stream ends are /// reachable from a test without a socket — the timeout in particular, which /// otherwise could only be exercised by waiting out /// [`GIT_STREAM_IDLE_TIMEOUT`]. +/// +/// `queued` is the arrears this stream has accrued, in bytes; every chunk taken +/// off the channel is subtracted from it, which is what lets the reader thread +/// see a consumer falling behind. See [`GIT_STREAM_QUEUE_BUDGET`]. fn drain_git_stream( rx: &mpsc::Receiver, + queued: &AtomicUsize, idle: Duration, on_line: &mut dyn FnMut(&str), ) -> io::Result> { @@ -493,7 +530,31 @@ fn drain_git_stream( // `GIT_STREAM_IDLE_TIMEOUT` for what this catches that neither the // request deadline nor keepalive can. match rx.recv_timeout(idle) { - Ok(GitStreamMsg::Chunk(bytes)) => split.push(&bytes, &mut *on_line), + Ok(GitStreamMsg::Chunk(bytes)) => { + // Before parsing, not after: the arrears the reader thread reads + // must fall as soon as the bytes are ours, or a slow parse of one + // chunk would count against the budget twice. + // + // Saturating, because the two sides of this counter are updated + // by different threads and only the *sum* is ever meaningful: a + // chunk that reached the channel before its charge landed would + // otherwise wrap the counter to `usize::MAX` and kill the next + // healthy stream for being over budget. + let _ = queued.fetch_update(Ordering::AcqRel, Ordering::Acquire, |q| { + Some(q.saturating_sub(bytes.len())) + }); + split.push(&bytes, &mut *on_line); + } + // The queue outgrew its budget, so the reader thread stopped filling + // it. Everything after the last delivered chunk is missing, which + // makes this a failed read rather than a short one — the same rule + // the timeout arm follows. + Ok(GitStreamMsg::Overrun) => { + return Err(io::Error::other(format!( + "the git stream outran this client by more than \ + {GIT_STREAM_QUEUE_BUDGET} queued bytes" + ))); + } Ok(GitStreamMsg::End { code, failed }) => { split.finish(&mut *on_line); return if failed { @@ -545,6 +606,20 @@ enum GitStreamMsg { code: Option, failed: bool, }, + /// This stream fell far enough behind to hit [`GIT_STREAM_QUEUE_BUDGET`], + /// so the reader thread cut it loose. Always the last message: its sender + /// is off the table by the time it is sent. + Overrun, +} + +/// Where one running stream's pushes go, plus what it owes. +struct StreamSink { + tx: mpsc::Sender, + /// Bytes handed to `tx` and not yet taken off it. Written by the reader + /// thread, subtracted by the drainer — the one number both sides of the + /// queue can see, and the only thing standing between an unbounded queue + /// and the whole diff. See [`GIT_STREAM_QUEUE_BUDGET`]. + queued: Arc, } /// Receivers for git streams currently running on this connection, keyed by the @@ -558,7 +633,7 @@ struct GitStreamRegistry { #[derive(Default)] struct StreamTable { - senders: HashMap>, + senders: HashMap, /// Set by [`GitStreamRegistry::close_all`] and never cleared: a /// `ControlClient` never comes back up, so once the link is gone no stream /// registered afterwards could ever be answered. Without it a `git_lines` @@ -568,11 +643,11 @@ struct StreamTable { } impl GitStreamRegistry { - fn insert(&self, id: u64, tx: mpsc::Sender) { + fn insert(&self, id: u64, sink: StreamSink) { if let Ok(mut m) = self.streams.lock() && !m.closed { - m.senders.insert(id, tx); + m.senders.insert(id, sink); } // Dropped rather than filed when the link is already down, which closes // the channel and sends the caller straight down the mid-stream arm. @@ -609,16 +684,46 @@ impl GitStreamRegistry { /// channel is unbounded and `send` never waits. A chunk for an id that has /// already finished (a cancelled read the server had not noticed yet) is /// dropped, which is the same unknown-id rule watches follow. + /// + /// Not blocking is what makes the queue everyone's problem, so this is also + /// where it is bounded: each chunk is charged to the stream's arrears, and a + /// stream whose drainer has fallen [`GIT_STREAM_QUEUE_BUDGET`] behind is cut + /// loose with an [`Overrun`](GitStreamMsg::Overrun) instead of being fed + /// further. Cutting it loose — rather than dropping the chunk — is the only + /// honest option: the queue is a byte stream being reassembled into lines, so + /// a hole in the middle of it is not a shorter diff, it is a wrong one. fn dispatch(&self, event: ControlEvent) { let (id, msg) = match event { ControlEvent::GitChunk { id, bytes } => (id, GitStreamMsg::Chunk(bytes)), ControlEvent::GitEnd { id, code, failed } => (id, GitStreamMsg::End { code, failed }), _ => return, }; - if let Ok(m) = self.streams.lock() - && let Some(tx) = m.senders.get(&id) - { - let _ = tx.send(msg); + let Ok(mut m) = self.streams.lock() else { + return; + }; + let over = match (m.senders.get(&id), &msg) { + (None, _) => return, + (Some(sink), GitStreamMsg::Chunk(bytes)) => { + sink.queued.fetch_add(bytes.len(), Ordering::AcqRel) + bytes.len() + > GIT_STREAM_QUEUE_BUDGET + } + // `End` and `Overrun` carry no payload to charge for, and an end must + // always get through — a stream that stops speaking without one is + // the shape the idle timeout exists to catch, at a cost of two + // minutes. + (Some(_), _) => false, + }; + if over { + // Taken off the table first, so the chunks still arriving for this id + // meet the unknown-id rule above instead of queueing behind a message + // that says the queue is full. + if let Some(sink) = m.senders.remove(&id) { + let _ = sink.tx.send(GitStreamMsg::Overrun); + } + return; + } + if let Some(sink) = m.senders.get(&id) { + let _ = sink.tx.send(msg); } } } @@ -999,7 +1104,8 @@ mod tests { let mut lines = Vec::new(); let started = Instant::now(); - let got = drain_git_stream(&rx, Duration::from_millis(120), &mut |l| { + let queued = AtomicUsize::new(b"alpha\n".len()); + let got = drain_git_stream(&rx, &queued, Duration::from_millis(120), &mut |l| { lines.push(l.to_string()) }); @@ -1034,7 +1140,9 @@ mod tests { let mut lines = Vec::new(); let started = Instant::now(); - let code = drain_git_stream(&rx, idle, &mut |l| lines.push(l.to_string())).unwrap(); + let queued = AtomicUsize::new(0); + let code = + drain_git_stream(&rx, &queued, idle, &mut |l| lines.push(l.to_string())).unwrap(); assert_eq!(code, Some(0)); assert_eq!(lines.len(), 5, "{lines:?}"); @@ -1044,6 +1152,120 @@ mod tests { ); } + /// A stream whose drainer falls far enough behind is cut loose instead of + /// being queued without limit. + /// + /// This is the bound that makes the streaming path's memory claim true on a + /// *remote* host. The read is incremental on both ends — 64 KiB at the + /// server, one line at the client — but between them sits a queue the reader + /// thread never waits on, and it cannot wait on it: that thread serves the + /// whole connection, so parking it there would stall every other reply and + /// the keepalive with it. Unbounded, a peer pushing faster than this client + /// parses rebuilds the whole-diff peak in the channel, which is the one thing + /// the buffered read was replaced to avoid. + /// + /// Driven through `dispatch`, not by hand, because the accounting is split + /// across the two threads and only their pairing is worth asserting. + #[test] + fn a_stream_that_outruns_its_queue_budget_is_cut_loose() { + let registry = GitStreamRegistry::default(); + let (tx, rx) = mpsc::channel(); + let queued = Arc::new(AtomicUsize::new(0)); + registry.insert( + 1, + StreamSink { + tx, + queued: Arc::clone(&queued), + }, + ); + + // Nobody is draining, so every chunk is arrears. One megabyte at a time + // to keep the test's own allocation modest. + let chunk = vec![b'x'; 1024 * 1024]; + let pushes = GIT_STREAM_QUEUE_BUDGET / chunk.len() + 2; + for _ in 0..pushes { + registry.dispatch(ControlEvent::GitChunk { + id: 1, + bytes: chunk.clone(), + }); + } + assert!( + queued.load(Ordering::Acquire) <= GIT_STREAM_QUEUE_BUDGET + chunk.len(), + "the arrears stopped growing at the budget, not at the diff's size" + ); + + // The reader thread also stops routing to it, so a stream that keeps + // arriving cannot queue behind the notice. + registry.dispatch(ControlEvent::GitChunk { + id: 1, + bytes: chunk.clone(), + }); + + // What the drainer sees: the chunks that fit, then the overrun, and an + // error rather than a short read reported as a success. + let mut lines = Vec::new(); + let err = drain_git_stream(&rx, &queued, Duration::from_secs(5), &mut |l| { + lines.push(l.to_string()) + }) + .expect_err("a cut-off stream must not read as a complete diff"); + assert!(err.to_string().contains("outran"), "{err}"); + } + + /// The budget must not fire on a stream that is merely *large*. It bounds + /// how far the consumer may fall behind, not how much may cross — a drainer + /// keeping up returns the arrears as fast as they are charged, so a diff of + /// any size passes through a queue that never grows. + /// + /// The feeder throttles itself on the same counter the reader thread charges, + /// which is what "a consumer keeping up" means here and is what keeps this + /// test a statement about the accounting rather than a race between two + /// threads' speeds. + #[test] + fn a_large_but_drained_stream_never_trips_the_budget() { + let registry = Arc::new(GitStreamRegistry::default()); + let (tx, rx) = mpsc::channel(); + let queued = Arc::new(AtomicUsize::new(0)); + registry.insert( + 1, + StreamSink { + tx, + queued: Arc::clone(&queued), + }, + ); + + // Twice the budget in total, in 1 MiB chunks, never more than 4 MiB of it + // outstanding at once. + let feeder = Arc::clone(®istry); + let feeder_queued = Arc::clone(&queued); + let chunks = GIT_STREAM_QUEUE_BUDGET / (1024 * 1024) * 2; + thread::spawn(move || { + for _ in 0..chunks { + let deadline = Instant::now() + Duration::from_secs(10); + while feeder_queued.load(Ordering::Acquire) > 4 * 1024 * 1024 { + if Instant::now() > deadline { + break; // the drainer is wedged; let the assertions say so + } + thread::yield_now(); + } + let mut bytes = vec![b'x'; 1024 * 1024 - 1]; + bytes.push(b'\n'); + feeder.dispatch(ControlEvent::GitChunk { id: 1, bytes }); + } + feeder.dispatch(ControlEvent::GitEnd { + id: 1, + code: Some(0), + failed: false, + }); + }); + + let mut lines = 0usize; + let code = drain_git_stream(&rx, &queued, Duration::from_secs(10), &mut |_| lines += 1) + .expect("a drained stream is not an overrun, however much crosses it"); + assert_eq!(code, Some(0)); + assert_eq!(lines, chunks, "every line arrived"); + assert_eq!(queued.load(Ordering::Acquire), 0, "the arrears settled"); + } + /// A link that dies mid-stream ends the read with an error rather than /// parking the thread that was draining it. /// diff --git a/crates/tty7-core/src/host/server.rs b/crates/tty7-core/src/host/server.rs index cb4cd6e0..4001719e 100644 --- a/crates/tty7-core/src/host/server.rs +++ b/crates/tty7-core/src/host/server.rs @@ -1337,11 +1337,6 @@ fn spawn_watch_forwarder(id: u64, rx: smol::channel::Receiver>, sin } } -/// Why a git stream stopped pushing chunks early. -/// -/// The two are not interchangeable, which is the whole reason the distinction -/// is carried: only one of them means the peer is gone. See -/// [`Conn::start_git_stream`]. /// One connection's claim on a concurrent git stream, given back on drop. /// /// A guard rather than a bare `fetch_sub` at the end of the thread, so a slot is @@ -1355,6 +1350,11 @@ impl Drop for StreamSlot { } } +/// Why a git stream stopped pushing chunks early. +/// +/// The two are not interchangeable, which is the whole reason the distinction +/// is carried: only one of them means the peer is gone. See +/// [`Conn::start_git_stream`]. #[derive(Clone, Copy, PartialEq, Eq)] enum StreamStop { /// The write failed: the link is retired or broken, and nothing more will diff --git a/src/terminal/git_diff.rs b/src/terminal/git_diff.rs index cf079a1c..237f8b32 100644 --- a/src/terminal/git_diff.rs +++ b/src/terminal/git_diff.rs @@ -57,7 +57,17 @@ pub const AUTO_COLLAPSE_LINES: u32 = 400; /// threshold matter here — this counts context lines too (they are rendered, /// so they are what costs), and it is a sum, so many medium files add up the /// way one big file does. -pub const AUTO_COLLAPSE_TOTAL_LINES: usize = 2_000; +/// +/// Counting context is also why this sits well above the row count that first +/// looks alarming. A hunk carries three lines of context each side by default, +/// so an ordinary afternoon — forty files, a handful of small hunks each — +/// retains four to six lines for every line it actually changed: at 2000 the +/// threshold fired on a tree whose `+N −N` read about 400, which is nobody's +/// idea of a diff too big to open. The number to compare against is +/// [`MAX_TOTAL_LINES`], the point past which the parser stops retaining at all; +/// this is deliberately a large fraction of it, because collapsing everything is +/// the heavier intervention of the two and should not arrive first by much. +pub const AUTO_COLLAPSE_TOTAL_LINES: usize = 8_000; /// The same idea by file count. Set well clear of a busy-but-ordinary tree — /// forty changed files of a few lines each is a normal afternoon and must still @@ -114,6 +124,24 @@ pub struct DiffSnapshot { /// files having disappeared. Read it through /// [`untracked_count`](Self::untracked_count). pub untracked_total: usize, + /// One of the two reads behind this snapshot did not complete, so the + /// emptiness below it means "we could not look", not "there is nothing". + /// + /// A failed probe still produces a snapshot — the overlay keeps its branch + /// and its shape, and the next refresh fills it in, which is better than + /// blanking. But an empty file list renders as *Working tree clean*, and + /// that sentence is a claim about the repository: saying it because a read + /// timed out tells the reader their changes are gone. This is the bit that + /// keeps the two apart. + /// + /// Newly reachable, too. A buffered read either arrived or errored; a + /// stream can also be refused ([`MAX_CONCURRENT_GIT_STREAMS`] on one + /// connection) or go silent mid-diff, so the empty-because-broken case is + /// no longer rare enough to leave conflated with the empty-because-clean + /// one. + /// + /// [`MAX_CONCURRENT_GIT_STREAMS`]: tty7_core::daemon::control::MAX_CONCURRENT_GIT_STREAMS + pub read_failed: bool, } impl DiffSnapshot { @@ -178,7 +206,10 @@ impl DiffSnapshot { totals: (added, removed), retained_lines, untracked_count, - oversized: self.files.len() + untracked_count > AUTO_COLLAPSE_TOTAL_FILES + // Changed files only. Untracked paths are bounded where they are + // *rendered* instead — see the field's own note for why collapsing + // bodies is the wrong lever for them. + oversized: self.files.len() > AUTO_COLLAPSE_TOTAL_FILES || retained_lines > AUTO_COLLAPSE_TOTAL_LINES, budget_exhausted, per_file_truncated, @@ -206,13 +237,15 @@ pub struct DiffStats { /// individual files still expand by click, which is the escape hatch the /// summary points at. /// - /// Untracked paths count toward the file axis because they cost the same - /// thing a collapsed file card costs — one row each, in the same - /// non-virtualized list. A tree with an un-ignored `node_modules` is the - /// exact stall this is here to prevent, and it reaches the overlay through - /// `ls-files`, not through the diff. The summary names whichever axis - /// tripped, so a big untracked list never reads as a claim that the *diff* - /// is big. + /// Changed files and retained lines only. Untracked paths are the same + /// one-row-per-entry cost, but this is not the lever that answers them: + /// collapsing every file body leaves the untracked section rendering exactly + /// as many rows as before, because that section has no bodies to fold. A + /// tree with an un-ignored `node_modules` and three edited files would have + /// folded away the three cheap things and kept the expensive one — while + /// telling the reader their working tree was too large to render. The + /// untracked list is bounded where it is actually built, by + /// [`MAX_UNTRACKED`] on retention and [`MAX_RENDERED_FILES`] on rows. pub oversized: bool, /// The repo-wide budget dropped hunks that a smaller diff would have kept — /// the overlay says so, so a missing body reads as a cap rather than as tty7 @@ -301,14 +334,18 @@ pub fn probe(host: &dyn Host, cwd: &Path) -> Option { // carry it and buffered where it can't, so the lines seen here are the same // either way. let mut parser = DiffParser::default(); - let files = match host.git_lines( + let diffed = host.git_lines( cwd, &["diff", "--no-color", "--no-ext-diff", "-M", "HEAD"], &mut |line| parser.push_line(line), - ) { + ); + let files = match diffed { // A failed diff (e.g. racing a concurrent git write) still yields a // snapshot — an empty file list with the branch — rather than hiding - // the overlay; the next refresh fills it in. + // the overlay; the next refresh fills it in. A *partial* read is + // discarded rather than shown: the stream is reassembled into lines, so + // what a cut one is missing is the tail of the diff, and half a diff + // presented as a whole one is worse than none. Ok(Some(0)) => parser.finish(), _ => Vec::new(), }; @@ -341,6 +378,7 @@ pub fn probe(host: &dyn Host, cwd: &Path) -> Option { files, untracked, untracked_total, + read_failed: !matches!(diffed, Ok(Some(0))) || !matches!(listed, Ok(Some(0))), }) } @@ -872,9 +910,16 @@ index 1..2 100644 }; assert!(by_files.stats().oversized); - // Few files, but past the line threshold. + // Few files, but past the line threshold. Spread over enough files to + // clear it without any one of them hitting `MAX_LINES_PER_FILE` first — + // the per-file cap would otherwise decide this test's outcome instead of + // the repo-wide threshold it is about. + let per_file = MAX_LINES_PER_FILE / 2; let by_lines = DiffSnapshot { - files: parse_unified(&many_files(2, AUTO_COLLAPSE_TOTAL_LINES)), + files: parse_unified(&many_files( + AUTO_COLLAPSE_TOTAL_LINES / per_file + 1, + per_file, + )), ..Default::default() }; assert!(by_lines.files.len() <= AUTO_COLLAPSE_TOTAL_FILES); diff --git a/src/ui/diff_overlay.rs b/src/ui/diff_overlay.rs index 70841072..8892d27b 100644 --- a/src/ui/diff_overlay.rs +++ b/src/ui/diff_overlay.rs @@ -417,7 +417,15 @@ impl Tty7App { let content = match &overlay.load { DiffLoad::Loading => self.diff_message("Reading diff…", cx), DiffLoad::NotARepo => self.diff_message("Not a git repository", cx), - DiffLoad::Ready(snap) if snap.files.is_empty() && snap.untracked.is_empty() => { + // Empty because the read broke, not because the tree is clean. Both + // land here as a snapshot with no files — see + // `DiffSnapshot::read_failed` — and only one of them may be reported + // as a fact about the repository. + DiffLoad::Ready(snap) if empty_snapshot(snap) && snap.read_failed => self.diff_message( + "Couldn't read the working-tree diff — retrying on the next refresh.", + cx, + ), + DiffLoad::Ready(snap) if empty_snapshot(snap) => { self.diff_message("Working tree clean", cx) } DiffLoad::Ready(snap) => { @@ -464,10 +472,15 @@ impl Tty7App { window: &mut Window, cx: &mut Context, ) -> impl IntoElement + use<> { + // Through `stats` like every other whole-snapshot question on the render + // path, rather than `totals` plus `untracked_count`: same single walk, + // and it keeps "ask the snapshot once" a rule with no exceptions to + // drift from. let (branch, files, untracked, added, removed) = match &overlay.load { DiffLoad::Ready(s) => { - let (a, r) = s.totals(); - (s.branch.clone(), s.files.len(), s.untracked_count(), a, r) + let stats = s.stats(); + let (a, r) = stats.totals; + (s.branch.clone(), s.files.len(), stats.untracked_count, a, r) } _ => (String::new(), 0, 0, 0, 0), }; @@ -1107,6 +1120,13 @@ fn focused_name(overlay: &DiffOverlayState) -> Option { Some(snap.files[idx].path.clone()) } +/// Nothing to show: no changed file and no untracked path. Says nothing about +/// *why* — [`DiffSnapshot::read_failed`] is what tells a clean tree apart from +/// a read that never landed. +fn empty_snapshot(snap: &DiffSnapshot) -> bool { + snap.files.is_empty() && snap.untracked.is_empty() +} + /// Whether a file's body shows. /// /// An explicit choice in `expanded` is final: it is answered before the default @@ -1437,15 +1457,15 @@ mod tests { assert!(file_expanded(&untouched, &picked, false)); } - /// Sixty files of forty lines each never trip the per-file threshold (each - /// is a tenth of it), yet would open 2400 diff rows at once. `oversized` - /// catches it on the line axis, and with everything collapsed the overlay - /// builds zero rows. + /// Sixty files of a hundred and fifty lines each never trip the per-file + /// threshold (each is well under it), yet would open 9000 diff rows at once. + /// `oversized` catches it on the line axis, and with everything collapsed + /// the overlay builds zero rows. #[test] fn many_medium_files_are_oversized_and_build_no_rows() { let snap = DiffSnapshot { files: (0..60) - .map(|i| small_file(&format!("f{i}.rs"), 40)) + .map(|i| small_file(&format!("f{i}.rs"), 150)) .collect(), ..Default::default() }; @@ -1470,32 +1490,102 @@ mod tests { .flat_map(|f| &f.hunks) .map(|h| split_hunk(&h.lines).len()) .sum(); - assert_eq!(rows_expanded, 2400, "what the old rule would have built"); + assert_eq!(rows_expanded, 9000, "what the old rule would have built"); assert_eq!(rows_collapsed, 0); } - /// The other side of the same coin: a busy-but-ordinary afternoon — forty - /// files, a few lines each — is *not* oversized and opens expanded exactly - /// as it does today. The thresholds must not tax a normal working tree. + /// The other side of the same coin: a busy-but-ordinary afternoon is *not* + /// oversized and opens expanded exactly as it does today. The thresholds + /// must not tax a normal working tree. + /// + /// Built out of context-heavy files rather than bare changed lines, because + /// that is what a real diff looks like and it is the difference the line + /// threshold is most easily mis-set against: git prints three lines of + /// context each side of every hunk, so `retained_lines` runs several times + /// the `+N −N` a person reads off the header. A tree of forty files with a + /// handful of small hunks each is an afternoon's work, and it must open. #[test] fn an_ordinary_busy_tree_is_not_oversized() { + // Forty files × ten hunks × (6 context + 1 changed) — 2800 retained + // lines behind a header reading `+400 −0`, which is a morning, not a + // refactor. Sized to sit above the threshold this used to carry and + // below the one it carries now: an assertion that passes either way + // would not be watching anything. let snap = DiffSnapshot { files: (0..40) - .map(|i| small_file(&format!("f{i}.rs"), 12)) + .map(|i| { + let mut f = context_heavy_file(&format!("f{i}.rs")); + f.hunks = std::iter::repeat_n(f.hunks[0].clone(), 10).collect(); + f.added = 10; + f + }) .collect(), ..Default::default() }; - assert!(!snap.stats().oversized); + let (added, removed) = snap.totals(); + assert!( + snap.stats().retained_lines > (added + removed) as usize * 4, + "the context lines dominate, as they do in a real diff" + ); + assert!( + !snap.stats().oversized, + "an ordinary afternoon must not read as a tree too large to render \ + ({} retained lines)", + snap.stats().retained_lines + ); let none = HashMap::new(); assert!(snap.files.iter().all(|f| file_expanded(f, &none, false))); } - /// A clean diff with a huge untracked list is oversized too. `ls-files - /// --others` reaches the overlay without going through the diff at all, so - /// an un-ignored `node_modules` produced thousands of rows past every bound - /// the rest of this change added. + /// An empty snapshot means one of two opposite things, and the overlay has + /// to tell them apart before it says either out loud. + /// + /// "Working tree clean" is a claim about the repository. A probe that could + /// not run — a refused stream, a read that went silent, a git racing a + /// concurrent write — produces exactly the same empty file list, and saying + /// it there tells someone their changes are gone. #[test] - fn a_huge_untracked_list_is_oversized() { + fn an_empty_snapshot_reads_as_clean_only_when_the_probe_worked() { + let clean = DiffSnapshot { + branch: "main".into(), + ..Default::default() + }; + assert!(empty_snapshot(&clean)); + assert!(!clean.read_failed, "nothing went wrong: this tree is clean"); + + let broken = DiffSnapshot { + branch: "main".into(), + read_failed: true, + ..Default::default() + }; + assert!( + empty_snapshot(&broken), + "indistinguishable by shape — which is the point" + ); + assert!(broken.read_failed, "and distinguishable by this"); + + // A read that failed *after* producing something is not the empty case + // at all: the file list renders, and no claim about emptiness is made. + let partial = DiffSnapshot { + files: vec![small_file("one.rs", 3)], + read_failed: true, + ..Default::default() + }; + assert!(!empty_snapshot(&partial)); + } + + /// A huge untracked list does *not* collapse the diff — and must not. + /// + /// It is the same one-row-per-entry cost, but `oversized` is not the lever + /// that answers it: folding every file body shut leaves the untracked + /// section rendering exactly as many rows as before, because that section + /// has no bodies to fold. Driving it from here meant a tree with an + /// un-ignored `node_modules` and three edited files hid the three cheap + /// things, kept the expensive one, and told the reader their working tree + /// was too large to render. What actually bounds it is the retention cap and + /// the row cap, asserted below. + #[test] + fn a_huge_untracked_list_does_not_collapse_the_diff() { let snap = DiffSnapshot { files: vec![small_file("one.rs", 3)], untracked: (0..git_diff::MAX_UNTRACKED) @@ -1507,8 +1597,25 @@ mod tests { assert!(snap.stats().retained_lines < git_diff::AUTO_COLLAPSE_TOTAL_LINES); assert!(snap.files.len() < git_diff::AUTO_COLLAPSE_TOTAL_FILES); assert!( - snap.stats().oversized, - "the untracked list alone must trip it" + !snap.stats().oversized, + "collapsing the diff would not have removed a single untracked row" + ); + + // The bound that does apply, on the rows that are actually expensive. + assert_eq!( + snap.untracked.len(), + git_diff::MAX_UNTRACKED, + "retention is capped at the parser" + ); + assert_eq!( + snap.untracked.len().min(MAX_RENDERED_FILES), + MAX_RENDERED_FILES, + "and rows at the renderer" + ); + assert_eq!( + snap.untracked_count(), + 40_000, + "while the reported count stays the true total" ); }