diff --git a/crates/with-watch/src/analysis.rs b/crates/with-watch/src/analysis.rs index 70433a9..012c62e 100644 --- a/crates/with-watch/src/analysis.rs +++ b/crates/with-watch/src/analysis.rs @@ -1131,20 +1131,27 @@ fn analyze_grep( index += 1; continue; } - if let Some(value) = token.strip_prefix("-e") { - if !value.is_empty() { - explicit_pattern = true; - index += 1; - continue; - } - } - if let Some(value) = token.strip_prefix("-f") { - if !value.is_empty() { - explicit_pattern = true; - push_inferred_input(&mut inputs, value, cwd)?; - index += 1; - continue; + if let Some(option) = parse_grep_short_pattern_option(token) { + explicit_pattern = true; + match option { + GrepShortPatternOption::Inline => {} + GrepShortPatternOption::Next => { + index += 2; + continue; + } + GrepShortPatternOption::FileInline(value) => { + push_inferred_input(&mut inputs, value, cwd)?; + } + GrepShortPatternOption::FileNext => { + if let Some(value) = argv.get(index + 1) { + push_inferred_input(&mut inputs, value.as_str(), cwd)?; + } + index += 2; + continue; + } } + index += 1; + continue; } if token.starts_with('-') { index += 1; @@ -1173,6 +1180,34 @@ fn analyze_grep( Ok(analysis) } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum GrepShortPatternOption<'a> { + Inline, + Next, + FileInline(&'a str), + FileNext, +} + +fn parse_grep_short_pattern_option(token: &str) -> Option> { + if !token.starts_with('-') || token == "-" || token.starts_with("--") { + return None; + } + + let flags = token.trim_start_matches('-'); + for (index, flag) in flags.char_indices() { + let value = &flags[index + flag.len_utf8()..]; + match flag { + 'e' if value.is_empty() => return Some(GrepShortPatternOption::Next), + 'e' => return Some(GrepShortPatternOption::Inline), + 'f' if value.is_empty() => return Some(GrepShortPatternOption::FileNext), + 'f' => return Some(GrepShortPatternOption::FileInline(value)), + _ => {} + } + } + + None +} + fn analyze_sed( argv: &[String], redirects: &[ShellRedirect], @@ -1367,9 +1402,11 @@ fn analyze_find( index += 1; continue; } - if !saw_expression && FIND_GLOBAL_OPTIONS.contains(&token) { - index += 1; - continue; + if !saw_expression { + if let Some(next_index) = consume_find_global_option(argv, index) { + index = next_index; + continue; + } } if !saw_expression && !is_find_expression_token(token) { push_inferred_input(&mut inputs, token, cwd)?; @@ -1398,7 +1435,31 @@ fn analyze_find( Ok(analysis) } -const FIND_GLOBAL_OPTIONS: &[&str] = &["-H", "-L", "-P", "-D", "-O"]; +fn consume_find_global_option(argv: &[String], index: usize) -> Option { + let token = argv[index].as_str(); + match token { + "-H" | "-L" | "-P" => Some(index + 1), + "-D" => Some((index + 2).min(argv.len())), + "-O" => { + if argv + .get(index + 1) + .is_some_and(|value| is_unsigned_integer(value)) + { + Some(index + 2) + } else { + Some(index + 1) + } + } + _ if token.starts_with("-D") && token.len() > 2 => Some(index + 1), + _ if token.starts_with("-O") && token.len() > 2 => Some(index + 1), + _ => None, + } +} + +fn is_unsigned_integer(token: &str) -> bool { + let trimmed = token.trim(); + !trimmed.is_empty() && trimmed.chars().all(|character| character.is_ascii_digit()) +} fn analyze_xargs( argv: &[String], @@ -2173,6 +2234,18 @@ mod tests { ) .expect("analyze"); assert_eq!(analysis.inputs.len(), 2); + + let grouped = analyze_argv( + &[ + OsString::from("grep"), + OsString::from("-rf"), + OsString::from("patterns.txt"), + OsString::from("src"), + ], + cwd.path(), + ) + .expect("analyze"); + assert_eq!(grouped.inputs.len(), 2); } #[test] @@ -2214,6 +2287,34 @@ mod tests { let analysis = analyze_argv(&[OsString::from("find")], cwd.path()).expect("analyze"); assert_eq!(analysis.inputs.len(), 1); assert!(analysis.default_watch_root_used); + + let analysis = analyze_argv( + &[ + OsString::from("find"), + OsString::from("-D"), + OsString::from("stat"), + OsString::from("-name"), + OsString::from("*.rs"), + ], + cwd.path(), + ) + .expect("analyze"); + assert_eq!(analysis.inputs.len(), 1); + assert!(analysis.default_watch_root_used); + + let analysis = analyze_argv( + &[ + OsString::from("find"), + OsString::from("-O"), + OsString::from("3"), + OsString::from("-name"), + OsString::from("*.rs"), + ], + cwd.path(), + ) + .expect("analyze"); + assert_eq!(analysis.inputs.len(), 1); + assert!(analysis.default_watch_root_used); } #[test] diff --git a/crates/with-watch/src/lib.rs b/crates/with-watch/src/lib.rs index 2a27c32..9292df7 100644 --- a/crates/with-watch/src/lib.rs +++ b/crates/with-watch/src/lib.rs @@ -123,6 +123,9 @@ fn explicit_watch_inputs(raw_inputs: &[String], cwd: &Path) -> Result bool { #[cfg(test)] mod tests { use super::explicit_watch_inputs; + use crate::error::WithWatchError; #[test] fn explicit_inputs_accept_globs_and_paths() { @@ -151,4 +155,13 @@ mod tests { assert_eq!(inputs.len(), 2); } + + #[test] + fn explicit_inputs_reject_blank_values() { + let temp_dir = tempfile::tempdir().expect("create tempdir"); + let error = explicit_watch_inputs(&[" ".to_string(), "\t".to_string()], temp_dir.path()) + .expect_err("blank inputs should fail"); + + assert!(matches!(error, WithWatchError::NoWatchInputs)); + } } diff --git a/crates/with-watch/src/parser.rs b/crates/with-watch/src/parser.rs index bff403d..42c0a38 100644 --- a/crates/with-watch/src/parser.rs +++ b/crates/with-watch/src/parser.rs @@ -140,9 +140,8 @@ fn build_shell_command(command: starbase_args::Command) -> Result shell_command.argv.push(flag); } starbase_args::Argument::Option(option, Some(value)) => { - shell_command - .argv - .push(format!("{option}={}", value.as_str())); + shell_command.argv.push(option); + shell_command.argv.push(value.as_str().to_string()); } starbase_args::Argument::Option(option, None) => { shell_command.argv.push(option); @@ -250,6 +249,17 @@ mod tests { assert_eq!(command.redirects[1].target, "output.txt"); } + #[test] + fn preserves_shell_option_values_as_separate_tokens() { + let parsed = parse_shell_expression("grep -f patterns.txt file.txt").expect("parse shell"); + + assert_eq!(parsed.commands.len(), 1); + assert_eq!( + parsed.commands[0].argv, + vec!["grep", "-f", "patterns.txt", "file.txt"] + ); + } + #[test] fn rejects_shell_control_flow_keywords() { let error = diff --git a/crates/with-watch/src/runner.rs b/crates/with-watch/src/runner.rs index 4a7bde6..3dde0c6 100644 --- a/crates/with-watch/src/runner.rs +++ b/crates/with-watch/src/runner.rs @@ -12,7 +12,7 @@ use tracing::{debug, info, warn}; use crate::{ analysis::{CommandAdapterId, CommandAnalysisStatus, SideEffectProfile}, error::{Result, WithWatchError}, - snapshot::{capture_snapshot, ChangeDetectionMode, CommandSource, WatchInput}, + snapshot::{capture_snapshot, ChangeDetectionMode, CommandSource, SnapshotState, WatchInput}, watch::{CollectedEvents, WatchLoop}, }; @@ -103,6 +103,27 @@ pub enum DelegatedCommand { } impl DelegatedCommand { + fn spawn_log_summary(&self) -> DelegatedCommandLogSummary { + match self { + Self::Argv(argv) => { + let program_name = argv + .first() + .map(program_name) + .unwrap_or_else(|| "".to_string()); + DelegatedCommandLogSummary { + execution_kind: "argv", + program_name, + arg_count: argv.len().saturating_sub(1), + } + } + Self::Shell(_) => DelegatedCommandLogSummary { + execution_kind: "shell", + program_name: "sh".to_string(), + arg_count: 2, + }, + } + } + fn display_name(&self) -> String { match self { Self::Argv(argv) => argv @@ -115,6 +136,13 @@ impl DelegatedCommand { } } +#[derive(Debug, Clone, PartialEq, Eq)] +struct DelegatedCommandLogSummary { + execution_kind: &'static str, + program_name: String, + arg_count: usize, +} + #[derive(Debug, Clone, Copy)] pub struct RunnerOptions { pub debounce_window: Duration, @@ -162,6 +190,7 @@ pub fn run(plan: ExecutionPlan, options: RunnerOptions) -> Result { let mut child = Some(spawn_command(&plan.delegated_command)?); let mut completed_runs = 0usize; let mut pending_rerun = false; + let mut suppressed_self_change_snapshot = None::; info!( command_source = plan.source.as_str(), @@ -191,12 +220,39 @@ pub fn run(plan: ExecutionPlan, options: RunnerOptions) -> Result { let post_run_snapshot = capture_snapshot(&plan.inputs, plan.detection_mode)?; let inputs_changed_since_baseline = post_run_snapshot.is_meaningfully_different(&baseline, plan.detection_mode); - let should_rerun = pending_rerun - && plan.metadata.side_effect_profile != SideEffectProfile::WritesWatchedInputs - && inputs_changed_since_baseline; + let additional_change_after_suppression = suppressed_self_change_snapshot + .as_ref() + .is_some_and(|snapshot| { + post_run_snapshot.is_meaningfully_different(snapshot, plan.detection_mode) + }); + let should_rerun = if plan.metadata.side_effect_profile + == SideEffectProfile::WritesWatchedInputs + { + pending_rerun || additional_change_after_suppression + } else { + pending_rerun && inputs_changed_since_baseline + }; if pending_rerun && plan.metadata.side_effect_profile == SideEffectProfile::WritesWatchedInputs + { + debug!( + rerun_queued = true, + side_effect_profile = plan.metadata.side_effect_profile.as_str(), + "Queued rerun after additional changes during self-mutating command \ + activity" + ); + } else if additional_change_after_suppression + && plan.metadata.side_effect_profile == SideEffectProfile::WritesWatchedInputs + { + debug!( + rerun_queued = true, + side_effect_profile = plan.metadata.side_effect_profile.as_str(), + "Queued rerun because post-run state diverged from the suppressed \ + self-change snapshot" + ); + } else if suppressed_self_change_snapshot.is_some() + && plan.metadata.side_effect_profile == SideEffectProfile::WritesWatchedInputs { debug!( rerun_suppressed = true, @@ -207,6 +263,7 @@ pub fn run(plan: ExecutionPlan, options: RunnerOptions) -> Result { baseline = post_run_snapshot; pending_rerun = false; + suppressed_self_change_snapshot = None; child = None; write_test_run_marker(completed_runs); @@ -238,7 +295,17 @@ pub fn run(plan: ExecutionPlan, options: RunnerOptions) -> Result { handle_watch_events(&events); let current_snapshot = capture_snapshot(&plan.inputs, plan.detection_mode)?; - if current_snapshot.is_meaningfully_different(&baseline, plan.detection_mode) { + let reference_snapshot = if child.is_some() + && plan.metadata.side_effect_profile == SideEffectProfile::WritesWatchedInputs + { + suppressed_self_change_snapshot + .as_ref() + .unwrap_or(&baseline) + } else { + &baseline + }; + + if current_snapshot.is_meaningfully_different(reference_snapshot, plan.detection_mode) { debug!( event_count = events.event_count, path_count = events.path_count, @@ -247,13 +314,24 @@ pub fn run(plan: ExecutionPlan, options: RunnerOptions) -> Result { ); if child.is_some() { - pending_rerun = true; + if plan.metadata.side_effect_profile == SideEffectProfile::WritesWatchedInputs + && suppressed_self_change_snapshot.is_none() + { + suppressed_self_change_snapshot = Some(current_snapshot); + debug!( + rerun_suppressed = true, + side_effect_profile = plan.metadata.side_effect_profile.as_str(), + "Suppressed the first in-run snapshot change for a self-mutating \ + command" + ); + } else { + pending_rerun = true; + } } else { baseline = current_snapshot; child = Some(spawn_command(&plan.delegated_command)?); } } else if child.is_some() { - pending_rerun = false; debug!( rerun_suppressed = true, "Ignored non-meaningful filesystem churn" @@ -283,24 +361,28 @@ fn handle_watch_events(events: &CollectedEvents) { } fn spawn_command(command: &DelegatedCommand) -> Result { + log_delegated_command_spawn(command); match command { DelegatedCommand::Argv(argv) => spawn_argv(argv), DelegatedCommand::Shell(expression) => spawn_shell(expression), } } +fn log_delegated_command_spawn(command: &DelegatedCommand) { + let summary = command.spawn_log_summary(); + info!( + execution_kind = summary.execution_kind, + program = summary.program_name, + arg_count = summary.arg_count, + "Spawning delegated command" + ); +} + fn spawn_argv(argv: &[OsString]) -> Result { let program = argv .first() .cloned() .ok_or(WithWatchError::MissingCommand)?; - let display_name = argv - .iter() - .map(|value| value.to_string_lossy().into_owned()) - .collect::>() - .join(" "); - - info!(command = display_name, "Spawning delegated argv command"); Command::new(&program) .args(argv.iter().skip(1)) @@ -323,7 +405,6 @@ fn spawn_shell(expression: &str) -> Result { #[cfg(unix)] { - info!(expression, "Spawning delegated shell command"); Command::new("/bin/sh") .arg("-c") .arg(expression) @@ -338,6 +419,14 @@ fn spawn_shell(expression: &str) -> Result { } } +fn program_name(program: &OsString) -> String { + std::path::Path::new(program) + .file_name() + .unwrap_or(program.as_os_str()) + .to_string_lossy() + .into_owned() +} + fn exit_code_from_status(status: ExitStatus) -> i32 { status.code().unwrap_or(1) } @@ -367,3 +456,83 @@ fn write_test_run_marker(completed_runs: usize) { ); } } + +#[cfg(test)] +mod tests { + use std::{ + ffi::OsString, + io::{self, Write}, + sync::{Arc, Mutex}, + }; + + use tracing::Level; + + use super::{log_delegated_command_spawn, DelegatedCommand}; + + #[test] + fn argv_spawn_logging_omits_argument_values() { + let output = capture_logs(|| { + log_delegated_command_spawn(&DelegatedCommand::Argv(vec![ + OsString::from("env"), + OsString::from("TOKEN=secret"), + OsString::from("cmd"), + ])); + }); + + assert!(output.contains("execution_kind=\"argv\"")); + assert!(output.contains("program=\"env\"")); + assert!(output.contains("arg_count=2")); + assert!(!output.contains("TOKEN=secret")); + assert!(!output.contains("cmd")); + } + + #[test] + fn shell_spawn_logging_omits_expression_text() { + let output = capture_logs(|| { + log_delegated_command_spawn(&DelegatedCommand::Shell( + "TOKEN=secret grep -f patterns.txt file.txt".to_string(), + )); + }); + + assert!(output.contains("execution_kind=\"shell\"")); + assert!(output.contains("program=\"sh\"")); + assert!(output.contains("arg_count=2")); + assert!(!output.contains("TOKEN=secret")); + assert!(!output.contains("patterns.txt")); + } + + fn capture_logs(callback: impl FnOnce()) -> String { + let buffer = Arc::new(Mutex::new(Vec::new())); + let writer = SharedWriter(buffer.clone()); + let subscriber = tracing_subscriber::fmt() + .with_ansi(false) + .with_target(false) + .with_level(false) + .without_time() + .with_max_level(Level::INFO) + .with_writer(move || writer.clone()) + .finish(); + + tracing::subscriber::with_default(subscriber, callback); + + let output = buffer.lock().expect("lock buffer").clone(); + String::from_utf8(output).expect("utf8 log output") + } + + #[derive(Clone)] + struct SharedWriter(Arc>>); + + impl Write for SharedWriter { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.0 + .lock() + .expect("lock log buffer") + .extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } + } +} diff --git a/crates/with-watch/tests/cli.rs b/crates/with-watch/tests/cli.rs index fd83fcd..2dd941e 100644 --- a/crates/with-watch/tests/cli.rs +++ b/crates/with-watch/tests/cli.rs @@ -221,6 +221,46 @@ fn exec_mode_reruns_when_an_explicit_input_changes() { assert_eq!(lines, vec!["alpha", "beta"]); } +#[cfg(unix)] +#[test] +fn self_mutating_shell_command_reruns_after_external_change_during_execution() { + let temp_dir = tempfile::tempdir().expect("create tempdir"); + let input_path = temp_dir.path().join("input.txt"); + let marker_dir = temp_dir.path().join("markers"); + fs::write(&input_path, "alpha\n").expect("write input"); + + let expression = format!( + "sed -i.bak -e 's/alpha/beta/' '{}' && sleep 1", + input_path.display() + ); + + let mut child = ProcessCommand::new(assert_cmd::cargo::cargo_bin!("with-watch")) + .current_dir(temp_dir.path()) + .env("WITH_WATCH_TEST_MAX_RUNS", "2") + .env("WITH_WATCH_TEST_DEBOUNCE_MS", "25") + .env("WITH_WATCH_TEST_RUN_MARKER_DIR", &marker_dir) + .args(["--shell", &expression]) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn with-watch"); + + wait_for_file_contents(&input_path, "beta\n"); + assert!(child.try_wait().expect("poll child").is_none()); + thread::sleep(Duration::from_millis(150)); + + fs::write(&input_path, "alpha\n").expect("rewrite input during sleep"); + wait_for_path(&marker_dir.join("run-2.done")); + + let status = wait_for_child_exit(&mut child, Duration::from_secs(10)); + assert!(status.success()); + assert_eq!( + fs::read_to_string(&input_path).expect("read input"), + "beta\n" + ); +} + #[cfg(unix)] fn wait_for_file_contents(path: &Path, expected_contents: &str) { for _ in 0..80 {