Skip to content

Commit 9bc287a

Browse files
codexByron
authored andcommitted
Address review feedback about hook resolution
- Review feedback: resolve core.hooksPath only when configured, avoid an unconditional git rev-parse dependency, and cover relative paths in a non-bare repository. - Read the effective Git configuration directly so unconfigured repositories retain the legacy .git/hooks lookup and configured relative paths resolve from the directory where hooks execute. Exercise both guarantees in the hook tests. - Review feedback: core.hooksPath can point outside index.repo.working_dir, where Path.relative_to() raises on Windows and prevents a valid hook from running. - Build the Bash argument with os.path.relpath so hooks in parent or other absolute locations remain executable. Fall back to the absolute POSIX-form path when Windows cannot form a relative path across drives, and add a focused command-construction regression test.
1 parent 406b98e commit 9bc287a

2 files changed

Lines changed: 46 additions & 13 deletions

File tree

git/index/fun.py

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -66,12 +66,13 @@ def hook_path(name: str, git_dir: PathLike) -> str:
6666

6767
def _commit_hook_path(name: str, index: "IndexFile") -> str:
6868
""":return: path to the named commit hook, respecting Git's core.hooksPath."""
69-
hp = index.repo.git.rev_parse("--git-path", f"hooks/{name}")
70-
if osp.isabs(hp):
71-
return hp
69+
with index.repo.config_reader() as config:
70+
hooks_dir = config.get("core", "hooksPath", fallback="")
7271

73-
base_dir = index.repo.working_dir or index.repo.git_dir
74-
return osp.abspath(osp.join(base_dir, hp))
72+
if not hooks_dir:
73+
return hook_path(name, index.repo.git_dir)
74+
75+
return osp.abspath(osp.join(index.repo.working_dir, osp.expanduser(hooks_dir), name))
7576

7677

7778
def _has_file_extension(path: str) -> str:
@@ -104,8 +105,14 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None:
104105
if sys.platform == "win32" and not _has_file_extension(hp):
105106
# Windows only uses extensions to determine how to open files
106107
# (doesn't understand shebangs). Try using bash to run the hook.
107-
relative_hp = Path(hp).relative_to(index.repo.working_dir).as_posix()
108-
cmd = ["bash.exe", relative_hp]
108+
try:
109+
bash_hp = osp.relpath(hp, index.repo.working_dir)
110+
except ValueError:
111+
# Different drives have no relative path on Windows. Git Bash accepts
112+
# an absolute path in this form, although a relative path is preferable
113+
# because it also works with the Windows Subsystem for Linux wrapper.
114+
bash_hp = hp
115+
cmd = ["bash.exe", Path(bash_hp).as_posix()]
109116

110117
process = safer_popen(
111118
cmd + list(args),

test/test_index.py

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1099,9 +1099,35 @@ class Mocked:
10991099
def test_run_commit_hook(self, rw_repo):
11001100
index = rw_repo.index
11011101
_make_hook(index.repo.git_dir, "fake-hook", "echo 'ran fake hook' >output.txt")
1102-
run_commit_hook("fake-hook", index)
1103-
output = Path(rw_repo.git_dir, "output.txt").read_text(encoding="utf-8")
1104-
self.assertEqual(output, "ran fake hook\n")
1102+
output = Path(rw_repo.git_dir, "output.txt")
1103+
with mock.patch.object(Repo, "config_level", ("repository",)):
1104+
with mock.patch.object(Git, "execute", side_effect=AssertionError("hook lookup must not run git")):
1105+
run_commit_hook("fake-hook", index)
1106+
self.assertEqual(output.read_text(encoding="utf-8"), "ran fake hook\n")
1107+
1108+
output.unlink()
1109+
with index.repo.config_writer() as writer:
1110+
writer.set_value("core", "hooksPath", "")
1111+
run_commit_hook("fake-hook", index)
1112+
1113+
self.assertEqual(output.read_text(encoding="utf-8"), "ran fake hook\n")
1114+
1115+
@with_rw_directory
1116+
def test_run_commit_hook_outside_worktree_on_windows(self, rw_dir):
1117+
root = Path(rw_dir).resolve()
1118+
repo = Repo.init(root / "repo")
1119+
hooks_dir = root / "hooks"
1120+
_make_hook(root, "fake-hook", "exit 0")
1121+
with repo.config_writer() as writer:
1122+
writer.set_value("core", "hooksPath", str(hooks_dir))
1123+
1124+
with mock.patch("git.index.fun.sys.platform", "win32"):
1125+
with mock.patch("git.index.fun.safer_popen") as popen, mock.patch("git.index.fun.handle_process_output"):
1126+
popen.return_value.returncode = 0
1127+
run_commit_hook("fake-hook", repo.index)
1128+
1129+
command = popen.call_args[0][0]
1130+
self.assertEqual(command, ["bash.exe", "../hooks/fake-hook"])
11051131

11061132
@ddt.data((False,), (True,))
11071133
@with_rw_directory
@@ -1170,10 +1196,10 @@ def test_pre_commit_hook_success(self, rw_repo):
11701196
reason="Currently uses the bash.exe of WSL, even with no WSL distro installed",
11711197
raises=HookExecutionError,
11721198
)
1173-
@with_rw_repo("HEAD", bare=True)
1199+
@with_rw_repo("HEAD")
11741200
def test_pre_commit_hook_respects_core_hooks_path(self, rw_repo):
11751201
index = rw_repo.index
1176-
hooks_dir = Path(index.repo.git_dir, "custom-hooks")
1202+
hooks_dir = Path(index.repo.working_dir, "custom-hooks")
11771203
hooks_dir.mkdir()
11781204
hp = hooks_dir / "pre-commit"
11791205
hp.write_text(HOOKS_SHEBANG + "echo 'ran custom hook' >custom-hook-output.txt", encoding="utf-8")
@@ -1183,7 +1209,7 @@ def test_pre_commit_hook_respects_core_hooks_path(self, rw_repo):
11831209
writer.set_value("core", "hooksPath", "custom-hooks")
11841210

11851211
index.commit("This should run the custom hook")
1186-
output = Path(rw_repo.git_dir, "custom-hook-output.txt").read_text(encoding="utf-8")
1212+
output = Path(rw_repo.working_dir, "custom-hook-output.txt").read_text(encoding="utf-8")
11871213
self.assertEqual(output, "ran custom hook\n")
11881214

11891215
@pytest.mark.xfail(

0 commit comments

Comments
 (0)