fix: unbreak installing of global git hooks

Problem: the fix from 1.32.1 to ignore the user git configuration when
running git commands broke `ggshield install -m global` because this
command modifies the global git config so it must not ignore the user git
configuration.

Solution:
- Add a `ignore_git_config` optional argument to `git_shell.git()`, which
  defaults to True. Set it to False in calls from cmd.install.
- Make `install_global()` install the git hooks in a sub-directory of the
  dir returned by `get_data_dir()`, this is required to be able to have
  tests which fail in the situation of 1.32.1.
- Rework test_install to be able to reproduce the bug. Add more tests.

chore: add changelog entry

Author: agateau-gg

changelog.d/20241010_171735_aurelien.gateau_fix_install_global.md

@@ -0,0 +1,3 @@
+### Fixed
+
+- Fixed a regression introduced in ggshield 1.32.1, which made `ggshield install -m global` crash (#972).

ggshield/cmd/install.py

@@ -7,6 +7,7 @@
 from click import UsageError
 
 from ggshield.cmd.utils.common_options import add_common_options
+from ggshield.core.dirs import get_data_dir
 from ggshield.core.errors import UnexpectedError
 from ggshield.utils.git_shell import check_git_dir, git
 
@@ -66,8 +67,11 @@ def install_global(hook_type: str, force: bool, append: bool) -> int:
     hook_dir_path = get_global_hook_dir_path()
 
     if not hook_dir_path:
-        hook_dir_path = Path("~/.git/hooks").expanduser()
-        git(["config", "--global", "core.hooksPath", str(hook_dir_path)])
+        hook_dir_path = get_default_global_hook_dir_path()
+        git(
+            ["config", "--global", "core.hooksPath", str(hook_dir_path)],
+            ignore_git_config=False,
+        )
 
     return create_hook(
         hook_dir_path=hook_dir_path,
@@ -78,10 +82,19 @@ def install_global(hook_type: str, force: bool, append: bool) -> int:
     )
 
 
+def get_default_global_hook_dir_path() -> Path:
+    """
+    Returns the directory in which ggshield creates its global hooks
+    """
+    return get_data_dir() / "git-hooks"
+
+
 def get_global_hook_dir_path() -> Optional[Path]:
-    """Return the default hooks path (if it exists)."""
+    """Return the default hooks path defined in git global config (if it exists)."""
     try:
-        out = git(["config", "--global", "--get", "core.hooksPath"])
+        out = git(
+            ["config", "--global", "--get", "core.hooksPath"], ignore_git_config=False
+        )
     except subprocess.CalledProcessError:
         return None
     return Path(click.format_filename(out)).expanduser()

ggshield/utils/git_shell.py

@@ -186,15 +186,17 @@ def git(
     check: bool = True,
     cwd: Optional[Union[str, Path]] = None,
     log_stderr: bool = True,
+    ignore_git_config: bool = True,
 ) -> str:
     """Calls git with the given arguments, returns stdout as a string"""
     env = os.environ.copy()
     # Ensure git messages are in English
     env["LANG"] = "C"
     # Ensure git behavior is not affected by the user git configuration, but give us a
     # way to set some configuration (useful for safe.directory)
-    env["GIT_CONFIG_GLOBAL"] = os.getenv("GG_GIT_CONFIG", "")
-    env["GIT_CONFIG_SYSTEM"] = ""
+    if ignore_git_config:
+        env["GIT_CONFIG_GLOBAL"] = os.getenv("GG_GIT_CONFIG", "")
+        env["GIT_CONFIG_SYSTEM"] = ""
 
     if cwd is None:
         cwd = Path.cwd()

tests/unit/cmd/test_install.py

@@ -1,12 +1,12 @@
 import os
 from pathlib import Path
-from unittest import mock
 from unittest.mock import Mock, patch
 
 import pytest
 from click.testing import CliRunner
 
 from ggshield.__main__ import cli
+from ggshield.cmd.install import get_default_global_hook_dir_path
 from ggshield.core.errors import ExitCode
 from tests.unit.conftest import assert_invoke_exited_with, assert_invoke_ok
 
@@ -20,15 +20,6 @@
 """
 
 
-@pytest.fixture(scope="class")
-def mockHookDirPath():
-    with mock.patch(
-        "ggshield.cmd.install.get_global_hook_dir_path",
-        return_value=Path("global/hooks"),
-    ):
-        yield
-
-
 class TestInstallLocal:
     def test_local_exist_is_dir(self, cli_fs_runner):
         os.system("git init")
@@ -187,69 +178,122 @@ def test_prepush_install(
         assert_invoke_ok(result)
 
 
+@pytest.fixture()
+def custom_global_git_config_path(tmp_path, monkeypatch):
+    config_path = tmp_path / "global-git-config"
+    monkeypatch.setenv("GIT_CONFIG_GLOBAL", str(config_path))
+    yield config_path
+
+
 class TestInstallGlobal:
-    def test_global_exist_is_dir(self, cli_fs_runner, mockHookDirPath):
-        global_hook_path = Path("global/hooks/pre-commit")
-        global_hook_path.mkdir(parents=True)
+    """
+    These tests use the cli_runner fixture and not the cli_fs_runner one. The reason for
+    this is they execute git commands and git commands are not run in the fake
+    filesystem created by cli_fs_runner so the fake filesystem is useless here.
+    """
+
+    def test_global_exist_is_dir(
+        self, cli_runner: CliRunner, custom_global_git_config_path: Path
+    ):
+        global_pre_commit_hook_path = get_default_global_hook_dir_path() / "pre-commit"
+        global_pre_commit_hook_path.mkdir(parents=True)
 
-        result = cli_fs_runner.invoke(cli, ["install", "-m", "global"])
+        result = cli_runner.invoke(cli, ["install", "-m", "global"])
         assert_invoke_exited_with(result, ExitCode.USAGE_ERROR)
         assert result.exception
 
-    def test_global_not_exist(self, cli_fs_runner, mockHookDirPath):
-        global_hook_path = Path("global/hooks/pre-commit")
-        assert not global_hook_path.exists()
+    def test_global_not_exist(self, cli_runner, custom_global_git_config_path: Path):
+        global_pre_commit_hook_path = get_default_global_hook_dir_path() / "pre-commit"
+        assert not global_pre_commit_hook_path.exists()
+
+        result = cli_runner.invoke(cli, ["install", "-m", "global"])
+        assert global_pre_commit_hook_path.is_file()
+        assert_invoke_ok(result)
+        assert (
+            f"pre-commit successfully added in {global_pre_commit_hook_path}"
+            in result.output
+        )
+
+    def test_install_custom_global_hook_dir(
+        self, cli_runner: CliRunner, tmp_path: Path, custom_global_git_config_path: Path
+    ):
+        """
+        GIVEN an existing global git config
+        AND a custom value for core.hooksPath in the global git config
+        WHEN `install -m global` is called
+        THEN it installs the hook in the custom core.hooksPath dir
+        """
+        custom_hooks_dir = tmp_path / "custom-hooks-dir"
+        custom_pre_commit_path = custom_hooks_dir / "pre-commit"
+        custom_global_git_config_path.write_text(
+            f"[core]\nhooksPath = {custom_hooks_dir.as_posix()}\n", encoding="utf-8"
+        )
 
-        result = cli_fs_runner.invoke(cli, ["install", "-m", "global"])
-        assert global_hook_path.is_file()
+        result = cli_runner.invoke(cli, ["install", "-m", "global"])
         assert_invoke_ok(result)
-        assert f"pre-commit successfully added in {global_hook_path}" in result.output
+        assert custom_pre_commit_path.is_file()
+        assert (
+            f"pre-commit successfully added in {custom_pre_commit_path}"
+            in result.output
+        )
 
     @pytest.mark.parametrize("hook_type", ["pre-push", "pre-commit"])
-    @patch("ggshield.cmd.install.get_global_hook_dir_path")
     def test_install_global(
         self,
-        get_global_hook_dir_path_mock: Mock,
         hook_type: str,
-        cli_fs_runner: CliRunner,
+        cli_runner: CliRunner,
+        custom_global_git_config_path: Path,
     ):
         """
         GIVEN None
         WHEN the command is run
         THEN it should create a pre-push git hook script in the global path
         """
-        global_hooks_dir = Path("global_hooks")
-        get_global_hook_dir_path_mock.return_value = global_hooks_dir
-        result = cli_fs_runner.invoke(
+
+        result = cli_runner.invoke(
             cli,
             ["install", "-m", "global", "-t", hook_type],
         )
 
-        hook_path = global_hooks_dir / hook_type
+        hook_path = get_default_global_hook_dir_path() / hook_type
         hook_str = hook_path.read_text()
         assert f"if [ -f .git/hooks/{hook_type} ]; then" in hook_str
         assert f"ggshield secret scan {hook_type}" in hook_str
 
         assert f"{hook_type} successfully added in {hook_path}\n" in result.output
         assert_invoke_ok(result)
 
-    def test_global_exist_not_force(self, cli_fs_runner, mockHookDirPath):
-        hook_path = Path("global/hooks/pre-commit")
+    def test_global_exist_not_force(
+        self, cli_runner: CliRunner, custom_global_git_config_path: Path
+    ):
+        """
+        GIVEN a global hook dir with an exising pre-commit script
+        WHEN install is called
+        THEN it fails
+        """
+        hook_path = get_default_global_hook_dir_path() / "pre-commit"
         hook_path.parent.mkdir(parents=True, exist_ok=True)
         hook_path.write_text("pre-commit file")
         assert hook_path.is_file()
 
-        result = cli_fs_runner.invoke(cli, ["install", "-m", "global"])
+        result = cli_runner.invoke(cli, ["install", "-m", "global"])
         assert_invoke_exited_with(result, ExitCode.UNEXPECTED_ERROR)
         assert result.exception
         assert f"Error: {hook_path} already exists." in result.output
 
-    def test_global_exist_force(self, cli_fs_runner, mockHookDirPath):
-        hook_path = Path("global/hooks/pre-commit")
+    def test_global_exist_force(
+        self, cli_runner: CliRunner, custom_global_git_config_path: Path
+    ):
+        """
+        GIVEN a global hook dir with an exising pre-commit script
+        WHEN install is called with -f
+        THEN it ignores the fact that the pre-commit script exists and succeeds
+        """
+        hook_path = get_default_global_hook_dir_path() / "pre-commit"
         hook_path.parent.mkdir(parents=True, exist_ok=True)
         hook_path.write_text("pre-commit file")
         assert hook_path.is_file()
 
-        result = cli_fs_runner.invoke(cli, ["install", "-m", "global", "-f"])
+        result = cli_runner.invoke(cli, ["install", "-m", "global", "-f"])
         assert_invoke_ok(result)
         assert f"pre-commit successfully added in {hook_path}" in result.output