Skip to content

Commit a567d5d

Browse files
max-sixtyclaude
andauthored
refactor: simplify command structure and remove dead code (#892)
- Extract switch handler from main.rs to handle_switch.rs - Centralize hook execution in hooks.rs (execute_hook, spawn_hook_background) - Remove dead for_action method, rename for_action_with_config to for_action - Move approval logic into handle_squash (matching step_commit pattern) - Remove redundant .clone() calls in return blocks (Rust is flow-sensitive) - Remove redundant .to_string()/.to_path_buf() on types that already return owned values - Delete test_squash_result_clone test (testing derived Clone adds no value) Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2067ac4 commit a567d5d

File tree

10 files changed

+433
-441
lines changed

10 files changed

+433
-441
lines changed

src/commands/context.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,10 @@ impl CommandEnv {
2828
///
2929
/// `action` describes what command is running (e.g., "merge", "squash").
3030
/// Used in error messages when the environment can't be loaded.
31-
pub fn for_action(action: &str) -> anyhow::Result<Self> {
31+
pub fn for_action(action: &str, config: UserConfig) -> anyhow::Result<Self> {
3232
let repo = Repository::current()?;
3333
let worktree_path = std::env::current_dir().context("Failed to get current directory")?;
3434
let branch = repo.require_current_branch(action)?;
35-
let config = UserConfig::load().context("Failed to load config")?;
3635

3736
Ok(Self {
3837
repo,

src/commands/handle_switch.rs

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
//! Switch command handler.
2+
3+
use std::collections::HashMap;
4+
5+
use anyhow::Context;
6+
use worktrunk::HookType;
7+
use worktrunk::config::{UserConfig, expand_template};
8+
use worktrunk::git::Repository;
9+
use worktrunk::styling::{eprintln, info_message};
10+
11+
use super::command_approval::approve_hooks;
12+
use super::command_executor::{CommandContext, build_hook_context};
13+
use super::worktree::{SwitchResult, execute_switch, plan_switch};
14+
use crate::output::{
15+
execute_user_command, handle_switch_output, is_shell_integration_active,
16+
prompt_shell_integration,
17+
};
18+
19+
/// Options for the switch command
20+
pub struct SwitchOptions<'a> {
21+
pub branch: &'a str,
22+
pub create: bool,
23+
pub base: Option<&'a str>,
24+
pub execute: Option<&'a str>,
25+
pub execute_args: &'a [String],
26+
pub yes: bool,
27+
pub clobber: bool,
28+
pub verify: bool,
29+
}
30+
31+
/// Handle the switch command.
32+
pub fn handle_switch(
33+
opts: SwitchOptions<'_>,
34+
config: &mut UserConfig,
35+
binary_name: &str,
36+
) -> anyhow::Result<()> {
37+
let SwitchOptions {
38+
branch,
39+
create,
40+
base,
41+
execute,
42+
execute_args,
43+
yes,
44+
clobber,
45+
verify,
46+
} = opts;
47+
48+
let repo = Repository::current().context("Failed to switch worktree")?;
49+
50+
// Validate FIRST (before approval) - fails fast if branch doesn't exist, etc.
51+
let plan = plan_switch(&repo, branch, create, base, clobber, config)?;
52+
53+
// "Approve at the Gate": collect and approve hooks upfront
54+
// This ensures approval happens once at the command entry point
55+
// If user declines, skip hooks but continue with worktree operation
56+
let approved = if verify {
57+
let ctx = CommandContext::new(
58+
&repo,
59+
config,
60+
Some(plan.branch()),
61+
plan.worktree_path(),
62+
yes,
63+
);
64+
// Approve different hooks based on whether we're creating or switching
65+
if plan.is_create() {
66+
approve_hooks(
67+
&ctx,
68+
&[
69+
HookType::PostCreate,
70+
HookType::PostStart,
71+
HookType::PostSwitch,
72+
],
73+
)?
74+
} else {
75+
// When switching to existing, only post-switch needs approval
76+
approve_hooks(&ctx, &[HookType::PostSwitch])?
77+
}
78+
} else {
79+
true // --no-verify: skip all hooks
80+
};
81+
82+
// Skip hooks if --no-verify or user declined approval
83+
let skip_hooks = !verify || !approved;
84+
85+
// Show message if user declined approval
86+
if !approved {
87+
eprintln!(
88+
"{}",
89+
info_message(if plan.is_create() {
90+
"Commands declined, continuing worktree creation"
91+
} else {
92+
"Commands declined"
93+
})
94+
);
95+
}
96+
97+
// Execute the validated plan
98+
let (result, branch_info) = execute_switch(&repo, plan, config, yes, skip_hooks)?;
99+
100+
// Show success message (temporal locality: immediately after worktree operation)
101+
// Returns path to display in hooks when user's shell won't be in the worktree
102+
// Also shows worktree-path hint on first --create (before shell integration warning)
103+
let hooks_display_path = handle_switch_output(&result, &branch_info)?;
104+
105+
// Offer shell integration if not already installed/active
106+
// (only shows prompt/hint when shell integration isn't working)
107+
// With --execute: show hints only (don't interrupt with prompt)
108+
// Best-effort: don't fail switch if offer fails
109+
if !is_shell_integration_active() {
110+
let skip_prompt = execute.is_some();
111+
let _ = prompt_shell_integration(config, binary_name, skip_prompt);
112+
}
113+
114+
// Build extra vars for base branch context (used by both hooks and --execute)
115+
// "base" is the branch we branched from when creating a new worktree.
116+
// For existing worktrees, there's no base concept.
117+
let (base_branch, base_worktree_path): (Option<&str>, Option<&str>) = match &result {
118+
SwitchResult::Created {
119+
base_branch,
120+
base_worktree_path,
121+
..
122+
} => (base_branch.as_deref(), base_worktree_path.as_deref()),
123+
SwitchResult::Existing { .. } | SwitchResult::AlreadyAt(_) => (None, None),
124+
};
125+
let extra_vars: Vec<(&str, &str)> = [
126+
base_branch.map(|b| ("base", b)),
127+
base_worktree_path.map(|p| ("base_worktree_path", p)),
128+
]
129+
.into_iter()
130+
.flatten()
131+
.collect();
132+
133+
// Spawn background hooks after success message
134+
// - post-switch: runs on ALL switches (shows "@ path" when shell won't be there)
135+
// - post-start: runs only when creating a NEW worktree
136+
if !skip_hooks {
137+
let ctx = CommandContext::new(&repo, config, Some(&branch_info.branch), result.path(), yes);
138+
139+
// Post-switch runs first (immediate "I'm here" signal)
140+
ctx.spawn_post_switch_commands(&extra_vars, hooks_display_path.as_deref())?;
141+
142+
// Post-start runs only on creation (setup tasks)
143+
if matches!(&result, SwitchResult::Created { .. }) {
144+
ctx.spawn_post_start_commands(&extra_vars, hooks_display_path.as_deref())?;
145+
}
146+
}
147+
148+
// Execute user command after post-start hooks have been spawned
149+
// Note: execute_args requires execute via clap's `requires` attribute
150+
if let Some(cmd) = execute {
151+
// Build template context for expansion (includes base vars when creating)
152+
let ctx = CommandContext::new(&repo, config, Some(&branch_info.branch), result.path(), yes);
153+
let template_vars = build_hook_context(&ctx, &extra_vars);
154+
let vars: HashMap<&str, &str> = template_vars
155+
.iter()
156+
.map(|(k, v)| (k.as_str(), v.as_str()))
157+
.collect();
158+
159+
// Expand template variables in command (shell_escape: true for safety)
160+
let expanded_cmd = expand_template(cmd, &vars, true, &repo, "--execute command")
161+
.map_err(|e| anyhow::anyhow!("Failed to expand --execute template: {}", e))?;
162+
163+
// Append any trailing args (after --) to the execute command
164+
// Each arg is also expanded, then shell-escaped
165+
let full_cmd = if execute_args.is_empty() {
166+
expanded_cmd
167+
} else {
168+
let expanded_args: Result<Vec<_>, _> = execute_args
169+
.iter()
170+
.map(|arg| {
171+
expand_template(arg, &vars, false, &repo, "--execute argument")
172+
.map_err(|e| anyhow::anyhow!("Failed to expand argument template: {}", e))
173+
})
174+
.collect();
175+
let escaped_args: Vec<_> = expanded_args?
176+
.iter()
177+
.map(|arg| shlex::try_quote(arg).unwrap_or(arg.into()).into_owned())
178+
.collect();
179+
format!("{} {}", expanded_cmd, escaped_args.join(" "))
180+
};
181+
execute_user_command(&full_cmd, hooks_display_path.as_deref())?;
182+
}
183+
184+
Ok(())
185+
}

src/commands/hook_commands.rs

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,11 @@ use super::command_executor::build_hook_context;
2626

2727
use super::command_executor::CommandContext;
2828
use super::context::CommandEnv;
29+
use super::hooks::execute_hook;
2930
use super::hooks::{
3031
HookFailureStrategy, check_name_filter_matched, prepare_hook_commands, run_hook_with_filter,
3132
spawn_hook_commands_background,
3233
};
33-
use super::merge::{
34-
execute_post_merge_commands, execute_pre_remove_commands, run_pre_merge_commands,
35-
};
3634
use super::project_config::collect_commands_for_hooks;
3735

3836
/// Handle `wt hook` command
@@ -224,35 +222,40 @@ pub fn run_hook(
224222
)
225223
}
226224
HookType::PreMerge => {
227-
// pre-merge, post-merge, pre-remove use functions from merge.rs
228-
// which already handle user hooks (approval already happened at gate)
229225
// Use current branch as target (matches approval prompt for wt hook)
230-
let project_cfg = project_config.unwrap_or_default();
231-
run_pre_merge_commands(
232-
&project_cfg,
226+
let mut vars = vec![("target", ctx.branch_or_head())];
227+
vars.extend(custom_vars_refs.iter().cloned());
228+
execute_hook(
233229
&ctx,
234-
ctx.branch_or_head(),
230+
HookType::PreMerge,
231+
&vars,
232+
HookFailureStrategy::FailFast,
235233
name_filter,
236-
&custom_vars_refs,
234+
crate::output::pre_hook_display_path(ctx.worktree_path),
237235
)
238236
}
239237
HookType::PostMerge => {
240238
// Manual wt hook: user stays at cwd (no cd happens)
241-
execute_post_merge_commands(
239+
let mut vars = vec![("target", ctx.branch_or_head())];
240+
vars.extend(custom_vars_refs.iter().cloned());
241+
execute_hook(
242242
&ctx,
243-
ctx.branch_or_head(),
243+
HookType::PostMerge,
244+
&vars,
245+
HookFailureStrategy::Warn,
244246
name_filter,
245247
crate::output::pre_hook_display_path(ctx.worktree_path),
246-
&custom_vars_refs,
247248
)
248249
}
249250
HookType::PreRemove => {
250251
// Manual wt hook: user stays at cwd (no cd happens)
251-
execute_pre_remove_commands(
252+
execute_hook(
252253
&ctx,
254+
HookType::PreRemove,
255+
&custom_vars_refs,
256+
HookFailureStrategy::FailFast,
253257
name_filter,
254258
crate::output::pre_hook_display_path(ctx.worktree_path),
255-
&custom_vars_refs,
256259
)
257260
}
258261
HookType::PostRemove => {

src/commands/hooks.rs

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,84 @@ pub fn run_hook_with_filter(
357357
Ok(())
358358
}
359359

360+
/// Look up user and project configs for a given hook type.
361+
fn lookup_hook_configs<'a>(
362+
user_hooks: &'a worktrunk::config::HooksConfig,
363+
project_config: Option<&'a worktrunk::config::ProjectConfig>,
364+
hook_type: HookType,
365+
) -> (Option<&'a CommandConfig>, Option<&'a CommandConfig>) {
366+
(
367+
user_hooks.get(hook_type),
368+
project_config.and_then(|c| c.hooks.get(hook_type)),
369+
)
370+
}
371+
372+
/// Run a hook type with automatic config lookup.
373+
///
374+
/// This is a convenience wrapper that:
375+
/// 1. Loads project config from the repository
376+
/// 2. Looks up user hooks from the config
377+
/// 3. Calls `run_hook_with_filter` with the appropriate hook configs
378+
/// 4. Adds the hook skip hint to errors
379+
///
380+
/// Use this instead of manually looking up configs and calling `run_hook_with_filter`.
381+
pub fn execute_hook(
382+
ctx: &CommandContext,
383+
hook_type: HookType,
384+
extra_vars: &[(&str, &str)],
385+
failure_strategy: HookFailureStrategy,
386+
name_filter: Option<&str>,
387+
display_path: Option<&Path>,
388+
) -> anyhow::Result<()> {
389+
let project_config = ctx.repo.load_project_config()?;
390+
let user_hooks = ctx.config.hooks(ctx.project_id().as_deref());
391+
let (user_config, proj_config) =
392+
lookup_hook_configs(&user_hooks, project_config.as_ref(), hook_type);
393+
394+
run_hook_with_filter(
395+
ctx,
396+
user_config,
397+
proj_config,
398+
hook_type,
399+
extra_vars,
400+
failure_strategy,
401+
name_filter,
402+
display_path,
403+
)
404+
.map_err(worktrunk::git::add_hook_skip_hint)
405+
}
406+
407+
/// Spawn hook commands as background processes with automatic config lookup.
408+
///
409+
/// This is a convenience wrapper that:
410+
/// 1. Loads project config from the repository
411+
/// 2. Looks up user hooks from the config
412+
/// 3. Prepares and spawns background commands
413+
pub fn spawn_hook_background(
414+
ctx: &CommandContext,
415+
hook_type: HookType,
416+
extra_vars: &[(&str, &str)],
417+
name_filter: Option<&str>,
418+
display_path: Option<&Path>,
419+
) -> anyhow::Result<()> {
420+
let project_config = ctx.repo.load_project_config()?;
421+
let user_hooks = ctx.config.hooks(ctx.project_id().as_deref());
422+
let (user_config, proj_config) =
423+
lookup_hook_configs(&user_hooks, project_config.as_ref(), hook_type);
424+
425+
let commands = prepare_hook_commands(
426+
ctx,
427+
user_config,
428+
proj_config,
429+
hook_type,
430+
extra_vars,
431+
name_filter,
432+
display_path,
433+
)?;
434+
435+
spawn_hook_commands_background(ctx, commands, hook_type)
436+
}
437+
360438
#[cfg(test)]
361439
mod tests {
362440
use super::*;

0 commit comments

Comments
 (0)