Fix clogged up git operations.
This commit is contained in:
@@ -23,11 +23,17 @@ That variable is the whole of the addressing. It names **this tab's** review, so
|
|||||||
there is no repository to pass and no way to address comments meant for another
|
there is no repository to pass and no way to address comments meant for another
|
||||||
worktree.
|
worktree.
|
||||||
|
|
||||||
|
Give every call a deadline — `--max-time 30`. The server runs `git` for some of
|
||||||
|
these, and a repository being built in another pane can make that slow; a curl
|
||||||
|
with no deadline turns that into a tool timeout that says nothing about what
|
||||||
|
went wrong. If one does trip, say so and try once more rather than treating it
|
||||||
|
as a missing review: the answer was late, not absent.
|
||||||
|
|
||||||
If it is empty — you are running outside playpen, or in a shell started before
|
If it is empty — you are running outside playpen, or in a shell started before
|
||||||
the server came up — discover it instead:
|
the server came up — discover it instead:
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
curl -s http://127.0.0.1:8420/api/tabs
|
curl -s --max-time 30 http://127.0.0.1:8420/api/tabs
|
||||||
```
|
```
|
||||||
|
|
||||||
That lists every tab with the `path` it is reviewing. Match `path` against
|
That lists every tab with the `path` it is reviewing. Match `path` against
|
||||||
@@ -40,7 +46,7 @@ stop.
|
|||||||
Confirm the review is open:
|
Confirm the review is open:
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
curl -s "$BASE/api/repo"
|
curl -s --max-time 30 "$BASE/api/repo"
|
||||||
```
|
```
|
||||||
|
|
||||||
- `{"open":true,…}` — good, go on.
|
- `{"open":true,…}` — good, go on.
|
||||||
@@ -50,7 +56,7 @@ curl -s "$BASE/api/repo"
|
|||||||
## 2. Fetch the pending comments
|
## 2. Fetch the pending comments
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
curl -s "$BASE/api/review/pending"
|
curl -s --max-time 30 "$BASE/api/review/pending"
|
||||||
```
|
```
|
||||||
|
|
||||||
Returns this review's submitted, unresolved comments. Each has:
|
Returns this review's submitted, unresolved comments. Each has:
|
||||||
@@ -88,7 +94,7 @@ For every pending comment, in order:
|
|||||||
3. **Post an inline reply** describing exactly what you did (or your answer):
|
3. **Post an inline reply** describing exactly what you did (or your answer):
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
curl -s -X POST "$BASE/api/comments/<id>/replies" \
|
curl -s --max-time 30 -X POST "$BASE/api/comments/<id>/replies" \
|
||||||
-H 'Content-Type: application/json' \
|
-H 'Content-Type: application/json' \
|
||||||
-d '{"body":"Done — main() now logs and returns the error instead of printing.","author":"claude"}'
|
-d '{"body":"Done — main() now logs and returns the error instead of printing.","author":"claude"}'
|
||||||
```
|
```
|
||||||
@@ -96,7 +102,7 @@ For every pending comment, in order:
|
|||||||
4. **Resolve the thread** once it's fully handled (skip if you asked a question):
|
4. **Resolve the thread** once it's fully handled (skip if you asked a question):
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
curl -s -X POST "$BASE/api/comments/<id>/resolve"
|
curl -s --max-time 30 -X POST "$BASE/api/comments/<id>/resolve"
|
||||||
```
|
```
|
||||||
|
|
||||||
Replies and resolutions appear in the review pane immediately over its live
|
Replies and resolutions appear in the review pane immediately over its live
|
||||||
|
|||||||
@@ -21,19 +21,26 @@ job, and your comments land in its queue automatically (see step 7).
|
|||||||
|
|
||||||
```bash
|
```bash
|
||||||
BASE="$PLAYPEN_REVIEW_URL" # e.g. http://127.0.0.1:8420/t/tab3
|
BASE="$PLAYPEN_REVIEW_URL" # e.g. http://127.0.0.1:8420/t/tab3
|
||||||
curl -s "$BASE/api/repo"
|
curl -s --max-time 30 "$BASE/api/repo"
|
||||||
```
|
```
|
||||||
|
|
||||||
`$PLAYPEN_REVIEW_URL` is exported into every terminal pane and names **this
|
`$PLAYPEN_REVIEW_URL` is exported into every terminal pane and names **this
|
||||||
tab's** review, so there is nothing to choose and no way to leave your review on
|
tab's** review, so there is nothing to choose and no way to leave your review on
|
||||||
someone else's branch.
|
someone else's branch.
|
||||||
|
|
||||||
|
Give every call a deadline — `--max-time 30`. The server runs `git` for some of
|
||||||
|
these, and a repository being built in another pane can make that slow; a curl
|
||||||
|
with no deadline turns that into a tool timeout that says nothing about what
|
||||||
|
went wrong. If one does trip, say so and try once more rather than treating it
|
||||||
|
as a missing review: the answer was late, not absent.
|
||||||
|
|
||||||
- `{"open":true,…}` with a `context` object — good, go to step 2.
|
- `{"open":true,…}` with a `context` object — good, go to step 2.
|
||||||
- `{"open":false}` or a 409 — this tab has no review pane. Tell the user to open
|
- `{"open":false}` or a 409 — this tab has no review pane. Tell the user to open
|
||||||
one (**Ctrl+Shift+D**) and stop. Don't review a different tab.
|
one (**Ctrl+Shift+D**) and stop. Don't review a different tab.
|
||||||
- `$PLAYPEN_REVIEW_URL` empty, or connection refused — list the tabs with
|
- `$PLAYPEN_REVIEW_URL` empty, or connection refused — list the tabs with
|
||||||
`curl -s http://127.0.0.1:8420/api/tabs` and match a tab's `path` against
|
`curl -s --max-time 30 http://127.0.0.1:8420/api/tabs` and match a tab's
|
||||||
`git rev-parse --show-toplevel`. If nothing matches, stop and say so.
|
`path` against `git rev-parse --show-toplevel`. If nothing matches, stop and
|
||||||
|
say so.
|
||||||
|
|
||||||
## 2. Find out which diff to review
|
## 2. Find out which diff to review
|
||||||
|
|
||||||
@@ -59,7 +66,7 @@ whose line numbers the pane can place a comment on.
|
|||||||
## 3. Get the diff
|
## 3. Get the diff
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
curl -s "$BASE/api/diff?base=main&uncommitted=true"
|
curl -s --max-time 30 "$BASE/api/diff?base=main&uncommitted=true"
|
||||||
```
|
```
|
||||||
|
|
||||||
The `patch` field is the exact bytes the pane renders, and `files` is the
|
The `patch` field is the exact bytes the pane renders, and `files` is the
|
||||||
@@ -163,7 +170,7 @@ Three details that decide whether a comment lands where you meant:
|
|||||||
**Don't repeat what's already been said.** Fetch the existing threads first:
|
**Don't repeat what's already been said.** Fetch the existing threads first:
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
curl -s "$BASE/api/comments"
|
curl -s --max-time 30 "$BASE/api/comments"
|
||||||
```
|
```
|
||||||
|
|
||||||
Skip anything the user already raised, and anything **you** raised on an earlier
|
Skip anything the user already raised, and anything **you** raised on an earlier
|
||||||
@@ -192,7 +199,7 @@ Write each body like a comment in a thread someone has to read:
|
|||||||
## 6. Post them
|
## 6. Post them
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
curl -s -X POST "$BASE/api/comments" -H 'Content-Type: application/json' -d '{
|
curl -s --max-time 30 -X POST "$BASE/api/comments" -H 'Content-Type: application/json' -d '{
|
||||||
"level":"line","file":"src/review/Store.zig","side":"new","line":84,"endLine":91,
|
"level":"line","file":"src/review/Store.zig","side":"new","line":84,"endLine":91,
|
||||||
"author":"claude",
|
"author":"claude",
|
||||||
"body":"Should fix: save() runs while the write lock is held, so a slow disk blocks every reader for the length of the write. Snapshot the list under the lock and write outside it."
|
"body":"Should fix: save() runs while the write lock is held, so a slow disk blocks every reader for the length of the write. Snapshot the list under the lock and write outside it."
|
||||||
|
|||||||
@@ -897,7 +897,7 @@ The endpoints, all under `/t/<tabId>/api`:
|
|||||||
| `GET repo` | the repository, its refs, the comment counts, and the diff selection on screen |
|
| `GET repo` | the repository, its refs, the comment counts, and the diff selection on screen |
|
||||||
| `POST repo/context` | what the page publishes when you change the base ref |
|
| `POST repo/context` | what the page publishes when you change the base ref |
|
||||||
| `GET diff` | `base`, `uncommitted`, `commit`, `force`, `ignoreWhitespace` |
|
| `GET diff` | `base`, `uncommitted`, `commit`, `force`, `ignoreWhitespace` |
|
||||||
| `GET diff/revision` | a digest of what that same selection resolves to now — one hash, so the page can poll it |
|
| `GET diff/revision` | a digest of what that same selection resolves to now — one hash, so the page can poll it; 503 while git is busy with work someone is waiting on |
|
||||||
| `GET file` | a file's contents at a ref, for expanding collapsed context |
|
| `GET file` | a file's contents at a ref, for expanding collapsed context |
|
||||||
| `GET/POST comments` | list, or open a thread |
|
| `GET/POST comments` | list, or open a thread |
|
||||||
| `PATCH/DELETE comments/{id}` | edit or delete one |
|
| `PATCH/DELETE comments/{id}` | edit or delete one |
|
||||||
@@ -923,12 +923,31 @@ itself is not that answer: it would lose your scroll position, your place in a
|
|||||||
hunk, and whatever you had half-typed into a composer.
|
hunk, and whatever you had half-typed into a composer.
|
||||||
|
|
||||||
So the page asks `GET diff/revision` every few seconds — a hash of the patch plus
|
So the page asks `GET diff/revision` every few seconds — a hash of the patch plus
|
||||||
`HEAD`, which is cheap enough to ask for on a timer and catches both an
|
`HEAD`, which catches both an uncommitted edit and work being committed out from
|
||||||
uncommitted edit and work being committed out from under the range. When it stops
|
under the range. When it stops matching the revision the diff came with, a banner
|
||||||
matching the revision the diff came with, a banner says so and offers the refresh.
|
says so and offers the refresh. Dismissing it keeps the diff you are reading and
|
||||||
Dismissing it keeps the diff you are reading and re-arms against what is there
|
re-arms against what is there now, so the *next* change tells you too. Polling
|
||||||
now, so the *next* change tells you too. Polling stops while the pane is hidden,
|
stops while the pane is hidden, and starts again the moment it comes back.
|
||||||
and starts again the moment it comes back.
|
|
||||||
|
The poll is a `git diff`, though, and the interesting case is a review open in
|
||||||
|
seven tabs on a repository that is being built in the terminal pane next door.
|
||||||
|
Left to a fixed interval, that is a git process running in this app most of the
|
||||||
|
time, and the agent's `GET diff` waits behind polls whose answers nobody is
|
||||||
|
reading.
|
||||||
|
|
||||||
|
So the server sorts git into two kinds of work and only ever holds one of them
|
||||||
|
up. Anything with somebody on the other end of it — a diff being opened, the
|
||||||
|
**Refresh** you just clicked, an agent fetching the patch it is about to review —
|
||||||
|
runs the moment it arrives, every time, and is never queued behind anything. A
|
||||||
|
poll runs only while nothing is being waited on and the app is not already busy
|
||||||
|
with a few of them; otherwise it is turned away with a 503 on the spot. That
|
||||||
|
costs nothing, because a poll has no answer anybody is waiting for, and it means
|
||||||
|
a click does not merely jump the queue — it clears the field of polls for as long
|
||||||
|
as it takes.
|
||||||
|
|
||||||
|
Each pane also spaces its own next poll by what the last one actually cost, so a
|
||||||
|
pane on a slow repository asks less often on its own, and a refused one waits
|
||||||
|
longer still. A pane on a small repository never notices any of it.
|
||||||
|
|
||||||
Comments are the other half, and they work the other way round: those arrive over
|
Comments are the other half, and they work the other way round: those arrive over
|
||||||
`GET events` and are applied live, because a thread appearing in the rail doesn't
|
`GET events` and are applied live, because a thread appearing in the rail doesn't
|
||||||
|
|||||||
@@ -232,7 +232,14 @@ export __EGL_VENDOR_LIBRARY_DIRS="$eglvendor"
|
|||||||
export LIBGL_DRIVERS_PATH="$dridrivers"
|
export LIBGL_DRIVERS_PATH="$dridrivers"
|
||||||
export GBM_BACKENDS_PATH="$gbmbackends"
|
export GBM_BACKENDS_PATH="$gbmbackends"
|
||||||
|
|
||||||
exec "$libexec/playpen" "\$@"
|
# Warnings — a git invocation that timed out, a review server that could not
|
||||||
|
# listen — are the only account of why a review pane is behaving oddly, and a
|
||||||
|
# desktop launcher gives the app no terminal to print them to. Truncated per
|
||||||
|
# launch rather than appended, so this stays a log of the session you are in
|
||||||
|
# and never grows without bound.
|
||||||
|
log="\${XDG_STATE_HOME:-\$HOME/.local/state}/playpen"
|
||||||
|
mkdir -p "\$log"
|
||||||
|
exec "$libexec/playpen" "\$@" 2>"\$log/playpen.log"
|
||||||
EOF
|
EOF
|
||||||
chmod 755 "$bindir/playpen"
|
chmod 755 "$bindir/playpen"
|
||||||
|
|
||||||
|
|||||||
+24
-5
@@ -6,6 +6,7 @@
|
|||||||
//! else — process management, rendering, and the GTK4 UI — lives here.
|
//! else — process management, rendering, and the GTK4 UI — lives here.
|
||||||
|
|
||||||
const std = @import("std");
|
const std = @import("std");
|
||||||
|
const builtin = @import("builtin");
|
||||||
const adw = @import("adw");
|
const adw = @import("adw");
|
||||||
const gio = @import("gio");
|
const gio = @import("gio");
|
||||||
|
|
||||||
@@ -21,10 +22,28 @@ pub const std_options: std.Options = .{
|
|||||||
.log_level = .info,
|
.log_level = .info,
|
||||||
};
|
};
|
||||||
|
|
||||||
var gpa: std.heap.DebugAllocator(.{}) = .init;
|
/// The process allocator, which everything in the app shares: the GTK main
|
||||||
|
/// loop, every terminal's parser, and every review connection thread.
|
||||||
|
///
|
||||||
|
/// `DebugAllocator` only in a debug build, and this is not a matter of taste.
|
||||||
|
/// It is thread-safe by way of one mutex around every allocation and every
|
||||||
|
/// free, and its backing allocator is the page allocator — so a large
|
||||||
|
/// allocation is an `mmap` and its release an `munmap`, both taken under that
|
||||||
|
/// single process-wide lock. A review pane's poll allocates a whole `git diff`
|
||||||
|
/// and drops it a moment later, several times a second across a handful of open
|
||||||
|
/// reviews, and each one of those was stalling the main loop and every other
|
||||||
|
/// request behind the same lock. `c_allocator` has a per-thread cache and no
|
||||||
|
/// global lock; libc is already linked for GTK.
|
||||||
|
var debug_gpa: std.heap.DebugAllocator(.{}) = .init;
|
||||||
|
|
||||||
|
fn gpa() std.mem.Allocator {
|
||||||
|
return if (builtin.mode == .Debug) debug_gpa.allocator() else std.heap.c_allocator;
|
||||||
|
}
|
||||||
|
|
||||||
pub fn main() u8 {
|
pub fn main() u8 {
|
||||||
defer _ = gpa.deinit();
|
defer if (builtin.mode == .Debug) {
|
||||||
|
_ = debug_gpa.deinit();
|
||||||
|
};
|
||||||
|
|
||||||
// Before the allocator's own teardown, since the settings arena comes out
|
// Before the allocator's own teardown, since the settings arena comes out
|
||||||
// of it. A no-op if the app never got as far as activating.
|
// of it. A no-op if the app never got as far as activating.
|
||||||
@@ -51,7 +70,7 @@ pub fn main() u8 {
|
|||||||
fn onActivate(app: *adw.Application, _: ?*anyopaque) callconv(.c) void {
|
fn onActivate(app: *adw.Application, _: ?*anyopaque) callconv(.c) void {
|
||||||
// The file both of the next two read from: the scheme, and the tabs the
|
// The file both of the next two read from: the scheme, and the tabs the
|
||||||
// window opens itself with.
|
// window opens itself with.
|
||||||
Settings.init(gpa.allocator());
|
Settings.init(gpa());
|
||||||
|
|
||||||
// Before the window, so that the first frame is drawn in the scheme the
|
// Before the window, so that the first frame is drawn in the scheme the
|
||||||
// user chose rather than repainted into it a moment later.
|
// user chose rather than repainted into it a moment later.
|
||||||
@@ -65,9 +84,9 @@ fn onActivate(app: *adw.Application, _: ?*anyopaque) callconv(.c) void {
|
|||||||
// created and every terminal is handed the endpoint of the tab it opens in.
|
// created and every terminal is handed the endpoint of the tab it opens in.
|
||||||
// Started unconditionally rather than on the first review pane, so an agent
|
// Started unconditionally rather than on the first review pane, so an agent
|
||||||
// running in a tab has a `PLAYPEN_REVIEW_URL` from the moment it starts.
|
// running in a tab has a `PLAYPEN_REVIEW_URL` from the moment it starts.
|
||||||
review.init(gpa.allocator());
|
review.init(gpa());
|
||||||
|
|
||||||
const window = Window.create(gpa.allocator(), app) catch |err| {
|
const window = Window.create(gpa(), app) catch |err| {
|
||||||
std.log.err("failed to create window: {s}", .{@errorName(err)});
|
std.log.err("failed to create window: {s}", .{@errorName(err)});
|
||||||
return;
|
return;
|
||||||
};
|
};
|
||||||
|
|||||||
+111
-11
@@ -69,6 +69,76 @@ const sse_heartbeat_ms = 15_000;
|
|||||||
/// reconnect anyway.
|
/// reconnect anyway.
|
||||||
const sse_backlog = 32;
|
const sse_backlog = 32;
|
||||||
|
|
||||||
|
/// How many git invocations may be in flight before a poll gives way.
|
||||||
|
///
|
||||||
|
/// Every review pane polls for a moved diff on a timer, and every one of those
|
||||||
|
/// polls is a `git diff` over a whole work tree. With a review open in seven
|
||||||
|
/// tabs that is a git process starting somewhere in this app most of the time,
|
||||||
|
/// on top of whatever the terminal beside it is already doing to the same
|
||||||
|
/// repository — which is usually a build. Unbounded, they land on the machine
|
||||||
|
/// all at once and the request someone is actually waiting on finishes last.
|
||||||
|
///
|
||||||
|
/// So polls yield and nothing else does. This is the threshold they yield at;
|
||||||
|
/// `GitLoad` is the rule itself.
|
||||||
|
const git_busy_at = 4;
|
||||||
|
|
||||||
|
/// What git is doing right now, and who for.
|
||||||
|
///
|
||||||
|
/// The distinction this exists to make is between a git invocation somebody is
|
||||||
|
/// waiting on — a diff being opened, a **Refresh** being clicked, an agent
|
||||||
|
/// fetching the patch it is about to review — and one that is a timer firing in
|
||||||
|
/// a page that will not look any different if it never gets an answer.
|
||||||
|
///
|
||||||
|
/// The first kind never waits. Not for a slot, not behind a queue, not for
|
||||||
|
/// anything: a person who clicks refresh has told you exactly what they want and
|
||||||
|
/// making them wait for a poll to finish first is indefensible. It runs
|
||||||
|
/// immediately and every time.
|
||||||
|
///
|
||||||
|
/// The second kind yields to the first. A poll runs only when nothing is being
|
||||||
|
/// waited on and the app is not already busy with `git_busy_at` of them, and
|
||||||
|
/// otherwise is turned away at once — which costs nothing, because the page
|
||||||
|
/// ignores a failed poll and asks again on its next tick. So a click does not
|
||||||
|
/// merely jump the queue; it clears the field of polls for as long as it runs.
|
||||||
|
const GitLoad = struct {
|
||||||
|
mutex: std.Io.Mutex = .init,
|
||||||
|
|
||||||
|
/// Invocations with somebody on the other end of them.
|
||||||
|
waited_on: u32 = 0,
|
||||||
|
|
||||||
|
/// Every invocation in flight, polls included.
|
||||||
|
total: u32 = 0,
|
||||||
|
|
||||||
|
/// Start work someone is waiting on. Never refused, never delayed.
|
||||||
|
fn enter(self: *GitLoad, io: std.Io) void {
|
||||||
|
self.mutex.lockUncancelable(io);
|
||||||
|
defer self.mutex.unlock(io);
|
||||||
|
self.waited_on += 1;
|
||||||
|
self.total += 1;
|
||||||
|
}
|
||||||
|
|
||||||
|
fn leave(self: *GitLoad, io: std.Io) void {
|
||||||
|
self.mutex.lockUncancelable(io);
|
||||||
|
defer self.mutex.unlock(io);
|
||||||
|
self.waited_on -= 1;
|
||||||
|
self.total -= 1;
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Start a poll, if this is a moment for one. False means it is not.
|
||||||
|
fn enterPoll(self: *GitLoad, io: std.Io) bool {
|
||||||
|
self.mutex.lockUncancelable(io);
|
||||||
|
defer self.mutex.unlock(io);
|
||||||
|
if (self.waited_on > 0 or self.total >= git_busy_at) return false;
|
||||||
|
self.total += 1;
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
fn leavePoll(self: *GitLoad, io: std.Io) void {
|
||||||
|
self.mutex.lockUncancelable(io);
|
||||||
|
defer self.mutex.unlock(io);
|
||||||
|
self.total -= 1;
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
/// Cap on a request body. Comment bodies are prose; anything larger is a mistake
|
/// Cap on a request body. Comment bodies are prose; anything larger is a mistake
|
||||||
/// or an attack, and either way there is nothing here worth the memory.
|
/// or an attack, and either way there is nothing here worth the memory.
|
||||||
const max_body = 4 * 1024 * 1024;
|
const max_body = 4 * 1024 * 1024;
|
||||||
@@ -149,6 +219,9 @@ mutex: std.Io.Mutex = .init,
|
|||||||
tabs: std.ArrayListUnmanaged(*Tab) = .empty,
|
tabs: std.ArrayListUnmanaged(*Tab) = .empty,
|
||||||
clients: std.ArrayListUnmanaged(*Client) = .empty,
|
clients: std.ArrayListUnmanaged(*Client) = .empty,
|
||||||
|
|
||||||
|
/// Who git is working for at the moment. See `GitLoad`.
|
||||||
|
git_load: GitLoad = .{},
|
||||||
|
|
||||||
listener: ?std.Io.net.Server = null,
|
listener: ?std.Io.net.Server = null,
|
||||||
port: u16 = 0,
|
port: u16 = 0,
|
||||||
|
|
||||||
@@ -648,7 +721,11 @@ fn handleRepo(
|
|||||||
tab: Resolved,
|
tab: Resolved,
|
||||||
) !void {
|
) !void {
|
||||||
const repo = tab.repo orelse return writeJson(a, request, .ok, .{ .open = false });
|
const repo = tab.repo orelse return writeJson(a, request, .ok, .{ .open = false });
|
||||||
const info = git.info(repo, a, self.io);
|
const info = blk: {
|
||||||
|
self.git_load.enter(self.io);
|
||||||
|
defer self.git_load.leave(self.io);
|
||||||
|
break :blk git.info(repo, a, self.io);
|
||||||
|
};
|
||||||
const store = tab.store.?;
|
const store = tab.store.?;
|
||||||
|
|
||||||
return writeJson(a, request, .ok, .{
|
return writeJson(a, request, .ok, .{
|
||||||
@@ -741,7 +818,11 @@ fn handleDiff(
|
|||||||
.ignore_whitespace = queryFlag(a, query, "ignoreWhitespace") catch false,
|
.ignore_whitespace = queryFlag(a, query, "ignoreWhitespace") catch false,
|
||||||
};
|
};
|
||||||
|
|
||||||
const payload = git.diff(repo, a, self.io, ctx, opts) catch |err| return writeError(
|
self.git_load.enter(self.io);
|
||||||
|
const produced = git.diff(repo, a, self.io, ctx, opts);
|
||||||
|
self.git_load.leave(self.io);
|
||||||
|
|
||||||
|
const payload = produced catch |err| return writeError(
|
||||||
a,
|
a,
|
||||||
request,
|
request,
|
||||||
if (err == error.BadCommit) .bad_request else .bad_gateway,
|
if (err == error.BadCommit) .bad_request else .bad_gateway,
|
||||||
@@ -774,7 +855,28 @@ fn handleRevision(
|
|||||||
.ignore_whitespace = queryFlag(a, query, "ignoreWhitespace") catch false,
|
.ignore_whitespace = queryFlag(a, query, "ignoreWhitespace") catch false,
|
||||||
};
|
};
|
||||||
|
|
||||||
const rev = git.revision(repo, a, self.io, ctx, opts) catch |err| return writeError(
|
// Best-effort, unlike every other git route here: this is a timer firing in
|
||||||
|
// a page that will look no different if the answer never comes, and the
|
||||||
|
// refresh somebody just clicked in the next tab wants git more. A poll that
|
||||||
|
// arrives at a bad moment says so and is asked again on the next tick — see
|
||||||
|
// `GitLoad`, and `POLL_MS` in the page for what it does with a refusal.
|
||||||
|
if (!self.git_load.enterPoll(self.io)) return writeError(
|
||||||
|
a,
|
||||||
|
request,
|
||||||
|
.service_unavailable,
|
||||||
|
"git is busy with something someone is waiting on — poll again shortly",
|
||||||
|
);
|
||||||
|
defer self.git_load.leavePoll(self.io);
|
||||||
|
|
||||||
|
// The branch rides along with the digest so the page can notice that the
|
||||||
|
// work tree moved to another branch — which the digest alone cannot always
|
||||||
|
// say. The digest hashes HEAD's *sha*, so branching off the commit you are
|
||||||
|
// already on (`git switch -c`) leaves it identical while the name in the
|
||||||
|
// review's header is now wrong. Both come out of one `rev-parse`, which is
|
||||||
|
// also the sha the digest wants — see `git.head`.
|
||||||
|
const at = git.head(repo, a, self.io);
|
||||||
|
|
||||||
|
const rev = git.revision(repo, a, self.io, ctx, opts, at.sha) catch |err| return writeError(
|
||||||
a,
|
a,
|
||||||
request,
|
request,
|
||||||
if (err == error.BadCommit) .bad_request else .bad_gateway,
|
if (err == error.BadCommit) .bad_request else .bad_gateway,
|
||||||
@@ -783,15 +885,9 @@ fn handleRevision(
|
|||||||
else => "git could not produce that diff — check the base ref",
|
else => "git could not produce that diff — check the base ref",
|
||||||
},
|
},
|
||||||
);
|
);
|
||||||
// The branch rides along with the digest so the page can notice that the
|
|
||||||
// work tree moved to another branch — which the digest alone cannot always
|
|
||||||
// say. `digest` hashes HEAD's *sha*, so branching off the commit you are
|
|
||||||
// already on (`git switch -c`) leaves it identical while the name in the
|
|
||||||
// review's header is now wrong. One `rev-parse` next to the `git diff` this
|
|
||||||
// endpoint already runs is not a cost worth avoiding.
|
|
||||||
return writeJson(a, request, .ok, .{
|
return writeJson(a, request, .ok, .{
|
||||||
.revision = rev,
|
.revision = rev,
|
||||||
.branch = git.currentBranch(repo, a, self.io),
|
.branch = at.branch,
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -812,7 +908,11 @@ fn handleFile(
|
|||||||
|
|
||||||
// A miss is ordinary — a newly added file has no base version — so this is a
|
// A miss is ordinary — a newly added file has no base version — so this is a
|
||||||
// 404 with no detail rather than something the UI has to explain.
|
// 404 with no detail rather than something the UI has to explain.
|
||||||
const content = git.fileAt(repo, a, self.io, ref, path) catch return notFound(request);
|
self.git_load.enter(self.io);
|
||||||
|
const read = git.fileAt(repo, a, self.io, ref, path);
|
||||||
|
self.git_load.leave(self.io);
|
||||||
|
|
||||||
|
const content = read catch return notFound(request);
|
||||||
return request.respond(content, .{
|
return request.respond(content, .{
|
||||||
.extra_headers = &.{.{ .name = "content-type", .value = "text/plain; charset=utf-8" }},
|
.extra_headers = &.{.{ .name = "content-type", .value = "text/plain; charset=utf-8" }},
|
||||||
});
|
});
|
||||||
|
|||||||
+64
-8
@@ -25,6 +25,14 @@ const model = @import("model.zig");
|
|||||||
/// forever.
|
/// forever.
|
||||||
const timeout_s = 60;
|
const timeout_s = 60;
|
||||||
|
|
||||||
|
/// The same, for the poll a review pane runs on a timer.
|
||||||
|
///
|
||||||
|
/// Much shorter, because a poll is disposable and holds one of the app's git
|
||||||
|
/// slots while it runs: a wedged one would otherwise keep a slot from the diff
|
||||||
|
/// somebody is waiting to open for a whole minute, and the answer it eventually
|
||||||
|
/// came back with would be a minute stale anyway.
|
||||||
|
const poll_timeout_s = 15;
|
||||||
|
|
||||||
/// Cap on what one git invocation may print. A patch is the big one: the
|
/// Cap on what one git invocation may print. A patch is the big one: the
|
||||||
/// oversize guard below is what normally keeps it in hand, and this is the
|
/// oversize guard below is what normally keeps it in hand, and this is the
|
||||||
/// backstop for the cases the guard cannot see coming.
|
/// backstop for the cases the guard cannot see coming.
|
||||||
@@ -71,6 +79,37 @@ pub fn open(gpa: std.mem.Allocator, io: std.Io, path: []const u8) Error!Repo {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// The short name of the checked-out branch, or "HEAD" when detached.
|
/// The short name of the checked-out branch, or "HEAD" when detached.
|
||||||
|
/// What HEAD is, both ways, from a single `rev-parse`.
|
||||||
|
///
|
||||||
|
/// The poll wants the sha (it goes into the digest) and the branch name (the
|
||||||
|
/// page notices a `git switch` with it) on every tick, and asking for them
|
||||||
|
/// separately was two process spawns where git will answer both from one — a
|
||||||
|
/// difference worth having when this runs once per review pane per tick.
|
||||||
|
///
|
||||||
|
/// A repository with no commits has no HEAD to resolve, which is not an error
|
||||||
|
/// here: the sha comes back empty — the digest is then the patch alone, exactly
|
||||||
|
/// as `digest` has always handled it — and the name falls back to `HEAD`, which
|
||||||
|
/// is what `currentBranch` answers for the same case.
|
||||||
|
pub const Head = struct {
|
||||||
|
sha: []const u8,
|
||||||
|
branch: []const u8,
|
||||||
|
};
|
||||||
|
|
||||||
|
pub fn head(repo: Repo, gpa: std.mem.Allocator, io: std.Io) Head {
|
||||||
|
const out = runFor(gpa, io, repo.path, &.{
|
||||||
|
"rev-parse", "HEAD", "--abbrev-ref", "HEAD",
|
||||||
|
}, poll_timeout_s) catch return .{ .sha = "", .branch = "HEAD" };
|
||||||
|
|
||||||
|
// Two lines, in the order the arguments were given: the sha, then the name.
|
||||||
|
var it = std.mem.splitScalar(u8, trim(out), '\n');
|
||||||
|
const sha = trim(it.next() orelse "");
|
||||||
|
const branch = trim(it.next() orelse "");
|
||||||
|
return .{
|
||||||
|
.sha = sha,
|
||||||
|
.branch = if (branch.len == 0) "HEAD" else branch,
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
pub fn currentBranch(repo: Repo, gpa: std.mem.Allocator, io: std.Io) []const u8 {
|
pub fn currentBranch(repo: Repo, gpa: std.mem.Allocator, io: std.Io) []const u8 {
|
||||||
const out = run(gpa, io, repo.path, &.{ "rev-parse", "--abbrev-ref", "HEAD" }) catch
|
const out = run(gpa, io, repo.path, &.{ "rev-parse", "--abbrev-ref", "HEAD" }) catch
|
||||||
return "HEAD";
|
return "HEAD";
|
||||||
@@ -360,13 +399,14 @@ pub fn revision(
|
|||||||
io: std.Io,
|
io: std.Io,
|
||||||
ctx: model.DiffContext,
|
ctx: model.DiffContext,
|
||||||
opts: Options,
|
opts: Options,
|
||||||
|
head_sha: []const u8,
|
||||||
) Error![]const u8 {
|
) Error![]const u8 {
|
||||||
try validateCommit(ctx.commit);
|
try validateCommit(ctx.commit);
|
||||||
const patch = try run(gpa, io, repo.path, try diffArgs(gpa, ctx, opts, &.{
|
const patch = try runFor(gpa, io, repo.path, try diffArgs(gpa, ctx, opts, &.{
|
||||||
"--no-color",
|
"--no-color",
|
||||||
"--find-renames",
|
"--find-renames",
|
||||||
}));
|
}), poll_timeout_s);
|
||||||
return digest(repo, gpa, io, patch);
|
return digestOf(gpa, head_sha, patch);
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The digest itself, given a patch already in hand: the patch, hashed, plus
|
/// The digest itself, given a patch already in hand: the patch, hashed, plus
|
||||||
@@ -387,9 +427,15 @@ fn digest(
|
|||||||
) Error![]const u8 {
|
) Error![]const u8 {
|
||||||
// Best-effort: a repository with no commits has no HEAD to resolve, and the
|
// Best-effort: a repository with no commits has no HEAD to resolve, and the
|
||||||
// patch on its own is still a usable digest.
|
// patch on its own is still a usable digest.
|
||||||
const head = run(gpa, io, repo.path, &.{ "rev-parse", "HEAD" }) catch "";
|
const at = run(gpa, io, repo.path, &.{ "rev-parse", "HEAD" }) catch "";
|
||||||
|
return digestOf(gpa, trim(at), patch);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The hash itself, for a caller that has already resolved HEAD — which the
|
||||||
|
/// poll has, because it needs the branch name from the same `rev-parse`.
|
||||||
|
fn digestOf(gpa: std.mem.Allocator, head_sha: []const u8, patch: []const u8) Error![]const u8 {
|
||||||
var hasher = std.hash.Wyhash.init(0);
|
var hasher = std.hash.Wyhash.init(0);
|
||||||
hasher.update(trim(head));
|
hasher.update(head_sha);
|
||||||
hasher.update(patch);
|
hasher.update(patch);
|
||||||
return std.fmt.allocPrint(gpa, "{x}", .{hasher.final()});
|
return std.fmt.allocPrint(gpa, "{x}", .{hasher.final()});
|
||||||
}
|
}
|
||||||
@@ -660,6 +706,16 @@ fn run(
|
|||||||
io: std.Io,
|
io: std.Io,
|
||||||
dir: []const u8,
|
dir: []const u8,
|
||||||
args: []const []const u8,
|
args: []const []const u8,
|
||||||
|
) Error![]const u8 {
|
||||||
|
return runFor(gpa, io, dir, args, timeout_s);
|
||||||
|
}
|
||||||
|
|
||||||
|
fn runFor(
|
||||||
|
gpa: std.mem.Allocator,
|
||||||
|
io: std.Io,
|
||||||
|
dir: []const u8,
|
||||||
|
args: []const []const u8,
|
||||||
|
seconds: u32,
|
||||||
) Error![]const u8 {
|
) Error![]const u8 {
|
||||||
var argv: std.ArrayListUnmanaged([]const u8) = .empty;
|
var argv: std.ArrayListUnmanaged([]const u8) = .empty;
|
||||||
try argv.ensureTotalCapacity(gpa, args.len + 1);
|
try argv.ensureTotalCapacity(gpa, args.len + 1);
|
||||||
@@ -671,7 +727,7 @@ fn run(
|
|||||||
.cwd = .{ .path = dir },
|
.cwd = .{ .path = dir },
|
||||||
.stdout_limit = .limited(max_output),
|
.stdout_limit = .limited(max_output),
|
||||||
.stderr_limit = .limited(64 * 1024),
|
.stderr_limit = .limited(64 * 1024),
|
||||||
.timeout = .{ .duration = .{ .raw = .fromSeconds(timeout_s), .clock = .awake } },
|
.timeout = .{ .duration = .{ .raw = .fromSeconds(seconds), .clock = .awake } },
|
||||||
}) catch |err| {
|
}) catch |err| {
|
||||||
std.log.warn("review: git {s}: {s}", .{ args[0], @errorName(err) });
|
std.log.warn("review: git {s}: {s}", .{ args[0], @errorName(err) });
|
||||||
return error.GitFailed;
|
return error.GitFailed;
|
||||||
@@ -850,8 +906,8 @@ test "diffArgs picks the right git subcommand for each selection" {
|
|||||||
}), one);
|
}), one);
|
||||||
|
|
||||||
// An empty base is HEAD, which is what a review opens on.
|
// An empty base is HEAD, which is what a review opens on.
|
||||||
const head = try diffArgs(a, .{ .uncommitted = true }, .{}, &.{});
|
const bare = try diffArgs(a, .{ .uncommitted = true }, .{}, &.{});
|
||||||
try std.testing.expectEqualDeep(@as([]const []const u8, &.{ "diff", "HEAD" }), head);
|
try std.testing.expectEqualDeep(@as([]const []const u8, &.{ "diff", "HEAD" }), bare);
|
||||||
}
|
}
|
||||||
|
|
||||||
test "oversized counts lines, and files for the binary case" {
|
test "oversized counts lines, and files for the binary case" {
|
||||||
|
|||||||
+42
-6
@@ -1,7 +1,7 @@
|
|||||||
import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
|
import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
|
||||||
import { parseDiff, type ViewType } from 'react-diff-view';
|
import { parseDiff, type ViewType } from 'react-diff-view';
|
||||||
|
|
||||||
import { api, tabId } from './api';
|
import { ApiError, api, tabId } from './api';
|
||||||
import type { Comment, DiffContext, DiffPayload, DraftTarget, RepoState } from './types';
|
import type { Comment, DiffContext, DiffPayload, DraftTarget, RepoState } from './types';
|
||||||
import { buildAnchors, isOutdated } from './lib/anchor';
|
import { buildAnchors, isOutdated } from './lib/anchor';
|
||||||
import { pathOf } from './lib/filetree';
|
import { pathOf } from './lib/filetree';
|
||||||
@@ -58,6 +58,23 @@ const MIN_DIFF = 420;
|
|||||||
// entirely while the pane is hidden or already known to be stale.
|
// entirely while the pane is hidden or already known to be stale.
|
||||||
const POLL_MS = 8000;
|
const POLL_MS = 8000;
|
||||||
|
|
||||||
|
// ...but "isn't free" stops being true when a review is open in seven tabs and
|
||||||
|
// the repository is a big one being built in the terminal next door. Every pane
|
||||||
|
// polling on the same fixed interval is how the app ends up with a git process
|
||||||
|
// running most of the time, and the request an agent is waiting on ends up
|
||||||
|
// queued behind polls whose answers nobody reads.
|
||||||
|
//
|
||||||
|
// So the interval is a floor, not a period: each pane spaces its next poll by
|
||||||
|
// what the last one actually cost, which means panes on a slow repository — the
|
||||||
|
// only ones expensive enough to matter — quietly ask less often, and a pane on a
|
||||||
|
// small one carries on at POLL_MS. `POLL_COST` is how many times the last poll's
|
||||||
|
// duration to wait, and the server's own "git is busy" refusal (503) counts as
|
||||||
|
// expensive whatever it cost, because it is the app telling this pane it is one
|
||||||
|
// of too many.
|
||||||
|
const POLL_MAX_MS = 60000;
|
||||||
|
const POLL_COST = 6;
|
||||||
|
const POLL_BUSY_MS = 20000;
|
||||||
|
|
||||||
const CTX_KEY = 'review-ctx-by-repo';
|
const CTX_KEY = 'review-ctx-by-repo';
|
||||||
const IGNORE_WS_KEY = 'review-ignore-whitespace';
|
const IGNORE_WS_KEY = 'review-ignore-whitespace';
|
||||||
|
|
||||||
@@ -411,17 +428,29 @@ export default function App() {
|
|||||||
|
|
||||||
let canceled = false;
|
let canceled = false;
|
||||||
let busy = false;
|
let busy = false;
|
||||||
|
let timer: number | undefined;
|
||||||
|
|
||||||
|
// Each poll arms the next one rather than a fixed interval doing it, so the
|
||||||
|
// spacing can answer to what the last one cost — see POLL_MAX_MS.
|
||||||
|
const arm = (ms: number) => {
|
||||||
|
window.clearTimeout(timer);
|
||||||
|
if (!canceled) timer = window.setTimeout(check, ms);
|
||||||
|
};
|
||||||
|
|
||||||
const check = async () => {
|
const check = async () => {
|
||||||
// A hidden pane is a pane nobody is reading. It gets checked the moment it
|
// A hidden pane is a pane nobody is reading. It gets checked the moment it
|
||||||
// comes back instead, which is when the answer matters.
|
// comes back instead, which is when the answer matters.
|
||||||
if (canceled || busy || document.hidden) return;
|
if (canceled || busy) return;
|
||||||
|
if (document.hidden) return arm(POLL_MS);
|
||||||
busy = true;
|
busy = true;
|
||||||
const seq = reqRef.current;
|
const seq = reqRef.current;
|
||||||
|
const started = performance.now();
|
||||||
|
let next = POLL_MS;
|
||||||
try {
|
try {
|
||||||
const { revision: now, branch: on } = await api.revision(ctx, {
|
const { revision: now, branch: on } = await api.revision(ctx, {
|
||||||
ignoreWhitespace: ignoreWs,
|
ignoreWhitespace: ignoreWs,
|
||||||
});
|
});
|
||||||
|
next = Math.max(POLL_MS, Math.round((performance.now() - started) * POLL_COST));
|
||||||
// A load that started while this was in flight has already answered the
|
// A load that started while this was in flight has already answered the
|
||||||
// question, with a revision this closure doesn't know about.
|
// question, with a revision this closure doesn't know about.
|
||||||
if (canceled || seq !== reqRef.current) return;
|
if (canceled || seq !== reqRef.current) return;
|
||||||
@@ -432,14 +461,21 @@ export default function App() {
|
|||||||
// you were reading, and the banner is what offers to move that.
|
// you were reading, and the banner is what offers to move that.
|
||||||
if (on && on !== branch) loadRepo();
|
if (on && on !== branch) loadRepo();
|
||||||
if (now && now !== revision) setStale(now);
|
if (now && now !== revision) setStale(now);
|
||||||
} catch {
|
} catch (e) {
|
||||||
// A failed poll says nothing about the diff — the next one will.
|
// A failed poll says nothing about the diff — the next one will. A 503
|
||||||
|
// says something about the app, though: too many panes are asking git
|
||||||
|
// for too much at once, and this one is part of that.
|
||||||
|
next =
|
||||||
|
e instanceof ApiError && e.status === 503
|
||||||
|
? POLL_BUSY_MS
|
||||||
|
: POLL_MS;
|
||||||
} finally {
|
} finally {
|
||||||
busy = false;
|
busy = false;
|
||||||
|
arm(Math.min(POLL_MAX_MS, next));
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
const timer = window.setInterval(check, POLL_MS);
|
arm(POLL_MS);
|
||||||
const onVisible = () => {
|
const onVisible = () => {
|
||||||
if (!document.hidden) check();
|
if (!document.hidden) check();
|
||||||
};
|
};
|
||||||
@@ -447,7 +483,7 @@ export default function App() {
|
|||||||
window.addEventListener('focus', onVisible);
|
window.addEventListener('focus', onVisible);
|
||||||
return () => {
|
return () => {
|
||||||
canceled = true;
|
canceled = true;
|
||||||
window.clearInterval(timer);
|
window.clearTimeout(timer);
|
||||||
document.removeEventListener('visibilitychange', onVisible);
|
document.removeEventListener('visibilitychange', onVisible);
|
||||||
window.removeEventListener('focus', onVisible);
|
window.removeEventListener('focus', onVisible);
|
||||||
};
|
};
|
||||||
|
|||||||
+16
-2
@@ -32,6 +32,20 @@ export const tabId = (() => {
|
|||||||
return match ? decodeURIComponent(match[1]) : '';
|
return match ? decodeURIComponent(match[1]) : '';
|
||||||
})();
|
})();
|
||||||
|
|
||||||
|
// An error the server answered with, carrying the status alongside the sentence.
|
||||||
|
// The status matters in one place: a 503 from the poll is the server rationing
|
||||||
|
// git rather than anything being wrong, and the poll backs off instead of asking
|
||||||
|
// again on the same tick — see the poll effect in App.
|
||||||
|
export class ApiError extends Error {
|
||||||
|
status: number;
|
||||||
|
|
||||||
|
constructor(status: number, message: string) {
|
||||||
|
super(message);
|
||||||
|
this.name = 'ApiError';
|
||||||
|
this.status = status;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
async function json<T>(res: Response): Promise<T> {
|
async function json<T>(res: Response): Promise<T> {
|
||||||
if (!res.ok) {
|
if (!res.ok) {
|
||||||
const body = await res.text();
|
const body = await res.text();
|
||||||
@@ -39,11 +53,11 @@ async function json<T>(res: Response): Promise<T> {
|
|||||||
// beats surfacing a status code, because it is written for a person.
|
// beats surfacing a status code, because it is written for a person.
|
||||||
try {
|
try {
|
||||||
const parsed = JSON.parse(body) as { error?: string };
|
const parsed = JSON.parse(body) as { error?: string };
|
||||||
if (parsed?.error) throw new Error(parsed.error);
|
if (parsed?.error) throw new ApiError(res.status, parsed.error);
|
||||||
} catch (e) {
|
} catch (e) {
|
||||||
if (e instanceof Error && e.message && !e.message.startsWith('Unexpected')) throw e;
|
if (e instanceof Error && e.message && !e.message.startsWith('Unexpected')) throw e;
|
||||||
}
|
}
|
||||||
throw new Error(`${res.status} ${res.statusText}: ${body}`);
|
throw new ApiError(res.status, `${res.status} ${res.statusText}: ${body}`);
|
||||||
}
|
}
|
||||||
if (res.status === 204) return undefined as T;
|
if (res.status === 204) return undefined as T;
|
||||||
return res.json() as Promise<T>;
|
return res.json() as Promise<T>;
|
||||||
|
|||||||
Reference in New Issue
Block a user