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

@@ -137,7 +137,7 @@ impl Database {
         &self,
         id: DevServerId,
         name: &str,
-        ssh_connection_string: &Option<String>,
+        ssh_connection_string: Option<&str>,
         user_id: UserId,
     ) -> crate::Result<proto::DevServerProjectsUpdate> {
         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)

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?;

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<'_>,

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)

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<TerminalWorkDir> {
         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<Model<Terminal>> {
         // 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::<Vec<String>>()
-                        .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<String, String>,
+    tmp_dir: &Path,
+    spawn_task: Option<&SpawnInTerminal>,
+    ssh_command: &str,
+    path: Option<&str>,
+) -> anyhow::Result<Shell> {
+    // 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 })
+}

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<PathBuf> {
+    /// 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,
         }
     }
 }

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"
         );
     }

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.

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| {