From c4e87444e7e99a91f07eb586b7f1ba4b18d84bc7 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Wed, 22 May 2024 14:36:15 +0300 Subject: [PATCH] Tidy up the code (#12116) Small follow-ups for https://github.com/zed-industries/zed/pull/12063 and https://github.com/zed-industries/zed/pull/12103 Release Notes: - N/A --- crates/collab/src/db/queries/dev_servers.rs | 6 +- crates/collab/src/rpc.rs | 2 +- crates/editor/src/tasks.rs | 2 +- crates/file_finder/src/file_finder.rs | 7 +- crates/project/src/terminals.rs | 186 +++++++++++--------- crates/task/src/lib.rs | 16 +- crates/task/src/task_template.rs | 9 +- crates/terminal/src/terminal_settings.rs | 2 +- crates/terminal_view/src/terminal_view.rs | 4 +- 9 files changed, 130 insertions(+), 104 deletions(-) diff --git a/crates/collab/src/db/queries/dev_servers.rs b/crates/collab/src/db/queries/dev_servers.rs index b61da6fd57..fd5bcc8c47 100644 --- a/crates/collab/src/db/queries/dev_servers.rs +++ b/crates/collab/src/db/queries/dev_servers.rs @@ -137,7 +137,7 @@ impl Database { &self, id: DevServerId, name: &str, - ssh_connection_string: &Option, + ssh_connection_string: Option<&str>, user_id: UserId, ) -> crate::Result { self.transaction(|tx| async move { @@ -150,7 +150,9 @@ impl Database { dev_server::Entity::update(dev_server::ActiveModel { name: ActiveValue::Set(name.trim().to_string()), - ssh_connection_string: ActiveValue::Set(ssh_connection_string.clone()), + ssh_connection_string: ActiveValue::Set( + ssh_connection_string.map(ToOwned::to_owned), + ), ..dev_server.clone().into_active_model() }) .exec(&*tx) diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index afd7e6fe28..c558606569 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -2442,7 +2442,7 @@ async fn rename_dev_server( .rename_dev_server( dev_server_id, &request.name, - &request.ssh_connection_string, + request.ssh_connection_string.as_deref(), session.user_id(), ) .await?; diff --git a/crates/editor/src/tasks.rs b/crates/editor/src/tasks.rs index c39f38e55c..90f8b799c9 100644 --- a/crates/editor/src/tasks.rs +++ b/crates/editor/src/tasks.rs @@ -50,7 +50,7 @@ pub(crate) fn task_context_for_location( }) } -pub(crate) fn task_context_with_editor( +fn task_context_with_editor( workspace: &Workspace, editor: &mut Editor, cx: &mut WindowContext<'_>, diff --git a/crates/file_finder/src/file_finder.rs b/crates/file_finder/src/file_finder.rs index 8e07446802..b21fcbcb45 100644 --- a/crates/file_finder/src/file_finder.rs +++ b/crates/file_finder/src/file_finder.rs @@ -285,9 +285,9 @@ impl Matches { cmp::Ordering::Greater } - (Match::History(_, match_a), Match::History(_, match_b)) => match_b - .cmp(match_a) - .then(history_score_a.cmp(history_score_b)), + (Match::History(_, match_a), Match::History(_, match_b)) => { + match_b.cmp(match_a) + } (Match::History(_, match_a), Match::Search(match_b)) => { Some(match_b).cmp(&match_a.as_ref()) } @@ -296,6 +296,7 @@ impl Matches { } (Match::Search(match_a), Match::Search(match_b)) => match_b.cmp(match_a), } + .then(history_score_a.cmp(history_score_b)) }) .take(100) .map(|(_, m)| m) diff --git a/crates/project/src/terminals.rs b/crates/project/src/terminals.rs index 49aabe973f..b68775ec9c 100644 --- a/crates/project/src/terminals.rs +++ b/crates/project/src/terminals.rs @@ -3,6 +3,7 @@ use collections::HashMap; use gpui::{ AnyWindowHandle, AppContext, Context, Entity, Model, ModelContext, SharedString, WeakModel, }; +use itertools::Itertools; use settings::{Settings, SettingsLocation}; use smol::channel::bounded; use std::{ @@ -34,18 +35,18 @@ pub struct ConnectRemoteTerminal { impl Project { pub fn terminal_work_dir_for( &self, - pathbuf: Option<&PathBuf>, + pathbuf: Option<&Path>, cx: &AppContext, ) -> Option { if self.is_local() { - return Some(TerminalWorkDir::Local(pathbuf?.clone())); + return Some(TerminalWorkDir::Local(pathbuf?.to_owned())); } let dev_server_project_id = self.dev_server_project_id()?; let projects_store = dev_server_projects::Store::global(cx).read(cx); let ssh_command = projects_store .dev_server_for_project(dev_server_project_id)? .ssh_connection_string - .clone()? + .as_ref()? .to_string(); let path = if let Some(pathbuf) = pathbuf { @@ -72,9 +73,7 @@ impl Project { ) -> anyhow::Result> { // used only for TerminalSettings::get let worktree = { - let terminal_cwd = working_directory - .as_ref() - .and_then(|cwd| cwd.local_path().clone()); + let terminal_cwd = working_directory.as_ref().and_then(|cwd| cwd.local_path()); let task_cwd = spawn_task .as_ref() .and_then(|spawn_task| spawn_task.cwd.as_ref()) @@ -104,88 +103,23 @@ impl Project { let venv_base_directory = working_directory .as_ref() - .and_then(|cwd| cwd.local_path().map(|path| path.clone())) - .unwrap_or_else(|| PathBuf::new()) - .clone(); + .and_then(|cwd| cwd.local_path()) + .unwrap_or_else(|| Path::new("")); let (spawn_task, shell) = match working_directory.as_ref() { Some(TerminalWorkDir::Ssh { ssh_command, path }) => { log::debug!("Connecting to a remote server: {ssh_command:?}"); - // Alacritty sets its terminfo to `alacritty`, this requiring hosts to have it installed - // to properly display colors. - // We do not have the luxury of assuming the host has it installed, - // so we set it to a default that does not break the highlighting via ssh. - env.entry("TERM".to_string()) - .or_insert_with(|| "xterm-256color".to_string()); - let tmp_dir = tempfile::tempdir()?; - let real_ssh = which::which("ssh")?; - let ssh_path = tmp_dir.path().join("ssh"); - let mut ssh_file = File::create(ssh_path.clone())?; - - let to_run = if let Some(spawn_task) = spawn_task.as_ref() { - Some(shlex::try_quote(&spawn_task.command)?.to_string()) - .into_iter() - .chain(spawn_task.args.iter().filter_map(|arg| { - shlex::try_quote(arg).ok().map(|arg| arg.to_string()) - })) - .collect::>() - .join(" ") - } else { - "exec $SHELL -l".to_string() - }; - - let (port_forward, local_dev_env) = - if env::var("ZED_RPC_URL") == Ok("http://localhost:8080/rpc".to_string()) { - ( - "-R 8080:localhost:8080", - "export ZED_RPC_URL=http://localhost:8080/rpc;", - ) - } else { - ("", "") - }; - - let commands = if let Some(path) = path { - // I've found that `ssh -t dev sh -c 'cd; cd /tmp; pwd'` gives /tmp - // but `ssh -t dev sh -c 'cd /tmp; pwd'` gives /root - format!("cd {}; {} {}", path, local_dev_env, to_run) - } else { - format!("cd; {} {}", local_dev_env, to_run) - }; - - let shell_invocation = &format!("sh -c {}", shlex::try_quote(&commands)?); - - // To support things like `gh cs ssh`/`coder ssh`, we run whatever command - // you have configured, but place our custom script on the path so that it will - // be run instead. - write!( - &mut ssh_file, - "#!/bin/sh\nexec {} \"$@\" {} {} {}", - real_ssh.to_string_lossy(), - if spawn_task.is_none() { "-t" } else { "" }, - port_forward, - shlex::try_quote(shell_invocation)?, - )?; - // todo(windows) - #[cfg(not(target_os = "windows"))] - std::fs::set_permissions( - ssh_path, - smol::fs::unix::PermissionsExt::from_mode(0o755), - )?; - let path = format!( - "{}:{}", - tmp_dir.path().to_string_lossy(), - env.get("PATH") - .cloned() - .or(env::var("PATH").ok()) - .unwrap_or_default() + let ssh_shell_result = prepare_ssh_shell( + &mut env, + tmp_dir.path(), + spawn_task.as_ref(), + ssh_command, + path.as_deref(), ); - env.insert("PATH".to_string(), path); - - let mut args = shlex::split(&ssh_command).unwrap_or_default(); - let program = args.drain(0..1).next().unwrap_or("ssh".to_string()); - retained_script = Some(tmp_dir); + let ssh_shell = ssh_shell_result?; + ( spawn_task.map(|spawn_task| TaskState { id: spawn_task.id, @@ -195,7 +129,7 @@ impl Project { status: TaskStatus::Running, completion_rx, }), - Shell::WithArguments { program, args }, + ssh_shell, ) } _ => { @@ -231,11 +165,14 @@ impl Project { }; let terminal = TerminalBuilder::new( - working_directory.and_then(|cwd| cwd.local_path()).clone(), + working_directory + .as_ref() + .and_then(|cwd| cwd.local_path()) + .map(ToOwned::to_owned), spawn_task, shell, env, - Some(settings.blinking.clone()), + Some(settings.blinking), settings.alternate_scroll, settings.max_scroll_history_lines, window, @@ -375,3 +312,84 @@ impl Project { &self.terminals.local_handles } } + +fn prepare_ssh_shell( + env: &mut HashMap, + tmp_dir: &Path, + spawn_task: Option<&SpawnInTerminal>, + ssh_command: &str, + path: Option<&str>, +) -> anyhow::Result { + // Alacritty sets its terminfo to `alacritty`, this requiring hosts to have it installed + // to properly display colors. + // We do not have the luxury of assuming the host has it installed, + // so we set it to a default that does not break the highlighting via ssh. + env.entry("TERM".to_string()) + .or_insert_with(|| "xterm-256color".to_string()); + + let real_ssh = which::which("ssh")?; + let ssh_path = tmp_dir.join("ssh"); + let mut ssh_file = File::create(&ssh_path)?; + + let to_run = if let Some(spawn_task) = spawn_task { + Some(shlex::try_quote(&spawn_task.command)?) + .into_iter() + .chain( + spawn_task + .args + .iter() + .filter_map(|arg| shlex::try_quote(arg).ok()), + ) + .join(" ") + } else { + "exec $SHELL -l".to_string() + }; + + let (port_forward, local_dev_env) = + if env::var("ZED_RPC_URL").as_deref() == Ok("http://localhost:8080/rpc") { + ( + "-R 8080:localhost:8080", + "export ZED_RPC_URL=http://localhost:8080/rpc;", + ) + } else { + ("", "") + }; + + let commands = if let Some(path) = path { + // I've found that `ssh -t dev sh -c 'cd; cd /tmp; pwd'` gives /tmp + // but `ssh -t dev sh -c 'cd /tmp; pwd'` gives /root + format!("cd {path}; {local_dev_env} {to_run}") + } else { + format!("cd; {local_dev_env} {to_run}") + }; + let shell_invocation = &format!("sh -c {}", shlex::try_quote(&commands)?); + + // To support things like `gh cs ssh`/`coder ssh`, we run whatever command + // you have configured, but place our custom script on the path so that it will + // be run instead. + write!( + &mut ssh_file, + "#!/bin/sh\nexec {} \"$@\" {} {} {}", + real_ssh.to_string_lossy(), + if spawn_task.is_none() { "-t" } else { "" }, + port_forward, + shlex::try_quote(shell_invocation)?, + )?; + + // todo(windows) + #[cfg(not(target_os = "windows"))] + std::fs::set_permissions(ssh_path, smol::fs::unix::PermissionsExt::from_mode(0o755))?; + let path = format!( + "{}:{}", + tmp_dir.to_string_lossy(), + env.get("PATH") + .cloned() + .or(env::var("PATH").ok()) + .unwrap_or_default() + ); + env.insert("PATH".to_string(), path); + + let mut args = shlex::split(&ssh_command).unwrap_or_default(); + let program = args.drain(0..1).next().unwrap_or("ssh".to_string()); + Ok(Shell::WithArguments { program, args }) +} diff --git a/crates/task/src/lib.rs b/crates/task/src/lib.rs index 36b67c30be..bdd4ca48d6 100644 --- a/crates/task/src/lib.rs +++ b/crates/task/src/lib.rs @@ -8,8 +8,8 @@ mod vscode_format; use collections::{HashMap, HashSet}; use gpui::SharedString; use serde::Serialize; -use std::borrow::Cow; use std::path::PathBuf; +use std::{borrow::Cow, path::Path}; pub use task_template::{RevealStrategy, TaskTemplate, TaskTemplates}; pub use vscode_format::VsCodeTaskFile; @@ -34,19 +34,19 @@ pub enum TerminalWorkDir { } impl TerminalWorkDir { - /// is_local + /// Returns whether the terminal task is supposed to be spawned on a local machine or not. pub fn is_local(&self) -> bool { match self { - TerminalWorkDir::Local(_) => true, - TerminalWorkDir::Ssh { .. } => false, + Self::Local(_) => true, + Self::Ssh { .. } => false, } } - /// local_path - pub fn local_path(&self) -> Option { + /// Returns a local CWD if the terminal is local, None otherwise. + pub fn local_path(&self) -> Option<&Path> { match self { - TerminalWorkDir::Local(path) => Some(path.clone()), - TerminalWorkDir::Ssh { .. } => None, + Self::Local(path) => Some(path), + Self::Ssh { .. } => None, } } } diff --git a/crates/task/src/task_template.rs b/crates/task/src/task_template.rs index a10cc47fa8..2a48e48f3d 100644 --- a/crates/task/src/task_template.rs +++ b/crates/task/src/task_template.rs @@ -384,8 +384,9 @@ mod tests { assert_eq!( resolved_task(&task_without_cwd, &cx) .cwd + .as_ref() .and_then(|cwd| cwd.local_path()), - Some(context_cwd.clone()), + Some(context_cwd.as_path()), "TaskContext's cwd should be taken on resolve if task's cwd is None" ); @@ -401,8 +402,9 @@ mod tests { assert_eq!( resolved_task(&task_with_cwd, &cx) .cwd + .as_ref() .and_then(|cwd| cwd.local_path()), - Some(task_cwd.clone()), + Some(task_cwd.as_path()), "TaskTemplate's cwd should be taken on resolve if TaskContext's cwd is None" ); @@ -413,8 +415,9 @@ mod tests { assert_eq!( resolved_task(&task_with_cwd, &cx) .cwd + .as_ref() .and_then(|cwd| cwd.local_path()), - Some(task_cwd.clone()), + Some(task_cwd.as_path()), "TaskTemplate's cwd should be taken on resolve if TaskContext's cwd is not None" ); } diff --git a/crates/terminal/src/terminal_settings.rs b/crates/terminal/src/terminal_settings.rs index 3aae19ddc5..69f0db1aa0 100644 --- a/crates/terminal/src/terminal_settings.rs +++ b/crates/terminal/src/terminal_settings.rs @@ -241,7 +241,7 @@ impl TerminalLineHeight { } } -#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, JsonSchema)] +#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, Eq, JsonSchema)] #[serde(rename_all = "snake_case")] pub enum TerminalBlink { /// Never blink the cursor, ignoring the terminal mode. diff --git a/crates/terminal_view/src/terminal_view.rs b/crates/terminal_view/src/terminal_view.rs index 6974fe9eb9..543c90678d 100644 --- a/crates/terminal_view/src/terminal_view.rs +++ b/crates/terminal_view/src/terminal_view.rs @@ -889,7 +889,9 @@ impl Item for TerminalView { .as_ref() .is_some_and(|from_db| !from_db.as_os_str().is_empty()) { - project.read(cx).terminal_work_dir_for(from_db.as_ref(), cx) + project + .read(cx) + .terminal_work_dir_for(from_db.as_deref(), cx) } else { let strategy = TerminalSettings::get_global(cx).working_directory.clone(); workspace.upgrade().and_then(|workspace| {