Skip to content

Commit 1729f24

Browse files
committed
PR review follow-ups
Rolling up all post-initial-push fixes from CI, Copilot review, and the second-round review agent into one commit. Loader & tests - Honour `(fboundp 'ghostel--new)' in the unified module loader so `cl-letf' stubs in the pure-Elisp test suite short-circuit as they did before the consolidation. Restores the title-tracking tests on the elisp-only CI job. CRLF across calls - Persist `last_input_was_cr' on the Terminal struct so a CRLF pair split across two `ghostel--write-input' calls isn't double- normalized into \r\r\n. Reset in `resize' alongside the other reset state. Regression tests: canonical split, empty chunk between CR and LF, standalone CR followed by complete CRLF. OSC iterator correctness - Treat a following `\e]' introducer as a payload boundary so one OSC missing its terminator doesn't cannibalize the next OSC's bytes as its own payload. Input `\e]7;PARTIAL\e]52;c;aGVsbG8=\a' previously dispatched OSC 7 with garbage and starved OSC 52 entirely; now OSC 7 is recognized as partial and skipped, OSC 52 dispatches normally. Regression test added. - Rename `OscEntry.term' -> `OscEntry.terminator' to remove the visual collision with `Terminal *term' in dispatchPostWriteOscs. - Clarify the `dispatchPostWriteOscs' docstring: it handles the four post-vtWrite OSCs; color queries use the same iterator from a different call site. Redraw hash gate - Skip `computeFirstScrollbackRowHash' at end-of-redraw when no writes happened this frame and the start-of-redraw rotation check didn't populate `cached_row0_hash'. Row 0 can't have moved, so the stored hash is still current. Covers cursor-only and idle-timer redraws. Naming and comments - Rename `last_wrote_cr' -> `last_input_was_cr': the field names the input-stream property, not a normalizer implementation detail that might change underneath it. - `loadRawCell' docstring notes the memoize assumes idempotent fetch (true today in libghostty). README - Refresh the 5 MB PTY throughput table: ghostel 70 -> 87 MB/s plain, 56 -> 64 MB/s URL-heavy. Other backends within noise.
1 parent bbe1c41 commit 1729f24

6 files changed

Lines changed: 156 additions & 30 deletions

File tree

README.md

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -646,11 +646,11 @@ Emacs 31.0.50:
646646

647647
| Backend | Plain ASCII | URL-heavy |
648648
|----------------------|------------:|----------:|
649-
| ghostel | 70 MB/s | 56 MB/s |
650-
| ghostel (no detect) | 70 MB/s | 70 MB/s |
651-
| vterm | 34 MB/s | 27 MB/s |
652-
| eat | 4.4 MB/s | 3.5 MB/s |
653-
| term | 5.6 MB/s | 4.7 MB/s |
649+
| ghostel | 87 MB/s | 64 MB/s |
650+
| ghostel (no detect) | 86 MB/s | 86 MB/s |
651+
| vterm | 35 MB/s | 27 MB/s |
652+
| eat | 4.7 MB/s | 3.5 MB/s |
653+
| term | 5.7 MB/s | 4.7 MB/s |
654654

655655
Ghostel scans terminal output for URLs and file paths, making them clickable.
656656
The "no detect" row shows throughput with this detection disabled

ghostel.el

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -675,8 +675,13 @@ When the module file is missing, trigger `ghostel-module-auto-install'.
675675
When PROMPT-USER is non-nil (called from an interactive command like
676676
`ghostel'), failures signal `user-error' so the calling flow aborts.
677677
Otherwise (load time), failures `display-warning' instead so the user
678-
can still compile ghostel, read docs, and so on."
679-
(unless (featurep 'ghostel-module)
678+
can still compile ghostel, read docs, and so on.
679+
680+
The guard also honours `ghostel--new' being already `fboundp', which
681+
covers the pure-Elisp test path where `cl-letf' stubs the native
682+
entry points so tests run without the module present."
683+
(unless (or (featurep 'ghostel-module)
684+
(fboundp 'ghostel--new))
680685
(let* ((dir (file-name-directory (or load-file-name
681686
(locate-library "ghostel")
682687
buffer-file-name)))

src/module.zig

Lines changed: 51 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,14 @@ fn fnWriteInput(raw_env: ?*c.emacs_env, _: isize, args: [*c]c.emacs_value, _: ?*
157157
// and then "\r\n". libghostty's VT state machine handles arbitrary
158158
// chunking (that's how the process filter already works), so no
159159
// scratch buffer, no allocation, no truncation fallback.
160+
//
161+
// `prev_was_cr` is seeded from `term.last_input_was_cr` so a CRLF
162+
// pair split across two writes — chunk A ending with \r, chunk B
163+
// starting with \n — is not mis-normalized into \r\r\n. The final
164+
// value is persisted back for the next call. An empty input
165+
// round-trips the flag unchanged.
160166
var seg_start: usize = 0;
161-
var prev_was_cr: bool = false;
167+
var prev_was_cr: bool = term.last_input_was_cr;
162168
for (raw, 0..) |ch, i| {
163169
if (ch == '\n' and !prev_was_cr) {
164170
if (i > seg_start) term.vtWrite(raw[seg_start..i]);
@@ -172,6 +178,7 @@ fn fnWriteInput(raw_env: ?*c.emacs_env, _: isize, args: [*c]c.emacs_value, _: ?*
172178
if (seg_start < raw.len) {
173179
term.vtWrite(raw[seg_start..]);
174180
}
181+
term.last_input_was_cr = prev_was_cr;
175182

176183
// Scan for OSC sequences that libghostty-vt discards (7, 51, 52, 133).
177184
// One pass, dispatched by code in document order.
@@ -191,14 +198,22 @@ const OscEntry = struct {
191198
/// Payload bytes between the code's trailing `;` and the terminator.
192199
payload: []const u8,
193200
/// Terminator bytes (BEL or ESC \) — forwarded back on replies.
194-
term: []const u8,
201+
terminator: []const u8,
195202
};
196203

197204
/// Single-pass iterator over well-formed OSC sequences in a byte slice.
198205
/// Advances past `ESC ]`, parses the decimal code up to `;`, locates the
199-
/// BEL/ST terminator, and yields `(code, payload, term)`. Partial
200-
/// sequences at the end of the buffer stop iteration so the caller
201-
/// doesn't act on half-received data.
206+
/// BEL/ST terminator, and yields `(code, payload, terminator)`.
207+
///
208+
/// An OSC payload is bounded by a real terminator (BEL or ESC \) OR by
209+
/// the next OSC introducer (ESC ]). Stopping at a following introducer
210+
/// handles malformed input where one OSC is missing its terminator
211+
/// before the next begins — otherwise the partial OSC would cannibalize
212+
/// the following OSC's bytes (including its terminator) as its own
213+
/// payload, producing a garbage dispatch AND starving the next OSC.
214+
/// On partial detection we advance past the current introducer and let
215+
/// iteration continue, so well-formed OSCs later in the same buffer
216+
/// still dispatch.
202217
const OscIterator = struct {
203218
data: []const u8,
204219
pos: usize = 0,
@@ -222,43 +237,57 @@ const OscIterator = struct {
222237
}
223238
const payload_start = code_end + 1;
224239

225-
// Terminator search. A partial OSC (no terminator before EOF)
226-
// stops iteration entirely: bytes after an unterminated payload
227-
// are opaque, so there's no safe way to continue scanning.
240+
// Scan for a terminator (BEL or ESC \) or the next OSC
241+
// introducer (ESC ]), whichever comes first. An intervening
242+
// introducer means the current OSC is partial.
228243
var end = payload_start;
229244
var term_len: usize = 0;
230245
while (end < self.data.len) : (end += 1) {
231-
if (self.data[end] == 0x07) {
246+
const ch = self.data[end];
247+
if (ch == 0x07) {
232248
term_len = 1;
233249
break;
234250
}
235-
if (self.data[end] == 0x1b and end + 1 < self.data.len and self.data[end + 1] == '\\') {
236-
term_len = 2;
237-
break;
251+
if (ch == 0x1b and end + 1 < self.data.len) {
252+
const next_ch = self.data[end + 1];
253+
if (next_ch == '\\') {
254+
term_len = 2;
255+
break;
256+
}
257+
if (next_ch == ']') {
258+
// Next introducer — current OSC is partial.
259+
break;
260+
}
238261
}
239262
}
240263
if (term_len == 0) {
241-
self.pos = self.data.len;
242-
return null;
264+
// Partial OSC: skip past the current introducer (which
265+
// we already parsed) and resume scanning. If `end` is
266+
// still `self.data.len` there were no more introducers,
267+
// and the next indexOfPos call will terminate iteration.
268+
self.pos = if (end > code_start) end else code_start;
269+
continue;
243270
}
244271

245272
self.pos = end + term_len;
246273
const code = std.fmt.parseInt(u32, self.data[code_start..code_end], 10) catch continue;
247274
return .{
248275
.code = code,
249276
.payload = self.data[payload_start..end],
250-
.term = self.data[end .. end + term_len],
277+
.terminator = self.data[end .. end + term_len],
251278
};
252279
}
253280
return null;
254281
}
255282
};
256283

257284
/// Dispatch OSC 7 / 51 / 52 / 133 from `data` in document order.
258-
/// These are the sequences that libghostty-vt discards, so ghostel
259-
/// has to scan for them itself. All four used to scan the buffer
260-
/// independently; one unified pass is strictly less work for bulk
261-
/// output and preserves source-order dispatch.
285+
/// These are the post-vtWrite sequences that libghostty-vt discards,
286+
/// so ghostel has to scan for them itself. All four used to scan the
287+
/// buffer independently; one unified pass is strictly less work for
288+
/// bulk output and preserves source-order dispatch. (OSC 4/10/11
289+
/// color queries use the same iterator but run before vtWrite — see
290+
/// `extractOscColorQueries`.)
262291
///
263292
/// Runs AFTER `vtWrite` so libghostty has already seen the bytes —
264293
/// OSC 7 calls back into libghostty (`setPwd`) and the others call
@@ -388,13 +417,13 @@ fn extractOscColorQueries(env: emacs.Env, term: *Terminal, data: []const u8) voi
388417
if (!std.mem.eql(u8, osc.payload, "?")) continue;
389418
var fg: gt.ColorRgb = undefined;
390419
if (!term.getColorForeground(&fg)) continue;
391-
sendDynamicColorReply(env, 10, fg, osc.term);
420+
sendDynamicColorReply(env, 10, fg, osc.terminator);
392421
},
393422
11 => {
394423
if (!std.mem.eql(u8, osc.payload, "?")) continue;
395424
var bg: gt.ColorRgb = undefined;
396425
if (!term.getColorBackground(&bg)) continue;
397-
sendDynamicColorReply(env, 11, bg, osc.term);
426+
sendDynamicColorReply(env, 11, bg, osc.terminator);
398427
},
399428
4 => {
400429
// Payload is a ';'-separated list of `index;value` pairs.
@@ -409,7 +438,7 @@ fn extractOscColorQueries(env: emacs.Env, term: *Terminal, data: []const u8) voi
409438
if (!term.getColorPalette(&palette)) break;
410439
palette_loaded = true;
411440
}
412-
sendPaletteColorReply(env, @intCast(idx), palette[idx], osc.term);
441+
sendPaletteColorReply(env, @intCast(idx), palette[idx], osc.terminator);
413442
}
414443
},
415444
else => {},

src/render.zig

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,12 @@ const RawTag = enum { unset, loaded, failed };
9494

9595
/// Lazily populate `out` with the raw cell; memoizes success/failure
9696
/// in `tag` so repeated calls within one cell do not re-issue the get.
97+
///
98+
/// The cache assumes `cells_get(RAW)` is idempotent for a given
99+
/// iterator position (which it is in libghostty today — it returns a
100+
/// handle into already-materialized cell data). If that ever
101+
/// changes, a failed first call would become sticky for the rest of
102+
/// the current cell.
97103
fn loadRawCell(cells: gt.RenderStateRowCells, out: *gt.c.GhosttyCell, tag: *RawTag) bool {
98104
switch (tag.*) {
99105
.unset => {
@@ -1291,8 +1297,18 @@ pub fn redraw(env: emacs.Env, term: *Terminal, force_full_arg: bool) void {
12911297
// rotation check. Reuse the start-of-redraw hash when it's still
12921298
// valid (no rotation, no trim) — avoids a second scroll-to-top +
12931299
// render_state_update round trip per redraw.
1300+
//
1301+
// If nothing was written AND we didn't populate `cached_row0_hash`
1302+
// at the top, row 0 cannot have moved since the previous redraw
1303+
// set `first_scrollback_row_hash`, so skip the compute entirely.
1304+
// This covers cursor-only redraws and idle-timer fires.
12941305
if (term.scrollback_in_buffer > 0) {
1295-
term.first_scrollback_row_hash = cached_row0_hash orelse computeFirstScrollbackRowHash(term);
1306+
if (cached_row0_hash) |h| {
1307+
term.first_scrollback_row_hash = h;
1308+
} else if (term.wrote_since_redraw) {
1309+
term.first_scrollback_row_hash = computeFirstScrollbackRowHash(term);
1310+
}
1311+
// else: no writes, no cached hash → existing value is still current.
12961312
} else {
12971313
term.first_scrollback_row_hash = 0;
12981314
}

src/terminal.zig

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,18 @@ wrote_since_redraw: bool = false,
4949
/// into the redraw pass where `inhibit-redisplay` prevents flicker.
5050
resize_pending: bool = false,
5151

52+
/// True iff the last byte of the previous `fnWriteInput` input was
53+
/// `\r`. Carries the bare-LF detection state across write-input calls
54+
/// so that a CR at the tail of one write and an LF at the head of the
55+
/// next don't get normalized into an extra `\r` (producing `\r\r\n`).
56+
///
57+
/// Named after the input stream rather than what was fed to libghostty
58+
/// because the two only differ in that the normalizer may insert a
59+
/// `\r` before a bare LF — it never drops or rewrites a trailing CR.
60+
/// Reset by `resize` since a reflow means the stream is effectively
61+
/// new.
62+
last_input_was_cr: bool = false,
63+
5264
/// Hash of the first scrollback row's content, sampled at the end of
5365
/// each redraw that touched scrollback. Used to detect rotation
5466
/// (libghostty evicting the oldest row in lockstep with new ones being
@@ -226,6 +238,7 @@ pub fn resize(self: *Self, cols: u16, rows: u16) !void {
226238
self.scrollback_in_buffer = 0;
227239
self.first_scrollback_row_hash = 0;
228240
self.resize_pending = true;
241+
self.last_input_was_cr = false;
229242
}
230243

231244
/// Scroll the viewport.

test/ghostel-test.el

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -643,6 +643,50 @@ than the full terminal `cols'."
643643
(should (equal 6 (car cur))) ; cursor col after LF
644644
(should (> (cdr cur) 0))))) ; cursor moved to row 1+
645645

646+
(ert-deftest ghostel-test-crlf-split-across-writes ()
647+
"CRLF pair split across two write-input calls must not double-insert \\r.
648+
Chunk A ends with \\r, chunk B starts with \\n. Without cross-call
649+
state the normalizer would treat the leading \\n as bare and emit
650+
\\r\\r\\n to libghostty. Visible effect: cursor lands on row 1 col 6
651+
after \"first\\r\" + \"\\nsecond\", exactly as if the pair were sent in
652+
one call; a bug would leave it on row 2 or otherwise desynced."
653+
(let ((term (ghostel--new 25 80 1000))
654+
(term-single (ghostel--new 25 80 1000)))
655+
(ghostel--write-input term "first\r")
656+
(ghostel--write-input term "\nsecond")
657+
(ghostel--write-input term-single "first\r\nsecond")
658+
(should (equal (ghostel-test--cursor term)
659+
(ghostel-test--cursor term-single)))))
660+
661+
(ert-deftest ghostel-test-crlf-split-with-empty-chunk ()
662+
"An empty write between \\r and \\n preserves the cross-call CR flag.
663+
Regression guard for a naive implementation that resets `last_input_was_cr'
664+
on every entry rather than only when input was consumed."
665+
(let ((term (ghostel--new 25 80 1000))
666+
(term-single (ghostel--new 25 80 1000)))
667+
(ghostel--write-input term "first\r")
668+
(ghostel--write-input term "") ; empty chunk must not clear flag
669+
(ghostel--write-input term "\nsecond")
670+
(ghostel--write-input term-single "first\r\nsecond")
671+
(should (equal (ghostel-test--cursor term)
672+
(ghostel-test--cursor term-single)))))
673+
674+
(ert-deftest ghostel-test-crlf-standalone-cr-then-crlf ()
675+
"A lone CR followed by a complete CRLF stays two logical line-endings.
676+
The normalizer must not collapse the trailing CR of write A and the
677+
leading \\r of write B's \\r\\n into a single sequence: the input
678+
\"a\\r\" + \"\\r\\nb\" is equivalent to sending \"a\\r\\r\\nb\" in one
679+
call. (Bare \\n comes from Emacs PTYs lacking ONLCR; bare \\r from
680+
programs that explicitly emit a carriage return — both must be passed
681+
through without cross-call munging.)"
682+
(let ((term (ghostel--new 25 80 1000))
683+
(term-single (ghostel--new 25 80 1000)))
684+
(ghostel--write-input term "a\r")
685+
(ghostel--write-input term "\r\nb")
686+
(ghostel--write-input term-single "a\r\r\nb")
687+
(should (equal (ghostel-test--cursor term)
688+
(ghostel-test--cursor term-single)))))
689+
646690
;; -----------------------------------------------------------------------
647691
;; Test: raw key sequence fallback
648692
;; -----------------------------------------------------------------------
@@ -993,6 +1037,25 @@ Mirrors the real zsh case where the directory still contains a
9931037
(ghostel--write-input term "\e]52;c;?\e\\")
9941038
(should (equal nil kill-ring))))) ; osc52 query ignored
9951039

1040+
(ert-deftest ghostel-test-osc-partial-does-not-starve-later ()
1041+
"A partial OSC must not cannibalize or starve a following complete OSC.
1042+
Input \"\\e]7;PARTIAL\\e]52;c;aGVsbG8=\\a\" would, under a naive
1043+
single-pass scanner, let the OSC 7 payload absorb the OSC 52's BEL
1044+
terminator — yielding a garbage PWD dispatch and no clipboard. The
1045+
iterator must treat the intervening \\e] as a partial-OSC boundary,
1046+
skip the OSC 7, and still dispatch the OSC 52."
1047+
(let ((term (ghostel--new 25 80 1000))
1048+
(ghostel-enable-osc52 t)
1049+
(kill-ring nil)
1050+
(pwd-before (ghostel--get-pwd (ghostel--new 25 80 1000))))
1051+
(ghostel--write-input term "\e]7;PARTIAL\e]52;c;aGVsbG8=\a")
1052+
;; OSC 52 dispatched: "hello" in kill-ring.
1053+
(should kill-ring)
1054+
(should (equal "hello" (car kill-ring)))
1055+
;; OSC 7 NOT dispatched with the garbage payload "PARTIAL\e]52;c;aGVsbG8="
1056+
;; — the PWD should still be whatever a fresh terminal reports (nil).
1057+
(should (equal pwd-before (ghostel--get-pwd term)))))
1058+
9961059
;; -----------------------------------------------------------------------
9971060
;; Test: OSC 4/10/11 color query responses
9981061
;; -----------------------------------------------------------------------

0 commit comments

Comments
 (0)