diff --git a/src/commands/config/state.rs b/src/commands/config/state.rs index 8da88b4d19..67eed48f26 100644 --- a/src/commands/config/state.rs +++ b/src/commands/config/state.rs @@ -1080,51 +1080,42 @@ fn clear_previous_branch_reported(repo: &Repository) -> anyhow::Result { Ok(false) } -fn clear_markers_reported(repo: &Repository) -> anyhow::Result { - let cleared = clear_all_markers(repo)?; - if cleared > 0 { - eprintln!( - "{}", - success_message(cformat!( - "Cleared {cleared} marker{}", - if cleared == 1 { "" } else { "s" } - )) - ); - return Ok(true); +fn report_cleared_count(cleared: usize, singular: &str, plural: &str) -> bool { + if cleared == 0 { + return false; } - Ok(false) + let noun = if cleared == 1 { singular } else { plural }; + eprintln!( + "{}", + success_message(cformat!("Cleared {cleared} {noun}")) + ); + true +} + +fn clear_markers_reported(repo: &Repository) -> anyhow::Result { + Ok(report_cleared_count( + clear_all_markers(repo)?, + "marker", + "markers", + )) } fn clear_ci_status_reported(repo: &Repository) -> anyhow::Result { // The PR-number width ratchet is part of the CI cache category — it is // derived from the same fetches and re-learns on the next one. - let cleared = CachedCiStatus::clear_all(repo)? + MaxPrNumber::clear(repo)?; - if cleared > 0 { - eprintln!( - "{}", - success_message(cformat!( - "Cleared {cleared} CI cache entr{}", - if cleared == 1 { "y" } else { "ies" } - )) - ); - return Ok(true); - } - Ok(false) + Ok(report_cleared_count( + CachedCiStatus::clear_all(repo)? + MaxPrNumber::clear(repo)?, + "CI cache entry", + "CI cache entries", + )) } fn clear_summary_reported(repo: &Repository) -> anyhow::Result { - let cleared = CachedSummary::clear_all(repo)?; - if cleared > 0 { - eprintln!( - "{}", - success_message(cformat!( - "Cleared {cleared} summary cache entr{}", - if cleared == 1 { "y" } else { "ies" } - )) - ); - return Ok(true); - } - Ok(false) + Ok(report_cleared_count( + CachedSummary::clear_all(repo)?, + "summary cache entry", + "summary cache entries", + )) } /// Clear all SHA-keyed git command caches: parsed results (merge-tree, @@ -1132,78 +1123,43 @@ fn clear_summary_reported(repo: &Repository) -> anyhow::Result { /// upstream-diff). Surfaced as one user-facing category — see the parity /// docstring at the top of this file. fn clear_git_commands_reported(repo: &Repository) -> anyhow::Result { - let cleared = sha_cache::clear_all(repo)? + preview_cache::clear_all(repo)?; - if cleared > 0 { - eprintln!( - "{}", - success_message(cformat!( - "Cleared {cleared} git commands cache entr{}", - if cleared == 1 { "y" } else { "ies" } - )) - ); - return Ok(true); - } - Ok(false) + Ok(report_cleared_count( + sha_cache::clear_all(repo)? + preview_cache::clear_all(repo)?, + "git commands cache entry", + "git commands cache entries", + )) } fn clear_vars_reported(repo: &Repository) -> anyhow::Result { - let cleared = clear_all_vars(repo)?; - if cleared > 0 { - eprintln!( - "{}", - success_message(cformat!( - "Cleared {cleared} variable{}", - if cleared == 1 { "" } else { "s" } - )) - ); - return Ok(true); - } - Ok(false) + Ok(report_cleared_count( + clear_all_vars(repo)?, + "variable", + "variables", + )) } fn clear_logs_reported(repo: &Repository) -> anyhow::Result { - let cleared = clear_logs(repo)?; - if cleared > 0 { - eprintln!( - "{}", - success_message(cformat!( - "Cleared {cleared} log file{}", - if cleared == 1 { "" } else { "s" } - )) - ); - return Ok(true); - } - Ok(false) + Ok(report_cleared_count( + clear_logs(repo)?, + "log file", + "log files", + )) } fn clear_hints_reported(repo: &Repository) -> anyhow::Result { - let cleared = repo.clear_all_hints()?; - if cleared > 0 { - eprintln!( - "{}", - success_message(cformat!( - "Cleared {cleared} hint{}", - if cleared == 1 { "" } else { "s" } - )) - ); - return Ok(true); - } - Ok(false) + Ok(report_cleared_count( + repo.clear_all_hints()?, + "hint", + "hints", + )) } fn clear_trash_reported(repo: &Repository) -> anyhow::Result { - let cleared = clear_trash(repo)?; - if cleared > 0 { - eprintln!( - "{}", - success_message(cformat!( - "Cleared {cleared} trash entr{}", - if cleared == 1 { "y" } else { "ies" } - )) - ); - return Ok(true); - } - Ok(false) + Ok(report_cleared_count( + clear_trash(repo)?, + "trash entry", + "trash entries", + )) } // ==================== State Show Commands ==================== @@ -1719,49 +1675,50 @@ pub fn handle_vars_clear( None => repo.require_current_branch("clear variable for current branch")?, }; - if !all && key.is_none() { - anyhow::bail!("Specify a key to clear, or use --all to clear all keys"); - } - - if all { - let entries: Vec<_> = repo.vars_entries(&branch_name).into_iter().collect(); - if entries.is_empty() { - eprintln!( - "{}", - info_message(cformat!("No variables for {branch_name}")) - ); - } else { - let count = entries.len(); - for (key, _) in entries { - let config_key = format!("worktrunk.state.{branch_name}.vars.{key}"); - repo.unset_config(&config_key)?; + match (all, key) { + (false, None) => { + anyhow::bail!("Specify a key to clear, or use --all to clear all keys"); + } + (true, _) => { + let entries: Vec<_> = repo.vars_entries(&branch_name).into_iter().collect(); + if entries.is_empty() { + eprintln!( + "{}", + info_message(cformat!("No variables for {branch_name}")) + ); + } else { + let count = entries.len(); + for (key, _) in entries { + let config_key = format!("worktrunk.state.{branch_name}.vars.{key}"); + repo.unset_config(&config_key)?; + } + eprintln!( + "{}", + success_message(cformat!( + "Cleared {count} variable{} for {branch_name}", + if count == 1 { "" } else { "s" } + )) + ); } - eprintln!( - "{}", - success_message(cformat!( - "Cleared {count} variable{} for {branch_name}", - if count == 1 { "" } else { "s" } - )) - ); } - } else { - let key = key.expect("key required when --all not set"); - validate_vars_key(key)?; - let config_key = format!("worktrunk.state.{branch_name}.vars.{key}"); - if repo.unset_config(&config_key)? { - eprintln!( - "{}", - success_message(cformat!( - "Cleared {key} for {branch_name}" - )) - ); - } else { - eprintln!( - "{}", - info_message(cformat!( - "No variable {key} for {branch_name}" - )) - ); + (false, Some(key)) => { + validate_vars_key(key)?; + let config_key = format!("worktrunk.state.{branch_name}.vars.{key}"); + if repo.unset_config(&config_key)? { + eprintln!( + "{}", + success_message(cformat!( + "Cleared {key} for {branch_name}" + )) + ); + } else { + eprintln!( + "{}", + info_message(cformat!( + "No variable {key} for {branch_name}" + )) + ); + } } } Ok(()) @@ -1775,7 +1732,11 @@ pub fn handle_vars_clear( /// `unset_config` call propagate errors so user-initiated clears never lie /// about success. fn clear_all_markers(repo: &Repository) -> anyhow::Result { - let output = repo.get_config_regexp(r"^worktrunk\.state\..+\.marker$")?; + clear_matching_config(repo, r"^worktrunk\.state\..+\.marker$") +} + +fn clear_matching_config(repo: &Repository, pattern: &str) -> anyhow::Result { + let output = repo.get_config_regexp(pattern)?; let mut cleared = 0; for line in output.lines() { if let Some(config_key) = line.split_whitespace().next() { @@ -1792,15 +1753,7 @@ fn clear_all_markers(repo: &Repository) -> anyhow::Result { /// config read failure surfaces as an error — the display-path helper /// absorbs errors as empty, which would silently report "cleared 0" here. fn clear_all_vars(repo: &Repository) -> anyhow::Result { - let output = repo.get_config_regexp(r"^worktrunk\.state\..+\.vars\.")?; - let mut cleared = 0; - for line in output.lines() { - if let Some(config_key) = line.split_whitespace().next() { - repo.unset_config(config_key)?; - cleared += 1; - } - } - Ok(cleared) + clear_matching_config(repo, r"^worktrunk\.state\..+\.vars\.") } // ==================== Marker Helpers ==================== diff --git a/src/commands/configure_shell.rs b/src/commands/configure_shell.rs index b449bfbeda..43515d0d68 100644 --- a/src/commands/configure_shell.rs +++ b/src/commands/configure_shell.rs @@ -1,6 +1,6 @@ use std::collections::HashSet; -use std::fs::{self, OpenOptions}; -use std::io::{self, BufRead, BufReader, Write}; +use std::fs; +use std::io::{self, Write}; use std::path::{Path, PathBuf}; use anstyle::Style; @@ -25,7 +25,6 @@ pub struct ConfigureResult { pub struct UninstallResult { pub shell: Shell, pub path: PathBuf, - pub action: UninstallAction, /// Path that replaces this one (for deprecated location cleanup) pub superseded_by: Option, /// The lines the action applies to, for a file worktrunk edits in place @@ -36,21 +35,81 @@ pub struct UninstallResult { /// one command that acts on it destructively. Empty for the fish/nushell /// wrappers, whole files worktrunk owns and the path already names. pub matched_lines: Vec, + preimage: UninstallPreimage, } pub struct UninstallScanResult { pub results: Vec, - pub completion_results: Vec, + pub completion_results: Vec, /// Shell extensions not found (bash/zsh show as "integration", fish as "shell extension") pub not_found: Vec<(Shell, PathBuf)>, /// Completion files not found (only fish has separate completion files) pub completion_not_found: Vec<(Shell, PathBuf)>, } -pub struct CompletionUninstallResult { - pub shell: Shell, - pub path: PathBuf, - pub action: UninstallAction, +enum UninstallPreimage { + WholeFile(Vec), + RcLines(Vec), +} + +impl UninstallResult { + fn apply(mut self) -> Result, String> { + let changed = match &self.preimage { + UninstallPreimage::WholeFile(expected) => remove_config_file(&self.path, expected)?, + UninstallPreimage::RcLines(lines) => { + self.matched_lines = uninstall_previewed_lines(&self.path, lines)?; + !self.matched_lines.is_empty() + } + }; + Ok(changed.then_some(self)) + } +} + +impl UninstallScanResult { + fn apply(mut self) -> Result { + let apply = |results: Vec| { + let mut applied = Vec::new(); + let mut not_found = Vec::new(); + for result in results { + let missing = (result.shell, result.path.clone()); + match result.apply()? { + Some(result) => applied.push(result), + None => not_found.push(missing), + } + } + Ok::<_, String>((applied, not_found)) + }; + let (results, not_found) = apply(self.results)?; + self.results = results; + self.not_found.extend(not_found); + let (completion_results, completion_not_found) = apply(self.completion_results)?; + self.completion_results = completion_results; + self.completion_not_found.extend(completion_not_found); + + Ok(self) + } +} + +fn remove_config_file(path: &Path, expected: &[u8]) -> Result { + match fs::read(path) { + Ok(current) if current != expected => Err(format!( + "Shell integration changed after preview @ {}; run the command again", + format_path_for_display(path) + )), + Ok(_) => match fs::remove_file(path) { + Ok(()) => Ok(true), + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(false), + Err(error) => Err(format!( + "Failed to remove {}: {error}", + format_path_for_display(path) + )), + }, + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(false), + Err(error) => Err(format!( + "Failed to verify shell integration @ {}: {error}", + format_path_for_display(path) + )), + } } pub struct ScanResult { @@ -71,28 +130,8 @@ pub struct CompletionResult { pub shell: Shell, pub path: PathBuf, pub action: ConfigAction, -} - -#[derive(Debug, PartialEq)] -pub enum UninstallAction { - Removed, - WouldRemove, -} - -impl UninstallAction { - pub fn description(&self) -> &str { - match self { - UninstallAction::Removed => "Removed", - UninstallAction::WouldRemove => "Will remove", - } - } - - pub fn symbol(&self) -> &'static str { - match self { - UninstallAction::Removed => SUCCESS_SYMBOL, - UninstallAction::WouldRemove => INFO_SYMBOL, - } - } + /// Exact preview-time contents; `None` means the file did not exist. + preimage: Option>, } #[derive(Debug, PartialEq)] @@ -319,31 +358,14 @@ pub fn handle_configure_shell( shell::validate_shell_command_name(&cmd)?; // First, do a dry-run to see what would be changed - let preview = scan_shell_configs(shell_filter, true, &cmd)?; - - // Preview completions that would be written - let shells: Vec<_> = preview.configured.iter().map(|r| r.shell).collect(); - let completion_preview = process_shell_completions(&shells, true, &cmd)?; + let mut preview = scan_shell_configs(shell_filter, true, &cmd)?; // If nothing to do, return early if preview.configured.is_empty() { - return Ok(ScanResult { - configured: preview.configured, - completion_results: completion_preview, - skipped: preview.skipped, - zsh_needs_compinit: false, - legacy_cleanups: Vec::new(), - }); + return Ok(preview); } - // Check if any changes are needed (not all are AlreadyExists) - let needs_shell_changes = preview - .configured - .iter() - .any(|r| !matches!(r.action, ConfigAction::AlreadyExists)); - let needs_completion_changes = completion_preview - .iter() - .any(|r| !matches!(r.action, ConfigAction::AlreadyExists)); + let needs_changes = install_changes_needed(&preview); // Detect (without removing) the legacy files a real install would delete, so // both the --dry-run preview and the confirmation prompt can name them before @@ -355,75 +377,65 @@ pub fn handle_configure_shell( if dry_run { let preview_text = show_install_preview( &preview.configured, - &completion_preview, + &preview.completion_results, &legacy_preview, &cmd, ); if !preview_text.is_empty() { println!("{preview_text}"); } - return Ok(ScanResult { - configured: preview.configured, - completion_results: completion_preview, - skipped: preview.skipped, - zsh_needs_compinit: false, - legacy_cleanups: legacy_preview, - }); + preview.legacy_cleanups = legacy_preview; + return Ok(preview); } - // If nothing needs to be changed, there may still be legacy files to clean up - // (a user who upgraded and has both functions/wt.fish and conf.d/wt.fish). That - // removal is destructive, so gate it behind the same confirmation as an install - // — never delete a hand-written file as a silent side effect (issue #3644). - if !needs_shell_changes && !needs_completion_changes { - if legacy_preview.is_empty() { - return Ok(ScanResult { - configured: preview.configured, - completion_results: completion_preview, - skipped: preview.skipped, - zsh_needs_compinit: false, - legacy_cleanups: Vec::new(), - }); - } - - if !skip_confirmation - && !prompt_for_install( - &preview.configured, - &completion_preview, - &legacy_preview, - &cmd, - "Remove deprecated shell integration files?", - )? - { - return Err("Cancelled by user".to_string()); - } - - let legacy_cleanups = collect_legacy_cleanups(&preview.configured, &cmd, false); - return Ok(ScanResult { - configured: preview.configured, - completion_results: completion_preview, - skipped: preview.skipped, - zsh_needs_compinit: false, - legacy_cleanups, - }); + if !needs_changes && legacy_preview.is_empty() { + return Ok(preview); } - // Show what will be done and ask for confirmation (unless --yes flag is used) + let prompt = if needs_changes { + "Install shell integration?" + } else { + "Remove deprecated shell integration files?" + }; if !skip_confirmation && !prompt_for_install( &preview.configured, - &completion_preview, + &preview.completion_results, &legacy_preview, &cmd, - "Install shell integration?", + prompt, )? { return Err("Cancelled by user".to_string()); } - // User confirmed (or --yes flag was used), now actually apply the changes - let result = scan_shell_configs(shell_filter, false, &cmd)?; - let completion_results = process_shell_completions(&shells, false, &cmd)?; + apply_confirmed_shell_config(preview, shell_filter, &cmd) +} + +fn install_changes_needed(scan: &ScanResult) -> bool { + scan.configured + .iter() + .any(|result| !matches!(result.action, ConfigAction::AlreadyExists)) + || scan + .completion_results + .iter() + .any(|result| !matches!(result.action, ConfigAction::AlreadyExists)) +} + +/// Apply a shell configuration plan that has already been confirmed. +pub(crate) fn apply_confirmed_shell_config( + mut preview: ScanResult, + shell_filter: Option, + cmd: &str, +) -> Result { + if !install_changes_needed(&preview) { + preview.legacy_cleanups = collect_legacy_cleanups(&preview.configured, cmd, false); + return Ok(preview); + } + + let mut result = scan_shell_configs(shell_filter, false, cmd)?; + result.completion_results = + apply_shell_completions(preview.completion_results, &result.configured, cmd)?; // Zsh completions require compinit to be enabled. Unlike bash/fish, zsh doesn't // enable its completion system by default - users must explicitly call compinit. @@ -462,15 +474,9 @@ pub fn handle_configure_shell( // (issue #566), plus any nushell wrapper stranded at a legacy autoload // location (issue #2878). The confirmation above listed these removals // (issue #3644). - let legacy_cleanups = collect_legacy_cleanups(&result.configured, &cmd, false); - - Ok(ScanResult { - configured: result.configured, - completion_results, - skipped: result.skipped, - zsh_needs_compinit, - legacy_cleanups, - }) + result.zsh_needs_compinit = zsh_needs_compinit; + result.legacy_cleanups = collect_legacy_cleanups(&result.configured, cmd, false); + Ok(result) } /// Check if we should auto-configure PowerShell profiles. @@ -606,9 +612,16 @@ pub fn scan_shell_configs( } } + let completion_results = if dry_run { + let configured_shells: Vec<_> = results.iter().map(|result| result.shell).collect(); + preview_shell_completions(&configured_shells, cmd)? + } else { + Vec::new() + }; + Ok(ScanResult { configured: results, - completion_results: Vec::new(), // Completions handled separately in handle_configure_shell + completion_results, skipped, zsh_needs_compinit: false, // Caller handles compinit detection legacy_cleanups: Vec::new(), // Caller handles legacy cleanup @@ -643,30 +656,28 @@ fn configure_shell_file( // For other shells, check if file exists if path.exists() { // Read the file and check if our integration already exists - let file = fs::File::open(path) + let content = fs::read(path) .map_err(|e| format!("Failed to read {}: {}", format_path_for_display(path), e))?; - let reader = BufReader::new(file); - // Check for the canonical line and older/manual forms for this shell. - for line in reader.lines() { - let line = line.map_err(|e| { + for line in content.split(|byte| *byte == b'\n') { + let line = std::str::from_utf8(line).map_err(|e| { format!( - "Failed to read line from {}: {}", - format_path_for_display(path), - e + "Failed to read line from {}: {e}", + format_path_for_display(path) ) })?; - - if is_install_shell_integration_line(&line, shell, cmd) { + if is_install_shell_integration_line(line.trim_end_matches('\r'), shell, cmd) { return Ok(Some(ConfigureResult { shell, path: path.to_path_buf(), action: ConfigAction::AlreadyExists, - config_line: config_line.clone(), + config_line, })); } } + let mut content = String::from_utf8(content) + .map_err(|e| format!("Failed to read {}: {e}", format_path_for_display(path)))?; // Line doesn't exist, add it if dry_run { @@ -674,21 +685,15 @@ fn configure_shell_file( shell, path: path.to_path_buf(), action: ConfigAction::WouldAdd, - config_line: config_line.clone(), + config_line, })); } - // Append the line with proper spacing - let mut file = OpenOptions::new().append(true).open(path).map_err(|e| { - format!( - "Failed to open {} for writing: {}", - format_path_for_display(path), - e - ) - })?; - // Add blank line before config, then the config line with its own newline - write!(file, "\n{}\n", config_line).map_err(|e| { + content.push('\n'); + content.push_str(&config_line); + content.push('\n'); + write_atomically(path, &content).map_err(|e| { format!( "Failed to write to {}: {}", format_path_for_display(path), @@ -700,7 +705,7 @@ fn configure_shell_file( shell, path: path.to_path_buf(), action: ConfigAction::Added, - config_line: config_line.clone(), + config_line, })) } else { // File doesn't exist @@ -711,7 +716,7 @@ fn configure_shell_file( shell, path: path.to_path_buf(), action: ConfigAction::WouldCreate, - config_line: config_line.clone(), + config_line, })); } @@ -739,7 +744,7 @@ fn configure_shell_file( shell, path: path.to_path_buf(), action: ConfigAction::Created, - config_line: config_line.clone(), + config_line, })) } else { // Don't create config files for shells the user might not use @@ -957,7 +962,7 @@ pub fn show_install_preview( /// narration and goes to stderr. See /writing-user-outputs. pub fn show_uninstall_preview( results: &[UninstallResult], - completion_results: &[CompletionUninstallResult], + completion_results: &[UninstallResult], ) -> String { let bold = Style::new().bold(); let mut lines: Vec = Vec::new(); @@ -970,16 +975,13 @@ pub fn show_uninstall_preview( if let Some(canonical) = &result.superseded_by { let canonical_path = format_path_for_display(canonical); lines.push(format!( - "{INFO_SYMBOL} {} {bold}{path}{bold:#} (deprecated; now using {bold}{canonical_path}{bold:#})", - result.action.description(), + "{INFO_SYMBOL} Will remove {bold}{path}{bold:#} (deprecated; now using {bold}{canonical_path}{bold:#})", )); } else { let what = shell_extension_label(shell); lines.push(format!( - "{} {} {what} for {bold}{shell}{bold:#} @ {bold}{path}{bold:#}{}", - result.action.symbol(), - result.action.description(), + "{INFO_SYMBOL} Will remove {what} for {bold}{shell}{bold:#} @ {bold}{path}{bold:#}{}", format_matched_lines(&result.matched_lines), )); } @@ -990,9 +992,7 @@ pub fn show_uninstall_preview( let path = format_path_for_display(&result.path); lines.push(format!( - "{} {} completions for {bold}{shell}{bold:#} @ {bold}{path}{bold:#}", - result.action.symbol(), - result.action.description(), + "{INFO_SYMBOL} Will remove completions for {bold}{shell}{bold:#} @ {bold}{path}{bold:#}", )); } @@ -1062,16 +1062,12 @@ complete --keep-order --exclusive --command {cmd} --arguments "(test -n \"\$WORK ) } -/// Process shell completions - either preview or write based on dry_run flag +/// Preview the shell completion files authorized by the install prompt. /// /// Note: Bash and Zsh use inline lazy completions in the init script. /// Fish uses a separate completion file at ~/.config/fish/completions/{cmd}.fish /// that finds the command in PATH (with WORKTRUNK_BIN as optional override) to bypass the shell wrapper. -pub fn process_shell_completions( - shells: &[Shell], - dry_run: bool, - cmd: &str, -) -> Result, String> { +fn preview_shell_completions(shells: &[Shell], cmd: &str) -> Result, String> { let mut results = Vec::new(); let fish_completion = fish_completion_content(cmd); @@ -1085,35 +1081,61 @@ pub fn process_shell_completions( .completion_path(cmd) .map_err(|e| format!("Failed to get completion path for {shell}: {e}"))?; - // Check if completions already exist with correct content - // Read errors (including not-found) fall through to "not configured" - if let Ok(existing) = fs::read_to_string(&completion_path) - && existing == fish_completion - { - results.push(CompletionResult { - shell, - path: completion_path, - action: ConfigAction::AlreadyExists, - }); + let (action, preimage) = match fs::read(&completion_path) { + Ok(existing) => { + let action = if existing == fish_completion.as_bytes() { + ConfigAction::AlreadyExists + } else { + ConfigAction::WouldAdd + }; + (action, Some(existing)) + } + Err(error) if error.kind() == io::ErrorKind::NotFound => { + (ConfigAction::WouldCreate, None) + } + Err(error) => { + return Err(format!( + "Failed to read {}: {error}", + format_path_for_display(&completion_path) + )); + } + }; + + results.push(CompletionResult { + shell, + path: completion_path, + action, + preimage, + }); + } + + Ok(results) +} + +/// Apply only completion writes authorized by the preview and still paired +/// with a shell extension in the apply-time scan. +fn apply_shell_completions( + preview: Vec, + configured: &[ConfigureResult], + cmd: &str, +) -> Result, String> { + let mut results = Vec::new(); + let fish_completion = fish_completion_content(cmd); + + for mut result in preview { + if !configured.iter().any(|entry| entry.shell == result.shell) { continue; } - - if dry_run { - let action = if completion_path.exists() { - ConfigAction::WouldAdd - } else { - ConfigAction::WouldCreate - }; - results.push(CompletionResult { - shell, - path: completion_path, - action, - }); + if matches!(result.action, ConfigAction::AlreadyExists) { + // No write was previewed, so concurrent changes win without validation. + results.push(result); continue; } + verify_completion_preimage(&result.path, result.preimage.as_deref())?; + // Create parent directory if needed - if let Some(parent) = completion_path.parent() { + if let Some(parent) = result.path.parent() { fs::create_dir_all(parent).map_err(|e| { format!( "Failed to create directory {}: {e}", @@ -1121,32 +1143,50 @@ pub fn process_shell_completions( ) })?; } + verify_completion_preimage(&result.path, result.preimage.as_deref())?; // Write the completion file - write_atomically(&completion_path, &fish_completion).map_err(|e| { + write_atomically(&result.path, &fish_completion).map_err(|e| { format!( "Failed to write {}: {e}", - format_path_for_display(&completion_path) + format_path_for_display(&result.path) ) })?; - results.push(CompletionResult { - shell, - path: completion_path, - action: ConfigAction::Created, - }); + result.action = ConfigAction::Created; + results.push(result); } Ok(results) } +fn verify_completion_preimage(path: &Path, expected: Option<&[u8]>) -> Result<(), String> { + let current = match fs::read(path) { + Ok(content) => Some(content), + Err(error) if error.kind() == io::ErrorKind::NotFound => None, + Err(error) => { + return Err(format!( + "Failed to verify shell completions @ {}: {error}", + format_path_for_display(path) + )); + } + }; + if current.as_deref() == expected { + Ok(()) + } else { + Err(format!( + "Shell completions changed after preview @ {}; run the command again", + format_path_for_display(path) + )) + } +} + pub fn handle_unconfigure_shell( shell_filter: Option, skip_confirmation: bool, dry_run: bool, ) -> Result { - // First, do a dry-run to see what would be changed - let preview = scan_for_uninstall(shell_filter, true)?; + let preview = scan_for_uninstall(shell_filter)?; // If nothing to do, return early if preview.results.is_empty() && preview.completion_results.is_empty() { @@ -1174,14 +1214,9 @@ pub fn handle_unconfigure_shell( return Err("Cancelled by user".to_string()); } - // User confirmed (or --yes flag was used), now actually apply the changes - scan_for_uninstall(shell_filter, false) -} - -/// Remove a config file with a context-rich error message. -fn remove_config_file(path: &std::path::Path) -> Result<(), String> { - fs::remove_file(path) - .map_err(|e| format!("Failed to remove {}: {e}", format_path_for_display(path))) + // User confirmed (or --yes flag was used), so apply exactly the paths and + // rc-line multiplicities represented by the preview above. + preview.apply() } /// Uninstall scans the shell-owned directories and removes every worktrunk-managed @@ -1193,10 +1228,7 @@ fn remove_config_file(path: &std::path::Path) -> Result<(), String> { /// completion header for `completions/`). For Bash/Zsh/PowerShell (line-based), /// it scans the rc/profile files and uses `is_shell_integration_line_for_uninstall_any_cmd`. /// No `--cmd` is needed because the marker is the content, not the file name. -fn scan_for_uninstall( - shell_filter: Option, - dry_run: bool, -) -> Result { +fn scan_for_uninstall(shell_filter: Option) -> Result { // For uninstall, scan every shell (Shell::all includes PowerShell) to clean // up any existing profiles. let default_shells = Shell::all(); @@ -1220,36 +1252,24 @@ fn scan_for_uninstall( let legacy = scan_managed_files(&confd_dir, "fish", is_worktrunk_managed_content)?; let found_any = !canonical.is_empty() || !legacy.is_empty(); - for path in &canonical { - let action = if dry_run { - UninstallAction::WouldRemove - } else { - remove_config_file(path)?; - UninstallAction::Removed - }; + for (path, content) in &canonical { results.push(UninstallResult { shell, path: path.clone(), - action, superseded_by: None, matched_lines: Vec::new(), + preimage: UninstallPreimage::WholeFile(content.clone()), }); } - for path in &legacy { + for (path, content) in &legacy { let superseded_by = path.file_name().map(|n| functions_dir.join(n)); - let action = if dry_run { - UninstallAction::WouldRemove - } else { - remove_config_file(path)?; - UninstallAction::Removed - }; results.push(UninstallResult { shell, path: path.clone(), - action, superseded_by, matched_lines: Vec::new(), + preimage: UninstallPreimage::WholeFile(content.clone()), }); } @@ -1264,20 +1284,14 @@ fn scan_for_uninstall( for autoload_dir in &candidates { let nu_files = scan_managed_files(autoload_dir, "nu", is_worktrunk_managed_content)?; - for path in &nu_files { + for (path, content) in &nu_files { found_any = true; - let action = if dry_run { - UninstallAction::WouldRemove - } else { - remove_config_file(path)?; - UninstallAction::Removed - }; results.push(UninstallResult { shell, path: path.clone(), - action, superseded_by: None, matched_lines: Vec::new(), + preimage: UninstallPreimage::WholeFile(content.clone()), }); } } @@ -1298,7 +1312,7 @@ fn scan_for_uninstall( continue; } - if let Some(result) = uninstall_from_file(shell, path, dry_run)? { + if let Some(result) = scan_rc_file_for_uninstall(shell, path)? { results.push(result); found = true; } @@ -1323,17 +1337,13 @@ fn scan_for_uninstall( if completions.is_empty() { completion_not_found.push((Shell::Fish, completions_dir)); } - for path in completions { - let action = if dry_run { - UninstallAction::WouldRemove - } else { - remove_config_file(&path)?; - UninstallAction::Removed - }; - completion_results.push(CompletionUninstallResult { + for (path, content) in completions { + completion_results.push(UninstallResult { shell: Shell::Fish, path, - action, + superseded_by: None, + matched_lines: Vec::new(), + preimage: UninstallPreimage::WholeFile(content), }); } } @@ -1352,7 +1362,7 @@ fn scan_managed_files( dir: &Path, extension: &str, is_managed: fn(&str) -> bool, -) -> Result, String> { +) -> Result)>, String> { if !dir.exists() { return Ok(Vec::new()); } @@ -1369,22 +1379,21 @@ fn scan_managed_files( if !path.is_file() { continue; } - let content = match fs::read_to_string(&path) { - Ok(s) => s, + let content = match fs::read(&path) { + Ok(content) => content, Err(_) => continue, // unreadable file: skip, don't fail the whole uninstall }; - if is_managed(&content) { - out.push(path); + if std::str::from_utf8(&content).is_ok_and(is_managed) { + out.push((path, content)); } } - out.sort(); + out.sort_by(|(left, _), (right, _)| left.cmp(right)); Ok(out) } -fn uninstall_from_file( +fn scan_rc_file_for_uninstall( shell: Shell, path: &Path, - dry_run: bool, ) -> Result, String> { let content = fs::read_to_string(path) .map_err(|e| format!("Failed to read {}: {}", format_path_for_display(path), e))?; @@ -1401,54 +1410,60 @@ fn uninstall_from_file( return Ok(None); } - let matched_lines: Vec = integration_lines + let raw_lines: Vec = integration_lines .iter() - .map(|(_, line)| line.trim().to_string()) + .map(|(_, line)| (*line).to_string()) + .collect(); + let matched_lines = raw_lines + .iter() + .map(|line| line.trim().to_string()) .collect(); - if dry_run { - return Ok(Some(UninstallResult { - shell, - path: path.to_path_buf(), - action: UninstallAction::WouldRemove, - superseded_by: None, - matched_lines, - })); - } + Ok(Some(UninstallResult { + shell, + path: path.to_path_buf(), + superseded_by: None, + matched_lines, + preimage: UninstallPreimage::RcLines(raw_lines), + })) +} + +fn uninstall_previewed_lines( + path: &Path, + previewed_lines: &[String], +) -> Result, String> { + let content = fs::read_to_string(path) + .map_err(|e| format!("Failed to read {}: {}", format_path_for_display(path), e))?; + let lines: Vec<&str> = content.split_inclusive('\n').collect(); + let mut remaining = previewed_lines.to_vec(); + let mut removed = Vec::new(); // Remove matching lines and any immediately preceding blank line // (install adds "\n{line}\n", so we remove both the blank and the integration line) - let mut indices_to_remove: HashSet = integration_lines.iter().map(|(i, _)| *i).collect(); - for &(i, _) in &integration_lines { - if i > 0 && lines[i - 1].trim().is_empty() { - indices_to_remove.insert(i - 1); + let mut indices_to_remove = HashSet::new(); + for (index, line) in lines.iter().enumerate() { + let line = line.trim_end_matches(['\r', '\n']); + if let Some(position) = remaining.iter().position(|candidate| candidate == line) { + removed.push(remaining.remove(position).trim().to_string()); + indices_to_remove.insert(index); + if index > 0 && lines[index - 1].trim().is_empty() { + indices_to_remove.insert(index - 1); + } } } - let new_lines: Vec<&str> = lines + if indices_to_remove.is_empty() { + return Ok(removed); + } + let new_content: String = lines .iter() .enumerate() .filter(|(i, _)| !indices_to_remove.contains(i)) .map(|(_, line)| *line) .collect(); - let new_content = new_lines.join("\n"); - // Preserve trailing newline if original had one - let new_content = if content.ends_with('\n') { - format!("{}\n", new_content) - } else { - new_content - }; - write_atomically(path, &new_content) .map_err(|e| format!("Failed to write {}: {e}", format_path_for_display(path)))?; - - Ok(Some(UninstallResult { - shell, - path: path.to_path_buf(), - action: UninstallAction::Removed, - superseded_by: None, - matched_lines, - })) + Ok(removed) } /// Show what uninstall would take, then ask. @@ -1459,7 +1474,7 @@ fn uninstall_from_file( /// the `--dry-run` answer on stdout. See /writing-user-outputs. fn prompt_for_uninstall_confirmation( results: &[UninstallResult], - completion_results: &[CompletionUninstallResult], + completion_results: &[UninstallResult], ) -> Result { eprintln!("{}", show_uninstall_preview(results, completion_results)); @@ -1608,18 +1623,6 @@ pub fn handle_show_theme() { mod tests { use super::*; - #[test] - fn test_uninstall_action_description() { - assert_eq!(UninstallAction::Removed.description(), "Removed"); - assert_eq!(UninstallAction::WouldRemove.description(), "Will remove"); - } - - #[test] - fn test_uninstall_action_emoji() { - assert_eq!(UninstallAction::Removed.symbol(), SUCCESS_SYMBOL); - assert_eq!(UninstallAction::WouldRemove.symbol(), INFO_SYMBOL); - } - #[test] fn test_config_action_description() { assert_eq!(ConfigAction::Added.description(), "Added"); @@ -1698,6 +1701,65 @@ mod tests { insta::assert_snapshot!(fish_completion_content("myapp")); } + #[test] + fn test_verify_completion_preimage_rejects_changed_state() { + let previewed = b"# previewed completion\n".as_slice(); + for (name, expected, current) in [ + ("created", None, Some(b"# user completion\r\n".as_slice())), + ("replaced", Some(previewed), Some(b"user\r\n".as_slice())), + ("deleted", Some(previewed), None), + ] { + let dir = tempfile::TempDir::new().unwrap(); + let completion = dir.path().join("wt.fish"); + if let Some(content) = current { + fs::write(&completion, content).unwrap(); + } + + let error = verify_completion_preimage(&completion, expected).unwrap_err(); + assert!(error.contains("changed after preview"), "{name}: {error}"); + assert_eq!(fs::read(&completion).ok().as_deref(), current, "{name}"); + } + } + + #[test] + fn test_apply_shell_completions_verifies_before_creating_parent() { + let dir = tempfile::TempDir::new().unwrap(); + let completion = dir.path().join("removed/wt.fish"); + let preview = CompletionResult { + shell: Shell::Fish, + path: completion.clone(), + action: ConfigAction::WouldAdd, + preimage: Some(b"# previewed completion\n".to_vec()), + }; + let configured = ConfigureResult { + shell: Shell::Fish, + path: dir.path().join("functions/wt.fish"), + action: ConfigAction::Created, + config_line: String::new(), + }; + + let result = apply_shell_completions(vec![preview], &[configured], "wt"); + + assert!(result.is_err(), "missing preimage should be rejected"); + assert!(!completion.parent().unwrap().exists()); + } + + #[test] + fn test_apply_shell_completions_skips_shell_missing_from_apply_scan() { + let dir = tempfile::TempDir::new().unwrap(); + let completion = dir.path().join("wt.fish"); + let preview = CompletionResult { + shell: Shell::Fish, + path: completion.clone(), + action: ConfigAction::WouldCreate, + preimage: None, + }; + + apply_shell_completions(vec![preview], &[], "wt").unwrap(); + + assert!(!completion.exists()); + } + // Note: should_auto_configure_powershell() is tested via WORKTRUNK_TEST_POWERSHELL_ENV // override in tests/integration_tests/configure_shell.rs. @@ -1771,20 +1833,130 @@ mod tests { ); } + #[test] + fn test_remove_config_file_requires_matching_preimage() { + let dir = tempfile::TempDir::new().unwrap(); + let wrapper = dir.path().join("wt.fish"); + let previewed = b"# worktrunk shell integration for fish\n"; + fs::write(&wrapper, "# user replacement\n").unwrap(); + + let error = remove_config_file(&wrapper, previewed).unwrap_err(); + + assert!(error.contains("changed after preview"), "{error}"); + assert_eq!(fs::read(&wrapper).unwrap(), b"# user replacement\n"); + + fs::write(&wrapper, previewed).unwrap(); + assert!(remove_config_file(&wrapper, previewed).unwrap()); + assert!(!wrapper.exists()); + assert!(!remove_config_file(&wrapper, previewed).unwrap()); + } + + #[test] + fn test_preimage_verification_rejects_directories() { + let dir = tempfile::TempDir::new().unwrap(); + + let integration_error = remove_config_file(dir.path(), b"").unwrap_err(); + let completion_error = verify_completion_preimage(dir.path(), None).unwrap_err(); + + assert!( + integration_error.contains("Failed to verify shell integration"), + "{integration_error}" + ); + assert!( + completion_error.contains("Failed to verify shell completions"), + "{completion_error}" + ); + } + + #[test] + fn test_configure_shell_rejects_invalid_utf8_before_modifying_file() { + let dir = tempfile::TempDir::new().unwrap(); + let rc = dir.path().join(".zshrc"); + let content = b"# non-UTF-8: \xff\n"; + fs::write(&rc, content).unwrap(); + + let error = configure_shell_file(Shell::Zsh, &rc, false, false, "wt") + .err() + .expect("invalid UTF-8 should be rejected"); + + assert!(error.contains("Failed to read line"), "{error}"); + assert_eq!(fs::read(rc).unwrap(), content); + } + + #[test] + fn test_configure_shell_already_exists_before_invalid_utf8() { + let dir = tempfile::TempDir::new().unwrap(); + let rc = dir.path().join(".zshrc"); + let content = b"eval \"$(wt config shell init zsh)\"\n# non-UTF-8: \xff\n"; + fs::write(&rc, content).unwrap(); + + let result = configure_shell_file(Shell::Zsh, &rc, false, false, "wt").unwrap(); + + assert_eq!(result.unwrap().action, ConfigAction::AlreadyExists); + assert_eq!(fs::read(&rc).unwrap(), content); + } + + #[test] + fn test_uninstall_previewed_lines_preserves_mixed_line_endings() { + let dir = tempfile::TempDir::new().unwrap(); + let rc = dir.path().join("profile.ps1"); + let integration = "Invoke-Expression (& wt config shell init powershell)"; + fs::write( + &rc, + format!( + "$env:EDITOR = 'hx'\r\n{integration}\nSet-Alias ll Get-ChildItem\r\n$env:PAGER = 'less'\n" + ), + ) + .unwrap(); + + let removed = + uninstall_previewed_lines(&rc, &[integration.to_owned(), integration.to_owned()]) + .unwrap(); + + assert_eq!(removed, [integration]); + assert_eq!( + fs::read(&rc).unwrap(), + b"$env:EDITOR = 'hx'\r\nSet-Alias ll Get-ChildItem\r\n$env:PAGER = 'less'\n" + ); + } + /// An rc file with one integration line and content the user owns on either /// side of it. Unix-only, like the atomic-rewrite tests that use it: mode /// and symlink semantics have no Windows equivalent. + #[cfg(unix)] + const RC_INTEGRATION: &str = "eval \"$(wt config shell init bash)\""; + #[cfg(unix)] + const RC_AFTER_UNINSTALL: &str = "export EDITOR=hx\nalias ll='ls -l'\n"; + #[cfg(unix)] fn write_rc(path: &Path) { fs::write( path, - "export EDITOR=hx\n\neval \"$(wt config shell init bash)\"\nalias ll='ls -l'\n", + format!("export EDITOR=hx\n\n{RC_INTEGRATION}\nalias ll='ls -l'\n"), ) .unwrap(); } #[cfg(unix)] - const RC_AFTER_UNINSTALL: &str = "export EDITOR=hx\nalias ll='ls -l'\n"; + fn uninstall_rc(path: &Path) -> Result<(), String> { + uninstall_previewed_lines(path, &[RC_INTEGRATION.to_owned()]).map(|_| ()) + } + + #[cfg(unix)] + fn make_directory_unwritable(path: &Path) -> bool { + use std::os::unix::fs::PermissionsExt; + + fs::set_permissions(path, fs::Permissions::from_mode(0o500)).unwrap(); + let probe = path.join("__probe"); + if fs::write(&probe, "").is_err() { + true + } else { + fs::remove_file(probe).unwrap(); + fs::set_permissions(path, fs::Permissions::from_mode(0o700)).unwrap(); + std::eprintln!("Skipping - running with elevated privileges"); + false + } + } #[cfg(unix)] #[test] @@ -1797,9 +1969,7 @@ mod tests { // is created with, so an uncopied mode shows up as a diff. fs::set_permissions(&rc, fs::Permissions::from_mode(0o644)).unwrap(); - uninstall_from_file(Shell::Bash, &rc, false) - .unwrap() - .unwrap(); + uninstall_rc(&rc).unwrap(); assert_eq!(fs::read_to_string(&rc).unwrap(), RC_AFTER_UNINSTALL); assert_eq!( @@ -1826,9 +1996,7 @@ mod tests { write_rc(&real); std::os::unix::fs::symlink(&real, &link).unwrap(); - uninstall_from_file(Shell::Bash, &link, false) - .unwrap() - .unwrap(); + uninstall_rc(&link).unwrap(); assert!( fs::symlink_metadata(&link) @@ -1853,9 +2021,11 @@ mod tests { // Read and traverse still work, so the rewrite gets as far as creating // its temp file and fails there — the point a truncate-in-place write // would already have emptied the file. - fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o500)).unwrap(); + if !make_directory_unwritable(dir.path()) { + return; + } - let result = uninstall_from_file(Shell::Bash, &rc, false); + let result = uninstall_rc(&rc); fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o700)).unwrap(); assert!( @@ -1865,6 +2035,53 @@ mod tests { assert_eq!(fs::read_to_string(&rc).unwrap(), original); } + #[cfg(unix)] + #[test] + fn test_preimage_writes_leave_files_intact_on_io_error() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::TempDir::new().unwrap(); + let wrapper = dir.path().join("wt.fish"); + let completion = dir.path().join("completions.fish"); + let wrapper_content = b"# worktrunk shell integration for fish\n"; + let completion_content = b"# previewed completion\n"; + fs::write(&wrapper, wrapper_content).unwrap(); + fs::write(&completion, completion_content).unwrap(); + if !make_directory_unwritable(dir.path()) { + return; + } + + let remove_result = remove_config_file(&wrapper, wrapper_content); + let completion_result = apply_shell_completions( + vec![CompletionResult { + shell: Shell::Fish, + path: completion.clone(), + action: ConfigAction::WouldAdd, + preimage: Some(completion_content.to_vec()), + }], + &[ConfigureResult { + shell: Shell::Fish, + path: dir.path().join("functions/wt.fish"), + action: ConfigAction::Created, + config_line: String::new(), + }], + "wt", + ); + + fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o700)).unwrap(); + let remove_error = remove_result.unwrap_err(); + let completion_error = completion_result + .err() + .expect("read-only completion directory should reject the write"); + assert!(remove_error.contains("Failed to remove"), "{remove_error}"); + assert!( + completion_error.contains("Failed to write"), + "{completion_error}" + ); + assert_eq!(fs::read(wrapper).unwrap(), wrapper_content); + assert_eq!(fs::read(completion).unwrap(), completion_content); + } + #[cfg(unix)] #[test] fn test_scan_managed_files_skips_unreadable_file() { @@ -1874,6 +2091,11 @@ mod tests { let unreadable = root.join("locked.fish"); fs::write(&unreadable, "function wt\nend\n").unwrap(); fs::set_permissions(&unreadable, fs::Permissions::from_mode(0o000)).unwrap(); + if fs::read(&unreadable).is_ok() { + fs::set_permissions(&unreadable, fs::Permissions::from_mode(0o644)).unwrap(); + std::eprintln!("Skipping - running with elevated privileges"); + return; + } // An unreadable wrapper file is skipped, not surfaced as an error. let found = scan_managed_files(root, "fish", is_worktrunk_managed_content); // Restore perms so TempDir cleanup can remove the file. diff --git a/src/commands/hook_plan.rs b/src/commands/hook_plan.rs index df42927d83..21f0c3b5da 100644 --- a/src/commands/hook_plan.rs +++ b/src/commands/hook_plan.rs @@ -503,9 +503,9 @@ mod tests { let approvals_path = temp_dir.path().join("approvals.toml"); let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( "proj".to_string(), - "echo project-hook".to_string(), + vec!["echo project-hook".to_string()], &approvals_path, ) .unwrap(); diff --git a/src/commands/picker/mod.rs b/src/commands/picker/mod.rs index 3b09885421..bf19c21ed9 100644 --- a/src/commands/picker/mod.rs +++ b/src/commands/picker/mod.rs @@ -3863,7 +3863,7 @@ pub mod tests { let approvals_path = approvals_dir.path().join("approvals.toml"); let mut approvals = Approvals::default(); approvals - .approve_command(pid, "false".to_string(), &approvals_path) + .approve_commands(pid, vec!["false".to_string()], &approvals_path) .unwrap(); // Build the row from the git-reported worktree path, not the raw temp diff --git a/src/commands/process.rs b/src/commands/process.rs index a6d31e54e9..6eb50a47bd 100644 --- a/src/commands/process.rs +++ b/src/commands/process.rs @@ -78,16 +78,6 @@ impl HookLog { } } - /// Create a per-branch internal operation log specification. - pub fn internal(op: InternalOp) -> Self { - Self::Internal(op) - } - - /// Create a repo-wide (branch-agnostic) internal operation log specification. - pub fn shared(op: InternalOp) -> Self { - Self::Shared(op) - } - /// Generate the full log path for a branch in the given log directory. /// /// Builds the nested path under `{log_dir}/{sanitized-branch}/...` for @@ -547,7 +537,7 @@ pub fn sweep_stale_trash(repo: &Repository) { &repo.wt_dir(), &command, "", - &HookLog::shared(InternalOp::TrashSweep), + &HookLog::Shared(InternalOp::TrashSweep), None, ) { tracing::debug!(error = %e, "Failed to spawn stale trash sweep: {e}"); @@ -1015,18 +1005,18 @@ mod tests { // Per-branch internal operation path: {log_dir}/{sanitized-branch}/internal/{op}.log assert_snapshot!( - HookLog::internal(InternalOp::Remove).path(log_dir, "main").to_slash_lossy(), + HookLog::Internal(InternalOp::Remove) + .path(log_dir, "main") + .to_slash_lossy(), @"/repo/.git/wt/logs/main/internal/remove.log" ); // Repo-wide (branch-agnostic) internal operation path: // {log_dir}/internal-{op}.log — the branch argument is ignored. assert_snapshot!( - HookLog::shared(InternalOp::TrashSweep).path(log_dir, "anything").to_slash_lossy(), - @"/repo/.git/wt/logs/internal-trash-sweep.log" - ); - assert_snapshot!( - HookLog::shared(InternalOp::TrashSweep).path(log_dir, "").to_slash_lossy(), + HookLog::Shared(InternalOp::TrashSweep) + .path(log_dir, "anything") + .to_slash_lossy(), @"/repo/.git/wt/logs/internal-trash-sweep.log" ); } diff --git a/src/commands/worktree/switch.rs b/src/commands/worktree/switch.rs index 1601c14845..493652186f 100644 --- a/src/commands/worktree/switch.rs +++ b/src/commands/worktree/switch.rs @@ -166,30 +166,6 @@ fn choose_pr_provider(repo: &Repository) -> anyhow::Result<&'static dyn RemoteRe } } -fn resolve_pr_target( - repo: &Repository, - number: u32, - create: bool, - base: Option<&str>, -) -> anyhow::Result { - if base.is_some() { - return Err(GitError::RefBaseConflict { - ref_type: RefType::Pr, - number, - } - .into()); - } - - resolve_remote_ref(repo, choose_pr_provider(repo)?, number, create, base) -} - -fn resolve_pr_base( - repo: &Repository, - number: u32, -) -> anyhow::Result<(String, Option<(String, String)>)> { - resolve_remote_ref_as_base(repo, choose_pr_provider(repo)?, number) -} - /// Fetch PR/MR info while showing a "still waiting" status. /// /// The host lookup (`gh`/`glab` API) captures its output and can stall on a slow @@ -215,16 +191,10 @@ fn resolve_remote_ref( provider: &dyn RemoteRefProvider, number: u32, create: bool, - base: Option<&str>, ) -> anyhow::Result { let ref_type = provider.ref_type(); let symbol = ref_type.symbol(); - // --base is invalid with pr:/mr: syntax (check early, no network needed) - if base.is_some() { - return Err(GitError::RefBaseConflict { ref_type, number }.into()); - } - // Fetch ref info (network call via gh/glab CLI) eprintln!( "{}", @@ -251,7 +221,15 @@ fn resolve_remote_ref( } // Same-repo ref: fetch the branch to ensure remote tracking refs exist - resolve_same_repo_ref(repo, &info) + fetch_same_repo_branch(repo, &info)?; + Ok(ResolvedTarget { + selector: Selector::rewritten_to(info.source_branch), + method: CreationMethod::Regular { + create_branch: false, + base_branch: None, + base_pr_upstream: None, + }, + }) } /// Resolve a fork (cross-repo) PR/MR. @@ -263,7 +241,7 @@ fn resolve_fork_ref( ) -> anyhow::Result { let ref_type = provider.ref_type(); let repo_root = repo.repo_path()?; - let local_branch = remote_ref::local_branch_name(info); + let local_branch = info.source_branch.clone(); let expected_remote = match remote_ref::find_remote(repo, info) { Ok(remote) => Some(remote), Err(e) => { @@ -411,23 +389,6 @@ fn resolve_fork_ref( }) } -/// Resolve a same-repo (non-fork) PR/MR. -fn resolve_same_repo_ref( - repo: &Repository, - info: &RemoteRefInfo, -) -> anyhow::Result { - fetch_same_repo_branch(repo, info)?; - - Ok(ResolvedTarget { - selector: Selector::rewritten_to(info.source_branch.clone()), - method: CreationMethod::Regular { - create_branch: false, - base_branch: None, - base_pr_upstream: None, - }, - }) -} - /// Fetch a same-repo PR/MR's source branch with an explicit refspec so the /// remote-tracking ref exists locally even in repos with limited fetch /// refspecs (single-branch clones, bare repos). @@ -485,12 +446,12 @@ fn resolve_base_ref( repo: &Repository, base: &str, ) -> anyhow::Result<(String, Option<(String, String)>)> { - match parse_ref_shortcut(base) { - Some((RefType::Pr, number)) => return resolve_pr_base(repo, number), - Some((RefType::Mr, number)) => { - return resolve_remote_ref_as_base(repo, &GitLabProvider, number); - } - None => {} + if let Some((ref_type, number)) = parse_ref_shortcut(base) { + let provider: &dyn RemoteRefProvider = match ref_type { + RefType::Pr => choose_pr_provider(repo)?, + RefType::Mr => &GitLabProvider, + }; + return resolve_remote_ref_as_base(repo, provider, number); } let selector = repo.expand_selector(base)?; @@ -569,12 +530,17 @@ fn resolve_switch_target( ) -> anyhow::Result { // `pr:N` dispatches to GitHub, Gitea, or Azure DevOps based on remotes; // `mr:N` to GitLab. Forge PR/MR web URLs normalise to the same shortcuts. - match parse_ref_shortcut(branch) { - Some((RefType::Pr, number)) => return resolve_pr_target(repo, number, create, base), - Some((RefType::Mr, number)) => { - return resolve_remote_ref(repo, &GitLabProvider, number, create, base); + if let Some((ref_type, number)) = parse_ref_shortcut(branch) { + // --base is invalid with pr:/mr: syntax (check before provider selection, + // which may invoke a forge CLI to inspect authentication). + if base.is_some() { + return Err(GitError::RefBaseConflict { ref_type, number }.into()); } - None => {} + let provider: &dyn RemoteRefProvider = match ref_type { + RefType::Pr => choose_pr_provider(repo)?, + RefType::Mr => &GitLabProvider, + }; + return resolve_remote_ref(repo, provider, number, create); } // Regular branch switch. `expand_selector` normalizes the token and diff --git a/src/config/approvals.rs b/src/config/approvals.rs index 62ffd11e9c..42a9bc81fb 100644 --- a/src/config/approvals.rs +++ b/src/config/approvals.rs @@ -357,16 +357,6 @@ impl Approvals { Ok(approvals) } - /// Add an approved command and save. See [`Self::approve_commands`]. - pub fn approve_command( - &mut self, - project: String, - command: String, - approvals_path: &Path, - ) -> Result<(), ConfigError> { - self.approve_commands(project, vec![command], approvals_path) - } - /// Add multiple approved commands in a single locked operation. /// /// Refuses a `project` containing `*`: reads treat such a key as a pattern @@ -624,9 +614,9 @@ approved-commands = ["npm install"] approvals.save_to(&path).unwrap(); approvals - .approve_command( + .approve_commands( "git.company.example/owner/repo".to_string(), - "npm test".to_string(), + vec!["npm test".to_string()], &path, ) .unwrap(); @@ -673,9 +663,9 @@ approved-commands = ["npm test"] let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( "github.com/user/repo".to_string(), - "npm install".to_string(), + vec!["npm install".to_string()], &path, ) .unwrap(); @@ -691,16 +681,16 @@ approved-commands = ["npm test"] let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( "github.com/user/repo".to_string(), - "npm install".to_string(), + vec!["npm install".to_string()], &path, ) .unwrap(); approvals - .approve_command( + .approve_commands( "github.com/user/repo".to_string(), - "npm install".to_string(), + vec!["npm install".to_string()], &path, ) .unwrap(); @@ -744,9 +734,9 @@ approved-commands = ["npm test"] ) .unwrap(); approvals - .approve_command( + .approve_commands( "github.com/user/repo2".to_string(), - "cargo build".to_string(), + vec!["cargo build".to_string()], &path, ) .unwrap(); @@ -764,16 +754,16 @@ approved-commands = ["npm test"] let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( "github.com/user/repo1".to_string(), - "npm install".to_string(), + vec!["npm install".to_string()], &path, ) .unwrap(); approvals - .approve_command( + .approve_commands( "github.com/user/repo2".to_string(), - "cargo build".to_string(), + vec!["cargo build".to_string()], &path, ) .unwrap(); @@ -879,9 +869,9 @@ approved-commands = ["npm test"] let mut approvals = Approvals::default(); // Approve with deprecated variable name approvals - .approve_command( + .approve_commands( "project".to_string(), - "echo {{ repo_root }}".to_string(), + vec!["echo {{ repo_root }}".to_string()], &path, ) .unwrap(); @@ -896,7 +886,11 @@ approved-commands = ["npm test"] let mut approvals = Approvals::default(); approvals - .approve_command("project".to_string(), "echo repo_root".to_string(), &path) + .approve_commands( + "project".to_string(), + vec!["echo repo_root".to_string()], + &path, + ) .unwrap(); assert!(!approvals.is_command_approved("project", "echo repo_path")); @@ -921,9 +915,9 @@ approved-commands = ["npm test"] let mut approvals = Approvals::default(); barrier.wait(); approvals - .approve_command( + .approve_commands( "github.com/user/repo".to_string(), - format!("command_{i}"), + vec![format!("command_{i}")], &config_path, ) .unwrap(); @@ -990,9 +984,9 @@ approved-commands = ["npm install"] // Approve a new command — reload_from should pick up config.toml fallback let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( "github.com/user/repo".to_string(), - "npm test".to_string(), + vec!["npm test".to_string()], &approvals_path, ) .unwrap(); @@ -1086,7 +1080,7 @@ approved-command = ["npm test"] let (_temp_dir, path) = test_dir(); let mut approvals = Approvals::default(); approvals - .approve_command("project-a".to_string(), "cmd1".to_string(), &path) + .approve_commands("project-a".to_string(), vec!["cmd1".to_string()], &path) .unwrap(); // Revoke a project that doesn't exist — should be a no-op approvals.revoke_project("nonexistent", &path).unwrap(); @@ -1132,7 +1126,7 @@ approved-command = ["npm test"] let (_temp_dir, path) = test_dir(); let mut approvals = Approvals::default(); approvals - .approve_command("project-a".to_string(), "cmd1".to_string(), &path) + .approve_commands("project-a".to_string(), vec!["cmd1".to_string()], &path) .unwrap(); // Manually clear the commands (without removing the project entry) approvals @@ -1222,10 +1216,10 @@ approved-command = ["npm test"] let mut approvals = Approvals::default(); approvals - .approve_command("project1".to_string(), "cmd1".to_string(), &path) + .approve_commands("project1".to_string(), vec!["cmd1".to_string()], &path) .unwrap(); approvals - .approve_command("project2".to_string(), "cmd2".to_string(), &path) + .approve_commands("project2".to_string(), vec!["cmd2".to_string()], &path) .unwrap(); let projects: Vec<_> = approvals.projects().collect(); diff --git a/src/git/remote_ref/mod.rs b/src/git/remote_ref/mod.rs index 3a565c7fa2..65a48175f8 100644 --- a/src/git/remote_ref/mod.rs +++ b/src/git/remote_ref/mod.rs @@ -309,15 +309,6 @@ pub fn branch_tracks_ref( crate::git::branch_tracks_ref(repo_root, branch, &expected_ref, expected_remote) } -/// Generate the local branch name for a remote ref. -/// -/// Uses the source branch name directly. This ensures the local branch name -/// matches the remote branch name, which is required for `git push` to work -/// correctly with `push.default = current`. -pub fn local_branch_name(info: &RemoteRefInfo) -> String { - info.source_branch.clone() -} - /// A forge PR/MR web URL decomposed into its parts. /// /// Detection is shape-based, not host-based: the URL must use `http(s)://` @@ -344,7 +335,7 @@ struct RefUrlParts<'a> { /// (GitHub including Enterprise, GitLab, Gitea, Azure DevOps). /// /// Shared by [`parse_ref_url`] (which formats the `pr:`/`mr:` shortcut) and -/// [`repo_url_from_ref_url`] (which keeps the path up to the marker). +/// [`repo_info_from_ref_url_with_provider`] (which keeps the path up to the marker). fn parse_ref_url_parts(input: &str) -> Option> { let trimmed = input.trim(); let scheme_end = trimmed.find("://")?; @@ -401,33 +392,14 @@ pub fn parse_ref_url(input: &str) -> Option { Some(format!("{}:{}", parts.kind, parts.number)) } -/// Derive the repository web URL from a PR/MR URL. -/// -/// Truncates the PR/MR path (`/pull/N`, `/pulls/N`, `/pullrequest/N`, or -/// `/-/merge_requests/N`) to leave the repository's web URL. The result names -/// the **target** repository: for a fork PR it is the upstream repo the PR was -/// opened against, not the contributor's fork. `wt list --format=json` uses -/// this to align `repo_url` with the PR/MR link in `ci.url`, since the primary -/// remote in a fork checkout points at the fork (the source). -/// -/// Detection is shape-based and host-agnostic (see `parse_ref_url_parts`). -/// Returns `None` when the input isn't a recognized PR/MR link. -pub fn repo_url_from_ref_url(input: &str) -> Option { - repo_info_from_ref_url(input).map(|info| info.url) -} - -/// Derive repository metadata from a PR/MR URL. -/// -/// The returned URL is identical to [`repo_url_from_ref_url`]. Provider and -/// owner/name fields are derived from the PR/MR URL shape: GitHub `/pull/N`, -/// Gitea `/pulls/N`, GitLab `/-/merge_requests/N`, and Azure DevOps -/// `/pullrequest/N`. -pub fn repo_info_from_ref_url(input: &str) -> Option { - repo_info_from_ref_url_with_provider(input, None) -} - /// Derive repository metadata from a PR/MR URL with an optional configured /// `[forge].platform` override. +/// +/// The repository URL truncates the PR/MR path (`/pull/N`, `/pulls/N`, +/// `/pullrequest/N`, or `/-/merge_requests/N`). Provider and owner/name fields +/// are derived from the same shape. The URL names the **target** repository: +/// for a fork PR it is the upstream repo the PR was opened against, not the +/// contributor's fork. pub fn repo_info_from_ref_url_with_provider( input: &str, provider_override: Option<&str>, @@ -688,7 +660,7 @@ mod tests { } #[test] - fn repo_url_from_ref_url_per_forge() { + fn repo_info_url_per_forge() { let cases = [ // GitHub, including a fork PR (target repo = the upstream owner). ( @@ -731,27 +703,23 @@ mod tests { ), ]; for (input, expected) in cases { - assert_eq!( - repo_url_from_ref_url(input).as_deref(), - Some(expected), - "input: {input}" - ); + let info = repo_info_from_ref_url_with_provider(input, None) + .expect("ref URL should produce repo info"); + assert_eq!(info.url, expected, "input: {input}"); } } #[test] - fn repo_url_from_ref_url_rejects_non_pr_urls() { - // Not a PR/MR link. - assert_eq!(repo_url_from_ref_url("https://github.com/owner/repo"), None); - assert_eq!( - repo_url_from_ref_url("https://github.com/o/r/issues/5"), - None - ); - // Too shallow. - assert_eq!(repo_url_from_ref_url("https://example.com/pull/1"), None); - // Not a URL. - assert_eq!(repo_url_from_ref_url("pr:123"), None); - assert_eq!(repo_url_from_ref_url(""), None); + fn repo_info_rejects_non_pr_urls() { + for input in [ + "https://github.com/owner/repo", // Not a PR/MR link. + "https://github.com/o/r/issues/5", // Not a PR/MR link. + "https://example.com/pull/1", // Too shallow. + "pr:123", // Not a URL. + "", + ] { + assert_eq!(repo_info_from_ref_url_with_provider(input, None), None); + } } #[test] @@ -814,9 +782,9 @@ mod tests { ]; for (input, url, provider, host, owner, name, project) in cases { - let info = repo_info_from_ref_url(input).expect("ref URL should produce repo info"); + let info = repo_info_from_ref_url_with_provider(input, None) + .expect("ref URL should produce repo info"); assert_eq!(info.url, url, "input: {input}"); - assert_eq!(repo_url_from_ref_url(input).as_deref(), Some(url)); assert_eq!(info.provider, provider, "input: {input}"); assert_eq!(info.host, host, "input: {input}"); assert_eq!(info.owner, owner, "input: {input}"); @@ -827,8 +795,9 @@ mod tests { #[test] fn repo_info_from_ref_url_unknown_pull_host() { - let info = repo_info_from_ref_url("https://git.example.com/owner/repo/pull/1") - .expect("shape is still parseable"); + let info = + repo_info_from_ref_url_with_provider("https://git.example.com/owner/repo/pull/1", None) + .expect("shape is still parseable"); assert_eq!(info.url, "https://git.example.com/owner/repo"); assert_eq!(info.provider, GitRepoProvider::Unknown); assert_eq!(info.host, "git.example.com"); @@ -839,20 +808,23 @@ mod tests { #[test] fn repo_info_from_ref_url_respects_host_boundaries_and_ports() { - let github = repo_info_from_ref_url("https://github.com:8443/owner/repo/pull/1") - .expect("GitHub PR URL"); + let github = + repo_info_from_ref_url_with_provider("https://github.com:8443/owner/repo/pull/1", None) + .expect("GitHub PR URL"); assert_eq!(github.provider, GitRepoProvider::GitHub); - let azure = repo_info_from_ref_url( + let azure = repo_info_from_ref_url_with_provider( "https://dev.azure.com:8443/org/project/_git/repo/pullrequest/9", + None, ) .expect("Azure DevOps PR URL"); assert_eq!(azure.provider, GitRepoProvider::AzureDevOps); assert_eq!(azure.owner, "org"); assert_eq!(azure.project.as_deref(), Some("project")); - let azure_with_forge_label = repo_info_from_ref_url( + let azure_with_forge_label = repo_info_from_ref_url_with_provider( "https://github.dev.azure.com/org/project/_git/repo/pullrequest/9", + None, ) .expect("Azure DevOps PR URL"); assert_eq!( @@ -860,16 +832,21 @@ mod tests { GitRepoProvider::AzureDevOps ); - let hostile_userinfo = - repo_info_from_ref_url("https://github.com@attacker.example/owner/repo/pull/1") - .expect("ref URL is structurally valid"); + let hostile_userinfo = repo_info_from_ref_url_with_provider( + "https://github.com@attacker.example/owner/repo/pull/1", + None, + ) + .expect("ref URL is structurally valid"); assert_eq!(hostile_userinfo.provider, GitRepoProvider::Unknown); assert_eq!(hostile_userinfo.host, "attacker.example"); assert_eq!(hostile_userinfo.url, "https://attacker.example/owner/repo"); // A brand anywhere in the host classifies here too. - let branded = repo_info_from_ref_url("https://github-mirror.example/owner/repo/pull/1") - .expect("ref URL is structurally valid"); + let branded = repo_info_from_ref_url_with_provider( + "https://github-mirror.example/owner/repo/pull/1", + None, + ) + .expect("ref URL is structurally valid"); assert_eq!(branded.provider, GitRepoProvider::GitHub); // The Azure service domains stay bounded by suffix, and a host outside @@ -878,7 +855,8 @@ mod tests { "https://dev.azure.com.attacker.example/org/project/_git/repo/pullrequest/9", "https://evil-visualstudio.com/org/project/_git/repo/pullrequest/9", ] { - let info = repo_info_from_ref_url(input).expect("ref URL is structurally valid"); + let info = repo_info_from_ref_url_with_provider(input, None) + .expect("ref URL is structurally valid"); assert_eq!(info.provider, GitRepoProvider::Unknown, "{input}"); } } @@ -888,10 +866,6 @@ mod tests { let input = "https://git.example.com/owner/repo/pull/1"; let info = repo_info_from_ref_url_with_provider(input, Some("github")) .expect("shape is still parseable"); - assert_eq!( - repo_url_from_ref_url(input).as_deref(), - Some(info.url.as_str()) - ); assert_eq!(info.url, "https://git.example.com/owner/repo"); assert_eq!(info.provider, GitRepoProvider::GitHub); assert_eq!(info.host, "git.example.com"); @@ -905,10 +879,6 @@ mod tests { let input = "https://git.example.com/org/project/_git/repo/pullrequest/9"; let info = repo_info_from_ref_url_with_provider(input, Some("azure-devops")) .expect("shape is still parseable"); - assert_eq!( - repo_url_from_ref_url(input).as_deref(), - Some(info.url.as_str()) - ); assert_eq!(info.url, "https://git.example.com/org/project/_git/repo"); assert_eq!(info.provider, GitRepoProvider::AzureDevOps); assert_eq!(info.host, "git.example.com"); @@ -920,11 +890,8 @@ mod tests { #[test] fn repo_info_from_ref_url_malformed_azure_is_unknown() { let input = "https://dev.azure.com/org/repo/pullrequest/9"; - let info = repo_info_from_ref_url(input).expect("shape is still parseable"); - assert_eq!( - repo_url_from_ref_url(input).as_deref(), - Some(info.url.as_str()) - ); + let info = + repo_info_from_ref_url_with_provider(input, None).expect("shape is still parseable"); assert_eq!(info.url, "https://dev.azure.com/org/repo"); assert_eq!(info.provider, GitRepoProvider::Unknown); assert_eq!(info.host, "dev.azure.com"); diff --git a/src/git/repository/branches.rs b/src/git/repository/branches.rs index aaf100f9ff..3fd1c204ef 100644 --- a/src/git/repository/branches.rs +++ b/src/git/repository/branches.rs @@ -223,21 +223,6 @@ impl Repository { .collect()) } - /// Get branches that don't have worktrees (available for switch). - pub fn available_branches(&self) -> anyhow::Result> { - let worktrees = self.list_worktrees()?; - let branches_with_worktrees: HashSet = worktrees - .iter() - .filter_map(|wt| wt.branch.clone()) - .collect(); - Ok(self - .local_branches()? - .iter() - .filter(|b| !branches_with_worktrees.contains(&b.name)) - .map(|b| b.name.clone()) - .collect()) - } - /// Get branches with metadata for shell completions. /// /// Returns branches in completion order: worktrees first, then local branches, @@ -593,6 +578,8 @@ mod tests { let repo = Repository::at(test.root_path()).unwrap(); let before = repo.default_branch_sha().expect("main resolves"); + let cloned_repo = repo.clone(); + assert!(std::sync::Arc::ptr_eq(&repo.cache, &cloned_repo.cache)); // Move main forward outside `repo`'s knowledge. std::fs::write(test.root_path().join("after.txt"), "after\n").unwrap(); @@ -601,8 +588,8 @@ mod tests { let real_after = test.git_output(&["rev-parse", "main"]); assert_ne!(before, real_after, "test setup: main should have moved"); - // Same `repo`: the cached inventory still serves the pre-move SHA. - assert_eq!(repo.default_branch_sha(), Some(before)); + // A clone shares the cached inventory and still serves the pre-move SHA. + assert_eq!(cloned_repo.default_branch_sha(), Some(before)); // A fresh `Repository::at` scans again and sees the new SHA. let repo2 = Repository::at(test.root_path()).unwrap(); diff --git a/src/git/repository/mod.rs b/src/git/repository/mod.rs index c2817c4ea9..9e002e9e03 100644 --- a/src/git/repository/mod.rs +++ b/src/git/repository/mod.rs @@ -1202,16 +1202,6 @@ impl Repository { }) } - /// Check if this repository shares its cache with another. - /// - /// Returns true if both repositories point to the same underlying cache. - /// This is primarily useful for testing that cloned repositories share - /// cached data. - #[doc(hidden)] - pub fn shares_cache_with(&self, other: &Repository) -> bool { - Arc::ptr_eq(&self.cache, &other.cache) - } - /// Resolve the git common directory for a path. /// /// Always returns a canonicalized absolute path to ensure consistent diff --git a/src/git/repository/ref_snapshot.rs b/src/git/repository/ref_snapshot.rs index d736ede5af..8fd2f3814e 100644 --- a/src/git/repository/ref_snapshot.rs +++ b/src/git/repository/ref_snapshot.rs @@ -32,8 +32,6 @@ use std::collections::HashMap; -use anyhow::bail; - use super::branches::LocalBranchInventory; use super::{LocalBranch, RemoteBranch, Repository}; @@ -78,14 +76,6 @@ impl RefSnapshot { self.commits.get(name).map(String::as_str) } - /// Resolve a ref name to its commit SHA, erroring when absent. - pub fn must_resolve(&self, name: &str) -> anyhow::Result<&str> { - match self.resolve(name) { - Some(sha) => Ok(sha), - None => bail!("ref not present in snapshot: {name}"), - } - } - /// Look up the configured upstream short name for a local branch. /// /// Returns `None` when no upstream is configured, when the branch is @@ -619,12 +609,12 @@ mod tests { } #[test] - fn must_resolve_errors_on_missing_ref() { + fn resolve_returns_none_for_uncaptured_refs() { let test = TestRepo::with_initial_commit(); let repo = Repository::at(test.root_path()).unwrap(); let snap = repo.capture_refs().unwrap(); - assert!(snap.must_resolve("does-not-exist").is_err()); + assert_eq!(snap.resolve("does-not-exist"), None); // HEAD is intentionally absent — callers fall back to rev-parse. assert_eq!(snap.resolve("HEAD"), None); } diff --git a/src/git/url.rs b/src/git/url.rs index d9edfcfa7d..5ab1e504da 100644 --- a/src/git/url.rs +++ b/src/git/url.rs @@ -250,13 +250,6 @@ impl GitRemoteUrl { self.forge_kind() == Some(ForgeKind::GitHub) } - /// Check if this URL points to a GitLab host. - /// - /// Matches gitlab.com and self-hosted GitLab instances (e.g., gitlab.example.com). - pub fn is_gitlab(&self) -> bool { - self.forge_kind() == Some(ForgeKind::GitLab) - } - /// Check if this URL points to a Gitea host. /// /// Matches gitea.com and self-hosted Gitea instances (e.g., gitea.example.com). @@ -576,7 +569,7 @@ mod tests { assert!(url.is_github()); let url = GitRemoteUrl::parse("git://gitlab.example.com/owner/repo.git").unwrap(); - assert!(url.is_gitlab()); + assert_eq!(url.forge_kind(), Some(ForgeKind::GitLab)); } /// URL-scheme authorities use the final `@` as the userinfo boundary, so @@ -781,40 +774,6 @@ mod tests { ); } - #[test] - fn test_is_gitlab() { - // GitLab.com - assert!( - GitRemoteUrl::parse("https://gitlab.com/owner/repo.git") - .unwrap() - .is_gitlab() - ); - assert!( - GitRemoteUrl::parse("git@gitlab.com:owner/repo.git") - .unwrap() - .is_gitlab() - ); - - // Self-hosted GitLab - assert!( - GitRemoteUrl::parse("https://gitlab.example.com/owner/repo.git") - .unwrap() - .is_gitlab() - ); - - // Not GitLab - assert!( - !GitRemoteUrl::parse("https://github.com/owner/repo.git") - .unwrap() - .is_gitlab() - ); - assert!( - !GitRemoteUrl::parse("https://bitbucket.org/owner/repo.git") - .unwrap() - .is_gitlab() - ); - } - #[test] fn forge_detection_reads_the_brand_and_bounds_the_azure_domains() { // A brand anywhere in the host classifies, ports included. @@ -1455,7 +1414,6 @@ mod tests { let url = GitRemoteUrl::parse("https://dev.azure.com/myorg/myproject/_git/myrepo").unwrap(); assert!(url.is_azure_devops()); assert!(!url.is_github()); - assert!(!url.is_gitlab()); // SSH ssh.dev.azure.com let url = GitRemoteUrl::parse("git@ssh.dev.azure.com:v3/myorg/myproject/myrepo").unwrap(); diff --git a/src/output/handlers.rs b/src/output/handlers.rs index 36148cf119..7a27f188e7 100644 --- a/src/output/handlers.rs +++ b/src/output/handlers.rs @@ -178,7 +178,7 @@ fn spawn_background_removal( main_path, &remove_command, log_label, - &HookLog::internal(InternalOp::Remove), + &HookLog::Internal(InternalOp::Remove), None, )?; } diff --git a/src/output/shell_integration.rs b/src/output/shell_integration.rs index 1502ffef1c..ee802a7a82 100644 --- a/src/output/shell_integration.rs +++ b/src/output/shell_integration.rs @@ -75,8 +75,8 @@ use worktrunk::styling::{ }; use crate::commands::configure_shell::{ - ConfigAction, UninstallScanResult, collect_legacy_cleanups, format_matched_lines, - handle_configure_shell, prompt_for_install, scan_shell_configs, + ConfigAction, UninstallScanResult, apply_confirmed_shell_config, collect_legacy_cleanups, + format_matched_lines, prompt_for_install, scan_shell_configs, }; /// Git config key tracking how many times the shell-integration install hint @@ -482,12 +482,10 @@ pub fn prompt_shell_integration( // TTY + first time: Show interactive prompt // Accepting installs for all shells with config files (same as `wt config shell install`) - // Detect (without removing) the legacy files the subsequent handle_configure_shell - // would delete, so this offer names them before the user consents — the removal is - // destructive and must not happen unpreviewed, exactly as `wt config shell install` - // now previews it (issue #3644). The list is computed from the same dry-run scan - // handle_configure_shell re-derives internally, so the prompt names precisely what - // the install removes. + // Detect (without removing) the legacy files the confirmed plan will delete, + // so this offer names them before the user consents — the removal is destructive + // and must not happen unpreviewed, exactly as `wt config shell install` previews it + // (issue #3644). let legacy_preview = collect_legacy_cleanups(&scan.configured, binary_name, true); let confirmed = prompt_for_install( &scan.configured, @@ -506,8 +504,9 @@ pub fn prompt_shell_integration( return Ok(false); } - // Install for all shells with config files (same as `wt config shell install`) - let install_result = handle_configure_shell(None, true, false, binary_name.to_string()) + // Apply the exact plan confirmed above; deriving another preview here would + // authorize filesystem changes made while the prompt was waiting. + let install_result = apply_confirmed_shell_config(scan, None, binary_name) .map_err(|e| anyhow::anyhow!("Failed to configure shell integration: {e}"))?; print_shell_install_result(&install_result); @@ -538,8 +537,7 @@ pub fn print_shell_uninstall_result(scan_result: &UninstallScanResult, explicit_ eprintln!( "{}{}", success_message(cformat!( - "{} {what} for {shell} @ {path}", - result.action.description(), + "Removed {what} for {shell} @ {path}", )), format_matched_lines(&result.matched_lines), ); @@ -553,8 +551,7 @@ pub fn print_shell_uninstall_result(scan_result: &UninstallScanResult, explicit_ eprintln!( "{}", success_message(cformat!( - "{} completions for {shell} @ {path}", - result.action.description(), + "Removed completions for {shell} @ {path}", )) ); } diff --git a/src/shell/detection.rs b/src/shell/detection.rs index f006b5242f..e09cca3fd4 100644 --- a/src/shell/detection.rs +++ b/src/shell/detection.rs @@ -560,7 +560,7 @@ pub fn scan_for_detection_details(cmd: &str) -> Result, ]; // Add Nushell vendor autoload paths (check all candidate locations) - config_files.extend(super::config_paths(super::Shell::Nushell, cmd).unwrap_or_default()); + config_files.extend(super::Shell::Nushell.config_paths(cmd).unwrap_or_default()); // Add PowerShell profiles config_files.extend(powershell_profile_paths(&home)); diff --git a/src/shell/mod.rs b/src/shell/mod.rs index 81e63369e4..4df6a648ef 100644 --- a/src/shell/mod.rs +++ b/src/shell/mod.rs @@ -18,10 +18,7 @@ pub use detection::{ BypassAlias, DetectedLine, FileDetectionResult, is_shell_integration_line, is_shell_integration_line_for_uninstall_any_cmd, scan_for_detection_details, }; -pub use paths::{ - completion_path, config_paths, home_dir_required, legacy_fish_conf_d_path, - line_based_config_paths, nushell_autoload_candidates, -}; +pub use paths::{home_dir_required, line_based_config_paths, nushell_autoload_candidates}; pub use utils::{ AncestorShell, ZshStartupScope, ancestor_shell, current_shell, current_shell_name, extract_filename_from_path, probe_zsh_compdef, diff --git a/src/shell/paths.rs b/src/shell/paths.rs index 2e6cb22dce..12f299c297 100644 --- a/src/shell/paths.rs +++ b/src/shell/paths.rs @@ -236,7 +236,7 @@ pub fn line_based_config_paths(shell: super::Shell, home: &std::path::Path) -> V /// /// The `cmd` parameter affects the Fish functions filename (e.g., `wt.fish` or `git-wt.fish`). /// Returns paths in order of preference. The first existing file should be used. -pub fn config_paths(shell: super::Shell, cmd: &str) -> Result, std::io::Error> { +pub(super) fn config_paths(shell: super::Shell, cmd: &str) -> Result, std::io::Error> { let home = home_dir_required()?; Ok(match shell { @@ -275,7 +275,7 @@ pub fn config_paths(shell: super::Shell, cmd: &str) -> Result, std: /// This caused issues with Homebrew PATH setup (see issue #566). We now install to /// `functions/{cmd}.fish` instead. This method returns the legacy path so install/uninstall /// can clean it up. -pub fn legacy_fish_conf_d_path(cmd: &str) -> Result { +pub(super) fn legacy_fish_conf_d_path(cmd: &str) -> Result { let home = home_dir_required()?; Ok(home .join(".config") @@ -292,7 +292,7 @@ pub fn legacy_fish_conf_d_path(cmd: &str) -> Result { /// Only Fish uses a separate completion file at ~/.config/fish/completions/ /// (installed by `wt config shell install`) that uses $WORKTRUNK_BIN to bypass /// the shell function wrapper. -pub fn completion_path(shell: super::Shell, cmd: &str) -> Result { +pub(super) fn completion_path(shell: super::Shell, cmd: &str) -> Result { let home = home_dir_required()?; // Use etcetera for XDG-compliant paths when available diff --git a/tests/CLAUDE.md b/tests/CLAUDE.md index c0324ce12f..e52c49e5ac 100644 --- a/tests/CLAUDE.md +++ b/tests/CLAUDE.md @@ -205,11 +205,11 @@ What the hole cost before this: any key in the developer's config applied to fix in-process unit test that calls library functions directly gets no such isolation: it runs in the test process, which inherits the real environment. -The `Approvals` and `UserConfig` mutation methods take an explicit `&Path`, so -a unit test passes a tempdir-backed path and the write stays isolated. The -global resolvers do not isolate: `Approvals::load()`, `approvals_path()`, -`config_path()`, and `system_config_path()` all fall back to the real -`~/.config/worktrunk/`. +`Approvals::approve_commands` and the `UserConfig` mutation methods take an +explicit `&Path`, so a unit test passes a tempdir-backed path and the write stays +isolated. The global resolvers do not isolate: `Approvals::load()`, +`approvals_path()`, `config_path()`, and `system_config_path()` all fall back to +the real `~/.config/worktrunk/`. @@ -218,7 +218,7 @@ Bad: ```rust let mut approvals = Approvals::load().unwrap(); -approvals.approve_command(project, command, &approvals_path).unwrap(); +approvals.approve_commands(project, vec![command], &approvals_path).unwrap(); ``` @@ -230,7 +230,7 @@ Good: let temp_dir = tempfile::tempdir().unwrap(); let approvals_path = temp_dir.path().join("approvals.toml"); let mut approvals = Approvals::default(); -approvals.approve_command(project, command, &approvals_path).unwrap(); +approvals.approve_commands(project, vec![command], &approvals_path).unwrap(); ``` diff --git a/tests/common/pty.rs b/tests/common/pty.rs index f8c82f5322..224178791f 100644 --- a/tests/common/pty.rs +++ b/tests/common/pty.rs @@ -1,10 +1,11 @@ //! PTY execution helpers for integration tests. //! -//! Three public functions — compose `build_pty_command` with a runner: +//! Compose `build_pty_command` with a runner: //! //! - **`build_pty_command`** — builds a `CommandBuilder` with env isolation //! - **`exec_cmd_in_pty`** — pre-buffers input, for non-interactive commands //! - **`exec_cmd_in_pty_prompted`** — waits for prompt marker before each input +//! - **`exec_cmd_in_pty_prompted_with`** — also changes test state at a prompt //! //! ```ignore //! use crate::common::pty::{build_pty_command, exec_cmd_in_pty_prompted}; @@ -349,6 +350,20 @@ pub fn exec_cmd_in_pty_prompted( cmd: CommandBuilder, inputs: &[&str], prompt_marker: &str, +) -> (String, i32) { + exec_cmd_in_pty_prompted_with(cmd, inputs, prompt_marker, |_| {}) +} + +/// Execute a command in a PTY, running an action after each prompt appears and +/// immediately before its input is sent. +/// +/// This synchronizes tests that need to change external state inside the +/// preview/confirmation window without sleeps or timing assumptions. +pub fn exec_cmd_in_pty_prompted_with( + cmd: CommandBuilder, + inputs: &[&str], + prompt_marker: &str, + before_input: impl FnMut(usize), ) -> (String, i32) { let pair = super::open_pty(); @@ -358,7 +373,14 @@ pub fn exec_cmd_in_pty_prompted( let reader = pair.master.try_clone_reader().unwrap(); let writer = pair.master.take_writer().unwrap(); - prompted_pty_interaction(reader, writer, &mut child, inputs, prompt_marker) + prompted_pty_interaction( + reader, + writer, + &mut child, + inputs, + prompt_marker, + before_input, + ) } /// Core prompt-waiting logic shared by all `_prompted` variants. @@ -372,6 +394,7 @@ fn prompted_pty_interaction( child: &mut Box, inputs: &[&str], prompt_marker: &str, + mut before_input: impl FnMut(usize), ) -> (String, i32) { use std::sync::mpsc; use std::time::{Duration, Instant}; @@ -403,7 +426,7 @@ fn prompted_pty_interaction( // For each input, wait for a NEW prompt marker to appear, then send let mut markers_seen: usize = 0; - for input in inputs { + for (input_index, input) in inputs.iter().enumerate() { let target = markers_seen + 1; let start = Instant::now(); @@ -451,6 +474,7 @@ fn prompted_pty_interaction( std::thread::sleep(poll); } + before_input(input_index); writer.write_all(input.as_bytes()).unwrap(); writer.flush().unwrap(); } diff --git a/tests/integration_tests/approval_save.rs b/tests/integration_tests/approval_save.rs index ce85886da6..b3dd4e2b34 100644 --- a/tests/integration_tests/approval_save.rs +++ b/tests/integration_tests/approval_save.rs @@ -5,7 +5,7 @@ use worktrunk::config::Approvals; use worktrunk::config::UserConfig; /// -/// This test uses `approve_command()` to ensure it never writes to the user's config +/// This test uses `approve_commands()` to ensure it never writes to the user's config #[test] fn test_approval_saves_to_disk() { let temp_dir = TempDir::new().unwrap(); @@ -16,9 +16,9 @@ fn test_approval_saves_to_disk() { // Add an approval to the explicit path approvals - .approve_command( + .approve_commands( "github.com/test/repo".to_string(), - "test command".to_string(), + vec!["test command".to_string()], &approvals_path, ) .unwrap(); @@ -52,16 +52,16 @@ fn test_duplicate_approvals_not_saved_twice() { // Add same approval twice approvals - .approve_command( + .approve_commands( "github.com/test/repo".to_string(), - "test".to_string(), + vec!["test".to_string()], &approvals_path, ) .ok(); approvals - .approve_command( + .approve_commands( "github.com/test/repo".to_string(), - "test".to_string(), + vec!["test".to_string()], &approvals_path, ) .ok(); @@ -85,23 +85,23 @@ fn test_multiple_project_approvals() { // Add approvals for different projects approvals - .approve_command( + .approve_commands( "github.com/user1/repo1".to_string(), - "npm install".to_string(), + vec!["npm install".to_string()], &approvals_path, ) .unwrap(); approvals - .approve_command( + .approve_commands( "github.com/user2/repo2".to_string(), - "cargo build".to_string(), + vec!["cargo build".to_string()], &approvals_path, ) .unwrap(); approvals - .approve_command( + .approve_commands( "github.com/user1/repo1".to_string(), - "npm test".to_string(), + vec!["npm test".to_string()], &approvals_path, ) .unwrap(); @@ -156,9 +156,9 @@ fn test_isolated_config_safety() { // Create isolated approvals and make changes let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( "github.com/safety-test/repo".to_string(), - "THIS SHOULD NOT APPEAR IN USER APPROVALS".to_string(), + vec!["THIS SHOULD NOT APPEAR IN USER APPROVALS".to_string()], &approvals_path, ) .unwrap(); @@ -214,9 +214,9 @@ fn test_approval_saves_to_new_approvals_file() { // Create approvals and save let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( "github.com/test/nested".to_string(), - "test command".to_string(), + vec!["test command".to_string()], &approvals_path, ) .unwrap(); @@ -307,9 +307,9 @@ fn test_concurrent_approve_preserves_all_approvals() { // Process A approves and saves "npm install" approvals_a - .approve_command( + .approve_commands( "github.com/user/repo".to_string(), - "npm install".to_string(), + vec!["npm install".to_string()], &approvals_path, ) .unwrap(); @@ -324,9 +324,9 @@ fn test_concurrent_approve_preserves_all_approvals() { // Process B (which loaded BEFORE Process A saved) now approves and saves "npm test" // The save method should merge with what's on disk, not overwrite approvals_b - .approve_command( + .approve_commands( "github.com/user/repo".to_string(), - "npm test".to_string(), + vec!["npm test".to_string()], &approvals_path, ) .unwrap(); @@ -359,16 +359,16 @@ fn test_concurrent_revoke_preserves_all_changes() { // Setup: approvals file has two commands approved let mut setup_approvals = Approvals::default(); setup_approvals - .approve_command( + .approve_commands( "github.com/user/repo".to_string(), - "npm install".to_string(), + vec!["npm install".to_string()], &approvals_path, ) .unwrap(); setup_approvals - .approve_command( + .approve_commands( "github.com/user/repo".to_string(), - "npm test".to_string(), + vec!["npm test".to_string()], &approvals_path, ) .unwrap(); @@ -411,9 +411,9 @@ fn test_concurrent_approve_different_projects() { // Process A approves for project1 approvals_a - .approve_command( + .approve_commands( "github.com/user/project1".to_string(), - "npm install".to_string(), + vec!["npm install".to_string()], &approvals_path, ) .unwrap(); @@ -421,9 +421,9 @@ fn test_concurrent_approve_different_projects() { // Process B approves for project2 // Should preserve project1's approval approvals_b - .approve_command( + .approve_commands( "github.com/user/project2".to_string(), - "cargo build".to_string(), + vec!["cargo build".to_string()], &approvals_path, ) .unwrap(); @@ -479,9 +479,9 @@ fn test_truly_concurrent_approve_with_threads() { // All threads try to approve at the same time approvals - .approve_command( + .approve_commands( "github.com/user/repo".to_string(), - format!("command_{i}"), + vec![format!("command_{i}")], &approvals_path, ) .unwrap(); @@ -508,7 +508,7 @@ fn test_truly_concurrent_approve_with_threads() { } /// -/// This tests the lower-level `approve_command()` method fails when permissions +/// This tests the lower-level `approve_commands()` method fails when permissions /// are denied. On an execution path the higher-level `approve_command_batch()` /// catches this error and displays a warning, allowing commands to execute even /// when the approval can't be saved; `wt config approvals add` instead @@ -557,9 +557,9 @@ fn test_permission_error_prevents_save() { // Try to save a new approval - this should fail let mut approvals = Approvals::default(); - let result = approvals.approve_command( + let result = approvals.approve_commands( "github.com/test/readonly".to_string(), - "test command".to_string(), + vec!["test command".to_string()], &approvals_path, ); diff --git a/tests/integration_tests/approvals.rs b/tests/integration_tests/approvals.rs index 4a760ca1c6..cecda4b72e 100644 --- a/tests/integration_tests/approvals.rs +++ b/tests/integration_tests/approvals.rs @@ -106,9 +106,9 @@ fn test_list_approvals_all_approved(repo: TestRepo) { let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( repo.project_id(), - "cargo test".to_string(), + vec!["cargo test".to_string()], repo.test_approvals_path(), ) .unwrap(); @@ -125,9 +125,9 @@ fn test_list_approvals_stale_only(repo: TestRepo) { let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( repo.project_id(), - "orphan command".to_string(), + vec!["orphan command".to_string()], repo.test_approvals_path(), ) .unwrap(); @@ -282,9 +282,9 @@ fn test_clear_approvals_with_approvals(repo: TestRepo) { // Manually approve the command using the same project id wt will compute. let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( repo.project_id(), - "echo 'test'".to_string(), + vec!["echo 'test'".to_string()], repo.test_approvals_path(), ) .unwrap(); @@ -310,9 +310,9 @@ fn test_clear_approvals_global_with_approvals(repo: TestRepo) { // Manually approve the command using the same project id wt will compute. let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( repo.project_id(), - "echo 'test'".to_string(), + vec!["echo 'test'".to_string()], repo.test_approvals_path(), ) .unwrap(); @@ -367,9 +367,9 @@ fn test_clear_approvals_exact_cleared_pattern_remains(repo: TestRepo) { // under the lock, so the pattern entry survives alongside. let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( repo.project_id(), - "echo 'test'".to_string(), + vec!["echo 'test'".to_string()], repo.test_approvals_path(), ) .unwrap(); @@ -389,9 +389,9 @@ fn test_clear_approvals_after_clear(repo: TestRepo) { // Manually approve the command using the same project id wt will compute. let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( repo.project_id(), - "echo 'test'".to_string(), + vec!["echo 'test'".to_string()], repo.test_approvals_path(), ) .unwrap(); @@ -440,9 +440,9 @@ fn test_clear_approvals_stale_none(repo: TestRepo) { let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( repo.project_id(), - "cargo test".to_string(), + vec!["cargo test".to_string()], repo.test_approvals_path(), ) .unwrap(); @@ -460,9 +460,9 @@ fn test_clear_approvals_stale_no_config(repo: TestRepo) { let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( repo.project_id(), - "orphan command".to_string(), + vec!["orphan command".to_string()], repo.test_approvals_path(), ) .unwrap(); @@ -543,23 +543,23 @@ lint = "echo 'third'" let project_id = repo.project_id(); let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( project_id.clone(), - "echo 'first'".to_string(), + vec!["echo 'first'".to_string()], repo.test_approvals_path(), ) .unwrap(); approvals - .approve_command( + .approve_commands( project_id.clone(), - "echo 'second'".to_string(), + vec!["echo 'second'".to_string()], repo.test_approvals_path(), ) .unwrap(); approvals - .approve_command( + .approve_commands( project_id, - "echo 'third'".to_string(), + vec!["echo 'third'".to_string()], repo.test_approvals_path(), ) .unwrap(); @@ -584,9 +584,9 @@ fn test_add_approvals_all_already_approved(repo: TestRepo) { // Manually approve the command using the same project id wt will compute. let mut approvals = Approvals::default(); approvals - .approve_command( + .approve_commands( repo.project_id(), - "echo 'test'".to_string(), + vec!["echo 'test'".to_string()], repo.test_approvals_path(), ) .unwrap(); diff --git a/tests/integration_tests/cache_sharing.rs b/tests/integration_tests/cache_sharing.rs deleted file mode 100644 index d1ed39df9d..0000000000 --- a/tests/integration_tests/cache_sharing.rs +++ /dev/null @@ -1,117 +0,0 @@ -//! Tests for Repository cache sharing. -//! -//! These tests verify that when Repository is cloned, the cache is shared -//! across clones via Arc. This is important for performance in `wt list` -//! where parallel tasks share cached git data. - -use rstest::rstest; -use worktrunk::git::Repository; - -mod common { - pub use crate::common::*; -} -use common::{TestRepo, repo}; - -/// Test that cloning a Repository shares the same cache via Arc. -#[rstest] -fn test_repository_clone_shares_cache(repo: TestRepo) { - let repo1 = Repository::at(repo.root_path()).unwrap(); - let repo2 = repo1.clone(); - - // Both should point to the same cache (Arc pointer equality) - assert!( - repo1.shares_cache_with(&repo2), - "Cloned repositories should share the same cache" - ); -} - -/// Test that cached values computed by one clone are visible to another. -#[rstest] -fn test_cache_shared_between_clones(repo: TestRepo) { - let repo1 = Repository::at(repo.root_path()).unwrap(); - let repo2 = repo1.clone(); - - // Call default_branch() on repo1 - this caches the result - let default1 = repo1.default_branch().unwrap(); - - // Call default_branch() on repo2 - should return cached value - let default2 = repo2.default_branch().unwrap(); - - assert_eq!(default1, default2); - assert_eq!(default1, "main"); // TestRepo uses main as default branch -} - -/// Test that merge_base cache is shared across clones. -#[rstest] -fn test_merge_base_cache_shared(mut repo: TestRepo) { - // Create a branch with a commit - repo.add_worktree("feature"); - let feature_path = repo.worktree_path("feature"); - repo.commit_in_worktree(feature_path, "feature.txt", "content", "feature commit"); - - let repo1 = Repository::at(repo.root_path()).unwrap(); - let repo2 = repo1.clone(); - - // Get HEAD commits - let main_head = repo.head_sha(); - let feature_head = repo.head_sha_in(feature_path); - - // Call merge_base on repo1 - caches the result - let base1 = repo1.merge_base(&main_head, &feature_head).unwrap(); - - // Call merge_base on repo2 - should use cached value - let base2 = repo2.merge_base(&main_head, &feature_head).unwrap(); - - assert_eq!(base1, base2); - // The merge base should be main's HEAD since feature branched from there - assert_eq!(base1, Some(main_head)); -} - -/// Test that parallel tasks share the cache when cloning Repository. -#[rstest] -fn test_parallel_tasks_share_cache(mut repo: TestRepo) { - use std::thread; - - // Create multiple worktrees - repo.add_worktree("feature-a"); - repo.add_worktree("feature-b"); - - let repo1 = Repository::at(repo.root_path()).unwrap(); - - // Spawn threads that clone the repo and access cached values - let handles: Vec<_> = (0..4) - .map(|i| { - let repo_clone = repo1.clone(); - thread::spawn(move || { - // Each thread accesses the same cached default_branch - let default = repo_clone.default_branch().unwrap(); - (i, default) - }) - }) - .collect(); - - // All threads should get the same cached value - let results: Vec<_> = handles.into_iter().map(|h| h.join().unwrap()).collect(); - for (_, branch) in &results { - assert_eq!(branch, "main"); - } -} - -/// Test that separate Repository instances (not clones) have separate caches. -#[rstest] -fn test_separate_repositories_have_separate_caches(repo: TestRepo) { - // Create two separate Repository instances (not clones) - let repo1 = Repository::at(repo.root_path()).unwrap(); - let repo2 = Repository::at(repo.root_path()).unwrap(); - - // They should NOT share the same cache - assert!( - !repo1.shares_cache_with(&repo2), - "Separately created repositories should have independent caches" - ); - - // But they should compute the same values - let default1 = repo1.default_branch().unwrap(); - let default2 = repo2.default_branch().unwrap(); - assert_eq!(default1, default2); -} diff --git a/tests/integration_tests/config_state.rs b/tests/integration_tests/config_state.rs index 0b46c53e0d..f1a0504770 100644 --- a/tests/integration_tests/config_state.rs +++ b/tests/integration_tests/config_state.rs @@ -2579,6 +2579,14 @@ fn test_vars_clear_single_key(repo: TestRepo) { assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), ""); } +#[rstest] +fn test_vars_clear_requires_key_or_all(repo: TestRepo) { + let output = wt_state_cmd(&repo, "vars", "clear", &[]).output().unwrap(); + + assert!(!output.status.success()); + assert_snapshot!(String::from_utf8_lossy(&output.stderr), @"✗ Specify a key to clear, or use --all to clear all keys"); +} + #[rstest] fn test_vars_clear_all(repo: TestRepo) { // Set multiple values diff --git a/tests/integration_tests/configure_shell.rs b/tests/integration_tests/configure_shell.rs index 449359331a..d322dc9672 100644 --- a/tests/integration_tests/configure_shell.rs +++ b/tests/integration_tests/configure_shell.rs @@ -5,13 +5,23 @@ use crate::common::{ use insta_cmd::assert_cmd_snapshot; use rstest::rstest; use std::fs; +#[cfg(unix)] +use std::os::unix::fs::{PermissionsExt, symlink}; use tempfile::TempDir; #[rstest] fn test_configure_shell_with_yes(repo: TestRepo, temp_home: TempDir) { // Create a fake .zshrc file let zshrc_path = temp_home.path().join(".zshrc"); - fs::write(&zshrc_path, "# Existing config\n").unwrap(); + fs::write(&zshrc_path, "# Existing config").unwrap(); + + #[cfg(unix)] + { + let target_path = temp_home.path().join("zshrc"); + fs::rename(&zshrc_path, &target_path).unwrap(); + fs::set_permissions(&target_path, fs::Permissions::from_mode(0o640)).unwrap(); + symlink(&target_path, &zshrc_path).unwrap(); + } let settings = setup_home_snapshot_settings(&temp_home); settings.bind(|| { @@ -44,14 +54,30 @@ fn test_configure_shell_with_yes(repo: TestRepo, temp_home: TempDir) { // Verify the file was modified let content = fs::read_to_string(&zshrc_path).unwrap(); - assert!(content.contains("eval \"$(command wt config shell init zsh)\"")); + assert_eq!( + content, + "# Existing config\nif command -v wt >/dev/null 2>&1; then eval \"$(command wt config shell init zsh)\"; fi\n" + ); + + #[cfg(unix)] + assert!( + fs::symlink_metadata(&zshrc_path) + .unwrap() + .file_type() + .is_symlink() + ); + #[cfg(unix)] + assert_eq!( + fs::metadata(&zshrc_path).unwrap().permissions().mode() & 0o777, + 0o640 + ); } #[rstest] fn test_configure_shell_specific_shell(repo: TestRepo, temp_home: TempDir) { // Create a fake .zshrc file let zshrc_path = temp_home.path().join(".zshrc"); - fs::write(&zshrc_path, "# Existing config\n").unwrap(); + fs::write(&zshrc_path, "").unwrap(); let settings = setup_home_snapshot_settings(&temp_home); settings.bind(|| { @@ -85,7 +111,10 @@ fn test_configure_shell_specific_shell(repo: TestRepo, temp_home: TempDir) { // Verify the file was modified let content = fs::read_to_string(&zshrc_path).unwrap(); - assert!(content.contains("eval \"$(command wt config shell init zsh)\"")); + assert_eq!( + content, + "\nif command -v wt >/dev/null 2>&1; then eval \"$(command wt config shell init zsh)\"; fi\n" + ); } #[rstest] @@ -239,6 +268,31 @@ fn test_configure_shell_fish(repo: TestRepo, temp_home: TempDir) { ); } +#[rstest] +#[cfg(unix)] +fn test_configure_shell_rejects_completion_directory(repo: TestRepo, temp_home: TempDir) { + let completion = temp_home.path().join(".config/fish/completions/wt.fish"); + fs::create_dir_all(&completion).unwrap(); + + let settings = setup_home_snapshot_settings(&temp_home); + settings.bind(|| { + let mut cmd = wt_command(); + repo.configure_wt_cmd(&mut cmd); + set_temp_home_env(&mut cmd, temp_home.path()); + cmd.args(["config", "shell", "install", "fish", "--yes"]) + .current_dir(repo.root_path()); + + assert_cmd_snapshot!(cmd, @" + success: false + exit_code: 1 + ----- stdout ----- + + ----- stderr ----- + ✗ Failed to read ~/.config/fish/completions/wt.fish: Is a directory (os error 21) + "); + }); +} + /// Test install dry-run shows preview with gutter-formatted config content #[rstest] fn test_configure_shell_fish_dry_run(repo: TestRepo, temp_home: TempDir) { @@ -1714,9 +1768,11 @@ fn test_install_uninstall_no_blank_line_accumulation(repo: TestRepo, temp_home: } let after_install = fs::read_to_string(&zshrc_path).unwrap(); - assert!( - after_install.contains("wt config shell init zsh"), - "Integration should be added" + assert_eq!( + after_install, + format!( + "{initial_content}\nif command -v wt >/dev/null 2>&1; then eval \"$(command wt config shell init zsh)\"; fi\n" + ) ); // Uninstall @@ -2313,7 +2369,7 @@ fn test_uninstall_shell_dry_run_nushell(repo: TestRepo, temp_home: TempDir) { // PTY-based tests for interactive install preview #[cfg(all(unix, feature = "shell-integration-tests"))] mod pty_tests { - use crate::common::pty::exec_cmd_in_pty_prompted; + use crate::common::pty::{exec_cmd_in_pty_prompted, exec_cmd_in_pty_prompted_with}; use crate::common::{ TestRepo, add_pty_filters, configure_pty_command, repo, temp_home, wt_bin, }; @@ -2323,8 +2379,10 @@ mod pty_tests { use std::fs; use tempfile::TempDir; - /// Execute shell install command in a PTY, waiting for prompt before input - fn exec_install_in_pty(temp_home: &TempDir, repo: &TestRepo, input: &str) -> (String, i32) { + const ZSH_INTEGRATION: &str = + "if command -v wt >/dev/null 2>&1; then eval \"$(command wt config shell init zsh)\"; fi"; + + fn install_command(temp_home: &TempDir, repo: &TestRepo) -> CommandBuilder { let mut cmd = CommandBuilder::new(wt_bin()); cmd.arg("-C"); cmd.arg(repo.root_path()); @@ -2349,7 +2407,26 @@ mod pty_tests { // Using MISSING=1 skips the probe while still showing the compinit advisory. cmd.env("WORKTRUNK_TEST_COMPINIT_MISSING", "1"); - exec_cmd_in_pty_prompted(cmd, &[input], "[y/N") + cmd + } + + /// Execute shell install command in a PTY, waiting for prompt before input + fn exec_install_in_pty(temp_home: &TempDir, repo: &TestRepo, input: &str) -> (String, i32) { + exec_cmd_in_pty_prompted(install_command(temp_home, repo), &[input], "[y/N") + } + + fn uninstall_command(temp_home: &TempDir, repo: &TestRepo) -> CommandBuilder { + let mut cmd = CommandBuilder::new(wt_bin()); + cmd.arg("-C"); + cmd.arg(repo.root_path()); + cmd.args(["config", "shell", "uninstall", "zsh"]); + cmd.cwd(repo.root_path()); + + configure_pty_command(&mut cmd); + cmd.env("HOME", temp_home.path()); + cmd.env("XDG_CONFIG_HOME", temp_home.path().join(".config")); + cmd.env("SHELL", "/bin/zsh"); + cmd } /// Create insta settings for install PTY tests. @@ -2409,6 +2486,29 @@ mod pty_tests { ); } + #[rstest] + fn test_install_preserves_unpreviewed_fish_completions(repo: TestRepo, temp_home: TempDir) { + fs::write(temp_home.path().join(".zshrc"), "# Existing config\n").unwrap(); + + let functions = temp_home.path().join(".config/fish/functions"); + let completion = temp_home.path().join(".config/fish/completions/wt.fish"); + let callback_completion = completion.clone(); + let user_content = b"# user-owned fish completions\r\n"; + let (output, exit_code) = exec_cmd_in_pty_prompted_with( + install_command(&temp_home, &repo), + &["y\n"], + "[y/N", + move |_| { + fs::create_dir_all(&functions).unwrap(); + fs::create_dir_all(callback_completion.parent().unwrap()).unwrap(); + fs::write(&callback_completion, user_content).unwrap(); + }, + ); + + assert_eq!(exit_code, 0, "install should succeed:\n{output}"); + assert_eq!(fs::read(completion).unwrap(), user_content); + } + /// Typing `?` at the install prompt re-shows the preview (the interactive /// re-preview path), then declining with `n` leaves the rcfile untouched. #[rstest] @@ -2443,21 +2543,10 @@ mod pty_tests { #[rstest] fn test_uninstall_preview_declined(repo: TestRepo, temp_home: TempDir) { let zshrc_path = temp_home.path().join(".zshrc"); - let initial = "# Existing config\nif command -v wt >/dev/null 2>&1; then eval \"$(command wt config shell init zsh)\"; fi\n"; - fs::write(&zshrc_path, initial).unwrap(); + let initial = format!("# Existing config\n{ZSH_INTEGRATION}\n"); + fs::write(&zshrc_path, &initial).unwrap(); - let mut cmd = CommandBuilder::new(wt_bin()); - cmd.arg("-C"); - cmd.arg(repo.root_path()); - cmd.arg("config"); - cmd.arg("shell"); - cmd.arg("uninstall"); - cmd.cwd(repo.root_path()); - - configure_pty_command(&mut cmd); - cmd.env("HOME", temp_home.path()); - cmd.env("XDG_CONFIG_HOME", temp_home.path().join(".config")); - cmd.env("SHELL", "/bin/zsh"); + let cmd = uninstall_command(&temp_home, &repo); let (output, exit_code) = exec_cmd_in_pty_prompted(cmd, &["n\n"], "[y/N"); @@ -2473,6 +2562,53 @@ mod pty_tests { "Declining uninstall should leave the rcfile untouched", ); } + + /// The preview authorizes an exact rc-line multiplicity, not a rescan. + #[rstest] + fn test_uninstall_preserves_lines_added_after_preview(repo: TestRepo, temp_home: TempDir) { + let zshrc_path = temp_home.path().join(".zshrc"); + let initial = format!("# Existing config\n{ZSH_INTEGRATION}\n"); + fs::write(&zshrc_path, initial).unwrap(); + + let cmd = uninstall_command(&temp_home, &repo); + let callback_path = zshrc_path.clone(); + let concurrent = + format!("# Existing config\n{ZSH_INTEGRATION}\n{ZSH_INTEGRATION}\nexport PAGER=less\n"); + let (output, exit_code) = exec_cmd_in_pty_prompted_with(cmd, &["y\n"], "[y/N", move |_| { + fs::write(&callback_path, &concurrent).unwrap(); + }); + + assert_eq!(exit_code, 0, "uninstall should succeed:\n{output}"); + assert_eq!( + fs::read_to_string(&zshrc_path).unwrap(), + format!("# Existing config\n{ZSH_INTEGRATION}\nexport PAGER=less\n"), + ); + } + + #[rstest] + fn test_uninstall_preserves_rc_replaced_after_preview(repo: TestRepo, temp_home: TempDir) { + let zshrc_path = temp_home.path().join(".zshrc"); + fs::write(&zshrc_path, format!("{ZSH_INTEGRATION}\n")).unwrap(); + + let cmd = uninstall_command(&temp_home, &repo); + let callback_path = zshrc_path.clone(); + let replacement = b"export PAGER=less\r\n"; + let (output, exit_code) = exec_cmd_in_pty_prompted_with(cmd, &["y\n"], "[y/N", move |_| { + fs::write(&callback_path, replacement).unwrap(); + }); + + assert_eq!(exit_code, 0, "uninstall should succeed:\n{output}"); + assert_eq!(fs::read(&zshrc_path).unwrap(), replacement); + install_pty_settings(&temp_home).bind(|| { + assert_snapshot!(output.trim_start_matches('\n'), @r#" + ○ Will remove shell extension & completions for zsh @ ~/.zshrc +   if command -v wt >/dev/null 2>&1; then eval "$(command wt config shell init zsh)"; fi + + ❯ Proceed? [y/N] y + ▲ No shell extension & completions found in ~/.zshrc + "#); + }); + } } /// Test installing nushell shell integration @@ -2639,8 +2775,8 @@ fn test_uninstall_shell_nushell(repo: TestRepo, temp_home: TempDir) { /// Test that nushell uninstall cleans up the wrapper at every candidate /// location — the canonical vendor-autoload dir and the legacy /// `/vendor/autoload` paths older worktrunk stranded files at -/// (issue #2878). `config_paths(Nushell)` returns all of them and uninstall -/// iterates the full list. +/// (issue #2878). `Shell::Nushell.config_paths(cmd)` returns all candidate +/// locations, and uninstall checks each one. #[rstest] fn test_uninstall_nushell_cleans_all_candidate_locations(repo: TestRepo, temp_home: TempDir) { let home = canonical_temp_home(&temp_home); diff --git a/tests/integration_tests/default_branch.rs b/tests/integration_tests/default_branch.rs index 97d24c4c18..0f2009a7e8 100644 --- a/tests/integration_tests/default_branch.rs +++ b/tests/integration_tests/default_branch.rs @@ -686,7 +686,7 @@ fn test_push_remote_url_returns_non_github_url(repo: TestRepo) { .expect("push_remote_url resolves the configured remote regardless of host"); let parsed = GitRemoteUrl::parse(&url).unwrap(); assert!(!parsed.is_github()); - assert!(parsed.is_gitlab()); + assert_eq!(parsed.host(), "gitlab.com"); } /// `push_remote_url`: result is cached on the Repository. @@ -738,7 +738,7 @@ fn test_push_remote_url_insteadof_resolves_to_non_github(repo: TestRepo) { .push_remote_url() .expect("push_remote_url resolves through insteadOf regardless of host"); let parsed = GitRemoteUrl::parse(&url).unwrap(); - assert!(parsed.is_gitlab()); + assert_eq!(parsed.host(), "gitlab.com"); assert!(!parsed.is_github()); } diff --git a/tests/integration_tests/mod.rs b/tests/integration_tests/mod.rs index af43060cca..bfc30ed7e0 100644 --- a/tests/integration_tests/mod.rs +++ b/tests/integration_tests/mod.rs @@ -10,7 +10,6 @@ pub mod approval_save; pub mod approval_ui; pub mod approvals; pub mod bare_repository; -pub mod cache_sharing; pub mod ci_status; pub mod completion; pub mod completion_validation; diff --git a/tests/integration_tests/repository.rs b/tests/integration_tests/repository.rs index c9181c438e..8821be5f3a 100644 --- a/tests/integration_tests/repository.rs +++ b/tests/integration_tests/repository.rs @@ -175,40 +175,41 @@ fn test_operation_in_progress_reads_the_queued_sequencer() { assert_eq!(repository.operation_in_progress().unwrap(), None); } -// ============================================================================= -// available_branches() tests -// ============================================================================= - -#[test] -fn test_available_branches_all_have_worktrees() { - let mut repo = TestRepo::new(); - // main branch already has a worktree (the main repo) - // Create feature branch with worktree - repo.add_worktree("feature"); - - let repository = Repository::at(repo.root_path().to_path_buf()).unwrap(); - let available = repository.available_branches().unwrap(); - - // Both main and feature have worktrees, so nothing should be available - assert!(available.is_empty()); -} - +/// `wt list` clones one repository into parallel tasks, so hot cache entries +/// must remain usable across both the clone and thread boundaries. #[test] -fn test_available_branches_some_without_worktrees() { - let repo = TestRepo::with_initial_commit(); - // Create a branch without a worktree - repo.git_command() - .args(["branch", "orphan-branch"]) - .run() - .unwrap(); - - let repository = Repository::at(repo.root_path().to_path_buf()).unwrap(); - let available = repository.available_branches().unwrap(); - - // orphan-branch has no worktree, so it should be available - assert!(available.contains(&"orphan-branch".to_string())); - // main has a worktree, so it should not be available - assert!(!available.contains(&"main".to_string())); +fn test_cloned_repository_shares_cached_queries_across_threads() { + let mut test = TestRepo::with_initial_commit(); + test.add_worktree("feature"); + let feature_path = test.worktree_path("feature"); + test.commit_in_worktree(feature_path, "feature.txt", "content", "feature commit"); + + let repository = Repository::at(test.root_path()).unwrap(); + let main_head = test.head_sha(); + let feature_head = test.head_sha_in(feature_path); + let expected_base = repository.merge_base(&main_head, &feature_head).unwrap(); + assert_eq!(expected_base, Some(main_head.clone())); + + let handles: Vec<_> = (0..4) + .map(|_| { + let repository = repository.clone(); + let main_head = main_head.clone(); + let feature_head = feature_head.clone(); + std::thread::spawn(move || { + ( + repository.default_branch(), + repository.merge_base(&main_head, &feature_head).unwrap(), + ) + }) + }) + .collect(); + + for handle in handles { + assert_eq!( + handle.join().unwrap(), + (Some("main".into()), expected_base.clone()) + ); + } } // ============================================================================= diff --git a/tests/integration_tests/shell_integration_prompt.rs b/tests/integration_tests/shell_integration_prompt.rs index 01c7c0ceba..bbf331bef7 100644 --- a/tests/integration_tests/shell_integration_prompt.rs +++ b/tests/integration_tests/shell_integration_prompt.rs @@ -305,7 +305,9 @@ fn test_process_tree_unsupported_shell_overrides_shell_env(repo: TestRepo) { #[cfg(all(unix, feature = "shell-integration-tests"))] mod pty_tests { use super::*; - use crate::common::pty::{build_pty_command, exec_cmd_in_pty, exec_cmd_in_pty_prompted}; + use crate::common::pty::{ + build_pty_command, exec_cmd_in_pty, exec_cmd_in_pty_prompted, exec_cmd_in_pty_prompted_with, + }; use crate::common::{add_pty_filters, setup_snapshot_settings, wt_bin}; use insta::assert_snapshot; use std::path::Path; @@ -493,6 +495,34 @@ mod pty_tests { }); } + #[rstest] + fn test_first_run_preserves_completion_created_after_preview(repo: TestRepo) { + let temp_home = TempDir::new().unwrap(); + fs::write(temp_home.path().join(".bashrc"), "# empty bashrc\n").unwrap(); + fs::create_dir_all(temp_home.path().join(".config/fish/functions")).unwrap(); + + let mut env_vars = repo.test_env_vars(); + env_vars.push(("SHELL".to_string(), "/bin/bash".to_string())); + let cmd = build_pty_command( + wt_bin().to_str().unwrap(), + &["switch", "--create", "feature"], + repo.root_path(), + &env_vars, + Some(temp_home.path()), + ); + let completion = temp_home.path().join(".config/fish/completions/wt.fish"); + let callback_path = completion.clone(); + let user_content = b"# user completion\r\n"; + + let (output, exit_code) = exec_cmd_in_pty_prompted_with(cmd, &["y\n"], "[y/N", move |_| { + fs::create_dir_all(callback_path.parent().unwrap()).unwrap(); + fs::write(&callback_path, user_content).unwrap(); + }); + + assert_eq!(exit_code, 0, "switch should still succeed:\n{output}"); + assert_eq!(fs::read(completion).unwrap(), user_content); + } + /// Test: User requests preview with ? then declines #[rstest] fn test_user_requests_preview_then_declines(repo: TestRepo) { @@ -606,6 +636,10 @@ mod pty_tests { output.contains("Will remove") && output.contains("conf.d/wt.fish"), "First-run offer preview must name the legacy fish removal: {output}" ); + assert!( + output.contains("Will create completions") && output.contains("completions/wt.fish"), + "First-run offer preview must name the fish completion write: {output}" + ); // Declining leaves the legacy file in place — the preview did not delete it. assert!(