diff --git a/src/hook/install.rs b/src/hook/install.rs index 5a571ec..2881dc3 100644 --- a/src/hook/install.rs +++ b/src/hook/install.rs @@ -18,8 +18,9 @@ pub fn install_hook() -> Result { // Already a cora-managed hook — just overwrite debug!("existing hook is cora-managed, overwriting"); } else { - // Non-cora hook — back it up and compose a wrapper - let backup = hooks_dir.join("pre-commit.bak"); + // Non-cora hook — back it up and compose a wrapper. + // Use `pre-commit.pre-cora.bak` to match the name uninstall_hook looks for. + let backup = hooks_dir.join("pre-commit.pre-cora.bak"); std::fs::copy(&hook_path, &backup)?; debug!(path = %backup.display(), "backed up existing non-cora hook"); @@ -65,7 +66,8 @@ pub fn install_hook() -> Result { pub fn uninstall_hook() -> Result<()> { let hooks_dir = find_git_hooks_dir()?; let hook_path = hooks_dir.join("pre-commit"); - let backup_path = hooks_dir.join("pre-commit.cora.bak"); + // Restore from backup if one exists, otherwise remove the hook. + // `pre-commit.pre-cora.bak` is the only backup name install_hook writes. let pre_backup = hooks_dir.join("pre-commit.pre-cora.bak"); if !hook_path.is_file() { @@ -79,11 +81,9 @@ pub fn uninstall_hook() -> Result<()> { return Ok(()); } - if backup_path.is_file() { - std::fs::rename(&backup_path, &hook_path).context("failed to restore backup hook")?; - debug!("restored hook from backup"); - } else if pre_backup.is_file() { - std::fs::rename(&pre_backup, &hook_path).context("failed to restore pre-cora backup")?; + if pre_backup.is_file() { + std::fs::rename(&pre_backup, &hook_path) + .context("failed to restore pre-cora backup hook")?; debug!("restored pre-cora hook from backup"); } else { std::fs::remove_file(&hook_path).context("failed to remove hook")?; @@ -136,3 +136,55 @@ pub fn is_hook_installed() -> Result { let content = std::fs::read_to_string(&hook_path).unwrap_or_default(); Ok(content.contains("cora")) } + +#[cfg(test)] +mod tests { + use std::fs; + use std::process::Command; + + /// Create a temp git repo — used to verify hook file operations. + fn temp_git_repo() -> tempfile::TempDir { + let tmp = tempfile::tempdir().unwrap(); + Command::new("git") + .args(["init"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + tmp + } + + #[test] + fn backup_filename_is_pre_cora_bak() { + let tmp = temp_git_repo(); + let hooks_dir = tmp.path().join(".git/hooks"); + fs::create_dir_all(&hooks_dir).unwrap(); + + // Simulate an existing non-cora hook + let hook_path = hooks_dir.join("pre-commit"); + fs::write(&hook_path, "#!/bin/sh\necho my-hook\n").unwrap(); + + // We can't call install_hook() directly because it uses `git rev-parse` + // from CWD, not from a configurable path. Instead, verify the backup + // filename constant is consistent between install and uninstall logic. + // + // The install path writes to: pre-commit.pre-cora.bak + // The uninstall path reads from: pre-commit.pre-cora.bak + // This test documents the expected filename. + let expected_backup = "pre-commit.pre-cora.bak"; + + // Verify no other backup names are referenced in the source + let source = include_str!("install.rs"); + assert!( + !source.contains("\"pre-commit.bak\""), + "install.rs should not use generic 'pre-commit.bak' — it was the root cause of data loss" + ); + assert!( + !source.contains("\"pre-commit.cora.bak\""), + "install.rs should not use 'pre-commit.cora.bak' — uninstall never wrote this name" + ); + assert!( + source.contains(&format!("\"{expected_backup}\"")), + "install.rs should consistently use '{expected_backup}' for backup" + ); + } +}