From d89c0b532df23ed5b48495f83725893d5d82042b Mon Sep 17 00:00:00 2001 From: Gabriel Schneider Date: Thu, 3 Sep 2026 15:39:43 -0300 Subject: errors: a save that could not happen, and two panics on an ordinary click MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review of what this program does when the environment says no. The finding that reframes it: there were almost NO panics on ordinary paths — the rule already held — but there was a great deal of silence, and one case worse than any panic. SILENT DATA LOSS ON SAVE. `saveFile` marked the pane saved the moment it QUEUED the effect, before any host had tried; `host_io.writeFd` returned void, so a short or failed write was indistinguishable from a complete one; and `writeFileBytes` returned true regardless. A save to a read-only file, or into a directory removed under the pane, therefore cleared the tag's ` *` and posted nothing — and `Del` makes no dirty check, so the next click threw the edits away with the screen saying they were safe. On a full disk it was worse: the file is already `O_TRUNC`'d when `write` fails, so the message row said `saved` over a file that had just been emptied. Now: `writeFd` reports, `writeFileBytes` returns WHY (`PermissionDenied`, `NoSpaceLeft`, `ReadOnlyFilesystem`, …) including a failed `close`, which is where write-back filesystems report at all; the core marks the pane saved around `perform` rather than at emit, which is also where the bytes are read; and a host that could not write calls `Pardes.saveFailed`, which puts the reason on the message row and takes the clean mark back. That is a CALL and not a return value because host.zig enforces, at comptime, that a `push_` method reaching every host in a fan-out cannot have one answer — the first attempt at this changed the signature and the compiler was right to refuse it. TWO PANICS ON AN ORDINARY KEYSTROKE, in look.zig's number scans. `v = v * 10 + d` over caller-supplied digits, reached from `parsePathLine` and the `@pN` scan — which every Look, every right-click and every n/N motion runs on whatever word is under the pointer. A hash in a log, a CSV column, any output shaped `foo:99999999999999999999`, and the editor died with "integer overflow". Both saturate now, the same way acmefs.zig's address parser already did; a saturated line is refused by `file_pane.open`'s `line <= total` and a saturated pane id by `focusPaneLine`'s `id < MAX_PANES`, so nothing addressable changes. A BOOT FILE THAT WILL NOT OPEN joins the missing-name case in the `+Errors` pane instead of taking the launch down: `pardes /root` resolves as a `.file`, could not be read, and left `error: PermissionDenied` and a return trace. `look.readFile` now says which errno it was, so the pane can say "permission denied" rather than a word from the source code. The tag-marker test drained no effects and passed anyway, which is exactly the defect; it drains now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016Q4RATpafkwahrovHQLKRf --- src/detached/server.zig | 5 +- src/gui/gui.zig | 13 +++-- src/host.zig | 24 ++++++-- src/host_io.zig | 50 ++++++++++++++--- src/look.zig | 60 +++++++++++++++++++- src/macos.zig | 7 ++- src/pardes.zig | 144 +++++++++++++++++++++++++++++++++++++++++++++--- src/tty/tty.zig | 11 +++- src/web.zig | 2 + 9 files changed, 277 insertions(+), 39 deletions(-) (limited to 'src') diff --git a/src/detached/server.zig b/src/detached/server.zig index bc293dce..db93e2e4 100644 --- a/src/detached/server.zig +++ b/src/detached/server.zig @@ -873,7 +873,8 @@ pub const Session = struct { fn writeFile(ctx: ?*anyopaque, pane: u8, path: []const u8, bytes: []const u8) void { const s = of(ctx); - if (!host_io.writeFileBytes(path, bytes)) return; + host_io.writeFileBytes(path, bytes) catch |err| + return s.core.saveFailed(pane, "save", err); // Our own write is about to come back as an inotify edge: restamp from // the bytes we just put there so the reconcile reads as "no change". // Only when this IS the pane's watched file — a `Save ` must @@ -898,7 +899,7 @@ pub const Session = struct { const s = of(ctx); var pbuf: [1024:0]u8 = undefined; const path = pardes.dump.outPath(&pbuf) orelse return; - if (!host_io.writeFileBytes(path, bytes)) return; + host_io.writeFileBytes(path, bytes) catch |err| return s.core.reportError(0, "dump", err); // Where it landed, which is what puts `Restore ` in the topbar // (pardes.zig `write_dump`). A dump of a detached session now lands in // the same directory a terminal session's does, rather than in whatever diff --git a/src/gui/gui.zig b/src/gui/gui.zig index 44e28125..a87eebeb 100644 --- a/src/gui/gui.zig +++ b/src/gui/gui.zig @@ -2806,7 +2806,7 @@ fn dumpGrid(gpa: std.mem.Allocator, surface: *pardes.Surface) !void { @memcpy(frame[at..][0..footer.len], footer); at += footer.len; std.debug.assert(at == frame.len); - host_io.writeFd(1, frame); + _ = host_io.writeFd(1, frame); } // ===================================================================== @@ -3890,7 +3890,7 @@ fn spawnPane(ctx: ?*anyopaque, pane: u8, cwd: []const u8) void { fn ptyWrite(ctx: ?*anyopaque, pane: u8, bytes: []const u8) void { const s = shellOf(ctx); - if (s.ptys[pane]) |pt| host_io.writeFd(pt.fd, bytes); + if (s.ptys[pane]) |pt| _ = host_io.writeFd(pt.fd, bytes); } fn ptyResize(ctx: ?*anyopaque, pane: u8, cols: u16, rows: u16) void { @@ -3920,7 +3920,8 @@ fn ttyTaken(ctx: ?*anyopaque, pane: u8) bool { fn writeFile(ctx: ?*anyopaque, pane: u8, path: []const u8, bytes: []const u8) void { const s = shellOf(ctx); - if (!host_io.writeFileBytes(path, bytes)) return; + host_io.writeFileBytes(path, bytes) catch |err| + return s.core.saveFailed(pane, "save", err); // our own write is about to come back as a watch event: restamp from the // bytes we just put there so it reads as "no change". Only for the pane's // OWN file — a `Put` elsewhere is a change like any other. @@ -3941,7 +3942,7 @@ fn writeDump(ctx: ?*anyopaque, bytes: []const u8) void { const s = shellOf(ctx); var pbuf: [1024:0]u8 = undefined; const path = pardes.dump.outPath(&pbuf) orelse return; - if (!host_io.writeFileBytes(path, bytes)) return; + host_io.writeFileBytes(path, bytes) catch |err| return s.core.reportError(0, "dump", err); s.core.setLastDump(path); } @@ -6048,7 +6049,7 @@ fn writeCapturePpm(g: *Gui, gpa: std.mem.Allocator, pixels: []const u8, width: u var header: [64]u8 = undefined; const hdr = std.fmt.bufPrint(&header, "P6\n{d} {d}\n255\n", .{ width, height }) catch return error.CaptureWriteFailed; - host_io.writeFd(fd, hdr); + _ = host_io.writeFd(fd, hdr); const row_rgb = try gpa.alloc(u8, @as(usize, width) * 3); defer gpa.free(row_rgb); @@ -6061,7 +6062,7 @@ fn writeCapturePpm(g: *Gui, gpa: std.mem.Allocator, pixels: []const u8, width: u row_rgb[di + 1] = src[si + 1]; row_rgb[di + 2] = src[si + if (bgr) @as(usize, 0) else 2]; } - host_io.writeFd(fd, row_rgb); + _ = host_io.writeFd(fd, row_rgb); } if (libc.rename(tmp_path, final_path) != 0) return error.CaptureWriteFailed; } diff --git a/src/host.zig b/src/host.zig index d5f5c983..2c6bd208 100644 --- a/src/host.zig +++ b/src/host.zig @@ -116,6 +116,17 @@ pub const Host = struct { /// `pane` travels with the bytes only so a host that posts a "saved" /// message row can name the right pane; the core already resolved the /// path and the content, so save_file and save_text both land here. + /// + /// A HOST THAT COULD NOT WRITE MUST CALL `Pardes.saveFailed`, and the + /// reason it is a call rather than a return value is the rule twenty + /// lines below: a `push_` reaches every host in a fan-out, so there is + /// no single answer to give back. The core marks the pane saved + /// optimistically around this call and `saveFailed` takes it back, so a + /// write that could not happen — a read-only file, a directory removed + /// under the pane, a full disk — leaves the ` *` in the tag where it + /// was. Until that existed the pane came clean on a save that never + /// happened, and `Del` makes no dirty check: the edits were one click + /// from gone with the screen saying they were safe. push_write_file: ?*const fn (ctx: ?*anyopaque, pane: u8, path: []const u8, bytes: []const u8) void = null, /// The session dump. Separate because the host also chooses WHERE it /// goes (dump.outPath is libc-bound; the freestanding core cannot). @@ -188,21 +199,26 @@ pub const Fallback = struct { f.link.deinit(f.gpa); } - pub fn writeFile(f: *Fallback, path: []const u8, bytes: []const u8) void { - const copy = f.gpa.dupe(u8, bytes) catch return; + /// True when the bytes are in the map. The core turns a false into the same + /// `saveFailed` a real host reports, so a virtual filesystem that could not + /// allocate does not leave a pane looking saved either. + pub fn writeFile(f: *Fallback, path: []const u8, bytes: []const u8) bool { + const copy = f.gpa.dupe(u8, bytes) catch return false; if (f.files.getEntry(path)) |e| { f.gpa.free(e.value_ptr.*); e.value_ptr.* = copy; - return; + return true; } const key = f.gpa.dupe(u8, path) catch { f.gpa.free(copy); - return; + return false; }; f.files.put(f.gpa, key, copy) catch { f.gpa.free(key); f.gpa.free(copy); + return false; }; + return true; } /// What this path holds now: the session's own write, else the embedded diff --git a/src/host_io.zig b/src/host_io.zig index 001446b1..6ffc890e 100644 --- a/src/host_io.zig +++ b/src/host_io.zig @@ -136,16 +136,43 @@ pub fn forkShell( /// Truncate-or-create `path` and put `bytes` there. False on any failure, and /// the caller reports it: a save that did not happen must not be announced as /// one. -pub fn writeFileBytes(path: []const u8, bytes: []const u8) bool { +/// WHY it failed, and not merely that it did. A save is the one operation in +/// this program whose failure a user must not be able to miss, and until this +/// returned an error there was nothing for a host to put on the message row: +/// the bool said "no" and every caller answered it with a bare `return`. +/// `NoSpaceLeft` is the one that most needs saying — the file has already been +/// truncated by the time it happens, so a save that reports nothing has +/// destroyed the file it was asked to preserve. +pub const WriteError = error{ + PathTooLong, + PermissionDenied, + IsDirectory, + ReadOnlyFilesystem, + NoSpaceLeft, + OpenFailed, + WriteFailed, +}; + +pub fn writeFileBytes(path: []const u8, bytes: []const u8) WriteError!void { var pathbuf: [4096:0]u8 = undefined; - if (path.len >= pathbuf.len) return false; + if (path.len >= pathbuf.len) return error.PathTooLong; @memcpy(pathbuf[0..path.len], path); pathbuf[path.len] = 0; const fd = libc.open(pathbuf[0..path.len :0], .{ .ACCMODE = .WRONLY, .CREAT = true, .TRUNC = true }, @as(libc.mode_t, 0o644)); - if (fd < 0) return false; - writeFd(fd, bytes); - _ = libc.close(fd); - return true; + if (fd < 0) return switch (libc.errno(fd)) { + .ACCES, .PERM => error.PermissionDenied, + .ISDIR => error.IsDirectory, + .ROFS => error.ReadOnlyFilesystem, + .NOSPC, .DQUOT => error.NoSpaceLeft, + .NAMETOOLONG => error.PathTooLong, + else => error.OpenFailed, + }; + const wrote = writeFd(fd, bytes); + // The close is part of the write. NFS and every write-back filesystem + // report a deferred error here and nowhere else, so a close that fails on a + // file we believe we wrote is a file we did not write. + const closed = libc.close(fd) == 0; + if (!wrote or !closed) return error.WriteFailed; } /// A whole-buffer write that finishes short writes, retries EINTR, and refuses @@ -164,15 +191,20 @@ pub fn writeFileBytes(path: []const u8, bytes: []const u8) bool { /// matters more now that detached/server.zig reaches this file from a /// single-threaded poll loop: a blocked `write` is one syscall a signal can /// interrupt, and a spin is 100% of a core with the whole session behind it. -pub fn writeFd(fd: c_int, data: []const u8) void { +/// True when every byte went. The answer is new: this used to return `void`, so +/// a full disk and a completed write were the same event to every caller — and +/// the one caller that matters had already truncated the file. A pty write +/// ignores it, which is what `_ =` at those call sites means. +pub fn writeFd(fd: c_int, data: []const u8) bool { var off: usize = 0; while (off < data.len) { const n = libc.write(fd, data[off..].ptr, data.len - off); if (n < 0) { if (libc.errno(n) == .INTR) continue; - return; + return false; } - if (n == 0) return; + if (n == 0) return false; off += @intCast(n); } + return true; } diff --git a/src/look.zig b/src/look.zig index 1730a456..f5d1488c 100644 --- a/src/look.zig +++ b/src/look.zig @@ -76,7 +76,17 @@ pub const Spot = struct { fn num(tok: []const u8, i: usize) struct { v: usize, end: usize } { var v: usize = 0; var j = i; - while (j < tok.len and std.ascii.isDigit(tok[j])) : (j += 1) v = v * 10 + (tok[j] - '0'); + // SATURATING, and this is not defensive programming — it is the fix for a + // crash on an ordinary keystroke. The digits come off whatever word is + // under the pointer, so `*` and `+` here run on text the user never wrote + // and cannot control: a right-click, an Enter, or an `n` on anything shaped + // `foo:99999999999999999999` — a hash in a log, a column of a CSV, the + // output of any program — overflowed a `usize` and took the editor down + // with "integer overflow". A number too big to be a line is not a line, and + // `maxInt` is refused by every consumer for free: `file_pane.open` asks + // `line <= total` and `focusPaneLine` asks `id < MAX_PANES`. Same shape + // acmefs.zig's address parser already uses. + while (j < tok.len and std.ascii.isDigit(tok[j])) : (j += 1) v = v *| 10 +| (tok[j] - '0'); return .{ .v = v, .end = j }; } @@ -175,6 +185,35 @@ test "parsePathLine: spots, ranges, and the paths that merely look like them" { } } +test "a number too big to be a line saturates instead of taking the editor down" { + // These are keystrokes, not arguments. `parsePathLine` runs on whatever + // word is under the pointer on a right-click, an Enter or an `n` — so the + // digits come out of a hash in a log, a CSV column, or any program's + // output, and an unchecked `v * 10` there is a panic on ordinary use. Every + // number in the token comes through the same scan, so all four are tried. + const huge = "99999999999999999999999999"; + const cases = [_][]const u8{ + "f.zig:" ++ huge, + "f.zig:" ++ huge ++ ":" ++ huge, + "f.zig:1-" ++ huge, + "f.zig:1:2-" ++ huge ++ ":" ++ huge, + }; + for (cases) |tok| { + const got = parsePathLine(tok); + try std.testing.expectEqualStrings("f.zig", got.path); + // Saturated rather than wrapped: a wrap would address a REAL line, and + // silently jumping somewhere is worse than not jumping. + try std.testing.expect(got.at.line >= 1); + } + + // ...and the pane address, which has its own scan. `focusPaneLine` refuses + // anything past MAX_PANES, so this resolves to a pane that cannot exist. + var realbuf: [4096]u8 = undefined; + const target = resolve("@p" ++ huge, "/tmp", &realbuf); + try std.testing.expect(target == .pane); + try std.testing.expect(target.pane.id >= 16); +} + test "parsePathLine: `end` separates a whole-token target from a lenient read" { // the whole token IS the target: every spelling the doc above lists for ([_][]const u8{ @@ -469,7 +508,10 @@ pub fn resolve(word_raw: []const u8, cwd: []const u8, realbuf: *[4096]u8) Target var id: usize = 0; for (word[config.pane_addr.len..]) |c| { if (!std.ascii.isDigit(c)) break; - id = id * 10 + (c - '0'); + // Saturating for the same reason `num` above is: this scan also + // runs on a word somebody merely clicked. `focusPaneLine` refuses + // anything past `MAX_PANES`, so a saturated id addresses nothing. + id = id *| 10 +| (c - '0'); } else return .{ .pane = .{ .id = id, .at = pl.at } }; } @@ -816,7 +858,19 @@ pub fn readFile(gpa: std.mem.Allocator, path: []const u8) ![]u8 { var pathbuf: [4096]u8 = undefined; const path_z = std.fmt.bufPrintSentinel(&pathbuf, "{s}", .{path}, 0) catch return error.PathTooLong; const fd = libc.open(path_z, .{ .ACCMODE = .RDONLY }); - if (fd < 0) return error.OpenFailed; + // WHY it would not open, not just that it would not. Every one of these is + // an ordinary thing to do by accident — `pardes /root`, a file left at mode + // 000, a name that was deleted between resolving and reading — and a caller + // that can only say "OpenFailed" has to show the human a word that means + // nothing to them. `errno` is libc's here, which is the only reason it can + // be read off a `-1`: see the raw-syscall note in `termCwd`. + if (fd < 0) return switch (libc.errno(fd)) { + .ACCES, .PERM => error.PermissionDenied, + .NOENT => error.FileNotFound, + .ISDIR => error.IsDirectory, + .NAMETOOLONG => error.PathTooLong, + else => error.OpenFailed, + }; defer _ = libc.close(fd); const end = libc.lseek(fd, 0, libc.SEEK.END); const size: usize = if (end < 0) 0 else @intCast(end); diff --git a/src/macos.zig b/src/macos.zig index f2e68a85..9d4f6039 100644 --- a/src/macos.zig +++ b/src/macos.zig @@ -2238,7 +2238,7 @@ fn spawnShell(ctx: ?*anyopaque, pane: u8, cwd: []const u8) void { fn ptyWrite(ctx: ?*anyopaque, pane: u8, bytes: []const u8) void { const st = hostState(ctx); - if (st.ptys[pane]) |pt| host_io.writeFd(pt.file.handle, bytes); + if (st.ptys[pane]) |pt| _ = host_io.writeFd(pt.file.handle, bytes); } fn ptyResize(ctx: ?*anyopaque, pane: u8, cols: u16, rows: u16) void { @@ -2271,7 +2271,8 @@ fn ttyTaken(ctx: ?*anyopaque, pane: u8) bool { /// resolved which path and which bytes. fn writeFile(ctx: ?*anyopaque, pane: u8, path: []const u8, bytes: []const u8) void { const st = hostState(ctx); - if (!host_io.writeFileBytes(path, bytes)) return; + host_io.writeFileBytes(path, bytes) catch |err| + return st.core.saveFailed(pane, "save", err); // The directory source will observe our own close. Move its baseline first // so that notification is a hash no-op instead of manufacturing an external // reload and undo boundary. @@ -2286,7 +2287,7 @@ fn writeDump(ctx: ?*anyopaque, bytes: []const u8) void { const st = hostState(ctx); var pbuf: [1024:0]u8 = undefined; const path = pardes.dump.outPath(&pbuf) orelse return; - if (!host_io.writeFileBytes(path, bytes)) return; + host_io.writeFileBytes(path, bytes) catch |err| return st.core.reportError(0, "dump", err); st.core.setLastDump(path); } diff --git a/src/pardes.zig b/src/pardes.zig index 55e111e6..4956150a 100644 --- a/src/pardes.zig +++ b/src/pardes.zig @@ -1384,6 +1384,12 @@ test "an unsaved file marker sits between its path and builtins until Save" { try std.testing.expect(marker_at < save_at); try std.testing.expect(p.executeBuiltinLine(0, "Save")); + // DRAINED FIRST, and the drain is the point rather than ceremony: the + // marker now clears when the write LANDS, not when the effect is queued. + // This test used to pass without it, which is exactly what was wrong — a + // save the host could not do cleared the marker anyway. No frame is + // affected, because `pump` drains before it renders. + while (p.nextEffect()) |effect| p.perform(effect); const saved = try p.tagText(p.scratch.allocator(), pane); try std.testing.expect(std.mem.indexOf(u8, saved, "/hxcase.txt *") == null); @@ -1903,6 +1909,43 @@ test "Save takes the path as an argument, relative to the pane's own directory" try std.testing.expect(drainForSavePath(p, &buf) == null); } +test "a save the host could not do leaves the pane dirty" { + const gpa = std.testing.allocator; + // A host that refuses every write: a read-only file, a directory that was + // removed under the pane, a full disk. The core cannot tell those apart and + // does not need to — the host says why on the message row, and this is the + // other half, which is that the pane must NOT come clean. + const Refusing = struct { + fn writeFile(ctx: ?*anyopaque, pane: u8, _: []const u8, _: []const u8) void { + const core: *Pardes = @ptrCast(@alignCast(ctx.?)); + core.saveFailed(pane, "save", error.PermissionDenied); + } + }; + const p = try Pardes.init(gpa, .{ .tty_only = true, .cols = 100, .rows = 30 }); + defer p.deinit(); + while (p.nextEffect()) |_| {} + const pane = try p.hxOpenFileContent("before\n"); + file_pane.setContent(p, &pane.file.?, try gpa.dupe(u8, "after\n")); + try std.testing.expect(pane.file.?.revision != pane.file.?.saved_revision); + + p.host = .{ .ctx = p, .vtable = &.{ .push_write_file = Refusing.writeFile } }; + try std.testing.expect(p.executeBuiltinLine(0, "Save")); + while (p.nextEffect()) |effect| p.perform(effect); + // STILL DIRTY. Until this, `saveFile` marked the pane saved the moment it + // QUEUED the effect, so the tag's ` *` cleared on a save that never + // happened — and `Del` makes no dirty check, so the next click threw the + // edits away with the screen saying they were safe. + try std.testing.expect(pane.file.?.revision != pane.file.?.saved_revision); + + // ...and the same save against a host that CAN write does come clean, so + // the guard above is not simply "never clean". + p.host = .{}; + try std.testing.expect(p.executeBuiltinLine(0, "Save")); + while (p.nextEffect()) |effect| p.perform(effect); + try std.testing.expectEqual(pane.file.?.revision, pane.file.?.saved_revision); + try std.testing.expectEqualStrings("after\n", p.fallback.get("/hxcase.txt").?); +} + test "Save elsewhere copies a file's bytes and keeps the pane on its own file" { const gpa = std.testing.allocator; const p = try Pardes.init(gpa, .{ .tty_only = true, .cols = 100, .rows = 30 }); @@ -6328,13 +6371,42 @@ pub const Pardes = struct { // is asking to READ it, not to be handed a shell you did not ask // for and have to close — and the launch directory is one Newcol // away when it is wanted. Doc, PDF and image all boot the same way. - _ = initial_doc: { + const opened = initial_doc: { if (comptime pdf_enabled) if (look.isPdfPath(path)) - break :initial_doc try pdf_pane.openPane(p, 0, path, opts.file_line); + break :initial_doc pdf_pane.openPane(p, 0, path, opts.file_line); if (look.isImagePath(path)) - break :initial_doc try image_pane.create(p, 0, path, &.{}); - break :initial_doc try file_pane.open(p, 0, path, opts.file_line); + break :initial_doc image_pane.create(p, 0, path, &.{}); + break :initial_doc file_pane.open(p, 0, path, opts.file_line); }; + // ...AND IF IT WILL NOT OPEN, SAY SO IN THE WINDOW. `pardes /root` + // is a directory that resolves and cannot be read, so it arrives + // here as a `.file` and used to take the whole launch down with + // `error: PermissionDenied` and a return trace out of `main` — a + // crash, to the human, for asking to read something they are not + // allowed to read. Every environment reason lands in the same + // `+Errors` pane a missing name does, because they are the same + // event to whoever typed it: pardes cannot show you that. + // + // OUT OF MEMORY IS NOT ONE OF THEM and goes back to the caller. + // A core that could not allocate a file cannot allocate the pane + // explaining it, and pretending otherwise turns a clean failure + // into a second one. Every opener does its fallible IO BEFORE it + // claims a pane slot (`look.readFile`, then `newDocPane`), so slot + // 0 is still free here — which is what makes this legal. + if (opened) |_| {} else |err| { + if (err == error.OutOfMemory) return err; + const why = switch (err) { + error.PermissionDenied => "permission denied", + error.FileNotFound => "no file of that name", + error.IsDirectory => "that is a directory, and not one that could be read", + error.PathTooLong => "that path is too long", + error.FileTooLarge => "that file is too large to open", + else => @errorName(err), + }; + const content = try std.fmt.allocPrint(gpa, "cannot open\n\n\t{s}\n\n{s}\n", .{ path, why }); + errdefer gpa.free(content); + _ = try output_pane.open(p, 0, std.fs.path.dirname(path) orelse "/", .errors, "", content); + } p.ncol = 1; p.col_n[0] = 1; p.col_terms[0][0] = 0; @@ -7117,7 +7189,22 @@ pub const Pardes = struct { fn hostWriteFile(p: *Pardes, pane: u8, path: []const u8, bytes: []const u8) void { if (p.host.vtable.push_write_file) |f| return f(p.host.ctx, pane, path, bytes); - p.fallback.writeFile(path, bytes); + // The in-process filesystem reports the same way a real host does, so + // an OOM here leaves the pane dirty rather than looking saved. + if (!p.fallback.writeFile(path, bytes)) p.saveFailed(pane, "save", error.OutOfMemory); + } + + /// A HOST'S ANSWER TO `save_file`, and the only one it needs to give: the + /// write did not happen. Puts the reason on the pane's message row and + /// takes back the optimistic clean mark `perform` made, so the tag keeps + /// its ` *` and the edits keep being edits. See host.zig + /// `push_write_file` for why this is a call and not a return value. + pub fn saveFailed(p: *Pardes, id: u8, what: []const u8, err: anyerror) void { + if (p.panes[id]) |pane| if (pane.file) |*f| { + // The `-%` spelling acmefs.zig already uses for "make this dirty". + f.saved_revision = f.revision -% 1; + }; + p.reportError(id, what, err); } /// The path a watch is about: a real file's, or a PDF's. @@ -7171,12 +7258,25 @@ pub const Pardes = struct { p.fallback.setLink(u.slice()), .save_file => |sf| { const pane = p.panes[sf.pane] orelse return; - const f = pane.file orelse return; + const f = if (pane.file) |*file| file else return; + // CLEAN HERE rather than where the effect was queued, and + // BEFORE the call so the host can take it back. `saveFile` used + // to set `saved_revision` at emit time, so a write that could + // not happen still cleared the tag's ` *` and left the edits one + // `Del` from gone — `Del` makes no dirty check. Here is also + // where `f.content` is read, so the revision recorded is the + // revision of the bytes that actually went out. + f.saved_revision = f.revision; p.hostWriteFile(sf.pane, f.path, f.content); }, .save_text => |st| { const pane = p.panes[st.pane] orelse return; if (pane.serial != st.serial) return; // a recycled slot: not ours + // `Save ` is a COPY: it does not clean this pane, + // because the file the pane has open is not the file that was + // written. The one case that does clean is a scratch buffer + // adopting the path, and `saveTo` handles that by emitting + // `save_file` for the pane's own path instead. if (pane.file) |f| return p.hostWriteFile(st.pane, st.path.slice(), f.content); if (!pane.isTerminal()) return; const text = term_pane.screenTextAlloc(pane, p.gpa) catch return; @@ -7191,7 +7291,7 @@ pub const Pardes = struct { // A real host reports where it landed, which is what puts // `Restore ` in the topbar; the virtual one owes the // same, or the bytes it holds are unreachable. - p.fallback.writeFile(fallback_dump_path, out); + _ = p.fallback.writeFile(fallback_dump_path, out); p.setLastDump(fallback_dump_path); } }, @@ -10306,8 +10406,10 @@ pub const Pardes = struct { const pane = p.panes[id] orelse return; const f = if (pane.file) |*file| file else return; if (f.output != null) return; // nothing behind it yet: saveTo, with a path + // NOT marked saved here: the effect has only been QUEUED. `perform`'s + // `.save_file` arm cleans the pane if and only if the host says the + // bytes landed — see there. p.emit(.{ .save_file = .{ .pane = @intCast(id) } }); - f.saved_revision = f.revision; } /// Commit a path — prompted, typed after the word, or chorded onto it. @@ -10356,7 +10458,6 @@ pub const Pardes = struct { f.path = owned; f.output = null; // an ordinary file pane from here on pane.cwd = .none; // its directory is now its own path's dirname - f.saved_revision = f.revision; pane.tag_init = false; // re-derive the tag as a plain file pane.tag_tail_len = 0; p.emit(.{ .save_file = .{ .pane = @intCast(id) } }); @@ -16460,6 +16561,31 @@ test "Esc alternates between two panes of the SAME kind" { } } +test "a boot file that will not open boots an errors pane rather than failing the launch" { + if (platform == .web) return; + const gpa = std.testing.allocator; + // A path `look.resolve` would have accepted and `readFile` cannot open. + // Spelled as a name that is simply not there rather than by chmod-ing a + // fixture to 000, because the second answers differently when the suite + // runs as root and this must fail the same way everywhere. + const p = try Pardes.init(gpa, .{ + .cols = 80, + .rows = 24, + .file = "/definitely/not/here/notes.md", + }); + defer p.deinit(); + p.update(.{ .resize = .{ .cols = 80, .rows = 24 } }); + + const pane = p.panes[0].?; + const f = pane.file.?; + try std.testing.expectEqual(output_pane.Origin.errors, f.output.?.from); + // The reason IN WORDS, not an error name: `PermissionDenied` on a screen + // is jargon, and the whole point of this pane is that a human reads it. + try std.testing.expect(std.mem.indexOf(u8, f.content, "cannot open") != null); + try std.testing.expect(std.mem.indexOf(u8, f.content, "no file of that name") != null); + try std.testing.expect(std.mem.indexOf(u8, f.content, "/definitely/not/here/notes.md") != null); +} + test "argv naming nothing boots an errors pane rather than failing the launch" { const gpa = std.testing.allocator; const p = try Pardes.init(gpa, .{ diff --git a/src/tty/tty.zig b/src/tty/tty.zig index ca263f90..de24f289 100644 --- a/src/tty/tty.zig +++ b/src/tty/tty.zig @@ -1352,7 +1352,7 @@ const Shell = struct { fn ptyWrite(ctx: ?*anyopaque, pane: u8, bytes: []const u8) void { const s = of(ctx); - if (s.ptys[pane]) |pt| host_io.writeFd(pt.file.handle, bytes); + if (s.ptys[pane]) |pt| _ = host_io.writeFd(pt.file.handle, bytes); } fn ptyResize(ctx: ?*anyopaque, pane: u8, cols: u16, rows: u16) void { @@ -1385,7 +1385,12 @@ const Shell = struct { fn writeFile(ctx: ?*anyopaque, pane: u8, path: []const u8, bytes: []const u8) void { const s = of(ctx); - if (!host_io.writeFileBytes(path, bytes)) return; + // SAY WHY, and tell the core it did not happen. A save that cannot be + // done is the one failure this program must never swallow: `saveFailed` + // puts the reason on the pane's message row and leaves the pane dirty, + // so the ` *` stays and `Del` cannot quietly take the edits. + host_io.writeFileBytes(path, bytes) catch |err| + return s.core.saveFailed(pane, "save", err); // our own write is about to come back as a watch event: restamp from // the bytes we just put there so it reads as "no change". Only when // this IS the pane's watched file — a `Save ` must not @@ -1407,7 +1412,7 @@ const Shell = struct { const s = of(ctx); var pbuf: [1024:0]u8 = undefined; const path = pardes.dump.outPath(&pbuf) orelse return; - if (!host_io.writeFileBytes(path, bytes)) return; + host_io.writeFileBytes(path, bytes) catch |err| return s.core.reportError(0, "dump", err); s.core.setLastDump(path); } diff --git a/src/web.zig b/src/web.zig index d642e108..bfef38a7 100644 --- a/src/web.zig +++ b/src/web.zig @@ -391,6 +391,8 @@ fn present(ctx: ?*anyopaque, surface: *const pardes.Surface) void { } fn writeFile(_: ?*anyopaque, _: u8, path: []const u8, bytes: []const u8) void { + // A download the browser has been handed IS the save here: there is no + // filesystem to fail against, so there is no `saveFailed` to report. host_download(path.ptr, path.len, bytes.ptr, bytes.len); } -- cgit v1.3