From 566f69d786453e8ba6d14875483c957d672f7a6e Mon Sep 17 00:00:00 2001 From: Jonas Aasberg Date: Thu, 17 Sep 2026 17:43:04 +0200 Subject: [PATCH 1/4] fix(repo): route Cancel by what can actually stop the op MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Can this be cancelled" and "what cancels it" were two answers to one question — a set of cancellable kinds in `repoActivity`, and a hardcoded `cancelNetworkOps()` at each of the two surfaces offering the button. That is free to disagree, and the disagreement's shape is a Cancel that runs and stops nothing. They are one table now (`cancelPath`), and one store action picks the mechanism for both surfaces (`cancelActivity`). `isCancellable` is derived from the table rather than sitting beside it. **Why:** #474 adds a cancellable operation that is NOT a subprocess — a libgit2 revwalk, stopped by setting a flag it polls rather than by signalling a process group. Without this, the command palette would have offered "Cancel network operation" for a history search and called the network path, which reaches nothing. The palette gets its own row for it, with one label rather than two: a walk polls a flag, so asking twice does what asking once did, and borrowing the network row's "Force stop" would promise an escalation that has no counterpart here. Also resets `cancelRequested` in the palette test's `setRepo`. It was left standing between tests, which is how a test that never clicked anything read "Force stop network operation". Co-Authored-By: Claude Opus 5 (1M context) --- src/features/palette/commands.test.ts | 47 ++++++++++++++++-- src/features/palette/commands.ts | 31 +++++++++--- src/features/repo/activityView.test.tsx | 1 + src/features/repo/activityView.ts | 20 ++++++-- src/features/repo/repoActivity.ts | 65 ++++++++++++++++++------- src/features/repo/useRepoStore.ts | 57 +++++++++++++++++++++- 6 files changed, 188 insertions(+), 33 deletions(-) diff --git a/src/features/palette/commands.test.ts b/src/features/palette/commands.test.ts index 15f523de..9ebe7e91 100644 --- a/src/features/palette/commands.test.ts +++ b/src/features/palette/commands.test.ts @@ -52,6 +52,10 @@ function setRepo(partial: Record) { repoState: "Clean", rebaseStatus: { inProgress: false, nextIndex: 0, total: 0, pauseReason: null }, activity: {}, + // Reset explicitly, or a test that asks for a cancel leaks the flag into + // the next one — which is how "Cancel network operation" read "Force stop" + // in a test that never clicked anything. + cancelRequested: false, ...partial, } as never); } @@ -198,15 +202,17 @@ describe("buildCommands", () => { }); it("appears while a fetch is running and cancels it", () => { - const cancelNetworkOps = vi.fn(); - setRepo({ activity: { fetch: act("Fetching origin…") }, cancelNetworkOps }); + const cancelActivity = vi.fn(); + setRepo({ activity: { fetch: act("Fetching origin…") }, cancelActivity }); const item = buildCommands().find((i) => i.id === "action:cancel-network"); expect(item).toBeTruthy(); // Names what it would stop — there can be several ops in flight, and a // bare "Cancel" in a palette is a row nobody dares press. expect(item!.detail).toBe("Fetching origin…"); item!.run(); - expect(cancelNetworkOps).toHaveBeenCalled(); + // Through `cancelActivity`, which picks the mechanism from the same table + // the gate reads (#474) — not straight to the network path. + expect(cancelActivity).toHaveBeenCalledWith("fetch"); }); it("stays away from an op the backend cannot stop", () => { @@ -217,6 +223,41 @@ describe("buildCommands", () => { expect(ids()).not.toContain("action:cancel-network"); }); + // #474: `history` is cancellable, but NOT by `cancel_network_op` — there is + // no subprocess to signal. Before the mechanism became part of the same + // table, the gate said yes and the row called the network path, which is a + // Cancel that runs and stops nothing. + it("offers its own row for a history search, and routes it by kind", () => { + const cancelActivity = vi.fn(); + setRepo({ + activity: { history: act("Searching history for src/main.rs…") }, + cancelActivity, + }); + + expect(ids()).not.toContain("action:cancel-network"); + const item = buildCommands().find( + (i) => i.id === "action:cancel-history-search", + ); + expect(item).toBeTruthy(); + expect(item!.detail).toBe("Searching history for src/main.rs…"); + item!.run(); + expect(cancelActivity).toHaveBeenCalledWith("history"); + }); + + // A walk polls a flag; asking twice does what asking once did. Borrowing + // the network row's "Force stop" would promise an escalation that does not + // exist. + it("does not promise an escalation the walk has no counterpart for", () => { + setRepo({ + activity: { history: act("Searching history…") }, + cancelRequested: true, + }); + const item = buildCommands().find( + (i) => i.id === "action:cancel-history-search", + ); + expect(item!.label).toBe("Stop searching history"); + }); + it("relabels itself once a cancel has been asked for", () => { // The second ask escalates SIGTERM → SIGKILL (#263), so the first one has // to have visibly changed something — here as in the status bar. diff --git a/src/features/palette/commands.ts b/src/features/palette/commands.ts index d55bb97d..fc176334 100644 --- a/src/features/palette/commands.ts +++ b/src/features/palette/commands.ts @@ -26,7 +26,7 @@ import { usePaletteStore } from "./usePaletteStore"; import { createBranchInputStep, switchRepoStep } from "./steps"; import { currentBranch, isConflicted, relativeTime } from "@/lib/derive"; import { headUpstream, refreshOp, resolveConflictsOp } from "@/features/repo/ops"; -import { isCancellable, primaryActivity } from "@/features/repo/repoActivity"; +import { cancelPath, primaryActivity } from "@/features/repo/repoActivity"; import type { ActionId } from "@/features/keymap"; import type { BranchInfo, CommitInfo, FileStatus } from "@/lib/types"; import type { PaletteItem, PaletteStep } from "./types"; @@ -940,12 +940,29 @@ export function buildCommands(): PaletteItem[] { // is: a row that only ever answers "nothing to cancel" is noise the rest of // the time, and a Cancel row that does nothing is worse than none. // - // The gate is `isCancellable` — the same one the status bar uses — so the two - // surfaces cannot drift into offering different things. It is the set of ops - // that go through `run_git_authenticated`, i.e. exactly what - // `cancel_network_op` can reach. + // The gate is `cancelPath` — the same table the status bar uses — so the two + // surfaces cannot drift into offering different things, and since #474 it + // also says WHICH mechanism reaches the op: a network subprocess is signalled + // (`cancel_network_op`), an in-process walk is asked to notice + // (`cancel_walk`). Both rows below hand that decision to + // `repo.cancelActivity`; a row that picked for itself would be a Cancel that + // signals a process while a revwalk carried on. const running = primaryActivity(repo.activity); - if (running && isCancellable(running.key)) { + const cancelVia = running ? cancelPath(running.key) : null; + if (running && cancelVia === "walk") { + items.push({ + type: "command", id: "action:cancel-history-search", + search: "cancel stop abort history search file walk", + // ONE label, unlike the network row's two: a walk polls a flag between + // commits, so a second ask does exactly what the first did. There is no + // SIGTERM→SIGKILL escalation here to warn anyone about. + label: "Stop searching history", + detail: running.state.label, + icon: "x", danger: true, + run: direct(() => void repo.cancelActivity(running.key)), + }); + } + if (running && cancelVia === "network") { items.push({ type: "command", id: "action:cancel-network", search: "Cancel stop abort network operation fetch pull push force", @@ -961,7 +978,7 @@ export function buildCommands(): PaletteItem[] { // showing is the least surprising thing it can say. detail: running.state.label, icon: "x", danger: true, - run: direct(() => void repo.cancelNetworkOps()), + run: direct(() => void repo.cancelActivity(running.key)), }); } diff --git a/src/features/repo/activityView.test.tsx b/src/features/repo/activityView.test.tsx index e549a17e..4d982f41 100644 --- a/src/features/repo/activityView.test.tsx +++ b/src/features/repo/activityView.test.tsx @@ -37,6 +37,7 @@ const CASES: Record = { action: "Running Format all…", lfs: "Fetching LFS objects…", submodule: "Updating submodules…", + history: "Searching history for src/main.rs…", }; function prime(activity: RepoActivity) { diff --git a/src/features/repo/activityView.ts b/src/features/repo/activityView.ts index 4ba4ff1e..b446989c 100644 --- a/src/features/repo/activityView.ts +++ b/src/features/repo/activityView.ts @@ -23,7 +23,7 @@ import React from "react"; import { activityCount, - isCancellable, + cancelPath, primaryActivity, type ActivityKey, type ActivityState, @@ -63,13 +63,23 @@ export interface ActivityView { export function useActivityView(): ActivityView | null { const activity = useRepoStore((s) => s.activity); const cancelRequested = useRepoStore((s) => s.cancelRequested); + + // `primaryActivity` is a pure pick over state already read above — not a hook + // — so it is free to run before the early return, and `cancel` needs it: the + // two cancellation mechanisms reach different things, and sending a click to + // the wrong one is a Cancel button that does nothing. + const primary = primaryActivity(activity); + const path = primary ? cancelPath(primary.key) : null; + // Above the early return, or a surface that mounts while idle and then sees an // operation start renders a different number of hooks on its second pass. + // `cancelActivity` picks the mechanism, so this hook and the command palette + // cannot drift into cancelling different things for the same entry. + const key = primary?.key; const cancel = React.useCallback(() => { - void useRepoStore.getState().cancelNetworkOps(); - }, []); + if (key) void useRepoStore.getState().cancelActivity(key); + }, [key]); - const primary = primaryActivity(activity); if (!primary) return null; return { @@ -78,7 +88,7 @@ export function useActivityView(): ActivityView | null { ? { ...primary.state, label: "Cancelling…", percent: undefined } : primary.state, others: activityCount(activity) - 1, - cancellable: isCancellable(primary.key), + cancellable: path !== null, cancelRequested, cancel, }; diff --git a/src/features/repo/repoActivity.ts b/src/features/repo/repoActivity.ts index afd3f98a..ed796658 100644 --- a/src/features/repo/repoActivity.ts +++ b/src/features/repo/repoActivity.ts @@ -76,6 +76,19 @@ export interface RepoActivity { * from one that never started. */ action?: ActivityState; + /** + * Searching one file's history (#474). + * + * The first entry here that is not a subprocess and not a mutation — it is a + * libgit2 revwalk, and it earns an entry for the one reason this module + * exists: on a large repository it is a wait long enough to need stopping. + * `file_history` walks from HEAD comparing trees at the path, so before the + * visit cap a rarely-touched file walked to the root of history — ~1.5 + * million commits on `torvalds/linux`, for a file the user clicked once, with + * a spinner and no way out. The cap bounds it; this is what makes the bounded + * wait visible and cancellable. + */ + history?: ActivityState; } export type ActivityKey = keyof RepoActivity; @@ -99,6 +112,9 @@ export const ACTIVITY_PRIORITY: readonly ActivityKey[] = [ "stash", "branch", "forge", + // Above difftool because the app really is busy here, below the git ops + // because a push the user started outranks a screen filling in behind it. + "history", "difftool", // Below difftool: like it, the app is not busy — someone else's program is — // and any real git op running underneath is the more urgent thing to say. @@ -124,25 +140,40 @@ export function activityCount(activity: RepoActivity): number { } /** - * The kinds `cancelNetworkOps` can actually stop. + * How a kind of operation is stopped — `null` for one that cannot be. + * + * ONE table rather than a set plus a branch at the button, because "can this be + * cancelled" and "what cancels it" are the same question asked twice, and two + * answers are free to disagree. `useActivityView` reads it to decide both. * - * Everything here runs as a `git` subprocess through - * `commands::net::run_git_authenticated`, which registers it under - * `cancel::Scope::Repo` — so `cancel_network_op` reaches it. The rest are - * libgit2 work inside one blocking call with nothing to signal: a rebase replay - * cannot be interrupted at all yet (#296 gap 6), and a checkout or stash is over - * before a button could be found. Offering Cancel on those would be a button - * that does nothing, which is worse than no button. + * * `"network"` — a `git` subprocess through + * `commands::net::run_git_authenticated`, registered under + * `cancel::Scope::Repo` and stopped by `cancel_network_op` signalling its + * process group. + * * `"walk"` — libgit2 inside one blocking call, registered under + * `cancel::Scope::Walk` and stopped by `cancel_walk` setting a flag the walk + * polls between commits (#474). There is no process to signal. + * + * Everything absent is libgit2 work with nothing to poll: a rebase replay + * cannot be interrupted at all yet (#296 gap 6), and a checkout or stash is + * over before a button could be found. Offering Cancel on those would be a + * button that does nothing, which is worse than no button. */ -const CANCELLABLE: ReadonlySet = new Set([ - "fetch", - "pull", - "push", - "lfs", - "submodule", - "forge", -]); +const CANCEL_PATH: Partial> = { + fetch: "network", + pull: "network", + push: "network", + lfs: "network", + submodule: "network", + forge: "network", + history: "walk", +}; + +/** What stops an operation of this kind, or null when nothing does. */ +export function cancelPath(key: ActivityKey): "network" | "walk" | null { + return CANCEL_PATH[key] ?? null; +} export function isCancellable(key: ActivityKey): boolean { - return CANCELLABLE.has(key); + return cancelPath(key) !== null; } diff --git a/src/features/repo/useRepoStore.ts b/src/features/repo/useRepoStore.ts index 98d3a7f1..62022785 100644 --- a/src/features/repo/useRepoStore.ts +++ b/src/features/repo/useRepoStore.ts @@ -97,6 +97,7 @@ import { runMergetool as runMergetoolFn, restartConflict as restartConflictFn, cancelNetworkOp, + cancelWalk, rememberCredential, setRemoteUrl, setUpstream as setUpstreamFn, @@ -135,7 +136,7 @@ import { sliceOf, type RepoSlice, } from "./repoSlice"; -import type { ActivityKey } from "./repoActivity"; +import { cancelPath, type ActivityKey } from "./repoActivity"; import { resolveUpdateRefs } from "@/features/commits/stackedRefs"; import { checkUndo, @@ -507,6 +508,31 @@ interface RepoStoreState extends RepoSlice { * `setErrorFor` drops and each `finally` clears the spinner for. */ cancelNetworkOps: () => Promise; + /** + * Stop the in-process history walks running on this repository (#474). + * + * `cancelNetworkOps` for the other kind of long operation. A `file_history` + * walk is libgit2 inside one blocking call with no subprocess to signal, so + * the backend sets a flag the walk polls between commits and the call returns + * `Cancelled` — which `setErrorFor` drops like any other cancellation. + * + * Repository-wide for the same reason network cancellation is: there is + * nothing finer for the user to point at, and two windows on one repository + * queue their walks behind each other anyway. + */ + cancelWalks: () => Promise; + /** + * Stop the running operation of this KIND, by whatever mechanism reaches it + * (#474). + * + * The ONE place the two cancellation paths are chosen between, because there + * are two surfaces offering Cancel — the status bar / history strip via + * `useActivityView`, and the command palette — and a surface that picked for + * itself would be a Cancel button that signals a process while a revwalk + * carries on. `cancelPath` decides; a kind nothing can reach is a no-op, + * which no caller should reach because both gate on the same table. + */ + cancelActivity: (key: ActivityKey) => Promise; // remote management addRemote: (name: string, url: string) => Promise; removeRemote: (name: string) => Promise; @@ -2218,6 +2244,35 @@ export const useRepoStore = create((set, get) => { }); }, + async cancelWalks() { + const repo = get().current; + if (!repo) return; + // Same intent flag as `cancelNetworkOps`, for the same reason: the status + // line has to show that the click landed. There is no SIGTERM→SIGKILL + // escalation behind this one — a walk polls a flag and stops at the next + // commit — but the label is what tells the user to stop clicking, and the + // two surfaces must not say different things about the same button. + set({ cancelRequested: true }); + await cancelWalk(repo.id).catch((e) => { + // The walk finishing first is the common way this "fails", and it is the + // outcome the click wanted. A real failure is worth a banner. + setErrorFor(repo.id, e); + }); + }, + + async cancelActivity(key) { + switch (cancelPath(key)) { + case "walk": + return get().cancelWalks(); + case "network": + return get().cancelNetworkOps(); + default: + // Nothing reaches this kind. Both surfaces gate on the same table, so + // this arm is unreachable by construction rather than by luck. + return; + } + }, + async addRemote(name, url) { const repo = get().current; if (!repo) return; From 3a73bd664cf0e7ecef0630854c8fbe6f8b9a9165 Mon Sep 17 00:00:00 2001 From: Jonas Aasberg Date: Thu, 17 Sep 2026 17:43:30 +0200 Subject: [PATCH 2/4] fix(history): cap the file-history walk, share the lock, allow a cancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #474. `file_history` walked from HEAD keeping commits whose tree differs from their parent's at the path, and stopped at `limit` MATCHES. A file with fewer changes than the limit had nothing to stop on, so the walk ran to the root of history with a tree comparison at every commit. Measured on torvalds/linux, warm, for one click: 135.6 s and 1,482,923 tree comparisons. That is most files in most large repositories, not a corner case. Three things, matching the issue: * **A visit cap beside the match cap**, defaulting to 50,000 commits, and the result now says which ceiling ended the walk (`HistoryStop`) plus how many commits it examined. The screen says it out loud — "Searched the newest 50,000 commits" — and offers "Search all of history", which is the old unbounded walk asked for deliberately. A cap nobody mentions is the silent wrong answer this area exists to avoid, and the number shown comes off the wire so it cannot drift from the policy applied. * **`with_repo_read`.** This is the longest read in the backend and it held the EXCLUSIVE lock for all of it, so one click queued every other operation on the repository behind a walk that could take minutes — the exact failure #400 set out to remove, surviving in the op least able to afford it. It writes nothing: a revwalk, `find_commit`, `Tree::get_path`, and a tree-to-tree diff in the pathspec case. Notably it never reads the index, so there is no accidental refresh for anything else to depend on. * **Cancellable**, because 18 s is still a wait worth being able to stop. A new `cancel::Scope::Walk` — the first scope with nothing to signal, so the command hands the backend a predicate to poll between commits and the backend knows nothing about cancellation. The screen also cancels on the way out, so leaving does not leave a walk running. **Why the sort order is untouched:** dropping `TOPOLOGICAL` looks like the obvious fix and buys nothing. libgit2's sorted revwalk pre-walks the whole graph before yielding anything — 14.2 s to the first oid, 14.7 s for all 1.48 million, measured within 1% for TIME and TIME|TOPOLOGICAL alike. `Sort::NONE` IS incremental and is unusable here: it yields commits in traversal order, so the first 50,000 are not the newest 50,000, and a capped walk could miss last week's change while reporting one from 2011. The cap has to mean "the newest N commits" or it cannot be said out loud. Co-Authored-By: Claude Opus 5 (1M context) --- src-tauri/benches/repo_bench.rs | 40 +++-- src-tauri/src/cancel.rs | 40 ++++- src-tauri/src/commands/commits.rs | 67 +++++++- src-tauri/src/git/cli.rs | 6 +- src-tauri/src/git/libgit2.rs | 54 +++++- src-tauri/src/git/mod.rs | 44 ++++- src-tauri/src/git/types.rs | 38 +++++ src-tauri/src/lib.rs | 1 + src-tauri/tests/embedded_repo.rs | 8 +- src-tauri/tests/file_history.rs | 256 ++++++++++++++++++++++++++++- src/lib/derive.fileHistory.test.ts | 93 +++++++++++ src/lib/derive.ts | 57 +++++++ src/lib/tauri.ts | 49 +++++- src/lib/types.ts | 32 ++++ src/screens/FileHistory.test.tsx | 187 +++++++++++++++++++++ src/screens/FileHistory.tsx | 184 +++++++++++++++++++-- 16 files changed, 1107 insertions(+), 49 deletions(-) create mode 100644 src/lib/derive.fileHistory.test.ts create mode 100644 src/screens/FileHistory.test.tsx diff --git a/src-tauri/benches/repo_bench.rs b/src-tauri/benches/repo_bench.rs index 87dd39a9..22d8f98d 100644 --- a/src-tauri/benches/repo_bench.rs +++ b/src-tauri/benches/repo_bench.rs @@ -260,16 +260,20 @@ struct Subject { head_oid: String, /// The path changed most often in recent history. /// - /// NOT simply "a path HEAD touched", which is the obvious choice and is - /// unusable: `file_history` filters a walk by path and stops at `limit` - /// matches, so on a rarely-touched file it never reaches 500 and walks the - /// WHOLE history with a tree comparison per commit. On `torvalds/linux` - /// that is 1.4 million commits and the benchmark simply does not finish. + /// NOT simply "a path HEAD touched", which is the obvious choice and was + /// unusable: `file_history` filters a walk by path and stopped at `limit` + /// matches only, so on a rarely-touched file it never reached 500 and + /// walked the WHOLE history with a tree comparison per commit. On + /// `torvalds/linux` that is 1.48 million commits, and the benchmark simply + /// did not finish — which is how #474 was found. /// - /// A hot file is also the honest subject: file history is a thing people - /// open on files that change. The pathological case is real and is written - /// up in `docs/dev/performance.md` rather than measured here, because - /// "unbounded" is not a number. + /// The visit cap (#474) bounds that case now: measured at 135.6 s uncapped + /// against 18.3 s capped, both in `docs/dev/performance.md`. This is still + /// the hot path, for two reasons that outlived the bug. A hot file is the + /// honest subject — file history is a thing people open on files that change + /// — and it is the only subject whose cost is a property of the REPOSITORY + /// rather than of the cap: a cold path now measures the cap, which is a + /// constant, so the number would stop tracking anything about the fixture. hot_path: Option, /// A path the working tree has modified, if any. `None` on a clean fixture, /// which simply skips the worktree diff. @@ -724,10 +728,22 @@ fn run_suite(subject: &Subject, cfg: &Config) -> Vec { "History of one file (500 commits)", subject, cfg, - |v: &Vec| { - format!("{} commits", thousands(v.len())) + |v: &platypusgit_lib::git::types::FileHistory| { + format!("{} commits", thousands(v.commits.len())) + }, + // The DEFAULT visit cap, because this measures what a click + // costs a user (#474) — not `None`, which is the uncapped walk + // that never finished here and is the reason the cap exists. + move |b, id| { + b.file_history( + id, + &for_history, + PAGE_SIZE, + Some(platypusgit_lib::git::FILE_HISTORY_VISIT_LIMIT), + &|| false, + ) + .expect("file_history") }, - move |b, id| b.file_history(id, &for_history, PAGE_SIZE).expect("file_history"), ), // Deliberately NOT `--follow`. `Libgit2Backend::file_history` is a // plain path filter over the walk — it does not detect renames — so diff --git a/src-tauri/src/cancel.rs b/src-tauri/src/cancel.rs index 4445ac88..a21f3256 100644 --- a/src-tauri/src/cancel.rs +++ b/src-tauri/src/cancel.rs @@ -1,4 +1,10 @@ -//! Cancelling an in-flight network git subprocess (#234, hardened by #263). +//! Cancelling an operation in flight (#234, hardened by #263, widened by #474). +//! +//! Two kinds of work end up here, and they stop in completely different ways: a +//! network **subprocess**, which is signalled, and a long in-process libgit2 +//! **walk**, which is asked to notice. One registry for both, because "what is +//! running that the user might want to stop" is one question — see +//! [`Scope::Walk`] for what the second kind does not get. //! //! Before this module a clone, fetch, pull or push that hung could only be //! escaped by force-quitting the app — `run_clone` said so in a comment, and @@ -13,6 +19,15 @@ //! would be a network op nobody can stop. A new network op inherits //! cancellation by using `run_git_authenticated`, with nothing to remember. //! +//! The in-process walk added by #474 registers at its own choke point, +//! `commands::commits::file_history`, and the command passes the registration's +//! [`Registration::is_cancelled`] into the backend as the predicate the walk +//! polls. The backend therefore knows nothing about this module: it is handed a +//! `&dyn Fn() -> bool` and asks it between commits. That is what keeps the +//! scope-to-path matching in ONE place — the command resolves the path with +//! `get_repo_path`, exactly as `cancel_walk` does, so the two cannot drift into +//! addressing different scopes. +//! //! ## Scope, not op id //! //! An op is registered under a [`Scope`] — "the clone", or "this repository" — @@ -94,6 +109,24 @@ pub enum Scope { /// the ops themselves get their `cwd` — the two must keep agreeing, or a /// cancel would silently match nothing. Repo(PathBuf), + /// Every long in-process libgit2 walk on the repository at this path + /// (#474) — today `file_history`. + /// + /// **The one scope with nothing to signal.** There is no subprocess: the + /// work is a revwalk on a blocking thread, so [`cancel`] can only mark the + /// entry and the walk itself has to notice — which it does by polling + /// [`Registration::is_cancelled`] between commits. These entries never + /// `attach` a pid, so `kill_tree` is unreachable for them and the + /// SIGTERM→SIGKILL escalation is meaningless here: a second click sets the + /// same flag the first one did. + /// + /// Separate from `Repo` rather than folded into it, even though both are + /// keyed by the same path. Cancelling a stalled fetch must not also abandon + /// a history search the user is waiting on, and cancelling the search must + /// not kill the fetch: they are different waits, and the status bar offers + /// Cancel for one of them at a time. `cancel_all` reaches both, which is + /// what a closing window wants. + Walk(PathBuf), } impl Scope { @@ -101,6 +134,11 @@ impl Scope { pub fn repo(cwd: &Path) -> Self { Scope::Repo(cwd.to_path_buf()) } + + /// The scope for an in-process walk on the repository at `cwd`. + pub fn walk(cwd: &Path) -> Self { + Scope::Walk(cwd.to_path_buf()) + } } /// A live op's entry in the registry. Removed on drop, so a finished op cannot diff --git a/src-tauri/src/commands/commits.rs b/src-tauri/src/commands/commits.rs index aa421156..9a45d876 100644 --- a/src-tauri/src/commands/commits.rs +++ b/src-tauri/src/commands/commits.rs @@ -3,8 +3,8 @@ use tauri::State; use crate::{ error::{AppError, AppResult}, git::types::{ - AheadBehind, AuthorOverride, CommitInfo, CommitNote, CommitOptions, CommitResult, LogFilter, - LogPage, RepoId, + AheadBehind, AuthorOverride, CommitInfo, CommitNote, CommitOptions, CommitResult, + FileHistory, LogFilter, LogPage, RepoId, }, state::AppState, }; @@ -317,17 +317,74 @@ pub async fn commit_notes( .map_err(|e| AppError::Internal(e.to_string()))? } +/// The commits that touched one path, newest first — bounded and cancellable +/// (#474). +/// +/// This command owns the two things the backend deliberately does not: the +/// default visit cap, and the walk's registration with `cancel`. +/// +/// `search_all` waives the cap. Absent means "no" on purpose: the uncapped walk +/// is the one that took 135 s on `torvalds/linux` for a single click, so it has +/// to be asked for, and the only thing that asks is the notice's own button +/// after the user has read what the cap did. #[tauri::command] pub async fn file_history( state: State<'_, AppState>, repo_id: String, path: String, limit: usize, -) -> AppResult> { + search_all: Option, +) -> AppResult { let backend = state.backend.clone(); let repo_id = RepoId(repo_id); let path = std::path::PathBuf::from(path); - tokio::task::spawn_blocking(move || backend.file_history(&repo_id, &path, limit)) + let visit_limit = if search_all.unwrap_or(false) { + None + } else { + Some(crate::git::FILE_HISTORY_VISIT_LIMIT) + }; + + // The same path `cancel_walk` resolves, from the same function, so the two + // cannot address different scopes — the agreement `cancel`'s module docs + // require. Momentarily exclusive (`repo_path` goes through `with_repo`), + // which the walk itself is not. + let path_backend = state.backend.clone(); + let path_id = repo_id.clone(); + let cwd = tokio::task::spawn_blocking(move || path_backend.repo_path(&path_id)) .await - .map_err(|e| AppError::Internal(e.to_string()))? + .map_err(|e| AppError::Internal(e.to_string()))??; + + // Registered before the walk starts and deregistered when the guard drops + // with the blocking task: `cancel_walk` can only reach an op on the + // register, and an entry outliving its walk would let a later click + // "cancel" a walk that had already finished. + let registration = crate::cancel::register(crate::cancel::Scope::walk(&cwd)); + // Cancelled between the click that started this and here — do not begin a + // walk nobody is waiting for. Same guard, same reason, as `run_clone`'s. + if registration.is_cancelled() { + return Err(AppError::Cancelled); + } + + tokio::task::spawn_blocking(move || { + let cancelled = || registration.is_cancelled(); + backend.file_history(&repo_id, &path, limit, visit_limit, &cancelled) + }) + .await + .map_err(|e| AppError::Internal(e.to_string()))? +} + +/// Stop the in-process walks running on one repository (#474). +/// +/// The sibling of `cancel_network_op` for work that has no subprocess: this +/// marks the registry entry, and the walk notices between commits and returns +/// `Cancelled`. Answers how many walks were signalled; zero is a normal answer, +/// because the walk can finish between the user clicking Cancel and this call. +#[tauri::command] +pub async fn cancel_walk(state: State<'_, AppState>, repo_id: String) -> AppResult { + let backend = state.backend.clone(); + let id = RepoId(repo_id); + let path = tokio::task::spawn_blocking(move || backend.repo_path(&id)) + .await + .map_err(|e| AppError::Internal(e.to_string()))??; + Ok(crate::cancel::cancel(&crate::cancel::Scope::walk(&path))) } diff --git a/src-tauri/src/git/cli.rs b/src-tauri/src/git/cli.rs index f7726773..d1d5f67f 100644 --- a/src-tauri/src/git/cli.rs +++ b/src-tauri/src/git/cli.rs @@ -7,7 +7,7 @@ use super::{ AheadBehind, BisectMark, BisectStatus, BlameResult, BranchInfo, CommitInfo, CommitNote, CommitOptions, CommitResult, ConflictSides, DeleteFailure, DiffKind, DiffToolTarget, FileContent, BulkFastForward, FastForward, - FileDiff, FileStatus, HeadInfo, LfsStatus, LogFilter, LogPage, RebaseProgressSink, RebaseStatus, RebaseStep, ReflogEntry, + FileDiff, FileHistory, FileStatus, HeadInfo, LfsStatus, LogFilter, LogPage, RebaseProgressSink, RebaseStatus, RebaseStep, ReflogEntry, BlobSource, ImagePreview, RemoteInfo, RepoHandle, RepoId, RepoState, ResetMode, ShallowInfo, StashInfo, StashSaveOptions, @@ -108,7 +108,9 @@ impl GitBackend for CliBackend { _repo_id: &RepoId, _path: &Path, _limit: usize, - ) -> AppResult> { + _visit_limit: Option, + _cancelled: &dyn Fn() -> bool, + ) -> AppResult { Err(AppError::NotImplemented) } fn diff_over_ceiling( diff --git a/src-tauri/src/git/libgit2.rs b/src-tauri/src/git/libgit2.rs index d08e9267..7698121b 100644 --- a/src-tauri/src/git/libgit2.rs +++ b/src-tauri/src/git/libgit2.rs @@ -28,7 +28,8 @@ use super::{ BlobSource, DiffKind, DiffToolTarget, - DiffLine, DiffLineKind, FastForward, FileContent, FileDiff, FileStatus, HeadInfo, ImagePreview, + DiffLine, DiffLineKind, FastForward, FileContent, FileDiff, FileHistory, FileStatus, + HeadInfo, HistoryStop, ImagePreview, LfsStatus, LogFilter, LogPage, OversizedBlob, @@ -7177,13 +7178,26 @@ impl GitBackend for Libgit2Backend { }) } + /// See the trait docs for the two ceilings and why the second one exists. + /// + /// **A READ** (`with_repo_read`, #400/#474). Everything below is object + /// reads — a revwalk over the refs, `find_commit`, `Tree::get_path`, and in + /// the pathspec case a tree-to-tree diff. Nothing touches the index, the + /// worktree, a ref or the odb's write side, so there is no write here to + /// interleave with another one. That matters more for this op than for most: + /// it is the longest read in the backend, and holding the exclusive lock for + /// it queued every other operation on the repository behind a walk that + /// could take minutes — the exact failure the read-concurrency work set out + /// to remove. fn file_history( &self, repo_id: &RepoId, path: &Path, limit: usize, - ) -> AppResult> { - self.with_repo(repo_id, |repo| { + visit_limit: Option, + cancelled: &dyn Fn() -> bool, + ) -> AppResult { + self.with_repo_read(repo_id, |repo| { // Without this the walk runs to completion and returns 0 commits — // indistinguishable from a file that genuinely has no history. reject_embedded_repo(repo, path)?; @@ -7195,20 +7209,50 @@ impl GitBackend for Libgit2Backend { Err(e.into()) } })?; + // TIME|TOPOLOGICAL, like every other walk in this file, and + // deliberately not the cheaper `Sort::NONE` (#474). libgit2's sorted + // revwalk pre-walks the whole graph before it yields anything — + // measured at ~14.5 s on `torvalds/linux`, identically for TIME and + // for TIME|TOPOLOGICAL, so the sort order is not what made this + // slow and dropping TOPOLOGICAL buys nothing. `Sort::NONE` IS + // incremental, and unusable: it yields commits in the order the + // traversal reaches them, so the first 50,000 of them are not the + // newest 50,000 and a capped walk could miss last week's change to + // the file while reporting one from 2011. The cap has to mean + // "the newest N commits" or it cannot be said out loud. revwalk.set_sorting(git2::Sort::TIME | git2::Sort::TOPOLOGICAL)?; - let mut out = Vec::with_capacity(limit); + let cap = visit_limit.unwrap_or(usize::MAX); + let mut out = Vec::with_capacity(limit.min(1024)); + let mut visited = 0usize; + let mut stopped_at = HistoryStop::Exhausted; for oid_res in revwalk { if out.len() >= limit { + stopped_at = HistoryStop::MatchLimit; + break; + } + if visited >= cap { + stopped_at = HistoryStop::VisitLimit; break; } + // Between commits rather than per tree lookup: the check takes a + // mutex, and at ~82 µs of work per commit this is already a + // sub-millisecond response to a click. + if cancelled() { + return Err(AppError::Cancelled); + } let oid = oid_res?; let commit = repo.find_commit(oid)?; + visited += 1; if commit_touches_path(repo, &commit, path)? { out.push(commit_to_info(&commit)); } } - Ok(out) + Ok(FileHistory { + commits: out, + visited, + stopped_at, + }) }) } diff --git a/src-tauri/src/git/mod.rs b/src-tauri/src/git/mod.rs index 7ae1015f..ba96e31e 100644 --- a/src-tauri/src/git/mod.rs +++ b/src-tauri/src/git/mod.rs @@ -31,7 +31,7 @@ use types::{ BisectMark, BisectStatus, BlameResult, BranchInfo, CommitInfo, CommitNote, CommitOptions, CommitResult, ConflictSides, DeleteFailure, DiffKind, DiffToolTarget, FileContent, BulkFastForward, FastForward, - FileDiff, FileStatus, HeadInfo, LfsStatus, LogFilter, LogPage, RebaseProgressSink, RebaseStatus, RebaseStep, ReflogEntry, + FileDiff, FileHistory, FileStatus, HeadInfo, LfsStatus, LogFilter, LogPage, RebaseProgressSink, RebaseStatus, RebaseStep, ReflogEntry, BlobSource, ImagePreview, RemoteInfo, RepoHandle, @@ -68,6 +68,24 @@ pub fn repo_path_key(path: &Path) -> PathBuf { PathBuf::from(trimmed) } +/// How many commits a default `file_history` walk will look at (#474). +/// +/// The number is a budget on the walk's MARGINAL cost, which is the part a cap +/// can control. Measured on `torvalds/linux` (`docs/dev/performance.md`): +/// libgit2's sorted revwalk pre-walks the whole graph before yielding anything, +/// ~14.5 s that no cap can remove, and each visited commit then costs ~82 µs of +/// tree comparison. So 50,000 visits is ~4 s of work on top of the floor, and +/// the uncapped walk that this replaces was ~1.5 million of them. +/// +/// 50,000 is also the scale the rest of this repo already treats as "large": it +/// is the `deep` benchmark fixture's commit count, chosen because it is where +/// other git GUIs are reported to fall over. Under it — which is nearly every +/// repository — this cap never fires and changes nothing. +/// +/// ONE definition, read by `commands::commits::file_history`. The number the +/// user is shown is the `visited` count that comes back, never this constant +/// re-spelled in the frontend, so the sentence cannot drift from the policy. +pub const FILE_HISTORY_VISIT_LIMIT: usize = 50_000; pub trait GitBackend: Send + Sync { // === existing reads === @@ -176,13 +194,33 @@ pub trait GitBackend: Send + Sync { /// revspec cannot be resolved to a commit; unrelated histories are a /// `merge_base: None`, not an error. fn ahead_behind(&self, repo_id: &RepoId, a: &str, b: &str) -> AppResult; - /// Commits that touched `path`, newest first, up to `limit`. + /// Commits that touched `path`, newest first, up to `limit` — and what + /// stopped the walk (#474). + /// + /// **Two ceilings, because one is not enough.** `limit` bounds the MATCHES; + /// `visit_limit` bounds how many commits are LOOKED AT. Without the second + /// one, a file with fewer changes than `limit` has nothing to stop on and + /// the walk runs to the root of history doing a tree comparison at every + /// commit — ~1.5 million of them on `torvalds/linux`, for a file the user + /// clicked once, which is most files in most large repositories rather than + /// a corner case. + /// + /// `visit_limit: None` is that unbounded walk, and it exists so the user who + /// read the notice can ask for it deliberately. It is never the default. + /// + /// `cancelled` is polled as the walk runs; `Cancelled` when it answers + /// true. A capped walk on a kernel-sized repository is still seconds + /// (`docs/dev/performance.md` has the figures), and this is the only way out + /// of one that is no longer wanted — nothing here is a subprocess, so there + /// is nothing for `cancel::cancel` to signal. fn file_history( &self, repo_id: &RepoId, path: &Path, limit: usize, - ) -> AppResult>; + visit_limit: Option, + cancelled: &dyn Fn() -> bool, + ) -> AppResult; /// Blame `path` as of HEAD (#253). /// /// `ignore_revs` selects between the two views a repository with a diff --git a/src-tauri/src/git/types.rs b/src-tauri/src/git/types.rs index 146d53bc..64bdd3c8 100644 --- a/src-tauri/src/git/types.rs +++ b/src-tauri/src/git/types.rs @@ -92,6 +92,44 @@ pub struct LogPage { pub next_cursor: Option>, } +/// Why a file-history walk stopped (#474). +/// +/// The walk has two independent ceilings — how many MATCHES to collect and how +/// many commits to LOOK AT — and which of them ended it changes what the list +/// in front of the user means. A list of five commits is either the file's +/// complete history, or the newest five of many, or every match in the part of +/// history that was searched with nothing said about the rest. Before #474 all +/// three were the same answer: a list that simply ended. +#[derive(Debug, Clone, Copy, Serialize, PartialEq, Eq)] +pub enum HistoryStop { + /// The walk reached the end of history. The list is complete. + Exhausted, + /// `limit` matches were collected. Older changes to the path exist. + MatchLimit, + /// The VISIT cap was reached first — the walk looked at `visited` commits + /// and stopped, so nothing is known about anything older. + VisitLimit, +} + +/// One file's history, with what bounded it (#474). +/// +/// Not a bare `Vec`, for the same reason `WorkingTreeDiff` is not a +/// bare `Vec`: this walk is BOUNDED and the bound has to be visible. +#[derive(Debug, Clone, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct FileHistory { + /// Commits that touched the path, newest first. + pub commits: Vec, + /// How many commits the walk examined to find them. + /// + /// Reported rather than left implicit because it is the number the notice + /// puts in front of the user ("searched the newest 50,000 commits"), and a + /// copy of the cap in the frontend would be free to drift from the one that + /// was applied. + pub visited: usize, + pub stopped_at: HistoryStop, +} + /// A whole-tree diff against the WORKING TREE (#131). /// /// Not a bare `Vec` because the untracked side has to be BOUNDED and diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index ccebaf33..7e502f8c 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -383,6 +383,7 @@ pub fn run() { commands::commits::format_patch, commands::branches::push_commit, commands::commits::file_history, + commands::commits::cancel_walk, commands::commits::verify_commit, commands::commits::commit_notes, commands::commits::get_commit_template, diff --git a/src-tauri/tests/embedded_repo.rs b/src-tauri/tests/embedded_repo.rs index 4a611d6e..84a0bcdb 100644 --- a/src-tauri/tests/embedded_repo.rs +++ b/src-tauri/tests/embedded_repo.rs @@ -248,7 +248,7 @@ fn file_history_rejects_the_embedded_repo_instead_of_returning_nothing() { for path in ["vendor/lib/", "vendor/lib"] { let err = backend - .file_history(&handle.id, &PathBuf::from(path), 50) + .file_history(&handle.id, &PathBuf::from(path), 50, None, &|| false) .unwrap_err(); assert!(matches!(err, AppError::EmbeddedRepo(p) if p == path)); } @@ -267,7 +267,11 @@ fn guards_leave_ordinary_paths_alone() { .is_ok()); assert!(backend.blame_file(&handle.id, &readme, true).is_ok()); assert_eq!( - backend.file_history(&handle.id, &readme, 50).unwrap().len(), + backend + .file_history(&handle.id, &readme, 50, None, &|| false) + .unwrap() + .commits + .len(), 1 ); assert!(backend.stage(&handle.id, &[readme]).is_ok()); diff --git a/src-tauri/tests/file_history.rs b/src-tauri/tests/file_history.rs index 8b4743b5..48f377e3 100644 --- a/src-tauri/tests/file_history.rs +++ b/src-tauri/tests/file_history.rs @@ -1,9 +1,30 @@ +//! `file_history` — what it finds, and what stops it looking (#474). +//! +//! The walk has two ceilings and it has to say which one ended it, because the +//! three outcomes look identical on screen and mean completely different +//! things. Before #474 there was only the MATCH ceiling, so a file with fewer +//! changes than the limit had nothing to stop on and the walk ran to the root +//! of history — measured at 135 s and 1,482,923 tree comparisons on +//! `torvalds/linux` for one click (`docs/dev/performance.md`), with the +//! exclusive repository lock held for all of it. + mod support; +use std::path::Path; +use std::sync::{Arc, Condvar, Mutex}; +use std::time::Duration; + +use platypusgit_lib::error::AppError; +use platypusgit_lib::git::types::HistoryStop; use platypusgit_lib::git::GitBackend; use support::fs::write_file; use support::TempRepo; +/// Never cancelled — the ordinary case, spelled once. +fn live() -> impl Fn() -> bool { + || false +} + #[test] fn file_history_returns_commits_that_touched_the_path() { let tr = TempRepo::with_initial_commit("hello\n"); @@ -22,9 +43,240 @@ fn file_history_returns_commits_that_touched_the_path() { tr.commit_all("edit foo"); let history = backend - .file_history(&handle.id, std::path::Path::new("foo.txt"), 100) + .file_history(&handle.id, Path::new("foo.txt"), 100, None, &live()) .unwrap(); - let summaries: Vec<&str> = history.iter().map(|c| c.summary.as_str()).collect(); + let summaries: Vec<&str> = history.commits.iter().map(|c| c.summary.as_str()).collect(); assert_eq!(summaries, vec!["edit foo", "add foo"]); } + +/// The whole-answer case: history ran out, so the list is everything. +#[test] +fn a_walk_that_reaches_the_end_of_history_says_exhausted() { + let tr = TempRepo::with_initial_commit("hello\n"); + let (backend, handle) = tr.open_with_backend(); + write_file(tr.path(), "foo.txt", "a\n"); + tr.commit_all("add foo"); + + let history = backend + .file_history(&handle.id, Path::new("foo.txt"), 100, None, &live()) + .unwrap(); + + assert_eq!(history.stopped_at, HistoryStop::Exhausted); + assert_eq!(history.commits.len(), 1); + assert_eq!(history.visited, 2, "both commits were examined"); +} + +/// **The regression test for the issue.** A file with fewer changes than the +/// match limit gives the walk nothing to stop on; the visit cap is what stops +/// it, and the count it stopped at is what the notice says out loud. +/// +/// The match is in the OLDEST commit on purpose: the walk starts at HEAD, so a +/// cap that works necessarily misses it. Finding it here would mean the cap did +/// not apply. +#[test] +fn the_visit_cap_stops_the_walk_short_of_the_end() { + let tr = TempRepo::with_initial_commit("hello\n"); + let (backend, handle) = tr.open_with_backend(); + write_file(tr.path(), "old.txt", "a\n"); + tr.commit_all("add old"); + for i in 0..8 { + write_file(tr.path(), "other.txt", &format!("{i}\n")); + tr.commit_all(&format!("unrelated {i}")); + } + + let capped = backend + .file_history(&handle.id, Path::new("old.txt"), 100, Some(3), &live()) + .unwrap(); + + assert_eq!(capped.stopped_at, HistoryStop::VisitLimit); + assert_eq!(capped.visited, 3, "exactly the cap, not one more"); + assert!( + capped.commits.is_empty(), + "the only change to old.txt is older than the cap reached", + ); + + // Uncapped — which is what the notice's "search all of history" asks for — + // finds it. Same repository, same path, so the cap is the only difference. + let all = backend + .file_history(&handle.id, Path::new("old.txt"), 100, None, &live()) + .unwrap(); + assert_eq!(all.stopped_at, HistoryStop::Exhausted); + assert_eq!(all.commits.len(), 1); + assert_eq!(all.visited, 10); +} + +/// The off-by-one that would make the notice lie about every ordinary +/// repository: a cap reached exactly as history ends is not a truncation, and +/// must not claim there might be more. +#[test] +fn a_cap_that_lands_exactly_on_the_end_of_history_is_not_a_truncation() { + let tr = TempRepo::with_initial_commit("hello\n"); + let (backend, handle) = tr.open_with_backend(); + write_file(tr.path(), "foo.txt", "a\n"); + tr.commit_all("add foo"); + + let history = backend + .file_history(&handle.id, Path::new("foo.txt"), 100, Some(2), &live()) + .unwrap(); + + assert_eq!( + history.stopped_at, + HistoryStop::Exhausted, + "two commits examined under a cap of two is the whole history, not a cap hit", + ); + assert_eq!(history.visited, 2); +} + +/// The other ceiling, reported as itself. It says "the list is full", which is +/// a different sentence from "the search stopped" and carries no offer to +/// search further — more searching would find more matches and the list would +/// still hold `limit` of them. +#[test] +fn filling_the_match_limit_is_reported_separately_from_the_visit_cap() { + let tr = TempRepo::with_initial_commit("hello\n"); + let (backend, handle) = tr.open_with_backend(); + for i in 0..4 { + write_file(tr.path(), "foo.txt", &format!("{i}\n")); + tr.commit_all(&format!("edit foo {i}")); + } + + let history = backend + .file_history(&handle.id, Path::new("foo.txt"), 2, None, &live()) + .unwrap(); + + assert_eq!(history.stopped_at, HistoryStop::MatchLimit); + assert_eq!(history.commits.len(), 2); + let summaries: Vec<&str> = history.commits.iter().map(|c| c.summary.as_str()).collect(); + assert_eq!( + summaries, + vec!["edit foo 3", "edit foo 2"], + "newest first — the limit takes from the OLD end", + ); +} + +/// Cancellation, and that it really stops rather than running to the end and +/// throwing the answer away: the predicate counts its calls. +#[test] +fn a_cancelled_walk_stops_where_it_was_asked_to() { + let tr = TempRepo::with_initial_commit("hello\n"); + let (backend, handle) = tr.open_with_backend(); + for i in 0..20 { + write_file(tr.path(), "foo.txt", &format!("{i}\n")); + tr.commit_all(&format!("edit foo {i}")); + } + + let asked = std::sync::atomic::AtomicUsize::new(0); + let cancel_after_three = || { + let n = asked.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + n >= 3 + }; + + let err = backend + .file_history(&handle.id, Path::new("foo.txt"), 100, None, &cancel_after_three) + .unwrap_err(); + + assert!( + matches!(err, AppError::Cancelled), + "a cancelled walk is Cancelled, not an empty list — an empty list is \ + indistinguishable from a file with no history: {err:?}", + ); + assert_eq!( + asked.load(std::sync::atomic::Ordering::SeqCst), + 4, + "asked three times, stopped on the fourth — it did not keep walking", + ); +} + +/// A walk already cancelled before it starts never examines anything. +#[test] +fn a_walk_cancelled_before_it_starts_does_no_work() { + let tr = TempRepo::with_initial_commit("hello\n"); + let (backend, handle) = tr.open_with_backend(); + + let err = backend + .file_history(&handle.id, Path::new("README.md"), 100, None, &|| true) + .unwrap_err(); + assert!(matches!(err, AppError::Cancelled)); +} + +/// **It runs on the SHARED lock** (#400/#474) — the half of the issue that is +/// not about speed at all. Holding the exclusive lock for a walk this long +/// queued every other operation on the repository behind it. +/// +/// Proved by rendezvous rather than by timing: the `cancelled` predicate is +/// called from inside the walk, with the lock held, so it is a place to stand +/// still and ask whether another read can get in. If `file_history` were +/// exclusive, the `status` below could not start until the walk finished, the +/// bounded wait would expire, and this fails — rather than hanging, and rather +/// than passing slowly. +/// +/// Verified by planting the violation: with `with_repo` in place of +/// `with_repo_read` in `file_history`, this test fails on the timeout and the +/// rest of the file still passes. +#[test] +fn the_walk_does_not_exclude_another_read_of_the_same_repository() { + let tr = TempRepo::with_initial_commit("hello\n"); + for i in 0..10 { + write_file(tr.path(), "foo.txt", &format!("{i}\n")); + tr.commit_all(&format!("edit foo {i}")); + } + let (backend, handle) = tr.open_with_backend(); + let backend = Arc::new(backend); + + // `(walk is inside the lock, the other read finished)` + let state = Arc::new((Mutex::new((false, false)), Condvar::new())); + + let reader = { + let backend = Arc::clone(&backend); + let id = handle.id.clone(); + let state = Arc::clone(&state); + std::thread::spawn(move || { + let (lock, cv) = &*state; + // Wait until the walk is demonstrably inside its lock. + let mut seen = lock.lock().unwrap(); + while !seen.0 { + let (g, t) = cv.wait_timeout(seen, Duration::from_secs(10)).unwrap(); + seen = g; + assert!(!t.timed_out(), "the walk never reached its first commit"); + } + drop(seen); + + // The read under test: shared, on the same repository, while the + // walk holds its lock. + backend.status(&id).expect("a concurrent read must be served"); + + let (lock, cv) = &*state; + lock.lock().unwrap().1 = true; + cv.notify_all(); + }) + }; + + let inside_the_walk = || { + let (lock, cv) = &*state; + let mut s = lock.lock().unwrap(); + if !s.0 { + s.0 = true; + cv.notify_all(); + } + // Stand still until the other read is through. A walk holding an + // EXCLUSIVE lock would keep it out, and this wait would expire. + while !s.1 { + let (g, t) = cv.wait_timeout(s, Duration::from_secs(10)).unwrap(); + s = g; + assert!( + !t.timed_out(), + "a concurrent read could not be served while file_history walked \ + — it is holding the EXCLUSIVE lock", + ); + } + false + }; + + let history = backend + .file_history(&handle.id, Path::new("foo.txt"), 100, None, &inside_the_walk) + .expect("the walk itself must still succeed"); + assert_eq!(history.commits.len(), 10); + + reader.join().expect("the concurrent reader panicked"); +} diff --git a/src/lib/derive.fileHistory.test.ts b/src/lib/derive.fileHistory.test.ts new file mode 100644 index 00000000..cd579232 --- /dev/null +++ b/src/lib/derive.fileHistory.test.ts @@ -0,0 +1,93 @@ +// What a bounded file-history list admits to (#474). +// +// Three stops, three different claims, and the whole reason `HistoryStop` +// exists: "this is the file's complete history", "this list is full", and "the +// search gave up before history did" look identical on screen — a list that +// ends — and only the first one is the whole answer. The third is the one the +// issue is about: before the visit cap, a file with fewer changes than the +// limit sent the walk to the root of history. + +import { describe, expect, it } from "vitest"; + +import { fileHistoryNotice } from "./derive"; +import type { CommitInfo, FileHistory, HistoryStop } from "./types"; + +const commit = (n: number): CommitInfo => ({ + oid: String(n).repeat(40).slice(0, 40), + shortOid: String(n).repeat(7).slice(0, 7), + summary: `change ${n}`, + body: null, + author: "Author", + email: "author@example.com", + timestamp: 1_700_000_000, + parents: [], + refs: [], +}); + +const history = ( + stoppedAt: HistoryStop, + visited: number, + matches = 3, +): FileHistory => ({ + commits: Array.from({ length: matches }, (_, i) => commit(i + 1)), + visited, + stoppedAt, +}); + +describe("fileHistoryNotice", () => { + it("says nothing when there is nothing to admit to", () => { + expect(fileHistoryNotice(null)).toBeNull(); + expect(fileHistoryNotice(undefined)).toBeNull(); + expect(fileHistoryNotice(history("Exhausted", 412))).toBeNull(); + }); + + it("names the number of commits the search actually looked at", () => { + const notice = fileHistoryNotice(history("VisitLimit", 50_000)); + expect(notice?.title).toContain("50,000"); + expect(notice?.detail).toMatch(/history/i); + }); + + // The cap is backend policy. A frontend that spelled 50,000 itself would keep + // saying 50,000 after the policy moved — so the sentence has to follow the + // number that came back, whatever it is. + it("reports the cap it was given, not a number of its own", () => { + expect(fileHistoryNotice(history("VisitLimit", 20_000))?.title).toContain( + "20,000", + ); + expect(fileHistoryNotice(history("VisitLimit", 1_500_000))?.title).toContain( + "1,500,000", + ); + }); + + it("offers the rest of history only when more searching would find it", () => { + expect(fileHistoryNotice(history("VisitLimit", 50_000))?.canSearchAll).toBe( + true, + ); + // A full list stays full however far the walk goes: the button would run + // for minutes and change nothing on screen. + expect(fileHistoryNotice(history("MatchLimit", 900, 200))?.canSearchAll).toBe( + false, + ); + }); + + it("counts the list, not the walk, when the list is what filled up", () => { + const notice = fileHistoryNotice(history("MatchLimit", 1_234, 200)); + expect(notice?.title).toContain("200"); + expect(notice?.title).not.toContain("1,234"); + }); + + // If two stops read the same, the type has bought nothing. + it("says something different for each stop", () => { + const visit = fileHistoryNotice(history("VisitLimit", 50_000)); + const match = fileHistoryNotice(history("MatchLimit", 50_000, 200)); + expect(visit?.title).not.toBe(match?.title); + expect(visit?.detail).not.toBe(match?.detail); + }); + + // Reaching the match limit means the WALK had commits left, not that any of + // them touched this file. Anything stronger says more than was measured. + it("does not claim older changes definitely exist when the list filled up", () => { + const notice = fileHistoryNotice(history("MatchLimit", 1_234, 200)); + expect(notice?.detail).toMatch(/may be/i); + }); +}); diff --git a/src/lib/derive.ts b/src/lib/derive.ts index 4996733a..d9f2b633 100644 --- a/src/lib/derive.ts +++ b/src/lib/derive.ts @@ -3,6 +3,7 @@ import type { BranchInfo, CommitInfo, FileDiff, + FileHistory, FileStatus, RefInfo, StatusFlag, @@ -182,6 +183,62 @@ export function truncatedDiffNotice( }; } +/** + * What bounded a file's history, when something did (#474). + * + * A file-history list is the newest commits that touched one path, and it can + * end for three completely different reasons: the file has no older changes, + * the match limit was reached, or the SEARCH stopped before the history did. + * Only the first is "this is the whole answer", and before #474 all three + * looked identical — a list that simply ended. + * + * The third one is the one that matters. `file_history` walks from HEAD and + * keeps commits whose tree differs from their parent's at the path, so a file + * with fewer matches than the limit had nothing to stop on and walked to the + * root of history: ~1.5 million commits and as many tree comparisons on + * `torvalds/linux`, for a file the user clicked once. The visit cap is what + * ends that, and a cap nobody mentions is the silent wrong answer — so the + * number searched is said out loud, and `canSearchAll` is the user's way past + * it, exactly as the blob ceiling's "diff it anyway" is past that one. + * + * `visited` comes off the wire rather than from a constant here, for the reason + * `oversizedDiffNotice` gives about the blob ceiling: a second copy of backend + * policy is free to drift from the one that was applied. + * + * The markup lives in `screens/FileHistory.tsx` because that is the only + * surface with a file history on it. A second one lifts it out — it does not + * write its own sentence. + */ +export function fileHistoryNotice( + history: FileHistory | null | undefined, +): { title: string; detail: string; canSearchAll: boolean } | null { + if (!history) return null; + const fmt = (n: number) => n.toLocaleString(); + switch (history.stoppedAt) { + case "VisitLimit": + return { + title: `Searched the newest ${fmt(history.visited)} commits`, + detail: + "This file's history may carry on past that point — the search stopped at its own limit, not at the end of history.", + canSearchAll: true, + }; + case "MatchLimit": + return { + title: `Showing the newest ${fmt(history.commits.length)} changes`, + // Deliberately "may be": reaching the match limit means the walk still + // had commits left, not that any of them touched this file. Claiming + // older changes exist would be saying more than was measured. + detail: + "The list stops at its own limit rather than at the end of the file's history, so there may be older changes to it.", + // Searching further would find more MATCHES, and the list would still + // hold the same `limit` of them. The button would change nothing. + canSearchAll: false, + }; + default: + return null; + } +} + /** * Never committed and never staged — git holds no copy of it. * diff --git a/src/lib/tauri.ts b/src/lib/tauri.ts index c315af6c..3fb8ccf9 100644 --- a/src/lib/tauri.ts +++ b/src/lib/tauri.ts @@ -27,6 +27,7 @@ import type { FastForward, FileContent, FileDiff, + FileHistory, FileStatus, ForgeCheckoutRequest, ForgeDetection, @@ -1381,12 +1382,54 @@ export async function rebaseAcknowledge(repoId: string): Promise { return invoke("rebase_acknowledge", { repoId }); } +/** + * The commits that touched one path, newest first, and what bounded the search + * (#474). + * + * Two ceilings, and the result says which one ended the walk. `limit` caps the + * MATCHES; the backend separately caps how many commits it will LOOK AT, which + * is what stops a file with fewer changes than `limit` from walking to the root + * of history — ~1.5 million commits on `torvalds/linux` for one click. + * + * `searchAll` waives the visit cap, for the user who read the notice and wants + * the rest of history searched anyway. It is the old unbounded walk, asked for + * deliberately and cancellable with `cancelWalk` — never the default. + * + * The cap itself is deliberately NOT a constant on this side: the number the + * notice shows is `visited`, off the wire, so it cannot drift from the number + * that was applied. + */ +/** + * How many changes one file-history page holds — the MATCH ceiling. + * + * Exported so the one screen that asks can say the same number the wrapper + * defaults to, rather than keeping a second copy of it. + */ +export const FILE_HISTORY_LIMIT = 200; + export async function fileHistory( repoId: string, path: string, - limit = 200, -): Promise { - return invoke("file_history", { repoId, path, limit }); + limit = FILE_HISTORY_LIMIT, + searchAll = false, +): Promise { + return invoke("file_history", { repoId, path, limit, searchAll }); +} + +/** + * Stop the in-process history walks running on one repository (#474). + * + * The sibling of `cancelNetworkOp`, for the other kind of long operation: a + * `file_history` walk is libgit2 inside one blocking call, with no subprocess + * to signal, so cancelling sets a flag the walk itself polls between commits. + * Scoped to the repository for the same reason network cancellation is — there + * is nothing finer for the user to point at. + * + * Answers how many walks were signalled. Zero is a normal answer: the walk can + * finish between the click and this call. + */ +export async function cancelWalk(repoId: string): Promise { + return invoke("cancel_walk", { repoId }); } export async function appendGitignore( diff --git a/src/lib/types.ts b/src/lib/types.ts index 215d3fb3..c77b538a 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -141,6 +141,38 @@ export interface LogPage { nextCursor: string[] | null; } +/** + * Why a file-history walk stopped. Mirrors Rust `HistoryStop` (#474). + * + * The walk has two independent ceilings — how many MATCHES to collect and how + * many commits to LOOK AT — and the difference between them is the whole point + * of this type. A list of five commits means something completely different + * depending on which one ended the walk, and before #474 nothing said which. + */ +export type HistoryStop = + /** History ran out. The list is every commit that touched the path — complete. */ + | "Exhausted" + /** `limit` matches were collected. Older changes to this path exist. */ + | "MatchLimit" + /** + * The visit cap was reached first. The list holds every match in the newest + * `visited` commits and says nothing at all about what is older. + */ + | "VisitLimit"; + +/** One file's history, with what bounded it. Mirrors Rust `FileHistory` (#474). */ +export interface FileHistory { + commits: CommitInfo[]; + /** + * How many commits the walk examined. Reported rather than assumed: it is + * what the notice puts in front of the user ("searched the last 50,000 + * commits"), and a copy of the cap on this side would be free to drift from + * the one that was actually applied. + */ + visited: number; + stoppedAt: HistoryStop; +} + /** * Backend commit-log filter. All set fields are ANDed. String matches are * case-insensitive substring matches except `shaPrefix` (matches a prefix of the full OID, hex). diff --git a/src/screens/FileHistory.test.tsx b/src/screens/FileHistory.test.tsx new file mode 100644 index 00000000..4c34348c --- /dev/null +++ b/src/screens/FileHistory.test.tsx @@ -0,0 +1,187 @@ +// The file-history screen, and the three things a bounded search owes the +// reader (#474). +// +// The list itself was never the problem. What was missing is that the walk +// behind it has limits, and hitting one looked exactly like reaching the end of +// the file's history — so on a large repository the screen either showed a +// truncated answer as if it were complete, or sat on a spinner for minutes with +// nothing to stop it. This covers what it now says, the way past the cap, and +// that leaving does not leave a walk running. + +import { beforeEach, describe, expect, it } from "vitest"; +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; + +import { FileHistoryScreen } from "./FileHistory"; +import { useRepoStore } from "@/features/repo/useRepoStore"; +import { useNavStore } from "@/features/nav/useNavStore"; +import { getInvokeCalls, mockInvoke, resetInvokeMock } from "@/test/invokeMock"; +import type { CommitInfo, FileHistory, HistoryStop } from "@/lib/types"; + +const commit = (n: number): CommitInfo => ({ + oid: String(n).repeat(40).slice(0, 40), + shortOid: `000000${n}`, + summary: `change ${n}`, + body: null, + author: "Author", + email: "author@example.com", + timestamp: 1_700_000_000, + parents: [], + refs: [], +}); + +const history = ( + stoppedAt: HistoryStop, + visited: number, + matches = 2, +): FileHistory => ({ + commits: Array.from({ length: matches }, (_, i) => commit(i + 1)), + visited, + stoppedAt, +}); + +const historyCalls = () => getInvokeCalls().filter((c) => c.cmd === "file_history"); +const cancelCalls = () => getInvokeCalls().filter((c) => c.cmd === "cancel_walk"); + +beforeEach(() => { + resetInvokeMock(); + useRepoStore.setState({ + current: { id: "r1", path: "/repo", head: "main" }, + activity: {}, + } as never); + useNavStore.setState({ intent: { kind: "file-history", path: "src/main.rs" } }); +}); + +describe("a complete answer", () => { + it("lists the commits and says nothing about limits", async () => { + mockInvoke("file_history", () => history("Exhausted", 12)); + render(); + + await waitFor(() => expect(screen.getByText("change 1")).toBeInTheDocument()); + expect(screen.queryByTestId("history-limit-notice")).toBeNull(); + }); + + it("asks for the capped walk by default — the uncapped one is 135 s on the kernel", async () => { + mockInvoke("file_history", () => history("Exhausted", 12)); + render(); + + await waitFor(() => expect(historyCalls()).toHaveLength(1)); + expect(historyCalls()[0].args.searchAll).toBe(false); + }); +}); + +describe("a search that stopped at its own cap", () => { + it("says how far it looked, and offers the rest", async () => { + mockInvoke("file_history", () => history("VisitLimit", 50_000, 0)); + render(); + + const notice = await screen.findByTestId("history-limit-notice"); + expect(notice.textContent).toContain("50,000"); + expect(screen.getByTestId("history-search-all")).toBeInTheDocument(); + }); + + it("waives the cap on the second ask, and keeps the answer", async () => { + mockInvoke("file_history", (args) => + args.searchAll ? history("Exhausted", 1_482_923, 3) : history("VisitLimit", 50_000, 0), + ); + render(); + + fireEvent.click(await screen.findByTestId("history-search-all")); + + await waitFor(() => expect(historyCalls()).toHaveLength(2)); + expect(historyCalls()[1].args.searchAll).toBe(true); + // The notice goes because the claim it made is no longer true. + await waitFor(() => expect(screen.getByText("change 3")).toBeInTheDocument()); + expect(screen.queryByTestId("history-limit-notice")).toBeNull(); + }); + + // A full list is a different claim with no remedy: walking further finds more + // matches and the list still holds `limit` of them. + it("does not offer to search further when the LIST is what filled up", async () => { + mockInvoke("file_history", () => history("MatchLimit", 9_000, 2)); + render(); + + await screen.findByTestId("history-limit-notice"); + expect(screen.queryByTestId("history-search-all")).toBeNull(); + }); +}); + +describe("while it runs, and when it is stopped", () => { + it("joins RepoActivity, so the status bar can name it and stop it", async () => { + // Initialised rather than `| null`: TypeScript does not narrow a `let` + // assigned inside a callback, so an optional call on it is `never`. + let release: (h: FileHistory) => void = () => {}; + mockInvoke("file_history", () => new Promise((r) => (release = r))); + render(); + + await waitFor(() => + expect(useRepoStore.getState().activity.history?.label).toContain("src/main.rs"), + ); + + release(history("Exhausted", 4)); + // …and lets go of it afterwards, or the status bar never falls silent. + await waitFor(() => expect(useRepoStore.getState().activity.history).toBeUndefined()); + }); + + it("stops the walk when the screen goes away", async () => { + mockInvoke("file_history", () => new Promise(() => {})); + mockInvoke("cancel_walk", () => 1); + const view = render(); + + await waitFor(() => expect(historyCalls()).toHaveLength(1)); + view.unmount(); + + await waitFor(() => expect(cancelCalls()).toHaveLength(1)); + expect(cancelCalls()[0].args.repoId).toBe("r1"); + expect(useRepoStore.getState().activity.history).toBeUndefined(); + }); + + it("does not signal a cancel for a walk that already finished", async () => { + mockInvoke("file_history", () => history("Exhausted", 4)); + const view = render(); + + await waitFor(() => expect(screen.getByText("change 1")).toBeInTheDocument()); + view.unmount(); + + expect(cancelCalls()).toHaveLength(0); + }); + + // `cancel_walk` addresses the REPOSITORY, not one walk — so a cancel still in + // flight would reach whatever is registered when it arrives, which after a + // file switch is the walk for the file the user just opened. Switching files + // would then cancel its own replacement, at random, depending on which IPC + // call won. + it("waits for a cancel to land before starting the next file's walk", async () => { + let releaseCancel: (n: number) => void = () => {}; + mockInvoke("file_history", () => new Promise(() => {})); + mockInvoke("cancel_walk", () => new Promise((r) => (releaseCancel = r))); + render(); + + await waitFor(() => expect(historyCalls()).toHaveLength(1)); + + useNavStore.setState({ intent: { kind: "file-history", path: "src/other.rs" } }); + await waitFor(() => expect(cancelCalls()).toHaveLength(1)); + expect(historyCalls()).toHaveLength(1); + + releaseCancel(1); + await waitFor(() => expect(historyCalls()).toHaveLength(2)); + expect(historyCalls()[1].args.path).toBe("src/other.rs"); + }); + + // The user's own click must not come back as a red failure. + it("reads a cancellation as a stop, not an error, and can try again", async () => { + let attempts = 0; + mockInvoke("file_history", () => { + attempts += 1; + if (attempts === 1) throw { kind: "Cancelled" }; + return history("Exhausted", 4); + }); + render(); + + const stopped = await screen.findByTestId("history-search-cancelled"); + expect(stopped.textContent).toContain("Search stopped"); + + fireEvent.click(screen.getByTestId("history-search-again")); + await waitFor(() => expect(screen.getByText("change 1")).toBeInTheDocument()); + expect(screen.queryByTestId("history-search-cancelled")).toBeNull(); + }); +}); diff --git a/src/screens/FileHistory.tsx b/src/screens/FileHistory.tsx index b998b091..3d38cf90 100644 --- a/src/screens/FileHistory.tsx +++ b/src/screens/FileHistory.tsx @@ -1,13 +1,78 @@ import React from "react"; -import { PGEmpty, PGSpinner } from "@/design"; -import { useRepoStore } from "@/features/repo/useRepoStore"; +import { PGButton, PGEmpty, PGIcon, PGSpinner } from "@/design"; +import { useRepoStore, setActivity } from "@/features/repo/useRepoStore"; import { ShallowNotice } from "@/features/repo/ShallowNotice"; import { useNavStore } from "@/features/nav/useNavStore"; -import { fileHistory } from "@/lib/tauri"; -import { appErrorMessage } from "@/lib/errors"; +import { cancelWalk, fileHistory, FILE_HISTORY_LIMIT } from "@/lib/tauri"; +import { appErrorMessage, isCancelledError } from "@/lib/errors"; +import { fileHistoryNotice } from "@/lib/derive"; import { DeepViewHeader } from "@/features/nav/DeepViewHeader"; import { PGPane, FocusableScroll, usePaneList } from "@/features/keymap"; -import type { CommitInfo } from "@/lib/types"; +import type { FileHistory } from "@/lib/types"; + +/** + * What bounded the search, and the way past it (#474). + * + * The sentence is `fileHistoryNotice`; this is only its layout, and it is + * deliberately the same layout as `ShallowNotice` next to it — both are "the + * list below you is not the whole truth, and here is why", and two strips that + * can stack on one screen must not look like two different kinds of thing. + * + * The button is the blob ceiling's "diff it anyway" in another place: a cap is + * a guess about intent, it is usually right, and when it is wrong the user has + * to be able to say so. Waiving it asks for the walk that takes 135 s on the + * kernel — which is why it is a click and not the default, and why it is + * cancellable once running. + */ +function HistoryLimitNotice({ + history, + searching, + onSearchAll, +}: { + history: FileHistory | null; + searching: boolean; + onSearchAll: () => void; +}) { + const notice = fileHistoryNotice(history); + if (!notice) return null; + return ( +
+ +
+
{notice.title}
+
{notice.detail}
+
+ {notice.canSearchAll && ( + + {searching ? "Searching…" : "Search all of history"} + + )} +
+ ); +} export function FileHistoryScreen() { const repo = useRepoStore((s) => s.current); @@ -16,10 +81,17 @@ export function FileHistoryScreen() { const setNavIntent = useNavStore((s) => s.setIntent); const [path, setPath] = React.useState(null); - const [commits, setCommits] = React.useState([]); + const [history, setHistory] = React.useState(null); const [selected, setSelected] = React.useState(0); const [loading, setLoading] = React.useState(false); const [error, setError] = React.useState(null); + /** The user asked for the visit cap to be waived for THIS path. */ + const [searchAll, setSearchAll] = React.useState(false); + /** Bumped to re-run the same search — the way back from a cancelled one. */ + const [attempt, setAttempt] = React.useState(0); + const [cancelled, setCancelled] = React.useState(false); + + const commits = history?.commits ?? []; React.useEffect(() => { if (intent?.kind === "file-history") { @@ -28,17 +100,77 @@ export function FileHistoryScreen() { } }, [intent, clearIntent]); + /** + * A cancel this screen has asked for and not yet seen land. + * + * `cancel_walk` addresses the REPOSITORY, not one walk — deliberately, so the + * status bar's Cancel button needs nothing to point at. The cost is that a + * cancel still in flight would reach whatever is registered when it arrives, + * including the walk started for the file the user just switched TO. So the + * next search waits for it: one extra round trip, and only when a walk was + * actually running. + */ + const pendingCancel = React.useRef | null>(null); + + // A new file is a new question: the previous answer's waiver and its + // cancellation do not carry over. + React.useEffect(() => { + setSearchAll(false); + setCancelled(false); + setHistory(null); + }, [path]); + React.useEffect(() => { if (!repo || !path) return; - let cancelled = false; + const repoId = repo.id; + let live = true; + let inFlight = true; setLoading(true); setError(null); - fileHistory(repo.id, path) - .then((c) => { if (!cancelled) { setCommits(c); setSelected(0); } }) - .catch((e) => { if (!cancelled) setError(appErrorMessage(e)); }) - .finally(() => { if (!cancelled) setLoading(false); }); - return () => { cancelled = true; }; - }, [repo?.id, path]); + setCancelled(false); + // `RepoActivity`, not a private spinner: this is the app's one answer to + // "what is running and can I stop it", and a walk that keeps its own busy + // flag gets neither the status line nor the Cancel button (#474). + setActivity( + repoId, + "history", + searchAll ? `Searching all history for ${path}…` : `Searching history for ${path}…`, + ); + + const cancelFirst = pendingCancel.current ?? Promise.resolve(); + cancelFirst + .then(() => fileHistory(repoId, path, FILE_HISTORY_LIMIT, searchAll)) + .then((h) => { + if (!live) return; + setHistory(h); + setSelected(0); + }) + .catch((e) => { + if (!live) return; + // A cancellation is an answer, not a failure: the user asked for the + // walk to stop. A red banner would report their own click as a fault. + if (isCancelledError(e)) setCancelled(true); + else setError(appErrorMessage(e)); + }) + .finally(() => { + inFlight = false; + if (!live) return; + setActivity(repoId, "history", null); + setLoading(false); + }); + + return () => { + live = false; + // Leaving the screen, or asking about a different file, must not leave a + // walk running: it costs a blocking thread and, on a large repository, + // seconds of tree comparisons for an answer nobody will read. + if (inFlight) pendingCancel.current = cancelWalk(repoId).catch(() => {}); + // Cleared here as well as in `finally`, and it has to be BOTH: this runs + // before the next effect sets its own label, so the order is clear → + // set, while `finally` is what clears a walk that simply ended. + setActivity(repoId, "history", null); + }; + }, [repo?.id, path, searchAll, attempt]); // Keyboard: arrows move the commit selection, Enter opens the commit's diff. usePaneList({ @@ -70,9 +202,33 @@ export function FileHistoryScreen() { {/* A file's history ends at the shallow boundary too, and the list gives no sign of it: it simply has fewer rows (#255). */} + {/* …and it ends at the search's own limits, which is a different claim + with a different remedy (#474). */} + setSearchAll(true)} + /> {loading &&
} {error &&
{error}
} - {!loading && !error && commits.length === 0 && ( + {cancelled && !loading && ( +
+ + Search stopped. Nothing below it was searched. + + setAttempt((n) => n + 1)} + > + Search again + +
+ )} + {!loading && !error && !cancelled && commits.length === 0 && ( )} From 821f994e8b0b44e58a40356e50f53e42f88fd355 Mon Sep 17 00:00:00 2001 From: Jonas Aasberg Date: Thu, 17 Sep 2026 17:43:41 +0200 Subject: [PATCH 3/4] test(e2e): one file's history over the real IPC boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The component tests drive `file_history` through a mocked `invoke`, so none of them can see the wire: the command answers with a struct now rather than an array, takes a new `searchAll` argument, and `cancel_walk` is a new command whose rejection the frontend deliberately swallows (a walk that finished first is the ordinary case). A serde rename arriving as `stopped_at`, or an argument Tauri would not convert, breaks none of the 4,178 mocked tests and every real click — the same shape of break as the argument-name break in #435 that only a real-IPC case caught. Two of the four cases therefore ask the commands directly: nothing on screen distinguishes a readable `stoppedAt` from an unreadable one, since both render the same list with no notice. Co-Authored-By: Claude Opus 5 (1M context) --- e2e/specs/file-history.e2e.ts | 185 ++++++++++++++++++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 e2e/specs/file-history.e2e.ts diff --git a/e2e/specs/file-history.e2e.ts b/e2e/specs/file-history.e2e.ts new file mode 100644 index 00000000..4e2a1c21 --- /dev/null +++ b/e2e/specs/file-history.e2e.ts @@ -0,0 +1,185 @@ +import { browser, $, $$, expect } from "@wdio/globals"; +import { dirtyRepo, TempRepo } from "../support/tempRepo"; +import { + changeRow, + executeOnce, + jsClickMenuItem, + jsContextMenu, + openRepo, + resetApp, + switchScreen, +} from "../support/app"; + +// One file's history over the real IPC boundary (#474). +// +// The interesting half is not the list — the component tests cover that against +// a mocked `invoke`. It is the WIRE: `file_history` answers with a struct now +// (`commits` / `visited` / `stoppedAt`) rather than an array, it takes a new +// `searchAll` argument, and `cancel_walk` is a new command. A serde rename that +// arrived as `stopped_at`, or an argument Tauri would not convert, breaks none +// of the 4,176 mocked tests and every real click — the shape of break this +// suite exists for (see `docs/dev/testing.md` on the same trap in #435). + +/** The file-history screen's own pane — a signal no other screen can satisfy. */ +const HISTORY_PANE = '[data-pg-pane="fileHistory.list"]'; +const HISTORY_ROWS = `${HISTORY_PANE} [data-pg-row]`; + +/** Every history row's text, in order. */ +async function rowTexts(): Promise { + const rows = await $$(HISTORY_ROWS); + const out: string[] = []; + for (const row of rows) out.push((await row.getText()).trim()); + return out; +} + +/** `window.__TAURI__.core`, as every direct-IPC spec spells it. */ +type Bridge = { + __TAURI__?: { core?: { invoke: (c: string, a?: unknown) => Promise } }; +}; + +describe("file history", () => { + let repo: TempRepo; + + beforeEach(async () => { + // `a.txt` is committed twice and `b.txt` once, so "the commits that touched + // THIS file" is a different list from "the log". + repo = dirtyRepo(); + await openRepo(repo.path); + await switchScreen("commit"); + await changeRow("a.txt").waitForDisplayed({ + timeout: 30_000, + timeoutMsg: "commit screen never showed the changes list", + }); + }); + + afterEach(async () => { + await resetApp(); + repo.dispose(); + }); + + async function openHistoryOfA(): Promise { + await jsContextMenu('[data-testid="changes-list"] [data-path="a.txt"]'); + await jsClickMenuItem("File history"); + // The destination's own pane first: a row selector could otherwise bind to + // the commit screen we are leaving. + await $(HISTORY_PANE).waitForDisplayed({ + timeout: 30_000, + timeoutMsg: "the file-history screen never opened", + }); + } + + it("lists the commits that touched the file, newest first", async () => { + await openHistoryOfA(); + + // Repo truth is the acceptance; the row count is the wait. + const subjects = repo + .git("log", "--format=%s", "--", "a.txt") + .trim() + .split("\n"); + expect(subjects).toEqual(["fix: update a.txt", "feat: add a.txt"]); + + await browser.waitUntil( + async () => (await rowTexts()).length === subjects.length, + { + timeout: 30_000, + timeoutMsg: `expected ${subjects.length} history rows for a.txt`, + }, + ); + + const texts = await rowTexts(); + expect(texts[0]).toContain("fix: update a.txt"); + expect(texts[1]).toContain("feat: add a.txt"); + // b.txt's commit is in the log and not in THIS file's history. + expect(texts.join("\n")).not.toContain("add b.txt"); + }); + + it("claims no search limit when it reached the end of history", async () => { + await openHistoryOfA(); + await browser.waitUntil(async () => (await rowTexts()).length === 2, { + timeout: 30_000, + timeoutMsg: "history rows never appeared", + }); + + // The notice is the whole user-visible point of `stoppedAt`. On a + // three-commit repository the honest answer is silence — a notice here + // would mean the frontend cannot read the field at all and is treating + // every walk as truncated. + await expect($('[data-testid="history-limit-notice"]')).not.toBeExisting(); + }); + + // The field names, over the real bridge. Nothing on screen distinguishes + // `stoppedAt` from an unreadable `stopped_at` — both render the same list + // with no notice — so this asks the backend directly. + it("answers with the struct the frontend reads, field for field", async () => { + const answer = await browser.execute(async (repoPath: string) => { + const core = (window as unknown as Bridge).__TAURI__?.core; + if (!core) return { error: "no bridge" }; + try { + const open = (await core.invoke("open_repo", { path: repoPath })) as { + id: string; + }; + const history = await core.invoke("file_history", { + repoId: open.id, + path: "a.txt", + limit: 200, + searchAll: false, + }); + return { history }; + } catch (e) { + return { error: String(e) }; + } + }, repo.path); + + expect(answer.error).toBeUndefined(); + const history = answer.history as { + commits: { summary: string }[]; + visited: number; + stoppedAt: string; + }; + expect(history.commits.map((c) => c.summary)).toEqual([ + "fix: update a.txt", + "feat: add a.txt", + ]); + // Every commit was examined to find those two, and the walk ended because + // history did — not because either ceiling was reached. + expect(history.visited).toBe( + Number(repo.git("rev-list", "--count", "HEAD").trim()), + ); + expect(history.stoppedAt).toBe("Exhausted"); + }); + + // `searchAll` and `cancel_walk` are both new argv over the bridge, and both + // fail SILENTLY when wrong: the screen only sends `searchAll: false`, and the + // frontend swallows a `cancel_walk` rejection because a walk that finished + // first is the ordinary case. A fixture cannot make a walk slow enough to + // cancel through the UI, so this asks the commands directly. + it("accepts a waived cap and a cancel for a repository with nothing running", async () => { + const answer = await executeOnce(async (repoPath: string) => { + const core = (window as unknown as Bridge).__TAURI__?.core; + if (!core) return { error: "no bridge" }; + try { + const open = (await core.invoke("open_repo", { path: repoPath })) as { + id: string; + }; + const all = (await core.invoke("file_history", { + repoId: open.id, + path: "a.txt", + limit: 200, + searchAll: true, + })) as { commits: unknown[]; stoppedAt: string }; + const signalled = (await core.invoke("cancel_walk", { + repoId: open.id, + })) as number; + return { matches: all.commits.length, stoppedAt: all.stoppedAt, signalled }; + } catch (e) { + return { error: String(e) }; + } + }, repo.path); + + expect(answer.error).toBeUndefined(); + expect(answer.matches).toBe(2); + expect(answer.stoppedAt).toBe("Exhausted"); + // Nothing was running, which is a count of zero and not a failure. + expect(answer.signalled).toBe(0); + }); +}); From dd4428e1ac7da211f35b912040bdd908a6bf9b74 Mon Sep 17 00:00:00 2001 From: Jonas Aasberg Date: Thu, 17 Sep 2026 17:43:57 +0200 Subject: [PATCH 4/4] docs: what a file-history walk costs, measured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `performance.md` said file history was "unbounded on a cold path", which is no longer true, and said it without a number because "unbounded is not a number". There are numbers now, taken on the `linux` fixture: 135.6 s uncapped against 18.3 s capped, of which 14.5 s is libgit2's own revwalk preparation and 4.1 s is 50,000 commits of tree comparison. The second bullet is the more generally useful finding, and it rules out the cheap fix the way the commit-graph measurement next to it already does: a sorted libgit2 revwalk pre-walks the whole graph before it yields anything, and the sort ORDER is not why — TIME and TIME|TOPOLOGICAL were within 1% of each other at every size measured. That floor is very probably the one behind `log_page`'s first page too, which is worth knowing before anyone tries to optimise the log by changing its sort. Also: `file_history` joins the shared-read list in `backend.md` with its proof and the rendezvous test that pins it, `cancel.rs`'s section grows the second KIND of cancellable work, `architecture.md` gains the two commands (`test/docs.test.ts` requires it), `frontend.md` gains the notice and the cancel-ordering trap, and `repo_bench.rs`'s `hottest_path` no longer explains itself with a bug that is fixed — it has two reasons that outlived it. Co-Authored-By: Claude Opus 5 (1M context) --- docs/dev/architecture.md | 15 +++++++++- docs/dev/backend.md | 47 +++++++++++++++++++++++++++---- docs/dev/frontend.md | 61 ++++++++++++++++++++++++++++++++++++++++ docs/dev/performance.md | 43 +++++++++++++++++++++++----- 4 files changed, 153 insertions(+), 13 deletions(-) diff --git a/docs/dev/architecture.md b/docs/dev/architecture.md index 42cebcfe..0490213a 100644 --- a/docs/dev/architecture.md +++ b/docs/dev/architecture.md @@ -425,7 +425,20 @@ commands/ Thin Tauri handlers, one file per area: │ with nothing naming the commit that vanished. NOTE it passes │ no `--` before the revision — that separator introduces │ PATHSPECS, so `-- ` selects nothing; safe because the oid -│ is a resolved 40-hex id, not user text), file_history, +│ is a resolved 40-hex id, not user text), file_history (#474 — +│ one path's commits, with TWO ceilings: `limit` bounds the +│ matches and a VISIT cap bounds how many commits are looked at, +│ because a file with fewer changes than the limit had nothing to +│ stop on and walked to the root of history — 135 s and 1.48M +│ tree comparisons on the kernel for one click. The command owns +│ the default cap (git::FILE_HISTORY_VISIT_LIMIT) and registers +│ the walk under cancel::Scope::Walk; the backend is handed a +│ predicate to poll and knows nothing about cancellation. +│ `searchAll` waives the cap for a user who read the notice. +│ The result carries `visited` + `stoppedAt`, which is what the +│ notice says out loud), cancel_walk (#474 — stops those walks on +│ one repository; cancel_network_op's sibling for work that has +│ no subprocess to signal, so it sets a flag the walk polls), │ verify_commit (SELECTED commit │ only, never per row), commit_notes, which is lazy for the │ same reason (#253 — the log walk is the hot path, so notes are diff --git a/docs/dev/backend.md b/docs/dev/backend.md index d5ecd881..43246489 100644 --- a/docs/dev/backend.md +++ b/docs/dev/backend.md @@ -271,8 +271,25 @@ Part of the `docs/dev/` set (`architecture`, `testing`, `frontend`, `backend`, them — the credential protocol is line-based, so a newline injects keys and could file a password against another host. -### Cancelling a stalled network op (#234, hardened by #263) - +### Cancelling a stalled network op (#234, hardened by #263, widened by #474) + +- **Two kinds of work, one registry.** A network SUBPROCESS is signalled; a long + in-process libgit2 WALK is asked to notice. `#474` added the second kind — + `Scope::Walk(workdir)`, today `file_history` — and it gets none of the + machinery below: no pid is ever attached, so `kill_tree` is unreachable for + those entries and the SIGTERM→SIGKILL escalation is meaningless (a second + click sets the same flag the first one did). What it gets instead is + `Registration::is_cancelled`, which the command hands to the backend as a + `&dyn Fn() -> bool` that the walk polls between commits — so the backend knows + nothing about this module, and the scope-to-path matching stays in the command + layer where `cancel_walk` resolves the same path the same way. + **`Walk` is deliberately NOT folded into `Repo`** even though both are keyed by + the workdir: cancelling a stalled fetch must not abandon a history search the + user is waiting on, and vice versa. `cancel_all` reaches both, which is what a + closing window wants. One consequence lands in the frontend: a repository-wide + cancel still in flight would reach whatever registers next, so the file-history + screen chains its next walk after the cancel it asked for rather than racing it + (`docs/dev/frontend.md`). - **One cancel path, at the same two choke points as the credential policy.** `cancel.rs` is a process-wide registry; `run_git_authenticated` and `run_clone` each register for as long as they run, so a network op that uses @@ -1471,15 +1488,35 @@ against 9,000 ms) rather than a direct measurement. **Which ops are shared today:** `status`, `branches`, `tags`, `stashes`, `remotes`, `log`/`log_page`, `repo_state`, `rebase_status`, `bisect_status`, `head_info`, `shallow_info`, `diff_commit`/`diff_commit_over_ceiling`, -`verify_commit`, `worktrees`. That is `refreshAll`'s eleven-read fan-out plus the -ops behind the history-arrowing ladder in #400. Every other read-only op — -`log_filtered`, `diff`, `diff_commits`, `file_history`, `blame_file`, +`verify_commit`, `worktrees`, `file_history`. That is `refreshAll`'s eleven-read +fan-out, plus the ops behind the history-arrowing ladder in #400, plus the one +op that made the loudest case for itself (below). Every other read-only op — +`log_filtered`, `diff`, `diff_commits`, `blame_file`, `read_file_content*`, `commits_since`, `ahead_behind`, `submodules`, `lfs_status`, `commit_notes`, `read_reflog`, `list_all_files`, `list_files_at_rev`, `commit_template`, `difftool_plan`, `conflict_sides` — is still exclusive. Not because it must be, but because each needs its own proof it writes nothing, and moving one is a one-word change. +**`file_history` was the one that mattered most, and it moved in #474.** It is +the longest read in the backend — 135 s on `torvalds/linux` before the visit +cap, 18 s after it — and it held the EXCLUSIVE lock for all of it, so one click +on "File history" queued every other operation on the repository behind a walk +that could take minutes. That is precisely the failure #400 set out to remove, +surviving in the op least able to afford it. The proof it writes nothing: a +revwalk over the refs, `find_commit`, `Tree::get_path`, and a tree-to-tree diff +in the pathspec case — no index read or write-back, no ref move, no object +written, no worktree touch. Note what that list does NOT include, because the +`status` regression is the cautionary tale next door: nothing here reads the +index, so there is no accidental index refresh for another op to have come to +depend on. + +`tests/file_history.rs` pins the sharing by RENDEZVOUS rather than by timing — +the walk's cancellation predicate is called with the lock held, so it is a place +to stand still and check whether another read can get in. Put `with_repo` back +and that test fails on its bounded wait (verified), while a timing assertion +would only have got slower. + ## What decorates a log row, and which KIND of ref it is `collect_ref_map` (`libgit2.rs`) scans every ref once per log call and hands the diff --git a/docs/dev/frontend.md b/docs/dev/frontend.md index efba0859..f0fdafb1 100644 --- a/docs/dev/frontend.md +++ b/docs/dev/frontend.md @@ -1225,6 +1225,52 @@ that is only partly here — which is the whole reason the notice exists. included, resets on a closed→open transition — a `--depth 1` chosen for one enormous repository must not quietly truncate the next. +## Saying a file's history was only partly searched (#474) + +The strip's other half, on the same screen and immediately below it. A shallow +clone is history the app never had; this is history the app declined to walk. + +- **Three stops, three claims, one pure sentence** — `fileHistoryNotice` + (`lib/derive.ts`, beside `truncatedDiffNotice`, which is the same idea about a + diff's lines). `Exhausted` says nothing at all: the list is the whole answer. + `MatchLimit` says the list is full. `VisitLimit` says the SEARCH stopped + before history did, which is the one the issue was about — a file with fewer + changes than the limit gave the walk nothing to stop on, and it ran to the + root of history. +- **The number comes off the wire (`visited`), never from a constant here.** + Same rule as the blob ceiling's: the cap is backend policy + (`git::FILE_HISTORY_VISIT_LIMIT`), and a copy of it on this side would go on + saying 50,000 after the policy moved. +- **`canSearchAll` only where more searching would change the screen.** The + visit cap gets "Search all of history" — the blob ceiling's "diff it anyway" + in another place, and for the same reason: a cap is a guess about intent, + usually right, and completely wrong when it is wrong. The match limit gets no + button, because walking further finds more matches and the list still holds + `limit` of them. +- **The markup lives in `screens/FileHistory.tsx`**, because that is the only + surface with a file history on it, and deliberately mirrors `ShallowNotice`'s + layout — the two can stack, and two strips that say the same KIND of thing + must not look like two different kinds of thing. A second surface lifts it + out rather than writing its own sentence. +- **The walk joins `RepoActivity` under `history`**, which is where its status + line, elapsed clock and Cancel button come from. Before #474 it was a bare + `PGSpinner` in the pane — for up to 135 s, with no way out. +- **Leaving the screen stops the walk.** The effect's cleanup cancels it: an + abandoned walk costs a blocking thread and seconds of tree comparisons for an + answer nobody will read. +- **…but the next walk waits for that cancel to land.** `cancel_walk` addresses + the REPOSITORY (see `docs/dev/backend.md` — that coarseness is what lets the + status bar's one Cancel button work with nothing to point at), so a cancel + still in flight would reach whatever is registered when it arrives — which + after a file switch is the walk for the file the user just opened. Switching + files would cancel its own replacement, at random, depending on which IPC call + won. So the screen keeps the cancel's promise in a ref and chains the next + request after it: one extra round trip, only when a walk was really running. +- **A cancellation is an answer, not a failure.** `isCancelledError` routes it to + a neutral "Search stopped" line with "Search again" beside it, never the red + error text — a red banner would report the user's own click as a fault, and an + empty list would be indistinguishable from a file with no history. + ## Checking out a remote branch — `features/branches/checkoutRemote.ts` A remote-tracking ref is not a branch you can be ON. `checkoutRef("origin/x")` @@ -1844,6 +1890,21 @@ checkout read as a click that did nothing. "Cancelling…" on one click of either one. Its case table is keyed by `ActivityKey`, so a kind added later cannot skip it — and a third surface lays out this hook rather than reading `activity` itself. +- **"Can it be cancelled" and "what cancels it" are ONE table** — + `cancelPath(key)` in `repoActivity.ts`, since #474. There are now two + mechanisms: `"network"` signals a git subprocess (`cancel_network_op`), and + `"walk"` sets a flag a libgit2 revwalk polls (`cancel_walk`, for + `activity.history`). A set of cancellable keys plus a branch at the button + would be the same question answered twice, free to disagree — and the + disagreement's shape is a Cancel button that runs and stops nothing. + `isCancellable` is now derived from the table rather than beside it. +- **`history` is the first entry that is neither a subprocess nor a mutation** + (#474) — `file_history` searching one file. It earns an entry for the only + reason this module exists: on a large repository it is a wait long enough to + need stopping (18 s on the kernel with the visit cap, 135 s without it). A + long op that keeps its busy state privately gets no status line, no elapsed + clock and no Cancel — which is how it sat behind a bare `PGSpinner` until + #474. - **The strip carries its own test hooks (`activity-strip-*`).** `cancel.e2e.ts` waits on `activity-label` and CLICKS `activity-cancel`; WebdriverIO's `$` takes the first match in document order, and the strip renders above the diff --git a/docs/dev/performance.md b/docs/dev/performance.md index 933311cb..18dcf761 100644 --- a/docs/dev/performance.md +++ b/docs/dev/performance.md @@ -224,13 +224,42 @@ it on ref writes. ### Known characteristics that are not on the tables -* **File history is unbounded on a cold path.** `file_history` stops at `limit` - matches, so on a file with fewer than 500 commits it walks to the root of - history with a tree comparison per commit. On the kernel that is 1.5 million - of them for one click. The benchmark measures the *most frequently changed* - path precisely so it terminates; see `hottest_path` in the harness. It also - takes the exclusive lock (`with_repo`, not `with_repo_read`), so it blocks - every other read on that repository while it runs. +* **File history was unbounded on a cold path, and is capped since #474.** + `file_history` stops at `limit` matches, so on a file with fewer changes than + that it had nothing to stop on and walked to the root of history with a tree + comparison per commit. Measured on the `linux` fixture, warm, for + `arch/powerpc/kernel/iommu.c`: + + | walk | cost | + | --- | --- | + | uncapped — 1,482,923 commits, every one compared | **135.6 s** | + | capped at 50,000 visits (the default since #474) | **18.3 s** | + | the revwalk's own preparation, before any tree work | 14.5 s | + | 50,000 commits of tree comparison | 4.1 s | + + So the cap removes ~117 s of the 135 s, and what is left is dominated by a + fixed cost the cap cannot touch (next bullet). The benchmark measures the + *most frequently changed* path precisely so the uncapped walk terminated at + all; see `hottest_path` in the harness. #474 also moved it to + `with_repo_read`, so it no longer blocks every other read on the repository + while it runs, and made it cancellable — 18 s is still a wait worth being + able to stop. + +* **A sorted libgit2 revwalk pre-walks the whole graph before it yields + anything, and the sort order is not why.** Getting the FIRST oid out of a + `push_head` walk on the kernel costs 14.2 s; walking 50,000 costs 14.2 s; and + walking all 1,482,923 costs 14.7 s — the same number three times, because the + traversal has already happened by the time the first one comes back. + `Sort::TIME` and `Sort::TIME | Sort::TOPOLOGICAL` were measured within 1% of + each other, so dropping `TOPOLOGICAL` buys nothing and the obvious + optimisation is a dead end. `Sort::NONE` IS incremental and is unusable for a + capped walk: it yields commits in the order the traversal reaches them, so the + first 50,000 are not the newest 50,000 and a capped file history could miss + last week's change while reporting one from 2011. Every walk in + `libgit2.rs` sorts, so this floor is very probably the one behind `log_page`'s + first page too — consistent with the commit-graph measurement below, which is + the fix git gets for exactly this and we do not, but measured here only for + `file_history`. * **No fixture carries a commit-graph file, and it would not help us if it did.** A fresh clone has none — `git clone` does not write one, and `gc --auto` does not fire on a single packfile — so this is what a user gets