Fix commit hooks to respect core.hooksPath configuration #2090
Conversation
Fixes gitpython-developers#2083 The run_commit_hook() function was hardcoded to look for hooks in $GIT_DIR/hooks, ignoring the core.hooksPath configuration option that Git has supported since v2.9. Changes: - Add _get_hooks_dir() helper that reads core.hooksPath from config - Handle both absolute and relative paths per git-config documentation - Update run_commit_hook() to use the new helper - Add comprehensive tests for both absolute and relative hooksPath Per git-config documentation: - Absolute paths are used as-is - Relative paths are resolved relative to the working tree root (or git_dir for bare repos) - If not set, defaults to $GIT_DIR/hooks The existing hook_path() function is preserved for backwards compatibility and documented to note it does not respect the config.
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #2083 by adding support for Git's core.hooksPath configuration option to the run_commit_hook() function. Previously, GitPython hardcoded the hooks directory to $GIT_DIR/hooks, ignoring any custom hooks path configured by users.
Key Changes:
- Introduced
_get_hooks_dir()helper function to read and resolvecore.hooksPathconfiguration - Updated
run_commit_hook()to use the new helper for determining the hooks directory - Added comprehensive tests for both absolute and relative
core.hooksPathconfigurations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| git/index/fun.py | Added _get_hooks_dir() helper function that respects core.hooksPath config; updated run_commit_hook() to use new helper; added documentation note to hook_path() function about config limitations |
| test/test_index.py | Added two new test cases covering absolute and relative core.hooksPath configurations with appropriate Windows-specific xfail decorators |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_run_commit_hook_respects_relative_core_hookspath(self, rw_repo): | ||
| """Test that run_commit_hook() handles relative core.hooksPath correctly. | ||
| Per git-config docs, relative paths for core.hooksPath are relative to | ||
| the directory where hooks are run (typically the working tree root). | ||
| """ | ||
| index = rw_repo.index | ||
|
|
||
| # Create a custom hooks directory with a relative path | ||
| custom_hooks_dir = Path(rw_repo.working_tree_dir) / "relative-hooks" | ||
| custom_hooks_dir.mkdir() | ||
|
|
||
| # Create a hook in the custom location | ||
| custom_hook = custom_hooks_dir / "fake-hook" | ||
| custom_hook.write_text(HOOKS_SHEBANG + "echo 'ran from relative hooks path' >output.txt") | ||
| custom_hook.chmod(0o744) | ||
|
|
||
| # Set core.hooksPath to a relative path | ||
| with rw_repo.config_writer() as config: | ||
| config.set_value("core", "hooksPath", "relative-hooks") | ||
|
|
||
| # Run the hook - it should resolve the relative path correctly | ||
| run_commit_hook("fake-hook", index) | ||
|
|
||
| output_file = Path(rw_repo.working_tree_dir) / "output.txt" | ||
| self.assertTrue(output_file.exists(), "Hook should have created output.txt") | ||
| output = output_file.read_text(encoding="utf-8") | ||
| self.assertEqual(output, "ran from relative hooks path\n") |
There was a problem hiding this comment.
The test only covers non-bare repositories (bare=False). Consider adding test coverage for bare repositories with relative core.hooksPath, since the logic in _get_hooks_dir() falls back to git_dir for bare repos when resolving relative paths (line 98 of git/index/fun.py).
git/index/fun.py
Outdated
| - If not set: defaults to $GIT_DIR/hooks | ||
| """ | ||
| # Import here to avoid circular imports | ||
| from git.repo.base import Repo |
There was a problem hiding this comment.
The import statement from git.repo.base import Repo is only used for type hinting in the function signature on line 71. This import is already conditionally imported at the module level within the TYPE_CHECKING block (see lines 48-52), making this local import unnecessary. Consider removing this local import and relying on the string annotation "Repo" which is already being used.
| from git.repo.base import Repo |
| def test_run_commit_hook_respects_core_hookspath(self, rw_repo): | ||
| """Test that run_commit_hook() respects core.hooksPath configuration. | ||
| This addresses issue #2083 where commit hooks were always looked for in | ||
| $GIT_DIR/hooks instead of respecting the core.hooksPath config setting. | ||
| """ | ||
| index = rw_repo.index | ||
|
|
||
| # Create a custom hooks directory outside of .git | ||
| custom_hooks_dir = Path(rw_repo.working_tree_dir) / "custom-hooks" | ||
| custom_hooks_dir.mkdir() | ||
|
|
||
| # Create a hook in the custom location | ||
| custom_hook = custom_hooks_dir / "fake-hook" | ||
| custom_hook.write_text(HOOKS_SHEBANG + "echo 'ran from custom hooks path' >output.txt") | ||
| custom_hook.chmod(0o744) | ||
|
|
||
| # Set core.hooksPath in the repo config | ||
| with rw_repo.config_writer() as config: | ||
| config.set_value("core", "hooksPath", str(custom_hooks_dir)) | ||
|
|
||
| # Run the hook - it should use the custom path | ||
| run_commit_hook("fake-hook", index) | ||
|
|
||
| output_file = Path(rw_repo.working_tree_dir) / "output.txt" | ||
| self.assertTrue(output_file.exists(), "Hook should have created output.txt") | ||
| output = output_file.read_text(encoding="utf-8") | ||
| self.assertEqual(output, "ran from custom hooks path\n") |
There was a problem hiding this comment.
The test only covers non-bare repositories (bare=False). Consider adding test coverage for bare repositories with core.hooksPath set, since the existing test_run_commit_hook test on line 1050 uses a bare repository. This is important because the logic in _get_hooks_dir() falls back to git_dir for bare repos when resolving relative paths (line 98 of git/index/fun.py).
Also clean up whitespace and move type import to TYPE_CHECKING block.
Place hooks inside git_dir to avoid path resolution issues on Windows where absolute paths outside the repo directory fail the relative_to check.
1 participant
Summary
Fixes #2083
The
run_commit_hook()function was hardcoded to look for hooks in$GIT_DIR/hooks, ignoring thecore.hooksPathconfiguration option that Git has supported since v2.9.Changes
_get_hooks_dir()helper function that readscore.hooksPathfrom config$GIT_DIR/hooksrun_commit_hook()to use the new helperhooksPathconfigurationsTesting
test_run_commit_hook_respects_core_hookspath- tests absolute pathtest_run_commit_hook_respects_relative_core_hookspath- tests relative pathBackwards Compatibility
The existing
hook_path()function is preserved for backwards compatibility and has been documented to note that it does not respect the config. Code that directly useshook_path()will behave as before. Only code usingrun_commit_hook()(includingindex.commit()) will benefit from the new behavior.References