starred/tty7
1
0
Fork 0
mirror of https://github.com/l0ng-ai/tty7.git synced 2026-09-22 00:02:23 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(diff): bound the stream queue, and stop two thresholds answering the wrong question

Browse Source 
Review follow-ups on the sidebar-diff branch. Five findings, four of them
about a bound that was claimed but not held.

The remote streaming read bounded both ends and not the middle. The reader
thread serves the whole connection, so it cannot wait on a slow consumer —
parking it there stalls every other reply and the keepalive with it — and an
unbounded queue was the price. That reassembles the whole diff in a channel,
which is the peak the buffered read was replaced to avoid, one container
further along. The queue is now bounded instead of back-pressured: each chunk
is charged to the stream's arrears, the drainer credits them back, and a
stream 32 MiB behind is cut loose with an error rather than served. Real
back-pressure would need credit-based flow control in the dialect; this is
not that, and says so.

`oversized` counted untracked paths on its file axis, and collapsing every
file body removes no untracked rows — that section has no bodies to fold. 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. The untracked list is bounded where it is built:
MAX_UNTRACKED on retention, MAX_RENDERED_FILES on rows.

AUTO_COLLAPSE_TOTAL_LINES counted the context lines git prints around every
hunk — four to six retained per line actually changed — against a threshold
set as if it were reading `+N -N`. It fired on trees whose header said 400.
8000, compared against the 20000 the parser stops retaining at, since
collapsing everything is the heavier of the two interventions and should not
arrive first by much.

A probe that could not run produced an empty file list, which renders as
"Working tree clean" — a claim about the repository, made because a read
timed out. Newly reachable, too: a stream can be refused or go silent where a
buffered read could only arrive or error. DiffSnapshot::read_failed keeps the
two apart.

Also: StreamStop's doc comment had been glued onto StreamSlot, leaving the
enum undocumented and the guard described twice; the overlay header asked
totals() beside stats() rather than through it; and the watch-teardown fix
riding along on this branch was in neither the PR body nor the CHANGELOG.

Tests: the queue budget both ways (a stream that outruns it is cut loose, a
larger one that is drained is not), the untracked axis, an ordinary
context-heavy afternoon sized to fail against the old threshold and pass
against the new, and empty-because-broken against empty-because-clean. Each
was checked to fail against the behaviour it replaces. 1603 pass, 0 fail;
fmt clean; no new clippy warnings in the touched files.
This commit is contained in:
l0ng-ai
2026-07-29 21:32:03 -07:00
parent af3b8b1373
commit 0ecaba115a
5 changed files with 457 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CHANGELOG.md
+27 -10
View File
@@ -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
crates/tty7-core/src/host/remote.rs
+238 -16
View File
@@ -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<GitStreamMsg>,
queued: &AtomicUsize,
idle: Duration,
on_line: &mut dyn FnMut(&str),
) -> io::Result<Option<i32>> {
@@ -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<i32>,
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<GitStreamMsg>,
/// 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<AtomicUsize>,
}
/// Receivers for git streams currently running on this connection, keyed by the
@@ -558,7 +633,7 @@ struct GitStreamRegistry {
#[derive(Default)]
struct StreamTable {
senders: HashMap<u64, mpsc::Sender<GitStreamMsg>>,
senders: HashMap<u64, StreamSink>,
/// 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<GitStreamMsg>) {
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(&registry);
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.
///
crates/tty7-core/src/host/server.rs
+5 -5
View File
@@ -1337,11 +1337,6 @@ fn spawn_watch_forwarder(id: u64, rx: smol::channel::Receiver<Vec<PathBuf>>, 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
src/terminal/git_diff.rs
+59 -14
View File
@@ -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<DiffSnapshot> {
// 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<DiffSnapshot> {
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);
src/ui/diff_overlay.rs
+128 -21
View File
@@ -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<Self>,
) -> 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<String> {
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"
);
}