From 4bbe0d7fe9b167a7a1f1255715cb4a9862342828 Mon Sep 17 00:00:00 2001 From: ddidderr Date: Mon, 12 Oct 2026 13:54:15 +0200 Subject: [PATCH] fix: last RV11 gaps - bench enum args, theme NUL paths, Zig error names Resolves the 3 remaining gaps in docs/rewrite/notes/review-rv11-codex.md (final-Z14.md): - ghostty-bench validates every occurrence of enum options in order (Args::take_enum; empty value resets) like cli/args.zig, so `--mode=bad --mode=noop` fails as in Zig. - Theme paths with an interior NUL open the prefix like Zig's C-string path handling (verified against /usr/bin/ghostty). - Filesystem diagnostics print Zig's error names (error.FileNotFound, NotDir, AccessDenied, IsDir, NameTooLong, SymLinkLoop, ...) via a shared ghostty_os::zig_error mapping used by theme/path/config-file/ file_load/edit-config/list-themes; messages match the oracle. Test Plan: - cargo test --workspace --no-fail-fast - just check-macos (both), just check-windows, just check-wasm --- crates/ghostty-bench/src/args.rs | 36 ++++++++++ crates/ghostty-bench/src/benches.rs | 68 +++++++++++-------- .../src/bin/ghostty-synthetic.rs | 13 ++-- crates/ghostty-cli/src/edit_config.rs | 6 +- crates/ghostty-cli/src/list_themes/preview.rs | 8 +-- crates/ghostty-config/src/config.rs | 38 +---------- crates/ghostty-config/src/file_load.rs | 11 ++- crates/ghostty-config/src/path.rs | 6 +- crates/ghostty-config/src/theme.rs | 49 +++++++++++-- crates/ghostty-os/src/lib.rs | 1 + crates/ghostty-os/src/zig_error.rs | 63 +++++++++++++++++ docs/rewrite/notes/final-Z14.md | 5 ++ 12 files changed, 215 insertions(+), 89 deletions(-) create mode 100644 crates/ghostty-os/src/zig_error.rs create mode 100644 docs/rewrite/notes/final-Z14.md diff --git a/crates/ghostty-bench/src/args.rs b/crates/ghostty-bench/src/args.rs index 8d674ba..9363545 100644 --- a/crates/ghostty-bench/src/args.rs +++ b/crates/ghostty-bench/src/args.rs @@ -122,6 +122,27 @@ impl Args { }) } + /// Take an enum value (`parseIntoField` on an enum): every occurrence is + /// validated against `variants` in order (a bad value is an error even if + /// a later one is fine); an empty value resets to `default`. + pub fn take_enum( + &mut self, + key: &str, + default: T, + variants: &[(&str, T)], + ) -> Result { + self.apply(key, default, |v| { + let v = v.ok_or_else(|| format!("option --{key} requires a value"))?; + let bad = || format!("invalid value for --{key}: {}", v.to_string_lossy()); + let v = v.to_str().ok_or_else(bad)?; + variants + .iter() + .find(|(n, _)| *n == v) + .map(|(_, t)| *t) + .ok_or_else(bad) + }) + } + /// Take a file path value (any bytes are allowed). pub fn take_opt_path(&mut self, key: &str) -> Result, String> { self.apply(key, None, |v| { @@ -197,6 +218,21 @@ mod tests { assert!(a.take("n", 0u16).is_err()); } + #[test] + fn take_enum_validates_every_occurrence() { + let t = |v: &[&str]| { + Args::parse(v.iter().map(OsString::from)) + .unwrap() + .take_enum("mode", 0u8, &[("noop", 0), ("table", 1)]) + }; + assert!(t(&["--mode=bad", "--mode=noop"]).is_err()); + assert!(t(&["--mode=noop", "--mode=bad"]).is_err()); + assert!(t(&["--mode=bad", "--mode="]).is_err()); + assert_eq!(t(&["--mode=table", "--mode="]).unwrap(), 0); + assert_eq!(t(&["--mode=noop", "--mode=table"]).unwrap(), 1); + assert_eq!(t(&[]).unwrap(), 0); + } + #[test] fn duplicates_apply_in_order_and_first_error_wins() { let a = |v: &[&str]| Args::parse(v.iter().map(OsString::from)).unwrap(); diff --git a/crates/ghostty-bench/src/benches.rs b/crates/ghostty-bench/src/benches.rs index 549db35..5ec1350 100644 --- a/crates/ghostty-bench/src/benches.rs +++ b/crates/ghostty-bench/src/benches.rs @@ -104,11 +104,7 @@ impl TerminalStream { let rows = args.take("terminal-rows", 80u16)?; let cols = args.take("terminal-cols", 120u16)?; let data = args.take_opt_path("data")?; - let full = match args.take_opt_string("handler")?.as_deref() { - None | Some("print") => false, - Some("full") => true, - Some(m) => return Err(format!("invalid value for --handler: {m}")), - }; + let full = args.take_enum("handler", false, &[("print", false), ("full", true)])?; let t = Terminal::new(TermOptions::new(cols, rows)).map_err(|e| format!("{e:?}"))?; Ok(TerminalStream { data: Data::new(data), @@ -259,13 +255,16 @@ pub struct CodepointWidth { impl CodepointWidth { pub fn create(args: &mut Args) -> std::result::Result { - let mode = match args.take_opt_string("mode")?.as_deref() { - None | Some("noop") => WidthMode::Noop, - Some("wcwidth") => WidthMode::Wcwidth, - Some("simd") => WidthMode::Simd, - Some("table") => WidthMode::Table, - Some(m) => return Err(format!("invalid value for --mode: {m}")), - }; + let mode = args.take_enum( + "mode", + WidthMode::Noop, + &[ + ("noop", WidthMode::Noop), + ("wcwidth", WidthMode::Wcwidth), + ("simd", WidthMode::Simd), + ("table", WidthMode::Table), + ], + )?; Ok(CodepointWidth { data: Data::new(args.take_opt_path("data")?), mode, @@ -384,11 +383,7 @@ pub struct GraphemeBreak { impl GraphemeBreak { pub fn create(args: &mut Args) -> std::result::Result { - let noop = match args.take_opt_string("mode")?.as_deref() { - None | Some("table") => false, - Some("noop") => true, - Some(m) => return Err(format!("invalid value for --mode: {m}")), - }; + let noop = args.take_enum("mode", false, &[("noop", true), ("table", false)])?; Ok(GraphemeBreak { data: Data::new(args.take_opt_path("data")?), noop, @@ -473,11 +468,11 @@ pub fn is_symbol_computed(cp: u32) -> bool { impl IsSymbol { pub fn create(args: &mut Args) -> std::result::Result { - let mode = match args.take_opt_string("mode")?.as_deref() { - None | Some("uucode") => SymbolMode::Uucode, - Some("table") => SymbolMode::Table, - Some(m) => return Err(format!("invalid value for --mode: {m}")), - }; + let mode = args.take_enum( + "mode", + SymbolMode::Uucode, + &[("uucode", SymbolMode::Uucode), ("table", SymbolMode::Table)], + )?; Ok(IsSymbol { data: Data::new(args.take_opt_path("data")?), mode, @@ -536,12 +531,15 @@ pub struct ScreenClone { impl ScreenClone { pub fn create(args: &mut Args) -> std::result::Result { - let mode = match args.take_opt_string("mode")?.as_deref() { - None | Some("clone") => CloneMode::Clone, - Some("noop") => CloneMode::Noop, - Some("render") => CloneMode::Render, - Some(m) => return Err(format!("invalid value for --mode: {m}")), - }; + let mode = args.take_enum( + "mode", + CloneMode::Clone, + &[ + ("clone", CloneMode::Clone), + ("noop", CloneMode::Noop), + ("render", CloneMode::Render), + ], + )?; let rows = args.take("terminal-rows", 80u16)?; let cols = args.take("terminal-cols", 120u16)?; let data = args.take_opt_path("data")?; @@ -718,6 +716,20 @@ mod tests { let _ = std::fs::remove_file(p); } + #[test] + fn duplicate_bad_enum_modes_fail() { + let mk = |v: &[&str]| Args::parse(v.iter().map(|s| (*s).to_owned())).unwrap(); + for v in [ + &["--mode=bad", "--mode=noop"][..], + &["--mode=bad", "--mode="][..], + ] { + assert!(CodepointWidth::create(&mut mk(v)).is_err()); + assert!(GraphemeBreak::create(&mut mk(v)).is_err()); + assert!(IsSymbol::create(&mut mk(v)).is_err()); + assert!(ScreenClone::create(&mut mk(v)).is_err()); + } + } + #[test] fn is_symbol_modes_run() { let p = sample_file("sym"); diff --git a/crates/ghostty-bench/src/bin/ghostty-synthetic.rs b/crates/ghostty-bench/src/bin/ghostty-synthetic.rs index 0d44e99..9c5c9ae 100644 --- a/crates/ghostty-bench/src/bin/ghostty-synthetic.rs +++ b/crates/ghostty-bench/src/bin/ghostty-synthetic.rs @@ -40,11 +40,14 @@ fn run() -> Result<(), String> { } "osc" => { let p_valid = args.take("p-valid", 0.5f64)?; - let style = match args.take_opt_string("style")?.as_deref() { - None | Some("streaming") => OscStyle::Streaming, - Some("parser") => OscStyle::Parser, - Some(s) => return Err(format!("invalid value for --style: {s}")), - }; + let style = args.take_enum( + "style", + OscStyle::Streaming, + &[ + ("streaming", OscStyle::Streaming), + ("parser", OscStyle::Parser), + ], + )?; args.finish()?; run_osc(&mut out, &mut rng, p_valid, style) } diff --git a/crates/ghostty-cli/src/edit_config.rs b/crates/ghostty-cli/src/edit_config.rs index 8f84045..c245276 100644 --- a/crates/ghostty-cli/src/edit_config.rs +++ b/crates/ghostty-cli/src/edit_config.rs @@ -122,11 +122,7 @@ fn exec_editor(stderr: &mut dyn Write, path: &EncodedOs) -> Result { .exec(); // If we reached this point then exec failed. - let err = match err.kind() { - std::io::ErrorKind::NotFound => "error.FileNotFound".to_string(), - std::io::ErrorKind::PermissionDenied => "error.AccessDenied".to_string(), - _ => err.to_string(), - }; + let err = ghostty_os::zig_error::io_error_name(&err); let msg = format!( "Failed to execute the editor. Error code={err}.\n\ \n\ diff --git a/crates/ghostty-cli/src/list_themes/preview.rs b/crates/ghostty-cli/src/list_themes/preview.rs index 8f0df93..2754f95 100644 --- a/crates/ghostty-cli/src/list_themes/preview.rs +++ b/crates/ghostty-cli/src/list_themes/preview.rs @@ -1396,11 +1396,9 @@ impl Preview { fn error_name(e: &ghostty_config::config::Error) -> String { use ghostty_config::config::Error; match e { - Error::Io(io) => match io.kind() { - std::io::ErrorKind::NotFound => "FileNotFound".to_string(), - std::io::ErrorKind::PermissionDenied => "AccessDenied".to_string(), - _ => io.to_string(), - }, + Error::Io(io) => ghostty_os::zig_error::io_error_name(io) + .trim_start_matches("error.") + .to_owned(), e => e.to_string(), } } diff --git a/crates/ghostty-config/src/config.rs b/crates/ghostty-config/src/config.rs index 17595db..a5c0998 100644 --- a/crates/ghostty-config/src/config.rs +++ b/crates/ghostty-config/src/config.rs @@ -3752,7 +3752,7 @@ crate::config_struct! { /// Errors from loading configuration. #[derive(Debug, thiserror::Error)] pub enum Error { - #[error("{0}")] + #[error("{}", ghostty_os::zig_error::io_error_name(.0))] Io(#[from] std::io::Error), #[error("{0}")] Arg(#[from] ArgError), @@ -4273,7 +4273,7 @@ impl Config { message: format!( "error opening config-file {}: {}", path.as_encoded(), - zig_io_error_name(&err) + ghostty_os::zig_error::io_error_name(&err) ), ..Diagnostic::default() }; @@ -4806,40 +4806,6 @@ impl Iterator for ChangeIterator<'_> { } } -/// Name of an IO error like Zig's `{}` formatting of an error value -/// (`error.FileNotFound`), used in user-visible diagnostics. -fn zig_io_error_name(err: &std::io::Error) -> String { - use std::io::ErrorKind; - #[cfg(unix)] - if let Some(errno) = err.raw_os_error() { - let name = match errno { - 2 => Some("FileNotFound"), - 13 => Some("AccessDenied"), - 1 => Some("PermissionDenied"), - 21 => Some("IsDir"), - 20 => Some("NotDir"), - 40 => Some("SymLinkLoop"), - 36 => Some("NameTooLong"), - 24 => Some("ProcessFdQuotaExceeded"), - 23 => Some("SystemFdQuotaExceeded"), - 12 => Some("SystemResources"), - 19 => Some("NoDevice"), - 16 => Some("DeviceBusy"), - 26 => Some("FileBusy"), - 30 => Some("ReadOnlyFileSystem"), - _ => None, - }; - if let Some(name) = name { - return format!("error.{name}"); - } - } - match err.kind() { - ErrorKind::NotFound => "error.FileNotFound".to_string(), - ErrorKind::PermissionDenied => "error.AccessDenied".to_string(), - _ => "error.Unexpected".to_string(), - } -} - fn file_kind_name(meta: &std::fs::Metadata) -> &'static str { let ft = meta.file_type(); if ft.is_dir() { diff --git a/crates/ghostty-config/src/file_load.rs b/crates/ghostty-config/src/file_load.rs index 1d68493..570b240 100644 --- a/crates/ghostty-config/src/file_load.rs +++ b/crates/ghostty-config/src/file_load.rs @@ -170,15 +170,20 @@ pub fn open(path: &EncodedOs) -> Result { OpenFileError::FileNotFound } else { log::warn!( - "unexpected file open error path={} err={err}", - real.display() + "unexpected file open error path={} err={}", + real.display(), + ghostty_os::zig_error::io_error_name(&err) ); OpenFileError::FileOpenFailed } })?; let meta = file.metadata().map_err(|err| { - log::warn!("error getting file stat path={} err={err}", real.display()); + log::warn!( + "error getting file stat path={} err={}", + real.display(), + ghostty_os::zig_error::io_error_name(&err) + ); OpenFileError::FileOpenFailed })?; if !meta.is_file() { diff --git a/crates/ghostty-config/src/path.rs b/crates/ghostty-config/src/path.rs index 5414db7..340e7f6 100644 --- a/crates/ghostty-config/src/path.rs +++ b/crates/ghostty-config/src/path.rs @@ -164,7 +164,11 @@ impl Path { } Err(err) => { diags.append(Diagnostic { - message: format!("error resolving file path {}: {err}", path.as_encoded()), + message: format!( + "error resolving file path {}: {}", + path.as_encoded(), + ghostty_os::zig_error::io_error_name(&err) + ), ..Diagnostic::default() }); diff --git a/crates/ghostty-config/src/theme.rs b/crates/ghostty-config/src/theme.rs index bc421b4..caf5bcd 100644 --- a/crates/ghostty-config/src/theme.rs +++ b/crates/ghostty-config/src/theme.rs @@ -128,7 +128,13 @@ pub(crate) fn special_kind_name(_: std::fs::FileType) -> &'static str { fn check_regular(file: &File, theme: &str, diags: &mut DiagnosticList) -> bool { match file.metadata() { Err(err) => { - diag(diags, format!("not reading theme from \"{theme}\": {err}")); + diag( + diags, + format!( + "not reading theme from \"{theme}\": {}", + ghostty_os::zig_error::io_error_name(&err) + ), + ); false } Ok(m) if !m.is_file() => { @@ -197,7 +203,7 @@ pub fn open( let mut it = LocationIterator::new(resources_dir); while let Some((_, dir)) = it.next_location() { let path = EncodedOs::from_path(&dir.to_path().ok()?.join(&theme_path)); - match path.to_path().and_then(File::open) { + match path.to_syscall_path().and_then(File::open) { Ok(file) => { if !check_regular(&file, theme_text, diags) { return None; @@ -211,8 +217,9 @@ pub fn open( diag( diags, format!( - "failed to load theme \"{theme_text}\" from the file \"{}\": {err}", - path.as_encoded() + "failed to load theme \"{theme_text}\" from the file \"{}\": {}", + path.as_encoded(), + ghostty_os::zig_error::io_error_name(&err) ), ); return None; @@ -241,7 +248,7 @@ pub fn open( /// then messages will be appended to the given error list and None is /// returned. pub fn open_absolute(theme: &EncodedOs, diags: &mut DiagnosticList) -> Option { - let path = theme.to_path(); + let path = theme.to_syscall_path(); let theme = theme.as_encoded(); match path.and_then(File::open) { Ok(f) => Some(f), @@ -254,7 +261,10 @@ pub fn open_absolute(theme: &EncodedOs, diags: &mut DiagnosticList) -> Option Zig error name (`{}` of an error value, e.g. +//! `error.FileNotFound`), following Zig std's posix errno mapping for the +//! calls Ghostty makes (`openat`, `fstat`, `realpath`, `execve`, ...). + +/// Zig error name (without the `error.` prefix) for a Linux errno. +#[must_use] +pub fn errno_name(errno: i32) -> &'static str { + match errno { + 1 => "PermissionDenied", // EPERM + 2 => "FileNotFound", // ENOENT + 3 => "ProcessNotFound", // ESRCH + 6 | 19 => "NoDevice", // ENXIO, ENODEV + 11 => "WouldBlock", // EAGAIN + 12 => "SystemResources", // ENOMEM + 13 => "AccessDenied", // EACCES + 16 => "DeviceBusy", // EBUSY + 17 => "PathAlreadyExists", // EEXIST + 20 => "NotDir", // ENOTDIR + 21 => "IsDir", // EISDIR + 23 => "SystemFdQuotaExceeded", // ENFILE + 24 => "ProcessFdQuotaExceeded", // EMFILE + 26 => "FileBusy", // ETXTBSY + 27 | 75 => "FileTooBig", // EFBIG, EOVERFLOW + 28 => "NoSpaceLeft", // ENOSPC + 30 => "ReadOnlyFileSystem", // EROFS + 36 => "NameTooLong", // ENAMETOOLONG + 40 => "SymLinkLoop", // ELOOP + 84 => "InvalidWtf8", // EILSEQ + 95 => "FileLocksNotSupported", // EOPNOTSUPP + _ => "Unexpected", + } +} + +/// `error.Name` text for an I/O error. +#[must_use] +pub fn io_error_name(err: &std::io::Error) -> String { + if let Some(errno) = err.raw_os_error() { + return format!("error.{}", errno_name(errno)); + } + match err.kind() { + std::io::ErrorKind::NotFound => "error.FileNotFound".into(), + std::io::ErrorKind::PermissionDenied => "error.AccessDenied".into(), + std::io::ErrorKind::InvalidInput => "error.BadPathName".into(), + _ => "error.Unexpected".into(), + } +} + +#[cfg(all(test, unix))] +mod tests { + use super::*; + + #[test] + fn names() { + let n = |e| io_error_name(&std::io::Error::from_raw_os_error(e)); + assert_eq!(n(2), "error.FileNotFound"); + assert_eq!(n(20), "error.NotDir"); + assert_eq!(n(13), "error.AccessDenied"); + assert_eq!(n(21), "error.IsDir"); + assert_eq!(n(36), "error.NameTooLong"); + assert_eq!(n(40), "error.SymLinkLoop"); + assert_eq!(n(9999), "error.Unexpected"); + } +} diff --git a/docs/rewrite/notes/final-Z14.md b/docs/rewrite/notes/final-Z14.md new file mode 100644 index 0000000..708c885 --- /dev/null +++ b/docs/rewrite/notes/final-Z14.md @@ -0,0 +1,5 @@ +# Z14 (RV11 gaps) + +1. ghostty-bench: `Args::take_enum` validates every occurrence of an enum option in order (empty value resets to default), used by codepoint-width, grapheme-break, is-symbol, screen-clone, terminal-stream `--handler` and synthetic `+osc --style`. Tests in args.rs / benches.rs. +2. Theme open (named and absolute) uses `to_syscall_path` so an interior NUL truncates the path like Zig's C string. Test `theme_absolute_interior_nul_opens_prefix`; checked against /usr/bin/ghostty (no diagnostics, loads). +3. New `ghostty_os::zig_error::{errno_name, io_error_name}` (Zig std posix errno mapping). Used by theme.rs, path.rs, config.rs (config-file open, `config::Error::Io` Display), file_load.rs logs, edit_config.rs, list_themes preview. Verified identical to oracle for FileNotFound, NotDir, AccessDenied, IsDir ("it is a directory" path), NameTooLong, SymLinkLoop.