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 <piotr@zed.dev>
2024-11-28 09:54:33 +00:00 · 2024-04-19 16:24:35 +03:00 · 2024-04-19 16:24:35 +03:00 · 13c17267b9
commit 13c17267b9
parent 9247da77a3
10 changed files with 55 additions and 85 deletions
--- a/Cargo.lock
+++ b/Cargo.lock
@ -9842,7 +9842,6 @@ dependencies = [
 "serde_json",
 "settings",
 "shellexpand",
- "shlex",
 "smol",
 "task",
 "terminal",
--- 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"
--- 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()
        },
--- 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<PathBuf>,
-        spawn_task: Option<SpawnTask>,
+        spawn_task: Option<SpawnInTerminal>,
        window: AnyWindowHandle,
        cx: &mut ModelContext<Self>,
    ) -> anyhow::Result<Model<Terminal>> {
@ -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,
                }),
--- 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<String>,
+    /// 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<PathBuf>,
    /// 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<String> {
-        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
--- 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!(
--- 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<Picker<Self>>) {
--- 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<String>,
-    pub env: HashMap<String, String>,
-    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;
--- 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
--- 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<Self>) {
-        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<PathBuf>,
        cx: &mut ViewContext<Self>,
    ) {
@ -469,7 +461,7 @@ impl TerminalPanel {
    fn add_terminal(
        &mut self,
        working_directory: Option<PathBuf>,
-        spawn_task: Option<SpawnTask>,
+        spawn_task: Option<SpawnInTerminal>,
        cx: &mut ViewContext<Self>,
    ) {
        let workspace = self.workspace.clone();
@ -561,7 +553,7 @@ impl TerminalPanel {
    fn replace_terminal(
        &self,
        working_directory: Option<PathBuf>,
-        spawn_task: SpawnTask,
+        spawn_task: SpawnInTerminal,
        terminal_item_index: usize,
        terminal_to_replace: View<TerminalView>,
        cx: &mut ViewContext<'_, Self>,