From 66ada79805a93672e4a4373e257c0a0be9dae6a8 Mon Sep 17 00:00:00 2001 From: gold-silver-copper <157451540+gold-silver-copper@users.noreply.github.com> Date: Sun, 25 Oct 2026 04:34:52 -0700 Subject: [PATCH 1/4] fix!: run commands as argv with no shell; filters name whole paths Commands ran through $SHELL -c (cmd /c on Windows), so quotes, $(), ; and globs in any argument were interpreted. run() and DirCommand now take an argv that is spawned directly. A .looprc alias expands into words (POSIX quoting, nothing expanded or run). A program that cannot start fails its repo instead of panicking. --include/--exclude values match a directory's whole path from the root ('.' for the root), never a substring of its absolute path. Implements [[tasks/harmony-2756]] --- Cargo.toml | 1 + src/lib.rs | 367 +++++++++++++++++++++++++-------------------------- src/tests.rs | 254 ++++++++++++++++++++++------------- 3 files changed, 340 insertions(+), 282 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 86f1809..825211b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,6 +15,7 @@ colored = "2.0" diff = "0.1" indicatif = "0.17" is-terminal = "0.4" +shlex = "1" [dev-dependencies] tempfile = "3.3" diff --git a/src/lib.rs b/src/lib.rs index 713abc2..4d677c5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -14,26 +14,6 @@ use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; use std::time::Duration; -/// Returns the shell program and command-line flag for executing commands. -/// On Unix: uses $SHELL or /bin/sh with -c -/// On Windows: uses cmd.exe with /c -fn get_shell_and_flag() -> (String, &'static str) { - #[cfg(windows)] - { - ( - env::var("COMSPEC").unwrap_or_else(|_| "cmd.exe".to_string()), - "/c", - ) - } - #[cfg(not(windows))] - { - ( - env::var("SHELL").unwrap_or_else(|_| "/bin/sh".to_string()), - "-c", - ) - } -} - #[derive(Debug, Clone, Deserialize, Serialize)] pub struct LoopConfig { #[serde(default)] @@ -81,7 +61,8 @@ pub struct LoopConfig { #[derive(Debug, Clone, Deserialize, Serialize)] pub struct DirCommand { pub dir: String, - pub cmd: String, + /// The program and its arguments, run as given: no shell parses them. + pub argv: Vec, /// Environment variables to set for this command's subprocess #[serde(default, skip_serializing_if = "Option::is_none")] pub env: Option>, @@ -236,19 +217,24 @@ fn display_name(dir: &Path, root: Option<&Path>) -> String { .unwrap_or(".") .to_string(); }; - match dir.strip_prefix(root) { - Ok(rel) if rel.as_os_str().is_empty() => ".".to_string(), - Ok(rel) => { - let parts: Vec<_> = rel - .components() - .map(|c| c.as_os_str().to_string_lossy()) - .collect(); - format!("./{}", parts.join("/")) - } - Err(_) => dir.display().to_string(), + match path_from(dir, root) { + Some(rel) if rel.is_empty() => ".".to_string(), + Some(rel) => format!("./{rel}"), + None => dir.display().to_string(), } } +/// `dir`'s path from `root`, `/`-separated and empty for `root` itself; +/// None when `dir` is not under `root`. +fn path_from(dir: &Path, root: &Path) -> Option { + let rel = dir.strip_prefix(root).ok()?; + let parts: Vec<_> = rel + .components() + .map(|c| c.as_os_str().to_string_lossy()) + .collect(); + Some(parts.join("/")) +} + /// [`display_name`], with the root's folder name added: `. (harmony)`. fn result_name(dir: &Path, root: Option<&Path>) -> String { let name = display_name(dir, root); @@ -266,9 +252,73 @@ fn result_name(dir: &Path, root: Option<&Path>) -> String { } } +/// `argv` with a `.looprc` alias for its program expanded. The alias value is +/// split into words the way a POSIX shell splits them (quotes are honored, +/// nothing is expanded or run) and replaces the program. +fn resolve_aliases(argv: &[String], aliases: &HashMap) -> Result> { + let Some((program, args)) = argv.split_first() else { + anyhow::bail!("No command given"); + }; + let Some(alias) = aliases.get(program) else { + return Ok(argv.to_vec()); + }; + let mut words = shlex::split(alias) + .filter(|words| !words.is_empty()) + .with_context(|| format!("Alias '{program}' is not a command: {alias}"))?; + words.extend_from_slice(args); + Ok(words) +} + +/// `argv` as one line, quoted so that it reads back as the same words. +pub fn display_command(argv: &[String]) -> String { + shlex::try_join(argv.iter().map(String::as_str)).unwrap_or_else(|_| argv.join(" ")) +} + +/// The process for `argv` in `dir`. The program runs directly: no shell sees +/// the arguments, so quotes, `$()`, `;` and globs in them stay literal. +fn build_command( + argv: &[String], + dir: &Path, + extra_env: Option<&HashMap>, +) -> Result { + let (program, args) = argv + .split_first() + .ok_or_else(|| anyhow::anyhow!("No command given"))?; + let mut cmd = Command::new(program); + cmd.args(args).current_dir(dir); + // Plugin-specified environment variables (e.g., GIT_PAGER=cat) + if let Some(extra) = extra_env { + cmd.envs(extra); + } + Ok(cmd) +} + +fn failed_result(dir: &Path, command: String, stderr: String) -> CommandResult { + CommandResult { + success: false, + exit_code: -1, + directory: dir.to_path_buf(), + command, + stdout: String::new(), + stderr, + } +} + +/// The dry-run line for `command` in `dir`. +fn dry_run_line(dir: &Path, command: &str) -> String { + let dir_display = if dir.as_os_str() == "." { + env::current_dir() + .map(|cwd| cwd.display().to_string()) + .unwrap_or_else(|_| ".".to_string()) + } else { + dir.display().to_string() + }; + format!("Would execute in {dir_display}:\n {command}") +} + pub fn execute_command_in_directory( dir: &Path, - command: &str, + argv: &[String], config: &LoopConfig, aliases: &HashMap, extra_env: Option<&HashMap>, @@ -278,49 +328,34 @@ pub fn execute_command_in_directory( println!( "\x1b[31m\n✗ {}: No directory found. Command: {} (Exit code: {})\x1b[0m", display_name(dir, config.root_dir.as_deref()), - command, + display_command(argv), 1 ); return CommandResult { - success: false, exit_code: 1, - directory: dir.to_path_buf(), - command: command.to_string(), - stdout: String::new(), - stderr: String::new(), + ..failed_result(dir, display_command(argv), String::new()) }; } - // Resolve aliases for display - let resolved_command = command - .split_whitespace() - .next() - .and_then(|cmd| aliases.get(cmd).map(|alias_cmd| (cmd, alias_cmd))) - .map(|(cmd, alias_cmd)| command.replacen(cmd, alias_cmd, 1)) - .unwrap_or_else(|| command.to_string()); + let argv = match resolve_aliases(argv, aliases) { + Ok(argv) => argv, + Err(e) => { + if !config.silent { + eprintln!("{e:#}"); + } + return failed_result(dir, display_command(argv), format!("{e:#}\n")); + } + }; + let command = display_command(&argv); // Dry run mode: print what would be executed without running it if config.dry_run { - let dir_display = if dir.as_os_str() == "." { - if let Ok(cwd) = std::env::current_dir() { - cwd.display().to_string() - } else { - ".".to_string() - } - } else { - dir.display().to_string() - }; - println!( - "{} Would execute in {}:\n {}", - "[DRY RUN]".cyan(), - dir_display.yellow(), - resolved_command - ); + println!("{} {}", "[DRY RUN]".cyan(), dry_run_line(dir, &command)); return CommandResult { success: true, exit_code: 0, directory: dir.to_path_buf(), - command: resolved_command, + command, stdout: String::new(), stderr: String::new(), }; @@ -332,25 +367,11 @@ pub fn execute_command_in_directory( if !config.silent { println!(); - io::stdout().flush().unwrap(); + io::stdout().flush().ok(); } - let (shell, shell_flag) = get_shell_and_flag(); - - let mut cmd_builder = Command::new(&shell); - cmd_builder - .arg(shell_flag) - .arg(&resolved_command) - .current_dir(dir) - .envs(env::vars()); - - // Apply plugin-specified environment variables (e.g., GIT_PAGER=cat) - if let Some(extra) = extra_env { - cmd_builder.envs(extra); - } - - let mut child = cmd_builder - .stdout(if config.silent { + let status = build_command(&argv, dir, extra_env).and_then(|mut cmd| { + cmd.stdout(if config.silent { Stdio::null() } else { Stdio::inherit() @@ -360,19 +381,18 @@ pub fn execute_command_in_directory( } else { Stdio::inherit() }) - .spawn() - .with_context(|| { - format!( - "Failed to execute command '{}' in directory '{}'", - resolved_command, - dir.display() - ) - }) - .expect("Failed to execute command"); - - let status = child.wait().expect("Failed to wait on child process"); - let exit_code = status.code().unwrap_or(-1); - let success = status.success(); + .status() + .with_context(|| format!("Failed to run {command}")) + }); + let (success, exit_code) = match status { + Ok(status) => (status.success(), status.code().unwrap_or(-1)), + Err(e) => { + if !config.silent { + eprintln!("{e:#}"); + } + (false, -1) + } + }; if !config.silent { let name = result_name(dir, config.root_dir.as_deref()); @@ -381,14 +401,14 @@ pub fn execute_command_in_directory( } else { println!("\x1b[31m\n✗ {name}: exited code {exit_code}\x1b[0m"); } - io::stdout().flush().unwrap(); + io::stdout().flush().ok(); } CommandResult { success, exit_code, directory: dir.to_path_buf(), - command: resolved_command, + command, stdout: String::new(), // Sequential mode uses Stdio::inherit(), so no capture stderr: String::new(), } @@ -397,64 +417,45 @@ pub fn execute_command_in_directory( /// Capturing version for parallel execution - captures stdout/stderr for display after completion pub fn execute_command_in_directory_capturing( dir: &Path, - command: &str, + argv: &[String], config: &LoopConfig, aliases: &HashMap, extra_env: Option<&HashMap>, ) -> CommandResult { if !dir.exists() { return CommandResult { - success: false, exit_code: 1, - directory: dir.to_path_buf(), - command: command.to_string(), - stdout: String::new(), - stderr: format!("Directory does not exist: {}", dir.display()), + ..failed_result( + dir, + display_command(argv), + format!("Directory does not exist: {}", dir.display()), + ) }; } - let resolved_command = command - .split_whitespace() - .next() - .and_then(|cmd| aliases.get(cmd).map(|alias_cmd| (cmd, alias_cmd))) - .map(|(cmd, alias_cmd)| command.replacen(cmd, alias_cmd, 1)) - .unwrap_or_else(|| command.to_string()); + let argv = match resolve_aliases(argv, aliases) { + Ok(argv) => argv, + Err(e) => return failed_result(dir, display_command(argv), format!("{e:#}\n")), + }; + let command = display_command(&argv); // Dry run mode: return what would be executed without running it if config.dry_run { - let dir_display = if dir.as_os_str() == "." { - if let Ok(cwd) = std::env::current_dir() { - cwd.display().to_string() - } else { - ".".to_string() - } - } else { - dir.display().to_string() - }; - let stdout_msg = format!("[DRY RUN] Would execute in {dir_display}:\n {resolved_command}"); + let stdout = format!("[DRY RUN] {}", dry_run_line(dir, &command)); return CommandResult { success: true, exit_code: 0, directory: dir.to_path_buf(), - command: resolved_command, - stdout: stdout_msg, + command, + stdout, stderr: String::new(), }; } - let (shell, shell_flag) = get_shell_and_flag(); - - let mut cmd_builder = Command::new(&shell); - cmd_builder - .arg(shell_flag) - .arg(&resolved_command) - .current_dir(dir) - .envs(env::vars()); - - // Apply plugin-specified environment variables (e.g., GIT_PAGER=cat) - if let Some(extra) = extra_env { - cmd_builder.envs(extra); - } + let mut cmd_builder = match build_command(&argv, dir, extra_env) { + Ok(cmd) => cmd, + Err(e) => return failed_result(dir, command, format!("{e:#}\n")), + }; // Force color output when parent stdout is a TTY // This is needed because piped stdout makes child processes think they're not in a terminal @@ -483,19 +484,16 @@ pub fn execute_command_in_directory_capturing( success, exit_code, directory: dir.to_path_buf(), - command: resolved_command, + command, stdout: String::from_utf8_lossy(&output.stdout).to_string(), stderr: String::from_utf8_lossy(&output.stderr).to_string(), } } - Err(e) => CommandResult { - success: false, - exit_code: -1, - directory: dir.to_path_buf(), - command: resolved_command, - stdout: String::new(), - stderr: format!("Failed to execute: {e}"), - }, + Err(e) => failed_result( + dir, + command.clone(), + format!("Failed to run {command}: {e}\n"), + ), } } @@ -524,51 +522,50 @@ pub fn expand_directories(directories: &[String], ignore: &[String]) -> Result Result<()> { +pub fn run(orig_config: &LoopConfig, argv: &[String]) -> Result<()> { // Handle special case: add_aliases_to_global_looprc if orig_config.add_aliases_to_global_looprc { return add_aliases_to_global_looprc(); } - // Apply include/exclude filters to directories - let mut dirs = orig_config.directories.clone(); - - if let Some(ref includes) = orig_config.include_filters { - if !includes.is_empty() { - dirs.retain(|p| includes.iter().any(|f| p.contains(f))); - } - } - - if let Some(ref excludes) = orig_config.exclude_filters { - if !excludes.is_empty() { - if orig_config.verbose { - println!("Exclude filters: {excludes:?}"); - } - dirs.retain(|p| { - let excluded = excludes.iter().any(|f| { - let f = f.trim_end_matches('/'); - p.contains(f) - }); - if orig_config.verbose { - println!("Dir: {p}, excluded: {excluded}"); - } - !excluded - }); - } - } - // Build DirCommand list with same command for each directory - let commands: Vec = dirs + let commands: Vec = orig_config + .directories .iter() .map(|dir| DirCommand { dir: dir.clone(), - cmd: command.to_string(), + argv: argv.to_vec(), env: orig_config.env.clone(), }) .collect(); - // Delegate to unified execution engine - execute_commands_internal(orig_config, &commands) + run_commands(orig_config, &commands) +} + +/// Whether an `--include`/`--exclude` value names `dir`: the value is the +/// whole path of `dir` from `root` (`.` for the root itself, a trailing `/` +/// and a leading `./` ignored), never a part of it. +pub fn filter_matches(filter: &str, dir: &str, root: Option<&Path>) -> bool { + let filter = filter.trim_end_matches('/'); + let filter = filter.strip_prefix("./").unwrap_or(filter); + if filter.is_empty() { + return false; + } + match root.and_then(|root| repo_path(dir, root)) { + Some(path) => filter == path, + None => Path::new(filter) == Path::new(dir), + } +} + +/// `dir`'s path from `root` as `--include`/`--exclude` name it: `/`-separated, +/// `.` for the root itself. A relative `dir` is taken from the current +/// directory. None when `dir` is not under `root`. +pub fn repo_path(dir: &str, root: &Path) -> Option { + let dir = env::current_dir().ok()?.join(dir); + match path_from(&dir, root)? { + rel if rel.is_empty() => Some(".".to_string()), + rel => Some(rel), + } } /// JSON output structure for command results @@ -675,7 +672,7 @@ fn execute_commands_internal(config: &LoopConfig, commands: &[DirCommand]) -> Re let result = execute_command_in_directory_capturing( &dir, - &dir_cmd.cmd, + &dir_cmd.argv, config, &aliases, dir_cmd.env.as_ref(), @@ -766,7 +763,7 @@ fn execute_commands_internal(config: &LoopConfig, commands: &[DirCommand]) -> Re // Capture output for JSON mode execute_command_in_directory_capturing( &dir, - &dir_cmd.cmd, + &dir_cmd.argv, config, &aliases, dir_cmd.env.as_ref(), @@ -774,7 +771,7 @@ fn execute_commands_internal(config: &LoopConfig, commands: &[DirCommand]) -> Re } else { execute_command_in_directory( &dir, - &dir_cmd.cmd, + &dir_cmd.argv, config, &aliases, dir_cmd.env.as_ref(), @@ -867,24 +864,20 @@ fn execute_commands_internal(config: &LoopConfig, commands: &[DirCommand]) -> Re /// This is the unified execution engine for plugins. /// Applies include/exclude filters from config before executing. pub fn run_commands(config: &LoopConfig, commands: &[DirCommand]) -> Result<()> { + let root = config.root_dir.as_deref(); + let named = + |filters: &[String], dir: &str| filters.iter().any(|f| filter_matches(f, dir, root)); let mut filtered: Vec = commands.to_vec(); - if let Some(ref includes) = config.include_filters { - if !includes.is_empty() { - filtered.retain(|c| includes.iter().any(|f| c.dir.contains(f))); - } + if let Some(includes) = config.include_filters.as_ref().filter(|f| !f.is_empty()) { + filtered.retain(|c| named(includes, &c.dir)); } - if let Some(ref excludes) = config.exclude_filters { - if !excludes.is_empty() { - filtered.retain(|c| { - let excluded = excludes.iter().any(|f| { - let f = f.trim_end_matches('/'); - c.dir.contains(f) - }); - !excluded - }); + if let Some(excludes) = &config.exclude_filters { + if config.verbose && !excludes.is_empty() { + println!("Exclude filters: {excludes:?}"); } + filtered.retain(|c| !named(excludes, &c.dir)); } execute_commands_internal(config, &filtered) diff --git a/src/tests.rs b/src/tests.rs index d78c8e7..79da5b7 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -2,22 +2,35 @@ use super::*; use std::fs; use tempfile::TempDir; +/// `script` run by the platform shell, the way a user asks for one +fn sh(script: &str) -> Vec { + let shell: &[&str] = if cfg!(windows) { + &["cmd", "/c"] + } else { + &["sh", "-c"] + }; + shell + .iter() + .map(|s| s.to_string()) + .chain([script.to_string()]) + .collect() +} + /// Cross-platform command that always fails with exit code 1 -#[cfg(windows)] -const FAIL_CMD: &str = "cmd /c exit 1"; -#[cfg(not(windows))] -const FAIL_CMD: &str = "false"; +fn fail_cmd() -> Vec { + sh("exit 1") +} /// Returns a cross-platform touch command for the given path #[cfg(windows)] -fn touch_cmd(path: &std::path::Path) -> String { +fn touch_cmd(path: &std::path::Path) -> Vec { // Simple echo redirect works with cmd /c // The > creates the file if it doesn't exist - format!("echo.>\"{}\"", path.display()) + sh(&format!("echo.>\"{}\"", path.display())) } #[cfg(not(windows))] -fn touch_cmd(path: &std::path::Path) -> String { - format!("touch {}", path.display()) +fn touch_cmd(path: &std::path::Path) -> Vec { + vec!["touch".to_string(), path.display().to_string()] } #[test] @@ -128,11 +141,11 @@ fn test_run() { root_dir: None, }; - let result = run(&config, "echo test"); + let result = run(&config, &sh("echo test")); assert!(result.is_ok()); // Test with a failing command - let result = run(&config, FAIL_CMD); + let result = run(&config, &fail_cmd()); assert!(result.is_err()); // The function should return an error if any command fails } @@ -166,11 +179,12 @@ fn test_execute_command_in_directory() { let temp_dir = TempDir::new().unwrap(); let result = - execute_command_in_directory(temp_dir.path(), "echo test", &config, &aliases, None); + execute_command_in_directory(temp_dir.path(), &sh("echo test"), &config, &aliases, None); assert!(result.success); assert_eq!(result.exit_code, 0); - let result = execute_command_in_directory(temp_dir.path(), FAIL_CMD, &config, &aliases, None); + let result = + execute_command_in_directory(temp_dir.path(), &fail_cmd(), &config, &aliases, None); assert!(!result.success); assert_eq!(result.exit_code, 1); } @@ -200,7 +214,7 @@ fn test_run_without_looprc() { root_dir: None, }; - let result = run(&config, "echo test"); + let result = run(&config, &sh("echo test")); assert!(result.is_ok()); } @@ -235,11 +249,11 @@ fn test_run_parallel() { root_dir: None, }; - let result = run(&config, "echo test"); + let result = run(&config, &sh("echo test")); assert!(result.is_ok()); // Test with a failing command in parallel mode - let result = run(&config, FAIL_CMD); + let result = run(&config, &fail_cmd()); assert!(result.is_err()); } @@ -255,7 +269,7 @@ fn test_execute_command_in_directory_capturing() { let result = execute_command_in_directory_capturing( temp_dir.path(), - "echo hello", + &sh("echo hello"), &config, &aliases, None, @@ -264,8 +278,13 @@ fn test_execute_command_in_directory_capturing() { assert_eq!(result.exit_code, 0); assert!(result.stdout.contains("hello")); - let result = - execute_command_in_directory_capturing(temp_dir.path(), FAIL_CMD, &config, &aliases, None); + let result = execute_command_in_directory_capturing( + temp_dir.path(), + &fail_cmd(), + &config, + &aliases, + None, + ); assert!(!result.success); assert_eq!(result.exit_code, 1); } @@ -283,7 +302,7 @@ fn test_execute_command_in_directory_capturing_stderr() { // Command that writes to stderr let result = execute_command_in_directory_capturing( temp_dir.path(), - "echo error >&2", + &sh("echo error >&2"), &config, &aliases, None, @@ -302,8 +321,13 @@ fn test_execute_command_nonexistent_directory() { let aliases = HashMap::new(); let nonexistent = Path::new("/nonexistent/path/that/does/not/exist"); - let result = - execute_command_in_directory_capturing(nonexistent, "echo test", &config, &aliases, None); + let result = execute_command_in_directory_capturing( + nonexistent, + &sh("echo test"), + &config, + &aliases, + None, + ); assert!(!result.success); assert_eq!(result.exit_code, 1); assert!(result.stderr.contains("does not exist")); @@ -318,75 +342,114 @@ fn test_command_result_default() { assert!(result.stderr.is_empty()); } -#[test] -fn test_include_filters() { - let temp_dir = TempDir::new().unwrap(); - let dir1 = temp_dir.path().join("project_a"); - let dir2 = temp_dir.path().join("project_b"); - let dir3 = temp_dir.path().join("other"); - fs::create_dir(&dir1).unwrap(); - fs::create_dir(&dir2).unwrap(); - fs::create_dir(&dir3).unwrap(); +fn strings(values: &[&str]) -> Vec { + values.iter().map(|s| s.to_string()).collect() +} +/// The repos `include`/`exclude` select among api, api-legacy and web. +fn selected(include: &[&str], exclude: &[&str]) -> Vec { + let temp_dir = TempDir::new().unwrap(); + let root = temp_dir.path().join("ws"); + let names = ["api", "api-legacy", "web"]; + for name in names { + fs::create_dir_all(root.join(name)).unwrap(); + } let config = LoopConfig { - directories: vec![ - dir1.to_str().unwrap().to_string(), - dir2.to_str().unwrap().to_string(), - dir3.to_str().unwrap().to_string(), - ], - ignore: vec![], - verbose: false, + directories: names + .iter() + .map(|name| root.join(name).display().to_string()) + .collect(), silent: true, - add_aliases_to_global_looprc: false, - include_filters: Some(vec!["project".to_string()]), - exclude_filters: None, - parallel: false, - dry_run: false, - json_output: false, - spawn_stagger_ms: 0, - env: None, - max_parallel: None, - root_dir: None, + include_filters: Some(strings(include)), + exclude_filters: Some(strings(exclude)), + root_dir: Some(root.clone()), + ..LoopConfig::default() }; - - // The run function should only execute on directories matching the filter - let result = run(&config, "echo test"); - assert!(result.is_ok()); + run(&config, &touch_cmd(Path::new("ran"))).unwrap(); + names + .iter() + .filter(|name| root.join(name).join("ran").exists()) + .map(|name| name.to_string()) + .collect() } #[test] -fn test_exclude_filters() { +fn filters_name_whole_paths_from_the_root() { + assert_eq!(selected(&["api"], &[]), ["api"]); + assert_eq!(selected(&["./api/"], &[]), ["api"]); + assert_eq!(selected(&[], &["api"]), ["api-legacy", "web"]); + assert_eq!(selected(&["api", "web"], &["web"]), ["api"]); + // Parts of a path, or of the absolute path, name nothing + assert!(selected(&["a"], &[]).is_empty()); + assert!(selected(&["ws/api"], &[]).is_empty()); +} + +/// What git receives as its arguments, quoted by git itself +fn git_sees(argv: &[&str], aliases: &HashMap) -> String { let temp_dir = TempDir::new().unwrap(); - let dir1 = temp_dir.path().join("project_a"); - let dir2 = temp_dir.path().join("project_b"); - let dir3 = temp_dir.path().join("excluded"); - fs::create_dir(&dir1).unwrap(); - fs::create_dir(&dir2).unwrap(); - fs::create_dir(&dir3).unwrap(); + let config = LoopConfig::default(); + let result = execute_command_in_directory_capturing( + temp_dir.path(), + &strings(argv), + &config, + aliases, + None, + ); + assert!(result.success, "{}", result.stderr); + assert!(fs::read_dir(temp_dir.path()).unwrap().next().is_none()); + result.stdout.trim().to_string() +} +#[test] +fn arguments_reach_the_program_as_given() { + let hostile = [ + "one arg", + "$(touch PWNED)", + "a;touch PWNED", + "*", + "%h %s", + "'\"", + "", + ]; + let argv: Vec<&str> = ["git", "rev-parse", "--sq-quote"] + .into_iter() + .chain(hostile) + .collect(); + let quoted: Vec = hostile + .iter() + .map(|arg| format!("'{}'", arg.replace('\'', "'\\''"))) + .collect(); + assert_eq!(git_sees(&argv, &HashMap::new()), quoted.join(" ")); +} + +#[test] +fn aliases_expand_into_words_not_shell() { + let aliases = HashMap::from([( + "q".to_string(), + "git rev-parse --sq-quote 'two words' ;".to_string(), + )]); + assert_eq!(git_sees(&["q", "$(x)"], &aliases), "'two words' ';' '$(x)'"); + + let broken = HashMap::from([("q".to_string(), "git 'unclosed".to_string())]); + let result = execute_command_in_directory_capturing( + Path::new("."), + &strings(&["q"]), + &LoopConfig::default(), + &broken, + None, + ); + assert!(!result.success); + assert!(result.stderr.contains("Alias 'q'"), "{}", result.stderr); +} + +#[test] +fn a_missing_program_fails_without_panicking() { let config = LoopConfig { - directories: vec![ - dir1.to_str().unwrap().to_string(), - dir2.to_str().unwrap().to_string(), - dir3.to_str().unwrap().to_string(), - ], - ignore: vec![], - verbose: false, + directories: vec![".".to_string()], silent: true, - add_aliases_to_global_looprc: false, - include_filters: None, - exclude_filters: Some(vec!["excluded".to_string()]), - parallel: false, - dry_run: false, - json_output: false, - spawn_stagger_ms: 0, - env: None, - max_parallel: None, - root_dir: None, + ..LoopConfig::default() }; - - let result = run(&config, "echo test"); - assert!(result.is_ok()); + assert!(run(&config, &strings(&["meta-no-such-program-xyz"])).is_err()); } // ============================================================================ @@ -434,7 +497,7 @@ fn test_dry_run_returns_success() { }; // Even a command that would fail should succeed in dry_run mode - let result = run(&config, FAIL_CMD); + let result = run(&config, &fail_cmd()); assert!(result.is_ok(), "dry_run should always succeed"); } @@ -448,7 +511,8 @@ fn test_execute_command_in_directory_dry_run() { let aliases = HashMap::new(); let temp_dir = TempDir::new().unwrap(); - let result = execute_command_in_directory(temp_dir.path(), FAIL_CMD, &config, &aliases, None); + let result = + execute_command_in_directory(temp_dir.path(), &fail_cmd(), &config, &aliases, None); assert!(result.success, "dry_run should return success"); assert_eq!(result.exit_code, 0); } @@ -465,7 +529,7 @@ fn test_execute_command_in_directory_capturing_dry_run() { let result = execute_command_in_directory_capturing( temp_dir.path(), - "echo hello", + &sh("echo hello"), &config, &aliases, None, @@ -479,11 +543,11 @@ fn test_execute_command_in_directory_capturing_dry_run() { fn test_dir_command_struct() { let cmd = DirCommand { dir: "/some/path".to_string(), - cmd: "git status".to_string(), + argv: vec!["git".to_string(), "status".to_string()], env: None, }; assert_eq!(cmd.dir, "/some/path"); - assert_eq!(cmd.cmd, "git status"); + assert_eq!(cmd.argv, ["git", "status"]); } #[test] @@ -511,12 +575,12 @@ fn test_run_commands_sequential() { let commands = vec![ DirCommand { dir: dir1.to_str().unwrap().to_string(), - cmd: "echo test1".to_string(), + argv: sh("echo test1"), env: None, }, DirCommand { dir: dir2.to_str().unwrap().to_string(), - cmd: "echo test2".to_string(), + argv: sh("echo test2"), env: None, }, ]; @@ -542,12 +606,12 @@ fn test_run_commands_parallel() { let commands = vec![ DirCommand { dir: dir1.to_str().unwrap().to_string(), - cmd: "echo test1".to_string(), + argv: sh("echo test1"), env: None, }, DirCommand { dir: dir2.to_str().unwrap().to_string(), - cmd: "echo test2".to_string(), + argv: sh("echo test2"), env: None, }, ]; @@ -575,12 +639,12 @@ fn test_run_commands_with_different_commands() { let commands = vec![ DirCommand { dir: dir1.to_str().unwrap().to_string(), - cmd: "echo command1".to_string(), + argv: sh("echo command1"), env: None, }, DirCommand { dir: dir2.to_str().unwrap().to_string(), - cmd: "echo command2".to_string(), + argv: sh("echo command2"), env: None, }, ]; @@ -605,7 +669,7 @@ fn test_run_commands_dry_run() { let commands = vec![DirCommand { dir: temp_dir.path().to_str().unwrap().to_string(), - cmd: touch_cmd(&marker_file), + argv: touch_cmd(&marker_file), env: None, }]; @@ -628,7 +692,7 @@ fn test_run_commands_failure_handling() { let commands = vec![DirCommand { dir: dir1.to_str().unwrap().to_string(), - cmd: FAIL_CMD.to_string(), // This command always fails + argv: fail_cmd(), // This command always fails env: None, }]; @@ -648,7 +712,7 @@ fn test_run_commands_failure_in_dry_run_succeeds() { let commands = vec![DirCommand { dir: temp_dir.path().to_str().unwrap().to_string(), - cmd: FAIL_CMD.to_string(), + argv: fail_cmd(), env: None, }]; @@ -752,18 +816,18 @@ fn test_loop_config_deserialization_missing_new_fields() { fn test_dir_command_serialization() { let cmd = DirCommand { dir: "/path/to/dir".to_string(), - cmd: "git status".to_string(), + argv: vec!["git".to_string(), "status".to_string()], env: None, }; let json = serde_json::to_string(&cmd).unwrap(); assert!(json.contains("\"dir\":\"/path/to/dir\"")); - assert!(json.contains("\"cmd\":\"git status\"")); + assert!(json.contains("\"argv\":[\"git\",\"status\"]")); // Test deserialization let parsed: DirCommand = serde_json::from_str(&json).unwrap(); assert_eq!(parsed.dir, "/path/to/dir"); - assert_eq!(parsed.cmd, "git status"); + assert_eq!(parsed.argv, ["git", "status"]); } #[test] From 1f7bedcea692fe6b95920f67c576856c400b6f06 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 25 Oct 2026 04:55:43 -0700 Subject: [PATCH 2/4] refactor!: drop include/exclude filtering; dry runs fail on commands that cannot run meta selects the repos before calling loop_lib, so LoopConfig no longer has include_filters/exclude_filters. A broken alias or missing directory now fails a dry run too. Implements [[tasks/harmony-2756]] --- src/lib.rs | 56 ++++--------------------------------------- src/tests.rs | 68 ++++++++++++---------------------------------------- 2 files changed, 19 insertions(+), 105 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 4d677c5..7f84323 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -27,10 +27,6 @@ pub struct LoopConfig { #[serde(default)] pub add_aliases_to_global_looprc: bool, #[serde(default)] - pub include_filters: Option>, - #[serde(default)] - pub exclude_filters: Option>, - #[serde(default)] pub parallel: bool, #[serde(default)] pub dry_run: bool, @@ -76,8 +72,6 @@ impl Default for LoopConfig { verbose: false, silent: false, add_aliases_to_global_looprc: false, - include_filters: None, - exclude_filters: None, parallel: false, dry_run: false, json_output: false, @@ -521,7 +515,6 @@ pub fn expand_directories(directories: &[String], ignore: &[String]) -> Result Result<()> { // Handle special case: add_aliases_to_global_looprc if orig_config.add_aliases_to_global_looprc { @@ -542,32 +535,6 @@ pub fn run(orig_config: &LoopConfig, argv: &[String]) -> Result<()> { run_commands(orig_config, &commands) } -/// Whether an `--include`/`--exclude` value names `dir`: the value is the -/// whole path of `dir` from `root` (`.` for the root itself, a trailing `/` -/// and a leading `./` ignored), never a part of it. -pub fn filter_matches(filter: &str, dir: &str, root: Option<&Path>) -> bool { - let filter = filter.trim_end_matches('/'); - let filter = filter.strip_prefix("./").unwrap_or(filter); - if filter.is_empty() { - return false; - } - match root.and_then(|root| repo_path(dir, root)) { - Some(path) => filter == path, - None => Path::new(filter) == Path::new(dir), - } -} - -/// `dir`'s path from `root` as `--include`/`--exclude` name it: `/`-separated, -/// `.` for the root itself. A relative `dir` is taken from the current -/// directory. None when `dir` is not under `root`. -pub fn repo_path(dir: &str, root: &Path) -> Option { - let dir = env::current_dir().ok()?.join(dir); - match path_from(&dir, root)? { - rel if rel.is_empty() => Some(".".to_string()), - rel => Some(rel), - } -} - /// JSON output structure for command results #[derive(Debug, Serialize)] pub struct JsonOutput { @@ -853,7 +820,9 @@ fn execute_commands_internal(config: &LoopConfig, commands: &[DirCommand]) -> Re } } - if failed_count > 0 && !config.dry_run { + // In a dry run only commands that cannot run (a missing directory, a + // broken alias) fail + if failed_count > 0 { return Err(anyhow::anyhow!("At least one command failed")); } @@ -862,25 +831,8 @@ fn execute_commands_internal(config: &LoopConfig, commands: &[DirCommand]) -> Re /// Execute a list of commands (each with its own directory) /// This is the unified execution engine for plugins. -/// Applies include/exclude filters from config before executing. pub fn run_commands(config: &LoopConfig, commands: &[DirCommand]) -> Result<()> { - let root = config.root_dir.as_deref(); - let named = - |filters: &[String], dir: &str| filters.iter().any(|f| filter_matches(f, dir, root)); - let mut filtered: Vec = commands.to_vec(); - - if let Some(includes) = config.include_filters.as_ref().filter(|f| !f.is_empty()) { - filtered.retain(|c| named(includes, &c.dir)); - } - - if let Some(excludes) = &config.exclude_filters { - if config.verbose && !excludes.is_empty() { - println!("Exclude filters: {excludes:?}"); - } - filtered.retain(|c| !named(excludes, &c.dir)); - } - - execute_commands_internal(config, &filtered) + execute_commands_internal(config, commands) } pub fn should_ignore(path: &Path, ignore: &[String]) -> bool { diff --git a/src/tests.rs b/src/tests.rs index 79da5b7..701cd8c 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -130,8 +130,6 @@ fn test_run() { verbose: false, silent: true, add_aliases_to_global_looprc: false, - include_filters: None, - exclude_filters: None, parallel: false, dry_run: false, json_output: false, @@ -203,8 +201,6 @@ fn test_run_without_looprc() { verbose: false, silent: true, add_aliases_to_global_looprc: false, - include_filters: None, - exclude_filters: None, parallel: false, dry_run: false, json_output: false, @@ -238,8 +234,6 @@ fn test_run_parallel() { verbose: false, silent: true, add_aliases_to_global_looprc: false, - include_filters: None, - exclude_filters: None, parallel: true, dry_run: false, json_output: false, @@ -346,44 +340,6 @@ fn strings(values: &[&str]) -> Vec { values.iter().map(|s| s.to_string()).collect() } -/// The repos `include`/`exclude` select among api, api-legacy and web. -fn selected(include: &[&str], exclude: &[&str]) -> Vec { - let temp_dir = TempDir::new().unwrap(); - let root = temp_dir.path().join("ws"); - let names = ["api", "api-legacy", "web"]; - for name in names { - fs::create_dir_all(root.join(name)).unwrap(); - } - let config = LoopConfig { - directories: names - .iter() - .map(|name| root.join(name).display().to_string()) - .collect(), - silent: true, - include_filters: Some(strings(include)), - exclude_filters: Some(strings(exclude)), - root_dir: Some(root.clone()), - ..LoopConfig::default() - }; - run(&config, &touch_cmd(Path::new("ran"))).unwrap(); - names - .iter() - .filter(|name| root.join(name).join("ran").exists()) - .map(|name| name.to_string()) - .collect() -} - -#[test] -fn filters_name_whole_paths_from_the_root() { - assert_eq!(selected(&["api"], &[]), ["api"]); - assert_eq!(selected(&["./api/"], &[]), ["api"]); - assert_eq!(selected(&[], &["api"]), ["api-legacy", "web"]); - assert_eq!(selected(&["api", "web"], &["web"]), ["api"]); - // Parts of a path, or of the absolute path, name nothing - assert!(selected(&["a"], &[]).is_empty()); - assert!(selected(&["ws/api"], &[]).is_empty()); -} - /// What git receives as its arguments, quoted by git itself fn git_sees(argv: &[&str], aliases: &HashMap) -> String { let temp_dir = TempDir::new().unwrap(); @@ -431,15 +387,21 @@ fn aliases_expand_into_words_not_shell() { assert_eq!(git_sees(&["q", "$(x)"], &aliases), "'two words' ';' '$(x)'"); let broken = HashMap::from([("q".to_string(), "git 'unclosed".to_string())]); - let result = execute_command_in_directory_capturing( - Path::new("."), - &strings(&["q"]), - &LoopConfig::default(), - &broken, - None, - ); - assert!(!result.success); - assert!(result.stderr.contains("Alias 'q'"), "{}", result.stderr); + for dry_run in [false, true] { + let config = LoopConfig { + dry_run, + ..LoopConfig::default() + }; + let result = execute_command_in_directory_capturing( + Path::new("."), + &strings(&["q"]), + &config, + &broken, + None, + ); + assert!(!result.success); + assert!(result.stderr.contains("Alias 'q'"), "{}", result.stderr); + } } #[test] From 0e92b45ef17b857547b92150682322848a65d794 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 25 Oct 2026 05:38:40 -0700 Subject: [PATCH 3/4] fix: no panic on a bad spinner template or a thread pool that can't start Implements [[tasks/harmony-2756]] --- src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 7f84323..ba7ddda 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -586,7 +586,7 @@ fn execute_commands_internal(config: &LoopConfig, commands: &[DirCommand]) -> Re None }; let spinner_style = ProgressStyle::with_template("{prefix:.bold.dim} {spinner} {wide_msg}") - .unwrap() + .unwrap_or_else(|_| ProgressStyle::default_spinner()) .tick_chars("⠁⠂⠄⡀⢀⠠⠐⠈ "); let total = commands.len(); @@ -666,7 +666,7 @@ fn execute_commands_internal(config: &LoopConfig, commands: &[DirCommand]) -> Re let pool = ThreadPoolBuilder::new() .num_threads(max) .build() - .expect("Failed to create thread pool"); + .context("Failed to create thread pool")?; pool.install(execute_parallel) } else { execute_parallel() From c15fab51089e8e3984de49ffe189cf8c176c27fd Mon Sep 17 00:00:00 2001 From: t Date: Sun, 25 Oct 2026 05:54:43 -0700 Subject: [PATCH 4/4] test: fix a stale dry-run comment Implements [[tasks/harmony-2756]] --- src/tests.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/tests.rs b/src/tests.rs index 701cd8c..926fe41 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -460,7 +460,10 @@ fn test_dry_run_returns_success() { // Even a command that would fail should succeed in dry_run mode let result = run(&config, &fail_cmd()); - assert!(result.is_ok(), "dry_run should always succeed"); + assert!( + result.is_ok(), + "a dry run of a command that would fail succeeds" + ); } #[test]