Support the Git "core.hooksPath" option when dealing with hook scripts

Previously, git-review would assume that the Git repository's hook
directory is .git/hooks, relative to the root of the checkout. This
assumption breaks if the user has set the core.hooksPath option on the
repository (or, for that matter, in ~/.gitconfig or /etc/gitconfig).

core.hooksPath can either be set to an absolute path, in which case it
is to be interpreted as-is, or to a relative path, in which case it
should be interpreted as relative to the root of the checkout.

Introduce a new convenience function to suss out the correct path, and
use it in places where the reference to .git/hooks was previously
hard-coded.

Reference:
https: //git-scm.com/docs/git-config#Documentation/git-config.txt-corehooksPath

Depends-on: I0f0f44e57a100420d8e6d2eaec7dbb5d77b654af
Change-Id: Id8a3ac464ff75e6d8207f198089f018cc790eca5

3 files changed, 81 insertions, 2 deletions

diff --git a/git_review/cmd.py b/git_review/cmd.py
index a97a20c..ccf4c09 100644
--- a/git_review/cmd.py
+++ b/git_review/cmd.py
@@ -247,7 +247,7 @@ def run_custom_script(action):
     script_file = "%s-review" % (action)
     (top_dir, git_dir) = git_directories()
     paths = [os.path.join(CONFIGDIR, "hooks", script_file),
-             os.path.join(git_dir, "hooks", script_file)]
+             os.path.join(git_get_hooks_path(top_dir, git_dir), script_file)]
     for fpath in paths:
         if os.path.isfile(fpath) and os.access(fpath, os.X_OK):
             status, output = run_command_status(fpath)
@@ -285,7 +285,26 @@ def git_config_get_value(section, option, default=None, as_bool=False):
         raise
 
 
+def git_get_hooks_path(top_dir, git_dir):
+    """Get the path where we need to store and retrieve Git hooks.
+
+    Normally hooks go into .git/hooks, but users can override with the
+    core.hooksPath option. This can either be an absolute path, in
+    which case we use it as-is, or a relative path, in which case we
+    must interpret it as relative to top_dir.
+    """
+    hook_dir = os.path.join(git_dir, "hooks")
+    hooks_path_option = git_config_get_value('core', 'hooksPath')
+    if hooks_path_option:
+        if os.path.isabs(hooks_path_option):
+            hook_dir = hooks_path_option
+        else:
+            hook_dir = os.path.join(top_dir, hooks_path_option)
+    return hook_dir
+
+
 class Config(object):
+
     """Expose as dictionary configuration options."""
 
     def __init__(self, config_file=None):
@@ -365,6 +384,9 @@ def set_hooks_commit_msg(remote, target_file):
             run_command_exc(CannotInstallHook, *cmd)
         # If there are submodules, the hook needs to be installed into
         # each of them.
+        # Here, we don't check for any nonstandard hooks path, because
+        # it should be safe to assume that very few users are inclined
+        # to set the core.hooksPath option in a submodule checkout.
         run_command_exc(
             CannotInstallHook,
             "git", "submodule", "foreach",
@@ -1686,7 +1708,8 @@ additional information:
     if options.custom_script:
         run_custom_script("pre")
 
-    hook_file = os.path.join(git_dir, "hooks", "commit-msg")
+    hook_dir = git_get_hooks_path(top_dir, git_dir)
+    hook_file = os.path.join(hook_dir, "commit-msg")
     have_hook = os.path.exists(hook_file) and os.access(hook_file, os.X_OK)
 
     if not have_hook:
diff --git a/git_review/tests/test_git_review.py b/git_review/tests/test_git_review.py
index d988e1d..ac02bb4 100644
--- a/git_review/tests/test_git_review.py
+++ b/git_review/tests/test_git_review.py
@@ -18,6 +18,7 @@
 import json
 import os
 import shutil
+import tempfile
 import testtools
 
 from git_review import tests
@@ -102,6 +103,53 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
         self._run_git_review(
             '-s', chdir=os.path.join(self.test_dir, 'subdirectory'))
 
+    def test_git_review_s_without_core_hooks_path_option(self):
+        """Test whether git-review -s correctly creates the commit-msg hook,
+        with the Git core.hooksPath option unset.
+        """
+        hooks_subdir = ".git/hooks"
+        self.reset_remote()
+
+        # There really isn't a good way to ensure that
+        # "core.hooksPath" option is presently unset. "git config
+        # --unset" is not idempotent; if you try to unset a config
+        # option that isn't defined it fails with an exit code of
+        # 5. Running "git config core.hooksPath" to retrieve the value
+        # returns 1 if unset, but we can't use self.assertRaises here
+        # either because run_cmd raises a generic Exception and that's
+        # too broad. So instead, we don't check core.hooksPath at all
+        # here, and instead rely on the next two tests to unset the
+        # option after they've set it.
+        self._run_git_review('-s')
+        self.assertTrue(os.path.exists(os.path.join(self.test_dir,
+                                                    hooks_subdir,
+                                                    'commit-msg')))
+
+    def test_git_review_s_with_core_hooks_path_option_relative(self):
+        """Test whether git-review -s correctly creates the commit-msg hook,
+        with the Git core.hooksPath option set to a relative path.
+        """
+        hooks_subdir = "foo"
+        self.reset_remote()
+        self._run_git("config", "core.hooksPath", hooks_subdir)
+        self._run_git_review('-s')
+        self.assertTrue(os.path.exists(os.path.join(self.test_dir,
+                                                    hooks_subdir,
+                                                    'commit-msg')))
+        self._run_git("config", "--unset", "core.hooksPath")
+
+    def test_git_review_s_with_core_hooks_path_option_absolute(self):
+        """Test whether git-review -s correctly creates the commit-msg hook,
+        with the Git core.hooksPath option set to an absolute path.
+        """
+        self.reset_remote()
+        with tempfile.TemporaryDirectory() as hooks_dir:
+            self._run_git("config", "core.hooksPath", hooks_dir)
+            self._run_git_review('-s')
+            self.assertTrue(os.path.exists(os.path.join(hooks_dir,
+                                                        'commit-msg')))
+        self._run_git("config", "--unset", "core.hooksPath")
+
     def test_git_review_d(self):
         """Test git-review -d."""
         self._run_git_review('-s')
diff --git a/releasenotes/notes/support-core-hooks-path-ea5402d5d6204f70.yaml b/releasenotes/notes/support-core-hooks-path-ea5402d5d6204f70.yaml
new file mode 100644
index 0000000..0fa36a3
--- /dev/null
+++ b/releasenotes/notes/support-core-hooks-path-ea5402d5d6204f70.yaml
@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    git-review now handles the Git "core.hooksPath" configuration
+    option correctly. Thus, it installs the commit-msg hook into the
+    core.hooksPath directory, if that option is set. Otherwise, it
+    continues to install the hook into .git/hooks, relative to the
+    root of the checkout.