diff --git a/src/context_engine/cli.py b/src/context_engine/cli.py index d657bdb..970cd0b 100644 --- a/src/context_engine/cli.py +++ b/src/context_engine/cli.py @@ -1809,17 +1809,9 @@ def uninstall(yes: bool) -> None: lines.append(section(f"Uninstall · {project_name}")) lines.append("") - # Remove git hooks - hooks_dir = project_dir / ".git" / "hooks" - removed_hooks = 0 - if hooks_dir.exists(): - for hook_name in ["post-commit", "post-checkout", "post-merge"]: - hook_file = hooks_dir / hook_name - if hook_file.exists(): - content = hook_file.read_text() - if "cce" in content.lower() or "context-engine" in content.lower(): - hook_file.unlink() - removed_hooks += 1 + # Remove git hooks (worktree-aware via git rev-parse --git-path). + from context_engine.indexer.git_hooks import uninstall_hooks + removed_hooks = uninstall_hooks(str(project_dir)) if removed_hooks: lines.append(f" {CROSS} {warn('Removed')} {removed_hooks} git hooks") else: diff --git a/src/context_engine/indexer/git_hooks.py b/src/context_engine/indexer/git_hooks.py index 1056668..9f6956e 100644 --- a/src/context_engine/indexer/git_hooks.py +++ b/src/context_engine/indexer/git_hooks.py @@ -1,6 +1,7 @@ """Git hook installer and handler for triggering re-indexing.""" import shutil import stat +import subprocess import sys from pathlib import Path @@ -8,6 +9,38 @@ HOOK_NAMES = ["post-commit", "post-checkout", "post-merge"] +def _resolve_hooks_dir(project_dir: str) -> Path | None: + """Return the directory git uses for hooks for `project_dir`, or None. + + Why this is non-trivial: in a regular checkout, hooks live at + `/.git/hooks/`. In a git worktree, `/.git` is a *file* + pointing at `
/.git/worktrees//`, and hooks are shared with the + main repo at `
/.git/hooks/`. Hardcoding the regular-checkout layout + makes the installer silently no-op inside worktrees. + + `git rev-parse --git-path hooks` resolves the right directory in both + cases (relative `.git/hooks` for regular checkouts, an absolute path to + the shared hooks dir for worktrees), and also respects `core.hooksPath` + if a project has overridden it. + """ + try: + result = subprocess.run( + ["git", "rev-parse", "--git-path", "hooks"], + cwd=project_dir, capture_output=True, text=True, timeout=5, + ) + except (FileNotFoundError, subprocess.TimeoutExpired): + return None + if result.returncode != 0: + return None + raw = result.stdout.strip() + if not raw: + return None + hooks_dir = Path(raw) + if not hooks_dir.is_absolute(): + hooks_dir = Path(project_dir) / hooks_dir + return hooks_dir + + def _resolve_cce_binary() -> str: """Find an absolute path to the `cce` launcher. @@ -38,9 +71,11 @@ def _hook_script() -> str: def install_hooks(project_dir: str) -> list[str]: - """Install CCE git hooks. Returns [] gracefully if not a git repo.""" - hooks_dir = Path(project_dir) / ".git" / "hooks" - if not hooks_dir.exists(): + """Install CCE git hooks. Returns [] gracefully if not a git repo or if + git is unavailable. Works correctly inside git worktrees, where hooks + live in the shared main-repo `.git/hooks` directory.""" + hooks_dir = _resolve_hooks_dir(project_dir) + if hooks_dir is None or not hooks_dir.exists(): return [] installed = [] for hook_name in HOOK_NAMES: @@ -63,6 +98,37 @@ def _install_single_hook(hook_path: Path) -> None: hook_path.chmod(hook_path.stat().st_mode | stat.S_IEXEC) +def uninstall_hooks(project_dir: str) -> int: + """Remove CCE-installed hook scripts from this project. Returns the number + of hook files actually removed. No-op outside a git repo or if git is + unavailable. + + Detects \"CCE-installed\" by file content (presence of \"cce\" or + \"context-engine\") rather than the marker alone, so legacy installations + that pre-date HOOK_MARKER are still cleaned up. Worktree-aware via the + same `_resolve_hooks_dir` used by install. + """ + hooks_dir = _resolve_hooks_dir(project_dir) + if hooks_dir is None or not hooks_dir.exists(): + return 0 + removed = 0 + for hook_name in HOOK_NAMES: + hook_path = hooks_dir / hook_name + if not hook_path.exists(): + continue + try: + content = hook_path.read_text() + except OSError: + continue + if "cce" in content.lower() or "context-engine" in content.lower(): + try: + hook_path.unlink() + removed += 1 + except OSError: + pass + return removed + + def get_changed_files_from_hook() -> list[str]: import subprocess try: diff --git a/tests/indexer/test_git_hooks.py b/tests/indexer/test_git_hooks.py index a1e2e87..2aaaaac 100644 --- a/tests/indexer/test_git_hooks.py +++ b/tests/indexer/test_git_hooks.py @@ -1,14 +1,64 @@ import os +import shutil import stat +import subprocess + import pytest -from context_engine.indexer.git_hooks import install_hooks + +from context_engine.indexer.git_hooks import install_hooks, uninstall_hooks + + +def _git(*args: str, cwd: str) -> str: + """Run git with config that doesn't require a real user/email — keeps the + tests usable on a fresh CI box where the user hasn't set git config.""" + env = { + **os.environ, + "GIT_AUTHOR_NAME": "test", "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "test", "GIT_COMMITTER_EMAIL": "t@t", + } + return subprocess.run( + ["git", *args], cwd=cwd, env=env, check=True, + capture_output=True, text=True, + ).stdout + + +def _git_available() -> bool: + return shutil.which("git") is not None + + +pytestmark = pytest.mark.skipif( + not _git_available(), + reason="git binary not available — hook resolution requires real git", +) @pytest.fixture def git_repo(tmp_path): - git_dir = tmp_path / ".git" / "hooks" - git_dir.mkdir(parents=True) - return tmp_path + """A real git repo at tmp_path. Avoids the previous fake-`.git/hooks/` dir + pattern, which silently masked the install logic — without `git init`, + `git rev-parse --git-path hooks` correctly refuses to claim the directory + is a repo and tests would not exercise the production path.""" + proj = tmp_path / "proj" + proj.mkdir() + _git("init", "-b", "main", cwd=str(proj)) + return proj + + +@pytest.fixture +def worktree(git_repo, tmp_path): + """A worktree of `git_repo` at tmp_path/wt-feat. Returns the worktree dir. + + Worktrees share their hooks directory with the main repo's `.git/hooks/` + via the gitfile pointer in the worktree's `.git` (which is a *file*, not + a directory). Pinning install-from-worktree behavior here matches the + real-world setup that surfaced issue #48. + """ + (git_repo / "file.txt").write_text("hi\n") + _git("add", "file.txt", cwd=str(git_repo)) + _git("commit", "-m", "init", cwd=str(git_repo)) + wt = tmp_path / "wt-feat" + _git("worktree", "add", "-b", "feat", str(wt), cwd=str(git_repo)) + return wt def test_install_hooks_creates_post_commit(git_repo): @@ -34,6 +84,7 @@ def test_install_hooks_creates_post_merge(git_repo): def test_install_hooks_preserves_existing(git_repo): existing_hook = git_repo / ".git" / "hooks" / "post-commit" + existing_hook.parent.mkdir(parents=True, exist_ok=True) existing_hook.write_text("#!/bin/sh\necho 'existing'\n") existing_hook.chmod(existing_hook.stat().st_mode | stat.S_IEXEC) install_hooks(project_dir=str(git_repo)) @@ -46,3 +97,66 @@ def test_install_hooks_returns_empty_for_non_git(tmp_path): """Non-git directory should return empty list, not raise.""" result = install_hooks(project_dir=str(tmp_path)) assert result == [] + + +def test_install_hooks_inside_worktree_writes_to_shared_hooks_dir(worktree, git_repo): + """Regression for issue #48: installing from inside a worktree previously + silently no-op'd because `/.git` is a file, not a directory. + Hooks must land in the shared main-repo hooks dir, where they fire for + every worktree.""" + installed = install_hooks(project_dir=str(worktree)) + assert installed, "install returned [] — worktree path was not resolved" + shared_hook = git_repo / ".git" / "hooks" / "post-commit" + assert shared_hook.exists(), ( + f"expected shared hook at {shared_hook}, got installed={installed}" + ) + assert "cce hook" in shared_hook.read_text() + # The per-worktree gitdir does not get its own hooks/ — that's by design, + # git's behavior puts shared hooks in the main repo. + per_worktree = git_repo / ".git" / "worktrees" / "wt-feat" / "hooks" + if per_worktree.exists(): + assert not (per_worktree / "post-commit").exists() + + +def test_uninstall_hooks_removes_only_cce_hooks(git_repo): + """Uninstall scrubs CCE-installed hooks but keeps unrelated ones.""" + install_hooks(project_dir=str(git_repo)) + foreign = git_repo / ".git" / "hooks" / "pre-commit" # not in HOOK_NAMES + foreign.write_text("#!/bin/sh\necho lint\n") + foreign.chmod(foreign.stat().st_mode | stat.S_IEXEC) + + removed = uninstall_hooks(project_dir=str(git_repo)) + assert removed == 3 + assert not (git_repo / ".git" / "hooks" / "post-commit").exists() + # Foreign hook untouched. + assert foreign.exists() + + +def test_uninstall_hooks_from_worktree_removes_shared_hooks(worktree, git_repo): + """Symmetry with install: uninstall run from a worktree must remove the + hooks the install path put in the shared dir, otherwise the user is left + with no way to clean up without cd'ing into the main checkout.""" + install_hooks(project_dir=str(worktree)) + shared_hook = git_repo / ".git" / "hooks" / "post-commit" + assert shared_hook.exists() + + removed = uninstall_hooks(project_dir=str(worktree)) + assert removed == 3 + assert not shared_hook.exists() + + +def test_uninstall_hooks_skips_non_cce_content(git_repo): + """A hand-rolled `post-commit` that has no CCE markers must be left alone. + Detection is content-based (presence of \"cce\" or \"context-engine\"), so + a user's pre-existing hook with neither token is preserved.""" + other = git_repo / ".git" / "hooks" / "post-commit" + other.parent.mkdir(parents=True, exist_ok=True) + other.write_text("#!/bin/sh\necho 'unrelated'\n") + other.chmod(other.stat().st_mode | stat.S_IEXEC) + removed = uninstall_hooks(project_dir=str(git_repo)) + assert removed == 0 + assert other.exists() + + +def test_uninstall_hooks_returns_zero_for_non_git(tmp_path): + assert uninstall_hooks(project_dir=str(tmp_path)) == 0