merge: fix unlink validation and HookRunner state batching (closes #10, #13)

crates/notfiles/src/linker.rs

@@ -217,11 +217,21 @@ pub fn link_package(
 }
 
 pub fn unlink_package(
-    _dotfiles_dir: &Path,
+    dotfiles_dir: &Path,
     state: &mut State,
     package: &str,
     opts: &LinkOptions,
 ) -> Result<(), NotfilesError> {
+    // Validate the package: it must either exist as a directory in dotfiles_dir
+    // or have entries in state.  A name that satisfies neither is a user error.
+    let package_dir = dotfiles_dir.join(package);
+    let has_state_entries = !state.entries_for_package(package).is_empty();
+    if !package_dir.is_dir() && !has_state_entries {
+        return Err(NotfilesError::PackageNotFound {
+            name: package.to_string(),
+        });
+    }
+
     let entries: Vec<StateEntry> = state
         .entries_for_package(package)
         .into_iter()

crates/nothooks/src/lib.rs

@@ -1,7 +1,7 @@
 pub mod runner;
 pub mod state;
 
-use notcore::{HookPhase, HookSpec, Report, StepStatus};
+use notcore::{HookPhase, HookSpec, Report};
 pub use runner::HookRunner;
 
 #[derive(Debug, PartialEq)]
@@ -12,16 +12,8 @@ pub enum HookResult {
 }
 
 /// Run all hooks matching `phase` and collect into a `Report`.
+///
+/// State is loaded once and saved once per call — not once per hook.
 pub fn run_phase(hooks: &[HookSpec], phase: &HookPhase, runner: &HookRunner) -> Report {
-    let mut report = Report::default();
+    runner.run_phase(hooks, phase)
-    for hook in hooks.iter().filter(|h| &h.phase == phase) {
-        let result = runner.run_hook(hook);
-        let status = match &result {
-            HookResult::Ok => StepStatus::Ok,
-            HookResult::Skipped => StepStatus::Skipped,
-            HookResult::Failed(msg) => StepStatus::Failed(msg.clone()),
-        };
-        report.add(&hook.name, status);
-    }
-    report
 }

crates/nothooks/src/runner.rs

@@ -1,6 +1,6 @@
 use crate::HookResult;
 use crate::state::HookState;
-use notcore::{HookPhase, HookSpec};
+use notcore::{HookPhase, HookSpec, Report, StepStatus};
 use std::path::PathBuf;
 use std::process::Command;
 
@@ -46,9 +46,41 @@ impl HookRunner {
         }
     }
 
-    pub fn run_hook(&self, spec: &HookSpec) -> HookResult {
+    /// Run all hooks in `hooks` that match `phase`.
+    ///
+    /// State is loaded once before iterating and saved once at the end (only if
+    /// at least one Setup hook completed successfully), avoiding N file reads
+    /// and writes per phase.
+    pub fn run_phase(&self, hooks: &[HookSpec], phase: &HookPhase) -> Report {
         let mut state = HookState::load(&self.state_dir).unwrap_or_default();
+        let mut state_dirty = false;
+        let mut report = Report::default();
 
+        for spec in hooks.iter().filter(|h| &h.phase == phase) {
+            let result = self.run_hook_with_state(spec, &mut state, &mut state_dirty);
+            let status = match &result {
+                HookResult::Ok => StepStatus::Ok,
+                HookResult::Skipped => StepStatus::Skipped,
+                HookResult::Failed(msg) => StepStatus::Failed(msg.clone()),
+            };
+            report.add(&spec.name, status);
+        }
+
+        if state_dirty {
+            let _ = state.save(&self.state_dir);
+        }
+
+        report
+    }
+
+    /// Execute a single hook, using the caller-owned `state` and `dirty` flag
+    /// instead of loading/saving per invocation.
+    fn run_hook_with_state(
+        &self,
+        spec: &HookSpec,
+        state: &mut HookState,
+        state_dirty: &mut bool,
+    ) -> HookResult {
         if spec.phase == HookPhase::Setup && !self.force && state.is_done(&spec.name) {
             return HookResult::Skipped;
         }
@@ -63,7 +95,7 @@ impl HookRunner {
             Ok(status) if status.success() => {
                 if spec.phase == HookPhase::Setup {
                     state.mark_done(&spec.name);
-                    let _ = state.save(&self.state_dir);
+                    *state_dirty = true;
                 }
                 HookResult::Ok
             }
@@ -71,4 +103,18 @@ impl HookRunner {
             Err(e) => HookResult::Failed(e.to_string()),
         }
     }
+
+    /// Run a single hook with its own load/save cycle.
+    ///
+    /// Prefer [`HookRunner::run_phase`] when running multiple hooks to avoid
+    /// repeated state I/O.
+    pub fn run_hook(&self, spec: &HookSpec) -> HookResult {
+        let mut state = HookState::load(&self.state_dir).unwrap_or_default();
+        let mut state_dirty = false;
+        let result = self.run_hook_with_state(spec, &mut state, &mut state_dirty);
+        if state_dirty {
+            let _ = state.save(&self.state_dir);
+        }
+        result
+    }
 }