From 13c17267b97f44fbaf28b6bd4961e070db113d33 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Fri, 19 Apr 2024 16:24:35 +0300 Subject: [PATCH] Properly pass nested script arguments for tasks (#10776) Closes https://github.com/zed-industries/zed/discussions/10732#discussion-6524347 introduced by https://github.com/zed-industries/zed/pull/10548 while keeping both Python and Bash run selection capabilities. Also replaced redundant `SpawnTask` struct with `SpawnInTerminal` that has identical fields. Release Notes: - Fixed incorrect task escaping of nested script arguments --------- Co-authored-by: Piotr Osiewicz --- Cargo.lock | 1 - Cargo.toml | 1 - crates/languages/src/python.rs | 8 +--- crates/project/src/terminals.rs | 14 ++---- crates/task/src/lib.rs | 17 ++------ crates/task/src/task_template.rs | 25 ++++++++--- crates/tasks_ui/src/modal.rs | 10 ++--- crates/terminal/src/terminal.rs | 13 +----- crates/terminal_view/Cargo.toml | 1 - crates/terminal_view/src/terminal_panel.rs | 50 +++++++++------------- 10 files changed, 55 insertions(+), 85 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index db5620acba..d3f300019d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9842,7 +9842,6 @@ dependencies = [ "serde_json", "settings", "shellexpand", - "shlex", "smol", "task", "terminal", diff --git a/Cargo.toml b/Cargo.toml index 0841a599bb..4b9bad5554 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -301,7 +301,6 @@ serde_json_lenient = { version = "0.1", features = [ ] } serde_repr = "0.1" sha2 = "0.10" -shlex = "1.3" shellexpand = "2.1.0" smallvec = { version = "1.6", features = ["union"] } smol = "1.2" diff --git a/crates/languages/src/python.rs b/crates/languages/src/python.rs index 192cb1b114..858dafb716 100644 --- a/crates/languages/src/python.rs +++ b/crates/languages/src/python.rs @@ -186,13 +186,7 @@ pub(super) fn python_task_context() -> ContextProviderWithTasks { TaskTemplate { label: "execute selection".to_owned(), command: "python3".to_owned(), - args: vec![ - "-c".to_owned(), - format!( - "exec(r'''{}''')", - VariableName::SelectedText.template_value() - ), - ], + args: vec!["-c".to_owned(), VariableName::SelectedText.template_value()], ignore_previously_resolved: true, ..TaskTemplate::default() }, diff --git a/crates/project/src/terminals.rs b/crates/project/src/terminals.rs index 12c743dc64..4d1829dd42 100644 --- a/crates/project/src/terminals.rs +++ b/crates/project/src/terminals.rs @@ -4,9 +4,10 @@ use gpui::{AnyWindowHandle, Context, Entity, Model, ModelContext, WeakModel}; use settings::Settings; use smol::channel::bounded; use std::path::{Path, PathBuf}; +use task::SpawnInTerminal; use terminal::{ terminal_settings::{self, Shell, TerminalSettings, VenvSettingsContent}, - SpawnTask, TaskState, TaskStatus, Terminal, TerminalBuilder, + TaskState, TaskStatus, Terminal, TerminalBuilder, }; use util::ResultExt; @@ -21,7 +22,7 @@ impl Project { pub fn create_terminal( &mut self, working_directory: Option, - spawn_task: Option, + spawn_task: Option, window: AnyWindowHandle, cx: &mut ModelContext, ) -> anyhow::Result> { @@ -55,14 +56,7 @@ impl Project { id: spawn_task.id, full_label: spawn_task.full_label, label: spawn_task.label, - command_label: spawn_task.args.iter().fold( - spawn_task.command.clone(), - |mut command_label, new_arg| { - command_label.push(' '); - command_label.push_str(new_arg); - command_label - }, - ), + command_label: spawn_task.command_label, status: TaskStatus::Running, completion_rx, }), diff --git a/crates/task/src/lib.rs b/crates/task/src/lib.rs index 327af8c3ff..2e3a0e778d 100644 --- a/crates/task/src/lib.rs +++ b/crates/task/src/lib.rs @@ -31,8 +31,11 @@ pub struct SpawnInTerminal { pub label: String, /// Executable command to spawn. pub command: String, - /// Arguments to the command. + /// Arguments to the command, potentially unsubstituted, + /// to let the shell that spawns the command to do the substitution, if needed. pub args: Vec, + /// A human-readable label, containing command and all of its arguments, joined and substituted. + pub command_label: String, /// Current working directory to spawn the command into. pub cwd: Option, /// Env overrides for the command, will be appended to the terminal's environment from the settings. @@ -76,18 +79,6 @@ impl ResolvedTask { &self.substituted_variables } - /// If the resolution produced a task with the command, returns a string, combined from its command and arguments. - pub fn resolved_command(&self) -> Option { - self.resolved.as_ref().map(|resolved| { - let mut command = resolved.command.clone(); - for arg in &resolved.args { - command.push(' '); - command.push_str(arg); - } - command - }) - } - /// A human-readable label to display in the UI. pub fn display_label(&self) -> &str { self.resolved diff --git a/crates/task/src/task_template.rs b/crates/task/src/task_template.rs index 2daf88b0de..397c83e94c 100644 --- a/crates/task/src/task_template.rs +++ b/crates/task/src/task_template.rs @@ -156,7 +156,7 @@ impl TaskTemplate { &variable_names, &mut substituted_variables, )?; - let args = substitute_all_template_variables_in_vec( + let args_with_substitutions = substitute_all_template_variables_in_vec( &self.args, &task_variables, &variable_names, @@ -187,8 +187,16 @@ impl TaskTemplate { cwd, full_label, label: human_readable_label, + command_label: args_with_substitutions.iter().fold( + command.clone(), + |mut command_label, arg| { + command_label.push(' '); + command_label.push_str(arg); + command_label + }, + ), command, - args, + args: self.args.clone(), env, use_new_terminal: self.use_new_terminal, allow_concurrent_runs: self.allow_concurrent_runs, @@ -524,11 +532,16 @@ mod tests { assert_eq!( spawn_in_terminal.args, &[ - "arg1 test_selected_text", - "arg2 5678", - &format!("arg3 {long_value}") + "arg1 $ZED_SELECTED_TEXT", + "arg2 $ZED_COLUMN", + "arg3 $ZED_SYMBOL", ], - "Args should be substituted with variables and those should not be shortened" + "Args should not be substituted with variables" + ); + assert_eq!( + spawn_in_terminal.command_label, + format!("{} arg1 test_selected_text arg2 5678 arg3 {long_value}", spawn_in_terminal.command), + "Command label args should be substituted with variables and those should not be shortened" ); assert_eq!( diff --git a/crates/tasks_ui/src/modal.rs b/crates/tasks_ui/src/modal.rs index 2fa045f481..f3587f94c3 100644 --- a/crates/tasks_ui/src/modal.rs +++ b/crates/tasks_ui/src/modal.rs @@ -307,16 +307,16 @@ impl PickerDelegate for TasksModalDelegate { let display_label = resolved_task.display_label(); let mut tooltip_label_text = if display_label != &template.label { - template.label.clone() + resolved_task.resolved_label.clone() } else { String::new() }; - if let Some(resolved_command) = resolved_task.resolved_command() { - if display_label != resolved_command { + if let Some(resolved) = resolved_task.resolved.as_ref() { + if display_label != resolved.command_label { if !tooltip_label_text.trim().is_empty() { tooltip_label_text.push('\n'); } - tooltip_label_text.push_str(&resolved_command); + tooltip_label_text.push_str(&resolved.command_label); } } let tooltip_label = if tooltip_label_text.trim().is_empty() { @@ -392,7 +392,7 @@ impl PickerDelegate for TasksModalDelegate { let task_index = self.matches.get(self.selected_index())?.candidate_id; let tasks = self.candidates.as_ref()?; let (_, task) = tasks.get(task_index)?; - task.resolved_command() + Some(task.resolved.as_ref()?.command_label.clone()) } fn confirm_input(&mut self, omit_history_entry: bool, cx: &mut ViewContext>) { diff --git a/crates/terminal/src/terminal.rs b/crates/terminal/src/terminal.rs index c30b87c015..54bbdc8f0c 100644 --- a/crates/terminal/src/terminal.rs +++ b/crates/terminal/src/terminal.rs @@ -39,7 +39,7 @@ use pty_info::PtyProcessInfo; use serde::{Deserialize, Serialize}; use settings::Settings; use smol::channel::{Receiver, Sender}; -use task::{RevealStrategy, TaskId}; +use task::TaskId; use terminal_settings::{AlternateScroll, Shell, TerminalBlink, TerminalSettings}; use theme::{ActiveTheme, Theme}; use util::truncate_and_trailoff; @@ -286,17 +286,6 @@ impl Display for TerminalError { } } -#[derive(Debug)] -pub struct SpawnTask { - pub id: TaskId, - pub full_label: String, - pub label: String, - pub command: String, - pub args: Vec, - pub env: HashMap, - pub reveal: RevealStrategy, -} - // https://github.com/alacritty/alacritty/blob/cb3a79dbf6472740daca8440d5166c1d4af5029e/extra/man/alacritty.5.scd?plain=1#L207-L213 const DEFAULT_SCROLL_HISTORY_LINES: usize = 10_000; const MAX_SCROLL_HISTORY_LINES: usize = 100_000; diff --git a/crates/terminal_view/Cargo.toml b/crates/terminal_view/Cargo.toml index 029ac36f5f..d9e3606d4d 100644 --- a/crates/terminal_view/Cargo.toml +++ b/crates/terminal_view/Cargo.toml @@ -28,7 +28,6 @@ search.workspace = true serde.workspace = true serde_json.workspace = true settings.workspace = true -shlex.workspace = true shellexpand.workspace = true smol.workspace = true terminal.workspace = true diff --git a/crates/terminal_view/src/terminal_panel.rs b/crates/terminal_view/src/terminal_panel.rs index 749c4f0b93..475e670cea 100644 --- a/crates/terminal_view/src/terminal_panel.rs +++ b/crates/terminal_view/src/terminal_panel.rs @@ -1,4 +1,4 @@ -use std::{borrow::Cow, ops::ControlFlow, path::PathBuf, sync::Arc}; +use std::{ops::ControlFlow, path::PathBuf, sync::Arc}; use crate::TerminalView; use collections::{HashMap, HashSet}; @@ -15,10 +15,7 @@ use search::{buffer_search::DivRegistrar, BufferSearchBar}; use serde::{Deserialize, Serialize}; use settings::Settings; use task::{RevealStrategy, SpawnInTerminal, TaskId}; -use terminal::{ - terminal_settings::{Shell, TerminalDockPosition, TerminalSettings}, - SpawnTask, -}; +use terminal::terminal_settings::{Shell, TerminalDockPosition, TerminalSettings}; use ui::{h_flex, ButtonCommon, Clickable, IconButton, IconSize, Selectable, Tooltip}; use util::{ResultExt, TryFutureExt}; use workspace::{ @@ -301,36 +298,31 @@ impl TerminalPanel { } fn spawn_task(&mut self, spawn_in_terminal: &SpawnInTerminal, cx: &mut ViewContext) { - let mut spawn_task = SpawnTask { - id: spawn_in_terminal.id.clone(), - full_label: spawn_in_terminal.full_label.clone(), - label: spawn_in_terminal.label.clone(), - command: spawn_in_terminal.command.clone(), - args: spawn_in_terminal.args.clone(), - env: spawn_in_terminal.env.clone(), - reveal: spawn_in_terminal.reveal, - }; + let mut spawn_task = spawn_in_terminal.clone(); // Set up shell args unconditionally, as tasks are always spawned inside of a shell. let Some((shell, mut user_args)) = (match TerminalSettings::get_global(cx).shell.clone() { - Shell::System => std::env::var("SHELL").ok().map(|shell| (shell, vec![])), - Shell::Program(shell) => Some((shell, vec![])), + Shell::System => std::env::var("SHELL").ok().map(|shell| (shell, Vec::new())), + Shell::Program(shell) => Some((shell, Vec::new())), Shell::WithArguments { program, args } => Some((program, args)), }) else { return; }; - let mut command = std::mem::take(&mut spawn_task.command); - let args = std::mem::take(&mut spawn_task.args); - for arg in args { - command.push(' '); - let arg = shlex::try_quote(&arg).unwrap_or(Cow::Borrowed(&arg)); - command.push_str(&arg); - } - spawn_task.command = shell; - user_args.extend(["-i".to_owned(), "-c".to_owned(), command]); + spawn_task.command_label = format!("{shell} -i -c `{}`", spawn_task.command_label); + let task_command = std::mem::replace(&mut spawn_task.command, shell); + let task_args = std::mem::take(&mut spawn_task.args); + let combined_command = task_args + .into_iter() + .fold(task_command, |mut command, arg| { + command.push(' '); + command.push_str(&arg); + command + }); + user_args.extend(["-i".to_owned(), "-c".to_owned(), combined_command]); spawn_task.args = user_args; - let reveal = spawn_task.reveal; + let spawn_task = spawn_task; + let reveal = spawn_task.reveal; let working_directory = spawn_in_terminal.cwd.clone(); let allow_concurrent_runs = spawn_in_terminal.allow_concurrent_runs; let use_new_terminal = spawn_in_terminal.use_new_terminal; @@ -406,7 +398,7 @@ impl TerminalPanel { fn spawn_in_new_terminal( &mut self, - spawn_task: SpawnTask, + spawn_task: SpawnInTerminal, working_directory: Option, cx: &mut ViewContext, ) { @@ -469,7 +461,7 @@ impl TerminalPanel { fn add_terminal( &mut self, working_directory: Option, - spawn_task: Option, + spawn_task: Option, cx: &mut ViewContext, ) { let workspace = self.workspace.clone(); @@ -561,7 +553,7 @@ impl TerminalPanel { fn replace_terminal( &self, working_directory: Option, - spawn_task: SpawnTask, + spawn_task: SpawnInTerminal, terminal_item_index: usize, terminal_to_replace: View, cx: &mut ViewContext<'_, Self>,