From 5d467418be7f19ea4e4da71fc132c0b85642f585 Mon Sep 17 00:00:00 2001 From: Lorenz Leutgeb Date: Tue, 5 Aug 2025 13:19:03 +0200 Subject: [PATCH] term: Revert using inquire to spawn editor This is morally a revert of 70fb0d3fef0bec250bdccd58bcb6f4601adbe065 while additionally getting to compile `crates/radicle-term/src/editor.rs` on Windows. To get the changes that were made on top of the revert, check git diff 70fb0d3fef~1 -- crates/radicle-term/src/editor.rs There were reports of the inquire editor being unrealiable. --- Cargo.lock | 2 + crates/radicle-cli/src/commands/config.rs | 22 +- crates/radicle-cli/src/commands/id.rs | 5 +- .../src/commands/patch/review/builder.rs | 7 +- crates/radicle-cli/src/terminal/issue.rs | 4 +- crates/radicle-cli/src/terminal/patch.rs | 26 +- crates/radicle-term/Cargo.toml | 2 + crates/radicle-term/src/editor.rs | 224 ++++++++++++++---- 8 files changed, 206 insertions(+), 86 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 94509a69..cd21647f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2793,8 +2793,10 @@ dependencies = [ "crossterm 0.29.0", "git2", "inquire", + "libc", "pretty_assertions", "radicle-signals", + "shlex", "tempfile", "thiserror 1.0.69", "unicode-display-width", diff --git a/crates/radicle-cli/src/commands/config.rs b/crates/radicle-cli/src/commands/config.rs index 33fe7033..cfd32531 100644 --- a/crates/radicle-cli/src/commands/config.rs +++ b/crates/radicle-cli/src/commands/config.rs @@ -185,24 +185,12 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { path.display() ); } - Operation::Edit => { - let config = std::fs::read_to_string(&path)?; - - match term::editor::Editor::new("Edit configuration", term::editor::Editor::HELP) - .editor(term::editor::default_editor_command().as_ref()) - .extension(".json") - .initial(&config) - .edit()? - { - Some(edited) => { - std::fs::write(&path, edited)?; - term::success!("Successfully made changes to the configuration at {path:?}"); - } - None => { - term::info!("No changes were made to the configuration at {path:?}") - } + Operation::Edit => match term::editor::Editor::new(&path)?.extension("json").edit()? { + Some(_) => { + term::success!("Successfully made changes to the configuration at {path:?}") } - } + None => term::info!("No changes were made to the configuration at {path:?}"), + }, } Ok(()) diff --git a/crates/radicle-cli/src/commands/id.rs b/crates/radicle-cli/src/commands/id.rs index 64723a94..45d9281a 100644 --- a/crates/radicle-cli/src/commands/id.rs +++ b/crates/radicle-cli/src/commands/id.rs @@ -411,9 +411,8 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { // If `--edit` is specified, the document can also be edited via a text edit. let proposal = if edit { match term::editor::Editor::comment() - .editor(term::editor::default_editor_command().as_ref()) - .extension(".json") - .initial(&serde_json::to_string_pretty(¤t.doc)?) + .extension("json") + .initial(serde_json::to_string_pretty(¤t.doc)?)? .edit()? { Some(proposal) => serde_json::from_str::(&proposal)?, diff --git a/crates/radicle-cli/src/commands/patch/review/builder.rs b/crates/radicle-cli/src/commands/patch/review/builder.rs index dbbd7df9..fe7f2448 100644 --- a/crates/radicle-cli/src/commands/patch/review/builder.rs +++ b/crates/radicle-cli/src/commands/patch/review/builder.rs @@ -836,8 +836,6 @@ enum Error { Format(#[from] std::fmt::Error), #[error(transparent)] Git(#[from] git::raw::Error), - #[error("editor error: {0}")] - Editor(#[from] term::editor::Error), } #[derive(Debug)] @@ -862,9 +860,8 @@ impl CommentBuilder { writeln!(&mut input, "> {line}")?; } let output = term::Editor::comment() - .editor(term::editor::default_editor_command().as_ref()) - .extension(".diff") - .initial(&input) + .extension("diff") + .initial(input)? .edit()?; if let Some(output) = output { diff --git a/crates/radicle-cli/src/terminal/issue.rs b/crates/radicle-cli/src/terminal/issue.rs index d2857e84..518608ad 100644 --- a/crates/radicle-cli/src/terminal/issue.rs +++ b/crates/radicle-cli/src/terminal/issue.rs @@ -1,3 +1,5 @@ +use std::io; + use radicle_term::table::TableOptions; use radicle_term::{Table, VStack}; @@ -32,7 +34,7 @@ pub enum Format { pub fn get_title_description( title: Option, description: Option, -) -> Result, term::patch::Error> { +) -> io::Result> { term::patch::Message::edit_title_description(title, description, OPEN_MSG) } diff --git a/crates/radicle-cli/src/terminal/patch.rs b/crates/radicle-cli/src/terminal/patch.rs index 8f05be3a..4ff4ceda 100644 --- a/crates/radicle-cli/src/terminal/patch.rs +++ b/crates/radicle-cli/src/terminal/patch.rs @@ -31,10 +31,6 @@ pub enum Error { Io(#[from] io::Error), #[error("invalid utf-8 string")] InvalidUtf8, - #[error("editor error: {0}")] - Editor(#[from] term::editor::Error), - #[error("a patch title must be provided")] - PatchTitleMissing, } /// The user supplied `Patch` description. @@ -51,14 +47,13 @@ pub enum Message { impl Message { /// Get the `Message` as a string according to the method. - pub fn get(self, help: &str) -> Result { + pub fn get(self, help: &str) -> std::io::Result { let comment = match self { Message::Edit => { if io::stderr().is_terminal() { term::Editor::comment() - .editor(term::editor::default_editor_command().as_ref()) - .extension(".md") - .initial(help) + .extension("markdown") + .initial(help)? .edit()? } else { Some(help.to_owned()) @@ -80,7 +75,7 @@ impl Message { title: Option, description: Option, help: &str, - ) -> Result, Error> { + ) -> std::io::Result> { let mut placeholder = String::new(); if let Some(title) = title { @@ -233,7 +228,11 @@ pub fn get_create_message( let (title, description) = (title.trim().to_string(), description.trim().to_string()); if title.is_empty() { - return Err(Error::PatchTitleMissing); + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "a patch title must be provided", + ) + .into()); } Ok((title, description)) @@ -250,7 +249,7 @@ fn edit_display_message(title: &str, description: &str) -> String { pub fn get_edit_message( patch_message: term::patch::Message, patch: &cob::patch::Patch, -) -> Result<(String, String), Error> { +) -> io::Result<(String, String)> { let display_msg = edit_display_message(patch.title(), patch.description()); let patch_message = patch_message.get(&display_msg)?; let patch_message = patch_message.replace(PATCH_MSG.trim(), ""); // Delete help message. @@ -261,7 +260,10 @@ pub fn get_edit_message( let (title, description) = (title.trim().to_string(), description.trim().to_string()); if title.is_empty() { - return Err(Error::PatchTitleMissing); + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "a patch title must be provided", + )); } Ok((title, description)) diff --git a/crates/radicle-term/Cargo.toml b/crates/radicle-term/Cargo.toml index 1f39823c..83ae1f75 100644 --- a/crates/radicle-term/Cargo.toml +++ b/crates/radicle-term/Cargo.toml @@ -22,9 +22,11 @@ unicode-display-width = "0.3.0" unicode-segmentation = "1.7.1" zeroize = { workspace = true } git2 = { workspace = true, features = ["vendored-libgit2"], optional = true } +shlex = { workspace = true } [target.'cfg(unix)'.dependencies] crossbeam-channel = { workspace = true } +libc = { workspace = true } radicle-signals = { workspace = true } [dev-dependencies] diff --git a/crates/radicle-term/src/editor.rs b/crates/radicle-term/src/editor.rs index de96e74c..4b4d4fd0 100644 --- a/crates/radicle-term/src/editor.rs +++ b/crates/radicle-term/src/editor.rs @@ -1,69 +1,197 @@ -use std::env; use std::ffi::OsString; -use std::path::Path; - -use inquire::InquireError; -use thiserror::Error; +use std::io::IsTerminal; +use std::io::Write; +use std::path::{Path, PathBuf}; +use std::process; +use std::{env, fs, io}; /// Allows for text input in the configured editor. -pub struct Editor<'a>(pub inquire::Editor<'a>); +pub struct Editor { + path: PathBuf, + truncate: bool, + cleanup: bool, +} -#[derive(Debug, Error)] -#[error(transparent)] -pub struct Error(#[from] InquireError); +impl Default for Editor { + fn default() -> Self { + Self::comment() + } +} -impl<'a> Editor<'a> { - pub const HELP: &'a str = "(e) to edit. (enter) to save and exit. (esc) to cancel and quit."; +impl Drop for Editor { + fn drop(&mut self) { + if self.cleanup { + fs::remove_file(&self.path).ok(); + } + } +} + +impl Editor { + /// Create a new editor. + pub fn new(path: impl AsRef) -> io::Result { + let path = path.as_ref(); + if path.try_exists()? { + let meta = fs::metadata(path)?; + if !meta.is_file() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "must be used to edit a file", + )); + } + } + Ok(Self { + path: path.to_path_buf(), + truncate: false, + cleanup: false, + }) + } - /// Create a new editor for editing a comment. pub fn comment() -> Self { - Self::new("Enter comment.", Self::HELP) + const COMMENT_FILE: &str = "RAD_COMMENT"; + + let path = env::temp_dir().join(COMMENT_FILE); + + Self { + path, + truncate: true, + cleanup: true, + } + } + + /// Set the file extension. + pub fn extension(mut self, ext: &str) -> Self { + let ext = ext.trim_start_matches('.'); + + self.path.set_extension(ext); + self + } + + /// Truncate the file to length 0 when opening + pub fn truncate(mut self, truncate: bool) -> Self { + self.truncate = truncate; + self + } + + /// Clean up the file after the [`Editor`] is dropped. + pub fn cleanup(mut self, cleanup: bool) -> Self { + self.cleanup = cleanup; + self + } + + /// Initialize the file with the provided `content`, as long as the file + /// does not already contain anything. + #[allow(clippy::byte_char_slices)] + pub fn initial(self, content: impl AsRef<[u8]>) -> io::Result { + let content = content.as_ref(); + let mut file = fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(self.truncate) + .open(&self.path)?; + + if file.metadata()?.len() == 0 { + file.write_all(content)?; + if !content.ends_with(&[b'\n']) { + file.write_all(b"\n")?; + } + file.flush()?; + } + Ok(self) } /// Open the editor and return the edited text. /// /// If the text hasn't changed from the initial contents of the editor, /// return `None`. - pub fn edit(self) -> Result, Error> { - match self.0.prompt() { - Err(InquireError::OperationCanceled | InquireError::OperationInterrupted) => Ok(None), - Err(e) => Err(Error::from(e)), - Ok(s) => Ok(Some(s)), - } - } -} - -impl<'a> Editor<'a> { - /// Create a new editor. - pub fn new(message: &'a str, help_message: &'a str) -> Self { - Self(inquire::Editor::new(message).with_help_message(help_message)) - } - - /// Set the file extension. - pub fn extension(self, ext: &'a str) -> Self { - debug_assert!( - ext.starts_with('.'), - "File extension should start with a dot." - ); - Self(self.0.with_file_extension(ext)) - } - - /// Initialize the file with the provided `content`, as long as the file - /// does not already contain anything. - pub fn initial(self, content: &'a str) -> Self { - Self(self.0.with_predefined_text(content)) - } - - pub fn editor(self, editor: Option<&'a OsString>) -> Self { - match editor { - Some(editor_command) => Self(self.0.with_editor_command(editor_command)), - None => self, + pub fn edit(&mut self) -> io::Result> { + let Some(cmd) = self::default_editor() else { + return Err(io::Error::new( + io::ErrorKind::NotFound, + "editor not configured: the `EDITOR` environment variable is not set", + )); + }; + let Some(parts) = shlex::split(cmd.to_string_lossy().as_ref()) else { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("invalid editor command {cmd:?}"), + )); + }; + let Some((program, args)) = parts.split_first() else { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("invalid editor command {cmd:?}"), + )); + }; + + let stdout: process::Stdio = { + #[cfg(unix)] + { + use std::os::fd::{AsRawFd as _, FromRawFd as _}; + + // We duplicate the stderr file descriptor to pass it to the child process, otherwise, if + // we simply pass the `RawFd` of our stderr, `Command` will close our stderr when the + // child exits. + + let stderr = io::stderr().as_raw_fd(); + unsafe { process::Stdio::from_raw_fd(libc::dup(stderr)) } + } + #[cfg(not(unix))] + { + // No duplication of the file handle for stderr on Windows. + // This might not always work, but is better than not being able to build for + // Windows. + io::stderr().into() + } + }; + + let stdin = if io::stdin().is_terminal() { + process::Stdio::inherit() + } else if cfg!(unix) { + // If standard input is not a terminal device, the editor won't work correctly. + // In that case, we use the terminal device, eg. `/dev/tty` as standard input. + let tty = fs::OpenOptions::new() + .read(true) + .write(true) + .open("/dev/tty")?; + process::Stdio::from(tty) + } else { + return Err(io::Error::new( + io::ErrorKind::Unsupported, + format!("standard input is not a terminal, refusing to execute editor {cmd:?}"), + )); + }; + + process::Command::new(program) + .stdout(stdout) + .stderr(process::Stdio::inherit()) + .stdin(stdin) + .args(args) + .arg(&self.path) + .spawn() + .map_err(|e| { + io::Error::new( + e.kind(), + format!("failed to spawn editor command {cmd:?}: {e}"), + ) + })? + .wait() + .map_err(|e| { + io::Error::new( + e.kind(), + format!("editor command {cmd:?} didn't spawn: {e}"), + ) + })?; + + let text = fs::read_to_string(&self.path)?; + if text.trim().is_empty() { + return Ok(None); } + Ok(Some(text)) } } /// Get the default editor command. -pub fn default_editor_command() -> Option { +fn default_editor() -> Option { // First check the standard environment variables. if let Ok(visual) = env::var("VISUAL") { if !visual.is_empty() {