Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork966
Fix commit hooks to respect core.hooksPath configuration#2090
Conversation
Fixesgitpython-developers#2083The run_commit_hook() function was hardcoded to look for hooks in$GIT_DIR/hooks, ignoring the core.hooksPath configuration optionthat 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 hooksPathPer 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/hooksThe existing hook_path() function is preserved for backwardscompatibility and documented to note it does not respect the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
This PR fixes issue#2083 by adding support for Git'score.hooksPath configuration option to therun_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 respectscore.hooksPath config; updatedrun_commit_hook() to use new helper; added documentation note tohook_path() function about config limitations |
| test/test_index.py | Added two new test cases covering absolute and relativecore.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") |
CopilotAIDec 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The test only covers non-bare repositories (bare=False). Consider adding test coverage for bare repositories with relativecore.hooksPath, since the logic in_get_hooks_dir() falls back togit_dir for bare repos when resolving relative paths (line 98 ofgit/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 |
CopilotAIDec 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The import statementfrom 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 theTYPE_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") |
CopilotAIDec 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The test only covers non-bare repositories (bare=False). Consider adding test coverage for bare repositories withcore.hooksPath set, since the existingtest_run_commit_hook test on line 1050 uses a bare repository. This is important because the logic in_get_hooks_dir() falls back togit_dir for bare repos when resolving relative paths (line 98 ofgit/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 Windowswhere absolute paths outside the repo directory fail the relative_to check.
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