fix: harden git worktree module (review findings)
- is_git_repo: check out.strip() == "true" (bare repos / .git dirs exit 0 with "false") - _validate_branch: new shared helper calls check-ref-format; used in all three public branch-taking functions to block option-injection (--detach, --all, etc.) - _ensure_worktrees_excluded: prepend newline when existing content lacks trailing newline - WorktreeState: make frozen=True (codebase precedent) - create_worktree invalid-branch ServiceError now includes git stderr - remove_worktree: log.debug for silently-swallowed branch -d / worktree prune failures - worktree_state docstring: document git-failure → clean-state behaviour - Tests: 9 new regression tests; raw subprocess.run commit calls replaced with _commit helper Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
+74
-16
@@ -7,6 +7,7 @@ import pytest
|
||||
|
||||
from hqt.errors import ServiceError
|
||||
from hqt.git.worktree import (
|
||||
WorktreeState,
|
||||
add_worktree_for_branch,
|
||||
create_worktree,
|
||||
is_git_repo,
|
||||
@@ -33,6 +34,13 @@ def _git(repo: Path, *args: str) -> str:
|
||||
return result.stdout.strip()
|
||||
|
||||
|
||||
def _commit(repo: Path, filename: str, message: str) -> None:
|
||||
"""Create a file, stage it, and commit it in repo."""
|
||||
(repo / filename).write_text("content")
|
||||
_git(repo, "add", filename)
|
||||
_git(repo, "commit", "-m", message)
|
||||
|
||||
|
||||
def _make_repo(path: Path) -> Path:
|
||||
"""Init a bare-minimum git repo with one commit; return path."""
|
||||
path.mkdir(parents=True, exist_ok=True)
|
||||
@@ -103,6 +111,36 @@ async def test_is_git_repo_missing_dir(tmp_path):
|
||||
assert await is_git_repo(missing) is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_is_git_repo_bare_repo_returns_false(tmp_path):
|
||||
"""git rev-parse --is-inside-work-tree exits 0 and prints 'false' in bare repos."""
|
||||
bare = tmp_path / "bare.git"
|
||||
bare.mkdir()
|
||||
subprocess.run(
|
||||
["git", "init", "--bare", str(bare)], check=True, capture_output=True
|
||||
)
|
||||
assert await is_git_repo(bare) is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_is_git_repo_dot_git_dir_returns_false(tmp_path):
|
||||
"""git rev-parse --is-inside-work-tree exits 0 and prints 'false' inside .git."""
|
||||
repo = _make_repo(tmp_path / "repo")
|
||||
dot_git = repo / ".git"
|
||||
assert await is_git_repo(dot_git) is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# WorktreeState frozen dataclass
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_worktree_state_is_frozen():
|
||||
state = WorktreeState(exists=True, dirty=False, unique_commits=0)
|
||||
with pytest.raises(Exception):
|
||||
state.exists = False # type: ignore[misc]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# create_worktree
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -158,6 +196,23 @@ async def test_create_worktree_invalid_branch_name_raises(tmp_path):
|
||||
await create_worktree(repo, "foo..bar")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_worktree_exclude_no_trailing_newline(tmp_path):
|
||||
"""Exclude file lacking trailing newline must not corrupt the last pattern."""
|
||||
repo = _make_repo(tmp_path / "repo")
|
||||
exclude = repo / ".git" / "info" / "exclude"
|
||||
exclude.parent.mkdir(parents=True, exist_ok=True)
|
||||
# Write a pattern WITHOUT a trailing newline.
|
||||
exclude.write_text("*.log")
|
||||
|
||||
await create_worktree(repo, "feature-x")
|
||||
|
||||
content = exclude.read_text()
|
||||
lines = [ln for ln in content.splitlines() if ln.strip()]
|
||||
assert "*.log" in lines, f"*.log missing from: {content!r}"
|
||||
assert ".worktrees/" in lines, f".worktrees/ missing from: {content!r}"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# add_worktree_for_branch
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -177,6 +232,14 @@ async def test_add_worktree_for_branch_creates_directory(tmp_path):
|
||||
assert branch == "feature-z"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_add_worktree_for_branch_option_like_name_raises(tmp_path):
|
||||
"""Option-looking branch names (e.g. --detach) must be rejected before git sees them."""
|
||||
repo = _make_repo(tmp_path / "repo")
|
||||
with pytest.raises(ServiceError, match="Invalid branch name"):
|
||||
await add_worktree_for_branch(repo, "--detach")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# worktree_state
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -205,14 +268,7 @@ async def test_worktree_state_dirty(tmp_path):
|
||||
async def test_worktree_state_unique_commits(tmp_path):
|
||||
repo = _make_repo(tmp_path / "repo")
|
||||
wt = await create_worktree(repo, "feature-c")
|
||||
(wt / "new.txt").write_text("content")
|
||||
_git(wt, "add", "new.txt")
|
||||
subprocess.run(
|
||||
["git", "commit", "-m", "branch commit"],
|
||||
cwd=wt,
|
||||
capture_output=True,
|
||||
check=True,
|
||||
)
|
||||
_commit(wt, "new.txt", "branch commit")
|
||||
state = await worktree_state(repo, wt, "feature-c")
|
||||
assert state.unique_commits == 1
|
||||
|
||||
@@ -227,6 +283,15 @@ async def test_worktree_state_missing_dir(tmp_path):
|
||||
assert state.unique_commits == 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_worktree_state_option_like_branch_raises(tmp_path):
|
||||
"""Option-looking branch names (e.g. --all) must be rejected."""
|
||||
repo = _make_repo(tmp_path / "repo")
|
||||
wt = repo / ".worktrees" / "whatever"
|
||||
with pytest.raises(ServiceError, match="Invalid branch name"):
|
||||
await worktree_state(repo, wt, "--all")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# remove_worktree
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -268,14 +333,7 @@ async def test_remove_worktree_dirty_with_force_removes(tmp_path):
|
||||
async def test_remove_worktree_unique_commits_branch_survives(tmp_path):
|
||||
repo = _make_repo(tmp_path / "repo")
|
||||
wt = await create_worktree(repo, "feature-g")
|
||||
(wt / "new.txt").write_text("content")
|
||||
_git(wt, "add", "new.txt")
|
||||
subprocess.run(
|
||||
["git", "commit", "-m", "unique"],
|
||||
cwd=wt,
|
||||
capture_output=True,
|
||||
check=True,
|
||||
)
|
||||
_commit(wt, "new.txt", "unique")
|
||||
# remove_worktree must NOT raise even though branch -d will fail.
|
||||
await remove_worktree(repo, wt, "feature-g", force=True)
|
||||
assert not wt.exists()
|
||||
|
||||
Reference in New Issue
Block a user