From 2140f4de842393c60223c80b9d236086ff9d6574 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:37:06 +0200 Subject: [PATCH 01/15] perf: avoid cloning AppConfig when constructing TaskRunner Moved the requires_sudo check above runner construction so we can move app_config into TaskRunner::new instead of cloning the whole task map. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/main.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/main.rs b/src/main.rs index cf566ae..0c77fff 100644 --- a/src/main.rs +++ b/src/main.rs @@ -82,12 +82,6 @@ async fn main() -> anyhow::Result<()> { let (event_tx, event_rx) = mpsc::unbounded_channel::(); let cancel = CancellationToken::new(); - // Set up runner - let runner = TaskRunner::new(app_config.clone(), cli.command.clone(), event_tx) - .with_config_dir(config_dir); - let force = cli.force; - let task_names_clone = task_names.clone(); - // Determine if we should use the TUI let use_tui = !cli.no_tui && std::io::stdout().is_terminal(); @@ -96,6 +90,12 @@ async fn main() -> anyhow::Result<()> { pre_authenticate_sudo(); } + // Set up runner (moves app_config) + let runner = TaskRunner::new(app_config, cli.command.clone(), event_tx) + .with_config_dir(config_dir); + let force = cli.force; + let task_names_clone = task_names.clone(); + if use_tui { // Spawn engine in background let engine_cancel = cancel.clone(); From e9213068714c5d40c4215475591ced0607916f24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:37:20 +0200 Subject: [PATCH 02/15] perf: avoid cloning selected task names in select_tasks Use std::mem::take to move each name out of the underlying vec instead of cloning; selections is sorted and unique so no slot is taken twice. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/main.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main.rs b/src/main.rs index 0c77fff..a22794c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -218,15 +218,18 @@ fn pre_authenticate_sudo() { } fn select_tasks(config: &config::types::AppConfig) -> anyhow::Result> { - let task_names: Vec = config.tasks.keys().cloned().collect(); + let mut task_names: Vec = config.tasks.keys().cloned().collect(); let selections = dialoguer::MultiSelect::new() .with_prompt("Select tasks to run") .items(&task_names) .interact()?; + // `std::mem::take` transfers ownership of each selected name out of the + // vec without cloning. `selections` is sorted and unique, so no slot is + // taken twice. Ok(selections .into_iter() - .map(|i| task_names[i].clone()) + .map(|i| std::mem::take(&mut task_names[i])) .collect()) } From e802f2718ff5cc66a7a593363516f464a08f9e31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:37:37 +0200 Subject: [PATCH 03/15] perf: iterate filtered_indices directly in task list render The TUI redraws at ~20Hz. Previously each frame allocated a HashSet from filtered_indices just to answer contains() for every task. Since filtered_indices is already sorted and unique, iterate it directly and look up each task by index. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/tui/widgets/task_list.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/tui/widgets/task_list.rs b/src/tui/widgets/task_list.rs index e6e8d53..801a469 100644 --- a/src/tui/widgets/task_list.rs +++ b/src/tui/widgets/task_list.rs @@ -18,14 +18,13 @@ pub fn render(f: &mut Frame, area: Rect, app: &App) { (area, None) }; - let filtered_set: std::collections::HashSet = - app.filtered_indices.iter().copied().collect(); - + // `filtered_indices` is already the exact set we want to render and is + // maintained in ascending order by `update_filter`, so we can iterate it + // directly without rebuilding a HashSet every frame. let items: Vec = app - .tasks + .filtered_indices .iter() - .enumerate() - .filter(|(i, _)| filtered_set.contains(i)) + .filter_map(|&i| app.tasks.get(i).map(|task| (i, task))) .map(|(i, task)| { let (symbol, style) = match &task.status { TaskStatus::Pending => (" ", Style::default().fg(Color::DarkGray)), From 9c33caeb2df25544943aa86cbce57bff40da4f44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:38:49 +0200 Subject: [PATCH 04/15] perf: pipe shell scripts directly instead of via temp file For bash/zsh, build_shell_command produces a script string that we already hold in memory; previously we wrote it to a temp file only to read it back and pipe to stdin. Now pipe the string directly. PowerShell still needs a temp file because we invoke it with -File, but that path is isolated. Split execute_script into stdin/file variants sharing a common wait_with_output helper. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/engine/commands/run.rs | 87 ++++++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 37 deletions(-) diff --git a/src/engine/commands/run.rs b/src/engine/commands/run.rs index 01b0d68..0a61717 100644 --- a/src/engine/commands/run.rs +++ b/src/engine/commands/run.rs @@ -59,68 +59,81 @@ async fn run_for_mode( let active_shell = args.shell.as_ref().unwrap_or(&ctx.default_shell); let script = shell::build_shell_command(commands, active_shell, &args.env)?; - // Write temp script - let script_path = shell::write_temp_script(&script, active_shell, &ctx.temp_dir)?; - ctx.log(format!( "Running {} command(s) with {}", commands.len(), active_shell )); - let result = execute_script(&script_path, active_shell, ctx).await; - - // Cleanup temp script - let _ = std::fs::remove_file(&script_path); - - result + match active_shell { + crate::config::types::Shell::Bash | crate::config::types::Shell::Zsh => { + // Pipe the in-memory script straight to the shell over stdin — + // no temp file round-trip needed. + execute_script_stdin(&script, active_shell, ctx).await + } + crate::config::types::Shell::PowerShell => { + // PowerShell needs -File for reliable execution on Windows, so + // we still materialize a script on disk for this path only. + let script_path = shell::write_temp_script(&script, active_shell, &ctx.temp_dir)?; + let result = execute_script_file(&script_path, active_shell, ctx).await; + let _ = std::fs::remove_file(&script_path); + result + } + } } -async fn execute_script( - script_path: &std::path::Path, +async fn execute_script_stdin( + script: &str, shell_type: &crate::config::types::Shell, ctx: &CommandContext, ) -> Result<()> { let shell_bin = shell::shell_binary(shell_type); let mut cmd = Command::new(shell_bin); + cmd.stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()); - let script_content = std::fs::read_to_string(script_path) - .map_err(|e| Error::ShellFailed(format!("Failed to read script: {e}")))?; + let mut child = cmd + .spawn() + .map_err(|e| Error::ShellFailed(format!("Failed to spawn {shell_bin}: {e}")))?; - match shell_type { - crate::config::types::Shell::Bash | crate::config::types::Shell::Zsh => { - // Pipe script via stdin to avoid path issues on Windows - // and newline-in-args issues with CreateProcess - cmd.stdin(std::process::Stdio::piped()); - } - crate::config::types::Shell::PowerShell => { - cmd.arg("-File").arg(script_path); - } + if let Some(mut stdin) = child.stdin.take() { + use tokio::io::AsyncWriteExt; + stdin + .write_all(script.as_bytes()) + .await + .map_err(|e| Error::ShellFailed(format!("Failed to write to stdin: {e}")))?; + // Drop stdin to signal EOF } + wait_with_output(child, ctx).await +} + +async fn execute_script_file( + script_path: &std::path::Path, + shell_type: &crate::config::types::Shell, + ctx: &CommandContext, +) -> Result<()> { + let shell_bin = shell::shell_binary(shell_type); + + let mut cmd = Command::new(shell_bin); + cmd.arg("-File").arg(script_path); + cmd.stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()); - let mut child = cmd + let child = cmd .spawn() .map_err(|e| Error::ShellFailed(format!("Failed to spawn {shell_bin}: {e}")))?; - // Write script to stdin for bash/zsh - if matches!( - shell_type, - crate::config::types::Shell::Bash | crate::config::types::Shell::Zsh - ) { - if let Some(mut stdin) = child.stdin.take() { - use tokio::io::AsyncWriteExt; - stdin - .write_all(script_content.as_bytes()) - .await - .map_err(|e| Error::ShellFailed(format!("Failed to write to stdin: {e}")))?; - // Drop stdin to signal EOF - } - } + wait_with_output(child, ctx).await +} +async fn wait_with_output( + mut child: tokio::process::Child, + ctx: &CommandContext, +) -> Result<()> { // Stream stdout and stderr concurrently, then wait for process let stdout_handle = child.stdout.take().map(|stdout| { let ctx_clone = ctx.clone(); From 1efa696b960a4cb588d990323b8e92f9f3a8548b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:39:53 +0200 Subject: [PATCH 05/15] perf: return iterator from RunArgs::all_command_strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every caller was asking len/is_empty/first — none actually needed the intermediate Vec<&str>. Returning impl Iterator lets callers drive what they need and removes the allocation on every description render and every requires_sudo scan. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/config/types.rs | 40 ++++++++++++++++++++------------------ src/config/validate.rs | 2 +- src/engine/commands/run.rs | 14 ++++++------- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/config/types.rs b/src/config/types.rs index 176edfa..49420bf 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -154,13 +154,13 @@ impl std::fmt::Display for CommandEntry { CommandEntry::Symlink(args) => write!(f, "symlink: {} -> {}", args.src, args.target), CommandEntry::Clone(args) => write!(f, "clone: {} -> {}", args.url, args.target), CommandEntry::Run(args) => { - let cmds = args.all_command_strings(); - if cmds.is_empty() { - write!(f, "run: (no commands)") - } else if cmds.len() == 1 { - write!(f, "run: {}", cmds[0]) - } else { - write!(f, "run: {} commands", cmds.len()) + let mut iter = args.all_command_strings(); + let first = iter.next(); + let second = iter.next(); + match (first, second) { + (None, _) => write!(f, "run: (no commands)"), + (Some(c), None) => write!(f, "run: {c}"), + (Some(_), Some(_)) => write!(f, "run: {} commands", 2 + iter.count()), } } CommandEntry::MachineSetup(args) => { @@ -230,14 +230,17 @@ pub struct RunArgs { } impl RunArgs { - /// Get all command strings regardless of mode (for display purposes). - pub fn all_command_strings(&self) -> Vec<&str> { - let mut cmds = Vec::new(); - cmds.extend(self.commands.as_slice().iter().map(|s| s.as_str())); - cmds.extend(self.install.as_slice().iter().map(|s| s.as_str())); - cmds.extend(self.update.as_slice().iter().map(|s| s.as_str())); - cmds.extend(self.uninstall.as_slice().iter().map(|s| s.as_str())); - cmds + /// Iterate all command strings regardless of mode (for display purposes). + /// Returns an iterator so callers that only need count/first/is_empty + /// don't force an intermediate Vec allocation. + pub fn all_command_strings(&self) -> impl Iterator { + self.commands + .as_slice() + .iter() + .chain(self.install.as_slice().iter()) + .chain(self.update.as_slice().iter()) + .chain(self.uninstall.as_slice().iter()) + .map(|s| s.as_str()) } /// Get commands for a specific mode. @@ -315,10 +318,9 @@ impl AppConfig { .filter(|(name, _)| task_names.iter().any(|t| t == *name)) .any(|(_, task)| { task.commands.iter().any(|cmd| match cmd { - CommandEntry::Run(args) => args - .all_command_strings() - .iter() - .any(|s| s.contains("sudo")), + CommandEntry::Run(args) => { + args.all_command_strings().any(|s| s.contains("sudo")) + } CommandEntry::Copy(args) => args.sudo, CommandEntry::Symlink(args) => args.sudo, _ => false, diff --git a/src/config/validate.rs b/src/config/validate.rs index 6324de6..167cd31 100644 --- a/src/config/validate.rs +++ b/src/config/validate.rs @@ -146,7 +146,7 @@ pub fn validate_config(config: &AppConfig, config_dir: &Path) -> Vec { - if args.all_command_strings().is_empty() { + if args.all_command_strings().next().is_none() { issues.push(ValidationIssue { task_name: name.clone(), message: format!("Run command has no commands defined: {cmd}"), diff --git a/src/engine/commands/run.rs b/src/engine/commands/run.rs index 0a61717..0fc0a2d 100644 --- a/src/engine/commands/run.rs +++ b/src/engine/commands/run.rs @@ -34,13 +34,13 @@ impl CommandExecutor for RunCommand { } fn description(&self) -> String { - let cmds = self.args.all_command_strings(); - if cmds.is_empty() { - "run: (no commands)".to_string() - } else if cmds.len() == 1 { - format!("run: {}", cmds[0]) - } else { - format!("run: {} commands", cmds.len()) + let mut iter = self.args.all_command_strings(); + let first = iter.next(); + let second = iter.next(); + match (first, second) { + (None, _) => "run: (no commands)".to_string(), + (Some(c), None) => format!("run: {c}"), + (Some(_), Some(_)) => format!("run: {} commands", 2 + iter.count()), } } } From 70ccd7d9680516b54a70dd34e5e95bbba4839e82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:40:10 +0200 Subject: [PATCH 06/15] perf: use HashSet for selected task lookup in requires_sudo The previous O(tasks * selected) scan becomes O(tasks + selected) by building a HashSet once up front. Not meaningful for small configs but free to fix. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/config/types.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/config/types.rs b/src/config/types.rs index 49420bf..e21b2bc 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -313,9 +313,11 @@ impl<'de> Deserialize<'de> for StringOrVec { impl AppConfig { /// Check if any commands in the selected tasks require sudo. pub fn requires_sudo(&self, task_names: &[String]) -> bool { + let selected: std::collections::HashSet<&str> = + task_names.iter().map(String::as_str).collect(); self.tasks .iter() - .filter(|(name, _)| task_names.iter().any(|t| t == *name)) + .filter(|(name, _)| selected.contains(name.as_str())) .any(|(_, task)| { task.commands.iter().any(|cmd| match cmd { CommandEntry::Run(args) => { From 575107771d28eb0a8ca81410cfe3218ba2d3474e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:41:19 +0200 Subject: [PATCH 07/15] perf: borrow task_names fast path in topological_sort When no task has a depends_on entry, the sort is a no-op but the old return type forced a Vec::to_vec() clone. Switch to Cow<'a, [String]> so the common no-deps path hands back a borrow. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/engine/runner.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/engine/runner.rs b/src/engine/runner.rs index 082c9e5..e4f9d1d 100644 --- a/src/engine/runner.rs +++ b/src/engine/runner.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::collections::{HashMap, HashSet, VecDeque}; use std::path::{Path, PathBuf}; use tokio::sync::mpsc; @@ -61,8 +62,10 @@ impl TaskRunner { /// Run specific tasks by name. pub async fn run_tasks(&self, task_names: &[String], force: bool) -> Result<()> { - // Resolve dependency order via topological sort + // Resolve dependency order via topological sort. When no task has + // dependencies, this borrows `task_names` instead of cloning it. let ordered = self.topological_sort(task_names)?; + let ordered: &[String] = &ordered; let temp_dir = expand_path(&self.config.temp_dir, None); let mut history = History::load(&temp_dir).unwrap_or_default(); @@ -73,7 +76,7 @@ impl TaskRunner { if self.config.parallel { // Parallel execution with dependency layers - let layers = self.dependency_layers(&ordered); + let layers = self.dependency_layers(ordered); for layer in layers { let mut handles = Vec::new(); @@ -122,7 +125,7 @@ impl TaskRunner { } } else { // Sequential execution - for name in &ordered { + for name in ordered { let task_config = &self.config.tasks[name]; if let Some(reason) = self.should_skip(task_config, name, force, &history) { @@ -211,8 +214,8 @@ impl TaskRunner { /// Topological sort of tasks respecting depends_on. /// Returns tasks in dependency order. Includes transitive dependencies - /// of the requested tasks. - fn topological_sort(&self, requested: &[String]) -> Result> { + /// of the requested tasks. Borrows the input when no sorting is needed. + fn topological_sort<'a>(&self, requested: &'a [String]) -> Result> { let all_tasks = &self.config.tasks; // If no task has dependencies, preserve original order @@ -221,7 +224,7 @@ impl TaskRunner { .filter_map(|n| all_tasks.get(n)) .any(|t| !t.depends_on.is_empty()); if !has_deps { - return Ok(requested.to_vec()); + return Ok(Cow::Borrowed(requested)); } // Collect all needed tasks (requested + transitive deps) @@ -293,7 +296,7 @@ impl TaskRunner { return Err(Error::CyclicDependency(remaining.join(", "))); } - Ok(sorted) + Ok(Cow::Owned(sorted)) } /// Group tasks into dependency layers for parallel execution. From 062fb82e24a2df1169fbd0bd32ce652b4257e38e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:42:02 +0200 Subject: [PATCH 08/15] perf: tighten to_string_lossy usage Two small wins: - setup.rs: keep the resolved PathBuf around so canonicalize can use it directly instead of re-parsing a stringified form; URLs skip the allocation entirely via Cow::Borrowed. - shell.rs: use Cow::into_owned instead of to_string_lossy().to_string(); same cost, clearer intent. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/engine/commands/setup.rs | 30 +++++++++++++++++------------- src/utils/shell.rs | 2 +- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/engine/commands/setup.rs b/src/engine/commands/setup.rs index f11a98c..d6a3272 100644 --- a/src/engine/commands/setup.rs +++ b/src/engine/commands/setup.rs @@ -43,27 +43,31 @@ impl CommandExecutor for SetupCommand { async fn run_sub_config(args: &MachineSetupArgs, ctx: &CommandContext) -> Result<()> { let is_url = args.config.starts_with("http://") || args.config.starts_with("https://"); - let config_str = if is_url { - args.config.clone() - } else { - let config_path = expand_path(&args.config, Some(&ctx.config_dir)); - config_path.to_string_lossy().to_string() - }; + // For URLs we pass the string through to load_config. For local paths + // we keep the resolved PathBuf so canonicalize below can reuse it + // without re-parsing a string representation. + let (config_str, config_path): (std::borrow::Cow<'_, str>, Option) = + if is_url { + (std::borrow::Cow::Borrowed(&args.config), None) + } else { + let path = expand_path(&args.config, Some(&ctx.config_dir)); + let s = path.to_string_lossy().into_owned(); + (std::borrow::Cow::Owned(s), Some(path)) + }; ctx.log(format!("Loading sub-config: {config_str}")); let config = crate::config::load_config(&config_str)?; - // Resolve the sub-config's directory for its own relative paths - // URLs fall back to parent's config_dir - let sub_config_dir = if is_url { - ctx.config_dir.clone() - } else { - std::path::Path::new(&config_str) + // Resolve the sub-config's directory for its own relative paths. + // URLs fall back to the parent's config_dir. + let sub_config_dir = match config_path { + Some(path) => path .canonicalize() .ok() .and_then(|p| p.parent().map(|p| p.to_path_buf())) - .unwrap_or_else(|| ctx.config_dir.clone()) + .unwrap_or_else(|| ctx.config_dir.clone()), + None => ctx.config_dir.clone(), }; let runner = diff --git a/src/utils/shell.rs b/src/utils/shell.rs index cd05248..67daa00 100644 --- a/src/utils/shell.rs +++ b/src/utils/shell.rs @@ -91,7 +91,7 @@ pub fn build_shell_command( let val = if value.starts_with('~') { crate::utils::path::expand_path(value, None) .to_string_lossy() - .to_string() + .into_owned() } else { value.clone() }; From b5e196a6424b52655d45621074a6691c4353840c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:42:57 +0200 Subject: [PATCH 09/15] refactor: extract config::resolve_config_dir helper The canonicalize-parent-or-cwd idiom was duplicated in main.rs (twice) and setup.rs. Centralize it in config::resolve_config_dir, parameterized on a fallback path so setup.rs can pass the parent config_dir while main.rs passes cwd. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/config/mod.rs | 17 ++++++++++++++++- src/engine/commands/setup.rs | 34 ++++++++++++---------------------- src/main.rs | 14 ++++---------- 3 files changed, 32 insertions(+), 33 deletions(-) diff --git a/src/config/mod.rs b/src/config/mod.rs index a3f4c11..d849326 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -3,11 +3,26 @@ pub mod os; pub mod types; pub mod validate; -use std::path::Path; +use std::path::{Path, PathBuf}; use crate::error::{Error, Result}; use types::AppConfig; +/// Return the directory that relative paths inside a config file should be +/// resolved against. For local configs this is the config file's parent +/// directory (after canonicalizing); for URLs and unresolvable paths we fall +/// back to `fallback`. +pub fn resolve_config_dir(path_or_url: &str, fallback: &Path) -> PathBuf { + if is_url(path_or_url) { + return fallback.to_path_buf(); + } + Path::new(path_or_url) + .canonicalize() + .ok() + .and_then(|p| p.parent().map(Path::to_path_buf)) + .unwrap_or_else(|| fallback.to_path_buf()) +} + /// Load a config from a local path or URL, auto-detecting format from extension. /// If the path has no extension, tries `.yaml`, `.yml`, then `.json`. /// diff --git a/src/engine/commands/setup.rs b/src/engine/commands/setup.rs index d6a3272..6d96740 100644 --- a/src/engine/commands/setup.rs +++ b/src/engine/commands/setup.rs @@ -41,34 +41,24 @@ impl CommandExecutor for SetupCommand { } async fn run_sub_config(args: &MachineSetupArgs, ctx: &CommandContext) -> Result<()> { - let is_url = args.config.starts_with("http://") || args.config.starts_with("https://"); + let is_url = crate::config::is_url(&args.config); - // For URLs we pass the string through to load_config. For local paths - // we keep the resolved PathBuf so canonicalize below can reuse it - // without re-parsing a string representation. - let (config_str, config_path): (std::borrow::Cow<'_, str>, Option) = - if is_url { - (std::borrow::Cow::Borrowed(&args.config), None) - } else { - let path = expand_path(&args.config, Some(&ctx.config_dir)); - let s = path.to_string_lossy().into_owned(); - (std::borrow::Cow::Owned(s), Some(path)) - }; + // For URLs we pass the string through to load_config; for local paths + // we resolve via expand_path against the parent's config_dir. + let config_str: std::borrow::Cow<'_, str> = if is_url { + std::borrow::Cow::Borrowed(&args.config) + } else { + let path = expand_path(&args.config, Some(&ctx.config_dir)); + std::borrow::Cow::Owned(path.to_string_lossy().into_owned()) + }; ctx.log(format!("Loading sub-config: {config_str}")); let config = crate::config::load_config(&config_str)?; - // Resolve the sub-config's directory for its own relative paths. - // URLs fall back to the parent's config_dir. - let sub_config_dir = match config_path { - Some(path) => path - .canonicalize() - .ok() - .and_then(|p| p.parent().map(|p| p.to_path_buf())) - .unwrap_or_else(|| ctx.config_dir.clone()), - None => ctx.config_dir.clone(), - }; + // Resolve the sub-config's directory for its own relative paths. URLs + // and unresolvable paths fall back to the parent's config_dir. + let sub_config_dir = crate::config::resolve_config_dir(&config_str, &ctx.config_dir); let runner = crate::engine::runner::TaskRunner::new(config, ctx.mode.clone(), ctx.event_tx.clone()) diff --git a/src/main.rs b/src/main.rs index a22794c..67cdec0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -31,11 +31,8 @@ async fn main() -> anyhow::Result<()> { // Handle validate command if cli.command == Command::Validate { - let config_dir = std::path::Path::new(&cli.config) - .canonicalize() - .ok() - .and_then(|p| p.parent().map(|p| p.to_path_buf())) - .unwrap_or_else(|| std::env::current_dir().unwrap_or_default()); + let cwd = std::env::current_dir().unwrap_or_default(); + let config_dir = config::resolve_config_dir(&cli.config, &cwd); let issues = config::validate::validate_config(&app_config, &config_dir); if issues.is_empty() { @@ -72,11 +69,8 @@ async fn main() -> anyhow::Result<()> { } // Resolve config directory for relative paths (URLs fall back to cwd) - let config_dir = std::path::Path::new(&cli.config) - .canonicalize() - .ok() - .and_then(|p| p.parent().map(|p| p.to_path_buf())) - .unwrap_or_else(|| std::env::current_dir().unwrap_or_default()); + let cwd = std::env::current_dir().unwrap_or_default(); + let config_dir = config::resolve_config_dir(&cli.config, &cwd); // Create event channel and cancellation token let (event_tx, event_rx) = mpsc::unbounded_channel::(); From 575f9f6300a9c5d5eb88513927c3906a3e251595 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:44:41 +0200 Subject: [PATCH 10/15] refactor: extract process::stream_and_wait for child output plumbing run.rs and clone.rs both spawned two tokio tasks to read child stdout and stderr line-by-line. Extract the shared machinery into a utility, parameterized on whether stderr should be labelled (shell commands) or passed through (git progress). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/engine/commands/clone.rs | 37 +++--------------------- src/engine/commands/run.rs | 39 ++----------------------- src/utils/mod.rs | 1 + src/utils/process.rs | 56 ++++++++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+), 69 deletions(-) create mode 100644 src/utils/process.rs diff --git a/src/engine/commands/clone.rs b/src/engine/commands/clone.rs index bc42ec6..ce17ab0 100644 --- a/src/engine/commands/clone.rs +++ b/src/engine/commands/clone.rs @@ -1,11 +1,11 @@ use async_trait::async_trait; -use tokio::io::{AsyncBufReadExt, BufReader}; use tokio::process::Command; use crate::config::types::CloneArgs; use crate::engine::context::CommandContext; use crate::error::{Error, Result}; use crate::utils::path::expand_path; +use crate::utils::process; use super::CommandExecutor; @@ -93,44 +93,15 @@ async fn run_git_command( cmd.current_dir(dir); } - let mut child = cmd + let child = cmd .spawn() .map_err(|e| Error::GitFailed(format!("Failed to spawn git: {e}")))?; - let stdout_handle = child.stdout.take().map(|stdout| { - let ctx_clone = ctx.clone(); - tokio::spawn(async move { - let reader = BufReader::new(stdout); - let mut lines = reader.lines(); - while let Ok(Some(line)) = lines.next_line().await { - ctx_clone.log(line); - } - }) - }); - - let stderr_handle = child.stderr.take().map(|stderr| { - let ctx_clone = ctx.clone(); - tokio::spawn(async move { - let reader = BufReader::new(stderr); - let mut lines = reader.lines(); - while let Ok(Some(line)) = lines.next_line().await { - ctx_clone.log(line); - } - }) - }); - - let status = child - .wait() + // git emits progress on stderr — don't tag those lines as errors. + let status = process::stream_and_wait(child, ctx, process::StderrLabel::Plain) .await .map_err(|e| Error::GitFailed(format!("Failed to wait for git: {e}")))?; - if let Some(h) = stdout_handle { - let _ = h.await; - } - if let Some(h) = stderr_handle { - let _ = h.await; - } - if !status.success() { return Err(Error::GitFailed(format!( "git {} exited with code {}", diff --git a/src/engine/commands/run.rs b/src/engine/commands/run.rs index 0fc0a2d..ce4331b 100644 --- a/src/engine/commands/run.rs +++ b/src/engine/commands/run.rs @@ -1,11 +1,10 @@ use async_trait::async_trait; -use tokio::io::{AsyncBufReadExt, BufReader}; use tokio::process::Command; use crate::config::types::RunArgs; use crate::engine::context::CommandContext; use crate::error::{Error, Result}; -use crate::utils::shell; +use crate::utils::{process, shell}; use super::CommandExecutor; @@ -131,45 +130,13 @@ async fn execute_script_file( } async fn wait_with_output( - mut child: tokio::process::Child, + child: tokio::process::Child, ctx: &CommandContext, ) -> Result<()> { - // Stream stdout and stderr concurrently, then wait for process - let stdout_handle = child.stdout.take().map(|stdout| { - let ctx_clone = ctx.clone(); - tokio::spawn(async move { - let reader = BufReader::new(stdout); - let mut lines = reader.lines(); - while let Ok(Some(line)) = lines.next_line().await { - ctx_clone.log(line); - } - }) - }); - - let stderr_handle = child.stderr.take().map(|stderr| { - let ctx_clone = ctx.clone(); - tokio::spawn(async move { - let reader = BufReader::new(stderr); - let mut lines = reader.lines(); - while let Ok(Some(line)) = lines.next_line().await { - ctx_clone.log(format!("[stderr] {line}")); - } - }) - }); - - let status = child - .wait() + let status = process::stream_and_wait(child, ctx, process::StderrLabel::Prefixed) .await .map_err(|e| Error::ShellFailed(format!("Failed to wait for shell: {e}")))?; - // Wait for output streams to finish flushing - if let Some(h) = stdout_handle { - let _ = h.await; - } - if let Some(h) = stderr_handle { - let _ = h.await; - } - if !status.success() { return Err(Error::ShellFailed(format!( "Shell exited with code {}", diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 46ccea1..51a13b1 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -1,3 +1,4 @@ pub mod path; +pub mod process; pub mod shell; pub mod sudo; diff --git a/src/utils/process.rs b/src/utils/process.rs new file mode 100644 index 0000000..7f26b45 --- /dev/null +++ b/src/utils/process.rs @@ -0,0 +1,56 @@ +use tokio::io::{AsyncBufReadExt, BufReader}; +use tokio::process::Child; + +use crate::engine::context::CommandContext; + +/// Whether to tag stderr lines when forwarding them to the log. +#[derive(Copy, Clone)] +pub enum StderrLabel { + /// Prefix stderr lines with `[stderr]` so the UI can style them. + Prefixed, + /// Forward stderr unchanged (useful for tools like `git` that emit + /// progress on stderr). + Plain, +} + +/// Stream a child process's stdout and stderr to the context's event +/// channel, wait for the child to exit, and return its exit status. +pub async fn stream_and_wait( + mut child: Child, + ctx: &CommandContext, + stderr_label: StderrLabel, +) -> std::io::Result { + let stdout_handle = child.stdout.take().map(|stdout| { + let ctx = ctx.clone(); + tokio::spawn(async move { + let mut lines = BufReader::new(stdout).lines(); + while let Ok(Some(line)) = lines.next_line().await { + ctx.log(line); + } + }) + }); + + let stderr_handle = child.stderr.take().map(|stderr| { + let ctx = ctx.clone(); + tokio::spawn(async move { + let mut lines = BufReader::new(stderr).lines(); + while let Ok(Some(line)) = lines.next_line().await { + match stderr_label { + StderrLabel::Prefixed => ctx.log(format!("[stderr] {line}")), + StderrLabel::Plain => ctx.log(line), + } + } + }) + }); + + let status = child.wait().await?; + + if let Some(h) = stdout_handle { + let _ = h.await; + } + if let Some(h) = stderr_handle { + let _ = h.await; + } + + Ok(status) +} From ef8a31053bb9bcb1cb177c8fe9db6c8c744f2cd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:46:13 +0200 Subject: [PATCH 11/15] refactor: extract walk_relative helper for recursive dir ops copy.rs and symlink.rs both duplicated a WalkDir + strip_prefix + should_ignore + target.join loop across their install and uninstall paths. Centralize that pattern in utils::path::walk_relative and let callers focus on the per-entry action. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/engine/commands/copy.rs | 38 ++++++++++------------------------ src/engine/commands/symlink.rs | 31 ++++++++------------------- src/utils/path.rs | 31 +++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 49 deletions(-) diff --git a/src/engine/commands/copy.rs b/src/engine/commands/copy.rs index 17d6ef7..bb08b58 100644 --- a/src/engine/commands/copy.rs +++ b/src/engine/commands/copy.rs @@ -1,11 +1,10 @@ use async_trait::async_trait; use std::path::Path; -use walkdir::WalkDir; use crate::config::types::CopyArgs; use crate::engine::context::CommandContext; use crate::error::{Error, Result}; -use crate::utils::path::{expand_path, should_ignore}; +use crate::utils::path::{expand_path, walk_relative}; use crate::utils::sudo; use super::CommandExecutor; @@ -74,19 +73,13 @@ impl CommandExecutor for CopyCommand { remove_file(&dest, use_sudo)?; } } else { - for entry in WalkDir::new(&src).into_iter().filter_map(|e| e.ok()) { - if entry.file_type().is_file() { - let relative = entry.path().strip_prefix(&src).unwrap(); - if should_ignore(relative, &self.args.ignore) { - continue; - } - let dest = target.join(relative); - if dest.exists() { - ctx.log(format!("Removing: {}", dest.display())); - remove_file(&dest, use_sudo)?; - } + walk_relative(&src, &target, &self.args.ignore, |entry, dest| { + if entry.file_type().is_file() && dest.exists() { + ctx.log(format!("Removing: {}", dest.display())); + remove_file(dest, use_sudo)?; } - } + Ok(()) + })?; } Ok(()) @@ -156,20 +149,11 @@ fn copy_directory( use_sudo: bool, ctx: &CommandContext, ) -> Result<()> { - for entry in WalkDir::new(src).into_iter().filter_map(|e| e.ok()) { - let relative = entry.path().strip_prefix(src).unwrap(); - - if should_ignore(relative, ignore) { - continue; - } - - let dest = target.join(relative); - + walk_relative(src, target, ignore, |entry, dest| { if entry.file_type().is_dir() { - mkdir(&dest, use_sudo)?; + mkdir(dest, use_sudo) } else { - copy_file(entry.path(), &dest, use_sudo, ctx)?; + copy_file(entry.path(), dest, use_sudo, ctx) } - } - Ok(()) + }) } diff --git a/src/engine/commands/symlink.rs b/src/engine/commands/symlink.rs index a037e9a..0b6f32e 100644 --- a/src/engine/commands/symlink.rs +++ b/src/engine/commands/symlink.rs @@ -1,10 +1,9 @@ use async_trait::async_trait; -use walkdir::WalkDir; use crate::config::types::SymlinkArgs; use crate::engine::context::CommandContext; use crate::error::{Error, Result}; -use crate::utils::path::{expand_path, should_ignore}; +use crate::utils::path::{expand_path, walk_relative}; use crate::utils::sudo; use super::CommandExecutor; @@ -46,21 +45,13 @@ impl CommandExecutor for SymlinkCommand { create_symlink(&src, &dest, self.args.force, use_sudo, ctx)?; } else { mkdir(&target, use_sudo)?; - for entry in WalkDir::new(&src).into_iter().filter_map(|e| e.ok()) { - let relative = entry.path().strip_prefix(&src).unwrap(); - - if should_ignore(relative, &self.args.ignore) { - continue; - } - - let dest = target.join(relative); - + walk_relative(&src, &target, &self.args.ignore, |entry, dest| { if entry.file_type().is_dir() { - mkdir(&dest, use_sudo)?; + mkdir(dest, use_sudo) } else { - create_symlink(entry.path(), &dest, self.args.force, use_sudo, ctx)?; + create_symlink(entry.path(), dest, self.args.force, use_sudo, ctx) } - } + })?; } Ok(()) @@ -83,16 +74,12 @@ impl CommandExecutor for SymlinkCommand { }; remove_symlink(&dest, use_sudo, ctx)?; } else { - for entry in WalkDir::new(&src).into_iter().filter_map(|e| e.ok()) { + walk_relative(&src, &target, &self.args.ignore, |entry, dest| { if entry.file_type().is_file() { - let relative = entry.path().strip_prefix(&src).unwrap(); - if should_ignore(relative, &self.args.ignore) { - continue; - } - let dest = target.join(relative); - remove_symlink(&dest, use_sudo, ctx)?; + remove_symlink(dest, use_sudo, ctx)?; } - } + Ok(()) + })?; } Ok(()) diff --git a/src/utils/path.rs b/src/utils/path.rs index 3afdb28..6441e80 100644 --- a/src/utils/path.rs +++ b/src/utils/path.rs @@ -1,5 +1,9 @@ use std::path::{Path, PathBuf}; +use walkdir::{DirEntry, WalkDir}; + +use crate::error::Result; + /// Expand `~` to the user's home directory, `$VAR` to environment variables, /// and resolve relative paths against the base directory. pub fn expand_path(path: &str, base_dir: Option<&Path>) -> PathBuf { @@ -76,6 +80,33 @@ fn expand_env_vars(input: &str) -> String { result } +/// Walk `src` and invoke `f` for each entry that isn't filtered out by +/// `ignore_list`. The closure receives the raw `DirEntry` plus the +/// precomputed destination path (`target` joined with the entry's +/// `src`-relative suffix). +/// +/// `strip_prefix` is guaranteed to succeed because every WalkDir entry is +/// rooted at `src`. +pub fn walk_relative( + src: &Path, + target: &Path, + ignore_list: &[String], + mut f: F, +) -> Result<()> +where + F: FnMut(&DirEntry, &Path) -> Result<()>, +{ + for entry in WalkDir::new(src).into_iter().filter_map(|e| e.ok()) { + let relative = entry.path().strip_prefix(src).unwrap_or(entry.path()); + if should_ignore(relative, ignore_list) { + continue; + } + let dest = target.join(relative); + f(&entry, &dest)?; + } + Ok(()) +} + /// Check if a path should be ignored based on the ignore list. pub fn should_ignore(path: &Path, ignore_list: &[String]) -> bool { let path_str = path.to_string_lossy(); From 75347c89f51d666805f4fe870ee598c77e3026dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:47:24 +0200 Subject: [PATCH 12/15] refactor: unify CommandEntry Display and executor description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Display impl on CommandEntry and each executor's description() expressed the same idea twice — and in subtly different ways (sudo prefix shown by executors but not Display). Impl Display on each args struct as the single source of truth, have CommandEntry::Display delegate, and have each executor's description() return args.to_string(). The `list` command output now shows the (sudo) marker on copy/symlink entries, which is incidental but actually more informative. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/config/types.rs | 66 +++++++++++++++++++++++----------- src/engine/commands/clone.rs | 2 +- src/engine/commands/copy.rs | 7 +--- src/engine/commands/run.rs | 9 +---- src/engine/commands/setup.rs | 6 +--- src/engine/commands/symlink.rs | 7 +--- 6 files changed, 51 insertions(+), 46 deletions(-) diff --git a/src/config/types.rs b/src/config/types.rs index e21b2bc..2e5c6b3 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -150,27 +150,53 @@ impl<'de> Deserialize<'de> for CommandEntry { impl std::fmt::Display for CommandEntry { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - CommandEntry::Copy(args) => write!(f, "copy: {} -> {}", args.src, args.target), - CommandEntry::Symlink(args) => write!(f, "symlink: {} -> {}", args.src, args.target), - CommandEntry::Clone(args) => write!(f, "clone: {} -> {}", args.url, args.target), - CommandEntry::Run(args) => { - let mut iter = args.all_command_strings(); - let first = iter.next(); - let second = iter.next(); - match (first, second) { - (None, _) => write!(f, "run: (no commands)"), - (Some(c), None) => write!(f, "run: {c}"), - (Some(_), Some(_)) => write!(f, "run: {} commands", 2 + iter.count()), - } - } - CommandEntry::MachineSetup(args) => { - write!(f, "machine_setup: {}", args.config)?; - if let Some(task) = &args.task { - write!(f, " (task: {task})")?; - } - Ok(()) - } + CommandEntry::Copy(args) => args.fmt(f), + CommandEntry::Symlink(args) => args.fmt(f), + CommandEntry::Clone(args) => args.fmt(f), + CommandEntry::Run(args) => args.fmt(f), + CommandEntry::MachineSetup(args) => args.fmt(f), + } + } +} + +impl std::fmt::Display for CopyArgs { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let prefix = if self.sudo { "copy (sudo)" } else { "copy" }; + write!(f, "{prefix}: {} -> {}", self.src, self.target) + } +} + +impl std::fmt::Display for SymlinkArgs { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let prefix = if self.sudo { "symlink (sudo)" } else { "symlink" }; + write!(f, "{prefix}: {} -> {}", self.src, self.target) + } +} + +impl std::fmt::Display for CloneArgs { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "clone: {} -> {}", self.url, self.target) + } +} + +impl std::fmt::Display for RunArgs { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut iter = self.all_command_strings(); + match (iter.next(), iter.next()) { + (None, _) => write!(f, "run: (no commands)"), + (Some(c), None) => write!(f, "run: {c}"), + (Some(_), Some(_)) => write!(f, "run: {} commands", 2 + iter.count()), + } + } +} + +impl std::fmt::Display for MachineSetupArgs { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "machine_setup: {}", self.config)?; + if let Some(task) = &self.task { + write!(f, " (task: {task})")?; } + Ok(()) } } diff --git a/src/engine/commands/clone.rs b/src/engine/commands/clone.rs index ce17ab0..f15aeeb 100644 --- a/src/engine/commands/clone.rs +++ b/src/engine/commands/clone.rs @@ -75,7 +75,7 @@ impl CommandExecutor for CloneCommand { } fn description(&self) -> String { - format!("clone: {} -> {}", self.args.url, self.args.target) + self.args.to_string() } } diff --git a/src/engine/commands/copy.rs b/src/engine/commands/copy.rs index bb08b58..1178487 100644 --- a/src/engine/commands/copy.rs +++ b/src/engine/commands/copy.rs @@ -86,12 +86,7 @@ impl CommandExecutor for CopyCommand { } fn description(&self) -> String { - let prefix = if self.args.sudo { - "copy (sudo)" - } else { - "copy" - }; - format!("{prefix}: {} -> {}", self.args.src, self.args.target) + self.args.to_string() } } diff --git a/src/engine/commands/run.rs b/src/engine/commands/run.rs index ce4331b..e8ddfa4 100644 --- a/src/engine/commands/run.rs +++ b/src/engine/commands/run.rs @@ -33,14 +33,7 @@ impl CommandExecutor for RunCommand { } fn description(&self) -> String { - let mut iter = self.args.all_command_strings(); - let first = iter.next(); - let second = iter.next(); - match (first, second) { - (None, _) => "run: (no commands)".to_string(), - (Some(c), None) => format!("run: {c}"), - (Some(_), Some(_)) => format!("run: {} commands", 2 + iter.count()), - } + self.args.to_string() } } diff --git a/src/engine/commands/setup.rs b/src/engine/commands/setup.rs index 6d96740..9b7c2dc 100644 --- a/src/engine/commands/setup.rs +++ b/src/engine/commands/setup.rs @@ -32,11 +32,7 @@ impl CommandExecutor for SetupCommand { } fn description(&self) -> String { - let mut desc = format!("machine_setup: {}", self.args.config); - if let Some(task) = &self.args.task { - desc.push_str(&format!(" (task: {task})")); - } - desc + self.args.to_string() } } diff --git a/src/engine/commands/symlink.rs b/src/engine/commands/symlink.rs index 0b6f32e..04202a1 100644 --- a/src/engine/commands/symlink.rs +++ b/src/engine/commands/symlink.rs @@ -86,12 +86,7 @@ impl CommandExecutor for SymlinkCommand { } fn description(&self) -> String { - let prefix = if self.args.sudo { - "symlink (sudo)" - } else { - "symlink" - }; - format!("{prefix}: {} -> {}", self.args.src, self.args.target) + self.args.to_string() } } From b480cd0ce73afcd843eae630510901fde3021547 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:47:48 +0200 Subject: [PATCH 13/15] refactor: simplify validate_env_key with let-else Use let-else to handle the empty-string case, eliminating both the explicit is_empty check and the chars.next().unwrap() that followed it. Same behavior, fewer footguns. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/utils/shell.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/utils/shell.rs b/src/utils/shell.rs index 67daa00..e8f83ce 100644 --- a/src/utils/shell.rs +++ b/src/utils/shell.rs @@ -34,15 +34,12 @@ pub fn shell_profile(shell: &Shell) -> Option { /// Check that an environment variable key is a valid identifier. pub fn validate_env_key(key: &str) -> bool { - if key.is_empty() { - return false; - } let mut chars = key.chars(); - let first = chars.next().unwrap(); - if !first.is_ascii_alphabetic() && first != '_' { + let Some(first) = chars.next() else { return false; - } - chars.all(|c| c.is_ascii_alphanumeric() || c == '_') + }; + (first.is_ascii_alphabetic() || first == '_') + && chars.all(|c| c.is_ascii_alphanumeric() || c == '_') } /// Escape a value for use inside a bash/zsh single-quoted string. From 484dd23d8b63c4ff89334836d7389551404b0cc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:48:28 +0200 Subject: [PATCH 14/15] refactor: take CommandEntry by value in create_executor Previously create_executor took &CommandEntry and each arm did args.clone(). Move the clone up to the single caller (iter().cloned()) so ownership transfer is explicit and the match arms can move args directly into the executor. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/engine/commands/mod.rs | 16 +++++++++------- src/engine/runner.rs | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/engine/commands/mod.rs b/src/engine/commands/mod.rs index b76330c..f9c95eb 100644 --- a/src/engine/commands/mod.rs +++ b/src/engine/commands/mod.rs @@ -22,13 +22,15 @@ pub trait CommandExecutor: Send + Sync { fn description(&self) -> String; } -/// Create a command executor from a config entry. -pub fn create_executor(entry: &CommandEntry) -> Box { +/// Create a command executor from a config entry. Takes ownership so the +/// args struct moves directly into the executor without an intermediate +/// clone inside each match arm. +pub fn create_executor(entry: CommandEntry) -> Box { match entry { - CommandEntry::Copy(args) => Box::new(copy::CopyCommand::new(args.clone())), - CommandEntry::Symlink(args) => Box::new(symlink::SymlinkCommand::new(args.clone())), - CommandEntry::Clone(args) => Box::new(clone::CloneCommand::new(args.clone())), - CommandEntry::Run(args) => Box::new(run::RunCommand::new(args.clone())), - CommandEntry::MachineSetup(args) => Box::new(setup::SetupCommand::new(args.clone())), + CommandEntry::Copy(args) => Box::new(copy::CopyCommand::new(args)), + CommandEntry::Symlink(args) => Box::new(symlink::SymlinkCommand::new(args)), + CommandEntry::Clone(args) => Box::new(clone::CloneCommand::new(args)), + CommandEntry::Run(args) => Box::new(run::RunCommand::new(args)), + CommandEntry::MachineSetup(args) => Box::new(setup::SetupCommand::new(args)), } } diff --git a/src/engine/runner.rs b/src/engine/runner.rs index e4f9d1d..b30b4cd 100644 --- a/src/engine/runner.rs +++ b/src/engine/runner.rs @@ -409,7 +409,7 @@ async fn run_task( }); let executors: Vec> = - task.commands.iter().map(create_executor).collect(); + task.commands.iter().cloned().map(create_executor).collect(); if task.parallel { // Run commands in parallel From 58ba40396d6665a4cd8002bdfc477a4d3c8ff8ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20Pr=C3=BC=C3=9Fe?= Date: Fri, 24 Apr 2026 19:49:31 +0200 Subject: [PATCH 15/15] style: apply rustfmt Co-Authored-By: Claude Opus 4.7 (1M context) --- src/config/types.rs | 6 +++++- src/engine/commands/run.rs | 5 +---- src/main.rs | 4 ++-- src/utils/path.rs | 7 +------ 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/config/types.rs b/src/config/types.rs index 2e5c6b3..18222dd 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -168,7 +168,11 @@ impl std::fmt::Display for CopyArgs { impl std::fmt::Display for SymlinkArgs { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let prefix = if self.sudo { "symlink (sudo)" } else { "symlink" }; + let prefix = if self.sudo { + "symlink (sudo)" + } else { + "symlink" + }; write!(f, "{prefix}: {} -> {}", self.src, self.target) } } diff --git a/src/engine/commands/run.rs b/src/engine/commands/run.rs index e8ddfa4..5d6c034 100644 --- a/src/engine/commands/run.rs +++ b/src/engine/commands/run.rs @@ -122,10 +122,7 @@ async fn execute_script_file( wait_with_output(child, ctx).await } -async fn wait_with_output( - child: tokio::process::Child, - ctx: &CommandContext, -) -> Result<()> { +async fn wait_with_output(child: tokio::process::Child, ctx: &CommandContext) -> Result<()> { let status = process::stream_and_wait(child, ctx, process::StderrLabel::Prefixed) .await .map_err(|e| Error::ShellFailed(format!("Failed to wait for shell: {e}")))?; diff --git a/src/main.rs b/src/main.rs index 67cdec0..832bf45 100644 --- a/src/main.rs +++ b/src/main.rs @@ -85,8 +85,8 @@ async fn main() -> anyhow::Result<()> { } // Set up runner (moves app_config) - let runner = TaskRunner::new(app_config, cli.command.clone(), event_tx) - .with_config_dir(config_dir); + let runner = + TaskRunner::new(app_config, cli.command.clone(), event_tx).with_config_dir(config_dir); let force = cli.force; let task_names_clone = task_names.clone(); diff --git a/src/utils/path.rs b/src/utils/path.rs index 6441e80..7023b23 100644 --- a/src/utils/path.rs +++ b/src/utils/path.rs @@ -87,12 +87,7 @@ fn expand_env_vars(input: &str) -> String { /// /// `strip_prefix` is guaranteed to succeed because every WalkDir entry is /// rooted at `src`. -pub fn walk_relative( - src: &Path, - target: &Path, - ignore_list: &[String], - mut f: F, -) -> Result<()> +pub fn walk_relative(src: &Path, target: &Path, ignore_list: &[String], mut f: F) -> Result<()> where F: FnMut(&DirEntry, &Path) -> Result<()>, {