Repository: gitguardian/ggshield
Issue: ggshield does not handle absence of git correctly

## GitGuardian Shield Version

- 1.13.0

## Command executed

Start an Ubuntu 22.04 docker container: `docker run -it --rm ubuntu:20.04`

Install ggshield using pip:

```
apt update
apt install pip
pip install ggshield
```

Try to run ggshield:

```
# ggshield --help
Traceback (most recent call last):
  File "/usr/local/bin/ggshield", line 5, in <module>
    from ggshield.cmd.main import main
  File "/usr/local/lib/python3.10/dist-packages/ggshield/cmd/main.py", line 10, in <module>
    from ggshield.cmd.auth import auth_group
  File "/usr/local/lib/python3.10/dist-packages/ggshield/cmd/auth/__init__.py", line 3, in <module>
    from .login import login_cmd
  File "/usr/local/lib/python3.10/dist-packages/ggshield/cmd/auth/login.py", line 6, in <module>
    from ggshield.cmd.auth.utils import check_instance_has_enabled_flow
  File "/usr/local/lib/python3.10/dist-packages/ggshield/cmd/auth/utils.py", line 4, in <module>
    from ggshield.core.client import create_session
  File "/usr/local/lib/python3.10/dist-packages/ggshield/core/client.py", line 13, in <module>
    from .config import Config
  File "/usr/local/lib/python3.10/dist-packages/ggshield/core/config/__init__.py", line 2, in <module>
    from .config import Config
  File "/usr/local/lib/python3.10/dist-packages/ggshield/core/config/config.py", line 7, in <module>
    from ggshield.core.config.user_config import UserConfig
  File "/usr/local/lib/python3.10/dist-packages/ggshield/core/config/user_config.py", line 24, in <module>
    from ggshield.core.utils import api_to_dashboard_url
  File "/usr/local/lib/python3.10/dist-packages/ggshield/core/utils.py", line 14, in <module>
    from .git_shell import get_git_root, is_git_dir
  File "/usr/local/lib/python3.10/dist-packages/ggshield/core/git_shell.py", line 38, in <module>
    GIT_PATH = get_git_path(os.getcwd())
  File "/usr/local/lib/python3.10/dist-packages/ggshield/core/git_shell.py", line 21, in get_git_path
    raise Exception("unable to find git executable in PATH/PATHEXT")
Exception: unable to find git executable in PATH/PATHEXT
```

## Describe the bug

ggshield assumes the `git` command is available at *import* time, and fails to start.

## Expected behavior

At a minimum, ggshield should print a proper error message when git is not available, not a stack trace.

Ideally, ggshield should not require the `git` command unless it's necessary. Commands like `secret scan docker` or `secret scan archiveè do not require git to be available.

## Tech details

Reworking `git_shell.py` to hide GIT_PATH and add a `git()` function should help.

Changed files:
ggshield/scan/scannable.py, ggshield/cmd/secret/scan/repo.py, ggshield/cmd/install.py, tests/unit/cmd/scan/test_ci.py, tests/unit/cmd/scan/test_prereceive.py, ggshield/core/git_shell.py, tests/unit/core/test_git_shell.py, tests/unit/scan/test_scan_repo.py

diff --git a/ggshield/cmd/install.py b/ggshield/cmd/install.py
index 47acb6a..9f60ba8 100644
--- a/ggshield/cmd/install.py
+++ b/ggshield/cmd/install.py
@@ -8,7 +8,7 @@ from click import UsageError
 
 from ggshield.cmd.common_options import add_common_options
 from ggshield.core.errors import UnexpectedError
-from ggshield.core.git_shell import GIT_PATH, check_git_dir, check_git_installed
+from ggshield.core.git_shell import check_git_dir, git
 
 
 @click.command(context_settings={"ignore_unknown_options": True})
@@ -43,14 +43,11 @@ def install_cmd(
 
 def install_global(hook_type: str, force: bool, append: bool) -> int:
     """Global pre-commit/pre-push hook installation."""
-    check_git_installed()
     hook_dir_path = get_global_hook_dir_path()
 
     if not hook_dir_path:
         hook_dir_path = os.path.expanduser("~/.git/hooks")
-        subprocess.run(
-            [GIT_PATH, "config", "--global", "core.hooksPath", hook_dir_path]
-        )
+        git(["config", "--global", "core.hooksPath", hook_dir_path])
 
     return create_hook(
         hook_dir_path=hook_dir_path,
@@ -63,16 +60,11 @@ def install_global(hook_type: str, force: bool, append: bool) -> int:
 
 def get_global_hook_dir_path() -> Optional[str]:
     """Return the default hooks path (if it exists)."""
-    with subprocess.Popen(
-        [GIT_PATH, "config", "--global", "--get", "core.hooksPath"],
-        stdout=subprocess.PIPE,
-    ) as process:
-        if process.returncode:
-            return None
-
-        return os.path.expanduser(
-            click.format_filename(process.communicate()[0].decode("utf-8")).strip()
-        )
+    try:
+        out = git(["config", "--global", "--get", "core.hooksPath"])
+    except subprocess.CalledProcessError:
+        return None
+    return os.path.expanduser(click.format_filename(out))
 
 
 def install_local(hook_type: str, force: bool, append: bool) -> int:
diff --git a/ggshield/cmd/secret/scan/repo.py b/ggshield/cmd/secret/scan/repo.py
index 82b0b1a..515a487 100644
--- a/ggshield/cmd/secret/scan/repo.py
+++ b/ggshield/cmd/secret/scan/repo.py
@@ -12,7 +12,7 @@ from ggshield.cmd.secret.scan.secret_scan_common_options import (
 )
 from ggshield.core.cache import Cache
 from ggshield.core.config import Config
-from ggshield.core.git_shell import GIT_PATH, shell
+from ggshield.core.git_shell import git
 from ggshield.core.utils import REGEX_GIT_URL
 from ggshield.scan import ScanContext, ScanMode
 from ggshield.scan.repo import scan_repo_path
@@ -56,7 +56,7 @@ def repo_cmd(
 
     if REGEX_GIT_URL.match(repository):
         with tempfile.TemporaryDirectory() as tmpdirname:
-            shell([GIT_PATH, "clone", repository, tmpdirname])
+            git(["clone", repository, tmpdirname])
             return scan_repo_path(
                 client=client,
                 cache=cache,
diff --git a/ggshield/core/git_shell.py b/ggshield/core/git_shell.py
index 6fed5b8..1f15572 100644
--- a/ggshield/core/git_shell.py
+++ b/ggshield/core/git_shell.py
@@ -3,7 +3,7 @@ import os
 import subprocess
 from functools import lru_cache
 from shutil import which
-from typing import Any, List, Optional
+from typing import List, Optional
 
 import click
 from click import UsageError
@@ -17,36 +17,35 @@ logger = logging.getLogger(__name__)
 
 
 @lru_cache(None)
-def get_git_path(cwd: str) -> str:
+def _get_git_path() -> str:
     git_path = which("git")
 
     if git_path is None:
-        raise Exception("unable to find git executable in PATH/PATHEXT")
+        raise UnexpectedError("unable to find git executable in PATH/PATHEXT")
 
     # lower()ing these would provide additional coverage on case-
     # insensitive filesystems but detection is problematic
     git_path = os.path.abspath(git_path)
-    cwd = os.path.abspath(cwd)
+    cwd = os.getcwd()
     path_env = [
         os.path.abspath(p) for p in os.environ.get("PATH", "").split(os.pathsep)
     ]
 
     # git was found - ignore git in cwd if cwd not in PATH
     if cwd == os.path.dirname(git_path) and cwd not in path_env:
-        raise Exception("rejecting git executable in CWD not in PATH")
+        raise UnexpectedError("rejecting git executable in CWD not in PATH")
 
+    logger.debug("Found git at %s", git_path)
     return git_path
 
 
-GIT_PATH = get_git_path(os.getcwd())
-
-
 @lru_cache(None)
 def is_git_dir(wd: str) -> bool:
-    check_git_installed()
-    cmd = [GIT_PATH, "-C", wd, "status"]
-    result = subprocess.run(cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
-    return result.returncode == 0
+    try:
+        git(["rev-parse", "--show-toplevel"], cwd=wd)
+    except subprocess.CalledProcessError:
+        return False
+    return True
 
 
 def check_git_dir(wd: Optional[str] = None) -> None:
@@ -58,65 +57,38 @@ def check_git_dir(wd: Optional[str] = None) -> None:
 
 
 def get_git_root(wd: Optional[str] = None) -> str:
-    cmd = [GIT_PATH]
-    if wd is not None:
-        cmd.extend(("-C", wd))
-
-    cmd.extend(("rev-parse", "--show-toplevel"))
-    return shell(cmd)
-
-
-@lru_cache(None)
-def check_git_installed() -> None:
-    """Check if git is installed."""
-    with subprocess.Popen(
-        [GIT_PATH, "--help"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL
-    ) as process:
-        if process.wait():
-            raise UnexpectedError("Git is not installed.")
+    return git(["rev-parse", "--show-toplevel"], cwd=wd)
 
 
-def shell(
+def git(
     command: List[str],
     timeout: int = COMMAND_TIMEOUT,
-    check: bool = False,
+    check: bool = True,
+    cwd: Optional[str] = None,
 ) -> str:
-    """Execute a command in a subprocess."""
+    """Calls git with the given arguments, returns stdout as a string"""
     env = os.environ.copy()
     env["LANG"] = "C"
 
     try:
         logger.debug("command=%s", command)
         result = subprocess.run(
-            command,
+            [_get_git_path()] + command,
             check=check,
             stdout=subprocess.PIPE,
             stderr=subprocess.PIPE,
             timeout=timeout,
             env=env,
+            cwd=cwd,
         )
         return result.stdout.decode("utf-8", errors="ignore").rstrip()
     except subprocess.TimeoutExpired:
         raise click.Abort('Command "{}" timed out'.format(" ".join(command)))
 
 
-def shell_split(command: List[str], **kwargs: Any) -> List[str]:
-    return shell(command, **kwargs).split("\\n")
-
-
-def git(command: List[str], timeout: int = COMMAND_TIMEOUT, check: bool = True) -> str:
-    """Calls git with the given arguments, returns stdout as a string"""
-    return shell([GIT_PATH] + command, timeout=timeout, check=check)
-
-
 def git_ls(wd: Optional[str] = None) -> List[str]:
-    cmd = [GIT_PATH]
-    if wd is not None:
-        cmd.extend(("-C", wd))
-
-    cmd.extend(("ls-files", "--recurse-submodules"))
-
-    return shell_split(cmd, timeout=600)
+    cmd = ["ls-files", "--recurse-submodules"]
+    return git(cmd, timeout=600, cwd=wd).split("\\n")
 
 
 def is_valid_git_commit_ref(ref: str) -> bool:
@@ -124,10 +96,10 @@ def is_valid_git_commit_ref(ref: str) -> bool:
     Check if a reference is valid and can be resolved to a commit
     """
     ref += "^{commit}"
-    cmd = [GIT_PATH, "cat-file", "-e", ref]
+    cmd = ["cat-file", "-e", ref]
 
     try:
-        shell(cmd, check=True)
+        git(cmd)
     except subprocess.CalledProcessError:
         return False
 
@@ -144,14 +116,14 @@ def get_list_commit_SHA(
     returns the *end* of the list, so max_count=3 returns [HEAD~2, HEAD~1, HEAD].
     """
 
-    cmd = [GIT_PATH, "rev-list", "--reverse", *commit_range.split()]
+    cmd = ["rev-list", "--reverse", *commit_range.split()]
     if max_count is not None:
         cmd.extend(["--max-count", str(max_count)])
     # Makes rev-list print "bad revision" instead of telling the range is ambiguous
     cmd.append("--")
 
     try:
-        commit_list = shell_split(cmd, check=True)
+        commit_list = git(cmd).split("\\n")
     except subprocess.CalledProcessError as e:
         if b"bad revision" in e.stderr and "~1.." in commit_range:
             # We got asked to list commits for A~1...B. If A~1 does not exist, but A
diff --git a/ggshield/scan/scannable.py b/ggshield/scan/scannable.py
index eea70ab..c5eab32 100644
--- a/ggshield/scan/scannable.py
+++ b/ggshield/scan/scannable.py
@@ -8,7 +8,7 @@ import charset_normalizer
 from pygitguardian.config import DOCUMENT_SIZE_THRESHOLD_BYTES
 
 from ggshield.core.filter import is_filepath_excluded
-from ggshield.core.git_shell import GIT_PATH, shell
+from ggshield.core.git_shell import git
 from ggshield.core.text_utils import STYLE, format_text
 from ggshield.core.utils import REGEX_HEADER_INFO, Filemode
 
@@ -286,9 +286,9 @@ class Commit(Files):
                 "-m",  # split multi-parent (aka merge) commits into several one-parent commits
             ]
             if self.sha:
-                self._patch = shell([GIT_PATH, "show", self.sha] + common_args)
+                self._patch = git(["show", self.sha] + common_args)
             else:
-                self._patch = shell([GIT_PATH, "diff", "--cached"] + common_args)
+                self._patch = git(["diff", "--cached"] + common_args)
 
         return self._patch
 
diff --git a/tests/unit/cmd/scan/test_ci.py b/tests/unit/cmd/scan/test_ci.py
index c6c790c..7239c30 100644
--- a/tests/unit/cmd/scan/test_ci.py
+++ b/tests/unit/cmd/scan/test_ci.py
@@ -1,4 +1,3 @@
-import json
 from typing import Dict
 from unittest.mock import Mock, patch
 
@@ -6,7 +5,7 @@ import click
 import pytest
 
 from ggshield.cmd.main import cli
-from ggshield.cmd.secret.scan.ci import EMPTY_SHA
+from ggshield.cmd.secret.scan.ci import EMPTY_SHA, gitlab_ci_range
 from ggshield.core.errors import ExitCode
 from tests.unit.conftest import assert_invoke_exited_with, assert_invoke_ok
 
@@ -58,7 +57,6 @@ def clear_current_ci_envs(monkeypatch):
         ({"HEAD": "head_sha"}, "HEAD~1..."),
     ],
 )
-@pytest.mark.parametrize("json_output", (False, True))
 def test_gitlab_ci_range(
     _: Mock,
     get_list_mock: Mock,
@@ -66,31 +64,29 @@ def test_gitlab_ci_range(
     monkeypatch,
     env: Dict[str, str],
     expected_parameter: str,
-    json_output: bool,
+    capsys,
 ):
+    """
+    GIVEN a GitLab CI environment
+    WHEN gitlab_ci_range(verbose=True) is called
+    THEN the correct commit range is requested
+    AND stdout is empty (to avoid polluting redirections)
+    AND stderr is not empty
+    """
     monkeypatch.setenv("CI", "1")
     monkeypatch.setenv("GITLAB_CI", "1")
     for k, v in env.items():
         monkeypatch.setenv(k, v)
 
     get_list_mock.return_value = ["a"] * 51
-    cli_fs_runner.mix_stderr = False
-    json_arg = ["--json"] if json_output else []
-    result = cli_fs_runner.invoke(
-        cli,
-        [
-            "-v",
-            "secret",
-            "scan",
-            "ci",
-            *json_arg,
-        ],
-    )
-    assert_invoke_ok(result)
-    if json_output:
-        json.loads(result.output)
+
+    gitlab_ci_range(verbose=True)
     get_list_mock.assert_called_once_with(expected_parameter)
 
+    captured = capsys.readouterr()
+    assert captured.out == ""
+    assert captured.err != ""
+
 
 @patch("ggshield.cmd.secret.scan.ci.scan_commit_range")
 @patch("ggshield.cmd.secret.scan.ci.get_list_commit_SHA")
diff --git a/tests/unit/cmd/scan/test_prereceive.py b/tests/unit/cmd/scan/test_prereceive.py
index 31f7712..5305338 100644
--- a/tests/unit/cmd/scan/test_prereceive.py
+++ b/tests/unit/cmd/scan/test_prereceive.py
@@ -7,7 +7,7 @@ from click.testing import CliRunner
 from ggshield.cmd.main import cli
 from ggshield.core.errors import ExitCode
 from ggshield.core.utils import EMPTY_SHA, Filemode
-from ggshield.scan import Result, Results, ScanCollection
+from ggshield.scan import Commit, Result, Results, ScanCollection
 from ggshield.scan.repo import cd
 from tests.repository import Repository
 from tests.unit.conftest import (
@@ -156,10 +156,12 @@ class TestPreReceive:
         )
 
     @patch("ggshield.cmd.secret.scan.prereceive.get_list_commit_SHA")
+    @patch("ggshield.scan.repo.get_commits_by_batch")
     @patch("ggshield.scan.repo.scan_commits_content")
     def test_stdin_supports_gitlab_web_ui(
         self,
         scan_commits_content_mock: Mock,
+        get_commits_by_batch_mock: Mock,
         get_list_mock: Mock,
         cli_fs_runner: CliRunner,
     ):
@@ -173,6 +175,7 @@ class TestPreReceive:
         old_sha = "56781234"
         new_sha = "1234abcd"
         get_list_mock.return_value = [new_sha]
+        get_commits_by_batch_mock.return_value = [[Commit(sha=new_sha)]]
         scan_commits_content_mock.return_value = ScanCollection(
             id="some_id",
             type="commit-ranges",
diff --git a/tests/unit/core/test_git_shell.py b/tests/unit/core/test_git_shell.py
index 475664e..ee0008b 100644
--- a/tests/unit/core/test_git_shell.py
+++ b/tests/unit/core/test_git_shell.py
@@ -4,17 +4,16 @@ import pytest
 from click import UsageError
 
 from ggshield.core.git_shell import (
-    GIT_PATH,
     check_git_dir,
+    git,
     is_git_dir,
     is_valid_git_commit_ref,
-    shell,
 )
 from ggshield.scan.repo import cd
 
 
 def test_git_shell():
-    assert "usage: git" in shell([GIT_PATH, "help"])
+    assert "usage: git" in git(["help"])
 
 
 def test_is_git_dir(tmp_path):
diff --git a/tests/unit/scan/test_scan_repo.py b/tests/unit/scan/test_scan_repo.py
index 6cee44e..d726b74 100644
--- a/tests/unit/scan/test_scan_repo.py
+++ b/tests/unit/scan/test_scan_repo.py
@@ -7,6 +7,7 @@ import pytest
 from ggshield.core.utils import Filemode
 from ggshield.scan import Commit, File, Result, Results
 from ggshield.scan.repo import get_commits_by_batch, scan_commits_content
+from ggshield.scan.scannable import CommitInformation
 from tests.unit.conftest import TWO_POLICY_BREAKS
 
 
@@ -77,11 +78,14 @@ def test_scan_2_commits_same_content(secret_scanner_mock):
     WHEN scan_commits_content returns 2 policy break for each commit
     THEN the total number of policy breaks is 4
     """
+    commit_info = CommitInformation("unknown", "", "")
     commit_1 = Commit(sha="some_sha_1")
     commit_1._files = [File(document="document", filename="filename")]
+    commit_1._info = commit_info
 
     commit_2 = Commit(sha="some_sha_2")
     commit_2._files = [File(document="document", filename="filename")]
+    commit_2._info = commit_info
 
     secret_scanner_mock.return_value.scan.return_value = Results(
         results=[