There was a problem hiding this comment.
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:
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| 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.
Sorry, something went wrong.
| 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).
Sorry, something went wrong.
| - 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 |
Sorry, something went wrong.
| 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).
Sorry, something went wrong.
Summary
Fixes #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
Testing
Backwards 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 uses hook_path() will behave as before. Only code using run_commit_hook() (including index.commit()) will benefit from the new behavior.
References