fix-deviant(cli): never lose or overwrite CLI output like Zig does
Intentional deviation from Ghostty 1.3.1 (upstream data-loss bugs, see docs/rewrite/DEVIANT_FIXES.md and docs/rewrite/notes/final-Z17.md): - Zig 0.15's File.writer writes positionally (pwrite) at each writer's own offset, starting at 0, while the file is seekable. When stdout is a regular file, two writers (e.g. +show-face) overwrite each other and any existing content, and File.Writer.end truncates. ZigWriter always appends at the current end and end() only flushes. - Writers that are never flushed lose their buffered output in Zig, e.g. +list-themes' stderr buffer, so "No themes found..." is never printed. ZigWriter flushes on drop. Zig's buffering and flush order (and thus pipe/tty interleaving) is kept unchanged; only the data loss is not replicated. Test Plan: - cargo test -p ghostty-cli zig_writer (regular_file_output_is_complete_and_appended, unflushed_buffer_is_flushed_on_drop) - ghostty +list-themes with a missing resources dir prints the message
This commit is contained in:
1 parent
f76179f49a
commit
410d8fc1a9
3 files changed
+74
-1
No files matched your search
@@ -233,6 +233,8 @@ pub fn run(args: &[impl AsRef<[u8]>]) -> Result<u8, Error> {
|
|||||||
crate::parse_options(&mut opts, args)?;
|
crate::parse_options(&mut opts, args)?;
|
||||||
|
|
||||||
let stdout = std::io::stdout();
|
let stdout = std::io::stdout();
|
||||||
|
// Zig's stderr is a 4096 byte buffer that it never flushes (the messages
|
||||||
|
// below are lost). That is an upstream bug: we flush when it is dropped.
|
||||||
let mut stderr = crate::zig_writer::ZigWriter::new(crate::zig_writer::stderr_sink(), 4096);
|
let mut stderr = crate::zig_writer::ZigWriter::new(crate::zig_writer::stderr_sink(), 4096);
|
||||||
|
|
||||||
let resources_dir = resources_dir();
|
let resources_dir = resources_dir();
|
||||||
|
|||||||
@@ -4,6 +4,13 @@
|
|||||||
//! file when it overflows or when the action calls `flush`, so two writers on
|
//! file when it overflows or when the action calls `flush`, so two writers on
|
||||||
//! the same file (as in `+show-face`) interleave in flush order, not call
|
//! the same file (as in `+show-face`) interleave in flush order, not call
|
||||||
//! order. This type keeps that ordering.
|
//! order. This type keeps that ordering.
|
||||||
|
//!
|
||||||
|
//! Two upstream data-loss behaviours are deliberately NOT replicated (see
|
||||||
|
//! `docs/rewrite/notes/final-Z17.md`): Zig's positional `pwrite` at the
|
||||||
|
//! writer's own offset 0 (writers overwrite each other and existing content
|
||||||
|
//! when stdout is a regular file) and buffers that are never flushed. Data is
|
||||||
|
//! always appended at the current end and anything still buffered is flushed
|
||||||
|
//! when the writer is dropped.
|
||||||
|
|
||||||
#[cfg(unix)]
|
#[cfg(unix)]
|
||||||
use std::os::fd::AsFd;
|
use std::os::fd::AsFd;
|
||||||
@@ -69,7 +76,7 @@ impl ZigWriter {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// `File.Writer.end`.
|
/// `File.Writer.end`: a flush (Zig also truncates; not replicated).
|
||||||
pub fn end(&mut self) -> io::Result<()> {
|
pub fn end(&mut self) -> io::Result<()> {
|
||||||
self.flush()
|
self.flush()
|
||||||
}
|
}
|
||||||
@@ -91,6 +98,12 @@ impl ZigWriter {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
impl Drop for ZigWriter {
|
||||||
|
fn drop(&mut self) {
|
||||||
|
let _ = self.flush();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
impl Write for ZigWriter {
|
impl Write for ZigWriter {
|
||||||
fn write(&mut self, data: &[u8]) -> io::Result<usize> {
|
fn write(&mut self, data: &[u8]) -> io::Result<usize> {
|
||||||
self.write_all(data)?;
|
self.write_all(data)?;
|
||||||
@@ -154,4 +167,32 @@ mod tests {
|
|||||||
w.write_all(b"h").unwrap();
|
w.write_all(b"h").unwrap();
|
||||||
assert_eq!(&*mem.0.borrow(), b"abcdefg");
|
assert_eq!(&*mem.0.borrow(), b"abcdefg");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(unix)]
|
||||||
|
#[test]
|
||||||
|
fn regular_file_output_is_complete_and_appended() {
|
||||||
|
let mut tmp = tempfile::tempfile().unwrap();
|
||||||
|
std::io::Write::write_all(&mut tmp, b"pre\n").unwrap();
|
||||||
|
let sink: Sink = Rc::new(RefCell::new(Target::Fd(tmp.try_clone().unwrap())));
|
||||||
|
let mut a = ZigWriter::new(sink.clone(), 1024);
|
||||||
|
let mut b = ZigWriter::new(sink, 1024);
|
||||||
|
a.write_all(b"0123456789\n").unwrap();
|
||||||
|
b.write_all(b"abc\n").unwrap();
|
||||||
|
a.flush().unwrap();
|
||||||
|
b.flush().unwrap();
|
||||||
|
let mut out = String::new();
|
||||||
|
std::io::Seek::rewind(&mut tmp).unwrap();
|
||||||
|
std::io::Read::read_to_string(&mut tmp, &mut out).unwrap();
|
||||||
|
assert_eq!(out, "pre\n0123456789\nabc\n");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn unflushed_buffer_is_flushed_on_drop() {
|
||||||
|
let mem = Mem::default();
|
||||||
|
let sink: Sink = stream_sink(mem.clone());
|
||||||
|
let mut w = ZigWriter::new(sink, 4096);
|
||||||
|
w.write_all(b"kept").unwrap();
|
||||||
|
drop(w);
|
||||||
|
assert_eq!(&*mem.0.borrow(), b"kept");
|
||||||
|
}
|
||||||
}
|
}
|
||||||
@@ -0,0 +1,30 @@
|
|||||||
|
# Z17: RV14 findings
|
||||||
|
|
||||||
|
1. `+show-face --cp` is `Option<U21>` (new `ghostty_config::cli_args::U21`, Zig `u21`); out of range gives an
|
||||||
|
`invalid value` diagnostic and exit 1 (no panic). Audit of Zig int-typed CLI/config fields against Rust
|
||||||
|
(Config.zig, cli/*.zig, nested types): all other widths already match.
|
||||||
|
2. `--string` is `Option<EncodedOs>` (raw bytes at option parse). Invalid UTF-8 is detected after the `--cp`
|
||||||
|
lookup: `Unable to parse string as unicode: error.InvalidUtf8` (no newline), exit 1.
|
||||||
|
3. New `crates/ghostty-cli/src/zig_writer.rs` models `File.writer(&buf)`: fixed buffer, drain-on-overflow,
|
||||||
|
explicit flush, so stdout/stderr interleave in Zig's flush order (`+show-face` creates both writers on
|
||||||
|
stdout; stdout is flushed first, then stderr; verified against the oracle on pipes). Anything still
|
||||||
|
buffered is flushed on drop. Applied to every Zig action using `File.writer`: show-face, help (+ any
|
||||||
|
`--help`), version, validate-config, show-config, list-actions, list-keybinds (plain), list-fonts,
|
||||||
|
list-themes, ssh-cache, crash-report, edit-config, new-window.
|
||||||
|
4. Windows basename: already Zig 0.15.2 (colon only at byte index 1); `λ:theme` stays whole. Added test.
|
||||||
|
5. `ghostty_os::osbytes::zig_is_absolute{,_posix,_windows}` + `EncodedOs::is_zig_absolute` (0.15.2
|
||||||
|
`isAbsolute`: `\x`, `/x`, `X:\`, `X:/`). Used by theme open, config path expand, file_load, Config
|
||||||
|
asserts, surface mouse path, GTK bell path (every Zig `isAbsolute` site).
|
||||||
|
6. `xev::available(IoUring)` is exactly `IoUring::new(256).is_ok()`. The termio reactor now handles a ring
|
||||||
|
without `EXT_ARG` by using `IORING_OP_TIMEOUT` (like libxev timers); test forces that path.
|
||||||
|
|
||||||
|
## Upstream bugs fixed (not replicated)
|
||||||
|
|
||||||
|
- Positional output overwrite: `std.fs.File.writer` (Zig 0.15.2 `fs/File.zig` `Writer.init`/`drain`)
|
||||||
|
`pwrite`s at the writer's own offset starting at 0. With stdout a regular file, `+show-face`
|
||||||
|
(`cli/show_face.zig:68-73`, two writers on `File.stdout()`) overwrites its own output, and any action
|
||||||
|
(`cli/help.zig:34`, `cli/validate_config.zig:43`, ...) overwrites pre-existing file content; `Writer.end`
|
||||||
|
(`validate_config.zig:46`, `show_config.zig:84`) also truncates it. Rust always appends at the current end.
|
||||||
|
- Never-flushed buffer: `cli/list_themes.zig:124-126,129-131,171` writes "Could not find the Ghostty
|
||||||
|
resources directory" / "No themes found..." to a 4096 byte stderr buffer that is never flushed, so the
|
||||||
|
messages are lost. Rust flushes it when the action ends.
|
||||||
Reference in new issue
Block a user